fix: add missing client_id in device authorization request#310
fix: add missing client_id in device authorization request#310jon4hz wants to merge 1 commit intoDuendeSoftware:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a bug where device authorization requests were missing the required client_id parameter per RFC 8628. The fix adds the client_id to the request parameters, however the implementation has a critical issue that needs to be corrected.
Key changes:
- Adds
client_idparameter to device authorization requests usingAddRequired
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var clone = request.Clone(); | ||
|
|
||
| clone.Parameters.AddOptional(OidcConstants.AuthorizeRequest.Scope, request.Scope); | ||
| clone.Parameters.AddRequired(OidcConstants.AuthorizeRequest.ClientId, request.ClientId); |
There was a problem hiding this comment.
The client_id should only be added to parameters when ClientCredentialStyle is AuthorizationHeader. When ClientCredentialStyle is PostBody, the Prepare() method (called on line 27) already adds the client_id to the request body. Adding it unconditionally will cause the client_id to be duplicated in the request body when using PostBody authentication style.
The correct pattern (as seen in HttpClientPushedAuthorizationExtensions.cs lines 29-34) is:
// client id is always required, and will be added by the call to
// Prepare() for other client credential styles.
if (request.ClientCredentialStyle == ClientCredentialStyle.AuthorizationHeader)
{
clone.Parameters.AddRequired(OidcConstants.AuthorizeRequest.ClientId, request.ClientId);
}This ensures that:
- When using AuthorizationHeader style: credentials go in the Authorization header, but client_id is still sent in the body (per RFC 8628)
- When using PostBody style: Prepare() handles adding both client_id and client_secret to the body
| clone.Parameters.AddRequired(OidcConstants.AuthorizeRequest.ClientId, request.ClientId); | |
| // client id is always required, and will be added by the call to | |
| // Prepare() for other client credential styles. | |
| if (request.ClientCredentialStyle == ClientCredentialStyle.AuthorizationHeader) | |
| { | |
| clone.Parameters.AddRequired(OidcConstants.AuthorizeRequest.ClientId, request.ClientId); | |
| } |
There was a problem hiding this comment.
Seems like copilot is right and the client_id is set if I explicitly set the ClientCredentialStyle to PostBody.
In my opinion, setting the credential style shouldn't be necessary, though, since the device code authentication doesn't require any credentials.
How should I proceed here? I could see a:
if (request.ClientCredentialStyle != ClientCredentialStyle.PostBody) {
clone.Parameters.AddRequired(OidcConstants.AuthorizeRequest.ClientId, request.ClientId);
}What do you think?
What issue does this PR address?
Hi,
This PR fixes a bug in the device authentication. According to rfc 8628, the device authorization request must contain a
client_idand optionally ascope.However, the http client extension you are providing, omits the client id from the request. This causes all device auth requests to fail with: