Add device authorization grant (device code flow - rfc 8628) #1539

duzumaki · 2025-01-07T15:57:37Z

Note to reviewers: I've made this a "commit by commit" pr which means it's easier to review the pr if you go commit by commit rather than look at all files changed at once

Fixes #962

follow up from
oauthlib/oauthlib#881
&
oauthlib/oauthlib#889

Description of the Change

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
CHANGELOG.md updated (only for user relevant changes)
author name in AUTHORS

dopry

This looks excellent, Only one thing grabbed my attention in my cursory code review, the type of the request parameter. Take a moment to double check that type. I've been bitten by OAuthLib's recasting of Request on a number of occasions. I hope to get time to more thoroughly review this by the end of the week

oauth2_provider/oauth2_backends.py

danlamanna

This looks awesome! I left some comments even though I'm not a maintainer, I'm just an excited downstream user :). If you're too busy to address any of my feedback let me know, I'd be happy to spend some time on it.

I got this up and running locally and was able to complete the authorization flow. Other than the comments I left inline, I have a few thoughts.

Were you planning on adding a default view and template to complete the flow, similar to the way other grant types operate? Obviously the device flow user interaction can be highly customized, but I think a simple view could provide a decent out of the box experience. This was the code I wrote on my application to test this end-to-end:

from oauthlib.oauth2.rfc8628.errors import (
    AccessDenied,
    ExpiredTokenError,
)
from oauth2_provider.models import get_device_model
from django import forms


class DeviceForm(forms.Form):
    user_code = forms.CharField(required=True)


@login_required
def oauth_device_authenticate(request):
    form = DeviceForm(request.POST or None)

    if request.method == "POST" and form.is_valid():
        user_code = form.cleaned_data["user_code"]
        device = get_device_model().objects.filter(user_code=user_code).first()

        if device is None:
            form.add_error("user_code", "Incorrect user code")
        else:
            if timezone.now() > device.expires:
                device.status = device.EXPIRED
                device.save(update_fields=["status"])
                raise ExpiredTokenError

            if device.status in (device.DENIED, device.AUTHORIZED):
                raise AccessDenied

            if device.user_code == user_code:
                device.status = device.AUTHORIZED
                device.save(update_fields=["status"])
                return HttpResponseRedirect(reverse("oauth-device-authenticate-success"))

    return render(request, "device_authenticate.html", {"form": form})


@login_required
def oauth_device_authenticate_success(request):
    return render(request, "device_authenticate_success.html")

Likewise, are downstreams expected to implement their own /token endpoint?
Should DOT be a little bit more opinionated about how to generate things like user_code? There seems to be a good bit in the RFC (6.1) about best practices that we could encode for downstreams: e.g. using a shorter code with enough entropy that has readable characters and is compared case-insensitively.

Thanks again for all this work :)

oauth2_provider/models.py

oauth2_provider/views/mixins.py

oauth2_provider/views/device.py

oauth2_provider/models.py

docs/tutorial/tutorial_06.rst

danlamanna · 2025-01-08T22:16:38Z

oauth2_provider/models.py

+        return self.get(client_id=client_id, device_code=device_code, user_code=user_code)
+
+
+class Device(AbstractDevice):


Likewise maybe DeviceGrant?

It's not the grant, it's the model that represents the device during the flow's session,
this is the device grant

In the context of oauthlib (which doesn't require Django) that is the device grant, yes. But in django-oauth-toolkit this represents the pending flow that gets persisted. This is very similar to DOT's Grant class which also has counterparts in oauthlib like AuthorizationCodeGrant.

I think this very much is the Grant. Note that most of the fields are the same, scope, client_id, expiration, etc.

But in django-oauth-toolkit this represents the pending flow that gets persisted.

This seems to be what I'm saying as well. Because it represents the device session and uses the device grant in that session.

Am I misunderstanding grant in Oauth2? I define it as a single object that is of some type but doesn't have anything to do with state/session

The Grant in DOT is for the authorization code flow. It may have similar columns for its table but that's not the full story with the device here.

Device here would be more akin to the authorization code flow itself than just the grant it uses

While Grant is a static object, Device here is used to track changing state through the flow

I would like to see the language around this resolved a bit... If this is in fact a grant session we should name it as such. I'm not a big fan of the bare Device here. To my thinking the device is just that, the physical device and this isn't strictly a representation of the physical device itself, but the relation between the device, a particular user, and the authorization state of the device relative to the user. Device's can have many users, such as public kiosks, or shared devices in homes, phones/tablets with multiple profile in corporate settings, etc. All that to say, Let's be more specific than device here.

If we look at the other grants.... I think within the scope of DOT, @danlamanna may have the more consistent interpretation.

per oauth2_provider/models.py:302

A Grant instance represents a token with a short lifetime that can be swapped for an access token, as described in :rfc:4.1.2

While I think @duzumaki is correct in a wider context and DOT's current 'AbstractGrant` is too tightly coupled to the AuthoizationCode Grant, and could be further generalized, and possible better named as a GrantSession, it is used to track the sessions details that culminate in the token exchange. The use of a Grant suffix also seems to maintain consistency with the relative relationships of the models in DOT with the grant types in OAuthLib, where DOT's models hold the information returned via the OAuthLib Grant classes. Here I favor consistency over correctness, and would strongly lean toward DeviceGrant in place of Device.

Here I favor consistency over correctness, and would strongly lean toward DeviceGrant in place of Device.

Also agreed that consistency is often just more practical.

I've updated to DeviceGrant (and AbstractDeviceGrant respectively)

oauth2_provider/models.py

duzumaki · 2025-01-10T09:32:52Z

@danlamanna

Were you planning on adding a default view and template to complete the flow, similar to the way other grant types operate? Obviously the device flow user interaction can be highly customized, but I think a simple view could provide a decent out of the box experience. This was the code I wrote on my application to test this end-to-end:

from oauthlib.oauth2.rfc8628.errors import (
    AccessDenied,
    ExpiredTokenError,
)
from oauth2_provider.models import get_device_model
from django import forms


class DeviceForm(forms.Form):
    user_code = forms.CharField(required=True)


@login_required
def oauth_device_authenticate(request):
    form = DeviceForm(request.POST or None)

    if request.method == "POST" and form.is_valid():
        user_code = form.cleaned_data["user_code"]
        device = get_device_model().objects.filter(user_code=user_code).first()

        if device is None:
            form.add_error("user_code", "Incorrect user code")
        else:
            if timezone.now() > device.expires:
                device.status = device.EXPIRED
                device.save(update_fields=["status"])
                raise ExpiredTokenError

            if device.status in (device.DENIED, device.AUTHORIZED):
                raise AccessDenied

            if device.user_code == user_code:
                device.status = device.AUTHORIZED
                device.save(update_fields=["status"])
                return HttpResponseRedirect(reverse("oauth-device-authenticate-success"))

    return render(request, "device_authenticate.html", {"form": form})


@login_required
def oauth_device_authenticate_success(request):
    return render(request, "device_authenticate_success.html")

This code I put in tutotorial_06.rst was a simplified version of how I implemented in my own authserver.
However, with the device view being highly customizable and specific to your own auth server and some implementations even working with open id connect(which is not part of the rfc), I opted to not include it. For example, I have plans on adding extensions similar to mutual TLS to my device flow in my auth server.

However this is up to the maintainers to decide but I'd rather get this merged and we add it later if we deem it important as I also worked on making sure oauthlib can support this grant so I've been working on this for quite some time now to put everything in place(this pr & this). Can always incrementally update django oauth toolkit but I would like to get the core tooling in first

Likewise, are downstreams expected to implement their own /token endpoint?

No , they can if they want but oauth toolkit provides that endpoint. They just need to have a working /token endpoint as a prerequisite

Should DOT be a little bit more opinionated about how to generate things like user_code? There seems to be a good bit in the RFC (6.1) about best practices that we could encode for downstreams: e.g. using a shorter code with enough entropy that has readable characters and is compared case-insensitively.

That's why I updated oauthlib to support the ability to pass in custom user code generator callables if you set the setting I made for it in oauth toolkit. I'm being core RFC focused here first and if anything opinionated needs to be added I think we can add it later, This pr is already chunky as is the way I see it. Nothing stopping us from releasing inceremental updates here instead of one big bang :)

Thanks again for all this work :)

Thank you!

tests/test_device.py

dopry · 2025-01-12T03:22:26Z

@danlamanna thanks for putting it through it's paces and for the code review. We always appreciate extra hands in the community kicking the tires on pull requests.

@duzumaki I haven't had a chance to get into a thorough review yet. It's high on my OSS priority list. It would be nice to have a working implementation in the example idp/rp in tests/app. If you need any help on the rp side there, I'm happy to lend a hand. That will reduce our testing overhead as maintainers. It's a lot to review an OAuth Flow without also having to implement part of it as well, especially as we haven't been as awash in the specification as you seem to have been for a bit. I am partial to the idea of having necessary default views in DOT, I really prefer as much of a batteries included experience for our users. If we give people a half implementation in an initial release it will be a lot of work for a lot of people, then when we add in our own view implementations it'll be an upgrade headache for all of those users. If we can deliver a view that adheres to best practice with reasonable defaults which users can override I would much prefer that.

duzumaki · 2025-01-12T12:18:36Z

@dopry

makes sense. i'll port some implementation i had in my own custom auth server over to this pr

n2ygk · 2025-01-14T17:05:58Z

@duzumaki It looks like you may be battling with pre-commit which is fixing your code formatting after push. Do you have pre-commit installed locally? See https://django-oauth-toolkit.readthedocs.io/en/latest/contributing.html#code-style

duzumaki · 2025-01-14T17:23:50Z

@n2ygk The pushes aren't because of the pre-commit. I use rebase so I'm fixing the history so it's easier to review

duzumaki · 2025-01-14T17:50:32Z

@n2ygk @dopry @danlamanna just added new commits that add everything needed to test the device flow end to end + a test that tests the whole flow touching all of the relevant views.

again, reviewing the commits, commit by commit, will help versus looking at all files changed at once(during your first pass review anyway)

@danlamanna I haven't addressed all of your comments yet. I just want to ensure we all agree on the complete set of views I've just added first then I'll go back to handle the smaller stuff you commented on

@dopry
In terms of the idp test app please see the updated doc here that provides instructions on starting the idp and shows everything step by step. I opted out from using the rp part because it's easier to just copy and paste the curl command I put in it and run it against the local server and the device flow is out of band anyway so in a sense doing it from your terminal is the "device" so it's a closer experience

all checks look good locally too

dopry · 2025-01-14T21:48:48Z

I'll take a look later this evening

A public device code grant doesn't have a client_secret to check

It needs handled differently depending on the device grant type or not it also needs to be rate limited to adhrere to the polling section in the spec so a device can't spam the token endpoint

This creates a user friendly but still high entropy user code to be used in the device flow

Tests the device flow end to end

Older version doesn't work with newer version of python

per https://github.com/cristiprg

dopry · 2025-05-19T16:25:51Z

oauth2_provider/models.py

+        return self.get(client_id=client_id, device_code=device_code, user_code=user_code)
+
+
+class Device(AbstractDevice):


I would like to see the language around this resolved a bit... If this is in fact a grant session we should name it as such. I'm not a big fan of the bare Device here. To my thinking the device is just that, the physical device and this isn't strictly a representation of the physical device itself, but the relation between the device, a particular user, and the authorization state of the device relative to the user. Device's can have many users, such as public kiosks, or shared devices in homes, phones/tablets with multiple profile in corporate settings, etc. All that to say, Let's be more specific than device here.

If we look at the other grants.... I think within the scope of DOT, @danlamanna may have the more consistent interpretation.

per oauth2_provider/models.py:302

A Grant instance represents a token with a short lifetime that can be swapped for an access token, as described in :rfc:4.1.2

While I think @duzumaki is correct in a wider context and DOT's current 'AbstractGrant` is too tightly coupled to the AuthoizationCode Grant, and could be further generalized, and possible better named as a GrantSession, it is used to track the sessions details that culminate in the token exchange. The use of a Grant suffix also seems to maintain consistency with the relative relationships of the models in DOT with the grant types in OAuthLib, where DOT's models hold the information returned via the OAuthLib Grant classes. Here I favor consistency over correctness, and would strongly lean toward DeviceGrant in place of Device.

dopry · 2025-05-19T17:12:49Z

oauth2_provider/views/device.py

+        return render(request, "oauth2_provider/device/user_code.html", {"form": form})
+
+    user_code: str = form.cleaned_data["user_code"]
+    device: Device = get_device_model().objects.get(user_code=user_code)


I would lean toward the User Code recommendations in rfc8628.

dopry · 2025-05-19T17:21:02Z

oauth2_provider/views/device.py

+# device login we're making the decision here to require being logged in
+# up front
+@login_required
+def device_user_code_view(request):


I think requiring that the user is authenticated is a good start. If a user's credentials are already compromised I think they have bigger problems than brute force device code attacks that are limited to the expiration time of the user code. That said it would be a good thing to highlight in the docs. As a follow on task it would be nice to have retry limits, but I wouldn't let that block this PR.

dopry · 2025-05-19T17:24:10Z

oauth2_provider/views/device.py

+        return render(request, "oauth2_provider/device/user_code.html", {"form": form})
+
+    user_code: str = form.cleaned_data["user_code"]
+    device: Device = get_device_model().objects.get(user_code=user_code)


I agree. From a UX perspective it is important that we return an appropriate error so clients can properly inform users about how to proceed. This is a form handler, so in theory wouldn't we catch this in form validation? Maybe we should render the form with an error here.

dopry · 2025-05-19T17:32:40Z

oauth2_provider/views/device.py

+def device_confirm_view(request: http.HttpRequest, device_code: str):
+    device: Device = get_device_model().objects.get(device_code=device_code)
+
+    if device.status in (device.AUTHORIZED, device.DENIED):


Isn't this a end user facing view, specifically it handles the 'approve' by the user on their browser. By redirecting with the device code, rather than the user code aren't we exposing the device code to the user? I feel like we're supposed to be avoiding that. We should be passing along the id of the grant (separate from the device and user code or user code at this step I believe, please correct me if I'm wrong.

per 3.3

The "device_code" is not intended for the end user
directly; thus, it should not be displayed during the interaction to
avoid confusing the end user.

showing it in the URI is displaying it in my book.

dopry · 2025-05-19T17:42:36Z

oauth2_provider/views/device.py

+        raise ExpiredTokenError
+
+    # User of device has already made their decision for this device
+    if device.status in (device.DENIED, device.AUTHORIZED):


Actually if this is the user facing confirmation view, I'm feel like these should be errors displayed on the form. I'm not sure an access denied is an appropriate response to an authenticated user with a valid user code at this juncture. The specification seems to leave the UX pretty open a this step

The authorization server should then inform the user about the action they are undertaking and ask them to approve or deny the request. Once the user interaction is complete, the server instructs the user to return to their device.

I think we should be redirecting them back to their device, not showing an error. We may even want to show them the status of the grant since they are authenticated after all.

dopry · 2025-05-19T17:44:06Z

pyproject.toml

@@ -36,7 +36,7 @@ classifiers = [
 dependencies = [
 	"django >= 4.2",
 	"requests >= 2.13.0",
-	"oauthlib >= 3.2.2",
+	"oauthlib @ git+https://github.com/oauthlib/oauthlib.git@master",


Do we have an official oauthlib release with this functionality yet?

Not yet unfortunately, but I asked here oauthlib/oauthlib#897.

codecov · 2025-05-19T18:03:24Z

Codecov Report

Attention: Patch coverage is 93.26425% with 13 lines in your changes missing coverage. Please review.

Project coverage is 97.04%. Comparing base (e34819a) to head (c61a4eb).
Report is 49 commits behind head on master.

Files with missing lines	Patch %	Lines
oauth2_provider/utils.py	58.33%	5 Missing ⚠️
oauth2_provider/models.py	95.00%	3 Missing ⚠️
oauth2_provider/views/device.py	95.58%	3 Missing ⚠️
oauth2_provider/oauth2_validators.py	75.00%	1 Missing ⚠️
oauth2_provider/views/base.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1539      +/-   ##
==========================================
- Coverage   97.41%   97.04%   -0.37%     
==========================================
  Files          34       35       +1     
  Lines        2164     2401     +237     
==========================================
+ Hits         2108     2330     +222     
- Misses         56       71      +15

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cristiprg · 2025-05-20T21:52:08Z

Thanks @dopry for the review! Hope you're well ✨ I will pick up the work for this PR tomorrow 💪

cristiprg · 2025-06-06T16:12:54Z

@dopry ready for a re-review 🙏 I think I addressed all comments, and also increased test coverage

In this commit I've addressed a few issues raised in comments about the flow. * Always return informative errors to user-facing views. If the device is not in the expected state, the errors are returned in the form, instead of raising exceptions. * Always return JSON response to device-facing view (aka TokenView). If the device is not in the expected state, the errors are returned according to the RFC. * Never involve device_code in frontend. The redirect to device-confirm view now takes client_id and user_code arguments instead of device_code * Increased test coverage. Added tests for expected error cases handled in the code

Perferred to use the Grant suffix in order to keep the naming consistent with other grant models.

duzumaki force-pushed the add-device-flow branch 5 times, most recently from d94410c to acc1753 Compare January 7, 2025 16:11

duzumaki mentioned this pull request Jan 7, 2025

Device Authorization Grant #962

Open

dopry reviewed Jan 7, 2025

View reviewed changes

oauth2_provider/oauth2_backends.py Show resolved Hide resolved

danlamanna reviewed Jan 9, 2025

View reviewed changes

dopry reviewed Jan 12, 2025

View reviewed changes

tests/test_device.py Outdated Show resolved Hide resolved

duzumaki force-pushed the add-device-flow branch 7 times, most recently from cd79c50 to 04f6ccc Compare January 14, 2025 17:02

duzumaki force-pushed the add-device-flow branch from 82ddc34 to b47408e Compare January 14, 2025 17:22

duzumaki force-pushed the add-device-flow branch 4 times, most recently from d73dcc0 to a9eb10e Compare January 14, 2025 17:46

duzumaki requested review from dopry and danlamanna January 14, 2025 17:50

duzumaki added 17 commits May 19, 2025 11:44

Add device poll change test

46eed3b

Add incorrect client id test

eac4329

Update authors and changelog

e83a3cd

Add the device user code form

bb6deec

Add approve deny form

01fe93c

Update request body validator

6306365

A public device code grant doesn't have a client_secret to check

Update device imports

1a027d9

Update token endpoint

1f1e9b0

It needs handled differently depending on the device grant type or not it also needs to be rate limited to adhrere to the polling section in the spec so a device can't spam the token endpoint

Prep the tests: Create user_code_generator util

0918cf4

This creates a user friendly but still high entropy user code to be used in the device flow

Add user code generator to main settings

377d12e

Add tests to test the whole flow

5ff24c8

Tests the device flow end to end

Update idp requirements

216ec65

Add tutotial doc

2e64efb

Update idp requirements

9de753e

Older version doesn't work with newer version of python

Ensure the django user sets the oauthlib request user

81b7710

Ensure device token errors are returning 400

bfd67a3

Add device code to refresh token mapping

c1cb26f

dopry force-pushed the add-device-flow branch from 5039617 to c1cb26f Compare May 19, 2025 15:44

chore: address documentation notes

223f991

per https://github.com/cristiprg

dopry requested changes May 19, 2025

View reviewed changes

cristiprg mentioned this pull request May 21, 2025

[QUESTION] Why releases have stopped? oauthlib/oauthlib#897

Open

chore: add more details to the tutorial docs

6dda42b

cristiprg force-pushed the add-device-flow branch from fcb8739 to 336b6c6 Compare June 6, 2025 16:07

cristiprg force-pushed the add-device-flow branch from 336b6c6 to 7a95bec Compare June 6, 2025 18:59

cristiprg added 2 commits June 6, 2025 20:09

Use term DeviceGrant over Device

c61a4eb

Perferred to use the Grant suffix in order to keep the naming consistent with other grant models.

cristiprg force-pushed the add-device-flow branch from 7a95bec to c61a4eb Compare June 6, 2025 19:09

		return self.get(client_id=client_id, device_code=device_code, user_code=user_code)


		class Device(AbstractDevice):

Uh oh!

Add device authorization grant (device code flow - rfc 8628) #1539

Are you sure you want to change the base?

Add device authorization grant (device code flow - rfc 8628) #1539

Uh oh!

Conversation

duzumaki commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Checklist

Uh oh!

dopry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danlamanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duzumaki Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duzumaki Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

duzumaki commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dopry commented Jan 12, 2025

Uh oh!

duzumaki commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n2ygk commented Jan 14, 2025

Uh oh!

duzumaki commented Jan 14, 2025

Uh oh!

duzumaki commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dopry commented Jan 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dopry May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

duzumaki commented Jan 7, 2025 •

edited

Loading

duzumaki Jan 10, 2025 •

edited

Loading

duzumaki Jan 24, 2025 •

edited

Loading

duzumaki commented Jan 10, 2025 •

edited

Loading

duzumaki commented Jan 12, 2025 •

edited

Loading

duzumaki commented Jan 14, 2025 •

edited

Loading

dopry May 19, 2025 •

edited

Loading

codecov bot commented May 19, 2025 •

edited

Loading