From ee09c4ccf953355a4951baeaef77efe4b2634b1d Mon Sep 17 00:00:00 2001 From: Uman Shahzad Date: Tue, 21 Nov 2017 18:31:15 +0500 Subject: [PATCH] Add a TPA pipeline step to allow force sync of user details. The feature is controlled by a switch on the provider. Emails are sent if the email changes during the sync. We skip syncing the username/email if there would be a conflict. --- .../0013_sync_learner_profile_data.py | 29 +++++ common/djangoapps/third_party_auth/models.py | 8 ++ .../djangoapps/third_party_auth/pipeline.py | 82 +++++++++++++- .../djangoapps/third_party_auth/settings.py | 1 + .../tests/test_pipeline_integration.py | 107 +++++++++++++++++- ...learner_profile_data_email_change_body.txt | 25 ++++ ...rner_profile_data_email_change_subject.txt | 6 + 7 files changed, 254 insertions(+), 4 deletions(-) create mode 100644 common/djangoapps/third_party_auth/migrations/0013_sync_learner_profile_data.py create mode 100644 common/templates/emails/sync_learner_profile_data_email_change_body.txt create mode 100644 common/templates/emails/sync_learner_profile_data_email_change_subject.txt diff --git a/common/djangoapps/third_party_auth/migrations/0013_sync_learner_profile_data.py b/common/djangoapps/third_party_auth/migrations/0013_sync_learner_profile_data.py new file mode 100644 index 0000000000..8c3ae928e5 --- /dev/null +++ b/common/djangoapps/third_party_auth/migrations/0013_sync_learner_profile_data.py @@ -0,0 +1,29 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('third_party_auth', '0012_auto_20170626_1135'), + ] + + operations = [ + migrations.AddField( + model_name='ltiproviderconfig', + name='sync_learner_profile_data', + field=models.BooleanField(default=False, help_text='Synchronize user profile data received from the identity provider with the edX user account on each SSO login. The user will be notified if the email address associated with their account is changed as a part of this synchronization.'), + ), + migrations.AddField( + model_name='oauth2providerconfig', + name='sync_learner_profile_data', + field=models.BooleanField(default=False, help_text='Synchronize user profile data received from the identity provider with the edX user account on each SSO login. The user will be notified if the email address associated with their account is changed as a part of this synchronization.'), + ), + migrations.AddField( + model_name='samlproviderconfig', + name='sync_learner_profile_data', + field=models.BooleanField(default=False, help_text='Synchronize user profile data received from the identity provider with the edX user account on each SSO login. The user will be notified if the email address associated with their account is changed as a part of this synchronization.'), + ), + ] diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 9dba1754ff..45925b3f74 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -181,6 +181,14 @@ class ProviderConfig(ConfigurationModel): "immediately after authenticating with the third party instead of the login page." ), ) + sync_learner_profile_data = models.BooleanField( + default=False, + help_text=_( + "Synchronize user profile data received from the identity provider with the edX user " + "account on each SSO login. The user will be notified if the email address associated " + "with their account is changed as a part of this synchronization." + ) + ) prefix = None # used for provider_id. Set to a string value in subclass backend_name = None # Set to a field or fixed value in subclass accepts_logins = True # Whether to display a sign-in button when the provider is enabled diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index d2dba1cb01..caaf9dc989 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -66,10 +66,12 @@ import string import urllib from collections import OrderedDict from logging import getLogger +from smtplib import SMTPException import analytics from django.conf import settings from django.contrib.auth.models import User +from django.core.mail.message import EmailMessage from django.core.urlresolvers import reverse from django.http import HttpResponseBadRequest from django.shortcuts import redirect @@ -79,7 +81,9 @@ from social_core.pipeline import partial from social_core.pipeline.social_auth import associate_by_email import student +from edxmako.shortcuts import render_to_string from eventtracking import tracker +from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from . import provider @@ -562,7 +566,7 @@ def ensure_user_information(strategy, auth_entry, backend=None, user=None, socia (current_provider.skip_email_verification or current_provider.send_to_registration_first)) if not user: - if auth_entry in [AUTH_ENTRY_LOGIN_API, AUTH_ENTRY_REGISTER_API]: + if is_api(auth_entry): return HttpResponseBadRequest() elif auth_entry == AUTH_ENTRY_LOGIN: # User has authenticated with the third party provider but we don't know which edX @@ -709,3 +713,79 @@ def associate_by_email_if_login_api(auth_entry, backend, details, user, current_ # email address and the legitimate user would now login to the illegitimate # account. return association_response + + +def user_details_force_sync(auth_entry, strategy, details, user=None, *args, **kwargs): + """ + Update normally protected user details using data from provider. + + This step in the pipeline is akin to `social_core.pipeline.user.user_details`, which updates + the user details but has an unconfigurable protection over updating the username & email, and + is unable to update information such as the user's full name which isn't on the user model, but + rather on the user profile model. + + Additionally, because the email field is normally used to log in, if the email is changed by this + forced synchronization, we send an email to both the old and new emails, letting the user know. + + This step is controlled by the `sync_learner_profile_data` flag on the provider's configuration. + """ + current_provider = provider.Registry.get_from_pipeline({'backend': strategy.request.backend.name, 'kwargs': kwargs}) + if user and current_provider.sync_learner_profile_data: + # Keep track of which incoming values get applied. + changed = {} + + # Map each incoming field from the provider to the name on the user model (by default, they always match). + field_mapping = {field: (user, field) for field in details.keys() if hasattr(user, field)} + + # This is a special case where the field mapping should go to the user profile object and not the user object, + # in some cases with differing field names (i.e. 'fullname' vs. 'name'). + field_mapping.update({ + 'fullname': (user.profile, 'name'), + 'country': (user.profile, 'country'), + }) + + # Track any fields that would raise an integrity error if there was a conflict. + integrity_conflict_fields = {'email': user.email, 'username': user.username} + + for provider_field, (model, field) in field_mapping.items(): + provider_value = details.get(provider_field) + current_value = getattr(model, field) + if provider_value is not None and current_value != provider_value: + if field in integrity_conflict_fields and User.objects.filter(**{field: provider_value}).exists(): + logger.warning('User with ID [%s] tried to synchronize profile data through [%s] ' + 'but there was a conflict with an existing [%s]: [%s].', + user.id, current_provider.name, field, provider_value) + continue + changed[provider_field] = current_value + setattr(model, field, provider_value) + + if changed: + logger.info( + "User [%s] performed SSO through [%s] who synchronizes profile data, and the " + "following fields were changed: %s", user.username, current_provider.name, changed.keys(), + ) + + # Save changes to user and user.profile models. + strategy.storage.user.changed(user) + user.profile.save() + + # Send an email to the old and new email to alert the user that their login email changed. + if changed.get('email'): + old_email = changed['email'] + new_email = user.email + email_context = {'old_email': old_email, 'new_email': new_email} + # Subjects shouldn't have new lines. + subject = ''.join(render_to_string( + 'emails/sync_learner_profile_data_email_change_subject.txt', + email_context + ).splitlines()) + body = render_to_string('emails/sync_learner_profile_data_email_change_body.txt', email_context) + from_email = configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL) + + email = EmailMessage(subject=subject, body=body, from_email=from_email, to=[old_email, new_email]) + email.content_subtype = "html" + try: + email.send() + except SMTPException: + logger.exception('Error sending IdP learner data sync-initiated email change ' + 'notification email for user [%s].', user.username) diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index 9b4389c70c..a97f0f0e0d 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -57,6 +57,7 @@ def apply_settings(django_settings): 'social_core.pipeline.social_auth.associate_user', 'social_core.pipeline.social_auth.load_extra_data', 'social_core.pipeline.user.user_details', + 'third_party_auth.pipeline.user_details_force_sync', 'third_party_auth.pipeline.set_logged_in_cookies', 'third_party_auth.pipeline.login_analytics', ] diff --git a/common/djangoapps/third_party_auth/tests/test_pipeline_integration.py b/common/djangoapps/third_party_auth/tests/test_pipeline_integration.py index b4a54c7f9b..953a6098bb 100644 --- a/common/djangoapps/third_party_auth/tests/test_pipeline_integration.py +++ b/common/djangoapps/third_party_auth/tests/test_pipeline_integration.py @@ -6,8 +6,10 @@ import mock import ddt from django import test from django.contrib.auth import models +from django.core import mail from social_django import models as social_models +from student.tests.factories import UserFactory from third_party_auth import pipeline, provider from third_party_auth.tests import testutil @@ -303,9 +305,6 @@ class TestPipelineUtilityFunctions(TestCase, test.TestCase): class EnsureUserInformationTestCase(testutil.TestCase, test.TestCase): """Tests ensuring that we have the necessary user information to proceed with the pipeline.""" - def setUp(self): - super(EnsureUserInformationTestCase, self).setUp() - @ddt.data( (True, '/register'), (False, '/login') @@ -335,3 +334,105 @@ class EnsureUserInformationTestCase(testutil.TestCase, test.TestCase): ) assert response.status_code == 302 assert response.url == expected_redirect_url + + +@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, testutil.AUTH_FEATURES_KEY + ' not enabled') +class UserDetailsForceSyncTestCase(testutil.TestCase, test.TestCase): + """Tests to ensure learner profile data is properly synced if the provider requires it.""" + + def setUp(self): + super(UserDetailsForceSyncTestCase, self).setUp() + # pylint: disable=attribute-defined-outside-init + self.user = UserFactory.create() + self.old_email = self.user.email + self.old_username = self.user.username + self.old_fullname = self.user.profile.name + self.details = { + 'email': 'new+{}'.format(self.user.email), + 'username': 'new_{}'.format(self.user.username), + 'fullname': 'Grown Up {}'.format(self.user.profile.name), + 'country': 'PK', + 'non_existing_field': 'value', + } + + # Mocks + self.strategy = mock.MagicMock() + self.strategy.storage.user.changed.side_effect = lambda user: user.save() + + get_from_pipeline = mock.patch('third_party_auth.pipeline.provider.Registry.get_from_pipeline') + self.get_from_pipeline = get_from_pipeline.start() + self.get_from_pipeline.return_value = mock.MagicMock(sync_learner_profile_data=True) + self.addCleanup(get_from_pipeline.stop) + + def test_user_details_force_sync(self): + """ + The user details are synced properly and an email is sent when the email is changed. + """ + # Begin the pipeline. + pipeline.user_details_force_sync( + auth_entry=pipeline.AUTH_ENTRY_LOGIN, + strategy=self.strategy, + details=self.details, + user=self.user, + ) + + # User now has updated information in the DB. + user = User.objects.get() + assert user.email == 'new+{}'.format(self.old_email) + assert user.username == 'new_{}'.format(self.old_username) + assert user.profile.name == 'Grown Up {}'.format(self.old_fullname) + assert user.profile.country == 'PK' + + assert len(mail.outbox) == 1 + + def test_user_details_force_sync_email_conflict(self): + """ + The user details were attempted to be synced but the incoming email already exists for another account. + """ + # Create a user with an email that conflicts with the incoming value. + UserFactory.create(email='new+{}'.format(self.old_email)) + + # Begin the pipeline. + pipeline.user_details_force_sync( + auth_entry=pipeline.AUTH_ENTRY_LOGIN, + strategy=self.strategy, + details=self.details, + user=self.user, + ) + + # The email is not changed, but everything else is. + user = User.objects.get(pk=self.user.pk) + assert user.email == self.old_email + assert user.username == 'new_{}'.format(self.old_username) + assert user.profile.name == 'Grown Up {}'.format(self.old_fullname) + assert user.profile.country == 'PK' + + # No email should be sent for an email change. + assert len(mail.outbox) == 0 + + def test_user_details_force_sync_username_conflict(self): + """ + The user details were attempted to be synced but the incoming username already exists for another account. + + An email should still be sent in this case. + """ + # Create a user with an email that conflicts with the incoming value. + UserFactory.create(username='new_{}'.format(self.old_username)) + + # Begin the pipeline. + pipeline.user_details_force_sync( + auth_entry=pipeline.AUTH_ENTRY_LOGIN, + strategy=self.strategy, + details=self.details, + user=self.user, + ) + + # The username is not changed, but everything else is. + user = User.objects.get(pk=self.user.pk) + assert user.email == 'new+{}'.format(self.old_email) + assert user.username == self.old_username + assert user.profile.name == 'Grown Up {}'.format(self.old_fullname) + assert user.profile.country == 'PK' + + # An email should still be sent because the email changed. + assert len(mail.outbox) == 1 diff --git a/common/templates/emails/sync_learner_profile_data_email_change_body.txt b/common/templates/emails/sync_learner_profile_data_email_change_body.txt new file mode 100644 index 0000000000..75483715b9 --- /dev/null +++ b/common/templates/emails/sync_learner_profile_data_email_change_body.txt @@ -0,0 +1,25 @@ +<%! from django.utils.translation import ugettext as _ %> +<%! from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers %> + +

+${_("The email associated with your {platform_name} account has changed from {old_email} to {new_email}.").format( + platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), + old_email=old_email, + new_email=new_email, +)} +

+ + +

${_("No action is needed on your part.")}

+ +

+${_("If this change is not correct, contact {link_start}{platform_name} Support{link_end} " + "or your administrator.").format( + platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), + link_start=u"".format( + support_link=configuration_helpers.get_value('SUPPORT_SITE_LINK', settings.SUPPORT_SITE_LINK), + ), + link_end=u"", + ) +} +

diff --git a/common/templates/emails/sync_learner_profile_data_email_change_subject.txt b/common/templates/emails/sync_learner_profile_data_email_change_subject.txt new file mode 100644 index 0000000000..48dd43534f --- /dev/null +++ b/common/templates/emails/sync_learner_profile_data_email_change_subject.txt @@ -0,0 +1,6 @@ +<%! from django.utils.translation import ugettext as _ %> +<%! from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers %> + +${_("Your {platform_name} account email has been updated").format( + platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME), +)}