diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 269dae81d3..1b7607b004 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -47,7 +47,6 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.django import modulestore from opaque_keys.edx.keys import CourseKey from functools import total_ordering -from courseware.access import has_access from certificates.models import GeneratedCertificate from course_modes.models import CourseMode @@ -796,7 +795,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. @@ -806,13 +805,19 @@ 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 @@ -823,6 +828,7 @@ class CourseEnrollment(models.Model): 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: @@ -835,25 +841,27 @@ class CourseEnrollment(models.Model): ) ) raise NonExistentCourseError - 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() + 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 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 + raise CourseFullError if CourseEnrollment.is_enrolled(user, course_key): log.warning( "User {0} attempted to enroll in {1}, but they were already enrolled".format( @@ -861,7 +869,8 @@ class CourseEnrollment(models.Model): course_key.to_deprecated_string() ) ) - raise AlreadyEnrolledError + 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) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 6594248595..8fb38b5d14 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -7,6 +7,7 @@ from abc import ABCMeta, abstractmethod from django.contrib.auth.models import User +from student.models import CourseAccessRole from xmodule_django.models import CourseKeyField @@ -15,7 +16,6 @@ class RoleCache(object): A cache of the CourseAccessRoles held by a particular user """ def __init__(self, user): - from student.models import CourseAccessRole self._roles = set( CourseAccessRole.objects.filter(user=user).all() ) @@ -140,7 +140,6 @@ class RoleBase(AccessRole): """ Remove the supplied django users from this role. """ - from student.models import CourseAccessRole entries = CourseAccessRole.objects.filter( user__in=users, role=self._role_name, org=self.org, course_id=self.course_key ) @@ -177,7 +176,6 @@ class CourseRole(RoleBase): @classmethod def course_group_already_exists(self, course_key): - from student.models import CourseAccessRole return CourseAccessRole.objects.filter(org=course_key.org, course_id=course_key).exists() @@ -271,7 +269,6 @@ class UserBasedRole(object): """ Grant this object's user the object's role for the supplied courses """ - from student.models import CourseAccessRole if self.user.is_authenticated and self.user.is_active: for course_key in course_keys: entry = CourseAccessRole(user=self.user, role=self.role, course_id=course_key, org=course_key.org) @@ -285,7 +282,6 @@ class UserBasedRole(object): """ Remove the supplied courses from this user's configured role. """ - from student.models import CourseAccessRole entries = CourseAccessRole.objects.filter(user=self.user, role=self.role, course_id__in=course_keys) entries.delete() if hasattr(self.user, '_roles'): @@ -300,5 +296,4 @@ class UserBasedRole(object): * course_id * role (will be self.role--thus uninteresting) """ - from student.models import CourseAccessRole return CourseAccessRole.objects.filter(role=self.role, user=self.user) 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 dc2cbfb69e..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,9 +661,25 @@ def change_enrollment(request, auto_register=False): Response """ + # 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. + # We keep them in the experimental condition even if later on the user + # tries to register using the control URL (e.g. because of a redirect from the login page, + # which is hard-coded to use the control URL). + if auto_register: + request.session['auto_register'] = True + 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: @@ -681,22 +697,6 @@ def change_enrollment(request, auto_register=False): ) return HttpResponseBadRequest(_("Invalid course id")) - # Ensure the user is authenticated - if not user.is_authenticated(): - return HttpResponseForbidden() - - # 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. - # We keep them in the experimental condition even if later on the user - # tries to register using the control URL (e.g. because of a redirect from the login page, - # which is hard-coded to use the control URL). - if auto_register: - request.session['auto_register'] = True - if request.session.get('auto_register') and not auto_register: - auto_register = True - # 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 @@ -733,7 +733,7 @@ def change_enrollment(request, auto_register=False): # for no such model to exist, even though we've set the enrollment type # to "honor". try: - CourseEnrollment.enroll(user, course_id, mode=current_mode.slug) + CourseEnrollment.enroll(user, course_id, check_access=check_access) except Exception: return HttpResponseBadRequest(_("Could not enroll")) @@ -769,7 +769,7 @@ def change_enrollment(request, auto_register=False): ) try: - CourseEnrollment.enroll(user, course_id, mode=current_mode.slug) + CourseEnrollment.enroll(user, course_id, mode=current_mode.slug, check_access=check_access) except Exception: return HttpResponseBadRequest(_("Could not enroll")) diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index a6747ca7af..4d8a82aa59 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -69,9 +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 +from student.models import CourseEnrollment, CourseEnrollmentException from opaque_keys.edx.locations import SlashSeparatedCourseKey +from logging import getLogger + from . import provider @@ -87,6 +89,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. @@ -404,7 +408,7 @@ def login_analytics(*args, **kwargs): } ) -@partial.partial +#@partial.partial def change_enrollment(*args, **kwargs): """ If the user accessed the third party auth flow after trying to register for @@ -418,5 +422,7 @@ def change_enrollment(*args, **kwargs): kwargs['strategy'].session_get('registration_course_id') ) ) - except: + except CourseEnrollmentException: pass + except Exception, e: + logger.exception(e) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 428a54d925..c29d76e533 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -21,6 +21,7 @@ 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 @@ -123,7 +124,6 @@ def _has_access_course_desc(user, action, course): """ Can this user access the forums in this course? """ - from student.models import CourseEnrollment return ( can_load() and ( @@ -148,7 +148,6 @@ def _has_access_course_desc(user, action, course): """ # if using registration method to restrict (say shibboleth) - from student.models import CourseEnrollmentAllowed if settings.FEATURES.get('RESTRICT_ENROLL_BY_REG_METHOD') and course.enrollment_domain: if user is not None and user.is_authenticated() and \ ExternalAuthMap.objects.filter(user=user, external_domain=course.enrollment_domain): 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: