Fix caching and checking of introspect responses (#173)

- Ensure introspect is cached even if 'active=False'
- Don't check claims pointlessly: exp and nbf can fail if your clock
  is drifted, so it's not safe to be checking them unless users can
  configure a leeway. Also, 'active' tells you everything you need in
  OAuth2.
- Capture scope validation failures with an exception, not None.
- Cache by token hash, not full token string.

changelog.d/20241008_171503_sirosen_fix_introspect_check.rst

@@ -0,0 +1,19 @@
+Features
+--------
+
+- The token introspect checking and caching performed in ``AuthState`` has
+  been improved.
+
+  - The cache is keyed off of token hashes, rather than raw token strings.
+
+  - The ``exp`` and ``nbf`` values are no longer verified, removing the
+    possibility of incorrect treatment of valid tokens as invalid due to clock
+    drift.
+
+  - Introspect response caching caches the raw response even for invalid
+    tokens, meaning that Action Providers will no longer repeatedly introspect
+    a token once it is known to be invalid.
+
+  - Scope validation raises a new, dedicated error class,
+    ``globus_action_provider_tools.authentication.InvalidTokenScopesError``, on
+    failure.

src/globus_action_provider_tools/authentication.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import functools
 import hashlib
 import logging
 from time import time
@@ -34,6 +35,19 @@ def identity_principal(id_: str) -> str:
     return f"urn:globus:auth:identity:{id_}"
 
 
+class InvalidTokenScopesError(ValueError):
+    def __init__(
+        self, expected_scopes: frozenset[str], actual_scopes: frozenset[str]
+    ) -> None:
+        self.expected_scopes = expected_scopes
+        self.actual_scopes = actual_scopes
+        super().__init__(
+            f"Token scopes were not valid. "
+            f"The valid scopes for this service are {self.expected_scopes} but the "
+            f"token contained {self.actual_scopes} upon inspection."
+        )
+
+
 class AuthState:
     # Cache for introspection operations, max lifetime: 30 seconds
     introspect_cache: TTLCache = TTLCache(maxsize=100, ttl=30)
@@ -49,7 +63,7 @@ def __init__(
         self,
         auth_client: ConfidentialAppAuthClient,
         bearer_token: str,
-        expected_scopes: Iterable[str],
+        expected_scopes: frozenset[str],
     ) -> None:
         self.auth_client = auth_client
         self.bearer_token = bearer_token
@@ -59,48 +73,46 @@ def __init__(
         self.errors: list[Exception] = []
         self._groups_client: GroupsClient | None = None
 
-    def introspect_token(self) -> GlobusHTTPResponse | None:
-        # There are cases where a null or empty string bearer token are present as a
-        # placeholder
-        if self.bearer_token is None:
-            return None
+    @functools.cached_property
+    def _token_hash(self) -> str:
+        return _hash_token(self.bearer_token)
 
-        resp = AuthState.introspect_cache.get(self.bearer_token)
-        if resp is not None:
-            log.info(
-                f"Using cached introspection response for token ***{self.sanitized_token}"
+    def _cached_introspect_call(self) -> GlobusHTTPResponse:
+        introspect_result = AuthState.introspect_cache.get(self._token_hash)
+        if introspect_result is not None:
+            log.debug(
+                f"Using cached introspection introspect_resultonse for <token_hash={self._token_hash}>"
             )
-            return resp
+            return introspect_result
 
-        log.info(f"Introspecting token ***{self.sanitized_token}")
-        resp = self.auth_client.oauth2_token_introspect(
+        log.debug(f"Introspecting token <token_hash={self._token_hash}>")
+        introspect_result = self.auth_client.oauth2_token_introspect(
             self.bearer_token, include="identity_set"
         )
-        now = time()
+        self.introspect_cache[self._token_hash] = introspect_result
 
-        try:
-            if resp.get("active", False) is not True:
-                raise AssertionError("Invalid token.")
-            if not resp.get("nbf", now + 4) < (time() + 3):
-                raise AssertionError("Token not yet valid -- check system clock?")
-            if not resp.get("exp", 0) > (time() - 3):
-                raise AssertionError("Token expired.")
-            scopes = frozenset(resp.get("scope", "").split())
-            if not scopes & set(self.expected_scopes):
-                raise AssertionError(
-                    "Token invalid scopes. "
-                    f"Expected one of: {self.expected_scopes}, got: {scopes}"
-                )
-            if "identity_set" not in resp:
-                raise AssertionError("Missing identity_set")
-        except AssertionError as err:
-            self.errors.append(err)
-            log.info(err)
+        return introspect_result
+
+    def introspect_token(self) -> GlobusHTTPResponse | None:
+        introspect_result = self._cached_introspect_call()
+
+        # FIXME: convert this to an exception, rather than 'None'
+        # the exception could be raised in _verify_introspect_result
+        if not introspect_result["active"]:
             return None
-        else:
-            log.info(f"Caching token response for token ***{self.sanitized_token}")
-            AuthState.introspect_cache[self.bearer_token] = resp
-            return resp
+
+        self._verify_introspect_result(introspect_result)
+        return introspect_result
+
+    def _verify_introspect_result(self, introspect_result: GlobusHTTPResponse) -> None:
+        """
+        A helper which checks token introspect properties and raises exceptions on failure.
+        """
+        # validate scopes, ensuring that the token provided accords with the service's
+        # notion of what operations exist and are supported
+        scopes = set(introspect_result.get("scope", "").split())
+        if any(s not in self.expected_scopes for s in scopes):
+            raise InvalidTokenScopesError(self.expected_scopes, frozenset(scopes))
 
     @property
     def effective_identity(self) -> str | None:

tests/test_authentication.py

@@ -6,24 +6,31 @@
 import pytest
 from globus_sdk._testing import load_response
 
-from globus_action_provider_tools.authentication import AuthState, identity_principal
+from globus_action_provider_tools.authentication import (
+    AuthState,
+    InvalidTokenScopesError,
+    identity_principal,
+)
 
 
 def get_auth_state_instance(expected_scopes: t.Iterable[str]) -> AuthState:
     return AuthState(
         auth_client=globus_sdk.ConfidentialAppAuthClient("bogus", "bogus"),
         bearer_token="bogus",
-        expected_scopes=expected_scopes,
+        expected_scopes=frozenset(expected_scopes),
     )
 
 
-@pytest.fixture
-def auth_state(mocked_responses) -> AuthState:
-    """Create an AuthState instance."""
-
+@pytest.fixture(autouse=True)
+def _clear_auth_state_cache():
     AuthState.dependent_tokens_cache.clear()
     AuthState.group_membership_cache.clear()
     AuthState.introspect_cache.clear()
+
+
+@pytest.fixture
+def auth_state(mocked_responses) -> AuthState:
+    """Create an AuthState instance."""
     # note that expected-scope MUST match the fixture data
     return get_auth_state_instance(["expected-scope"])
 
@@ -90,3 +97,13 @@ def test_invalid_grant_exception(auth_state):
     load_response("token-introspect", case="success")
     load_response("token", case="invalid-grant")
     assert auth_state.get_authorizer_for_scope("doesn't matter") is None
+
+
+def test_invalid_scopes_error():
+    auth_state = get_auth_state_instance(["bad-scope"])
+    load_response("token-introspect", case="success")
+    with pytest.raises(InvalidTokenScopesError) as excinfo:
+        auth_state.introspect_token()
+
+    assert excinfo.value.expected_scopes == {"bad-scope"}
+    assert excinfo.value.actual_scopes == {"expected-scope"}