diff --git a/common/djangoapps/student/migrations/0019_auto_20181221_0540.py b/common/djangoapps/student/migrations/0019_auto_20181221_0540.py new file mode 100644 index 0000000000000000000000000000000000000000..2b351a384d7ce41b4f10f3c2ee6d9a104cebd097 --- /dev/null +++ b/common/djangoapps/student/migrations/0019_auto_20181221_0540.py @@ -0,0 +1,34 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.16 on 2018-12-21 10:40 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import openedx.core.djangolib.model_mixins + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('student', '0018_remove_password_history'), + ] + + operations = [ + migrations.CreateModel( + name='PendingSecondaryEmailChange', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('new_secondary_email', models.CharField(blank=True, db_index=True, max_length=255)), + ('activation_key', models.CharField(db_index=True, max_length=32, unique=True, verbose_name=b'activation key')), + ('user', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + bases=(openedx.core.djangolib.model_mixins.DeletableByUserValue, models.Model), + ), + migrations.AddField( + model_name='accountrecovery', + name='is_active', + field=models.BooleanField(default=False), + ), + ] diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 2203aba9db6c8547332dd72a2ebca30268dc8f64..9c5387a2c80c542330d7f7afe2683cee673318aa 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -785,6 +785,15 @@ class PendingEmailChange(DeletableByUserValue, models.Model): return self.activation_key +class PendingSecondaryEmailChange(DeletableByUserValue, models.Model): + """ + This model keeps track of pending requested changes to a user's secondary email address. + """ + user = models.OneToOneField(User, unique=True, db_index=True, on_delete=models.CASCADE) + new_secondary_email = models.CharField(blank=True, max_length=255, db_index=True) + activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) + + EVENT_NAME_ENROLLMENT_ACTIVATED = 'edx.course.enrollment.activated' EVENT_NAME_ENROLLMENT_DEACTIVATED = 'edx.course.enrollment.deactivated' EVENT_NAME_ENROLLMENT_MODE_CHANGED = 'edx.course.enrollment.mode_changed' @@ -2692,6 +2701,7 @@ class AccountRecovery(models.Model): null=False, blank=False, ) + is_active = models.BooleanField(default=False) class Meta(object): db_table = "auth_accountrecovery" diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index f0ba4135b4dfbaea7740abb43b611af90ca77c0f..7dd29304f99641f8c290e2145bbcab004bfd5d3f 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -575,3 +575,112 @@ class EmailChangeConfirmationTests(EmailTestMixin, CacheIsolationMixin, Transact confirm_email_change(self.request, self.key) mock_rollback.assert_called_with() + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', "Test only valid in LMS") +class SecondaryEmailChangeRequestTests(EventTestMixin, EmailTemplateTagMixin, CacheIsolationTestCase): + """ + Test changing a user's email address + """ + + def setUp(self, tracker='student.views.management.tracker'): + super(SecondaryEmailChangeRequestTests, self).setUp(tracker) + self.user = UserFactory.create() + self.new_secondary_email = 'new.secondary.email@edx.org' + self.req_factory = RequestFactory() + self.request = self.req_factory.post('unused_url', data={ + 'password': 'test', + 'new_email': self.new_secondary_email + }) + self.request.user = self.user + self.user.email_user = Mock() + + def do_email_validation(self, email): + """ + Executes validate_new_secondary_email, returning any resulting error message. + """ + try: + validate_new_email(self.request.user, email) + except ValueError as err: + return text_type(err) + + def do_secondary_email_change(self, user, email, activation_key=None): + """ + Executes do_secondary_email_change_request, returning any resulting error message. + """ + with patch('crum.get_current_request', return_value=self.fake_request): + do_email_change_request( + user=user, + new_email=email, + activation_key=activation_key, + secondary_email_change_request=True + ) + + def assertFailedRequest(self, response_data, expected_error): + """ + Assert that `response_data` indicates a failed request that returns `expected_error` + """ + self.assertFalse(response_data['success']) + self.assertEquals(expected_error, response_data['error']) + self.assertFalse(self.user.email_user.called) + + def test_invalid_emails(self): + """ + Assert the expected error message from the email validation method for an invalid + (improperly formatted) email address. + """ + for email in ('bad_email', 'bad_email@', '@bad_email'): + self.assertEqual(self.do_email_validation(email), 'Valid e-mail address required.') + + @patch('django.core.mail.send_mail') + def test_email_failure(self, send_mail): + """ + Test the return value if sending the email for the user to click fails. + """ + send_mail.side_effect = [Exception, None] + with self.assertRaisesRegexp(ValueError, 'Unable to send email activation link. Please try again later.'): + self.do_secondary_email_change(self.user, "valid@email.com") + + self.assert_no_events_were_emitted() + + def test_email_success(self): + """ + Test email was sent if no errors encountered. + """ + new_email = "valid@example.com" + registration_key = "test-registration-key" + + self.do_secondary_email_change(self.user, new_email, registration_key) + + self._assert_email( + subject=u'Request to change édX account secondary e-mail', + body_fragments=[ + u'We received a request to change the secondary e-mail associated with', + u'your édX account to {new_email}.'.format( + new_email=new_email, + ), + u'If this is correct, please confirm your new secondary e-mail address by visiting:', + u'http://edx.org/activate_secondary_email/{key}'.format(key=registration_key), + u'If you didn\'t request this, you don\'t need to do anything;', + u'you won\'t receive any more email from us.', + u'Please do not reply to this e-mail; if you require assistance,', + u'check the help section of the édX web site.', + ], + ) + + def _assert_email(self, subject, body_fragments): + """ + Verify that the email was sent. + """ + assert len(mail.outbox) == 1 + assert len(body_fragments) > 1, 'Should provide at least two body fragments' + + message = mail.outbox[0] + text = message.body + html = message.alternatives[0][0] + + assert message.subject == subject + + for body in text, html: + for fragment in body_fragments: + assert fragment in body diff --git a/common/djangoapps/student/urls.py b/common/djangoapps/student/urls.py index 55b3911b1107cc1257897ff8dad8470a66286930..01da47f04f291cd99efc0f663af46c5888e20d23 100644 --- a/common/djangoapps/student/urls.py +++ b/common/djangoapps/student/urls.py @@ -33,6 +33,13 @@ urlpatterns = [ url(r'^course_run/{}/refund_status$'.format(settings.COURSE_ID_PATTERN), views.course_run_refund_status, name="course_run_refund_status"), + + url( + r'^activate_secondary_email/(?P<key>[^/]*)$', + views.activate_secondary_email, + name='activate_secondary_email' + ), + ] # password reset django views (see above for password reset views) diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 48e0e792ba224d03375488a663e435e4980ecc50..dd185641214c0bf2d6a3b85201718c7d853b4b47 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -76,6 +76,7 @@ from student.models import ( AccountRecovery, CourseEnrollment, PendingEmailChange, + PendingSecondaryEmailChange, Registration, RegistrationCookieConfiguration, UserAttribute, @@ -984,26 +985,35 @@ def validate_secondary_email(account_recovery, new_email): raise ValueError(message) -def do_email_change_request(user, new_email, activation_key=None): +def do_email_change_request(user, new_email, activation_key=None, secondary_email_change_request=False): """ Given a new email for a user, does some basic verification of the new address and sends an activation message to the new address. If any issues are encountered with verification or sending the message, a ValueError will be thrown. """ - pec_list = PendingEmailChange.objects.filter(user=user) - if len(pec_list) == 0: - pec = PendingEmailChange() - pec.user = user - else: - pec = pec_list[0] - # if activation_key is not passing as an argument, generate a random key if not activation_key: activation_key = uuid.uuid4().hex - pec.new_email = new_email - pec.activation_key = activation_key - pec.save() + confirm_link = reverse('confirm_email_change', kwargs={'key': activation_key, }) + + if secondary_email_change_request: + PendingSecondaryEmailChange.objects.update_or_create( + user=user, + defaults={ + 'new_secondary_email': new_email, + 'activation_key': activation_key, + } + ) + confirm_link = reverse('activate_secondary_email', kwargs={'key': activation_key}) + else: + PendingEmailChange.objects.update_or_create( + user=user, + defaults={ + 'new_email': new_email, + 'activation_key': activation_key, + } + ) use_https = theming_helpers.get_current_request().is_secure() @@ -1011,18 +1021,17 @@ def do_email_change_request(user, new_email, activation_key=None): message_context = get_base_template_context(site) message_context.update({ 'old_email': user.email, - 'new_email': pec.new_email, + 'new_email': new_email, + 'is_secondary_email_change_request': secondary_email_change_request, 'confirm_link': '{protocol}://{site}{link}'.format( protocol='https' if use_https else 'http', site=configuration_helpers.get_value('SITE_NAME', settings.SITE_NAME), - link=reverse('confirm_email_change', kwargs={ - 'key': pec.activation_key, - }), + link=confirm_link, ), }) msg = EmailChange().personalize( - recipient=Recipient(user.username, pec.new_email), + recipient=Recipient(user.username, new_email), language=preferences_api.get_user_preference(user, LANGUAGE_KEY), user_context=message_context, ) @@ -1034,18 +1043,42 @@ def do_email_change_request(user, new_email, activation_key=None): log.error(u'Unable to send email activation link to user from "%s"', from_address, exc_info=True) raise ValueError(_('Unable to send email activation link. Please try again later.')) - # When the email address change is complete, a "edx.user.settings.changed" event will be emitted. - # But because changing the email address is multi-step, we also emit an event here so that we can - # track where the request was initiated. - tracker.emit( - SETTING_CHANGE_INITIATED, - { - "setting": "email", - "old": message_context['old_email'], - "new": message_context['new_email'], - "user_id": user.id, - } - ) + if not secondary_email_change_request: + # When the email address change is complete, a "edx.user.settings.changed" event will be emitted. + # But because changing the email address is multi-step, we also emit an event here so that we can + # track where the request was initiated. + tracker.emit( + SETTING_CHANGE_INITIATED, + { + "setting": "email", + "old": message_context['old_email'], + "new": message_context['new_email'], + "user_id": user.id, + } + ) + + +@ensure_csrf_cookie +def activate_secondary_email(request, key): # pylint: disable=unused-argument + """ + This is called when the activation link is clicked. We activate the secondary email + for the requested user. + """ + try: + pending_secondary_email_change = PendingSecondaryEmailChange.objects.get(activation_key=key) + except PendingSecondaryEmailChange.DoesNotExist: + return render_to_response("invalid_email_key.html", {}) + + try: + account_recovery_obj = AccountRecovery.objects.get(user_id=pending_secondary_email_change.user) + except AccountRecovery.DoesNotExist: + return render_to_response("secondary_email_change_failed.html", { + 'secondary_email': pending_secondary_email_change.new_secondary_email + }) + + account_recovery_obj.is_active = True + account_recovery_obj.save() + return render_to_response("secondary_email_change_successful.html") @ensure_csrf_cookie diff --git a/common/templates/student/edx_ace/emailchange/email/body.html b/common/templates/student/edx_ace/emailchange/email/body.html index 93cc842e146fb6c3802394edf51a98389e2995ac..8462c2fa7f339b383dd3cafbe4d829becefc052a 100644 --- a/common/templates/student/edx_ace/emailchange/email/body.html +++ b/common/templates/student/edx_ace/emailchange/email/body.html @@ -10,7 +10,11 @@ {% trans "Email Change" %} </h1> <p style="color: rgba(0,0,0,.75);"> - {% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} + {% if is_secondary_email_change_request %} + {% blocktrans %}We received a request to change the secondary e-mail associated with your {{ platform_name }} account to {{ new_email }}. If this is correct, please confirm your new secondary e-mail address by visiting:{% endblocktrans %} + {% else %} + {% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} + {% endif %} <br /> </p> diff --git a/common/templates/student/edx_ace/emailchange/email/body.txt b/common/templates/student/edx_ace/emailchange/email/body.txt index 34c86d59bfdaf66bea89c9fe0ddb15c5d6bd7987..1b9e381748e6a7e7c46cee3244ff276ac850b26e 100644 --- a/common/templates/student/edx_ace/emailchange/email/body.txt +++ b/common/templates/student/edx_ace/emailchange/email/body.txt @@ -1,5 +1,9 @@ {% load i18n %}{% autoescape off %} -{% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} +{% if is_secondary_email_change_request %} + {% blocktrans %}We received a request to change the secondary e-mail associated with your {{ platform_name }} account to {{ new_email }}. If this is correct, please confirm your new secondary e-mail address by visiting:{% endblocktrans %} +{% else %} + {% blocktrans %}We received a request to change the e-mail associated with your {{ platform_name }} account from {{ old_email }} to {{ new_email }}. If this is correct, please confirm your new e-mail address by visiting:{% endblocktrans %} +{% endif %} {{ confirm_link }} diff --git a/common/templates/student/edx_ace/emailchange/email/subject.txt b/common/templates/student/edx_ace/emailchange/email/subject.txt index 54313b5c8facc18057a267e13b869ec0ba345ed5..ea24a76f50503884939f6adf0a093e7132b01758 100644 --- a/common/templates/student/edx_ace/emailchange/email/subject.txt +++ b/common/templates/student/edx_ace/emailchange/email/subject.txt @@ -1,4 +1,8 @@ {% load i18n %} {% autoescape off %} -{% blocktrans trimmed %}Request to change {{ platform_name }} account e-mail{% endblocktrans %} +{% if is_secondary_email_change_request %} + {% blocktrans trimmed %}Request to change {{ platform_name }} account secondary e-mail{% endblocktrans %} +{% else %} + {% blocktrans trimmed %}Request to change {{ platform_name }} account e-mail{% endblocktrans %} +{% endif %} {% endautoescape %} diff --git a/lms/static/js/student_account/views/account_settings_factory.js b/lms/static/js/student_account/views/account_settings_factory.js index 514a35ed0b44007c0ddfe7c6fff7c20a4407c2ee..d8231d9f2ae304677044c53943dd58a92057391a 100644 --- a/lms/static/js/student_account/views/account_settings_factory.js +++ b/lms/static/js/student_account/views/account_settings_factory.js @@ -246,7 +246,17 @@ // Secondary email address if (isSecondaryEmailFeatureEnabled) { secondaryEmailFieldView = { - view: new AccountSettingsFieldViews.EmailFieldView(secondaryEmailFieldData) + view: new AccountSettingsFieldViews.EmailFieldView(secondaryEmailFieldData), + successMessage: function() { + return HtmlUtils.joinHtml( + this.indicators.success, + StringUtils.interpolate( + gettext('We\'ve sent a confirmation message to {new_secondary_email_address}. Click the link in the message to update your secondary email address.'), // eslint-disable-line max-len + { + new_secondary_email_address: this.fieldValue() + } + ) + );} }; emailFieldViewIndex = aboutSectionsData[0].fields.indexOf(emailFieldView); diff --git a/lms/templates/secondary_email_change_failed.html b/lms/templates/secondary_email_change_failed.html new file mode 100644 index 0000000000000000000000000000000000000000..cff94c902046edee631b6bc85b2fc4673fb43dbc --- /dev/null +++ b/lms/templates/secondary_email_change_failed.html @@ -0,0 +1,20 @@ +## xss-lint: disable=mako-missing-default,python-wrap-html +<%! from django.utils.translation import ugettext as _ %> + +<%inherit file="main.html" /> + +<section class="container activation"> + + <section class="message"> + <h1 class="invalid">${_("Secondary e-mail change failed")}</h1> + <hr class="horizontal-divider"> + + % if err_msg: + <p>${err_msg}</p> + % else: + <p>${_("We were unable to activate your secondary email {secondary_email}").format(secondary_email=secondary_email)}</p> + % endif + ## xss-lint: disable=python-wrap-html + <p>${_('Go back to the {link_start}home page{link_end}.').format(link_start='<a href="/">', link_end='</a>')}</p> + </section> +</section> diff --git a/lms/templates/secondary_email_change_successful.html b/lms/templates/secondary_email_change_successful.html new file mode 100644 index 0000000000000000000000000000000000000000..0d8d8a0b6efc208d9a7219b1aa68af15fc2ee3f5 --- /dev/null +++ b/lms/templates/secondary_email_change_successful.html @@ -0,0 +1,20 @@ +## xss-lint: disable=mako-missing-default +<%inherit file="main.html" /> +<%! +from django.utils.translation import ugettext as _ +from django.urls import reverse +from openedx.core.djangolib.markup import HTML, Text + +%> + +<section class="container activation"> + <section class="message"> + <h1 class="valid">${_("Secondary e-mail change successful!")}</h1> + <hr class="horizontal-divider"> + + <p>${Text(_('Your secondary email has been activated. Please visit {link_start}dashboard{link_end} for courses.')).format( + link_start=HTML('<a href="{url}">').format(url=reverse('dashboard')), + link_end=HTML('</a>'), + )}</p> + </section> +</section> diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 3bc667eaafef95def7cdac3ce26f8d094e5b76fc..296e94d033f002aba054054730e4448148ba8c98 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -299,6 +299,18 @@ def update_account_settings(requesting_user, update, username=None): u"Error thrown from do_email_change_request: '{}'".format(text_type(err)), user_message=text_type(err) ) + if changing_secondary_email: + try: + student_views.do_email_change_request( + user=existing_user, + new_email=update["secondary_email"], + secondary_email_change_request=True, + ) + except ValueError as err: + raise AccountUpdateError( + u"Error thrown from do_email_change_request: '{}'".format(text_type(err)), + user_message=text_type(err) + ) @helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError])