Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@amanning9 this generally looks right... to help make yourself more confident and assist me in validating could you help me understand how we are using set_oauthlib_user_to_device_request_user upstream? Do all the contexts in which is it used make sense to also set scope? Maybe we should rename it if we're setting both user and scope? Should request.scope even be set or should developer be loading the device grant or token to lookup the scope? |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the device code grant flow where the issued access token was not respecting the scopes requested during device authorization. Instead, tokens were being issued with the DEFAULT_SCOPES regardless of what was requested.
Changes:
- Added logic to set the request scopes from the device grant's stored scope in the
set_oauthlib_user_to_device_request_userfunction - Added a comprehensive test to verify that requested scopes are used instead of default scopes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| oauth2_provider/utils.py | Added scope handling to the set_oauthlib_user_to_device_request_user function to set request.scopes from the device grant's scope field |
| tests/test_device.py | Added test test_device_flow_uses_requested_scope_not_default to verify that specific requested scopes are used in the issued token rather than default scopes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| device: DeviceGrant = get_device_grant_model().objects.get(device_code=request._params["device_code"]) | ||
| request.user = device.user | ||
| request.scopes = device.scope.split() if device.scope else [] |
There was a problem hiding this comment.
The scope splitting should be consistent with other grant types. In oauth2_validators.py line 491, the authorization code grant uses split(" ") with an explicit space delimiter. Consider using device.scope.split(" ") instead of device.scope.split() for consistency, though both will work for space-separated scopes.
| request.scopes = device.scope.split() if device.scope else [] | |
| request.scopes = device.scope.split(" ") if device.scope else [] |
There was a problem hiding this comment.
I don't think this is used consistently across the code-base currently. Lots of places in outh2_provider/models.py split the scopes with a base .split(), which two other places split it with .split(" "). I don't think there is much to choose between them, but happy to swap it to either.
amanning9
force-pushed
the
device-code-obey-scopes
branch
from
265268c to
f5a870a
Compare
|
@amanning9 can you rebase the PR? |
amanning9
force-pushed
the
device-code-obey-scopes
branch
from
f5a870a to
69e000b
Compare
|
Rebased. Apologies for not getting a chance to reply to your earlier comment- I just haven't had time to follow the code path through. The existing behavior is definitely in error, and this PR does fix it- I just haven't had time to follow everything through and work out if is the best way to fix it! - I wonder if any of the authors of the original device code PR might be able to comment on that? |
Description of the Change
The new device code grant does not give the resulting token the scopes that were asked for. This PR fixes that bug and adds a test.
I'm not /completely/ sure that I've fixed it in the correct place- If There is a better way or place to fix the problem please let me know.
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS