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

@@ -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

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("<a href='{account_setting_page}'>").format(
-                    account_setting_page=reverse('account_settings'),
-                ),
-                link_end=HTML("</a>")
-            )
-        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("<a href='{account_setting_page}'>").format(
+                        account_setting_page=reverse('account_settings'),
+                    ),
+                    link_end = HTML("</a>")
                 )
+        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

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")
 
 

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