fix: configurable oauth scope instead of hardcoded 'openid' scope - #2566
Conversation
Reviewer's GuideThis PR makes the OAuth scope used for client_credentials flows configurable instead of hard-coded to "openid", plumbs the new optional scope through CLI/env configuration, OpenID provider config, and token acquisition/refresh logic, and adds tests ensuring correct propagation and defaults. Sequence diagram for client_credentials flow with configurable scopesequenceDiagram
participant Args as OpenIdTokenProviderConfigArguments
participant Config as OpenIdTokenProviderConfig
participant Provider as OpenIdTokenProvider
participant Client as openid::Client
Args->>Config: try_from_arguments(arguments)
Config-->>Config: set scope Option<String>
Config->>Provider: from_config(config)
Provider-->>Provider: store scope
rect rgb(230,230,250)
note over Provider,Client: Initial client_credentials token
Provider->>Client: request_token_using_client_credentials(scope.as_deref())
Client-->>Provider: TemporalBearerGuard
end
rect rgb(230,250,230)
note over Provider,Client: Refreshing token when refresh_token is present
Provider->>Client: refresh_token(current_token, scope.as_deref())
Client-->>Provider: TemporalBearerGuard
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing the default from always requesting
openidto sending no scope by default alters behavior for existing deployments; consider preserving the previous default (or adding an explicit compatibility switch) so Keycloak-based setups don’t silently lose scopes after upgrade. - Since multiple scopes are encoded as a space-separated string, you might want a small helper or type to construct/validate scopes (e.g., from
Vec<String>), to avoid subtle formatting issues and make multi-scope usage clearer at call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing the default from always requesting `openid` to sending no scope by default alters behavior for existing deployments; consider preserving the previous default (or adding an explicit compatibility switch) so Keycloak-based setups don’t silently lose scopes after upgrade.
- Since multiple scopes are encoded as a space-separated string, you might want a small helper or type to construct/validate scopes (e.g., from `Vec<String>`), to avoid subtle formatting issues and make multi-scope usage clearer at call sites.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| default_value = "false" | ||
| )] | ||
| pub tls_insecure: bool, | ||
| /// OAuth scope(s) to request in the client_credentials token request. |
There was a problem hiding this comment.
I wonder if this is a breaking change ? what happens with existing deployments (and have we tested )
There was a problem hiding this comment.
This PR is essentially building upon #2552, so there should be nothing breaking here. We're bringing back the default of no scope being explicitly requested, with an option to request scopes if necessary. What possible breaking change did you have in mind?
There was a problem hiding this comment.
I guess my request is if we have tested with an existing env which has named scope and if there is any migration path concerns ... if you have none, then I have none ;)
| @@ -243,7 +258,11 @@ impl OpenIdTokenProvider { | |||
| Some(current_token) => { | |||
| log::debug!("Refreshing token ... "); | |||
There was a problem hiding this comment.
just noticed while in the area - I think this should be converted to tracing call (As per CONVENTIONS.md)
rh-jfuller
left a comment
There was a problem hiding this comment.
as long as this change has been tested with appropriate existing deployments LGTM
|
Successfully created backport PR for |
AWS Cognito rejects client_credential grant requests containing system-reserved scopes such as
openid, therefore we can't force the scope in the general case. From previous testing, theopenidscope was required for Keycloak auth with EI, hence the original introduction of the hard-codedopenidscope. This PR introduces the option to set a scope if necessary, leaving it unset by default (some cognito & keycloak setups may require custom scopes to be requested that cannot be set as default scopes for whatever reasons).From the AWS docs for machine-to-machine authorization "You can authorize only custom scopes from resource servers in access tokens for client credentials grants."
Fixes TC-5584
Summary by Sourcery
Make OAuth scopes configurable for OpenID client_credentials flows instead of hardcoding the
openidscope, with sensible defaults and propagation through configuration and provider code.New Features:
Enhancements:
Tests: