From 189a5c7c09280e33d5a58d0aa6470730c23caeeb Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 4 Jan 2024 14:43:39 -0500 Subject: [PATCH 1/2] Revert "feat: JWT vs session user check with username (#422)" This reverts commit d4f50d7b58a61e3361b9401b6b447d12ca851b06. --- CHANGELOG.rst | 14 - edx_rest_framework_extensions/__init__.py | 2 +- .../auth/jwt/authentication.py | 133 +++++--- .../auth/jwt/tests/test_authentication.py | 321 ++++++++++++++++-- edx_rest_framework_extensions/config.py | 15 +- edx_rest_framework_extensions/settings.py | 2 + 6 files changed, 391 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d3954bf2..ff4192ee 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,20 +12,6 @@ Change Log Unreleased ---------- -[9.1.0] - 2024-01-03 --------------------- - -Updated -~~~~~~~ - -* Simplified JWT cookie vs session user check by checking username instead of lms user id. - - * Removed ``VERIFY_LMS_USER_ID_PROPERTY_NAME``, which is no longer needed. - * Removed custom attribute ``jwt_auth_get_lms_user_id_status``, since we no longer attempt to get the lms_user_id from the user object. - * Renames custom attribute ``jwt_auth_mismatch_session_lms_user_id`` to ``jwt_auth_mismatch_session_username``. - * Adds custom attribute ``jwt_auth_mismatch_jwt_cookie_username``. - * Adds custom attribute ``jwt_cookie_unsafe_decode_issue`` for when a JWT cookie cannot even be unsafely decoded. - [9.0.1] - 2023-12-06 -------------------- diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index 1a55d23b..10c15fe9 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__ = '9.1.0' # pragma: no cover +__version__ = '9.0.1' # pragma: no cover diff --git a/edx_rest_framework_extensions/auth/jwt/authentication.py b/edx_rest_framework_extensions/auth/jwt/authentication.py index 0d882433..74b5b99a 100644 --- a/edx_rest_framework_extensions/auth/jwt/authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/authentication.py @@ -17,6 +17,7 @@ from edx_rest_framework_extensions.config import ( ENABLE_FORGIVING_JWT_COOKIES, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, + VERIFY_LMS_USER_ID_PROPERTY_NAME, ) from edx_rest_framework_extensions.settings import get_setting @@ -272,22 +273,15 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request): - Also adds monitoring details for mismatches. - Should only be called for JWT cookies. """ + # adds early monitoring for the JWT LMS user_id + jwt_lms_user_id = self._get_and_monitor_jwt_cookie_lms_user_id(request) + 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 - jwt_username, jwt_lms_user_id = self._get_unsafe_jwt_cookie_username_and_lms_user_id(request) - - # add early monitoring for the JWT LMS user_id for observability for a variety of user cases - - # .. custom_attribute_name: jwt_cookie_lms_user_id - # .. custom_attribute_description: The LMS user_id pulled from the - # JWT cookie, or None if the JWT was corrupt and it wasn't found. - # Note that the decoding is unsafe, so this isn't just for valid cookies. - set_custom_attribute('jwt_cookie_lms_user_id', jwt_lms_user_id) - - # If we set the request user in middleware for JWT auth, then we'd actually be checking JWT vs JWT username. + # 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(): @@ -309,8 +303,8 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request): # Get the session-based user from the underlying HttpRequest object. # This line taken from DRF SessionAuthentication. - session_user = getattr(wsgi_request, 'user', None) - if not session_user: # pragma: no cover + user = getattr(wsgi_request, 'user', None) + 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 @@ -318,54 +312,101 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request): set_custom_attribute('jwt_auth_request_user_not_found', True) return False - if not session_user.is_authenticated or not session_user.username or session_user.username == jwt_username: + if user.is_authenticated: + session_lms_user_id = self._get_lms_user_id_from_user(user) + else: + session_lms_user_id = None + + if not session_lms_user_id or session_lms_user_id == jwt_lms_user_id: return False - # .. custom_attribute_name: jwt_auth_mismatch_session_username - # .. custom_attribute_description: The session authentication username if it - # does not match the JWT cookie username. If there is no session user, - # or if it matches the JWT cookie username, this attribute will not be included. - # Session authentication may have completed in middleware + # .. custom_attribute_name: jwt_auth_mismatch_session_lms_user_id + # .. custom_attribute_description: The session authentication LMS user id if it + # does not match the JWT cookie LMS user id. If there is no session user, + # or no LMS user id for the session user, or if it matches the JWT cookie user id, + # this attribute will not be included. Session authentication may have completed in middleware # before getting to DRF. Although this authentication won't stick, # because it will be replaced by DRF authentication, we record it, # because it sometimes does not match the JWT cookie user. - set_custom_attribute('jwt_auth_mismatch_session_username', session_user.username) - # .. custom_attribute_name: jwt_auth_mismatch_jwt_cookie_username - # .. custom_attribute_description: The JWT cookie username if it - # does not match the session authentication username. - # See jwt_auth_mismatch_session_username description for more details. - # Note that there is a low chance that a corrupt JWT cookie will contain a - # username and user id that do not correlate, so we capture the actual username, - # even though it is likely redundant to jwt_cookie_lms_user_id. To minimize - # the use of PII, this attribute is only captured in the case of a mismatch. - set_custom_attribute('jwt_auth_mismatch_jwt_cookie_username', jwt_username) + set_custom_attribute('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) return True - def _get_unsafe_jwt_cookie_username_and_lms_user_id(self, request): + def _get_and_monitor_jwt_cookie_lms_user_id(self, request): """ - Returns a tuple of the (username, lms user id) from the JWT cookie, or (None, None) if not found. - """ - - # .. custom_attribute_name: jwt_cookie_unsafe_decode_issue - # .. custom_attribute_description: Since we are doing an unsafe JWT decode, it should generally work unless - # the JWT cookie were tampered with. This attribute will contain the value 'missing-claim' if either the - # username or user_id claim is missing, or 'decode-error' if the JWT cookie can't be decoded at all. This - # attribute will not exist if there is no issue decoding the cookie. + Returns the LMS user id from the JWT cookie, or None if not found + Notes: + - Also provides monitoring details for mismatches. + """ try: cookie_token = JSONWebTokenAuthentication.get_token_from_cookies(request.COOKIES) - unsafe_decoded_jwt = unsafe_jwt_decode_handler(cookie_token) - jwt_username = unsafe_decoded_jwt.get('username', None) - jwt_lms_user_id = unsafe_decoded_jwt.get('user_id', None) - if not jwt_username or not jwt_lms_user_id: - set_custom_attribute('jwt_cookie_unsafe_decode_issue', 'missing-claim') + invalid_decoded_jwt = unsafe_jwt_decode_handler(cookie_token) + jwt_lms_user_id = invalid_decoded_jwt.get('user_id', None) + jwt_lms_user_id_attribute_value = jwt_lms_user_id if jwt_lms_user_id else 'not-found' # pragma: no cover except Exception: # pylint: disable=broad-exception-caught - jwt_username = None jwt_lms_user_id = None - set_custom_attribute('jwt_cookie_unsafe_decode_issue', 'decode-error') + jwt_lms_user_id_attribute_value = 'decode-error' + + # .. custom_attribute_name: jwt_cookie_lms_user_id + # .. custom_attribute_description: The LMS user_id pulled from the + # JWT cookie. If the user_id claim is not found in the JWT, the attribute + # value will be 'not-found'. If the JWT simply can't be decoded, + # the attribute value will be 'decode-error'. Note that the id will be + # set in the case of expired JWTs, or other failures that can still be + # decoded. + set_custom_attribute('jwt_cookie_lms_user_id', jwt_lms_user_id_attribute_value) - return (jwt_username, jwt_lms_user_id) + return jwt_lms_user_id + + def _get_lms_user_id_from_user(self, user): + """ + Returns the lms_user_id from the user object if found, or None if not found. + + This is intended for use only by LMS user id matching code, and thus will provide appropriate error + logs in the case of misconfiguration. + """ + # .. custom_attribute_name: jwt_auth_get_lms_user_id_status + # .. custom_attribute_description: This custom attribute is intended to be temporary. It will allow + # us visibility into when and how the LMS user id is being found from the session user, which + # allows us to check the session's LMS user id with the JWT's LMS user id. Possible values include: + # - skip-check (disabled check, useful when lms_user_id would have been available), + # - not-configured (setting was None and lms_user_id is not found), + # - misconfigured (the property name supplied could not be found), + # - id-found (the id was found using the property name), + # - id-not-found (the property exists, but returned None) + + lms_user_id_property_name = get_setting(VERIFY_LMS_USER_ID_PROPERTY_NAME) + + # This special value acts like an emergency disable toggle in the event that the user object has an lms_user_id, + # but this LMS id check starts causing unforeseen issues and needs to be disabled. + skip_check_property_name = 'skip-check' + if lms_user_id_property_name == skip_check_property_name: + set_custom_attribute('jwt_auth_get_lms_user_id_status', skip_check_property_name) + return None + + if not lms_user_id_property_name: + if hasattr(user, 'lms_user_id'): + # The custom attribute will be set below. + lms_user_id_property_name = 'lms_user_id' + else: + set_custom_attribute('jwt_auth_get_lms_user_id_status', 'not-configured') + return None + + if not hasattr(user, lms_user_id_property_name): + logger.error(f'Misconfigured VERIFY_LMS_USER_ID_PROPERTY_NAME. User object has no attribute with name' + f' [{lms_user_id_property_name}]. User id validation will be skipped.') + set_custom_attribute('jwt_auth_get_lms_user_id_status', 'misconfigured') + return None + + # If the property is found, but returns None, validation will be skipped with no messaging. + lms_user_id = getattr(user, lms_user_id_property_name, None) + if lms_user_id: + set_custom_attribute('jwt_auth_get_lms_user_id_status', 'id-found') + else: # pragma: no cover + set_custom_attribute('jwt_auth_get_lms_user_id_status', 'id-not-found') + + return lms_user_id _IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY = '_is_request_user_for_jwt_set' 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 d2321cc8..6910fa13 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py @@ -36,6 +36,7 @@ from edx_rest_framework_extensions.config import ( ENABLE_FORGIVING_JWT_COOKIES, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, + VERIFY_LMS_USER_ID_PROPERTY_NAME, ) from edx_rest_framework_extensions.settings import get_setting from edx_rest_framework_extensions.tests import factories @@ -234,6 +235,97 @@ def test_authenticate_with_correct_jwt_cookie(self, mock_set_custom_attribute, m set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_with_django_request' not in set_custom_attribute_keys + @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_and_mistmatched_lms_user_id( + self, mock_set_custom_attribute, mock_enforce_csrf + ): + """ + Verify authenticate succeeds with a valid JWT cookie with a mismatched lms_user_id attribute. + + Notes: + - When VERIFY_LMS_USER_ID_PROPERTY_NAME is None, it will also check for an `lms_user_id` attribute. + - This test mocks lms_user_id with a different value from the the JWT cookie LMS user id. + - At this time, we still allow authentication to succeed with the JWT cookie user, but we add observability. + - The other mismatch tests use the middleware and less mocking, but it was too difficult to add the + lms_user_id attribute. + """ + request = RequestFactory().post('/') + request.META[USE_JWT_COOKIE_HEADER] = 'true' + request.COOKIES[jwt_cookie_name()] = self._get_test_jwt_token() + session_user = factories.UserFactory(id=111) + + # set up request user with an lms_user_id attribute that will be compared to the JWT LMS user id + session_lms_user_id = 333 + session_user.lms_user_id = session_lms_user_id + request.user = session_user + + 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') + set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] + assert 'jwt_auth_with_django_request' not in set_custom_attribute_keys + + if is_forgiving_jwt_cookies_enabled: + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') + else: + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys + + @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_and_skipped_check( + self, mock_set_custom_attribute, mock_enforce_csrf + ): + """ + Verify authenticate succeeds with a valid JWT cookie and a skipped user id check. + + Notes: + - When VERIFY_LMS_USER_ID_PROPERTY_NAME is 'skip-check', the `lms_user_id` attribute should be ignored. + - This mocks lms_user_id with a different value from the the JWT cookie LMS user id. + - The other mismatch tests use the middleware and less mocking, but it was too difficult to add the + lms_user_id attribute. + """ + request = RequestFactory().post('/') + request.META[USE_JWT_COOKIE_HEADER] = 'true' + request.COOKIES[jwt_cookie_name()] = self._get_test_jwt_token() + session_user = factories.UserFactory(id=111) + + # set up request user with an lms_user_id attribute that will be compared to the JWT LMS user id + session_lms_user_id = 333 + session_user.lms_user_id = session_lms_user_id + request.user = session_user + + drf_request = Request(request) + + is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES) + with override_settings( + EDX_DRF_EXTENSIONS={ + ENABLE_FORGIVING_JWT_COOKIES: is_forgiving_jwt_cookies_enabled, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'skip-check', + }, + ): + 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') + set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] + assert 'jwt_auth_with_django_request' not in set_custom_attribute_keys + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + if is_forgiving_jwt_cookies_enabled: + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'skip-check') + else: + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys + @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_and_django_request( @@ -361,8 +453,9 @@ def test_authenticate_with_bearer_token(self, mock_set_custom_attribute): @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') def test_authenticate_jwt_and_session_mismatch(self, mock_set_custom_attribute): """ Tests monitoring for JWT cookie when there is a session user mismatch """ - session_user = factories.UserFactory(id=111, username='session-name') - jwt_user = factories.UserFactory(id=222, username='jwt-name') + session_lms_user_id = 111 + session_user = factories.UserFactory(id=session_lms_user_id) + jwt_user = factories.UserFactory(id=222) self.client.cookies = SimpleCookie({ jwt_cookie_name(): self._get_test_jwt_token(user=jwt_user), }) @@ -371,6 +464,7 @@ def test_authenticate_jwt_and_session_mismatch(self, mock_set_custom_attribute): with override_settings( EDX_DRF_EXTENSIONS={ ENABLE_FORGIVING_JWT_COOKIES: is_forgiving_jwt_cookies_enabled, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'id', }, ): self.client.force_login(session_user) @@ -381,15 +475,12 @@ def test_authenticate_jwt_and_session_mismatch(self, mock_set_custom_attribute): ) 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_cookie_lms_user_id', jwt_user.id # pylint: disable=no-member - ) - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_username', session_user.username) - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', jwt_user.username) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') else: set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] - assert 'jwt_auth_mismatch_session_username' not in set_custom_attribute_keys - assert 'jwt_auth_mismatch_jwt_cookie_username' not in set_custom_attribute_keys + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 200 @override_settings( @@ -402,8 +493,10 @@ def test_authenticate_jwt_and_session_mismatch(self, mock_set_custom_attribute): @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') def test_authenticate_jwt_and_session_mismatch_bad_signature_cookie(self, mock_set_custom_attribute): """ Tests monitoring for JWT cookie with a bad signature when there is a session user mismatch """ - session_user = factories.UserFactory(id=111, username='session-name') - jwt_user = factories.UserFactory(id=222, username='jwt-name') + session_lms_user_id = 111 + session_user = factories.UserFactory(id=session_lms_user_id) + jwt_user_id = 222 + jwt_user = factories.UserFactory(id=jwt_user_id) self.client.cookies = SimpleCookie({ jwt_cookie_name(): self._get_test_jwt_token(user=jwt_user, is_valid_signature=False), }) @@ -412,6 +505,7 @@ def test_authenticate_jwt_and_session_mismatch_bad_signature_cookie(self, mock_s with override_settings( EDX_DRF_EXTENSIONS={ ENABLE_FORGIVING_JWT_COOKIES: is_forgiving_jwt_cookies_enabled, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'id', }, ): self.client.force_login(session_user) @@ -420,18 +514,16 @@ def test_authenticate_jwt_and_session_mismatch_bad_signature_cookie(self, mock_s 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_cookie_lms_user_id', jwt_user_id) if is_forgiving_jwt_cookies_enabled: - mock_set_custom_attribute.assert_any_call( - 'jwt_cookie_lms_user_id', jwt_user.id # pylint: disable=no-member - ) mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_username', session_user.username) - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', jwt_user.username) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') 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_username' not in set_custom_attribute_keys - assert 'jwt_auth_mismatch_jwt_cookie_username' not in set_custom_attribute_keys + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 401 @override_settings( @@ -444,7 +536,8 @@ def test_authenticate_jwt_and_session_mismatch_bad_signature_cookie(self, mock_s @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') def test_authenticate_jwt_and_session_mismatch_invalid_cookie(self, mock_set_custom_attribute): """ Tests monitoring for invalid JWT cookie when there is a session user mismatch """ - session_user = factories.UserFactory(id=111, username='session-name') + session_lms_user_id = 111 + session_user = factories.UserFactory(id=session_lms_user_id) self.client.cookies = SimpleCookie({ jwt_cookie_name(): 'invalid-cookie', }) @@ -453,6 +546,7 @@ def test_authenticate_jwt_and_session_mismatch_invalid_cookie(self, mock_set_cus with override_settings( EDX_DRF_EXTENSIONS={ ENABLE_FORGIVING_JWT_COOKIES: is_forgiving_jwt_cookies_enabled, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'id', }, ): self.client.force_login(session_user) @@ -461,17 +555,16 @@ def test_authenticate_jwt_and_session_mismatch_invalid_cookie(self, mock_set_cus 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_cookie_lms_user_id', 'decode-error') if is_forgiving_jwt_cookies_enabled: - mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', None) - mock_set_custom_attribute.assert_any_call('jwt_cookie_unsafe_decode_issue', 'decode-error') mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-failure') - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_username', session_user.username) - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', None) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') 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_username' not in set_custom_attribute_keys - assert 'jwt_auth_mismatch_jwt_cookie_username' not in set_custom_attribute_keys + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 401 @override_settings( @@ -493,6 +586,7 @@ def test_authenticate_jwt_and_session_match(self, mock_set_custom_attribute): with override_settings( EDX_DRF_EXTENSIONS={ ENABLE_FORGIVING_JWT_COOKIES: is_forgiving_jwt_cookies_enabled, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'id', }, ): self.client.force_login(test_user) @@ -503,6 +597,10 @@ def test_authenticate_jwt_and_session_match(self, mock_set_custom_attribute): ) set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + if is_forgiving_jwt_cookies_enabled: + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') + else: + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 200 @override_settings( @@ -527,6 +625,7 @@ def test_authenticate_jwt_and_no_session(self, mock_set_custom_attribute): 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_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 200 @override_settings( @@ -541,17 +640,121 @@ def test_authenticate_jwt_and_no_session(self, mock_set_custom_attribute): ), ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_authentication', ) + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.logger', autospec=True) + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute', autospec=True) + def test_authenticate_no_lms_user_id_property_and_set_request_user(self, mock_set_custom_attribute, mock_logger): + """ + Tests JWT cookie success when VERIFY_LMS_USER_ID_PROPERTY_NAME is not set 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. + - Uses default of VERIFY_LMS_USER_ID_PROPERTY_NAME as None, and skips user id validation. + - This test is kept with the rest of the JWT vs session user tests. + """ + session_user = factories.UserFactory(id=111) + jwt_user_id = 222 + jwt_user = factories.UserFactory(id=jwt_user_id) + jwt_header_payload, jwt_signature = self._get_test_jwt_token_payload_and_signature(user=jwt_user) + # Cookie parts will be recombined by JwtAuthCookieMiddleware + self.client.cookies = SimpleCookie({ + jwt_cookie_header_payload_name(): jwt_header_payload, + jwt_cookie_signature_name(): jwt_signature, + }) + + self.client.force_login(session_user) + + response = self.client.get(reverse('authenticated-view')) + assert response.status_code == 200 + + # 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_result', 'success-cookie') + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'not-configured') + mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', jwt_user_id) + set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_failed' not in set_custom_attribute_keys + mock_logger.error.assert_not_called() + + @override_settings( + EDX_DRF_EXTENSIONS={ + ENABLE_FORGIVING_JWT_COOKIES: True, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'unknown_property', + }, + MIDDLEWARE=( + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', + 'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware', + ), + ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_authentication', + ) + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.logger', autospec=True) + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute', autospec=True) + def test_authenticate_unknown_user_id_property_and_set_request_user(self, mock_set_custom_attribute, mock_logger): + """ + Tests JWT cookie success when VERIFY_LMS_USER_ID_PROPERTY_NAME is misconfigured with 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. + - The misconfigured VERIFY_LMS_USER_ID_PROPERTY_NAME will result in an error log. + - This test is kept with the rest of the JWT vs session user tests. + """ + session_user = factories.UserFactory(id=111) + jwt_lms_user_id = 222 + jwt_header_payload, jwt_signature = self._get_test_jwt_token_payload_and_signature( + user=session_user, lms_user_id=jwt_lms_user_id + ) + # Cookie parts will be recombined by JwtAuthCookieMiddleware + self.client.cookies = SimpleCookie({ + jwt_cookie_header_payload_name(): jwt_header_payload, + jwt_cookie_signature_name(): jwt_signature, + }) + + self.client.force_login(session_user) + + response = self.client.get(reverse('authenticated-view')) + assert response.status_code == 200 + + # 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_result', 'success-cookie') + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'misconfigured') + mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', jwt_lms_user_id) + set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] + assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_failed' not in set_custom_attribute_keys + + # assert for error log for misconfigured VERIFY_LMS_USER_ID_PROPERTY_NAME + assert 'unknown_property' in mock_logger.error.call_args_list[0][0][0] + + @override_settings( + EDX_DRF_EXTENSIONS={ + ENABLE_FORGIVING_JWT_COOKIES: True, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'id', + }, + MIDDLEWARE=( + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', + 'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware', + ), + ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_authentication', + ) @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') - def test_authenticate_mismatch_with_set_request_user(self, mock_set_custom_attribute): + def test_authenticate_user_id_property_and_set_request_user(self, mock_set_custom_attribute): """ - Tests failure for JWT cookie when there is a session user mismatch with a request to set user. + Tests failure for JWT cookie when there is a session user mismatch with id property 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. + - Uses VERIFY_LMS_USER_ID_PROPERTY_NAME of 'id', as would be used by the identity service (LMS). - This test is kept with the rest of the JWT vs session user tests. """ - session_user = factories.UserFactory(id=111, username='session-name') - jwt_user = factories.UserFactory(id=222, username='jwt-name') + session_lms_user_id = 111 + session_user = factories.UserFactory(id=session_lms_user_id) + jwt_user_id = 222 + jwt_user = factories.UserFactory(id=jwt_user_id) jwt_header_payload, jwt_signature = self._get_test_jwt_token_payload_and_signature(user=jwt_user) # Cookie parts will be recombined by JwtAuthCookieMiddleware self.client.cookies = SimpleCookie({ @@ -567,11 +770,60 @@ def test_authenticate_mismatch_with_set_request_user(self, mock_set_custom_attri # 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_username', session_user.username) - mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', jwt_user.username) - mock_set_custom_attribute.assert_any_call( - 'jwt_cookie_lms_user_id', jwt_user.id # pylint: disable=no-member - ) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') + mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', jwt_user_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-enforced-failure') + mock_set_custom_attribute.assert_any_call('jwt_auth_failed', mock.ANY) + + @override_settings( + EDX_DRF_EXTENSIONS={ + ENABLE_FORGIVING_JWT_COOKIES: True, + ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: True, + VERIFY_LMS_USER_ID_PROPERTY_NAME: 'username', + }, + MIDDLEWARE=( + 'django.contrib.sessions.middleware.SessionMiddleware', + 'django.contrib.auth.middleware.AuthenticationMiddleware', + 'edx_rest_framework_extensions.auth.jwt.middleware.JwtAuthCookieMiddleware', + ), + ROOT_URLCONF='edx_rest_framework_extensions.auth.jwt.tests.test_authentication', + ) + @mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute') + def test_authenticate_other_user_property_and_set_request_user(self, mock_set_custom_attribute): + """ + Tests failure for JWT cookie with a session user mismatch with username property 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 = 222 + # Some services introduce an lms_user_id property to the user, which ideally is what we'd be testing. + # However, we use the username property because it is simplifies the testing. + session_user_lms_id = '111' # must be string because we are using username + session_user = factories.UserFactory(id=session_user_id, username=session_user_lms_id) + # In this test, the service's user id matches the JWT LMS user id, which ordinarily would never happen. + # However, for the purpose of this test, we want to ensure that this doesn't prevent the mismatch. + jwt_user_id = session_user_id + jwt_header_payload, jwt_signature = self._get_test_jwt_token_payload_and_signature(user=session_user) + # Cookie parts will be recombined by JwtAuthCookieMiddleware + self.client.cookies = SimpleCookie({ + jwt_cookie_header_payload_name(): jwt_header_payload, + jwt_cookie_signature_name(): jwt_signature, + }) + + self.client.force_login(session_user) + + with self.assertRaises(JwtSessionUserMismatchError): + 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_lms_user_id', session_user_lms_id) + mock_set_custom_attribute.assert_any_call('jwt_auth_get_lms_user_id_status', 'id-found') + mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', jwt_user_id) mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'user-mismatch-enforced-failure') mock_set_custom_attribute.assert_any_call('jwt_auth_failed', mock.ANY) @@ -616,6 +868,7 @@ def test_authenticate_jwt_and_no_session_and_set_request_user(self, mock_set_cus mock_set_custom_attribute.assert_any_call('jwt_cookie_lms_user_id', jwt_lms_user_id) set_custom_attribute_keys = [call.args[0] for call in mock_set_custom_attribute.call_args_list] assert 'jwt_auth_mismatch_session_lms_user_id' not in set_custom_attribute_keys + assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys assert response.status_code == 200 def _get_test_jwt_token(self, user=None, is_valid_signature=True, lms_user_id=None): diff --git a/edx_rest_framework_extensions/config.py b/edx_rest_framework_extensions/config.py index 14f7d09e..6de1d70c 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -7,7 +7,8 @@ # .. toggle_default: False # .. toggle_description: Toggle for setting request.user with jwt cookie authentication. This makes the JWT cookie # user available to middleware while processing the request, if the session user wasn't already available. This -# requires JwtAuthCookieMiddleware to work. +# requires JwtAuthCookieMiddleware to work. It is recommended to set VERIFY_LMS_USER_ID_PROPERTY_NAME if possible +# when using this feature. # .. toggle_use_cases: temporary # .. toggle_creation_date: 2019-10-15 # .. toggle_target_removal_date: 2024-12-31 @@ -25,3 +26,15 @@ # .. 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' + +# .. setting_name: EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME] +# .. setting_default: None ('lms_user_id' if found) +# .. setting_description: This setting should be set to the name of the user object property containing the LMS +# user id, if one exists. Examples might be 'id' or 'lms_user_id'. To enhance security and provide ease of use +# for this setting, if None is supplied, the property 'lms_user_id' will be used if found. In case of unforeseen +# issues using lms_user_id, the check can be fully disabled using 'skip-check' as the property name. The default +# was not set to 'lms_user_id' directly to avoid misconfiguration logging for services without an lms_user_id +# property. The property named by this setting will be used by JWT cookie authentication to verify that the (LMS) +# user id in the JWT is the same as the LMS user id for a service's session. This will cause failures in the case +# of forgiving cookies, and will simply be used for additional monitoring for successful cookie authentication. +VERIFY_LMS_USER_ID_PROPERTY_NAME = 'VERIFY_LMS_USER_ID_PROPERTY_NAME' diff --git a/edx_rest_framework_extensions/settings.py b/edx_rest_framework_extensions/settings.py index 57572afa..f56f21fc 100644 --- a/edx_rest_framework_extensions/settings.py +++ b/edx_rest_framework_extensions/settings.py @@ -18,6 +18,7 @@ from edx_rest_framework_extensions.config import ( ENABLE_FORGIVING_JWT_COOKIES, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, + VERIFY_LMS_USER_ID_PROPERTY_NAME, ) @@ -35,6 +36,7 @@ 'JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES': (), ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE: False, ENABLE_FORGIVING_JWT_COOKIES: False, + VERIFY_LMS_USER_ID_PROPERTY_NAME: None, } From 03f41e94ef7df5a0750d7e034a9158298d02f1e6 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Thu, 4 Jan 2024 14:48:47 -0500 Subject: [PATCH 2/2] fix: upgrade revert to 9.1.1 Revert of 9.1.0 until a fix is ready. --- CHANGELOG.rst | 12 ++++++++++++ edx_rest_framework_extensions/__init__.py | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ff4192ee..20463128 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,18 @@ Change Log Unreleased ---------- +[9.1.1] - 2024-01-04 +-------------------- +Updated +~~~~~~~ +* Reverted 9.1.0 change until issue can be fixed. + +[9.1.0] - 2024-01-03 +-------------------- +Updated +~~~~~~~ +* (Now reverted) Simplified JWT cookie vs session user check by checking username instead of lms user id. Note: this was reverted on 9.1.1. + [9.0.1] - 2023-12-06 -------------------- diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index 10c15fe9..cc193cc1 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__ = '9.0.1' # pragma: no cover +__version__ = '9.1.1' # pragma: no cover