From e5e96c9dde15f8a90a9710f266f7715b613997ed Mon Sep 17 00:00:00 2001 From: Mike O'Connell Date: Fri, 14 Feb 2020 09:59:36 -0500 Subject: [PATCH 1/3] Don't update account recovery until after activation Rather than to create or update the account recovery record when the Account Settings page is updated, defer updating until the new recovery email is confirmed ENT-2607 --- common/djangoapps/student/models.py | 24 +++--- common/djangoapps/student/views/dashboard.py | 83 ++++++++++--------- common/djangoapps/student/views/management.py | 21 +++-- .../core/djangoapps/user_api/accounts/api.py | 42 +++------- 4 files changed, 80 insertions(+), 90 deletions(-) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 0491471de1..7d088c355b 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -18,31 +18,30 @@ import logging import uuid from collections import OrderedDict, defaultdict, namedtuple from datetime import datetime, timedelta -from django.core.validators import FileExtensionValidator from functools import total_ordering from importlib import import_module import six from config_models.models import ConfigurationModel +from course_modes.models import CourseMode, get_cosmetic_verified_display_price from django.apps import apps from django.conf import settings -from django.contrib.auth.hashers import make_password from django.contrib.auth.models import User from django.contrib.auth.signals import user_logged_in, user_logged_out from django.contrib.sites.models import Site from django.core.cache import cache from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist +from django.core.validators import FileExtensionValidator from django.db import IntegrityError, models from django.db.models import Count, Q, Index from django.db.models.signals import post_save, pre_save from django.db.utils import ProgrammingError from django.dispatch import receiver -from django.utils import timezone +from django.utils.encoding import python_2_unicode_compatible from django.utils.functional import cached_property from django.utils.translation import ugettext_lazy as _ from django.utils.translation import ugettext_noop from django_countries.fields import CountryField -from django.utils.encoding import python_2_unicode_compatible from edx_django_utils.cache import RequestCache from edx_rest_api_client.exceptions import SlumberBaseException from eventtracking import tracker @@ -55,18 +54,22 @@ from six import text_type from six.moves import range from six.moves.urllib.parse import urlencode from slumber.exceptions import HttpClientError, HttpServerError +from student.signals import ENROLL_STATUS_CHANGE, ENROLLMENT_TRACK_UPDATED, UNENROLL_DONE +from track import contexts, segment from user_util import user_util +from util.milestones_helpers import is_entrance_exams_enabled +from util.model_utils import emit_field_changed_events, get_changed_fields_dict +from util.query import use_read_replica_if_available -from course_modes.models import CourseMode, get_cosmetic_verified_display_price +import openedx.core.djangoapps.django_comment_common.comment_client as cc +from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.courseware.models import ( CourseDynamicUpgradeDeadlineConfiguration, DynamicUpgradeDeadlineConfiguration, OrgDynamicUpgradeDeadlineConfiguration ) -from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -import openedx.core.djangoapps.django_comment_common.comment_client as cc from openedx.core.djangoapps.enrollments.api import ( _default_course_mode, get_enrollment_attributes, @@ -75,11 +78,6 @@ from openedx.core.djangoapps.enrollments.api import ( from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.xmodule_django.models import NoneToEmptyManager from openedx.core.djangolib.model_mixins import DeletableByUserValue -from student.signals import ENROLL_STATUS_CHANGE, ENROLLMENT_TRACK_UPDATED, UNENROLL_DONE -from track import contexts, segment -from util.milestones_helpers import is_entrance_exams_enabled -from util.model_utils import emit_field_changed_events, get_changed_fields_dict -from util.query import use_read_replica_if_available log = logging.getLogger(__name__) AUDIT_LOG = logging.getLogger("audit") @@ -2983,7 +2981,7 @@ class AccountRecovery(models.Model): email (str): New email address to be set as the secondary email address. """ self.secondary_email = email - self.is_active = False + self.is_active = True self.save() @classmethod diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py index 8b9ae0cf17..d8cd1c43e8 100644 --- a/common/djangoapps/student/views/dashboard.py +++ b/common/djangoapps/student/views/dashboard.py @@ -7,6 +7,10 @@ import datetime import logging from collections import defaultdict +import track.views +from bulk_email.api import is_bulk_email_feature_enabled +from bulk_email.models import Optout # pylint: disable=import-error +from course_modes.models import CourseMode from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required @@ -15,18 +19,26 @@ from django.urls import reverse from django.utils.translation import ugettext as _ from django.views.decorators.csrf import ensure_csrf_cookie from edx_django_utils import monitoring as monitoring_utils -from opaque_keys.edx.keys import CourseKey -from pytz import UTC -from six import iteritems, text_type - -import track.views -from bulk_email.api import is_bulk_email_feature_enabled -from bulk_email.models import Optout # pylint: disable=import-error -from course_modes.models import CourseMode -from lms.djangoapps.courseware.access import has_access from edxmako.shortcuts import render_to_response, render_to_string from entitlements.models import CourseEntitlement +from opaque_keys.edx.keys import CourseKey +from pytz import UTC +from shoppingcart.models import CourseRegistrationCode, DonationConfiguration +from six import iteritems, text_type +from student.helpers import cert_info, check_verify_status_by_course, get_resume_urls_for_enrollments +from student.models import ( + AccountRecovery, + CourseEnrollment, + CourseEnrollmentAttribute, + DashboardConfiguration, + PendingSecondaryEmailChange, + UserProfile +) +from util.milestones_helpers import get_pre_requisite_courses_not_completed +from xmodule.modulestore.django import modulestore + from lms.djangoapps.commerce.utils import EcommerceService # pylint: disable=import-error +from lms.djangoapps.courseware.access import has_access from lms.djangoapps.experiments.utils import get_dashboard_course_info from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.catalog.utils import ( @@ -39,22 +51,10 @@ from openedx.core.djangoapps.programs.models import ProgramsApiConfig from openedx.core.djangoapps.programs.utils import ProgramDataExtender, ProgramProgressMeter from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api.accounts.utils import is_secondary_email_feature_enabled_for_user -from openedx.core.djangoapps.user_authn.cookies import set_logged_in_cookies from openedx.core.djangoapps.util.maintenance_banner import add_maintenance_banner from openedx.core.djangoapps.waffle_utils import WaffleFlag, WaffleFlagNamespace from openedx.core.djangolib.markup import HTML, Text from openedx.features.enterprise_support.api import get_dashboard_consent_notification -from shoppingcart.models import CourseRegistrationCode, DonationConfiguration -from student.helpers import cert_info, check_verify_status_by_course, get_resume_urls_for_enrollments -from student.models import ( - AccountRecovery, - CourseEnrollment, - CourseEnrollmentAttribute, - DashboardConfiguration, - UserProfile -) -from util.milestones_helpers import get_pre_requisite_courses_not_completed -from xmodule.modulestore.django import modulestore log = logging.getLogger("edx.student") @@ -654,28 +654,31 @@ def student_dashboard(request): recovery_email_message = recovery_email_activation_message = None if is_secondary_email_feature_enabled_for_user(user=user): try: - account_recovery_obj = AccountRecovery.objects.get(user=user) - except AccountRecovery.DoesNotExist: - recovery_email_message = Text( - _( - "Add a recovery email to retain access when single-sign on is not available. " - "Go to {link_start}your Account Settings{link_end}.") - ).format( - link_start=HTML("").format( - account_setting_page=reverse('account_settings'), - ), - link_end=HTML("") - ) - else: - if not account_recovery_obj.is_active: - recovery_email_activation_message = Text( + pending_email = PendingSecondaryEmailChange.objects.get(user=user) + except PendingSecondaryEmailChange.DoesNotExist: + try: + account_recovery_obj = AccountRecovery.objects.get(user=user) + except AccountRecovery.DoesNotExist: + recovery_email_message = Text( _( - "Recovery email is not activated yet. " - "Kindly visit your email and follow the instructions to activate it." - ) + "Add a recovery email to retain access when single-sign on is not available. " + "Go to {link_start}your Account Settings{link_end}.") + ).format( + link_start = HTML("").format( + account_setting_page=reverse('account_settings'), + ), + link_end = HTML("") ) + else: + recovery_email_activation_message = Text( + _( + "Recovery email is not activated yet. " + "Kindly visit your email and follow the instructions to activate it." + ) + ) - # Disable lookup of Enterprise consent_required_course due to ENT-727 + +# Disable lookup of Enterprise consent_required_course due to ENT-727 # Will re-enable after fixing WL-1315 consent_required_courses = set() enterprise_customer_name = None diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 10ff1dfe70..3096af94d5 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -601,7 +601,7 @@ def validate_new_email(user, new_email): raise ValueError(_('Old email is the same as the new email.')) -def validate_secondary_email(account_recovery, new_email): +def validate_secondary_email(user, new_email): """ Enforce valid email addresses. """ @@ -612,11 +612,13 @@ def validate_secondary_email(account_recovery, new_email): if get_email_validation_error(new_email): raise ValueError(_('Valid e-mail address required.')) - if new_email == account_recovery.secondary_email: - raise ValueError(_('Old email is the same as the new email.')) + # Make sure that if there is an active recovery email address, that is not the same as the new one. + if hasattr(user, "account_recovery"): + if user.account_recovery.is_active and new_email == user.account_recovery.secondary_email: + raise ValueError(_('Old email is the same as the new email.')) # Make sure that secondary email address is not same as user's primary email. - if new_email == account_recovery.user.email: + if new_email == user.email: raise ValueError(_('Cannot be same as your sign in email address.')) # Make sure that secondary email address is not same as any of the primary emails currently in use or retired @@ -723,14 +725,19 @@ def activate_secondary_email(request, key): # pylint: disable=unused-argument return render_to_response("invalid_email_key.html", {}) try: - account_recovery_obj = AccountRecovery.objects.get(user_id=pending_secondary_email_change.user) + account_recovery = pending_secondary_email_change.user.account_recovery except AccountRecovery.DoesNotExist: + account_recovery = AccountRecovery(user=pending_secondary_email_change.user) + + try: + account_recovery.update_recovery_email(pending_secondary_email_change.new_secondary_email) + except ValidationError: 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() + pending_secondary_email_change.delete() + return render_to_response("secondary_email_change_successful.html") diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 71145a8f10..79f6c13b7b 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -15,30 +15,27 @@ from django.utils.translation import override as override_language from django.utils.translation import ugettext as _ from pytz import UTC from six import text_type # pylint: disable=ungrouped-imports - -from openedx.core.djangoapps.user_api import accounts, errors, helpers -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle -from openedx.core.djangoapps.user_api.errors import ( - AccountUpdateError, - AccountValidationError, - PreferenceValidationError -) -from openedx.core.djangoapps.user_api.preferences.api import update_user_preferences -from openedx.core.lib.api.view_utils import add_serializer_errors -from openedx.core.djangoapps.user_authn.views.registration_form import validate_name, validate_username -from openedx.features.enterprise_support.utils import get_enterprise_readonly_account_fields from student import views as student_views from student.models import ( AccountRecovery, - Registration, User, UserProfile, email_exists_or_retired, username_exists_or_retired ) from util.model_utils import emit_setting_changed_event -from util.password_policy_validators import normalize_password, validate_password +from util.password_policy_validators import validate_password +from openedx.core.djangoapps.user_api import accounts, errors, helpers +from openedx.core.djangoapps.user_api.errors import ( + AccountUpdateError, + AccountValidationError, + PreferenceValidationError +) +from openedx.core.djangoapps.user_api.preferences.api import update_user_preferences +from openedx.core.djangoapps.user_authn.views.registration_form import validate_name, validate_username +from openedx.core.lib.api.view_utils import add_serializer_errors +from openedx.features.enterprise_support.utils import get_enterprise_readonly_account_fields from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer, UserReadOnlySerializer, _visible_fields # Public access point for this function. @@ -221,16 +218,13 @@ def _validate_secondary_email(user, data, field_errors): if "secondary_email" not in data: return - account_recovery = _get_account_recovery(user) try: - student_views.validate_secondary_email(account_recovery, data["secondary_email"]) + student_views.validate_secondary_email(user, data["secondary_email"]) except ValueError as err: field_errors["secondary_email"] = { "developer_message": u"Error thrown from validate_secondary_email: '{}'".format(text_type(err)), "user_message": text_type(err) } - else: - account_recovery.update_recovery_email(data["secondary_email"]) def _validate_name_change(user_profile, data, field_errors): @@ -453,18 +447,6 @@ def _get_user_and_profile(username): return existing_user, existing_user_profile -def _get_account_recovery(user): - """ - helper method to return the account recovery object based on user. - """ - try: - account_recovery = user.account_recovery - except ObjectDoesNotExist: - account_recovery = AccountRecovery(user=user) - - return account_recovery - - def _validate(validation_func, err, *args): """Generic validation function that returns default on no errors, but the message associated with the err class From 7d67dcfb59437c8f2ab548a2b41face413b29da6 Mon Sep 17 00:00:00 2001 From: Mike O'Connell Date: Fri, 14 Feb 2020 14:23:27 -0500 Subject: [PATCH 2/3] Don't update account recovery until after activation Corrected PEP8 errors ENT-2607 --- common/djangoapps/student/views/dashboard.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py index d8cd1c43e8..38d4ef1fda 100644 --- a/common/djangoapps/student/views/dashboard.py +++ b/common/djangoapps/student/views/dashboard.py @@ -664,10 +664,10 @@ def student_dashboard(request): "Add a recovery email to retain access when single-sign on is not available. " "Go to {link_start}your Account Settings{link_end}.") ).format( - link_start = HTML("").format( + link_start=HTML("").format( account_setting_page=reverse('account_settings'), ), - link_end = HTML("") + link_end=HTML("") ) else: recovery_email_activation_message = Text( From 2c5264cb94e6df38cfd5233d2a65c853e34259d5 Mon Sep 17 00:00:00 2001 From: Mike O'Connell Date: Wed, 19 Feb 2020 14:00:07 -0500 Subject: [PATCH 3/3] Unit test for account recovery Add a unit test to create and activate a recovery email address ENT-2607 --- .../user_api/accounts/tests/test_api.py | 68 +++++++++++++------ 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 89a5839789..cea200d797 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -6,26 +6,31 @@ Most of the functionality is covered in test_views.py. import itertools -import re import unicodedata import ddt -import pytest -from dateutil.parser import parse as parse_datetime from django.conf import settings from django.contrib.auth.hashers import make_password from django.contrib.auth.models import User -from django.core import mail +from django.http import HttpResponse from django.test import TestCase from django.test.client import RequestFactory from django.urls import reverse from mock import Mock, patch from six import iteritems from social_django.models import UserSocialAuth +from student.models import ( + AccountRecovery, + PendingEmailChange, + PendingSecondaryEmailChange, + UserProfile +) +from student.tests.factories import UserFactory +from student.tests.tests import UserSettingsEventTestMixin +from student.views.management import activate_secondary_email from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin -from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory -from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY, USERNAME_MAX_LENGTH +from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY from openedx.core.djangoapps.user_api.accounts.api import ( get_account_settings, update_account_settings @@ -35,30 +40,14 @@ from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( fake_requested_retirement, setup_retirement_states ) -from openedx.core.djangoapps.user_api.accounts.tests.testutils import ( - INVALID_EMAILS, - INVALID_PASSWORDS, - INVALID_USERNAMES, - VALID_USERNAMES_UNICODE -) -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle from openedx.core.djangoapps.user_api.errors import ( - AccountEmailInvalid, - AccountPasswordInvalid, - AccountRequestError, AccountUpdateError, - AccountUserAlreadyExists, - AccountUsernameInvalid, AccountValidationError, - UserAPIInternalError, UserNotAuthorized, UserNotFound ) from openedx.core.djangolib.testing.utils import skip_unless_lms from openedx.features.enterprise_support.tests.factories import EnterpriseCustomerUserFactory -from student.models import PendingEmailChange, Registration -from student.tests.factories import UserFactory -from student.tests.tests import UserSettingsEventTestMixin def mock_render_to_string(template_name, context): @@ -66,6 +55,15 @@ def mock_render_to_string(template_name, context): return str((template_name, sorted(iteritems(context)))) +def mock_render_to_response(template_name): + """ + Return an HttpResponse with content that encodes template_name and context + """ + # This simulates any db access in the templates. + UserProfile.objects.exists() + return HttpResponse(template_name) + + class CreateAccountMixin(object): def create_account(self, username, password, email): # pylint: disable=missing-docstring @@ -82,6 +80,7 @@ class CreateAccountMixin(object): @skip_unless_lms @ddt.ddt +@patch('student.views.management.render_to_response', Mock(side_effect=mock_render_to_response, autospec=True)) class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, CreateAccountMixin, RetirementTestCase): """ These tests specifically cover the parts of the API methods that are not covered by test_views.py. @@ -457,6 +456,31 @@ class TestAccountApi(UserSettingsEventTestMixin, EmailTemplateTagMixin, CreateAc verify_event_emitted([{"code": "en"}, {"code": "fr"}], [{"code": "en"}, {"code": "fr"}]) verify_event_emitted([], [{"code": "en"}, {"code": "fr"}]) + def test_add_account_recovery(self): + test_email = "test@example.com" + pending_secondary_email_changes = PendingSecondaryEmailChange.objects.filter(user=self.user) + self.assertEqual(0, len(pending_secondary_email_changes)) + + account_recovery_objects = AccountRecovery.objects.filter(user=self.user) + self.assertEqual(0, len(account_recovery_objects)) + + with patch('crum.get_current_request', return_value=self.fake_request): + update = {"secondary_email": test_email} + update_account_settings(self.user, update) + + pending_secondary_email_change = PendingSecondaryEmailChange.objects.get(user=self.user) + self.assertIsNot(pending_secondary_email_change, None) + self.assertEqual(pending_secondary_email_change.new_secondary_email, test_email) + + activate_secondary_email(self.fake_request, pending_secondary_email_change.activation_key) + + pending_secondary_email_changes = PendingSecondaryEmailChange.objects.filter(user=self.user) + self.assertEqual(0, len(pending_secondary_email_changes)) + + account_recovery = AccountRecovery.objects.get(user=self.user) + self.assertIsNot(account_recovery, None) + self.assertEqual(account_recovery.secondary_email, test_email) + @patch('openedx.core.djangoapps.user_api.accounts.image_helpers._PROFILE_IMAGE_SIZES', [50, 10]) @patch.dict(