From 4b247013fff60191555bcd881f3a351ea2389f93 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Fri, 23 Apr 2021 11:55:06 -0400 Subject: [PATCH] feat: account activation now supports a next query param. ENT-4433 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change causes the activation link that’s emailed to a newly-registered user to utilize a next query parameter. The impetus for this change is an edX Enterprise use-case: we'd like newly registered Enterprise Customer admins and learners to be directed to the Enterprise Learner Portal (or Admin Portal) upon account activation. This is likely a broad enough use case to be valuable in other endeavors. --- common/djangoapps/student/helpers.py | 2 +- .../student/tests/test_activate_account.py | 55 +++++++++++++++++++ .../djangoapps/student/tests/test_helpers.py | 4 +- common/djangoapps/student/tests/test_tasks.py | 2 +- common/djangoapps/student/views/management.py | 45 ++++++++++++--- .../student_account/models/RegisterModel.js | 3 +- .../js/student_account/views/AccessView.js | 3 +- .../djangoapps/user_authn/views/register.py | 4 +- 8 files changed, 103 insertions(+), 15 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index b7321c563d..f84af9acc3 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -366,7 +366,7 @@ def _get_redirect_to(request_host, request_headers, request_params, request_is_h redirect_to = None elif mime_type: log.warning( - "Redirect to url path with specified filed type '%(mime_type)s' not allowed: '%(redirect_to)s'", + "Redirect to url path with specified file type '%(mime_type)s' not allowed: '%(redirect_to)s'", {"redirect_to": redirect_to, "mime_type": mime_type} ) redirect_to = None diff --git a/common/djangoapps/student/tests/test_activate_account.py b/common/djangoapps/student/tests/test_activate_account.py index d9795d7ecb..32cd518b37 100644 --- a/common/djangoapps/student/tests/test_activate_account.py +++ b/common/djangoapps/student/tests/test_activate_account.py @@ -10,6 +10,7 @@ from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.test import TestCase, override_settings from django.urls import reverse +from django.utils.http import urlencode from edx_toggles.toggles.testutils import override_waffle_flag from common.djangoapps.student.models import Registration @@ -163,6 +164,60 @@ class TestActivateAccount(TestCase): self.assertRedirects(response, login_page_url) self.assertContains(response, 'Your account could not be activated') + @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:1991']) + @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_waffle_flag(REDIRECT_TO_AUTHN_MICROFRONTEND, active=True) + def test_account_activation_with_valid_next_url(self): + """ + Verify that an activation link with a valid next URL will redirect + 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) + + 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, follow=True, HTTP_ACCEPT='*/*') + + # There's not actually a server running at localhost:1991 for testing, + # so we should expect to land on `redirect_url` but with a status code of 404. + self.assertRedirects(response, redirect_url, target_status_code=404) + self._assert_user_active_state(expected_active_state=True) + + @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:9876']) + @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_waffle_flag(REDIRECT_TO_AUTHN_MICROFRONTEND, active=False) + def test_account_activation_invalid_next_url_redirects_dashboard(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. + """ + 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}), + ) + + 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._assert_user_active_state(expected_active_state=True) + @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) @override_waffle_flag(REDIRECT_TO_AUTHN_MICROFRONTEND, active=True) def test_unauthenticated_user_redirects_to_mfe(self): diff --git a/common/djangoapps/student/tests/test_helpers.py b/common/djangoapps/student/tests/test_helpers.py index ef864a103b..beb5f4555a 100644 --- a/common/djangoapps/student/tests/test_helpers.py +++ b/common/djangoapps/student/tests/test_helpers.py @@ -48,9 +48,9 @@ class TestLoginHelper(TestCase): "Redirect to non html content 'image/*' detected from 'test/agent' after login page: '" + static_url + "dummy.png" + "'"), (logging.WARNING, "WARNING", "test.png", "text/html", None, - "Redirect to url path with specified filed type 'image/png' not allowed: 'test.png'"), + "Redirect to url path with specified file type 'image/png' not allowed: 'test.png'"), (logging.WARNING, "WARNING", static_url + "dummy.png", "text/html", None, - "Redirect to url path with specified filed type 'image/png' not allowed: '" + static_url + "dummy.png" + "'"), + "Redirect to url path with specified file type 'image/png' not allowed: '" + static_url + "dummy.png" + "'"), ) @ddt.unpack def test_next_failures(self, log_level, log_name, unsafe_url, http_accept, user_agent, expected_log): diff --git a/common/djangoapps/student/tests/test_tasks.py b/common/djangoapps/student/tests/test_tasks.py index a02f290d37..21fc329ef8 100644 --- a/common/djangoapps/student/tests/test_tasks.py +++ b/common/djangoapps/student/tests/test_tasks.py @@ -26,7 +26,7 @@ class SendActivationEmailTestCase(TestCase): registration = Registration() registration.register(self.student) - self.msg = compose_activation_email("http://www.example.com", self.student, registration) + self.msg = compose_activation_email(self.student, registration) def test_ComposeEmail(self): """ diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index e1aa78ae49..0fd0782fba 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -5,6 +5,7 @@ Student Views import datetime import logging +import urllib.parse import uuid from collections import namedtuple @@ -40,6 +41,7 @@ from common.djangoapps.course_modes.models import CourseMode from lms.djangoapps.courseware.courses import get_courses, sort_by_announcement, sort_by_start_date from common.djangoapps.edxmako.shortcuts import marketing_link, render_to_response, render_to_string # lint-amnesty, pylint: disable=unused-import from common.djangoapps.entitlements.models import CourseEntitlement +from common.djangoapps.student.helpers import get_next_url_for_login_page, get_redirect_url_with_host from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.catalog.utils import get_programs_with_type from openedx.core.djangoapps.embargo import api as embargo_api @@ -162,7 +164,7 @@ def index(request, extra_context=None, user=AnonymousUser()): return render_to_response('index.html', context) -def compose_activation_email(root_url, user, user_registration=None, route_enabled=False, profile_name=''): +def compose_activation_email(user, user_registration=None, route_enabled=False, profile_name='', redirect_url=None): """ Construct all the required params for the activation email through celery task @@ -172,10 +174,7 @@ def compose_activation_email(root_url, user, user_registration=None, route_enabl message_context = generate_activation_email_context(user, user_registration) message_context.update({ - 'confirm_activation_link': '{root_url}/activate/{activation_key}'.format( - root_url=root_url, - activation_key=message_context['key'] - ), + 'confirm_activation_link': _get_activation_confirmation_link(message_context['key'], redirect_url), 'route_enabled': route_enabled, 'routed_user': user.username, 'routed_user_email': user.email, @@ -196,7 +195,26 @@ def compose_activation_email(root_url, user, user_registration=None, route_enabl return msg -def compose_and_send_activation_email(user, profile, user_registration=None): +def _get_activation_confirmation_link(activation_key, redirect_url=None): + """ + Helper function to build an activation confirmation URL given an activation_key. + The confirmation URL will include a "?next={redirect_url}" query if redirect_url + is not null. + """ + root_url = configuration_helpers.get_value('LMS_ROOT_URL', settings.LMS_ROOT_URL) + confirmation_link = '{root_url}/activate/{activation_key}'.format( + root_url=root_url, + activation_key=activation_key, + ) + if not redirect_url: + return confirmation_link + + scheme, netloc, path, params, _, fragment = urllib.parse.urlparse(confirmation_link) + query = urllib.parse.urlencode({'next': redirect_url}) + return urllib.parse.urlunparse((scheme, netloc, path, params, query, fragment)) + + +def compose_and_send_activation_email(user, profile, user_registration=None, redirect_url=None): """ Construct all the required params and send the activation email through celery task @@ -205,11 +223,11 @@ def compose_and_send_activation_email(user, profile, user_registration=None): user: current logged-in user profile: profile object of the current logged-in user user_registration: registration of the current logged-in user + redirect_url: The URL to redirect to after successful activation """ route_enabled = settings.FEATURES.get('REROUTE_ACTIVATION_EMAIL') - root_url = configuration_helpers.get_value('LMS_ROOT_URL', settings.LMS_ROOT_URL) - msg = compose_activation_email(root_url, user, user_registration, route_enabled, profile.name) + msg = compose_activation_email(user, user_registration, route_enabled, profile.name, redirect_url) send_activation_email.delay(str(msg)) @@ -554,6 +572,17 @@ def activate_account(request, key): extra_tags='account-activation aa-icon', ) + # If a (safe) `next` parameter is provided in the request + # and it's not the same as the dashboard, redirect there. + # The `get_next_url_for_login_page()` function will only return a safe redirect URL. + # If the provided `next` URL is not safe, that function will fill `redirect_to` + # with a value of `reverse('dashboard')`. + if request.GET.get('next'): + redirect_to, root_url = get_next_url_for_login_page(request, include_host=True) + if redirect_to != reverse('dashboard'): + redirect_url = get_redirect_url_with_host(root_url, redirect_to) + return redirect(redirect_url) + if should_redirect_to_authn_microfrontend() and not request.user.is_authenticated: url_path = f'/login?account_activation_status={activation_message_type}' return redirect(settings.AUTHN_MICROFRONTEND_URL + url_path) diff --git a/lms/static/js/student_account/models/RegisterModel.js b/lms/static/js/student_account/models/RegisterModel.js index 136f429191..2833564349 100644 --- a/lms/static/js/student_account/models/RegisterModel.js +++ b/lms/static/js/student_account/models/RegisterModel.js @@ -20,11 +20,12 @@ initialize: function(attributes, options) { this.ajaxType = options.method; this.urlRoot = options.url; + this.nextUrl = options.nextUrl; }, sync: function(method, model) { var headers = {'X-CSRFToken': $.cookie('csrftoken')}, - data = {}, + data = {next: model.nextUrl}, courseId = $.url('?course_id'); // If there is a course ID in the query string param, diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index ff51d0f6e2..8c692f4671 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -199,7 +199,8 @@ register: function(data) { var model = new RegisterModel({}, { method: data.method, - url: data.submit_url + url: data.submit_url, + nextUrl: this.nextUrl }); this.subview.register = new RegisterView({ diff --git a/openedx/core/djangoapps/user_authn/views/register.py b/openedx/core/djangoapps/user_authn/views/register.py index 7730a7ac49..b1afc17126 100644 --- a/openedx/core/djangoapps/user_authn/views/register.py +++ b/openedx/core/djangoapps/user_authn/views/register.py @@ -235,7 +235,9 @@ def create_account_with_params(request, params): if skip_email: registration.activate() else: - compose_and_send_activation_email(user, profile, registration) + redirect_to, root_url = get_next_url_for_login_page(request, include_host=True) + redirect_url = get_redirect_url_with_host(root_url, redirect_to) + compose_and_send_activation_email(user, profile, registration, redirect_url) if settings.FEATURES.get('ENABLE_DISCUSSION_EMAIL_DIGEST'): try: