Skip to content

Commit

Permalink
squash! fix for 9.1.0
Browse files Browse the repository at this point in the history
The fix was to use `preferred_username` in place of `username`.
This also fixes/updates some tests for updated custom attributes.

Squashed commit message below:
------------------------------
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.

Part of edx/edx-arch-experiments#429
  • Loading branch information
robrap committed Jan 5, 2024
1 parent 6684ba6 commit 07115be
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 8 deletions.
13 changes: 13 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@ Change Log
Unreleased
----------

[9.1.2] - 2024-01-05
--------------------
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
Expand Down
2 changes: 1 addition & 1 deletion edx_rest_framework_extensions/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
""" edx Django REST Framework extensions. """

__version__ = '9.1.1' # pragma: no cover
__version__ = '9.1.2' # pragma: no cover
2 changes: 1 addition & 1 deletion edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ def _get_unsafe_jwt_cookie_username_and_lms_user_id(self, request):
try:
cookie_token = JSONWebTokenAuthentication.get_token_from_cookies(request.COOKIES)
unsafe_decoded_jwt = unsafe_jwt_decode_handler(cookie_token)
jwt_username = unsafe_decoded_jwt.get('username', None)
jwt_username = JSONWebTokenAuthentication.jwt_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')

Check warning on line 362 in edx_rest_framework_extensions/auth/jwt/authentication.py

View check run for this annotation

Codecov / codecov/patch

edx_rest_framework_extensions/auth/jwt/authentication.py#L362

Added line #L362 was not covered by tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +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
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(
Expand All @@ -526,7 +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_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(
Expand Down Expand Up @@ -615,7 +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_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):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions edx_rest_framework_extensions/auth/jwt/tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,20 @@ 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())
ttl = 600
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,
Expand Down
3 changes: 3 additions & 0 deletions test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -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': """
Expand Down

0 comments on commit 07115be

Please sign in to comment.