Merge pull request #496 from Robin-Van-de-Merghel/robin-fix-read_token

[HotFix]: Bad error handling can lead to a 500 error

Author: aldbr

diracx-logic/src/diracx/logic/auth/utils.py

@@ -8,7 +8,6 @@
 import httpx
 from authlib.integrations.starlette_client import OAuthError
 from authlib.jose import JsonWebKey, JsonWebToken
-from authlib.jose.errors import DecodeError
 from authlib.oidc.core import IDToken
 from cachetools import TTLCache
 from cryptography.fernet import Fernet
@@ -195,13 +194,10 @@ def read_token(
 ) -> dict:
     # First transform it into bytes, then return a jwt object
     # Don't take settings as a parameter to allow claims_options to be None or something else
-    try:
-        encoded_payload = payload.encode("ascii")
-        jwt = JsonWebToken(token_algorithm)
-        decoded_jwt = jwt.decode(encoded_payload, key, claims_options=claims_options)
-        decoded_jwt.validate()
-    except DecodeError as e:
-        raise ValueError("wrong json payload") from e
+    encoded_payload = payload.encode("ascii")
+    jwt = JsonWebToken(token_algorithm)
+    decoded_jwt = jwt.decode(encoded_payload, key, claims_options=claims_options)
+    decoded_jwt.validate()
     return decoded_jwt
 
 

diracx-routers/src/diracx/routers/auth/management.py

@@ -9,6 +9,7 @@
 import logging
 from typing import Annotated, Any
 
+from authlib.jose import JoseError
 from fastapi import Depends, HTTPException, status
 from typing_extensions import TypedDict
 from uuid_utils import UUID
@@ -79,7 +80,7 @@ async def revoke_refresh_token_by_refresh_token(
         await revoke_refresh_token_by_refresh_token_bl(
             auth_db, None, refresh_token, settings
         )
-    except ValueError:
+    except JoseError:
         logger.warning("Someone tried to revoke its token but failed.")
 
     return "Refresh token revoked"

diracx-routers/tests/auth/test_standard.py

@@ -685,6 +685,20 @@ async def test_refresh_token_invalid(test_client, auth_httpx_mock: HTTPXMock):
     assert data["detail"] == "Invalid JWT: bad_signature: "
 
 
+async def test_bad_access_token(test_client):
+    """Test accessing a resource with a bad token."""
+    # From https://github.com/DIRACGrid/diracx/pull/496
+    r = test_client.get(
+        "/api/auth/userinfo", headers={"Authorization": "Bearer thisisabadbearer"}
+    )
+    data = r.json()
+
+    # Should raise a 401, with "Invalid JWT"
+    # Not Invalid Authorization Header because raised when decoding the token
+    assert r.status_code == 401, data
+    assert data["detail"] == "Invalid JWT"
+
+
 async def test_list_refresh_tokens(test_client, auth_httpx_mock: HTTPXMock):
     """Test the refresh token listing with 2 users, a normal one and token manager:
     - normal user gets a refresh token and lists it
@@ -875,7 +889,6 @@ async def test_revoke_refresh_token_classic(test_client, auth_httpx_mock: HTTPXM
             "refresh_token": "does-not-exist",
             "client_id": DIRAC_CLIENT_ID,
         },
-        headers={"Authorization": f"Bearer {normal_user_access_token}"},
     )
     assert r.status_code == 200
 
@@ -886,7 +899,6 @@ async def test_revoke_refresh_token_classic(test_client, auth_httpx_mock: HTTPXM
             "refresh_token": normal_user_refresh_token,
             "client_id": DIRAC_CLIENT_ID,
         },
-        headers={"Authorization": f"Bearer {normal_user_access_token}"},
     )
     assert r.status_code == 200
 
@@ -905,7 +917,6 @@ async def test_revoke_refresh_token_classic(test_client, auth_httpx_mock: HTTPXM
             "refresh_token": normal_user_refresh_token,
             "client_id": "a_wrong_dirac_client_id",
         },
-        headers={"Authorization": f"Bearer {normal_user_access_token}"},
     )
     assert r.status_code == 403
     assert r.json()["detail"] == "Unrecognised client_id"