diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 2fdbccd2fb..75866b7dda 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -555,6 +555,7 @@ def user_signup_handler(sender, **kwargs): # pylint: disable=unused-argument log.info(u'user {} originated from a white labeled "Microsite"'.format(kwargs['instance'].id)) +@transaction.non_atomic_requests def create_account_with_params(request, params): """ Given a request and a dict of parameters (which may or may not have come @@ -570,14 +571,13 @@ def create_account_with_params(request, params): parameters is invalid for any other reason. Issues with this code: - * It is not transactional. If there is a failure part-way, an incomplete - account will be created and left in the database. + * It is non-transactional except where explicitly wrapped in atomic to + alleviate deadlocks and improve performance. This means failures at + different places in registration can leave users in inconsistent + states. * Third-party auth passwords are not verified. There is a comment that they are unused, but it would be helpful to have a sanity check that they are sane. - * It is over 300 lines long (!) and includes disprate functionality, from - registration e-mails to all sorts of other things. It should be broken - up into semantically meaningful functions. * The user-facing text is rather unfriendly (e.g. "Username must be a minimum of two characters long" rather than "Please use a username of at least two characters"). @@ -662,8 +662,12 @@ def create_account_with_params(request, params): ) custom_form = get_registration_extension_form(data=params) + third_party_provider = None + running_pipeline = None + new_user = None + # Perform operations within a transaction that are critical to account creation - with transaction.atomic(): + with outer_atomic(read_committed=True): # first, create the account (user, profile, registration) = do_create_account(form, custom_form) @@ -701,6 +705,39 @@ def create_account_with_params(request, params): request.social_strategy.clean_partial_pipeline(social_access_token) raise ValidationError({'access_token': [error_message]}) + # If the user is registering via 3rd party auth, track which provider they use + if is_third_party_auth_enabled and pipeline.running(request): + running_pipeline = pipeline.get(request) + third_party_provider = provider.Registry.get_from_pipeline(running_pipeline) + + new_user = authenticate_new_user(request, user.username, params['password']) + django_login(request, new_user) + request.session.set_expiry(0) + + if do_external_auth: + eamap.user = new_user + eamap.dtsignup = datetime.datetime.now(UTC) + eamap.save() + AUDIT_LOG.info(u"User registered with external_auth %s", new_user.username) + AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', new_user.username, eamap) + + if settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'): + log.info('bypassing activation email') + new_user.is_active = True + new_user.save() + AUDIT_LOG.info( + u"Login activated on extauth account - {0} ({1})".format(new_user.username, new_user.email)) + + # Check if 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 skip_email: + registration.activate() + else: + compose_and_send_activation_email(user, profile, registration) + # Perform operations that are non-critical parts of account creation create_or_set_user_attribute_created_on_site(user, request.site) @@ -714,13 +751,6 @@ def create_account_with_params(request, params): dog_stats_api.increment("common.student.account_created") - # If the user is registering via 3rd party auth, track which provider they use - third_party_provider = None - running_pipeline = None - if is_third_party_auth_enabled and pipeline.running(request): - running_pipeline = pipeline.get(request) - third_party_provider = provider.Registry.get_from_pipeline(running_pipeline) - # Track the user's registration if hasattr(settings, 'LMS_SEGMENT_KEY') and settings.LMS_SEGMENT_KEY: tracking_context = tracker.get_tracker().resolve_context() @@ -770,20 +800,6 @@ def create_account_with_params(request, params): create_comments_service_user(user) - # 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 skip_email: - registration.activate() - else: - compose_and_send_activation_email(user, profile, registration) - - new_user = authenticate_new_user(request, user.username, params['password']) - django_login(request, new_user) - request.session.set_expiry(0) - try: record_registration_attributions(request, new_user) # Don't prevent a user from registering due to attribution errors. @@ -795,19 +811,6 @@ def create_account_with_params(request, params): if new_user is not None: AUDIT_LOG.info(u"Login success on new account creation - {0}".format(new_user.username)) - if do_external_auth: - eamap.user = new_user - eamap.dtsignup = datetime.datetime.now(UTC) - eamap.save() - AUDIT_LOG.info(u"User registered with external_auth %s", new_user.username) - AUDIT_LOG.info(u'Updated ExternalAuthMap for %s to be %s', new_user.username, eamap) - - if settings.FEATURES.get('BYPASS_ACTIVATION_EMAIL_FOR_EXTAUTH'): - log.info('bypassing activation email') - new_user.is_active = True - new_user.save() - AUDIT_LOG.info(u"Login activated on extauth account - {0} ({1})".format(new_user.username, new_user.email)) - return new_user @@ -919,6 +922,7 @@ def record_registration_attributions(request, user): @csrf_exempt +@transaction.non_atomic_requests def create_account(request, post_override=None): """ JSON call to create new edX account. diff --git a/lms/djangoapps/branding/views.py b/lms/djangoapps/branding/views.py index d189ebcf64..4ac14721ce 100644 --- a/lms/djangoapps/branding/views.py +++ b/lms/djangoapps/branding/views.py @@ -6,6 +6,7 @@ from django.conf import settings from django.contrib.staticfiles.storage import staticfiles_storage from django.core.cache import cache from django.core.urlresolvers import reverse +from django.db import transaction from django.http import Http404, HttpResponse from django.shortcuts import redirect from django.utils import translation @@ -26,12 +27,12 @@ log = logging.getLogger(__name__) @ensure_csrf_cookie +@transaction.non_atomic_requests @cache_if_anonymous() def index(request): - ''' + """ Redirects to main page -- info page if user authenticated, or marketing if not - ''' - + """ if request.user.is_authenticated(): # Only redirect to dashboard if user has # courses in his/her dashboard. Otherwise UX is a bit cryptic. diff --git a/openedx/core/djangoapps/external_auth/tests/test_shib.py b/openedx/core/djangoapps/external_auth/tests/test_shib.py index fc6b79f3bc..2b94f3d3b7 100644 --- a/openedx/core/djangoapps/external_auth/tests/test_shib.py +++ b/openedx/core/djangoapps/external_auth/tests/test_shib.py @@ -302,7 +302,6 @@ class ShibSPTest(CacheIsolationTestCase): @unittest.skipUnless(settings.FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") @data(*gen_all_identities()) - @pytest.mark.django111_expected_failure def test_registration_form_submit(self, identity): """ Tests user creation after the registration form that pops is submitted. If there is no shib @@ -332,21 +331,23 @@ class ShibSPTest(CacheIsolationTestCase): self.assertEquals(len(audit_log_calls), 3) method_name, args, _kwargs = audit_log_calls[0] self.assertEquals(method_name, 'info') - self.assertEquals(len(args), 1) - self.assertIn(u'Login success on new account creation', args[0]) - self.assertIn(u'post_username', args[0]) - method_name, args, _kwargs = audit_log_calls[1] - self.assertEquals(method_name, 'info') self.assertEquals(len(args), 2) self.assertIn(u'User registered with external_auth', args[0]) self.assertEquals(u'post_username', args[1]) - method_name, args, _kwargs = audit_log_calls[2] + + method_name, args, _kwargs = audit_log_calls[1] self.assertEquals(method_name, 'info') self.assertEquals(len(args), 3) self.assertIn(u'Updated ExternalAuthMap for ', args[0]) self.assertEquals(u'post_username', args[1]) self.assertEquals(u'test_user@stanford.edu', args[2].external_id) + method_name, args, _kwargs = audit_log_calls[2] + self.assertEquals(method_name, 'info') + self.assertEquals(len(args), 1) + self.assertIn(u'Login success on new account creation', args[0]) + self.assertIn(u'post_username', args[0]) + user = User.objects.get(id=self.client.session['_auth_user_id']) # check that the created user has the right email, either taken from shib or user input diff --git a/openedx/core/djangoapps/external_auth/views.py b/openedx/core/djangoapps/external_auth/views.py index 440d893fc6..80fd430f02 100644 --- a/openedx/core/djangoapps/external_auth/views.py +++ b/openedx/core/djangoapps/external_auth/views.py @@ -19,6 +19,7 @@ from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.core.urlresolvers import reverse from django.core.validators import validate_email +from django.db import transaction from django.http import HttpResponse, HttpResponseForbidden, HttpResponseRedirect from django.shortcuts import redirect from django.utils.http import is_safe_url, urlquote @@ -39,6 +40,7 @@ from openedx.core.djangoapps.external_auth.models import ExternalAuthMap from openedx.core.djangoapps.site_configuration.helpers import get_value from student.helpers import get_next_url_for_login_page from student.models import UserProfile +from util.db import outer_atomic from xmodule.modulestore.django import modulestore if settings.FEATURES.get('AUTH_USE_CAS'): @@ -82,12 +84,14 @@ def generate_password(length=12, chars=string.letters + string.digits): return ''.join([choice(chars) for _i in range(length)]) +@transaction.non_atomic_requests @csrf_exempt def openid_login_complete(request, redirect_field_name=REDIRECT_FIELD_NAME, # pylint: disable=unused-argument render_failure=None): - """Complete the openid login process""" - + """ + Complete the openid login process + """ render_failure = (render_failure or default_render_failure) openid_response = openid_views.parse_openid_response(request) @@ -127,23 +131,31 @@ def _external_login_or_signup(request, email, fullname, retfun=None): - """Generic external auth login or signup""" + """ + Generic external auth login or signup + """ # pylint: disable=too-many-statements # see if we have a map from this external_id to an edX username - try: - eamap = ExternalAuthMap.objects.get(external_id=external_id, - external_domain=external_domain) - log.debug(u'Found eamap=%s', eamap) - except ExternalAuthMap.DoesNotExist: - # go render form for creating edX user - eamap = ExternalAuthMap(external_id=external_id, - external_domain=external_domain, - external_credentials=json.dumps(credentials)) - eamap.external_email = email - eamap.external_name = fullname - eamap.internal_password = generate_password() + eamap_defaults = { + 'external_credentials': json.dumps(credentials), + 'external_email': email, + 'external_name': fullname, + 'internal_password': generate_password() + } + + # We are not guaranteed to be in a transaction here since some upstream views + # use non_atomic_requests + with outer_atomic(): + eamap, created = ExternalAuthMap.objects.get_or_create( + external_id=external_id, + external_domain=external_domain, + defaults=eamap_defaults + ) + + if created: log.debug(u'Created eamap=%s', eamap) - eamap.save() + else: + log.debug(u'Found eamap=%s', eamap) log.info(u"External_Auth login_or_signup for %s : %s : %s : %s", external_domain, external_id, email, fullname) uses_shibboleth = settings.FEATURES.get('AUTH_USE_SHIB') and external_domain.startswith(SHIBBOLETH_DOMAIN_PREFIX) @@ -156,25 +168,26 @@ def _external_login_or_signup(request, # Since the id the idps return is not user-editable, and is of the from "username@stanford.edu", # use the id to link accounts instead. try: - link_user = User.objects.get(email=eamap.external_id) - if not ExternalAuthMap.objects.filter(user=link_user).exists(): - # if there's no pre-existing linked eamap, we link the user - eamap.user = link_user - eamap.save() - internal_user = link_user - log.info(u'SHIB: Linking existing account for %s', eamap.external_id) - # now pass through to log in - else: - # otherwise, there must have been an error, b/c we've already linked a user with these external - # creds - failure_msg = _( - "You have already created an account using " - "an external login like WebAuth or Shibboleth. " - "Please contact {tech_support_email} for support." - ).format( - tech_support_email=get_value('email_from_address', settings.TECH_SUPPORT_EMAIL), - ) - return default_render_failure(request, failure_msg) + with outer_atomic(): + link_user = User.objects.get(email=eamap.external_id) + if not ExternalAuthMap.objects.filter(user=link_user).exists(): + # if there's no pre-existing linked eamap, we link the user + eamap.user = link_user + eamap.save() + internal_user = link_user + log.info(u'SHIB: Linking existing account for %s', eamap.external_id) + # now pass through to log in + else: + # otherwise, there must have been an error, b/c we've already linked a user with these external + # creds + failure_msg = _( + "You have already created an account using " + "an external login like WebAuth or Shibboleth. " + "Please contact {tech_support_email} for support." + ).format( + tech_support_email=get_value('email_from_address', settings.TECH_SUPPORT_EMAIL), + ) + return default_render_failure(request, failure_msg) except User.DoesNotExist: log.info(u'SHIB: No user for %s yet, doing signup', eamap.external_email) return _signup(request, eamap, retfun) @@ -184,6 +197,7 @@ def _external_login_or_signup(request, # We trust shib's authentication, so no need to authenticate using the password again uname = internal_user.username + if uses_shibboleth: user = internal_user # Assuming this 'AUTHENTICATION_BACKENDS' is set in settings, which I think is safe @@ -206,6 +220,7 @@ def _external_login_or_signup(request, AUDIT_LOG.info(u'Linked user "{0}" logged in via SSL certificate'.format(user.email)) else: user = authenticate(username=uname, password=eamap.internal_password, request=request) + if user is None: # we want to log the failure, but don't want to log the password attempted: if settings.FEATURES['SQUELCH_PII_IN_LOGS']: @@ -378,6 +393,8 @@ def ssl_login_shortcut(func): Python function decorator for login procedures, to allow direct login based on existing ExternalAuth record and MIT ssl certificate. """ + + @transaction.non_atomic_requests def wrapped(*args, **kwargs): """ This manages the function wrapping, by determining whether to inject @@ -485,6 +502,7 @@ def cas_login(request, next_page=None, required=False): # ----------------------------------------------------------------------------- # Shibboleth (Stanford and others. Uses *Apache* environment variables) # ----------------------------------------------------------------------------- +@transaction.non_atomic_requests def shib_login(request): """ Uses Apache's REMOTE_USER environment variable as the external id. diff --git a/openedx/core/djangoapps/user_api/views.py b/openedx/core/djangoapps/user_api/views.py index 628ccbc4a0..1eb7be2f04 100644 --- a/openedx/core/djangoapps/user_api/views.py +++ b/openedx/core/djangoapps/user_api/views.py @@ -2,6 +2,7 @@ from django.contrib.auth.models import User from django.core.exceptions import NON_FIELD_ERRORS, PermissionDenied, ValidationError +from django.db import transaction from django.http import HttpResponse, HttpResponseForbidden from django.utils.decorators import method_decorator from django.utils.translation import ugettext as _ @@ -171,6 +172,7 @@ class RegistrationView(APIView): set_logged_in_cookies(request, response, user) return response + @method_decorator(transaction.non_atomic_requests) @method_decorator(sensitive_post_parameters("password")) def dispatch(self, request, *args, **kwargs): return super(RegistrationView, self).dispatch(request, *args, **kwargs)