Revert "fix: fixed the redirection logic"#123
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request reverts PR #121, which had modified the redirection logic for the authentication microfrontend (MFE). The revert reinstates logic that blocks ALL third-party authentication flows (SAML, OAuth, and others) from redirecting to the MFE, rather than potentially blocking only SAML providers.
Changes:
- Added
has_running_pipelinecheck to detect any active third-party auth pipeline - Simplified SAML provider detection to always use
is_saml_provider()utility function - Updated
has_external_providercondition to include any running pipeline, not just SAML
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Also explicitly check for SAML if pipeline exists | ||
| saml_provider = False | ||
| if running_pipeline: | ||
| backend_name = running_pipeline.get("backend") | ||
| kwargs = running_pipeline.get("kwargs", {}) | ||
| # Check if backend is SAML (either explicitly 'tpa-saml' or via provider registry) | ||
| if backend_name == 'tpa-saml': | ||
| saml_provider = True | ||
| else: | ||
| # Also check via provider registry (for configured SAML providers) | ||
| saml_provider, __ = third_party_auth.utils.is_saml_provider( | ||
| backend=backend_name, | ||
| kwargs=kwargs, | ||
| ) | ||
| # is_saml_provider returns a tuple (bool, provider_name) | ||
| saml_provider, __ = third_party_auth.utils.is_saml_provider( | ||
| backend=backend_name, | ||
| kwargs=kwargs, | ||
| ) |
There was a problem hiding this comment.
The SAML provider check on lines 227-235 is redundant. Since has_running_pipeline is already included in the has_external_provider condition on line 239, and this check only executes when running_pipeline is not None (which is the same as has_running_pipeline being True), the result of saml_provider doesn't affect the final outcome. Whether the running pipeline is SAML or not, has_running_pipeline will be True and will block the MFE redirect regardless. Consider removing this redundant check to simplify the logic.
| # If there's ANY running pipeline, treat it as an external provider | ||
| # This handles SAML, OAuth, and any other third-party auth flows | ||
| has_running_pipeline = running_pipeline is not None | ||
| # Also explicitly check for SAML if pipeline exists |
There was a problem hiding this comment.
The comment "Also explicitly check for SAML if pipeline exists" is misleading. Since has_running_pipeline is included in the has_external_provider check on line 239, this SAML check is redundant. When a pipeline exists (any type), has_running_pipeline will be True and will block MFE redirect regardless of whether it's SAML or another provider type like OAuth. Either remove this redundant check or update the comment to explain why it's needed (if there's a valid reason not apparent from the code).
| @@ -220,24 +220,23 @@ def _maybe_redirect_to_authn_mfe(request, initial_mode, redirect_to): | |||
| # External providers (SAML / TPA hint) must NEVER redirect to MFE. | |||
There was a problem hiding this comment.
The comment on line 220 states "External providers (SAML / TPA hint)" but the code now blocks ALL third-party auth providers, not just SAML. The comments on lines 221 and 224 confirm this is intentional ("catches all third-party auth" and "SAML, OAuth, and any other third-party auth flows"). Consider updating line 220 to say "External providers (all third-party auth / TPA hint)" to accurately reflect the actual behavior.
| # External providers (SAML / TPA hint) must NEVER redirect to MFE. | |
| # External providers (all third-party auth / TPA hint) must NEVER redirect to MFE. |
subhashree-sahu31
deleted the
revert-121-AUT-60-implement-waffle-gated-auth-n-mfe-redirection-for-enterprise-b-2-b-users
branch
2 participants
Reverts #121