Skip to content

fix: skip hinted login if pipeline already running#174

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

fix: skip hinted login if pipeline already running#174
jono-booth merged 1 commit intorelease-ulmofrom
jb/saml-redirect-loop

Conversation

@jono-booth
Copy link

login_and_registration_form checks tpa_hint before checking saml_provider:

# Lines 167-183: tpa_hint + skip_hinted_login_dialog → RETURN to Auth0 (no pipeline check!)
if '?' in redirect_to:
  if tpa_hint_provider.skip_hinted_login_dialog:
      return redirect(pipeline.get_login_url(...))  #  loops back to Auth0

# Lines 196-208: saml_provider check (never reached if above fires)
running_pipeline = pipeline.get(request)
if running_pipeline:
  saml_provider, __ = is_saml_provider(...)
if should_redirect_to_authn_microfrontend() and not saml_provider:
  return redirect(AUTHN_MFE_URL)

Once redirect_to has tpa_hint from THIRD_PARTY_AUTH_HINT, the function redirects to Auth0 without ever checking if a SAML pipeline is already running. The pipeline check at line 197 is never reached.

Two changes, working together:

  1. running_pipeline is computed first (lines 163-169) — before any tpa_hint check. Previously it was computed at line 197, unreachable if the skip_hinted_login_dialog redirect fired.
  2. and not running_pipeline added to the redirect condition (line 182) — if a TPA pipeline is already in flight, the skip_hinted_login_dialog redirect is suppressed. The user falls through to initial_mode = "hinted_login" and sees the login/register form with the provider pre-selected, which is the correct behavior for a new SAML user who needs to complete registration.

Copilot AI review requested due to automatic review settings March 10, 2026 11:57
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

This PR prevents infinite redirect loops during third-party auth hinted login flows by suppressing the “skip hinted login dialog” redirect when a third-party auth pipeline is already running (e.g., users bounced back to /login or /register mid-pipeline).

Changes:

  • Compute running_pipeline (and derived saml_provider) earlier in login_and_registration_form so it can guard the hinted-login redirect path.
  • Add and not running_pipeline to the skip_hinted_login_dialog redirect condition to avoid redirect loops.
  • Add a regression test ensuring the form renders (200) instead of redirecting when a pipeline is running.

Reviewed changes

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

File Description
openedx/core/djangoapps/user_authn/views/login_form.py Moves running-pipeline detection earlier and uses it to suppress skip-dialog redirects.
openedx/core/djangoapps/user_authn/views/tests/test_logistration.py Adds coverage for skip-dialog behavior when a pipeline is already running.

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

Comment on lines +168 to +170
saml_provider, __ = third_party_auth.utils.is_saml_provider(
running_pipeline.get('backend'), running_pipeline.get('kwargs')
)
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

The new early SAML detection relies solely on third_party_auth.utils.is_saml_provider(backend, kwargs), but that helper returns None (not False) when the running pipeline lacks kwargs['response']['idp_name'] (common for tpa-saml partials). This is a regression from the removed backend_name == 'tpa-saml' special-case and can incorrectly treat a running SAML pipeline as non-SAML, allowing redirects (e.g., to the authn MFE) while a SAML pipeline is in flight. Consider restoring the tpa-saml backend-name fast path (treat backend == 'tpa-saml' as SAML even without idp_name) and/or explicitly coercing the first tuple element from is_saml_provider to a boolean so saml_provider is always True/False.

Suggested change
saml_provider, __ = third_party_auth.utils.is_saml_provider(
running_pipeline.get('backend'), running_pipeline.get('kwargs')
)
backend = running_pipeline.get('backend')
pipeline_kwargs = running_pipeline.get('kwargs') or {}
is_saml, __ = third_party_auth.utils.is_saml_provider(backend, pipeline_kwargs)
# Coerce to a strict boolean so saml_provider is never None.
saml_provider = bool(is_saml)
# Restore the legacy fast path: treat the tpa-saml backend as SAML even when
# idp_name is not yet present in the pipeline kwargs (e.g., for partials).
if not saml_provider and backend == 'tpa-saml':
saml_provider = True

Copilot uses AI. Check for mistakes.
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.

looks good

@jono-booth jono-booth force-pushed the jb/saml-redirect-loop branch from b323f6c to a26b5a9 Compare March 10, 2026 13:50
@jono-booth jono-booth enabled auto-merge March 10, 2026 14:01
@jono-booth jono-booth merged commit 924fa52 into release-ulmo Mar 10, 2026
63 checks passed
@jono-booth jono-booth deleted the jb/saml-redirect-loop branch March 10, 2026 14:12
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