Repository navigation
Conversation
…A write guards Follow-ups from the LeanerCloud#567 review: - (d) When EnableMFA's guarded write is refused, re-check the row on the database clock: if MFA is still off under the same pending secret and its expiry has passed, return ErrMFAEnrollmentExpired (400) instead of ErrUserChanged (409). Every other refusal, including the losers of concurrent enables, stays ErrUserChanged. - (a) Pin each conjunct of the DisableMFA and ReplaceMFARecoveryCodes guards on its own, and add the zero-recovery-codes stale regenerate-after-disable subtest. - (c) Say why MFADisable clones the recovery codes before spending one, and pin it with a unit test that spends a non-last code. - (e) Pin that DisableMFA clears the pending secret and its expiry. - (f) Bound LoginLockoutExpiry's timestamps on the database clock (clock_timestamp()) instead of the test process clock. Closes LeanerCloud#568 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This works through the follow-ups listed in the #567 auth review. Each item was checked again against current
main(a14dc8f). One item, (d), needed a code fix. The others needed a comment or a test that pins existing behavior, and (f) needed a test fix.mfa_enabled/mfa_secretguards inDisableMFAandReplaceMFARecoveryCodeseach survive being dropped aloneTestIntegration_MFAWriteGuardsArePinnedIndividuallyputs the row in a state where exactly one ofmfa_enabled,mfa_secretormfa_recovery_codesdiffers from the caller's read, for both writes. The write must be refused and the row left untouched. The issue's suggested subteststale-regenerate-after-disable-with-zero-recovery-codesis added too. Both guards are kept rather than dropping one, because they are cheap and each pins a separate invariant.slices.CloneinMFADisableneeds a commentconsumeRecoveryCodestill said "persist via UpdateUser".consumeRecoveryCode. A new unit test,TestMFADisable_WithRecoveryCodePassesCodesAsRead, spends a code that is not the last one. The existing unit test used a single code, and with one code the in-place shift is invisible.EnableMFArefused by the SQL expiry check returnsErrUserChanged(409) instead of "enrollment expired" (400)TestIntegration_MFAEnableExpiryEnforcedInWriteassertedErrUserChanged.DisableMFAclearsmfa_pending_secretandmfa_pending_secret_expires_atdisable-clears-every-mfa-column.TestIntegration_LoginLockoutExpirycompares DB timestamps against Go-clock boundsNo item was deferred. I checked every in-flight auth branch for overlap with the lines this PR edits (
internal/auth/store_postgres_credentials.goEnableMFA,service_mfa.goaroundconsumeRecoveryCodeandMFADisable,service_mfa_cas_db_test.go,service_login_db_test.goTestIntegration_LoginLockoutExpiry,service_mfa_test.go). For the PRs I usedgh pr diff; for the branches,git diff origin/main...fork/<branch>.fix/issue-64:store_postgres_db_test.goand a new authTestMain. I don't add aTestMainand don't touch that file.fix/issue-169,-310,-312,-313,-419: none of them touch these files or lines.-312editsinternal/auth/errors.go, which this PR leaves alone.-313editsstore_postgres_pgxmock_test.go, so the new pgxmock tests go in a separate file.fix/issue-559: no such branch exists on the fork (git fetch fork fix/issue-559says "couldn't find remote ref").Root cause and fix
(d) The
EnableMFAwrite rejects for three different reasons: MFA already on, the pending secret was replaced or cleared, or the pending secret has expired. All three collapsed intoErrUserChanged. The service checks expiry with the Go clock before the write, so the SQL expiry check only fires at the boundary. When it does, the user got 409 "account changed concurrently" when the real problem was an expired enrollment that needs a fresh setup.When the write affects no rows,
EnableMFAnow runs a read-only follow-up on the database clock (enableMFARefusal):truemaps toErrMFAEnrollmentExpired, which the API already maps to 400. No row, or not expired, maps toErrUserChanged. If the check itself errors, the result is stillErrUserChanged, with the cause wrapped. The write itself is unchanged, so this cannot accept anything that was refused before. It only changes which error a refused call reports.Why the separate read is race-safe. The read can only report "expired" if, after the refused write, the row still has MFA off, the same pending secret, and a passed expiry. Pending secrets are random. They leave the row only by being cleared (enable, disable, cancel) or replaced (a new setup). A row that still holds this secret with MFA off therefore held it at write time too. The other two refusal reasons are ruled out, which leaves the expiry.
NOW()does not go backwards between the two statements, so an expiry that refused the write is still past at the read. Concurrent enables of one live enrollment: the winner flipsmfa_enabled, so every loser's read finds no row and getsErrUserChanged. A real-Postgres race test covers this: 8 racers × 5 rounds, exactly one winner, every loser getsErrUserChangedand none gets "expired".(f)
last_login_atandlocked_untilare stamped with the server'sNOW(), but the test bounded them withtime.Now()from the test process. Two clocks within a few ms of each other can disagree. The test now readsSELECT clock_timestamp()before and afterLogin. The stampedNOW()comes from a transaction that starts between those two statements, on the same clock and at the same microsecond precision, so the inclusive bounds always hold. There is no tolerance window left to tune.Verification
All runs used
export PATH=$HOME/.local/go/bin:$HOME/go/bin:$HOME/.local/bin:$PATH AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=trueon Linux, with Postgres 16 testcontainers.Static checks and unit tests:
go build ./...,go vet ./...,go vet -tags integration ./...: cleango test ./internal/auth/ -count=1: okgo test ./internal/mocks/ -count=1: okgo test ./internal/api/ -count=1 -run 'MFA|Mfa': okgolangci-lint run --allow-parallel-runners ./internal/auth/and the same with--build-tags integration: 0 issuesgocyclo -over 10 -ignore "_test\.go" internal/auth/store_postgres_credentials.go internal/auth/service_mfa.go: nothing over 10Fail-before/pass-after. "Before" means the
origin/mainstore_postgres_credentials.goswapped into this tree; "mutation" means one SQL clause removed by hand.go test ./internal/auth/ -count=1 -run TestPGXMock_EnableMFA -vTestPGXMock_EnableMFA_RefusalReasonfails in all 4 subtests.go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_MFAEnableExpiryEnforcedInWrite|TestIntegration_EnableMFARefusalReason' ./internal/auth/TestIntegration_MFAEnableExpiryEnforcedInWritefails (expected: "MFA enrollment expired"), and so doesTestIntegration_EnableMFARefusalReason/expired. The other refusal cases and the concurrent-enable race already passed before, so the fix does not change them.mfa_enabled = falsefailsTestIntegration_EnableMFARefusalReason/mfa-on-with-expired-pending.mfa_pending_secret = $2fails.../replaced-by-expired-setup.TestIntegration_MFAWriteGuardsArePinnedIndividually. Each mutation fails exactly the matching subtest:mfa_enabled→disable-refused-on-mfa-enabled-falsemfa_secret→disable-refused-on-mfa-secret-replacedmfa_recovery_codes→disable-refused-on-mfa-recovery-codes-changedregenerate-refused-on-*subtestsmfa_pending_secret = '', or withoutmfa_pending_secret_expires_at = NULL→disable-clears-every-mfa-columnmfa_recovery_codesalone →TestIntegration_MFADisableAndRegenerateAreColumnScoped/stale-regenerate-after-disable-with-zero-recovery-codesslices.CloneinMFADisablemakesTestMFADisable_WithRecoveryCodePassesCodesAsReadfail (unit). The existingTestMFADisable_WithRecoveryCodestill passes under that mutation, because it uses a single code.TestIntegration_LoginLockoutExpirypasses.go test -tags integration -count=1 -timeout 20m -run 'TestIntegration_MFAEnableExpiryEnforcedInWrite|TestIntegration_EnableMFARefusalReason|TestIntegration_MFAWriteGuardsArePinnedIndividually|TestIntegration_MFADisableAndRegenerateAreColumnScoped|TestIntegration_LoginLockoutExpiry' ./internal/auth/: ok.TestIntegration_EnrollmentCodeIsSpent, which also exercisesEnableMFA, passes as well. One run of it failed before any test code ran, because the container did not start ("wait until ready ... context deadline exceeded") while the machine was loaded. It passed on rerun.Whole integration packages,
-raceand git-secrets were not run here. They run on the combined tree below.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope / notes
mfa_enabled = falsewith the secret and codes still set. Normal flows don't produce those states, because disable clears all three columns together. That is exactly why a single guard could not be isolated through the service API. The realistic zero-codes scenario is covered separately at the service level.MFAEnablelosers of a concurrent enable could arguably getErrMFAAlreadyEnabledinstead ofErrUserChanged. Both are 409, and this PR leaves that as it is.Closes #568
Maintainers: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/s.🤖 Generated with Claude Code