diff --git a/internal/auth/errors.go b/internal/auth/errors.go index af2753395..7d29ea85c 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -153,4 +153,8 @@ var ( // ErrAPIKeyPasswordRotated: the owner's password changed after the key was // minted (issue #402). ErrAPIKeyPasswordRotated = errors.New("API key predates the owner's last password change") + + // ErrSessionPasswordRotated: the user's password changed after the session + // was issued (issue #398). + ErrSessionPasswordRotated = errors.New("session predates the user's last password change") ) diff --git a/internal/auth/service.go b/internal/auth/service.go index edaa3795d..83c681f35 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -343,14 +343,8 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return nil, fmt.Errorf("session expired") } - // UpdateUser revokes sessions on deactivation; this rejects any session that - // outlives that (a failed delete, or Active flipped another way). Issue #89. - user, err := s.loadUser(ctx, session.UserID) - if err != nil { - return nil, fmt.Errorf("failed to load session user: %w", err) - } - if !user.Active { - return nil, fmt.Errorf("session not found") + if err := s.checkSessionUser(ctx, session); err != nil { + return nil, err } // Return a copy of the session with the original token (not the hash) for client use. @@ -360,6 +354,27 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session, return &result, nil } +// checkSessionUser rejects a session whose user can no longer use it. +func (s *Service) checkSessionUser(ctx context.Context, session *Session) error { + // UpdateUser revokes sessions on deactivation; this rejects any session that + // outlives that (a failed delete, or Active flipped another way). Issue #89. + user, err := s.loadUser(ctx, session.UserID) + if err != nil { + return fmt.Errorf("failed to load session user: %w", err) + } + if !user.Active { + return fmt.Errorf("session not found") + } + // A password change deletes sessions only best-effort, and a login that + // verified the old password can insert its session after that delete. + // The version bump in the password write itself is what revokes them + // (issue #398). + if session.PasswordVersion != user.PasswordVersion { + return ErrSessionPasswordRotated + } + return nil +} + // ValidateCSRFToken validates the CSRF token for a session. // // The expected CSRF token is derived as HMAC-SHA256(csrfKey, rawSessionToken), diff --git a/internal/auth/service_helpers.go b/internal/auth/service_helpers.go index 35eeaad34..19df575e5 100644 --- a/internal/auth/service_helpers.go +++ b/internal/auth/service_helpers.go @@ -94,6 +94,9 @@ func (s *Service) createSession(ctx context.Context, user *User, userAgent, ipAd UserAgent: userAgent, IPAddress: ipAddress, CSRFToken: csrfToken, // MAC stored for diagnostics only; not used in validation + // The version of the row whose password this login verified, so a + // rotation that commits after that read still revokes the session. + PasswordVersion: user.PasswordVersion, } if err := s.store.CreateSession(ctx, storedSession); err != nil { diff --git a/internal/auth/service_helpers_test.go b/internal/auth/service_helpers_test.go index 0f021b929..fc2c7bfdb 100644 --- a/internal/auth/service_helpers_test.go +++ b/internal/auth/service_helpers_test.go @@ -1,9 +1,12 @@ package auth import ( + "context" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -93,3 +96,58 @@ func TestContainsAny(t *testing.T) { assert.True(t, containsAny(allowed, requested)) }) } + +// A session records the password version of the user row its login verified +// (issue #398). +func TestCreateSession_RecordsPasswordVersion(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + + var stored *Session + mockStore.On("CreateSession", ctx, mock.AnythingOfType("*auth.Session")). + Run(func(args mock.Arguments) { stored = args.Get(1).(*Session) }).Return(nil).Once() + + _, err := service.createSession(ctx, &User{ID: "user-123", Email: "test@example.com", PasswordVersion: 3}, "", "") + require.NoError(t, err) + require.NotNil(t, stored) + assert.Equal(t, int64(3), stored.PasswordVersion) + mockStore.AssertExpectations(t) +} + +// ValidateSession rejects a session issued under another password version, +// whether or not the best-effort session delete ran (issue #398). +func TestValidateSession_PasswordVersion(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name string + sessionVersion int64 + userVersion int64 + wantErr error + }{ + {name: "same version", sessionVersion: 2, userVersion: 2}, + {name: "password changed after login", sessionVersion: 1, userVersion: 2, wantErr: ErrSessionPasswordRotated}, + {name: "session newer than the user row", sessionVersion: 3, userVersion: 2, wantErr: ErrSessionPasswordRotated}, + } { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockStore) + service := createTestService(mockStore, new(MockEmailSender)) + hashed := hashSessionToken("raw-token") + mockStore.On("GetSession", ctx, hashed).Return(&Session{ + Token: hashed, UserID: "user-123", ExpiresAt: time.Now().Add(time.Hour), PasswordVersion: tc.sessionVersion, + }, nil).Once() + mockStore.On("GetUserByID", ctx, "user-123"). + Return(&User{ID: "user-123", Active: true, PasswordVersion: tc.userVersion}, nil).Once() + + session, err := service.ValidateSession(ctx, "raw-token") + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + assert.Nil(t, session) + } else { + require.NoError(t, err) + assert.Equal(t, "raw-token", session.Token) + } + mockStore.AssertExpectations(t) + }) + } +} diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 3be2d3027..b4557bb1b 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -215,7 +215,10 @@ func (s *Service) checkCommonPasswords(password string) error { // attacker out (issue #102). // // Both cleanups are best-effort: failures are logged, not returned, because -// the new password has already taken effect by the time this runs. Shared +// the new password has already taken effect by the time this runs. They are +// not what enforces the rotation: the password write bumps +// users.password_version, and ValidateSession and ValidateUserAPIKey reject +// any credential issued under an older version (issues #398, #402). Shared // by ChangePassword, ConfirmPasswordReset and UpdateUserProfile so the two // checks live in one place instead of three. func (s *Service) invalidateUserCredentialsBestEffort(ctx context.Context, userID, opName string) { diff --git a/internal/auth/service_session_rotation_db_test.go b/internal/auth/service_session_rotation_db_test.go new file mode 100644 index 000000000..142f0e1d5 --- /dev/null +++ b/internal/auth/service_session_rotation_db_test.go @@ -0,0 +1,150 @@ +//go:build integration + +package auth + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// failingInvalidationStore makes both best-effort cleanups after a password +// change fail, as a transient database error would (issue #398). +type failingInvalidationStore struct{ failingKeyListStore } + +func (failingInvalidationStore) DeleteUserSessions(context.Context, string) error { + return errors.New("synthetic session delete failure") +} + +// sessionReadHook runs onRead once, right after the first GetSession returns. +type sessionReadHook struct { + *PostgresStore + onRead func() +} + +func (s *sessionReadHook) GetSession(ctx context.Context, token string) (*Session, error) { + session, err := s.PostgresStore.GetSession(ctx, token) + if hook := s.onRead; hook != nil { + s.onRead = nil + hook() + } + return session, err +} + +func loginRotationUser(t *testing.T, svc *Service, email, password string) string { + t.Helper() + resp, err := svc.Login(t.Context(), LoginRequest{Email: email, Password: password}) + require.NoError(t, err) + return resp.Token +} + +// rotatePassword changes the user's password through each service entry point +// that rotates it, using svc for the write. +var rotatePassword = map[string]func(t *testing.T, svc *Service, store *PostgresStore, user *User){ + "change-password": func(t *testing.T, svc *Service, _ *PostgresStore, user *User) { + require.NoError(t, svc.ChangePassword(t.Context(), user.ID, + ChangePasswordRequest{CurrentPassword: rotationPassword, NewPassword: rotationNewPassword})) + }, + "update-profile": func(t *testing.T, svc *Service, _ *PostgresStore, user *User) { + require.NoError(t, svc.UpdateUserProfile(t.Context(), user.ID, user.Email, rotationPassword, rotationNewPassword)) + }, + "reset-confirm": func(t *testing.T, svc *Service, store *PostgresStore, user *User) { + token := "session-rotation-reset-" + user.ID + _, err := store.db.Exec(t.Context(), `UPDATE users SET password_reset_token = $2, + password_reset_expiry = NOW() + interval '1 hour' WHERE id = $1`, user.ID, hashSessionToken(token)) + require.NoError(t, err) + require.NoError(t, svc.ConfirmPasswordReset(t.Context(), PasswordResetConfirm{Token: token, NewPassword: rotationNewPassword})) + }, +} + +func TestIntegration_SessionRejectedAfterPasswordRotation(t *testing.T) { + store := NewPostgresStore(setupAuthTestDB(t)) + svc := newRotationService(store) + ctx := t.Context() + change := ChangePasswordRequest{CurrentPassword: rotationPassword, NewPassword: rotationNewPassword} + expiry := time.Now().Add(time.Hour) + perms := []Permission{{Action: ActionView, Resource: ResourceRecommendations}} + + for name, rotate := range rotatePassword { + t.Run("invalidation-fails/"+name, func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-invalidation-"+name+"@example.com") + session := loginRotationUser(t, svc, user.Email, rotationPassword) + apiKey, _, err := svc.CreateAPIKey(ctx, user.ID, "ci", rotationPassword, perms, &expiry) + require.NoError(t, err) + rotate(t, newRotationService(failingInvalidationStore{failingKeyListStore{store}}), store, user) + + _, err = svc.ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated) + _, _, err = svc.ValidateUserAPIKey(ctx, apiKey) + require.ErrorIs(t, err, ErrAPIKeyPasswordRotated, "already enforced by migration 000103 (issue #402)") + + fresh := loginRotationUser(t, svc, user.Email, rotationNewPassword) + _, err = svc.ValidateSession(ctx, fresh) + require.NoError(t, err, "a session issued after the change must stay valid") + }) + } + + t.Run("login-across-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-login-race@example.com") + racing := &credentialReadBarrier{StoreInterface: store, afterRead: func(ctx context.Context, _ *User) { + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + }} + session := loginRotationUser(t, newRotationService(racing), user.Email, rotationPassword) + + _, err := svc.ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated, + "the login verified the password it read before the rotation, after the session sweep ran") + }) + + t.Run("session-use-across-rotation", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-use-race@example.com") + session := loginRotationUser(t, svc, user.Email, rotationPassword) + racing := &sessionReadHook{PostgresStore: store, onRead: func() { + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + }} + + _, err := newRotationService(racing).ValidateSession(ctx, session) + require.ErrorIs(t, err, ErrSessionPasswordRotated) + }) + + t.Run("concurrent-logins", func(t *testing.T) { + user := createRotationUser(t, svc, store, "session-concurrent-logins@example.com") + var ( + mu sync.Mutex + tokens []string + wg sync.WaitGroup + ) + started := make(chan struct{}) + var once sync.Once + // Each client logs in with the old password until the rotation makes + // it fail, so some logins straddle the rotation's commit and sweep. + for range 16 { + wg.Go(func() { + for range 1000 { + resp, err := svc.Login(ctx, LoginRequest{Email: user.Email, Password: rotationPassword}) + if err != nil { + return + } + mu.Lock() + tokens = append(tokens, resp.Token) + mu.Unlock() + once.Do(func() { close(started) }) + } + }) + } + <-started + require.NoError(t, svc.ChangePassword(ctx, user.ID, change)) + wg.Wait() + + // A session minted before the sweep is deleted; one minted after it + // survives the sweep and must be rejected by its password version. + for _, token := range tokens { + _, err := svc.ValidateSession(ctx, token) + require.Error(t, err, "every session from the old password must be rejected") + } + }) +} diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index 052ac8131..9f8909aa2 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -641,8 +641,8 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err query := ` INSERT INTO sessions ( token, user_id, email, expires_at, created_at, - user_agent, ip_address, csrf_token - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + user_agent, ip_address, csrf_token, password_version + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) ` _, err := s.db.Exec(ctx, query, @@ -654,6 +654,7 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err session.UserAgent, session.IPAddress, session.CSRFToken, + session.PasswordVersion, ) if err != nil { @@ -667,7 +668,7 @@ func (s *PostgresStore) CreateSession(ctx context.Context, session *Session) err func (s *PostgresStore) GetSession(ctx context.Context, token string) (*Session, error) { query := ` SELECT token, user_id, email, expires_at, created_at, - user_agent, ip_address, csrf_token + user_agent, ip_address, csrf_token, password_version FROM sessions WHERE token = $1 AND expires_at > NOW() ` @@ -682,6 +683,7 @@ func (s *PostgresStore) GetSession(ctx context.Context, token string) (*Session, &session.UserAgent, &session.IPAddress, &session.CSRFToken, + &session.PasswordVersion, ) if err != nil { diff --git a/internal/auth/types.go b/internal/auth/types.go index 898465075..9c8f60863 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -259,6 +259,9 @@ type Session struct { UserAgent string `json:"user_agent,omitempty" dynamodbav:"UserAgent"` IPAddress string `json:"ip_address,omitempty" dynamodbav:"IPAddress"` CSRFToken string `json:"csrf_token,omitempty" dynamodbav:"CSRFToken"` + // PasswordVersion is the user's User.PasswordVersion read by the login that + // issued the session; ValidateSession rejects the session once they differ. + PasswordVersion int64 `json:"-" dynamodbav:"PasswordVersion"` } // LoginRequest represents a login attempt. diff --git a/internal/database/postgres/migrations/000138_session_password_version.down.sql b/internal/database/postgres/migrations/000138_session_password_version.down.sql new file mode 100644 index 000000000..ac15b71d1 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version.down.sql @@ -0,0 +1,3 @@ +-- Reverse 000138: sessions are no longer bound to the password version, and a +-- password change again relies on deleting them (issue #398). +ALTER TABLE sessions DROP COLUMN IF EXISTS password_version; diff --git a/internal/database/postgres/migrations/000138_session_password_version.up.sql b/internal/database/postgres/migrations/000138_session_password_version.up.sql new file mode 100644 index 000000000..54c83cd58 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version.up.sql @@ -0,0 +1,19 @@ +-- Migration 000138: bind sessions to the user's password (issue #398). +-- +-- A password change deletes the user's sessions in a separate, best-effort +-- step after the new hash commits. A session survived that change when the +-- delete failed, or when a login that had verified the old password inserted +-- its session after the delete ran. A session now records the +-- users.password_version (migration 000103) its login read, and +-- ValidateSession rejects it once the two differ, so the trigger's bump in the +-- password write itself revokes every earlier session. +-- +-- Existing sessions take their owner's current version, so the deploy does not +-- sign everyone out. Any that outlived a failed delete before this migration +-- stay valid until they expire (24 hours by default). +ALTER TABLE sessions ADD COLUMN password_version BIGINT NOT NULL DEFAULT 0; + +UPDATE sessions s +SET password_version = u.password_version +FROM users u +WHERE u.id = s.user_id; diff --git a/internal/database/postgres/migrations/000138_session_password_version_test.go b/internal/database/postgres/migrations/000138_session_password_version_test.go new file mode 100644 index 000000000..8586c1068 --- /dev/null +++ b/internal/database/postgres/migrations/000138_session_password_version_test.go @@ -0,0 +1,58 @@ +//go:build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// TestMigration_SessionPasswordVersion covers 000138 up, the backfill and down (#398). +func TestMigration_SessionPasswordVersion(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { require.NoError(t, container.Cleanup(context.Background())) }) + pool, path := container.DB.Pool(), getMigrationsPath() + + hasColumn := func() bool { + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM information_schema.columns + WHERE table_name = 'sessions' AND column_name = 'password_version'`).Scan(&n)) + return n == 1 + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 138))) + require.False(t, hasColumn()) + + var userID string + require.NoError(t, pool.QueryRow(ctx, `INSERT INTO users (email, password_hash, salt, group_ids) + VALUES ('session-version@example.com', 'hash-1', '', ARRAY[$1::uuid]) RETURNING id`, + adminGroupIDForMinAdminTest).Scan(&userID)) + _, err := pool.Exec(ctx, `UPDATE users SET password_hash = 'hash-2' WHERE id = $1`, userID) + require.NoError(t, err) + _, err = pool.Exec(ctx, `UPDATE users SET password_hash = 'hash-3' WHERE id = $1`, userID) + require.NoError(t, err) + _, err = pool.Exec(ctx, `INSERT INTO sessions (token, user_id, email, expires_at) + VALUES ('existing-session', $1, 'session-version@example.com', NOW() + interval '1 hour')`, userID) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 138)) + var version int64 + require.NoError(t, pool.QueryRow(ctx, `SELECT password_version FROM sessions WHERE token = 'existing-session'`).Scan(&version)) + require.Equal(t, int64(2), version, "an existing session takes its owner's current version") + + _, err = pool.Exec(ctx, `INSERT INTO sessions (token, user_id, email, expires_at) + VALUES ('writer-without-version', $1, 'session-version@example.com', NOW() + interval '1 hour')`, userID) + require.NoError(t, err) + require.NoError(t, pool.QueryRow(ctx, `SELECT password_version FROM sessions WHERE token = 'writer-without-version'`).Scan(&version)) + require.Zero(t, version, "a writer that omits the version gets 0, which fails closed once the user has rotated") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 138))) + require.False(t, hasColumn()) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 138)) + require.True(t, hasColumn()) +}