diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..b362f49d --- /dev/null +++ b/.editorconfig @@ -0,0 +1,100 @@ +# *************************** +# ** DO NOT EDIT THIS FILE ** +# *************************** +# +# This file was generated by edx-lint: https://github.com/openedx/edx-lint +# +# If you want to change this file, you have two choices, depending on whether +# you want to make a local change that applies only to this repo, or whether +# you want to make a central change that applies to all repos using edx-lint. +# +# Note: If your .editorconfig file is simply out-of-date relative to the latest +# .editorconfig in edx-lint, ensure you have the latest edx-lint installed +# and then follow the steps for a "LOCAL CHANGE". +# +# LOCAL CHANGE: +# +# 1. Edit the local .editorconfig_tweaks file to add changes just to this +# repo's file. +# +# 2. Run: +# +# $ edx_lint write .editorconfig +# +# 3. This will modify the local file. Submit a pull request to get it +# checked in so that others will benefit. +# +# +# CENTRAL CHANGE: +# +# 1. Edit the .editorconfig file in the edx-lint repo at +# https://github.com/openedx/edx-lint/blob/master/edx_lint/files/.editorconfig +# +# 2. install the updated version of edx-lint (in edx-lint): +# +# $ pip install . +# +# 3. Run (in edx-lint): +# +# $ edx_lint write .editorconfig +# +# 4. Make a new version of edx_lint, submit and review a pull request with the +# .editorconfig update, and after merging, update the edx-lint version and +# publish the new version. +# +# 5. In your local repo, install the newer version of edx-lint. +# +# 6. Run: +# +# $ edx_lint write .editorconfig +# +# 7. This will modify the local file. Submit a pull request to get it +# checked in so that others will benefit. +# +# +# +# +# +# STAY AWAY FROM THIS FILE! +# +# +# +# +# +# SERIOUSLY. +# +# ------------------------------ +# Generated by edx-lint version: 5.3.4 +# ------------------------------ +[*] +end_of_line = lf +insert_final_newline = true +charset = utf-8 +indent_style = space +indent_size = 4 +max_line_length = 120 +trim_trailing_whitespace = true + +[{Makefile, *.mk}] +indent_style = tab +indent_size = 8 + +[*.{yml,yaml,json}] +indent_size = 2 + +[*.js] +indent_size = 2 + +[*.diff] +trim_trailing_whitespace = false + +[.git/*] +trim_trailing_whitespace = false + +[COMMIT_EDITMSG] +max_line_length = 72 + +[*.rst] +max_line_length = 79 + +# bbcbced841ed335dd8abb7456a6b13485d701b40 diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ff664ca6..1892cfab 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,22 @@ Change Log Unreleased ---------- +[8.9.0] - 2023-08-14 +-------------------- + +Added +~~~~~ + +* Added capability to forgive JWT cookie authentication failures as a replacement for the now deprecated ``USE-JWT-COOKIE`` header. See DEPR https://github.com/openedx/edx-drf-extensions/issues/371. + * For now, this capability must be enabled using the ``ENABLE_FORGIVING_JWT_COOKIES`` toggle. + * Added temporary custom attributes ``is_forgiving_jwt_cookies_enabled`` and ``use_jwt_cookie_requested`` to help with this deprecation. +* Added custom attributes ``has_jwt_cookie`` and ``jwt_auth_result`` for JWT authentication observability. + +Changed +~~~~~~~ + +* Two features that were gated on the presence of the ``USE-JWT-COOKIE`` header will now be gated on the presence of a JWT cookie instead, regardless of the state of the new ``ENABLE_FORGIVING_JWT_COOKIES`` toggle. The new behavior should be nearly equivalent in most cases, and should cause no issues in the exceptional cases. The two features include CSRF protection for JWT cookies, and the setting of the request user when ``ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE`` is enabled. + [8.8.0] - 2023-05-16 -------------------- diff --git a/docs/authentication.rst b/docs/authentication.rst index 9584933c..12c296cb 100644 --- a/docs/authentication.rst +++ b/docs/authentication.rst @@ -1,7 +1,7 @@ Authentication ============== -Authentication classes are used to associate a request with a user. Unless otherwise noted, all of the classes below -adhere to the Django `REST Framework's API for authentication classes `_. + +Authentication classes are used to associate a request with a user. Unless otherwise noted, all of the classes below adhere to the Django `REST Framework's API for authentication classes `_. .. automodule:: edx_rest_framework_extensions.authentication :members: diff --git a/docs/decisions/0002-remove-use-jwt-cookie-header.rst b/docs/decisions/0002-remove-use-jwt-cookie-header.rst new file mode 100644 index 00000000..69668b3c --- /dev/null +++ b/docs/decisions/0002-remove-use-jwt-cookie-header.rst @@ -0,0 +1,50 @@ +2. Remove HTTP_USE_JWT_COOKIE Header +==================================== + +Status +------ + +Accepted + +Context +------- + +This ADR explains `why the request header HTTP_USE_JWT_COOKIE`_ was added. + +Use of this header has several problems: + +* The purpose and need for the ``HTTP_USE_JWT_COOKIE`` header is confusing. It has led to developer confusion when trying to test API calls using JWT cookies, but having the auth fail because they didn't realize this special header was also required. +* In some cases, the JWT cookies are sent to services, but go unused because of this header. Additional oauth redirects then become required in circumstances where they otherwise wouldn't be needed. +* Some features have been added, like `JwtRedirectToLoginIfUnauthenticatedMiddleware`_, that can be greatly simplified or possibly removed altogether if the ``HTTP_USE_JWT_COOKIE`` header were retired. + + +Decision +-------- + +Replace the `HTTP_USE_JWT_COOKIE` header with forgiving authentication when using JWT cookies. By "forgiving", we mean that JWT authentication would no longer raise exceptions for failed authentication when using JWT cookies, but instead would simply return None. + +By returning None from JwtAuthentication, rather than raising an authentication failure, we enable services to move on to other classes, like SessionAuthentication, rather than aborting the authentication process. Failure messages could still be surfaced using `set_custom_metric` for debugging purposes. + +Rather than checking for the `HTTP_USE_JWT_COOKIE`, the `JwtAuthCookieMiddleware`_ would always reconstitute the JWT cookie if the parts were available. + +The proposal includes protecting all changes with a temporary rollout feature toggle ``ENABLE_FORGIVING_JWT_COOKIES``. This can be used to ensure no harm is done for each service before cleaning up the old header. + +.. _JwtAuthCookieMiddleware: https://github.com/edx/edx-drf-extensions/blob/270cf521a72b506d7df595c4c479c7ca232b4bec/edx_rest_framework_extensions/auth/jwt/middleware.py#L164 + +Consequences +------------ + +* Makes authentication simpler, more clear, and more predictable. + + * For example, local testing of endpoints outside of MFEs will use JWT cookies rather than failing, which has been misleading for engineers. + +* Greatly simplifies features like `JwtRedirectToLoginIfUnauthenticatedMiddleware`_. +* Service authentication can take advantage of JWT cookies more often. +* Services can more consistently take advantage of the JWT payload of the JWT cookie. +* Additional clean-up when retiring the ``HTTP_USE_JWT_COOKIE`` header will be needed: + + * ``HTTP_USE_JWT_COOKIE`` should be removed from frontend-platform auth code when ready. + * ADR that explains `why the request header HTTP_USE_JWT_COOKIE`_ was created should be updated to point to this ADR. + +.. _why the request header HTTP_USE_JWT_COOKIE: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst#login---cookie---api +.. _JwtRedirectToLoginIfUnauthenticatedMiddleware: https://github.com/edx/edx-drf-extensions/blob/270cf521a72b506d7df595c4c479c7ca232b4bec/edx_rest_framework_extensions/auth/jwt/middleware.py#L87 diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index ebc78e20..6b805d5f 100644 --- a/edx_rest_framework_extensions/__init__.py +++ b/edx_rest_framework_extensions/__init__.py @@ -1,3 +1,3 @@ """ edx Django REST Framework extensions. """ -__version__ = '8.8.0' # pragma: no cover +__version__ = '8.9.0' # pragma: no cover diff --git a/edx_rest_framework_extensions/auth/jwt/authentication.py b/edx_rest_framework_extensions/auth/jwt/authentication.py index ff5e8422..56b6991a 100644 --- a/edx_rest_framework_extensions/auth/jwt/authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/authentication.py @@ -8,8 +8,9 @@ from rest_framework import exceptions from rest_framework_jwt.authentication import JSONWebTokenAuthentication -from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER +from edx_rest_framework_extensions.auth.jwt.cookies import jwt_cookie_name from edx_rest_framework_extensions.auth.jwt.decoder import configured_jwt_decode_handler +from edx_rest_framework_extensions.config import ENABLE_FORGIVING_JWT_COOKIES from edx_rest_framework_extensions.settings import get_setting @@ -59,30 +60,65 @@ def get_jwt_claim_mergeable_attributes(self): return get_setting('JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES') def authenticate(self, request): + is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) + # .. custom_attribute_name: is_forgiving_jwt_cookies_enabled + # .. custom_attribute_description: This is temporary custom attribute to show + # whether ENABLE_FORGIVING_JWT_COOKIES is toggled on or off. + # See docs/decisions/0002-remove-use-jwt-cookie-header.rst + set_custom_attribute('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) + + # .. custom_attribute_name: jwt_auth_result + # .. custom_attribute_description: The result of the JWT authenticate process, + # which can having the following values: + # 'skipped': When JWT Authentication doesn't apply. + # 'success-auth-header': Successfully authenticated using the Authorization header. + # 'success-cookie': Successfully authenticated using a JWT cookie. + # 'forgiven-failure': Returns None instead of failing for JWT cookies. This handles + # the case where expired cookies won't prevent another authentication class, like + # SessionAuthentication, from having a chance to succeed. + # See docs/decisions/0002-remove-use-jwt-cookie-header.rst for details. + # 'failed-auth-header': JWT Authorization header authentication failed. This prevents + # other authentication classes from attempting authentication. + # 'failed-cookie': JWT cookie authentication failed. This prevents other + # authentication classes from attempting authentication. + + has_jwt_cookie = jwt_cookie_name() in request.COOKIES try: user_and_auth = super().authenticate(request) # Unauthenticated, CSRF validation not required if not user_and_auth: + set_custom_attribute('jwt_auth_result', 'skipped') return user_and_auth - # Not using JWT cookies, CSRF validation not required - use_jwt_cookie_requested = request.META.get(USE_JWT_COOKIE_HEADER) - if not use_jwt_cookie_requested: + # Not using JWT cookie, CSRF validation not required + if not has_jwt_cookie: + set_custom_attribute('jwt_auth_result', 'success-auth-header') return user_and_auth self.enforce_csrf(request) # CSRF passed validation with authenticated user + set_custom_attribute('jwt_auth_result', 'success-cookie') return user_and_auth except Exception as exception: # Errors in production do not need to be logged (as they may be noisy), # but debug logging can help quickly resolve issues during development. logger.debug('Failed JWT Authentication,', exc_info=exception) - # Note: I think this case should only include AuthenticationFailed and PermissionDenied, - # but will monitor the custom attribute to verify. + # .. custom_attribute_name: jwt_auth_failed + # .. custom_attribute_description: Includes a summary of the JWT failure exception + # for debugging. set_custom_attribute('jwt_auth_failed', 'Exception:{}'.format(repr(exception))) + + is_jwt_failure_forgiven = is_forgiving_jwt_cookies_enabled and has_jwt_cookie + if is_jwt_failure_forgiven: + set_custom_attribute('jwt_auth_result', 'forgiven-failure') + return None + if has_jwt_cookie: + set_custom_attribute('jwt_auth_result', 'failed-cookie') + else: + set_custom_attribute('jwt_auth_result', 'failed-auth-header') raise def authenticate_credentials(self, payload): diff --git a/edx_rest_framework_extensions/auth/jwt/cookies.py b/edx_rest_framework_extensions/auth/jwt/cookies.py index 9fa25821..654d60aa 100644 --- a/edx_rest_framework_extensions/auth/jwt/cookies.py +++ b/edx_rest_framework_extensions/auth/jwt/cookies.py @@ -8,6 +8,11 @@ def jwt_cookie_name(): + # Warning: This method should probably not supply a default outside + # of JWT_AUTH_COOKIE, because JwtAuthentication will never see + # the cookie without the setting. This default should probably be + # removed, but that would take some further investigation. In the + # meantime, this default has been duplicated to test_settings.py. return settings.JWT_AUTH.get('JWT_AUTH_COOKIE') or 'edx-jwt-cookie' diff --git a/edx_rest_framework_extensions/auth/jwt/middleware.py b/edx_rest_framework_extensions/auth/jwt/middleware.py index 85dad746..b403c756 100644 --- a/edx_rest_framework_extensions/auth/jwt/middleware.py +++ b/edx_rest_framework_extensions/auth/jwt/middleware.py @@ -23,7 +23,10 @@ jwt_cookie_name, jwt_cookie_signature_name, ) -from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE +from edx_rest_framework_extensions.config import ( + ENABLE_FORGIVING_JWT_COOKIES, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, +) from edx_rest_framework_extensions.permissions import ( LoginRedirectIfUnauthenticated, NotJwtRestrictedApplication, @@ -133,8 +136,16 @@ def process_view(self, request, view_func, view_args, view_kwargs): # pylint: d Enables Jwt Authentication for endpoints using the LoginRedirectIfUnauthenticated permission class. """ self._check_and_cache_login_required_found(view_func) - if self.is_jwt_auth_enabled_with_login_required(request, view_func): - request.META[USE_JWT_COOKIE_HEADER] = 'true' + + # Forgiving JWT cookies is an alternative to the older USE_JWT_COOKIE_HEADER. + # If the rollout for forgiving JWT cookies succeeds, we would need to see if + # this middleware could be simplified or replaced by a simpler solution, because + # at least one part of the original solution required this middleware to insert + # the USE_JWT_COOKIE_HEADER. + is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) + if not is_forgiving_jwt_cookies_enabled: + if self.is_jwt_auth_enabled_with_login_required(request, view_func): + request.META[USE_JWT_COOKIE_HEADER] = 'true' def process_response(self, request, response): """ @@ -200,59 +211,59 @@ class JwtAuthCookieMiddleware(MiddlewareMixin): """ - def _get_missing_cookie_message_and_attribute(self, cookie_name): - """ Returns tuple with missing cookie (log_message, custom_attribute_value) """ - cookie_missing_message = '{} cookie is missing. JWT auth cookies will not be reconstituted.'.format( + def _get_missing_cookie_message(self, cookie_name): + """ Returns missing cookie log_message """ + return '{} cookie is missing. JWT auth cookies will not be reconstituted.'.format( cookie_name ) - request_jwt_cookie = f'missing-{cookie_name}' - return cookie_missing_message, request_jwt_cookie # Note: Using `process_view` over `process_request` so JwtRedirectToLoginIfUnauthenticatedMiddleware which # uses `process_view` can update the request before this middleware. Method `process_request` happened too early. def process_view(self, request, view_func, view_args, view_kwargs): # pylint: disable=unused-argument """ Reconstitute the full JWT and add a new cookie on the request object. + + Additionally, may add the user to the request to make it available in process_view. (See below.) """ assert hasattr(request, 'session'), "The Django authentication middleware requires session middleware to be installed. Edit your MIDDLEWARE setting to insert 'django.contrib.sessions.middleware.SessionMiddleware'." # noqa E501 line too long - use_jwt_cookie_requested = request.META.get(USE_JWT_COOKIE_HEADER) + # .. custom_attribute_name: use_jwt_cookie_requested + # .. custom_attribute_description: True if USE_JWT_COOKIE_HEADER was found. + # This is a temporary attribute, because this header is being deprecated/removed. + monitoring.set_custom_attribute('use_jwt_cookie_requested', bool(request.META.get(USE_JWT_COOKIE_HEADER))) + + if not get_setting(ENABLE_FORGIVING_JWT_COOKIES): + if not request.META.get(USE_JWT_COOKIE_HEADER): + return + header_payload_cookie = request.COOKIES.get(jwt_cookie_header_payload_name()) signature_cookie = request.COOKIES.get(jwt_cookie_signature_name()) - 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)) - - if not use_jwt_cookie_requested: - attribute_value = 'not-requested' - elif header_payload_cookie and signature_cookie: - # Reconstitute JWT auth cookie if split cookies are available and jwt cookie - # authentication was requested by the client. + if header_payload_cookie and signature_cookie: + # Reconstitute JWT auth cookie if split cookies are available. request.COOKIES[jwt_cookie_name()] = '{}{}{}'.format( header_payload_cookie, JWT_DELIMITER, signature_cookie, ) - attribute_value = 'success' elif header_payload_cookie or signature_cookie: # Log unexpected case of only finding one cookie. if not header_payload_cookie: - log_message, attribute_value = self._get_missing_cookie_message_and_attribute( - jwt_cookie_header_payload_name() - ) + log_message = self._get_missing_cookie_message(jwt_cookie_header_payload_name()) if not signature_cookie: - log_message, attribute_value = self._get_missing_cookie_message_and_attribute( - jwt_cookie_signature_name() - ) + log_message = self._get_missing_cookie_message(jwt_cookie_signature_name()) log.warning(log_message) - else: - attribute_value = 'missing-both' - log.warning('Both JWT auth cookies missing. JWT auth cookies will not be reconstituted.') - monitoring.set_custom_attribute('request_jwt_cookie', attribute_value) + has_reconstituted_jwt_cookie = jwt_cookie_name() in request.COOKIES + # .. custom_attribute_name: has_jwt_cookie + # .. custom_attribute_description: Enables us to see requests which have the full reconstituted + # JWT cookie. If this attribute is missing, it is assumed to be False. + monitoring.set_custom_attribute('has_jwt_cookie', has_reconstituted_jwt_cookie) + + if has_reconstituted_jwt_cookie and get_setting(ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE): + # 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)) def _get_user_from_jwt(request, view_func): @@ -276,7 +287,7 @@ def _get_user_from_jwt(request, view_func): 'Jwt Authentication expected, but view %s is not using a JwtAuthentication class.', view_func ) except Exception: # pylint: disable=broad-except - log.exception('Unknown error attempting to complete Jwt Authentication.') # pragma: no cover + log.exception('Unknown Jwt Authentication error attempting to retrieve the user.') # pragma: no cover return user diff --git a/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py b/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py index 96c3a2a9..5ab2f740 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py @@ -11,11 +11,14 @@ from edx_rest_framework_extensions.auth.jwt import authentication from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER +from edx_rest_framework_extensions.auth.jwt.cookies import jwt_cookie_name from edx_rest_framework_extensions.auth.jwt.decoder import jwt_decode_handler from edx_rest_framework_extensions.auth.jwt.tests.utils import ( generate_jwt_token, generate_latest_version_payload, ) +from edx_rest_framework_extensions.config import ENABLE_FORGIVING_JWT_COOKIES +from edx_rest_framework_extensions.settings import get_setting from edx_rest_framework_extensions.tests import factories @@ -174,19 +177,51 @@ def test_authenticate_credentials_no_usernames(self): with self.assertRaises(AuthenticationFailed): JwtAuthentication().authenticate_credentials({'email': 'test@example.com'}) + @mock.patch.object(JwtAuthentication, 'enforce_csrf') + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') + def test_authenticate_with_correct_jwt_cookie(self, mock_set_custom_attribute, mock_enforce_csrf): + """ Verify authenticate succeeds with a valid JWT cookie. """ + request = RequestFactory().post('/') + + request.META[USE_JWT_COOKIE_HEADER] = 'true' + + request.COOKIES[jwt_cookie_name()] = self._get_test_jwt_token() + + assert JwtAuthentication().authenticate(request) + mock_enforce_csrf.assert_called_with(request) + mock_set_custom_attribute.assert_any_call( + 'is_forgiving_jwt_cookies_enabled', + get_setting(ENABLE_FORGIVING_JWT_COOKIES) + ) + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'success-cookie') + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') def test_authenticate_csrf_protected(self, mock_set_custom_attribute): - """ Verify authenticate exception for CSRF protected cases. """ + """ + Ensure authenticate for JWTs properly handles CSRF errors. + + Note: When using forgiving JWTs, all JWT cookie exceptions, including CSRF, will + result in a None so that other authentication classes will also be checked. + """ request = RequestFactory().post('/') request.META[USE_JWT_COOKIE_HEADER] = 'true' + # Set a sample JWT cookie. We mock the auth response but we still want + # to ensure that there is jwt set because there is other logic that + # checks for the jwt to be set before moving forward with CSRF checks. + request.COOKIES[jwt_cookie_name()] = 'foo' with mock.patch.object(JSONWebTokenAuthentication, 'authenticate', return_value=('mock-user', "mock-auth")): - with self.assertRaises(PermissionDenied) as context_manager: - JwtAuthentication().authenticate(request) - - assert context_manager.exception.detail.startswith('CSRF Failed') - mock_set_custom_attribute.assert_called_once_with( + if get_setting(ENABLE_FORGIVING_JWT_COOKIES): + assert JwtAuthentication().authenticate(request) is None + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'forgiven-failure') + else: + with self.assertRaises(PermissionDenied) as context_manager: + JwtAuthentication().authenticate(request) + assert context_manager.exception.detail.startswith('CSRF Failed') + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-cookie') + + mock_set_custom_attribute.assert_any_call( 'jwt_auth_failed', "Exception:PermissionDenied('CSRF Failed: CSRF cookie not set.')", ) @@ -206,28 +241,38 @@ def test_get_decoded_jwt_from_auth(self, is_jwt_authentication): decoded_jwt = authentication.get_decoded_jwt_from_auth(mock_request_with_cookie) self.assertEqual(expected_decoded_jwt, decoded_jwt) - def test_authenticate_with_correct_jwt_authorization(self): + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') + def test_authenticate_with_correct_jwt_authorization(self, mock_set_custom_attribute): """ With JWT header it continues and validates the credentials and throws error. Note: CSRF protection should be skipped for this case, with no PermissionDenied. """ jwt_token = self._get_test_jwt_token() - request = RequestFactory().get('/', HTTP_AUTHORIZATION=jwt_token) - JwtAuthentication().authenticate(request) + request = RequestFactory().get('/', HTTP_AUTHORIZATION=f"JWT {jwt_token}") + assert JwtAuthentication().authenticate(request) + mock_set_custom_attribute.assert_any_call( + 'is_forgiving_jwt_cookies_enabled', + get_setting(ENABLE_FORGIVING_JWT_COOKIES) + ) + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'success-auth-header') - def test_authenticate_with_incorrect_jwt_authorization(self): + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') + def test_authenticate_with_incorrect_jwt_authorization(self, mock_set_custom_attribute): """ With JWT header it continues and validates the credentials and throws error. """ auth_header = '{token_name} {token}'.format(token_name='JWT', token='wrongvalue') request = RequestFactory().get('/', HTTP_AUTHORIZATION=auth_header) with self.assertRaises(AuthenticationFailed): JwtAuthentication().authenticate(request) + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-auth-header') - def test_authenticate_with_bearer_token(self): + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') + def test_authenticate_with_bearer_token(self, mock_set_custom_attribute): """ Returns a None for bearer header request. """ auth_header = '{token_name} {token}'.format(token_name='Bearer', token='abc123') request = RequestFactory().get('/', HTTP_AUTHORIZATION=auth_header) self.assertIsNone(JwtAuthentication().authenticate(request)) + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'skipped') def _get_test_jwt_token(self): """ Returns a user and jwt token """ @@ -235,3 +280,10 @@ def _get_test_jwt_token(self): payload = generate_latest_version_payload(user) jwt_token = generate_jwt_token(payload) return jwt_token + + +# We want to duplicate these tests for now while we have two major code paths. It will get unified once we have a +# single way of doing JWT authentication again. +@override_settings(EDX_DRF_EXTENSIONS={ENABLE_FORGIVING_JWT_COOKIES: True}) +class ForgivingJwtAuthenticationTests(JwtAuthenticationTests): # pylint: disable=test-inherits-tests + pass diff --git a/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py b/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py index 3497666b..08d03ecb 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py @@ -29,7 +29,10 @@ JwtAuthCookieMiddleware, JwtRedirectToLoginIfUnauthenticatedMiddleware, ) -from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE +from edx_rest_framework_extensions.config import ( + ENABLE_FORGIVING_JWT_COOKIES, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, +) from edx_rest_framework_extensions.permissions import ( IsStaff, IsSuperuser, @@ -37,6 +40,7 @@ LoginRedirectIfUnauthenticated, NotJwtRestrictedApplication, ) +from edx_rest_framework_extensions.settings import get_setting from edx_rest_framework_extensions.tests.factories import UserFactory @@ -321,6 +325,8 @@ def setUp(self): def test_login_required_middleware(self, url, has_jwt_cookies, expected_status): if has_jwt_cookies: self.client.cookies = _get_test_cookie() + if get_setting(ENABLE_FORGIVING_JWT_COOKIES): + expected_status = 200 response = self.client.get(url) self.assertEqual(expected_status, response.status_code) if response.status_code == 302: @@ -349,12 +355,22 @@ def test_login_required_middleware(self, url, has_jwt_cookies, expected_status): def test_login_required_overridden_middleware(self, url, has_jwt_cookies, expected_status): if has_jwt_cookies: self.client.cookies = _get_test_cookie() + if get_setting(ENABLE_FORGIVING_JWT_COOKIES): + expected_status = 200 response = self.client.get(url) self.assertEqual(expected_status, response.status_code) if response.status_code == 302: self.assertEqual('/overridden/login/?next=' + url, response.url) +# We want to duplicate these tests for now while we have two major code paths. It will get unified once we have a +# single way of doing JWT authentication again. +@ddt.ddt +@override_settings(EDX_DRF_EXTENSIONS={ENABLE_FORGIVING_JWT_COOKIES: True}) +class TestForgivingJwtRedirectToLoginIfUnauthenticatedMiddleware(TestJwtRedirectToLoginIfUnauthenticatedMiddleware): # pylint: disable=test-inherits-tests # noqa E501 line too long + pass + + class CheckRequestUserForJwtAuthMiddleware(MiddlewareMixin): """ This test middleware can be used to confirm that our Jwt Authentication related middleware correctly @@ -385,7 +401,7 @@ def setUp(self): def test_do_not_use_jwt_cookies(self, mock_set_custom_attribute): self.middleware.process_view(self.request, None, None, None) self.assertIsNone(self.request.COOKIES.get(jwt_cookie_name())) - mock_set_custom_attribute.assert_called_once_with('request_jwt_cookie', 'not-requested') + mock_set_custom_attribute.assert_any_call('use_jwt_cookie_requested', False) @ddt.data( (jwt_cookie_header_payload_name(), jwt_cookie_signature_name()), @@ -405,16 +421,16 @@ def test_missing_cookies( '%s cookie is missing. JWT auth cookies will not be reconstituted.' % missing_cookie_name ) - mock_set_custom_attribute.assert_called_once_with( - 'request_jwt_cookie', f'missing-{missing_cookie_name}' - ) + mock_set_custom_attribute.assert_any_call('use_jwt_cookie_requested', True) + mock_set_custom_attribute.assert_any_call('has_jwt_cookie', False) @patch('edx_django_utils.monitoring.set_custom_attribute') def test_no_cookies(self, mock_set_custom_attribute): self.request.META[USE_JWT_COOKIE_HEADER] = 'true' self.middleware.process_view(self.request, None, None, None) self.assertIsNone(self.request.COOKIES.get(jwt_cookie_name())) - mock_set_custom_attribute.assert_called_once_with('request_jwt_cookie', 'missing-both') + mock_set_custom_attribute.assert_any_call('use_jwt_cookie_requested', True) + mock_set_custom_attribute.assert_any_call('has_jwt_cookie', False) @patch('edx_django_utils.monitoring.set_custom_attribute') def test_success(self, mock_set_custom_attribute): @@ -423,7 +439,8 @@ def test_success(self, mock_set_custom_attribute): self.request.COOKIES[jwt_cookie_signature_name()] = 'signature' self.middleware.process_view(self.request, None, None, None) self.assertEqual(self.request.COOKIES[jwt_cookie_name()], 'header.payload.signature') - mock_set_custom_attribute.assert_called_once_with('request_jwt_cookie', 'success') + mock_set_custom_attribute.assert_any_call('use_jwt_cookie_requested', True) + mock_set_custom_attribute.assert_any_call('has_jwt_cookie', True) _LOG_WARN_AUTHENTICATION_FAILED = 0 _LOG_WARN_MISSING_JWT_AUTHENTICATION_CLASS = 1 @@ -480,6 +497,14 @@ def test_set_request_user_with_use_jwt_cookie( mock_log.warn.assert_not_called() +# We want to duplicate these tests for now while we have two major code paths. It will get unified once we have a +# single way of doing JWT authentication again. +@ddt.ddt +@override_settings(EDX_DRF_EXTENSIONS={ENABLE_FORGIVING_JWT_COOKIES: True}) +class TestForgivingJwtAuthCookieMiddleware(TestJwtAuthCookieMiddleware): # pylint: disable=test-inherits-tests + pass + + def _get_test_cookie(is_cookie_valid=True): header_payload_value = 'header.payload' if is_cookie_valid else 'header.payload.invalid' return SimpleCookie({ diff --git a/edx_rest_framework_extensions/config.py b/edx_rest_framework_extensions/config.py index 3f42b5a0..bf5e94c5 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -12,3 +12,13 @@ # .. toggle_warning: This feature fixed ecommerce, but broke edx-platform. The toggle enables us to fix over time. # .. toggle_tickets: ARCH-1210, ARCH-1199, ARCH-1197 ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE = 'ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE' + +# .. toggle_name: EDX_DRF_EXTENSIONS[ENABLE_FORGIVING_JWT_COOKIES] +# .. toggle_implementation: DjangoSetting +# .. toggle_default: False +# .. toggle_description: If True, return None rather than an exception when authentication fails with JWT cookies. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2023-08-01 +# .. toggle_target_removal_date: 2023-10-01 +# .. toggle_tickets: https://github.com/openedx/edx-drf-extensions/issues/371 +ENABLE_FORGIVING_JWT_COOKIES = 'ENABLE_FORGIVING_JWT_COOKIES' diff --git a/edx_rest_framework_extensions/middleware.py b/edx_rest_framework_extensions/middleware.py index bfe4745d..234e0d85 100644 --- a/edx_rest_framework_extensions/middleware.py +++ b/edx_rest_framework_extensions/middleware.py @@ -8,7 +8,6 @@ from edx_django_utils.cache import DEFAULT_REQUEST_CACHE import edx_rest_framework_extensions -from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER from edx_rest_framework_extensions.auth.jwt.cookies import jwt_cookie_name @@ -148,14 +147,6 @@ def _set_request_user_agent_attributes(self, request): def _set_request_auth_type_guess_attribute(self, request): """ Add custom attribute 'request_auth_type_guess' for the authentication type used. - - NOTE: This is a best guess at this point. Possible values include: - no-user - unauthenticated - jwt/bearer/other-token-type - jwt-cookie - session-or-other (catch all) - """ if not hasattr(request, 'user') or not request.user: auth_type = 'no-user' @@ -168,10 +159,19 @@ def _set_request_auth_type_guess_attribute(self, request): auth_type = token_parts[0].lower() # 'jwt' or 'bearer' (for example) else: auth_type = 'other-token-type' - elif USE_JWT_COOKIE_HEADER in request.META and jwt_cookie_name() in request.COOKIES: + elif jwt_cookie_name() in request.COOKIES: auth_type = 'jwt-cookie' else: auth_type = 'session-or-other' + + # .. custom_attribute_name: request_auth_type_guess + # .. custom_attribute_description: This is a somewhat odd custom attribute, because + # we are taking a guess at authentication. Possible values include: + # no-user, + # unauthenticated, + # jwt/bearer/other-token-type, + # jwt-cookie, + # session-or-other (catch all). monitoring.set_custom_attribute('request_auth_type_guess', auth_type) AUTHENTICATED_USER_FOUND_CACHE_KEY = 'edx-drf-extensions.authenticated_user_found_in_middleware' diff --git a/edx_rest_framework_extensions/settings.py b/edx_rest_framework_extensions/settings.py index 76551586..57572afa 100644 --- a/edx_rest_framework_extensions/settings.py +++ b/edx_rest_framework_extensions/settings.py @@ -15,7 +15,10 @@ 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 +from edx_rest_framework_extensions.config import ( + ENABLE_FORGIVING_JWT_COOKIES, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, +) logger = logging.getLogger(__name__) @@ -31,6 +34,7 @@ }, 'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (), ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False, + ENABLE_FORGIVING_JWT_COOKIES: False, } diff --git a/pylintrc b/pylintrc index f57d8673..d8f191fd 100644 --- a/pylintrc +++ b/pylintrc @@ -64,7 +64,7 @@ # SERIOUSLY. # # ------------------------------ -# Generated by edx-lint version: 5.3.1 +# Generated by edx-lint version: 5.3.4 # ------------------------------ [MASTER] ignore = @@ -381,6 +381,6 @@ ext-import-graph = int-import-graph = [EXCEPTIONS] -overgeneral-exceptions = Exception +overgeneral-exceptions = builtins.Exception -# 946ebd6fd171e86977f195144fd695fe2f9d6ba4 +# 9e4689ffce27e736fb99d1efea50aaefc664f0a1 diff --git a/test_settings.py b/test_settings.py index 9cc83c27..d1ac9ddc 100644 --- a/test_settings.py +++ b/test_settings.py @@ -35,6 +35,8 @@ # See https://github.com/GetBlimp/django-rest-framework-jwt/blob/master/rest_framework_jwt/settings.py JWT_AUTH = { + 'JWT_AUTH_COOKIE': 'edx-jwt-cookie', + 'JWT_AUDIENCE': 'test-aud', 'JWT_DECODE_HANDLER': 'edx_rest_framework_extensions.auth.jwt.decoder.jwt_decode_handler',