-
-
Notifications
You must be signed in to change notification settings - Fork 487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DeviceCodeGrant type for device code flow(rfc8628) section 3.4 & 3.5 #889
base: master
Are you sure you want to change the base?
Add DeviceCodeGrant type for device code flow(rfc8628) section 3.4 & 3.5 #889
Conversation
These are from section 3.5 of the rfc https://datatracker.ietf.org/doc/html/rfc8628#section-3.5
d7824bb
to
48ea5d6
Compare
d585a81
to
f5f2266
Compare
In the device flow, the device must poll the token endpoint to check if the user has submitted the user_code. This commit adds that handler to validate the request and return the token.
Any caller of DeviceApplicationServer() needs control of the inerval polling time. This is part of the flow where the device polls the auth server to check in the user has submitted the user_code
Easier to read, easier to derive where the module lives.
This grant type handler exists in both the openid conenct server and the oauth 2.0 server. Why? It sets up for a potentitial open id over device flow implementation. It also alo ensures the access_token and refresh_token still get returned whether openid is enabled or not. device flow isn't a spec that's part of open id but some IDps like microsoft, auth0, okta implement an off-spec version. However, that's not the concern of the pr this commit is apart of but if returning an id_token(openid) is needed down the line that will have to be a seperate issue rasied.
f5f2266
to
07e6918
Compare
The 255 is too high, leads to lines that are too long. I'll get a pr to format the entire repo after this one
07e6918
to
27396f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also there are tons of unrelated changes to this PR
@@ -6,6 +6,11 @@ | |||
for consuming and providing OAuth 2.0 Device Authorization RFC8628. | |||
""" | |||
|
|||
from oauthlib.oauth2.rfc8628.errors import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where and how this errors are being used? can you elaborate please?
""" | ||
|
||
|
||
class AuthorizationPendingError(OAuth2Error): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using 4 different class, can we consider a single class with python Enum? asking as I could not find out the usage of this codes.
Which changes? the Ruff one? That was done because in a previous PR we wanted better formatting but the ruff default is 255 in the repo regarding usage of the errors. It's my understanding of oauthlib that it provides the building blocks needed to build your own auth server. In this case, these are errors that are raised when the device is polling the token endpoint, which is where you'll use the error object from oauthlib and raise it there Where I used it was in a branch in django oauth toolkit and ultimately an auth server I've built. As usage of db models is also involved in determining when these errors are raised (by checking if for example the device flow session is expired and adding a rate limit to the token endpoint as the device is polling the token endpoint which is where you'll raise the slow down error, in your own rate limit logic). Oauthlib doesn't concern itself with where data is persisted But please advise if there's some hook pattern for this in oauthlib somewhere I might have missed to plugin database object usage |
some examples would be nice |
47c3e15
to
d502cc2
Compare
142a4ae
to
75c42f3
Compare
@auvipy added a pseudocode implementation. Do let me know if anything doesn't make sense or may need a change |
75c42f3
to
980db40
Compare
980db40
to
f5f6f90
Compare
@thedrow I think Asif is on parental leave. Is this a pr you could review please? Not sure who else to tag. It doesn't let me request reviews |
Note to maintainers; this pr is best reviewed commit by commit.
Device grant type to support 3.4 & 3.5
This pr introduces the final set of objects needed for someone trying to implement the device flow in their own authorization server (in conjunction with #881)
I have a branch in django-oauth-toolkit that's making use of the code in this pr), which my own custom auth server uses django oauth toolkit to implement a working device code flow
Will open a pr in DOT as well after this is merged and released. Also linking the issue here
In case anyone asks about openid connect on top of the device flow code.
I am aware some identity providers likemicrosoft, okta, auth0 all return id tokens with their device flows.
Keep in mind those are off-spec implementations and openid makes no mention of the device flow. To reduce the scope of this pr I opted to not add openID to the device flow here. I may have a follow up pr that implements it depending on my needs but either way a standard on-spec device code flow should be merged and released first