Restructure OIDC patron auth for reuse by future OIDC-based providers (PP-3110)

[tdilauro]

Description

• Restructures the OIDC patron auth integration so that any future OIDC-based providers can reuse the machinery. Most of this change is just moving code around.
• Fixes a latent bug in back-channel logout that resulted in failure to logout.
• Fixes an issue in front-channel logout that would have failed to find the patron if a regex were present.
• Otherwise, no behavior changes for existing OIDC deployments. Live sessions and stored credentials will survive these changes.

Motivation and Context

We are planning an patron authentication integration for Clever, which is a standard OIDC provider with some extended functionality. Given the large overlap, it would be ideal to avoid duplicating the common parts.

[Jira PP-3110]

How Has This Been Tested?

• Tests added and updated to accommodate the refactoring.
• All tests pass locally.
• CI tests / checks pass.

Checklist

• N/A - I have updated the documentation accordingly.
• All new and existing tests passed.

[claude]

Claude finished @tdilauro's task in 8m 45s —— View job

---

Code Review

• Gather PR context and diff
• Review changed source files (authenticator.py, auth.py, configuration/model.py, controller.py, credential.py, provider.py, util.py, validator.py)
• Verify removed code (store_pkce/retrieve_pkce, extract_patron_id/validate_and_extract) has no remaining callers at the base commit
• Verify the new abstract contract has no other implementers, and that from __future__ import annotations covers the TYPE_CHECKING-only return annotations
• Trace back-channel logout savepoint semantics against the request teardown (routes.py shutdown_session)
• Review tests
• Post review

---

Summary

This is a clean refactor and the mechanical moves check out: the settings aliases preserve declaration order (now guarded by test_configuration_form_field_order), evaluate_patron_filters / build_oidc_authentication_document are faithful extractions, OIDCConnectionSettings covers exactly the attributes auth.py reads, and the removed store_pkce/retrieve_pkce/extract_patron_id/validate_and_extract had no callers outside their own tests at the base commit. The back-channel logout rework is the substantive part and it holds up — I traced the savepoint design against shutdown_session in routes.py:36-42, which commits on any non-exception return, so the 400 path does keep the completed invalidations from other libraries, as the new test asserts. Replacing the _settings/_credential_manager reach-throughs with a declared contract also means the Mock(spec=BaseOIDCAuthenticationProvider) test helper now genuinely enforces it. Two minor points below; nothing blocking.

Details

Minor: src/palace/manager/integration/patron_auth/oidc/provider.py:491

remote_patron_lookup_from_oidc_claims rejects on is None, but both logout callers (controller.py:460 and controller.py:642) reject on falsiness. extract_patron_identifier can return "" — a patron_id_regular_expression whose named group can match empty (e.g. (?P<patron_id>[^@]*)@example\.edu against @example.edu) reaches line 469 with an empty string. Login would then create the patron with an empty authorization_identifier while every logout path refuses the same claims, so that patron's credentials could never be invalidated locally. Since extract_patron_identifier is now the shared contract, matching the callers keeps the three sites consistent:

if not patron_id:
    raise ProblemDetailException(problem_detail=OIDC_CANNOT_DETERMINE_PATRON)

circulation/src/palace/manager/integration/patron_auth/oidc/provider.py

Line 491 in c3d0074

if patron_id is None:

Minor: src/palace/manager/integration/patron_auth/oidc/controller.py:628-639

Caching only successes means an integration that deterministically rejects the token gets a full validate_logout_token — provider metadata lookup, JWKS read, JWT signature verification — repeated once per library it serves. This only bites deployments with more than one OIDC integration and many libraries, but there it is pure waste on every logout notification, and unlike the old code the sweep no longer stops once a provider has succeeded. The transient-vs-permanent distinction the comment is protecting is already available from the exception type: OIDCDiscoveryError and network failures are the retryable ones, while audience/issuer/signature mismatches surface as OIDCAuthenticationError / OIDCTokenSignatureError and will never succeed for a sibling. Caching a rejection marker for the latter keeps the transient-failure guarantee intact while collapsing the repeats.

circulation/src/palace/manager/integration/patron_auth/oidc/controller.py

Lines 628 to 639 in c3d0074

	claims = claims_by_integration.get(provider.integration_id)
	if claims is None:
	try:
	auth_manager = provider.get_authentication_manager()
	claims = auth_manager.validate_logout_token(logout_token)
	except Exception as e:
	# This provider couldn't validate the token; try the next one.
	self.log.debug(
	f"Provider {provider.label()} could not validate logout token: {e}"
	)
	continue
	claims_by_integration[provider.integration_id] = claims

--- | Branch: chore/oidc-refactor

[greptile-apps]

Greptile Summary

This PR extracts reusable OIDC authentication contracts and utilities while correcting logout behavior.

• Introduces shared provider interfaces, connection settings, authentication-document builders, filter evaluation, and patron-identifier extraction.
• Updates front-channel and back-channel logout to use the shared provider contract and process library-scoped provider instances.
• Scopes credential invalidation by both token type and data source, resolving the previously reported cross-provider invalidation issue.
• Expands and updates OIDC tests for the refactored components and logout behavior.

Confidence Score: 5/5

The PR appears safe to merge, and the previously reported credential-invalidation namespace issue is fully addressed.

No blocking failure remains within the scope of the previous review thread.

Important Files Changed

Filename	Overview
src/palace/manager/integration/patron_auth/oidc/credential.py	Credential namespaces are now consistently configurable, and invalidation correctly filters by both data source and token type.
src/palace/manager/integration/patron_auth/oidc/controller.py	OIDC controller paths now use the common provider interface and handle back-channel logout across library-scoped provider instances.
src/palace/manager/integration/patron_auth/oidc/provider.py	Provider-specific behavior is decomposed into reusable OIDC helpers without an accepted blocking regression.
src/palace/manager/api/authenticator.py	The base OIDC provider now explicitly defines the contract required by shared controller dispatch.

_{Reviews (13): Last reviewed commit: "Cache only successful logout token valid..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (53e3a1d) to head (c3d0074).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3593   +/-   ##
=======================================
  Coverage   93.52%   93.52%           
=======================================
  Files         512      512           
  Lines       46760    46765    +5     
  Branches     6379     6376    -3     
=======================================
+ Hits        43731    43738    +7     
+ Misses       1958     1957    -1     
+ Partials     1071     1070    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[greptile-apps]

Want your agent to iterate on Greptile's feedback? Try greploops.