Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #17867 from edx/bmedx/registration_deadlocks

Browse Source 
WIP: Refactor registration to improve reliability
This commit is contained in:
Brian Mesick
2018-04-06 15:24:13 -04:00
committed by GitHub
parent ba130c87e3 634234966a
commit 5782119341
5 changed files with 111 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files

84
common/djangoapps/student/views/management.py
View File

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

7
lms/djangoapps/branding/views.py
View File

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

15
openedx/core/djangoapps/external_auth/tests/test_shib.py
View File

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

88
openedx/core/djangoapps/external_auth/views.py
View File

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

2
openedx/core/djangoapps/user_api/views.py
View File

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