From 7c78a0999a09873e67303e104f2a12bd021aa1f2 Mon Sep 17 00:00:00 2001 From: stephensanchez Date: Thu, 4 Dec 2014 16:16:32 +0000 Subject: [PATCH] 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)