[backend] shared dev credentials#2588
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces shared OAuth credentials support for Instant applications. It adds schema entities for persisted OAuth clients, implements credential lookup and validation logic in the model layer, extends OAuth client management routes to support shared credentials, adds redirect URI and request origin authorization with shared-credentials-aware defaults, and integrates user limit enforcement for shared credential usage with comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant DashRoutes as Dash Routes
participant OAuthModel as OAuth Client Model
participant SharedModel as Shared OAuth Model
participant Flags as Flags/Query
participant DB as Database
Client->>DashRoutes: POST /oauth-client (meta.useSharedCredentials=true)
DashRoutes->>OAuthModel: Resolve provider_name from provider_id
DashRoutes->>SharedModel: get-shared-credential!(provider_name, app_id)
SharedModel->>Flags: shared-oauth-clients[provider_name]
Flags->>DB: Query shared-oauth-clients by provider
DB-->>Flags: Encrypted credential row
Flags-->>SharedModel: Credential with decrypted secret
SharedModel-->>DashRoutes: Shared credential found or validation error
DashRoutes->>OAuthModel: create! with shared-credential values
OAuthModel->>OAuthModel: Override client_id/secret from shared cred
OAuthModel->>DB: Store OAuth client
DB-->>OAuthModel: Record created
OAuthModel-->>DashRoutes: OAuth client entity
DashRoutes-->>Client: HTTP 200 with created client
sequenceDiagram
participant Client
participant RuntimeRoutes as Runtime Routes
participant AuthModel as Authorized Origin Model
participant OAuthModel as OAuth Client Model
participant SharedModel as Shared OAuth Model
Client->>RuntimeRoutes: GET /oauth/start?redirect_uri=http://localhost:3000&provider_id=X
RuntimeRoutes->>OAuthModel: Load OAuth client to get use_shared_credentials
RuntimeRoutes->>AuthModel: assert-authorized-redirect-uri!(redirect_uri, use_shared_credentials?)
alt Shared Credentials Enabled
AuthModel->>AuthModel: shared-credentials-default-match(localhost)
AuthModel-->>RuntimeRoutes: Match found (service="localhost")
else Not Enabled
AuthModel->>AuthModel: find-match against configured origins
AuthModel-->>RuntimeRoutes: Match found or error
end
RuntimeRoutes->>RuntimeRoutes: Initiate OAuth flow
RuntimeRoutes-->>Client: HTTP 302 to provider
Client->>RuntimeRoutes: GET /oauth/token-callback?code=...&state=...
RuntimeRoutes->>RuntimeRoutes: Load OAuth client (use_shared_credentials=true)
RuntimeRoutes->>AuthModel: assert-authorized-request-origin!(request.origin, use_shared_credentials?)
AuthModel->>AuthModel: shared-credentials-default-match(request.origin)
AuthModel-->>RuntimeRoutes: Origin authorized
RuntimeRoutes->>RuntimeRoutes: upsert-oauth-link! with use_shared_credentials=true
RuntimeRoutes->>SharedModel: assert-shared-credentials-allowed!(app_id) if new user
SharedModel-->>RuntimeRoutes: User count within limit or validation error
RuntimeRoutes-->>Client: OAuth link created/updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/flags.clj`:
- Around line 152-159: The shared-oauth-clients transform currently calls
crypt-util/hex-string->bytes directly and a malformed
encryptedClientSecretHexString can throw and abort the whole flags transform;
update the mapping in shared-oauth-clients to validate/parse each credential
individually by wrapping the hex-string->bytes call in a safe/try (or use a
helper like safe-parse-hex) so that when hex-string->bytes throws you catch the
exception, log an error mentioning the credential id/provider (use :id or (get c
"id") and :providerName), and skip that single credential instead of letting the
error bubble; ensure parse-uuid is still applied to valid entries only and the
final group-by :providerName operates on the filtered, safe sequence.
In `@server/src/instant/model/app_oauth_client.clj`:
- Around line 35-41: get-provider-by-id! currently returns the raw provider
record whose provider name key is :name, but callers expect :provider_name;
after fetching via app-oauth-service-provider-model/get-by-id in
get-provider-by-id! normalize the record by assoc'ing :provider_name (e.g.
(assoc provider :provider_name (:name provider))) before asserting/returning so
downstream destructuring of :provider_name works for shared credential lookup
and GitHub client construction.
- Around line 23-25: encrypt-client-secret currently calls String/.getBytes with
no charset which uses the platform default; change it to explicitly use UTF-8 so
encryption matches decryption (which expects UTF-8). Update the
encrypt-client-secret function to obtain UTF-8 bytes for client-secret (use the
UTF-8 charset API) and keep associated-data as-is (uuid-util/->bytes id) so
decryption will succeed across platforms.
In `@server/src/instant/runtime/routes.clj`:
- Around line 299-313: The signup flow generates an ID for validation then lets
create! generate a different one; bind the chosen id once and reuse it so
assert-signup! and app-user-model/create! operate on the exact same user id. In
the let block around assert-signup! and app-user-model/create! introduce a local
id (e.g., id (or guest-user-id (random-uuid))) and replace the ad-hoc uses so
both assert-signup! and create! receive that id (also update any new-user
construction to use that bound id).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 621b7531-9f5a-4a68-8e9b-581ab2e40222
📒 Files selected for processing (7)
server/flags-config/instant.schema.tsserver/src/instant/dash/routes.cljserver/src/instant/flags.cljserver/src/instant/model/app_authorized_redirect_origin.cljserver/src/instant/model/app_oauth_client.cljserver/src/instant/model/app_user.cljserver/src/instant/runtime/routes.clj
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/model/app_oauth_client.clj`:
- Around line 20-21: The current use-shared-credentials? function uses boolean
which treats strings like "false" or "0" as truthy; change the check to require
an actual boolean true value (e.g., use true? or compare to true) when reading
"useSharedCredentials" from meta so only an explicit boolean true enables shared
credentials; update the body of use-shared-credentials? in app_oauth_client.clj
to call true? (or = true ...) on (get meta "useSharedCredentials") and leave
other logic unchanged.
- Around line 211-215: The REPL helper uses a string placeholder and an
unrequired tool/copy wrapper; change the id argument passed to
encrypt-client-secret to a UUID object (e.g., convert the pasted id with
java.util.UUID/fromString) and remove the unnecessary tool/copy call so the
expression directly calls crypt-util/bytes->hex-string on (encrypt-client-secret
...); ensure you only pass a UUID to encrypt-client-secret (which calls
uuid-util/->bytes) and drop the tool/copy wrapper since tool/copy is not in the
namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caf53402-98f0-48bb-a639-37b7e9fa08c6
📒 Files selected for processing (3)
server/src/instant/model/app_oauth_client.cljserver/src/instant/model/app_oauth_service_provider.cljserver/src/instant/runtime/routes.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/instant/runtime/routes.clj
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/instant/model/app_oauth_client.clj (1)
20-21:⚠️ Potential issue | 🟠 MajorRequire an exact boolean to enable shared credentials.
booleantreats"false"and"0"as enabled. Since client-providedmetais only checked as a map before this helper is called, malformed input can unintentionally switch the OAuth client onto shared credentials.Suggested fix
(defn use-shared-credentials? [meta] - (boolean (get meta "useSharedCredentials"))) + (true? (get meta "useSharedCredentials")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/model/app_oauth_client.clj` around lines 20 - 21, The helper use-shared-credentials? currently uses boolean which treats values like "false" or "0" as truthy; change it to require an exact true value by checking equality to true (e.g. use true? or (= true (get meta "useSharedCredentials"))) so only a literal boolean true enables shared credentials; update the use-shared-credentials? function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/scripts/shared_oauth_clients.clj`:
- Around line 39-40: The docstring mentioning `rotate!` is stale because this
namespace doesn't define `rotate!`; update the docstring in
shared_oauth_clients.clj (the comment that currently says "use `delete!` first
(or call `rotate!` for rotation)") to remove the `rotate!` reference and only
instruct callers to use the existing `delete!` (or point to the correct helper
if there is a different rotation function), ensuring `rotate!` is not
referenced.
---
Duplicate comments:
In `@server/src/instant/model/app_oauth_client.clj`:
- Around line 20-21: The helper use-shared-credentials? currently uses boolean
which treats values like "false" or "0" as truthy; change it to require an exact
true value by checking equality to true (e.g. use true? or (= true (get meta
"useSharedCredentials"))) so only a literal boolean true enables shared
credentials; update the use-shared-credentials? function accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c33a4a7-abe3-4121-bbea-bb2350bd3653
📒 Files selected for processing (7)
server/src/instant/dash/routes.cljserver/src/instant/model/app_oauth_client.cljserver/src/instant/scripts/shared_oauth_clients.cljserver/test/instant/dash/routes_test.cljserver/test/instant/model/app_authorized_redirect_origin_test.cljserver/test/instant/model/app_user_test.cljserver/test/instant/runtime/routes_test.clj
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/instant/model/shared_oauth_client.clj (1)
49-55:get-shared-credentialsilently drops duplicates if the uniqueness invariant is ever relaxed.
(first (get ...))discards the rest of the group without warning. The PR notes mention potentially allowing multiple clients per provider later — when that happens, you'll want a deterministic selection strategy (round-robin, environment-tagged, etc.) rather than implicitfirst. Fine for now givenproviderNameis unique-indexed in the schema, but worth an inline comment orwhen (> (count ...) 1)warning to make the assumption explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/instant/model/shared_oauth_client.clj` around lines 49 - 55, get-shared-credential currently returns (first ...) and silently drops duplicates; make the behavior explicit by detecting when (count candidates) > 1 and either logging/warning with provider-name and the candidate ids or enforcing a deterministic selection (e.g., sort by a stable key like :id and pick the first, or implement round-robin via an atom) so duplicates are not implicitly discarded; update both get-shared-credential and get-shared-credential! to perform the duplicate check and selection (or raise) and add an inline comment documenting the chosen strategy for future multi-client support.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/runtime/routes.clj`:
- Around line 300-321: The shared-credentials user cap check
(shared-oauth-client-model/assert-shared-credentials-allowed!) is subject to a
TOCTOU race with app-user-model/create! (two signups at limit-1 can both pass
the check and exceed the cap); document this caveat next to the check and either
(a) if you need a strict cap, move the allowance check into the same DB
transaction as app-user-model/create! or enforce the limit with a DB-level
constraint/index, or (b) keep as-is for dev-only behavior but add a clear
comment referencing assert-shared-credentials-allowed!, app-user-model/create!,
and app-user-oauth-link-model/create! explaining the race and rationale.
---
Nitpick comments:
In `@server/src/instant/model/shared_oauth_client.clj`:
- Around line 49-55: get-shared-credential currently returns (first ...) and
silently drops duplicates; make the behavior explicit by detecting when (count
candidates) > 1 and either logging/warning with provider-name and the candidate
ids or enforcing a deterministic selection (e.g., sort by a stable key like :id
and pick the first, or implement round-robin via an atom) so duplicates are not
implicitly discarded; update both get-shared-credential and
get-shared-credential! to perform the duplicate check and selection (or raise)
and add an inline comment documenting the chosen strategy for future
multi-client support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6c4320d-2999-408f-bc11-12d5d35b1ec0
📒 Files selected for processing (5)
server/src/instant/dash/routes.cljserver/src/instant/model/app_oauth_client.cljserver/src/instant/model/app_oauth_service_provider.cljserver/src/instant/model/shared_oauth_client.cljserver/src/instant/runtime/routes.clj
Backend foundation for shared OAuth credentials. Splits the server- only changes out of the full sharedcred PR so it can be reviewed on its own; the dashboard UI follows in a separate PR rebased on top. ## What this enables Apps can opt an OAuth client into Instant-provided dev credentials instead of supplying their own client_id / client_secret. Primary use case: a developer can test Google sign-in on localhost / exp:// without ever touching Google Cloud Console. ## Config - New flag `shared-oauth-clients` in instant-config (keyed by provider name, contains clientId + encryptedClientSecretHexString). See `server/flags-config/instant.schema.ts` + `server/src/instant/ flags.clj`. ## Model changes server/src/instant/model/app_oauth_client.clj - `use-shared-credentials?` predicate on a client's meta. - `get-shared-credential!` (throws when absent) and `get-provider-by-id!` (canonical provider-row lookup; returns the full record so callers destructure). - `->OAuthClient` now derives provider-name from provider_id and, on a shared-credential client, substitutes the app's row with the shared cred values at request time. No more meta.providerName. - `assert-shared-credentials-allowed!` enforces the 100-user cap (development-only guardrail). Callers: oauth-clients-post, update-oauth-client, and the new-user branch of upsert-oauth-link!. - `update!` extended to accept :client-id and :encrypted-client- secret so the upgrade-from-shared flow is an in-place update instead of delete + recreate. server/src/instant/model/app_authorized_redirect_origin.clj - `shared-credentials-default-match` auto-allows localhost (http/https) and exp:// for shared-credential clients, without them having to register those origins manually. Custom domains still go through the existing authorized-origins list. server/src/instant/model/app_user.clj - `users-at-least?` bounded existence check: SELECT COUNT(*) over a LIMIT-n subquery of the $users.id attr. Used by the cap above. ## Route changes server/src/instant/runtime/routes.clj - Origin/redirect-uri validation consolidated behind `assert-authorized-redirect-uri!` / `assert-authorized-request-origin!`. Both branch on use-shared-credentials? to fall back to the default-match. - `oauth-id-token-callback` now validates origins too (previously gated on client_secret, which shared-cred rows don't have on the raw record). - 100-user cap checked inside upsert-oauth-link!'s new-user branch so returning users are never blocked. server/src/instant/dash/routes.clj - oauth-clients-post / update-oauth-client: when meta.useSharedCredentials is true, require that a shared cred exists for the provider (via get-shared-credential!) and enforce the user cap before saving. - update-oauth-client now accepts client_id / client_secret so the dashboard's in-place "update credentials" action can swap creds without destroying and recreating the client.
99dc2f7 to
ebd90c9
Compare
| "rule-where-testing": i.entity({ | ||
| enabled: i.boolean(), | ||
| }), | ||
| "shared-oauth-clients": i.entity({ |
There was a problem hiding this comment.
Can we put the shared clients in resources/config/prod.edn and resources/config/dev.edn instead?
That would make setup for dev much easier and then we wouldn't have to do a db lookup to get the creds.
There was a problem hiding this comment.
Good point! Initially I was hesitant, thinking "there's going to be so many clients". But I think I was overthinking it. Will update 🫡
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Per Daniel's review on the previous PR: store shared OAuth client
credentials in resources/config/{dev,prod}.edn instead of the
shared-oauth-clients entity on the instant-config app. This makes dev
setup easier (no more import-from-prod!) and removes the runtime DB
lookup for credentials.
Credentials decrypt once at server startup alongside every other config
secret (Stripe, Postmark, :google-oauth-client, etc.). Lookup is now an
in-process map read.
## Changes
- config_edn.clj: ::shared-oauth-clients spec, added to ::config and
::config-prod (:opt-un)
- config.clj: shared-oauth-clients accessor
- model/shared_oauth_client.clj: rewritten to read from config; removed
make-row, import-from-prod!, and the comment block
- model/app_oauth_client.clj: ->OAuthClient handles the new shape
(kebab-case keys, already-decrypted Secret) and skips the per-row
AEAD decrypt for shared clients
- flags.clj: removed :shared-oauth-clients from query, transform, and
accessor; dropped now-unused crypt-util require
- flags-config/instant.schema.ts: removed shared-oauth-clients and
shared-oauth-clients-instant-dev entities
- dash/routes_test.clj: tests redef config/shared-oauth-clients with
the new shape
## Config data
dev.edn gets the existing localhost-callback Google client. prod.edn
gets the existing api.instantdb.com-callback Google client. Each
:client-secret is encrypted under that env's hybrid keyset via the
existing tasks/encrypt-config-secret pattern. staging.edn left alone
(no staging-callback creds; spec is :opt-un).
## Migration follow-up
Rows in instant-config's shared-oauth-clients and
shared-oauth-clients-instant-dev entities are now orphaned but
harmless. Push the schema change and clean up rows after prod soaks on
the new path for a release cycle.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Daniel's review: instead of stashing useSharedCredentials inside the meta JSON blob, give it a real boolean attr on $oauthClients. ## Changes - system_catalog.clj: new useSharedCredentials boolean attr on $oauthClients (with a usesharcr shortcode) - system_catalog_ops.clj: :useSharedCredentials → :use_shared_credentials label translation - model/app_oauth_client.clj: use-shared-credentials? now reads the top-level :use_shared_credentials field on the oauth-client row; create! and update! write/handle the new triple - dash/routes.clj: oauth-clients-post and update-oauth-client read use_shared_credentials from the request body (top-level, not from meta) and include it in the response payload - runtime/routes.clj: callers pass the client row directly to use-shared-credentials? instead of digging into :meta Tests updated accordingly. The :meta attr stays on the schema (still used by oauth-id-token-callback for allowUnverifiedEmail). ## Deploy note Per the comment in system_catalog.clj on adding new system attrs: the useSharedCredentials ident name should be reserved in the :reserved-system-catalog-ident-names instant-config flag before this ships, so a user app can't race in and create the attr first. The ensure-attrs-on-system-catalog-app migration runs at server startup and creates the attr; afterwards remove it from the reservation flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two small follow-ups: - The use-shared-credentials? helper was a thin (boolean ...) wrapper over a keyword lookup. Now that the field is a typed boolean attr on $oauthClients, callers just read :use_shared_credentials directly. - The (w/stringify-keys ...) on the dash :meta param was added in this branch only because the model used (get meta "useSharedCredentials") on a string key. With useSharedCredentials promoted to its own field, restore the original parser shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The file is small enough that the dividers are noise, not navigation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
make encrypt-secret-{dev,staging,prod} is the user-facing way to do
this; the underlying clj -X invocation is an implementation detail.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
when-some lets us treat "key missing" and explicit null the same way (both skip the update); thread-first reads cleaner than get-in for a two-step lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This PR implements the backend for shared dev credentials!
For the dashboard PR, check out: #2553
Note: I will have a follow-on PR shortly for the CLI
Product Context
Say you want to add Google Oauth.
Problem: Even to test in development, you have to go through Google Cloud Console and create an Oauth client. Google Cloud Console is quite annoying to navigate, which means poor devex for us.
Solution: We will provide default oauth credentials for development!
Users don't have to go through Google Cloud Console. They can Oauth Clients without providing
client-idandclient-secretvalues. In development, Instant will handle those values for them. We'll also default allowed origins tolocalhostandexp, so they don't have to create those redirect origins either.This has some limits though:
client-id.client-idwith the user, in case we end up needing to change the oauth client.Implementation
1. Shared credentials live in
resources/config/{dev,prod}.ednThe shared
client-id/client-secretfor each provider are stored as a top-level:shared-oauth-clientskey in our per-env config files, alongside every other config secret (Stripe, Postmark,:google-oauth-client, etc.).:shared-oauth-clients [{:provider-name "google" :client-id "...apps.googleusercontent.com" :client-secret {:enc "..."}}]:client-secretis encrypted under the env'shybrid-keysetand decrypted once at startup, so the runtime lookup is an in-process map read with no DB round-trip.dev.ednholds the credential registered againstlocalhost:8888;prod.ednholds the credential registered againstapi.instantdb.com. Engineers don't need to seed anything locally — pulling the branch is enough.2.
$oauthClientshas auseSharedCredentialsboolean fielduseSharedCredentialsis a typed boolean attr on the$oauthClientssystem-catalog etype (not nested inmeta). When true:->OAuthClientlooks up the provider'sclient-id+client-secretinconfig/shared-oauth-clientsand substitutes them in.http://localhost,https://localhost, andexp://. Users can still add their own custom origins.Adding new shared credentials
To add or rotate a provider's secret in a config file, run
make encrypt-secret-dev(or-staging/-prod), paste the plaintext, and drop the resulting{:enc "..."}into the relevant*.ednunder:shared-oauth-clients. Same workflow as every other secret on the server.Deploy Plan
["$oauthClients" "useSharedCredentials"]to the:reserved-system-catalog-ident-namesinstant-config flag so a user app can't race in and create the attr first.ensure-attrs-on-system-catalog-appmigration creates the new attr automatically.shared-oauth-clientsandshared-oauth-clients-instant-deventities have been removed from the instant-config schema in this PR. Their rows in prod's instant-config app are now orphaned but harmless — clean those up after a soak cycle.@dwwoelfel @nezaj @drew-harris