From 6f950fdec3a18d15efb832adb96e867cb00547c3 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Fri, 20 Feb 2026 09:07:38 -0500 Subject: [PATCH 01/26] OpenID Connect logout support (PP-3726) --- src/palace/manager/api/routes.py | 2 +- .../integration/patron_auth/oidc/auth.py | 67 ++++- .../patron_auth/oidc/configuration/model.py | 16 +- .../patron_auth/oidc/controller.py | 42 +++- .../integration/patron_auth/oidc/provider.py | 19 +- tests/manager/api/test_routes.py | 9 +- .../integration/patron_auth/oidc/test_auth.py | 232 ++++++++++++++++++ .../patron_auth/oidc/test_controller.py | 139 ++++++++++- .../patron_auth/oidc/test_provider.py | 96 +++++++- 9 files changed, 598 insertions(+), 24 deletions(-) diff --git a/src/palace/manager/api/routes.py b/src/palace/manager/api/routes.py index 53010f75a7..f9c3c74081 100644 --- a/src/palace/manager/api/routes.py +++ b/src/palace/manager/api/routes.py @@ -581,7 +581,7 @@ def oidc_callback(): @returns_problem_detail def oidc_logout(): return app.manager.oidc_controller.oidc_logout_initiate( - flask.request.args, app.manager._db + flask.request.args, dict(flask.request.headers), 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..e5c5986aa9 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,62 @@ def build_logout_url( self.log.info(f"Built logout URL for provider: {end_session_endpoint}") return logout_url + def supports_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 logout is supported + """ + try: + metadata = self.get_provider_metadata() + if metadata.get("end_session_endpoint"): + return True + except OIDCDiscoveryError: + pass + return bool(self._settings.end_session_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 Exception: + 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..96ec06c75a 100644 --- a/src/palace/manager/integration/patron_auth/oidc/configuration/model.py +++ b/src/palace/manager/integration/patron_auth/oidc/configuration/model.py @@ -162,7 +162,20 @@ class OIDCAuthSettings(AuthProviderSettings, LoggerMixin): "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..7b44a9c848 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -6,6 +6,7 @@ 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.orm import Session @@ -345,11 +346,15 @@ 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], + request_headers: dict[str, str], + db: Session, ) -> BaseResponse | ProblemDetail: """Initiate OIDC RP-Initiated Logout flow. :param request_args: Request arguments from Flask + :param request_headers: Request headers from Flask :param db: Database session :return: Redirect to provider logout endpoint or error """ @@ -381,10 +386,33 @@ def oidc_logout_initiate( _("No authenticator found for library") ) + # Validate CM bearer token — ensures patron has an active CM session. + auth_header = request_headers.get("Authorization", "") + if not auth_header.startswith("Bearer "): + return OIDC_INVALID_REQUEST.detailed( + _("Missing or invalid Authorization header") + ) + + cm_jwt = auth_header.removeprefix("Bearer ") + try: + _provider_name, provider_token = library_authenticator.decode_bearer_token( + cm_jwt + ) + except jwt.exceptions.InvalidTokenError: + return OIDC_INVALID_REQUEST.detailed(_("Invalid bearer token")) + provider = library_authenticator.oidc_provider_lookup(provider_name) if isinstance(provider, ProblemDetail): return provider + oidc_credential = provider._credential_manager.lookup_oidc_token_by_value( # type: ignore[attr-defined] + db, provider_token, library.id + ) + if not oidc_credential: + return OIDC_INVALID_REQUEST.detailed( + _("Bearer token is expired or not found") + ) + try: auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] claims = auth_manager.validate_id_token_hint(id_token_hint) @@ -415,6 +443,18 @@ def oidc_logout_initiate( except Exception as e: self.log.exception("Failed to invalidate credentials") + # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication + # at the IdP after local CM logout. Errors are non-fatal. + try: + token_data = provider._credential_manager.extract_token_data(oidc_credential) # type: ignore[attr-defined] + refresh_token = token_data.get("refresh_token") + if refresh_token: + auth_manager.revoke_token(refresh_token) + except Exception: + self.log.warning( + "Failed to revoke refresh token (non-critical)", exc_info=True + ) + callback_url = url_for("oidc_logout_callback", _external=True) logout_state_data = { diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index f82f190722..75cc0c774d 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -15,6 +15,7 @@ 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, @@ -166,7 +167,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 +175,24 @@ 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": logout_url}) return { "type": self.flow_type, "description": self.label(), - "links": [link], + "links": links, } def _run_self_tests(self, db: Session) -> Generator[SelfTestResult]: diff --git a/tests/manager/api/test_routes.py b/tests/manager/api/test_routes.py index e45cc1a29d..8fcde1ea49 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[0][1], dict) 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/test_auth.py b/tests/manager/integration/patron_auth/oidc/test_auth.py index 5d57a414ae..0540a25323 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,185 @@ def mock_get_side_effect(url, **kwargs): with pytest.raises(OIDCAuthenticationError, match=error_match): manager.validate_logout_token(token) + + +class TestOIDCAuthManagerSupportsLogout: + """Tests for OIDCAuthenticationManager.supports_logout().""" + + @pytest.mark.parametrize( + "metadata,manual_end_session_endpoint,expected", + [ + pytest.param( + {"end_session_endpoint": "https://idp.example.com/logout"}, + None, + True, + id="discovered-end-session-endpoint", + ), + pytest.param( + {}, + HttpUrl("https://idp.example.com/logout"), + True, + id="no-discovered-endpoint-manual-setting-present", + ), + pytest.param( + {}, + None, + False, + id="neither-discovered-nor-manual", + ), + ], + ) + def test_supports_logout( + self, + oidc_settings_with_discovery, + metadata, + manual_end_session_endpoint, + expected, + ): + """Test supports_logout with various discovery and manual config combinations.""" + if manual_end_session_endpoint: + oidc_settings_with_discovery = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer_url=TEST_ISSUER_URL, + end_session_endpoint=manual_end_session_endpoint, + ) + + 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( + "manual_end_session_endpoint,expected", + [ + pytest.param( + HttpUrl("https://idp.example.com/logout"), + True, + id="discovery-error-manual-setting-present", + ), + pytest.param( + None, + False, + id="discovery-error-no-manual-setting", + ), + ], + ) + def test_supports_logout_discovery_error( + self, + oidc_settings_with_discovery, + manual_end_session_endpoint, + expected, + ): + """Test supports_logout falls back to manual setting on OIDCDiscoveryError.""" + if manual_end_session_endpoint: + oidc_settings_with_discovery = OIDCAuthSettings( + client_id=TEST_CLIENT_ID, + client_secret=TEST_CLIENT_SECRET, + issuer_url=TEST_ISSUER_URL, + end_session_endpoint=manual_end_session_endpoint, + ) + + 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..7241c45f00 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -2,6 +2,7 @@ from unittest.mock import MagicMock, Mock, patch +import jwt import pytest from palace.manager.api.authentication.base import PatronData @@ -728,6 +729,19 @@ 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", + "provider-token", + ) + mock_oidc_credential = Mock() + mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( + mock_oidc_credential + ) + mock_provider._credential_manager.extract_token_data.return_value = { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "access-token", + "refresh_token": "refresh-token", + } # Set up library_authenticators dict controller._authenticator.library_authenticators[library.short_name] = ( @@ -750,8 +764,9 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + headers = {"Authorization": "Bearer valid.jwt.token"} - result = controller.oidc_logout_initiate(params, db.session) + result = controller.oidc_logout_initiate(params, headers, db.session) assert result.status_code == 302 assert "https://oidc.provider.test/logout" in result.location @@ -760,6 +775,8 @@ 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 refresh token revocation was attempted + mock_auth_manager.revoke_token.assert_called_once_with("refresh-token") @pytest.mark.parametrize( "library_index,patron_email,logout_url", @@ -823,6 +840,13 @@ def test_oidc_logout_initiate_uses_correct_library( mock_provider._credential_manager.invalidate_patron_credentials.return_value = ( 1 ) + mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( + Mock() + ) + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + "provider-token", + ) mock_library_auth.oidc_provider_lookup.return_value = mock_provider mock_library_auths.append(mock_library_auth) @@ -854,8 +878,9 @@ def test_oidc_logout_initiate_uses_correct_library( "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + headers = {"Authorization": "Bearer valid.jwt.token"} - result = controller.oidc_logout_initiate(params, db.session) + result = controller.oidc_logout_initiate(params, headers, db.session) # Verify correct library's authenticator was used target_auth.oidc_provider_lookup.assert_called_once_with("Test OIDC") @@ -917,7 +942,7 @@ def test_oidc_logout_initiate_missing_parameter( else OIDC_INVALID_ID_TOKEN_HINT ) - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate(params, {}, db.session) assert result == error_constant.detailed(expected_message) @@ -937,15 +962,97 @@ def test_oidc_logout_initiate_no_authenticator_for_library( "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) 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, + "Bearer token is expired or not found", + id="credential-not-found", + ), + ], + ) + def test_oidc_logout_initiate_bearer_token_errors( + self, + logout_controller, + db, + auth_header, + decode_side_effect, + expected_detail, + ): + """Test logout initiate bearer token validation errors.""" + library = db.default_library() + + mock_library_auth = Mock() + mock_provider = Mock() + mock_provider._credential_manager = Mock() + + if decode_side_effect: + mock_library_auth.decode_bearer_token.side_effect = decode_side_effect + else: + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + "provider-token", + ) + + # Credential not found for the last test case + mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = None + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + + logout_controller._authenticator.library_authenticators[library.short_name] = ( + mock_library_auth + ) + + request_headers = {} if auth_header is None else {"Authorization": auth_header} + params = { + "provider": "Test OIDC", + "id_token_hint": "test.id.token", + "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, request_headers, db.session + ) + + assert result.uri == OIDC_INVALID_REQUEST.uri + assert expected_detail in result.detail + 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] = ( @@ -961,8 +1068,9 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + headers = {"Authorization": "Bearer valid.jwt.token"} - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate(params, headers, db.session) assert result == UNKNOWN_OIDC_PROVIDER @@ -998,9 +1106,17 @@ def test_oidc_logout_initiate_id_token_hint_errors( 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_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( + Mock() + ) mock_auth_manager = Mock() if validation_error: @@ -1023,8 +1139,9 @@ def test_oidc_logout_initiate_id_token_hint_errors( "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + headers = {"Authorization": "Bearer valid.jwt.token"} - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate(params, headers, db.session) assert result.uri == expected_uri assert expected_message in result.detail @@ -1074,10 +1191,17 @@ def test_oidc_logout_initiate_exceptions( mock_library_auth = Mock() mock_library_auth.bearer_token_signing_secret = "test-secret" + 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_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( + Mock() + ) if patron_found: mock_provider._credential_manager.lookup_patron_by_identifier.return_value = Mock( @@ -1127,8 +1251,9 @@ def test_oidc_logout_initiate_exceptions( "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } + headers = {"Authorization": "Bearer valid.jwt.token"} - result = logout_controller.oidc_logout_initiate(params, db.session) + result = logout_controller.oidc_logout_initiate(params, headers, db.session) if expected_status: assert result.status_code == expected_status diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 21950c24e9..57d99afd3a 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -7,6 +7,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, @@ -73,10 +74,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 +142,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 +188,68 @@ 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"] + + 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_run_self_tests(self, db: DatabaseTransactionFixture, oidc_provider): results = list(oidc_provider._run_self_tests(db.session)) assert results == [] From 182f95f0f0816c2ade4720aa04b3310cb74ac56e Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 24 Feb 2026 10:22:46 -0500 Subject: [PATCH 02/26] WIP - experimenting, but problems with endpoint logout --- .../integration/patron_auth/oidc/auth.py | 25 ++- .../patron_auth/oidc/controller.py | 56 ++--- .../patron_auth/oidc/credential.py | 21 +- .../integration/patron_auth/oidc/provider.py | 3 + .../integration/patron_auth/oidc/test_auth.py | 120 +++++++++-- .../patron_auth/oidc/test_controller.py | 194 +++++++++++++----- 6 files changed, 316 insertions(+), 103 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index e5c5986aa9..1228467ea8 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -486,13 +486,13 @@ def build_logout_url( self.log.info(f"Built logout URL for provider: {end_session_endpoint}") return logout_url - def supports_logout(self) -> bool: + 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 logout is supported + :return: True if RP-Initiated Logout is supported """ try: metadata = self.get_provider_metadata() @@ -502,6 +502,27 @@ def supports_logout(self) -> bool: pass return 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 + """ + try: + metadata = self.get_provider_metadata() + if metadata.get("end_session_endpoint") or metadata.get( + "revocation_endpoint" + ): + return True + except OIDCDiscoveryError: + pass + return 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). diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 7b44a9c848..997262891d 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -327,6 +327,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) @@ -359,7 +360,6 @@ def oidc_logout_initiate( :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) if not provider_name: @@ -367,11 +367,6 @@ def oidc_logout_initiate( _("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") @@ -413,21 +408,28 @@ def oidc_logout_initiate( _("Bearer token is expired or not found") ) + auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] + + # Extract the stored token data from the credential rather than requiring + # the client to re-supply the raw id_token_hint as a separate parameter. try: - auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] - claims = auth_manager.validate_id_token_hint(id_token_hint) + token_data = provider._credential_manager.extract_token_data(oidc_credential) # type: ignore[attr-defined] 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)}") - ) + self.log.exception("Failed to extract token data from credential") + return OIDC_INVALID_REQUEST.detailed(_("Invalid credential data")) - patron_identifier = claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] + id_token_claims = token_data.get("id_token_claims", {}) + patron_identifier = id_token_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") ) + # The raw id_token JWT stored at authentication time; used as id_token_hint + # for RP-Initiated Logout. May be absent for credentials created before this + # feature was introduced — in that case RP-Initiated Logout is skipped. + stored_id_token = token_data.get("id_token") + try: credential_manager = provider._credential_manager # type: ignore[attr-defined] patron = credential_manager.lookup_patron_by_identifier( @@ -445,15 +447,23 @@ def oidc_logout_initiate( # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication # at the IdP after local CM logout. Errors are non-fatal. - try: - token_data = provider._credential_manager.extract_token_data(oidc_credential) # type: ignore[attr-defined] - refresh_token = token_data.get("refresh_token") - if refresh_token: + refresh_token = token_data.get("refresh_token") + if refresh_token: + try: auth_manager.revoke_token(refresh_token) - except Exception: - self.log.warning( - "Failed to revoke refresh token (non-critical)", exc_info=True + except Exception: + self.log.warning( + "Failed to revoke refresh token (non-critical)", exc_info=True + ) + + # If the provider does not support RP-Initiated Logout (only token revocation), + # or if no id_token was stored (credentials pre-dating this feature), redirect + # directly — local invalidation and token revocation are already done. + if not auth_manager.supports_rp_initiated_logout() or not stored_id_token: + final_redirect_uri = self._add_params_to_url( + post_logout_redirect_uri, {self.LOGOUT_STATUS: "success"} ) + return redirect(final_redirect_uri) callback_url = url_for("oidc_logout_callback", _external=True) @@ -471,7 +481,7 @@ 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: self.log.exception("Failed to build logout URL") diff --git a/src/palace/manager/integration/patron_auth/oidc/credential.py b/src/palace/manager/integration/patron_auth/oidc/credential.py index 5493603fcf..7c70ddccb1 100644 --- a/src/palace/manager/integration/patron_auth/oidc/credential.py +++ b/src/palace/manager/integration/patron_auth/oidc/credential.py @@ -57,21 +57,25 @@ 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) @@ -108,6 +112,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 +123,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 +144,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 +285,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 75cc0c774d..c1f12808b1 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -291,6 +291,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. @@ -299,6 +300,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) @@ -315,6 +317,7 @@ def oidc_callback( refresh_token, expires_in, self._settings.session_lifetime, + id_token, ) return credential, patron, patron_data diff --git a/tests/manager/integration/patron_auth/oidc/test_auth.py b/tests/manager/integration/patron_auth/oidc/test_auth.py index 0540a25323..3e626b1b27 100644 --- a/tests/manager/integration/patron_auth/oidc/test_auth.py +++ b/tests/manager/integration/patron_auth/oidc/test_auth.py @@ -1559,27 +1559,110 @@ def mock_get_side_effect(url, **kwargs): 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,manual_end_session_endpoint,expected", + "metadata,settings_kwargs,expected", [ pytest.param( {"end_session_endpoint": "https://idp.example.com/logout"}, - None, + {}, True, id="discovered-end-session-endpoint", ), pytest.param( + {"revocation_endpoint": "https://idp.example.com/revoke"}, {}, - HttpUrl("https://idp.example.com/logout"), True, - id="no-discovered-endpoint-manual-setting-present", + 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( + {}, {}, - None, False, id="neither-discovered-nor-manual", ), @@ -1589,16 +1672,16 @@ def test_supports_logout( self, oidc_settings_with_discovery, metadata, - manual_end_session_endpoint, + settings_kwargs, expected, ): """Test supports_logout with various discovery and manual config combinations.""" - if manual_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, - end_session_endpoint=manual_end_session_endpoint, + **settings_kwargs, ) manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) @@ -1609,15 +1692,20 @@ def test_supports_logout( assert result is expected @pytest.mark.parametrize( - "manual_end_session_endpoint,expected", + "settings_kwargs,expected", [ pytest.param( - HttpUrl("https://idp.example.com/logout"), + {"end_session_endpoint": HttpUrl("https://idp.example.com/logout")}, True, - id="discovery-error-manual-setting-present", + id="discovery-error-manual-end-session-present", ), pytest.param( - None, + {"revocation_endpoint": HttpUrl("https://idp.example.com/revoke")}, + True, + id="discovery-error-manual-revocation-present", + ), + pytest.param( + {}, False, id="discovery-error-no-manual-setting", ), @@ -1626,16 +1714,16 @@ def test_supports_logout( def test_supports_logout_discovery_error( self, oidc_settings_with_discovery, - manual_end_session_endpoint, + settings_kwargs, expected, ): - """Test supports_logout falls back to manual setting on OIDCDiscoveryError.""" - if manual_end_session_endpoint: + """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, - end_session_endpoint=manual_end_session_endpoint, + **settings_kwargs, ) manager = OIDCAuthenticationManager(settings=oidc_settings_with_discovery) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 7241c45f00..0d0eee5d6f 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -13,7 +13,6 @@ OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.controller import ( - OIDC_INVALID_ID_TOKEN_HINT, OIDC_INVALID_REQUEST, OIDC_INVALID_RESPONSE, OIDC_INVALID_STATE, @@ -714,6 +713,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" @@ -741,6 +741,7 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): "id_token_claims": {"sub": "user123@example.com"}, "access_token": "access-token", "refresh_token": "refresh-token", + "id_token": "raw.id.token.jwt", } # Set up library_authenticators dict @@ -761,7 +762,6 @@ 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", } headers = {"Authorization": "Bearer valid.jwt.token"} @@ -777,6 +777,92 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): ) # Verify refresh token revocation was attempted mock_auth_manager.revoke_token.assert_called_once_with("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, db): + """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. + """ + 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_provider._credential_manager.extract_token_data.return_value = { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "access-token", + "refresh_token": "refresh-token", + } + + mock_library_auth.oidc_provider_lookup.return_value = mock_provider + mock_library_auth.decode_bearer_token.return_value = ( + "Test OIDC", + "provider-token", + ) + mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( + Mock() + ) + + 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", + "id_token_hint": "test.id.token", + "post_logout_redirect_uri": "https://app.example.com/logout/callback", + } + headers = {"Authorization": "Bearer valid.jwt.token"} + + result = controller.oidc_logout_initiate(params, headers, db.session) + + # 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 + + # Verify RP-Initiated Logout was NOT attempted + mock_auth_manager.build_logout_url.assert_not_called() + + # Verify token was revoked + mock_auth_manager.revoke_token.assert_called_once_with("refresh-token") + + # Verify CM credentials were invalidated + mock_provider._credential_manager.invalidate_patron_credentials.assert_called_once_with( + db.session, patron.id + ) @pytest.mark.parametrize( "library_index,patron_email,logout_url", @@ -806,6 +892,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 @@ -825,9 +912,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" ) @@ -843,12 +928,19 @@ def test_oidc_logout_initiate_uses_correct_library( mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( Mock() ) + mock_provider._credential_manager.extract_token_data.return_value = { + "id_token_claims": {"sub": patron.authorization_identifier}, + "access_token": "access-token", + "refresh_token": "refresh-token", + "id_token": f"raw.id.token.{i+1}", + } mock_library_auth.decode_bearer_token.return_value = ( "Test OIDC", "provider-token", ) 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] = ( @@ -894,57 +986,40 @@ 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): + # Verify correct provider's credential data was used + mock_providers[ + library_index + ]._credential_manager.extract_token_data.assert_called_once() + for i, provider in enumerate(mock_providers): if i != library_index: - auth_manager.validate_id_token_hint.assert_not_called() + provider._credential_manager.extract_token_data.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) - 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 @@ -958,7 +1033,6 @@ 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", } @@ -1030,7 +1104,6 @@ def test_oidc_logout_initiate_bearer_token_errors( request_headers = {} if auth_header is None else {"Authorization": auth_header} params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } @@ -1065,7 +1138,6 @@ 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", } headers = {"Authorization": "Bearer valid.jwt.token"} @@ -1075,34 +1147,31 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): assert result == UNKNOWN_OIDC_PROVIDER @pytest.mark.parametrize( - "validation_error,claims_returned,expected_uri,expected_message", + "extract_side_effect,token_data,expected_message", [ pytest.param( - Exception("Token expired"), + ValueError("Corrupt credential"), None, - OIDC_INVALID_ID_TOKEN_HINT.uri, - "ID token hint validation failed", - id="validation-exception", + "Invalid credential data", + id="extract-token-data-exception", ), pytest.param( None, - {"iss": "issuer"}, - OIDC_INVALID_ID_TOKEN_HINT.uri, - "missing patron identifier claim", + {"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, + extract_side_effect, + token_data, expected_message, ): - """Test logout initiate ID token hint validation errors.""" + """Test logout initiate errors when stored credential data is invalid.""" library = db.default_library() mock_library_auth = Mock() @@ -1118,11 +1187,16 @@ def test_oidc_logout_initiate_id_token_hint_errors( Mock() ) - mock_auth_manager = Mock() - if validation_error: - mock_auth_manager.validate_id_token_hint.side_effect = validation_error + if extract_side_effect: + mock_provider._credential_manager.extract_token_data.side_effect = ( + extract_side_effect + ) else: - mock_auth_manager.validate_id_token_hint.return_value = claims_returned + mock_provider._credential_manager.extract_token_data.return_value = ( + token_data + ) + + mock_auth_manager = Mock() mock_provider.get_authentication_manager.return_value = mock_auth_manager mock_library_auth.oidc_provider_lookup.return_value = mock_provider @@ -1136,14 +1210,13 @@ 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", } headers = {"Authorization": "Bearer valid.jwt.token"} result = logout_controller.oidc_logout_initiate(params, headers, db.session) - assert result.uri == expected_uri + assert result.uri == OIDC_INVALID_REQUEST.uri assert expected_message in result.detail @pytest.mark.parametrize( @@ -1220,8 +1293,20 @@ def test_oidc_logout_initiate_exceptions( None ) + token_data: dict = { + "id_token_claims": {"sub": "user@test.com"}, + "access_token": "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_provider._credential_manager.extract_token_data.return_value = token_data + 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: @@ -1248,7 +1333,6 @@ def test_oidc_logout_initiate_exceptions( params = { "provider": "Test OIDC", - "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } headers = {"Authorization": "Bearer valid.jwt.token"} From 9132cb05b4b2681bfa597a7ae621adbe04dc8028 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Sun, 8 Mar 2026 18:31:37 -0400 Subject: [PATCH 03/26] Parse token data directly from bearer token instead of DB lookup --- .../patron_auth/oidc/controller.py | 35 ++--- .../patron_auth/oidc/test_controller.py | 126 +++++++----------- 2 files changed, 59 insertions(+), 102 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 997262891d..3e26067bba 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -55,13 +55,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, @@ -87,7 +80,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" @@ -400,22 +392,17 @@ def oidc_logout_initiate( if isinstance(provider, ProblemDetail): return provider - oidc_credential = provider._credential_manager.lookup_oidc_token_by_value( # type: ignore[attr-defined] - db, provider_token, library.id - ) - if not oidc_credential: - return OIDC_INVALID_REQUEST.detailed( - _("Bearer token is expired or not found") - ) - auth_manager = provider.get_authentication_manager() # type: ignore[attr-defined] - # Extract the stored token data from the credential rather than requiring - # the client to re-supply the raw id_token_hint as a separate parameter. + # 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: - token_data = provider._credential_manager.extract_token_data(oidc_credential) # type: ignore[attr-defined] - except Exception as e: - self.log.exception("Failed to extract token data from credential") + token_data = json.loads(provider_token) + if "id_token_claims" not in token_data or "access_token" not in token_data: + raise ValueError("Missing required fields in token data") + except Exception: + self.log.exception("Failed to parse token data from bearer token") return OIDC_INVALID_REQUEST.detailed(_("Invalid credential data")) id_token_claims = token_data.get("id_token_claims", {}) @@ -477,7 +464,11 @@ def oidc_logout_initiate( ) utility = OIDCUtility(self._circulation_manager.services.redis.client()) - utility.store_logout_state(logout_state, post_logout_redirect_uri) + utility.store_logout_state( + logout_state, + post_logout_redirect_uri, + metadata={"library_short_name": library.short_name}, + ) try: logout_url = auth_manager.build_logout_url( diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 0d0eee5d6f..5b2db20551 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1,5 +1,6 @@ """Tests for OIDC controller.""" +import json from unittest.mock import MagicMock, Mock, patch import jwt @@ -731,18 +732,15 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): mock_library_auth.oidc_provider_lookup.return_value = mock_provider mock_library_auth.decode_bearer_token.return_value = ( "Test OIDC", - "provider-token", - ) - mock_oidc_credential = Mock() - mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( - mock_oidc_credential + json.dumps( + { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "access-token", + "refresh_token": "refresh-token", + "id_token": "raw.id.token.jwt", + } + ), ) - mock_provider._credential_manager.extract_token_data.return_value = { - "id_token_claims": {"sub": "user123@example.com"}, - "access_token": "access-token", - "refresh_token": "refresh-token", - "id_token": "raw.id.token.jwt", - } # Set up library_authenticators dict controller._authenticator.library_authenticators[library.short_name] = ( @@ -816,19 +814,17 @@ def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): mock_patron ) mock_provider._credential_manager.invalidate_patron_credentials.return_value = 1 - mock_provider._credential_manager.extract_token_data.return_value = { - "id_token_claims": {"sub": "user123@example.com"}, - "access_token": "access-token", - "refresh_token": "refresh-token", - } mock_library_auth.oidc_provider_lookup.return_value = mock_provider mock_library_auth.decode_bearer_token.return_value = ( "Test OIDC", - "provider-token", - ) - mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( - Mock() + json.dumps( + { + "id_token_claims": {"sub": "user123@example.com"}, + "access_token": "access-token", + "refresh_token": "refresh-token", + } + ), ) controller._authenticator.library_authenticators[library.short_name] = ( @@ -925,18 +921,16 @@ def test_oidc_logout_initiate_uses_correct_library( mock_provider._credential_manager.invalidate_patron_credentials.return_value = ( 1 ) - mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( - Mock() - ) - mock_provider._credential_manager.extract_token_data.return_value = { - "id_token_claims": {"sub": patron.authorization_identifier}, - "access_token": "access-token", - "refresh_token": "refresh-token", - "id_token": f"raw.id.token.{i+1}", - } mock_library_auth.decode_bearer_token.return_value = ( "Test OIDC", - "provider-token", + json.dumps( + { + "id_token_claims": {"sub": patron.authorization_identifier}, + "access_token": "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) @@ -986,14 +980,6 @@ def test_oidc_logout_initiate_uses_correct_library( assert result.status_code == 302 assert logout_url in result.location - # Verify correct provider's credential data was used - mock_providers[ - library_index - ]._credential_manager.extract_token_data.assert_called_once() - for i, provider in enumerate(mock_providers): - if i != library_index: - provider._credential_manager.extract_token_data.assert_not_called() - @pytest.mark.parametrize( "params,expected_message", [ @@ -1065,8 +1051,8 @@ def test_oidc_logout_initiate_no_authenticator_for_library( pytest.param( "Bearer valid.jwt", None, - "Bearer token is expired or not found", - id="credential-not-found", + "Invalid credential data", + id="invalid-token-json", ), ], ) @@ -1083,18 +1069,16 @@ def test_oidc_logout_initiate_bearer_token_errors( mock_library_auth = Mock() mock_provider = Mock() - mock_provider._credential_manager = 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", - "provider-token", + "not-valid-json", ) - # Credential not found for the last test case - mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = None mock_library_auth.oidc_provider_lookup.return_value = mock_provider logout_controller._authenticator.library_authenticators[library.short_name] = ( @@ -1147,17 +1131,17 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): assert result == UNKNOWN_OIDC_PROVIDER @pytest.mark.parametrize( - "extract_side_effect,token_data,expected_message", + "provider_token,expected_message", [ pytest.param( - ValueError("Corrupt credential"), - None, + "not-valid-json", "Invalid credential data", - id="extract-token-data-exception", + id="invalid-json-in-token", ), pytest.param( - None, - {"id_token_claims": {"iss": "issuer"}, "access_token": "tok"}, + json.dumps( + {"id_token_claims": {"iss": "issuer"}, "access_token": "tok"} + ), "Credential missing patron identifier claim", id="missing-patron-claim", ), @@ -1167,34 +1151,21 @@ def test_oidc_logout_initiate_credential_data_errors( self, logout_controller, db, - extract_side_effect, - token_data, + provider_token, expected_message, ): - """Test logout initiate errors when stored credential data is invalid.""" + """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", + provider_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_oidc_token_by_value.return_value = ( - Mock() - ) - - if extract_side_effect: - mock_provider._credential_manager.extract_token_data.side_effect = ( - extract_side_effect - ) - else: - mock_provider._credential_manager.extract_token_data.return_value = ( - token_data - ) mock_auth_manager = Mock() mock_provider.get_authentication_manager.return_value = mock_auth_manager @@ -1262,19 +1233,24 @@ def test_oidc_logout_initiate_exceptions( library = db.default_library() patron = db.patron() + token_data: dict = { + "id_token_claims": {"sub": "user@test.com"}, + "access_token": "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", - "provider-token", + json.dumps(token_data), ) mock_provider = Mock() mock_provider._settings = Mock() mock_provider._settings.patron_id_claim = "sub" mock_provider._credential_manager = Mock() - mock_provider._credential_manager.lookup_oidc_token_by_value.return_value = ( - Mock() - ) if patron_found: mock_provider._credential_manager.lookup_patron_by_identifier.return_value = Mock( @@ -1293,16 +1269,6 @@ def test_oidc_logout_initiate_exceptions( None ) - token_data: dict = { - "id_token_claims": {"sub": "user@test.com"}, - "access_token": "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_provider._credential_manager.extract_token_data.return_value = token_data - mock_auth_manager = Mock() mock_auth_manager.supports_rp_initiated_logout.return_value = bool( build_url_error From f1922cd58300e85535a0284b67bd398c57f3d0a7 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Sun, 8 Mar 2026 20:30:21 -0400 Subject: [PATCH 04/26] Only fetch config at startup and after config change --- .../integration/patron_auth/oidc/provider.py | 10 +++++++++- .../patron_auth/oidc/test_provider.py | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index c1f12808b1..0219354574 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -80,6 +80,7 @@ def __init__( self._credential_manager = OIDCCredentialManager() self._settings = settings + self._auth_manager: OIDCAuthenticationManager | None = None @classmethod def label(cls) -> str: @@ -232,9 +233,16 @@ def authenticated_patron( def get_authentication_manager(self) -> OIDCAuthenticationManager: """Return OIDC authentication manager for this provider. + The manager is cached for the lifetime of the provider instance, which + matches the configuration epoch — the provider is recreated whenever + settings change. This prevents repeated fetches of the OIDC discovery + document on every authenticated request. + :return: OIDC authentication manager """ - return OIDCAuthenticationManager(self._settings) + if self._auth_manager is None: + self._auth_manager = OIDCAuthenticationManager(self._settings) + return self._auth_manager def remote_patron_lookup_from_oidc_claims( self, id_token_claims: dict[str, str] diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 57d99afd3a..b5abdcf150 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -447,3 +447,19 @@ def test_get_authentication_manager(self, oidc_provider): assert manager is not None assert manager._settings == oidc_provider._settings + + # Same instance returned on repeated calls — avoids re-fetching OIDC discovery doc. + assert oidc_provider.get_authentication_manager() is manager + + # A new provider instance (simulating a config reload) produces a new manager. + new_provider = OIDCAuthenticationProvider( + library_id=oidc_provider.library_id, + integration_id=oidc_provider.integration_id, + settings=OIDCAuthSettings( + issuer_url="https://new-idp.example.com", + client_id="new-client-id", + client_secret="new-client-secret", + ), + library_settings=OIDCAuthLibrarySettings(), + ) + assert new_provider.get_authentication_manager() is not manager From 01030584b692ec3c8a6e8ed0ba164d2109963547 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 11 Mar 2026 15:53:32 -0400 Subject: [PATCH 05/26] Code review feedback --- src/palace/manager/api/routes.py | 3 +- .../patron_auth/oidc/configuration/model.py | 2 +- .../patron_auth/oidc/controller.py | 34 ++++++---- .../integration/patron_auth/oidc/util.py | 20 ++++++ tests/manager/api/test_routes.py | 2 +- .../patron_auth/oidc/test_controller.py | 65 +++++++++++-------- .../integration/patron_auth/oidc/test_util.py | 24 +++++++ 7 files changed, 105 insertions(+), 45 deletions(-) diff --git a/src/palace/manager/api/routes.py b/src/palace/manager/api/routes.py index f9c3c74081..803e0a46b9 100644 --- a/src/palace/manager/api/routes.py +++ b/src/palace/manager/api/routes.py @@ -580,8 +580,9 @@ 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, dict(flask.request.headers), app.manager._db + flask.request.args, app.manager._db, auth_header=auth_header ) 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 96ec06c75a..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,7 +157,7 @@ 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. " diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 3e26067bba..1c4295d69b 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -14,7 +14,10 @@ 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.util import ( + OIDCStateValidationError, + OIDCUtility, +) from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import ( ProblemDetail, @@ -341,14 +344,15 @@ def oidc_authentication_callback( def oidc_logout_initiate( self, request_args: dict[str, str], - request_headers: dict[str, str], db: Session, + *, + auth_header: str, ) -> BaseResponse | ProblemDetail: """Initiate OIDC RP-Initiated Logout flow. :param request_args: Request arguments from Flask - :param request_headers: Request headers 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) @@ -374,7 +378,6 @@ def oidc_logout_initiate( ) # Validate CM bearer token — ensures patron has an active CM session. - auth_header = request_headers.get("Authorization", "") if not auth_header.startswith("Bearer "): return OIDC_INVALID_REQUEST.detailed( _("Missing or invalid Authorization header") @@ -396,7 +399,7 @@ def oidc_logout_initiate( # 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). + # value changes on refresh, but the patron's bearer token still has the old value). try: token_data = json.loads(provider_token) if "id_token_claims" not in token_data or "access_token" not in token_data: @@ -413,8 +416,7 @@ def oidc_logout_initiate( ) # The raw id_token JWT stored at authentication time; used as id_token_hint - # for RP-Initiated Logout. May be absent for credentials created before this - # feature was introduced — in that case RP-Initiated Logout is skipped. + # for RP-Initiated Logout. stored_id_token = token_data.get("id_token") try: @@ -464,11 +466,7 @@ def oidc_logout_initiate( ) utility = OIDCUtility(self._circulation_manager.services.redis.client()) - utility.store_logout_state( - logout_state, - post_logout_redirect_uri, - metadata={"library_short_name": library.short_name}, - ) + utility.store_logout_state(logout_state, post_logout_redirect_uri) try: logout_url = auth_manager.build_logout_url( @@ -510,7 +508,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: + self.log.exception("Failed to decode logout state payload") + return OIDC_INVALID_STATE + + library_short_name = state_payload.get("library_short_name") if not library_short_name: return OIDC_INVALID_STATE.detailed(_("Missing library in logout state")) @@ -526,7 +532,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/util.py b/src/palace/manager/integration/patron_auth/oidc/util.py index 301ac44e1c..bcead1cf91 100644 --- a/src/palace/manager/integration/patron_auth/oidc/util.py +++ b/src/palace/manager/integration/patron_auth/oidc/util.py @@ -235,6 +235,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 8fcde1ea49..e8994030da 100644 --- a/tests/manager/api/test_routes.py +++ b/tests/manager/api/test_routes.py @@ -534,7 +534,7 @@ def test_oidc_route( for key, value in expected_params.items(): assert call_args[0][0][key] == value if passes_headers: - assert isinstance(call_args[0][1], dict) + 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/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 5b2db20551..2c1252e3e7 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -762,9 +762,9 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): "provider": "Test OIDC", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = controller.oidc_logout_initiate(params, headers, 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 @@ -840,9 +840,9 @@ def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = controller.oidc_logout_initiate(params, headers, db.session) + 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 @@ -964,9 +964,9 @@ def test_oidc_logout_initiate_uses_correct_library( "id_token_hint": "test.id.token", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = controller.oidc_logout_initiate(params, headers, 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") @@ -1003,7 +1003,9 @@ def test_oidc_logout_initiate_missing_parameter( self, logout_controller, db, params, expected_message ): """Test OIDC logout initiate with missing required parameters.""" - result = logout_controller.oidc_logout_initiate(params, {}, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="" + ) assert result == OIDC_INVALID_REQUEST.detailed(expected_message) @@ -1022,7 +1024,9 @@ def test_oidc_logout_initiate_no_authenticator_for_library( "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 @@ -1085,7 +1089,6 @@ def test_oidc_logout_initiate_bearer_token_errors( mock_library_auth ) - request_headers = {} if auth_header is None else {"Authorization": auth_header} params = { "provider": "Test OIDC", "post_logout_redirect_uri": "https://app.example.com/logout/callback", @@ -1096,7 +1099,9 @@ def test_oidc_logout_initiate_bearer_token_errors( return_value=library, ): result = logout_controller.oidc_logout_initiate( - params, request_headers, db.session + params, + db.session, + auth_header="" if auth_header is None else auth_header, ) assert result.uri == OIDC_INVALID_REQUEST.uri @@ -1124,9 +1129,9 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): "provider": "Unknown", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = logout_controller.oidc_logout_initiate(params, headers, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) assert result == UNKNOWN_OIDC_PROVIDER @@ -1183,9 +1188,9 @@ def test_oidc_logout_initiate_credential_data_errors( "provider": "Test OIDC", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = logout_controller.oidc_logout_initiate(params, headers, db.session) + result = logout_controller.oidc_logout_initiate( + params, db.session, auth_header="Bearer valid.jwt.token" + ) assert result.uri == OIDC_INVALID_REQUEST.uri assert expected_message in result.detail @@ -1301,9 +1306,9 @@ def test_oidc_logout_initiate_exceptions( "provider": "Test OIDC", "post_logout_redirect_uri": "https://app.example.com/logout/callback", } - headers = {"Authorization": "Bearer valid.jwt.token"} - - result = logout_controller.oidc_logout_initiate(params, headers, 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 @@ -1337,7 +1342,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} @@ -1425,7 +1429,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} @@ -1438,13 +1441,20 @@ def test_oidc_logout_callback_no_authenticator_for_library( 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 @@ -1455,7 +1465,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_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.""" From be33b96992348e3c042636167ca19a0c78f858b7 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Mon, 16 Mar 2026 21:29:20 -0400 Subject: [PATCH 06/26] Logout URL updates --- .../patron_auth/oidc/controller.py | 4 ++-- .../integration/patron_auth/oidc/provider.py | 24 ++++++++++++++++++- .../integration/patron_auth/oidc/util.py | 3 +++ .../patron_auth/oidc/test_provider.py | 12 ++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 1c4295d69b..f667dbf678 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -15,6 +15,7 @@ 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 ( + LOGOUT_REDIRECT_QUERY_PARAM, OIDCStateValidationError, OIDCUtility, ) @@ -83,7 +84,6 @@ class OIDCController(LoggerMixin): CODE = "code" ACCESS_TOKEN = "access_token" PATRON_INFO = "patron_info" - POST_LOGOUT_REDIRECT_URI = "post_logout_redirect_uri" LOGOUT_STATUS = "logout_status" LOGOUT_TOKEN = "logout_token" @@ -356,7 +356,7 @@ def oidc_logout_initiate( :return: Redirect to provider logout endpoint or error """ provider_name = request_args.get(self.PROVIDER_NAME) - 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( diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index 0219354574..e5e6fffc50 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -22,6 +22,7 @@ OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.credential import OIDCCredentialManager +from palace.manager.integration.patron_auth.oidc.util import LOGOUT_REDIRECT_QUERY_PARAM from palace.manager.service.analytics.analytics import Analytics from palace.manager.sqlalchemy.model.credential import Credential from palace.manager.sqlalchemy.model.patron import Patron @@ -33,6 +34,13 @@ if TYPE_CHECKING: from palace.manager.core.selftest import SelfTestResult +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, @@ -188,7 +196,21 @@ def _authentication_flow_document(self, db: Session) -> dict[str, Any]: library_short_name=library.short_name, provider=self.label(), ) - links.append({"rel": "logout", "href": logout_url}) + 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, diff --git a/src/palace/manager/integration/patron_auth/oidc/util.py b/src/palace/manager/integration/patron_auth/oidc/util.py index bcead1cf91..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.""" diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index b5abdcf150..1675cbb774 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -15,8 +15,12 @@ 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 from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.util.problem_detail import ProblemDetailException from tests.fixtures.database import DatabaseTransactionFixture @@ -222,6 +226,14 @@ def test_authentication_flow_document_with_logout_link( 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 From 45262356015d11ee2d9de7d766152339638883ed Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 21:27:42 -0400 Subject: [PATCH 07/26] Reorder decorators --- src/palace/manager/api/routes.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/api/routes.py b/src/palace/manager/api/routes.py index 803e0a46b9..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 @@ -587,8 +587,8 @@ def oidc_logout(): # 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 @@ -623,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 From 898e265dc31bf25ad0ab2440a4106f41203b7088 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 22:33:54 -0400 Subject: [PATCH 08/26] id_token may be absent after token refresh --- .../patron_auth/oidc/controller.py | 17 ++++- .../patron_auth/oidc/test_controller.py | 64 +++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index f667dbf678..e6b22269d1 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -446,14 +446,25 @@ def oidc_logout_initiate( ) # If the provider does not support RP-Initiated Logout (only token revocation), - # or if no id_token was stored (credentials pre-dating this feature), redirect - # directly — local invalidation and token revocation are already done. - if not auth_manager.supports_rp_initiated_logout() or not stored_id_token: + # redirect directly — local invalidation and token revocation are already done. + if not auth_manager.supports_rp_initiated_logout(): final_redirect_uri = self._add_params_to_url( post_logout_redirect_uri, {self.LOGOUT_STATUS: "success"} ) return redirect(final_redirect_uri) + # If no id_token is stored, skip RP-Initiated Logout. The provider session + # may remain active. + if not stored_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 = { diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 2c1252e3e7..fe83455a83 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -860,6 +860,70 @@ def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): db.session, patron.id ) + def test_oidc_logout_initiate_no_stored_id_token(self, logout_controller, db): + """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": "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 + + mock_auth_manager.build_logout_url.assert_not_called() + @pytest.mark.parametrize( "library_index,patron_email,logout_url", [ From 2181aa3a3e0a81776132f76223a21873f199360f Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 22:41:08 -0400 Subject: [PATCH 09/26] Tighten exceptions --- .../integration/patron_auth/oidc/auth.py | 2 +- .../patron_auth/oidc/controller.py | 7 ++- .../patron_auth/oidc/test_controller.py | 62 ++++++++++++++++++- 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index 1228467ea8..365f65518f 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -560,7 +560,7 @@ def revoke_token(self, token: str, token_type_hint: str = "refresh_token") -> No allowed_response_codes=["2xx"], ) self.log.info(f"Successfully revoked {token_type_hint}") - except Exception: + 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]: diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index e6b22269d1..4ded4be243 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -9,6 +9,8 @@ import jwt from flask import redirect, url_for from flask_babel import lazy_gettext as _ +from requests import RequestException +from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session from werkzeug.wrappers import Response as BaseResponse @@ -19,6 +21,7 @@ OIDCStateValidationError, OIDCUtility, ) +from palace.manager.util.http.exception import RequestNetworkException from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import ( ProblemDetail, @@ -431,7 +434,7 @@ def oidc_logout_initiate( else: self.log.warning(f"Patron not found for identifier {patron_identifier}") - except Exception as e: + except (SQLAlchemyError, AttributeError) as e: self.log.exception("Failed to invalidate credentials") # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication @@ -440,7 +443,7 @@ def oidc_logout_initiate( if refresh_token: try: auth_manager.revoke_token(refresh_token) - except Exception: + except (RequestNetworkException, RequestException): self.log.warning( "Failed to revoke refresh token (non-critical)", exc_info=True ) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index fe83455a83..f1529528ec 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -5,6 +5,7 @@ import jwt import pytest +from sqlalchemy.exc import SQLAlchemyError from palace.manager.api.authentication.base import PatronData from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider @@ -1272,7 +1273,7 @@ def test_oidc_logout_initiate_credential_data_errors( ), pytest.param( True, - Exception("Database error"), + SQLAlchemyError("Database error"), None, 302, None, @@ -1381,6 +1382,65 @@ def test_oidc_logout_initiate_exceptions( else: assert result.uri == expected_uri + def test_oidc_logout_initiate_credential_invalidation_is_nonfatal( + self, logout_controller, db + ): + """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": "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_called_once_with("refresh-token") + def test_oidc_logout_callback_success(self, logout_controller, db): library = db.default_library() From 68699766908660d4d6ff8cd14334996db48c2acb Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 22:50:58 -0400 Subject: [PATCH 10/26] DRY credential field validation --- .../patron_auth/oidc/controller.py | 7 ++-- .../patron_auth/oidc/credential.py | 32 +++++++++------ .../patron_auth/oidc/test_credential.py | 39 +++++++++++++++++++ 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 4ded4be243..f8773ced64 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -16,6 +16,7 @@ from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider from palace.manager.api.util.flask import get_request_library +from palace.manager.integration.patron_auth.oidc.credential import OIDCCredentialManager from palace.manager.integration.patron_auth.oidc.util import ( LOGOUT_REDIRECT_QUERY_PARAM, OIDCStateValidationError, @@ -404,10 +405,8 @@ def oidc_logout_initiate( # 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: - token_data = json.loads(provider_token) - if "id_token_claims" not in token_data or "access_token" not in token_data: - raise ValueError("Missing required fields in token data") - except Exception: + 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")) diff --git a/src/palace/manager/integration/patron_auth/oidc/credential.py b/src/palace/manager/integration/patron_auth/oidc/credential.py index 7c70ddccb1..654f198034 100644 --- a/src/palace/manager/integration/patron_auth/oidc/credential.py +++ b/src/palace/manager/integration/patron_auth/oidc/credential.py @@ -79,23 +79,19 @@ def _create_token_value( return json.dumps(token_data) - def extract_token_data(self, credential: Credential) -> dict[str, Any]: - """Extract token data from credential. + @staticmethod + def parse_token_value(value: str) -> dict[str, Any]: + """Parse and validate a raw OIDC credential JSON string. - :param credential: Credential object containing OIDC token - :return: Dictionary with id_token_claims, access_token, refresh_token + :param value: JSON string produced by :meth:`_create_token_value` + :raises ValueError: If the string is not valid JSON or is missing required fields + :return: Parsed token data dictionary """ - 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)) + token_data = cast(dict[str, Any], json.loads(value)) except json.JSONDecodeError 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: @@ -103,6 +99,20 @@ def extract_token_data(self, credential: Credential) -> dict[str, Any]: return token_data + def extract_token_data(self, credential: Credential) -> dict[str, Any]: + """Extract token data from credential. + + :param credential: Credential object containing OIDC token + :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: + return self.parse_token_value(credential_value) + except ValueError as e: + self.log.exception("Failed to decode OIDC token data") + raise + def create_oidc_token( self, db: Session, diff --git a/tests/manager/integration/patron_auth/oidc/test_credential.py b/tests/manager/integration/patron_auth/oidc/test_credential.py index 351f5ecbde..53e365030f 100644 --- a/tests/manager/integration/patron_auth/oidc/test_credential.py +++ b/tests/manager/integration/patron_auth/oidc/test_credential.py @@ -78,6 +78,45 @@ 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 id_token_claims", + id="missing-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, From bb1e796deb6fc9fb1c337b1f1dc009b9ae2233a0 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 23:05:54 -0400 Subject: [PATCH 11/26] Don't conflate unsupport RP-Initiate Logout with missing id token --- .../integration/patron_auth/oidc/controller.py | 1 + .../patron_auth/oidc/test_controller.py | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index f8773ced64..48d7cfc8e1 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -450,6 +450,7 @@ def oidc_logout_initiate( # 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"} ) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index f1529528ec..c447efcd6e 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1,6 +1,7 @@ """Tests for OIDC controller.""" import json +import logging from unittest.mock import MagicMock, Mock, patch import jwt @@ -783,12 +784,18 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): mock_auth_manager.build_logout_url.call_args[0][2], ) - def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): + 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" @@ -849,6 +856,7 @@ def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): 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() @@ -861,7 +869,12 @@ def test_oidc_logout_initiate_revocation_only(self, logout_controller, db): db.session, patron.id ) - def test_oidc_logout_initiate_no_stored_id_token(self, logout_controller, db): + 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. @@ -922,6 +935,7 @@ def test_oidc_logout_initiate_no_stored_id_token(self, logout_controller, db): 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() From 389550e23129349963d8216c216b261ff778c221 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 23:16:50 -0400 Subject: [PATCH 12/26] Narrow another exception --- .../patron_auth/oidc/controller.py | 4 +++- .../patron_auth/oidc/test_controller.py | 23 ++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 48d7cfc8e1..bbbeba8efe 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -16,9 +16,11 @@ from palace.manager.api.authenticator import BaseOIDCAuthenticationProvider from palace.manager.api.util.flask import get_request_library +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, ) @@ -486,7 +488,7 @@ def oidc_logout_initiate( logout_url = auth_manager.build_logout_url( stored_id_token, callback_url, logout_state ) - except Exception as e: + except (OIDCDiscoveryError, OIDCAuthenticationError) as e: self.log.exception("Failed to build logout URL") return OIDC_LOGOUT_NOT_SUPPORTED.detailed(_(str(e))) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index c447efcd6e..7225d66b7d 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -11,6 +11,7 @@ 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, @@ -1296,7 +1297,7 @@ def test_oidc_logout_initiate_credential_data_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", @@ -1305,14 +1306,14 @@ def test_oidc_logout_initiate_credential_data_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() @@ -1397,8 +1398,8 @@ def test_oidc_logout_initiate_exceptions( assert result.uri == expected_uri def test_oidc_logout_initiate_credential_invalidation_is_nonfatal( - self, logout_controller, db - ): + 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 From 5c876ca88b9c9090f23e0b318c07735a959861c9 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 23:19:55 -0400 Subject: [PATCH 13/26] Add a helper --- .../integration/patron_auth/oidc/auth.py | 36 ++++++++++--------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index 365f65518f..0c59b78cd4 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -486,6 +486,20 @@ 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. @@ -494,13 +508,9 @@ def supports_rp_initiated_logout(self) -> bool: :return: True if RP-Initiated Logout is supported """ - try: - metadata = self.get_provider_metadata() - if metadata.get("end_session_endpoint"): - return True - except OIDCDiscoveryError: - pass - return bool(self._settings.end_session_endpoint) + 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. @@ -511,15 +521,9 @@ def supports_logout(self) -> bool: :return: True if any logout mechanism is supported """ - try: - metadata = self.get_provider_metadata() - if metadata.get("end_session_endpoint") or metadata.get( - "revocation_endpoint" - ): - return True - except OIDCDiscoveryError: - pass - return bool( + return self._has_metadata_endpoints( + "end_session_endpoint", "revocation_endpoint" + ) or bool( self._settings.end_session_endpoint or self._settings.revocation_endpoint ) From fd1c940951b4cb8cc6fb9170b60661a29a5b4fc4 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Tue, 17 Mar 2026 23:30:01 -0400 Subject: [PATCH 14/26] Dead try/except --- .../integration/patron_auth/oidc/controller.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index bbbeba8efe..2c69431f50 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -9,7 +9,6 @@ import jwt from flask import redirect, url_for from flask_babel import lazy_gettext as _ -from requests import RequestException from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm import Session from werkzeug.wrappers import Response as BaseResponse @@ -24,7 +23,6 @@ OIDCStateValidationError, OIDCUtility, ) -from palace.manager.util.http.exception import RequestNetworkException from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import ( ProblemDetail, @@ -439,15 +437,10 @@ def oidc_logout_initiate( self.log.exception("Failed to invalidate credentials") # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication - # at the IdP after local CM logout. Errors are non-fatal. + # at the IdP after local CM logout. revoke_token suppresses all errors internally. refresh_token = token_data.get("refresh_token") if refresh_token: - try: - auth_manager.revoke_token(refresh_token) - except (RequestNetworkException, RequestException): - self.log.warning( - "Failed to revoke refresh token (non-critical)", exc_info=True - ) + auth_manager.revoke_token(refresh_token) # If the provider does not support RP-Initiated Logout (only token revocation), # redirect directly — local invalidation and token revocation are already done. From e4adcce5b5b4697e9e48824c913a8290ce3652af Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 09:27:11 -0400 Subject: [PATCH 15/26] Retry configuration if auto-discovery previously failed --- .../integration/patron_auth/oidc/auth.py | 5 + .../integration/patron_auth/oidc/provider.py | 29 +++- .../patron_auth/oidc/test_provider.py | 125 ++++++++++++++++-- 3 files changed, 139 insertions(+), 20 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index 0c59b78cd4..1b6a58c425 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -78,6 +78,11 @@ def __init__( self._validator = OIDCTokenValidator() self._metadata: dict[str, Any] | None = None + @property + def is_configured(self) -> bool: + """Return True if provider metadata has been successfully loaded.""" + return self._metadata is not None + def _extract_error_detail_from_response( self, exception: RequestNetworkException ) -> str: diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index e5e6fffc50..afefbcc5c5 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -22,7 +22,10 @@ OIDCAuthSettings, ) from palace.manager.integration.patron_auth.oidc.credential import OIDCCredentialManager -from palace.manager.integration.patron_auth.oidc.util import LOGOUT_REDIRECT_QUERY_PARAM +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 @@ -255,15 +258,27 @@ def authenticated_patron( def get_authentication_manager(self) -> OIDCAuthenticationManager: """Return OIDC authentication manager for this provider. - The manager is cached for the lifetime of the provider instance, which - matches the configuration epoch — the provider is recreated whenever - settings change. This prevents repeated fetches of the OIDC discovery - document on every authenticated request. + The manager is cached once it has successfully loaded provider metadata + (i.e. ``is_configured`` is True). If configuration fails — for example + because the IdP is temporarily unreachable — the manager is returned + uncached so the next call retries configuration from scratch. :return: OIDC authentication manager """ - if self._auth_manager is None: - self._auth_manager = 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( diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 1675cbb774..49e591ea55 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 @@ -20,7 +21,10 @@ PALACE_REDIRECT_URI_TERM, OIDCAuthenticationProvider, ) -from palace.manager.integration.patron_auth.oidc.util import LOGOUT_REDIRECT_QUERY_PARAM +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 @@ -262,6 +266,83 @@ def test_authentication_flow_document_without_logout_link( 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.""" + with patch( + "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" + ) as MockManager: + mock_manager = MagicMock() + mock_manager.is_configured = True + 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.""" + 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.""" + 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 == [] @@ -454,24 +535,42 @@ 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: + 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 not None - assert manager._settings == oidc_provider._settings + 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 + # 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=OIDCAuthSettings( - issuer_url="https://new-idp.example.com", - client_id="new-client-id", - client_secret="new-client-secret", - ), + settings=new_settings, library_settings=OIDCAuthLibrarySettings(), ) - assert new_provider.get_authentication_manager() is not manager + 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 new_provider.get_authentication_manager() is not manager From deaff05a4ff2794a5243c4be4146d8eebfac36cf Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 09:49:12 -0400 Subject: [PATCH 16/26] Fix slow tests --- .../manager/integration/patron_auth/oidc/conftest.py | 12 +++++++++++- .../integration/patron_auth/oidc/test_provider.py | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-) 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_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 49e591ea55..0926a1c3ff 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -270,6 +270,7 @@ 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: @@ -290,6 +291,7 @@ def test_get_authentication_manager_not_cached_on_discovery_failure( 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( @@ -321,6 +323,7 @@ def test_get_authentication_manager_cached_after_recovery( 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( @@ -538,6 +541,7 @@ def test_oidc_callback(self, db: DatabaseTransactionFixture, oidc_provider): 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: From 9618e043ad7c2f43d8d630622fed6592d537a7e8 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 11:07:10 -0400 Subject: [PATCH 17/26] A little extra type checking --- .../manager/integration/patron_auth/oidc/controller.py | 2 +- .../manager/integration/patron_auth/oidc/credential.py | 9 +++++---- .../integration/patron_auth/oidc/test_credential.py | 9 +++++++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 2c69431f50..c193bf633c 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -410,7 +410,7 @@ def oidc_logout_initiate( self.log.exception("Failed to parse token data from bearer token") return OIDC_INVALID_REQUEST.detailed(_("Invalid credential data")) - id_token_claims = token_data.get("id_token_claims", {}) + id_token_claims = token_data["id_token_claims"] patron_identifier = id_token_claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] if not patron_identifier: return OIDC_INVALID_REQUEST.detailed( diff --git a/src/palace/manager/integration/patron_auth/oidc/credential.py b/src/palace/manager/integration/patron_auth/oidc/credential.py index 654f198034..4e614f0149 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, @@ -90,12 +91,12 @@ def parse_token_value(value: str) -> dict[str, Any]: try: token_data = cast(dict[str, Any], json.loads(value)) except json.JSONDecodeError as e: - raise ValueError(f"Invalid OIDC token format: {str(e)}") from e + raise PalaceValueError(f"Invalid OIDC token format: {str(e)}") from e - if "id_token_claims" not in token_data: - raise ValueError("OIDC token missing id_token_claims") + 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 ValueError("OIDC token missing access_token") + raise PalaceValueError("OIDC token missing access_token") return token_data diff --git a/tests/manager/integration/patron_auth/oidc/test_credential.py b/tests/manager/integration/patron_auth/oidc/test_credential.py index 53e365030f..1eb946b4de 100644 --- a/tests/manager/integration/patron_auth/oidc/test_credential.py +++ b/tests/manager/integration/patron_auth/oidc/test_credential.py @@ -84,9 +84,14 @@ def test_create_token_value( pytest.param("not-json", "Invalid OIDC token format", id="invalid-json"), pytest.param( json.dumps({"access_token": "tok"}), - "missing id_token_claims", + "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", @@ -184,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, From 0a2b3d8767cb011d2843bb6b280846c2a6207a1c Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 11:27:14 -0400 Subject: [PATCH 18/26] Code clean up --- src/palace/manager/integration/patron_auth/oidc/controller.py | 2 +- src/palace/manager/integration/patron_auth/oidc/provider.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index c193bf633c..4fb9f8955c 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -433,7 +433,7 @@ def oidc_logout_initiate( else: self.log.warning(f"Patron not found for identifier {patron_identifier}") - except (SQLAlchemyError, AttributeError) as e: + except (SQLAlchemyError, AttributeError): self.log.exception("Failed to invalidate credentials") # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index afefbcc5c5..65de7eef20 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -37,6 +37,8 @@ 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" From b06e4167eb0e678f15cae419a871d06f2aec1dad Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 11:53:04 -0400 Subject: [PATCH 19/26] Ensure token provider matches actual provider --- .../patron_auth/oidc/controller.py | 8 +++- .../patron_auth/oidc/test_controller.py | 43 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 4fb9f8955c..e405a00733 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -389,12 +389,15 @@ def oidc_logout_initiate( cm_jwt = auth_header.removeprefix("Bearer ") try: - _provider_name, provider_token = library_authenticator.decode_bearer_token( - cm_jwt + decoded_provider_name, provider_token = ( + library_authenticator.decode_bearer_token(cm_jwt) ) except jwt.exceptions.InvalidTokenError: 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 @@ -483,6 +486,7 @@ def oidc_logout_initiate( ) 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) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 7225d66b7d..866d2b317f 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1215,6 +1215,40 @@ def test_oidc_logout_initiate_unknown_provider(self, logout_controller, db): 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( "provider_token,expected_message", [ @@ -1379,6 +1413,9 @@ 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" @@ -1396,6 +1433,12 @@ 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 From 57ef3b926fefb2dcd076281a06984990a1fd8a95 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 12:42:34 -0400 Subject: [PATCH 20/26] Try harder to revoke session --- .../patron_auth/oidc/controller.py | 7 ++- .../patron_auth/oidc/test_controller.py | 50 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index e405a00733..ecd714dfb1 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -415,7 +415,13 @@ def oidc_logout_initiate( id_token_claims = token_data["id_token_claims"] patron_identifier = id_token_claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] + refresh_token = token_data.get("refresh_token") + if not patron_identifier: + # Best-effort cleanup: revoke the refresh token so the IdP session doesn't + # persist even though we can't identify (and thus invalidate) the patron. + if refresh_token: + auth_manager.revoke_token(refresh_token) return OIDC_INVALID_REQUEST.detailed( _("Credential missing patron identifier claim") ) @@ -441,7 +447,6 @@ def oidc_logout_initiate( # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication # at the IdP after local CM logout. revoke_token suppresses all errors internally. - refresh_token = token_data.get("refresh_token") if refresh_token: auth_manager.revoke_token(refresh_token) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 866d2b317f..eb868787da 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1309,6 +1309,56 @@ def test_oidc_logout_initiate_credential_data_errors( 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": "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_called_once_with("refresh-token") + @pytest.mark.parametrize( "patron_found,invalidation_error,build_url_error,expected_status,expected_uri", [ From 7ba10e829b8e9e59ea3a3be5a947a375f17aeb6d Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 12:46:07 -0400 Subject: [PATCH 21/26] Drop some more dead code --- src/palace/manager/integration/patron_auth/oidc/auth.py | 5 ----- .../manager/integration/patron_auth/oidc/provider.py | 7 +++---- .../manager/integration/patron_auth/oidc/test_provider.py | 1 - 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/auth.py b/src/palace/manager/integration/patron_auth/oidc/auth.py index 1b6a58c425..0c59b78cd4 100644 --- a/src/palace/manager/integration/patron_auth/oidc/auth.py +++ b/src/palace/manager/integration/patron_auth/oidc/auth.py @@ -78,11 +78,6 @@ def __init__( self._validator = OIDCTokenValidator() self._metadata: dict[str, Any] | None = None - @property - def is_configured(self) -> bool: - """Return True if provider metadata has been successfully loaded.""" - return self._metadata is not None - def _extract_error_detail_from_response( self, exception: RequestNetworkException ) -> str: diff --git a/src/palace/manager/integration/patron_auth/oidc/provider.py b/src/palace/manager/integration/patron_auth/oidc/provider.py index 65de7eef20..466a3dbb29 100644 --- a/src/palace/manager/integration/patron_auth/oidc/provider.py +++ b/src/palace/manager/integration/patron_auth/oidc/provider.py @@ -260,10 +260,9 @@ def authenticated_patron( def get_authentication_manager(self) -> OIDCAuthenticationManager: """Return OIDC authentication manager for this provider. - The manager is cached once it has successfully loaded provider metadata - (i.e. ``is_configured`` is True). If configuration fails — for example - because the IdP is temporarily unreachable — the manager is returned - uncached so the next call retries configuration from scratch. + 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 """ diff --git a/tests/manager/integration/patron_auth/oidc/test_provider.py b/tests/manager/integration/patron_auth/oidc/test_provider.py index 0926a1c3ff..b41ca47814 100644 --- a/tests/manager/integration/patron_auth/oidc/test_provider.py +++ b/tests/manager/integration/patron_auth/oidc/test_provider.py @@ -275,7 +275,6 @@ def test_get_authentication_manager_cached_after_successful_configuration( "palace.manager.integration.patron_auth.oidc.provider.OIDCAuthenticationManager" ) as MockManager: mock_manager = MagicMock() - mock_manager.is_configured = True mock_manager.get_provider_metadata.return_value = {} MockManager.return_value = mock_manager From 845f19c4f928b74b73c1cd1822a344d79e7f1e1e Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 12:54:22 -0400 Subject: [PATCH 22/26] PalaceValueError vs ValueError --- src/palace/manager/integration/patron_auth/oidc/credential.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/credential.py b/src/palace/manager/integration/patron_auth/oidc/credential.py index 4e614f0149..221a901ceb 100644 --- a/src/palace/manager/integration/patron_auth/oidc/credential.py +++ b/src/palace/manager/integration/patron_auth/oidc/credential.py @@ -85,7 +85,9 @@ 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 ValueError: If the string is not valid JSON or is missing required fields + :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: From 3a82d78d3b01058be6ab6f0fa0a6cf008f6b790f Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 13:03:41 -0400 Subject: [PATCH 23/26] Log warning for invalid bearer token --- .../integration/patron_auth/oidc/controller.py | 1 + .../patron_auth/oidc/test_controller.py | 17 +++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index ecd714dfb1..82f47acb48 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -393,6 +393,7 @@ def oidc_logout_initiate( 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: diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index eb868787da..2d16261e92 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1142,13 +1142,15 @@ def test_oidc_logout_initiate_no_authenticator_for_library( ) def test_oidc_logout_initiate_bearer_token_errors( self, - logout_controller, - db, - auth_header, - decode_side_effect, - expected_detail, - ): + 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() @@ -1185,7 +1187,10 @@ def test_oidc_logout_initiate_bearer_token_errors( ) 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.""" From 9ca70752dcabb9fee9240d5c739f8dcbf2cd9353 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 13:08:12 -0400 Subject: [PATCH 24/26] Clean up some stale test data --- tests/manager/integration/patron_auth/oidc/test_controller.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index 2d16261e92..dba22b40d4 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -846,7 +846,6 @@ def test_oidc_logout_initiate_revocation_only( ): 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( @@ -1041,7 +1040,6 @@ 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( From 98e2e5299327662642a5966ab1f32c4992a03d36 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 13:23:37 -0400 Subject: [PATCH 25/26] More detailed PD and add a test --- .../patron_auth/oidc/controller.py | 4 ++-- .../patron_auth/oidc/test_controller.py | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 82f47acb48..4c9f2b4bb6 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -531,9 +531,9 @@ def oidc_logout_callback( # is needed to look up the signing secret before full validation. try: state_payload = OIDCUtility.decode_state_payload(state) - except OIDCStateValidationError: + except OIDCStateValidationError as e: self.log.exception("Failed to decode logout state payload") - return OIDC_INVALID_STATE + return OIDC_INVALID_STATE.detailed(_(str(e))) library_short_name = state_payload.get("library_short_name") if not library_short_name: diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index dba22b40d4..da3c80e17b 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -1673,6 +1673,30 @@ 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 ): From 96d13a5115b8356d56ec2eec26ee7408f4af6dd6 Mon Sep 17 00:00:00 2001 From: Tim DiLauro Date: Wed, 18 Mar 2026 14:11:28 -0400 Subject: [PATCH 26/26] Revoke access token when revoking refresh token --- .../patron_auth/oidc/controller.py | 29 ++++++-------- .../patron_auth/oidc/test_controller.py | 38 ++++++++++++------- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/src/palace/manager/integration/patron_auth/oidc/controller.py b/src/palace/manager/integration/patron_auth/oidc/controller.py index 4c9f2b4bb6..eacc532b74 100644 --- a/src/palace/manager/integration/patron_auth/oidc/controller.py +++ b/src/palace/manager/integration/patron_auth/oidc/controller.py @@ -416,21 +416,20 @@ def oidc_logout_initiate( id_token_claims = token_data["id_token_claims"] patron_identifier = id_token_claims.get(provider._settings.patron_id_claim) # type: ignore[attr-defined] - refresh_token = token_data.get("refresh_token") + + # 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) if not patron_identifier: - # Best-effort cleanup: revoke the refresh token so the IdP session doesn't - # persist even though we can't identify (and thus invalidate) the patron. - if refresh_token: - auth_manager.revoke_token(refresh_token) return OIDC_INVALID_REQUEST.detailed( _("Credential missing patron identifier claim") ) - # The raw id_token JWT stored at authentication time; used as id_token_hint - # for RP-Initiated Logout. - stored_id_token = token_data.get("id_token") - + # Invalidate our local patron credentials. try: credential_manager = provider._credential_manager # type: ignore[attr-defined] patron = credential_manager.lookup_patron_by_identifier( @@ -442,15 +441,9 @@ 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 (SQLAlchemyError, AttributeError): self.log.exception("Failed to invalidate credentials") - # Best-effort: revoke the OAuth refresh token to prevent silent re-authentication - # at the IdP after local CM logout. revoke_token suppresses all errors internally. - if refresh_token: - auth_manager.revoke_token(refresh_token) - # 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(): @@ -460,9 +453,9 @@ def oidc_logout_initiate( ) return redirect(final_redirect_uri) - # If no id_token is stored, skip RP-Initiated Logout. The provider session - # may remain active. - if not stored_id_token: + # 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." diff --git a/tests/manager/integration/patron_auth/oidc/test_controller.py b/tests/manager/integration/patron_auth/oidc/test_controller.py index da3c80e17b..eb734be47e 100644 --- a/tests/manager/integration/patron_auth/oidc/test_controller.py +++ b/tests/manager/integration/patron_auth/oidc/test_controller.py @@ -738,7 +738,7 @@ def test_oidc_logout_initiate_success(self, logout_controller, db): json.dumps( { "id_token_claims": {"sub": "user123@example.com"}, - "access_token": "access-token", + "access_token": "test-access-token", "refresh_token": "refresh-token", "id_token": "raw.id.token.jwt", } @@ -776,8 +776,11 @@ 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 refresh token revocation was attempted - mock_auth_manager.revoke_token.assert_called_once_with("refresh-token") + # 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", @@ -830,7 +833,7 @@ def test_oidc_logout_initiate_revocation_only( json.dumps( { "id_token_claims": {"sub": "user123@example.com"}, - "access_token": "access-token", + "access_token": "test-access-token", "refresh_token": "refresh-token", } ), @@ -861,8 +864,11 @@ def test_oidc_logout_initiate_revocation_only( # Verify RP-Initiated Logout was NOT attempted mock_auth_manager.build_logout_url.assert_not_called() - # Verify token was revoked - mock_auth_manager.revoke_token.assert_called_once_with("refresh-token") + # 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( @@ -910,7 +916,7 @@ def test_oidc_logout_initiate_no_stored_id_token( json.dumps( { "id_token_claims": {"sub": "user123@example.com"}, - "access_token": "access-token", + "access_token": "test-access-token", # No id_token field } ), @@ -1005,7 +1011,7 @@ def test_oidc_logout_initiate_uses_correct_library( json.dumps( { "id_token_claims": {"sub": patron.authorization_identifier}, - "access_token": "access-token", + "access_token": "test-access-token", "refresh_token": "refresh-token", "id_token": f"raw.id.token.{i+1}", } @@ -1323,7 +1329,7 @@ def test_oidc_logout_initiate_missing_patron_claim_revokes_token( library = db.default_library() token_data = { "id_token_claims": {"iss": "issuer"}, # 'sub' / patron_id_claim absent - "access_token": "access-token", + "access_token": "test-access-token", "refresh_token": "refresh-token", } @@ -1360,7 +1366,10 @@ def test_oidc_logout_initiate_missing_patron_claim_revokes_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_called_once_with("refresh-token") + 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", @@ -1407,7 +1416,7 @@ def test_oidc_logout_initiate_exceptions( token_data: dict = { "id_token_claims": {"sub": "user@test.com"}, - "access_token": "access-token", + "access_token": "test-access-token", } if build_url_error: # RP-Initiated Logout path — needs id_token and the method to return True @@ -1511,7 +1520,7 @@ def test_oidc_logout_initiate_credential_invalidation_is_nonfatal( json.dumps( { "id_token_claims": {"sub": "user@test.com"}, - "access_token": "access-token", + "access_token": "test-access-token", "refresh_token": "refresh-token", } ), @@ -1550,7 +1559,10 @@ def test_oidc_logout_initiate_credential_invalidation_is_nonfatal( assert result.status_code == 302 assert "logout_status=success" in result.location - mock_auth_manager.revoke_token.assert_called_once_with("refresh-token") + 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()