Support authz code flow for oauth2 in OAGW.#4224
Conversation
📝 WalkthroughWalkthroughChangesThe pull request adds credential-store mutation APIs and runtime support, then builds interactive per-user OAuth authorization-code enrollment across the toolkit, OAGW REST API, credstore-backed enrollment service, and OAuth authentication plugin. Credential storage and OAuth integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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/credstore/plugins/static-credstore-plugin/src/domain/service.rs`:
- Around line 308-338: Update Service::delete so it never removes entries from
store.global_secrets for ordinary tenant callers; retain the existing private,
tenant, and shared scoped deletion behavior, and make a missed scoped lookup
return without mutating global configuration. If administrative authorization
already exists, only permit global deletion through that explicit authorization
path.
In `@gears/system/oagw/oagw-sdk/src/oauth.rs`:
- Around line 47-54: Update the OAuth status construction in enrollment.rs so
OAuthConnectionStatus.connected is true only when the stored access token is
unexpired or a refresh token is present. Preserve expires_at_unix, and ensure
expired records without refresh capability report connected: false.
- Around line 38-45: Update CompleteOAuthAuthorizationRequest so its Debug
output cannot expose the authorization state or code: remove the derived Debug
implementation or provide a manual redacting implementation. Preserve Clone and
ensure both sensitive fields are replaced with non-sensitive placeholders in any
retained debug representation.
In `@gears/system/oagw/oagw/src/gear.rs`:
- Around line 161-164: Update the OAuthEnrollmentServiceImpl::new construction
in the interactive OAuth enrollment service to pass the configured effective
HttpClientConfig used by the token HTTP policy, rather than letting enrollment
restore its HTTPS-only default. Ensure OAuth discovery, registration, and code
exchange use the same allow_http_upstream setting as the data plane.
In `@gears/system/oagw/oagw/src/infra/oauth/enrollment.rs`:
- Around line 291-302: The token expiry calculation must safely handle
server-controlled expires_in values in both OAuth persistence paths. In
gears/system/oagw/oagw/src/infra/oauth/enrollment.rs lines 291-302, update the
OAuthTokenRecord construction to convert expires_in with an upper bound and use
saturating_add with now_unix(); apply the same change in
gears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth.rs lines 208-220
when persisting refreshed tokens.
- Around line 332-355: The OAuth status method should report disconnected when
an expired token record has no refresh token. In
gears/system/oagw/oagw/src/infra/oauth/enrollment.rs:332-355, update status to
evaluate expiry and refresh-token availability consistently with request
authentication; preserve connected status for usable future-expiry records. In
gears/system/oagw/oagw/src/infra/oauth/enrollment_tests.rs:360-382, cover a
future-expiry connected record and add an expired, non-refreshable record
expecting disconnected.
- Around line 46-61: Update PendingAuthorization to persist an expires_at_unix
timestamp, and assign it when creating records in the authorization begin flow
using the intended short-lived TTL. In load_pending, validate the timestamp
immediately after deserialization; delete the stored record and return no
pending authorization when it is expired, while preserving the existing path for
valid records.
In `@gears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth.rs`:
- Around line 194-233: Update the refresh path around the authentication method
and its self.refresh call to serialize refreshes using a lock keyed by the
subject and secret_ref. Acquire the lock before refreshing, then reload the
credential record and recheck its expiry so a concurrent refresh can be reused;
only refresh and persist when it remains expired. Prevent an
AuthorizationRequired cleanup from deleting a newer record by using
compare-and-set/delete semantics or verifying the record still matches the stale
version before deletion.
In `@libs/toolkit-auth/Cargo.toml`:
- Line 44: Add the required cf-studio-path configuration in the toolkit-auth
Cargo manifest, setting it to ".cf-studio" alongside the existing workspace
dependency configuration.
In `@libs/toolkit-auth/src/oauth2/authcode.rs`:
- Around line 317-339: Update build_authorize_url to validate the parsed
authorization endpoint scheme after Url::parse, accepting HTTPS and only the
explicitly permitted HTTP development exception; return TokenError::ConfigError
for all other schemes before appending query parameters or returning the URL.
- Around line 119-125: Update discover_authorization_server to validate the
fetched AuthorizationServerMetadata.issuer against the requested issuer with
exact equality before returning success; return an appropriate TokenError on
mismatch, while preserving the existing fetch and success path when the values
match.
- Around line 103-110: Update both OAuth discovery paths, including
discover_protected_resource, to insert the well-known segment immediately after
the URL authority while preserving the resource or issuer path. Do not use
origin_of(resource_url) as the base, since it removes path-scoped identifiers;
construct each endpoint from the original URL’s authority and path so distinct
paths produce distinct discovery URLs.
🪄 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: d2f08ddc-bef0-4787-acae-5d76cd3146fe
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
gears/credstore/credstore-sdk/src/api.rsgears/credstore/credstore-sdk/src/plugin_api.rsgears/credstore/credstore/src/domain/local_client.rsgears/credstore/credstore/src/domain/service.rsgears/credstore/credstore/src/domain/test_support.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/system/oagw/oagw-sdk/src/api.rsgears/system/oagw/oagw-sdk/src/lib.rsgears/system/oagw/oagw-sdk/src/oauth.rsgears/system/oagw/oagw/src/api/rest/dto.rsgears/system/oagw/oagw/src/api/rest/handlers/mod.rsgears/system/oagw/oagw/src/api/rest/handlers/oauth.rsgears/system/oagw/oagw/src/api/rest/routes/mod.rsgears/system/oagw/oagw/src/api/rest/routes/oauth.rsgears/system/oagw/oagw/src/domain/gts_helpers.rsgears/system/oagw/oagw/src/domain/plugin/mod.rsgears/system/oagw/oagw/src/domain/services/client.rsgears/system/oagw/oagw/src/domain/services/mod.rsgears/system/oagw/oagw/src/domain/test_support.rsgears/system/oagw/oagw/src/gear.rsgears/system/oagw/oagw/src/infra/mod.rsgears/system/oagw/oagw/src/infra/oauth/enrollment.rsgears/system/oagw/oagw/src/infra/oauth/enrollment_tests.rsgears/system/oagw/oagw/src/infra/oauth/mod.rsgears/system/oagw/oagw/src/infra/plugin/apikey_auth.rsgears/system/oagw/oagw/src/infra/plugin/mod.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth_tests.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsgears/system/oagw/oagw/src/infra/plugin/registry.rsgears/system/oagw/oagw/src/infra/proxy/service.rslibs/toolkit-auth/Cargo.tomllibs/toolkit-auth/src/oauth2/authcode.rslibs/toolkit-auth/src/oauth2/error.rslibs/toolkit-auth/src/oauth2/mod.rs
| let record = self | ||
| .load_record(ctx, &secret_ref) | ||
| .await? | ||
| .ok_or_else(|| PluginError::AuthorizationRequired(resource.clone()))?; | ||
|
|
||
| let margin = i64::try_from(self.refresh_margin.as_secs()).unwrap_or(60); | ||
| if record.expires_at_unix > now_unix() + margin { | ||
| inject_bearer(ctx, &record.access_token); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Expired (or near-expiry): attempt a refresh. | ||
| match self.refresh(&record).await { | ||
| Ok(tokens) => { | ||
| let refreshed = OAuthTokenRecord { | ||
| client_id: record.client_id.clone(), | ||
| client_secret: record.client_secret.clone(), | ||
| token_endpoint: record.token_endpoint.clone(), | ||
| access_token: tokens.access_token.expose().to_owned(), | ||
| refresh_token: tokens | ||
| .refresh_token | ||
| .as_ref() | ||
| .map(|s| s.expose().to_owned()) | ||
| .or_else(|| record.refresh_token.clone()), | ||
| expires_at_unix: now_unix() | ||
| + i64::try_from(tokens.expires_in.as_secs()).unwrap_or(0), | ||
| scope: tokens.scope.clone().or_else(|| record.scope.clone()), | ||
| }; | ||
| self.persist(ctx, &secret_ref, &refreshed).await?; | ||
| inject_bearer(ctx, &refreshed.access_token); | ||
| Ok(()) | ||
| } | ||
| Err(PluginError::AuthorizationRequired(_)) => { | ||
| // Refresh rejected or unavailable: drop the stale record and | ||
| // require the user to re-authorize. | ||
| let _ = self | ||
| .credstore | ||
| .delete(&ctx.security_context, &secret_ref) | ||
| .await; | ||
| Err(PluginError::AuthorizationRequired(resource)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize refreshes per subject and token reference.
Concurrent requests can load the same expired record and refresh simultaneously. If one stores rotated credentials while another receives invalid_grant, Lines 229-232 delete the newly valid record. Guard refresh with a keyed lock and reload the record after acquiring it; ideally use compare-and-set/delete semantics.
🤖 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/oagw/src/infra/plugin/oauth2_auth_code_auth.rs` around
lines 194 - 233, Update the refresh path around the authentication method and
its self.refresh call to serialize refreshes using a lock keyed by the subject
and secret_ref. Acquire the lock before refreshing, then reload the credential
record and recheck its expiry so a concurrent refresh can be reused; only
refresh and persist when it remains expired. Prevent an AuthorizationRequired
cleanup from deleting a newer record by using compare-and-set/delete semantics
or verifying the record still matches the stale version before deletion.
| let issuer = Url::parse(&issuer_str).map_err(|e| { | ||
| TokenError::InvalidResponse(format!("invalid authorization server URL: {e}")) | ||
| })?; | ||
| discover_authorization_server(client, &issuer).await |
There was a problem hiding this comment.
MEDIUM
discover_from_resource follows the remote-supplied authorization_servers[0] and POSTs the auth code, PKCE verifier and refresh token to the discovered endpoints without validating the returned issuer matches the requested one (RFC 8414 §3.3) or enforcing https.
Malicious protected-resource metadata can redirect the token exchange to attacker-controlled or internal endpoints (OAuth mix-up / SSRF), exfiltrating the code and tokens.
Validate AuthorizationServerMetadata.issuer equals the requested issuer and that all discovered endpoints share that origin and use https before use.
| &http, | ||
| registration_endpoint, | ||
| &client_name, | ||
| std::slice::from_ref(&redirect_uri), |
There was a problem hiding this comment.
MEDIUM
The caller-supplied redirect_uri from BeginOAuthRequest is passed straight into dynamic client registration and the authorize URL with no validation that it is an absolute https URL or an allowlisted callback.
Unvalidated redirect_uri is the canonical authorization-code interception vector; the authorization server delivers the code to whatever destination the request names.
Validate redirect_uri as an absolute https URL against an explicit allowlist (or OAGW's own callback) before registration and authorize-URL construction.
| /// | ||
| /// Self-contained so the plugin needs only the credstore `token_ref` in its | ||
| /// binding config; the authorization-server coordinates travel with the record. | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] |
There was a problem hiding this comment.
MEDIUM
OAuthTokenRecord derives Debug while holding plaintext access_token, refresh_token and client_secret as String.
A single debug log of this record leaks live bearer and refresh tokens, contradicting the module's own claim that secret material is never logged.
Wrap the token fields in SecretString or implement a redacting Debug so tokens and client secrets cannot appear in formatted output.
| /// | ||
| /// Returns [`TokenError::ConfigError`] if `authorization_endpoint` is not a | ||
| /// valid URL. | ||
| pub fn build_authorize_url( |
There was a problem hiding this comment.
MEDIUM
build_authorize_url has no unit test asserting the emitted query params (response_type=code, state, code_challenge, code_challenge_method=S256, scope join) or its ConfigError path.
This function builds the security-critical browser authorization request; wrong/missing state or code_challenge params would not be caught since no test inspects the produced URL.
Add tests parsing the returned Url and asserting each expected query pair (including code_challenge_method=S256 and space-joined scopes), plus an Err case for a malformed endpoint.
| .begin( | ||
| &test_ctx(), | ||
| Uuid::nil(), | ||
| vec!["read".to_owned()], |
There was a problem hiding this comment.
MEDIUM
The begin tests only pass an already-supported scope or an empty list, so the scope-intersection filter in begin (drops scopes not in scopes_supported) is never exercised and the authorize-URL scope param is never asserted.
Scope intersection is a security control (prevents requesting unadvertised/over-broad scopes); with only supported/empty inputs the filter could be removed and every test would still pass.
Add a begin test requesting a mix of supported and unsupported scopes (e.g. ["read","admin"]) and assert authorization_url contains scope=read and not admin.
| server.mock(|when, then| { | ||
| when.method(POST) | ||
| .path("/token") | ||
| .body_includes("grant_type=authorization_code") |
There was a problem hiding this comment.
MEDIUM
The complete-flow token mock matches only grant_type and code, so the PKCE code_verifier that begin generated is never asserted to be sent on the exchange.
PKCE binding is only effective if the verifier is actually transmitted on code exchange; the test would still pass if code_verifier were dropped from the request.
Add .body_includes("code_verifier=") (and ideally redirect_uri) to the /token mock matcher so the exchange is bound to the PKCE verifier.
| let mut ctx = ctx_with_token_ref(); | ||
|
|
||
| plugin.authenticate(&mut ctx).await.unwrap(); | ||
| assert_eq!(ctx.headers.get("authorization").unwrap(), "Bearer at-new"); |
There was a problem hiding this comment.
MEDIUM
expired_with_refresh_injects_new_bearer uses MockCredStoreClient whose put/delete are no-ops, so it only asserts the injected header and never verifies the rotated record (refresh_token=rt-new) is persisted back.
Refresh-token rotation persistence is security-critical (the old refresh token must be replaced); with a no-op store the persist() call could be deleted and this test would still pass.
Use a stateful credstore mock and assert the stored record contains access_token=at-new and refresh_token=rt-new after authenticate.
| scopes: Vec<String>, | ||
| } | ||
|
|
||
| fn now_unix() -> i64 { |
There was a problem hiding this comment.
LOW
now_unix() is duplicated verbatim in both infra/oauth/enrollment.rs and infra/plugin/oauth2_auth_code_auth.rs.
The same time-conversion logic (including i64::try_from saturation) is implemented twice, so a change must be made in two places.
Extract now_unix() into a single shared helper and import it from both call sites.
| /// Begin an interactive OAuth authorization-code flow for an upstream on | ||
| /// behalf of the calling user (discovery + dynamic client registration + | ||
| /// PKCE), returning the browser authorization URL and CSRF state. | ||
| async fn begin_oauth_authorization( |
There was a problem hiding this comment.
That 100% shall not be a part of the api.
Signed-off-by: ga <7kozlik+github@gmail.com>
ed34966 to
30e05f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gears/system/oagw/oagw/src/infra/plugin/registry.rs (1)
69-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid double instantiation of the plugin.
Since
OAuth2AuthCodeAuthPlugin::with_http_configis an associated function (unlikeOAuth2ClientCredAuthPlugin::with_http_configwhich takesmut self), the current logic instantiates the plugin, immediately discards it iftoken_http_configis provided, and then instantiates it again. You can simplify this and avoid the redundant allocation with anif-elseexpression.♻️ Proposed refactor
- let mut auth_code_plugin = OAuth2AuthCodeAuthPlugin::new(credstore.clone()); - if let Some(ref cfg) = token_http_config { - auth_code_plugin = - OAuth2AuthCodeAuthPlugin::with_http_config(credstore.clone(), cfg.clone()); - } + let auth_code_plugin = if let Some(ref cfg) = token_http_config { + OAuth2AuthCodeAuthPlugin::with_http_config(credstore.clone(), cfg.clone()) + } else { + OAuth2AuthCodeAuthPlugin::new(credstore.clone()) + };🤖 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/oagw/src/infra/plugin/registry.rs` around lines 69 - 73, Update the OAuth2AuthCodeAuthPlugin initialization to use an if-else expression based on token_http_config, constructing it exactly once via either new or with_http_config; remove the initial mutable instantiation and reassignment.
🤖 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.
Nitpick comments:
In `@gears/system/oagw/oagw/src/infra/plugin/registry.rs`:
- Around line 69-73: Update the OAuth2AuthCodeAuthPlugin initialization to use
an if-else expression based on token_http_config, constructing it exactly once
via either new or with_http_config; remove the initial mutable instantiation and
reassignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd05707e-c14f-4924-9906-b6cec99a511c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
gears/credstore/credstore-sdk/src/api.rsgears/credstore/credstore-sdk/src/plugin_api.rsgears/credstore/credstore/src/domain/local_client.rsgears/credstore/credstore/src/domain/service.rsgears/credstore/credstore/src/domain/test_support.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client.rsgears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service.rsgears/credstore/plugins/static-credstore-plugin/src/domain/service_tests.rsgears/system/oagw/oagw-sdk/src/api.rsgears/system/oagw/oagw-sdk/src/lib.rsgears/system/oagw/oagw-sdk/src/oauth.rsgears/system/oagw/oagw/src/api/rest/dto.rsgears/system/oagw/oagw/src/api/rest/handlers/mod.rsgears/system/oagw/oagw/src/api/rest/handlers/oauth.rsgears/system/oagw/oagw/src/api/rest/routes/mod.rsgears/system/oagw/oagw/src/api/rest/routes/oauth.rsgears/system/oagw/oagw/src/domain/gts_helpers.rsgears/system/oagw/oagw/src/domain/plugin/mod.rsgears/system/oagw/oagw/src/domain/services/client.rsgears/system/oagw/oagw/src/domain/services/mod.rsgears/system/oagw/oagw/src/domain/test_support.rsgears/system/oagw/oagw/src/gear.rsgears/system/oagw/oagw/src/infra/mod.rsgears/system/oagw/oagw/src/infra/oauth/enrollment.rsgears/system/oagw/oagw/src/infra/oauth/enrollment_tests.rsgears/system/oagw/oagw/src/infra/oauth/mod.rsgears/system/oagw/oagw/src/infra/plugin/apikey_auth.rsgears/system/oagw/oagw/src/infra/plugin/mod.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth_tests.rsgears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rsgears/system/oagw/oagw/src/infra/plugin/registry.rsgears/system/oagw/oagw/src/infra/proxy/service.rslibs/toolkit-auth/Cargo.tomllibs/toolkit-auth/src/oauth2/authcode.rslibs/toolkit-auth/src/oauth2/error.rslibs/toolkit-auth/src/oauth2/mod.rs
🚧 Files skipped from review as they are similar to previous changes (33)
- gears/system/oagw/oagw/src/infra/oauth/mod.rs
- gears/system/oagw/oagw/src/infra/plugin/mod.rs
- gears/system/oagw/oagw/src/api/rest/handlers/mod.rs
- gears/system/oagw/oagw/src/infra/plugin/apikey_auth.rs
- gears/system/oagw/oagw/src/domain/plugin/mod.rs
- gears/system/oagw/oagw-sdk/src/oauth.rs
- gears/system/oagw/oagw/src/api/rest/routes/oauth.rs
- gears/credstore/credstore-sdk/src/plugin_api.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/client.rs
- gears/system/oagw/oagw-sdk/src/lib.rs
- gears/system/oagw/oagw/src/domain/gts_helpers.rs
- gears/system/oagw/oagw/src/api/rest/dto.rs
- gears/credstore/credstore-sdk/src/api.rs
- libs/toolkit-auth/Cargo.toml
- gears/system/oagw/oagw/src/api/rest/handlers/oauth.rs
- gears/system/oagw/oagw/src/infra/oauth/enrollment_tests.rs
- gears/system/oagw/oagw/src/infra/plugin/oauth2_client_cred_auth.rs
- gears/system/oagw/oagw/src/domain/services/client.rs
- gears/system/oagw/oagw/src/api/rest/routes/mod.rs
- gears/system/oagw/oagw/src/infra/mod.rs
- gears/credstore/credstore/src/domain/local_client.rs
- gears/system/oagw/oagw-sdk/src/api.rs
- gears/credstore/credstore/src/domain/test_support.rs
- gears/system/oagw/oagw/src/domain/services/mod.rs
- gears/system/oagw/oagw/src/domain/test_support.rs
- gears/system/oagw/oagw/src/gear.rs
- gears/credstore/plugins/static-credstore-plugin/src/domain/client_tests.rs
- gears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth_tests.rs
- gears/credstore/credstore/src/domain/service.rs
- gears/system/oagw/oagw/src/infra/plugin/oauth2_auth_code_auth.rs
- gears/system/oagw/oagw/src/infra/proxy/service.rs
- gears/system/oagw/oagw/src/infra/oauth/enrollment.rs
- libs/toolkit-auth/src/oauth2/authcode.rs
3 participants
Support authz code flow for oauth2 in OAGW.
Summary by CodeRabbit