fix(auth): add an SDK path for pre-registered OAuth clients#994
Conversation
... and exercise it in conformance tests
github-actions
Bot
added
T-core
| mut config: OAuthClientConfig, | ||
| ) -> Result<(), AuthError> { | ||
| let placeholder = self.placeholder().await?; | ||
| if let OAuthState::Unauthorized(mut manager) = std::mem::replace(self, placeholder) { |
There was a problem hiding this comment.
std::mem::replace installs the placeholder before discovery, client configuration, and authorization-URL generation. How should callers retry after a transient discovery/configuration failure without losing the original base URL, stores, and state?
| /// Use this when the client was registered with the authorization server | ||
| /// out of band and already holds a `client_id` (and optionally a | ||
| /// `client_secret`). If `config.scopes` is empty, scopes are selected | ||
| /// from the discovered authorization server metadata. |
There was a problem hiding this comment.
select_scopes prefers scopes from WWW-Authenticate and PRM before ARM, so this is narrower than the implementation.
| /// from the discovered authorization server metadata. | |
| /// using the SDK's normal scope-selection policy. |
| /// dynamic client registration and URL-based client IDs. | ||
| /// | ||
| /// The manager must already have discovered authorization server metadata. | ||
| pub async fn with_preregistered_client( |
There was a problem hiding this comment.
The same impl currently exposes new and for_scope_upgrade. While with_* is valid Rust naming, the three constructors do not seem to communicate an obvious shared convention. I was just thinking we could clean up the API, since v3 allows us to make breaking changes.
| /// out of band and already holds a `client_id` (and optionally a | ||
| /// `client_secret`). If `config.scopes` is empty, scopes are selected | ||
| /// from the discovered authorization server metadata. | ||
| pub async fn start_authorization_with_preregistered_client( |
There was a problem hiding this comment.
Could we add targeted tests using the existing recording HTTP client to verify that this path skips the registration endpoint, handles default and explicit scopes, and etc? Those tests would isolate the new orchestration from the broader conformance harness.
| /// out of band and already holds a `client_id` (and optionally a | ||
| /// `client_secret`). If `config.scopes` is empty, scopes are selected | ||
| /// from the discovered authorization server metadata. | ||
| pub async fn start_authorization_with_preregistered_client( |
There was a problem hiding this comment.
The draft authorization spec recommends the priority pre-registered client information → CIMD → DCR, but the current API exposes these choices through separate entry points. In particular, start_authorization() passes no metadata URL and therefore cannot take the preferred CIMD path, while start_authorization_with_metadata_url() is the method that actually implements CIMD with DCR fallback. This PR understandably extends that pattern with another mechanism-specific method, but we might want to consolidates these paths behind one start_authorization API. This is out of scope and non-blocking for this PR. Let me know what you think, and we can follow up on this separately.
2 participants
... and exercise it in conformance tests
fixes #984
Motivation and Context
The sdk didn't have a high-level path for preregistered oauth clients, so the conformance client implemented it in a more custom way, which didn't pick up the
iss-parameter preservation.How Has This Been Tested?
Ran the conformance client.
Breaking Changes
None
Types of changes
Checklist
Additional context