Merge pull request #23114 from edx/ENT-2607-2

Don't update account recovery until after activation

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

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(