Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent first-use issuer provisioning can fail valid personal access token creation requests.
1 open issue
What changed in this PR
Moves personal access tokens onto a first-party OAuth client while preserving existing token strings, permissions, and non-expiry.
Changes:
- Provisions the issuer and backfills existing tokens.
- Supports both token forms while blocking OAuth grants for the issuer.
- Adds compatibility tests and updates conformance documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/models/oauth/client_test.rb | Tests issuer provisioning and protections. |
| test/migrations/personal_access_client_test.rb | Tests backfill, rollback, and plaintext preservation. |
| test/integration/personal_access_token_forms_test.rb | Tests compatibility of both token forms. |
| test/fixtures/oauth/clients.yml | Adds the personal-access issuer fixture. |
| docs/oauth/spec_conformance.md | Documents issuer behavior and compatibility. |
| db/schema.rb | Advances MySQL schema version. |
| db/schema_sqlite.rb | Advances SQLite schema version. |
| db/migrate/20261016120100_backfill_personal_access_client.rb | Assigns existing tokens to the issuer. |
| db/migrate/20261016120000_create_personal_access_client.rb | Provisions the issuer with rollback support. |
| db/migrate/20261015120100_backfill_token_digests.rb | Preserves client-backed personal tokens’ plaintext. |
| app/models/webhook.rb | Excludes personal-token issuer attribution. |
| app/models/oauth/client.rb | Defines and protects the first-party issuer. |
| app/models/identity/access_token.rb | Recognizes both personal-token forms. |
| app/controllers/users/push_subscriptions_controller.rb | Keeps personal subscriptions independent of apps. |
| app/controllers/oauth/tokens_controller.rb | Enforces allowed grant types. |
| app/controllers/oauth/revocations_controller.rb | Updates revocation comments only. |
| app/controllers/oauth/authorizations_controller.rb | Rejects authorization for the issuer. |
| app/controllers/my/access_tokens_controller.rb | Issues tokens through the first-party client. |
| app/controllers/concerns/authentication.rb | Distinguishes personal tokens from OAuth grants. |
Review effort: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b536a6e84
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🔵 Needs a closer look
Authentication changes and manual migrations against the shared production database require human verification of integration-branch deployment sequencing.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
|
Review threads: 2 resolved (1 fixed, 1 declined with the reasoning in the thread). |
1ef16f3 to
2bf7e87
Compare
f63f306 to
1b38841
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2bf7e87 to
dcb914c
Compare
1b38841 to
acc41d7
Compare
|
@codex review |
d646550 to
19ae6c0
Compare
19ae6c0 to
59d6546
Compare
… forms as personal A personal access token becomes a one-off access token of the fizzy-personal-access client, as bc3's are of bc-personal-access. The client is never self-registered, so the sweep passes it by, and it is listed for no grant type, so the authorization and token endpoints refuse it. Every rule that read "no client" as "personal" now reads either form: the personal and oauth scopes, honoring while OAuth is dark, idle lapse, expiry and refresh tokens, the token prefix and plaintext, revocation by possession, Connected Apps, the OAuth grant fence, and webhook and push attribution. Client-less tokens stay personal for production code that still writes them.
…o it The client is created by migration, so self-hosted installs that migrate get it too, and on first use where the schema was loaded instead. The backfill only sets oauth_client_id, in batches with update_all: token strings, digests, reach and non-expiry stay, updated_at doesn't move, and nothing is re-issued. It reruns over client-less tokens written since. Down returns the tokens to the client-less form before removing the client. The digest backfill, which is rerun for tokens production writes in plaintext only, now keeps the plaintext of client-backed personal tokens as well.
Losing the race at the uniqueness validation raised RecordInvalid, which create_or_find_by! doesn't recover, and losing it at the unique index retried a find on every attribute, JSON columns included. Both now find the issuer by client_id.
…it takes #3158 answers a read-only grant's write with 403 and keeps 401 for personal tokens, telling them apart by whether they have a client. A token of the personal access client has one, so it reads the token's connected app instead.
#3159's lock_client no longer reloads the row, so the guard reads the client_id in the database rather than one changed in memory but not saved.
…a heal A personal token's push subscription has no app, so it delivers while OAuth is dark, and outside the primary datacenter an undigested personal token of either form authenticates without being healed.
A request that looked the issuer up before down removed it could issue a token after its batch was cleared, leaving it naming a client that's gone, which every version reads as an OAuth grant. The issuer now goes first and the batches repeat until no token names it. Down is for after code that doesn't know the issuer is deployed.
…nce no code knows it Deleting the issuer first and repeating the batches only narrowed the window for a token issued alongside down: issuance takes no lock, and code that knows the issuer makes it again on first use. Down's precondition is what closes it. With that settled, clearing the tokens before removing the issuer makes an interrupted down safe to retry, since the issuer is still there to find.
59d6546 to
d9c68dc
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Restacked onto #3192 |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |

Warning
This PR contains migrations. Don't merge it directly. It has to go through the beta integration branch (
oauth-integration-2026-10-15) and be migrated by hand on beta before it lands. beta1 runs againstfizzy_production(saas/config/database.ymlendsbeta: *production), so migrating on beta1 is migrating production. The rows to add to that branch's migration inventory are below; this PR doesn't push to the integration branch.Stacked on #3192 (train: #2296 → #3080 → #3081 → #3082 → #3160 → #3161 → #3159 → #3163 → #3169 → #3192 → this).
Implements #3164 steps 1 to 3: a personal access token becomes a one-off access token of a first-party client,
fizzy-personal-access, the way bc3's are ofbc-personal-access. Rubric checks LG-1 and LG-2 (bc3doc/oauth/rubric.md, branchoauth-standard):*-personal-accessclient, and a one-off access token in the same digest-stored table. It is shown once, can be revoked, and is listed with connected apps. Identity-wide reach is rechecked live against membership."LG-1 is met; its "listed with connected apps" is settled as personal access tokens staying on their own page (below). LG-2 is met. No token is re-issued or re-digested: the backfill sets only
oauth_client_id, and #3192 already digested every token in place.The issuer (
Oauth::Client)PERSONAL_ACCESS_CLIENT_ID = "fizzy-personal-access", named "Personal Access Tokens". It istrusted, public (none), has scopesread write, and has no redirect URIs. It's the only client exempt from the redirect requirement.Oauth::Client.stale, which takes onlydynamically_registeredclients) never picks it up. A test ages it a year and sweeps.GRANT_TYPES = %w[ authorization_code refresh_token ]is now an allowlist that both endpoints check (#grant_types,#allows_grant_type?), and the issuer's list is empty, as bc3'sgrant_types: []is.unauthorized_client(set_client).unauthorized_client, for any grant type, before looking up a code or refresh token (validate_client_grant_type).find_by_refresh_tokenis OAuth-scoped. The allowlist makes the refusal explicit instead of incidental.has_secure_token, base58) never do.before_destroy, prepended so it runs ahead of Sweep self-registered OAuth clients left without a grant for 30 days #3159'slock_clientand of the cascade, aborts on the storedclient_id, so an unsaved change in memory doesn't get past it. Destroying it would cascade (dependent: :destroy) to every client-backed personal access token on the install.Oauth::Client.personal_access_issuer(create_or_find_by!on the uniqueclient_idindex). A fresh install loads the schema rather than running migrations, so this covers it.Minting (
My::AccessTokensController#create) now passesoauth_client: Oauth::Client.personal_access_issuer. The JSON response shape fizzy-cli reads is unchanged:id token description permission created_at, and a test pins the key set.Every "no client means personal" check, for both forms
A personal access token is now "no client, or the personal access client". The client-less form stays fully supported: production
maindoesn't know the issuer, keeps writing client-less tokens until this train ships, and shares the database with beta.personal/oauthscopesaccess_token.rb#L33personal= nil or the issuer (subquery).oauthexcludes both formshonored?: personal tokens stay honored while OAuth is dark#L157oauth_client_id.nil? || acceptance_enabled? || personal? || pilot. The client is only loaded while OAuth is dark, as beforelapsedscope andlapsed?: personal tokens never lapse#L37,#L187oauth#L67if: :oauth?fizzy_pat_) and the D1 plaintext kept for productionmainset_tokenpersonal?find_and_digest_undigestedpersonal, so it now covers both formsrevocable_tokenpersonal. The issuer has no secret andoauth.where(oauth_client: issuer)is empty, so naming it asclient_idrevokes nothing extraMy::ConnectedAppsController#oauth_tokens,users/showoauth, so personal tokens stay off Connected Apps, andDELETE /my/connected_apps/<issuer id>is a 404insufficient_scope, a personal token keeps the 401 shipped clients have always hadauthentication.rb#L108access_token.connected_app ? :forbidden : :unauthorized. #3158 keyed it onoauth_client_id?, which would have given a token of the personal access client the 403disallow_oauth_grants): personal tokens may still manage personal tokens (fizzy-cli), the join code and so onauthentication.rb#L51Current.access_token&.oauth?. Not listed in #3164. Missing it would have answered 403 to fizzy-cli creating tokens with a client-backed tokencreated_viaand pushclaim_for: what a personal token sets up belongs to no appwebhook.rb,Users::PushSubscriptionsControllerCurrent.access_token&.connected_app, which is nil for a personal token. Not listed in #3164. Missing it would have marked a CLI's webhooks as "via Personal Access Tokens", and tied its push subscriptions to the issuer, making them undeliverable (deliverable_toonly takes clients with anoauthgrant)touch_oauth_client,end_app_plumbingif: :oauth?. The issuer has no retention period, so its row isn't written on every personal token's create and destroy20261015120100_backfill_token_digests.rbmainreads from any client-backed token that had no digest yet. It now keeps it for the issuer's tokens tooThe last three rows weren't in #3164's list. I found them by
rg-ing foroauth_client_id,oauth_client,.personaland.oauthacrossapp,lib,configandsaas. Nothing insaas/reads token kinds.Migration
20261016120000_create_personal_access_client.rb:upinserts the issuer unless it exists.downsetsoauth_client_idback to NULL on the issuer's tokens, in batches withupdate_all, then deletes the row. Every version of the code reads that client-less form as personal. The issuer stays until every token is handed back, so a down that stops partway can be rerun (tested). Run it only once no code with this PR is serving: that code issues the issuer's tokens without a lock and recreates the issuer on first use, so no ordering inside down could keep up with it.20261016120100_backfill_personal_access_client.rb:where(oauth_client_id: nil).in_batches(of: 500).update_all(oauth_client_id: issuer.id), withdisable_ddl_transaction!.oauth_client_idchanges.token,token_digest,permission,expires_at(NULL) andupdated_atall stay.downis a no-op, sodb:migrate:redo:primary VERSION=20261016120100reruns it. That picks up client-less tokens productionmainwrites in the meantime.db/schema.rbanddb/schema_sqlite.rbchange only their version.Not in this PR, on purpose
oauth_client_idstays nullable (Implement PATs in terms of OAuth (as Basecamp does) #3164 step 4). Productionmainkeeps writing client-less tokens until this train is deployed there. Dropping the branch and adding NOT NULL is a later PR, after the backfill has been rerun once production runs this code.permissiondoesn't become a scope string, PATs don't get bc3's 10-year TTL (they keep no expiry, per LG-2), and they keep thefizzy_pat_prefix rather than sharingfizzy_at_the way bc3's sharebc_at_(Implement PATs in terms of OAuth (as Basecamp does) #3164 step 5).Decisions
Both open questions were decided for the OAuth program on 2026-10-17. Jeremy can overrule either.
fizzy_pat_prefix. TK-1 wants an app-specific prefix for secret scanning, andfizzy_pat_is what Prefix newly issued access tokens, refresh tokens and personal access tokens #3163 shipped and what fizzy-cli and scanners already recognise.Migration inventory rows for the integration branch
For
docs/oauth/integration-migrations.mdonoauth-integration-2026-10-15. This PR doesn't change that branch. These rows extend its table, and itsmigrate:up/downlists, after #3192's #10:20261016120000_create_personal_access_client.rboauth_clientsrow,client_idfizzy-personal-access, unless it exists.downsets the issuer's tokens'oauth_client_idback to NULL (batches of 500,update_all), then deletes the row; rerunnable if interrupted. Only once no code with this PR is servingoauth_clients(one INSERT), and on downidentity_access_tokens20261016120100_backfill_personal_access_client.rbUPDATE identity_access_tokens SET oauth_client_id = ? WHERE oauth_client_id IS NULL AND id …, in batches of 500, withdisable_ddl_transaction!.updated_atdoesn't move.downis a no-opidentity_access_tokens: ~3,500 rows, almost all personalNotes for the runbook:
20261016120100(no-op), then20261016120000, which returns every personal token to the client-less form and removes the issuer. Deploy code without this PR first: code with it keeps issuing the issuer's tokens, and recreates the issuer on first use. No token string, digest orupdated_atchanges in either direction.16d8b82to99fd4c0#11 and Bump rails from16d8b82to36b2dbf#12 run on beta1, and don't deploy integration-branch code without it afterwards. Train code below this PR reads any token with a client as an OAuth grant. It would refuse a backfilled personal token while OAuth is dark, and its idle-lapse sweep, wherever it runs, would delete every backfilled token idle for 90 days. Productionmainis unaffected: it doesn't knowoauth_client_id. Beta'srecurring.ymlruns only Solid Queue maintenance.16d8b82to36b2dbf#12 before the follow-up that drops the "no client" branch, once production runs this code:bin/rails db:migrate:redo:primary VERSION=20261016120100. Rerunning it is harmless at any time.20261015120100is safe after Bump rails from16d8b82to36b2dbf#12. That's what the change to that file is for, and a test covers it.Verified on thelio, with this branch's code:
MIGRATE=1). Starting from a loaded schema with both versions migrated down, I seeded a client-less personal access token (write,updated_at2026-01-01) and an OAuth grant. Then: up, down, down, up,redoof Bump rails from16d8b82to36b2dbf#12, andredoof Store tokens and client secrets as digests (D1 of 3: read by digest, plaintext fallback) #3192's20261015120100.oauth_client_idis the issuer or NULL;permissionandupdated_atare unchanged;personal?and authenticates for POST;db/schema.rbcame out identical.Tests
test/integration/personal_access_token_forms_test.rbruns every rule once per form, client-less and client-backed:fizzy_pat_prefix, plaintext kept, no expiry and no refresh token;client_id, or naming the issuer;insufficient_scopeandscope="write", not a grant's 403;updated_at.unauthorized_client.Oauth::ClientTest:client_id;test/migrations/personal_access_client_test.rb:updated_at, and leaves grants alone;updated_at, and a down that stops partway finds the issuer again and finishes;Each guard is pinned by a failing test. I reverted each of the 16 changes on its own:
honored?, thelapsedscope,lapsed?, the expiry callback, the prefix, the plaintext, thepersonalscope, theoauthscope,connected_app, the issuer touch,oauth_grant?, the grant-type allowlist, the redirect exemption, the destroy guard, the digest-backfill fix and minting with the issuer. Each revert made at least one of the new tests fail: 16 of 16. The three guards added since (webhook attribution on create, push subscriptions recording their app on insert, and the read-only write's 401) each break their test when reverted too.#3191 pins. I merged
test/pin-shipped-client-authinto this branch locally (not pushed) and ranShippedClientAuthenticationTestand the SaaSAdmin::AuditsControllerTest. All pass: OSS SQLite with 8 tests and SaaS MySQL 8.4 with 17 tests, both exit 0.Suites (thelio, run as
test.ymldoes:RAILS_ENV=test bin/rails db:setup test, against a dedicated MySQL 8.4 container,PARALLEL_WORKERS=12because thelio was at load ~100 and the default 32 workers timed out on MySQL, gated on exit status), ate0d3ea389(later commits add only the PR number to the conformance doc and, from review, the issuer's first-use race fix with two tests; a rebase onto the third cascade at2bf7e87ea, then ontodcb914ceb, which carries #3158, and onto3a2bb7953, where #3159'slock_clientno longer reloads, so the destroy guard reads the storedclient_id. For #3158, the read-only-write status now readsconnected_app, and two tests use #3158'sfind_honored. GitHub CI covers the head):DATABASE_ADAPTER=mysql)RuboCop (937 files), Brakeman (
--exit-on-warn) and gitleaks are clean. I didn't run system tests locally; CI runs them.No new third party or personal-data flow.