fix(auth): serialize initial administrator setup - #798
Conversation
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 93 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e09fd14f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| select { | ||
| case <-release: | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
Give the query barrier an independent timeout
When SILO_TEST_DATABASE_URL is set but only one traced pool reaches the count query—for example, because the database has too few connection slots for the control pool plus both traced pools—that goroutine waits on release forever. The fallback cannot help because ctx comes from t.Context(), which is canceled only when the test is already finishing, while the test cannot finish because wg.Wait() is blocked on this goroutine. Use a timeout or cancellation source that can fire while the test is running so a connection failure does not stall the package until the global go test timeout.
Useful? React with 👍 / 👎.
Problem
Related issue: N/A — audit-validated security fix
POST /api/v1/auth/setupchecked whether any users existed and created the first administrator in separate autocommit operations. Two requests could both observe an emptyuserstable, then create different administrator accounts and login sessions.The finding was reproduced on fresh
origin/mainat8164fd594b9fdd8c1944bb0b6251f2d00e4a24ca: two synchronized setup requests both returned201, leaving two administrators and two sessions.Approach
Add a repository operation dedicated to first-user creation. It opens a PostgreSQL transaction, takes a
SHARE ROW EXCLUSIVElock onusers, rechecks that the table is empty, inserts through the same transaction, and commits. Competing setup requests and ordinary user writers therefore serialize on the database connection that owns the transaction. If the connection is lost, PostgreSQL rolls the transaction back and releases the lock.The normal account-provisioning path is unchanged. Initial setup reuses its profile creation and cleanup flow but selects the database-enforced first-user operation. No migration or API shape change is required.
The regression test uses two independent one-connection pools. A query tracer holds both setup attempts after their initial zero-user checks and releases them together. It asserts one token-pair success, one
ErrSetupAlreadyComplete, and exactly one user, administrator, and session.Validation
Focused and database-backed checks:
go test ./internal/auth ./internal/database/pglock— passgo test -race ./internal/auth ./internal/database/pglock— passgo vet ./internal/auth ./internal/database/pglock— passgo test ./internal/api/handlers -run '^TestAuthProviders' -count=1— passSILO_TEST_DATABASE_URL=<isolated-empty-database> go test ./internal/auth -run '^TestSetupInitialUserConcurrentAcrossPoolsDB$' -count=20— passSILO_TEST_DATABASE_URL=<isolated-empty-database> go test -race ./internal/auth -run '^TestSetupInitialUserConcurrentAcrossPoolsDB$' -count=5— passFull repository gate:
make embed-stub— passgo build ./...— passgofmt -l .— pass, no outputgo vet ./...— passgolangci-lint run --new-from-merge-base="origin/main" ./...— pass, 0 issues; it printed one warning about an unrelated stale worktree pathmake test-goon macOS — not passing because of repeatable failures in untouched packages:internal/httpstream/TestReadFromRollsDeadlineUnderProductionStepandinternal/jellycompat/TestBeginWebOperationRecoversDeadProcessLock/TestBeginWebOperationRejectsLiveProcessLockgo test ./...against the exact candidate source on the Linux dev-builder — pass, includinginternal/httpstreamandinternal/jellycompatpnpm install --frozen-lockfile— passpnpm run lint— pass with 167 inherited warnings and 0 errorspnpm run format:check— passpnpm run build— pass with existing asset/chunk-size warningsmake test-web— pass: 294 files, 2,184 testsmake verify-settings-bindings-all— passmake verify-playback-fixtures— passmake verify-local-paths— passIsolated dev-builder validation in
audit-bootstrap-admin:doctorpassed before and after validation201and produced one administrator, one user, and one session201and401 setup_completeRisks
The table lock briefly blocks writes to
userswhile first-user creation hashes the password and inserts the row. This happens only while the database has no users. Setup-status reads remain available. There is no migration, stored-data rewrite, client contract change, or Jellyfin compatibility change.Rollback is a revert of this commit; no schema or data rollback is needed.
AI Disclosure
Checklist