diff --git a/common/djangoapps/student/tests/test_activate_account.py b/common/djangoapps/student/tests/test_activate_account.py index 3fd6a1b432e8..24c658e17eb8 100644 --- a/common/djangoapps/student/tests/test_activate_account.py +++ b/common/djangoapps/student/tests/test_activate_account.py @@ -5,6 +5,7 @@ from unittest.mock import patch from uuid import uuid4 +import ddt from django.conf import settings from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.test import TestCase, override_settings @@ -15,10 +16,10 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangolib.testing.utils import skip_unless_lms -from openedx.features.enterprise_support.tests.factories import EnterpriseCustomerUserFactory @skip_unless_lms +@ddt.ddt class TestActivateAccount(TestCase): """Tests for account creation""" @@ -136,10 +137,7 @@ def test_account_activation_notification_on_logistration(self): Verify that logistration page displays success/error/info messages about account activation. """ - login_page_url = "{login_url}?next={redirect_url}".format( - login_url=reverse('signin_user'), - redirect_url=reverse('dashboard'), - ) + login_page_url = reverse('signin_user') self._assert_user_active_state(expected_active_state=False) # Access activation link, message should say that account has been activated. @@ -177,15 +175,14 @@ def test_email_confirmation_notification_on_logistration(self): self.assertContains(response, 'Your email could not be confirmed') @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:1991']) - @override_settings(ENABLE_AUTHN_MICROFRONTEND=True, ENABLE_ENTERPRISE_INTEGRATION=True) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) def test_authenticated_account_activation_with_valid_next_url(self): """ Verify that an activation link with a valid next URL will redirect - the activated enterprise user to that next URL, even if the AuthN - MFE is active and redirects to it are enabled. + the activated user to that next URL, even if the AuthN MFE is active + and redirects to it are enabled. """ self._assert_user_active_state(expected_active_state=False) - EnterpriseCustomerUserFactory(user_id=self.user.id) # Make sure the user is authenticated before activation. self.login() @@ -205,13 +202,41 @@ def test_authenticated_account_activation_with_valid_next_url(self): self.assertRedirects(response, redirect_url, target_status_code=404) self._assert_user_active_state(expected_active_state=True) + @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:1991']) + def test_unauthenticated_user_redirects_to_login_with_valid_next_url(self): + """ + Verify that when the AuthN MFE is disabled, an unauthenticated user activating + with a valid next URL is sent to the legacy login page with that URL preserved + as the `next` parameter, rather than having it dropped in favour of the + dashboard. The legacy login page renders the activation message itself. + """ + self._assert_user_active_state(expected_active_state=False) + + redirect_url = 'http://localhost:1991/pied-piper/learn' + base_activation_url = reverse('activate', args=[self.registration.activation_key]) + activation_url = '{base}?{params}'.format( + base=base_activation_url, + params=urlencode({'next': redirect_url}), + ) + + # HTTP_ACCEPT is needed so the safe redirect checks pass. + response = self.client.get(activation_url, HTTP_ACCEPT='*/*') + + expected_destination = '{login_url}?{params}'.format( + login_url=reverse('signin_user'), + params=urlencode({'next': redirect_url}), + ) + assert response.url == expected_destination + self._assert_user_active_state(expected_active_state=True) + @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:9876']) - def test_account_activation_invalid_next_url_redirects_dashboard(self): + def test_account_activation_invalid_next_url_redirects_login(self): """ Verify that an activation link with an invalid next URL (i.e. it's for a domain - not in the allowed list of redirect destinations) will redirect - the activated, but unauthenticated, user to a login URL - that points to 'dashboard' as the next URL. + not in the allowed list of redirect destinations) will redirect the activated, + but unauthenticated, user to a bare login URL. The unsafe destination is dropped + rather than being replaced by an explicit 'next' of 'dashboard', leaving the + login page free to apply its own default destination. """ self._assert_user_active_state(expected_active_state=False) @@ -224,11 +249,7 @@ def test_account_activation_invalid_next_url_redirects_dashboard(self): response = self.client.get(activation_url, follow=True, HTTP_ACCEPT='*/*') - expected_destination = "{login_url}?next={redirect_url}".format( - login_url=reverse('signin_user'), - redirect_url=reverse('dashboard'), - ) - self.assertRedirects(response, expected_destination) + self.assertRedirects(response, reverse('signin_user')) self._assert_user_active_state(expected_active_state=True) @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) @@ -286,6 +307,33 @@ def test_unauthenticated_user_redirects_to_mfe_with_valid_next_url(self): response = self.client.get(activation_url, HTTP_ACCEPT='*/*') assert response.url == (login_page_url + 'info&' + encoded_next_param) + @ddt.data( + {'authn_mfe_enabled': True, 'is_authenticated': True}, + {'authn_mfe_enabled': True, 'is_authenticated': False}, + {'authn_mfe_enabled': False, 'is_authenticated': True}, + {'authn_mfe_enabled': False, 'is_authenticated': False}, + ) + @ddt.unpack + def test_activation_clears_cta_cookie(self, authn_mfe_enabled, is_authenticated): + """ + Verify that a successful activation always deletes the account activation CTA + cookie, regardless of whether or not the AuthN MFE is enabled, and regardless of + whether or not the learner is logged-in. The cookie means "this learner still + needs to activate", which can't ever be true after successful activation. + """ + if is_authenticated: + self.login() + self.client.cookies[settings.SHOW_ACTIVATE_CTA_POPUP_COOKIE_NAME] = 'True' + self._assert_user_active_state(expected_active_state=False) + + with override_settings(ENABLE_AUTHN_MICROFRONTEND=authn_mfe_enabled): + response = self.client.get(reverse('activate', args=[self.registration.activation_key])) + + cta_cookie = response.cookies[settings.SHOW_ACTIVATE_CTA_POPUP_COOKIE_NAME] + assert cta_cookie.value == '' + assert cta_cookie['max-age'] == 0 + self._assert_user_active_state(expected_active_state=True) + def test_authenticated_user_cannot_activate_another_account(self): """ Verify that if a user is authenticated and tries to activate another account, diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index fd8bd23cfd41..bcce34ce8546 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -698,7 +698,7 @@ def activate_account(request, key): if request.GET.get('next'): redirect_to, root_login_url = get_next_url_for_login_page(request, include_host=True) - # Don't automatically redirect authenticated users to the redirect_url + # Don't automatically redirect to the redirect_url # if the `next` value is either: # 1. "/dashboard" or # 2. "https://{LMS_ROOT_URL}/dashboard" (which we might provide as a value from the AuthN MFE) @@ -708,14 +708,23 @@ def activate_account(request, key): ): redirect_url = get_redirect_url_with_host(root_login_url, redirect_to) - if should_redirect_to_authn_microfrontend() and not request.user.is_authenticated: - params = {'account_activation_status': activation_message_type} + # Visitors who are not signed in have to authenticate before they can use their + # destination, so force a detour to the login page. + if not request.user.is_authenticated: + params = {} + if should_redirect_to_authn_microfrontend(): + login_url = settings.AUTHN_MICROFRONTEND_URL + '/login' + params['account_activation_status'] = activation_message_type + else: + login_url = reverse('signin_user') + if redirect_url: params['next'] = redirect_url - url_path = '/login?{}'.format(urllib.parse.urlencode(params)) # noqa: UP032 - return redirect(settings.AUTHN_MICROFRONTEND_URL + url_path) + if params: + login_url = f'{login_url}?{urllib.parse.urlencode(params)}' + redirect_url = login_url - response = redirect(redirect_url) if redirect_url and is_enterprise_learner(request.user) else redirect('dashboard') + response = redirect(redirect_url or 'dashboard') if show_account_activation_popup: response.delete_cookie( settings.SHOW_ACTIVATE_CTA_POPUP_COOKIE_NAME,