From bf5ae92022179b94ad2f1f7f484056649bfb5935 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 8 Nov 2023 13:20:06 -0500 Subject: [PATCH] feat!: make forgiving JWTs the default This is the final step (in this library) of the rollout of forgiving JWTs as a replacement for the USE-JWT-COOKIE header. **BREAKING CHANGE:** Removed ENABLE_FORGIVING_JWT_COOKIES toggle. It is now permanently enabled. - The header USE-JWT-COOKIE was removed because it has been fully replaced by forgiving JWTs. - Removed temporary rollout custom attributes: use_jwt_cookie_requested, jwt_auth_request_user_not_found, and skip_jwt_vs_session_check. See ADR 0002-remove-use-jwt-cookie-header.rst for details. --- CHANGELOG.rst | 10 ++ .../0002-remove-use-jwt-cookie-header.rst | 16 ++- edx_rest_framework_extensions/__init__.py | 2 +- .../auth/jwt/authentication.py | 43 +----- .../auth/jwt/constants.py | 1 - .../auth/jwt/middleware.py | 68 ++------- .../auth/jwt/tests/test_authentication.py | 79 ++--------- .../auth/jwt/tests/test_middleware.py | 133 ++---------------- edx_rest_framework_extensions/config.py | 10 -- edx_rest_framework_extensions/settings.py | 13 +- .../tests/test_middleware.py | 2 - 11 files changed, 63 insertions(+), 314 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5e6c033f..63dfbb57 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,16 @@ Change Log Unreleased ---------- +[9.0.0] - 2023-11-08 +-------------------- + +Removed +~~~~~~~ +* **BREAKING CHANGE:** Removed ENABLE_FORGIVING_JWT_COOKIES toggle. It is now permanently enabled. + + * The header USE-JWT-COOKIE was removed because it has been fully replaced by forgiving JWTs. + * Removed temporary rollout custom attributes: use_jwt_cookie_requested, jwt_auth_request_user_not_found, and skip_jwt_vs_session_check. + [8.13.0] - 2023-10-30 --------------------- diff --git a/docs/decisions/0002-remove-use-jwt-cookie-header.rst b/docs/decisions/0002-remove-use-jwt-cookie-header.rst index 0a13ad49..148694c3 100644 --- a/docs/decisions/0002-remove-use-jwt-cookie-header.rst +++ b/docs/decisions/0002-remove-use-jwt-cookie-header.rst @@ -1,4 +1,4 @@ -2. Remove HTTP_USE_JWT_COOKIE Header +2. Replace HTTP_USE_JWT_COOKIE Header ==================================== Status @@ -21,14 +21,16 @@ Use of this header has several problems: 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. +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. +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. +**Update:** As of Nov-2023, the ``ENABLE_FORGIVING_JWT_COOKIES`` toggle and ``HTTP_USE_JWT_COOKIE`` have been fully removed, and forgiving JWTs is the default and only implementation remaining. + Unfortunately, there are certain rare cases where the user inside the JWT and the session user do not match: - If the JWT cookie succeeds authentication, and: @@ -47,13 +49,13 @@ Consequences * 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`_. +* 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. + * ADR that explains `why the request header HTTP_USE_JWT_COOKIE`_ was updated in https://github.com/openedx/edx-platform/pull/33680. .. _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 @@ -61,6 +63,10 @@ Consequences Change History -------------- +2023-11-08 +~~~~~~~~~~ +* Updated implementation status, since forgiving JWTs has been rolled out and the ENABLE_FORGIVING_JWT_COOKIES toggle and HTTP_USE_JWT_COOKIE header have been fully removed. + 2023-10-30 ~~~~~~~~~~ * Details added for handling of a variety of situations when the JWT cookie user and the session user do not match. diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index 0e576376..810707af 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.13.0' # pragma: no cover +__version__ = '9.0.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 ec2b84a0..225eae29 100644 --- a/edx_rest_framework_extensions/auth/jwt/authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/authentication.py @@ -14,10 +14,7 @@ configured_jwt_decode_handler, unsafe_jwt_decode_handler, ) -from edx_rest_framework_extensions.config import ( - ENABLE_FORGIVING_JWT_COOKIES, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, -) +from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE from edx_rest_framework_extensions.settings import get_setting @@ -77,13 +74,6 @@ 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: @@ -150,14 +140,11 @@ def authenticate(self, request): if is_authenticating_with_jwt_cookie: # This check also adds monitoring details for all failed JWT cookies is_user_mismatch = self._is_failed_jwt_cookie_and_session_user_mismatch(request) - if is_forgiving_jwt_cookies_enabled: - if is_user_mismatch: - set_custom_attribute('jwt_auth_result', 'user-mismatch-failure') - raise - set_custom_attribute('jwt_auth_result', 'forgiven-failure') - return None - set_custom_attribute('jwt_auth_result', 'failed-cookie') - raise + if is_user_mismatch: + set_custom_attribute('jwt_auth_result', 'user-mismatch-failure') + raise + set_custom_attribute('jwt_auth_result', 'forgiven-failure') + return None set_custom_attribute('jwt_auth_result', 'failed-auth-header') raise @@ -310,35 +297,19 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id): jwt_user_id (int): The user_id of the JWT, None if not found. Other notes: - - If ENABLE_FORGIVING_JWT_COOKIES is toggled off, always return False. - Also adds monitoring details for mismatches. - Should only be called for JWT cookies. """ - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - # This toggle provides a temporary safety valve for rollout. - if not is_forgiving_jwt_cookies_enabled: - return False - # If we set the request user in middleware for JWT auth, then we'd actually be checking JWT vs JWT user id. # Additionally, somehow the setting of request.user and the retrieving of request.user below causes some # unknown issue in production-like environments, and this allows us to skip that case. if _is_request_user_set_for_jwt_auth(): - # .. custom_attribute_name: skip_jwt_vs_session_check - # .. custom_attribute_description: This is temporary custom attribute to show that we skipped the check. - # This is probably redundant with the custom attribute set_user_from_jwt_status, but temporarily - # adding during initial rollout. - set_custom_attribute('skip_jwt_vs_session_check', True) return False # Get the session-based user from the underlying HttpRequest object. # This line taken from DRF SessionAuthentication. user = getattr(request._request, 'user', None) # pylint: disable=protected-access - if not user: # pragma: no cover - # .. custom_attribute_name: jwt_auth_request_user_not_found - # .. custom_attribute_description: This custom attribute shows when a - # session user was not found during JWT cookie authentication. This - # attribute will not exist if the session user is found. - set_custom_attribute('jwt_auth_request_user_not_found', True) + if not user: return False if user.is_authenticated: diff --git a/edx_rest_framework_extensions/auth/jwt/constants.py b/edx_rest_framework_extensions/auth/jwt/constants.py index 7cf21375..d50de7a4 100644 --- a/edx_rest_framework_extensions/auth/jwt/constants.py +++ b/edx_rest_framework_extensions/auth/jwt/constants.py @@ -3,4 +3,3 @@ """ JWT_DELIMITER = '.' -USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE' diff --git a/edx_rest_framework_extensions/auth/jwt/middleware.py b/edx_rest_framework_extensions/auth/jwt/middleware.py index 932be78f..b9c1c20d 100644 --- a/edx_rest_framework_extensions/auth/jwt/middleware.py +++ b/edx_rest_framework_extensions/auth/jwt/middleware.py @@ -18,19 +18,13 @@ from edx_rest_framework_extensions.auth.jwt.authentication import ( set_flag_is_request_user_set_for_jwt_auth, ) -from edx_rest_framework_extensions.auth.jwt.constants import ( - JWT_DELIMITER, - USE_JWT_COOKIE_HEADER, -) +from edx_rest_framework_extensions.auth.jwt.constants import JWT_DELIMITER from edx_rest_framework_extensions.auth.jwt.cookies import ( jwt_cookie_header_payload_name, jwt_cookie_name, jwt_cookie_signature_name, ) -from edx_rest_framework_extensions.config import ( - ENABLE_FORGIVING_JWT_COOKIES, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, -) +from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE from edx_rest_framework_extensions.permissions import ( LoginRedirectIfUnauthenticated, NotJwtRestrictedApplication, @@ -106,9 +100,6 @@ class JwtRedirectToLoginIfUnauthenticatedMiddleware(MiddlewareMixin): using the LoginRedirectIfUnauthenticated permission class. Enables a DRF view to redirect the user to login when they are unauthenticated. - It automatically enables JWT-cookie-based authentication by setting the - `USE_JWT_COOKIE_HEADER` for endpoints using the LoginRedirectIfUnauthenticated - permission. This can be used to convert a plain Django view using @login_required into a DRF APIView, which is useful to enable our DRF JwtAuthentication class. @@ -139,18 +130,10 @@ def process_view(self, request, view_func, view_args, view_kwargs): # pylint: d """ Enables Jwt Authentication for endpoints using the LoginRedirectIfUnauthenticated permission class. """ + # Note: Rather than caching here, this could be called directly in process_response based on the request, + # which would require using reverse to determine the view. self._check_and_cache_login_required_found(view_func) - # 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): """ Redirects unauthenticated users to login when LoginRedirectIfUnauthenticated permission class was used. @@ -230,15 +213,6 @@ def process_view(self, request, view_func, view_args, view_kwargs): # pylint: d """ 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 - # .. 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. - 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()) @@ -275,18 +249,12 @@ def process_view(self, request, view_func, view_args, view_kwargs): # pylint: d # handled through a more traditional AuthenticationMiddleware that handles both JWT cookies and sessions in # the future. if has_reconstituted_jwt_cookie and get_setting(ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE): - if get_setting(ENABLE_FORGIVING_JWT_COOKIES): - # Since this call to the user is not made lazily, and has the potential to cause issues, we - # ensure it is only used in the case of ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE. - if not get_user(request).is_authenticated: - # Similar to django/contrib/auth/middleware.py AuthenticationMiddleware. - set_flag_is_request_user_set_for_jwt_auth() - request.user = SimpleLazyObject(lambda: _get_cached_user_from_jwt(request, view_func)) - else: - # Disabling ENABLE_FORGIVING_JWT_COOKIES will provide the original implementation, - # without checking the user outside of the lazy call (and without caching). This - # ensures there is no change in behavior until we test ENABLE_FORGIVING_JWT_COOKIES. - request.user = SimpleLazyObject(lambda: _get_user_from_jwt_original(request, view_func)) + # Since this call to the user is not made lazily, and has the potential to cause issues, we + # ensure it is only used in the case of ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE. + if not get_user(request).is_authenticated: + # Similar to django/contrib/auth/middleware.py AuthenticationMiddleware. + set_flag_is_request_user_set_for_jwt_auth() + request.user = SimpleLazyObject(lambda: _get_cached_user_from_jwt(request, view_func)) def _get_module_request_cache(): @@ -307,22 +275,6 @@ def _get_cached_user_from_jwt(request, view_func): return _get_module_request_cache()[_JWT_USER_CACHE_KEY] -def _get_user_from_jwt_original(request, view_func): - """ - Returns user from JWT authentication using the original implementation. - - Although this implementation is not ideal, it is temporarily being left in place - for backward-compatibility when ENABLE_FORGIVING_JWT_COOKIES is disabled. - """ - # The original implementation first returned the result of the authenticated session - # user if it were available, which is not really what this method says it will do. - user = get_user(request) - if user.is_authenticated: - return user - - return _get_user_from_jwt(request, view_func) - - def _get_user_from_jwt(request, view_func): """ Performs JWT Authentication and returns the user, or AnonymousUser if the user is 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 f0d110bd..5ec1fc17 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py @@ -10,7 +10,7 @@ from django.urls import reverse from edx_django_utils.cache import RequestCache from jwt import exceptions as jwt_exceptions -from rest_framework.exceptions import AuthenticationFailed, PermissionDenied +from rest_framework.exceptions import AuthenticationFailed from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response @@ -22,7 +22,6 @@ JwtAuthentication, JwtSessionUserMismatchError, ) -from edx_rest_framework_extensions.auth.jwt.constants import USE_JWT_COOKIE_HEADER from edx_rest_framework_extensions.auth.jwt.cookies import ( jwt_cookie_header_payload_name, jwt_cookie_name, @@ -33,11 +32,7 @@ generate_jwt_token, generate_latest_version_payload, ) -from edx_rest_framework_extensions.config import ( - ENABLE_FORGIVING_JWT_COOKIES, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, -) -from edx_rest_framework_extensions.settings import get_setting +from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE from edx_rest_framework_extensions.tests import factories @@ -221,14 +216,11 @@ def test_authenticate_credentials_no_usernames(self): 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() drf_request = Request(request) assert JwtAuthentication().authenticate(drf_request) mock_enforce_csrf.assert_called_with(drf_request) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'success-cookie') @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') @@ -236,11 +228,10 @@ def test_authenticate_csrf_protected(self, mock_set_custom_attribute): """ Ensure authenticate for JWTs properly handles CSRF errors. - Note: When using forgiving JWTs, all JWT cookie exceptions, including CSRF, will + Note: With 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. @@ -248,14 +239,8 @@ def test_authenticate_csrf_protected(self, mock_set_custom_attribute): drf_request = Request(request) with mock.patch.object(JSONWebTokenAuthentication, 'authenticate', return_value=('mock-user', "mock-auth")): - if get_setting(ENABLE_FORGIVING_JWT_COOKIES): - assert JwtAuthentication().authenticate(drf_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(drf_request) - assert context_manager.exception.detail.startswith('CSRF Failed') - mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-cookie') + assert JwtAuthentication().authenticate(drf_request) is None + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'forgiven-failure') mock_set_custom_attribute.assert_any_call( 'jwt_auth_failed', @@ -287,10 +272,6 @@ def test_authenticate_with_correct_jwt_authorization(self, mock_set_custom_attri jwt_token = self._get_test_jwt_token() 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') @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') @@ -343,14 +324,8 @@ def test_authenticate_jwt_and_session_mismatch(self, mock_set_custom_attribute): self.client.force_login(session_user) response = self.client.get(reverse('authenticated-view')) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'success-cookie') - if is_forgiving_jwt_cookies_enabled: - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) - else: - set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] - assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) assert response.status_code == 200 @override_settings( @@ -374,16 +349,9 @@ def test_authenticate_jwt_and_session_mismatch_bad_signature_cookie(self, mock_s self.client.force_login(session_user) response = self.client.get(reverse('authenticated-view')) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) mock_set_custom_attribute.assert_any_call('failed_jwt_cookie_user_id', jwt_user_id) - if is_forgiving_jwt_cookies_enabled: - mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) - else: - mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-cookie') - set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] - assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) assert response.status_code == 401 @override_settings( @@ -405,16 +373,9 @@ def test_authenticate_jwt_and_session_mismatch_invalid_cookie(self, mock_set_cus self.client.force_login(session_user) response = self.client.get(reverse('authenticated-view')) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) mock_set_custom_attribute.assert_any_call('failed_jwt_cookie_user_id', 'decode-error') - if is_forgiving_jwt_cookies_enabled: - mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) - else: - mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-cookie') - set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] - assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) assert response.status_code == 401 @override_settings( @@ -435,8 +396,6 @@ def test_authenticate_jwt_and_session_match(self, mock_set_custom_attribute): self.client.force_login(test_user) response = self.client.get(reverse('authenticated-view')) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys assert response.status_code == 200 @@ -459,15 +418,12 @@ def test_authenticate_jwt_and_no_session(self, mock_set_custom_attribute): # unlike other tests, there is no force_login call to start the session response = self.client.get(reverse('authenticated-view')) - is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled) set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys assert response.status_code == 200 @override_settings( EDX_DRF_EXTENSIONS={ - ENABLE_FORGIVING_JWT_COOKIES: True, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, }, MIDDLEWARE=( @@ -483,7 +439,6 @@ def test_authenticate_jwt_and_session_mismatch_and_set_request_user(self, mock_s Tests failure for JWT cookie when there is a session user mismatch and a request to set user. - This tests coordination between ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE in middleware and JwtAuthentication. - - This test requires ENABLE_FORGIVING_JWT_COOKIES to get to ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE. - This test is kept with the rest of the JWT vs session user tests. """ session_user_id = 111 @@ -503,8 +458,6 @@ def test_authenticate_jwt_and_session_mismatch_and_set_request_user(self, mock_s response = self.client.get(reverse('authenticated-view')) assert response.status_code == 401 - # The case where forgiving JWTs is disabled is tested under other tests, including the middleware tests. - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', True) mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_user_id', session_user_id) mock_set_custom_attribute.assert_any_call('failed_jwt_cookie_user_id', jwt_user_id) mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-enforced-failure') @@ -512,7 +465,6 @@ def test_authenticate_jwt_and_session_mismatch_and_set_request_user(self, mock_s @override_settings( EDX_DRF_EXTENSIONS={ - ENABLE_FORGIVING_JWT_COOKIES: True, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, }, MIDDLEWARE=( @@ -528,7 +480,6 @@ def test_authenticate_jwt_and_no_session_and_set_request_user(self, mock_set_cus Tests success for JWT cookie when there is no session user and there is a request to set user. - This tests coordination between ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE in middleware and JwtAuthentication. - - This test requires ENABLE_FORGIVING_JWT_COOKIES to get to ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE. - This test is kept with the rest of the JWT vs session user tests. """ test_user = factories.UserFactory() @@ -542,9 +493,6 @@ def test_authenticate_jwt_and_no_session_and_set_request_user(self, mock_set_cus # unlike other tests, there is no force_login call to start the session response = self.client.get(reverse('authenticated-view')) - # The case where forgiving JWTs is disabled is tested under other tests, including the middleware tests. - mock_set_custom_attribute.assert_any_call('is_forgiving_jwt_cookies_enabled', True) - mock_set_custom_attribute.assert_any_call('skip_jwt_vs_session_check', True) set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_mismatch_session_user_id' not in set_custom_attribute_keys assert response.status_code == 200 @@ -568,13 +516,6 @@ def _get_test_jwt_token_payload_and_signature(self, user=None): return header_and_payload, signature -# 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 - - class TestLowestJWTException: """ Test that we're getting the correct exception out of a stack of exceptions when checking a JWT for auth Fails. 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 d86b677a..0de1f90b 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py @@ -2,7 +2,7 @@ Unit tests for jwt authentication middlewares. """ from itertools import product -from unittest.mock import ANY, Mock, patch +from unittest.mock import Mock, patch import ddt from django.http.cookie import SimpleCookie @@ -18,7 +18,6 @@ from rest_framework.viewsets import ViewSet 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_header_payload_name, jwt_cookie_name, @@ -28,13 +27,8 @@ EnsureJWTAuthSettingsMiddleware, JwtAuthCookieMiddleware, JwtRedirectToLoginIfUnauthenticatedMiddleware, - _get_cached_user_from_jwt, - _get_user_from_jwt_original, -) -from edx_rest_framework_extensions.config import ( - ENABLE_FORGIVING_JWT_COOKIES, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, ) +from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE from edx_rest_framework_extensions.permissions import ( IsStaff, IsSuperuser, @@ -42,7 +36,6 @@ LoginRedirectIfUnauthenticated, NotJwtRestrictedApplication, ) -from edx_rest_framework_extensions.settings import get_setting from edx_rest_framework_extensions.tests.factories import UserFactory @@ -311,7 +304,7 @@ def setUp(self): ('/isauthenticatedandloginredirect/', False, 302), ('/isauthenticatedandloginredirect/', True, 200), ('/isauthenticated/', False, 401), - ('/isauthenticated/', True, 401), + ('/isauthenticated/', True, 200), ('/nopermissionsrequired/', False, 200), ('/nopermissionsrequired/', True, 200), ) @@ -328,8 +321,6 @@ 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: @@ -337,11 +328,11 @@ def test_login_required_middleware(self, url, has_jwt_cookies, expected_status): @ddt.data( ('/loginredirectifunauthenticated/', False, 302), - ('/loginredirectifunauthenticated/', True, 302), + ('/loginredirectifunauthenticated/', True, 200), ('/isauthenticatedandloginredirect/', False, 302), - ('/isauthenticatedandloginredirect/', True, 302), + ('/isauthenticatedandloginredirect/', True, 200), ('/isauthenticated/', False, 401), - ('/isauthenticated/', True, 401), + ('/isauthenticated/', True, 200), ('/nopermissionsrequired/', False, 200), ('/nopermissionsrequired/', True, 200), ) @@ -358,22 +349,12 @@ 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 @@ -402,12 +383,6 @@ def setUp(self): self.mock_response = Mock() self.middleware = JwtAuthCookieMiddleware(self.mock_response) - @patch('edx_rest_framework_extensions.auth.jwt.middleware.set_custom_attribute') - 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_any_call('use_jwt_cookie_requested', False) - @ddt.data( (jwt_cookie_header_payload_name(), jwt_cookie_signature_name()), (jwt_cookie_signature_name(), jwt_cookie_header_payload_name()), @@ -418,7 +393,6 @@ def test_do_not_use_jwt_cookies(self, mock_set_custom_attribute): def test_missing_cookies( self, set_cookie_name, missing_cookie_name, mock_set_custom_attribute, mock_log ): - self.request.META[USE_JWT_COOKIE_HEADER] = 'true' self.request.COOKIES[set_cookie_name] = 'test' self.middleware.process_view(self.request, None, None, None) self.assertIsNone(self.request.COOKIES.get(jwt_cookie_name())) @@ -426,99 +400,25 @@ def test_missing_cookies( '%s cookie is missing. JWT auth cookies will not be reconstituted.' % 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_rest_framework_extensions.auth.jwt.middleware.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_any_call('use_jwt_cookie_requested', True) mock_set_custom_attribute.assert_any_call('has_jwt_cookie', False) @patch('edx_rest_framework_extensions.auth.jwt.middleware.set_custom_attribute') def test_success(self, mock_set_custom_attribute): - self.request.META[USE_JWT_COOKIE_HEADER] = 'true' self.request.COOKIES[jwt_cookie_header_payload_name()] = 'header.payload' 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_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 - @patch('edx_rest_framework_extensions.auth.jwt.middleware.log') - @patch( - 'edx_rest_framework_extensions.auth.jwt.middleware._get_cached_user_from_jwt', - wraps=_get_cached_user_from_jwt - ) - @patch( - 'edx_rest_framework_extensions.auth.jwt.middleware._get_user_from_jwt_original', - wraps=_get_user_from_jwt_original - ) - @ddt.data( - ('/nopermissionsrequired/', True, True, True, None, False), - ('/nopermissionsrequired/', True, False, False, None, False), - ('/nopermissionsrequired/', False, False, True, _LOG_WARN_AUTHENTICATION_FAILED, False), - ('/nopermissionsrequired/', False, False, False, None, False), - ('/nopermissionsrequired/', True, True, True, None, True), - ('/unauthenticated/', True, False, True, _LOG_WARN_MISSING_JWT_AUTHENTICATION_CLASS, False), - ('/unauthenticated/', True, False, True, None, False), - ) - @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, send_post, - mock_protected_get_user_original, mock_protected_get_cached_user_from_jwt, mock_log, - ): - header = {USE_JWT_COOKIE_HEADER: 'true'} - 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=( - '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_FORGIVING_JWT_COOKIES: False, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: is_toggle_enabled, - } - ): - if send_post: - response = self.client.post(url, {}, content_type="application/json", **header) - else: - response = self.client.get(url, **header) - self.assertEqual(200, response.status_code) - - if log_warning == self._LOG_WARN_AUTHENTICATION_FAILED: - mock_log.debug.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.debug.assert_called_once_with( - 'Jwt Authentication expected, but view %s is not using a JwtAuthentication class.', ANY - ) - else: - mock_log.warn.assert_not_called() - # Ensure _get_user_from_jwt_original is called, since ENABLE_FORGIVING_JWT_COOKIES is disabled - if is_request_user_set: - mock_protected_get_user_original.assert_called() - # Ensure _get_cached_user_from_jwt is not called, since ENABLE_FORGIVING_JWT_COOKIES is disabled - mock_protected_get_cached_user_from_jwt.assert_not_called() - - @patch( - 'edx_rest_framework_extensions.auth.jwt.middleware._get_cached_user_from_jwt', - wraps=_get_cached_user_from_jwt - ) @override_settings( ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_middleware', MIDDLEWARE=( @@ -527,16 +427,11 @@ def test_set_request_user_with_use_jwt_cookie( 'django.contrib.auth.middleware.AuthenticationMiddleware', 'edx_rest_framework_extensions.auth.jwt.tests.test_middleware.CheckRequestUserForJwtAuthMiddleware', ), - EDX_DRF_EXTENSIONS={ - ENABLE_FORGIVING_JWT_COOKIES: True, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, - } + EDX_DRF_EXTENSIONS={ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True} ) - def test_set_request_user_with_use_jwt_cookie_and_forgiving_jwt_cookie_enabled( - self, mock_protected_get_cached_user_from_jwt - ): + def test_set_request_user_with_use_jwt_cookie_middleware(self): """ - Tests set request user when ENABLE_FORGIVING_JWT_COOKIES is also enabled. + Tests set request user with the JwtAuthCookieMiddleware. """ self.client.cookies = _get_test_cookie(is_cookie_valid=True) @@ -544,16 +439,6 @@ def test_set_request_user_with_use_jwt_cookie_and_forgiving_jwt_cookie_enabled( # additional assertions in CheckRequestUserForJwtAuthMiddleware self.assertEqual(200, response.status_code) - # Ensure _get_cached_user_from_jwt is called, since ENABLE_FORGIVING_JWT_COOKIES is enabled - mock_protected_get_cached_user_from_jwt.assert_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): diff --git a/edx_rest_framework_extensions/config.py b/edx_rest_framework_extensions/config.py index bf5e94c5..3f42b5a0 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -12,13 +12,3 @@ # .. 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/settings.py b/edx_rest_framework_extensions/settings.py index 57572afa..7bb12c6d 100644 --- a/edx_rest_framework_extensions/settings.py +++ b/edx_rest_framework_extensions/settings.py @@ -15,26 +15,23 @@ from django.conf import settings from rest_framework_jwt.settings import api_settings -from edx_rest_framework_extensions.config import ( - ENABLE_FORGIVING_JWT_COOKIES, - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, -) +from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE logger = logging.getLogger(__name__) DEFAULT_SETTINGS = { - 'OAUTH2_USER_INFO_URL': None, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False, + 'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (), # Map JWT claims to user attributes. 'JWT_PAYLOAD_USER_ATTRIBUTE_MAPPING': { 'administrator': 'is_staff', 'email': 'email', }, - 'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (), - ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False, - ENABLE_FORGIVING_JWT_COOKIES: False, + + 'OAUTH2_USER_INFO_URL': None, } diff --git a/edx_rest_framework_extensions/tests/test_middleware.py b/edx_rest_framework_extensions/tests/test_middleware.py index c49b0061..c17040cc 100644 --- a/edx_rest_framework_extensions/tests/test_middleware.py +++ b/edx_rest_framework_extensions/tests/test_middleware.py @@ -9,7 +9,6 @@ from django.test import RequestFactory, TestCase from edx_django_utils.cache import RequestCache -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.middleware import ( RequestCustomAttributesMiddleware, @@ -112,7 +111,6 @@ def test_request_auth_type_guess_token_attribute(self, token, expected_token_typ @patch('edx_django_utils.monitoring.set_custom_attribute') def test_request_auth_type_guess_jwt_cookie_attribute(self, mock_set_custom_attribute): self.request.user = UserFactory() - self.request.META[USE_JWT_COOKIE_HEADER] = True self.request.COOKIES[jwt_cookie_name()] = 'reconstituted-jwt-cookie' self.middleware.process_response(self.request, None)