Skip to content

fix: check for existing pipeline session in tpa login + cleanup#178

Merged
jono-booth merged 1 commit intorelease-ulmofrom
jb/saml-redirect-loop
Mar 12, 2026
Merged

fix: check for existing pipeline session in tpa login + cleanup#178
jono-booth merged 1 commit intorelease-ulmofrom
jb/saml-redirect-loop

Conversation

@jono-booth
Copy link

Fix SAML redirect loop for new users and hinted login with skip dialog

Problem

New SAML users authenticated via a hinted IdP (tpa_hint) with skip_hinted_login_dialog=True could get stuck in an infinite redirect loop

  1. User hits /login?next=...?tpa_hint= → auto-redirected to the IdP (correct)
  2. User authenticates at the IdP → SAMLResponse posted to /auth/complete/tpa-saml/
  3. Pipeline determines the user is new → dispatches to /register?next=...?tpa_hint=
  4. The register view sees tpa_hint in the next URL → overrides initial_mode to "hinted_login"
  5. User is shown the "Sign in using X" dialog again instead of the registration form →
    A secondary failure mode existed in deployments with DB read replicas: pipeline.get() performs a database lookup for the partial pipeline object. If the partial hasn't replicated to the read replica yet, pipeline.get() returns None even though the pipeline IS running (the token
    exists in the session). This caused the skip-dialog guard to incorrectly trigger a fresh redirect to the IdP

login_form.py — two changes

  1. Session token fallback: When pipeline.get() returns None, check for the partial pipeline token directly in the session (partial_pipeline_token / partial_pipeline_token_). This avoids the DB read-replica race condition.
  2. Don't override initial_mode when a pipeline is in progress: initial_mode = "hinted_login" is now only set when no pipeline is running. If a pipeline is already active (user was dispatched back here after IdP auth), the original mode (register or login) is preserved so the user
    sees the appropriate form instead of being re-offered the IdP button

pipeline.py / provider.py / toggles.py — cleanup

  • The saml_provider_site_fallback waffle flag is promoted to unconditional behaviour: get_complete_url() and Registry.get_from_pipeline() always apply the site-independent SAML fallback lookup. The flag and its toggle function are removed.
  • Verbose debug logging added during investigation is removed from ensure_user_information.
  • is_provider_saml() replaced with a direct current_partial.backend == 'tpa-saml' check, which works correctly even when the provider isn't visible to the site-filtered registry

devstack.py — SAML SP-initiated flows require the browser to send the session cookie on the cross-site POST from the IdP back to /auth/complete/tpa-saml/. SameSite=Lax blocks this. The devstack settings now disable SameSite on session cookies to allow local end-to-end SAML testing

Tests

  • New test test_hinted_login_dialog_disabled_with_session_token_only covers the read-replica lag scenario: pipeline.get() returns None but the session contains a partial token → no redirect to the IdP.
  • Existing test_complete_url_does_not_raise_for_tpa_saml_with_no_enabled_providers added for the now-unconditional SAML site fallback in get_complete_url().
  • EnsureUserInformationTestCase updated to reflect the removal of the registry mock (SAML detection is now via backend name, not registry lookup).

Housekeeping

The third_party_auth.saml_provider_site_fallback behaviour has been tested and confirmed working, so this removes the waffle flag and makes the fallback unconditional. Also removes the verbose debug logging that was added during investigation, keeping only the two dispatch path log lines in ensure_user_information.

Copilot AI review requested due to automatic review settings March 12, 2026 07:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates third-party authentication (especially SAML) flow handling to prevent redirect loops and to behave correctly when pipeline state is partially available (e.g., session token present but DB partial object not yet readable), and adjusts local devstack cookie settings intended to support SAML POST-backs.

Changes:

  • Add hinted-login tests to ensure no redirect occurs when a TPA pipeline is running or when only the partial pipeline token exists in session.
  • In login/register form rendering, treat a session-held partial pipeline token as “pipeline running” to avoid redirect loops when pipeline.get() returns None.
  • Remove the SAML provider site-fallback waffle toggle and make the tpa-saml fallback behavior unconditional; update pipeline behavior/tests accordingly.
  • Update devstack SameSite settings aiming to allow SAML SP-initiated flows locally.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
openedx/core/djangoapps/user_authn/views/tests/test_logistration.py Adds coverage for hinted-login redirect guards when pipeline state is only partially observable.
openedx/core/djangoapps/user_authn/views/login_form.py Adds session-token-based pipeline detection to avoid redirect loops / false “no pipeline” conditions.
lms/envs/devstack.py Changes SameSite settings for local devstack to try to support SAML cross-site POST behavior.
common/djangoapps/third_party_auth/toggles.py Removes the SAML provider site-fallback waffle flag and helper.
common/djangoapps/third_party_auth/tests/test_pipeline_integration.py Adds/updates tests for unconditional tpa-saml complete URL behavior and backend-based SAML detection.
common/djangoapps/third_party_auth/provider.py Makes tpa-saml provider lookup fallback unconditional (no waffle flag gate).
common/djangoapps/third_party_auth/pipeline.py Removes toggle usage, special-cases tpa-saml complete URL behavior, and simplifies SAML detection in ensure_user_information.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from da71cab to 40de82b Compare March 12, 2026 07:46
Copilot AI review requested due to automatic review settings March 12, 2026 08:00
@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from 40de82b to fcd06b5 Compare March 12, 2026 08:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from fcd06b5 to af44d38 Compare March 12, 2026 08:15
Copilot AI review requested due to automatic review settings March 12, 2026 08:27
@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from af44d38 to bf05fd4 Compare March 12, 2026 08:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from bf05fd4 to dc30cbb Compare March 12, 2026 08:37
Copy link

@subhashree-sahu31 subhashree-sahu31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM🚀

@jono-booth jono-booth merged commit 5a3ec63 into release-ulmo Mar 12, 2026
100 of 103 checks passed
@jono-booth jono-booth deleted the jb/saml-redirect-loop branch March 12, 2026 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants