fix: addressed comments; enhanced preconditions and responses of views

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

Author: cristiprg

docs/tutorial/tutorial_06.rst

@@ -43,6 +43,7 @@ then start the server
     python manage.py runserver
 
 .. _RFC: https://www.rfc-editor.org/rfc/rfc8628
+.. _RFC3.5: https://datatracker.ietf.org/doc/html/rfc8628#section-3.5
 
 2. To initiate device authorization, send this request (in the real world, the device
 makes this request). In `RFC`_ Figure 1, this is step (A).
@@ -92,14 +93,16 @@ Send the following request (in the real world, the device makes this request). I
         --data-urlencode 'client_id={your application client id}' \
         --data-urlencode 'grant_type=urn:ietf:params:oauth:grant-type:device_code'
 
-In `RFC`_ Figure 1, there are two options for step (F). Until the user enters the code in the browser and approves,
-the response will be 400:
+In `RFC`_ Figure 1, there are multiple options for step (F), as per `_RFC3.5`. Until the user enters the code in the browser
+and approves, the response will be 400:
 
 .. code-block:: json
 
     {"error": "authorization_pending"}
 
-After the user approves, the response will be 200:
+The error could also be `access_denied` or `expired_token`.
+
+However, after the user approves, the response will be 200:
 
 .. code-block:: json
 

oauth2_provider/models.py

@@ -698,10 +698,19 @@ class Meta:
 
     def is_expired(self):
         """
-        Check device flow session expiration.
+        Check device flow session expiration and set the status to "expired" if current time
+        is past the "expires" deadline.
         """
+        if self.status == self.EXPIRED:
+            return True
+
         now = datetime.now(tz=dt_timezone.utc)
-        return now >= self.expires
+        if now >= self.expires:
+            self.status = self.EXPIRED
+            self.save(update_fields=["status"])
+            return True
+
+        return False
 
 
 class DeviceManager(models.Manager):

oauth2_provider/urls.py

@@ -13,7 +13,7 @@
     path("introspect/", views.IntrospectTokenView.as_view(), name="introspect"),
     path("device-authorization/", views.DeviceAuthorizationView.as_view(), name="device-authorization"),
     path("device/", views.device_user_code_view, name="device"),
-    path("device-confirm/<str:device_code>", views.device_confirm_view, name="device-confirm"),
+    path("device-confirm/<str:client_id>/<str:user_code>", views.device_confirm_view, name="device-confirm"),
 ]
 
 

oauth2_provider/views/base.py

@@ -13,10 +13,7 @@
 from django.views.decorators.csrf import csrf_exempt
 from django.views.decorators.debug import sensitive_post_parameters
 from django.views.generic import FormView, View
-from oauthlib.oauth2.rfc8628.errors import (
-    AccessDenied,
-    AuthorizationPendingError,
-)
+from oauthlib.oauth2.rfc8628 import errors as rfc8628_errors
 
 from oauth2_provider.models import Device
 
@@ -320,34 +317,56 @@ def authorization_flow_token_response(
     def device_flow_token_response(
         self, request: http.HttpRequest, device_code: str, *args, **kwargs
     ) -> http.HttpResponse:
-        device = Device.objects.get(device_code=device_code)
+        try:
+            device = Device.objects.get(device_code=device_code)
+        except Device.DoesNotExist:
+            # The RFC does not mention what to return when the device is not found,
+            # but to keep it consistent with the other errors, we return the error
+            # in json format with an "error" key and the value formatted in the same
+            # way.
+            return http.HttpResponseNotFound(
+                content='{"error": "device_not_found"}',
+                content_type="application/json",
+            )
 
+        # Here we are returning the errors according to
+        # https://datatracker.ietf.org/doc/html/rfc8628#section-3.5
+        # TODO: "slow_down" error (essentially rate-limiting).
         if device.status == device.AUTHORIZATION_PENDING:
-            pending_error = AuthorizationPendingError()
-            return http.HttpResponse(
-                content=pending_error.json, status=pending_error.status_code, content_type="application/json"
+            error = rfc8628_errors.AuthorizationPendingError()
+        elif device.status == device.DENIED:
+            error = rfc8628_errors.AccessDenied()
+        elif device.status == device.EXPIRED:
+            error = rfc8628_errors.ExpiredTokenError()
+        elif device.status != device.AUTHORIZED:
+            # It's technically impossible to get here because we've exhausted
+            # all the possible values for status. However, it does act as a
+            # reminder for developers when they add, in the future, new values
+            # (such as slow_down) that they must handle here.
+            return http.HttpResponseServerError(
+                content='{"error": "internal_error"}',
+                content_type="application/json",
             )
+        else:
+            # AUTHORIZED is the only accepted state, anything else is
+            # rejected.
+            error = None
 
-        if device.status == device.DENIED:
-            access_denied_error = AccessDenied()
+        if error:
             return http.HttpResponse(
-                content=access_denied_error.json,
-                status=access_denied_error.status_code,
+                content=error.json,
+                status=error.status_code,
                 content_type="application/json",
             )
 
         url, headers, body, status = self.create_token_response(request)
-
         if status != 200:
             return http.JsonResponse(data=json.loads(body), status=status)
 
         response = http.JsonResponse(data=json.loads(body), status=status)
-
         for k, v in headers.items():
             response[k] = v
 
-        device.status = device.EXPIRED
-        device.save(update_fields=["status"])
         return response
 
     def post(self, request: http.HttpRequest, *args, **kwargs) -> http.HttpResponse:

oauth2_provider/views/device.py

@@ -2,16 +2,13 @@
 
 from django import forms, http
 from django.contrib.auth.decorators import login_required
+from django.db.models import Q
 from django.shortcuts import render
 from django.urls import reverse
 from django.utils.decorators import method_decorator
 from django.views.decorators.csrf import csrf_exempt
 from django.views.generic import View
 from oauthlib.oauth2 import DeviceApplicationServer
-from oauthlib.oauth2.rfc8628.errors import (
-    AccessDenied,
-    ExpiredTokenError,
-)
 
 from oauth2_provider.compat import login_not_required
 from oauth2_provider.models import Device, DeviceCodeResponse, DeviceRequest, create_device, get_device_model
@@ -41,52 +38,71 @@ class DeviceForm(forms.Form):
     user_code = forms.CharField(required=True)
 
 
-# it's common to see in real world products
-# device flow's only asking the user to sign in after they input the
-# user code but since the user has to be signed in regardless to approve the
-# device login we're making the decision here to require being logged in
-# up front
 @login_required
 def device_user_code_view(request):
+    """
+    The view where the user is instructed (by the device) to come to in order to
+    enter the user code. More details in this section of the RFC:
+    https://datatracker.ietf.org/doc/html/rfc8628#section-3.3
+
+    Note: it's common to see in other implementations of this RFC that only ask the
+    user to sign in after they input the user code but since the user has to be signed
+    in regardless, to approve the device login we're making the decision here, for
+    simplicity, to require being logged in up front.
+    """
     form = DeviceForm(request.POST)
 
     if request.method != "POST":
         return render(request, "oauth2_provider/device/user_code.html", {"form": form})
 
     if not form.is_valid():
-        return render(request, "oauth2_provider/device/user_code.html", {"form": form})
+        form.add_error(None, "Form invalid")
+        return render(request, "oauth2_provider/device/user_code.html", {"form": form}, status=400)
 
     user_code: str = form.cleaned_data["user_code"]
-    device: Device = get_device_model().objects.get(user_code=user_code)
+    try:
+        device: Device = get_device_model().objects.get(user_code=user_code)
+    except Device.DoesNotExist:
+        form.add_error("user_code", "Incorrect user code")
+        return render(request, "oauth2_provider/device/user_code.html", {"form": form}, status=404)
 
     device.user = request.user
     device.save(update_fields=["user"])
 
-    if device is None:
-        form.add_error("user_code", "Incorrect user code")
-        return render(request, "oauth2_provider/device/user_code.html", {"form": form})
-
     if device.is_expired():
-        device.status = device.EXPIRED
-        device.save(update_fields=["status"])
-        raise ExpiredTokenError
+        form.add_error("user_code", "Expired user code")
+        return render(request, "oauth2_provider/device/user_code.html", {"form": form}, status=400)
 
     # User of device has already made their decision for this device
-    if device.status in (device.DENIED, device.AUTHORIZED):
-        raise AccessDenied
+    if device.status != device.AUTHORIZATION_PENDING:
+        form.add_error("user_code", "User code has already been used")
+        return render(request, "oauth2_provider/device/user_code.html", {"form": form}, status=400)
 
     # 308 to indicate we want to keep the redirect being a POST request
     return http.HttpResponsePermanentRedirect(
-        reverse("oauth2_provider:device-confirm", kwargs={"device_code": device.device_code}), status=308
+        reverse(
+            "oauth2_provider:device-confirm",
+            kwargs={"client_id": device.client_id, "user_code": user_code},
+        ),
+        status=308,
     )
 
 
 @login_required
-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):
-        return http.HttpResponse("Invalid")
+def device_confirm_view(request: http.HttpRequest, client_id: str, user_code: str):
+    try:
+        device: Device = get_device_model().objects.get(
+            # there is a db index on client_id
+            Q(client_id=client_id) & Q(user_code=user_code)
+        )
+    except Device.DoesNotExist:
+        return http.HttpResponseNotFound("<h1>Device not found</h1>")
+
+    if device.status != device.AUTHORIZATION_PENDING:
+        # AUTHORIZATION_PENDING is the only accepted state, anything else implies
+        # that the user already approved/denied OR the deadline has passed (aka
+        # expired)
+        return http.HttpResponseBadRequest("Invalid")
 
     action = request.POST.get("action")
 

tests/app/README.md

@@ -29,9 +29,9 @@ password: password
 
   You can update data in the IDP and then dump the data to a new seed file as follows.
 
-  ```
+```
 python -Xutf8 ./manage.py dumpdata -e sessions  -e admin.logentry -e auth.permission -e contenttypes.contenttype -e oauth2_provider.accesstoken  -e oauth2_provider.refreshtoken -e oauth2_provider.idtoken --natural-foreign --natural-primary --indent 2 > fixtures/seed.json
-  ```
+```
 
 ## /test/app/rp