From dfcef9ddcb6db419c910b111064e8eab74c59f36 Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 17 Mar 2015 15:00:33 -0400 Subject: [PATCH] MA-95: Update Registration endpoint to support 3rd party credentials Automatically link edX account with social account in login mobile API path --- common/djangoapps/oauth_exchange/forms.py | 2 +- .../oauth_exchange/tests/test_forms.py | 11 +- .../oauth_exchange/tests/test_views.py | 11 +- .../djangoapps/oauth_exchange/tests/utils.py | 66 ++----- common/djangoapps/student/tests/test_login.py | 55 ++---- common/djangoapps/student/views.py | 50 ++++- .../djangoapps/third_party_auth/pipeline.py | 157 ++++++++-------- .../djangoapps/third_party_auth/settings.py | 1 + .../tests/test_change_enrollment.py | 4 +- .../third_party_auth/tests/utils.py | 79 ++++++++ lms/urls.py | 2 + .../djangoapps/user_api/tests/test_views.py | 171 ++++++++++++++++-- 12 files changed, 404 insertions(+), 205 deletions(-) create mode 100644 common/djangoapps/third_party_auth/tests/utils.py diff --git a/common/djangoapps/oauth_exchange/forms.py b/common/djangoapps/oauth_exchange/forms.py index 36051684e8..772de6d156 100644 --- a/common/djangoapps/oauth_exchange/forms.py +++ b/common/djangoapps/oauth_exchange/forms.py @@ -57,7 +57,7 @@ class AccessTokenExchangeForm(ScopeMixin, OAuthForm): } ) - self.request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_API + self.request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_LOGIN_API client_id = self.cleaned_data["client_id"] try: diff --git a/common/djangoapps/oauth_exchange/tests/test_forms.py b/common/djangoapps/oauth_exchange/tests/test_forms.py index 8bf3b6d427..392935c6ab 100644 --- a/common/djangoapps/oauth_exchange/tests/test_forms.py +++ b/common/djangoapps/oauth_exchange/tests/test_forms.py @@ -12,11 +12,8 @@ from provider import scope import social.apps.django_app.utils as social_utils from oauth_exchange.forms import AccessTokenExchangeForm -from oauth_exchange.tests.utils import ( - AccessTokenExchangeTestMixin, - AccessTokenExchangeMixinFacebook, - AccessTokenExchangeMixinGoogle -) +from oauth_exchange.tests.utils import AccessTokenExchangeTestMixin +from third_party_auth.tests.utils import ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle class AccessTokenExchangeFormTest(AccessTokenExchangeTestMixin): @@ -50,7 +47,7 @@ class AccessTokenExchangeFormTest(AccessTokenExchangeTestMixin): @httpretty.activate class AccessTokenExchangeFormTestFacebook( AccessTokenExchangeFormTest, - AccessTokenExchangeMixinFacebook, + ThirdPartyOAuthTestMixinFacebook, TestCase ): """ @@ -64,7 +61,7 @@ class AccessTokenExchangeFormTestFacebook( @httpretty.activate class AccessTokenExchangeFormTestGoogle( AccessTokenExchangeFormTest, - AccessTokenExchangeMixinGoogle, + ThirdPartyOAuthTestMixinGoogle, TestCase ): """ diff --git a/common/djangoapps/oauth_exchange/tests/test_views.py b/common/djangoapps/oauth_exchange/tests/test_views.py index 14e49c1a8b..d58aa6edc4 100644 --- a/common/djangoapps/oauth_exchange/tests/test_views.py +++ b/common/djangoapps/oauth_exchange/tests/test_views.py @@ -14,11 +14,8 @@ import provider.constants from provider import scope from provider.oauth2.models import AccessToken -from oauth_exchange.tests.utils import ( - AccessTokenExchangeTestMixin, - AccessTokenExchangeMixinFacebook, - AccessTokenExchangeMixinGoogle -) +from oauth_exchange.tests.utils import AccessTokenExchangeTestMixin +from third_party_auth.tests.utils import ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin): @@ -95,7 +92,7 @@ class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin): @httpretty.activate class AccessTokenExchangeViewTestFacebook( AccessTokenExchangeViewTest, - AccessTokenExchangeMixinFacebook, + ThirdPartyOAuthTestMixinFacebook, TestCase ): """ @@ -109,7 +106,7 @@ class AccessTokenExchangeViewTestFacebook( @httpretty.activate class AccessTokenExchangeViewTestGoogle( AccessTokenExchangeViewTest, - AccessTokenExchangeMixinGoogle, + ThirdPartyOAuthTestMixinGoogle, TestCase ): """ diff --git a/common/djangoapps/oauth_exchange/tests/utils.py b/common/djangoapps/oauth_exchange/tests/utils.py index 3ac278e956..74cb02604b 100644 --- a/common/djangoapps/oauth_exchange/tests/utils.py +++ b/common/djangoapps/oauth_exchange/tests/utils.py @@ -1,17 +1,13 @@ """ Test utilities for OAuth access token exchange """ -import json - -import httpretty import provider.constants -from provider.oauth2.models import Client from social.apps.django_app.default.models import UserSocialAuth -from student.tests.factories import UserFactory +from third_party_auth.tests.utils import ThirdPartyOAuthTestMixin -class AccessTokenExchangeTestMixin(object): +class AccessTokenExchangeTestMixin(ThirdPartyOAuthTestMixin): """ A mixin to define test cases for access token exchange. The following methods must be implemented by subclasses: @@ -21,40 +17,12 @@ class AccessTokenExchangeTestMixin(object): def setUp(self): super(AccessTokenExchangeTestMixin, self).setUp() - self.client_id = "test_client_id" - self.oauth_client = Client.objects.create( - client_id=self.client_id, - client_type=provider.constants.PUBLIC - ) - self.social_uid = "test_social_uid" - self.user = UserFactory() - UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid) - self.access_token = "test_access_token" # Initialize to minimal data self.data = { "access_token": self.access_token, "client_id": self.client_id, } - def _setup_provider_response(self, success): - """ - Register a mock response for the third party user information endpoint; - success indicates whether the response status code should be 200 or 400 - """ - if success: - status = 200 - body = json.dumps({self.UID_FIELD: self.social_uid}) - else: - status = 400 - body = json.dumps({}) - httpretty.register_uri( - httpretty.GET, - self.USER_URL, - body=body, - status=status, - content_type="application/json" - ) - def _assert_error(self, _data, _expected_error, _expected_error_description): """ Given request data, execute a test and check that the expected error @@ -101,6 +69,12 @@ class AccessTokenExchangeTestMixin(object): "test_client_id is not a public client" ) + def test_inactive_user(self): + self.user.is_active = False + self.user.save() # pylint: disable=no-member + self._setup_provider_response(success=True) + self._assert_success(self.data, expected_scopes=[]) + def test_invalid_acess_token(self): self._setup_provider_response(success=False) self._assert_error(self.data, "invalid_grant", "access_token is not valid") @@ -110,18 +84,14 @@ class AccessTokenExchangeTestMixin(object): self._setup_provider_response(success=True) self._assert_error(self.data, "invalid_grant", "access_token is not valid") + def test_user_automatically_linked_by_email(self): + UserSocialAuth.objects.all().delete() + self._setup_provider_response(success=True, email=self.user.email) + self._assert_success(self.data, expected_scopes=[]) -class AccessTokenExchangeMixinFacebook(object): - """Tests access token exchange with the Facebook backend""" - BACKEND = "facebook" - USER_URL = "https://graph.facebook.com/me" - # In facebook responses, the "id" field is used as the user's identifier - UID_FIELD = "id" - - -class AccessTokenExchangeMixinGoogle(object): - """Tests access token exchange with the Google backend""" - BACKEND = "google-oauth2" - USER_URL = "https://www.googleapis.com/oauth2/v1/userinfo" - # In google-oauth2 responses, the "email" field is used as the user's identifier - UID_FIELD = "email" + def test_inactive_user_not_automatically_linked(self): + UserSocialAuth.objects.all().delete() + self._setup_provider_response(success=True, email=self.user.email) + self.user.is_active = False + self.user.save() # pylint: disable=no-member + self._assert_error(self.data, "invalid_grant", "access_token is not valid") diff --git a/common/djangoapps/student/tests/test_login.py b/common/djangoapps/student/tests/test_login.py index e70b5ccac7..bbbe83028c 100644 --- a/common/djangoapps/student/tests/test_login.py +++ b/common/djangoapps/student/tests/test_login.py @@ -6,21 +6,24 @@ import unittest from django.test import TestCase from django.test.client import Client -from django.test.utils import override_settings from django.conf import settings from django.core.cache import cache from django.core.urlresolvers import reverse, NoReverseMatch from django.http import HttpResponseBadRequest, HttpResponse -from external_auth.models import ExternalAuthMap import httpretty from mock import patch from social.apps.django_app.default.models import UserSocialAuth +from external_auth.models import ExternalAuthMap from student.tests.factories import UserFactory, RegistrationFactory, UserProfileFactory from student.views import login_oauth_token - +from third_party_auth.tests.utils import ( + ThirdPartyOAuthTestMixin, + ThirdPartyOAuthTestMixinFacebook, + ThirdPartyOAuthTestMixinGoogle +) from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, TEST_DATA_MOCK_MODULESTORE +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase class LoginTest(TestCase): @@ -438,7 +441,7 @@ class ExternalAuthShibTest(ModuleStoreTestCase): @httpretty.activate -class LoginOAuthTokenMixin(object): +class LoginOAuthTokenMixin(ThirdPartyOAuthTestMixin): """ Mixin with tests for the login_oauth_token view. A TestCase that includes this must define the following: @@ -449,30 +452,8 @@ class LoginOAuthTokenMixin(object): """ def setUp(self): - self.client = Client() + super(LoginOAuthTokenMixin, self).setUp() self.url = reverse(login_oauth_token, kwargs={"backend": self.BACKEND}) - self.social_uid = "social_uid" - self.user = UserFactory() - UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid) - - def _setup_user_response(self, success): - """ - Register a mock response for the third party user information endpoint; - success indicates whether the response status code should be 200 or 400 - """ - if success: - status = 200 - body = json.dumps({self.UID_FIELD: self.social_uid}) - else: - status = 400 - body = json.dumps({}) - httpretty.register_uri( - httpretty.GET, - self.USER_URL, - body=body, - status=status, - content_type="application/json" - ) def _assert_error(self, response, status_code, error): """Assert that the given response was a 400 with the given error code""" @@ -481,13 +462,13 @@ class LoginOAuthTokenMixin(object): self.assertNotIn("partial_pipeline", self.client.session) def test_success(self): - self._setup_user_response(success=True) + self._setup_provider_response(success=True) response = self.client.post(self.url, {"access_token": "dummy"}) self.assertEqual(response.status_code, 204) self.assertEqual(self.client.session['_auth_user_id'], self.user.id) # pylint: disable=no-member def test_invalid_token(self): - self._setup_user_response(success=False) + self._setup_provider_response(success=False) response = self.client.post(self.url, {"access_token": "dummy"}) self._assert_error(response, 401, "invalid_token") @@ -497,7 +478,7 @@ class LoginOAuthTokenMixin(object): def test_unlinked_user(self): UserSocialAuth.objects.all().delete() - self._setup_user_response(success=True) + self._setup_provider_response(success=True) response = self.client.post(self.url, {"access_token": "dummy"}) self._assert_error(response, 401, "invalid_token") @@ -508,17 +489,13 @@ class LoginOAuthTokenMixin(object): # This is necessary because cms does not implement third party auth @unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled") -class LoginOAuthTokenTestFacebook(LoginOAuthTokenMixin, TestCase): +class LoginOAuthTokenTestFacebook(LoginOAuthTokenMixin, ThirdPartyOAuthTestMixinFacebook, TestCase): """Tests login_oauth_token with the Facebook backend""" - BACKEND = "facebook" - USER_URL = "https://graph.facebook.com/me" - UID_FIELD = "id" + pass # This is necessary because cms does not implement third party auth @unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled") -class LoginOAuthTokenTestGoogle(LoginOAuthTokenMixin, TestCase): +class LoginOAuthTokenTestGoogle(LoginOAuthTokenMixin, ThirdPartyOAuthTestMixinGoogle, TestCase): """Tests login_oauth_token with the Google backend""" - BACKEND = "google-oauth2" - USER_URL = "https://www.googleapis.com/oauth2/v1/userinfo" - UID_FIELD = "email" + pass diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 4521d9fb00..6c2816b716 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -3,10 +3,10 @@ Student Views """ import datetime import logging -import re import uuid import time import json +import warnings from collections import defaultdict from pytz import UTC from ipware.ip import get_ip @@ -44,9 +44,9 @@ from requests import HTTPError from social.apps.django_app import utils as social_utils from social.backends import oauth as social_oauth +from social.exceptions import AuthException, AuthAlreadyAssociated from edxmako.shortcuts import render_to_response, render_to_string -from mako.exceptions import TopLevelLookupException from course_modes.models import CourseMode from shoppingcart.api import order_history @@ -1171,11 +1171,13 @@ def login_oauth_token(request, backend): retrieve information from a third party and matching that information to an existing user. """ + warnings.warn("Please use AccessTokenExchangeView instead.", DeprecationWarning) + backend = request.social_strategy.backend if isinstance(backend, social_oauth.BaseOAuth1) or isinstance(backend, social_oauth.BaseOAuth2): if "access_token" in request.POST: # Tell third party auth pipeline that this is an API call - request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_API + request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_LOGIN_API user = None try: user = backend.do_auth(request.POST["access_token"]) @@ -1423,7 +1425,14 @@ def create_account_with_params(request, params): getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) ) - if third_party_auth.is_enabled() and pipeline.running(request): + # Boolean of whether a 3rd party auth provider and credentials were provided in + # the API so the newly created account can link with the 3rd party account. + # + # Note: this is orthogonal to the 3rd party authentication pipeline that occurs + # when the account is created via the browser and redirect URLs. + should_link_with_social_auth = third_party_auth.is_enabled() and 'provider' in params + + if should_link_with_social_auth or (third_party_auth.is_enabled() and pipeline.running(request)): params["password"] = pipeline.make_random_password() # if doing signup for an external authorization, then get email, password, name from the eamap @@ -1464,13 +1473,38 @@ def create_account_with_params(request, params): extended_profile_fields=extended_profile_fields, enforce_username_neq_password=True, enforce_password_policy=enforce_password_policy, - tos_required=tos_required + tos_required=tos_required, ) with transaction.commit_on_success(): - ret = _do_create_account(form) + # first, create the account + (user, profile, registration) = _do_create_account(form) - (user, profile, registration) = ret + # next, link the account with social auth, if provided + if should_link_with_social_auth: + request.social_strategy = social_utils.load_strategy(backend=params['provider'], request=request) + social_access_token = params.get('access_token') + if not social_access_token: + raise ValidationError({ + 'access_token': [ + _("An access_token is required when passing value ({}) for provider.").format( + params['provider'] + ) + ] + }) + request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_REGISTER_API + pipeline_user = None + error_message = "" + try: + pipeline_user = request.social_strategy.backend.do_auth(social_access_token, user=user) + except AuthAlreadyAssociated: + error_message = _("The provided access_token is already associated with another user.") + except (HTTPError, AuthException): + error_message = _("The provided access_token is not valid.") + if not pipeline_user or not isinstance(pipeline_user, User): + # Ensure user does not re-enter the pipeline + request.social_strategy.clean_partial_pipeline() + raise ValidationError({'access_token': [error_message]}) if settings.FEATURES.get('ENABLE_DISCUSSION_EMAIL_DIGEST'): try: @@ -1604,6 +1638,8 @@ def create_account(request, post_override=None): JSON call to create new edX account. Used by form in signup_modal.html, which is included into navigation.html """ + warnings.warn("Please use RegistrationView instead.", DeprecationWarning) + try: create_account_with_params(request, post_override or request.POST) except AccountValidationError as exc: diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 97c905fac0..d1df35a9f0 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -72,6 +72,7 @@ from django.shortcuts import redirect from social.apps.django_app.default import models from social.exceptions import AuthException from social.pipeline import partial +from social.pipeline.social_auth import associate_by_email import student from embargo import api as embargo_api @@ -108,6 +109,8 @@ AUTH_REDIRECT_KEY = 'next' AUTH_ENROLL_COURSE_ID_KEY = 'enroll_course_id' AUTH_EMAIL_OPT_IN_KEY = 'email_opt_in' + +# The following are various possible values for the AUTH_ENTRY_KEY. AUTH_ENTRY_DASHBOARD = 'dashboard' AUTH_ENTRY_LOGIN = 'login' AUTH_ENTRY_REGISTER = 'register' @@ -119,7 +122,14 @@ AUTH_ENTRY_REGISTER = 'register' AUTH_ENTRY_LOGIN_2 = 'account_login' AUTH_ENTRY_REGISTER_2 = 'account_register' -AUTH_ENTRY_API = 'api' +# Entry modes into the authentication process by a remote API call (as opposed to a browser session). +AUTH_ENTRY_LOGIN_API = 'login_api' +AUTH_ENTRY_REGISTER_API = 'register_api' + + +def is_api(auth_entry): + """Returns whether the auth entry point is via an API call.""" + return (auth_entry == AUTH_ENTRY_LOGIN_API) or (auth_entry == AUTH_ENTRY_REGISTER_API) # URLs associated with auth entry points # These are used to request additional user information @@ -154,7 +164,8 @@ _AUTH_ENTRY_CHOICES = frozenset([ AUTH_ENTRY_LOGIN_2, AUTH_ENTRY_REGISTER_2, - AUTH_ENTRY_API, + AUTH_ENTRY_LOGIN_API, + AUTH_ENTRY_REGISTER_API, ]) _DEFAULT_RANDOM_PASSWORD_LENGTH = 12 @@ -433,39 +444,11 @@ def parse_query_params(strategy, response, *args, **kwargs): if not (auth_entry and auth_entry in _AUTH_ENTRY_CHOICES): raise AuthEntryError(strategy.backend, 'auth_entry missing or invalid') - # Note: We expect only one member of this dictionary to be `True` at any - # given time. If something changes this convention in the future, please look - # at the `login_analytics` function in this file as well to ensure logging - # is still done properly - return { - # 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 in [AUTH_ENTRY_LOGIN, AUTH_ENTRY_LOGIN_2], - # Whether the auth pipeline entered from /register. - 'is_register': auth_entry in [AUTH_ENTRY_REGISTER, AUTH_ENTRY_REGISTER_2], - # Whether the auth pipeline entered from an API - 'is_api': auth_entry == AUTH_ENTRY_API, - } + return {'auth_entry': auth_entry} @partial.partial -def ensure_user_information( - strategy, - details, - response, - uid, - is_dashboard=None, - is_login=None, - is_profile=None, - is_register=None, - is_login_2=None, - is_register_2=None, - is_api=None, - user=None, - *args, - **kwargs -): +def ensure_user_information(strategy, auth_entry, user=None, *args, **kwargs): """ Ensure that we have the necessary information about a user (either an existing account or registration data) to proceed with the pipeline. @@ -482,33 +465,33 @@ def ensure_user_information( # 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 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() - - 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: + def dispatch_to_login(): + """Redirects to the login page.""" return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_LOGIN], strategy)) - if dispatch_to_register: + def dispatch_to_register(): + """Redirects to the registration page.""" return redirect(_create_redirect_url(AUTH_DISPATCH_URLS[AUTH_ENTRY_REGISTER], strategy)) + user_inactive = user and not user.is_active + + if auth_entry in [AUTH_ENTRY_LOGIN_API, AUTH_ENTRY_REGISTER_API]: + if not user: + return HttpResponseBadRequest() + + elif auth_entry in [AUTH_ENTRY_LOGIN, AUTH_ENTRY_LOGIN_2]: + if not user or user_inactive: + return dispatch_to_login() + + elif auth_entry in [AUTH_ENTRY_REGISTER, AUTH_ENTRY_REGISTER_2]: + if not user: + return dispatch_to_register() + elif user_inactive: + # 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. + return dispatch_to_login() + def _create_redirect_url(url, strategy): """ Given a URL and a Strategy, construct the appropriate redirect URL. @@ -540,7 +523,7 @@ def _create_redirect_url(url, strategy): @partial.partial -def set_logged_in_cookie(backend=None, user=None, request=None, is_api=None, *args, **kwargs): +def set_logged_in_cookie(backend=None, user=None, request=None, auth_entry=None, *args, **kwargs): """This pipeline step sets the "logged in" cookie for authenticated users. Some installations have a marketing site front-end separate from @@ -565,7 +548,7 @@ def set_logged_in_cookie(backend=None, user=None, request=None, is_api=None, *ar to the next pipeline step. """ - if user is not None and user.is_authenticated() and not is_api: + if not is_api(auth_entry) and user is not None and user.is_authenticated(): if request is not None: # Check that the cookie isn't already set. # This ensures that we allow the user to continue to the next @@ -585,27 +568,14 @@ def set_logged_in_cookie(backend=None, user=None, request=None, is_api=None, *ar @partial.partial -def login_analytics(strategy, *args, **kwargs): +def login_analytics(strategy, auth_entry, *args, **kwargs): """ Sends login info to Segment.io """ + event_name = None - - action_to_event_name = { - '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 - # `True` at any given time - for action in action_to_event_name.keys(): - if kwargs.get(action): - event_name = action_to_event_name[action] + if auth_entry in [AUTH_ENTRY_LOGIN, AUTH_ENTRY_LOGIN_2]: + event_name = 'edx.bi.user.account.authenticated' + elif auth_entry in [AUTH_ENTRY_DASHBOARD]: + event_name = 'edx.bi.user.account.linked' if event_name is not None: tracking_context = tracker.get_tracker().resolve_context() @@ -626,7 +596,7 @@ def login_analytics(strategy, *args, **kwargs): @partial.partial -def change_enrollment(strategy, user=None, is_dashboard=False, *args, **kwargs): +def change_enrollment(strategy, auth_entry=None, user=None, *args, **kwargs): """Enroll a user in a course. If a user entered the authentication flow when trying to enroll @@ -646,10 +616,8 @@ def change_enrollment(strategy, user=None, is_dashboard=False, *args, **kwargs): (configured using the ?next parameter to the third party auth login url). Keyword Arguments: + auth_entry: The entry mode into the pipeline. user (User): The user being authenticated. - is_dashboard (boolean): Whether the user entered the authentication - pipeline from the "link account" button on the student dashboard. - """ # We skip enrollment if the user entered the flow from the "link account" # button on the student dashboard. At this point, either: @@ -662,7 +630,7 @@ def change_enrollment(strategy, user=None, is_dashboard=False, *args, **kwargs): # args when sending users to this page, successfully authenticating through this page # would also enroll the student in the course. enroll_course_id = strategy.session_get('enroll_course_id') - if enroll_course_id and not is_dashboard: + if enroll_course_id and auth_entry != AUTH_ENTRY_DASHBOARD: course_id = CourseKey.from_string(enroll_course_id) modes = CourseMode.modes_for_course_dict(course_id) @@ -712,11 +680,34 @@ def change_enrollment(strategy, user=None, is_dashboard=False, *args, **kwargs): except ( CourseDoesNotExistException, ItemAlreadyInCartException, - AlreadyEnrolledInCourseException + AlreadyEnrolledInCourseException, ): pass # It's more important to complete login than to # ensure that the course was added to the shopping cart. # Log errors, but don't stop the authentication pipeline. - except Exception as ex: + except Exception as ex: # pylint: disable=broad-except logger.exception(ex) + + +@partial.partial +def associate_by_email_if_login_api(auth_entry, strategy, details, user, *args, **kwargs): + """ + This pipeline step associates the current social auth with the user with the + same email address in the database. It defers to the social library's associate_by_email + implementation, which verifies that only a single database user is associated with the email. + + This association is done ONLY if the user entered the pipeline through a LOGIN API. + """ + if auth_entry == AUTH_ENTRY_LOGIN_API: + association_response = associate_by_email(strategy, details, user, *args, **kwargs) + if ( + association_response and + association_response.get('user') and + association_response['user'].is_active + ): + # Only return the user matched by email if their email has been activated. + # Otherwise, an illegitimate user can create an account with another user's + # email address and the legitimate user would now login to the illegitimate + # account. + return association_response diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 9bba287743..b30076579d 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -103,6 +103,7 @@ def _set_global_settings(django_settings): 'social.pipeline.social_auth.social_uid', 'social.pipeline.social_auth.auth_allowed', 'social.pipeline.social_auth.social_user', + 'third_party_auth.pipeline.associate_by_email_if_login_api', 'social.pipeline.user.get_username', 'third_party_auth.pipeline.ensure_user_information', 'social.pipeline.user.create_user', diff --git a/common/djangoapps/third_party_auth/tests/test_change_enrollment.py b/common/djangoapps/third_party_auth/tests/test_change_enrollment.py index 48f08b48e3..caf71e5da1 100644 --- a/common/djangoapps/third_party_auth/tests/test_change_enrollment.py +++ b/common/djangoapps/third_party_auth/tests/test_change_enrollment.py @@ -147,7 +147,7 @@ class PipelineEnrollmentTest(UrlResetMixin, ModuleStoreTestCase): # Simulate completing the pipeline from the student dashboard's # "link account" button. - result = pipeline.change_enrollment(strategy, 1, user=self.user, is_dashboard=True) # pylint: disable=assignment-from-no-return,redundant-keyword-arg + result = pipeline.change_enrollment(strategy, 1, user=self.user, auth_entry=pipeline.AUTH_ENTRY_DASHBOARD) # pylint: disable=assignment-from-no-return,redundant-keyword-arg # Verify that we were NOT enrolled self.assertEqual(result, {}) @@ -165,7 +165,7 @@ class PipelineEnrollmentTest(UrlResetMixin, ModuleStoreTestCase): details=None, response=None, uid=None, - is_register=True, + auth_entry=pipeline.AUTH_ENTRY_REGISTER, backend=backend ) self.assertIsNotNone(response) diff --git a/common/djangoapps/third_party_auth/tests/utils.py b/common/djangoapps/third_party_auth/tests/utils.py new file mode 100644 index 0000000000..6dcc2662cd --- /dev/null +++ b/common/djangoapps/third_party_auth/tests/utils.py @@ -0,0 +1,79 @@ +"""Common utility for testing third party oauth2 features.""" +import json + +import httpretty + +from provider.constants import PUBLIC +from provider.oauth2.models import Client +from social.apps.django_app.default.models import UserSocialAuth + +from student.tests.factories import UserFactory + + +@httpretty.activate +class ThirdPartyOAuthTestMixin(object): + """ + Mixin with tests for third party oauth views. A TestCase that includes + this must define the following: + + BACKEND: The name of the backend from python-social-auth + USER_URL: The URL of the endpoint that the backend retrieves user data from + UID_FIELD: The field in the user data that the backend uses as the user id + """ + def setUp(self, create_user=True): + super(ThirdPartyOAuthTestMixin, self).setUp() + self.social_uid = "test_social_uid" + self.access_token = "test_access_token" + self.client_id = "test_client_id" + self.oauth_client = Client.objects.create( + client_id=self.client_id, + client_type=PUBLIC + ) + if create_user: + self.user = UserFactory() + UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid) + + def _setup_provider_response(self, success=False, email=''): + """ + Register a mock response for the third party user information endpoint; + success indicates whether the response status code should be 200 or 400 + """ + if success: + status = 200 + response = {self.UID_FIELD: self.social_uid} + if email: + response.update({'email': email}) + body = json.dumps(response) + else: + status = 400 + body = json.dumps({}) + + self._setup_provider_response_with_body(status, body) + + def _setup_provider_response_with_body(self, status, body): + """ + Register a mock response for the third party user information endpoint with given status and body. + """ + httpretty.register_uri( + httpretty.GET, + self.USER_URL, + body=body, + status=status, + content_type="application/json" + ) + + +class ThirdPartyOAuthTestMixinFacebook(object): + """Tests oauth with the Facebook backend""" + BACKEND = "facebook" + USER_URL = "https://graph.facebook.com/me" + # In facebook responses, the "id" field is used as the user's identifier + UID_FIELD = "id" + + +class ThirdPartyOAuthTestMixinGoogle(object): + """Tests oauth with the Google backend""" + BACKEND = "google-oauth2" + USER_URL = "https://www.googleapis.com/oauth2/v1/userinfo" + # In google-oauth2 responses, the "email" field is used as the user's identifier + UID_FIELD = "email" diff --git a/lms/urls.py b/lms/urls.py index 844e0d249d..b0dd1a0d6f 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -595,6 +595,8 @@ if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'): oauth_exchange.views.AccessTokenExchangeView.as_view(), name="exchange_access_token" ), + # NOTE: The following login_oauth_token endpoint is DEPRECATED. + # Please use the exchange_access_token endpoint instead. url(r'^login_oauth_token/(?P[^/]+)/$', 'student.views.login_oauth_token'), ) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index e4f2bbe69d..57be600155 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -4,26 +4,33 @@ import datetime import base64 import json import re +from unittest import skipUnless, SkipTest + +import ddt +import httpretty +from pytz import UTC +import mock from django.conf import settings from django.core.urlresolvers import reverse from django.core import mail from django.contrib.auth.models import User from django.test import TestCase +from django.test.testcases import TransactionTestCase from django.test.utils import override_settings -from unittest import skipUnless -import ddt -from pytz import UTC -import mock + +from social.apps.django_app.default.models import UserSocialAuth + +from opaque_keys.edx.locations import SlashSeparatedCourseKey + +from django_comment_common import models +from student.tests.factories import UserFactory +from third_party_auth.tests.testutil import simulate_running_pipeline +from third_party_auth.tests.utils import ( + ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle +) from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase - -from student.tests.factories import UserFactory -from unittest import SkipTest -from django_comment_common import models -from opaque_keys.edx.locations import SlashSeparatedCourseKey -from third_party_auth.tests.testutil import simulate_running_pipeline - from ..accounts.api import get_account_settings from ..api import account as account_api, profile as profile_api from ..models import UserOrgTag @@ -1518,6 +1525,148 @@ class RegistrationViewTest(ApiTestCase): self.assertIn(expected_field, form_desc["fields"]) +@httpretty.activate +@ddt.ddt +class ThirdPartyRegistrationTestMixin(ThirdPartyOAuthTestMixin): + """ + Tests for the User API registration endpoint with 3rd party authentication. + """ + def setUp(self): + super(ThirdPartyRegistrationTestMixin, self).setUp(create_user=False) + self.url = reverse('user_api_registration') + + def data(self, user=None): + """Returns the request data for the endpoint.""" + return { + "provider": self.BACKEND, + "access_token": self.access_token, + "client_id": self.client_id, + "honor_code": "true", + "country": "US", + "username": user.username if user else "test_username", + "name": user.first_name if user else "test name", + "email": user.email if user else "test@test.com", + } + + def _assert_existing_user_error(self, response): + """Assert that the given response was an error with the given status_code and error code.""" + self.assertEqual(response.status_code, 409) + errors = json.loads(response.content) + for conflict_attribute in ["username", "email"]: + self.assertIn(conflict_attribute, errors) + self.assertIn("belongs to an existing account", errors[conflict_attribute][0]["user_message"]) + self.assertNotIn("partial_pipeline", self.client.session) + + def _assert_access_token_error(self, response, expected_error_message): + """Assert that the given response was an error for the access_token field with the given error message.""" + self.assertEqual(response.status_code, 400) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + {"access_token": [{"user_message": expected_error_message}]} + ) + self.assertNotIn("partial_pipeline", self.client.session) + + def _verify_user_existence(self, user_exists, social_link_exists, user_is_active=None, username=None): + """Verifies whether the user object exists.""" + users = User.objects.filter(username=(username if username else "test_username")) + self.assertEquals(users.exists(), user_exists) + if user_exists: + self.assertEquals(users[0].is_active, user_is_active) + self.assertEqual( + UserSocialAuth.objects.filter(user=users[0], provider=self.BACKEND).exists(), + social_link_exists + ) + else: + self.assertEquals(UserSocialAuth.objects.count(), 0) + + def test_success(self): + self._verify_user_existence(user_exists=False, social_link_exists=False) + + self._setup_provider_response(success=True) + response = self.client.post(self.url, self.data()) + self.assertEqual(response.status_code, 200) + + self._verify_user_existence(user_exists=True, social_link_exists=True, user_is_active=False) + + def test_unlinked_active_user(self): + user = UserFactory() + response = self.client.post(self.url, self.data(user)) + self._assert_existing_user_error(response) + self._verify_user_existence( + user_exists=True, social_link_exists=False, user_is_active=True, username=user.username + ) + + def test_unlinked_inactive_user(self): + user = UserFactory(is_active=False) + response = self.client.post(self.url, self.data(user)) + self._assert_existing_user_error(response) + self._verify_user_existence( + user_exists=True, social_link_exists=False, user_is_active=False, username=user.username + ) + + def test_user_already_registered(self): + self._setup_provider_response(success=True) + user = UserFactory() + UserSocialAuth.objects.create(user=user, provider=self.BACKEND, uid=self.social_uid) + response = self.client.post(self.url, self.data(user)) + self._assert_existing_user_error(response) + self._verify_user_existence( + user_exists=True, social_link_exists=True, user_is_active=True, username=user.username + ) + + def test_social_user_conflict(self): + self._setup_provider_response(success=True) + user = UserFactory() + UserSocialAuth.objects.create(user=user, provider=self.BACKEND, uid=self.social_uid) + response = self.client.post(self.url, self.data()) + self._assert_access_token_error(response, "The provided access_token is already associated with another user.") + self._verify_user_existence( + user_exists=True, social_link_exists=True, user_is_active=True, username=user.username + ) + + def test_invalid_token(self): + self._setup_provider_response(success=False) + response = self.client.post(self.url, self.data()) + self._assert_access_token_error(response, "The provided access_token is not valid.") + self._verify_user_existence(user_exists=False, social_link_exists=False) + + def test_missing_token(self): + data = self.data() + data.pop("access_token") + response = self.client.post(self.url, data) + self._assert_access_token_error( + response, + "An access_token is required when passing value ({}) for provider.".format(self.BACKEND) + ) + self._verify_user_existence(user_exists=False, social_link_exists=False) + + +@skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled") +class TestFacebookRegistrationView( + ThirdPartyRegistrationTestMixin, ThirdPartyOAuthTestMixinFacebook, TransactionTestCase +): + """Tests the User API registration endpoint with Facebook authentication.""" + def test_social_auth_exception(self): + """ + According to the do_auth method in social.backends.facebook.py, + the Facebook API sometimes responds back a JSON with just False as value. + """ + self._setup_provider_response_with_body(200, json.dumps("false")) + response = self.client.post(self.url, self.data()) + self._assert_access_token_error(response, "The provided access_token is not valid.") + self._verify_user_existence(user_exists=False, social_link_exists=False) + + + +@skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled") +class TestGoogleRegistrationView( + ThirdPartyRegistrationTestMixin, ThirdPartyOAuthTestMixinGoogle, TransactionTestCase +): + """Tests the User API registration endpoint with Google authentication.""" + pass + + @ddt.ddt class UpdateEmailOptInTestCase(ApiTestCase, ModuleStoreTestCase): """Tests the UpdateEmailOptInPreference view. """