docs(oagw): design for OAuth2 authorization-code auth plugin (DRAFT)#4225
Draft
striped-zebra-dev wants to merge 1 commit into
Draft
docs(oagw): design for OAuth2 authorization-code auth plugin (DRAFT)#4225striped-zebra-dev wants to merge 1 commit into
striped-zebra-dev wants to merge 1 commit into
Conversation
Add a design-only ADR and scenarios for per-user OAuth2 authorization-code (RFC 6749 §4.1 + PKCE) support in OAGW, as a peer of the existing client-credentials auth plugin. No implementation. - OAGW acts as a standard OAuth 2.1 client: discovery (RFC 8414/9728), DCR (RFC 7591), PKCE, code/refresh exchange live in toolkit-auth; the plugin orchestrates them. - Standard OAuth callback endpoint (no custom "complete" step), reachable either directly (OAGW exposed) or reverse-proxied by the BFF (OAGW internal); the handler is identical in both topologies. - Re-authorization surfaces through the existing CanonicalError/reason channel (WWW-Authenticate challenge, AUTHORIZATION_REQUIRED); the cross-gear ServiceGatewayClientV1 SDK is unchanged. - Per-user token stored in CredStore (Private sharing, opaque value) using its already-designed put/get/delete; transient PKCE/CSRF state stays out of CredStore in an ephemeral TTL store. DRAFT / not final: open questions remain (multi-instance pending store, callback-exposure policy, stateless-state variant, durable CredStore backend, toolkit-auth additions, WWW-Authenticate challenge shape). See the ADR's Open Questions section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a draft ADR for a built-in OAuth2 authorization-code PKCE plugin and documents enrollment, token refresh, authorization-required failures, revocation, and subject-isolation scenarios. ChangesOAuth2 authorization-code plugin design
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
striped-zebra-dev
marked this pull request as draft
July 17, 2026 15:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md`:
- Around line 347-353: Update the refresh flow around UserTokenStore.load,
refresh_token, UserTokenStore.save, and UserTokenStore.delete to coordinate
concurrent refreshes for each (subject, upstream_id), using per-key
single-flight locking or compare-and-swap. Ensure only one request refreshes a
given record at a time, and on rejection delete only the version that failed
rather than a newer rotated record.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required.md`:
- Around line 39-40: Update the OAuth2 authorization-required scenario assertion
around FailureReason::AuthorizationRequired to match the canonical consumer
contract, using upstream_id and authorize instead of
authorization_url/authorization_uri; if the ADR defines the alternate shape as
canonical, update FailureReason::AuthorizationRequired and its projection
accordingly, then keep the scenario and contract consistent.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/positive-9.11-oauth2-authorization-code-enrollment.md`:
- Around line 29-30: Validate the caller-controlled return_to value in the OAuth
authorization flow before persisting it or using it for redirects. Update the
POST /oagw/v1/upstreams/{id}/oauth/authorize handling and corresponding callback
path to accept only same-origin or server-side allowlisted destinations,
rejecting all other URLs.
- Around line 44-49: The OAuth callback flow must consume or atomically mark the
pending state as used before exchanging the authorization code. Update the state
lookup/consumption behavior described in the callback flow so any later callback
is rejected, including when token exchange fails, while preserving the existing
matching, verifier, redirect URI, and token-storage behavior.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/positive-9.12-oauth2-authorization-code-refresh.md`:
- Around line 39-44: Update the refresh-token flow documented in the expected
behavior to serialize writes and deletes per subject, using single-flight or
conditional update/delete semantics. Ensure a stale refresh attempt cannot
delete credentials rotated by another request, and preserve writing rotated
tokens under the same Private reference before continuing with the refreshed
bearer token.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8146871f-426e-4df7-98ae-41fb22999251
📒 Files selected for processing (8)
gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.mdgears/system/oagw/docs/adr-oauth2-client-credentials-auth-plugin.mdgears/system/oagw/scenarios/INDEX.mdgears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required.mdgears/system/oagw/scenarios/proxy-api/authentication/negative-9.14-oauth2-authorization-code-refresh-rejected.mdgears/system/oagw/scenarios/proxy-api/authentication/negative-9.15-oauth2-authorization-code-cross-subject-isolation.mdgears/system/oagw/scenarios/proxy-api/authentication/positive-9.11-oauth2-authorization-code-enrollment.mdgears/system/oagw/scenarios/proxy-api/authentication/positive-9.12-oauth2-authorization-code-refresh.md
Comment on lines
+347
to
+353
| UserTokenStore.load(ctx, upstream_id) | ||
| None -> Err(AuthorizationRequired(resource)) | ||
| valid (> now + margin) -> inject Authorization: Bearer; Ok | ||
| expired, has refresh -> refresh_token(...) | ||
| ok -> UserTokenStore.save(rotated); inject Bearer; Ok | ||
| rejected -> UserTokenStore.delete(); Err(AuthorizationRequired(resource)) | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize refreshes per (subject, upstream_id) or use compare-and-swap.
Two concurrent proxy requests can load the same expiring record. If refresh-token rotation is enabled, one request can save the rotated record while the other refreshes with the old token, receives rejection, and deletes the newly saved record. Use single-flight/per-key locking or conditional writes, and only delete the version that failed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md` around
lines 347 - 353, Update the refresh flow around UserTokenStore.load,
refresh_token, UserTokenStore.save, and UserTokenStore.delete to coordinate
concurrent refreshes for each (subject, upstream_id), using per-key
single-flight locking or compare-and-swap. Ensure only one request refreshes a
given record at a time, and on rejection delete only the version that failed
rather than a newer rotated record.
Comment on lines
+39
to
+40
| - `oagw_sdk::reason::auth::FailureReason::from(&err)` projects to | ||
| `AuthorizationRequired { authorization_url }`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'FILES:\n'
git ls-files 'gears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required.md' \
'gears/system/oagw' | sed -n '1,200p'
printf '\nSEARCH AUTH FAILURE SHAPES:\n'
rg -n --hidden --glob '!**/.git/**' \
'AuthorizationRequired|authorization_url|upstream_id|authorize' \
gears/system/oagw | sed -n '1,240p'
printf '\nSCENARIO CONTEXT:\n'
cat -n gears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required.md | sed -n '1,120p'Repository: constructorfabric/gears-rust
Length of output: 47087
🏁 Script executed:
set -euo pipefail
pwd
git ls-files | rg '^gears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required\.md$|^gears/system/oagw/'Repository: constructorfabric/gears-rust
Length of output: 20426
🏁 Script executed:
set -euo pipefail
printf 'ADR EXCERPT:\n'
sed -n '180,220p;300,315p' gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md | cat -n
printf '\nSDK REASON EXCERPT:\n'
sed -n '1,260p' gears/system/oagw/oagw-sdk/src/reason.rs | cat -n | sed -n '1,260p'
printf '\nSCENARIO INDEX EXCERPT:\n'
sed -n '200,214p' gears/system/oagw/scenarios/INDEX.md | cat -nRepository: constructorfabric/gears-rust
Length of output: 14902
🏁 Script executed:
set -euo pipefail
rg -n 'AuthorizationRequired|authorization_url|authorize\b|upstream_id' gears/system/oagw/oagw-sdk/src gears/system/oagw/oagw/src | sed -n '1,240p'Repository: constructorfabric/gears-rust
Length of output: 12861
Align FailureReason::AuthorizationRequired with the ADR. The scenario currently documents authorization_url/authorization_uri, but the consumer contract projects AuthorizationRequired { upstream_id, authorize }. Update the scenario or the canonical contract so tests don’t lock in the wrong shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/negative-9.13-oauth2-authorization-code-authorization-required.md`
around lines 39 - 40, Update the OAuth2 authorization-required scenario
assertion around FailureReason::AuthorizationRequired to match the canonical
consumer contract, using upstream_id and authorize instead of
authorization_url/authorization_uri; if the ADR defines the alternate shape as
canonical, update FailureReason::AuthorizationRequired and its projection
accordingly, then keep the scenario and contract consistent.
Comment on lines
+29
to
+30
| 1. `POST /oagw/v1/upstreams/{id}/oauth/authorize` with | ||
| `{ "scope": "profile email", "return_to": "https://app.example/connected" }`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Constrain return_to before persisting and redirecting.
This accepts a caller-controlled URL and later redirects the browser to it. Require a same-origin or server-side allowlisted destination; otherwise the callback becomes an open-redirect/phishing primitive.
Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/positive-9.11-oauth2-authorization-code-enrollment.md`
around lines 29 - 30, Validate the caller-controlled return_to value in the
OAuth authorization flow before persisting it or using it for redirects. Update
the POST /oagw/v1/upstreams/{id}/oauth/authorize handling and corresponding
callback path to accept only same-origin or server-side allowlisted
destinations, rejecting all other URLs.
Comment on lines
+44
to
+49
| - The callback matches the pending record by `state` (single-use) and exchanges | ||
| the `code` with the `code_verifier` and the same `redirect_uri` from the | ||
| pending record (so exposed and relayed paths behave identically). | ||
| - The resulting token is stored in `cred_store` with `SharingMode::Private`, | ||
| scoped to the calling subject; the stored value is opaque (never logged). | ||
| - The pending record is removed after a successful exchange, and the callback |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Consume OAuth state before exchanging the code.
The checklist says state is single-use but also says the pending record is removed only after successful exchange. Atomically consume or mark the state used before token exchange, and reject all subsequent callbacks even if the exchange fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/positive-9.11-oauth2-authorization-code-enrollment.md`
around lines 44 - 49, The OAuth callback flow must consume or atomically mark
the pending state as used before exchanging the authorization code. Update the
state lookup/consumption behavior described in the callback flow so any later
callback is rejected, including when token exchange fails, while preserving the
existing matching, verifier, redirect URI, and token-storage behavior.
Comment on lines
+39
to
+44
| Expected: | ||
| - The plugin exchanges the refresh token at the token endpoint. | ||
| - The rotated record (new access token, and rotated refresh token when the | ||
| server issues one) is written back to `cred_store` under the same | ||
| `Private` reference. | ||
| - The request proceeds with the refreshed bearer token. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the scenario file and related index around the referenced behavior.
printf '\n== scenario file ==\n'
sed -n '1,120p' gears/system/oagw/scenarios/proxy-api/authentication/positive-9.12-oauth2-authorization-code-refresh.md
printf '\n== scenario index matches ==\n'
rg -n "refresh|rejected refresh|delete|cred_store|Private reference|rotate" gears/system/oagw/scenarios/INDEX.md gears/system/oagw/scenarios/proxy-api/authentication -g '*.md'
printf '\n== nearby auth scenarios ==\n'
fd -a 'positive-9.12-oauth2-authorization-code-refresh.md|INDEX.md' gears/system/oagw/scenariosRepository: constructorfabric/gears-rust
Length of output: 8720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== negative scenario ==\n'
sed -n '1,140p' gears/system/oagw/scenarios/proxy-api/authentication/negative-9.14-oauth2-authorization-code-refresh-rejected.md
printf '\n== auth ADR mentions ==\n'
rg -n "single-flight|compare-and-swap|CAS|conditional delete|refresh token|rotate|mutex|lock|serialization|version" gears/system/oagw/docs gears/system/oagw/scenarios -g '*.md'
printf '\n== auth plugin related scenario titles ==\n'
rg -n "^#|^## Scenario|Mechanism|Expected:" gears/system/oagw/scenarios/proxy-api/authentication/*oauth2* -g '*.md'Repository: constructorfabric/gears-rust
Length of output: 44018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth ADR refresh flow ==\n'
sed -n '300,390p' gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md
printf '\n== search for auth plugin implementation references ==\n'
rg -n "oauth2_auth_code|UserTokenStore|refresh token|invalid_grant|AUTHORIZATION_REQUIRED|cred_store|Private" gears -g '*.{rs,md,ts,tsx,py,json,yml,yaml}'
printf '\n== candidate source files ==\n'
fd -a 'oauth2|auth.*code|token.*store|cred_store' gears -t fRepository: constructorfabric/gears-rust
Length of output: 48955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== ADR around token store semantics ==\n'
sed -n '250,290p' gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md
printf '\n== token-store related lines ==\n'
rg -n "UserTokenStore|single-use|single-flight|save\\(|delete\\(|refreshes|rotated|same `Private`" gears/system/oagw/docs/adr-oauth2-authorization-code-auth-plugin.md gears/system/oagw/scenarios/proxy-api/authentication -g '*.md'Repository: constructorfabric/gears-rust
Length of output: 6625
Serialize refresh-token writes and deletes. Concurrent refreshes can race: one request may rotate the token while another gets invalid_grant and deletes the same Private record, wiping the fresh credentials and forcing re-authorization. Use per-subject single-flight or conditional update/delete semantics so only the still-current token can be removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@gears/system/oagw/scenarios/proxy-api/authentication/positive-9.12-oauth2-authorization-code-refresh.md`
around lines 39 - 44, Update the refresh-token flow documented in the expected
behavior to serialize writes and deletes per subject, using single-flight or
conditional update/delete semantics. Ensure a stale refresh attempt cannot
delete credentials rotated by another request, and preserve writing rotated
tokens under the same Private reference before continuing with the refreshed
bearer token.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Design only - no implementation. This is a DRAFT for discussion and will need another round of re-thinking before it is built.
Proposes per-user OAuth2 Authorization Code (RFC 6749 §4.1 + PKCE) support in OAGW as a new built-in auth plugin (
oauth2_auth_code), a peer of the existing client-credentials plugin. Adds an ADR and five scenarios; changes no code.What's in here
docs/adr-oauth2-authorization-code-auth-plugin.md(new) - the design.scenarios/proxy-api/authentication/positive-9.11..negative-9.15(new) - enrollment, refresh, authorization-required, refresh-rejection, cross-subject isolation.docs/adr-oauth2-client-credentials-auth-plugin.md,scenarios/INDEX.md(edited) - cross-links / index entries.Design principles it commits to
WWW-Authenticate) follow published RFCs. Aligns with the MCP authorization profile and the IETF BFF pattern.ServiceGatewayClientV1is unchanged; re-authorization rides the existingCanonicalError/reasonchannel. The consumer surface is "callproxy, handle one typed error."This is a first coherent cut, not a settled design. See the ADR's Open Questions section. Known blockers:
state-routed callbacks. Unresolved.statevariant - pre-registered client + self-contained encryptedstatewould remove the pending store and multi-instance constraint entirely. Evaluate first.Secrets:Write); not part of this design.toolkit-authadditions - authorization-code primitives named but not designed in detail.WWW-Authenticateparameters still to be pinned and round-trip-tested.Please review the direction and trade-offs rather than treating this as implementation-ready. Follow-up ADRs likely for (1)/(2)/(3).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation