From 22433a907c941d1f81ceec587418627971dc08b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=CC=81s=20Gonza=CC=81lez?= Date: Fri, 22 Nov 2024 10:20:07 -0300 Subject: [PATCH 1/3] fix: Redirect non-enterprise SAML to authn MFE The original request was that enterprise users with tpa hint and SAML should not be redirected to MFE. The current condition also excludes regular non-enterprise users with SAML authentication from the MFE. --- .../core/djangoapps/user_authn/views/login_form.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/login_form.py b/openedx/core/djangoapps/user_authn/views/login_form.py index bb78a9df1a3c..3729c40868f6 100644 --- a/openedx/core/djangoapps/user_authn/views/login_form.py +++ b/openedx/core/djangoapps/user_authn/views/login_form.py @@ -187,12 +187,7 @@ def login_and_registration_form(request, initial_mode="login"): log.exception("Unknown tpa_hint provider: %s", ex) # Redirect to authn MFE if it is enabled - # AND - # user is not an enterprise user - # AND - # tpa_hint_provider is not available - # AND - # user is not coming from a SAML IDP. + # except if user is an enterprise user with tpa_hint_provider coming from a SAML IDP. saml_provider = False running_pipeline = pipeline.get(request) if running_pipeline: @@ -202,10 +197,8 @@ def login_and_registration_form(request, initial_mode="login"): enterprise_customer = enterprise_customer_for_request(request) - if should_redirect_to_authn_microfrontend() and \ - not enterprise_customer and \ - not tpa_hint_provider and \ - not saml_provider: + if should_redirect_to_authn_microfrontend() and not \ + (enterprise_customer and tpa_hint_provider and saml_provider): # 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 From affb44ae16c444de4add26f20230737a0e3a2320 Mon Sep 17 00:00:00 2001 From: Leonardo Beroes Date: Wed, 29 Jan 2025 15:49:37 -0400 Subject: [PATCH 2/3] test: Add test for enterprise SAML authentication MFE redirection logic This test validates the conditional redirection to the authentication microfrontend (MFE) for enterprise and SAML authentication scenarios. The test covers different combinations of: - Enterprise customer presence - Third-party authentication provider - SAML provider status - Redirection setting Ensures that enterprise customers with SAML providers are not redirected to the authentication MFE, while other scenarios follow the standard redirection rules. --- .../views/tests/test_logistration.py | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index 3220cd513974..5b387adaf8ca 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -648,6 +648,80 @@ def test_browser_language_dialent(self): assert response['Content-Language'] == 'es-es' + @ddt.data( + (None, None, None, True), + ({ + 'name': 'Test Enterprise', + 'uuid': 'test-uuid' + }, None, None, True), + ({ + 'name': 'Test Enterprise', + 'uuid': 'test-uuid' + }, 'test-provider', None, True), + ({ + 'name': 'Test Enterprise', + 'uuid': 'test-uuid' + }, 'test-provider', True, False), + ) + @ddt.unpack + @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + def test_enterprise_saml_redirection(self, enterprise_customer_data, provider_id, is_saml, should_redirect): + """ + Test that authentication MFE redirection respects the enterprise + SAML provider conditions. + In particular, verify that if we have an enterprise customer with a SAML-based tpa_hint_provider, + we do NOT redirect to the MFE, but handle the request in LMS. All other combinations should + redirect to the MFE when it's enabled. + """ + if provider_id and is_saml: + self.enable_saml() + self._configure_testshib_provider('TestShib', provider_id) + + with mock.patch( + 'openedx.core.djangoapps.user_authn.views.login_form.enterprise_customer_for_request') as mock_ec, \ + mock.patch( + 'openedx.core.djangoapps.user_authn.views.login_form.should_redirect_to_authn_microfrontend') as mock_should_redirect, \ + mock.patch( + 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.utils.is_saml_provider') as mock_is_saml: + + mock_ec.return_value = enterprise_customer_data + mock_should_redirect.return_value = should_redirect + mock_is_saml.return_value = (True, None) if is_saml else (False, None) + + params = {} + if provider_id: + params['tpa_hint'] = provider_id + + if provider_id and is_saml: + pipeline_target = 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.pipeline' + with mock.patch(pipeline_target + '.get') as mock_pipeline: + pipeline_data = { + 'backend': 'tpa-saml', + 'kwargs': { + 'response': { + 'idp_name': provider_id + }, + 'details': { + 'email': 'test@example.com', + 'fullname': 'Test User', + 'username': 'testuser' + } + } + } + mock_pipeline.return_value = pipeline_data + response = self.client.get(reverse('signin_user'), params) + else: + response = self.client.get(reverse('signin_user'), params) + + if should_redirect: + self.assertRedirects( + response, + settings.AUTHN_MICROFRONTEND_URL + '/login' + + ('?' + urlencode(params) if params else ''), + fetch_redirect_response=False + ) + else: + self.assertEqual(response.status_code, 200) + @skip_unless_lms class AccountCreationTestCaseWithSiteOverrides(SiteMixin, TestCase): From 5d69279ca486eef7f24e90d8d28d6747c25a163b Mon Sep 17 00:00:00 2001 From: Leonardo Beroes Date: Wed, 5 Mar 2025 12:21:55 -0400 Subject: [PATCH 3/3] fix: change spaced between line codes in test_logistration.py --- .../views/tests/test_logistration.py | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index 5b387adaf8ca..c187e7e6c0db 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -650,18 +650,9 @@ def test_browser_language_dialent(self): @ddt.data( (None, None, None, True), - ({ - 'name': 'Test Enterprise', - 'uuid': 'test-uuid' - }, None, None, True), - ({ - 'name': 'Test Enterprise', - 'uuid': 'test-uuid' - }, 'test-provider', None, True), - ({ - 'name': 'Test Enterprise', - 'uuid': 'test-uuid' - }, 'test-provider', True, False), + ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, None, None, True), + ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, 'test-provider', None, True), + ({'name': 'Test Enterprise', 'uuid': 'test-uuid'}, 'test-provider', True, False) ) @ddt.unpack @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) @@ -676,13 +667,17 @@ def test_enterprise_saml_redirection(self, enterprise_customer_data, provider_id self.enable_saml() self._configure_testshib_provider('TestShib', provider_id) - with mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.enterprise_customer_for_request') as mock_ec, \ + with ( mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.should_redirect_to_authn_microfrontend') as mock_should_redirect, \ + 'openedx.core.djangoapps.user_authn.views.login_form.enterprise_customer_for_request' + ) as mock_ec, mock.patch( - 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.utils.is_saml_provider') as mock_is_saml: - + 'openedx.core.djangoapps.user_authn.views.login_form.should_redirect_to_authn_microfrontend' + ) as mock_should_redirect, + mock.patch( + 'openedx.core.djangoapps.user_authn.views.login_form.third_party_auth.utils.is_saml_provider' + ) as mock_is_saml + ): mock_ec.return_value = enterprise_customer_data mock_should_redirect.return_value = should_redirect mock_is_saml.return_value = (True, None) if is_saml else (False, None)