You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Converge Fizzy's first-party OAuth clients from one registry - #3252
Adds Oauth::FirstPartyClients, a registry of the OAuth clients Fizzy ships with, modelled on bc3's. Before this, each client was made its own way: a migration and a first-use fallback for the personal access issuer, and oauth:provision_mcp_client for each MCP server. Each wrote its own subset of attributes.
What it does
Definitions:
fizzy-personal-access, the personal access token issuer.
The hosted MCP servers, by the resource each fronts: fizzy-mcp-server for https://mcp.fizzy.do/mcp, and fizzy-mcp-server-beta1 for https://beta1-mcp.fizzy-beta.com/mcp.
A total BASELINE. Each definition merges over every policy column oauth_clients has, so a converge resets drift in anything a definition doesn't name. A test pins the baseline to the table's columns, so a new column must be added to it.
One transaction. Every client converges, or none does.
No secret is ever minted or rotated. A confidential client's live secret survives a converge. A new confidential client is created with no secret at all, so any client without one, never minted or since cleared, is named in the task output on every run until oauth:mint_client_secret gives it one. A public row loses any stray secret.
No client is turned back on. Fizzy has no disabled flag. A hosted MCP client is turned off by clearing its secret, which no converge writes back, or by dropping its resource from FIZZY_OAUTH_MCP_RESOURCES, which takes the client out of the converge altogether.
No repointing. An MCP client is refused rather than moved when it fronts a resource that's still configured, as before, or a hosted resource. A hosted resource is configured only on its own destination, so its row isn't safe to move from any of them.
One entry point for operators and seeds.bin/rails oauth:provision_first_party_clients runs it, and so do the development seeds. Oauth::Client.personal_access_issuer takes its attributes from the registry, so the issuer made on first use matches what a converge writes.
Design choices
Hosted MCP clients are scoped by FIZZY_OAUTH_MCP_RESOURCES. The resource URLs are literals in the registry, but each one converges only where FIZZY_OAUTH_MCP_RESOURCES names its resource. That's the only place its client can exchange tokens (Oauth::Client#delegation_client?).
Production and the betas share a database. Under this scoping, production's converge never writes beta1's row, and beta1's never writes production's.
Development and self-hosted installs serve neither resource, so they get neither client.
The alternative was to converge every literal everywhere, as bc3's registry does. That creates beta rows from a production console, and production's row on every install.
oauth:provision_mcp_client stays, as a thin wrapper. It's still the way to provision an MCP server the registry doesn't host, such as a self-hosted install's own. The registry can't name those, and retiring the task would leave them no way in. It now converges through the same converge! and BASELINE as everything else, so its rows reset drift too. A hosted resource and its client can't be provisioned apart, which closes a hole: a shared database could otherwise have hosted rows repointed from a destination where their resource isn't configured. Oauth::Client.provision_mcp_client! moves into the registry as provision_mcp_server!.
oauth:mint_client_secret is unchanged.
Staging is not in the registry.deploy.staging.yml names a staging MCP resource but no exchange client, and staging sets no FIZZY_OAUTH_MCP_RESOURCES. It can be added when staging is lit.
Tests
test/models/oauth/first_party_clients_test.rb and a task test. Each guarantee was checked by breaking it and watching its test fail:
Break
Test that fails
touch every row on converge
converging again changes nothing
merge without BASELINE, or without part of it
drift in any baseline attribute is reset
let BASELINE's nil client_secret_digest through on confidential rows
a live secret survives a converge
mint a secret for a confidential row that has none
a client turned off by clearing its secret stays off
converge every hosted definition regardless of configuration
a production converge creates no beta client; a client turned off by dropping its resource stays off; an install that serves no hosted MCP resource gets no MCP client
Oauth::FirstPartyClients, modelled on bc3's, names the clients Fizzy ships
with: the personal access token issuer, and the MCP servers 37signals hosts,
by the resource each fronts (production's fizzy-mcp-server, beta1's
fizzy-mcp-server-beta1). Each definition merges over a BASELINE of every
policy column oauth_clients has, so a converge resets drift in any attribute
a definition doesn't name. The batch runs in one transaction.
A converge never mints or rotates a secret: a confidential client's comes
only from oauth:mint_client_secret, and one without a secret is reported for
it. Hosted MCP clients converge only where FIZZY_OAUTH_MCP_RESOURCES names
their resource, the one place each can exchange tokens. Production and the
betas share a database, so production's converge never writes a beta's row,
and a client turned off by dropping its resource is left alone.
bin/rails oauth:provision_first_party_clients runs it, and so do the
development seeds. The personal access issuer made on first use takes its
attributes from the registry. oauth:provision_mcp_client stays for an MCP
server the registry doesn't host, such as a self-hosted one, and converges
its client over the same baseline; a hosted resource and its client can't be
provisioned apart.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Provisioning can repoint an existing production OAuth client and break token exchanges, while repeated runs can incorrectly report an unminted client as ready for authentication.
Centralizes Fizzy’s first-party OAuth client provisioning in one registry, shared by operator tasks, development seeds, and personal-access issuer creation.
Changes:
Adds transactional convergence with a complete policy baseline and environment-scoped hosted MCP definitions.
Routes existing provisioning through the registry and reports clients awaiting secrets.
Adds coverage for drift correction, secret preservation, resource isolation, and rollback.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
… and let non-MCP rows shed a stray resource
A hosted row's resource is configured only on its own destination, and
production and the betas share a database, so the repointing guard now
refuses to move a hosted row from anywhere, including when
oauth:provision_mcp_client defaults to fizzy-mcp-server.
The guard applies only to MCP definitions, so a personal access row that
drifted onto a configured resource converges back to none.
A new registry client keeps no throwaway secret, so awaiting_secret? reports
it on every converge until oauth:mint_client_secret gives it one.
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Explicitly passing the legacy default CLIENT_ID now raises for self-hosted resources, breaking existing provisioning commands even though omitting it still provisions the same ID.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Allow explicit legacy default client ID for self-hosted resources
app/models/oauth/first_party_clients.rb:97
For a self-hosted resource, explicitly passing CLIENT_ID=fizzy-mcp-server now raises here, while omitting CLIENT_ID still provisions that exact ID. This breaks existing provisioning commands that explicitly name the previous default, even when updating their own unchanged self-hosted row. Allow the legacy default ID consistently; the fronting_for_good? check in converge! already prevents it from taking over an existing hosted row.
converge! already keeps fizzy-mcp-server off production's row wherever that
row exists, so the hosted-pair check needs to hold only for the betas' ids.
Fixed the finding in Copilot's review on 51baece, the one titled "Allow explicit legacy default client ID for self-hosted resources", in 8dcb0a3. CLIENT_ID=fizzy-mcp-server is accepted again for any resource, the same as leaving it out. The hosted-pair check now applies only to the betas' client ids, and converge! still refuses to move production's row wherever it exists. Test: "a self-hosted MCP server may name the default client_id, as it may omit it".
All five earlier review threads are resolved: four were fixed in 51baece. The fifth, race-safe creation, I declined on merit, and the reasoning is in #3252 (comment).
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
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
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Adds
Oauth::FirstPartyClients, a registry of the OAuth clients Fizzy ships with, modelled on bc3's. Before this, each client was made its own way: a migration and a first-use fallback for the personal access issuer, andoauth:provision_mcp_clientfor each MCP server. Each wrote its own subset of attributes.What it does
fizzy-personal-access, the personal access token issuer.fizzy-mcp-serverforhttps://mcp.fizzy.do/mcp, andfizzy-mcp-server-beta1forhttps://beta1-mcp.fizzy-beta.com/mcp.oauth_clientshas, so a converge resets drift in anything a definition doesn't name. A test pins the baseline to the table's columns, so a new column must be added to it.oauth:mint_client_secretgives it one. A public row loses any stray secret.FIZZY_OAUTH_MCP_RESOURCES, which takes the client out of the converge altogether.bin/rails oauth:provision_first_party_clientsruns it, and so do the development seeds.Oauth::Client.personal_access_issuertakes its attributes from the registry, so the issuer made on first use matches what a converge writes.Design choices
FIZZY_OAUTH_MCP_RESOURCES. The resource URLs are literals in the registry, but each one converges only whereFIZZY_OAUTH_MCP_RESOURCESnames its resource. That's the only place its client can exchange tokens (Oauth::Client#delegation_client?).oauth:provision_mcp_clientstays, as a thin wrapper. It's still the way to provision an MCP server the registry doesn't host, such as a self-hosted install's own. The registry can't name those, and retiring the task would leave them no way in. It now converges through the sameconverge!and BASELINE as everything else, so its rows reset drift too. A hosted resource and its client can't be provisioned apart, which closes a hole: a shared database could otherwise have hosted rows repointed from a destination where their resource isn't configured.Oauth::Client.provision_mcp_client!moves into the registry asprovision_mcp_server!.oauth:mint_client_secretis unchanged.deploy.staging.ymlnames a staging MCP resource but no exchange client, and staging sets noFIZZY_OAUTH_MCP_RESOURCES. It can be added when staging is lit.Tests
test/models/oauth/first_party_clients_test.rband a task test. Each guarantee was checked by breaking it and watching its test fail:client_secret_digestthrough on confidential rows