From 79ca56c3db7c529e2bc6baf94aadd66a51e19364 Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Wed, 3 Dec 2014 22:47:40 +0000 Subject: [PATCH 1/2] Allowing email optin to work on register. --- .../djangoapps/user_api/tests/test_views.py | 15 ++++++ common/djangoapps/user_api/views.py | 48 ++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 76f839fb48..17069f2171 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -1526,3 +1526,18 @@ class UpdateEmailOptInTestCase(ApiTestCase): response = self.client.post(self.url, params) self.assertHttpBadRequest(response) + + def test_update_email_opt_in_inactive_user(self): + """Test that an inactive user can still update email.""" + self.user.is_active = False + self.user.save() + # Register, which should trigger an activation email + response = self.client.post(self.url, { + "course_id": unicode(self.course.id), + "email_opt_in": u"True" + }) + self.assertHttpOK(response) + preference = UserOrgTag.objects.get( + user=self.user, org=self.course.id.org, key="email-optin" + ) + self.assertEquals(preference.value, u"True") diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 39ce384eed..73b066efea 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -47,6 +47,52 @@ class ApiKeyHeaderPermission(permissions.BasePermission): ) +class SessionAuthenticationAllowInactiveUser(authentication.SessionAuthentication): + """Ensure that the user is logged in, but do not require the account to be active. + + We use this in the special case that a user has created an account, + but has not yet activated it. We still want to allow the user to + enroll in courses, so we remove the usual restriction + on session authentication that requires an active account. + + You should use this authentication class ONLY for end-points that + it's safe for an un-activated user to access. For example, + we can allow a user to update his/her own enrollments without + activating an account. + + """ + def authenticate(self, request): + """Authenticate the user, requiring a logged-in account and CSRF. + + This is exactly the same as the `SessionAuthentication` implementation, + with the `user.is_active` check removed. + + Args: + request (HttpRequest) + + Returns: + Tuple of `(user, token)` + + Raises: + PermissionDenied: The CSRF token check failed. + + """ + # Get the underlying HttpRequest object + request = request._request # pylint: disable=protected-access + user = getattr(request, 'user', None) + + # Unauthenticated, CSRF validation not required + # This is where regular `SessionAuthentication` checks that the user is active. + # We have removed that check in this implementation. + if not user: + return None + + self.enforce_csrf(request) + + # CSRF passed with authenticated user + return (user, None) + + class LoginSessionView(APIView): """HTTP end-points for logging in users. """ @@ -842,7 +888,7 @@ class PreferenceUsersListView(generics.ListAPIView): class UpdateEmailOptInPreference(APIView): """View for updating the email opt in preference. """ - authentication_classes = (authentication.SessionAuthentication,) + authentication_classes = (SessionAuthenticationAllowInactiveUser,) @method_decorator(require_post_params(["course_id", "email_opt_in"])) @method_decorator(ensure_csrf_cookie) From 7c78a0999a09873e67303e104f2a12bd021aa1f2 Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Thu, 4 Dec 2014 16:16:32 +0000 Subject: [PATCH 2/2] Code review comments --- common/djangoapps/enrollment/views.py | 47 +----------------- .../djangoapps/user_api/tests/test_views.py | 2 +- common/djangoapps/user_api/views.py | 47 +----------------- common/djangoapps/util/authentication.py | 48 +++++++++++++++++++ 4 files changed, 51 insertions(+), 93 deletions(-) create mode 100644 common/djangoapps/util/authentication.py diff --git a/common/djangoapps/enrollment/views.py b/common/djangoapps/enrollment/views.py index 2b0f409e18..73d4ced3d6 100644 --- a/common/djangoapps/enrollment/views.py +++ b/common/djangoapps/enrollment/views.py @@ -11,6 +11,7 @@ from rest_framework.response import Response from rest_framework.throttling import UserRateThrottle from enrollment import api from student.models import NonExistentCourseError, CourseEnrollmentException +from util.authentication import SessionAuthenticationAllowInactiveUser class EnrollmentUserThrottle(UserRateThrottle): @@ -19,52 +20,6 @@ class EnrollmentUserThrottle(UserRateThrottle): rate = '50/second' -class SessionAuthenticationAllowInactiveUser(SessionAuthentication): - """Ensure that the user is logged in, but do not require the account to be active. - - We use this in the special case that a user has created an account, - but has not yet activated it. We still want to allow the user to - enroll in courses, so we remove the usual restriction - on session authentication that requires an active account. - - You should use this authentication class ONLY for end-points that - it's safe for an unactived user to access. For example, - we can allow a user to update his/her own enrollments without - activating an account. - - """ - def authenticate(self, request): - """Authenticate the user, requiring a logged-in account and CSRF. - - This is exactly the same as the `SessionAuthentication` implementation, - with the `user.is_active` check removed. - - Args: - request (HttpRequest) - - Returns: - Tuple of `(user, token)` - - Raises: - PermissionDenied: The CSRF token check failed. - - """ - # Get the underlying HttpRequest object - request = request._request # pylint: disable=protected-access - user = getattr(request, 'user', None) - - # Unauthenticated, CSRF validation not required - # This is where regular `SessionAuthentication` checks that the user is active. - # We have removed that check in this implementation. - if not user: - return None - - self.enforce_csrf(request) - - # CSRF passed with authenticated user - return (user, None) - - @api_view(['GET']) @authentication_classes((OAuth2Authentication, SessionAuthentication)) @permission_classes((IsAuthenticated,)) diff --git a/common/djangoapps/user_api/tests/test_views.py b/common/djangoapps/user_api/tests/test_views.py index 17069f2171..69380eb220 100644 --- a/common/djangoapps/user_api/tests/test_views.py +++ b/common/djangoapps/user_api/tests/test_views.py @@ -1528,7 +1528,7 @@ class UpdateEmailOptInTestCase(ApiTestCase): self.assertHttpBadRequest(response) def test_update_email_opt_in_inactive_user(self): - """Test that an inactive user can still update email.""" + """Test that an inactive user can still update their email optin preference.""" self.user.is_active = False self.user.save() # Register, which should trigger an activation email diff --git a/common/djangoapps/user_api/views.py b/common/djangoapps/user_api/views.py index 73b066efea..41e69aa3f5 100644 --- a/common/djangoapps/user_api/views.py +++ b/common/djangoapps/user_api/views.py @@ -26,6 +26,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from edxmako.shortcuts import marketing_link import third_party_auth +from util.authentication import SessionAuthenticationAllowInactiveUser from user_api.api import account as account_api, profile as profile_api from user_api.helpers import FormDescription, shim_student_view, require_post_params @@ -47,52 +48,6 @@ class ApiKeyHeaderPermission(permissions.BasePermission): ) -class SessionAuthenticationAllowInactiveUser(authentication.SessionAuthentication): - """Ensure that the user is logged in, but do not require the account to be active. - - We use this in the special case that a user has created an account, - but has not yet activated it. We still want to allow the user to - enroll in courses, so we remove the usual restriction - on session authentication that requires an active account. - - You should use this authentication class ONLY for end-points that - it's safe for an un-activated user to access. For example, - we can allow a user to update his/her own enrollments without - activating an account. - - """ - def authenticate(self, request): - """Authenticate the user, requiring a logged-in account and CSRF. - - This is exactly the same as the `SessionAuthentication` implementation, - with the `user.is_active` check removed. - - Args: - request (HttpRequest) - - Returns: - Tuple of `(user, token)` - - Raises: - PermissionDenied: The CSRF token check failed. - - """ - # Get the underlying HttpRequest object - request = request._request # pylint: disable=protected-access - user = getattr(request, 'user', None) - - # Unauthenticated, CSRF validation not required - # This is where regular `SessionAuthentication` checks that the user is active. - # We have removed that check in this implementation. - if not user: - return None - - self.enforce_csrf(request) - - # CSRF passed with authenticated user - return (user, None) - - class LoginSessionView(APIView): """HTTP end-points for logging in users. """ diff --git a/common/djangoapps/util/authentication.py b/common/djangoapps/util/authentication.py new file mode 100644 index 0000000000..901f3ad6e8 --- /dev/null +++ b/common/djangoapps/util/authentication.py @@ -0,0 +1,48 @@ +""" Common Authentication Handlers used across projects. """ +from rest_framework import authentication + + +class SessionAuthenticationAllowInactiveUser(authentication.SessionAuthentication): + """Ensure that the user is logged in, but do not require the account to be active. + + We use this in the special case that a user has created an account, + but has not yet activated it. We still want to allow the user to + enroll in courses, so we remove the usual restriction + on session authentication that requires an active account. + + You should use this authentication class ONLY for end-points that + it's safe for an un-activated user to access. For example, + we can allow a user to update his/her own enrollments without + activating an account. + + """ + def authenticate(self, request): + """Authenticate the user, requiring a logged-in account and CSRF. + + This is exactly the same as the `SessionAuthentication` implementation, + with the `user.is_active` check removed. + + Args: + request (HttpRequest) + + Returns: + Tuple of `(user, token)` + + Raises: + PermissionDenied: The CSRF token check failed. + + """ + # Get the underlying HttpRequest object + request = request._request # pylint: disable=protected-access + user = getattr(request, 'user', None) + + # Unauthenticated, CSRF validation not required + # This is where regular `SessionAuthentication` checks that the user is active. + # We have removed that check in this implementation. + if not user: + return None + + self.enforce_csrf(request) + + # CSRF passed with authenticated user + return (user, None)