Code review comments
This commit is contained in:
@@ -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,))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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. """
|
||||
|
||||
|
||||
48
common/djangoapps/util/authentication.py
Normal file
48
common/djangoapps/util/authentication.py
Normal file
@@ -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)
|
||||
Reference in New Issue
Block a user