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.

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

%%{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=..."

%%{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=..."

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

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.

@@           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.

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.