Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

duzumaki
Copy link
Contributor

@duzumaki duzumaki commented Nov 16, 2024

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

@duzumaki duzumaki marked this pull request as draft November 16, 2024 16:09
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch 5 times, most recently from d7824bb to 48ea5d6 Compare November 16, 2024 16:25
@duzumaki duzumaki changed the title Add DeviceCodeGrant type for device code flow(rfc8628) section 3.4 & 5.5 Add DeviceCodeGrant type for device code flow(rfc8628) section 3.4 & 3.5 Nov 16, 2024
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch 2 times, most recently from d585a81 to f5f2266 Compare November 16, 2024 16:35
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.
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch from f5f2266 to 07e6918 Compare November 16, 2024 16:38
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
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch from 07e6918 to 27396f0 Compare November 16, 2024 16:47
@duzumaki duzumaki marked this pull request as ready for review November 16, 2024 16:48
@duzumaki
Copy link
Contributor Author

#626

@auvipy auvipy self-requested a review November 17, 2024 11:47
Copy link
Contributor

@auvipy auvipy left a 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 (
Copy link
Contributor

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):
Copy link
Contributor

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.

@duzumaki
Copy link
Contributor Author

duzumaki commented Nov 17, 2024

@auvipy

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

@auvipy
Copy link
Contributor

auvipy commented Nov 17, 2024

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).

some examples would be nice

@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch 3 times, most recently from 47c3e15 to d502cc2 Compare November 18, 2024 12:33
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch 3 times, most recently from 142a4ae to 75c42f3 Compare November 18, 2024 12:40
@duzumaki
Copy link
Contributor Author

duzumaki commented Nov 18, 2024

@auvipy added a pseudocode implementation.

Do let me know if anything doesn't make sense or may need a change

@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch from 75c42f3 to 980db40 Compare November 18, 2024 13:03
@duzumaki duzumaki requested a review from auvipy November 18, 2024 13:24
@duzumaki duzumaki force-pushed the device_grant_for_section_34_35 branch from 980db40 to f5f6f90 Compare November 18, 2024 13:34
@duzumaki
Copy link
Contributor Author

duzumaki commented Nov 25, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants