Fix client authentication for DeviceCodeGrant when getting a token#920
Fix client authentication for DeviceCodeGrant when getting a token#920hekhuisk wants to merge 2 commits into
Conversation
…AuthorizationCodeGrant, DeviceCodeGrant, RefreshTokenGrant, and ResourceOwnerPasswordCredentialsGrant. Add some missing imports to the oauth2 __init__.py
| if self.request_validator.client_authentication_required( | ||
| request | ||
| ) and not self.request_validator.authenticate_client(request): | ||
| raise rfc6749_errors.InvalidClientError(request=request) |
There was a problem hiding this comment.
This already happens inside self.validate_token_request(request) below.
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes client authentication logic by introducing a validate_client_authentication helper in the base grant type and replaces duplicated authentication checks in several grant flows. It also adds missing imports for device‐code endpoints and errors in the top‐level oauth2 package and includes new tests for DeviceCodeGrant authentication.
- Introduce
validate_client_authenticationinGrantTypeBaseand apply it in Authorization Code, Device Code, Refresh Token, and Resource Owner Password flows. - Replace inline client authentication blocks in grant type implementations with the new helper.
- Add tests for public vs. confidential client authentication errors in
DeviceCodeGrant. - Add missing imports for device‐code endpoints and errors in
oauthlib/oauth2/__init__.py.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/oauth2/rfc8628/grant_types/test_device_code.py | Add tests for public and confidential client authentication errors |
| oauthlib/oauth2/rfc8628/grant_types/device_code.py | Replace inline auth logic with validate_client_authentication |
| oauthlib/oauth2/rfc6749/grant_types/resource_owner_password_credentials.py | Use validate_client_authentication instead of inline checks |
| oauthlib/oauth2/rfc6749/grant_types/refresh_token.py | Replace inline auth checks with validate_client_authentication |
| oauthlib/oauth2/rfc6749/grant_types/base.py | Introduce validate_client_authentication helper |
| oauthlib/oauth2/rfc6749/grant_types/authorization_code.py | Invoke validate_client_authentication in token request validation |
| oauthlib/oauth2/init.py | Add missing relative imports for device‐code endpoints and errors |
Comments suppressed due to low confidence (2)
oauthlib/oauth2/rfc6749/grant_types/base.py:178
- [nitpick] The docstring only notes failure of client authentication; consider expanding it to describe behavior for both confidential and public clients and reference relevant RFC sections.
def validate_client_authentication(self, request):
oauthlib/oauth2/rfc6749/grant_types/authorization_code.py:455
- There are no existing tests verifying the client authentication logic for the Authorization Code grant; consider adding tests covering both confidential and public client scenarios.
self.validate_client_authentication(request)
| raise errors.InvalidClientError(request=request) | ||
|
|
||
| # Handles public clients | ||
| elif not self.request_validator.authenticate_client_id(request.client_id, request): |
There was a problem hiding this comment.
validate_client_authentication invokes authenticate_client_id without checking if request.client_id is set; if client_id is None, this may raise an unexpected exception. Consider adding an explicit guard or raising an InvalidClientError when request.client_id is missing before calling authenticate_client_id.
There was a problem hiding this comment.
I do not believe this to be a valid issue.
The elif branch is reached only when client_authentication_required(request) returns False — i.e. the deployment treats this as a public client. Then it calls authenticate_client_id(None, request).
authenticate_client_id is documented (request_validator.py:90) to return True/False. For a conformant validator, None → lookup finds nothing → returns False → InvalidClientError. That is the correct, spec-compliant outcome. Per RFC 6749 §3.2.1, a public client at the token endpoint MUST send client_id, so None is unambiguously invalid — and the current code already produces the right invalid_client response.
So there is no functional bug for any validator that honors its contract.
Adds a helper method for validating client authentication and use it in
AuthorizationCodeGrant,DeviceCodeGrant,RefreshTokenGrant, andResourceOwnerPasswordCredentialsGrant.Putting the helper method in
DeviceCodeGrantalso fixes an issue where the client being public/confidential was not being taken into consideration.Add some missing imports to the oauth2 init.py that I noticed weren't there when implementing changes in my own code.
Resolves #919