diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 20463128..480850f8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,6 +12,19 @@ Change Log Unreleased ---------- +[9.1.2] - 2024-01-07 +-------------------- +Updated +~~~~~~~ +* Restores and fixes simplified JWT cookie vs session user check by checking username instead of lms user id (originally introduced in 9.1.0, and removed in 9.1.1). + + * 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. + * Fixes mock JWT creation for tests to use ``preferred_username``, which is configured in each Open edX service. + [9.1.1] - 2024-01-04 -------------------- Updated diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py index cc193cc1..3aa81165 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.1' # pragma: no cover +__version__ = '9.1.2' # pragma: no cover diff --git a/edx_rest_framework_extensions/auth/jwt/authentication.py b/edx_rest_framework_extensions/auth/jwt/authentication.py index 74b5b99a..a79925ad 100644 --- a/edx_rest_framework_extensions/auth/jwt/authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/authentication.py @@ -17,7 +17,6 @@ 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 @@ -171,7 +170,7 @@ def authenticate_credentials(self, payload): """Get or create an active user with the username contained in the payload.""" # TODO it would be good to refactor this heavily-nested function. # pylint: disable=too-many-nested-blocks - username = payload.get('preferred_username') or payload.get('username') + username = self._get_username_from_payload(payload) if username is None: raise exceptions.AuthenticationFailed('JWT must include a preferred_username or username claim!') try: @@ -273,15 +272,22 @@ 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 - # If we set the request user in middleware for JWT auth, then we'd actually be checking JWT vs JWT user id. + 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. # 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(): @@ -303,8 +309,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. - user = getattr(wsgi_request, 'user', None) - if not user: # pragma: no cover + session_user = getattr(wsgi_request, 'user', None) + if not session_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 @@ -312,101 +318,65 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request): set_custom_attribute('jwt_auth_request_user_not_found', True) return False - 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: + if not session_user.is_authenticated or not session_user.username or session_user.username == jwt_username: return False - # .. 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 + # .. 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 # 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_lms_user_id', session_lms_user_id) + 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) return True - def _get_and_monitor_jwt_cookie_lms_user_id(self, request): + def _get_unsafe_jwt_cookie_username_and_lms_user_id(self, request): """ - Returns the LMS user id from the JWT cookie, or None if not found - - Notes: - - Also provides monitoring details for mismatches. + 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. + try: cookie_token = JSONWebTokenAuthentication.get_token_from_cookies(request.COOKIES) - 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 + unsafe_decoded_jwt = unsafe_jwt_decode_handler(cookie_token) + jwt_username = self._get_username_from_payload(unsafe_decoded_jwt) + 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') except Exception: # pylint: disable=broad-exception-caught + jwt_username = None jwt_lms_user_id = None - 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) + set_custom_attribute('jwt_cookie_unsafe_decode_issue', 'decode-error') - return jwt_lms_user_id + return (jwt_username, jwt_lms_user_id) - def _get_lms_user_id_from_user(self, user): + def _get_username_from_payload(self, payload): """ - Returns the lms_user_id from the user object if found, or None if not found. + Returns the username from the payload. - This is intended for use only by LMS user id matching code, and thus will provide appropriate error - logs in the case of misconfiguration. + WARNING: + 1. This doesn't play well with JSONWebTokenAuthentication.jwt_get_username_from_payload, but + some services do not have JWT_PAYLOAD_GET_USERNAME_HANDLER configured. + 2. It's unclear if `username` is used for any old JWTs, but this could probably be removed. """ - # .. 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 + return payload.get('preferred_username') or payload.get('username') _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 6910fa13..c3f1900b 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py @@ -36,7 +36,6 @@ 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 @@ -235,97 +234,6 @@ 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( @@ -453,9 +361,8 @@ 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_lms_user_id = 111 - session_user = factories.UserFactory(id=session_lms_user_id) - jwt_user = factories.UserFactory(id=222) + session_user = factories.UserFactory(id=111, username='session-name') + jwt_user = factories.UserFactory(id=222, username='jwt-name') self.client.cookies = SimpleCookie({ jwt_cookie_name(): self._get_test_jwt_token(user=jwt_user), }) @@ -464,7 +371,6 @@ 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) @@ -475,12 +381,15 @@ 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_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 # 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) 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 '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 response.status_code == 200 @override_settings( @@ -493,10 +402,8 @@ 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_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) + session_user = factories.UserFactory(id=111, username='session-name') + jwt_user = factories.UserFactory(id=222, username='jwt-name') self.client.cookies = SimpleCookie({ jwt_cookie_name(): self._get_test_jwt_token(user=jwt_user, is_valid_signature=False), }) @@ -505,7 +412,6 @@ 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) @@ -514,16 +420,18 @@ 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_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_auth_mismatch_session_username', session_user.username) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', jwt_user.username) 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 '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 response.status_code == 401 @override_settings( @@ -536,8 +444,7 @@ 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_lms_user_id = 111 - session_user = factories.UserFactory(id=session_lms_user_id) + session_user = factories.UserFactory(id=111, username='session-name') self.client.cookies = SimpleCookie({ jwt_cookie_name(): 'invalid-cookie', }) @@ -546,7 +453,6 @@ 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) @@ -555,16 +461,17 @@ 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_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_auth_mismatch_session_username', session_user.username) + mock_set_custom_attribute.assert_any_call('jwt_auth_mismatch_jwt_cookie_username', None) 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 '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 response.status_code == 401 @override_settings( @@ -586,7 +493,6 @@ 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) @@ -596,11 +502,8 @@ def test_authenticate_jwt_and_session_match(self, mock_set_custom_attribute): '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 '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 response.status_code == 200 @override_settings( @@ -624,8 +527,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_lms_user_id' not in set_custom_attribute_keys - assert 'jwt_auth_get_lms_user_id_status' not in set_custom_attribute_keys + 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 response.status_code == 200 @override_settings( @@ -640,121 +543,17 @@ 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_user_id_property_and_set_request_user(self, mock_set_custom_attribute): + def test_authenticate_mismatch_with_set_request_user(self, mock_set_custom_attribute): """ - Tests failure for JWT cookie when there is a session user mismatch with id property and a request to set user. + Tests failure for JWT cookie when there is a session user mismatch 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. - - 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_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) + session_user = factories.UserFactory(id=111, username='session-name') + jwt_user = factories.UserFactory(id=222, username='jwt-name') 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({ @@ -770,60 +569,11 @@ def test_authenticate_user_id_property_and_set_request_user(self, mock_set_custo # 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_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_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_result', 'user-mismatch-enforced-failure') mock_set_custom_attribute.assert_any_call('jwt_auth_failed', mock.ANY) @@ -867,8 +617,8 @@ def test_authenticate_jwt_and_no_session_and_set_request_user(self, mock_set_cus mock_set_custom_attribute.assert_any_call('skip_jwt_vs_session_check', True) 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 '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 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/auth/jwt/tests/test_decoder.py b/edx_rest_framework_extensions/auth/jwt/tests/test_decoder.py index 0ac9b1d6..9d62fb25 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/test_decoder.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/test_decoder.py @@ -247,7 +247,7 @@ def test_unsafe_success_with_invalid_token(self): # Generate a token using the invalid signing key token = generate_jwt_token(self.payload, invalid_signing_key) decoded_token = unsafe_jwt_decode_handler(token) - assert decoded_token['username'] is not None + assert decoded_token['preferred_username'] is not None def _jwt_decode_handler_with_defaults(token): # pylint: disable=unused-argument diff --git a/edx_rest_framework_extensions/auth/jwt/tests/utils.py b/edx_rest_framework_extensions/auth/jwt/tests/utils.py index 8cd565db..5aa82bbf 100644 --- a/edx_rest_framework_extensions/auth/jwt/tests/utils.py +++ b/edx_rest_framework_extensions/auth/jwt/tests/utils.py @@ -62,6 +62,9 @@ def generate_latest_version_payload(user, scopes=None, filters=None, version=Non def generate_unversioned_payload(user): """ Generate an unversioned valid JWT payload given a user. + + WARNING: This test utility is mocking JWT creation of the identity service (LMS). + - A safer alternative might be to move the LMS's JWT creation code to this library. """ jwt_issuer_data = settings.JWT_AUTH['JWT_ISSUERS'][0] now = int(time()) @@ -69,8 +72,10 @@ def generate_unversioned_payload(user): payload = { 'iss': jwt_issuer_data['ISSUER'], 'aud': jwt_issuer_data['AUDIENCE'], - 'username': user.username, - 'user_id': user.id, # this should be conditionally added based on the scope + 'preferred_username': user.username, # preferred_username is used by Open edX JWTs. + # WARNING: This `user_id` implementation could lead to bugs because `user_id` should be + # conditionally added based on scope, and should not always be available. + 'user_id': user.id, 'email': user.email, 'iat': now, 'exp': now + ttl, diff --git a/edx_rest_framework_extensions/config.py b/edx_rest_framework_extensions/config.py index 6de1d70c..14f7d09e 100644 --- a/edx_rest_framework_extensions/config.py +++ b/edx_rest_framework_extensions/config.py @@ -7,8 +7,7 @@ # .. 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. It is recommended to set VERIFY_LMS_USER_ID_PROPERTY_NAME if possible -# when using this feature. +# requires JwtAuthCookieMiddleware to work. # .. toggle_use_cases: temporary # .. toggle_creation_date: 2019-10-15 # .. toggle_target_removal_date: 2024-12-31 @@ -26,15 +25,3 @@ # .. 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 f56f21fc..57572afa 100644 --- a/edx_rest_framework_extensions/settings.py +++ b/edx_rest_framework_extensions/settings.py @@ -18,7 +18,6 @@ from edx_rest_framework_extensions.config import ( ENABLE_FORGIVING_JWT_COOKIES, ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, - VERIFY_LMS_USER_ID_PROPERTY_NAME, ) @@ -36,7 +35,6 @@ '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, } diff --git a/test_settings.py b/test_settings.py index dc0f0674..da05aa35 100644 --- a/test_settings.py +++ b/test_settings.py @@ -46,6 +46,9 @@ 'JWT_LEEWAY': 1, + # This matches the configuration of all Open edX services. + 'JWT_PAYLOAD_GET_USERNAME_HANDLER': lambda d: d.get('preferred_username'), + 'JWT_SECRET_KEY': 'test-key', 'JWT_PUBLIC_SIGNING_JWK_SET': """