From a8bed5ce98bba86bf96b20ff7b40da0412581126 Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Wed, 26 Nov 2014 15:35:29 -0500 Subject: [PATCH] Make logistration generally available if feature flag is active Makes logistration available at /login and /register as well as /accounts/login/ and /accounts/register/. In addition: - Adds support for redirect URLs in third party auth for combined login/registration page - Adds support for external auth on the combined login/registration page - Removes old login and registration acceptance tests - Adds deprecation warnings to old login and register views - Moves third party auth util to student_account - Adds exception for microsites (theming) --- .../external_auth/login_and_register.py | 92 ++++++ .../external_auth/tests/test_ssl.py | 3 +- common/djangoapps/student/helpers.py | 78 +---- common/djangoapps/student/tests/test_login.py | 40 +-- .../tests/test_login_registration_forms.py | 15 +- common/djangoapps/student/views.py | 85 ++--- .../djangoapps/third_party_auth/pipeline.py | 80 ++--- .../third_party_auth/tests/specs/base.py | 33 +- .../pages/lms/login_and_register.py | 4 +- common/test/acceptance/tests/lms/test_lms.py | 44 --- .../courseware/features/login.feature | 58 ---- lms/djangoapps/courseware/features/login.py | 59 ---- .../courseware/features/signup.feature | 19 -- lms/djangoapps/courseware/features/signup.py | 33 -- .../tests/test_registration_extra_vars.py | 293 ------------------ lms/djangoapps/instructor/enrollment.py | 2 +- lms/djangoapps/instructor/views/legacy.py | 2 +- lms/djangoapps/student_account/helpers.py | 77 ++++- .../student_account/test/test_views.py | 45 ++- lms/djangoapps/student_account/views.py | 48 ++- .../js/student_account/views/AccessView.js | 2 +- .../courseware/mktg_course_about.html | 2 +- lms/urls.py | 21 +- 23 files changed, 341 insertions(+), 794 deletions(-) create mode 100644 common/djangoapps/external_auth/login_and_register.py delete mode 100644 lms/djangoapps/courseware/features/login.feature delete mode 100644 lms/djangoapps/courseware/features/login.py delete mode 100644 lms/djangoapps/courseware/features/signup.feature delete mode 100644 lms/djangoapps/courseware/features/signup.py delete mode 100644 lms/djangoapps/courseware/tests/test_registration_extra_vars.py diff --git a/common/djangoapps/external_auth/login_and_register.py b/common/djangoapps/external_auth/login_and_register.py new file mode 100644 index 0000000000..fa4ab2e8ed --- /dev/null +++ b/common/djangoapps/external_auth/login_and_register.py @@ -0,0 +1,92 @@ +"""Intercept login and registration requests. + +This module contains legacy code originally from `student.views`. +""" +import re + +from django.conf import settings +from django.shortcuts import redirect +from django.core.urlresolvers import reverse +import external_auth.views + +from xmodule.modulestore.django import modulestore +from opaque_keys.edx.keys import CourseKey + + +# pylint: disable=fixme +# TODO: This function is kind of gnarly/hackish/etc and is only used in one location. +# It'd be awesome if we could get rid of it; manually parsing course_id strings form larger strings +# seems Probably Incorrect +def _parse_course_id_from_string(input_str): + """ + Helper function to determine if input_str (typically the queryparam 'next') contains a course_id. + @param input_str: + @return: the course_id if found, None if not + """ + m_obj = re.match(r'^/courses/{}'.format(settings.COURSE_ID_PATTERN), input_str) + if m_obj: + return CourseKey.from_string(m_obj.group('course_id')) + return None + + +def _get_course_enrollment_domain(course_id): + """ + Helper function to get the enrollment domain set for a course with id course_id + @param course_id: + @return: + """ + course = modulestore().get_course(course_id) + if course is None: + return None + + return course.enrollment_domain + + +def login(request): + """Allow external auth to intercept and handle a login request. + + Arguments: + request (Request): A request for the login page. + + Returns: + Response or None + + """ + # Default to a `None` response, indicating that external auth + # is not handling the request. + response = None + + if settings.FEATURES['AUTH_USE_CERTIFICATES'] and external_auth.views.ssl_get_cert_from_request(request): + # SSL login doesn't require a view, so redirect + # branding and allow that to process the login if it + # is enabled and the header is in the request. + response = external_auth.views.redirect_with_get('root', request.GET) + elif settings.FEATURES.get('AUTH_USE_CAS'): + # If CAS is enabled, redirect auth handling to there + response = redirect(reverse('cas-login')) + elif settings.FEATURES.get('AUTH_USE_SHIB'): + redirect_to = request.GET.get('next') + if redirect_to: + course_id = _parse_course_id_from_string(redirect_to) + if course_id and _get_course_enrollment_domain(course_id): + response = external_auth.views.course_specific_login(request, course_id.to_deprecated_string()) + + return response + + +def register(request): + """Allow external auth to intercept and handle a registration request. + + Arguments: + request (Request): A request for the registration page. + + Returns: + Response or None + + """ + response = None + if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): + # Redirect to branding to process their certificate if SSL is enabled + # and registration is disabled. + response = external_auth.views.redirect_with_get('root', request.GET) + return response diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index 18cee1e35b..ba47942b6e 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -220,8 +220,7 @@ class SSLClientTest(ModuleStoreTestCase): # Test that they do signin if they don't have a cert response = self.client.get(reverse('signin_user')) self.assertEqual(200, response.status_code) - self.assertTrue('login_form' in response.content - or 'login-form' in response.content) + self.assertTrue('login-and-registration-container' in response.content) # And get directly logged in otherwise response = self.client.get( diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 912fa5453a..d6093e8c01 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -4,83 +4,9 @@ from datetime import datetime from pytz import UTC from django.utils.http import cookie_date from django.conf import settings -from django.core.urlresolvers import reverse -from opaque_keys.edx.keys import CourseKey -from course_modes.models import CourseMode -from third_party_auth import ( # pylint: disable=unused-import - pipeline, provider, - is_enabled as third_party_auth_enabled -) from verify_student.models import SoftwareSecurePhotoVerification # pylint: disable=F0401 - - -def auth_pipeline_urls(auth_entry, redirect_url=None, course_id=None, email_opt_in=None): - """Retrieve URLs for each enabled third-party auth provider. - - These URLs are used on the "sign up" and "sign in" buttons - on the login/registration forms to allow users to begin - authentication with a third-party provider. - - Optionally, we can redirect the user to an arbitrary - url after auth completes successfully. We use this - to redirect the user to a page that required login, - or to send users to the payment flow when enrolling - in a course. - - Args: - auth_entry (string): Either `pipeline.AUTH_ENTRY_LOGIN` or `pipeline.AUTH_ENTRY_REGISTER` - - Keyword Args: - redirect_url (unicode): If provided, send users to this URL - after they successfully authenticate. - - course_id (unicode): The ID of the course the user is enrolling in. - We use this to send users to the track selection page - if the course has a payment option. - Note that `redirect_url` takes precedence over the redirect - to the track selection page. - - email_opt_in (unicode): The user choice to opt in for organization wide emails. If set to 'true' - (case insensitive), user will be opted into organization-wide email. All other values will - be treated as False, and the user will be opted out of organization-wide email. - - Returns: - dict mapping provider names to URLs - - """ - if not third_party_auth_enabled(): - return {} - - if redirect_url is not None: - pipeline_redirect = redirect_url - elif course_id is not None: - # If the course is white-label (paid), then we send users - # to the shopping cart. (There is a third party auth pipeline - # step that will add the course to the cart.) - if CourseMode.is_white_label(CourseKey.from_string(course_id)): - pipeline_redirect = reverse("shoppingcart.views.show_cart") - - # Otherwise, send the user to the track selection page. - # The track selection page may redirect the user to the dashboard - # (if the only available mode is honor), or directly to verification - # (for professional ed). - else: - pipeline_redirect = reverse( - "course_modes_choose", - kwargs={'course_id': unicode(course_id)} - ) - else: - pipeline_redirect = None - - return { - provider.NAME: pipeline.get_login_url( - provider.NAME, auth_entry, - enroll_course_id=course_id, - email_opt_in=email_opt_in, - redirect_url=pipeline_redirect - ) - for provider in provider.Registry.enabled() - } +from course_modes.models import CourseMode +from student_account.helpers import auth_pipeline_urls # pylint: disable=unused-import,import-error def set_logged_in_cookie(request, response): diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index 8cf07e3a80..273cc2af47 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/common/djangoapps/student/tests/test_login.py @@ -14,18 +14,13 @@ from django.http import HttpResponseBadRequest, HttpResponse from external_auth.models import ExternalAuthMap import httpretty from mock import patch -from opaque_keys.edx.locations import SlashSeparatedCourseKey from social.apps.django_app.default.models import UserSocialAuth -from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory -from student.views import ( - _parse_course_id_from_string, - _get_course_enrollment_domain, - login_oauth_token, -) +from student.views import login_oauth_token + from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_MOCK_MODULESTORE class LoginTest(TestCase): @@ -324,24 +319,6 @@ class LoginTest(TestCase): self.assertNotIn(log_string, format_string) -class UtilFnTest(TestCase): - """ - Tests for utility functions in student.views - """ - def test__parse_course_id_from_string(self): - """ - Tests the _parse_course_id_from_string util function - """ - COURSE_ID = u'org/num/run' # pylint: disable=invalid-name - COURSE_URL = u'/courses/{}/otherstuff'.format(COURSE_ID) # pylint: disable=invalid-name - NON_COURSE_URL = u'/blahblah' # pylint: disable=invalid-name - self.assertEqual( - _parse_course_id_from_string(COURSE_URL), - SlashSeparatedCourseKey.from_deprecated_string(COURSE_ID) - ) - self.assertIsNone(_parse_course_id_from_string(NON_COURSE_URL)) - - @override_settings(MODULESTORE=TEST_DATA_MOCK_MODULESTORE) class ExternalAuthShibTest(ModuleStoreTestCase): """ @@ -387,15 +364,6 @@ class ExternalAuthShibTest(ModuleStoreTestCase): 'redirect': reverse('shib-login'), }) - @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") - def test__get_course_enrollment_domain(self): - """ - Tests the _get_course_enrollment_domain utility function - """ - self.assertIsNone(_get_course_enrollment_domain(SlashSeparatedCourseKey("I", "DONT", "EXIST"))) - self.assertIsNone(_get_course_enrollment_domain(self.course.id)) - self.assertEqual(self.shib_course.enrollment_domain, _get_course_enrollment_domain(self.shib_course.id)) - @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") def test_login_required_dashboard(self): """ @@ -416,7 +384,7 @@ class ExternalAuthShibTest(ModuleStoreTestCase): noshib_response = self.client.get(TARGET_URL, follow=True) self.assertEqual(noshib_response.redirect_chain[-1], ('http://testserver/accounts/login?next={url}'.format(url=TARGET_URL), 302)) - self.assertContains(noshib_response, ("Log into your {platform_name} Account | {platform_name}" + self.assertContains(noshib_response, ("Sign in or Register | {platform_name}" .format(platform_name=settings.PLATFORM_NAME))) self.assertEqual(noshib_response.status_code, 200) diff --git a/common/djangoapps/student/tests/test_login_registration_forms.py b/common/djangoapps/student/tests/test_login_registration_forms.py index a1feff45d9..13883bb5cb 100644 --- a/common/djangoapps/student/tests/test_login_registration_forms.py +++ b/common/djangoapps/student/tests/test_login_registration_forms.py @@ -2,11 +2,14 @@ import urllib import unittest from collections import OrderedDict + +import ddt from mock import patch from django.conf import settings from django.core.urlresolvers import reverse -import ddt from django.test.utils import override_settings + +from util.testing import UrlResetMixin from xmodule.modulestore.tests.factories import CourseFactory from student.tests.factories import CourseModeFactory from xmodule.modulestore.tests.django_utils import ( @@ -42,10 +45,11 @@ def _third_party_login_url(backend_name, auth_entry, course_id=None, redirect_ur @ddt.ddt @override_settings(MODULESTORE=MODULESTORE_CONFIG) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') -class LoginFormTest(ModuleStoreTestCase): +class LoginFormTest(UrlResetMixin, ModuleStoreTestCase): """Test rendering of the login form. """ - + @patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False}) def setUp(self): + super(LoginFormTest, self).setUp('lms.urls') self.url = reverse("signin_user") self.course = CourseFactory.create() self.course_id = unicode(self.course.id) @@ -153,10 +157,11 @@ class LoginFormTest(ModuleStoreTestCase): @ddt.ddt @override_settings(MODULESTORE=MODULESTORE_CONFIG) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') -class RegisterFormTest(ModuleStoreTestCase): +class RegisterFormTest(UrlResetMixin, ModuleStoreTestCase): """Test rendering of the registration form. """ - + @patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False}) def setUp(self): + super(RegisterFormTest, self).setUp('lms.urls') self.url = reverse("register_user") self.course = CourseFactory.create() self.course_id = unicode(self.course.id) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 0e2e4be5c6..774cf23168 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -76,6 +76,10 @@ from django_comment_common.models import Role from external_auth.models import ExternalAuthMap import external_auth.views +from external_auth.login_and_register import ( + login as external_auth_login, + register as external_auth_register +) from bulk_email.models import Optout, CourseAuthorization import shoppingcart @@ -354,18 +358,10 @@ def _cert_info(user, course, cert_status): @ensure_csrf_cookie def signin_user(request): - """ - This view will display the non-modal login form - """ - if (settings.FEATURES['AUTH_USE_CERTIFICATES'] and - external_auth.views.ssl_get_cert_from_request(request)): - # SSL login doesn't require a view, so redirect - # branding and allow that to process the login if it - # is enabled and the header is in the request. - return external_auth.views.redirect_with_get('root', request.GET) - if settings.FEATURES.get('AUTH_USE_CAS'): - # If CAS is enabled, redirect auth handling to there - return redirect(reverse('cas-login')) + """Deprecated. To be replaced by :class:`student_account.views.login_and_registration_form`.""" + external_auth_response = external_auth_login(request) + if external_auth_response is not None: + return external_auth_response if request.user.is_authenticated(): return redirect(reverse('dashboard')) @@ -391,15 +387,13 @@ def signin_user(request): @ensure_csrf_cookie def register_user(request, extra_context=None): - """ - This view will display the non-modal registration form - """ + """Deprecated. To be replaced by :class:`student_account.views.login_and_registration_form`.""" if request.user.is_authenticated(): return redirect(reverse('dashboard')) - if settings.FEATURES.get('AUTH_USE_CERTIFICATES_IMMEDIATE_SIGNUP'): - # Redirect to branding to process their certificate if SSL is enabled - # and registration is disabled. - return external_auth.views.redirect_with_get('root', request.GET) + + external_auth_response = external_auth_register(request) + if external_auth_response is not None: + return external_auth_response course_id = request.GET.get('course_id') email_opt_in = request.GET.get('email_opt_in') @@ -918,56 +912,15 @@ def change_enrollment(request, check_access=True): return HttpResponseBadRequest(_("Enrollment action is invalid")) -# pylint: disable=fixme -# TODO: This function is kind of gnarly/hackish/etc and is only used in one location. -# It'd be awesome if we could get rid of it; manually parsing course_id strings form larger strings -# seems Probably Incorrect -def _parse_course_id_from_string(input_str): - """ - Helper function to determine if input_str (typically the queryparam 'next') contains a course_id. - @param input_str: - @return: the course_id if found, None if not - """ - m_obj = re.match(r'^/courses/{}'.format(settings.COURSE_ID_PATTERN), input_str) - if m_obj: - return SlashSeparatedCourseKey.from_deprecated_string(m_obj.group('course_id')) - return None - - -def _get_course_enrollment_domain(course_id): - """ - Helper function to get the enrollment domain set for a course with id course_id - @param course_id: - @return: - """ - course = modulestore().get_course(course_id) - if course is None: - return None - - return course.enrollment_domain - - @never_cache @ensure_csrf_cookie def accounts_login(request): - """ - This view is mainly used as the redirect from the @login_required decorator. I don't believe that - the login path linked from the homepage uses it. - """ - if settings.FEATURES.get('AUTH_USE_CAS'): - return redirect(reverse('cas-login')) - if settings.FEATURES['AUTH_USE_CERTIFICATES']: - # SSL login doesn't require a view, so login - # directly here - return external_auth.views.ssl_login(request) - # see if the "next" parameter has been set, whether it has a course context, and if so, whether - # there is a course-specific place to redirect - redirect_to = request.GET.get('next') - if redirect_to: - course_id = _parse_course_id_from_string(redirect_to) - if course_id and _get_course_enrollment_domain(course_id): - return external_auth.views.course_specific_login(request, course_id.to_deprecated_string()) + """Deprecated. To be replaced by :class:`student_account.views.login_and_registration_form`.""" + external_auth_response = external_auth_login(request) + if external_auth_response is not None: + return external_auth_response + redirect_to = request.GET.get('next') context = { 'pipeline_running': 'false', 'pipeline_url': auth_pipeline_urls(pipeline.AUTH_ENTRY_LOGIN, redirect_url=redirect_to), @@ -1173,7 +1126,7 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un AUDIT_LOG.warning(u"Login failed - Account not active for user {0}, resending activation".format(username)) reactivation_email_for_user(user) - not_activated_msg = _("This account has not been activated. We have sent another activation message. Please check your e-mail for the activation instructions.") + not_activated_msg = _("This account has not been activated. We have sent another activation message. Please check your email for the activation instructions.") return JsonResponse({ "success": False, "value": not_activated_msg, diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index e46e34c994..cc403e890f 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -113,10 +113,10 @@ AUTH_ENTRY_LOGIN = 'login' AUTH_ENTRY_PROFILE = 'profile' AUTH_ENTRY_REGISTER = 'register' -# pylint: disable=fixme -# TODO (ECOM-369): Replace `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` -# with these values once the A/B test completes, then delete -# these constants. +# This is left-over from an A/B test +# of the new combined login/registration page (ECOM-369) +# We need to keep both the old and new entry points +# until every session from before the test ended has expired. AUTH_ENTRY_LOGIN_2 = 'account_login' AUTH_ENTRY_REGISTER_2 = 'account_register' @@ -134,9 +134,10 @@ AUTH_DISPATCH_URLS = { AUTH_ENTRY_LOGIN: '/login', AUTH_ENTRY_REGISTER: '/register', - # TODO (ECOM-369): Replace the dispatch URLs - # for `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` - # with these values, but DO NOT DELETE THESE KEYS. + # This is left-over from an A/B test + # of the new combined login/registration page (ECOM-369) + # We need to keep both the old and new entry points + # until every session from before the test ended has expired. AUTH_ENTRY_LOGIN_2: '/account/login/', AUTH_ENTRY_REGISTER_2: '/account/register/', @@ -152,11 +153,10 @@ _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_PROFILE, AUTH_ENTRY_REGISTER, - # TODO (ECOM-369): For the A/B test of the combined - # login/registration, we needed to introduce two - # additional end-points. Once the test completes, - # delete these constants from the choices list. - # pylint: disable=fixme + # This is left-over from an A/B test + # of the new combined login/registration page (ECOM-369) + # We need to keep both the old and new entry points + # until every session from before the test ended has expired. AUTH_ENTRY_LOGIN_2, AUTH_ENTRY_REGISTER_2, @@ -447,31 +447,16 @@ def parse_query_params(strategy, response, *args, **kwargs): # Whether the auth pipeline entered from /dashboard. 'is_dashboard': auth_entry == AUTH_ENTRY_DASHBOARD, # Whether the auth pipeline entered from /login. - 'is_login': auth_entry == AUTH_ENTRY_LOGIN, + 'is_login': auth_entry in [AUTH_ENTRY_LOGIN, AUTH_ENTRY_LOGIN_2], # Whether the auth pipeline entered from /register. - 'is_register': auth_entry == AUTH_ENTRY_REGISTER, + 'is_register': auth_entry in [AUTH_ENTRY_REGISTER, AUTH_ENTRY_REGISTER_2], # Whether the auth pipeline entered from /profile. 'is_profile': auth_entry == AUTH_ENTRY_PROFILE, # Whether the auth pipeline entered from an API 'is_api': auth_entry == AUTH_ENTRY_API, - - # TODO (ECOM-369): Delete these once the A/B test - # for the combined login/registration form completes. - # pylint: disable=fixme - 'is_login_2': auth_entry == AUTH_ENTRY_LOGIN_2, - 'is_register_2': auth_entry == AUTH_ENTRY_REGISTER_2, } -# TODO (ECOM-369): Once the A/B test of the combined login/registration -# form completes, we will be able to remove the extra login/registration -# end-points. HOWEVER, users who used the new forms during the A/B -# test may still have values for "is_login_2" and "is_register_2" -# in their sessions. For this reason, we need to continue accepting -# these kwargs in `redirect_to_supplementary_form`, but -# these should redirect to the same location as "is_login" and "is_register" -# (whichever login/registration end-points win in the test). -# pylint: disable=fixme @partial.partial def ensure_user_information( strategy, @@ -507,44 +492,31 @@ def ensure_user_information( # invariants have been violated and future misbehavior is likely. user_inactive = user and not user.is_active user_unset = user is None - dispatch_to_login = is_login and (user_unset or user_inactive) + + dispatch_to_login = ( + ((is_login or is_login_2) and (user_unset or user_inactive)) + or + ((is_register or is_register_2) and user_inactive) + ) + dispatch_to_register = (is_register or is_register_2) and user_unset reject_api_request = is_api and (user_unset or user_inactive) + if reject_api_request: # Content doesn't matter; we just want to exit the pipeline return HttpResponseBadRequest() - # TODO (ECOM-369): Consolidate this with `dispatch_to_login` - # once the A/B test completes. # pylint: disable=fixme - dispatch_to_login_2 = is_login_2 and (user_unset or user_inactive) - if is_dashboard or is_profile: return + # If the user has a linked account, but has not yet activated + # we should send them to the login page. The login page + # will tell them that they need to activate their account. if dispatch_to_login: return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], strategy)) - # TODO (ECOM-369): Consolidate this with `dispatch_to_login` - # once the A/B test completes. # pylint: disable=fixme - if dispatch_to_login_2: - return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2], strategy)) - - if is_register and user_unset: + if dispatch_to_register: return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER], strategy)) - # TODO (ECOM-369): Consolidate this with `is_register` - # once the A/B test completes. # pylint: disable=fixme - if is_register_2 and user_unset: - return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER_2], strategy)) - - # If the user has a linked account, but has not yet activated - # we should send them to the login page. The login page - # will tell them that they need to activate their account. - if is_register and user_inactive: - return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], strategy)) - - if is_register_2 and user_inactive: - return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN_2], strategy)) - def _create_redirect_url(url, strategy): """ Given a URL and a Strategy, construct the appropriate redirect URL. diff --git a/common/djangoapps/third_party_auth/tests/specs/base.py b/common/djangoapps/third_party_auth/tests/specs/base.py index 859fe427f9..11b27252a1 100644 --- a/common/djangoapps/third_party_auth/tests/specs/base.py +++ b/common/djangoapps/third_party_auth/tests/specs/base.py @@ -202,13 +202,6 @@ class IntegrationTest(testutil.TestCase, test.TestCase): self.assertFalse(payload.get('success')) self.assertIn('incorrect', payload.get('value')) - def assert_javascript_would_submit_login_form(self, boolean, response): - """Asserts we pass form submit JS the right boolean string.""" - argument_string = re.search( - r'function\ post_form_if_pipeline_running.*\(([a-z]+)\)', response.content, re.DOTALL).groups()[0] - self.assertIn(argument_string, ['true', 'false']) - self.assertEqual(boolean, True if argument_string == 'true' else False) - def assert_json_failure_response_is_inactive_account(self, response): """Asserts failure on /login for inactive account looks right.""" self.assertEqual(200, response.status_code) # Yes, it's a 200 even though it's a failure. @@ -238,15 +231,14 @@ class IntegrationTest(testutil.TestCase, test.TestCase): def assert_login_response_before_pipeline_looks_correct(self, response): """Asserts a GET of /login not in the pipeline looks correct.""" self.assertEqual(200, response.status_code) - self.assertIn('Sign in with ' + self.PROVIDER_CLASS.NAME, response.content) - self.assert_javascript_would_submit_login_form(False, response) - self.assert_signin_button_looks_functional(response.content, pipeline.AUTH_ENTRY_LOGIN) + # The combined login/registration page dynamically generates the login button, + # but we can still check that the provider name is passed in the data attribute + # for the container element. + self.assertIn(self.PROVIDER_CLASS.NAME, response.content) def assert_login_response_in_pipeline_looks_correct(self, response): """Asserts a GET of /login in the pipeline looks correct.""" self.assertEqual(200, response.status_code) - # Make sure the form submit JS is told to submit the form: - self.assert_javascript_would_submit_login_form(True, response) def assert_password_overridden_by_pipeline(self, username, password): """Verifies that the given password is not correct. @@ -268,25 +260,20 @@ class IntegrationTest(testutil.TestCase, test.TestCase): def assert_redirect_to_login_looks_correct(self, response): """Asserts a response would redirect to /login.""" self.assertEqual(302, response.status_code) - self.assertEqual('/' + pipeline.AUTH_ENTRY_LOGIN, response.get('Location')) + self.assertEqual('/login', response.get('Location')) def assert_redirect_to_register_looks_correct(self, response): """Asserts a response would redirect to /register.""" self.assertEqual(302, response.status_code) - self.assertEqual('/' + pipeline.AUTH_ENTRY_REGISTER, response.get('Location')) + self.assertEqual('/register', response.get('Location')) def assert_register_response_before_pipeline_looks_correct(self, response): """Asserts a GET of /register not in the pipeline looks correct.""" self.assertEqual(200, response.status_code) - self.assertIn('Sign up with ' + self.PROVIDER_CLASS.NAME, response.content) - self.assert_signin_button_looks_functional(response.content, pipeline.AUTH_ENTRY_REGISTER) - - def assert_signin_button_looks_functional(self, content, auth_entry): - """Asserts JS is available to signin buttons and has the right args.""" - self.assertTrue(re.search(r'function thirdPartySignin', content)) - self.assertEqual( - pipeline.get_login_url(self.PROVIDER_CLASS.NAME, auth_entry), - re.search(r"thirdPartySignin\(event, '([^']+)", content).groups()[0]) + # The combined login/registration page dynamically generates the register button, + # but we can still check that the provider name is passed in the data attribute + # for the container element. + self.assertIn(self.PROVIDER_CLASS.NAME, response.content) def assert_social_auth_does_not_exist_for_user(self, user, strategy): """Asserts a user does not have an auth with the expected provider.""" diff --git a/common/test/acceptance/pages/lms/login_and_register.py b/common/test/acceptance/pages/lms/login_and_register.py index 33d5779a01..0faac7acf5 100644 --- a/common/test/acceptance/pages/lms/login_and_register.py +++ b/common/test/acceptance/pages/lms/login_and_register.py @@ -70,7 +70,9 @@ class CombinedLoginAndRegisterPage(PageObject): in the bok choy settings. When enabled, the new page is available from either - `/account/login` or `/account/register`. + `/login` or `/register`; the new page is also served at + `/account/login/` or `/account/register/`, where it was + available for a time during an A/B test. Users can reach this page while attempting to enroll in a course, in which case users will be auto-enrolled diff --git a/common/test/acceptance/tests/lms/test_lms.py b/common/test/acceptance/tests/lms/test_lms.py index dc45d7e865..cb10f6bea2 100644 --- a/common/test/acceptance/tests/lms/test_lms.py +++ b/common/test/acceptance/tests/lms/test_lms.py @@ -19,8 +19,6 @@ from ..helpers import ( from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.create_mode import ModeCreationPage from ...pages.common.logout import LogoutPage -from ...pages.lms.find_courses import FindCoursesPage -from ...pages.lms.course_about import CourseAboutPage from ...pages.lms.course_info import CourseInfoPage from ...pages.lms.tab_nav import TabNavPage from ...pages.lms.course_nav import CourseNavPage @@ -36,48 +34,6 @@ from ...pages.lms.pay_and_verify import PaymentAndVerificationFlow, FakePaymentP from ...fixtures.course import CourseFixture, XBlockFixtureDesc, CourseUpdateDesc -class RegistrationTest(UniqueCourseTest): - """ - Test the registration process. - """ - - def setUp(self): - """ - Initialize pages and install a course fixture. - """ - super(RegistrationTest, self).setUp() - - self.find_courses_page = FindCoursesPage(self.browser) - self.course_about_page = CourseAboutPage(self.browser, self.course_id) - - # Create a course to register for - CourseFixture( - self.course_info['org'], self.course_info['number'], - self.course_info['run'], self.course_info['display_name'] - ).install() - - def test_register(self): - - # Visit the main page with the list of courses - self.find_courses_page.visit() - - # Go to the course about page and click the register button - self.course_about_page.visit() - register_page = self.course_about_page.register() - - # Fill in registration info and submit - username = "test_" + self.unique_id[0:6] - register_page.provide_info( - username + "@example.com", "test", username, "Test User" - ) - dashboard = register_page.submit() - - # We should end up at the dashboard - # Check that we're registered for the course - course_names = dashboard.available_courses - self.assertIn(self.course_info['display_name'], course_names) - - @attr('shard_1') class LoginFromCombinedPageTest(UniqueCourseTest): """Test that we can log in using the combined login/registration page. diff --git a/lms/djangoapps/courseware/features/login.feature b/lms/djangoapps/courseware/features/login.feature deleted file mode 100644 index acb5b38e7e..0000000000 --- a/lms/djangoapps/courseware/features/login.feature +++ /dev/null @@ -1,58 +0,0 @@ -@shard_1 -Feature: LMS.Login in as a registered user - As a registered user - In order to access my content - I want to be able to login in to edX - - Scenario: Login to an unactivated account - Given I am an edX user - And I am an unactivated user - And I visit the homepage - When I click the link with the text "Sign in" - And I submit my credentials on the login form - Then I should see the login error message "This account has not been activated" - - # firefox will not redirect properly when the whole suite is run - @skip_firefox - Scenario: Login to an activated account - Given I am an edX user - And I am an activated user - And I visit the homepage - When I click the link with the text "Sign in" - And I submit my credentials on the login form - Then I should be on the dashboard page - - Scenario: Logout of a signed in account - Given I am logged in - When I click the dropdown arrow - And I click the link with the text "Sign out" - Then I should see a link with the text "Sign in" - And I should see that the path is "/" - - Scenario: Login with valid redirect - Given I am an edX user - And The course "6.002x" exists - And I am registered for the course "6.002x" - And I am not logged in - And I visit the url "/courses/{}/courseware" - And I should see that the path is "/accounts/login?next=/courses/{}/courseware" - When I submit my credentials on the login form - And I wait for "2" seconds - Then the page title should contain "6.002x Courseware" - - Scenario: Login with an invalid redirect - Given I am an edX user - And I am not logged in - And I visit the url "/login?next=http://www.google.com/" - When I submit my credentials on the login form - Then I should be on the dashboard page - - Scenario: Login with a redirect with parameters - Given I am an edX user - And I am not logged in - And I visit the url "/debug/show_parameters?foo=hello&bar=world" - And I should see that the path is "/accounts/login?next=/debug/show_parameters%3Ffoo%3Dhello%26bar%3Dworld" - When I submit my credentials on the login form - And I wait for "2" seconds - Then I should see "foo: u'hello'" somewhere on the page - And I should see "bar: u'world'" somewhere on the page diff --git a/lms/djangoapps/courseware/features/login.py b/lms/djangoapps/courseware/features/login.py deleted file mode 100644 index 0881a81eea..0000000000 --- a/lms/djangoapps/courseware/features/login.py +++ /dev/null @@ -1,59 +0,0 @@ -# pylint: disable=missing-docstring -# pylint: disable=redefined-outer-name - -from lettuce import step, world -from django.contrib.auth.models import User - - -@step('I am an unactivated user$') -def i_am_an_unactivated_user(step): - user_is_an_unactivated_user('robot') - - -@step('I am an activated user$') -def i_am_an_activated_user(step): - user_is_an_activated_user('robot') - - -@step('I submit my credentials on the login form') -def i_submit_my_credentials_on_the_login_form(step): - fill_in_the_login_form('email', 'robot@edx.org') - fill_in_the_login_form('password', 'test') - - def submit_login_form(): - login_form = world.browser.find_by_css('form#login-form') - login_form.find_by_name('submit').click() - world.retry_on_exception(submit_login_form) - - -@step(u'I should see the login error message "([^"]*)"$') -def i_should_see_the_login_error_message(step, msg): - login_error_div = world.browser.find_by_css('.submission-error.is-shown') - assert (msg in login_error_div.text) - - -@step(u'click the dropdown arrow$') -def click_the_dropdown(step): - world.css_click('.dropdown') - -#### helper functions - - -def user_is_an_unactivated_user(uname): - u = User.objects.get(username=uname) - u.is_active = False - u.save() - - -def user_is_an_activated_user(uname): - u = User.objects.get(username=uname) - u.is_active = True - u.save() - - -def fill_in_the_login_form(field, value): - def fill_login_form(): - login_form = world.browser.find_by_css('form#login-form') - form_field = login_form.find_by_name(field) - form_field.fill(value) - world.retry_on_exception(fill_login_form) diff --git a/lms/djangoapps/courseware/features/signup.feature b/lms/djangoapps/courseware/features/signup.feature deleted file mode 100644 index 60067d5b20..0000000000 --- a/lms/djangoapps/courseware/features/signup.feature +++ /dev/null @@ -1,19 +0,0 @@ -@shard_2 -Feature: LMS.Sign in - In order to use the edX content - As a new user - I want to signup for a student account - - # firefox will not redirect properly - @skip_firefox - Scenario: Sign up from the homepage - Given I visit the homepage - When I click the link with the text "Register Now" - And I fill in "email" on the registration form with "robot2@edx.org" - And I fill in "password" on the registration form with "test" - And I fill in "username" on the registration form with "robot2" - And I fill in "name" on the registration form with "Robot Two" - And I check the checkbox named "terms_of_service" - And I check the checkbox named "honor_code" - And I submit the registration form - Then I should see "Thanks for Registering!" in the dashboard banner diff --git a/lms/djangoapps/courseware/features/signup.py b/lms/djangoapps/courseware/features/signup.py deleted file mode 100644 index 5f35a40e9a..0000000000 --- a/lms/djangoapps/courseware/features/signup.py +++ /dev/null @@ -1,33 +0,0 @@ -# pylint: disable=missing-docstring -# pylint: disable=redefined-outer-name - -from lettuce import world, step - - -@step('I fill in "([^"]*)" on the registration form with "([^"]*)"$') -def when_i_fill_in_field_on_the_registration_form_with_value(step, field, value): - def fill_in_registration(): - register_form = world.browser.find_by_css('form#register-form') - form_field = register_form.find_by_name(field) - form_field.fill(value) - world.retry_on_exception(fill_in_registration) - - -@step('I submit the registration form$') -def i_press_the_button_on_the_registration_form(step): - def submit_registration(): - register_form = world.browser.find_by_css('form#register-form') - register_form.find_by_name('submit').click() - world.retry_on_exception(submit_registration) - - -@step('I check the checkbox named "([^"]*)"$') -def i_check_checkbox(step, checkbox): - css_selector = 'input[name={}]'.format(checkbox) - world.css_check(css_selector) - - -@step('I should see "([^"]*)" in the dashboard banner$') -def i_should_see_text_in_the_dashboard_banner_section(step, text): - css_selector = "section.dashboard-banner h2" - assert (text in world.css_text(css_selector)) diff --git a/lms/djangoapps/courseware/tests/test_registration_extra_vars.py b/lms/djangoapps/courseware/tests/test_registration_extra_vars.py deleted file mode 100644 index 0be1646ce5..0000000000 --- a/lms/djangoapps/courseware/tests/test_registration_extra_vars.py +++ /dev/null @@ -1,293 +0,0 @@ -# -*- coding: utf-8 -""" -Tests for extra registration variables -""" -import json -from django.conf import settings -from django.test import TestCase -from django.core.urlresolvers import reverse -from mock import patch -from bs4 import BeautifulSoup -from django.utils import translation - - -class TestSortedCountryList(TestCase): - """ - Test that country list is always sorted alphabetically - """ - def setUp(self): - super(TestSortedCountryList, self).setUp() - self.url = reverse('register_user') - - def find_option_by_code(self, options, code): # pylint: disable=missing-docstring - for index, option in enumerate(options): - if option.attrs['value'] == code: - return (index, option) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) - def test_country_sorting_english(self): - """ - Test that country list is always sorted alphabetically in English - """ - response = self.client.get(self.url) - soup = BeautifulSoup(response.content) - country = soup.find(id="country") - options = country.findAll("option") - (af_index, af_option) = self.find_option_by_code(options, 'AF') - self.assertEqual( - af_option.text, - u'Afghanistan', - ) - (us_index, us_option) = self.find_option_by_code(options, 'US') - self.assertEqual( - us_option.text, - u'United States', - ) - # testing that the Afghan entry is always before the US entry - self.assertLess(af_index, us_index) - # testing two option elements to be in alphabetical order - self.assertLess(options[1].text, options[10].text) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) - def test_country_sorting_french(self): - """ - Test that country list is always sorted alphabetically in French - """ - user_language = 'fr' - with translation.override(user_language): - self.client.cookies['django_language'] = user_language - response = self.client.get(self.url) - soup = BeautifulSoup(response.content) - country = soup.find(id="country") - options = country.findAll("option") - (af_index, af_option) = self.find_option_by_code(options, 'AF') - self.assertEqual( - af_option.text, - u'Afghanistan', - ) - (us_index, us_option) = self.find_option_by_code(options, 'US') - self.assertEqual( - us_option.text, - u'États-Unis', - ) - # testing that the Afghan entry is always before the US entry - self.assertLess(af_index, us_index) - # testing two option elements to be in alphabetical order - self.assertLess(options[1].text, options[10].text) - - -class TestExtraRegistrationVariables(TestCase): - """ - Test that extra registration variables are properly checked according to settings - """ - def setUp(self): - super(TestExtraRegistrationVariables, self).setUp() - self.url = reverse('create_account') - - self.url_params = { - 'username': 'username', - 'name': 'name', - 'email': 'foo_bar@bar.com', - 'password': 'password', - 'terms_of_service': 'true', - 'honor_code': 'true', - } - - def test_default_missing_honor(self): - """ - By default, the honor code must be required - """ - self.url_params['honor_code'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'To enroll, you must follow the honor code.', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'honor_code': 'optional'}) - def test_optional_honor(self): - """ - With the honor code is made optional, should pass without extra vars - """ - self.url_params['honor_code'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertEqual(obj['success'], True) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, { - 'level_of_education': 'hidden', - 'gender': 'hidden', - 'year_of_birth': 'hidden', - 'mailing_address': 'hidden', - 'goals': 'hidden', - 'honor_code': 'hidden', - 'city': 'hidden', - 'country': 'hidden'}) - def test_all_hidden(self): - """ - When the fields are all hidden, should pass without extra vars - """ - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'}) - def test_required_city_missing(self): - """ - Should require the city if configured as 'required' but missing - """ - self.url_params['city'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'A city is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'city': 'required'}) - def test_required_city(self): - """ - Should require the city if configured as 'required' but missing - """ - self.url_params['city'] = 'New York' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) - def test_required_country_missing(self): - """ - Should require the country if configured as 'required' but missing - """ - self.url_params['country'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'A country is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'country': 'required'}) - def test_required_country(self): - self.url_params['country'] = 'New York' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'}) - def test_required_level_of_education_missing(self): - """ - Should require the level_of_education if configured as 'required' but missing - """ - self.url_params['level_of_education'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'A level of education is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'level_of_education': 'required'}) - def test_required_level_of_education(self): - self.url_params['level_of_education'] = 'p' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'}) - def test_required_gender_missing(self): - """ - Should require the gender if configured as 'required' but missing - """ - self.url_params['gender'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'Your gender is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'gender': 'required'}) - def test_required_gender(self): - self.url_params['gender'] = 'm' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'}) - def test_required_year_of_birth_missing(self): - """ - Should require the year_of_birth if configured as 'required' but missing - """ - self.url_params['year_of_birth'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'Your year of birth is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'year_of_birth': 'required'}) - def test_required_year_of_birth(self): - self.url_params['year_of_birth'] = '1982' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'}) - def test_required_mailing_address_missing(self): - """ - Should require the mailing_address if configured as 'required' but missing - """ - self.url_params['mailing_address'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'Your mailing address is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'mailing_address': 'required'}) - def test_required_mailing_address(self): - self.url_params['mailing_address'] = 'my address' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'}) - def test_required_goals_missing(self): - """ - Should require the goals if configured as 'required' but missing - """ - self.url_params['goals'] = '' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - u'A description of your goals is required', - ) - - @patch.dict(settings.REGISTRATION_EXTRA_FIELDS, {'goals': 'required'}) - def test_required_goals(self): - self.url_params['goals'] = 'my goals' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index da7874e657..04d1cbd3df 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -260,7 +260,7 @@ def get_email_params(course, auto_enroll, secure=True): registration_url = u'{proto}://{site}{path}'.format( proto=protocol, site=stripped_site_name, - path=reverse('student.views.register_user') + path=reverse('register_user') ) course_url = u'{proto}://{site}{path}'.format( proto=protocol, diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 1c6180fe09..4bb65e5849 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -825,7 +825,7 @@ def _do_enroll_students(course, course_key, students, secure=False, overload=Fal registration_url = '{proto}://{site}{path}'.format( proto=protocol, site=stripped_site_name, - path=reverse('student.views.register_user') + path=reverse('register_user') ) course_url = '{proto}://{site}{path}'.format( proto=protocol, diff --git a/lms/djangoapps/student_account/helpers.py b/lms/djangoapps/student_account/helpers.py index cd2e8d947f..5c39d8141f 100644 --- a/lms/djangoapps/student_account/helpers.py +++ b/lms/djangoapps/student_account/helpers.py @@ -1,4 +1,77 @@ """Helper functions for the student account app. """ +from django.core.urlresolvers import reverse +from opaque_keys.edx.keys import CourseKey +from course_modes.models import CourseMode +from third_party_auth import ( # pylint: disable=W0611 + pipeline, provider, + is_enabled as third_party_auth_enabled +) -# TODO: move this function here instead of importing it from student # pylint: disable=fixme -from student.helpers import auth_pipeline_urls # pylint: disable=unused-import + +def auth_pipeline_urls(auth_entry, redirect_url=None, course_id=None, email_opt_in=None): + """Retrieve URLs for each enabled third-party auth provider. + + These URLs are used on the "sign up" and "sign in" buttons + on the login/registration forms to allow users to begin + authentication with a third-party provider. + + Optionally, we can redirect the user to an arbitrary + url after auth completes successfully. We use this + to redirect the user to a page that required login, + or to send users to the payment flow when enrolling + in a course. + + Args: + auth_entry (string): Either `pipeline.AUTH_ENTRY_LOGIN` or `pipeline.AUTH_ENTRY_REGISTER` + + Keyword Args: + redirect_url (unicode): If provided, send users to this URL + after they successfully authenticate. + + course_id (unicode): The ID of the course the user is enrolling in. + We use this to send users to the track selection page + if the course has a payment option. + Note that `redirect_url` takes precedence over the redirect + to the track selection page. + + email_opt_in (unicode): The user choice to opt in for organization wide emails. If set to 'true' + (case insensitive), user will be opted into organization-wide email. All other values will + be treated as False, and the user will be opted out of organization-wide email. + + Returns: + dict mapping provider names to URLs + + """ + if not third_party_auth_enabled(): + return {} + + if redirect_url is not None: + pipeline_redirect = redirect_url + elif course_id is not None: + # If the course is white-label (paid), then we send users + # to the shopping cart. (There is a third party auth pipeline + # step that will add the course to the cart.) + if CourseMode.is_white_label(CourseKey.from_string(course_id)): + pipeline_redirect = reverse("shoppingcart.views.show_cart") + + # Otherwise, send the user to the track selection page. + # The track selection page may redirect the user to the dashboard + # (if the only available mode is honor), or directly to verification + # (for professional ed). + else: + pipeline_redirect = reverse( + "course_modes_choose", + kwargs={'course_id': unicode(course_id)} + ) + else: + pipeline_redirect = None + + return { + provider.NAME: pipeline.get_login_url( + provider.NAME, auth_entry, + enroll_course_id=course_id, + email_opt_in=email_opt_in, + redirect_url=pipeline_redirect + ) + for provider in provider.Registry.enabled() + } diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 9c2076dc50..838fcee99c 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -438,14 +438,14 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): { "name": "Facebook", "iconClass": "fa-facebook", - "loginUrl": self._third_party_login_url("facebook", "account_login"), - "registerUrl": self._third_party_login_url("facebook", "account_register") + "loginUrl": self._third_party_login_url("facebook", "login"), + "registerUrl": self._third_party_login_url("facebook", "register") }, { "name": "Google", "iconClass": "fa-google-plus", - "loginUrl": self._third_party_login_url("google-oauth2", "account_login"), - "registerUrl": self._third_party_login_url("google-oauth2", "account_register") + "loginUrl": self._third_party_login_url("google-oauth2", "login"), + "registerUrl": self._third_party_login_url("google-oauth2", "register") } ] self._assert_third_party_auth_data(response, current_provider, expected_providers) @@ -472,12 +472,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Facebook", "iconClass": "fa-facebook", "loginUrl": self._third_party_login_url( - "facebook", "account_login", + "facebook", "login", course_id=unicode(course.id), redirect_url=course_modes_choose_url ), "registerUrl": self._third_party_login_url( - "facebook", "account_register", + "facebook", "register", course_id=unicode(course.id), redirect_url=course_modes_choose_url ) @@ -486,12 +486,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Google", "iconClass": "fa-google-plus", "loginUrl": self._third_party_login_url( - "google-oauth2", "account_login", + "google-oauth2", "login", course_id=unicode(course.id), redirect_url=course_modes_choose_url ), "registerUrl": self._third_party_login_url( - "google-oauth2", "account_register", + "google-oauth2", "register", course_id=unicode(course.id), redirect_url=course_modes_choose_url ) @@ -520,12 +520,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Facebook", "iconClass": "fa-facebook", "loginUrl": self._third_party_login_url( - "facebook", "account_login", + "facebook", "login", course_id=unicode(course.id), redirect_url=shoppingcart_url ), "registerUrl": self._third_party_login_url( - "facebook", "account_register", + "facebook", "register", course_id=unicode(course.id), redirect_url=shoppingcart_url ) @@ -534,12 +534,12 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): "name": "Google", "iconClass": "fa-google-plus", "loginUrl": self._third_party_login_url( - "google-oauth2", "account_login", + "google-oauth2", "login", course_id=unicode(course.id), redirect_url=shoppingcart_url ), "registerUrl": self._third_party_login_url( - "google-oauth2", "account_register", + "google-oauth2", "register", course_id=unicode(course.id), redirect_url=shoppingcart_url ) @@ -550,6 +550,27 @@ class StudentAccountLoginAndRegistrationTest(ModuleStoreTestCase): response = self.client.get(reverse("account_login"), {"course_id": unicode(course.id)}) self._assert_third_party_auth_data(response, None, expected_providers) + @override_settings(SITE_NAME=settings.MICROSITE_TEST_HOSTNAME) + def test_microsite_uses_old_login_page(self): + # Retrieve the login page from a microsite domain + # and verify that we're served the old page. + resp = self.client.get( + reverse("account_login"), + HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME + ) + self.assertContains(resp, "Log into your Test Microsite Account") + self.assertContains(resp, "login-form") + + def test_microsite_uses_old_register_page(self): + # Retrieve the register page from a microsite domain + # and verify that we're served the old page. + resp = self.client.get( + reverse("account_register"), + HTTP_HOST=settings.MICROSITE_TEST_HOSTNAME + ) + self.assertContains(resp, "Register for Test Microsite") + self.assertContains(resp, "register-form") + def _assert_third_party_auth_data(self, response, current_provider, providers): """Verify that third party auth info is rendered correctly in a DOM data attribute. """ auth_info = markupsafe.escape( diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 8af87b358e..c270004466 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -17,6 +17,14 @@ from django.views.decorators.http import require_http_methods from edxmako.shortcuts import render_to_response, render_to_string from microsite_configuration import microsite import third_party_auth +from external_auth.login_and_register import ( + login as external_auth_login, + register as external_auth_register +) +from student.views import ( + signin_user as old_login_view, + register_user as old_register_view +) from openedx.core.djangoapps.user_api.api import account as account_api from openedx.core.djangoapps.user_api.api import profile as profile_api @@ -62,7 +70,7 @@ def login_and_registration_form(request, initial_mode="login"): the user_api. Keyword Args: - initial_mode (string): Either "login" or "registration". + initial_mode (string): Either "login" or "register". """ # If we're already logged in, redirect to the dashboard @@ -72,6 +80,19 @@ def login_and_registration_form(request, initial_mode="login"): # Retrieve the form descriptions from the user API form_descriptions = _get_form_descriptions(request) + # If this is a microsite, revert to the old login/registration pages. + # We need to do this for now to support existing themes. + if microsite.is_request_in_microsite(): + if initial_mode == "login": + return old_login_view(request) + elif initial_mode == "register": + return old_register_view(request) + + # Allow external auth to intercept and handle the request + ext_auth_response = _external_auth_intercept(request, initial_mode) + if ext_auth_response is not None: + return ext_auth_response + # Otherwise, render the combined login/registration page context = { 'disable_courseware_js': True, @@ -299,13 +320,15 @@ def _third_party_auth_context(request): course_id = request.GET.get("course_id") email_opt_in = request.GET.get('email_opt_in') + redirect_to = request.GET.get("next") login_urls = auth_pipeline_urls( - third_party_auth.pipeline.AUTH_ENTRY_LOGIN_2, + third_party_auth.pipeline.AUTH_ENTRY_LOGIN, course_id=course_id, - email_opt_in=email_opt_in + email_opt_in=email_opt_in, + redirect_url=redirect_to ) register_urls = auth_pipeline_urls( - third_party_auth.pipeline.AUTH_ENTRY_REGISTER_2, + third_party_auth.pipeline.AUTH_ENTRY_REGISTER, course_id=course_id, email_opt_in=email_opt_in ) @@ -377,3 +400,20 @@ def _local_server_get(url, session): # Return the content of the response return response.content + + +def _external_auth_intercept(request, mode): + """Allow external auth to intercept a login/registration request. + + Arguments: + request (Request): The original request. + mode (str): Either "login" or "register" + + Returns: + Response or None + + """ + if mode == "login": + return external_auth_login(request) + elif mode == "register": + return external_auth_register(request) diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index d36e9385fe..1788a40125 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -178,7 +178,7 @@ var edx = edx || {}; this.element.scrollTop( $anchor ); // Update url without reloading page - History.pushState( null, document.title, '/account/' + type + '/' + queryStr ); + History.pushState( null, document.title, '/' + type + queryStr ); analytics.page( 'login_and_registration', type ); }, diff --git a/lms/templates/courseware/mktg_course_about.html b/lms/templates/courseware/mktg_course_about.html index a210ee2bea..2a4ea5c49d 100644 --- a/lms/templates/courseware/mktg_course_about.html +++ b/lms/templates/courseware/mktg_course_about.html @@ -83,7 +83,7 @@ %elif allow_registration: ${_("Enroll in")} ${course.display_number_with_default | h} %if len(course_modes) > 1: diff --git a/lms/urls.py b/lms/urls.py index 3e752032f3..3f70e22d72 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -20,8 +20,6 @@ urlpatterns = ('', # nopep8 url(r'^dashboard$', 'student.views.dashboard', name="dashboard"), url(r'^login_ajax$', 'student.views.login_user', name="login"), url(r'^login_ajax/(?P[^/]*)$', 'student.views.login_user'), - url(r'^login$', 'student.views.signin_user', name="signin_user"), - url(r'^register$', 'student.views.register_user', name="register_user"), url(r'^admin_dashboard$', 'dashboard.views.dashboard'), @@ -35,7 +33,6 @@ urlpatterns = ('', # nopep8 url(r'^segmentio/event$', 'track.views.segmentio.segmentio_event'), url(r'^t/(?P