diff --git a/src/palace/manager/api/routes.py b/src/palace/manager/api/routes.py index 53010f75a7..bf648e164a 100644 --- a/src/palace/manager/api/routes.py +++ b/src/palace/manager/api/routes.py @@ -567,8 +567,8 @@ def oidc_authenticate(): # While OIDC allows multiple redirect URIs to be registered (unlike SAML), # a constant callback URL means we only need to register one URI with each provider. # Library information is passed via the state parameter and processed in the controller. -@returns_problem_detail @app.route("/oidc/callback", methods=["GET"]) +@returns_problem_detail def oidc_callback(): return app.manager.oidc_controller.oidc_authentication_callback( flask.request.args, app.manager._db @@ -580,14 +580,15 @@ def oidc_callback(): @has_library @returns_problem_detail def oidc_logout(): + auth_header = flask.request.headers.get("Authorization", "") return app.manager.oidc_controller.oidc_logout_initiate( - flask.request.args, app.manager._db + flask.request.args, app.manager._db, auth_header=auth_header ) # Redirect URI for OIDC logout callback -@returns_problem_detail @app.route("/oidc/logout_callback", methods=["GET"]) +@returns_problem_detail def oidc_logout_callback(): return app.manager.oidc_controller.oidc_logout_callback( flask.request.args, app.manager._db @@ -622,8 +623,8 @@ def saml_authenticate(): # the IdP will fail this request because the URL mentioned in the request and # the URL saved in the SP's metadata configured in this IdP will differ. # Library's name is passed as a part of the relay state and processed in SAMLController.saml_authentication_callback -@returns_problem_detail @app.route("/saml_callback", methods=["POST"]) +@returns_problem_detail def saml_callback(): return app.manager.saml_controller.saml_authentication_callback( request, app.manager._db diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index 6511d3bc8c..0c59b78cd4 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -23,6 +23,7 @@ OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.util import ( + OIDCDiscoveryError, OIDCUtility, ) from palace.manager.integration.patron_auth.oidc.validator import ( @@ -257,9 +258,17 @@ def get_provider_metadata(self, use_cache: bool = True) -> dict[str, Any]: "jwks_uri": self._settings.jwks_uri, } - # Add optional userinfo endpoint + # Add optional endpoints if self._settings.userinfo_endpoint: self._metadata["userinfo_endpoint"] = self._settings.userinfo_endpoint + if self._settings.end_session_endpoint: + self._metadata["end_session_endpoint"] = str( + self._settings.end_session_endpoint + ) + if self._settings.revocation_endpoint: + self._metadata["revocation_endpoint"] = str( + self._settings.revocation_endpoint + ) return self._metadata @@ -477,6 +486,87 @@ def build_logout_url( self.log.info(f"Built logout URL for provider: {end_session_endpoint}") return logout_url + def _has_metadata_endpoints(self, *keys: str) -> bool: + """Return True if any of the given keys are present in provider metadata. + + Silently returns False if metadata discovery fails. + + :param keys: Metadata keys to check (e.g. ``"end_session_endpoint"``) + :return: True if any key has a truthy value in the discovered metadata + """ + try: + metadata = self.get_provider_metadata() + return any(metadata.get(k) for k in keys) + except OIDCDiscoveryError: + return False + + def supports_rp_initiated_logout(self) -> bool: + """Check if the OIDC provider supports RP-Initiated Logout. + + Returns True if an end_session_endpoint is available via auto-discovery + or manual configuration. + + :return: True if RP-Initiated Logout is supported + """ + return self._has_metadata_endpoints("end_session_endpoint") or bool( + self._settings.end_session_endpoint + ) + + def supports_logout(self) -> bool: + """Check if the OIDC provider supports any form of logout. + + Returns True if either an end_session_endpoint (RP-Initiated Logout) or + a revocation_endpoint (RFC 7009 token revocation) is available via + auto-discovery or manual configuration. + + :return: True if any logout mechanism is supported + """ + return self._has_metadata_endpoints( + "end_session_endpoint", "revocation_endpoint" + ) or bool( + self._settings.end_session_endpoint or self._settings.revocation_endpoint + ) + + def revoke_token(self, token: str, token_type_hint: str = "refresh_token") -> None: + """Revoke an OAuth 2.0 token via the token revocation endpoint (RFC 7009). + + This is a best-effort operation. If the provider does not support revocation + or the call fails, the error is logged and suppressed. + + :param token: Token to revoke + :param token_type_hint: Type hint for the token ('refresh_token' or 'access_token') + """ + try: + metadata = self.get_provider_metadata() + except OIDCDiscoveryError: + self.log.debug("Cannot revoke token: failed to get provider metadata") + return + + revocation_endpoint = metadata.get("revocation_endpoint") + if not revocation_endpoint: + self.log.debug( + "Provider does not support token revocation (no revocation_endpoint)" + ) + return + + data: dict[str, str] = { + "token": token, + "token_type_hint": token_type_hint, + } + auth = self._prepare_token_endpoint_auth(data) + + try: + HTTP.post_with_timeout( + str(revocation_endpoint), + data=data, + auth=auth, + headers={"Accept": "application/json"}, + allowed_response_codes=["2xx"], + ) + self.log.info(f"Successfully revoked {token_type_hint}") + except (RequestNetworkException, RequestException): + self.log.warning("Token revocation failed (non-critical)", exc_info=True) + def validate_id_token_hint(self, id_token: str) -> dict[str, Any]: """Validate ID token hint for logout. diff --git a/src/palace/manager/integration/patron_auth/oidc/configuration/model.py b/src/palace/manager/integration/patron_auth/oidc/configuration/model.py index 7c01bb4428..dd375770e4 100644 --- a/src/palace/manager/integration/patron_auth/oidc/configuration/model.py +++ b/src/palace/manager/integration/patron_auth/oidc/configuration/model.py @@ -157,12 +157,25 @@ class OIDCAuthSettings(AuthProviderSettings, LoggerMixin): end_session_endpoint: Annotated[ HttpUrl | None, FormMetadata( - label=_("End Session Endpoint (Optional)"), + label=_("End Session Endpoint (Manual Mode - Optional)"), description=_( "OIDC provider's end session endpoint URL for RP-Initiated Logout. " "Optional - enables logout functionality if supported by provider. " "Automatically discovered if Issuer URL is provided. " - "Example: https://accounts.google.com/o/oauth2/revoke" + "Example: https://login.microsoftonline.com/common/oauth2/v2.0/logout" + ), + ), + ] = None + + revocation_endpoint: Annotated[ + HttpUrl | None, + FormMetadata( + label=_("Revocation Endpoint (Manual Mode - Optional)"), + description=_( + "OIDC provider's token revocation endpoint URL (RFC 7009). " + "Used to revoke access and refresh tokens on logout. " + "Automatically discovered if Issuer URL is provided. " + "Example: https://oauth2.googleapis.com/revoke" ), ), ] = None @@ -417,6 +430,7 @@ def validate_configuration_mode(self) -> OIDCAuthSettings: "jwks_uri", "userinfo_endpoint", "end_session_endpoint", + "revocation_endpoint", ) @classmethod def validate_url_fields( diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index d3d4f831e6..eacc532b74 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -6,14 +6,23 @@ from typing import TYPE_CHECKING from urllib.parse import SplitResult, parse_qs, urlencode, urlsplit +import jwt from flask import redirect, url_for from flask_babel import lazy_gettext as _ +from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session from werkzeug.wrappers import Response as BaseResponse from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider from palace.manager.api.util.flask import get_request_library -from palace.manager.integration.patron_auth.oidc.util import OIDCUtility +from palace.manager.integration.patron_auth.oidc.auth import OIDCAuthenticationError +from palace.manager.integration.patron_auth.oidc.credential import OIDCCredentialManager +from palace.manager.integration.patron_auth.oidc.util import ( + LOGOUT_REDIRECT_QUERY_PARAM, + OIDCDiscoveryError, + OIDCStateValidationError, + OIDCUtility, +) from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import ( ProblemDetail, @@ -54,13 +63,6 @@ detail=_("The OIDC provider does not support RP-Initiated Logout."), ) -OIDC_INVALID_ID_TOKEN_HINT = pd( - "http://palaceproject.io/terms/problem/auth/unrecoverable/oidc/invalid-id-token-hint", - status_code=400, - title=_("Invalid ID token hint."), - detail=_("The ID token hint is missing or invalid."), -) - OIDC_INVALID_LOGOUT_TOKEN = pd( "http://palaceproject.io/terms/problem/auth/unrecoverable/oidc/invalid-logout-token", status_code=400, @@ -86,8 +88,6 @@ class OIDCController(LoggerMixin): CODE = "code" ACCESS_TOKEN = "access_token" PATRON_INFO = "patron_info" - ID_TOKEN_HINT = "id_token_hint" - POST_LOGOUT_REDIRECT_URI = "post_logout_redirect_uri" LOGOUT_STATUS = "logout_status" LOGOUT_TOKEN = "logout_token" @@ -326,6 +326,7 @@ def oidc_authentication_callback( tokens.get("access_token"), tokens.get("refresh_token"), tokens.get("expires_in"), + id_token, ) except ProblemDetailException as e: return self._redirect_with_error(redirect_uri, e.problem_detail) @@ -345,28 +346,27 @@ def oidc_authentication_callback( return redirect(final_redirect_uri) def oidc_logout_initiate( - self, request_args: dict[str, str], db: Session + self, + request_args: dict[str, str], + db: Session, + *, + auth_header: str, ) -> BaseResponse | ProblemDetail: """Initiate OIDC RP-Initiated Logout flow. :param request_args: Request arguments from Flask :param db: Database session + :param auth_header: "Authorization" header from Flask or empty string :return: Redirect to provider logout endpoint or error """ provider_name = request_args.get(self.PROVIDER_NAME) - id_token_hint = request_args.get(self.ID_TOKEN_HINT) - post_logout_redirect_uri = request_args.get(self.POST_LOGOUT_REDIRECT_URI) + post_logout_redirect_uri = request_args.get(LOGOUT_REDIRECT_QUERY_PARAM) if not provider_name: return OIDC_INVALID_REQUEST.detailed( _("Missing 'provider' parameter in logout request") ) - if not id_token_hint: - return OIDC_INVALID_ID_TOKEN_HINT.detailed( - _("Missing 'id_token_hint' parameter in logout request") - ) - if not post_logout_redirect_uri: return OIDC_INVALID_REQUEST.detailed( _("Missing 'post_logout_redirect_uri' parameter in logout request") @@ -381,25 +381,55 @@ def oidc_logout_initiate( _("No authenticator found for library") ) + # Validate CM bearer token — ensures patron has an active CM session. + if not auth_header.startswith("Bearer "): + return OIDC_INVALID_REQUEST.detailed( + _("Missing or invalid Authorization header") + ) + + cm_jwt = auth_header.removeprefix("Bearer ") + try: + decoded_provider_name, provider_token = ( + library_authenticator.decode_bearer_token(cm_jwt) + ) + except jwt.exceptions.InvalidTokenError: + self.log.warning("Invalid bearer token in logout request", exc_info=True) + return OIDC_INVALID_REQUEST.detailed(_("Invalid bearer token")) + + if decoded_provider_name != provider_name: + return OIDC_INVALID_REQUEST.detailed(_("Provider mismatch in bearer token")) + provider = library_authenticator.oidc_provider_lookup(provider_name) if isinstance(provider, ProblemDetail): return provider + auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] + + # The bearer token's payload IS the credential JSON — parse it directly. + # This avoids a DB lookup that would fail after token refresh (the credential + # value changes on refresh, but the patron's bearer token still has the old value). try: - auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] - claims = auth_manager.validate_id_token_hint(id_token_hint) - except Exception as e: - self.log.exception("ID token hint validation failed") - return OIDC_INVALID_ID_TOKEN_HINT.detailed( - _(f"ID token hint validation failed: {str(e)}") - ) + token_data = OIDCCredentialManager.parse_token_value(provider_token) + except ValueError: + self.log.exception("Failed to parse token data from bearer token") + return OIDC_INVALID_REQUEST.detailed(_("Invalid credential data")) + + id_token_claims = token_data["id_token_claims"] + patron_identifier = id_token_claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] + + # Best-effort: revoke access and refresh tokens to prevent silent re-authentication + # at the IdP after local CM logout. revoke_token suppresses all errors internally. + access_token = token_data["access_token"] + auth_manager.revoke_token(access_token, "access_token") + if refresh_token := token_data.get("refresh_token"): + auth_manager.revoke_token(refresh_token) - patron_identifier = claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] if not patron_identifier: - return OIDC_INVALID_ID_TOKEN_HINT.detailed( - _("ID token hint missing patron identifier claim") + return OIDC_INVALID_REQUEST.detailed( + _("Credential missing patron identifier claim") ) + # Invalidate our local patron credentials. try: credential_manager = provider._credential_manager # type: ignore[attr-defined] patron = credential_manager.lookup_patron_by_identifier( @@ -411,10 +441,30 @@ def oidc_logout_initiate( self.log.info(f"Invalidated credentials for patron {patron_identifier}") else: self.log.warning(f"Patron not found for identifier {patron_identifier}") - - except Exception as e: + except (SQLAlchemyError, AttributeError): self.log.exception("Failed to invalidate credentials") + # If the provider does not support RP-Initiated Logout (only token revocation), + # redirect directly — local invalidation and token revocation are already done. + if not auth_manager.supports_rp_initiated_logout(): + self.log.info("Skipping RP-Initiated Logout: provider does not support it.") + final_redirect_uri = self._add_params_to_url( + post_logout_redirect_uri, {self.LOGOUT_STATUS: "success"} + ) + return redirect(final_redirect_uri) + + # If the raw id_token JWT (stored at authentication time) is not present, + # skip RP-Initiated Logout. The provider session may remain active. + if not (stored_id_token := token_data.get("id_token")): + self.log.warning( + "Skipping RP-Initiated Logout: no id_token stored in credential. " + "Provider session may remain active." + ) + final_redirect_uri = self._add_params_to_url( + post_logout_redirect_uri, {self.LOGOUT_STATUS: "partial"} + ) + return redirect(final_redirect_uri) + callback_url = url_for("oidc_logout_callback", _external=True) logout_state_data = { @@ -431,10 +481,11 @@ def oidc_logout_initiate( try: logout_url = auth_manager.build_logout_url( - id_token_hint, callback_url, logout_state + stored_id_token, callback_url, logout_state ) - except Exception as e: + except (OIDCDiscoveryError, OIDCAuthenticationError) as e: self.log.exception("Failed to build logout URL") + utility.delete_logout_state(logout_state) return OIDC_LOGOUT_NOT_SUPPORTED.detailed(_(str(e))) return redirect(logout_url) @@ -469,7 +520,15 @@ def oidc_logout_callback( _("Missing redirect_uri in logout state") ) - library_short_name = logout_data.get("library_short_name") + # Decode the (unverified) state payload to get library_short_name, which + # is needed to look up the signing secret before full validation. + try: + state_payload = OIDCUtility.decode_state_payload(state) + except OIDCStateValidationError as e: + self.log.exception("Failed to decode logout state payload") + return OIDC_INVALID_STATE.detailed(_(str(e))) + + library_short_name = state_payload.get("library_short_name") if not library_short_name: return OIDC_INVALID_STATE.detailed(_("Missing library in logout state")) @@ -485,7 +544,7 @@ def oidc_logout_callback( state_data = OIDCUtility.validate_state( state, library_authenticator.bearer_token_signing_secret ) - except Exception as e: + except OIDCStateValidationError as e: self.log.exception("Logout state validation failed") return OIDC_INVALID_STATE.detailed(_(f"State validation failed: {str(e)}")) diff --git a/src/palace/manager/integration/patron_auth/oidc/credential.py b/src/palace/manager/integration/patron_auth/oidc/credential.py index 5493603fcf..221a901ceb 100644 --- a/src/palace/manager/integration/patron_auth/oidc/credential.py +++ b/src/palace/manager/integration/patron_auth/oidc/credential.py @@ -17,6 +17,7 @@ from sqlalchemy import and_, exists from sqlalchemy.orm import Session +from palace.manager.core.exceptions import PalaceValueError from palace.manager.integration.patron_auth.oidc.auth import ( OIDCAuthenticationManager, OIDCRefreshTokenError, @@ -57,24 +58,50 @@ def _create_token_value( id_token_claims: dict[str, Any], access_token: str, refresh_token: str | None = None, + id_token: str | None = None, ) -> str: """Create OIDC token value by serializing token data. :param id_token_claims: Validated ID token claims :param access_token: Access token :param refresh_token: Optional refresh token + :param id_token: Raw ID token JWT (stored for use as id_token_hint on logout) :return: JSON-serialized token data """ - token_data = { + token_data: dict[str, Any] = { "id_token_claims": id_token_claims, "access_token": access_token, } if refresh_token: token_data["refresh_token"] = refresh_token + if id_token: + token_data["id_token"] = id_token return json.dumps(token_data) + @staticmethod + def parse_token_value(value: str) -> dict[str, Any]: + """Parse and validate a raw OIDC credential JSON string. + + :param value: JSON string produced by :meth:`_create_token_value` + :raises PalaceValueError: If the string is not valid JSON or is missing required fields. + `PalaceValueError` is a subclass of `ValueError`, so callers that catch + `ValueError` will still handle it correctly. + :return: Parsed token data dictionary + """ + try: + token_data = cast(dict[str, Any], json.loads(value)) + except json.JSONDecodeError as e: + raise PalaceValueError(f"Invalid OIDC token format: {str(e)}") from e + + if not isinstance(token_data.get("id_token_claims"), dict): + raise PalaceValueError("OIDC token missing or invalid id_token_claims") + if "access_token" not in token_data: + raise PalaceValueError("OIDC token missing access_token") + + return token_data + def extract_token_data(self, credential: Credential) -> dict[str, Any]: """Extract token data from credential. @@ -82,22 +109,12 @@ def extract_token_data(self, credential: Credential) -> dict[str, Any]: :return: Dictionary with id_token_claims, access_token, refresh_token """ self.log.debug(f"Extracting OIDC token data from credential {credential.id}") - credential_value = credential.credential if credential.credential else "{}" - try: - token_data = cast(dict[str, Any], json.loads(credential_value)) - except json.JSONDecodeError as e: + return self.parse_token_value(credential_value) + except ValueError as e: self.log.exception("Failed to decode OIDC token data") - raise ValueError(f"Invalid OIDC token format: {str(e)}") from e - - # Validate structure - if "id_token_claims" not in token_data: - raise ValueError("OIDC token missing id_token_claims") - if "access_token" not in token_data: - raise ValueError("OIDC token missing access_token") - - return token_data + raise def create_oidc_token( self, @@ -108,6 +125,7 @@ def create_oidc_token( refresh_token: str | None = None, expires_in: int | None = None, session_lifetime_days: int | None = None, + id_token: str | None = None, ) -> Credential: """Create a Credential object for OIDC tokens. @@ -118,6 +136,7 @@ def create_oidc_token( :param refresh_token: Optional refresh token :param expires_in: Token lifetime in seconds (from provider) :param session_lifetime_days: Override session lifetime in days + :param id_token: Raw ID token JWT (stored for use as id_token_hint on logout) :return: Created Credential object """ # Calculate expiry @@ -138,7 +157,7 @@ def create_oidc_token( # Create token value token_value = self._create_token_value( - id_token_claims, access_token, refresh_token + id_token_claims, access_token, refresh_token, id_token ) # Get data source @@ -279,25 +298,26 @@ def refresh_token_if_needed( self.log.exception("Failed to refresh OIDC token") raise - # Validate new ID token if present - new_id_token_claims = token_data[ - "id_token_claims" - ] # Keep old claims as fallback + # Validate new ID token if present; fall back to stored values. + new_id_token_claims = token_data["id_token_claims"] + new_id_token = token_data.get("id_token") if "id_token" in new_tokens: try: # Validate new ID token (no nonce check for refresh) new_id_token_claims = auth_manager.validate_id_token( new_tokens["id_token"], nonce=None ) + new_id_token = new_tokens["id_token"] except Exception as e: self.log.warning(f"Failed to validate refreshed ID token: {e}") - # Keep using old claims if new token validation fails + # Keep using old claims and token if new token validation fails # Update credential with new tokens new_token_value = self._create_token_value( new_id_token_claims, new_tokens["access_token"], new_tokens.get("refresh_token", refresh_token), # Use new or keep old + new_id_token, ) credential.credential = new_token_value diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index f82f190722..466a3dbb29 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -15,12 +15,17 @@ from palace.manager.api.authentication.base import PatronData, PatronLookupNotSupported from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider +from palace.manager.core.exceptions import PalaceValueError from palace.manager.integration.patron_auth.oidc.auth import OIDCAuthenticationManager from palace.manager.integration.patron_auth.oidc.configuration.model import ( OIDCAuthLibrarySettings, OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.credential import OIDCCredentialManager +from palace.manager.integration.patron_auth.oidc.util import ( + LOGOUT_REDIRECT_QUERY_PARAM, + OIDCDiscoveryError, +) from palace.manager.service.analytics.analytics import Analytics from palace.manager.sqlalchemy.model.credential import Credential from palace.manager.sqlalchemy.model.patron import Patron @@ -32,6 +37,15 @@ if TYPE_CHECKING: from palace.manager.core.selftest import SelfTestResult +# TODO: These OPDS constants are provider-agnostic and should be moved to a shared +# constants module if other authentication providers (e.g., SAML) need them. +OPDS_URI_TEMPLATE_VARIABLES_PROPERTY = "uri_template_variables" +OPDS_URI_TEMPLATE_VARIABLES_TYPE = ( + "http://palaceproject.io/terms/uri-template/variables" +) + +PALACE_REDIRECT_URI_TERM = "http://palaceproject.io/terms/redirect-uri" + OIDC_CANNOT_DETERMINE_PATRON = pd( "http://palaceproject.io/terms/problem/auth/unrecoverable/oidc/cannot-identify-patron", status_code=401, @@ -79,6 +93,7 @@ def __init__( self._credential_manager = OIDCCredentialManager() self._settings = settings + self._auth_manager: OIDCAuthenticationManager | None = None @classmethod def label(cls) -> str: @@ -166,7 +181,7 @@ def _authentication_flow_document(self, db: Session) -> dict[str, Any]: """ library = self.library(db) if not library: - raise ValueError("Library not found") + raise PalaceValueError("Library not found") authenticate_url = url_for( "oidc_authenticate", @@ -174,12 +189,38 @@ def _authentication_flow_document(self, db: Session) -> dict[str, Any]: library_short_name=library.short_name, provider=self.label(), ) - link = self._create_authentication_link(authenticate_url) + links: list[dict[str, Any]] = [ + self._create_authentication_link(authenticate_url) + ] + + auth_manager = self.get_authentication_manager() + if auth_manager.supports_logout(): + logout_url = url_for( + "oidc_logout", + _external=True, + library_short_name=library.short_name, + provider=self.label(), + ) + links.append( + { + "rel": "logout", + "href": f"{logout_url}{{&{LOGOUT_REDIRECT_QUERY_PARAM}}}", + "templated": True, + "properties": { + OPDS_URI_TEMPLATE_VARIABLES_PROPERTY: { + "type": OPDS_URI_TEMPLATE_VARIABLES_TYPE, + "map": { + LOGOUT_REDIRECT_QUERY_PARAM: PALACE_REDIRECT_URI_TERM, + }, + }, + }, + } + ) return { "type": self.flow_type, "description": self.label(), - "links": [link], + "links": links, } def _run_self_tests(self, db: Session) -> Generator[SelfTestResult]: @@ -219,9 +260,27 @@ def authenticated_patron( def get_authentication_manager(self) -> OIDCAuthenticationManager: """Return OIDC authentication manager for this provider. + The manager is cached once provider metadata loads successfully. If + discovery fails — for example because the IdP is temporarily unreachable + — the manager is returned uncached so the next call retries from scratch. + :return: OIDC authentication manager """ - return OIDCAuthenticationManager(self._settings) + if self._auth_manager is not None: + return self._auth_manager + + manager = OIDCAuthenticationManager(self._settings) + try: + manager.get_provider_metadata() + except OIDCDiscoveryError as e: + self.log.warning( + f"Failed to configure OIDC authentication manager: {e}. " + "Will retry on next request." + ) + return manager + + self._auth_manager = manager + return self._auth_manager def remote_patron_lookup_from_oidc_claims( self, id_token_claims: dict[str, str] @@ -278,6 +337,7 @@ def oidc_callback( access_token: str, refresh_token: str | None = None, expires_in: int | None = None, + id_token: str | None = None, ) -> tuple[Credential, Patron, PatronData]: """Handle OIDC callback after successful authentication. @@ -286,6 +346,7 @@ def oidc_callback( :param access_token: Access token from token exchange :param refresh_token: Optional refresh token :param expires_in: Token expiry in seconds + :param id_token: Raw ID token JWT (stored for use as id_token_hint on logout) :return: 3-tuple (Credential, Patron, PatronData) """ patron_data = self.remote_patron_lookup_from_oidc_claims(id_token_claims) @@ -302,6 +363,7 @@ def oidc_callback( refresh_token, expires_in, self._settings.session_lifetime, + id_token, ) return credential, patron, patron_data diff --git a/src/palace/manager/integration/patron_auth/oidc/util.py b/src/palace/manager/integration/patron_auth/oidc/util.py index 301ac44e1c..0d924757c3 100644 --- a/src/palace/manager/integration/patron_auth/oidc/util.py +++ b/src/palace/manager/integration/patron_auth/oidc/util.py @@ -27,6 +27,9 @@ from palace.manager.util.http.http import HTTP from palace.manager.util.log import LoggerMixin +# OIDC RP-Initiated Logout parameter name (per spec). +LOGOUT_REDIRECT_QUERY_PARAM = "post_logout_redirect_uri" + class OIDCUtilityError(BasePalaceException): """Base exception for OIDC utility errors.""" @@ -235,6 +238,26 @@ def generate_state(data: dict[str, Any], secret: str) -> str: # Combine: {signature}.{data} return f"{encoded_signature}.{encoded_data}" + @staticmethod + def decode_state_payload(state: str) -> dict[str, Any]: + """Decode a state token payload without verifying the signature. + + Useful for extracting fields (e.g. `library_short_name`) needed to + look up the signing secret before full validation via `validate_state`. + + :param state: Signed state token + :raises OIDCStateValidationError: If the token is malformed + :return: Decoded payload + """ + try: + _, encoded_data = state.split(".", 1) + json_data = base64.urlsafe_b64decode(encoded_data).decode("utf-8") + return cast(dict[str, Any], json.loads(json_data)) + except (ValueError, UnicodeDecodeError, json.JSONDecodeError) as e: + raise OIDCStateValidationError( + f"Invalid state parameter format: {str(e)}" + ) from e + @classmethod def validate_state( cls, state: str, secret: str, max_age: int | None = None diff --git a/tests/manager/api/test_routes.py b/tests/manager/api/test_routes.py index e45cc1a29d..e8994030da 100644 --- a/tests/manager/api/test_routes.py +++ b/tests/manager/api/test_routes.py @@ -463,7 +463,7 @@ def test_health_check(self, route_test: RouteTestFixture): class TestOIDCRoutes: @pytest.mark.parametrize( - "route_func,patch_method,query_string,expected_params", + "route_func,patch_method,query_string,expected_params,passes_headers", [ pytest.param( routes.oidc_authenticate, @@ -473,6 +473,7 @@ class TestOIDCRoutes: "provider": "OpenID Connect", "redirect_uri": "https://app.example.com", }, + False, id="authenticate", ), pytest.param( @@ -480,6 +481,7 @@ class TestOIDCRoutes: "oidc_authentication_callback", "/?code=test-code&state=test-state", {"code": "test-code", "state": "test-state"}, + False, id="callback", ), pytest.param( @@ -491,6 +493,7 @@ class TestOIDCRoutes: "id_token_hint": "test-token", "post_logout_redirect_uri": "https://app.example.com", }, + True, id="logout", ), pytest.param( @@ -498,6 +501,7 @@ class TestOIDCRoutes: "oidc_logout_callback", "/?state=test-logout-state", {"state": "test-logout-state"}, + False, id="logout-callback", ), ], @@ -508,6 +512,7 @@ def test_oidc_route( patch_method, query_string, expected_params, + passes_headers, controller_fixture: ControllerFixture, ): """Test OIDC route controller logic.""" @@ -528,6 +533,8 @@ def test_oidc_route( assert isinstance(call_args[0][0], ImmutableMultiDict) for key, value in expected_params.items(): assert call_args[0][0][key] == value + if passes_headers: + assert isinstance(call_args.kwargs.get("auth_header"), str) def test_oidc_backchannel_logout_route(self, controller_fixture: ControllerFixture): """Test the /oidc/backchannel_logout route controller logic.""" diff --git a/tests/manager/integration/patron_auth/oidc/conftest.py b/tests/manager/integration/patron_auth/oidc/conftest.py index 08a7a14c1f..a13dd0a69b 100644 --- a/tests/manager/integration/patron_auth/oidc/conftest.py +++ b/tests/manager/integration/patron_auth/oidc/conftest.py @@ -2,9 +2,12 @@ from __future__ import annotations +from unittest.mock import patch + import pytest from pydantic import HttpUrl +from palace.manager.integration.patron_auth.oidc.auth import OIDCAuthenticationManager from palace.manager.integration.patron_auth.oidc.configuration.model import ( OIDCAuthLibrarySettings, OIDCAuthSettings, @@ -32,12 +35,19 @@ def oidc_provider(db: DatabaseTransactionFixture) -> OIDCAuthenticationProvider: client_secret="test-client-secret", ) library_settings = OIDCAuthLibrarySettings() - return OIDCAuthenticationProvider( + provider = OIDCAuthenticationProvider( library_id=library.id, integration_id=1, settings=settings, library_settings=library_settings, ) + # Pre-configure the auth manager to prevent live OIDC discovery calls in tests. + # Tests that exercise the initialization path must reset _auth_manager = None first. + with patch.object( + OIDCAuthenticationManager, "get_provider_metadata", return_value={} + ): + provider.get_authentication_manager() + return provider @pytest.fixture diff --git a/tests/manager/integration/patron_auth/oidc/test_auth.py b/tests/manager/integration/patron_auth/oidc/test_auth.py index 5d57a414ae..3e626b1b27 100644 --- a/tests/manager/integration/patron_auth/oidc/test_auth.py +++ b/tests/manager/integration/patron_auth/oidc/test_auth.py @@ -8,6 +8,7 @@ from urllib.parse import quote import pytest +from pydantic import HttpUrl from requests import RequestException from palace.manager.integration.patron_auth.oidc.auth import ( @@ -19,6 +20,7 @@ from palace.manager.integration.patron_auth.oidc.configuration.model import ( OIDCAuthSettings, ) +from palace.manager.integration.patron_auth.oidc.util import OIDCDiscoveryError from palace.manager.util.http.exception import ( RequestNetworkException, ) @@ -119,6 +121,54 @@ def test_get_provider_metadata_with_manual_config( assert str(metadata["jwks_uri"]) == f"{TEST_ISSUER_URL}/.well-known/jwks.json" assert str(metadata["userinfo_endpoint"]) == f"{TEST_ISSUER_URL}/userinfo" assert metadata["issuer"] == TEST_ISSUER_URL + assert "end_session_endpoint" not in metadata + assert "revocation_endpoint" not in metadata + + def test_get_provider_metadata_with_manual_config_and_end_session_endpoint( + self, redis_fixture + ): + """Test that end_session_endpoint from settings is included in manual mode metadata.""" + end_session_url = f"{TEST_ISSUER_URL}/logout" + settings = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer=TEST_ISSUER_URL, + authorization_endpoint=HttpUrl(f"{TEST_ISSUER_URL}/authorize"), + token_endpoint=HttpUrl(f"{TEST_ISSUER_URL}/token"), + jwks_uri=HttpUrl(f"{TEST_ISSUER_URL}/.well-known/jwks.json"), + end_session_endpoint=HttpUrl(end_session_url), + ) + manager = OIDCAuthenticationManager( + settings=settings, + redis_client=redis_fixture.client, + ) + + metadata = manager.get_provider_metadata() + + assert metadata["end_session_endpoint"] == end_session_url + + def test_get_provider_metadata_with_manual_config_and_revocation_endpoint( + self, redis_fixture + ): + """Test that revocation_endpoint from settings is included in manual mode metadata.""" + revocation_url = f"{TEST_ISSUER_URL}/revoke" + settings = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer=TEST_ISSUER_URL, + authorization_endpoint=HttpUrl(f"{TEST_ISSUER_URL}/authorize"), + token_endpoint=HttpUrl(f"{TEST_ISSUER_URL}/token"), + jwks_uri=HttpUrl(f"{TEST_ISSUER_URL}/.well-known/jwks.json"), + revocation_endpoint=HttpUrl(revocation_url), + ) + manager = OIDCAuthenticationManager( + settings=settings, + redis_client=redis_fixture.client, + ) + + metadata = manager.get_provider_metadata() + + assert metadata["revocation_endpoint"] == revocation_url def test_get_provider_metadata_caching( self, oidc_settings_with_discovery, redis_fixture, mock_discovery_document @@ -1507,3 +1557,273 @@ def mock_get_side_effect(url, **kwargs): with pytest.raises(OIDCAuthenticationError, match=error_match): manager.validate_logout_token(token) + + +class TestOIDCAuthManagerSupportsRpInitiatedLogout: + """Tests for OIDCAuthenticationManager.supports_rp_initiated_logout().""" + + @pytest.mark.parametrize( + "metadata,settings_kwargs,expected", + [ + pytest.param( + {"end_session_endpoint": "https://idp.example.com/logout"}, + {}, + True, + id="discovered-end-session-endpoint", + ), + pytest.param( + {"revocation_endpoint": "https://idp.example.com/revoke"}, + {}, + False, + id="revocation-endpoint-only-not-rp-initiated", + ), + pytest.param( + {}, + {"end_session_endpoint": HttpUrl("https://idp.example.com/logout")}, + True, + id="manual-end-session-endpoint", + ), + pytest.param( + {}, + {"revocation_endpoint": HttpUrl("https://idp.example.com/revoke")}, + False, + id="manual-revocation-only-not-rp-initiated", + ), + pytest.param( + {}, + {}, + False, + id="neither", + ), + ], + ) + def test_supports_rp_initiated_logout( + self, + oidc_settings_with_discovery, + metadata, + settings_kwargs, + expected, + ): + """Test supports_rp_initiated_logout only returns True for end_session_endpoint.""" + if settings_kwargs: + oidc_settings_with_discovery = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer_url=TEST_ISSUER_URL, + **settings_kwargs, + ) + + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + + with patch.object(manager, "get_provider_metadata", return_value=metadata): + result = manager.supports_rp_initiated_logout() + + assert result is expected + + +class TestOIDCAuthManagerSupportsLogout: + """Tests for OIDCAuthenticationManager.supports_logout().""" + + @pytest.mark.parametrize( + "metadata,settings_kwargs,expected", + [ + pytest.param( + {"end_session_endpoint": "https://idp.example.com/logout"}, + {}, + True, + id="discovered-end-session-endpoint", + ), + pytest.param( + {"revocation_endpoint": "https://idp.example.com/revoke"}, + {}, + True, + id="discovered-revocation-endpoint-only", + ), + pytest.param( + { + "end_session_endpoint": "https://idp.example.com/logout", + "revocation_endpoint": "https://idp.example.com/revoke", + }, + {}, + True, + id="discovered-both-endpoints", + ), + pytest.param( + {}, + {"end_session_endpoint": HttpUrl("https://idp.example.com/logout")}, + True, + id="no-discovered-endpoint-manual-end-session-present", + ), + pytest.param( + {}, + {"revocation_endpoint": HttpUrl("https://idp.example.com/revoke")}, + True, + id="no-discovered-endpoint-manual-revocation-present", + ), + pytest.param( + {}, + {}, + False, + id="neither-discovered-nor-manual", + ), + ], + ) + def test_supports_logout( + self, + oidc_settings_with_discovery, + metadata, + settings_kwargs, + expected, + ): + """Test supports_logout with various discovery and manual config combinations.""" + if settings_kwargs: + oidc_settings_with_discovery = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer_url=TEST_ISSUER_URL, + **settings_kwargs, + ) + + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + + with patch.object(manager, "get_provider_metadata", return_value=metadata): + result = manager.supports_logout() + + assert result is expected + + @pytest.mark.parametrize( + "settings_kwargs,expected", + [ + pytest.param( + {"end_session_endpoint": HttpUrl("https://idp.example.com/logout")}, + True, + id="discovery-error-manual-end-session-present", + ), + pytest.param( + {"revocation_endpoint": HttpUrl("https://idp.example.com/revoke")}, + True, + id="discovery-error-manual-revocation-present", + ), + pytest.param( + {}, + False, + id="discovery-error-no-manual-setting", + ), + ], + ) + def test_supports_logout_discovery_error( + self, + oidc_settings_with_discovery, + settings_kwargs, + expected, + ): + """Test supports_logout falls back to manual settings on OIDCDiscoveryError.""" + if settings_kwargs: + oidc_settings_with_discovery = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer_url=TEST_ISSUER_URL, + **settings_kwargs, + ) + + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + + with patch.object( + manager, + "get_provider_metadata", + side_effect=OIDCDiscoveryError("Discovery failed"), + ): + result = manager.supports_logout() + + assert result is expected + + +class TestOIDCAuthManagerRevokeToken: + """Tests for OIDCAuthenticationManager.revoke_token().""" + + @pytest.mark.parametrize( + "metadata,token_type_hint,expect_http_call", + [ + pytest.param( + {"revocation_endpoint": "https://idp.example.com/revoke"}, + "refresh_token", + True, + id="revocation-endpoint-present-refresh-token", + ), + pytest.param( + {"revocation_endpoint": "https://idp.example.com/revoke"}, + "access_token", + True, + id="revocation-endpoint-present-access-token", + ), + pytest.param( + {}, + "refresh_token", + False, + id="no-revocation-endpoint", + ), + ], + ) + def test_revoke_token( + self, + oidc_settings_with_discovery, + metadata, + token_type_hint, + expect_http_call, + ): + """Test revoke_token with various metadata configurations.""" + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + + with ( + patch.object(manager, "get_provider_metadata", return_value=metadata), + patch( + "palace.manager.integration.patron_auth.oidc.auth.HTTP.post_with_timeout" + ) as mock_post, + ): + manager.revoke_token("test-token", token_type_hint) + + if expect_http_call: + mock_post.assert_called_once() + call_kwargs = mock_post.call_args + assert call_kwargs[0][0] == "https://idp.example.com/revoke" + posted_data = call_kwargs[1]["data"] + assert posted_data["token"] == "test-token" + assert posted_data["token_type_hint"] == token_type_hint + else: + mock_post.assert_not_called() + + def test_revoke_token_http_failure_is_non_fatal(self, oidc_settings_with_discovery): + """Test that HTTP failure during revocation is logged but not raised.""" + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + metadata = {"revocation_endpoint": "https://idp.example.com/revoke"} + + with ( + patch.object(manager, "get_provider_metadata", return_value=metadata), + patch( + "palace.manager.integration.patron_auth.oidc.auth.HTTP.post_with_timeout", + side_effect=RequestNetworkException( + "https://idp.example.com/revoke", "Connection refused" + ), + ), + ): + # Should not raise + manager.revoke_token("test-token") + + def test_revoke_token_discovery_error_is_non_fatal( + self, oidc_settings_with_discovery + ): + """Test that OIDCDiscoveryError during metadata fetch is swallowed.""" + manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) + + with ( + patch.object( + manager, + "get_provider_metadata", + side_effect=OIDCDiscoveryError("Discovery failed"), + ), + patch( + "palace.manager.integration.patron_auth.oidc.auth.HTTP.post_with_timeout" + ) as mock_post, + ): + manager.revoke_token("test-token") + mock_post.assert_not_called() diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index aa3306ad92..eb734be47e 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1,18 +1,22 @@ """Tests for OIDC controller.""" +import json +import logging from unittest.mock import MagicMock, Mock, patch +import jwt import pytest +from sqlalchemy.exc import SQLAlchemyError from palace.manager.api.authentication.base import PatronData from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider from palace.manager.api.problem_details import LIBRARY_NOT_FOUND, UNKNOWN_OIDC_PROVIDER +from palace.manager.integration.patron_auth.oidc.auth import OIDCAuthenticationError from palace.manager.integration.patron_auth.oidc.configuration.model import ( OIDCAuthLibrarySettings, OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.controller import ( - OIDC_INVALID_ID_TOKEN_HINT, OIDC_INVALID_REQUEST, OIDC_INVALID_RESPONSE, OIDC_INVALID_STATE, @@ -713,6 +717,7 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): mock_auth_manager.validate_id_token_hint.return_value = { "sub": "user123@example.com" } + mock_auth_manager.supports_rp_initiated_logout.return_value = True mock_auth_manager.build_logout_url.return_value = ( "https://oidc.provider.test/logout" "?id_token_hint=test.token" @@ -728,6 +733,17 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): mock_provider._credential_manager.invalidate_patron_credentials.return_value = 1 mock_library_auth.oidc_provider_lookup.return_value = mock_provider + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps( + { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "test-access-token", + "refresh_token": "refresh-token", + "id_token": "raw.id.token.jwt", + } + ), + ) # Set up library_authenticators dict controller._authenticator.library_authenticators[library.short_name] = ( @@ -747,11 +763,11 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - - result = controller.oidc_logout_initiate(params, db.session) + result = controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) assert result.status_code == 302 assert "https://oidc.provider.test/logout" in result.location @@ -760,6 +776,174 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): mock_provider._credential_manager.invalidate_patron_credentials.assert_called_once_with( db.session, patron.id ) + # Verify both tokens were revoked + mock_auth_manager.revoke_token.assert_any_call( + "test-access-token", "access_token" + ) + mock_auth_manager.revoke_token.assert_any_call("refresh-token") + # Verify stored id_token was used as id_token_hint for RP-Initiated Logout + mock_auth_manager.build_logout_url.assert_called_once_with( + "raw.id.token.jwt", + "https://cm.test/oidc_logout_callback", + mock_auth_manager.build_logout_url.call_args[0][2], + ) + + def test_oidc_logout_initiate_revocation_only( + self, + logout_controller: OIDCController, + db: DatabaseTransactionFixture, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Test logout for providers with revocation_endpoint but no end_session_endpoint. + + The flow should: invalidate CM credential, revoke token, redirect directly + to post_logout_redirect_uri without going through the IdP. + """ + caplog.set_level(logging.INFO) + controller = logout_controller + patron = db.patron() + patron.authorization_identifier = "user123@example.com" + db.session.commit() + + library = db.default_library() + + mock_library_auth = Mock() + mock_library_auth.bearer_token_signing_secret = "test-secret" + mock_provider = Mock() + mock_provider._settings = Mock() + mock_provider._settings.patron_id_claim = "sub" + mock_provider._credential_manager = Mock() + mock_provider.integration_id = 1 + + mock_auth_manager = Mock() + # Provider has revocation but NOT RP-Initiated Logout + mock_auth_manager.supports_rp_initiated_logout.return_value = False + mock_provider.get_authentication_manager.return_value = mock_auth_manager + + mock_patron = Mock() + mock_patron.id = patron.id + mock_provider._credential_manager.lookup_patron_by_identifier.return_value = ( + mock_patron + ) + mock_provider._credential_manager.invalidate_patron_credentials.return_value = 1 + + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps( + { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "test-access-token", + "refresh_token": "refresh-token", + } + ), + ) + + controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + params = { + "provider": "Test OIDC", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + } + result = controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) + + # Should redirect directly, not to the IdP + assert result.status_code == 302 + assert "https://app.example.com/logout/callback" in result.location + assert "logout_status=success" in result.location + assert "provider does not support it" in caplog.text + + # Verify RP-Initiated Logout was NOT attempted + mock_auth_manager.build_logout_url.assert_not_called() + + # Verify both tokens were revoked + mock_auth_manager.revoke_token.assert_any_call( + "test-access-token", "access_token" + ) + mock_auth_manager.revoke_token.assert_any_call("refresh-token") + + # Verify CM credentials were invalidated + mock_provider._credential_manager.invalidate_patron_credentials.assert_called_once_with( + db.session, patron.id + ) + + def test_oidc_logout_initiate_no_stored_id_token( + self, + logout_controller: OIDCController, + db: DatabaseTransactionFixture, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Test logout when provider supports RP-Initiated Logout but no id_token is stored. + + Should redirect directly with logout_status=partial and log a warning. + """ + controller = logout_controller + patron = db.patron() + patron.authorization_identifier = "user123@example.com" + db.session.commit() + + library = db.default_library() + + mock_library_auth = Mock() + mock_library_auth.bearer_token_signing_secret = "test-secret" + mock_provider = Mock() + mock_provider._settings = Mock() + mock_provider._settings.patron_id_claim = "sub" + mock_provider._credential_manager = Mock() + + mock_auth_manager = Mock() + mock_auth_manager.supports_rp_initiated_logout.return_value = True + mock_provider.get_authentication_manager.return_value = mock_auth_manager + + mock_patron = Mock() + mock_patron.id = patron.id + mock_provider._credential_manager.lookup_patron_by_identifier.return_value = ( + mock_patron + ) + mock_provider._credential_manager.invalidate_patron_credentials.return_value = 1 + + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps( + { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "test-access-token", + # No id_token field + } + ), + ) + + controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + params = { + "provider": "Test OIDC", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + } + result = controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) + + assert result.status_code == 302 + assert "https://app.example.com/logout/callback" in result.location + assert "logout_status=partial" in result.location + assert "no id_token stored in credential" in caplog.text + + mock_auth_manager.build_logout_url.assert_not_called() @pytest.mark.parametrize( "library_index,patron_email,logout_url", @@ -789,6 +973,7 @@ def test_oidc_logout_initiate_uses_correct_library( patrons = [] mock_library_auths = [] mock_auth_managers = [] + mock_providers = [] for i, library in enumerate(libraries): # Create patron @@ -808,9 +993,7 @@ def test_oidc_logout_initiate_uses_correct_library( mock_provider.integration_id = i + 1 mock_auth_manager = Mock() - mock_auth_manager.validate_id_token_hint.return_value = { - "sub": patron.authorization_identifier - } + mock_auth_manager.supports_rp_initiated_logout.return_value = True mock_auth_manager.build_logout_url.return_value = ( f"https://provider{i+1}.test/logout" ) @@ -823,8 +1006,20 @@ def test_oidc_logout_initiate_uses_correct_library( mock_provider._credential_manager.invalidate_patron_credentials.return_value = ( 1 ) + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps( + { + "id_token_claims": {"sub": patron.authorization_identifier}, + "access_token": "test-access-token", + "refresh_token": "refresh-token", + "id_token": f"raw.id.token.{i+1}", + } + ), + ) mock_library_auth.oidc_provider_lookup.return_value = mock_provider mock_library_auths.append(mock_library_auth) + mock_providers.append(mock_provider) # Register library authenticator controller._authenticator.library_authenticators[library.short_name] = ( @@ -851,11 +1046,11 @@ def test_oidc_logout_initiate_uses_correct_library( params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - - result = controller.oidc_logout_initiate(params, db.session) + result = controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) # Verify correct library's authenticator was used target_auth.oidc_provider_lookup.assert_called_once_with("Test OIDC") @@ -869,57 +1064,34 @@ def test_oidc_logout_initiate_uses_correct_library( assert result.status_code == 302 assert logout_url in result.location - # Verify correct auth manager was called - target_auth_manager.validate_id_token_hint.assert_called_once() - for i, auth_manager in enumerate(mock_auth_managers): - if i != library_index: - auth_manager.validate_id_token_hint.assert_not_called() - @pytest.mark.parametrize( - "params,error_constant_name,expected_message", + "params,expected_message", [ pytest.param( { - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", }, - "OIDC_INVALID_REQUEST", "Missing 'provider' parameter in logout request", id="missing-provider", ), pytest.param( { "provider": "Test OIDC", - "post_logout_redirect_uri": "https://app.example.com/logout/callback", - }, - "OIDC_INVALID_ID_TOKEN_HINT", - "Missing 'id_token_hint' parameter in logout request", - id="missing-id-token-hint", - ), - pytest.param( - { - "provider": "Test OIDC", - "id_token_hint": "test.id.token", }, - "OIDC_INVALID_REQUEST", "Missing 'post_logout_redirect_uri' parameter in logout request", id="missing-post-logout-redirect-uri", ), ], ) def test_oidc_logout_initiate_missing_parameter( - self, logout_controller, db, params, error_constant_name, expected_message + self, logout_controller, db, params, expected_message ): """Test OIDC logout initiate with missing required parameters.""" - error_constant = ( - OIDC_INVALID_REQUEST - if error_constant_name == "OIDC_INVALID_REQUEST" - else OIDC_INVALID_ID_TOKEN_HINT + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="" ) - result = logout_controller.oidc_logout_initiate(params, db.session) - - assert result == error_constant.detailed(expected_message) + assert result == OIDC_INVALID_REQUEST.detailed(expected_message) def test_oidc_logout_initiate_no_authenticator_for_library( self, logout_controller, db @@ -933,19 +1105,105 @@ def test_oidc_logout_initiate_no_authenticator_for_library( ): params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="" + ) assert result.uri == OIDC_INVALID_REQUEST.uri assert "No authenticator found for library" in result.detail + @pytest.mark.parametrize( + "auth_header,decode_side_effect,expected_detail", + [ + pytest.param( + None, + None, + "Missing or invalid Authorization header", + id="missing-authorization-header", + ), + pytest.param( + "NotBearer token", + None, + "Missing or invalid Authorization header", + id="non-bearer-scheme", + ), + pytest.param( + "Bearer invalid.jwt", + jwt.exceptions.InvalidTokenError("bad token"), + "Invalid bearer token", + id="invalid-jwt", + ), + pytest.param( + "Bearer valid.jwt", + None, + "Invalid credential data", + id="invalid-token-json", + ), + ], + ) + def test_oidc_logout_initiate_bearer_token_errors( + self, + logout_controller: OIDCController, + db: DatabaseTransactionFixture, + caplog: pytest.LogCaptureFixture, + auth_header: str | None, + decode_side_effect: jwt.exceptions.InvalidTokenError | None, + expected_detail: str, + ) -> None: + """Test logout initiate bearer token validation errors.""" + caplog.set_level(logging.WARNING) + library = db.default_library() + + mock_library_auth = Mock() + mock_provider = Mock() + + if decode_side_effect: + mock_library_auth.decode_bearer_token.side_effect = decode_side_effect + else: + # Return non-JSON so the token data parsing fails for the invalid-token-json case + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + "not-valid-json", + ) + + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + + logout_controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + params = { + "provider": "Test OIDC", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + } + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + result = logout_controller.oidc_logout_initiate( + params, + db.session, + auth_header="" if auth_header is None else auth_header, + ) + + assert result.uri == OIDC_INVALID_REQUEST.uri + assert result.detail is not None + assert expected_detail in result.detail + if decode_side_effect: + assert "Invalid bearer token in logout request" in caplog.text + def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): """Test logout initiate with unknown provider.""" library = db.default_library() mock_library_auth = Mock() + mock_library_auth.decode_bearer_token.return_value = ( + "Unknown", + "provider-token", + ) mock_library_auth.oidc_provider_lookup.return_value = UNKNOWN_OIDC_PROVIDER logout_controller._authenticator.library_authenticators[library.short_name] = ( @@ -958,55 +1216,86 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): ): params = { "provider": "Unknown", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) assert result == UNKNOWN_OIDC_PROVIDER + def test_oidc_logout_initiate_provider_mismatch( + self, logout_controller: OIDCController, db: DatabaseTransactionFixture + ) -> None: + """Test logout initiate when bearer token was issued by a different provider. + + A patron authenticated with provider A must not be able to initiate logout + against provider B by supplying provider=B in the request parameters. + """ + library = db.default_library() + mock_library_auth = Mock() + mock_library_auth.decode_bearer_token.return_value = ( + "Provider A", + "provider-token", + ) + logout_controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + params = { + "provider": "Provider B", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + } + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) + + assert result.uri == OIDC_INVALID_REQUEST.uri + assert result.detail is not None + assert "Provider mismatch in bearer token" in result.detail + @pytest.mark.parametrize( - "validation_error,claims_returned,expected_uri,expected_message", + "provider_token,expected_message", [ pytest.param( - Exception("Token expired"), - None, - OIDC_INVALID_ID_TOKEN_HINT.uri, - "ID token hint validation failed", - id="validation-exception", + "not-valid-json", + "Invalid credential data", + id="invalid-json-in-token", ), pytest.param( - None, - {"iss": "issuer"}, - OIDC_INVALID_ID_TOKEN_HINT.uri, - "missing patron identifier claim", + json.dumps( + {"id_token_claims": {"iss": "issuer"}, "access_token": "tok"} + ), + "Credential missing patron identifier claim", id="missing-patron-claim", ), ], ) - def test_oidc_logout_initiate_id_token_hint_errors( + def test_oidc_logout_initiate_credential_data_errors( self, logout_controller, db, - validation_error, - claims_returned, - expected_uri, + provider_token, expected_message, ): - """Test logout initiate ID token hint validation errors.""" + """Test logout initiate errors when token data in bearer token is invalid.""" library = db.default_library() mock_library_auth = Mock() + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + provider_token, + ) mock_provider = Mock() mock_provider._settings = Mock() mock_provider._settings.patron_id_claim = "sub" + mock_provider._credential_manager = Mock() mock_auth_manager = Mock() - if validation_error: - mock_auth_manager.validate_id_token_hint.side_effect = validation_error - else: - mock_auth_manager.validate_id_token_hint.return_value = claims_returned mock_provider.get_authentication_manager.return_value = mock_auth_manager mock_library_auth.oidc_provider_lookup.return_value = mock_provider @@ -1020,15 +1309,68 @@ def test_oidc_logout_initiate_id_token_hint_errors( ): params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) - result = logout_controller.oidc_logout_initiate(params, db.session) - - assert result.uri == expected_uri + assert result.uri == OIDC_INVALID_REQUEST.uri assert expected_message in result.detail + def test_oidc_logout_initiate_missing_patron_claim_revokes_token( + self, logout_controller: OIDCController, db: DatabaseTransactionFixture + ) -> None: + """Refresh token is revoked even when the patron identifier claim is absent. + + Without a patron identifier we cannot look up or invalidate the patron's + credentials, but we can still revoke the refresh token to close the IdP session. + """ + library = db.default_library() + token_data = { + "id_token_claims": {"iss": "issuer"}, # 'sub' / patron_id_claim absent + "access_token": "test-access-token", + "refresh_token": "refresh-token", + } + + mock_library_auth = Mock() + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps(token_data), + ) + mock_provider = Mock() + mock_provider._settings = Mock() + mock_provider._settings.patron_id_claim = "sub" + + mock_auth_manager = Mock() + mock_provider.get_authentication_manager.return_value = mock_auth_manager + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + + logout_controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + result = logout_controller.oidc_logout_initiate( + { + "provider": "Test OIDC", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + }, + db.session, + auth_header="Bearer valid.jwt.token", + ) + + assert result.uri == OIDC_INVALID_REQUEST.uri + assert result.detail is not None + assert "Credential missing patron identifier claim" in result.detail + mock_auth_manager.revoke_token.assert_any_call( + "test-access-token", "access_token" + ) + mock_auth_manager.revoke_token.assert_any_call("refresh-token") + @pytest.mark.parametrize( "patron_found,invalidation_error,build_url_error,expected_status,expected_uri", [ @@ -1042,7 +1384,7 @@ def test_oidc_logout_initiate_id_token_hint_errors( ), pytest.param( True, - Exception("Database error"), + SQLAlchemyError("Database error"), None, 302, None, @@ -1051,7 +1393,7 @@ def test_oidc_logout_initiate_id_token_hint_errors( pytest.param( True, None, - Exception("Logout not supported"), + OIDCAuthenticationError("Logout not supported"), None, "http://palaceproject.io/terms/problem/auth/unrecoverable/oidc/logout-not-supported", id="build-logout-url-exception", @@ -1060,20 +1402,32 @@ def test_oidc_logout_initiate_id_token_hint_errors( ) def test_oidc_logout_initiate_exceptions( self, - logout_controller, - db, - patron_found, - invalidation_error, - build_url_error, - expected_status, - expected_uri, - ): + logout_controller: OIDCController, + db: DatabaseTransactionFixture, + patron_found: bool, + invalidation_error: SQLAlchemyError | None, + build_url_error: OIDCAuthenticationError | None, + expected_status: int | None, + expected_uri: str | None, + ) -> None: """Test logout initiate with patron lookup and exception scenarios.""" library = db.default_library() patron = db.patron() + token_data: dict = { + "id_token_claims": {"sub": "user@test.com"}, + "access_token": "test-access-token", + } + if build_url_error: + # RP-Initiated Logout path — needs id_token and the method to return True + token_data["id_token"] = "test.id.token" + mock_library_auth = Mock() mock_library_auth.bearer_token_signing_secret = "test-secret" + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps(token_data), + ) mock_provider = Mock() mock_provider._settings = Mock() mock_provider._settings.patron_id_claim = "sub" @@ -1097,7 +1451,9 @@ def test_oidc_logout_initiate_exceptions( ) mock_auth_manager = Mock() - mock_auth_manager.validate_id_token_hint.return_value = {"sub": "user@test.com"} + mock_auth_manager.supports_rp_initiated_logout.return_value = bool( + build_url_error + ) if build_url_error: mock_auth_manager.build_logout_url.side_effect = build_url_error else: @@ -1119,16 +1475,19 @@ def test_oidc_logout_initiate_exceptions( patch( "palace.manager.integration.patron_auth.oidc.controller.url_for" ) as mock_url_for, + patch( + "palace.manager.integration.patron_auth.oidc.controller.OIDCUtility" + ) as MockOIDCUtility, ): mock_url_for.return_value = "https://cm.test/oidc_logout_callback" params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) if expected_status: assert result.status_code == expected_status @@ -1136,6 +1495,74 @@ def test_oidc_logout_initiate_exceptions( mock_provider._credential_manager.invalidate_patron_credentials.assert_not_called() else: assert result.uri == expected_uri + if build_url_error: + mock_utility = MockOIDCUtility.return_value + stored_state = mock_utility.store_logout_state.call_args[0][0] + mock_utility.delete_logout_state.assert_called_once_with( + stored_state + ) + + def test_oidc_logout_initiate_credential_invalidation_is_nonfatal( + self, logout_controller: OIDCController, db: DatabaseTransactionFixture + ) -> None: + """Credential invalidation failure must not abort the logout flow. + + Token revocation and the final redirect must still happen even when + invalidate_patron_credentials raises SQLAlchemyError. + """ + library = db.default_library() + patron = db.patron() + + mock_library_auth = Mock() + mock_library_auth.bearer_token_signing_secret = "test-secret" + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + json.dumps( + { + "id_token_claims": {"sub": "user@test.com"}, + "access_token": "test-access-token", + "refresh_token": "refresh-token", + } + ), + ) + mock_provider = Mock() + mock_provider._settings = Mock() + mock_provider._settings.patron_id_claim = "sub" + mock_provider._credential_manager = Mock() + mock_provider._credential_manager.lookup_patron_by_identifier.return_value = ( + Mock(id=patron.id) + ) + mock_provider._credential_manager.invalidate_patron_credentials.side_effect = ( + SQLAlchemyError("DB error") + ) + + mock_auth_manager = Mock() + mock_auth_manager.supports_rp_initiated_logout.return_value = False + mock_provider.get_authentication_manager.return_value = mock_auth_manager + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + logout_controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + with patch( + "palace.manager.integration.patron_auth.oidc.controller.get_request_library", + return_value=library, + ): + result = logout_controller.oidc_logout_initiate( + { + "provider": "Test OIDC", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + }, + db.session, + auth_header="Bearer valid.jwt.token", + ) + + assert result.status_code == 302 + assert "logout_status=success" in result.location + mock_auth_manager.revoke_token.assert_any_call( + "test-access-token", "access_token" + ) + mock_auth_manager.revoke_token.assert_any_call("refresh-token") def test_oidc_logout_callback_success(self, logout_controller, db): library = db.default_library() @@ -1162,7 +1589,6 @@ def test_oidc_logout_callback_success(self, logout_controller, db): utility.store_logout_state( state_token, "https://app.example.com/logout/callback", - metadata={"library_short_name": library.short_name}, ) params = {"state": state_token} @@ -1250,7 +1676,6 @@ def test_oidc_logout_callback_no_authenticator_for_library( utility.store_logout_state( state_token, "https://app.example.com/logout/callback", - metadata={"library_short_name": library.short_name}, ) params = {"state": state_token} @@ -1260,16 +1685,47 @@ def test_oidc_logout_callback_no_authenticator_for_library( assert result.uri == OIDC_INVALID_REQUEST.uri assert "No authenticator found for library" in result.detail + def test_oidc_logout_callback_malformed_state_payload( + self, logout_controller: OIDCController, db: DatabaseTransactionFixture + ) -> None: + """Test logout callback when the state token cannot be decoded at all. + + A token that has no '.' separator fails decode_state_payload before we + can even attempt signature validation. + """ + malformed_state = "not-a-valid-state-token" + + mock_redis = logout_controller._circulation_manager.services.redis.client() + utility = OIDCUtility(mock_redis) + utility.store_logout_state( + malformed_state, "https://app.example.com/logout/callback" + ) + + result = logout_controller.oidc_logout_callback( + {"state": malformed_state}, db.session + ) + + assert result.uri == OIDC_INVALID_STATE.uri + assert result.detail is not None + assert "Invalid state parameter format" in result.detail + def test_oidc_logout_callback_state_validation_exception( self, logout_controller, db ): - """Test logout callback when state validation raises exception.""" + """Test logout callback when state signature validation fails.""" library = db.default_library() - state_token = "invalid-state-token" + # Generate a structurally valid state token signed with the wrong secret + # so that decode_state_payload succeeds but validate_state fails. + logout_state_data = { + "provider_name": "Test OIDC", + "library_short_name": library.short_name, + "redirect_uri": "https://app.example.com/logout/callback", + } + state_token = OIDCUtility.generate_state(logout_state_data, "wrong-secret") mock_library_auth = Mock() - mock_library_auth.bearer_token_signing_secret = "test-secret" + mock_library_auth.bearer_token_signing_secret = "correct-secret" logout_controller._authenticator.library_authenticators[library.short_name] = ( mock_library_auth @@ -1280,7 +1736,6 @@ def test_oidc_logout_callback_state_validation_exception( utility.store_logout_state( state_token, "https://app.example.com/logout/callback", - metadata={"library_short_name": library.short_name}, ) params = {"state": state_token} diff --git a/tests/manager/integration/patron_auth/oidc/test_credential.py b/tests/manager/integration/patron_auth/oidc/test_credential.py index 351f5ecbde..1eb946b4de 100644 --- a/tests/manager/integration/patron_auth/oidc/test_credential.py +++ b/tests/manager/integration/patron_auth/oidc/test_credential.py @@ -78,6 +78,50 @@ def test_create_token_value( else: assert "refresh_token" not in token_data + @pytest.mark.parametrize( + "value,expected_error", + [ + pytest.param("not-json", "Invalid OIDC token format", id="invalid-json"), + pytest.param( + json.dumps({"access_token": "tok"}), + "missing or invalid id_token_claims", + id="missing-id-token-claims", + ), + pytest.param( + json.dumps({"id_token_claims": "not-a-dict", "access_token": "tok"}), + "missing or invalid id_token_claims", + id="non-dict-id-token-claims", + ), + pytest.param( + json.dumps({"id_token_claims": {"sub": "u"}}), + "missing access_token", + id="missing-access-token", + ), + ], + ) + def test_parse_token_value_invalid(self, value: str, expected_error: str) -> None: + """Test parse_token_value raises ValueError for malformed input.""" + with pytest.raises(ValueError, match=expected_error): + OIDCCredentialManager.parse_token_value(value) + + def test_parse_token_value_success( + self, sample_id_token_claims: dict[str, str] + ) -> None: + """Test parse_token_value returns parsed dict for valid input.""" + value = json.dumps( + { + "id_token_claims": sample_id_token_claims, + "access_token": "tok", + "refresh_token": "ref", + "id_token": "raw.jwt", + } + ) + result = OIDCCredentialManager.parse_token_value(value) + assert result["id_token_claims"] == sample_id_token_claims + assert result["access_token"] == "tok" + assert result["refresh_token"] == "ref" + assert result["id_token"] == "raw.jwt" + def test_extract_token_data_success( self, manager, @@ -145,7 +189,7 @@ def test_extract_token_data_missing_id_token_claims( with pytest.raises(ValueError) as exc_info: manager.extract_token_data(credential) - assert "missing id_token_claims" in str(exc_info.value) + assert "missing or invalid id_token_claims" in str(exc_info.value) def test_extract_token_data_missing_access_token( self, diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 21950c24e9..b41ca47814 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -1,5 +1,6 @@ """Tests for OIDC authentication provider.""" +import logging import re from unittest.mock import MagicMock, patch @@ -7,6 +8,7 @@ from freezegun import freeze_time from palace.manager.api.authentication.base import PatronData, PatronLookupNotSupported +from palace.manager.core.exceptions import PalaceValueError from palace.manager.integration.patron_auth.oidc.configuration.model import ( OIDCAuthLibrarySettings, OIDCAuthSettings, @@ -14,8 +16,15 @@ from palace.manager.integration.patron_auth.oidc.provider import ( OIDC_CANNOT_DETERMINE_PATRON, OIDC_TOKEN_EXPIRED, + OPDS_URI_TEMPLATE_VARIABLES_PROPERTY, + OPDS_URI_TEMPLATE_VARIABLES_TYPE, + PALACE_REDIRECT_URI_TERM, OIDCAuthenticationProvider, ) +from palace.manager.integration.patron_auth.oidc.util import ( + LOGOUT_REDIRECT_QUERY_PARAM, + OIDCDiscoveryError, +) from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.util.problem_detail import ProblemDetailException from tests.fixtures.database import DatabaseTransactionFixture @@ -73,10 +82,19 @@ def test_authentication_flow_document( self, db: DatabaseTransactionFixture, oidc_provider ): library = db.default_library() - - with patch( - "palace.manager.integration.patron_auth.oidc.provider.url_for" - ) as mock_url_for: + mock_auth_manager = MagicMock() + mock_auth_manager.supports_logout.return_value = False + + with ( + patch( + "palace.manager.integration.patron_auth.oidc.provider.url_for" + ) as mock_url_for, + patch.object( + oidc_provider, + "get_authentication_manager", + return_value=mock_auth_manager, + ), + ): mock_url_for.return_value = ( f"https://example.com/{library.short_name}/oidc_authenticate" ) @@ -132,10 +150,19 @@ def test_authentication_flow_document_with_authorization_link_settings( settings=settings, library_settings=library_settings, ) - - with patch( - "palace.manager.integration.patron_auth.oidc.provider.url_for" - ) as mock_url_for: + mock_auth_manager = MagicMock() + mock_auth_manager.supports_logout.return_value = False + + with ( + patch( + "palace.manager.integration.patron_auth.oidc.provider.url_for" + ) as mock_url_for, + patch.object( + provider, + "get_authentication_manager", + return_value=mock_auth_manager, + ), + ): mock_url_for.return_value = ( f"https://example.com/{library.short_name}/oidc/authenticate" ) @@ -169,9 +196,155 @@ def test_authentication_flow_document_no_library( ): oidc_provider.library_id = 999999 - with pytest.raises(ValueError, match="Library not found"): + with pytest.raises(PalaceValueError, match="Library not found"): oidc_provider._authentication_flow_document(db.session) + def test_authentication_flow_document_with_logout_link( + self, db: DatabaseTransactionFixture, oidc_provider + ): + """Test that logout link is included when provider supports logout.""" + library = db.default_library() + mock_auth_manager = MagicMock() + mock_auth_manager.supports_logout.return_value = True + + with ( + patch( + "palace.manager.integration.patron_auth.oidc.provider.url_for" + ) as mock_url_for, + patch.object( + oidc_provider, + "get_authentication_manager", + return_value=mock_auth_manager, + ), + ): + mock_url_for.side_effect = [ + f"https://example.com/{library.short_name}/oidc_authenticate", + f"https://example.com/{library.short_name}/oidc/logout", + ] + + result = oidc_provider._authentication_flow_document(db.session) + + assert len(result["links"]) == 2 + auth_link = result["links"][0] + assert auth_link["rel"] == "authenticate" + logout_link = result["links"][1] + assert logout_link["rel"] == "logout" + assert library.short_name in logout_link["href"] + assert f"{{&{LOGOUT_REDIRECT_QUERY_PARAM}}}" in logout_link["href"] + assert logout_link["templated"] is True + assert logout_link["properties"] == { + OPDS_URI_TEMPLATE_VARIABLES_PROPERTY: { + "type": OPDS_URI_TEMPLATE_VARIABLES_TYPE, + "map": {LOGOUT_REDIRECT_QUERY_PARAM: PALACE_REDIRECT_URI_TERM}, + } + } + + def test_authentication_flow_document_without_logout_link( + self, db: DatabaseTransactionFixture, oidc_provider + ): + """Test that logout link is omitted when provider does not support logout.""" + library = db.default_library() + mock_auth_manager = MagicMock() + mock_auth_manager.supports_logout.return_value = False + + with ( + patch( + "palace.manager.integration.patron_auth.oidc.provider.url_for" + ) as mock_url_for, + patch.object( + oidc_provider, + "get_authentication_manager", + return_value=mock_auth_manager, + ), + ): + mock_url_for.return_value = ( + f"https://example.com/{library.short_name}/oidc_authenticate" + ) + + result = oidc_provider._authentication_flow_document(db.session) + + assert len(result["links"]) == 1 + assert result["links"][0]["rel"] == "authenticate" + + def test_get_authentication_manager_cached_after_successful_configuration( + self, oidc_provider: OIDCAuthenticationProvider + ) -> None: + """Manager is cached once metadata loads successfully.""" + oidc_provider._auth_manager = None + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockManager: + mock_manager = MagicMock() + mock_manager.get_provider_metadata.return_value = {} + MockManager.return_value = mock_manager + + first = oidc_provider.get_authentication_manager() + second = oidc_provider.get_authentication_manager() + + assert first is second + MockManager.assert_called_once() + + def test_get_authentication_manager_not_cached_on_discovery_failure( + self, + oidc_provider: OIDCAuthenticationProvider, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Manager is not cached when metadata discovery fails; next call retries.""" + oidc_provider._auth_manager = None + caplog.set_level(logging.WARNING) + + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockManager: + first_manager = MagicMock() + first_manager.get_provider_metadata.side_effect = OIDCDiscoveryError( + "IdP unreachable" + ) + second_manager = MagicMock() + second_manager.get_provider_metadata.side_effect = OIDCDiscoveryError( + "IdP unreachable" + ) + MockManager.side_effect = [first_manager, second_manager] + + first = oidc_provider.get_authentication_manager() + second = oidc_provider.get_authentication_manager() + + assert first is first_manager + assert second is second_manager + assert first is not second + assert MockManager.call_count == 2 + assert "Failed to configure OIDC authentication manager" in caplog.text + assert "Will retry on next request" in caplog.text + + def test_get_authentication_manager_cached_after_recovery( + self, + oidc_provider: OIDCAuthenticationProvider, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Manager is cached once IdP becomes reachable after previous failures.""" + oidc_provider._auth_manager = None + caplog.set_level(logging.WARNING) + + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockManager: + failing_manager = MagicMock() + failing_manager.get_provider_metadata.side_effect = OIDCDiscoveryError( + "IdP unreachable" + ) + succeeding_manager = MagicMock() + succeeding_manager.get_provider_metadata.return_value = {} + MockManager.side_effect = [failing_manager, succeeding_manager] + + first = oidc_provider.get_authentication_manager() + second = oidc_provider.get_authentication_manager() + third = oidc_provider.get_authentication_manager() + + assert first is failing_manager + assert second is succeeding_manager + assert third is succeeding_manager # cached after recovery + assert MockManager.call_count == 2 + def test_run_self_tests(self, db: DatabaseTransactionFixture, oidc_provider): results = list(oidc_provider._run_self_tests(db.session)) assert results == [] @@ -364,8 +537,43 @@ def test_oidc_callback(self, db: DatabaseTransactionFixture, oidc_provider): assert credential.credential is not None assert credential.patron == patron - def test_get_authentication_manager(self, oidc_provider): - manager = oidc_provider.get_authentication_manager() + def test_get_authentication_manager( + self, oidc_provider: OIDCAuthenticationProvider + ) -> None: + oidc_provider._auth_manager = None + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockManager: + mock_manager = MagicMock() + mock_manager.get_provider_metadata.return_value = {} + MockManager.return_value = mock_manager + + manager = oidc_provider.get_authentication_manager() + + assert manager is mock_manager + assert MockManager.call_args[0][0] == oidc_provider._settings + + # Same instance returned on repeated calls — avoids re-fetching OIDC discovery doc. + assert oidc_provider.get_authentication_manager() is manager + MockManager.assert_called_once() + + # A new provider instance (simulating a config reload) produces a new manager. + new_settings = OIDCAuthSettings( + issuer_url="https://new-idp.example.com", + client_id="new-client-id", + client_secret="new-client-secret", + ) + new_provider = OIDCAuthenticationProvider( + library_id=oidc_provider.library_id, + integration_id=oidc_provider.integration_id, + settings=new_settings, + library_settings=OIDCAuthLibrarySettings(), + ) + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockNewManager: + new_mock_manager = MagicMock() + new_mock_manager.get_provider_metadata.return_value = {} + MockNewManager.return_value = new_mock_manager - assert manager is not None - assert manager._settings == oidc_provider._settings + assert new_provider.get_authentication_manager() is not manager diff --git a/tests/manager/integration/patron_auth/oidc/test_util.py b/tests/manager/integration/patron_auth/oidc/test_util.py index 659269c750..cda8cb7086 100644 --- a/tests/manager/integration/patron_auth/oidc/test_util.py +++ b/tests/manager/integration/patron_auth/oidc/test_util.py @@ -191,6 +191,30 @@ def test_validate_state_custom_max_age(self): ) assert decoded["library_short_name"] == "TESTLIB" + def test_decode_state_payload_success(self): + data = {"library_short_name": "TESTLIB", "provider": "Test OIDC"} + state = OIDCUtility.generate_state(data, TEST_SECRET_KEY) + + payload = OIDCUtility.decode_state_payload(state) + + assert payload["library_short_name"] == "TESTLIB" + assert payload["provider"] == "Test OIDC" + assert "timestamp" in payload + + @pytest.mark.parametrize( + "invalid_state", + [ + pytest.param("nodothere", id="no-separator"), + pytest.param("sig.!!!invalid-base64!!!", id="bad-base64"), + pytest.param( + "sig." + urlsafe_b64encode(b"not-json").decode(), id="bad-json" + ), + ], + ) + def test_decode_state_payload_invalid(self, invalid_state): + with pytest.raises(OIDCStateValidationError): + OIDCUtility.decode_state_payload(invalid_state) + class TestOIDCUtilityDiscovery: """Tests for OIDC discovery document fetching."""