diff --git a/cmd/api/main.go b/cmd/api/main.go index 1302207..17577a5 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -405,7 +405,7 @@ func main() { api.Use(middleware.RequestTimeoutMiddleware(15 * time.Second)) // Centralized auth middleware — all routes require auth except explicit public paths - api.Use(middleware.AuthMiddleware(jwtManager, []string{ + api.Use(middleware.AuthMiddlewareWithState(jwtManager, []string{ "/auth/register", "/auth/login", "/auth/refresh", @@ -424,7 +424,7 @@ func main() { // unlike this literal-path allowlist, "/sign/:token" is a gin route // *pattern* that AuthMiddleware matches via c.FullPath(). "/sign/:token", - })) + }, authService.SessionState)) if os.Getenv("DISABLE_RATE_LIMIT") != "true" { // Coarse global limiter on every authenticated route. Previously only diff --git a/db/migrations/000050_add_tokens_valid_after.down.sql b/db/migrations/000050_add_tokens_valid_after.down.sql new file mode 100644 index 0000000..13dfe36 --- /dev/null +++ b/db/migrations/000050_add_tokens_valid_after.down.sql @@ -0,0 +1 @@ +ALTER TABLE users DROP COLUMN IF EXISTS tokens_valid_after; diff --git a/db/migrations/000050_add_tokens_valid_after.up.sql b/db/migrations/000050_add_tokens_valid_after.up.sql new file mode 100644 index 0000000..3998bce --- /dev/null +++ b/db/migrations/000050_add_tokens_valid_after.up.sql @@ -0,0 +1,14 @@ +-- Session epoch for access-token invalidation. +-- +-- Access tokens are stateless 15-minute JWTs and AuthMiddleware only checked +-- the signature and expiry, so nothing could revoke one early. Disabling an +-- account or changing a password deleted the user's REFRESH tokens, which only +-- stops the session being extended -- the outstanding access token kept working +-- for up to 15 more minutes, including writes. +-- +-- Bumping tokens_valid_after invalidates every access token issued before that +-- instant. NULL means "no invalidation event yet" and all tokens are accepted. +ALTER TABLE users ADD COLUMN tokens_valid_after TIMESTAMPTZ; + +COMMENT ON COLUMN users.tokens_valid_after IS + 'Access tokens issued before this instant are rejected. Bumped on password change, account disable and admin 2FA reset.'; diff --git a/internal/api/handlers/admin_users.go b/internal/api/handlers/admin_users.go index a4e09fb..69e244f 100644 --- a/internal/api/handlers/admin_users.go +++ b/internal/api/handlers/admin_users.go @@ -164,9 +164,15 @@ func (h *APIHandler) DisableUser(c *gin.Context, userId openapi_types.UUID) { return } - // Revoke all tokens + // Revoke all tokens. Deleting refresh tokens only prevents the session + // being extended -- bump the session epoch too so the target's outstanding + // access token stops working immediately rather than up to 15 minutes + // later, which is exactly the window that matters when disabling an + // abusive or compromised account. _, _ = h.db.ExecContext(c.Request.Context(), "DELETE FROM refresh_tokens WHERE user_id = $1", targetID) + _, _ = h.db.ExecContext(c.Request.Context(), + "UPDATE users SET tokens_valid_after = NOW() WHERE id = $1", targetID) h.logAdminAction(c, adminUserID, "disable_user", &targetID, map[string]any{"email": user.Email}) @@ -252,6 +258,14 @@ func (h *APIHandler) ResetUser2fa(c *gin.Context, userId openapi_types.UUID) { return } + // Removing the second factor is a security event: end existing sessions so + // the reset cannot be used to keep a session that was established with the + // factor now being cleared. + _, _ = h.db.ExecContext(c.Request.Context(), + "DELETE FROM refresh_tokens WHERE user_id = $1", targetID) + _, _ = h.db.ExecContext(c.Request.Context(), + "UPDATE users SET tokens_valid_after = NOW() WHERE id = $1", targetID) + h.logAdminAction(c, adminUserID, "reset_2fa", &targetID, map[string]any{"email": user.Email}) c.JSON(http.StatusOK, gin.H{"message": "2FA reset for user"}) diff --git a/internal/api/handlers/handlers_test.go b/internal/api/handlers/handlers_test.go index 1943067..cff620f 100644 --- a/internal/api/handlers/handlers_test.go +++ b/internal/api/handlers/handlers_test.go @@ -1776,6 +1776,14 @@ func (m *mockHandlerNotifRepo) GetNotificationHistory(_ context.Context, _ uuid. return []*models.NotificationLog{}, 0, nil } +func (m *mockUserRepo) InvalidateTokensBefore(_ context.Context, id uuid.UUID, at time.Time) error { + if u, ok := m.users[id]; ok { + t := at + u.TokensValidAfter = &t + } + return nil +} + // ConsumeRecoveryCode mirrors the atomic DB behaviour: it removes the hash and // reports whether THIS call was the one that removed it. func (m *mockUserRepo) ConsumeRecoveryCode(_ context.Context, id uuid.UUID, codeHash string) (bool, error) { diff --git a/internal/api/middleware/auth.go b/internal/api/middleware/auth.go index e2663a3..33658d3 100644 --- a/internal/api/middleware/auth.go +++ b/internal/api/middleware/auth.go @@ -3,6 +3,9 @@ package middleware import ( "net/http" "strings" + "time" + + "github.com/google/uuid" "github.com/fjaeckel/ninerlog-api/pkg/jwt" "github.com/gin-gonic/gin" @@ -11,7 +14,27 @@ import ( // AuthMiddleware enforces JWT authentication on all routes except explicitly // allowed public paths. It extracts the user ID from the token and sets it // in the Gin context as "userID". +// UserSessionState reports whether a user still exists, is enabled, and the +// instant before which their access tokens are no longer valid. Implemented by +// the auth service; nil disables the check (used by tests). +type UserSessionState func(userID uuid.UUID) (disabled bool, tokensValidAfter *time.Time, err error) + +// AuthMiddleware enforces JWT authentication. See AuthMiddlewareWithState for +// the session-revocation variant. func AuthMiddleware(jwtManager *jwt.Manager, publicPaths []string) gin.HandlerFunc { + return AuthMiddlewareWithState(jwtManager, publicPaths, nil) +} + +// AuthMiddlewareWithState additionally rejects tokens belonging to a disabled +// or deleted user, and tokens issued before the user's session epoch. +// +// Access tokens are stateless 15-minute JWTs, so without this a token stayed +// usable for its full lifetime no matter what happened to the account: +// disabling a user or changing a password only deleted REFRESH tokens, which +// merely stops the session being extended. Both were confirmed against a +// running instance -- a disabled user's token still read and created flights, +// and an old token still worked after a password change. +func AuthMiddlewareWithState(jwtManager *jwt.Manager, publicPaths []string, state UserSessionState) gin.HandlerFunc { // Build a set for O(1) lookup public := make(map[string]bool, len(publicPaths)) for _, p := range publicPaths { @@ -57,6 +80,30 @@ func AuthMiddleware(jwtManager *jwt.Manager, publicPaths []string) gin.HandlerFu return } + if state != nil { + disabled, validAfter, err := state(claims.UserID) + if err != nil { + // Unknown or deleted user. + c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or expired token"}) + c.Abort() + return + } + if disabled { + c.JSON(http.StatusForbidden, gin.H{"error": "Account disabled"}) + c.Abort() + return + } + // Reject tokens minted before the last invalidation event. IssuedAt + // has second resolution, so a token issued in the same second as the + // event is also rejected (Before would let it through). + if validAfter != nil && claims.IssuedAt != nil && + !claims.IssuedAt.Time.After(*validAfter) { + c.JSON(http.StatusUnauthorized, gin.H{"error": "Session expired, please sign in again"}) + c.Abort() + return + } + } + // Set user ID in context for handlers to use c.Set("userID", claims.UserID) c.Next() diff --git a/internal/api/middleware/auth_session_state_test.go b/internal/api/middleware/auth_session_state_test.go new file mode 100644 index 0000000..a01f68a --- /dev/null +++ b/internal/api/middleware/auth_session_state_test.go @@ -0,0 +1,99 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/fjaeckel/ninerlog-api/pkg/jwt" + "github.com/gin-gonic/gin" + "github.com/google/uuid" +) + +// Access tokens are stateless 15-minute JWTs, so before this check nothing +// could revoke one early: disabling an account or changing a password only +// deleted REFRESH tokens, which merely stops the session being extended. Both +// were confirmed against a running instance -- a disabled user's token still +// read and created flights, and an old token still worked after a password +// change. +func newStateRouter(t *testing.T, mgr *jwt.Manager, state UserSessionState) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + r := gin.New() + api := r.Group("/api/v1") + api.Use(AuthMiddlewareWithState(mgr, []string{"/auth/login"}, state)) + api.GET("/users/me", func(c *gin.Context) { c.Status(http.StatusOK) }) + return r +} + +func doAuthed(r *gin.Engine, token string) int { + req := httptest.NewRequest("GET", "/api/v1/users/me", nil) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w.Code +} + +func TestAuthMiddleware_RejectsDisabledUser(t *testing.T) { + mgr := jwt.NewManager("access-secret", "refresh-secret", 15*time.Minute, time.Hour) + uid := uuid.New() + tok, err := mgr.GenerateAccessToken(uid) + if err != nil { + t.Fatalf("token: %v", err) + } + + enabled := newStateRouter(t, mgr, func(uuid.UUID) (bool, *time.Time, error) { return false, nil, nil }) + if got := doAuthed(enabled, tok); got != http.StatusOK { + t.Fatalf("enabled user: %d, want 200", got) + } + + disabled := newStateRouter(t, mgr, func(uuid.UUID) (bool, *time.Time, error) { return true, nil, nil }) + if got := doAuthed(disabled, tok); got != http.StatusForbidden { + t.Errorf("disabled user's token still accepted: %d, want 403", got) + } +} + +func TestAuthMiddleware_RejectsTokenIssuedBeforeSessionEpoch(t *testing.T) { + mgr := jwt.NewManager("access-secret", "refresh-secret", 15*time.Minute, time.Hour) + tok, err := mgr.GenerateAccessToken(uuid.New()) + if err != nil { + t.Fatalf("token: %v", err) + } + + // Epoch in the future: the token predates it and must be rejected. + future := time.Now().Add(time.Hour) + stale := newStateRouter(t, mgr, func(uuid.UUID) (bool, *time.Time, error) { return false, &future, nil }) + if got := doAuthed(stale, tok); got != http.StatusUnauthorized { + t.Errorf("token issued before the session epoch was accepted: %d, want 401", got) + } + + // Epoch in the past: the token was issued after it and stays valid. + past := time.Now().Add(-time.Hour) + fresh := newStateRouter(t, mgr, func(uuid.UUID) (bool, *time.Time, error) { return false, &past, nil }) + if got := doAuthed(fresh, tok); got != http.StatusOK { + t.Errorf("token issued after the session epoch was rejected: %d, want 200", got) + } +} + +func TestAuthMiddleware_RejectsDeletedUser(t *testing.T) { + mgr := jwt.NewManager("access-secret", "refresh-secret", 15*time.Minute, time.Hour) + tok, _ := mgr.GenerateAccessToken(uuid.New()) + + gone := newStateRouter(t, mgr, func(uuid.UUID) (bool, *time.Time, error) { + return false, nil, http.ErrNoLocation // stand-in for "not found" + }) + if got := doAuthed(gone, tok); got != http.StatusUnauthorized { + t.Errorf("deleted user's token accepted: %d, want 401", got) + } +} + +// A nil state function preserves the previous behaviour for callers that do not +// supply one (AuthMiddleware delegates here). +func TestAuthMiddleware_NilStateSkipsChecks(t *testing.T) { + mgr := jwt.NewManager("access-secret", "refresh-secret", 15*time.Minute, time.Hour) + tok, _ := mgr.GenerateAccessToken(uuid.New()) + if got := doAuthed(newStateRouter(t, mgr, nil), tok); got != http.StatusOK { + t.Errorf("nil state: %d, want 200", got) + } +} diff --git a/internal/models/user.go b/internal/models/user.go index 8d6c487..74462dc 100644 --- a/internal/models/user.go +++ b/internal/models/user.go @@ -19,17 +19,22 @@ type User struct { RecoveryCodes pq.StringArray `json:"-"` // never exposed in JSON FailedLoginAttempts int `json:"-"` LockedUntil *time.Time `json:"-"` - Disabled bool `json:"disabled"` - LastLoginAt *time.Time `json:"lastLoginAt,omitempty"` - TimeDisplayFormat string `json:"timeDisplayFormat"` - DateFormat string `json:"dateFormat"` - DecimalSeparator string `json:"decimalSeparator"` - PreferredLocale string `json:"preferredLocale"` + // TokensValidAfter invalidates access tokens issued before this instant. + // Bumped on password change, account disable and admin 2FA reset so an + // outstanding 15-minute access token cannot outlive the event. Nil means + // no invalidation event has occurred. + TokensValidAfter *time.Time `json:"-"` + Disabled bool `json:"disabled"` + LastLoginAt *time.Time `json:"lastLoginAt,omitempty"` + TimeDisplayFormat string `json:"timeDisplayFormat"` + DateFormat string `json:"dateFormat"` + DecimalSeparator string `json:"decimalSeparator"` + PreferredLocale string `json:"preferredLocale"` // Informational 90-day recency indicator preferences (FCL.060(b)-style) - RecencyPerModel bool `json:"recencyPerModel"` - RecencyPerRegistration bool `json:"recencyPerRegistration"` - CreatedAt time.Time `json:"created_at"` - UpdatedAt time.Time `json:"updated_at"` + RecencyPerModel bool `json:"recencyPerModel"` + RecencyPerRegistration bool `json:"recencyPerRegistration"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` } // RefreshToken represents a refresh token in the system diff --git a/internal/repository/interfaces.go b/internal/repository/interfaces.go index 9036323..cd37c84 100644 --- a/internal/repository/interfaces.go +++ b/internal/repository/interfaces.go @@ -36,6 +36,10 @@ type UserRepository interface { LockAccount(ctx context.Context, id uuid.UUID, until time.Time) error // MarkEmailVerified flips the email_verified flag to true. + // InvalidateTokensBefore bumps the user's session epoch so access tokens + // issued before the given instant are rejected. + InvalidateTokensBefore(ctx context.Context, id uuid.UUID, at time.Time) error + // ConsumeRecoveryCode atomically removes a recovery code hash, returning // true only for the caller that actually removed it. Prevents the same // single-use code authenticating more than once under concurrency. diff --git a/internal/repository/postgres/user.go b/internal/repository/postgres/user.go index 80b5afd..8bc1aac 100644 --- a/internal/repository/postgres/user.go +++ b/internal/repository/postgres/user.go @@ -60,7 +60,7 @@ func (r *UserRepository) Create(ctx context.Context, user *models.User) error { func (r *UserRepository) GetByEmail(ctx context.Context, email string) (*models.User, error) { query := ` SELECT id, email, password_hash, name, email_verified, two_factor_enabled, two_factor_secret, recovery_codes, - failed_login_attempts, locked_until, disabled, last_login_at, time_display_format, date_format, decimal_separator, preferred_locale, recency_per_model, recency_per_registration, created_at, updated_at + failed_login_attempts, locked_until, disabled, last_login_at, time_display_format, date_format, decimal_separator, preferred_locale, recency_per_model, recency_per_registration, tokens_valid_after, created_at, updated_at FROM users WHERE email = $1 ` @@ -85,6 +85,7 @@ func (r *UserRepository) GetByEmail(ctx context.Context, email string) (*models. &user.PreferredLocale, &user.RecencyPerModel, &user.RecencyPerRegistration, + &user.TokensValidAfter, &user.CreatedAt, &user.UpdatedAt, ) @@ -102,7 +103,7 @@ func (r *UserRepository) GetByEmail(ctx context.Context, email string) (*models. func (r *UserRepository) GetByID(ctx context.Context, id uuid.UUID) (*models.User, error) { query := ` SELECT id, email, password_hash, name, email_verified, two_factor_enabled, two_factor_secret, recovery_codes, - failed_login_attempts, locked_until, disabled, last_login_at, time_display_format, date_format, decimal_separator, preferred_locale, recency_per_model, recency_per_registration, created_at, updated_at + failed_login_attempts, locked_until, disabled, last_login_at, time_display_format, date_format, decimal_separator, preferred_locale, recency_per_model, recency_per_registration, tokens_valid_after, created_at, updated_at FROM users WHERE id = $1 ` @@ -127,6 +128,7 @@ func (r *UserRepository) GetByID(ctx context.Context, id uuid.UUID) (*models.Use &user.PreferredLocale, &user.RecencyPerModel, &user.RecencyPerRegistration, + &user.TokensValidAfter, &user.CreatedAt, &user.UpdatedAt, ) @@ -239,6 +241,16 @@ func (r *UserRepository) LockAccount(ctx context.Context, id uuid.UUID, until ti return err } +// InvalidateTokensBefore bumps the session epoch so every access token issued +// before now is rejected by AuthMiddleware. Called on password change, account +// disable and admin 2FA reset -- events after which an outstanding 15-minute +// access token must not keep working. +func (r *UserRepository) InvalidateTokensBefore(ctx context.Context, id uuid.UUID, at time.Time) error { + _, err := r.db.ExecContext(ctx, + `UPDATE users SET tokens_valid_after = $1, updated_at = $1 WHERE id = $2`, at, id) + return err +} + // ConsumeRecoveryCode atomically removes one recovery code hash from the user's // list, returning true only if this call was the one that removed it. // diff --git a/internal/repository/postgres/user_test.go b/internal/repository/postgres/user_test.go index 0f6c9a0..38306ca 100644 --- a/internal/repository/postgres/user_test.go +++ b/internal/repository/postgres/user_test.go @@ -54,8 +54,8 @@ func TestUserGetByEmail(t *testing.T) { repo := NewUserRepository(db) ctx := context.Background() - rows := sqlmock.NewRows([]string{"id", "email", "password_hash", "name", "email_verified", "two_factor_enabled", "two_factor_secret", "recovery_codes", "failed_login_attempts", "locked_until", "disabled", "last_login_at", "time_display_format", "date_format", "decimal_separator", "preferred_locale", "recency_per_model", "recency_per_registration", "created_at", "updated_at"}). - AddRow(uuid.New(), "test@example.com", "hashed_password", "Test User", true, false, nil, nil, 0, nil, false, nil, "hm", "DD.MM.YYYY", "comma", "en", true, false, time.Now(), time.Now()) + rows := sqlmock.NewRows([]string{"id", "email", "password_hash", "name", "email_verified", "two_factor_enabled", "two_factor_secret", "recovery_codes", "failed_login_attempts", "locked_until", "disabled", "last_login_at", "time_display_format", "date_format", "decimal_separator", "preferred_locale", "recency_per_model", "recency_per_registration", "tokens_valid_after", "created_at", "updated_at"}). + AddRow(uuid.New(), "test@example.com", "hashed_password", "Test User", true, false, nil, nil, 0, nil, false, nil, "hm", "DD.MM.YYYY", "comma", "en", true, false, nil, time.Now(), time.Now()) mock.ExpectQuery("SELECT (.+) FROM users WHERE email"). WithArgs("test@example.com"). diff --git a/internal/service/auth.go b/internal/service/auth.go index 96f41f0..439f381 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -535,6 +535,17 @@ func (s *AuthService) generateTokenPair(ctx context.Context, userID uuid.UUID) ( }, nil } +// SessionState reports whether a user is disabled and the instant before which +// their access tokens are invalid. Wired into AuthMiddleware so revocation +// events take effect immediately rather than after the token expires. +func (s *AuthService) SessionState(userID uuid.UUID) (bool, *time.Time, error) { + user, err := s.userRepo.GetByID(context.Background(), userID) + if err != nil { + return false, nil, err + } + return user.Disabled, user.TokensValidAfter, nil +} + // GetUserByID retrieves a user by ID func (s *AuthService) GetUserByID(ctx context.Context, userID uuid.UUID) (*models.User, error) { return s.userRepo.GetByID(ctx, userID) @@ -589,6 +600,14 @@ func (s *AuthService) ChangePassword(ctx context.Context, userID uuid.UUID, curr return err } + // Revoking refresh tokens alone only stops the session being EXTENDED; the + // outstanding access token stays valid for up to 15 more minutes. Bump the + // session epoch so it is rejected immediately -- a user who changes their + // password because they suspect compromise expects the other session gone. + if err := s.userRepo.InvalidateTokensBefore(ctx, userID, time.Now()); err != nil { + return err + } + // Revoke all refresh tokens (force re-login on all devices) return s.refreshTokenRepo.RevokeAllForUser(ctx, userID) } diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index 3d89486..b10af2e 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -1030,6 +1030,16 @@ func TestResetPassword_ShortPassword(t *testing.T) { } } +func (m *mockUserRepo) InvalidateTokensBefore(_ context.Context, id uuid.UUID, at time.Time) error { + for _, u := range m.users { + if u.ID == id { + t := at + u.TokensValidAfter = &t + } + } + return nil +} + // ConsumeRecoveryCode mirrors the atomic DB behaviour: it removes the hash and // reports whether THIS call was the one that removed it. func (m *mockUserRepo) ConsumeRecoveryCode(_ context.Context, id uuid.UUID, codeHash string) (bool, error) { diff --git a/internal/service/flight_signature_test.go b/internal/service/flight_signature_test.go index dabb18c..efd8c65 100644 --- a/internal/service/flight_signature_test.go +++ b/internal/service/flight_signature_test.go @@ -548,6 +548,14 @@ func TestCompleteFromToken_UsesDBSourcedOwnerEmail(t *testing.T) { } } +func (m *mockUserRepoForSignature) InvalidateTokensBefore(_ context.Context, id uuid.UUID, at time.Time) error { + if u, ok := m.users[id]; ok { + t := at + u.TokensValidAfter = &t + } + return nil +} + // ConsumeRecoveryCode mirrors the atomic DB behaviour: it removes the hash and // reports whether THIS call was the one that removed it. func (m *mockUserRepoForSignature) ConsumeRecoveryCode(_ context.Context, id uuid.UUID, codeHash string) (bool, error) { diff --git a/internal/service/notification_custom_test.go b/internal/service/notification_custom_test.go index d0a7ba2..dba2f10 100644 --- a/internal/service/notification_custom_test.go +++ b/internal/service/notification_custom_test.go @@ -122,6 +122,10 @@ func TestCustomCurrencyNotifications_RespectsEmailDisabled(t *testing.T) { } } +func (m *mockNotifUserRepo) InvalidateTokensBefore(_ context.Context, _ uuid.UUID, _ time.Time) error { + return nil +} + func (m *mockNotifUserRepo) ConsumeRecoveryCode(_ context.Context, _ uuid.UUID, _ string) (bool, error) { return true, nil } diff --git a/internal/service/twofactor_encryption_test.go b/internal/service/twofactor_encryption_test.go index 5a4f688..9a15b96 100644 --- a/internal/service/twofactor_encryption_test.go +++ b/internal/service/twofactor_encryption_test.go @@ -108,6 +108,10 @@ func TestEncrypted2FA_ReadsLegacyPlaintextSecret(t *testing.T) { } } +func (m *mock2FAUserRepo) InvalidateTokensBefore(_ context.Context, _ uuid.UUID, _ time.Time) error { + return nil +} + func (m *mock2FAUserRepo) ConsumeRecoveryCode(_ context.Context, _ uuid.UUID, _ string) (bool, error) { return true, nil }