diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index c8c5477e8d..751046ee27 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/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): diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 7da86eaca6..c3c3cd0151 100644 --- a/common/djangoapps/student/views.py +++ b/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.