fix(auth): reject sessions issued before the user's last password change

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")
 )

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),

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 {

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)
+		})
+	}
+}

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) {

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")
+		}
+	})
+}

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 {

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.

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;

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;