chore(auth): drop role-model leftovers, route user lookups through loadUser, correct post-fetch compare comments

internal/auth/interfaces.go

@@ -65,7 +65,6 @@ type StoreInterface interface {
 	GetAPIKeyByHash(ctx context.Context, keyHash string) (*UserAPIKey, error)
 	ListAPIKeysByUser(ctx context.Context, userID string) ([]*UserAPIKey, error)
 	UpdateAPIKey(ctx context.Context, key *UserAPIKey) error
-	UpdateAPIKeyLastUsed(ctx context.Context, keyID string) error
 	// RecordAPIKeyUsage atomically updates last_used_at and adds delta to
 	// request_count_total + request_count_window. delta is the number of
 	// requests in this flush (greater than 1 when concurrent requests were
@@ -85,6 +84,5 @@ type StoreInterface interface {
 // EmailSenderInterface defines the methods required for sending emails.
 type EmailSenderInterface interface {
 	SendPasswordResetEmail(ctx context.Context, email, resetURL string) error
-	SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error
 	SendUserInviteEmail(ctx context.Context, email, setupURL string) error
 }

internal/auth/service.go

@@ -325,9 +325,12 @@ func (s *Service) ValidateSession(ctx context.Context, token string) (*Session,
 		return nil, fmt.Errorf("session not found")
 	}
 
-	// Constant-time comparison after fetch to close the SQL-equality timing oracle.
-	// GetSession does `WHERE token = $1` which is not constant-time in PostgreSQL;
-	// align with ValidateUserAPIKey / validateResetToken (issue #392 PR #837).
+	// Store-integrity check, not a timing defense (issue #313). GetSession
+	// selected this row by `WHERE token = $1` on hashedToken, so the two are
+	// equal unless the store returned a row for a different token; refuse it
+	// if it ever does. The equality timing of that lookup is harmless: the
+	// value compared is the SHA-256 of a 256-bit random token, which an
+	// attacker can neither guess nor steer byte by byte.
 	if subtle.ConstantTimeCompare([]byte(session.Token), []byte(hashedToken)) != 1 {
 		return nil, fmt.Errorf("session not found")
 	}

internal/auth/service_apikeys.go

@@ -45,35 +45,14 @@ const (
 	MaxAPIKeyLifetime = 365 * 24 * time.Hour
 )
 
-// loadActiveUserForAPIKey resolves userID and ensures the account exists and
-// is active. Pulled out of CreateAPIKey to keep its cyclomatic complexity
-// under the project's gocyclo gate as new checks (e.g. issue #61's empty-
-// permissions rejection, issue #102's password/expiry validation) are added.
-func (s *Service) loadActiveUserForAPIKey(ctx context.Context, userID string) (*User, error) {
-	user, err := s.store.GetUserByID(ctx, userID)
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user not found")
-		}
-		return nil, fmt.Errorf("failed to get user: %w", err)
-	}
-	if user == nil {
-		return nil, fmt.Errorf("user not found")
-	}
-	if !user.Active {
-		return nil, fmt.Errorf("user account is not active")
-	}
-	return user, nil
-}
-
 // CreateAPIKey creates a new user API key with scoped permissions. The
 // caller's current password must be supplied and is re-verified against the
 // stored hash before minting a key: a stolen session token alone must not be
 // enough to create a durable, potentially wildcard-scoped credential, the
 // same defense-in-depth MFASetup/MFADisable already apply (issue #102).
 // Returns the full API key (shown only once), key info, and error.
 func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password string, permissions []Permission, expiresAt *time.Time) (string, *UserAPIKey, error) {
-	user, err := s.loadActiveUserForAPIKey(ctx, userID)
+	user, err := s.loadActiveUser(ctx, userID)
 	if err != nil {
 		return "", nil, err
 	}
@@ -213,16 +192,9 @@ func (s *Service) validateAPIKeyPermissions(ctx context.Context, user *User, per
 // already rejects it, and the row only stays active when the best-effort
 // revocation scan after the password change failed (issue #534).
 func (s *Service) ListUserAPIKeys(ctx context.Context, userID string) ([]*UserAPIKey, error) {
-	// Validate user exists
-	user, err := s.store.GetUserByID(ctx, userID)
+	user, err := s.loadUser(ctx, userID)
 	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user not found")
-		}
-		return nil, fmt.Errorf("failed to get user: %w", err)
-	}
-	if user == nil {
-		return nil, fmt.Errorf("user not found")
+		return nil, err
 	}
 
 	keys, err := s.store.ListAPIKeysByUser(ctx, userID)
@@ -286,15 +258,8 @@ func (s *Service) authorizeAPIKeyAccess(ctx context.Context, userID, keyID, acti
 		return nil, fmt.Errorf("API key not found")
 	}
 
-	user, err := s.store.GetUserByID(ctx, userID)
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user not found")
-		}
-		return nil, fmt.Errorf("failed to get user: %w", err)
-	}
-	if user == nil {
-		return nil, fmt.Errorf("user not found")
+	if _, err := s.loadUser(ctx, userID); err != nil {
+		return nil, err
 	}
 
 	if key.UserID != userID {
@@ -382,24 +347,6 @@ func validateAPIKeyStatus(key *UserAPIKey) error {
 	return nil
 }
 
-// lookupAPIKeyUser retrieves and validates the user associated with an API key.
-func (s *Service) lookupAPIKeyUser(ctx context.Context, userID string) (*User, error) {
-	user, err := s.store.GetUserByID(ctx, userID)
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user account not found")
-		}
-		return nil, fmt.Errorf("failed to get user: %w", err)
-	}
-	if user == nil {
-		return nil, fmt.Errorf("user account not found")
-	}
-	if !user.Active {
-		return nil, fmt.Errorf("user account is not active")
-	}
-	return user, nil
-}
-
 // ValidateUserAPIKey validates an API key and returns the key info and associated user.
 //
 // Validation is deliberately side-effect free: it books no usage. A single
@@ -422,9 +369,12 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA
 		return nil, nil, fmt.Errorf("invalid API key")
 	}
 
-	// Constant-time comparison after fetch to close the SQL-equality timing oracle.
-	// The DB lookup already found the row by hash; this redundant check ensures the
-	// hot path does not leak timing information through early return differences.
+	// Store-integrity check, not a timing defense (issue #313). The store
+	// selected this row by `WHERE key_hash = $1` on keyHash, so the two are
+	// equal unless it returned a row for a different key; refuse it if it ever
+	// does. The equality timing of that lookup is harmless: the value compared
+	// is the SHA-256 of a 256-bit random key, which an attacker can neither
+	// guess nor steer byte by byte.
 	if subtle.ConstantTimeCompare([]byte(key.KeyHash), []byte(keyHash)) != 1 {
 		return nil, nil, fmt.Errorf("invalid API key")
 	}
@@ -434,7 +384,7 @@ func (s *Service) ValidateUserAPIKey(ctx context.Context, apiKey string) (*UserA
 		return nil, nil, err
 	}
 
-	user, err := s.lookupAPIKeyUser(ctx, key.UserID)
+	user, err := s.loadActiveUser(ctx, key.UserID)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -564,13 +514,6 @@ func (s *Service) peekPendingUsage(keyID string) int64 {
 	return counter.Load()
 }
 
-// UpdateLastUsed updates only the last used timestamp for an API key.
-// Retained for backwards compatibility; new code paths should use
-// RecordUsage so the request_count_* counters stay current.
-func (s *Service) UpdateLastUsed(ctx context.Context, keyID string) error {
-	return s.store.UpdateAPIKeyLastUsed(ctx, keyID)
-}
-
 // RecordUsage updates last_used_at and adds delta to both the lifetime
 // counter and the fixed-window request counter for the key. delta is the
 // number of requests being recorded, which is greater than 1 whenever

internal/auth/service_apikeys_api.go

@@ -61,9 +61,9 @@ func effectiveWindowUsage(key *UserAPIKey, now time.Time) (int64, *time.Time) {
 // predates the counter. Reporting that as 0 would assert a request volume we
 // do not have; nil says "unknown" and lets the UI show it as such.
 //
-// This relies on last_used_at and request_count_total moving together. Any new
-// caller of the legacy UpdateAPIKeyLastUsed (which bumps only the timestamp)
-// would break the invariant and make a genuinely-unused key look unknown.
+// This relies on last_used_at and request_count_total moving together. A new
+// writer that bumps only the timestamp (as the removed UpdateAPIKeyLastUsed
+// did) would break the invariant and make a genuinely-unused key look unknown.
 func effectiveLifetimeUsage(key *UserAPIKey) *int64 {
 	if key.RequestCountTotal == 0 && key.LastUsedAt != nil {
 		return nil
@@ -424,8 +424,8 @@ func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyI
 	if key == nil {
 		return false, fmt.Errorf("API key not found")
 	}
-	// lookupAPIKeyUser also verifies that the user is still active.
-	user, err := s.lookupAPIKeyUser(ctx, userID)
+	// loadActiveUser also verifies that the user is still active.
+	user, err := s.loadActiveUser(ctx, userID)
 	if err != nil {
 		return false, err
 	}
@@ -477,7 +477,7 @@ func (s *Service) GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, u
 	if err != nil {
 		return nil, err
 	}
-	user, err := s.lookupAPIKeyUser(ctx, userID)
+	user, err := s.loadActiveUser(ctx, userID)
 	if err != nil {
 		return nil, err
 	}

internal/auth/service_apikeys_test.go

@@ -873,8 +873,8 @@ func TestService_ValidateUserAPIKey(t *testing.T) {
 	})
 
 	t.Run("reject when stored hash does not match derived hash", func(t *testing.T) {
-		// Simulates a DB row whose key_hash field was tampered or
-		// belongs to a different key -- the constant-time check must reject it.
+		// Simulates a store returning a row whose key_hash belongs to a
+		// different key -- the post-fetch store-integrity check must reject it.
 		mockStore := new(MockStore)
 		service := &Service{store: mockStore}
 
@@ -901,34 +901,6 @@ func TestService_ValidateUserAPIKey(t *testing.T) {
 	})
 }
 
-func TestService_UpdateLastUsed(t *testing.T) {
-	ctx := context.Background()
-
-	t.Run("successfully update last used", func(t *testing.T) {
-		mockStore := new(MockStore)
-		service := &Service{store: mockStore}
-
-		mockStore.On("UpdateAPIKeyLastUsed", ctx, "key-1").Return(nil)
-
-		err := service.UpdateLastUsed(ctx, "key-1")
-
-		require.NoError(t, err)
-		mockStore.AssertExpectations(t)
-	})
-
-	t.Run("return error when update fails", func(t *testing.T) {
-		mockStore := new(MockStore)
-		service := &Service{store: mockStore}
-
-		mockStore.On("UpdateAPIKeyLastUsed", ctx, "key-1").Return(assert.AnError)
-
-		err := service.UpdateLastUsed(ctx, "key-1")
-
-		assert.Error(t, err)
-		mockStore.AssertExpectations(t)
-	})
-}
-
 func TestService_RecordUsage(t *testing.T) {
 	ctx := context.Background()
 
@@ -1308,7 +1280,7 @@ func TestService_HasAPIKeyPermissionForConstraintsAPI_OwnerCapEnforced(t *testin
 		mockStore := new(MockStore)
 		t.Cleanup(func() { mockStore.AssertExpectations(t) })
 		mockStore.On("GetAPIKeyByID", ctx, keyID).Return(key, nil)
-		// lookupAPIKeyUser + GetAuthContext both call GetUserByID.
+		// loadActiveUser + GetAuthContext both call GetUserByID.
 		mockStore.On("GetUserByID", ctx, userID).Return(owner, nil)
 		mockStore.On("GetGroup", ctx, grpID).Return(ownerGroup, nil)
 		return mockStore

internal/auth/service_credentials_db_test.go

@@ -65,9 +65,6 @@ func (m *recordingMailSink) SendPasswordResetEmail(_ context.Context, email, _ s
 	m.resets = append(m.resets, email)
 	return m.err
 }
-func (m *recordingMailSink) SendWelcomeEmail(context.Context, string, string, string) error {
-	return nil
-}
 func (m *recordingMailSink) SendUserInviteEmail(context.Context, string, string) error { return nil }
 
 type credentialRaceFixture struct {

internal/auth/service_group.go

@@ -73,16 +73,10 @@ func (s *Service) ListGroups(ctx context.Context) ([]Group, error) {
 // permission set. A nil group (the store returns nil, nil for a deleted/
 // missing group) is skipped without error.
 func (s *Service) GetUserPermissions(ctx context.Context, userID string) ([]Permission, error) {
-	user, err := s.store.GetUserByID(ctx, userID)
+	user, err := s.loadUser(ctx, userID)
 	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user not found")
-		}
 		return nil, err
 	}
-	if user == nil {
-		return nil, fmt.Errorf("user not found")
-	}
 
 	// Permissions come exclusively from group memberships.
 	return s.permissionsForGroups(ctx, user.GroupIDs)
@@ -171,16 +165,10 @@ func (s *Service) accountsForGroups(ctx context.Context, groupIDs []string) ([]s
 // user's group memberships; a user with no groups gets an empty context and
 // is denied everything (fail closed).
 func (s *Service) BuildAuthContext(ctx context.Context, userID string) (*AuthContext, error) {
-	user, err := s.store.GetUserByID(ctx, userID)
+	user, err := s.loadUser(ctx, userID)
 	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return nil, fmt.Errorf("user not found")
-		}
 		return nil, err
 	}
-	if user == nil {
-		return nil, fmt.Errorf("user not found")
-	}
 
 	authCtx := &AuthContext{
 		User:            user,

internal/auth/service_password.go

@@ -229,16 +229,10 @@ func (s *Service) invalidateUserCredentialsBestEffort(ctx context.Context, userI
 
 // ChangePassword allows a user to change their password.
 func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangePasswordRequest) error {
-	user, err := s.store.GetUserByID(ctx, userID)
+	user, err := s.loadUser(ctx, userID)
 	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return fmt.Errorf("user not found")
-		}
 		return err
 	}
-	if user == nil {
-		return fmt.Errorf("user not found")
-	}
 
 	// Verify current password
 	if !s.verifyPassword(req.CurrentPassword, user.PasswordHash) {
@@ -491,7 +485,10 @@ func (s *Service) ResetTokenStatus(ctx context.Context, token string) (ResetToke
 		return ResetTokenStateUsed, ResetTokenFlowReset, nil
 	}
 
-	// Constant-time comparison after fetch to close the SQL-equality timing oracle.
+	// Store-integrity check, not a timing defense (issue #313): the row was
+	// selected by `WHERE password_reset_token = $1` on tokenHash, so this only
+	// fails if the store returned a row for a different token. See the note
+	// in ValidateSession on why the lookup's equality timing is harmless.
 	if subtle.ConstantTimeCompare([]byte(user.PasswordResetToken), []byte(tokenHash)) != 1 {
 		return ResetTokenStateUsed, ResetTokenFlowReset, nil
 	}
@@ -524,7 +521,10 @@ func (s *Service) validateResetToken(ctx context.Context, token string) (*User,
 		return nil, ErrInvalidResetToken
 	}
 
-	// Constant-time comparison after fetch to close the SQL-equality timing oracle.
+	// Store-integrity check, not a timing defense (issue #313): the row was
+	// selected by `WHERE password_reset_token = $1` on tokenHash, so this only
+	// fails if the store returned a row for a different token. See the note
+	// in ValidateSession on why the lookup's equality timing is harmless.
 	if subtle.ConstantTimeCompare([]byte(user.PasswordResetToken), []byte(tokenHash)) != 1 {
 		return nil, ErrInvalidResetToken
 	}

internal/auth/service_password_test.go

@@ -629,10 +629,11 @@ func TestService_ConfirmPasswordReset(t *testing.T) {
 		mockStore.AssertExpectations(t)
 	})
 
-	t.Run("constant-time mismatch rejects token even when DB row exists", func(t *testing.T) {
-		// The DB returned a user but the stored reset-token hash does not match
-		// the hash we derived from the supplied token string. The constant-time
-		// guard must reject the request to avoid timing-based disclosure.
+	t.Run("stored hash mismatch rejects token even when DB row exists", func(t *testing.T) {
+		// The store returned a user whose reset-token hash does not match the
+		// hash we derived from the supplied token string, which a correct store
+		// never does (it selects by that hash). The post-fetch store-integrity
+		// check must still reject it (issue #313).
 		mockStore := new(MockStore)
 		mockEmail := new(MockEmailSender)
 		service := createTestService(mockStore, mockEmail)
@@ -894,11 +895,11 @@ func TestService_ResetTokenStatus(t *testing.T) {
 		mockStore.AssertExpectations(t)
 	})
 
-	t.Run("constant-time mismatch returns used even if DB row exists", func(t *testing.T) {
-		// The DB returned a row but the stored token hash does not match the
-		// hash derived from the supplied token. The constant-time check must
-		// treat this as a missing/consumed token rather than branching on
-		// expiry state, to avoid leaking information through timing.
+	t.Run("stored hash mismatch returns used even if DB row exists", func(t *testing.T) {
+		// The store returned a row whose token hash does not match the hash
+		// derived from the supplied token, which a correct store never does.
+		// The post-fetch store-integrity check must treat this as a
+		// missing/consumed token rather than reporting the row's expiry state.
 		mockStore := new(MockStore)
 		mockEmail := new(MockEmailSender)
 		service := createTestService(mockStore, mockEmail)

internal/auth/service_test.go

@@ -226,12 +226,11 @@ func TestService_ValidateSession(t *testing.T) {
 		mockStore.AssertExpectations(t)
 	})
 
-	t.Run("constant-time mismatch rejects session even when DB row exists", func(t *testing.T) {
-		// The DB returned a session row but its stored token hash does not
-		// match the hash we derived from the supplied token string. The
-		// constant-time guard (added alongside the API-key and reset-token
-		// equivalents in PR #837) must reject the request to avoid leaking
-		// timing information through the SQL equality oracle.
+	t.Run("stored hash mismatch rejects session even when DB row exists", func(t *testing.T) {
+		// The store returned a session row whose token hash does not match
+		// the hash we derived from the supplied token string, which a correct
+		// store never does (it selects by that hash). The post-fetch
+		// store-integrity check must still reject it (issue #313).
 		mockStore := new(MockStore)
 		mockEmail := new(MockEmailSender)
 		service := createTestService(mockStore, mockEmail)