Merge pull request #889 from lsst-sqre/tickets/DM-41424

DM-41424: Refactor to reduce complexity

pyproject.toml

@@ -163,10 +163,12 @@ ignore = [
     "FIX002",  # point of a TODO comment is that we're not ready to fix it
     "G004",    # forbidding logging f-strings is appealing, but not our style
     "RET505",  # disagree that omitting else always makes code more readable
+    "PLR0911", # often many returns is clearer and simpler style
     "PLR0913", # factory pattern uses constructors with many arguments
     "PLR2004", # too aggressive about magic values
     "S105",    # good idea but too many false positives on non-passwords
     "S106",    # good idea but too many false positives on non-passwords
+    "S107",    # good idea but too many false positives on non-passwords
     "S603",    # not going to manually mark every subprocess call as reviewed
     "S607",    # using PATH is not a security vulnerability
     "SIM102",  # sometimes the formatting of nested if statements is clearer
@@ -227,11 +229,8 @@ builtins-ignorelist = [
 fixture-parentheses = false
 mark-parentheses = false
 
-[tool.ruff.pep8-naming]
-classmethod-decorators = [
-    "pydantic.root_validator",
-    "pydantic.validator",
-]
+[tool.ruff.mccabe]
+max-complexity = 11
 
 [tool.ruff.pydocstyle]
 convention = "numpy"

src/gafaelfawr/auth.py

@@ -277,9 +277,9 @@ def generate_unauthorized_challenge(
 
 
 def parse_authorization(context: RequestContext) -> str | None:
-    """Find a handle or token in the Authorization header.
+    """Find a token in the Authorization header.
 
-    Supports either ``Bearer`` or ``Basic`` authorization types.  Rebinds the
+    Supports either ``Bearer`` or ``Basic`` authorization types. Rebinds the
     logging context to include the source of the token, if one is found.
 
     Parameters
@@ -290,21 +290,18 @@ def parse_authorization(context: RequestContext) -> str | None:
     Returns
     -------
     str or None
-        The handle or token if one was found, otherwise `None`.
+        Token if one was found, otherwise `None`.
 
     Raises
     ------
     InvalidRequestError
-        If the Authorization header is malformed.
+        Raised if the Authorization header is malformed.
 
     Notes
     -----
     A Basic Auth authentication string is normally a username and a password
-    separated by colon and then base64-encoded.  Support a username of the
-    token (or session handle) and a password of ``x-oauth-basic``, or a
-    username of ``x-oauth-basic`` and a password of the token (or session
-    handle).  If neither is the case, assume the token or session handle is
-    the username.
+    separated by colon and then base64-encoded. This method accepts a token in
+    either the username or the password field.
     """
     header = context.request.headers.get("Authorization")
 

src/gafaelfawr/dependencies/auth.py

@@ -73,7 +73,7 @@ def __init__(
         self.auth_type = auth_type
         self.ajax_forbidden = ajax_forbidden
 
-    async def authenticate(  # noqa: C901
+    async def authenticate(
         self, context: RequestContext, x_csrf_token: str | None = None
     ) -> TokenData:
         """Authenticate the request.
@@ -104,19 +104,11 @@ async def authenticate(  # noqa: C901
         fastapi.HTTPException
             Raised if authentication is not provided or is not valid.
         """
-        token = context.state.token
-        if token:
-            context.rebind_logger(token_source="cookie")
+        token = self._find_token(context)
+
+        # Check CSRF if and only if the token was found in a cookie.
+        if context.state.token:
             self._verify_csrf(context, x_csrf_token)
-        elif not self.require_session:
-            try:
-                token_str = parse_authorization(context)
-                if token_str:
-                    token = Token.from_str(token_str)
-            except (InvalidRequestError, InvalidTokenError) as e:
-                raise generate_challenge(context, self.auth_type, e) from None
-        if not token:
-            raise self._redirect_or_error(context)
 
         if self.allow_bootstrap_token:
             if token == context.config.bootstrap_token:
@@ -150,6 +142,39 @@ async def authenticate(  # noqa: C901
 
         return data
 
+    def _find_token(self, context: RequestContext) -> Token:
+        """Find the authentication tomen in the request.
+
+        Parameters
+        ----------
+        context
+            The request context.
+
+        Returns
+        -------
+        Token
+            Token found in the request.
+
+        Raises
+        ------
+        fastapi.HTTPException
+            Raised if authentication is not provided or is not valid.
+        """
+        token = context.state.token
+        if token:
+            context.rebind_logger(token_source="cookie")
+            return token
+        elif not self.require_session:
+            try:
+                token_str = parse_authorization(context)
+                if token_str:
+                    return Token.from_str(token_str)
+            except (InvalidRequestError, InvalidTokenError) as e:
+                raise generate_challenge(context, self.auth_type, e) from None
+
+        # Fall through when no token was found.
+        raise self._redirect_or_error(context)
+
     def _redirect_or_error(self, context: RequestContext) -> HTTPException:
         """Redirect to the ``/login`` route or return a 401 error.
 

src/gafaelfawr/exceptions.py

@@ -44,6 +44,7 @@
     "MissingUsernameClaimError",
     "NoAvailableGidError",
     "NoAvailableUidError",
+    "NoScopesError",
     "NotConfiguredError",
     "NotFoundError",
     "OAuthError",
@@ -152,6 +153,13 @@ def __init__(self, message: str) -> None:
         super().__init__(message, ErrorLocation.body, ["scopes"])
 
 
+class NoScopesError(InputValidationError):
+    """The user has no valid scopes and therefore cannot log in."""
+
+    error = "permission_denied"
+    status_code = status.HTTP_403_FORBIDDEN
+
+
 class NotConfiguredError(InputValidationError):
     """The requested operation was not configured."""
 

src/gafaelfawr/handlers/auth.py

@@ -344,7 +344,7 @@ async def get_anonymous(
     return {"status": "ok"}
 
 
-async def build_success_headers(  # noqa: C901
+async def build_success_headers(
     context: RequestContext, auth_config: AuthConfig, token_data: TokenData
 ) -> list[tuple[str, str]]:
     """Construct the headers for successful authorization.
@@ -393,7 +393,54 @@ async def build_success_headers(  # noqa: C901
     if user_info.email:
         headers.append(("X-Auth-Request-Email", user_info.email))
 
-    delegated_token = None
+    # Add the delegated token, if there should be one.
+    delegated = await build_delegated_token(context, auth_config, token_data)
+    if delegated:
+        headers.append(("X-Auth-Request-Token", delegated))
+
+    # If told to put the delegated token in the Authorization header, do that.
+    # Otherwise, strip authentication tokens from the Authorization headers of
+    # the incoming request and reflect the remainder back in the response.
+    # Always do this with the Cookie header. ingress-nginx can then be
+    # configured to lift those headers up into the proxy request, preventing
+    # the user's cookie from being passed down to the protected application.
+    if auth_config.use_authorization:
+        if delegated:
+            headers.append(("Authorization", f"Bearer {delegated}"))
+    elif "Authorization" in context.request.headers:
+        raw_authorizations = context.request.headers.getlist("Authorization")
+        authorizations = clean_authorization(raw_authorizations)
+        if authorizations:
+            headers.extend(("Authorization", v) for v in authorizations)
+    if "Cookie" in context.request.headers:
+        raw_cookies = context.request.headers.getlist("Cookie")
+        cookies = clean_cookies(raw_cookies)
+        if cookies:
+            headers.extend(("Cookie", v) for v in cookies)
+
+    return headers
+
+
+async def build_delegated_token(
+    context: RequestContext, auth_config: AuthConfig, token_data: TokenData
+) -> str | None:
+    """Construct the delegated token for this request.
+
+    Parameters
+    ----------
+    context
+        Context of the incoming request.
+    auth_config
+        Configuration parameters for the authorization.
+    token_data
+        Data from the authentication token.
+
+    Returns
+    -------
+    str or None
+        Delegated token to include in the request, or `None` if none should be
+        included.
+    """
     if auth_config.notebook:
         token_service = context.factory.create_token_service()
         async with context.session.begin():
@@ -402,8 +449,7 @@ async def build_success_headers(  # noqa: C901
                 ip_address=context.ip_address,
                 minimum_lifetime=auth_config.minimum_lifetime,
             )
-        delegated_token = str(token)
-        headers.append(("X-Auth-Request-Token", delegated_token))
+        return str(token)
     elif auth_config.delegate_to:
         # Delegated scopes are optional; if the authenticating token doesn't
         # have the scope, it's omitted from the delegated token.  (To make it
@@ -422,27 +468,6 @@ async def build_success_headers(  # noqa: C901
                 ip_address=context.ip_address,
                 minimum_lifetime=auth_config.minimum_lifetime,
             )
-        delegated_token = str(token)
-        headers.append(("X-Auth-Request-Token", delegated_token))
-
-    # If told to put the delegated token in the Authorization header, do that.
-    # Otherwise, strip authentication tokens from the Authorization headers of
-    # the incoming request and reflect the remainder back in the response.
-    # Always do this with the Cookie header.  ingress-nginx can then be
-    # configured to lift those headers up into the proxy request, preventing
-    # the user's cookie from being passed down to the protected application.
-    if auth_config.use_authorization:
-        if delegated_token:
-            headers.append(("Authorization", f"Bearer {delegated_token}"))
-    elif "Authorization" in context.request.headers:
-        raw_authorizations = context.request.headers.getlist("Authorization")
-        authorizations = clean_authorization(raw_authorizations)
-        if authorizations:
-            headers.extend(("Authorization", v) for v in authorizations)
-    if "Cookie" in context.request.headers:
-        raw_cookies = context.request.headers.getlist("Cookie")
-        cookies = clean_cookies(raw_cookies)
-        if cookies:
-            headers.extend(("Cookie", v) for v in cookies)
-
-    return headers
+        return str(token)
+    else:
+        return None