From 4d77b0552f8a0e92228ba8eca4b32dba5f208b76 Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <julia@edx.org>
Date: Mon, 22 Sep 2014 20:00:25 +0000
Subject: [PATCH 1/5] Move server-side validation to enroll method

---
 common/djangoapps/student/models.py | 67 ++++++++++++++++++++++++++++-
 common/djangoapps/student/views.py  | 51 +++++++---------------
 2 files changed, 81 insertions(+), 37 deletions(-)

diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py
index 89e81d6529..056b514180 100644
--- a/common/djangoapps/student/models.py
+++ b/common/djangoapps/student/models.py
@@ -589,6 +589,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
@@ -795,10 +811,59 @@ class CourseEnrollment(models.Model):
                until we have these mapped out.
 
         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.
         """
+
+        # All the server-side checks for whether a user is allowed to enroll.
+        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")
+                )
+            )
+            raise CourseEnrollmentException
+        try:
+            course = modulestore().get_course(course_id)
+        except ItemNotFoundError:
+            log.warning(
+                "User {0} failed to enroll in non-existent course {1}".format(
+                    user.username,
+                    course_id
+                )
+            )
+            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_id
+                )
+            )
+            raise EnrollmentClosedError
+        if CourseEnrollment.is_course_full(course):
+            log.warning(
+                "User {0} failed to enroll in full course {1}".format(
+                    user.username,
+                    course_id
+                )
+            )
+            raise CourseFullError
+        if CourseEnrollment.is_enrolled(user, course_id):
+            log.warning(
+                "User {0} attempted to enroll in {1}, but they were already enrolled".format(
+                    user.username,
+                    course_id
+                )
+            )
+            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/views.py b/common/djangoapps/student/views.py
index 093f2f5360..c43ae54f6f 100644
--- a/common/djangoapps/student/views.py
+++ b/common/djangoapps/student/views.py
@@ -661,20 +661,19 @@ def change_enrollment(request, auto_register=False):
         Response
 
     """
+    # Get the user
     user = request.user
 
+    # 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"))
+    # 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.
@@ -686,6 +685,7 @@ def change_enrollment(request, auto_register=False):
     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
     # registering for one of these courses, immediately switch to the control.
@@ -694,34 +694,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 +720,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, mode=current_mode.slug)
+                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 +756,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)
+            except Exception:
+                return HttpResponseBadRequest(_("Could not enroll"))
 
             return HttpResponse()
 

From 5b3c69bc87620144c21e9479cdf036aa476bcc60 Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <julia@edx.org>
Date: Tue, 23 Sep 2014 06:19:36 +0000
Subject: [PATCH 2/5] Tests fixed

---
 common/djangoapps/student/models.py           | 31 ++++++++-----------
 common/djangoapps/student/roles.py            |  9 +++++-
 common/djangoapps/student/views.py            | 16 ++++++++--
 .../djangoapps/third_party_auth/pipeline.py   |  7 +++++
 .../djangoapps/third_party_auth/settings.py   |  1 +
 lms/djangoapps/courseware/access.py           |  4 +--
 6 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py
index 056b514180..7ef8e5a945 100644
--- a/common/djangoapps/student/models.py
+++ b/common/djangoapps/student/models.py
@@ -38,13 +38,16 @@ 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
+from courseware.access import has_access
 
 from certificates.models import GeneratedCertificate
 from course_modes.models import CourseMode
@@ -818,31 +821,23 @@ class CourseEnrollment(models.Model):
 
         # All the server-side checks for whether a user is allowed to enroll.
         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")
-                )
-            )
-            raise CourseEnrollmentException
-        try:
-            course = modulestore().get_course(course_id)
+            course = modulestore().get_course(course_key)
         except ItemNotFoundError:
             log.warning(
                 "User {0} failed to enroll in non-existent course {1}".format(
                     user.username,
-                    course_id
+                    course_key.to_deprecated_string()
                 )
             )
             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_id
+                    course_key.to_deprecated_string()
                 )
             )
             raise EnrollmentClosedError
@@ -850,15 +845,15 @@ class CourseEnrollment(models.Model):
             log.warning(
                 "User {0} failed to enroll in full course {1}".format(
                     user.username,
-                    course_id
+                    course_key.to_deprecated_string()
                 )
             )
             raise CourseFullError
-        if CourseEnrollment.is_enrolled(user, course_id):
+        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_id
+                    course_key.to_deprecated_string()
                 )
             )
             raise AlreadyEnrolledError
diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py
index 6061ce1ea2..6594248595 100644
--- a/common/djangoapps/student/roles.py
+++ b/common/djangoapps/student/roles.py
@@ -6,7 +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
 
 
@@ -15,6 +15,7 @@ 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()
         )
@@ -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)
@@ -138,6 +140,7 @@ 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
         )
@@ -174,6 +177,7 @@ 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()
 
 
@@ -267,6 +271,7 @@ 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)
@@ -280,6 +285,7 @@ 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'):
@@ -294,4 +300,5 @@ 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/views.py b/common/djangoapps/student/views.py
index c43ae54f6f..dc2cbfb69e 100644
--- a/common/djangoapps/student/views.py
+++ b/common/djangoapps/student/views.py
@@ -669,6 +669,18 @@ def change_enrollment(request, auto_register=False):
     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"))
+
     # Ensure the user is authenticated
     if not user.is_authenticated():
         return HttpResponseForbidden()
@@ -721,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, mode=current_mode.slug)
                 except Exception:
                     return HttpResponseBadRequest(_("Could not enroll"))
 
@@ -757,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)
             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 dc9d021e5e..bbb481d560 100644
--- a/common/djangoapps/third_party_auth/pipeline.py
+++ b/common/djangoapps/third_party_auth/pipeline.py
@@ -399,3 +399,10 @@ def login_analytics(*args, **kwargs):
                 }
             }
         )
+
+@partial.partial
+def change_enrollment(*args, **kwargs):
+    try:
+        CourseEnrollment.enroll(user, 'foo')
+    except:
+        pass
diff --git a/common/djangoapps/third_party_auth/settings.py b/common/djangoapps/third_party_auth/settings.py
index 07e46a3bed..fdd93e92cb 100644
--- a/common/djangoapps/third_party_auth/settings.py
+++ b/common/djangoapps/third_party_auth/settings.py
@@ -110,6 +110,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..428a54d925 100644
--- a/lms/djangoapps/courseware/access.py
+++ b/lms/djangoapps/courseware/access.py
@@ -14,11 +14,9 @@ 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
@@ -125,6 +123,7 @@ 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
             (
@@ -149,6 +148,7 @@ 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):

From 3fbbe57cef4fc0d0f5f3eb2d80f9c85c7e9e67a1 Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <julia@edx.org>
Date: Tue, 23 Sep 2014 12:10:56 +0000
Subject: [PATCH 3/5] Add auto-registration to third party login flow

---
 .../djangoapps/third_party_auth/pipeline.py   | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py
index bbb481d560..a6747ca7af 100644
--- a/common/djangoapps/third_party_auth/pipeline.py
+++ b/common/djangoapps/third_party_auth/pipeline.py
@@ -69,6 +69,9 @@ from social.apps.django_app.default import models
 from social.exceptions import AuthException
 from social.pipeline import partial
 
+from student.models import CourseEnrollment
+from opaque_keys.edx.locations import SlashSeparatedCourseKey
+
 from . import provider
 
 
@@ -369,6 +372,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 = {
@@ -402,7 +406,17 @@ def login_analytics(*args, **kwargs):
 
 @partial.partial
 def change_enrollment(*args, **kwargs):
-    try:
-        CourseEnrollment.enroll(user, 'foo')
-    except:
-        pass
+    """
+    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:
+            pass

From 5d5ff8d9c17903e60d6ce8ced6dc3843ff161038 Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <julia@edx.org>
Date: Tue, 23 Sep 2014 12:22:59 +0000
Subject: [PATCH 4/5] Docs

---
 common/djangoapps/student/models.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py
index 7ef8e5a945..269dae81d3 100644
--- a/common/djangoapps/student/models.py
+++ b/common/djangoapps/student/models.py
@@ -813,6 +813,11 @@ class CourseEnrollment(models.Model):
                may include "audit", "verified_id", etc. Please don't use it
                until we have these mapped out.
 
+        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.
 

From 6b061ad23b67fd34c5c652cff88c2049988f139c Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <julia@edx.org>
Date: Tue, 23 Sep 2014 13:37:19 +0000
Subject: [PATCH 5/5] Response to CR

---
 common/djangoapps/student/models.py           | 49 +++++++++++--------
 common/djangoapps/student/roles.py            |  7 +--
 .../student/tests/test_enrollment.py          | 24 ++++++++-
 common/djangoapps/student/views.py            | 38 +++++++-------
 .../djangoapps/third_party_auth/pipeline.py   | 12 +++--
 lms/djangoapps/courseware/access.py           |  3 +-
 lms/djangoapps/courseware/tests/helpers.py    |  1 +
 7 files changed, 83 insertions(+), 51 deletions(-)

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: