Support alternate SAML auth flow with ForceAuthn query param (PP-4792)

[tdilauro]

Description

• Adds support for an optional force_authn query parameter ("true" or "false") on the SAML authentication redirect endpoint.
• When force_authn=true is requested, the generated SAML AuthnRequest sets ForceAuthn="true", instructing the identity provider to re-authenticate the patron even if the patron already has an active IdP session (SAML 2.0 Core 3.4.1).
• Invalid values are rejected before any IdP round-trip, in which case the patron is redirected back to the client with a problem detail error.

Motivation and Context

A patron with an existing IdP session for the wrong account can get stuck at sign-in. For example, when a patron is signed into a Microsoft account from a different tenant, Microsoft may silently reuse that session and fail with an unrecoverable AADSTS50020 error instead of prompting for credentials. This gives clients (e.g., Palace Web Catalog) an escape hatch: an alternate sign-in path can be used to force the IdP to present a fresh login prompt. ForceAuthn is part of the SAML 2.0 Core specification, so it is supported across the identity providers we integrate with, unlike vendor-specific alternatives.

[Jira PP-4792]

How Has This Been Tested?

• New and updated tests.
• All tests and checks pass locally.
• CI tests and checks pass.

Checklist

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

[claude]

Claude finished @tdilauro's task in 4m 17s —— View job

---

Code Review

I reviewed the diff against origin/main. This is a clean, well-scoped change: the force_authn query parameter is validated before any IdP round-trip, invalid values are rejected with a static (non-reflected) error message, and the default path is unchanged — force_authn=False omits the ForceAuthn attribute entirely, which the new test_start_authentication parametrization confirms. force_authn is a documented keyword of OneLogin_Saml2_Auth.login in python3-saml ≥ 1.16, so auth.login(..., force_authn=...) is correct, and widening the params annotations from ImmutableMultiDict to MultiDict is safe since request.args is still an ImmutableMultiDict (a subclass). Type hints and test coverage are thorough. I found no functional bugs. One small note below, plus a process note: per CLAUDE.md, PRs on a feature/ branch should carry the feature label, which this PR currently lacks.

Details

Nit: src/palace/manager/integration/patron_auth/saml/auth.py:22

This line imports ProblemDetail twice under two names (ProblemDetail and pd). After the change the module mixes the aliases — the new start_authentication and start_logout annotations use ProblemDetail, while the five module-level constants and finish_logout's bool | pd return type still use pd. Consider consolidating on a single name (e.g. use ProblemDetail everywhere and drop the redundant pd alias) so the module reads consistently.

circulation/src/palace/manager/integration/patron_auth/saml/auth.py

Line 22 in 8a6b623

from palace.manager.util.problem_detail import ProblemDetail, ProblemDetail as pd

[greptile-apps]

Greptile Summary

This PR adds support for an optional force_authn query parameter on the SAML authentication redirect endpoint, allowing clients to instruct the IdP to re-authenticate a patron even when an existing IdP session is present (per SAML 2.0 Core § 3.4.1). Invalid values are rejected before any IdP round-trip and the patron is redirected back to the client with a structured error.

• SAMLController.saml_authentication_redirect now validates force_authn against {"true", "false"} and converts the string to a boolean before forwarding it to SAMLAuthenticationManager.start_authentication, which passes it to OneLogin_Saml2_Auth.login(force_authn=...).
• Type hint for both saml_authentication_redirect and saml_logout_redirect is widened from ImmutableMultiDict to MultiDict, which is the correct supertype and is compatible with request.args.
• New parametrized tests cover None, "true", "false", "TRUE", "1", and "" inputs, and the XML-level ForceAuthn attribute is verified directly in the decoded SAMLRequest.

Confidence Score: 5/5

Safe to merge; the new code path is additive, correctly guarded, and fully exercised by tests.

The feature is purely opt-in (defaults to False), so existing flows are unaffected. Validation of the query parameter happens before any IdP round-trip and uses an explicit allowlist. The force_authn flag is forwarded to the python-saml library's login() method, which has supported this parameter for years. Test coverage is thorough — valid, absent, and several invalid values are all exercised, and the SAML XML attribute is verified at the byte level. The only observation is a leftover pd alias in finish_logout's return annotation that was not updated alongside start_logout in the same file.

No files require special attention.

Important Files Changed

Filename	Overview
src/palace/manager/integration/patron_auth/saml/controller.py	Adds FORCE_AUTHN constant and VALID_FORCE_AUTHN_VALUES set; validates the optional force_authn query param and passes it to start_authentication; also widens ImmutableMultiDict → MultiDict on both saml_authentication_redirect and saml_logout_redirect.
src/palace/manager/integration/patron_auth/saml/auth.py	Adds force_authn: bool = False to start_authentication and forwards it to OneLogin_Saml2_Auth.login(); adds proper return-type annotations; also updates start_logout return type from str
tests/manager/integration/patron_auth/saml/test_auth.py	Parametrizes test_start_authentication for force_authn=False/True; asserts ForceAuthn attribute in the decoded SAMLRequest XML; adds isinstance guard and type annotations.
tests/manager/integration/patron_auth/saml/test_controller.py	Adds new test_saml_authentication_redirect_force_authn covering valid (None/"true"/"false") and invalid ("TRUE"/"1"/"") values; verifies the error redirect path and that start_authentication is called with the correct force_authn kwarg.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Client
    participant SAMLController
    participant SAMLAuthManager
    participant IdP

    Client->>SAMLController: "GET /saml_authenticate?provider=...&idp_entity_id=...&redirect_uri=...&force_authn=true"
    SAMLController->>SAMLController: Validate required params (provider, idp_entity_id, redirect_uri)
    SAMLController->>SAMLController: "Validate optional force_authn ("true"|"false")"
    alt force_authn invalid (e.g. "TRUE", "1", "")
        SAMLController-->>Client: "302 redirect to redirect_uri?error=..."
    end
    SAMLController->>SAMLAuthManager: "start_authentication(db, idp_entity_id, relay_state, force_authn=True)"
    SAMLAuthManager->>IdP: "AuthnRequest with ForceAuthn="true""
    SAMLAuthManager-->>SAMLController: IdP SSO URL
    SAMLController-->>Client: 302 redirect to IdP SSO URL
    IdP-->>SAMLController: POST /saml_callback (SAMLResponse + RelayState)
    SAMLController->>SAMLAuthManager: finish_authentication(db, idp_entity_id)
    SAMLAuthManager-->>SAMLController: SAMLSubject
    SAMLController-->>Client: "302 redirect to redirect_uri?access_token=...&patron_info=..."

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Client
    participant SAMLController
    participant SAMLAuthManager
    participant IdP

    Client->>SAMLController: "GET /saml_authenticate?provider=...&idp_entity_id=...&redirect_uri=...&force_authn=true"
    SAMLController->>SAMLController: Validate required params (provider, idp_entity_id, redirect_uri)
    SAMLController->>SAMLController: "Validate optional force_authn ("true"|"false")"
    alt force_authn invalid (e.g. "TRUE", "1", "")
        SAMLController-->>Client: "302 redirect to redirect_uri?error=..."
    end
    SAMLController->>SAMLAuthManager: "start_authentication(db, idp_entity_id, relay_state, force_authn=True)"
    SAMLAuthManager->>IdP: "AuthnRequest with ForceAuthn="true""
    SAMLAuthManager-->>SAMLController: IdP SSO URL
    SAMLController-->>Client: 302 redirect to IdP SSO URL
    IdP-->>SAMLController: POST /saml_callback (SAMLResponse + RelayState)
    SAMLController->>SAMLAuthManager: finish_authentication(db, idp_entity_id)
    SAMLAuthManager-->>SAMLController: SAMLSubject
    SAMLController-->>Client: "302 redirect to redirect_uri?access_token=...&patron_info=..."

Loading

Comments Outside Diff (1)

1. src/palace/manager/integration/patron_auth/saml/auth.py, line 329 (link)

   This PR updated start_logout's return annotation from str | pd to str | ProblemDetail, but finish_logout still uses the legacy pd alias. A one-line fix would make the file internally consistent now that ProblemDetail is already imported directly.

_{Reviews (1): Last reviewed commit: "Support alternate SAML auth flow with `F..." | Re-trigger Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (c54cedb) to head (8a6b623).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3574   +/-   ##
=======================================
  Coverage   93.51%   93.52%           
=======================================
  Files         512      512           
  Lines       46754    46760    +6     
  Branches     6378     6379    +1     
=======================================
+ Hits        43722    43731    +9     
+ Misses       1960     1958    -2     
+ Partials     1072     1071    -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.

[tdilauro]

Both Claude and greptile don't like my extra problem detail alias (great that they agree on something 🙄), but I didn't like pd for the type annotation and I didn't want to change the other uses and pollute the PR.

[jonathangreen]

I support this revolution against the robots! 🤖