From e5e96c9dde15f8a90a9710f266f7715b613997ed Mon Sep 17 00:00:00 2001 From: Mike O'Connell Date: Fri, 14 Feb 2020 09:59:36 -0500 Subject: [PATCH] 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