diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 3745d407ea..411a5cd9de 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -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 diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 6061ce1ea2..8fb38b5d14 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -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) diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index 9621dc16d8..6defca14f0 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -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 diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 093f2f5360..e13100560d 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -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() diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 9d472c4ffb..c76e5199bb 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -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) diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py index a9b80f1794..ecb430581f 100644 --- a/common/djangoapps/third_party_auth/settings.py +++ b/common/djangoapps/third_party_auth/settings.py @@ -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. diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 9c900185ab..c29d76e533 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -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 diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 82cc606e9c..d0bf1bc45c 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -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: