diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4a0d04f7..e094dcc8 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 set to None (the default), the check will use the lms_user_id property if it is found, and otherwise will skip this additional protection. In case of an unforeseen issue, use 'skip-check' to skip the check, even when there is an lms_user_id property. + * 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 appropriately in order to provide the existing Session vs JWT user id check. Note that only LMS/CMS will likely need to set this value. + [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..0837c2a9 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,74 @@ 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. 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 3dec69f9..204d8628 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 @@ -225,6 +226,42 @@ def test_authenticate_with_correct_jwt_cookie(self, mock_set_custom_attribute, m 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') + 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) @@ -233,6 +270,62 @@ 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 + 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( @@ -360,25 +453,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 +493,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 +536,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 +582,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 +624,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 +640,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 +767,60 @@ 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 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_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 +861,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..6de1d70c 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -5,11 +5,15 @@ # .. toggle_name: EDX_DRF_EXTENSIONS[ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE] # .. toggle_implementation: DjangoSetting # .. toggle_default: False -# .. toggle_description: Toggle for setting request.user with jwt cookie authentication +# .. toggle_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. 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: 2019-12-31 -# .. toggle_warning: This feature fixed ecommerce, but broke edx-platform. The toggle enables us to fix over time. +# .. toggle_target_removal_date: 2024-12-31 +# .. toggle_warning: This feature caused a memory leak in edx-platform. This toggle is temporary only if we can make it +# work in all services, or find a replacement. Consider making this a permanent toggle instead. # .. toggle_tickets: ARCH-1210, ARCH-1199, ARCH-1197 ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE = 'ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE' @@ -22,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, }