feat: Refactor redirection logic in login_and_registration_form view#120
Conversation
- Updated the redirection block to handle external providers (SAML/TPA) that should never redirect to the AuthN MFE. - Added logic to determine segment eligibility for redirection based on enterprise customer status and waffle flag. - Ensured proper handling of authenticated users without logged-in cookies to avoid unnecessary redirection loops. - Improved code readability and maintainability by restructuring the redirection logic. This change ensures a more robust and accurate redirection process for login and registration flows.
There was a problem hiding this comment.
Pull request overview
This PR refactors the redirection decision in login_and_registration_form to support an Enterprise (B2B) rollout of the AuthN microfrontend (MFE) behind a new waffle flag, while explicitly preventing redirects for external auth providers (SAML/TPA).
Changes:
- Adds an Enterprise-segment eligibility check for AuthN MFE redirects, gated by a new waffle flag.
- Refactors redirect conditionals for readability (introducing
has_external_providerandis_segment_eligible). - Introduces a new
WaffleFlagtoggle to control Enterprise redirect behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
openedx/core/djangoapps/user_authn/views/login_form.py |
Refactors AuthN MFE redirect logic to include Enterprise gating and external-provider exclusions. |
openedx/core/djangoapps/user_authn/toggles.py |
Adds a new waffle flag (ENABLE_ENTERPRISE_REDIRECT_TO_AUTHN) to gate Enterprise redirects to the AuthN MFE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Pin to a version below 82 to ensure is included. - Add a verification step to confirm the availability of . - Ensure compatibility with Python 3.12 in the dependency check process. - Maintain exclusions for specific repositories during the dependency check. This update ensures the workflow runs reliably and avoids issues with missing in the CI/CD pipeline.
- Added tests to verify the behavior of the ENABLE_ENTERPRISE_REDIRECT_TO_AUTHN flag: - Enterprise customer + flag disabled => no MFE redirect. - Enterprise customer + flag enabled => MFE redirect. - Enterprise customer + SAML provider => no MFE redirect. - Enterprise customer + TPA hint => no MFE redirect. - Ensured proper handling of enterprise customers and external providers in login and registration flows. - Improved test coverage for edge cases involving third-party authentication and enterprise configurations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added missing blank newlines at the end of the following files: - toggles.py - waffle.py - login_form.py - test_logistration.py This resolves the W292 linting errors and ensures compliance with Python style guidelines.
- Removed trailing whitespace on blank lines (W293). - Added a newline at the end of the file (W292). These changes ensure compliance with Python style guidelines.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
openedx/core/djangoapps/user_authn/views/login_form.py:223
- Inside the AuthN MFE redirect block, the nested
if request.user.is_authenticated and redirect_to:branch is unreachable because the view already returns early for any authenticated user near the top of the function. This dead branch should be removed or the earlier authenticated-user handling should be reworked so the intent (avoiding redirect loops when cookies are missing) is actually reachable/testable.
# This is to handle a case where a logged-in cookie is not present but the user is authenticated.
# Note: If we don't handle this learner is redirected to authn MFE and then back to dashboard
# instead of the desired redirect URL (e.g. finish_auth) resulting in learners not enrolling
# into the courses.
if request.user.is_authenticated and redirect_to:
return redirect(redirect_to)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the view to prevent redirection to the AuthN MFE for enterprise customers with SAML providers or TPA hints. - Ensured that the presence of external providers explicitly blocks the redirect. - Verified the fix with updated test cases. This resolves the test failures related to enterprise customer redirection logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
openedx/core/djangoapps/user_authn/views/login_form.py:247
- The inner check
if request.user.is_authenticated and redirect_to:is unreachable because the function returns earlier for any authenticated user (lines 158-161). This dead code makes the redirect-loop protection ineffective and adds confusion; remove it or adjust the earlier authenticated-user branch so the intended edge case is actually handled.
# This is to handle a case where a logged-in cookie is not present but the user is authenticated.
# Note: If we don't handle this learner is redirected to authn MFE and then back to dashboard
# instead of the desired redirect URL (e.g. finish_auth) resulting in learners not enrolling
# into the courses.
if request.user.is_authenticated and redirect_to:
return redirect(redirect_to)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def login_and_registration_form(request, initial_mode="login"): # noqa: R0915 | ||
| """Render the combined login/registration form, defaulting to login |
There was a problem hiding this comment.
# noqa: R0915 won’t suppress the existing pylint too-many-statements warning (this repo typically uses # pylint: disable=too-many-statements or # lint-amnesty, pylint: disable=...). As-is, linting may still fail. Replace with a pylint disable comment (or extract helper functions to reduce statements) instead of using flake8-style noqa for a pylint rule.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| account_activation_messages = [ | ||
| { | ||
| 'message': message.message, 'tags': message.tags | ||
| } for message in messages.get_messages(request) if 'account-activation' in message.tags | ||
| "message": message.message, | ||
| "tags": message.tags, | ||
| } | ||
| for message in messages.get_messages(request) | ||
| if "account-activation" in message.tags | ||
| ] | ||
|
|
||
| account_recovery_messages = [ | ||
| { | ||
| 'message': message.message, 'tags': message.tags | ||
| } for message in messages.get_messages(request) if 'account-recovery' in message.tags | ||
| "message": message.message, | ||
| "tags": message.tags, | ||
| } | ||
| for message in messages.get_messages(request) | ||
| if "account-recovery" in message.tags | ||
| ] |
There was a problem hiding this comment.
messages.get_messages(request) is iterated twice (once for activation and once for recovery). Django’s message storage is consumed on iteration, so the second list will typically be empty if any messages were read in the first comprehension. Consider iterating once (e.g., cast to list(messages.get_messages(request))) and then splitting/filtering into activation vs recovery messages.
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | ||
| if request.user.is_authenticated: | ||
| redirect_to_target = get_next_url_for_login_page(request) | ||
| if redirect_to_target: | ||
| return redirect(redirect_to_target) | ||
|
|
There was a problem hiding this comment.
The request.user.is_authenticated branch in _maybe_redirect_to_authn_mfe is effectively unreachable because login_and_registration_form returns early for authenticated users before calling this helper. This adds confusion and can be removed (or the helper’s contract adjusted if it’s intended to be reusable elsewhere).
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | |
| if request.user.is_authenticated: | |
| redirect_to_target = get_next_url_for_login_page(request) | |
| if redirect_to_target: | |
| return redirect(redirect_to_target) |
| enterprise_customer = enterprise_customer_for_request(request) | ||
| if enterprise_customer: | ||
| # Enterprise / B2B: gated by the Enterprise waffle flag | ||
| is_segment_eligible = ENABLE_ENTERPRISE_REDIRECT_TO_AUTHN.is_enabled() | ||
| else: | ||
| # B2C: eligible by default when global AuthN MFE is on | ||
| is_segment_eligible = True |
There was a problem hiding this comment.
enterprise_customer_for_request(request) is called in _maybe_redirect_to_authn_mfe and then again later in login_and_registration_form. Since enterprise_customer_for_request may hit the Enterprise API when the session cache is cold, consider fetching it once in the view and passing the value into helpers to avoid duplicate work and log noise.
| def _build_logistration_context( | ||
| request, | ||
| redirect_to, | ||
| initial_mode, | ||
| third_party_auth_hint, | ||
| form_descriptions, | ||
| account_activation_messages, | ||
| account_recovery_messages, | ||
| enterprise_customer, | ||
| ): |
There was a problem hiding this comment.
_build_logistration_context accepts an enterprise_customer parameter but doesn’t use it when building the context dict. Consider removing the parameter (or using it) to keep the helper signature minimal and avoid implying the value affects the returned context.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _handle_tpa_hint(request, redirect_to, initial_mode): | ||
| """ | ||
| Handle TPA hint logic and return: | ||
| (third_party_auth_hint, updated_initial_mode, optional_redirect_response). | ||
|
|
||
| This relies on the JS to asynchronously load the actual form from | ||
| the user_api. | ||
| - Preserves existing behavior for hinted login dialog & skip_hinted_login_dialog. | ||
| - Does NOT decide about MFE redirect; that is handled separately. | ||
| """ | ||
| third_party_auth_hint = None | ||
|
|
||
| # Early return if no query string in redirect_to | ||
| if "?" not in redirect_to: | ||
| return third_party_auth_hint, initial_mode, None | ||
|
|
||
| try: | ||
| next_args = urllib.parse.parse_qs( | ||
| urllib.parse.urlparse(redirect_to).query | ||
| ) | ||
|
|
||
| # Early return if no tpa_hint in query params | ||
| if "tpa_hint" not in next_args: | ||
| return third_party_auth_hint, initial_mode, None | ||
|
|
||
| provider_id = next_args["tpa_hint"][0] | ||
| tpa_hint_provider = third_party_auth.provider.Registry.get( | ||
| provider_id=provider_id | ||
| ) | ||
|
|
||
| # Early return if provider not found | ||
| if not tpa_hint_provider: | ||
| return third_party_auth_hint, initial_mode, None | ||
|
|
||
| # Handle skip_hinted_login_dialog | ||
| if tpa_hint_provider.skip_hinted_login_dialog: | ||
| auth_entry = ( | ||
| pipeline.AUTH_ENTRY_REGISTER | ||
| if initial_mode == "register" | ||
| else pipeline.AUTH_ENTRY_LOGIN | ||
| ) | ||
| redirect_response = redirect( | ||
| pipeline.get_login_url( | ||
| provider_id, | ||
| auth_entry, | ||
| redirect_url=redirect_to, | ||
| ) | ||
| ) | ||
| return None, initial_mode, redirect_response | ||
|
|
||
| # Set hint and mode for hinted login | ||
| third_party_auth_hint = provider_id | ||
| initial_mode = "hinted_login" | ||
|
|
||
| except (KeyError, ValueError, IndexError) as ex: | ||
| log.exception("Unknown tpa_hint provider: %s", ex) | ||
|
|
||
| return third_party_auth_hint, initial_mode, None |
There was a problem hiding this comment.
The _handle_tpa_hint function only checks for tpa_hint nested inside the redirect_to URL (e.g., in a ?next= parameter), but doesn't check for tpa_hint passed directly as a query parameter to the login/registration page (e.g., /login?tpa_hint=google-oauth2).
Meanwhile, _has_tpa_hint correctly checks both request.GET and redirect_to. This inconsistency means that direct tpa_hint parameters will block MFE redirect but won't trigger hinted login behavior or set the third_party_auth_hint in the context.
The test at line 608-625 passes tpa_hint as a direct query parameter and expects the legacy page to render, which will work because _has_tpa_hint blocks the MFE redirect. However, the hinted login UI may not display correctly because _handle_tpa_hint doesn't detect the hint.
Consider updating _handle_tpa_hint to also check request.GET for tpa_hint, similar to how _has_tpa_hint does.
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | ||
| if request.user.is_authenticated: | ||
| redirect_to_target = get_next_url_for_login_page(request) | ||
| if redirect_to_target: | ||
| return redirect(redirect_to_target) | ||
|
|
There was a problem hiding this comment.
The authenticated user handling in _maybe_redirect_to_authn_mfe creates an unnecessary redirect. At line 370-374 in the main login_and_registration_form function, authenticated users are already redirected early. This second check at lines 248-251 will never be reached for authenticated users because the early check happens before this function is even called. This code is unreachable and should be removed to avoid confusion.
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | |
| if request.user.is_authenticated: | |
| redirect_to_target = get_next_url_for_login_page(request) | |
| if redirect_to_target: | |
| return redirect(redirect_to_target) |
.github/workflows/unit-tests.yml
Outdated
|
|
||
| - name: Start MongoDB | ||
| uses: supercharge/mongodb-github-action@1.12.0 | ||
| uses: supercharge/mongodb-github-action@1.11.0 |
There was a problem hiding this comment.
The PR description mentions refactoring redirection logic in the login_and_registration_form view, but this change downgrades the MongoDB GitHub Action from version 1.12.0 to 1.11.0. This appears unrelated to the PR's stated purpose and should either be explained in the PR description or moved to a separate PR. If this is intentional, please clarify the reason for the downgrade in the PR description.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/unit-tests.yml:166
- The PR description focuses on refactoring redirection logic in the login_and_registration_form view, but this change to upgrade Docker in the unit-tests workflow appears unrelated to that purpose. This Docker upgrade should either be explained in the PR description or moved to a separate PR focused on infrastructure updates.
python-version: 3.11
- name: install system requirements
run: |
sudo apt-get update && sudo apt-get install libxmlsec1-dev
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Should render legacy page, not redirect | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertContains(response, 'initial_mode') | ||
|
|
There was a problem hiding this comment.
The new enterprise-specific tests cover the case where enterprise customers with SAML or TPA hint should not be redirected to MFE. However, there are no corresponding tests for B2C users (non-enterprise) with SAML or TPA hint when the MFE feature is enabled. The code at lines 229-230 and 240-244 should block MFE redirect for these cases regardless of enterprise status, but this behavior is not covered by tests. Consider adding tests similar to test_enterprise_customer_flag_enabled_saml_no_redirect and test_enterprise_customer_flag_enabled_tpa_hint_no_redirect, but without mocking the enterprise customer to ensure B2C users with external providers are also handled correctly.
| @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) | |
| @override_waffle_flag(ENABLE_ENTERPRISE_REDIRECT_TO_AUTHN, active=True) | |
| def test_b2c_saml_no_redirect_with_mfe_enabled(self): | |
| """ | |
| Test that B2C users with SAML provider are NOT redirected to MFE | |
| when the MFE feature flag and enterprise redirect flag are enabled. | |
| """ | |
| pipeline_target = "openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.pipeline" | |
| # Simulate SAML provider in pipeline for a non-enterprise request | |
| with simulate_running_pipeline(pipeline_target, 'tpa-saml', {'backend': 'tpa-saml', 'kwargs': {}}): | |
| response = self.client.get(reverse("signin_user")) | |
| # Should render legacy page, not redirect | |
| self.assertEqual(response.status_code, 200) | |
| self.assertContains(response, 'initial_mode') | |
| @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) | |
| @override_waffle_flag(ENABLE_ENTERPRISE_REDIRECT_TO_AUTHN, active=True) | |
| def test_b2c_tpa_hint_no_redirect_with_mfe_enabled(self): | |
| """ | |
| Test that B2C users with TPA hint are NOT redirected to MFE | |
| when the MFE feature flag and enterprise redirect flag are enabled. | |
| """ | |
| response = self.client.get( | |
| reverse("signin_user"), | |
| {'tpa_hint': 'oa2-google-oauth2'} | |
| ) | |
| # Should render legacy page, not redirect | |
| self.assertEqual(response.status_code, 200) | |
| self.assertContains(response, 'initial_mode') |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _get_account_messages(request): | ||
| """ | ||
| Return (account_activation_messages, account_recovery_messages) from Django messages. | ||
| """ | ||
| account_activation_messages = [ | ||
| { | ||
| 'message': message.message, 'tags': message.tags | ||
| } for message in messages.get_messages(request) if 'account-activation' in message.tags | ||
| "message": message.message, | ||
| "tags": message.tags, | ||
| } | ||
| for message in messages.get_messages(request) | ||
| if "account-activation" in message.tags | ||
| ] | ||
|
|
||
| account_recovery_messages = [ | ||
| { | ||
| 'message': message.message, 'tags': message.tags | ||
| } for message in messages.get_messages(request) if 'account-recovery' in message.tags | ||
| "message": message.message, | ||
| "tags": message.tags, | ||
| } | ||
| for message in messages.get_messages(request) | ||
| if "account-recovery" in message.tags | ||
| ] | ||
|
|
||
| # Otherwise, render the combined login/registration page | ||
| context = { | ||
| 'data': { | ||
| 'login_redirect_url': redirect_to, | ||
| 'initial_mode': initial_mode, | ||
| 'third_party_auth': third_party_auth_context(request, redirect_to, third_party_auth_hint), | ||
| 'third_party_auth_hint': third_party_auth_hint or '', | ||
| 'platform_name': configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), | ||
| 'support_link': configuration_helpers.get_value('SUPPORT_SITE_LINK', settings.SUPPORT_SITE_LINK), | ||
| 'password_reset_support_link': configuration_helpers.get_value( | ||
| 'PASSWORD_RESET_SUPPORT_LINK', settings.PASSWORD_RESET_SUPPORT_LINK | ||
| ) or settings.SUPPORT_SITE_LINK, | ||
| 'account_activation_messages': account_activation_messages, | ||
| 'account_recovery_messages': account_recovery_messages, | ||
| return account_activation_messages, account_recovery_messages |
There was a problem hiding this comment.
_get_account_messages calls messages.get_messages(request) twice; iterating the storage consumes messages, so the second comprehension will miss (and effectively drop) any non-activation messages. Consider iterating messages.get_messages(request) once and partitioning into activation vs recovery lists so both sets are preserved.
| "registration": RegistrationFormFactory() | ||
| .get_registration_form(request) | ||
| .to_json(), |
There was a problem hiding this comment.
The registration entry in _get_form_descriptions uses a chained-call line break starting with . at a different indentation level. This pattern often triggers pycodestyle E12x continuation-indentation errors under this repo’s setup.cfg settings; consider wrapping the expression in parentheses and indenting consistently (or keep it on one line) to satisfy pycodestyle.
| "registration": RegistrationFormFactory() | |
| .get_registration_form(request) | |
| .to_json(), | |
| "registration": RegistrationFormFactory().get_registration_form(request).to_json(), |
| # External providers (SAML / TPA hint) must NEVER redirect to MFE. | ||
| # Check for any running pipeline first (this catches all third-party auth) | ||
| running_pipeline = pipeline.get(request) | ||
|
|
||
| # 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 | ||
| saml_provider = False | ||
| if running_pipeline: | ||
| backend_name = running_pipeline.get("backend") | ||
| kwargs = running_pipeline.get("kwargs", {}) | ||
| # is_saml_provider returns a tuple (bool, provider_name) | ||
| saml_provider, __ = third_party_auth.utils.is_saml_provider( | ||
| running_pipeline.get('backend'), running_pipeline.get('kwargs') | ||
| backend=backend_name, | ||
| kwargs=kwargs, | ||
| ) | ||
|
|
||
| # Check for TPA hint in request or redirect URL | ||
| has_tpa_hint = _has_tpa_hint(request, redirect_to) | ||
|
|
||
| # Treat ANY of these as external provider (hard stop for MFE redirect) | ||
| has_external_provider = bool(has_running_pipeline or saml_provider or has_tpa_hint) |
There was a problem hiding this comment.
There are blank lines containing trailing whitespace in _maybe_redirect_to_authn_mfe (visible around the pipeline.get / has_tpa_hint sections). This can fail pycodestyle with W293; please remove the trailing spaces on otherwise-empty lines.
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | ||
| if request.user.is_authenticated: | ||
| redirect_to_target = get_next_url_for_login_page(request) | ||
| if redirect_to_target: | ||
| return redirect(redirect_to_target) | ||
|
|
There was a problem hiding this comment.
The if request.user.is_authenticated block inside _maybe_redirect_to_authn_mfe is effectively unreachable because login_and_registration_form returns early for authenticated users before calling _maybe_redirect_to_authn_mfe. Consider removing this block (or restructuring the call flow) to avoid dead/duplicated logic.
| # Handle authenticated user with a specific redirect target (finish_auth, etc.) | |
| if request.user.is_authenticated: | |
| redirect_to_target = get_next_url_for_login_page(request) | |
| if redirect_to_target: | |
| return redirect(redirect_to_target) |
| def _build_logistration_context( | ||
| request, | ||
| redirect_to, | ||
| initial_mode, | ||
| third_party_auth_hint, | ||
| form_descriptions, | ||
| account_activation_messages, | ||
| account_recovery_messages, | ||
| enterprise_customer, | ||
| ): |
There was a problem hiding this comment.
_build_logistration_context accepts enterprise_customer but does not use it anywhere when building the context. If it’s intentionally unused, consider removing the parameter to reduce confusion, or use it directly in the context-building step to justify passing it through.
subhashree-sahu31
deleted the
AUT-60-implement-waffle-gated-auth-n-mfe-redirection-for-enterprise-b-2-b-users
branch
2 participants
This change ensures a more robust and accurate redirection process for login and registration flows.