Skip to content

Commit

Permalink
Add toggle to determine whether to set request.user.
Browse files Browse the repository at this point in the history
Although setting the `request.user` during `process_view` worked to
solve ARCH-1197 in ecommerce, it introduced an OOM bug in edx-platform.

This introduces a temporary toggle using a Django Setting so that the
fix can remain in ecommerce without affecting other IDAs like
edx-platform.

Once ARCH-1199 is complete and the issue is fixed, it is expected that
this toggle will be removed.

ARCH-1210
  • Loading branch information
robrap committed Oct 16, 2019
1 parent c31cf4e commit 86256f1
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 54 deletions.
2 changes: 1 addition & 1 deletion edx_rest_framework_extensions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
""" edx Django REST Framework extensions. """

__version__ = '2.4.1' # pragma: no cover
__version__ = '2.4.2' # pragma: no cover
5 changes: 4 additions & 1 deletion edx_rest_framework_extensions/auth/jwt/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
from rest_framework.request import Request
from rest_framework_jwt.authentication import BaseJSONWebTokenAuthentication

from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
from edx_rest_framework_extensions.auth.jwt.cookies import (
jwt_cookie_name,
jwt_cookie_header_payload_name,
jwt_cookie_signature_name,
)
from edx_rest_framework_extensions.auth.jwt.constants import JWT_DELIMITER
from edx_rest_framework_extensions.permissions import LoginRedirectIfUnauthenticated, NotJwtRestrictedApplication
from edx_rest_framework_extensions.settings import get_setting

log = logging.getLogger(__name__)
USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE'
Expand Down Expand Up @@ -203,7 +205,8 @@ def process_view(self, request, view_func, view_args, view_kwargs): # pylint: d
header_payload_cookie = request.COOKIES.get(jwt_cookie_header_payload_name())
signature_cookie = request.COOKIES.get(jwt_cookie_signature_name())

if use_jwt_cookie_requested:
is_set_request_user_for_jwt_cookie_enabled = get_setting(ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE)
if use_jwt_cookie_requested and is_set_request_user_for_jwt_cookie_enabled:
# DRF does not set request.user until process_response. This makes it available in process_view.
# For more info, see https://github.com/jpadilla/django-rest-framework-jwt/issues/45#issuecomment-74996698
request.user = SimpleLazyObject(lambda: _get_user_from_jwt(request, view_func))
Expand Down
86 changes: 40 additions & 46 deletions edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from rest_framework.views import APIView
from rest_framework.viewsets import ViewSet

from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
from edx_rest_framework_extensions.auth.jwt.cookies import (
jwt_cookie_name,
jwt_cookie_header_payload_name,
Expand Down Expand Up @@ -381,56 +382,49 @@ def test_success(self, mock_set_custom_metric):
self.assertEqual(self.request.COOKIES[jwt_cookie_name()], 'header.payload.signature')
mock_set_custom_metric.assert_called_once_with('request_jwt_cookie', 'success')

@override_settings(
ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_middleware',
MIDDLEWARE_CLASSES=(
'django.contrib.sessions.middleware.SessionMiddleware',
'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'edx_rest_framework_extensions.auth.jwt.tests.test_middleware.CheckRequestUserForJwtAuthMiddleware',
),
)
def test_success_sets_request_user(self):
header = {USE_JWT_COOKIE_HEADER: 'true'}
self.client.cookies = _get_test_cookie()
response = self.client.get('/nopermissionsrequired/', **header)
self.assertEqual(200, response.status_code)
_LOG_WARN_AUTHENTICATION_FAILED = 0
_LOG_WARN_MISSING_JWT_AUTHENTICATION_CLASS = 1

@override_settings(
ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_middleware',
MIDDLEWARE_CLASSES=(
'django.contrib.sessions.middleware.SessionMiddleware',
'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'edx_rest_framework_extensions.auth.jwt.tests.test_middleware.CheckRequestUserAnonymousForJwtAuthMiddleware',
),
)
@patch('edx_rest_framework_extensions.auth.jwt.middleware.log')
def test_invalid_cookie_logs_warning(self, mock_log):
header = {USE_JWT_COOKIE_HEADER: 'true'}
self.client.cookies = _get_test_cookie(is_cookie_valid=False)
response = self.client.get('/nopermissionsrequired/', **header)
self.assertEqual(200, response.status_code)
mock_log.warn.assert_called_once_with('Jwt Authentication failed and request.user could not be set.')

@override_settings(
ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_middleware',
MIDDLEWARE_CLASSES=(
'django.contrib.sessions.middleware.SessionMiddleware',
'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'edx_rest_framework_extensions.auth.jwt.tests.test_middleware.CheckRequestUserAnonymousForJwtAuthMiddleware',
),
@ddt.data(
('/nopermissionsrequired/', True, True, True, None),
('/nopermissionsrequired/', True, False, False, None),
('/nopermissionsrequired/', False, False, True, _LOG_WARN_AUTHENTICATION_FAILED),
('/nopermissionsrequired/', False, False, False, None),
('/unauthenticated/', True, False, True, _LOG_WARN_MISSING_JWT_AUTHENTICATION_CLASS),
('/unauthenticated/', True, False, False, None),
)
@patch('edx_rest_framework_extensions.auth.jwt.middleware.log')
def test_unauthenticated_view_logs_warning(self, mock_log):
@ddt.unpack
def test_set_request_user_with_use_jwt_cookie(
self, url, is_cookie_valid, is_request_user_set, is_toggle_enabled, log_warning,
mock_log,
):
header = {USE_JWT_COOKIE_HEADER: 'true'}
self.client.cookies = _get_test_cookie()
response = self.client.get('/unauthenticated/', **header)
self.assertEqual(200, response.status_code)
mock_log.warn.assert_called_once_with(
'Jwt Authentication expected, but view %s is not using a JwtAuthentication class.', ANY
)
self.client.cookies = _get_test_cookie(is_cookie_valid=is_cookie_valid)
check_user_middleware_assertion_class = 'CheckRequestUserForJwtAuthMiddleware' if is_request_user_set else 'CheckRequestUserAnonymousForJwtAuthMiddleware'
with override_settings(
ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_middleware',
MIDDLEWARE_CLASSES=(
'django.contrib.sessions.middleware.SessionMiddleware',
'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware',
'django.contrib.auth.middleware.AuthenticationMiddleware',
'edx_rest_framework_extensions.auth.jwt.tests.test_middleware.{}'.format(check_user_middleware_assertion_class),
),
EDX_DRF_EXTENSIONS={
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: is_toggle_enabled,
}
):
response = self.client.get(url, **header)
self.assertEqual(200, response.status_code)

if log_warning == self._LOG_WARN_AUTHENTICATION_FAILED:
mock_log.warn.assert_called_once_with('Jwt Authentication failed and request.user could not be set.')
elif log_warning == self._LOG_WARN_MISSING_JWT_AUTHENTICATION_CLASS:
mock_log.warn.assert_called_once_with(
'Jwt Authentication expected, but view %s is not using a JwtAuthentication class.', ANY
)
else:
mock_log.warn.assert_not_called()


def _get_test_cookie(is_cookie_valid=True):
Expand Down
18 changes: 15 additions & 3 deletions edx_rest_framework_extensions/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@
Application configuration constants and code.
"""

NAMESPACE_SWITCH = 'oauth2'
SWITCH_ENFORCE_JWT_SCOPES = 'enforce_jwt_scopes'
NAMESPACED_SWITCH_ENFORCE_JWT_SCOPES = '{}.{}'.format(NAMESPACE_SWITCH, SWITCH_ENFORCE_JWT_SCOPES)
OAUTH_TOGGLE_NAMESPACE = 'oauth2'
SWITCH_ENFORCE_JWT_SCOPES = '{}.enforce_jwt_scopes'.format(OAUTH_TOGGLE_NAMESPACE)

# .. toggle_name: EDX_DRF_EXTENSIONS[ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE]
# .. toggle_implementation: DjangoSetting
# .. toggle_default: False
# .. toggle_description: Toggle for setting request.user with jwt cookie authentication
# .. toggle_category: experiments
# .. toggle_use_cases: incremental_release
# .. toggle_creation_date: 2019-10-15
# .. toggle_expiration_date: 2019-12-31
# .. toggle_warnings: This feature fixed ecommerce, but broke edx-platform. The toggle enables us to fix over time.
# .. toggle_tickets: ARCH-1210, ARCH-1199, ARCH-1197
# .. toggle_status: supported
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE = 'ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE'
4 changes: 2 additions & 2 deletions edx_rest_framework_extensions/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from edx_rest_framework_extensions.auth.jwt.decoder import (
decode_jwt_filters, decode_jwt_scopes, decode_jwt_is_restricted
)
from edx_rest_framework_extensions.config import NAMESPACED_SWITCH_ENFORCE_JWT_SCOPES
from edx_rest_framework_extensions.config import SWITCH_ENFORCE_JWT_SCOPES

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -52,7 +52,7 @@ def has_permission(self, request, view):

@classmethod
def is_enforced_and_jwt_restricted_app(cls, request):
is_enforcement_enabled = waffle.switch_is_active(NAMESPACED_SWITCH_ENFORCE_JWT_SCOPES)
is_enforcement_enabled = waffle.switch_is_active(SWITCH_ENFORCE_JWT_SCOPES)
ret_val = is_enforcement_enabled and is_jwt_authenticated(request) and decode_jwt_is_restricted(request.auth)
log.debug(u"Permission JwtRestrictedApplication: returns %s.", ret_val)
return ret_val
Expand Down
5 changes: 4 additions & 1 deletion edx_rest_framework_extensions/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
from django.conf import settings
from rest_framework_jwt.settings import api_settings

from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE

logger = logging.getLogger(__name__)


Expand All @@ -26,7 +28,8 @@
'administrator': 'is_staff',
'email': 'email',
},
'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': ()
'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (),
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False,
}


Expand Down

0 comments on commit 86256f1

Please sign in to comment.