Add logging and setting overrides for active SAML ACS URL(s) (PP-4899)

[tdilauro]

Description

SAML Service Provider metadata may declare more than one callback (Assertion Consumer Service)
endpoint. Until now, the endpoint named in our authentication requests was fixed, so moving to a different one was not possible without a code or per-integration SP metadata change.

• Each SAML integration can now choose how that endpoint is selected:

  • Lowest index (default) — reproduces current behavior
  • Metadata default — honors the isDefault attribute in the metadata
  • Defer to the identity provider — name no endpoint, letting the identity provider use whichever one it has registered (not all IdPs may support this).
  • A new environment variable may override the site-wide default / fallback for integrations that do not choose one, and clearing the setting in the Admin UI returns an integration to the active site-wide default.

• Adds our own logging that identifies when a response arrives at an endpoint other than one we expected, which previously could only be diagnosed from an (external) IdPs error message.

• Improves endpoint selection, which previously ignored the metadata's own default attribute and compared endpoint indexes as text rather than as numbers.

Existing integrations are unaffected, as the default reproduces current behavior.

Motivation and Context

Some patron auth integrations need to move to a different SAML callback endpoint, but that change has to be coordinated with each identity provider partner individually. Making the choice a per-integration setting allows endpoints to be migrated incrementally, rather than requiring every partner to change at the same time.

[Jira PP-4899]

How Has This Been Tested?

• Manual testing of admin UI interaction in local dev environment.
• New and updated tests for the new functionality.
• All tests / checks pass locally.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

[greptile-apps]

Greptile Summary

The PR adds configurable SAML Assertion Consumer Service endpoint selection and improves diagnostics for ACS mismatches.

• Supports lowest-index, metadata-default, and identity-provider-selected ACS policies.
• Allows an environment-level default with per-integration overrides.
• Adds request and response endpoint logging.
• Corrects numeric ACS index and XML isDefault parsing.
• Documents the new environment variable and adds coverage for configuration, parsing, and authentication behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/palace/manager/integration/patron_auth/saml/auth.py	Adds ACS omission for deferred selection and endpoint-aware authentication diagnostics; no eligible follow-up issue remains.
src/palace/manager/integration/patron_auth/saml/configuration/model.py	Adds integration-level policy configuration and resolves it against the environment and built-in defaults.
src/palace/manager/integration/patron_auth/saml/configuration/service_provider.py	Adds validated environment configuration for the site-wide ACS selection policy.
src/palace/manager/integration/patron_auth/saml/metadata/parser.py	Implements policy-driven ACS selection, numeric index handling, XML boolean parsing, and completes the previously requested constructor annotations.
src/palace/manager/integration/patron_auth/saml/metadata/model.py	Defines the externally persisted ACS selection policies and compatibility-preserving default.

Sequence Diagram

sequenceDiagram
  participant Patron
  participant Palace
  participant Metadata as SP Metadata
  participant IdP
  Palace->>Metadata: Resolve effective ACS selection policy
  Metadata-->>Palace: Selected ACS endpoint
  Patron->>Palace: Start SAML authentication
  Palace->>IdP: AuthnRequest with selected ACS or no ACS
  IdP-->>Palace: SAML response at an ACS endpoint
  Palace->>Palace: Log endpoint choice or mismatch
  Palace->>Palace: Validate SAML response

Loading

_{Reviews (3): Last reviewed commit: "Add a clarifying comment" | Re-trigger Greptile}

[claude]

Claude finished @tdilauro's task in 11m 37s —— View job

---

Code Review

• Gather context (fetch base, read diff)
• Review SAML configuration / model changes
• Review metadata parser + auth changes
• Review tests and docs
• Post review

---

Summary

Solid, well-documented change: the policy resolution (integration setting → env → built-in default) has a single source of truth in get_acs_selection_policy(), the AcsOmittingAuthnRequest strip is done before get_request() so the redirect signature covers what is actually sent, and the mismatch/failure logging genuinely makes an IdP-side endpoint mismatch diagnosable from our own logs. Test coverage is thorough and the README is updated for the new env var per the repo convention. One substantive concern below, about whether FIRST_INDEX really is behavior-preserving. Separately, per CLAUDE.md this is a feature/ branch, so the PR should carry the feature label — it currently has none.

Details

src/palace/manager/integration/patron_auth/saml/metadata/parser.py:467-475

FIRST_INDEX is not equivalent to the pre-PR ACS selection whenever any AssertionConsumerService node carries an isDefault attribute, so the claim that "existing integrations are unaffected" holds only for SP metadata that has no isDefault anywhere. The old code path was _select_default_or_first_indexed_element, whose filter was [node for node in nodes if node.get("isDefault", False)] — since lxml returns the attribute as a string, any isDefault value (including "false") made a node "default" and won over a lower index. Concretely, with the metadata shape used in the new tests, today's code selects /saml/callback and this PR's default selects /saml_callback:

<md:AssertionConsumerService index="1" Location="https://example.org/saml_callback"/>
<md:AssertionConsumerService index="2" isDefault="true" Location="https://example.org/saml/callback"/>

If any deployed SP metadata already marks an endpoint isDefault, merging this changes the endpoint named in every AuthnRequest for those integrations, and IdPs registered against the previously-selected endpoint will reject the request — the exact coordinated change the setting is meant to avoid making unilaterally. Worth confirming against the SP metadata actually in use; if any of it does mark an isDefault endpoint, those integrations need metadata_default set (or the env default flipped) in the same release, and the "reproduces current behavior" wording in the README/PR description needs the isDefault caveat.

circulation/src/palace/manager/integration/patron_auth/saml/metadata/parser.py

Lines 467 to 475 in 27f4776

	# DEFER_TO_IDP resolves an endpoint just as METADATA_DEFAULT does, even though
	# requests under it name no endpoint, because the SAML toolkit requires an ACS
	# URL in its settings. Choosing the policy is the caller's business.
	if self._acs_selection_policy is SAMLACSSelectionPolicy.FIRST_INDEX:
	acs_service_node = self._select_first_indexed_element(acs_service_nodes)
	else:
	acs_service_node = self._select_default_or_first_indexed_element(
	acs_service_nodes
	)

| Branch: feature/saml-per-integration-acs

[codecov]

Codecov Report

❌ Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.54%. Comparing base (53e3a1d) to head (27f4776).
⚠️ Report is 51 commits behind head on main.

Files with missing lines	Patch %	Lines
...alace/manager/integration/patron_auth/saml/auth.py	95.34%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3631      +/-   ##
==========================================
+ Coverage   93.52%   93.54%   +0.02%     
==========================================
  Files         512      509       -3     
  Lines       46760    46854      +94     
  Branches     6379     6406      +27     
==========================================
+ Hits        43731    43829      +98     
+ Misses       1958     1955       -3     
+ 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.

[jonathangreen]

Looks good! I didn't go through the test cases in detail, but they looked good at a high level and the non-test code looks great.

[jonathangreen]

I like the type hint here! I usually annotate these as FrozenDict, but Final[Mapping[ is really nice.

[jonathangreen]

Nit: You could probably use StrEnum and auto() to avoid repeating these.

[tdilauro]

I was going to use strEnum, but noticed that just about everything else there was enums. I'll got ahead and do that.

[tdilauro]

Actually, I don't think I want to use auto() here, since the values might be used by the env var override. So someone changing the member name would break that. I'll add a comment about it, though, so it's more evident.

[tdilauro]

Claude's assertion in its latest round

FIRST_INDEX is not equivalent to the pre-PR ACS selection whenever any AssertionConsumerService node carries an isDefault attribute, so the claim that "existing integrations are unaffected" holds only for SP metadata that has no isDefault anywhere.

is simply wrong. That broken behavior was the main impetus for this PR. It is implemented as a settings feature only because simply fixing the bug would've broken any active integration that was affected by the bug, which would've been most all of them.