Merge pull request #16321 from edx/saleem-latif/ENT-669

ENT-669: Activation email received when skip_email_verification flag is 'True'

common/djangoapps/student/tests/test_create_account.py

@@ -27,10 +27,48 @@ from openedx.core.djangoapps.user_api.accounts import (
 )
 from openedx.core.djangoapps.user_api.preferences.api import get_user_preference
 from student.models import UserAttribute
-from student.views import REGISTRATION_AFFILIATE_ID, REGISTRATION_UTM_CREATED_AT, REGISTRATION_UTM_PARAMETERS
+from student.views import REGISTRATION_AFFILIATE_ID, REGISTRATION_UTM_CREATED_AT, REGISTRATION_UTM_PARAMETERS, \
+    skip_activation_email
+from student.tests.factories import UserFactory
+from third_party_auth.tests import factories as third_party_auth_factory
 
 TEST_CS_URL = 'https://comments.service.test:123/'
 
+TEST_USERNAME = 'test_user'
+TEST_EMAIL = 'test@test.com'
+
+
+def get_mock_pipeline_data(username=TEST_USERNAME, email=TEST_EMAIL):
+    """
+    Return mock pipeline data.
+    """
+    return {
+        'backend': 'tpa-saml',
+        'kwargs': {
+            'username': username,
+            'auth_entry': 'register',
+            'request': {
+                'SAMLResponse': [],
+                'RelayState': [
+                    'testshib-openedx'
+                ]
+            },
+            'is_new': True,
+            'new_association': True,
+            'user': None,
+            'social': None,
+            'details': {
+                'username': username,
+                'fullname': 'Test Test',
+                'last_name': 'Test',
+                'first_name': 'Test',
+                'email': email,
+            },
+            'response': {},
+            'uid': 'testshib-openedx:{}'.format(username)
+        }
+    }
+
 
 @ddt.ddt
 @override_settings(
@@ -423,6 +461,88 @@ class TestCreateAccount(SiteMixin, TestCase):
         profile = self.create_account_and_fetch_profile(host=self.site.domain)
         self.assertEqual(UserAttribute.get_user_attribute(profile.user, 'created_on_site'), self.site.domain)
 
+    @ddt.data(
+        (
+            False, False, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False,
+            },
+            False  # Do not skip activation email for normal scenario.
+        ),
+        (
+            False, False, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': True, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False,
+            },
+            True  # Skip activation email when `SKIP_EMAIL_VALIDATION` FEATURE flag is active.
+        ),
+        (
+            False, False, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': True,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False,
+            },
+            True  # Skip activation email when `AUTOMATIC_AUTH_FOR_TESTING` FEATURE flag is active.
+        ),
+        (
+            True, False, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': True,
+            },
+            True  # Skip activation email for external auth scenario.
+        ),
+        (
+            False, False, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': True,
+            },
+            False  # Do not skip activation email when `BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH` feature flag is set
+                   # but it is not external auth scenario.
+        ),
+        (
+            False, True, get_mock_pipeline_data(),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False,
+            },
+            True  # Skip activation email if `skip_email_verification` is set for third party authentication.
+        ),
+        (
+            False, False, get_mock_pipeline_data(email='invalid@yopmail.com'),
+            {
+                'SKIP_EMAIL_VALIDATION': False, 'AUTOMATIC_AUTH_FOR_TESTING': False,
+                'BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH': False,
+            },
+            False  # Send activation email when `skip_email_verification` is not set.
+        )
+    )
+    @ddt.unpack
+    @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
+    def test_should_skip_activation_email(
+            self, do_external_auth, skip_email_verification, running_pipeline, feature_overrides, expected,
+    ):
+        """
+        Test `skip_activation_email` works as expected.
+        """
+        third_party_provider = third_party_auth_factory.SAMLProviderConfigFactory(
+            skip_email_verification=skip_email_verification,
+        )
+        user = UserFactory(username=TEST_USERNAME, email=TEST_EMAIL)
+
+        with override_settings(FEATURES=dict(settings.FEATURES, **feature_overrides)):
+            result = skip_activation_email(
+                user=user,
+                do_external_auth=do_external_auth,
+                running_pipeline=running_pipeline,
+                third_party_provider=third_party_provider
+            )
+
+            assert result == expected
+
 
 @ddt.ddt
 class TestCreateAccountValidation(TestCase):

common/djangoapps/student/views.py

@@ -51,6 +51,7 @@ from social_django import utils as social_utils
 import dogstats_wrapper as dog_stats_api
 import openedx.core.djangoapps.external_auth.views
 import third_party_auth
+from third_party_auth.saml import SAP_SUCCESSFACTORS_SAML_KEY
 import track.views
 from bulk_email.models import BulkEmailFlag, Optout  # pylint: disable=import-error
 from certificates.api import get_certificate_url, has_html_certificates_enabled  # pylint: disable=import-error
@@ -2038,32 +2039,16 @@ def create_account_with_params(request, params):
 
     create_comments_service_user(user)
 
-    # Don't send email if we are:
-    #
-    # 1. Doing load testing.
-    # 2. Random user generation for other forms of testing.
-    # 3. External auth bypassing activation.
-    # 4. Have the platform configured to not require e-mail activation.
-    # 5. Registering a new user using a trusted third party provider (with skip_email_verification=True)
-    #
-    # Note that this feature is only tested as a flag set one way or
-    # the other for *new* systems. we need to be careful about
-    # changing settings on a running system to make sure no users are
-    # left in an inconsistent state (or doing a migration if they are).
-    send_email = (
-        not settings.FEATURES.get('SKIP_EMAIL_VALIDATION', None) and
-        not settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING') and
-        not (do_external_auth and settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH')) and
-        not (
-            third_party_provider and third_party_provider.skip_email_verification and
-            user.email == running_pipeline['kwargs'].get('details', {}).get('email')
-        )
+    # Check if we system is configured to skip activation email for the current user.
+    skip_email = skip_activation_email(
+        user, do_external_auth, running_pipeline, third_party_provider,
     )
-    if send_email:
-        compose_and_send_activation_email(user, profile, registration)
-    else:
+
+    if skip_email:
         registration.activate()
         _enroll_user_in_pending_courses(user)  # Enroll student in any pending courses
+    else:
+        compose_and_send_activation_email(user, profile, registration)
 
     # Immediately after a user creates an account, we log them in. They are only
     # logged in until they close the browser. They can't log in again until they click
@@ -2099,6 +2084,54 @@ def create_account_with_params(request, params):
     return new_user
 
 
+def skip_activation_email(user, do_external_auth, running_pipeline, third_party_provider):
+    """
+    Return `True` if activation email should be skipped.
+
+    Skip email if we are:
+        1. Doing load testing.
+        2. Random user generation for other forms of testing.
+        3. External auth bypassing activation.
+        4. Have the platform configured to not require e-mail activation.
+        5. Registering a new user using a trusted third party provider (with skip_email_verification=True)
+
+    Note that this feature is only tested as a flag set one way or
+    the other for *new* systems. we need to be careful about
+    changing settings on a running system to make sure no users are
+    left in an inconsistent state (or doing a migration if they are).
+
+    Arguments:
+        user (User): Django User object for the current user.
+        do_external_auth (bool): True if external authentication is in progress.
+        running_pipeline (dict): Dictionary containing user and pipeline data for third party authentication.
+        third_party_provider (ProviderConfig): An instance of third party provider configuration.
+
+    Returns:
+        (bool): `True` if account activation email should be skipped, `False` if account activation email should be
+            sent.
+    """
+    sso_pipeline_email = running_pipeline and running_pipeline['kwargs'].get('details', {}).get('email')
+
+    # Email is valid if the SAML assertion email matches the user account email or
+    # no email was provided in the SAML assertion. Some IdP's use a callback
+    # to retrieve additional user account information (including email) after the
+    # initial account creation.
+    valid_email = (
+        sso_pipeline_email == user.email or (
+            sso_pipeline_email is None and
+            third_party_provider and
+            getattr(third_party_provider, "identity_provider_type", None) == SAP_SUCCESSFACTORS_SAML_KEY
+        )
+    )
+
+    return (
+        settings.FEATURES.get('SKIP_EMAIL_VALIDATION', None) or
+        settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING') or
+        (settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH') and do_external_auth) or
+        (third_party_provider and third_party_provider.skip_email_verification and valid_email)
+    )
+
+
 def _enroll_user_in_pending_courses(student):
     """
     Enroll student in any pending courses he/she may have.