Merge pull request #178 from globus/eliminate-none-return-values

Reduce the prevalence of `None` as a return value

changelog.d/20241018_082221_kurtmckee_eliminate_none_return_values.rst

@@ -0,0 +1,14 @@
+Changes
+-------
+
+-   ``AuthState.introspect_token()`` will no longer return ``None`` if the token is not active.
+
+    Instead, a new exception, ``InactiveTokenError``, will be raised.
+    ``InactiveTokenError`` is a subclass of ``ValueError``.
+
+    Existing code that calls ``AuthState.introspect_token()`` no longer returns ``None``, either,
+    but will instead raise ``ValueError`` (or a subclass) or a ``globus_sdk.GlobusAPIError``:
+
+    *   ``AuthState.get_authorizer_for_scope``
+    *   ``AuthState.effective_identity``
+    *   ``AuthState.identities``

src/globus_action_provider_tools/authentication.py

@@ -48,6 +48,10 @@ def __init__(
         )
 
 
+class InactiveTokenError(ValueError):
+    """Indicates that the token is not valid (its 'active' field is False)."""
+
+
 class AuthState:
     # Cache for introspection operations, max lifetime: 30 seconds
     introspect_cache: TypedTTLCache[globus_sdk.GlobusHTTPResponse] = TypedTTLCache(
@@ -95,7 +99,7 @@ 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}>"
+                f"Using cached introspection response for token_hash={self._token_hash}"
             )
             return introspect_result
 
@@ -107,40 +111,34 @@ def _cached_introspect_call(self) -> GlobusHTTPResponse:
 
         return introspect_result
 
-    def introspect_token(self) -> GlobusHTTPResponse | None:
+    def introspect_token(self) -> GlobusHTTPResponse:
         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
-
         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.
         """
+
+        if not introspect_result["active"]:
+            raise InactiveTokenError("The token is invalid.")
+
         # 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:
+    def effective_identity(self) -> str:
         tkn_details = self.introspect_token()
-        if tkn_details is None:
-            return None
         effective = identity_principal(tkn_details["sub"])
         return effective
 
     @property
     def identities(self) -> frozenset[str]:
         tkn_details = self.introspect_token()
-        if tkn_details is None:
-            return frozenset()
         return frozenset(map(identity_principal, tkn_details["identity_set"]))
 
     @property
@@ -221,7 +219,7 @@ def get_authorizer_for_scope(
         self,
         scope: str,
         required_authorizer_expiration_time: int = 60,
-    ) -> RefreshTokenAuthorizer | AccessTokenAuthorizer | None:
+    ) -> RefreshTokenAuthorizer | AccessTokenAuthorizer:
         """
         Get dependent tokens for the caller's token, then retrieve token data for the
         requested scope and attempt to build an authorizer from that data.
@@ -240,39 +238,33 @@ def get_authorizer_for_scope(
         If the access token is or will be expired within the
         ``required_authorizer_expiration_time``, it is treated as a failure.
         """
-        # this block is intentionally nested under a single exception handler
-        # if either dependent token callout fails, the entire operation is treated as a failure
-        try:
-            had_cached_value, dependent_tokens = self._get_cached_dependent_tokens()
 
-            # if the dependent token data (which could have been cached) failed to meet
-            # the requirements...
+        retrieved_from_cache, dependent_tokens = self._get_cached_dependent_tokens()
+
+        # if the dependent token data (which could have been cached) failed to meet
+        # the requirements...
+        if not self._dependent_token_response_satisfies_scope_request(
+            dependent_tokens, scope, required_authorizer_expiration_time
+        ):
+            # if there was no cached value, we just got fresh dependent tokens
+            # to do this work but they weren't sufficient
+            # there's no reason to expect new tokens would do better
+            # fail, but do not clear the cache -- it could be satisfactory
+            # for some other scope request
+            if not retrieved_from_cache:
+                raise ValueError("Dependent tokens do not match request.")
+
+            # otherwise, the cached value was bad -- fetch and check again,
+            # by clearing the cache and asking for the same data
+            del self.dependent_tokens_cache[self._dependent_token_cache_key]
+            _, dependent_tokens = self._get_cached_dependent_tokens()
+
+            # check again against requirements --
+            # this is guaranteed to be fresh data
             if not self._dependent_token_response_satisfies_scope_request(
                 dependent_tokens, scope, required_authorizer_expiration_time
             ):
-                # if there was no cached value, we just got fresh dependent tokens to do this work
-                # and they weren't sufficient
-                # there's no reason to expect new tokens would do better
-                # fail, but do not clear the cache -- it could be satisfactory for some other scope request
-                if not had_cached_value:
-                    return None  # FIXME: raise an exception instead
-
-                # otherwise, the cached value was bad -- fetch and check again, by clearing the cache and
-                # asking for the same data
-                del self.dependent_tokens_cache[self._dependent_token_cache_key]
-                _, dependent_tokens = self._get_cached_dependent_tokens()
-
-                # check again against requirements -- this is guaranteed to be fresh data
-                if not self._dependent_token_response_satisfies_scope_request(
-                    dependent_tokens, scope, required_authorizer_expiration_time
-                ):
-                    return None  # FIXME: raise an exception instead
-        except globus_sdk.AuthAPIError:
-            log.warning(
-                f"Unable to create GlobusAuthorizer for scope {scope}. Using 'None'",
-                exc_info=True,
-            )
-            return None
+                raise ValueError("Dependent tokens do not match request.")
 
         token_data = dependent_tokens.by_scopes[scope]
 

src/globus_action_provider_tools/flask/apt_blueprint.py

@@ -543,7 +543,3 @@ def _check_token(self) -> None:
             return
 
         g.auth_state = self.state_builder.build_from_request()
-        if g.auth_state.effective_identity is None:
-            current_app.logger.info(
-                f"Request failed authentication due to: {g.auth_state.errors}"
-            )

tests/test_authentication.py

@@ -101,24 +101,19 @@ def test_auth_state_caching_across_instances(auth_state, freeze_time, mocked_res
 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
+    with pytest.raises(globus_sdk.GlobusAPIError):
+        auth_state.get_authorizer_for_scope("doesn't matter")
 
 
 def test_dependent_token_callout_500_fails_dependent_authorization(auth_state):
-    """
-    On a 5xx response, getting an authorizer fails.
-
-    FIXME: currently this simply emits 'None' -- in the future the error should propagate
-    """
+    """On a 5xx response, getting an authorizer fails."""
     RegisteredResponse(
         service="auth", path="/v2/oauth2/token", method="POST", status=500
     ).add()
-    assert (
+    with pytest.raises(globus_sdk.GlobusAPIError):
         auth_state.get_authorizer_for_scope(
             "urn:globus:auth:scope:groups.api.globus.org:view_my_groups_and_memberships"
         )
-        is None
-    )
 
 
 def test_dependent_token_callout_success_fixes_bad_cache(auth_state):