Skip to content

SSO/logistration: lazy‑load third_party_auth_context to break import cycle#225

Open
subhashree-sahu31 wants to merge 2 commits intorelease-ulmofrom
AUT-112-organization-names-not-being-recognized-in-sso-login-screen
Open

SSO/logistration: lazy‑load third_party_auth_context to break import cycle#225
subhashree-sahu31 wants to merge 2 commits intorelease-ulmofrom
AUT-112-organization-names-not-being-recognized-in-sso-login-screen

Conversation

@subhashree-sahu31
Copy link
Copy Markdown

@subhashree-sahu31 subhashree-sahu31 commented Apr 9, 2026

PR Description
This PR fixes a circular import issue affecting enterprise SSO/authn context generation.

Problem

  • openedx/features/enterprise_support/utils.py imported third_party_auth_context at module load time from openedx/core/djangoapps/user_authn/views/utils.py.
  • That authn utils module also depends on enterprise utilities, creating a circular import path in certain load orders.

Root Cause
A top-level cross-module import between enterprise_support.utils and user_authn.views.utils caused Python to initialize partially loaded modules, which can break org-name/SSO context resolution flows.

Fix

  • Removed the top-level import of third_party_auth_context from enterprise_support.utils.
  • Added a function-scoped import inside get_mfe_context():
  • from openedx.core.djangoapps.user_authn.views.utils import third_party_auth_context
  • This preserves existing behavior while deferring the import until runtime, avoiding circular module initialization.

Why this is safe

  • No functional logic changes to auth flow, provider selection, or enterprise branding payload.
  • Same helper is used with the same arguments; only import timing changed.
  • Pattern is consistent with existing lazy imports in enterprise/authn code for circular dependency prevention.

Impact

  • Prevents circular import failures in enterprise-authn context assembly.
  • Reduces risk of intermittent SSO login context issues related to module import order.
  • No API contract changes.

Testing / Validation

  • Verified get_mfe_context() still builds:
  • third_party_auth_context
  • countryCode
  • enterpriseBranding (when enterprise customer is present)

Suggested regression checks:

  • Enterprise login page load with org flow
  • TPA hinted login (tpa_hint) flow
  • Non-enterprise login flow
  • Authn MFE context endpoint response shape unchanged

JIRA
https://2u-internal.atlassian.net/browse/AUT-112

Copilot AI review requested due to automatic review settings April 9, 2026 08:36
Copy link
Copy Markdown

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 addresses a circular import affecting enterprise SSO/authn context generation by deferring an import until get_mfe_context() runtime, reducing the chance of partially initialized modules during certain import orders.

Changes:

  • Removed the module-level import of third_party_auth_context from enterprise_support.utils.
  • Added a function-scoped import of third_party_auth_context inside get_mfe_context() to break the circular dependency at import time.

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

@subhashree-sahu31 subhashree-sahu31 force-pushed the AUT-112-organization-names-not-being-recognized-in-sso-login-screen branch 2 times, most recently from 8d40000 to 9fb01d8 Compare April 9, 2026 08:51
@subhashree-sahu31 subhashree-sahu31 force-pushed the AUT-112-organization-names-not-being-recognized-in-sso-login-screen branch from 9fb01d8 to 4a693f4 Compare April 9, 2026 08:56
@ssurendrannair ssurendrannair requested a review from ferdis April 9, 2026 11:34
Copy link
Copy Markdown
Member

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but I really don't like where this landed.

return f"{base_url}/{enterprise_customer['slug']}"


def get_mfe_context(request, redirect_to, tpa_hint=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm really not a fan of #139 introducing a duplicate get_mfe_context definition. Now we just have two identical get_mfe_context functions defined in different modules, independently tested, and each lazily imports modules from functions in each other's module. Very messy.

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.

5 participants