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]

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]

@danlamanna @dopry

I noticed the scopes aren't properly being passed on to the token. If I initiate the authorization flow with scope identity, it properly shows up for the Device but the tokens issued have the read write scope.

It is. you aren't setting the SCOPES setting correctly

---

I think the OAUTH_DEVICE_VERIFICATION_URI_* settings might be a mistake. Given a single codebase gets deployed at multiple locations, would it be better to derive the URL via the request object and request.build_absolute_uri?

don't see why an env var isn't sufficient here, if the code base is deployed in multiple locations then it's just a matter of those servers having different env vars set which from the perspective of someone implementing their own auth server, is easier to do

plus something has to tell oauthlib what string to use in the response for the verification_uri regardless, I don't see how we can derive it from the request object if someone implementing their own custom auth server can make their own custom device verification endpoint with custom naming

also passing this information down via an env var is familiar practice since we already send other data down to oauthlib via env vars already

---

The flow is still raising uncaught exceptions triggering 500 errors when it should be 400 e.g. raise AuthorizationPendingError and raise AccessDenied.

Fixed