From a17fc69ac777862f88d30099d8a4d983e9794f49 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Mon, 20 Nov 2023 17:44:01 -0500 Subject: [PATCH] fix!: bug in JWT vs session user id compare If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any service other than the identity service(LMS/CMS), where the session's local service user id would never match the JWT LMS user id when compared. This has been fixed. - The custom attribute jwt_auth_mismatch_session_lms_user_id was renamed to jwt_auth_mismatch_session_lms_user_id to make this more clear. - The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME] was added to enable choosing the user object property that contains the lms_user_id, if one exists. If this is unset, a service will simply skip this additional protection. - The custom attribute jwt_auth_get_lms_user_id_status was added to provide observability into the new functionality. BREAKING CHANGE: The breaking change only affects services with ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the original Session vs JWT user id check. Part of DEPR: https://github.com/openedx/edx-drf-extensions/issues/371 --- CHANGELOG.rst | 11 + edx_rest_framework_extensions/__init__.py | 2 +- .../auth/jwt/authentication.py | 53 ++- .../auth/jwt/tests/test_authentication.py | 313 ++++++++++++++---- edx_rest_framework_extensions/config.py | 8 + edx_rest_framework_extensions/settings.py | 2 + 6 files changed, 321 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4a0d04f7..ed2240e0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,17 @@ Change Log Unreleased ---------- +[9.0.0] - 2023-11-27 + +Fixed +~~~~~ +* **BREAKING CHANGE**: Fixes a bug for any service other than the identity service (LMS/CMS), where the session's local service user id would never match the JWT LMS user id when compared. + + * The custom attribute jwt_auth_mismatch_session_lms_user_id was renamed to jwt_auth_mismatch_session_lms_user_id to make this more clear. + * The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME] was added to enable choosing the user object property that contains the lms_user_id, if one exists. If this is unset, a service will simply skip this additional protection. + * The custom attribute jwt_auth_get_lms_user_id_status was added to provide observability into the new functionality. + * The breaking change only affects services with ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the existing Session vs JWT user id check. + [8.13.1] - 2023-11-15 --------------------- diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index 4bcb74bf..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.1' # 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 a765f1fd..c8d65ef2 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 @@ -335,7 +336,7 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id): # .. custom_attribute_name: jwt_auth_with_django_request # .. custom_attribute_description: There exists custom authentication code in the platform that is # calling JwtAuthentication with a Django request, rather than the expected DRF request. This - # custom attribute could be used to track down those usages and find ways to elimitate custom + # custom attribute could be used to track down those usages and find ways to eliminate custom # authentication code that lives outside of this library. set_custom_attribute('jwt_auth_with_django_request', True) @@ -351,25 +352,57 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id): return False if user.is_authenticated: - session_user_id = user.id + session_lms_user_id = self._get_lms_user_id_from_user(user) else: - session_user_id = None + session_lms_user_id = None - if not session_user_id or session_user_id == jwt_user_id: + if not session_lms_user_id or session_lms_user_id == jwt_user_id: return False - # .. custom_attribute_name: jwt_auth_mismatch_session_user_id - # .. custom_attribute_description: The session authentication user id if it - # does not match the JWT cookie user id. If there is no session user, - # or if it matches the JWT cookie user id, 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_user_id', session_user_id) + set_custom_attribute('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id) return True + 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. + + lms_user_id_property_name = get_setting(VERIFY_LMS_USER_ID_PROPERTY_NAME) + if not lms_user_id_property_name: + 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 3dec69f9..da955ae2 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 @@ -360,25 +361,35 @@ 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_id = 111 - session_user = factories.UserFactory(id=session_user_id) + 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), }) - 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 - assert response.status_code == 200 + 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) + response = self.client.get(reverse('authenticated-view')) + + 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_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_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( MIDDLEWARE=( @@ -390,28 +401,38 @@ 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_id = 111 - session_user = factories.UserFactory(id=session_user_id) + 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), }) - 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 - assert response.status_code == 401 + 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) + response = self.client.get(reverse('authenticated-view')) + + 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_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_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( MIDDLEWARE=( @@ -423,26 +444,36 @@ 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_id = 111 - session_user = factories.UserFactory(id=session_user_id) + session_lms_user_id = 111 + session_user = factories.UserFactory(id=session_lms_user_id) self.client.cookies = SimpleCookie({ jwt_cookie_name(): 'invalid-cookie', }) - 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 - assert response.status_code == 401 + 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) + response = self.client.get(reverse('authenticated-view')) + + 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_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_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( MIDDLEWARE=( @@ -459,14 +490,26 @@ def test_authenticate_jwt_and_session_match(self, mock_set_custom_attribute): jwt_cookie_name(): self._get_test_jwt_token(user=test_user), }) - 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 + 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) + response = self.client.get(reverse('authenticated-view')) + + 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 + 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( MIDDLEWARE=( @@ -489,7 +532,8 @@ def test_authenticate_jwt_and_no_session(self, mock_set_custom_attribute): 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 '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( @@ -504,17 +548,116 @@ 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 = factories.UserFactory(id=222) + 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') + 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 'failed_jwt_cookie_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_user = factories.UserFactory(id=222) + 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', 'misconfigured') + 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 'failed_jwt_cookie_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_jwt_and_session_mismatch_and_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 and 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_id = 111 - session_user = factories.UserFactory(id=session_user_id) + 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) @@ -532,7 +675,62 @@ def test_authenticate_jwt_and_session_mismatch_and_set_request_user(self, mock_s # 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('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('failed_jwt_cookie_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 lms_user_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. + - 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_user = session_user + 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) + + 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('failed_jwt_cookie_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) @@ -573,7 +771,8 @@ def test_authenticate_jwt_and_no_session_and_set_request_user(self, mock_set_cus 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 '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): diff --git a/edx_rest_framework_extensions/config.py b/edx_rest_framework_extensions/config.py index bf5e94c5..9d77a2e4 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -22,3 +22,11 @@ # .. 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 +# .. setting_description: This setting should be set to the name of the user attribute property containing the LMS +# user id. Examples might be `id` or `lms_user_id`. If there is no property, leave the default value of None. +# This is 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. +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, }