diff --git a/internal/auth/interfaces.go b/internal/auth/interfaces.go index b2171df1..2b859b91 100644 --- a/internal/auth/interfaces.go +++ b/internal/auth/interfaces.go @@ -11,7 +11,6 @@ type StoreInterface interface { GetUserByID(ctx context.Context, userID string) (*User, error) GetUserByEmail(ctx context.Context, email string) (*User, error) CreateUser(ctx context.Context, user *User) error - UpdateUser(ctx context.Context, user *User) error UpdateUserAdminFields(ctx context.Context, user *User, readEmail string, readGroupIDs []string, readActive bool) error UpdateUserCredentials(ctx context.Context, user *User, readEmail, readPasswordHash string) error SetPasswordResetToken(ctx context.Context, user *User, readExpiry *time.Time) error diff --git a/internal/auth/membership_ceiling_test.go b/internal/auth/membership_ceiling_test.go index 80541384..8d16d965 100644 --- a/internal/auth/membership_ceiling_test.go +++ b/internal/auth/membership_ceiling_test.go @@ -8,7 +8,7 @@ package auth // newCeilingService) are shared with the permission-ceiling tests; see // group_ceiling_fixtures_test.go. // -// Every refusal case stubs NO CreateUser / UpdateUser expectation on the +// Every refusal case stubs NO CreateUser / UpdateUserAdminFields expectation on the // mock store, so a removed guard fails the test loudly (testify panics on an // un-stubbed call) rather than passing silently. diff --git a/internal/auth/self_account_scope_test.go b/internal/auth/self_account_scope_test.go index 7b22a544..94d1bc04 100644 --- a/internal/auth/self_account_scope_test.go +++ b/internal/auth/self_account_scope_test.go @@ -63,7 +63,7 @@ func acctAPlanAuthorsGroup() *Group { // stubScopedActor wires the actor's user row and the groups the change touches. // -// Refusal cases add a permissive UpdateUser stub on purpose: without one, a +// Refusal cases add a permissive UpdateUserAdminFields stub on purpose: without one, a // guard removal kills the test by PANICKING on an unstubbed write, and that // kill evaporates the moment someone adds a stub while tidying fixtures. With // it, the removal is caught by an assertion instead. diff --git a/internal/auth/self_escalation_carveout_test.go b/internal/auth/self_escalation_carveout_test.go index 328093bd..78309242 100644 --- a/internal/auth/self_escalation_carveout_test.go +++ b/internal/auth/self_escalation_carveout_test.go @@ -17,7 +17,7 @@ import ( // admin:* grants, so it passed. Closing only the group-permission write path // would have left this open and #1550 would have auto-closed over it. // -// As elsewhere in this package, refusal cases stub NO UpdateUser expectation: +// As elsewhere in this package, refusal cases stub NO UpdateUserAdminFields expectation: // testify panics if the write lands, so the assertions cannot be vacuous. const ( diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index 0b735218..6a118f30 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -198,7 +198,7 @@ func TestService_UpdateUserAPI(t *testing.T) { // toast would fire, but the users.email column was never touched. We // assert that (a) the email-uniqueness lookup happens (proves the // admin path runs the same validation as self-edit), and (b) the - // user persisted via store.UpdateUser carries the new email value, + // user persisted via store.UpdateUserAdminFields carries the new email value, // NOT just that the call succeeded. t.Run("successful email update persists to store", func(t *testing.T) { mockStore := new(MockStore) @@ -214,7 +214,7 @@ func TestService_UpdateUserAPI(t *testing.T) { mockStore.On("GetUserByID", ctx, "user-123").Return(existingUser, nil).Once() // Uniqueness check: new email not yet in use. mockStore.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil).Once() - // Capture the User passed to UpdateUser and assert its email is + // Capture the User passed to UpdateUserAdminFields and assert its email is // the NEW one. Without the fix, this fails because the User would // still carry old@example.com. mockStore.On("UpdateUserAdminFields", ctx, mock.MatchedBy(func(u *User) bool { @@ -268,7 +268,7 @@ func TestService_UpdateUserAPI(t *testing.T) { // Regression for issue #89: before this fix, APIUpdateUserRequest had no // Active field at all, so a PUT /api/users/{id} body could never // deactivate a user -- the same field-silently-dropped defect class - // issue #892 fixed for Email. We assert the User passed to store.UpdateUser + // issue #892 fixed for Email. We assert the User passed to store.UpdateUserAdminFields // carries Active=false, not just that the call succeeds, so the test // would have failed on the pre-fix struct (which drops the field on the // floor and always persists whatever GetUserByID returned, i.e. true). diff --git a/internal/auth/service_credentials_db_test.go b/internal/auth/service_credentials_db_test.go index fd9987c3..8c7bee1b 100644 --- a/internal/auth/service_credentials_db_test.go +++ b/internal/auth/service_credentials_db_test.go @@ -5,6 +5,7 @@ package auth import ( "context" "errors" + "slices" "testing" "time" @@ -129,6 +130,17 @@ func (f *credentialRaceFixture) stored() *User { return u } +// adminEdit applies edit to the stored row through UpdateUserAdminFields, the +// column-scoped writer an admin's email or active change goes through, so the +// competing write is the one production would make. +func (f *credentialRaceFixture) adminEdit(ctx context.Context, edit func(u *User)) { + u, err := f.store.GetUserByID(ctx, f.user.ID) + require.NoError(f.t, err) + readEmail, readGroupIDs, readActive := u.Email, slices.Clone(u.GroupIDs), u.Active + edit(u) + require.NoError(f.t, f.store.UpdateUserAdminFields(ctx, u, readEmail, readGroupIDs, readActive)) +} + // requireFactorEnforced proves the victim's factor survived: login without it // is refused and login with it succeeds. func (f *credentialRaceFixture) requireFactorEnforced(email, password string, enrolled *User) { @@ -244,10 +256,7 @@ func TestIntegration_CredentialWritesRejectStaleCredentials(t *testing.T) { t.Run("profile-after-concurrent-email-change", func(t *testing.T) { f := newCredentialRaceFixture(t, store, "stale-profile@example.com") winner := f.onRead(func(ctx context.Context, _ *Service) { - u, err := store.GetUserByID(ctx, f.user.ID) - require.NoError(t, err) - u.Email = "stale-profile-admin@example.com" - require.NoError(t, store.UpdateUser(ctx, u)) + f.adminEdit(ctx, func(u *User) { u.Email = "stale-profile-admin@example.com" }) }) err := f.svc.UpdateUserProfile(ctx, f.user.ID, "stale-profile-self@example.com", credentialRacePassword, credentialRaceNew) require.ErrorIs(t, err, ErrUserChanged) @@ -259,11 +268,10 @@ func TestIntegration_CredentialWritesRejectStaleCredentials(t *testing.T) { t.Run("reset-after-concurrent-deactivation", func(t *testing.T) { f := newCredentialRaceFixture(t, store, "stale-reset-deactivated@example.com") winner := f.onRead(func(ctx context.Context, _ *Service) { - u, err := store.GetUserByID(ctx, f.user.ID) - require.NoError(t, err) - now := time.Now() - u.Active, u.DeactivatedAt = false, &now - require.NoError(t, store.UpdateUser(ctx, u)) + f.adminEdit(ctx, func(u *User) { + now := time.Now() + u.Active, u.DeactivatedAt = false, &now + }) }) require.NoError(t, f.svc.RequestPasswordReset(ctx, f.user.Email)) assert.Empty(t, f.mail.resets) @@ -273,10 +281,7 @@ func TestIntegration_CredentialWritesRejectStaleCredentials(t *testing.T) { t.Run("reset-after-concurrent-email-change", func(t *testing.T) { f := newCredentialRaceFixture(t, store, "stale-reset-email@example.com") winner := f.onRead(func(ctx context.Context, _ *Service) { - u, err := store.GetUserByID(ctx, f.user.ID) - require.NoError(t, err) - u.Email = "stale-reset-email-new@example.com" - require.NoError(t, store.UpdateUser(ctx, u)) + f.adminEdit(ctx, func(u *User) { u.Email = "stale-reset-email-new@example.com" }) }) require.NoError(t, f.svc.RequestPasswordReset(ctx, f.user.Email)) assert.Empty(t, f.mail.resets, "a token must not be mailed to the address the user just left") @@ -401,11 +406,10 @@ func TestIntegration_ResetConfirmRejectsStaleRead(t *testing.T) { f := newCredentialRaceFixture(t, store, "reset-after-deactivation@example.com") token := f.issueResetToken() winner := f.onRead(func(ctx context.Context, _ *Service) { - u, err := store.GetUserByID(ctx, f.user.ID) - require.NoError(t, err) - now := time.Now() - u.Active, u.DeactivatedAt = false, &now - require.NoError(t, store.UpdateUser(ctx, u)) + f.adminEdit(ctx, func(u *User) { + now := time.Now() + u.Active, u.DeactivatedAt = false, &now + }) }) err := f.svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew}) require.ErrorIs(t, err, ErrUserChanged) diff --git a/internal/auth/service_group_only_authz_test.go b/internal/auth/service_group_only_authz_test.go index 081155d6..0b093927 100644 --- a/internal/auth/service_group_only_authz_test.go +++ b/internal/auth/service_group_only_authz_test.go @@ -248,7 +248,7 @@ func TestUpdateUser_SelfEscalationDenied(t *testing.T) { mockStore.On("GetGroup", ctx, viewerGroup().ID).Return(viewerGroup(), nil) // Permissive write stub, deliberately: without it, removing the // self-escalation guard kills this test by panicking on an unstubbed - // UpdateUser -- a kill that disappears the moment someone adds a stub + // UpdateUserAdminFields -- a kill that disappears the moment someone adds a stub // while tidying fixtures. With it, the removal is caught by the // AssertNotCalled below, which is the actual security property. mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() diff --git a/internal/auth/service_lockout_test.go b/internal/auth/service_lockout_test.go index 5874ee92..d01adea7 100644 --- a/internal/auth/service_lockout_test.go +++ b/internal/auth/service_lockout_test.go @@ -61,7 +61,6 @@ func TestLogin_AccountLockout_Bookkeeping(t *testing.T) { assert.EqualError(t, err, genericLoginError) } } - store.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) store.AssertExpectations(t) }) } @@ -109,7 +108,6 @@ func TestLogin_BookkeepingErrors(t *testing.T) { assert.EqualError(t, err, genericLoginError) } } - store.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) store.AssertExpectations(t) }) } diff --git a/internal/auth/service_login_db_test.go b/internal/auth/service_login_db_test.go index 7c93cb0b..7b224476 100644 --- a/internal/auth/service_login_db_test.go +++ b/internal/auth/service_login_db_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/jackc/pgx/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -26,6 +27,37 @@ func (s *loginReadBarrierStore) GetUserByEmail(ctx context.Context, email string return u, err } +// overwriteSecurityColumns stands in for every security change that can land +// while a login is in flight (credentials, MFA, reset token, password history, +// membership and active state), all in one statement. No store writer covers +// them all: the whole-row UpdateUser that did was removed in #583. It writes +// group_ids and active, so like the store's own writers it takes the +// admin-invariant lock first (see adminInvariantAdvisoryLockKey). +func overwriteSecurityColumns(ctx context.Context, store *PostgresStore, u *User) error { + return store.withAdminInvariantLock(ctx, "overwrite security columns", func(tx pgx.Tx) error { + tag, err := tx.Exec(ctx, ` + UPDATE users SET + email = $2, password_hash = $3, salt = $4, group_ids = $5, + active = $6, deactivated_at = $7, mfa_enabled = $8, mfa_secret = $9, + mfa_pending_secret = $10, mfa_pending_secret_expires_at = $11, + mfa_recovery_codes = $12, password_reset_token = $13, + password_reset_expiry = $14, password_history = $15, updated_at = NOW() + WHERE id = $1`, + u.ID, u.Email, u.PasswordHash, u.Salt, u.GroupIDs, + u.Active, u.DeactivatedAt, u.MFAEnabled, u.MFASecret, + u.MFAPendingSecret, u.MFAPendingSecretExpiresAt, + u.MFARecoveryCodes, u.PasswordResetToken, + u.PasswordResetExpiry, u.PasswordHistory) + if err != nil { + return err + } + if tag.RowsAffected() != 1 { + return fmt.Errorf("overwrite security columns: %d rows for user %s", tag.RowsAffected(), u.ID) + } + return nil + }) +} + func TestIntegration_LoginPreservesSecurityChanges(t *testing.T) { store := NewPostgresStore(setupAuthTestDB(t)) ctx := t.Context() @@ -55,7 +87,7 @@ func TestIntegration_LoginPreservesSecurityChanges(t *testing.T) { current.Active, current.DeactivatedAt = false, &stamp current.PasswordResetToken, current.PasswordResetExpiry = "new-reset-token", &stamp current.PasswordHistory = []string{"new-history"} - if updateErr := store.UpdateUser(ctx, current); updateErr != nil { + if updateErr := overwriteSecurityColumns(ctx, store, current); updateErr != nil { return updateErr } changed, readErr = store.GetUserByID(ctx, current.ID) diff --git a/internal/auth/service_mfa_test.go b/internal/auth/service_mfa_test.go index 8414f87e..72de6f4f 100644 --- a/internal/auth/service_mfa_test.go +++ b/internal/auth/service_mfa_test.go @@ -236,7 +236,7 @@ func TestMFASetup_WrongPassword(t *testing.T) { _, err := service.MFASetup(ctx, user.ID, "WrongPassword!@#") require.Error(t, err) assert.Contains(t, err.Error(), "invalid password") - mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "SetPendingMFASecret", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestMFAEnable_HappyPath(t *testing.T) { @@ -392,7 +392,7 @@ func TestMFADisable_WrongPassword(t *testing.T) { err := service.MFADisable(ctx, user.ID, "wrong", "000000") require.Error(t, err) assert.Contains(t, err.Error(), "invalid password") - mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "DisableMFA", mock.Anything, mock.Anything, mock.Anything, mock.Anything) } func TestMFADisable_IdempotentWhenAlreadyDisabled(t *testing.T) { diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index 4aee4866..a8b437a3 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -290,7 +290,7 @@ func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangeP // Invalidate sessions and API keys only after the new password is // persisted (non-critical, best-effort -- issue #102). Invalidating - // first would mean a failed UpdateUser leaves the caller with revoked + // first would mean a failed UpdateUserCredentials leaves the caller with revoked // credentials for a password that never actually changed (CodeRabbit // finding on #392). s.invalidateUserCredentialsBestEffort(ctx, userID, "password change") diff --git a/internal/auth/service_password_callback_test.go b/internal/auth/service_password_callback_test.go index 3fa5d329..cb49061f 100644 --- a/internal/auth/service_password_callback_test.go +++ b/internal/auth/service_password_callback_test.go @@ -84,10 +84,10 @@ func TestService_OnPasswordChange_ChangePassword(t *testing.T) { testUser := createTestUser(t, "OldSecure123!") - // Invalidation now runs only after UpdateUser succeeds (issue #102 + // Invalidation now runs only after UpdateUserCredentials succeeds (issue #102 // CodeRabbit finding on #392: invalidating first would revoke // credentials for a password that never actually changed), so a - // failed UpdateUser must never reach DeleteUserSessions/ListAPIKeysByUser. + // failed UpdateUserCredentials must never reach DeleteUserSessions/ListAPIKeysByUser. mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once() mockStore.On("UpdateUserCredentials", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything).Return(assert.AnError).Once() diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index b10c1b44..3657c4a4 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -334,7 +334,7 @@ func TestService_RequestPasswordReset(t *testing.T) { // Regression test for issue #89 / audit finding A03-006: an admin- // deactivated account must not receive a working reset link. It must be // refused with the exact same no-error, no-email response as "email - // does not exist" -- neither UpdateUser (which would mint a fresh reset + // does not exist" -- neither SetPasswordResetToken (which would mint a fresh reset // token) nor SendPasswordResetEmail may be called, or the response shape // would let a caller distinguish deactivated from missing. t.Run("deactivated account gets no reset link, same response as missing email", func(t *testing.T) { @@ -353,7 +353,7 @@ func TestService_RequestPasswordReset(t *testing.T) { } mockStore.On("GetUserByEmail", ctx, "deactivated@example.com").Return(deactivatedUser, nil).Once() - // UpdateUser and SendPasswordResetEmail must NOT be called. + // SetPasswordResetToken and SendPasswordResetEmail must NOT be called. err := service.RequestPasswordReset(ctx, "deactivated@example.com") require.NoError(t, err, "deactivated-account reset request must not error (enumeration protection)") @@ -362,7 +362,7 @@ func TestService_RequestPasswordReset(t *testing.T) { // TestService_RequestPasswordReset_RateLimit is the regression test for 03-M5. // An active token issued within PasswordResetRateLimit must be preserved: the -// second call must return nil without calling UpdateUser or sending an email. +// second call must return nil without calling SetPasswordResetToken or sending an email. func TestService_RequestPasswordReset_RateLimit(t *testing.T) { ctx := context.Background() mockStore := new(MockStore) @@ -382,11 +382,11 @@ func TestService_RequestPasswordReset_RateLimit(t *testing.T) { } mockStore.On("GetUserByEmail", ctx, "ratelimit@example.com").Return(userWithFreshToken, nil).Once() - // UpdateUser and SendPasswordResetEmail must NOT be called. + // SetPasswordResetToken and SendPasswordResetEmail must NOT be called. err := service.RequestPasswordReset(ctx, "ratelimit@example.com") require.NoError(t, err, "rate-limited reset must not return an error (enumeration protection)") - // Verify by asserting no UpdateUser or email calls happened (handled by AssertExpectations). + // Verify by asserting no SetPasswordResetToken or email calls happened (handled by AssertExpectations). } func TestService_ConfirmPasswordReset(t *testing.T) { diff --git a/internal/auth/service_security_test.go b/internal/auth/service_security_test.go index 2d12041d..242c4853 100644 --- a/internal/auth/service_security_test.go +++ b/internal/auth/service_security_test.go @@ -114,8 +114,8 @@ func TestMFARegenerateRecoveryCodes_EmptyCode(t *testing.T) { _, err := service.MFARegenerateRecoveryCodes(ctx, user.ID, "") require.Error(t, err, "MFARegenerateRecoveryCodes with empty code must return an error") - // UpdateUser must NOT have been called. - mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) + // The codes must NOT have been replaced. + mockStore.AssertNotCalled(t, "ReplaceMFARecoveryCodes", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) // The pre-existing codes must be untouched. assert.Equal(t, []string{"$2a$04$preexistingstub"}, user.MFARecoveryCodes, diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index d2e1fe6c..a4d285a5 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -459,7 +459,7 @@ func TestService_DeleteUser_ConcurrentLastTwoAdmins(t *testing.T) { // simultaneously deactivate the last two active admins. Both read count == 2 // and pass the soft check. The 000065 deferred trigger now counts only *active* // members and serializes via an advisory xact lock, so exactly one commit is -// rejected. We simulate the DB outcome by having the second UpdateUser return +// rejected. We simulate the DB outcome by having the second UpdateUserAdminFields return // the trigger violation, and assert the invariant: never zero active admins. func TestService_UpdateUser_ConcurrentDeactivateLastTwoAdmins(t *testing.T) { ctx := context.Background() @@ -475,7 +475,7 @@ func TestService_UpdateUser_ConcurrentDeactivateLastTwoAdmins(t *testing.T) { mockStore.On("GetUserByID", ctx, "admin-b").Return(adminB, nil).Once() mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Twice() - // Session revocation now happens only after store.UpdateUser succeeds, + // Session revocation now happens only after store.UpdateUserAdminFields succeeds, // so only admin-a (whose commit succeeds below) gets DeleteUserSessions; // admin-b hits the deferred trigger and must not be logged out for a // deactivation that never actually took effect. @@ -900,7 +900,7 @@ func TestService_UpdateUser(t *testing.T) { // Soft check passes: two admins visible at read time. mockStore.On("GetUserByID", ctx, "admin-1").Return(adminUser, nil).Once() mockStore.On("CountGroupMembers", ctx, DefaultAdminGroupID).Return(2, nil).Once() - // UpdateUser hits the deferred trigger at commit time. + // UpdateUserAdminFields hits the deferred trigger at commit time. triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one member of the Administrators group must remain") mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(triggerErr).Once() @@ -987,7 +987,7 @@ func TestService_UpdateUser(t *testing.T) { triggerErr := fmt.Errorf("last_admin_constraint_violation: at least one active member of the Administrators group must remain") mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(triggerErr).Once() // DeleteUserSessions must NOT be called: session revocation now - // happens only after store.UpdateUser succeeds, so a request that + // happens only after store.UpdateUserAdminFields succeeds, so a request that // hits the deferred trigger must not log the user out for a change // that never actually took effect. diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index c1984b31..c71cd318 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -186,72 +186,6 @@ func isLastAdminConstraintViolation(err error) bool { return strings.HasPrefix(err.Error(), sentinel) } -// UpdateUser updates an existing user. It rewrites group_ids and active, so it -// takes the admin-invariant lock first (see adminInvariantAdvisoryLockKey). -func (s *PostgresStore) UpdateUser(ctx context.Context, user *User) error { - user.UpdatedAt = time.Now() - - query := ` - UPDATE users SET - email = $2, - password_hash = $3, - salt = $4, - group_ids = $5, - active = $6, - mfa_enabled = $7, - mfa_secret = $8, - mfa_pending_secret = $9, - mfa_pending_secret_expires_at = $10, - mfa_recovery_codes = $11, - password_reset_token = $12, - password_reset_expiry = $13, - failed_login_attempts = $14, - locked_until = $15, - password_history = $16, - updated_at = $17, - last_login_at = $18, - deactivated_at = $19 - WHERE id = $1 - ` - - // See CreateUser for the nil-slice substitution rationale. - recoveryCodes := user.MFARecoveryCodes - if recoveryCodes == nil { - recoveryCodes = []string{} - } - - return s.withAdminInvariantLock(ctx, "update user", func(tx pgx.Tx) error { - result, err := tx.Exec(ctx, query, - user.ID, - user.Email, - user.PasswordHash, - user.Salt, - user.GroupIDs, - user.Active, - user.MFAEnabled, - user.MFASecret, - user.MFAPendingSecret, - user.MFAPendingSecretExpiresAt, - recoveryCodes, - user.PasswordResetToken, - user.PasswordResetExpiry, - user.FailedLoginAttempts, - user.LockedUntil, - user.PasswordHistory, - user.UpdatedAt, - user.LastLoginAt, - user.DeactivatedAt, - ) - if err != nil { - return fmt.Errorf("failed to update user: %w", err) - } - if result.RowsAffected() == 0 { - return fmt.Errorf("user not found: %s", user.ID) - } - return nil - }) -} - // DeleteUser deletes a user. Deleting an Administrators-group member lowers // the admin count, so it takes the admin-invariant lock first (see // adminInvariantAdvisoryLockKey). diff --git a/internal/auth/store_postgres_admin_lock_db_test.go b/internal/auth/store_postgres_admin_lock_db_test.go index 547b1eca..cbcfd782 100644 --- a/internal/auth/store_postgres_admin_lock_db_test.go +++ b/internal/auth/store_postgres_admin_lock_db_test.go @@ -46,11 +46,6 @@ func adminDemotingWrites() []adminDemotingWrite { next.GroupIDs = []string{DefaultPurchaserGroupID} return store.UpdateUserAdminFields(ctx, &next, target.Email, target.GroupIDs, target.Active) }}, - {"UpdateUser deactivate", func(ctx context.Context, store *PostgresStore, target *User) error { - next := *target - next.Active = false - return store.UpdateUser(ctx, &next) - }}, {"DeleteUser", func(ctx context.Context, store *PostgresStore, target *User) error { return store.DeleteUser(ctx, target.ID) }}, diff --git a/internal/auth/store_postgres_admin_lock_test.go b/internal/auth/store_postgres_admin_lock_test.go index 4ab953f6..bfa53c5b 100644 --- a/internal/auth/store_postgres_admin_lock_test.go +++ b/internal/auth/store_postgres_admin_lock_test.go @@ -9,6 +9,7 @@ package auth import ( "context" "errors" + "reflect" "testing" "time" @@ -19,8 +20,6 @@ import ( ) const ( - updateUserSQLRegex = `(?s)UPDATE users SET\s+email = \$2,.*deactivated_at = \$19\s+WHERE id = \$1` - updateUserAdminFieldsSQLRegex = `(?s)UPDATE users SET email = \$2, group_ids = \$3, active = \$4, ` + `deactivated_at = \$5, updated_at = NOW\(\)\s+` + `WHERE id = \$1 AND email = \$6 AND group_ids = \$7 AND active = \$8\s+RETURNING updated_at` @@ -74,17 +73,6 @@ func adminLockedWrites() []adminLockedWrite { assert.Contains(t, err.Error(), "user not found: "+lockedWriteUserID) } return []adminLockedWrite{ - { - name: "UpdateUser", - expect: func(mock pgxmock.PgxPoolIface, outcome writeOutcome) { - args := append([]any{lockedWriteUserID}, anyArgs(18)...) - expectLockedExec(mock, updateUserSQLRegex, "UPDATE", args, outcome) - }, - call: func(ctx context.Context, store *PostgresStore) error { - return store.UpdateUser(ctx, &User{ID: lockedWriteUserID, GroupIDs: []string{DefaultPurchaserGroupID}}) - }, - assertNoRow: assertUserNotFound, - }, { name: "UpdateUserAdminFields", expect: func(mock pgxmock.PgxPoolIface, outcome writeOutcome) { @@ -224,6 +212,22 @@ func TestPGXMock_AdminWrites_CommitTriggerViolationStaysDetectable(t *testing.T) } } +// TestStore_HasNoWholeRowUserWriter pins issue #583: the whole-row +// UpdateUser(ctx, *User) is gone from the store, so adminLockedWrites above +// lists every store write that can lower the active-admin count. A new caller +// has to pick a column-scoped writer, and cannot bring back a stale whole-row +// save that overwrites MFA, password or membership changes made since its +// read. +func TestStore_HasNoWholeRowUserWriter(t *testing.T) { + for _, typ := range []reflect.Type{ + reflect.TypeFor[StoreInterface](), + reflect.TypeFor[*PostgresStore](), + } { + _, found := typ.MethodByName("UpdateUser") + assert.False(t, found, "%s still has the whole-row UpdateUser", typ) + } +} + func TestPGXMock_UpdateUserAdminFields_ReturnsStoredUpdatedAt(t *testing.T) { mock := newAuthPgxMock(t) stored := time.Unix(1700000000, 0) diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index dcd7b6a0..abaecae1 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -45,11 +45,6 @@ func (m *MockStore) CreateUser(ctx context.Context, user *User) error { return args.Error(0) } -func (m *MockStore) UpdateUser(ctx context.Context, user *User) error { - args := m.Called(ctx, user) - return args.Error(0) -} - func (m *MockStore) UpdateUserAdminFields(ctx context.Context, user *User, readEmail string, readGroupIDs []string, readActive bool) error { return m.Called(ctx, user, readEmail, readGroupIDs, readActive).Error(0) } diff --git a/internal/mocks/stores.go b/internal/mocks/stores.go index 757b1b01..dd98ce19 100644 --- a/internal/mocks/stores.go +++ b/internal/mocks/stores.go @@ -1147,12 +1147,6 @@ func (m *MockAuthStore) CreateUser(ctx context.Context, user *auth.User) error { return args.Error(0) } -// UpdateUser mocks the UpdateUser operation. -func (m *MockAuthStore) UpdateUser(ctx context.Context, user *auth.User) error { - args := m.Called(ctx, user) - return args.Error(0) -} - func (m *MockAuthStore) UpdateUserAdminFields(ctx context.Context, user *auth.User, readEmail string, readGroupIDs []string, readActive bool) error { return m.Called(ctx, user, readEmail, readGroupIDs, readActive).Error(0) } diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index 043e3e7d..60fccb34 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -363,7 +363,7 @@ func TestAuthServiceAdapter_UpdateUserAPI(t *testing.T) { // (issue #907 self-escalation guard). Actor != target, so the // self-escalation guard skips; the group change does not remove the // Administrators group, so the last-admin guard skips; the store - // UpdateUser call must therefore fire. Without that, the test silently + // UpdateUserAdminFields call must therefore fire. Without that, the test silently // passes even if the adapter stopped forwarding actorUserID, which is // exactly the regression CR flagged. _, err := adapter.UpdateUserAPI(ctx, "actor-1", "user-1", auth.APIUpdateUserRequest{ diff --git a/internal/server/health_test.go b/internal/server/health_test.go index 0a1823ed..d11a8689 100644 --- a/internal/server/health_test.go +++ b/internal/server/health_test.go @@ -28,10 +28,6 @@ func (m *mockAuthStoreForHealth) CreateUser(ctx context.Context, user *auth.User return nil } -func (m *mockAuthStoreForHealth) UpdateUser(ctx context.Context, user *auth.User) error { - return nil -} - func (m *mockAuthStoreForHealth) UpdateUserAdminFields(context.Context, *auth.User, string, []string, bool) error { return nil }