Skip to content

Commit

Permalink
fix: bug in JWT vs session user id compare
Browse files Browse the repository at this point in the history
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[LMS_USER_ID_PROPERTY_NAME] was added
to enable choosing the user object property that contains the
lms_user_id, if one exists.

Part of DEPR:
#371
  • Loading branch information
robrap committed Nov 20, 2023
1 parent de0ea5f commit 1ac7da0
Show file tree
Hide file tree
Showing 6 changed files with 289 additions and 67 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@ Change Log
Unreleased
----------

[8.13.2] - 2023-11-27

Fixed
~~~~~
* 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[LMS_USER_ID_PROPERTY_NAME] was added to enable choosing the user object property that contains the lms_user_id, if one exists.

[8.13.1] - 2023-11-15
---------------------

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__ = '8.13.1' # pragma: no cover
__version__ = '8.13.2' # pragma: no cover
39 changes: 30 additions & 9 deletions edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from edx_rest_framework_extensions.config import (
ENABLE_FORGIVING_JWT_COOKIES,
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE,
LMS_USER_ID_PROPERTY_NAME,
)
from edx_rest_framework_extensions.settings import get_setting

Expand Down Expand Up @@ -351,25 +352,45 @@ 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.
"""
lms_user_id_property_name = get_setting(LMS_USER_ID_PROPERTY_NAME)
if not lms_user_id_property_name:
return None

if not hasattr(user, lms_user_id_property_name):
logger.error(f'Misconfigured LMS_USER_ID_PROPERTY_NAME. User object has no attribute with name'
f' [{lms_user_id_property_name}]. User id validation will be skipped.')
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)
return lms_user_id


_IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY = '_is_request_user_for_jwt_set'

Expand Down
Loading

0 comments on commit 1ac7da0

Please sign in to comment.