Skip to content

fix(auth): make user authority revocation atomic - #800

Open
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-atomic-user-revocation
Open

fix(auth): make user authority revocation atomic#800
Quick104 wants to merge 1 commit into
mainfrom
codex/fix-atomic-user-revocation

Conversation

@Quick104

Copy link
Copy Markdown
Contributor

Problem

Related issue: N/A — audit-validated security fix

Sensitive user changes updated users before separately revoking native and Jellyfin sessions. A database failure or concurrent credential grant could therefore leave a role downgrade, disable, or password reset committed while an older access token, refresh token, impersonation session, device-login grant, or Jellyfin credential remained usable. Jellyfin's per-process cache also let another replica continue serving a cached session after local invalidation.

Approach

Add a monotonic users.auth_revision authority epoch and snapshot it into native sessions. Native middleware, refresh, impersonation, and Jellyfin session validation now compare the snapshot with current subject and impersonator state.

Sensitive admin mutations run the user update, native owner/impersonator revocation, and Jellyfin persistence cleanup in one database transaction. Credential creation that can cross the middleware boundary—API keys, impersonation sessions, and device-login polling—revalidates the non-impersonated authorizing session while locking the same user rows, so it is ordered with an authority reset.

Jellyfin persistence now binds each compat session to its native session. Cache hits validate shared database state before use, and guarded upserts cannot make a revoked session usable again on another replica.

The migration expires existing Jellyfin sessions once. Their encrypted native tokens do not expose enough information for SQL to backfill the native session IDs safely.

Validation

Repository gate on the complete diff:

  • make embed-stub — passed
  • go build ./... — passed
  • gofmt -l . — passed with no output
  • go vet ./... — passed
  • golangci-lint run --new-from-merge-base="origin/main" ./... with the CI-pinned v2.12.2 binary — passed, 0 issues
  • make test-go on the exact Linux candidate source — passed
  • pnpm install --frozen-lockfile — passed
  • pnpm run lint — passed with existing warnings only
  • pnpm run format:check — passed
  • pnpm run build — passed with existing font/chunk-size warnings only
  • make test-web — passed: 294 files, 2,184 tests
  • make migrate-validate — passed
  • make verify-settings-bindings-all — passed
  • make verify-playback-fixtures — passed
  • make verify-local-paths — passed

Security regressions on isolated PostgreSQL, including -race, passed for role downgrade, disable, password reset, mutation/revocation rollback injection, stale session insertion, device-login grants, API-key creation after middleware authorization, impersonation creation, and two independent Jellyfin replica pools.

The exact candidate was built and deployed in the isolated audit-session-revoke sandbox. doctor passed container, database, API, and frontend checks. A synthetic-user HTTP probe verified:

  • ordinary native, device-login, API-key, impersonation, and Jellyfin authentication succeeds before revocation;
  • role downgrade rejects old native access/refresh, impersonation, and Jellyfin credentials;
  • disable rejects old native access/refresh and Jellyfin credentials;
  • password reset rejects old native access/refresh, Jellyfin, device-login grants, and API-key creation, while the replacement password and new credentials work.

The sandbox and its temporary probe were removed after validation.

On macOS, make test-go still fails the two inherited internal/jellycompat process-lock identity tests because the implementation cannot read a Linux-style process start token there. Those files are unchanged. Both tests and the complete suite pass on the exact Linux candidate used above.

Risks

  • Applying the migration logs out all existing Jellyfin clients once. Rolling the migration back removes the new columns but cannot restore those expired sessions.
  • Jellyfin cache hits now perform a shared database authority check and fail closed during database errors. This adds one query per authenticated compat request.
  • Authority resets and durable credential creation briefly serialize on user rows. The locks cover only the credential transaction.
  • Existing approved-but-unconsumed device-login requests lack an authorizing session binding and must be restarted after migration.
  • There is no native v1 response-shape change and no Apple or Android client change is required. Jellyfin clients remain protocol-compatible but must reauthenticate once after migration.

AI Disclosure

  • Tool(s): OpenAI Codex desktop; Modern Go Guidelines CLI
  • Model(s): GPT-5
  • Involvement: AI-assisted
  • Adversarial review: Two fresh read-only reviewers independently traced native access/refresh, login sessions, impersonation, API keys, device login, Jellyfin persistence, and replica caches. The boundary review confirmed the original atomicity failure. The bypass/regression review found three remaining races in the first candidate—approved device grants, API-key creation after middleware authorization, and impersonation creation after source-session revocation. The final patch binds or serializes all three against the same authority epoch and adds focused regressions for each.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 23 minutes.

View limit details

Limit details: You’ve used the included review currently available. Your 94 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.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a38bfdec-da22-4696-bff6-ba92138338c2

📥 Commits

Reviewing files that changed from the base of the PR and between 8164fd5 and 2054632.

📒 Files selected for processing (20)
  • internal/api/handlers/admin.go
  • internal/api/handlers/admin_scoped_api_key_test.go
  • internal/api/handlers/api_keys.go
  • internal/api/handlers/api_keys_test.go
  • internal/api/handlers/auth_device.go
  • internal/auth/api_key_repository.go
  • internal/auth/device_login.go
  • internal/auth/device_login_test.go
  • internal/auth/repository.go
  • internal/auth/service.go
  • internal/auth/session.go
  • internal/auth/user_authority_test.go
  • internal/jellycompat/login.go
  • internal/jellycompat/session_repository.go
  • internal/jellycompat/session_repository_authority_test.go
  • internal/jellycompat/session_store.go
  • internal/jellycompat/session_store_test.go
  • internal/models/session.go
  • internal/models/user.go
  • migrations/sql/20260827201847_atomic_user_authority_revision.sql

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2054632d21

ℹ️ 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".

Comment thread internal/auth/session.go
AND (NOT $3 OR authorizer.role = 'admin')
AND subject.enabled
AND subject.auth_revision = $5
FOR UPDATE OF authorizer, subject

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Lock the source session while minting delegated credentials

If Logout or RevokeSession updates the source session after this query reads it but before the creation transaction commits, that revocation can commit because source is not one of the locked rows; the transaction can then still create an API key, device-login session, or impersonation session from authority that was already revoked. Those derived credentials remain independently valid, so include the source session in the FOR UPDATE lock, with consistent lock ordering, to serialize revocation and credential creation.

Useful? React with 👍 / 👎.

Comment on lines +108 to +109
if claims.TokenType == auth.TokenTypeAPIKey || claims.SessionID == "" || claims.ImpersonatorUserID != nil {
writeError(w, http.StatusForbidden, "forbidden", "API key management requires an ordinary native session")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Update the API-key contract for ordinary sessions

When an impersonated access JWT, or another JWT without a native session_id, calls an API-key management endpoint, this new condition returns 403; however, docs/api-keys-api.md lines 45–48 still state that a JWT access token is sufficient and identify only API-key authentication as rejected. Update that API document with the ordinary, non-impersonated session requirement and its failure response so third-party clients do not implement against an inaccurate contract.

AGENTS.md reference: AGENTS.md:L127-L135

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant