oauth2/token returns JSON instead of HTML error (#723)

uc-cdis · Oct 23, 2019 · 4d29bca · 4d29bca
1 parent 65540bf
commit 4d29bca
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 26 deletions.
diff --git a/fence/blueprints/oauth2.py b/fence/blueprints/oauth2.py
@@ -14,13 +14,17 @@
 stateless.
 """
 
-import flask
 
 from authlib.common.urls import add_params_to_uri
 from authlib.oauth2.rfc6749 import AccessDeniedError, InvalidRequestError, OAuth2Error
+import flask
+import json
+
+from authutils.errors import JWTExpiredError
 
 from fence.blueprints.login import IDP_URL_MAP
 from fence.errors import Unauthorized, UserError
+from fence.jwt.errors import JWTError
 from fence.jwt.token import SCOPE_DESCRIPTION
 from fence.models import Client
 from fence.oidc.endpoints import RevocationEndpoint
@@ -282,7 +286,19 @@ def get_token(*args, **kwargs):
     See the OpenAPI documentation for detailed specification, and the OAuth2
     tests for examples of some operation and correct behavior.
     """
-    return server.create_token_response()
+    try:
+        response = server.create_token_response()
+    except (JWTError, JWTExpiredError) as e:
+        # - in Authlib 0.11, create_token_response does not raise OAuth2Error
+        # - fence.jwt.errors.JWTError: blacklisted refresh token
+        # - JWTExpiredError (cdiserrors.AuthNError subclass): expired
+        #   refresh token
+        # Returns code 400 per OAuth2 spec
+        body = {"error": "invalid_grant", "error_description": e.message}
+        response = flask.Response(
+            json.dumps(body), mimetype="application/json", status=400
+        )
+    return response
 
 
 @blueprint.route("/revoke", methods=["POST"])

diff --git a/fence/jwt/token.py b/fence/jwt/token.py
@@ -376,29 +376,29 @@ def generate_signed_access_token(
     }
 
     if include_project_access:
-      # NOTE: "THIS IS A TERRIBLE STOP-GAP SOLUTION SO THAT USERS WITH
-      #       MINIMAL ACCESS CAN STILL USE LATEST VERSION OF FENCE
-      #       WITH VERSIONS OF PEREGRINE/SHEEPDOG THAT DO NOT CURENTLY
-      #       SUPPORT AUTHORIZATION CHECKS AGAINST ARBORIST (AND INSTEAD
-      #       RELY ON THE PROJECTS IN THE TOKEN). If the token is too large
-      #       everything breaks. I'm sorry" --See PXP-3717
-      if len(dict(user.project_access)) < config["TOKEN_PROJECTS_CUTOFF"]:
-          claims["context"]["user"]["projects"] = dict(user.project_access)
-      else:
-          # truncate to configured number of projects in token
-          projects = dict(user.project_access)
-          for key in list(projects)[config["TOKEN_PROJECTS_CUTOFF"]:]:
-              del projects[key]
-          claims["context"]["user"]["projects"] = projects
-          logger.warning(
-              "NOT including project_access = {} in claims for user {} because there are too many projects for the token\n".format(
-                  {
-                      k: dict(user.project_access)[k]
-                      for k in set(dict(user.project_access)) - set(projects)
-                  },
-                  user.username,
-              )
-          )
+        # NOTE: "THIS IS A TERRIBLE STOP-GAP SOLUTION SO THAT USERS WITH
+        #       MINIMAL ACCESS CAN STILL USE LATEST VERSION OF FENCE
+        #       WITH VERSIONS OF PEREGRINE/SHEEPDOG THAT DO NOT CURENTLY
+        #       SUPPORT AUTHORIZATION CHECKS AGAINST ARBORIST (AND INSTEAD
+        #       RELY ON THE PROJECTS IN THE TOKEN). If the token is too large
+        #       everything breaks. I'm sorry" --See PXP-3717
+        if len(dict(user.project_access)) < config["TOKEN_PROJECTS_CUTOFF"]:
+            claims["context"]["user"]["projects"] = dict(user.project_access)
+        else:
+            # truncate to configured number of projects in token
+            projects = dict(user.project_access)
+            for key in list(projects)[config["TOKEN_PROJECTS_CUTOFF"] :]:
+                del projects[key]
+            claims["context"]["user"]["projects"] = projects
+            logger.warning(
+                "NOT including project_access = {} in claims for user {} because there are too many projects for the token\n".format(
+                    {
+                        k: dict(user.project_access)[k]
+                        for k in set(dict(user.project_access)) - set(projects)
+                    },
+                    user.username,
+                )
+            )
 
     # only add google linkage information if provided
     if linked_google_email:

diff --git a/tests/rfc6749/test_oauth2.py b/tests/rfc6749/test_oauth2.py
@@ -85,4 +85,4 @@ def test_oauth2_token_post_revoke(oauth_test_client):
     refresh_token = oauth_test_client.token_response.refresh_token
     oauth_test_client.refresh(refresh_token, do_asserts=False)
     response = oauth_test_client.refresh_response.response
-    assert response.status_code == 401
+    assert response.status_code == 400