Merge pull request #5328 from edx/flowerhack/auto-login
Automatically log in users who access third party login after trying to register for a course
This commit is contained in:
@@ -38,11 +38,13 @@ from importlib import import_module
|
||||
|
||||
from opaque_keys.edx.locations import SlashSeparatedCourseKey
|
||||
from xmodule.modulestore import Location
|
||||
from xmodule.modulestore.django import modulestore
|
||||
from opaque_keys import InvalidKeyError
|
||||
|
||||
import lms.lib.comment_client as cc
|
||||
from util.query import use_read_replica_if_available
|
||||
from xmodule_django.models import CourseKeyField, NoneToEmptyManager
|
||||
from xmodule.modulestore.exceptions import ItemNotFoundError
|
||||
from xmodule.modulestore.django import modulestore
|
||||
from opaque_keys.edx.keys import CourseKey
|
||||
from functools import total_ordering
|
||||
|
||||
@@ -659,6 +661,22 @@ class LoginFailures(models.Model):
|
||||
return
|
||||
|
||||
|
||||
class CourseEnrollmentException(Exception):
|
||||
pass
|
||||
|
||||
class NonExistentCourseError(CourseEnrollmentException):
|
||||
pass
|
||||
|
||||
class EnrollmentClosedError(CourseEnrollmentException):
|
||||
pass
|
||||
|
||||
class CourseFullError(CourseEnrollmentException):
|
||||
pass
|
||||
|
||||
class AlreadyEnrolledError(CourseEnrollmentException):
|
||||
pass
|
||||
|
||||
|
||||
class CourseEnrollment(models.Model):
|
||||
"""
|
||||
Represents a Student's Enrollment record for a single Course. You should
|
||||
@@ -847,7 +865,7 @@ class CourseEnrollment(models.Model):
|
||||
log.exception('Unable to emit event %s for user %s and course %s', event_name, self.user.username, self.course_id)
|
||||
|
||||
@classmethod
|
||||
def enroll(cls, user, course_key, mode="honor"):
|
||||
def enroll(cls, user, course_key, mode="honor", check_access=False):
|
||||
"""
|
||||
Enroll a user in a course. This saves immediately.
|
||||
|
||||
@@ -857,18 +875,74 @@ class CourseEnrollment(models.Model):
|
||||
attribute), this method will automatically save it before
|
||||
adding an enrollment for it.
|
||||
|
||||
`course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
|
||||
`course_key` is our usual course_id string (e.g. "edX/Test101/2013_Fall)
|
||||
|
||||
`mode` is a string specifying what kind of enrollment this is. The
|
||||
default is "honor", meaning honor certificate. Future options
|
||||
may include "audit", "verified_id", etc. Please don't use it
|
||||
until we have these mapped out.
|
||||
|
||||
`check_access`: if True, we check that an accessible course actually
|
||||
exists for the given course_key before we enroll the student.
|
||||
The default is set to False to avoid breaking legacy code or
|
||||
code with non-standard flows (ex. beta tester invitations), but
|
||||
for any standard enrollment flow you probably want this to be True.
|
||||
|
||||
Exceptions that can be raised: NonExistentCourseError,
|
||||
EnrollmentClosedError, CourseFullError, AlreadyEnrolledError. All these
|
||||
are subclasses of CourseEnrollmentException if you want to catch all of
|
||||
them in the same way.
|
||||
|
||||
It is expected that this method is called from a method which has already
|
||||
verified the user authentication and access.
|
||||
verified the user authentication.
|
||||
|
||||
Also emits relevant events for analytics purposes.
|
||||
"""
|
||||
from courseware.access import has_access
|
||||
|
||||
# All the server-side checks for whether a user is allowed to enroll.
|
||||
try:
|
||||
course = modulestore().get_course(course_key)
|
||||
except ItemNotFoundError:
|
||||
log.warning(
|
||||
"User {0} failed to enroll in non-existent course {1}".format(
|
||||
user.username,
|
||||
course_key.to_deprecated_string()
|
||||
)
|
||||
)
|
||||
raise NonExistentCourseError
|
||||
|
||||
if check_access:
|
||||
if course is None:
|
||||
raise NonExistentCourseError
|
||||
if not has_access(user, 'enroll', course):
|
||||
log.warning(
|
||||
"User {0} failed to enroll in course {1} because enrollment is closed".format(
|
||||
user.username,
|
||||
course_key.to_deprecated_string()
|
||||
)
|
||||
)
|
||||
raise EnrollmentClosedError
|
||||
|
||||
if CourseEnrollment.is_course_full(course):
|
||||
log.warning(
|
||||
"User {0} failed to enroll in full course {1}".format(
|
||||
user.username,
|
||||
course_key.to_deprecated_string()
|
||||
)
|
||||
)
|
||||
raise CourseFullError
|
||||
if CourseEnrollment.is_enrolled(user, course_key):
|
||||
log.warning(
|
||||
"User {0} attempted to enroll in {1}, but they were already enrolled".format(
|
||||
user.username,
|
||||
course_key.to_deprecated_string()
|
||||
)
|
||||
)
|
||||
if check_access:
|
||||
raise AlreadyEnrolledError
|
||||
|
||||
# User is allowed to enroll if they've reached this point.
|
||||
enrollment = cls.get_or_create_enrollment(user, course_key)
|
||||
enrollment.update_enrollment(is_active=True, mode=mode)
|
||||
return enrollment
|
||||
|
||||
@@ -6,6 +6,7 @@ adding users, removing users, and listing members
|
||||
from abc import ABCMeta, abstractmethod
|
||||
|
||||
from django.contrib.auth.models import User
|
||||
|
||||
from student.models import CourseAccessRole
|
||||
from xmodule_django.models import CourseKeyField
|
||||
|
||||
@@ -127,6 +128,7 @@ class RoleBase(AccessRole):
|
||||
"""
|
||||
# silently ignores anonymous and inactive users so that any that are
|
||||
# legit get updated.
|
||||
from student.models import CourseAccessRole
|
||||
for user in users:
|
||||
if user.is_authenticated and user.is_active and not self.has_user(user):
|
||||
entry = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org)
|
||||
|
||||
@@ -13,9 +13,12 @@ from xmodule.modulestore.tests.django_utils import (
|
||||
ModuleStoreTestCase, mixed_store_config
|
||||
)
|
||||
from xmodule.modulestore.tests.factories import CourseFactory
|
||||
from social.strategies.django_strategy import DjangoStrategy
|
||||
from django.test.client import RequestFactory
|
||||
from student.tests.factories import UserFactory, CourseModeFactory
|
||||
from student.models import CourseEnrollment
|
||||
|
||||
from student.views import register_user
|
||||
from third_party_auth.pipeline import change_enrollment as change_enrollment_third_party
|
||||
|
||||
# Since we don't need any XML course fixtures, use a modulestore configuration
|
||||
# that disables the XML modulestore.
|
||||
@@ -153,6 +156,25 @@ class EnrollmentTest(ModuleStoreTestCase):
|
||||
self.assertIn('auto_register', self.client.session)
|
||||
self.assertTrue(self.client.session['auto_register'])
|
||||
|
||||
def test_enroll_from_redirect_autoreg_third_party(self):
|
||||
"""
|
||||
Test that, when a user visits the registration page *after* visiting a course,
|
||||
if they go on to register and/or log in via third-party auth, they'll be registered
|
||||
in that course.
|
||||
|
||||
The testing here is a bit hackish, since we just ping the registration page, then
|
||||
directly call the step in the third party pipeline that registers the user if
|
||||
`registration_course_id` is set in the session, but it should catch any major breaks.
|
||||
"""
|
||||
self.client.logout()
|
||||
self.client.get(reverse('register_user'), {'course_id': self.course.id})
|
||||
self.client.login(username=self.USERNAME, password=self.PASSWORD)
|
||||
self.dummy_request = RequestFactory().request()
|
||||
self.dummy_request.session = self.client.session
|
||||
strategy = DjangoStrategy(RequestFactory, request=self.dummy_request)
|
||||
change_enrollment_third_party(is_register=True, strategy=strategy, user=self.user)
|
||||
self.assertTrue(CourseEnrollment.is_enrolled(self.user, self.course.id))
|
||||
|
||||
# TODO (ECOM-16): Remove once the auto-registration A/B test completes
|
||||
def test_enroll_auto_registration_excluded_course(self):
|
||||
# Create the course modes
|
||||
|
||||
@@ -626,7 +626,7 @@ def try_change_enrollment(request):
|
||||
|
||||
|
||||
@require_POST
|
||||
def change_enrollment(request, auto_register=False):
|
||||
def change_enrollment(request, auto_register=False, check_access=True):
|
||||
"""
|
||||
Modify the enrollment status for the logged-in user.
|
||||
|
||||
@@ -661,20 +661,7 @@ def change_enrollment(request, auto_register=False):
|
||||
Response
|
||||
|
||||
"""
|
||||
user = request.user
|
||||
|
||||
action = request.POST.get("enrollment_action")
|
||||
if 'course_id' not in request.POST:
|
||||
return HttpResponseBadRequest(_("Course id not specified"))
|
||||
|
||||
try:
|
||||
course_id = SlashSeparatedCourseKey.from_deprecated_string(request.POST.get("course_id"))
|
||||
except InvalidKeyError:
|
||||
log.warning("User {username} tried to {action} with invalid course id: {course_id}".format(
|
||||
username=user.username, action=action, course_id=request.POST.get("course_id")
|
||||
))
|
||||
return HttpResponseBadRequest(_("Invalid course id"))
|
||||
|
||||
# Sets the auto_register flag, if that's desired
|
||||
# TODO (ECOM-16): Remove this once the auto-registration A/B test completes
|
||||
# If a user is in the experimental condition (auto-registration enabled),
|
||||
# immediately set a session flag so they stay in the experimental condition.
|
||||
@@ -686,6 +673,31 @@ def change_enrollment(request, auto_register=False):
|
||||
if request.session.get('auto_register') and not auto_register:
|
||||
auto_register = True
|
||||
|
||||
# Get the user
|
||||
user = request.user
|
||||
|
||||
# Ensure the user is authenticated
|
||||
if not user.is_authenticated():
|
||||
return HttpResponseForbidden()
|
||||
|
||||
# Ensure we received a course_id
|
||||
action = request.POST.get("enrollment_action")
|
||||
if 'course_id' not in request.POST:
|
||||
return HttpResponseBadRequest(_("Course id not specified"))
|
||||
|
||||
try:
|
||||
course_id = SlashSeparatedCourseKey.from_deprecated_string(request.POST.get("course_id"))
|
||||
except InvalidKeyError:
|
||||
log.warning(
|
||||
"User {username} tried to {action} with invalid course id: {course_id}".format(
|
||||
username=user.username,
|
||||
action=action,
|
||||
course_id=request.POST.get("course_id")
|
||||
)
|
||||
)
|
||||
return HttpResponseBadRequest(_("Invalid course id"))
|
||||
|
||||
# Don't execute auto-register for the set of courses excluded from auto-registration
|
||||
# TODO (ECOM-16): Remove this once the auto-registration A/B test completes
|
||||
# We've agreed to exclude certain courses from the A/B test. If we find ourselves
|
||||
# registering for one of these courses, immediately switch to the control.
|
||||
@@ -694,34 +706,7 @@ def change_enrollment(request, auto_register=False):
|
||||
if 'auto_register' in request.session:
|
||||
del request.session['auto_register']
|
||||
|
||||
if not user.is_authenticated():
|
||||
return HttpResponseForbidden()
|
||||
|
||||
if action == "enroll":
|
||||
# Make sure the course exists
|
||||
# We don't do this check on unenroll, or a bad course id can't be unenrolled from
|
||||
try:
|
||||
course = modulestore().get_course(course_id)
|
||||
except ItemNotFoundError:
|
||||
log.warning("User {0} tried to enroll in non-existent course {1}"
|
||||
.format(user.username, course_id))
|
||||
return HttpResponseBadRequest(_("Course id is invalid"))
|
||||
|
||||
if not has_access(user, 'enroll', course):
|
||||
return HttpResponseBadRequest(_("Enrollment is closed"))
|
||||
|
||||
# see if we have already filled up all allowed enrollments
|
||||
is_course_full = CourseEnrollment.is_course_full(course)
|
||||
|
||||
if is_course_full:
|
||||
return HttpResponseBadRequest(_("Course is full"))
|
||||
|
||||
# check to see if user is currently enrolled in that course
|
||||
if CourseEnrollment.is_enrolled(user, course_id):
|
||||
return HttpResponseBadRequest(
|
||||
_("Student is already enrolled")
|
||||
)
|
||||
|
||||
# We use this flag to determine which condition of an AB-test
|
||||
# for auto-registration we're currently in.
|
||||
# (We have two URLs that both point to this view, but vary the
|
||||
@@ -747,7 +732,10 @@ def change_enrollment(request, auto_register=False):
|
||||
# by its slug. If they do, it's possible (based on the state of the database)
|
||||
# for no such model to exist, even though we've set the enrollment type
|
||||
# to "honor".
|
||||
CourseEnrollment.enroll(user, course.id)
|
||||
try:
|
||||
CourseEnrollment.enroll(user, course_id, check_access=check_access)
|
||||
except Exception:
|
||||
return HttpResponseBadRequest(_("Could not enroll"))
|
||||
|
||||
# If we have more than one course mode or professional ed is enabled,
|
||||
# then send the user to the choose your track page.
|
||||
@@ -780,7 +768,10 @@ def change_enrollment(request, auto_register=False):
|
||||
reverse("course_modes_choose", kwargs={'course_id': unicode(course_id)})
|
||||
)
|
||||
|
||||
CourseEnrollment.enroll(user, course.id, mode=current_mode.slug)
|
||||
try:
|
||||
CourseEnrollment.enroll(user, course_id, mode=current_mode.slug, check_access=check_access)
|
||||
except Exception:
|
||||
return HttpResponseBadRequest(_("Could not enroll"))
|
||||
|
||||
return HttpResponse()
|
||||
|
||||
|
||||
@@ -69,6 +69,11 @@ from social.apps.django_app.default import models
|
||||
from social.exceptions import AuthException
|
||||
from social.pipeline import partial
|
||||
|
||||
from student.models import CourseEnrollment, CourseEnrollmentException
|
||||
from opaque_keys.edx.locations import SlashSeparatedCourseKey
|
||||
|
||||
from logging import getLogger
|
||||
|
||||
from . import provider
|
||||
|
||||
|
||||
@@ -86,6 +91,8 @@ _AUTH_ENTRY_CHOICES = frozenset([
|
||||
_DEFAULT_RANDOM_PASSWORD_LENGTH = 12
|
||||
_PASSWORD_CHARSET = string.letters + string.digits
|
||||
|
||||
logger = getLogger(__name__)
|
||||
|
||||
|
||||
class AuthEntryError(AuthException):
|
||||
"""Raised when auth_entry is missing or invalid on URLs.
|
||||
@@ -373,6 +380,7 @@ def redirect_to_supplementary_form(strategy, details, response, uid, is_dashboar
|
||||
|
||||
@partial.partial
|
||||
def login_analytics(*args, **kwargs):
|
||||
""" Sends login info to Segment.io """
|
||||
event_name = None
|
||||
|
||||
action_to_event_name = {
|
||||
@@ -404,3 +412,22 @@ def login_analytics(*args, **kwargs):
|
||||
}
|
||||
}
|
||||
)
|
||||
|
||||
#@partial.partial
|
||||
def change_enrollment(*args, **kwargs):
|
||||
"""
|
||||
If the user accessed the third party auth flow after trying to register for
|
||||
a course, we automatically log them into that course.
|
||||
"""
|
||||
if kwargs['strategy'].session_get('registration_course_id'):
|
||||
try:
|
||||
CourseEnrollment.enroll(
|
||||
kwargs['user'],
|
||||
SlashSeparatedCourseKey.from_deprecated_string(
|
||||
kwargs['strategy'].session_get('registration_course_id')
|
||||
)
|
||||
)
|
||||
except CourseEnrollmentException:
|
||||
pass
|
||||
except Exception, e:
|
||||
logger.exception(e)
|
||||
|
||||
@@ -117,6 +117,7 @@ def _set_global_settings(django_settings):
|
||||
'social.pipeline.social_auth.load_extra_data',
|
||||
'social.pipeline.user.user_details',
|
||||
'third_party_auth.pipeline.login_analytics',
|
||||
'third_party_auth.pipeline.change_enrollment',
|
||||
)
|
||||
|
||||
# We let the user specify their email address during signup.
|
||||
|
||||
@@ -14,15 +14,14 @@ from xmodule.x_module import XModule
|
||||
|
||||
from xblock.core import XBlock
|
||||
|
||||
from student.models import CourseEnrollmentAllowed
|
||||
from external_auth.models import ExternalAuthMap
|
||||
from courseware.masquerade import is_masquerading_as_student
|
||||
from django.utils.timezone import UTC
|
||||
from student.models import CourseEnrollment
|
||||
from student.roles import (
|
||||
GlobalStaff, CourseStaffRole, CourseInstructorRole,
|
||||
OrgStaffRole, OrgInstructorRole, CourseBetaTesterRole
|
||||
)
|
||||
from student.models import CourseEnrollment, CourseEnrollmentAllowed
|
||||
from opaque_keys.edx.keys import CourseKey, UsageKey
|
||||
DEBUG_ACCESS = False
|
||||
|
||||
|
||||
@@ -116,6 +116,7 @@ class LoginEnrollmentTestCase(TestCase):
|
||||
resp = self.client.post(reverse('change_enrollment'), {
|
||||
'enrollment_action': 'enroll',
|
||||
'course_id': course.id.to_deprecated_string(),
|
||||
'check_access': True,
|
||||
})
|
||||
result = resp.status_code == 200
|
||||
if verify:
|
||||
|
||||
Reference in New Issue
Block a user