fix(auth): scope profile, password and reset writes to their own columns

[cristim]

What

UpdateUserProfile, ChangePassword and RequestPasswordReset read a whole user row, changed a few fields, then wrote the entire row back through UpdateUser (WHERE id only). If MFA enrollment committed between the read and the write, the write put back mfa_enabled=false and the old MFA fields. The three schedules in the issue (profile update, password change, public reset request) are all reachable.

How

• PostgresStore.UpdateUserCredentials(user, readEmail, readPasswordHash) writes only email, password_hash, salt, password_history and updated_at, and only WHERE id AND email = readEmail AND password_hash = readPasswordHash. Profile updates (email-only or with a new password) and ChangePassword both use this one writer. Including the hash in the predicate also covers two concurrent password changes, so the history is never computed from a stale hash.
• PostgresStore.SetPasswordResetToken(user, readExpiry) writes only the token, its expiry and updated_at, WHERE id AND email AND deactivated_at IS NULL AND password_reset_expiry IS NOT DISTINCT FROM readExpiry. A concurrent deactivation, email change or second reset issuance therefore wins.
• When a write loses the race the store returns auth.ErrUserChanged. The profile and password handlers map it to 409. Sessions are not revoked and the password-change hook does not run. A reset request that loses the race returns the same uniform success and sends no mail.
• No migration and no new columns: the predicates compare columns that already exist.

Verification

Tests run against a real PostgreSQL 16 in the repo's testcontainers harness (setupAuthTestDB), with synthetic accounts and a recording mail sink. internal/auth/service_credentials_db_test.go uses a one-shot read barrier: it pauses each service call after its user read, completes the competing action through a second, real auth.Service, resumes the call, and then reads fresh SQL state.

• MFA scenarios (profile email-only, profile email+password, change password, reset request with failing delivery): the victim's MFA secret and recovery hashes survive. Every other column is unchanged apart from the ones the operation owns. Login without the factor returns ErrMFARequired, and login with the victim's TOTP succeeds. On the password paths the old password is rejected, the history holds the previous hash and the existing sessions are revoked. A later reissue over an expired token still succeeds, which covers the expiry round-trip.
• Stale-credential scenarios: a concurrent password change or email change makes the stale write fail with ErrUserChanged, the winner's row is untouched and the winner's session stays valid. A concurrent deactivation or reset makes the losing reset request store nothing and send no mail.

Before the fix. These tests ran against the parent service code, which writes the whole row through UpdateUser. All 8 subtests failed:

--- FAIL: .../profile-email-only                       stale write erased the concurrent MFA enrollment
--- FAIL: .../profile-email-and-password               stale write erased the concurrent MFA enrollment
--- FAIL: .../change-password                          stale write erased the concurrent MFA enrollment
--- FAIL: .../reset-request-with-failing-delivery      stale write erased the concurrent MFA enrollment
--- FAIL: .../change-password-after-concurrent-change  Expected error with "account changed concurrently; reload and retry" in chain but got nil.
--- FAIL: .../profile-after-concurrent-email-change    Expected error with "account changed concurrently; reload and retry" in chain but got nil.
--- FAIL: .../reset-after-concurrent-deactivation      Should be empty, but was [stale-reset-deactivated@example.com]
--- FAIL: .../reset-after-concurrent-reset             Should be empty, but was [stale-reset-twice@example.com]
FAIL	github.com/LeanerCloud/cloud-commitments-platform/internal/auth

After the fix:

--- PASS: TestIntegration_CredentialWritesPreserveConcurrentMFA (profile-email-only, profile-email-and-password, change-password, reset-request-with-failing-delivery)
--- PASS: TestIntegration_CredentialWritesRejectStaleCredentials (change-password-after-concurrent-change, profile-after-concurrent-email-change, reset-after-concurrent-deactivation, reset-after-concurrent-reset)
ok  	github.com/LeanerCloud/cloud-commitments-platform/internal/auth

TestHandler_credentialWritesMapConcurrentChangeToConflict checks the 409 mapping. It fails when the handler change is reverted.

Commands run on macOS (GOTOOLCHAIN=go1.26.6 GOWORK=off GOFLAGS='-p=2 -count=1' AWS_EC2_METADATA_DISABLED=true):

• go build ./..., go vet ./... and go vet -tags integration on auth, api, server and mocks
• go test on ./internal/auth/... ./internal/api/... ./internal/server/... ./internal/mocks/...
• go test -tags integration ./internal/auth/ (the full auth integration suite)
• golangci-lint run --build-tags integration on the touched packages, plus the pre-commit hooks, including gocyclo

The tests drive the service directly. The HTTP handlers forward to these service calls unchanged, and the real auth.Service reaches the API only through internal/server's adapter, so no API-package test uses it. The handler-level evidence is therefore the mock-based 409 test.

Scope notes for reviewers

• Refs sec(auth): MFA can be replaced with a session and password alone #227. This PR does not change the MFA lifecycle writes (MFASetup, MFAEnable, MFADisable, regenerate). On main, MFASetup on an account that already has MFA enabled is still accepted, so the issue's "attacker setup is denied" assertion depends on sec(auth): MFA can be replaced with a session and password alone #227. These tests do not assert it. sec(auth): MFA can be replaced with a session and password alone #227 should stay open.
• Other writers still persist a stale whole-row snapshot through UpdateUser, outside this issue's three paths: ConfirmPasswordReset (needs a valid reset token), admin UpdateUser (service_user.go), and recovery-code consumption at login (service.go). They may need the same treatment in a follow-up.
• ChangePassword errors other than ErrUserChanged still go to the 500 path, as before.

Closes #474

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 16 billable files and costs up to $4.00.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 55 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 4acff979-c296-48a4-b226-44a2e25bba12

📥 Commits

Reviewing files that changed from the base of the PR and between a11cacc and ec7ddd6.

📒 Files selected for processing (16)

• internal/api/handler_auth.go
• internal/api/handler_auth_test.go
• internal/auth/errors.go
• internal/auth/interfaces.go
• internal/auth/service_api_test.go
• internal/auth/service_credentials_db_test.go
• internal/auth/service_password.go
• internal/auth/service_password_callback_test.go
• internal/auth/service_password_test.go
• internal/auth/service_test.go
• internal/auth/service_user.go
• internal/auth/service_user_test.go
• internal/auth/store_postgres_credentials.go
• internal/auth/test_helpers.go
• internal/mocks/stores.go
• internal/server/health_test.go

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai full review

[coderabbitai]

⚠️ Action not completed

Review rate limited.