diff --git a/pyproject.toml b/pyproject.toml index 251c05774..97b5d23e6 100644 --- a/pyproject.toml +++ b/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" diff --git a/src/gafaelfawr/auth.py b/src/gafaelfawr/auth.py index 0d9e0a699..b3997f30c 100644 --- a/src/gafaelfawr/auth.py +++ b/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") diff --git a/src/gafaelfawr/dependencies/auth.py b/src/gafaelfawr/dependencies/auth.py index 118ebb27f..31c023ba2 100644 --- a/src/gafaelfawr/dependencies/auth.py +++ b/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. diff --git a/src/gafaelfawr/exceptions.py b/src/gafaelfawr/exceptions.py index 59424788d..0f861d10d 100644 --- a/src/gafaelfawr/exceptions.py +++ b/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.""" diff --git a/src/gafaelfawr/handlers/auth.py b/src/gafaelfawr/handlers/auth.py index 7c10052ec..38ec38cca 100644 --- a/src/gafaelfawr/handlers/auth.py +++ b/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 diff --git a/src/gafaelfawr/handlers/login.py b/src/gafaelfawr/handlers/login.py index 38232a958..2850122a5 100644 --- a/src/gafaelfawr/handlers/login.py +++ b/src/gafaelfawr/handlers/login.py @@ -15,11 +15,13 @@ FirestoreError, InvalidReturnURLError, LDAPError, + NoScopesError, OIDCNotEnrolledError, PermissionDeniedError, ProviderError, ProviderWebError, ) +from ..models.token import Token, TokenUserInfo from ..templates import templates router = APIRouter(route_class=SlackRouteErrorHandler) @@ -102,6 +104,8 @@ async def get_login( updated with the ``/login`` route. """ if code: + if not state: + return _error_user(context, LoginError.STATE_MISSING) return await handle_provider_return(code, state, context) else: return await redirect_to_provider(return_url, context) @@ -172,8 +176,8 @@ async def redirect_to_provider( return RedirectResponse(redirect_url) -async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 - code: str, state: str | None, context: RequestContext +async def handle_provider_return( + code: str, state: str, context: RequestContext ) -> Response: """Handle the return from an external authentication provider. @@ -183,20 +187,19 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 Parameters ---------- code - The authentication code from the provider. + Authentication code from the provider. state - The opaque state used to verify that this user initiated the - authentication. This can be `None`, but that will always be an - error. + Opaque state used to verify that this user initiated the + authentication. context - The context of the incoming request. + Context of the incoming request. Returns ------- fastapi.Response Either a redirect to the resource the user was trying to reach before - authentication or an HTML page with an error message if the - authentication failed. + authentication, to the login URL, or an HTML page with an error + message if the authentication failed. Raises ------ @@ -204,10 +207,6 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 The authentication request is invalid or retrieving authentication information from the provider failed. """ - if not state: - return _error_user(context, LoginError.STATE_MISSING) - - # Extract details from the reply, check state, and get the return URL. if state != context.state.state: return _error_user(context, LoginError.STATE_INVALID) return_url = context.state.return_url @@ -216,10 +215,11 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 context.rebind_logger(return_url=return_url) # Retrieve the user identity and authorization information based on the - # reply from the authentication provider. + # reply from the authentication provider, and construct a token. provider = context.factory.create_provider() try: user_info = await provider.create_user_info(code, state, context.state) + token = await _construct_token(context, user_info) except OIDCNotEnrolledError as e: if context.config.oidc and context.config.oidc.enrollment_url: url = context.config.oidc.enrollment_url @@ -234,41 +234,66 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 return await _error_system(context, LoginError.PROVIDER_NETWORK, e) except LDAPError as e: return await _error_system(context, LoginError.LDAP_FAILED, e) + except NoScopesError as e: + await provider.logout(context.state) + return _error_user(context, LoginError.GROUPS_MISSING, str(e)) except ProviderError as e: return await _error_system(context, LoginError.PROVIDER_FAILED, e) except PermissionDeniedError as e: await provider.logout(context.state) return _error_user(context, LoginError.INVALID_USERNAME, str(e)) - # Get the scopes for this user. + # Successful login, so store the token, clear the login state, and send + # the user back to what they were doing. + context.state.token = token + context.state.state = None + context.state.return_url = None + return RedirectResponse(return_url) + + +async def _construct_token( + context: RequestContext, user_info: TokenUserInfo +) -> Token: + """Construct a token for the authenticated user. + + Parameters + ---------- + context + Context of the incoming request. + user_info + User information for the user. + + Returns + ------- + Token + Newly-minted token for the user. + + Raises + ------ + NoScopesError + Raised if the user's group memberships do not entitle them to any + scopes, and therefore they cannot log in. + PermissionDeniedError + Raised if the user's username is invalid. + """ user_info_service = context.factory.create_user_info_service() + admin_service = context.factory.create_admin_service() + token_service = context.factory.create_token_service() + + # Get the user's scopes. scopes = await user_info_service.get_scopes(user_info) if scopes is None: - await provider.logout(context.state) await user_info_service.invalidate_cache(user_info.username) msg = f"{user_info.username} is not a member of any authorized groups" - return _error_user(context, LoginError.GROUPS_MISSING, msg) + raise NoScopesError(msg) # Construct a token. - admin_service = context.factory.create_admin_service() - token_service = context.factory.create_token_service() - try: - async with context.session.begin(): - if await admin_service.is_admin(user_info.username): - scopes = sorted([*scopes, "admin:token"]) - token = await token_service.create_session_token( - user_info, scopes=scopes, ip_address=context.ip_address - ) - except PermissionDeniedError as e: - await provider.logout(context.state) - return _error_user(context, LoginError.INVALID_USERNAME, str(e)) - context.state.token = token - - # Successful login, so clear the login state and send the user back to - # what they were doing. - context.state.state = None - context.state.return_url = None - return RedirectResponse(return_url) + async with context.session.begin(): + if await admin_service.is_admin(user_info.username): + scopes = sorted([*scopes, "admin:token"]) + return await token_service.create_session_token( + user_info, scopes=scopes, ip_address=context.ip_address + ) async def _error_system( @@ -284,16 +309,16 @@ async def _error_system( Parameters ---------- context - The context of the incoming request. + Context of the incoming request. error - The type of error. + Type of error. exc - The exception representing the error. + Exception representing the error. Returns ------- fastapi.Response - The response to send back to the user. + Response to send back to the user. """ context.logger.error(error.value, error=str(exc)) slack_client = context.factory.create_slack_client() @@ -325,16 +350,16 @@ def _error_user( Parameters ---------- context - The context of the incoming request. + Context of the incoming request. error - The type of error. + Type of error. details Additional error details, if provided. Returns ------- fastapi.Response - The response to send back to the user. + Response to send back to the user. """ if details: context.logger.warning(error.value, error=details) diff --git a/src/gafaelfawr/models/kubernetes.py b/src/gafaelfawr/models/kubernetes.py index df9f9a6c8..cea23839f 100644 --- a/src/gafaelfawr/models/kubernetes.py +++ b/src/gafaelfawr/models/kubernetes.py @@ -7,6 +7,7 @@ from datetime import datetime, timedelta from enum import Enum from typing import Literal, Self +from urllib.parse import urlencode from kubernetes_asyncio.client import ( V1HTTPIngressPath, @@ -296,6 +297,37 @@ def _validate_conflicts(self) -> Self: return self + def to_auth_url(self) -> str: + """Generate the auth URL corresponding to this ingress configuration. + + Returns + ------- + str + Authentication request URL for the Gafaelfawr ``/auth`` route that + corresponds to this ingress configuration. + """ + base_url = self.base_url.rstrip("/") + query = [("scope", s) for s in self.scopes.scopes] + if self.scopes.satisfy != Satisfy.ALL: + query.append(("satisfy", self.scopes.satisfy.value)) + if self.delegate: + if self.delegate.notebook: + query.append(("notebook", "true")) + elif self.delegate.internal: + service = self.delegate.internal.service + query.append(("delegate_to", service)) + scopes = ",".join(self.delegate.internal.scopes) + query.append(("delegate_scope", scopes)) + if self.delegate.minimum_lifetime: + minimum_lifetime = self.delegate.minimum_lifetime + minimum_str = str(int(minimum_lifetime.total_seconds())) + query.append(("minimum_lifetime", minimum_str)) + if self.delegate.use_authorization: + query.append(("use_authorization", "true")) + if self.auth_type: + query.append(("auth_type", self.auth_type.value)) + return f"{base_url}/auth?" + urlencode(query) + class GafaelfawrIngressMetadata(CamelCaseModel): """Metadata used to create an ``Ingress`` object.""" diff --git a/src/gafaelfawr/services/kubernetes.py b/src/gafaelfawr/services/kubernetes.py index e1725241b..9602fa900 100644 --- a/src/gafaelfawr/services/kubernetes.py +++ b/src/gafaelfawr/services/kubernetes.py @@ -3,7 +3,6 @@ from __future__ import annotations from base64 import b64decode -from urllib.parse import urlencode from kubernetes_asyncio.client import ( V1Ingress, @@ -22,7 +21,6 @@ KubernetesError, PermissionDeniedError, ) -from ..models.auth import Satisfy from ..models.kubernetes import ( GafaelfawrIngress, GafaelfawrIngressScopesBase, @@ -107,29 +105,7 @@ async def update( def _build_annotations(self, ingress: GafaelfawrIngress) -> dict[str, str]: """Build annotations for an ``Ingress``.""" - base_url = ingress.config.base_url.rstrip("/") - - query = [("scope", s) for s in ingress.config.scopes.scopes] - if ingress.config.scopes.satisfy != Satisfy.ALL: - query.append(("satisfy", ingress.config.scopes.satisfy.value)) - if ingress.config.delegate: - if ingress.config.delegate.notebook: - query.append(("notebook", "true")) - elif ingress.config.delegate.internal: - service = ingress.config.delegate.internal.service - query.append(("delegate_to", service)) - scopes = ",".join(ingress.config.delegate.internal.scopes) - query.append(("delegate_scope", scopes)) - if ingress.config.delegate.minimum_lifetime: - minimum_lifetime = ingress.config.delegate.minimum_lifetime - minimum_str = str(int(minimum_lifetime.total_seconds())) - query.append(("minimum_lifetime", minimum_str)) - if ingress.config.delegate.use_authorization: - query.append(("use_authorization", "true")) - if ingress.config.auth_type: - query.append(("auth_type", ingress.config.auth_type.value)) - auth_url = f"{base_url}/auth?" + urlencode(query) - + auth_url = ingress.config.to_auth_url() snippet_key = "nginx.ingress.kubernetes.io/configuration-snippet" snippet = ingress.template.metadata.annotations.get(snippet_key, "") if snippet and not snippet.endswith("\n"): @@ -144,7 +120,7 @@ def _build_annotations(self, ingress: GafaelfawrIngress) -> dict[str, str]: snippet_key: snippet, } if ingress.config.login_redirect: - url = f"{base_url}/login" + url = ingress.config.base_url.rstrip("/") + "/login" annotations["nginx.ingress.kubernetes.io/auth-signin"] = url return annotations diff --git a/src/gafaelfawr/services/token.py b/src/gafaelfawr/services/token.py index 9f03517f1..9de38bf9a 100644 --- a/src/gafaelfawr/services/token.py +++ b/src/gafaelfawr/services/token.py @@ -4,6 +4,7 @@ import ipaddress import re +from collections.abc import Iterable from datetime import datetime, timedelta from safir.datetime import current_datetime, format_datetime_for_logging @@ -79,9 +80,7 @@ def __init__( self._token_change_store = token_change_store self._logger = logger - async def audit( # noqa: PLR0912,PLR0915,C901 - self, *, fix: bool = False - ) -> list[str]: + async def audit(self, *, fix: bool = False) -> list[str]: """Check Gafaelfawr data stores for consistency. If any errors are found and Slack is configured, report them to Slack @@ -98,15 +97,15 @@ async def audit( # noqa: PLR0912,PLR0915,C901 list of str A list of human-readable alert messages formatted in Markdown. """ + alert: str | None alerts = [] now = current_datetime() db_tokens = { t.token: t for t in await self._token_db_store.list_with_parents() } db_token_keys = set(db_tokens.keys()) - redis_token_keys = set(await self._token_redis_store.list()) redis_tokens = {} - for key in redis_token_keys: + for key in await self._token_redis_store.list(): token_data = await self._token_redis_store.get_data_by_key(key) if token_data: redis_tokens[key] = token_data @@ -147,83 +146,22 @@ async def audit( # noqa: PLR0912,PLR0915,C901 alert += " (fixed)" alerts.append(alert) - # Check that the data matches between the database and Redis. Older + # Check that the data matches between the database and Redis. Older # versions of Gafaelfawr didn't sort the scopes in Redis, so we have # to sort them here or we get false positives with old tokens. for key in db_token_keys & redis_token_keys: db = db_tokens[key] redis = redis_tokens[key] - mismatches = [] - if db.username != redis.username: - mismatches.append("username") - if db.token_type != redis.token_type: - mismatches.append("type") - if db.scopes != sorted(redis.scopes): - # There was a bug where Redis wasn't updated when the scopes - # were changed but the database was. Redis is canonical, so - # set the database scopes to match. - if fix: - await self._token_db_store.modify(key, scopes=redis.scopes) - mismatches.append("scopes [fixed]") - else: - mismatches.append("scopes") - if db.created != redis.created: - mismatches.append("created") - if db.expires != redis.expires: - mismatches.append("expires") - if mismatches: - self._logger.warning( - "Token does not match between database and Redis", - token=key, - user=redis.username, - mismatches=mismatches, - ) - alerts.append( - f"Token `{key}` for `{redis.username}` does not match" - f' between database and Redis ({", ".join(mismatches)})' - ) - if db.parent and db.parent in db_tokens: - parent = db_tokens[db.parent] - exp = db.expires - if parent.expires and (not exp or exp > parent.expires): - self._logger.warning( - "Token expires after its parent", - token=key, - user=redis.username, - expires=exp, - parent_expires=parent.expires, - ) - alerts.append( - f"Token `{key}` for `{redis.username}` expires after" - " its parent token" - ) + parent = db_tokens.get(db.parent) if db.parent else None + alerts.extend( + await self._audit_token(key, db, redis, parent, fix=fix) + ) # Check for orphaned tokens. - for token in await self._token_db_store.list_orphaned(): - self._logger.warning( - "Token has no parent", token=token.token, user=token.username - ) - alerts.append( - f"Token `{token.token}` for `{token.username}` has no parent" - " token" - ) + alerts.extend(await self._audit_orphaned()) # Check for unknown scopes. - for token_data in redis_tokens.values(): - known_scopes = set(self._config.known_scopes.keys()) - for scope in token_data.scopes: - if scope not in known_scopes: - self._logger.warning( - "Token has unknown scope", - token=token_data.token.key, - user=token_data.username, - scope=scope, - ) - alerts.append( - f"Token `{token_data.token.key}` for" - f" `{token_data.username}` has unknown scope" - f" (`{scope}`)" - ) + alerts.extend(self._audit_unknown_scopes(redis_tokens.values())) # Return any errors. return alerts @@ -1052,6 +990,133 @@ def _check_authorization( self._logger.warning("Permission denied", error=msg) raise PermissionDeniedError(msg) + async def _audit_orphaned(self) -> list[str]: + """Audit for orphaned tokens. + + Returns + ------- + list of str + Alerts to report. + """ + alerts = [] + for token in await self._token_db_store.list_orphaned(): + key = token.token + username = token.username + msg = "Token has no parent" + self._logger.warning(msg, token=key, user=username) + alert = f"Token `{key}` for `{username}` has no parent token" + alerts.append(alert) + return alerts + + async def _audit_token( + self, + key: str, + db: TokenInfo, + redis: TokenData, + parent: TokenInfo | None, + *, + fix: bool = False, + ) -> list[str]: + """Audit a single token. + + Compares the Redis data for a token against the database data for a + token and reports and optionally fixes any issues. + + Parameters + ---------- + key + Key of the token. + db + Database version of token. + parent + Database record of parent of token, if any. + redis + Redis version of token. + fix + Whether to fix any issues found. + + Returns + ------- + str or `None` + List of alerts to report. + """ + alerts = [] + mismatches = [] + if db.username != redis.username: + mismatches.append("username") + if db.token_type != redis.token_type: + mismatches.append("type") + if db.scopes != sorted(redis.scopes): + # There was a bug where Redis wasn't updated when the scopes were + # changed but the database was. Redis is canonical, so set the + # database scopes to match. + if fix: + await self._token_db_store.modify(key, scopes=redis.scopes) + mismatches.append("scopes [fixed]") + else: + mismatches.append("scopes") + if db.created != redis.created: + mismatches.append("created") + if db.expires != redis.expires: + mismatches.append("expires") + if mismatches: + self._logger.warning( + "Token does not match between database and Redis", + token=key, + user=redis.username, + mismatches=mismatches, + ) + alerts.append( + f"Token `{key}` for `{redis.username}` does not match" + f' between database and Redis ({", ".join(mismatches)})' + ) + if parent: + exp = db.expires + if parent.expires and (not exp or exp > parent.expires): + self._logger.warning( + "Token expires after its parent", + token=key, + user=redis.username, + expires=exp, + parent_expires=parent.expires, + ) + alerts.append( + f"Token `{key}` for `{redis.username}` expires after" + " its parent token" + ) + return alerts + + def _audit_unknown_scopes(self, tokens: Iterable[TokenData]) -> list[str]: + """Audit Redis tokens for unknown scopes. + + Parameters + ---------- + tokens + List of all Redis tokens to audit. + + Returns + ------- + list of str + List of alerts to report. + """ + alerts = [] + for token_data in tokens: + known_scopes = set(self._config.known_scopes.keys()) + for scope in token_data.scopes: + if scope not in known_scopes: + self._logger.warning( + "Token has unknown scope", + token=token_data.token.key, + user=token_data.username, + scope=scope, + ) + alerts.append( + f"Token `{token_data.token.key}` for" + f" `{token_data.username}` has unknown scope" + f" (`{scope}`)" + ) + return alerts + async def _delete_one_token( self, key: str, diff --git a/src/gafaelfawr/services/userinfo.py b/src/gafaelfawr/services/userinfo.py index fa1cdbbf1..14a9ea0a7 100644 --- a/src/gafaelfawr/services/userinfo.py +++ b/src/gafaelfawr/services/userinfo.py @@ -76,7 +76,7 @@ def __init__( self._firestore = firestore self._logger = logger - async def get_user_info_from_token( # noqa: PLR0912,C901 + async def get_user_info_from_token( self, token_data: TokenData ) -> TokenUserInfo: """Get the user information from a token. @@ -98,8 +98,7 @@ async def get_user_info_from_token( # noqa: PLR0912,C901 Raises ------ FirestoreError - UID/GID allocation using Firestore failed, probably because the UID - or GID space has been exhausted. + UID/GID allocation using Firestore failed. LDAPError Gafaelfawr was configured to get user groups, username, or numeric UID from LDAP, but the attempt failed due to some error. @@ -134,18 +133,7 @@ async def get_user_info_from_token( # noqa: PLR0912,C901 groups = token_data.groups if groups is None: - if self._firestore: - group_names = await self._ldap.get_group_names(username, gid) - groups = [] - for group_name in group_names: - try: - group_gid = await self._firestore.get_gid(group_name) - except FirestoreError as e: - e.user = username - raise - groups.append(TokenGroup(name=group_name, id=group_gid)) - else: - groups = await self._ldap.get_groups(username, gid) + groups = await self._get_groups_from_ldap(username, gid) # When adding the user private group, be careful not to change # the groups array, since it may be cached in the LDAP cache @@ -298,6 +286,52 @@ def _calculate_quota( api[service] = extra.api[service] return Quota(api=api, notebook=notebook) + async def _get_groups_from_ldap( + self, username: str, primary_gid: int | None + ) -> list[TokenGroup]: + """Get user group information from LDAP. + + Add GIDs from Firestore if configured to do so. + + Parameters + ---------- + username + Username of the user. + primary_gid + Primary GID if set. If not `None`, the user's groups will be + checked for this GID. If it's not found, search for the group with + this GID and add it to the user's group memberships. This handles + LDAP configurations where the user's primary group is represented + only by their GID and not their group memberships. + + Returns + ------- + list of TokenGroup + User's groups from LDAP. + + Raises + ------ + FirestoreError + GID allocation using Firestore failed. + LDAPError + Raised if some error occurred when searching LDAP. + """ + if not self._ldap: + raise RuntimeError("LDAP not configured") + if self._firestore: + names = await self._ldap.get_group_names(username, primary_gid) + groups = [] + for group_name in names: + try: + group_gid = await self._firestore.get_gid(group_name) + except FirestoreError as e: + e.user = username + raise + groups.append(TokenGroup(name=group_name, id=group_gid)) + return groups + else: + return await self._ldap.get_groups(username, primary_gid) + class OIDCUserInfoService(UserInfoService): """Retrieve user metadata from external systems for OIDC authentication. diff --git a/src/gafaelfawr/storage/ldap.py b/src/gafaelfawr/storage/ldap.py index 498df91bb..57f263c77 100644 --- a/src/gafaelfawr/storage/ldap.py +++ b/src/gafaelfawr/storage/ldap.py @@ -121,7 +121,7 @@ async def get_groups( return groups - async def get_data(self, username: str) -> LDAPUserData: # noqa: C901 + async def get_data(self, username: str) -> LDAPUserData: """Get the data for an LDAP user. Parameters @@ -148,20 +148,11 @@ async def get_data(self, username: str) -> LDAPUserData: # noqa: C901 search = f"({self._config.user_search_attr}={username})" logger = self._logger.bind(ldap_search=search, user=username) - attrs = [] - if self._config.name_attr: - attrs.append(self._config.name_attr) - if self._config.email_attr: - attrs.append(self._config.email_attr) - if self._config.uid_attr: - attrs.append(self._config.uid_attr) - if self._config.gid_attr: - attrs.append(self._config.gid_attr) results = await self._query( self._config.user_base_dn, bonsai.LDAPSearchScope.ONE, search, - attrs, + self._build_user_search_attrs(), username, ) logger.debug("LDAP entries for user data", ldap_results=results) @@ -217,6 +208,19 @@ def _build_group_member_search(self, username: str) -> str: target = username return f"(&(objectClass={group_class})({member_attr}={target}))" + def _build_user_search_attrs(self) -> list[str]: + """Construct the list of user attributes to search for.""" + attrs = [] + if self._config.name_attr: + attrs.append(self._config.name_attr) + if self._config.email_attr: + attrs.append(self._config.email_attr) + if self._config.uid_attr: + attrs.append(self._config.uid_attr) + if self._config.gid_attr: + attrs.append(self._config.gid_attr) + return attrs + async def _find_group_for_gid( self, gid: int, username: str ) -> TokenGroup | None: