diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 14be87b300..4585b32c49 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -92,6 +92,7 @@ from util.password_policy_validators import ( validate_password_dictionary ) +import third_party_auth from third_party_auth import pipeline, provider from xmodule.error_module import ErrorDescriptor from shoppingcart.models import CourseRegistrationCode @@ -406,7 +407,7 @@ def register_user(request, extra_context=None): # If third-party auth is enabled, prepopulate the form with data from the # selected provider. - if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')) and pipeline.running(request): + if third_party_auth.is_enabled() and pipeline.running(request): running_pipeline = pipeline.get(request) current_provider = provider.Registry.get_by_backend_name(running_pipeline.get('backend')) overrides = current_provider.get_register_form_data(running_pipeline.get('kwargs')) @@ -619,7 +620,7 @@ def dashboard(request): 'provider_states': [], } - if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')): + if third_party_auth.is_enabled(): context['duplicate_provider'] = pipeline.get_duplicate_provider(messages.get_messages(request)) context['provider_user_states'] = pipeline.get_provider_user_states(user) @@ -911,7 +912,7 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un redirect_url = None response = None running_pipeline = None - third_party_auth_requested = microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')) and pipeline.running(request) + third_party_auth_requested = third_party_auth.is_enabled() and pipeline.running(request) third_party_auth_successful = False trumped_by_first_party_auth = bool(request.POST.get('email')) or bool(request.POST.get('password')) user = None @@ -933,7 +934,7 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un AUDIT_LOG.warning( u'Login failed - user with username {username} has no social auth with backend_name {backend_name}'.format( username=username, backend_name=backend_name)) - return HttpResponseBadRequest( + return HttpResponse( _("You've successfully logged into your {provider_name} account, but this account isn't linked with an {platform_name} account yet.").format( platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.NAME ) @@ -1344,7 +1345,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) ) - if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')) and pipeline.running(request): + if third_party_auth.is_enabled() and pipeline.running(request): post_vars = dict(post_vars.items()) post_vars.update({'password': pipeline.make_random_password()}) @@ -1524,7 +1525,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many # If the user is registering via 3rd party auth, track which provider they use provider_name = None - if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and pipeline.running(request): + if third_party_auth.is_enabled() and pipeline.running(request): running_pipeline = pipeline.get(request) current_provider = provider.Registry.get_by_backend_name(running_pipeline.get('backend')) provider_name = current_provider.NAME @@ -1615,7 +1616,7 @@ def create_account(request, post_override=None): # pylint: disable-msg=too-many redirect_url = try_change_enrollment(request) # Resume the third-party-auth pipeline if necessary. - if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')) and pipeline.running(request): + if third_party_auth.is_enabled() and pipeline.running(request): running_pipeline = pipeline.get(request) redirect_url = pipeline.get_complete_url(running_pipeline['backend']) diff --git a/common/djangoapps/third_party_auth/__init__.py b/common/djangoapps/third_party_auth/__init__.py index e69de29bb2..5aab76de69 100644 --- a/common/djangoapps/third_party_auth/__init__.py +++ b/common/djangoapps/third_party_auth/__init__.py @@ -0,0 +1,15 @@ +"""Third party authentication. """ + +from microsite_configuration import microsite + + +def is_enabled(): + """Check whether third party authentication has been enabled. """ + + # We do this imports internally to avoid initializing settings prematurely + from django.conf import settings + + return microsite.get_value( + "ENABLE_THIRD_PARTY_AUTH", + settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH") + ) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index c76e5199bb..1992971912 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -82,12 +82,27 @@ AUTH_ENTRY_DASHBOARD = 'dashboard' AUTH_ENTRY_LOGIN = 'login' AUTH_ENTRY_PROFILE = 'profile' AUTH_ENTRY_REGISTER = 'register' + +# TODO (ECOM-369): Repace `AUTH_ENTRY_LOGIN` and `AUTH_ENTRY_REGISTER` +# with these values once the A/B test completes, then delete +# these constants. +AUTH_ENTRY_LOGIN_2 = 'account_login' +AUTH_ENTRY_REGISTER_2 = 'account_register' + _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_DASHBOARD, AUTH_ENTRY_LOGIN, AUTH_ENTRY_PROFILE, - AUTH_ENTRY_REGISTER + 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. + AUTH_ENTRY_LOGIN_2, + AUTH_ENTRY_REGISTER_2, ]) + _DEFAULT_RANDOM_PASSWORD_LENGTH = 12 _PASSWORD_CHARSET = string.letters + string.digits @@ -346,11 +361,28 @@ def parse_query_params(strategy, response, *args, **kwargs): 'is_register': auth_entry == AUTH_ENTRY_REGISTER, # Whether the auth pipeline entered from /profile. 'is_profile': auth_entry == AUTH_ENTRY_PROFILE, + + # TODO (ECOM-369): Delete these once the A/B test + # for the combined login/registration form completes. + '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). @partial.partial -def redirect_to_supplementary_form(strategy, details, response, uid, is_dashboard=None, is_login=None, is_profile=None, is_register=None, user=None, *args, **kwargs): +def redirect_to_supplementary_form( + strategy, details, response, uid, + is_dashboard=None, is_login=None, is_profile=None, is_register=None, + is_login_2=None, is_register_2=None, + user=None, *args, **kwargs +): """Dispatches user to views outside the pipeline if necessary.""" # We're deliberately verbose here to make it clear what the intended @@ -364,20 +396,33 @@ def redirect_to_supplementary_form(strategy, details, response, uid, is_dashboar # It is important that we always execute the entire pipeline. Even if # behavior appears correct without executing a step, it means important # 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) + # TODO (ECOM-369): Consolidate this with `dispatch_to_login` + # once the A/B test completes. + dispatch_to_login_2 = is_login_2 and (user_unset or user_inactive) + if is_dashboard or is_profile: return if dispatch_to_login: return redirect('/login', name='signin_user') + # TODO (ECOM-369): Consolidate this with `dispatch_to_login` + # once the A/B test completes. + if dispatch_to_login_2: + return redirect(reverse(AUTH_ENTRY_LOGIN_2)) + if is_register and user_unset: return redirect('/register', name='register_user') + # TODO (ECOM-369): Consolidate this with `is_register` + # once the A/B test completes. + if is_register_2 and user_unset: + return redirect(reverse(AUTH_ENTRY_REGISTER_2)) + @partial.partial def login_analytics(*args, **kwargs): """ Sends login info to Segment.io """ @@ -387,6 +432,12 @@ def login_analytics(*args, **kwargs): 'is_login': 'edx.bi.user.account.authenticated', 'is_dashboard': 'edx.bi.user.account.linked', 'is_profile': 'edx.bi.user.account.linked', + + # Backwards compatibility: during an A/B test for the combined + # login/registration form, we introduced a new login end-point. + # Since users may continue to have this in their sessions after + # the test concludes, we need to continue accepting this action. + 'is_login_2': 'edx.bi.user.account.authenticated', } # Note: we assume only one of the `action` kwargs (is_dashboard, is_login) to be @@ -408,7 +459,7 @@ def login_analytics(*args, **kwargs): }, context={ 'Google Analytics': { - 'clientId': tracking_context.get('client_id') + 'clientId': tracking_context.get('client_id') } } ) diff --git a/common/djangoapps/third_party_auth/tests/testutil.py b/common/djangoapps/third_party_auth/tests/testutil.py index f4f3600df7..eb3f84e5e6 100644 --- a/common/djangoapps/third_party_auth/tests/testutil.py +++ b/common/djangoapps/third_party_auth/tests/testutil.py @@ -4,7 +4,9 @@ Utilities for writing third_party_auth tests. Used by Django and non-Django tests; must not have Django deps. """ +from contextlib import contextmanager import unittest +import mock from third_party_auth import provider @@ -37,3 +39,81 @@ class TestCase(unittest.TestCase): provider.Registry._reset() provider.Registry.configure_once(self._original_providers) super(TestCase, self).tearDown() + + +@contextmanager +def simulate_running_pipeline(pipeline_target, backend, email=None, fullname=None, username=None): + """Simulate that a pipeline is currently running. + + You can use this context manager to test packages that rely on third party auth. + + This uses `mock.patch` to override some calls in `third_party_auth.pipeline`, + so you will need to provide the "target" module *as it is imported* + in the software under test. For example, if `foo/bar.py` does this: + + >>> from third_party_auth import pipeline + + then you will need to do something like this: + + >>> with simulate_running_pipeline("foo.bar.pipeline", "google-oauth2"): + >>> bar.do_something_with_the_pipeline() + + If, on the other hand, `foo/bar.py` had done this: + + >>> import third_party_auth + + then you would use the target "foo.bar.third_party_auth.pipeline" instead. + + Arguments: + + pipeline_target (string): The path to `third_party_auth.pipeline` as it is imported + in the software under test. + + backend (string): The name of the backend currently running, for example "google-oauth2". + Note that this is NOT the same as the name of the *provider*. See the Python + social auth documentation for the names of the backends. + + Keyword Arguments: + email (string): If provided, simulate that the current provider has + included the user's email address (useful for filling in the registration form). + + fullname (string): If provided, simulate that the current provider has + included the user's full name (useful for filling in the registration form). + + username (string): If provided, simulate that the pipeline has provided + this suggested username. This is something that the `third_party_auth` + app generates itself and should be available by the time the user + is authenticating with a third-party provider. + + Returns: + None + + """ + pipeline_data = { + "backend": backend, + "kwargs": { + "details": {} + } + } + if email is not None: + pipeline_data["kwargs"]["details"]["email"] = email + if fullname is not None: + pipeline_data["kwargs"]["details"]["fullname"] = fullname + if username is not None: + pipeline_data["kwargs"]["username"] = username + + pipeline_get = mock.patch("{pipeline}.get".format(pipeline=pipeline_target), spec=True) + pipeline_running = mock.patch("{pipeline}.running".format(pipeline=pipeline_target), spec=True) + + mock_get = pipeline_get.start() + mock_running = pipeline_running.start() + + mock_get.return_value = pipeline_data + mock_running.return_value = True + + try: + yield + + finally: + pipeline_get.stop() + pipeline_running.stop() diff --git a/common/djangoapps/user_api/helpers.py b/common/djangoapps/user_api/helpers.py index 3be67abea6..6640ca15ea 100644 --- a/common/djangoapps/user_api/helpers.py +++ b/common/djangoapps/user_api/helpers.py @@ -2,6 +2,7 @@ Helper functions for the account/profile Python APIs. This is NOT part of the public API. """ +from collections import defaultdict from functools import wraps import logging import json @@ -101,6 +102,12 @@ class FormDescription(object): "password": ["min_length", "max_length"], } + OVERRIDE_FIELD_PROPERTIES = [ + "label", "type", "default", "placeholder", + "instructions", "required", "restrictions", + "options" + ] + def __init__(self, method, submit_url): """Configure how the form should be submitted. @@ -112,6 +119,7 @@ class FormDescription(object): self.method = method self.submit_url = submit_url self.fields = [] + self._field_overrides = defaultdict(dict) def add_field( self, name, label=u"", field_type=u"text", default=u"", @@ -161,8 +169,8 @@ class FormDescription(object): raise InvalidFieldError(msg) field_dict = { - "label": label, "name": name, + "label": label, "type": field_type, "default": default, "placeholder": placeholder, @@ -192,6 +200,10 @@ class FormDescription(object): ) raise InvalidFieldError(msg) + # If there are overrides for this field, apply them now. + # Any field property can be overwritten (for example, the default value or placeholder) + field_dict.update(self._field_overrides.get(name, {})) + self.fields.append(field_dict) def to_json(self): @@ -244,6 +256,31 @@ class FormDescription(object): "fields": self.fields }) + def override_field_properties(self, field_name, **kwargs): + """Override properties of a field. + + The overridden values take precedence over the values provided + to `add_field()`. + + Field properties not in `OVERRIDE_FIELD_PROPERTIES` will be ignored. + + Arguments: + field_name (string): The name of the field to override. + + Keyword Args: + Same as to `add_field()`. + + """ + # Transform kwarg "field_type" to "type" (a reserved Python keyword) + if "field_type" in kwargs: + kwargs["type"] = kwargs["field_type"] + + self._field_overrides[field_name].update({ + property_name: property_value + for property_name, property_value in kwargs.iteritems() + if property_name in self.OVERRIDE_FIELD_PROPERTIES + }) + def shim_student_view(view_func, check_logged_in=False): """Create a "shim" view for a view function from the student Django app. @@ -320,16 +357,28 @@ def shim_student_view(view_func, check_logged_in=False): success = True redirect_url = None - # If the user is not authenticated, and we expect them to be - # send a status 403. - if check_logged_in and not request.user.is_authenticated(): - response.status_code = 403 + # If the user is not authenticated when we expect them to be + # send the appropriate status code. + # We check whether the user attribute is set to make + # it easier to test this without necessarily running + # the request through authentication middleware. + is_authenticated = ( + getattr(request, 'user', None) is not None + and request.user.is_authenticated() + ) + if check_logged_in and not is_authenticated: + # Preserve the 401 status code so the client knows + # that the user successfully authenticated with third-party auth + # but does not have a linked account. + # Otherwise, send a 403 to indicate that the login failed. + if response.status_code != 401: + response.status_code = 403 response.content = msg # If the view wants to redirect us, send a status 302 elif redirect_url is not None: response.status_code = 302 - response.content = redirect_url + response['Location'] = redirect_url # If an error condition occurs, send a status 400 elif response.status_code != 200 or not success: @@ -343,6 +392,9 @@ def shim_student_view(view_func, check_logged_in=False): # If the response is successful, then return the content # of the response directly rather than including it # in a JSON-serialized dictionary. + # This will also preserve error status codes such as a 401 + # (if the user is trying to log in using a third-party provider + # but hasn't yet linked his or her account.) else: response.content = msg diff --git a/common/djangoapps/user_api/tests/test_helpers.py b/common/djangoapps/user_api/tests/test_helpers.py index 49ddff9dfa..f08059b329 100644 --- a/common/djangoapps/user_api/tests/test_helpers.py +++ b/common/djangoapps/user_api/tests/test_helpers.py @@ -144,6 +144,15 @@ class StudentViewShimTest(TestCase): self.assertNotIn("enrollment_action", self.captured_request.POST) self.assertNotIn("course_id", self.captured_request.POST) + @ddt.data(True, False) + def test_preserve_401_status(self, check_logged_in): + view = self._shimmed_view( + HttpResponse(status=401), + check_logged_in=check_logged_in + ) + response = view(HttpRequest()) + self.assertEqual(response.status_code, 401) + def test_non_json_response(self): view = self._shimmed_view(HttpResponse(content="Not a JSON dict")) response = view(HttpRequest()) @@ -160,7 +169,7 @@ class StudentViewShimTest(TestCase): ) response = view(HttpRequest()) self.assertEqual(response.status_code, 302) - self.assertEqual(response.content, "/redirect") + self.assertEqual(response['Location'], "/redirect") def test_error_from_json(self): view = self._shimmed_view( @@ -180,8 +189,13 @@ class StudentViewShimTest(TestCase): response = view(HttpRequest()) self.assertEqual(response["test-header"], "test") - def _shimmed_view(self, response): + def test_check_logged_in(self): + view = self._shimmed_view(HttpResponse(), check_logged_in=True) + response = view(HttpRequest()) + self.assertEqual(response.status_code, 403) + + def _shimmed_view(self, response, check_logged_in=False): def stub_view(request): self.captured_request = request return response - return shim_student_view(stub_view) + return shim_student_view(stub_view, check_logged_in=check_logged_in) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 350a5e4e44..0d1731d92f 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -13,7 +13,7 @@ from django.test.utils import override_settings from unittest import SkipTest, skipUnless import ddt from pytz import UTC -from mock import patch +import mock from user_api.api import account as account_api, profile as profile_api @@ -21,6 +21,7 @@ from student.tests.factories import UserFactory from user_api.tests.factories import UserPreferenceFactory from django_comment_common import models from opaque_keys.edx.locations import SlashSeparatedCourseKey +from third_party_auth.tests.testutil import simulate_running_pipeline from user_api.tests.test_constants import SORTED_COUNTRIES @@ -808,6 +809,82 @@ class RegistrationViewTest(ApiTestCase): } ) + def test_register_form_third_party_auth_running(self): + no_extra_fields_setting = {} + + with simulate_running_pipeline( + "user_api.views.third_party_auth.pipeline", + "google-oauth2", email="bob@example.com", + fullname="Bob", username="Bob123" + ): + # Password field should be hidden + self._assert_reg_field( + no_extra_fields_setting, + { + "name": "password", + "default": "", + "type": "hidden", + "required": False, + "label": "", + "placeholder": "", + "instructions": "", + "restrictions": {}, + } + ) + + # Email should be filled in + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"email", + u"default": u"bob@example.com", + u"type": u"text", + u"required": True, + u"label": u"E-mail", + u"placeholder": u"example: username@domain.com", + u"instructions": u"This is the e-mail address you used to register with edX", + u"restrictions": { + u"min_length": 3, + u"max_length": 254 + }, + } + ) + + # Full name should be filled in + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"name", + u"default": u"Bob", + u"type": u"text", + u"required": True, + u"label": u"Full Name", + u"placeholder": u"", + u"instructions": u"Needed for any certificates you may earn", + u"restrictions": { + "max_length": 255, + } + } + ) + + # Username should be filled in + self._assert_reg_field( + no_extra_fields_setting, + { + u"name": u"username", + u"default": u"Bob123", + u"type": u"text", + u"required": True, + u"label": u"Public Username", + u"placeholder": u"", + u"instructions": u"Will be shown in any discussions or forums you participate in (cannot be changed)", + u"restrictions": { + u"min_length": 2, + u"max_length": 30, + } + } + ) + def test_register_form_level_of_education(self): self._assert_reg_field( {"level_of_education": "optional"}, @@ -950,7 +1027,7 @@ class RegistrationViewTest(ApiTestCase): @override_settings( MKTG_URLS={"ROOT": "https://www.test.com/", "HONOR": "honor"}, ) - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) + @mock.patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) def test_registration_honor_code_mktg_site_enabled(self): self._assert_reg_field( {"honor_code": "required"}, @@ -967,7 +1044,7 @@ class RegistrationViewTest(ApiTestCase): ) @override_settings(MKTG_URLS_LINK_MAP={"HONOR": "honor"}) - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) + @mock.patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) def test_registration_honor_code_mktg_site_disabled(self): self._assert_reg_field( {"honor_code": "required"}, @@ -988,7 +1065,7 @@ class RegistrationViewTest(ApiTestCase): "HONOR": "honor", "TOS": "tos", }) - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) + @mock.patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) def test_registration_separate_terms_of_service_mktg_site_enabled(self): # Honor code field should say ONLY honor code, # not "terms of service and honor code" @@ -1022,7 +1099,7 @@ class RegistrationViewTest(ApiTestCase): ) @override_settings(MKTG_URLS_LINK_MAP={"HONOR": "honor", "TOS": "tos"}) - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) + @mock.patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) def test_registration_separate_terms_of_service_mktg_site_disabled(self): # Honor code field should say ONLY honor code, # not "terms of service and honor code" diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 7d891fae82..d93ed7bb2b 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -24,6 +24,8 @@ from django_comment_common.models import Role from opaque_keys.edx.locations import SlashSeparatedCourseKey from edxmako.shortcuts import marketing_link +import third_party_auth +from microsite_configuration import microsite from user_api.api import account as account_api, profile as profile_api from user_api.helpers import FormDescription, shim_student_view, require_post_params @@ -111,6 +113,8 @@ class LoginSessionView(APIView): Returns: HttpResponse: 200 on success HttpResponse: 400 if the request is not valid. + HttpResponse: 401 if the user successfully authenticated with a third-party + provider but does not have a linked account. HttpResponse: 403 if authentication failed. HttpResponse: 302 if redirecting to another page. @@ -189,6 +193,7 @@ class RegistrationView(APIView): """ form_desc = FormDescription("post", reverse("user_api_registration")) + self._apply_third_party_auth_overrides(request, form_desc) # Default fields are always required for field_name in self.DEFAULT_FIELDS: @@ -408,6 +413,53 @@ class RegistrationView(APIView): [("", "--")] + list(options) ) + def _apply_third_party_auth_overrides(self, request, form_desc): + """Modify the registration form if the user has authenticated with a third-party provider. + + If a user has successfully authenticated with a third-party provider, + but does not yet have an account with EdX, we want to fill in + the registration form with any info that we get from the + provider. + + This will also hide the password field, since we assign users a default + (random) password on the assumption that they will be using + third-party auth to log in. + + Arguments: + request (HttpRequest): The request for the registration form, used + to determine if the user has successfully authenticated + with a third-party provider. + + form_desc (FormDescription): The registration form description + + """ + if third_party_auth.is_enabled(): + running_pipeline = third_party_auth.pipeline.get(request) + if running_pipeline: + current_provider = third_party_auth.provider.Registry.get_by_backend_name(running_pipeline.get('backend')) + + # Override username / email / full name + field_overrides = current_provider.get_register_form_data( + running_pipeline.get('kwargs') + ) + + for field_name in self.DEFAULT_FIELDS: + if field_name in field_overrides: + form_desc.override_field_properties( + field_name, default=field_overrides[field_name] + ) + + # Hide the password field + form_desc.override_field_properties( + "password", + default="", + field_type="hidden", + required=False, + label="", + instructions="", + restrictions={} + ) + class UserViewSet(viewsets.ReadOnlyModelViewSet): authentication_classes = (authentication.SessionAuthentication,) diff --git a/lms/djangoapps/student_account/test/test_views.py b/lms/djangoapps/student_account/test/test_views.py index 051dfe8f9c..358180fb19 100644 --- a/lms/djangoapps/student_account/test/test_views.py +++ b/lms/djangoapps/student_account/test/test_views.py @@ -6,7 +6,7 @@ from unittest import skipUnless from urllib import urlencode import json -from mock import patch +import mock import ddt from django.test import TestCase from django.conf import settings @@ -14,14 +14,15 @@ from django.core.urlresolvers import reverse from django.core import mail from util.testing import UrlResetMixin +from third_party_auth.tests.testutil import simulate_running_pipeline from user_api.api import account as account_api from user_api.api import profile as profile_api from util.bad_request_rate_limiter import BadRequestRateLimiter @ddt.ddt -class StudentAccountViewTest(UrlResetMixin, TestCase): - """ Tests for the student account views. """ +class StudentAccountUpdateTest(UrlResetMixin, TestCase): + """ Tests for the student account views that update the user's account information. """ USERNAME = u"heisenberg" ALTERNATE_USERNAME = u"walt" @@ -51,9 +52,9 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): INVALID_KEY = u"123abc" - @patch.dict(settings.FEATURES, {'ENABLE_NEW_DASHBOARD': True}) + @mock.patch.dict(settings.FEATURES, {'ENABLE_NEW_DASHBOARD': True}) def setUp(self): - super(StudentAccountViewTest, self).setUp("student_account.urls") + super(StudentAccountUpdateTest, self).setUp("student_account.urls") # Create/activate a new account activation_key = account_api.create_account(self.USERNAME, self.OLD_PASSWORD, self.OLD_EMAIL) @@ -67,37 +68,6 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): response = self.client.get(reverse('account_index')) self.assertContains(response, "Student Account") - @ddt.data( - ("account_login", "login"), - ("account_register", "register"), - ) - @ddt.unpack - def test_login_and_registration_form(self, url_name, initial_mode): - response = self.client.get(reverse(url_name)) - expected_data = u"data-initial-mode=\"{mode}\"".format(mode=initial_mode) - self.assertContains(response, expected_data) - - @ddt.data("account_login", "account_register") - def test_login_and_registration_third_party_auth_urls(self, url_name): - response = self.client.get(reverse(url_name)) - - # This relies on the THIRD_PARTY_AUTH configuration in the test settings - expected_data = u"data-third-party-auth-providers=\"{providers}\"".format( - providers=json.dumps([ - { - u'icon_class': u'icon-facebook', - u'login_url': u'/auth/login/facebook/?auth_entry=login', - u'name': u'Facebook' - }, - { - u'icon_class': u'icon-google-plus', - u'login_url': u'/auth/login/google-oauth2/?auth_entry=login', - u'name': u'Google' - } - ]) - ) - self.assertContains(response, expected_data) - def test_change_email(self): response = self._change_email(self.NEW_EMAIL, self.OLD_PASSWORD) self.assertEquals(response.status_code, 200) @@ -144,7 +114,7 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): def test_email_change_request_no_user(self): # Patch account API to raise an internal error when an email change is requested - with patch('student_account.views.account_api.request_email_change') as mock_call: + with mock.patch('student_account.views.account_api.request_email_change') as mock_call: mock_call.side_effect = account_api.AccountUserNotFound response = self._change_email(self.NEW_EMAIL, self.OLD_PASSWORD) @@ -215,7 +185,7 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): activation_key = account_api.request_email_change(self.USERNAME, self.NEW_EMAIL, self.OLD_PASSWORD) # Patch account API to return an internal error - with patch('student_account.views.account_api.confirm_email_change') as mock_call: + with mock.patch('student_account.views.account_api.confirm_email_change') as mock_call: mock_call.side_effect = account_api.AccountInternalError response = self.client.get(reverse('email_change_confirm', kwargs={'key': activation_key})) @@ -392,3 +362,88 @@ class StudentAccountViewTest(UrlResetMixin, TestCase): data['email'] = email return self.client.post(path=reverse('password_change_request'), data=data) + + +@ddt.ddt +class StudentAccountLoginAndRegistrationTest(TestCase): + """ Tests for the student account views that update the user's account information. """ + + USERNAME = "bob" + EMAIL = "bob@example.com" + PASSWORD = "password" + + @ddt.data( + ("account_login", "login"), + ("account_register", "register"), + ) + @ddt.unpack + def test_login_and_registration_form(self, url_name, initial_mode): + response = self.client.get(reverse(url_name)) + expected_data = u"data-initial-mode=\"{mode}\"".format(mode=initial_mode) + self.assertContains(response, expected_data) + + @ddt.data("account_login", "account_register") + def test_login_and_registration_form_already_authenticated(self, url_name): + # Create/activate a new account and log in + activation_key = account_api.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) + account_api.activate_account(activation_key) + result = self.client.login(username=self.USERNAME, password=self.PASSWORD) + self.assertTrue(result) + + # Verify that we're redirected to the dashboard + response = self.client.get(reverse(url_name)) + self.assertRedirects(response, reverse("dashboard")) + + @mock.patch.dict(settings.FEATURES, {"ENABLE_THIRD_PARTY_AUTH": False}) + @ddt.data("account_login", "account_register") + def test_third_party_auth_disabled(self, url_name): + response = self.client.get(reverse(url_name)) + expected_data = "data-third-party-auth='{auth_info}'".format( + auth_info=json.dumps({ + "currentProvider": None, + "providers": [] + }) + ) + self.assertContains(response, expected_data) + + @ddt.data( + ("account_login", None, None), + ("account_register", None, None), + ("account_login", "google-oauth2", "Google"), + ("account_register", "google-oauth2", "Google"), + ("account_login", "facebook", "Facebook"), + ("account_register", "facebook", "Facebook"), + ) + @ddt.unpack + def test_third_party_auth(self, url_name, current_backend, current_provider): + # Simulate a running pipeline + if current_backend is not None: + pipeline_target = "student_account.views.third_party_auth.pipeline" + with simulate_running_pipeline(pipeline_target, current_backend): + response = self.client.get(reverse(url_name)) + + # Do NOT simulate a running pipeline + else: + response = self.client.get(reverse(url_name)) + + # This relies on the THIRD_PARTY_AUTH configuration in the test settings + expected_data = u"data-third-party-auth='{auth_info}'".format( + auth_info=json.dumps({ + "currentProvider": current_provider, + "providers": [ + { + "name": "Facebook", + "iconClass": "icon-facebook", + "loginUrl": "/auth/login/facebook/?auth_entry=account_login", + "registerUrl": "/auth/login/facebook/?auth_entry=account_register", + }, + { + "name": "Google", + "iconClass": "icon-google-plus", + "loginUrl": "/auth/login/google-oauth2/?auth_entry=account_login", + "registerUrl": "/auth/login/google-oauth2/?auth_entry=account_register", + } + ] + }) + ) + self.assertContains(response, expected_data) diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index 916f09f5d8..f8f554bae5 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -6,13 +6,15 @@ from django.conf import settings from django.http import ( HttpResponse, HttpResponseBadRequest, HttpResponseForbidden ) +from django.shortcuts import redirect +from django.core.urlresolvers import reverse from django.core.mail import send_mail from django_future.csrf import ensure_csrf_cookie from django.contrib.auth.decorators import login_required from django.views.decorators.http import require_http_methods from edxmako.shortcuts import render_to_response, render_to_string -import third_party_auth from microsite_configuration import microsite +import third_party_auth from user_api.api import account as account_api from user_api.api import profile as profile_api @@ -58,24 +60,17 @@ def login_and_registration_form(request, initial_mode="login"): initial_mode (string): Either "login" or "registration". """ + # If we're already logged in, redirect to the dashboard + if request.user.is_authenticated(): + return redirect(reverse('dashboard')) + + # Otherwise, render the combined login/registration page context = { 'disable_courseware_js': True, 'initial_mode': initial_mode, - 'third_party_auth_providers': json.dumps([]) + 'third_party_auth': json.dumps(_third_party_auth_context(request)), } - if microsite.get_value("ENABLE_THIRD_PARTY_AUTH", settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH")): - context["third_party_auth_providers"] = json.dumps([ - { - "name": enabled.NAME, - "icon_class": enabled.ICON_CLASS, - "login_url": third_party_auth.pipeline.get_login_url( - enabled.NAME, third_party_auth.pipeline.AUTH_ENTRY_LOGIN - ), - } - for enabled in third_party_auth.provider.Registry.enabled() - ]) - return render_to_response('student_account/login_and_register.html', context) @@ -266,3 +261,44 @@ def password_change_request_handler(request): return HttpResponse(status=200) else: return HttpResponseBadRequest("No email address provided.") + + +def _third_party_auth_context(request): + """Context for third party auth providers and the currently running pipeline. + + Arguments: + request (HttpRequest): The request, used to determine if a pipeline + is currently running. + + Returns: + dict + + """ + context = { + "currentProvider": None, + "providers": [] + } + + if third_party_auth.is_enabled(): + context["providers"] = [ + { + "name": enabled.NAME, + "iconClass": enabled.ICON_CLASS, + "loginUrl": third_party_auth.pipeline.get_login_url( + enabled.NAME, third_party_auth.pipeline.AUTH_ENTRY_LOGIN_2 + ), + "registerUrl": third_party_auth.pipeline.get_login_url( + enabled.NAME, third_party_auth.pipeline.AUTH_ENTRY_REGISTER_2 + ) + } + for enabled in third_party_auth.provider.Registry.enabled() + ] + + running_pipeline = third_party_auth.pipeline.get(request) + if running_pipeline is not None: + current_provider = third_party_auth.provider.Registry.get_by_backend_name( + running_pipeline.get('backend') + ) + context["currentProvider"] = current_provider.NAME + + return context diff --git a/lms/djangoapps/student_profile/views.py b/lms/djangoapps/student_profile/views.py index 4ec52f3f40..a7a6a3c0d0 100644 --- a/lms/djangoapps/student_profile/views.py +++ b/lms/djangoapps/student_profile/views.py @@ -13,7 +13,7 @@ from django.contrib.auth.decorators import login_required from edxmako.shortcuts import render_to_response from user_api.api import profile as profile_api from lang_pref import LANGUAGE_KEY, api as language_api -from third_party_auth import pipeline +import third_party_auth @login_required @@ -60,8 +60,8 @@ def _get_profile(request): 'disable_courseware_js': True } - if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'): - context['provider_user_states'] = pipeline.get_provider_user_states(user) + if third_party_auth.is_enabled(): + context['provider_user_states'] = third_party_auth.pipeline.get_provider_user_states(user) return render_to_response('student_profile/index.html', context) diff --git a/lms/static/js/student_account/accessApp.js b/lms/static/js/student_account/accessApp.js index b3e3a893b9..1981015b55 100644 --- a/lms/static/js/student_account/accessApp.js +++ b/lms/static/js/student_account/accessApp.js @@ -7,7 +7,7 @@ var edx = edx || {}; edx.student.account = edx.student.account || {}; return new edx.student.account.AccessView({ - mode: $('#login-and-registration-container').data('initial-mode') || 'login', - thirdPartyAuth: $('#login-and-registration-container').data('third-party-auth-providers') || false + mode: $('#login-and-registration-container').data('initial-mode'), + thirdPartyAuth: $('#login-and-registration-container').data('third-party-auth') }); -})(jQuery); \ No newline at end of file +})(jQuery); diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index bdb75e6011..1bf7b70846 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -26,7 +26,13 @@ var edx = edx || {}; initialize: function( obj ) { this.tpl = $(this.tpl).html(); - this.activeForm = obj.mode; + this.activeForm = obj.mode || 'login'; + this.thirdPartyAuth = obj.thirdPartyAuth || { + currentProvider: null, + providers: [] + }; + console.log(obj); + this.render(); }, @@ -48,12 +54,12 @@ var edx = edx || {}; loadForm: function( type ) { if ( type === 'login' ) { - this.subview.login = new edx.student.account.LoginView(); + this.subview.login = new edx.student.account.LoginView( this.thirdPartyAuth ); // Listen for 'password-help' event to toggle sub-views this.listenTo( this.subview.login, 'password-help', this.resetPassword ); } else if ( type === 'register' ) { - this.subview.register = new edx.student.account.RegisterView(); + this.subview.register = new edx.student.account.RegisterView( this.thirdPartyAuth ); } else if ( type === 'reset' ) { this.subview.passwordHelp = new edx.student.account.PasswordResetView(); } diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 22461368c1..21ed098373 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -17,15 +17,20 @@ var edx = edx || {}; events: { 'click .js-login': 'submitForm', - 'click .forgot-password': 'forgotPassword' + 'click .forgot-password': 'forgotPassword', + 'click .login-provider': 'thirdPartyAuth' }, errors: [], $form: {}, - initialize: function() { + initialize: function( thirdPartyAuthInfo ) { this.tpl = $(this.tpl).html(); + + this.providers = thirdPartyAuthInfo.providers || []; + this.currentProvider = thirdPartyAuthInfo.currentProvider || ""; + this.getInitialData(); }, @@ -34,7 +39,9 @@ var edx = edx || {}; var fields = html || ''; $(this.el).html( _.template( this.tpl, { - fields: fields + fields: fields, + currentProvider: this.currentProvider, + providers: this.providers })); this.postRender(); @@ -44,9 +51,16 @@ var edx = edx || {}; postRender: function() { var $container = $(this.el); - this.$form = $container.find('form'); this.$errors = $container.find('.error-msg'); + this.$alreadyAuthenticatedMsg = $container.find('.already-authenticated-msg'); + + // If we're already authenticated with a third-party + // provider, try logging in. The easiest way to do this + // is to simply submit the form. + if (this.currentProvider) { + this.model.save(); + } }, getInitialData: function() { @@ -58,8 +72,8 @@ var edx = edx || {}; url: '/user_api/v1/account/login_session/', success: function( data ) { console.log(data); - that.buildForm( data.fields ); that.initModel( data.submit_url, data.method ); + that.buildForm( data.fields ); }, error: function( jqXHR, textStatus, errorThrown ) { console.log('fail ', errorThrown); @@ -72,9 +86,7 @@ var edx = edx || {}; url: url }); - this.listenTo( this.model, 'error', function( error ) { - console.log(error.status, ' error: ', error.responseText); - }); + this.listenTo( this.model, 'error', this.saveError ); }, buildForm: function( data ) { @@ -148,6 +160,16 @@ var edx = edx || {}; } }, + thirdPartyAuth: function( event ) { + var providerUrl = $(event.target).data("provider-url") || ""; + if (providerUrl) { + window.location.href = providerUrl; + } else { + // TODO -- error handling here + console.log("No URL available for third party auth provider"); + } + }, + toggleErrorMsg: function( show ) { if ( show ) { this.$errors.removeClass('hidden'); @@ -158,6 +180,23 @@ var edx = edx || {}; validate: function( $el ) { return edx.utils.validate( $el ); + }, + + saveError: function( error ) { + console.log(error.status, ' error: ', error.responseText); + + // If we've gotten a 401 error, it means that we've successfully + // authenticated with a third-party provider, but we haven't + // linked the account to an EdX account. In this case, + // we need to prompt the user to enter a little more information + // to complete the registration process. + if (error.status === 401 && this.currentProvider) { + this.$alreadyAuthenticatedMsg.removeClass("hidden"); + } + else { + this.$alreadyAuthenticatedMsg.addClass("hidden"); + // TODO -- display the error + } } }); diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index 42bd1974eb..26dc23a0f1 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -16,15 +16,19 @@ var edx = edx || {}; fieldTpl: $('#form_field-tpl').html(), events: { - 'click .js-register': 'submitForm' + 'click .js-register': 'submitForm', + 'click .login-provider': 'thirdPartyAuth' }, errors: [], $form: {}, - initialize: function() { + initialize: function( thirdPartyAuthInfo ) { this.tpl = $(this.tpl).html(); + + this.providers = thirdPartyAuthInfo.providers || []; + this.currentProvider = thirdPartyAuthInfo.currentProvider || ""; this.getInitialData(); }, @@ -33,7 +37,9 @@ var edx = edx || {}; var fields = html || ''; $(this.el).html( _.template( this.tpl, { - fields: fields + fields: fields, + currentProvider: this.currentProvider, + providers: this.providers })); this.postRender(); @@ -81,8 +87,10 @@ var edx = edx || {}; i, len = data.length, fieldTpl = this.fieldTpl; -console.log('buildForm ', data); for ( i=0; i <%! from django.template import RequestContext %> +<%! import third_party_auth %> <%! from third_party_auth import pipeline %> <%! from microsite_configuration import microsite %> @@ -95,7 +96,7 @@ <%include file='dashboard/_dashboard_info_language.html' /> %endif - % if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')): + % if third_party_auth.is_enabled():
  • ## Translators: this section lists all the third-party authentication providers (for example, Google and LinkedIn) the user can link with or unlink from their edX account. diff --git a/lms/templates/login.html b/lms/templates/login.html index 6e4267f836..e217496e3a 100644 --- a/lms/templates/login.html +++ b/lms/templates/login.html @@ -4,6 +4,7 @@ <%! from django.core.urlresolvers import reverse %> <%! from django.utils.translation import ugettext as _ %> +<%! import third_party_auth %> <%! from third_party_auth import provider, pipeline %> <%block name="pagetitle">${_("Log into your {platform_name} Account").format(platform_name=platform_name)} @@ -48,8 +49,16 @@ $('#login-form').on('ajax:error', function(event, request, status_string) { toggleSubmitButton(true); - $('.third-party-signin.message').addClass('is-shown').focus(); - $('.third-party-signin.message .instructions').html(request.responseText); + + if (request.status === 401) { + $('.message.submission-error').removeClass('is-shown'); + $('.third-party-signin.message').addClass('is-shown').focus(); + $('.third-party-signin.message .instructions').html(request.responseText); + } else { + $('.third-party-signin.message').removeClass('is-shown'); + $('.message.submission-error').addClass('is-shown').focus(); + $('.message.submission-error').html(gettext("Your request could not be completed. Please try again.")); + } }); $('#login-form').on('ajax:success', function(event, json, xhr) { @@ -194,7 +203,7 @@ - % if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')): + % if third_party_auth.is_enabled(): ## Developers: this is a sentence fragment, which is usually frowned upon. The design of the pags uses this fragment to provide an "else" clause underneath a number of choices. It's OK to leave it. diff --git a/lms/templates/register.html b/lms/templates/register.html index 13cb548f61..3aed605226 100644 --- a/lms/templates/register.html +++ b/lms/templates/register.html @@ -12,6 +12,7 @@ <%! from django.utils.translation import ugettext as _ %> <%! from student.models import UserProfile %> <%! from datetime import date %> +<%! import third_party_auth %> <%! from third_party_auth import pipeline, provider %> <%! import calendar %> @@ -116,7 +117,7 @@
    - % if microsite.get_value('ENABLE_THIRD_PARTY_AUTH', settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH')): + % if third_party_auth.is_enabled(): % if not running_pipeline: @@ -182,7 +183,7 @@ ${_('Will be shown in any discussions or forums you participate in')} (${_('cannot be changed later')})
    • - % if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and running_pipeline: + % if third_party_auth.is_enabled() and running_pipeline: