From 2f0bb896c6231f35c5e29088057e8375688c30f6 Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Sat, 5 Sep 2026 01:47:18 +0530 Subject: [PATCH 1/6] feat: issue a revocable jti on every JWT Session JWTs carried no unique identifier, so there was nothing to key a revocation list on: the only way to invalidate a single token was to rotate JWT_SECRET, which logs out every user at once. Add a jti claim to every issued token. generateJWT is the sole issuing path -- both the JSON API login and the admin UI's form login (#92) call it -- so no token can escape without one. The identifier is 128 bits from crypto/rand rather than a counter: a guessable jti would let an attacker pre-emptively revoke other users' tokens. parseSessionToken now also requires an exp claim. generateJWT always sets one, and the revocation list that follows records each token's expiry, so a token without exp is not one this server issued. No behavior change for clients yet -- this is the claim the blocklist needs. --- admin_session.go | 3 +- auth.go | 43 ++++++++++++++++++++++----- auth_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 9 deletions(-) diff --git a/admin_session.go b/admin_session.go index 2cc2b9a..102bfef 100644 --- a/admin_session.go +++ b/admin_session.go @@ -3,7 +3,6 @@ package main import ( "log/slog" "net/http" - "time" "github.com/golang-jwt/jwt/v5" ) @@ -31,7 +30,7 @@ func setSessionCookie(w http.ResponseWriter, r *http.Request, token string, trus Name: sessionCookieName, Value: token, Path: "/", - MaxAge: int((24 * time.Hour).Seconds()), + MaxAge: int(tokenLifetime.Seconds()), HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: requestIsSecure(r, trustProxy), diff --git a/auth.go b/auth.go index 5acca08..481a6f2 100644 --- a/auth.go +++ b/auth.go @@ -2,6 +2,8 @@ package main import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -129,15 +131,37 @@ func handleLogin(fetcher UserFetcher, secret []byte, limiter *LoginRateLimiter, } } -// generateJWT creates a signed JWT valid for 24 hours. +// tokenLifetime is how long an issued session JWT stays valid. +const tokenLifetime = 24 * time.Hour + +// newJTI returns a random 128-bit token identifier, hex-encoded. It must come +// from crypto/rand rather than math/rand or a counter: a guessable jti would +// let an attacker pre-emptively revoke other users' tokens. +func newJTI() (string, error) { + b := make([]byte, 16) + if _, err := rand.Read(b); err != nil { + return "", fmt.Errorf("generate jti: %w", err) + } + return hex.EncodeToString(b), nil +} + +// generateJWT creates a signed JWT valid for tokenLifetime. It is the only +// path that issues session tokens — both the JSON API login and the admin +// UI's form login call it — so every token carries a jti. func generateJWT(user *User, secret []byte) (string, error) { now := time.Now() + jti, err := newJTI() + if err != nil { + return "", err + } + claims := jwt.MapClaims{ "sub": fmt.Sprintf("%d", user.ID), "email": user.Email, "role": user.Role, - "exp": now.Add(24 * time.Hour).Unix(), + "jti": jti, + "exp": now.Add(tokenLifetime).Unix(), "iat": now.Unix(), "iss": "vehicle-positions-api", } @@ -173,17 +197,22 @@ func requireAdmin() func(http.Handler) http.Handler { } } -// parseSessionToken validates an HS256 session JWT (algorithm, issuer) and -// returns its claims. It is the single validation path shared by the API -// middleware and the admin UI's cookie session (adminClaimsFromCookie), so -// changes to token validation cannot silently diverge between the two. +// parseSessionToken validates an HS256 session JWT (algorithm, issuer, +// expiry) and returns its claims. It is the single validation path shared by +// the API middleware and the admin UI's cookie session (adminClaimsFromCookie), +// so changes to token validation cannot silently diverge between the two. +// +// WithExpirationRequired rejects a signed token that carries no exp claim. +// generateJWT always sets one, so a token without exp is not one this server +// issued. func parseSessionToken(tokenString string, secret []byte) (jwt.MapClaims, error) { token, err := jwt.Parse(tokenString, func(t *jwt.Token) (interface{}, error) { if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) } return secret, nil - }, jwt.WithValidMethods([]string{"HS256"}), jwt.WithIssuer("vehicle-positions-api")) + }, jwt.WithValidMethods([]string{"HS256"}), jwt.WithIssuer("vehicle-positions-api"), + jwt.WithExpirationRequired()) if err != nil { return nil, err } diff --git a/auth_test.go b/auth_test.go index 974a9a7..b70b16f 100644 --- a/auth_test.go +++ b/auth_test.go @@ -470,3 +470,78 @@ func TestRequireAdmin_NoAuthHeader(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, w.Code) } + +// jtiOf extracts the jti claim from a signed token. +func jtiOf(t *testing.T, tokenStr string) string { + t.Helper() + claims, err := parseSessionToken(tokenStr, testSecret) + require.NoError(t, err) + jti, ok := claims["jti"].(string) + require.True(t, ok, "token must carry a string jti") + require.NotEmpty(t, jti) + return jti +} + +func TestGenerateJWT_IncludesJti(t *testing.T) { + tokenStr, err := generateJWT(&User{ID: 7, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + claims, err := parseSessionToken(tokenStr, testSecret) + require.NoError(t, err) + + jti, ok := claims["jti"].(string) + require.True(t, ok, "jti must be present and a string") + assert.NotEmpty(t, jti) + assert.Len(t, jti, 32, "128 random bits, hex-encoded") +} + +func TestGenerateJWT_JtiIsUnique(t *testing.T) { + user := &User{ID: 7, Email: "driver@test.com", Role: "driver"} + + first, err := generateJWT(user, testSecret) + require.NoError(t, err) + second, err := generateJWT(user, testSecret) + require.NoError(t, err) + + assert.NotEqual(t, jtiOf(t, first), jtiOf(t, second), + "each token needs its own identifier, or revoking one revokes them all") +} + +func TestNewJTI_Unique(t *testing.T) { + seen := make(map[string]struct{}, 100) + for i := 0; i < 100; i++ { + jti, err := newJTI() + require.NoError(t, err) + require.NotEmpty(t, jti) + _, dup := seen[jti] + require.False(t, dup, "newJTI must not repeat") + seen[jti] = struct{}{} + } + assert.Len(t, seen, 100) +} + +// TestRequireAuth_RejectsTokenWithoutExp pins the tightened parseSessionToken: +// a signed token with no exp is not one generateJWT issued. +func TestRequireAuth_RejectsTokenWithoutExp(t *testing.T) { + claims := jwt.MapClaims{ + "sub": "1", + "email": "driver@test.com", + "role": "driver", + "jti": "abc123", + "iat": time.Now().Unix(), + "iss": "vehicle-positions-api", + } + tokenStr, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(testSecret) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+tokenStr) + w := httptest.NewRecorder() + requireAuth(testSecret)(dummyHandler()).ServeHTTP(w, req) + + assert.Equal(t, http.StatusUnauthorized, w.Code) + + var resp map[string]string + require.NoError(t, json.NewDecoder(w.Body).Decode(&resp)) + assert.Equal(t, "invalid token", resp["error"]) +} From cc098884ef91df5c7227f423749a651488c123ee Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Sat, 5 Sep 2026 01:47:19 +0530 Subject: [PATCH 2/6] feat: add revoked_tokens table, queries, and store The blocklist the jti claim is keyed on. jti is the primary key, so there is no separate index on it -- the PK already creates one. The user_id foreign key cascades on delete, matching user_vehicles (000008). Inserts are ON CONFLICT (jti) DO NOTHING so a repeat logout cannot error. TokenRevoker and TokenChecker are kept as two minimal interfaces so the logout handler depends only on the write and the auth middleware only on the read. expires_at is recorded for a periodic cleanup job that does not exist yet; the column is inert in this change and the comment on RevokeToken says so. The index on it exists for that job rather than for any query here. Migration number: main is at 000010, PRs #93 and #94 both claim 000011, and #97 claims 000012, so 000013 is the first free number. The gap is deliberate -- those versions are spoken for, and golang-migrate tolerates gaps (000007 is already missing). --- db/models.go | 7 + db/query.sql | 9 ++ db/query.sql.go | 29 ++++ migrations/000013_add_revoked_tokens.down.sql | 1 + migrations/000013_add_revoked_tokens.up.sql | 11 ++ store_revocation.go | 53 +++++++ store_revocation_test.go | 141 ++++++++++++++++++ 7 files changed, 251 insertions(+) create mode 100644 migrations/000013_add_revoked_tokens.down.sql create mode 100644 migrations/000013_add_revoked_tokens.up.sql create mode 100644 store_revocation.go create mode 100644 store_revocation_test.go diff --git a/db/models.go b/db/models.go index edcba7e..9426e6c 100644 --- a/db/models.go +++ b/db/models.go @@ -22,6 +22,13 @@ type LocationPoint struct { DriverID string } +type RevokedToken struct { + Jti string + UserID int64 + ExpiresAt pgtype.Timestamptz + RevokedAt pgtype.Timestamptz +} + type Trip struct { ID int64 UserID int64 diff --git a/db/query.sql b/db/query.sql index 2726c10..abbd765 100644 --- a/db/query.sql +++ b/db/query.sql @@ -181,3 +181,12 @@ FROM trips t JOIN users u ON u.id = t.user_id WHERE t.status = 'active' ORDER BY t.vehicle_id, t.start_time DESC; + +-- name: RevokeToken :exec +-- Idempotent: logging out twice must not error. +INSERT INTO revoked_tokens (jti, user_id, expires_at) +VALUES ($1, $2, $3) +ON CONFLICT (jti) DO NOTHING; + +-- name: IsTokenRevoked :one +SELECT EXISTS(SELECT 1 FROM revoked_tokens WHERE jti = $1); diff --git a/db/query.sql.go b/db/query.sql.go index 6d4c4cf..0ebb33a 100644 --- a/db/query.sql.go +++ b/db/query.sql.go @@ -458,6 +458,17 @@ func (q *Queries) InsertLocationPoint(ctx context.Context, arg InsertLocationPoi return err } +const isTokenRevoked = `-- name: IsTokenRevoked :one +SELECT EXISTS(SELECT 1 FROM revoked_tokens WHERE jti = $1) +` + +func (q *Queries) IsTokenRevoked(ctx context.Context, jti string) (bool, error) { + row := q.db.QueryRow(ctx, isTokenRevoked, jti) + var exists bool + err := row.Scan(&exists) + return exists, err +} + const listActiveTripsByVehicle = `-- name: ListActiveTripsByVehicle :many SELECT DISTINCT ON (t.vehicle_id) t.vehicle_id, t.id, t.route_id, t.gtfs_trip_id, t.user_id, u.name AS driver_name @@ -750,6 +761,24 @@ func (q *Queries) ListVehiclesByUser(ctx context.Context, userID int64) ([]UserV return items, nil } +const revokeToken = `-- name: RevokeToken :exec +INSERT INTO revoked_tokens (jti, user_id, expires_at) +VALUES ($1, $2, $3) +ON CONFLICT (jti) DO NOTHING +` + +type RevokeTokenParams struct { + Jti string + UserID int64 + ExpiresAt pgtype.Timestamptz +} + +// Idempotent: logging out twice must not error. +func (q *Queries) RevokeToken(ctx context.Context, arg RevokeTokenParams) error { + _, err := q.db.Exec(ctx, revokeToken, arg.Jti, arg.UserID, arg.ExpiresAt) + return err +} + const setUserActive = `-- name: SetUserActive :execrows UPDATE users SET active = $2 WHERE id = $1 ` diff --git a/migrations/000013_add_revoked_tokens.down.sql b/migrations/000013_add_revoked_tokens.down.sql new file mode 100644 index 0000000..ade7f96 --- /dev/null +++ b/migrations/000013_add_revoked_tokens.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS revoked_tokens; diff --git a/migrations/000013_add_revoked_tokens.up.sql b/migrations/000013_add_revoked_tokens.up.sql new file mode 100644 index 0000000..4472da2 --- /dev/null +++ b/migrations/000013_add_revoked_tokens.up.sql @@ -0,0 +1,11 @@ +CREATE TABLE IF NOT EXISTS revoked_tokens ( + jti TEXT PRIMARY KEY CHECK (jti != ''), + user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + expires_at TIMESTAMPTZ NOT NULL, + revoked_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +-- Supports the periodic cleanup of expired rows that is planned as a +-- follow-up (DELETE FROM revoked_tokens WHERE expires_at < NOW()). jti needs +-- no index of its own: the primary key already provides one. +CREATE INDEX IF NOT EXISTS idx_revoked_tokens_expires_at ON revoked_tokens (expires_at); diff --git a/store_revocation.go b/store_revocation.go new file mode 100644 index 0000000..732763e --- /dev/null +++ b/store_revocation.go @@ -0,0 +1,53 @@ +package main + +import ( + "context" + "fmt" + "time" + + "github.com/OneBusAway/vehicle-positions/db" + "github.com/jackc/pgx/v5/pgtype" +) + +// TokenRevoker records a token's jti so it is rejected for the rest of its +// lifetime. Kept separate from TokenChecker so the logout handler depends on +// the write and the auth middleware only on the read. +type TokenRevoker interface { + RevokeToken(ctx context.Context, jti string, userID int64, expiresAt time.Time) error +} + +// TokenChecker reports whether a token's jti has been revoked. +type TokenChecker interface { + IsTokenRevoked(ctx context.Context, jti string) (bool, error) +} + +// RevokeToken adds a jti to the revocation list. It is idempotent, so logging +// out twice with the same token succeeds both times. +// +// expires_at is recorded so revocation rows can be aged out, but nothing +// deletes them yet: this table grows one row per logout. A periodic cleanup +// job (DELETE FROM revoked_tokens WHERE expires_at < NOW()) is needed as a +// follow-up. +func (s *Store) RevokeToken(ctx context.Context, jti string, userID int64, expiresAt time.Time) error { + err := s.queries.RevokeToken(ctx, db.RevokeTokenParams{ + Jti: jti, + UserID: userID, + ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, + }) + if err != nil { + return fmt.Errorf("revoke token: %w", err) + } + return nil +} + +// IsTokenRevoked returns true if the jti has been revoked. +func (s *Store) IsTokenRevoked(ctx context.Context, jti string) (bool, error) { + revoked, err := s.queries.IsTokenRevoked(ctx, jti) + if err != nil { + return false, fmt.Errorf("check token revocation: %w", err) + } + return revoked, nil +} + +var _ TokenRevoker = (*Store)(nil) +var _ TokenChecker = (*Store)(nil) diff --git a/store_revocation_test.go b/store_revocation_test.go new file mode 100644 index 0000000..40b62e7 --- /dev/null +++ b/store_revocation_test.go @@ -0,0 +1,141 @@ +package main + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// insertRevocationTestUser creates a user the revocation rows can reference +// and returns its ID. revoked_tokens.user_id is a foreign key, so every test +// here needs a real user row. +func insertRevocationTestUser(t *testing.T, store *Store) int64 { + t.Helper() + ctx := context.Background() + email := uniqueEmail(t) + t.Cleanup(func() { cleanupTestUsers(t, store, email) }) + + var id int64 + err := store.pool.QueryRow(ctx, + `INSERT INTO users (name, email, password_hash, role) VALUES ($1, $2, $3, $4) RETURNING id`, + "Revocation Test User", + email, + "$2a$10$92IXUNpkjO0rOQ5byMi.Ye4oKoEa3Ro9llC/.og/at2.uheWG/igi", + "driver", + ).Scan(&id) + require.NoError(t, err) + require.NotZero(t, id) + return id +} + +// countRevocationRows returns how many revocation rows exist for a jti. +func countRevocationRows(t *testing.T, store *Store, jti string) int { + t.Helper() + var n int + err := store.pool.QueryRow(context.Background(), + "SELECT COUNT(*) FROM revoked_tokens WHERE jti = $1", jti).Scan(&n) + require.NoError(t, err) + return n +} + +func TestStore_RevokeAndCheckToken(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + userID := insertRevocationTestUser(t, store) + + jti, err := newJTI() + require.NoError(t, err) + + revoked, err := store.IsTokenRevoked(ctx, jti) + require.NoError(t, err) + require.False(t, revoked, "jti must not be revoked before RevokeToken runs") + + expiresAt := time.Now().Add(24 * time.Hour) + require.NoError(t, store.RevokeToken(ctx, jti, userID, expiresAt)) + + revoked, err = store.IsTokenRevoked(ctx, jti) + require.NoError(t, err) + assert.True(t, revoked) + + // Round-trip the stored row: expires_at is what a future cleanup job + // will filter on, so a wrong value would silently break it. + var gotUserID int64 + var gotExpiresAt, gotRevokedAt time.Time + err = store.pool.QueryRow(ctx, + "SELECT user_id, expires_at, revoked_at FROM revoked_tokens WHERE jti = $1", jti, + ).Scan(&gotUserID, &gotExpiresAt, &gotRevokedAt) + require.NoError(t, err) + assert.Equal(t, userID, gotUserID) + assert.WithinDuration(t, expiresAt, gotExpiresAt, time.Second) + assert.WithinDuration(t, time.Now(), gotRevokedAt, time.Minute, "revoked_at defaults to NOW()") +} + +func TestStore_IsTokenRevoked_Unknown(t *testing.T) { + store := newTestStore(t) + + jti, err := newJTI() + require.NoError(t, err) + + revoked, err := store.IsTokenRevoked(context.Background(), jti) + assert.NoError(t, err, "an unknown jti is not an error, just not revoked") + assert.False(t, revoked) +} + +func TestStore_RevokeToken_Idempotent(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + userID := insertRevocationTestUser(t, store) + + jti, err := newJTI() + require.NoError(t, err) + + expiresAt := time.Now().Add(24 * time.Hour) + require.NoError(t, store.RevokeToken(ctx, jti, userID, expiresAt)) + assert.NoError(t, store.RevokeToken(ctx, jti, userID, expiresAt), + "revoking the same jti twice must not error (ON CONFLICT DO NOTHING)") + + assert.Equal(t, 1, countRevocationRows(t, store, jti), "the second revoke must not add a row") +} + +func TestStore_RevokeToken_UnknownUserFK(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + jti, err := newJTI() + require.NoError(t, err) + + // -1 can never be a users.id (the column is a positive-only sequence). + err = store.RevokeToken(ctx, jti, -1, time.Now().Add(24*time.Hour)) + require.Error(t, err, "a revocation for a non-existent user must fail the foreign key") + + assert.Equal(t, 0, countRevocationRows(t, store, jti), "the failed insert must leave no row behind") +} + +func TestStore_RevokeToken_EmptyJtiRejected(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + userID := insertRevocationTestUser(t, store) + + err := store.RevokeToken(ctx, "", userID, time.Now().Add(24*time.Hour)) + assert.Error(t, err, "the CHECK (jti != '') constraint must reject an empty jti") +} + +func TestStore_RevokeToken_CascadesOnUserDelete(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + userID := insertRevocationTestUser(t, store) + + jti, err := newJTI() + require.NoError(t, err) + require.NoError(t, store.RevokeToken(ctx, jti, userID, time.Now().Add(24*time.Hour))) + require.Equal(t, 1, countRevocationRows(t, store, jti)) + + _, err = store.pool.Exec(ctx, "DELETE FROM users WHERE id = $1", userID) + require.NoError(t, err) + + assert.Equal(t, 0, countRevocationRows(t, store, jti), + "ON DELETE CASCADE must remove the deleted user's revocation rows") +} From 1f240d459741eb8daa12642321096b9dbe83e63d Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Sat, 5 Sep 2026 01:47:19 +0530 Subject: [PATCH 3/6] feat: reject revoked tokens on the header and cookie paths parseSessionToken stays pure -- it is signature and claims validation with no I/O, and threading a store through it would force a database into every caller including tests. Revocation is a separate checkRevoked step that both token paths invoke: requireAuth (the Authorization header and its vp_session cookie fallback) and adminClaimsFromCookie (the admin UI's pages). The comment on parseSessionToken warns that the two paths must not diverge, but a comment cannot fail CI, so TestAdminCookiePath_RejectsRevokedToken and TestRequireAuthCookiePath_RejectsRevokedToken pin it instead. The check fails closed. If the store cannot answer, the request is rejected rather than allowed through -- the opposite of the rate limiter's fail-open-at-capacity rule, and deliberately so: a limiter failing open costs some unthrottled requests, an auth check failing open costs an accepted logged-out token. A revoked token returns the existing 401 {"error": "invalid token"}, identical to a malformed one, so the client learns the token is unusable without learning it was specifically revoked. Every rejection is logged. Tokens issued before this change have no jti and cannot be revoked. They are still accepted, so deploying does not sign everyone out, and each acceptance logs a warning. Tokens live 24h and every new token carries a jti, so that warning should stop appearing within a day of deploy; a TODO marks where the shim gets removed. --- admin_page_handlers.go | 8 +- admin_session.go | 23 ++++- admin_session_test.go | 48 +++++++++- auth.go | 59 +++++++++++- auth_test.go | 208 +++++++++++++++++++++++++++++++++++++---- main.go | 4 +- route_wiring_test.go | 6 ++ 7 files changed, 324 insertions(+), 32 deletions(-) diff --git a/admin_page_handlers.go b/admin_page_handlers.go index 97f9a8c..36b9cc4 100644 --- a/admin_page_handlers.go +++ b/admin_page_handlers.go @@ -76,6 +76,7 @@ type adminUI struct { vehicleCreator VehicleCreator userManager userManager assignments assignmentManager + tokenChecker TokenChecker jwtSecret []byte loginLimiter *LoginRateLimiter cfg adminUIConfig @@ -103,6 +104,7 @@ func newAdminUI(store appStore, tracker *Tracker, jwtSecret []byte, limiter *Log vehicleCreator: store, userManager: store, assignments: store, + tokenChecker: store, jwtSecret: jwtSecret, loginLimiter: limiter, cfg: cfg, @@ -113,7 +115,7 @@ func newAdminUI(store appStore, tracker *Tracker, jwtSecret []byte, limiter *Log // static assets — that's the caller's responsibility (main's handler // construction), keeping this function focused on admin routes only. func registerAdminUI(mux *http.ServeMux, ui *adminUI) { - protect := requireAdminPage(ui.jwtSecret) + protect := requireAdminPage(ui.jwtSecret, ui.tokenChecker) mux.HandleFunc("GET /admin/login", ui.loginPage) mux.HandleFunc("POST /admin/login", ui.loginSubmit) @@ -142,7 +144,7 @@ func registerAdminUI(mux *http.ServeMux, ui *adminUI) { } func (ui *adminUI) rootRedirect(w http.ResponseWriter, r *http.Request) { - if _, ok := adminClaimsFromCookie(r, ui.jwtSecret); ok { + if _, ok := adminClaimsFromCookie(r, ui.jwtSecret, ui.tokenChecker); ok { http.Redirect(w, r, "/admin/dashboard", http.StatusSeeOther) return } @@ -150,7 +152,7 @@ func (ui *adminUI) rootRedirect(w http.ResponseWriter, r *http.Request) { } func (ui *adminUI) loginPage(w http.ResponseWriter, r *http.Request) { - if _, ok := adminClaimsFromCookie(r, ui.jwtSecret); ok { + if _, ok := adminClaimsFromCookie(r, ui.jwtSecret, ui.tokenChecker); ok { http.Redirect(w, r, "/admin/dashboard", http.StatusSeeOther) return } diff --git a/admin_session.go b/admin_session.go index 102bfef..a7b96f5 100644 --- a/admin_session.go +++ b/admin_session.go @@ -49,8 +49,14 @@ func clearSessionCookie(w http.ResponseWriter) { } // adminClaimsFromCookie validates the session cookie's JWT via the shared -// parseSessionToken path and additionally requires the admin role. -func adminClaimsFromCookie(r *http.Request, secret []byte) (jwt.MapClaims, bool) { +// parseSessionToken path, rejects a revoked token via the shared checkRevoked +// path, and additionally requires the admin role. +// +// This is the browser half of the revocation enforcement in requireAuth: both +// carry the same JWT, so a token revoked through POST /api/v1/auth/logout must +// end the admin session too. A checker error is treated as "no session" — +// fail closed, matching requireAuth. +func adminClaimsFromCookie(r *http.Request, secret []byte, checker TokenChecker) (jwt.MapClaims, bool) { c, err := r.Cookie(sessionCookieName) if err != nil || c.Value == "" { return nil, false @@ -59,6 +65,15 @@ func adminClaimsFromCookie(r *http.Request, secret []byte) (jwt.MapClaims, bool) if err != nil { return nil, false } + revoked, err := checkRevoked(r.Context(), claims, checker) + if err != nil { + slog.Error("admin session: revocation check failed", "error", err, "path", r.URL.Path) + return nil, false + } + if revoked { + slog.Warn("admin session: rejected revoked token", "sub", claims["sub"], "path", r.URL.Path) + return nil, false + } if role, _ := claims["role"].(string); role != "admin" { return nil, false } @@ -67,10 +82,10 @@ func adminClaimsFromCookie(r *http.Request, secret []byte) (jwt.MapClaims, bool) // requireAdminPage guards HTML admin pages: unauthenticated or non-admin // visitors are redirected to the login page (303) rather than given JSON. -func requireAdminPage(secret []byte) func(http.Handler) http.Handler { +func requireAdminPage(secret []byte, checker TokenChecker) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - claims, ok := adminClaimsFromCookie(r, secret) + claims, ok := adminClaimsFromCookie(r, secret, checker) if !ok { http.Redirect(w, r, "/admin/login", http.StatusSeeOther) return diff --git a/admin_session_test.go b/admin_session_test.go index dff825f..a8fb75b 100644 --- a/admin_session_test.go +++ b/admin_session_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "net/http" "net/http/httptest" "testing" @@ -33,7 +34,7 @@ func TestSetSessionCookieAttributes(t *testing.T) { func TestRequireAdminPage(t *testing.T) { next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }) - h := requireAdminPage(testSecret)(next) + h := requireAdminPage(testSecret, newFakeRevocations())(next) cases := []struct { name string @@ -89,3 +90,48 @@ func TestFlashRoundTrip(t *testing.T) { req3.AddCookie(&http.Cookie{Name: flashCookieName, Value: ""}) assert.Equal(t, "", takeFlash(httptest.NewRecorder(), req3)) } + +// TestAdminCookiePath_RejectsRevokedToken is the divergence guard from the +// plan's D2: requireAuth and the admin UI's cookie session validate the same +// JWT through parseSessionToken, so a token revoked through +// POST /api/v1/auth/logout must end the browser session too. Without this +// test the two paths could silently drift apart. +func TestAdminCookiePath_RejectsRevokedToken(t *testing.T) { + cookie := cookieFor(t, "admin") + revocations := newFakeRevocations() + revocations.revoked[jtiOf(t, cookie.Value)] = struct{}{} + + reached := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodGet, "/admin/dashboard", nil) + req.AddCookie(cookie) + w := httptest.NewRecorder() + requireAdminPage(testSecret, revocations)(next).ServeHTTP(w, req) + + assert.Equal(t, http.StatusSeeOther, w.Code, "a revoked session cookie must not reach an admin page") + assert.Equal(t, "/admin/login", w.Header().Get("Location")) + assert.False(t, reached, "the page handler must not run") +} + +// TestAdminCookiePath_CheckerErrorFailsClosed mirrors +// TestRequireAuth_CheckerErrorFailsClosed for the browser path: an +// undecidable revocation check sends the visitor to the login page rather +// than through to the page. +func TestAdminCookiePath_CheckerErrorFailsClosed(t *testing.T) { + revocations := newFakeRevocations() + revocations.err = errors.New("database unavailable") + + req := httptest.NewRequest(http.MethodGet, "/admin/dashboard", nil) + req.AddCookie(cookieFor(t, "admin")) + w := httptest.NewRecorder() + + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }) + requireAdminPage(testSecret, revocations)(next).ServeHTTP(w, req) + + assert.Equal(t, http.StatusSeeOther, w.Code) + assert.Equal(t, "/admin/login", w.Header().Get("Location")) +} diff --git a/auth.go b/auth.go index 481a6f2..4d6769e 100644 --- a/auth.go +++ b/auth.go @@ -131,7 +131,8 @@ func handleLogin(fetcher UserFetcher, secret []byte, limiter *LoginRateLimiter, } } -// tokenLifetime is how long an issued session JWT stays valid. +// tokenLifetime is how long an issued session JWT stays valid. It also bounds +// how long a revocation row has to be honoured (see revoked_tokens.expires_at). const tokenLifetime = 24 * time.Hour // newJTI returns a random 128-bit token identifier, hex-encoded. It must come @@ -147,7 +148,7 @@ func newJTI() (string, error) { // generateJWT creates a signed JWT valid for tokenLifetime. It is the only // path that issues session tokens — both the JSON API login and the admin -// UI's form login call it — so every token carries a jti. +// UI's form login call it — so every token carries a jti and can be revoked. func generateJWT(user *User, secret []byte) (string, error) { now := time.Now() @@ -202,9 +203,13 @@ func requireAdmin() func(http.Handler) http.Handler { // the API middleware and the admin UI's cookie session (adminClaimsFromCookie), // so changes to token validation cannot silently diverge between the two. // +// It deliberately performs no I/O: revocation is a separate step (checkRevoked) +// that both callers invoke, so a database dependency never has to be threaded +// through JWT parsing. +// // WithExpirationRequired rejects a signed token that carries no exp claim. -// generateJWT always sets one, so a token without exp is not one this server -// issued. +// generateJWT always sets one, and a revocation row needs the expiry to record +// expires_at, so a token without exp is not one this server issued. func parseSessionToken(tokenString string, secret []byte) (jwt.MapClaims, error) { token, err := jwt.Parse(tokenString, func(t *jwt.Token) (interface{}, error) { if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { @@ -226,8 +231,32 @@ func parseSessionToken(tokenString string, secret []byte) (jwt.MapClaims, error) return claims, nil } +// checkRevoked reports whether the token behind claims has been logged out. +// It is the second half of validation, kept out of parseSessionToken so that +// function stays pure; both token paths (the API's Authorization header and +// the admin UI's vp_session cookie) must call it, and +// TestAdminCookiePath_RejectsRevokedToken pins that they do. +func checkRevoked(ctx context.Context, claims jwt.MapClaims, checker TokenChecker) (bool, error) { + jti, _ := claims["jti"].(string) + if jti == "" { + // Intentional backwards compatibility: tokens issued before jti + // existed carry no identifier to revoke, so they are accepted rather + // than logging every existing session out on deploy. They are also + // permanently unrevokable, which is why this is a warning — every + // token issued from here on has a jti and tokens live tokenLifetime, + // so this should stop appearing within a day of deploying. + // TODO: drop this shim and reject tokens without a jti once all + // pre-revocation tokens have expired (tokenLifetime after deploy). + slog.Warn("accepted token without jti; it cannot be revoked", "sub", claims["sub"]) + return false, nil + } + return checker.IsTokenRevoked(ctx, jti) +} + // requireAuth is middleware that validates the Bearer JWT on protected routes. -func requireAuth(secret []byte) func(http.Handler) http.Handler { +// checker is consulted for every validated token so a logged-out one is +// rejected for the rest of its lifetime. +func requireAuth(secret []byte, checker TokenChecker) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { authHeader := r.Header.Get("Authorization") @@ -256,6 +285,26 @@ func requireAuth(secret []byte) func(http.Handler) http.Handler { writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) return } + + revoked, err := checkRevoked(r.Context(), claims, checker) + if err != nil { + // Fail closed. A rate limiter that can't decide should let + // the request through — the cost of being wrong is a few + // unthrottled requests. An auth check that can't decide must + // not, because the cost of being wrong is an accepted + // logged-out token. + slog.Error("revocation check failed", "error", err, "path", r.URL.Path) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "internal server error"}) + return + } + if revoked { + // Same 401 body as a malformed token: the client learns the + // token is unusable, not that it was specifically revoked. + slog.Warn("rejected revoked token", "sub", claims["sub"], "path", r.URL.Path) + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + } + ctx := contextWithClaims(r.Context(), claims) next.ServeHTTP(w, r.WithContext(ctx)) }) diff --git a/auth_test.go b/auth_test.go index b70b16f..b38f06c 100644 --- a/auth_test.go +++ b/auth_test.go @@ -5,6 +5,8 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" + "log/slog" "net/http" "net/http/httptest" "testing" @@ -185,7 +187,7 @@ func TestRequireAuth_MissingHeader(t *testing.T) { req := httptest.NewRequest("POST", "/api/v1/locations", nil) w := httptest.NewRecorder() - requireAuth(testSecret)(dummyHandler()).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(dummyHandler()).ServeHTTP(w, req) assert.Equal(t, http.StatusUnauthorized, w.Code) } @@ -205,7 +207,7 @@ func TestRequireAuth_MalformedHeader(t *testing.T) { req.Header.Set("Authorization", tc.header) w := httptest.NewRecorder() - requireAuth(testSecret)(dummyHandler()).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(dummyHandler()).ServeHTTP(w, req) assert.Equal(t, http.StatusUnauthorized, w.Code) }) @@ -217,7 +219,7 @@ func TestRequireAuth_InvalidToken(t *testing.T) { req.Header.Set("Authorization", "Bearer notavalidtoken") w := httptest.NewRecorder() - requireAuth(testSecret)(dummyHandler()).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(dummyHandler()).ServeHTTP(w, req) assert.Equal(t, http.StatusUnauthorized, w.Code) } @@ -234,7 +236,7 @@ func TestRequireAuth_ExpiredToken(t *testing.T) { req.Header.Set("Authorization", "Bearer "+tokenStr) rr := httptest.NewRecorder() - middleware := requireAuth(testSecret) + middleware := requireAuth(testSecret, newFakeRevocations()) handler := middleware(dummyHandler()) handler.ServeHTTP(rr, req) @@ -260,7 +262,7 @@ func TestRequireAuth_ValidToken(t *testing.T) { w.WriteHeader(http.StatusOK) }) - requireAuth(testSecret)(handler).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(handler).ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) } @@ -269,7 +271,7 @@ func TestRequireAuthCookieFallback(t *testing.T) { token, err := generateJWT(&User{ID: 3, Email: "admin@test.com", Role: "admin", Active: true}, testSecret) require.NoError(t, err) next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }) - h := requireAuth(testSecret)(next) + h := requireAuth(testSecret, newFakeRevocations())(next) t.Run("cookie only → 200", func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/", nil) @@ -337,7 +339,7 @@ func TestRequireAuth_WrongSecret(t *testing.T) { req.Header.Set("Authorization", "Bearer "+tokenStr) rr := httptest.NewRecorder() - middleware := requireAuth(testSecret) + middleware := requireAuth(testSecret, newFakeRevocations()) handler := middleware(dummyHandler()) handler.ServeHTTP(rr, req) @@ -354,7 +356,7 @@ func TestRequireAuth_AlgorithmConfusion(t *testing.T) { req.Header.Set("Authorization", "Bearer "+tokenStr) rr := httptest.NewRecorder() - middleware := requireAuth(testSecret) + middleware := requireAuth(testSecret, newFakeRevocations()) handler := middleware(dummyHandler()) handler.ServeHTTP(rr, req) @@ -377,7 +379,7 @@ func TestRequireAdmin_AdminAllowed(t *testing.T) { w.WriteHeader(http.StatusOK) }) - requireAuth(testSecret)(requireAdmin()(handler)).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(requireAdmin()(handler)).ServeHTTP(w, req) assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, "admin", receivedRole) @@ -391,7 +393,7 @@ func TestRequireAdmin_DriverDenied(t *testing.T) { req.Header.Set("Authorization", "Bearer "+token) w := httptest.NewRecorder() - requireAuth(testSecret)(requireAdmin()(dummyHandler())).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(requireAdmin()(dummyHandler())).ServeHTTP(w, req) assert.Equal(t, http.StatusForbidden, w.Code) @@ -423,7 +425,7 @@ func TestRequireAdmin_EmptyRole(t *testing.T) { req.Header.Set("Authorization", "Bearer "+token) w := httptest.NewRecorder() - requireAuth(testSecret)(requireAdmin()(dummyHandler())).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(requireAdmin()(dummyHandler())).ServeHTTP(w, req) assert.Equal(t, http.StatusForbidden, w.Code) @@ -452,7 +454,7 @@ func TestRequireAdmin_InvalidRoleType(t *testing.T) { req.Header.Set("Authorization", "Bearer "+tokenStr) w := httptest.NewRecorder() - requireAuth(testSecret)(requireAdmin()(dummyHandler())).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(requireAdmin()(dummyHandler())).ServeHTTP(w, req) assert.Equal(t, http.StatusForbidden, w.Code) @@ -466,11 +468,48 @@ func TestRequireAdmin_NoAuthHeader(t *testing.T) { req := httptest.NewRequest("GET", "/api/v1/admin/status", nil) w := httptest.NewRecorder() - requireAuth(testSecret)(requireAdmin()(dummyHandler())).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(requireAdmin()(dummyHandler())).ServeHTTP(w, req) assert.Equal(t, http.StatusUnauthorized, w.Code) } +// fakeRevocations is an in-memory TokenChecker/TokenRevoker for middleware and +// handler tests. Setting err makes both methods fail, which exercises the +// fail-closed paths. +type fakeRevocations struct { + revoked map[string]struct{} + err error + lastUserID int64 + lastExpiresAt time.Time + revokeCalls int +} + +func newFakeRevocations() *fakeRevocations { + return &fakeRevocations{revoked: make(map[string]struct{})} +} + +func (f *fakeRevocations) IsTokenRevoked(_ context.Context, jti string) (bool, error) { + if f.err != nil { + return false, f.err + } + _, ok := f.revoked[jti] + return ok, nil +} + +func (f *fakeRevocations) RevokeToken(_ context.Context, jti string, userID int64, expiresAt time.Time) error { + if f.err != nil { + return f.err + } + f.revokeCalls++ + f.lastUserID = userID + f.lastExpiresAt = expiresAt + f.revoked[jti] = struct{}{} + return nil +} + +var _ TokenChecker = (*fakeRevocations)(nil) +var _ TokenRevoker = (*fakeRevocations)(nil) + // jtiOf extracts the jti claim from a signed token. func jtiOf(t *testing.T, tokenStr string) string { t.Helper() @@ -482,6 +521,14 @@ func jtiOf(t *testing.T, tokenStr string) string { return jti } +// errorBody decodes a JSON error response and returns its "error" field. +func errorBody(t *testing.T, w *httptest.ResponseRecorder) string { + t.Helper() + var resp map[string]string + require.NoError(t, json.NewDecoder(w.Body).Decode(&resp)) + return resp["error"] +} + func TestGenerateJWT_IncludesJti(t *testing.T) { tokenStr, err := generateJWT(&User{ID: 7, Email: "driver@test.com", Role: "driver"}, testSecret) require.NoError(t, err) @@ -520,8 +567,116 @@ func TestNewJTI_Unique(t *testing.T) { assert.Len(t, seen, 100) } +func TestRequireAuth_RejectsRevokedToken(t *testing.T) { + token, err := generateJWT(&User{ID: 1, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + revocations.revoked[jtiOf(t, token)] = struct{}{} + + reached := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + requireAuth(testSecret, revocations)(next).ServeHTTP(w, req) + + assert.Equal(t, http.StatusUnauthorized, w.Code) + assert.Equal(t, "invalid token", errorBody(t, w), + "a revoked token must be indistinguishable from a malformed one") + assert.False(t, reached, "the downstream handler must not run") +} + +func TestRequireAuth_AllowsUnrevokedToken(t *testing.T) { + token, err := generateJWT(&User{ID: 1, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + // A different token is revoked: the check must be per-jti, not per-user. + other, err := generateJWT(&User{ID: 1, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + revocations := newFakeRevocations() + revocations.revoked[jtiOf(t, other)] = struct{}{} + + reached := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + claims, ok := r.Context().Value(claimsKey).(jwt.MapClaims) + require.True(t, ok, "claims must reach the downstream handler") + assert.Equal(t, jtiOf(t, token), claims["jti"]) + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + requireAuth(testSecret, revocations)(next).ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code) + assert.True(t, reached, "the downstream handler must run") +} + +// TestRequireAuth_AllowsTokenWithoutJti covers the backwards-compatibility +// shim in checkRevoked: tokens issued before jti existed are still accepted +// (they just can't be revoked), and doing so is logged. +// Not safe for t.Parallel(); uses global logger. +func TestRequireAuth_AllowsTokenWithoutJti(t *testing.T) { + claims := jwt.MapClaims{ + "sub": "1", + "email": "driver@test.com", + "role": "driver", + "exp": time.Now().Add(time.Hour).Unix(), + "iat": time.Now().Unix(), + "iss": "vehicle-positions-api", + } + tokenStr, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(testSecret) + require.NoError(t, err) + + var logs bytes.Buffer + original := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(original) }) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+tokenStr) + w := httptest.NewRecorder() + requireAuth(testSecret, newFakeRevocations())(dummyHandler()).ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code, "a pre-jti token must not be locked out") + assert.Contains(t, logs.String(), "accepted token without jti", + "accepting an unrevokable token must be logged") +} + +func TestRequireAuth_CheckerErrorFailsClosed(t *testing.T) { + token, err := generateJWT(&User{ID: 1, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + revocations.err = errors.New("database unavailable") + + reached := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + requireAuth(testSecret, revocations)(next).ServeHTTP(w, req) + + assert.NotEqual(t, http.StatusOK, w.Code, "an undecidable revocation check must not allow the request") + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Equal(t, "internal server error", errorBody(t, w)) + assert.False(t, reached, "the downstream handler must not run") +} + // TestRequireAuth_RejectsTokenWithoutExp pins the tightened parseSessionToken: -// a signed token with no exp is not one generateJWT issued. +// a signed token with no exp is not one generateJWT issued, and a revocation +// row could not record an expiry for it. func TestRequireAuth_RejectsTokenWithoutExp(t *testing.T) { claims := jwt.MapClaims{ "sub": "1", @@ -537,11 +692,28 @@ func TestRequireAuth_RejectsTokenWithoutExp(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) req.Header.Set("Authorization", "Bearer "+tokenStr) w := httptest.NewRecorder() - requireAuth(testSecret)(dummyHandler()).ServeHTTP(w, req) + requireAuth(testSecret, newFakeRevocations())(dummyHandler()).ServeHTTP(w, req) assert.Equal(t, http.StatusUnauthorized, w.Code) + assert.Equal(t, "invalid token", errorBody(t, w)) +} - var resp map[string]string - require.NoError(t, json.NewDecoder(w.Body).Decode(&resp)) - assert.Equal(t, "invalid token", resp["error"]) +// TestRequireAuthCookiePath_RejectsRevokedToken is the divergence guard for +// requireAuth's cookie fallback: the admin UI's vp_session cookie carries the +// same JWT as the Authorization header, so revocation must be enforced no +// matter which one delivers it. +func TestRequireAuthCookiePath_RejectsRevokedToken(t *testing.T) { + token, err := generateJWT(&User{ID: 3, Email: "admin@test.com", Role: "admin", Active: true}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + revocations.revoked[jtiOf(t, token)] = struct{}{} + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: token}) + w := httptest.NewRecorder() + requireAuth(testSecret, revocations)(dummyHandler()).ServeHTTP(w, req) + + assert.Equal(t, http.StatusUnauthorized, w.Code) + assert.Equal(t, "invalid token", errorBody(t, w)) } diff --git a/main.go b/main.go index 1146909..281d94c 100644 --- a/main.go +++ b/main.go @@ -49,6 +49,8 @@ type appStore interface { VehicleChecker DriverVehicleLister AdminStatsCounter + TokenRevoker + TokenChecker } // newMux wires all application routes and returns the configured ServeMux. @@ -58,7 +60,7 @@ type appStore interface { func newMux(store appStore, tracker *Tracker, rateLimiter *VehicleRateLimiter, jwtSecret []byte, startTime time.Time, loginLimiter *LoginRateLimiter, trustProxy bool) *http.ServeMux { mux := http.NewServeMux() - authMiddleware := requireAuth(jwtSecret) + authMiddleware := requireAuth(jwtSecret, store) adminMiddleware := requireAdmin() mux.Handle("POST /api/v1/auth/login", handleLogin(store, jwtSecret, loginLimiter, trustProxy)) diff --git a/route_wiring_test.go b/route_wiring_test.go index 9ab48ae..737ebda 100644 --- a/route_wiring_test.go +++ b/route_wiring_test.go @@ -120,6 +120,12 @@ func (n *noopStore) ListTripLocations(_ context.Context, _ int64) ([]LocationPoi func (n *noopStore) ListActiveTripsByVehicle(_ context.Context) (map[string]ActiveTripInfo, error) { return nil, nil } +func (n *noopStore) RevokeToken(_ context.Context, _ string, _ int64, _ time.Time) error { + return nil +} +func (n *noopStore) IsTokenRevoked(_ context.Context, _ string) (bool, error) { + return false, nil +} // TestAdminRoutes_DriverTokenRejected verifies that every /api/v1/admin/* route // is wrapped with adminMiddleware. A valid driver-role JWT must receive 403 on From a92adafe13a4966dc59bc13bac8dff23699155d7 Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Sat, 5 Sep 2026 01:47:19 +0530 Subject: [PATCH 4/6] feat: add POST /api/v1/auth/logout and revoke on admin sign-out Authenticated but not admin-gated: every user logs themselves out, and can only revoke their own token, read from the context claims requireAuth already set. The sub claim is a string rather than a number (JSON number precision), so it is parsed back with strconv rather than asserted as a float. Returns 204 No Content. PR #58 returned 200 with a message body; logout is a void operation with nothing useful to say, per the review suggestion there. POST /admin/logout previously only cleared the cookie, leaving the JWT live for anyone who had copied its value -- the same gap this change exists to close, on the admin surface. It now revokes the token first. That is best-effort by design: the route is deliberately unauthenticated so an expired session can still sign out, and a failed revocation still clears the cookie and redirects rather than stranding the user on an error page. Because both surfaces carry the same JWT, logging out through the API also ends an admin browser session, and vice versa. --- admin_page_handlers.go | 9 +++ admin_page_handlers_test.go | 34 ++++++++ admin_session.go | 44 ++++++++++ admin_session_test.go | 45 +++++++++++ auth.go | 61 ++++++++++++++ auth_test.go | 156 ++++++++++++++++++++++++++++++++++++ main.go | 1 + route_wiring_test.go | 32 ++++++++ 8 files changed, 382 insertions(+) diff --git a/admin_page_handlers.go b/admin_page_handlers.go index 36b9cc4..32bd465 100644 --- a/admin_page_handlers.go +++ b/admin_page_handlers.go @@ -77,6 +77,7 @@ type adminUI struct { userManager userManager assignments assignmentManager tokenChecker TokenChecker + tokenRevoker TokenRevoker jwtSecret []byte loginLimiter *LoginRateLimiter cfg adminUIConfig @@ -105,6 +106,7 @@ func newAdminUI(store appStore, tracker *Tracker, jwtSecret []byte, limiter *Log userManager: store, assignments: store, tokenChecker: store, + tokenRevoker: store, jwtSecret: jwtSecret, loginLimiter: limiter, cfg: cfg, @@ -225,7 +227,14 @@ func (ui *adminUI) renderLogin(w http.ResponseWriter, status int, errMsg, email }) } +// logout revokes the session's JWT server-side and clears the cookie. The +// route is deliberately unauthenticated (an expired session must still be +// able to log out), so a cookie that no longer validates is simply cleared. +// A revocation failure is logged but still clears the cookie and redirects: +// leaving the user stuck on an error page would not make the token any less +// valid. func (ui *adminUI) logout(w http.ResponseWriter, r *http.Request) { + revokeSessionCookie(r, ui.jwtSecret, ui.tokenRevoker) clearSessionCookie(w) http.Redirect(w, r, "/admin/login", http.StatusSeeOther) } diff --git a/admin_page_handlers_test.go b/admin_page_handlers_test.go index 994b658..09b1506 100644 --- a/admin_page_handlers_test.go +++ b/admin_page_handlers_test.go @@ -149,6 +149,40 @@ func TestAdminLogout(t *testing.T) { assert.Equal(t, -1, w.Result().Cookies()[0].MaxAge) } +// TestAdminLogoutRevokesSession verifies the admin UI's sign-out ends the +// token server-side, not just the browser's copy of it: after logging out, +// the same cookie value must no longer open an admin page. +func TestAdminLogoutRevokesSession(t *testing.T) { + ui := newTestAdminUI(t) + revocations := newFakeRevocations() + ui.tokenChecker = revocations + ui.tokenRevoker = revocations + mux := http.NewServeMux() + registerAdminUI(mux, ui) + + cookie := cookieFor(t, "admin") + + before := httptest.NewRequest(http.MethodGet, "/admin/dashboard", nil) + before.AddCookie(cookie) + beforeRec := httptest.NewRecorder() + mux.ServeHTTP(beforeRec, before) + require.Equal(t, http.StatusOK, beforeRec.Code, "the session must work before logout") + + logout := httptest.NewRequest(http.MethodPost, "/admin/logout", nil) + logout.AddCookie(cookie) + logoutRec := httptest.NewRecorder() + mux.ServeHTTP(logoutRec, logout) + require.Equal(t, http.StatusSeeOther, logoutRec.Code) + assert.Contains(t, revocations.revoked, jtiOf(t, cookie.Value), "sign-out must revoke the session token") + + after := httptest.NewRequest(http.MethodGet, "/admin/dashboard", nil) + after.AddCookie(cookie) + afterRec := httptest.NewRecorder() + mux.ServeHTTP(afterRec, after) + assert.Equal(t, http.StatusSeeOther, afterRec.Code, "a replayed cookie must no longer work") + assert.Equal(t, "/admin/login", afterRec.Header().Get("Location")) +} + func TestAdminPagesRedirectWithoutSession(t *testing.T) { ui := newTestAdminUI(t) mux := http.NewServeMux() diff --git a/admin_session.go b/admin_session.go index a7b96f5..d1500b5 100644 --- a/admin_session.go +++ b/admin_session.go @@ -3,6 +3,7 @@ package main import ( "log/slog" "net/http" + "strconv" "github.com/golang-jwt/jwt/v5" ) @@ -37,6 +38,49 @@ func setSessionCookie(w http.ResponseWriter, r *http.Request, token string, trus }) } +// revokeSessionCookie revokes the JWT carried by the session cookie, so +// signing out of the admin UI ends the token itself and not just the browser's +// copy of it. It is best-effort by design: the caller clears the cookie and +// redirects regardless, and a cookie that no longer parses has nothing to +// revoke. Errors are logged rather than returned for the same reason. +func revokeSessionCookie(r *http.Request, secret []byte, revoker TokenRevoker) { + c, err := r.Cookie(sessionCookieName) + if err != nil || c.Value == "" { + return + } + claims, err := parseSessionToken(c.Value, secret) + if err != nil { + slog.Debug("admin logout: session cookie no longer valid, nothing to revoke", "error", err) + return + } + jti, _ := claims["jti"].(string) + if jti == "" { + // Pre-revocation token; see checkRevoked. + slog.Warn("admin logout: session token has no jti, nothing to revoke", "sub", claims["sub"]) + return + } + sub, err := claims.GetSubject() + if err != nil { + slog.Warn("admin logout: unreadable sub claim", "error", err) + return + } + userID, err := strconv.ParseInt(sub, 10, 64) + if err != nil { + slog.Warn("admin logout: sub claim is not a user ID", "sub", sub, "error", err) + return + } + expiresAt, err := claims.GetExpirationTime() + if err != nil || expiresAt == nil { + slog.Warn("admin logout: unreadable exp claim", "sub", sub, "error", err) + return + } + if err := revoker.RevokeToken(r.Context(), jti, userID, expiresAt.Time); err != nil { + slog.Error("admin logout: failed to revoke session token", "sub", sub, "error", err) + return + } + slog.Info("admin session token revoked", "sub", sub) +} + func clearSessionCookie(w http.ResponseWriter) { http.SetCookie(w, &http.Cookie{ Name: sessionCookieName, diff --git a/admin_session_test.go b/admin_session_test.go index a8fb75b..87a6f77 100644 --- a/admin_session_test.go +++ b/admin_session_test.go @@ -135,3 +135,48 @@ func TestAdminCookiePath_CheckerErrorFailsClosed(t *testing.T) { assert.Equal(t, http.StatusSeeOther, w.Code) assert.Equal(t, "/admin/login", w.Header().Get("Location")) } + +func TestRevokeSessionCookie(t *testing.T) { + cookie := cookieFor(t, "admin") + + t.Run("valid cookie is revoked", func(t *testing.T) { + revocations := newFakeRevocations() + req := httptest.NewRequest(http.MethodPost, "/admin/logout", nil) + req.AddCookie(cookie) + + revokeSessionCookie(req, testSecret, revocations) + + assert.Contains(t, revocations.revoked, jtiOf(t, cookie.Value)) + assert.Equal(t, int64(9), revocations.lastUserID, "cookieFor issues tokens for user 9") + }) + + t.Run("no cookie revokes nothing", func(t *testing.T) { + revocations := newFakeRevocations() + req := httptest.NewRequest(http.MethodPost, "/admin/logout", nil) + + revokeSessionCookie(req, testSecret, revocations) + + assert.Zero(t, revocations.revokeCalls) + }) + + t.Run("unparseable cookie revokes nothing", func(t *testing.T) { + revocations := newFakeRevocations() + req := httptest.NewRequest(http.MethodPost, "/admin/logout", nil) + req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: "garbage"}) + + revokeSessionCookie(req, testSecret, revocations) + + assert.Zero(t, revocations.revokeCalls, "a cookie that no longer validates has no jti to revoke") + }) + + t.Run("store error still returns", func(t *testing.T) { + revocations := newFakeRevocations() + revocations.err = errors.New("database unavailable") + req := httptest.NewRequest(http.MethodPost, "/admin/logout", nil) + req.AddCookie(cookie) + + // Best-effort by design: the caller clears the cookie and redirects + // regardless, so this must not panic or block. + assert.NotPanics(t, func() { revokeSessionCookie(req, testSecret, revocations) }) + }) +} diff --git a/auth.go b/auth.go index 4d6769e..99ed072 100644 --- a/auth.go +++ b/auth.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "net/http" + "strconv" "strings" "time" @@ -310,3 +311,63 @@ func requireAuth(secret []byte, checker TokenChecker) func(http.Handler) http.Ha }) } } + +// handleLogout revokes the caller's own token, ending the session server-side +// rather than relying on the client to discard it. It must be wrapped in +// requireAuth, which puts the validated claims on the context. +// +// Every user may log themselves out, so this is authenticated but not +// admin-gated. Because the admin UI's vp_session cookie carries the same JWT, +// logging out through the API also ends that browser session. +func handleLogout(revoker TokenRevoker) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + claims, ok := r.Context().Value(claimsKey).(jwt.MapClaims) + if !ok { + slog.Warn("logout: claims missing from context") + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "unauthorized"}) + return + } + + jti, _ := claims["jti"].(string) + if jti == "" { + // A pre-revocation token (see checkRevoked) has nothing to record. + // Report the same 204 so old and new clients see one contract; the + // warning marks a session that outlives its logout. + slog.Warn("logout: token has no jti, nothing to revoke", "sub", claims["sub"]) + w.WriteHeader(http.StatusNoContent) + return + } + + // sub is a string, not a number (JSON number precision, see + // generateJWT), so parse it rather than asserting a float64. + sub, err := claims.GetSubject() + if err != nil { + slog.Warn("logout: unreadable sub claim", "error", err) + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + } + userID, err := strconv.ParseInt(sub, 10, 64) + if err != nil { + slog.Warn("logout: sub claim is not a user ID", "sub", sub, "error", err) + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + } + + // parseSessionToken requires exp, so a validated token always has one. + expiresAt, err := claims.GetExpirationTime() + if err != nil || expiresAt == nil { + slog.Warn("logout: unreadable exp claim", "sub", sub, "error", err) + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + } + + if err := revoker.RevokeToken(r.Context(), jti, userID, expiresAt.Time); err != nil { + slog.Error("logout: failed to revoke token", "sub", sub, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "internal server error"}) + return + } + + slog.Info("token revoked", "sub", sub) + w.WriteHeader(http.StatusNoContent) + } +} diff --git a/auth_test.go b/auth_test.go index b38f06c..b8b6186 100644 --- a/auth_test.go +++ b/auth_test.go @@ -717,3 +717,159 @@ func TestRequireAuthCookiePath_RejectsRevokedToken(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, w.Code) assert.Equal(t, "invalid token", errorBody(t, w)) } + +// logoutRequest builds an authenticated logout request whose context carries +// the claims requireAuth would have put there. +func logoutRequest(t *testing.T, tokenStr string) *http.Request { + t.Helper() + claims, err := parseSessionToken(tokenStr, testSecret) + require.NoError(t, err) + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/logout", nil) + return req.WithContext(contextWithClaims(req.Context(), claims)) +} + +func TestHandleLogout_Returns204(t *testing.T) { + token, err := generateJWT(&User{ID: 5, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + w := httptest.NewRecorder() + handleLogout(newFakeRevocations())(w, logoutRequest(t, token)) + + assert.Equal(t, http.StatusNoContent, w.Code) + assert.Empty(t, w.Body.String(), "204 means no body") +} + +func TestHandleLogout_RevokesCallerToken(t *testing.T) { + token, err := generateJWT(&User{ID: 5, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + w := httptest.NewRecorder() + handleLogout(revocations)(w, logoutRequest(t, token)) + + require.Equal(t, http.StatusNoContent, w.Code) + assert.Contains(t, revocations.revoked, jtiOf(t, token), "the caller's own jti must be revoked") + assert.Equal(t, int64(5), revocations.lastUserID, "user_id comes from the string sub claim") + assert.WithinDuration(t, time.Now().Add(tokenLifetime), revocations.lastExpiresAt, time.Minute, + "expires_at must be the token's own exp") +} + +// TestHandleLogout_Idempotent covers the handler and store contract: a repeat +// logout must not error. End to end the second call actually gets a 401, +// because requireAuth rejects the now-revoked token before the handler runs — +// the idempotency that matters is the store's ON CONFLICT DO NOTHING. +func TestHandleLogout_Idempotent(t *testing.T) { + token, err := generateJWT(&User{ID: 5, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + for i := 0; i < 2; i++ { + w := httptest.NewRecorder() + handleLogout(revocations)(w, logoutRequest(t, token)) + assert.Equal(t, http.StatusNoContent, w.Code, "a repeat logout must not error") + } + assert.Equal(t, 2, revocations.revokeCalls, "both calls reach the store; the store deduplicates") +} + +func TestHandleLogout_MissingClaims(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/logout", nil) + w := httptest.NewRecorder() + handleLogout(newFakeRevocations())(w, req) + + assert.Equal(t, http.StatusUnauthorized, w.Code) + assert.Equal(t, "unauthorized", errorBody(t, w)) +} + +// TestHandleLogout_TokenWithoutJti covers the compatibility shim's logout +// side: there is nothing to revoke, but the client still gets a 204 so old +// and new clients see one contract. +// Not safe for t.Parallel(); uses global logger. +func TestHandleLogout_TokenWithoutJti(t *testing.T) { + claims := jwt.MapClaims{ + "sub": "5", + "exp": time.Now().Add(time.Hour).Unix(), + "iss": "vehicle-positions-api", + } + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/logout", nil) + req = req.WithContext(contextWithClaims(req.Context(), claims)) + + var logs bytes.Buffer + original := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(original) }) + + revocations := newFakeRevocations() + w := httptest.NewRecorder() + handleLogout(revocations)(w, req) + + assert.Equal(t, http.StatusNoContent, w.Code) + assert.Zero(t, revocations.revokeCalls, "there is no jti to record") + assert.Contains(t, logs.String(), "nothing to revoke") +} + +// Not safe for t.Parallel(); uses global logger. +func TestHandleLogout_StoreError(t *testing.T) { + token, err := generateJWT(&User{ID: 5, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + revocations := newFakeRevocations() + revocations.err = errors.New("database unavailable") + + var logs bytes.Buffer + original := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelError}))) + t.Cleanup(func() { slog.SetDefault(original) }) + + w := httptest.NewRecorder() + handleLogout(revocations)(w, logoutRequest(t, token)) + + assert.Equal(t, http.StatusInternalServerError, w.Code) + assert.Equal(t, "internal server error", errorBody(t, w)) + assert.Contains(t, logs.String(), "failed to revoke token", + "a failed revocation must be logged, not swallowed") +} + +// TestLoginLogoutRevokeFlow walks the whole lifecycle through the real mux: +// log in, use the token, log out, then find the same token rejected. +func TestLoginLogoutRevokeFlow(t *testing.T) { + hash, err := bcrypt.GenerateFromPassword([]byte("password"), bcryptCost) + require.NoError(t, err) + users := &mockUserStore{user: &User{ + ID: 11, + Email: "driver@test.com", + PasswordHash: string(hash), + Role: "driver", + Active: true, + }} + revocations := newFakeRevocations() + + login := handleLogin(users, testSecret, nil, false) + w := postLogin(login, "driver@test.com", "password") + require.Equal(t, http.StatusOK, w.Code) + var loginResp LoginResponse + require.NoError(t, json.NewDecoder(w.Body).Decode(&loginResp)) + require.NotEmpty(t, loginResp.Token) + + authed := requireAuth(testSecret, revocations) + protected := authed(dummyHandler()) + + call := func() *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, "/api/v1/locations", nil) + req.Header.Set("Authorization", "Bearer "+loginResp.Token) + rec := httptest.NewRecorder() + protected.ServeHTTP(rec, req) + return rec + } + + require.Equal(t, http.StatusOK, call().Code, "the fresh token must work") + + logoutReq := httptest.NewRequest(http.MethodPost, "/api/v1/auth/logout", nil) + logoutReq.Header.Set("Authorization", "Bearer "+loginResp.Token) + logoutRec := httptest.NewRecorder() + authed(handleLogout(revocations)).ServeHTTP(logoutRec, logoutReq) + require.Equal(t, http.StatusNoContent, logoutRec.Code) + + after := call() + assert.Equal(t, http.StatusUnauthorized, after.Code, "the same token must now be rejected") + assert.Equal(t, "invalid token", errorBody(t, after)) +} diff --git a/main.go b/main.go index 281d94c..4b821f0 100644 --- a/main.go +++ b/main.go @@ -64,6 +64,7 @@ func newMux(store appStore, tracker *Tracker, rateLimiter *VehicleRateLimiter, j adminMiddleware := requireAdmin() mux.Handle("POST /api/v1/auth/login", handleLogin(store, jwtSecret, loginLimiter, trustProxy)) + mux.Handle("POST /api/v1/auth/logout", authMiddleware(handleLogout(store))) mux.HandleFunc("GET /gtfs-rt/vehicle-positions", handleGetFeed(tracker)) mux.Handle("GET /api/v1/admin/status", authMiddleware(adminMiddleware(handleAdminStatus(tracker, startTime)))) mux.Handle("GET /api/v1/admin/vehicles", authMiddleware(adminMiddleware(handleListVehicles(store)))) diff --git a/route_wiring_test.go b/route_wiring_test.go index 737ebda..bb42588 100644 --- a/route_wiring_test.go +++ b/route_wiring_test.go @@ -322,6 +322,38 @@ func TestAdminPageRoutes_Wiring(t *testing.T) { } } +// TestLogoutRoute_Wiring verifies POST /api/v1/auth/logout is authenticated +// but not admin-gated: any logged-in user must be able to log themselves out, +// and an unauthenticated caller must not reach the handler. +func TestLogoutRoute_Wiring(t *testing.T) { + driverToken, err := generateJWT(&User{ID: 1, Email: "driver@test.com", Role: "driver"}, testSecret) + require.NoError(t, err) + + mux := newMux(&noopStore{}, nil, nil, testSecret, time.Time{}, nil, false) + + tests := []struct { + name string + authHeader string + wantStatus int + }{ + {"no token", "", http.StatusUnauthorized}, + {"driver token", "Bearer " + driverToken, http.StatusNoContent}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/logout", nil) + if tc.authHeader != "" { + req.Header.Set("Authorization", tc.authHeader) + } + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + assert.Equal(t, tc.wantStatus, w.Code) + }) + } +} + // TestDriverVehiclesRoute_Wiring verifies GET /api/v1/vehicles requires // authentication (401 with no token) and accepts any authenticated driver // (200 with a driver-role token) — no admin role required, unlike the From 83abd0b65ab927a2ff0f202cc60d44bfd5eac044 Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Sat, 5 Sep 2026 01:47:19 +0530 Subject: [PATCH 5/6] docs: document token revocation and the logout endpoint Adds POST /api/v1/auth/logout to the endpoint table and the Milestone 2 deliverables, plus a section covering its status codes, the fail-closed behavior, the shared cookie/API session, and the pre-jti compatibility window. Updates the deactivation-window note in both README.md and docs/development.md: a session can now be ended on demand, but deactivating a user still does not auto-revoke their existing tokens. That needs a per-user cutoff rather than the per-token blocklist added here, and is called out as a follow-up so the note does not overstate what landed. Status codes verified against the handlers rather than assumed. --- README.md | 53 ++++++++++++++++++++++++++++++++++++++++++--- docs/development.md | 11 ++++++++-- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 8c71a84..08de235 100644 --- a/README.md +++ b/README.md @@ -42,9 +42,15 @@ Sign in with an existing admin account. To create the first one: - **Local development:** load [`seed_dev.sql`](seed_dev.sql), which seeds `admin@test.com` / `password` (alongside a seed driver). -Deactivating a user blocks new logins immediately, but it doesn't revoke -sessions already issued — any existing session cookie or JWT for that user -stays valid until it expires (up to 24 hours). +Signing out of the admin UI revokes that session's token server-side, so the +cookie is dead even if someone copied its value — the same revocation +`POST /api/v1/auth/logout` performs for API clients. + +Deactivating a user still blocks new logins immediately without revoking +sessions already issued: an existing session cookie or JWT for that user stays +valid until it expires (up to 24 hours) or until that session is logged out. +Forcing a deactivated user's sessions to end is a follow-up — it needs a +per-user cutoff rather than the per-token blocklist added here. Behind a reverse proxy (nginx, an ALB, etc.), set `TRUST_PROXY_HEADERS=true` so the server reads the real client IP and scheme from `X-Forwarded-For` / @@ -155,6 +161,7 @@ The feed is served at a configurable HTTP endpoint (e.g., `GET /gtfs-rt/vehicle- |Endpoint |Method|Purpose | |--------------------------------|------|----------------------------------------------------| |`POST /api/v1/auth/login` |POST |Driver login → returns JWT | +|`POST /api/v1/auth/logout` |POST |Revoke the caller's own JWT → 204 No Content | |`POST /api/v1/locations` |POST |Single location report from driver app | |`GET /gtfs-rt/vehicle-positions`|GET |GTFS-RT feed (protobuf or JSON) | |`GET /api/v1/admin/vehicles` |GET |List vehicles | @@ -218,6 +225,45 @@ curl -i -X POST http://localhost:8080/api/v1/locations \ -d '{"vehicle_id":"bus-1","latitude":-1.29,"longitude":36.82,"timestamp":1752566400}{"extra":1}' ``` +**`POST /api/v1/auth/logout` — token revocation** + +JWTs are stateless, so until now the only way to end a session early was to +rotate `JWT_SECRET`, which logs out every user at once. Logout records the +token's `jti` claim in a `revoked_tokens` blocklist, and every authenticated +request checks it, so a single token can be retired for the rest of its +lifetime. + +- Authenticated (`Authorization: Bearer `), but not admin-only — every + user can log themselves out. +- Revokes the caller's own token; there is no way to revoke someone else's. +- Returns `204 No Content` on success, `401 Unauthorized` without a valid + token, and `500 Internal Server Error` if the revocation can't be recorded. +- The revocation check fails closed: if the database is unreachable, + authenticated requests are rejected rather than allowed through. +- The admin UI's `vp_session` cookie carries the same JWT, so logging out + through the API also ends that browser session (and vice versa). + +```bash +# Log in, use the token, then revoke it +TOKEN=$(curl -s -X POST http://localhost:8080/api/v1/auth/login \ + -H "Content-Type: application/json" \ + -d '{"email":"admin@test.com","password":"password"}' | jq -r .token) + +curl -i -X POST http://localhost:8080/api/v1/auth/logout \ + -H "Authorization: Bearer $TOKEN" # -> 204 No Content + +curl -i http://localhost:8080/api/v1/admin/status \ + -H "Authorization: Bearer $TOKEN" # -> 401 {"error":"invalid token"} +``` + +Tokens issued before this endpoint existed carry no `jti` and cannot be +revoked. They are still accepted (so deploying doesn't sign everyone out) and +logged with a warning; because tokens live 24 hours, that warning stops +appearing within a day of deploying. + +Revocation rows are never deleted — the table grows one row per logout. A +periodic cleanup job keyed on `expires_at` is a planned follow-up. + **Technology Stack:** - **Language:** Go (aligns with Maglev and OTSF’s server-side direction) @@ -353,6 +399,7 @@ This timeline follows the GSoC 2026 standard coding period (May 25 – August 24 - Implement user authentication: - `POST /api/v1/auth/login` — email + password → JWT token + - `POST /api/v1/auth/logout` — revoke the caller's token server-side - JWT middleware for all authenticated endpoints - Token refresh flow - Implement API key authentication for feed consumers (separate from user auth) diff --git a/docs/development.md b/docs/development.md index e7fcf16..a86918a 100644 --- a/docs/development.md +++ b/docs/development.md @@ -89,9 +89,16 @@ instead of the seed one, set `ADMIN_BOOTSTRAP_EMAIL` / `ADMIN_BOOTSTRAP_PASSWORD` before the server's first boot — it only creates an admin when none exist yet, so it's safe to leave set across restarts. +Signing out (the admin UI's sign-out button, or `POST /api/v1/auth/logout` +for API clients) revokes that session's JWT server-side, so the token is +rejected from then on rather than merely dropped by the client. Both surfaces +share one token, so logging out of either ends both. + Deactivating a user blocks new logins immediately, but existing sessions and -tokens for that user remain valid until they expire (up to 24 hours) — this -isn't instant revocation. +tokens for that user remain valid until they expire (up to 24 hours) or are +logged out — deactivation still isn't instant revocation. That needs a +per-user cutoff rather than the per-token blocklist logout uses, and is a +planned follow-up. If you're changing anything under `web/templates` or `web/styles/input.css`, rebuild the compiled Tailwind CSS before checking your changes in the From 7052e1c63ce746663a6913ba7b61c50674b2773d Mon Sep 17 00:00:00 2001 From: DIVESH PATIL Date: Mon, 7 Sep 2026 01:04:41 +0530 Subject: [PATCH 6/6] fix: keep revocation rows when their user is deleted revoked_tokens.user_id was NOT NULL REFERENCES users(id) ON DELETE CASCADE, copying the shape of user_vehicles. That was the wrong lifetime to copy: an assignment row is meaningless once its user is gone, but a revocation row is most needed exactly then. DELETE /api/v1/admin/users/{id} is a hard delete (DELETE FROM users WHERE id = $1), so it cascaded away that user's revocation rows. Nothing in the request path consults the users table -- requireAuth and adminClaimsFromCookie decide on the blocklist alone -- so once the row went, an explicitly revoked token was accepted again for the remainder of its 24h life, carrying its original role claim. A revoked admin token regained every /api/v1/admin/* route. user_id is now nullable with ON DELETE SET NULL, so the row outlives the account with its owner forgotten. The FK still rejects a revocation for a user that never existed. Only the expiry-based cleanup job should ever remove a row, which is what expires_at and its index are already there for. TestStore_RevokeToken_CascadesOnUserDelete asserted the old behavior as intended, so nothing would have caught this later. Replaced with TestStore_RevokeToken_SurvivesUserDelete, which asserts the row remains, the token still reads as revoked, and user_id is NULL rather than dangling. Reported by @aaronbrethorst in review on #98. --- db/models.go | 2 +- db/query.sql.go | 2 +- migrations/000013_add_revoked_tokens.up.sql | 2 +- store_revocation.go | 12 +++++++++-- store_revocation_test.go | 24 ++++++++++++++++++--- 5 files changed, 34 insertions(+), 8 deletions(-) diff --git a/db/models.go b/db/models.go index 9426e6c..100c56a 100644 --- a/db/models.go +++ b/db/models.go @@ -24,7 +24,7 @@ type LocationPoint struct { type RevokedToken struct { Jti string - UserID int64 + UserID pgtype.Int8 ExpiresAt pgtype.Timestamptz RevokedAt pgtype.Timestamptz } diff --git a/db/query.sql.go b/db/query.sql.go index 0ebb33a..4263ef9 100644 --- a/db/query.sql.go +++ b/db/query.sql.go @@ -769,7 +769,7 @@ ON CONFLICT (jti) DO NOTHING type RevokeTokenParams struct { Jti string - UserID int64 + UserID pgtype.Int8 ExpiresAt pgtype.Timestamptz } diff --git a/migrations/000013_add_revoked_tokens.up.sql b/migrations/000013_add_revoked_tokens.up.sql index 4472da2..04d769c 100644 --- a/migrations/000013_add_revoked_tokens.up.sql +++ b/migrations/000013_add_revoked_tokens.up.sql @@ -1,6 +1,6 @@ CREATE TABLE IF NOT EXISTS revoked_tokens ( jti TEXT PRIMARY KEY CHECK (jti != ''), - user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + user_id BIGINT REFERENCES users(id) ON DELETE SET NULL, expires_at TIMESTAMPTZ NOT NULL, revoked_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); diff --git a/store_revocation.go b/store_revocation.go index 732763e..a2932ae 100644 --- a/store_revocation.go +++ b/store_revocation.go @@ -24,14 +24,22 @@ type TokenChecker interface { // RevokeToken adds a jti to the revocation list. It is idempotent, so logging // out twice with the same token succeeds both times. // +// A revocation row deliberately outlives the user it belongs to. user_id is +// ON DELETE SET NULL rather than CASCADE because nothing in the request path +// consults the users table — requireAuth and adminClaimsFromCookie decide on +// the blocklist alone — so cascading the row away on a hard user delete would +// make an already-revoked token valid again for the rest of its lifetime, +// carrying its original role claim. The row's job is to block a jti until it +// expires, and that job does not end when the account does. +// // expires_at is recorded so revocation rows can be aged out, but nothing // deletes them yet: this table grows one row per logout. A periodic cleanup // job (DELETE FROM revoked_tokens WHERE expires_at < NOW()) is needed as a -// follow-up. +// follow-up, and is the only thing that should ever remove a row. func (s *Store) RevokeToken(ctx context.Context, jti string, userID int64, expiresAt time.Time) error { err := s.queries.RevokeToken(ctx, db.RevokeTokenParams{ Jti: jti, - UserID: userID, + UserID: pgtype.Int8{Int64: userID, Valid: true}, ExpiresAt: pgtype.Timestamptz{Time: expiresAt, Valid: true}, }) if err != nil { diff --git a/store_revocation_test.go b/store_revocation_test.go index 40b62e7..4fa9eec 100644 --- a/store_revocation_test.go +++ b/store_revocation_test.go @@ -123,7 +123,15 @@ func TestStore_RevokeToken_EmptyJtiRejected(t *testing.T) { assert.Error(t, err, "the CHECK (jti != '') constraint must reject an empty jti") } -func TestStore_RevokeToken_CascadesOnUserDelete(t *testing.T) { +// TestStore_RevokeToken_SurvivesUserDelete is the regression test for the +// review finding on #98: with ON DELETE CASCADE, hard-deleting a user (which +// DELETE /api/v1/admin/users/{id} does) dropped their revocation rows, and +// because nothing in the request path consults the users table, an already +// revoked token became valid again for the rest of its 24h life — with its +// original role claim, so a revoked admin token regained every admin route. +// The row must outlive the account; only the expiry-based cleanup job should +// ever remove it. +func TestStore_RevokeToken_SurvivesUserDelete(t *testing.T) { store := newTestStore(t) ctx := context.Background() userID := insertRevocationTestUser(t, store) @@ -136,6 +144,16 @@ func TestStore_RevokeToken_CascadesOnUserDelete(t *testing.T) { _, err = store.pool.Exec(ctx, "DELETE FROM users WHERE id = $1", userID) require.NoError(t, err) - assert.Equal(t, 0, countRevocationRows(t, store, jti), - "ON DELETE CASCADE must remove the deleted user's revocation rows") + assert.Equal(t, 1, countRevocationRows(t, store, jti), + "deleting the user must not remove the revocation row") + + revoked, err := store.IsTokenRevoked(ctx, jti) + require.NoError(t, err) + assert.True(t, revoked, "the token must stay revoked after its user is deleted") + + // ON DELETE SET NULL: the row survives with its owner forgotten. + var ownerID *int64 + err = store.pool.QueryRow(ctx, "SELECT user_id FROM revoked_tokens WHERE jti = $1", jti).Scan(&ownerID) + require.NoError(t, err) + assert.Nil(t, ownerID, "user_id must be NULLed, not carry a dangling id") }