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

[duzumaki]

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

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1208402026227156

[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

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

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

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

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

[danlamanna]

Should there perhaps be some way of configuring verification_uri_complete similar to verification_uri? That way clients wanting to use a QR code won't have to do their own URI assembly.

[duzumaki]

Yeah can do

[duzumaki]

A change in oauthlib is required for this. 1 liner here

[dopry]

Is this a PR pending upstream? Should we wait for that to land before merging this?

[duzumaki]

Oauthlib is still due a release even so this was never going to be merged first. I'll make that pr after we iron out the major parts here in DOT

[danlamanna]

Likewise maybe DeviceGrant?

[duzumaki]

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

[danlamanna]

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.

[duzumaki]

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

[duzumaki]

@danlamanna

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

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

3. 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!

[dopry]

@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]

@dopry

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

[n2ygk]

@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]

@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]

@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]

I'll take a look later this evening

[duzumaki]

@dopry @danlamanna

We're moving in the right direction. I'd love to see those open comments get resolved.

just addressed all open comments

[danlamanna]

Thanks for taking another pass at this. After taking a closer look, I noticed that the tokens being issued have null user ids, so I don't think things are working quite correctly out of the box.

I saw your message on commit 1167cd6 (#1539) and I think that this is the crux of the problem. For other flows, like auth code, the user can also be absent. The way it gets set on request.user is via DOT extracting it from the Grant and overriding it. What I'm proposing is that this flow does something similar and creates the token with the user retrieved from the Grant (now named Device). The one caveat is that the DeviceGrant would need to have a nullable User unlike other grants, since the user isn't known when it's initially created.

[danlamanna]

I'm not sure how this should interoperate with downstreams overriding OAUTH_DEVICE_USER_CODE_GENERATOR, but should we follow the guidance around stripping whitespace/punctuation and uppercasing the user_code before looking it up?

[duzumaki]

I'm not following why. when OAUTH_DEVICE_USER_CODE_GENERATOR is set that's the user code that will be used, it's up to downstream to determine how they want their user code to be. if I strip anything out it won't be an exact match and is making an assumption that user was looking for user code X instead of user code Y(the one they're actually looking for)

[danlamanna]

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.

[danlamanna]

Does this (and the below AccessDenied) get translated into an HTTP 400? In my development environment it returns a 500 due to an uncaught exception, but I might have things misconfigured.

[danlamanna]

Suggested change

	device: Device = get_device_model().objects.get(user_code=user_code)
	device: Device = get_device_model().objects.filter(user_code=user_code).first()

I realized this code will 500 if it can't find an object otherwise.

[duzumaki]

Using filter here is semantically different than using .get()
I'm saying "there should only be 1 device using this code during any active device session in the auth server"

It's better the exception is caught here

Since the entropy should be high enough of the user code and the rfc doesn't mention collision handling during active sessions I am not handling it in DOT. The .get() with exception catching should be appropriate here

[dopry]

Is there a unique constraint on the schema? I feel like this is too late to throw an error for a unique constraint. Exception handling is kind of expensive as well compare to just check if the response is empty and emitting an error. What should the response be if no code is found in this scenario? Which part of the spec covers it?

[duzumaki]

There's a unique constraint on the device code (following the RFC) but nothing mentioned uniqueness of the user code

However in practice this is a problem(that the rfc should mention but doesn't)

The original version of this pr had the constraint but i removed it since it isn't part of the rfc.
I can add the constraint back in but it's up to the downstream user(the downstream authserver) to inform the device to send another request
and do any other custom logic like making the user code only unique for the duration device flow session

I've seen some other open source implementations face this issue as well

[duzumaki]

@danlamanna

I saw your message on commit 1167cd6 (#1539) and I think that this is the crux of the problem. For other flows, like auth code, the user can also be absent. The way it gets set on request.user is via DOT extracting it from the Grant and overriding it. What I'm proposing is that this flow does something similar and creates the token with the user retrieved from the Grant (now named Device). The one caveat is that the DeviceGrant would need to have a nullable User unlike other grants, since the user isn't known when it's initially created.

I agree. I'll have this change added tomorrow.
Since we're required to login when the user visits /device in the browser the device user column(to be added) can be set there and then probably here we can set
request.user = device.user*

*Thanks for pointing this out. since oauthlib and DOT have no typing it makes understanding the flow of what's calling what quite cumbersome. I saw that line you pointed where request.user = grant.user before and only realised now it's not a WSGI object but an ouathlib Request. The lack of typing is throwing me off