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
If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any
service other than the identity service(LMS/CMS), where the session's
local service user id would never match the JWT LMS user id when
compared. This has been fixed.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
  renamed to jwt_auth_mismatch_session_lms_user_id to make this more
  clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
  was added to enable choosing the user object property that contains
  the lms_user_id, if one exists. If this is unset, a service will
  simply skip this additional protection.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
  provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the
original Session vs JWT user id check.

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

[9.0.0] - 2023-11-27

Fixed
~~~~~
* **BREAKING CHANGE**: Fixes a bug for any service other than the identity service (LMS/CMS), where the session's local service user id would never match the JWT LMS user id when compared.

* The custom attribute jwt_auth_mismatch_session_lms_user_id was renamed to jwt_auth_mismatch_session_lms_user_id to make this more clear.
* The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME] was added to enable choosing the user object property that contains the lms_user_id, if one exists. If this is unset, a service will simply skip this additional protection.
* The custom attribute jwt_auth_get_lms_user_id_status was added to provide observability into the new functionality.
* The breaking change only affects services with ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the existing Session vs JWT user id check.

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

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__ = '9.0.0' # pragma: no cover
53 changes: 43 additions & 10 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,
VERIFY_LMS_USER_ID_PROPERTY_NAME,
)
from edx_rest_framework_extensions.settings import get_setting

Expand Down Expand Up @@ -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)

Expand All @@ -351,25 +352,57 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
return False

if user.is_authenticated:
session_user_id = user.id
session_lms_user_id = self._get_lms_user_id_from_user(user)
else:
session_user_id = None
session_lms_user_id = None

if not session_user_id or session_user_id == jwt_user_id:
if not session_lms_user_id or session_lms_user_id == jwt_user_id:
return False

# .. custom_attribute_name: jwt_auth_mismatch_session_user_id
# .. custom_attribute_description: The session authentication user id if it
# does not match the JWT cookie user id. If there is no session user,
# or if it matches the JWT cookie user id, this attribute will not be
# included. Session authentication may have completed in middleware
# .. custom_attribute_name: jwt_auth_mismatch_session_lms_user_id
# .. custom_attribute_description: The session authentication LMS user id if it
# does not match the JWT cookie LMS user id. If there is no session user,
# or no LMS user id for the session user, or if it matches the JWT cookie user id,
# this attribute will not be included. Session authentication may have completed in middleware
# before getting to DRF. Although this authentication won't stick,
# because it will be replaced by DRF authentication, we record it,
# because it sometimes does not match the JWT cookie user.
set_custom_attribute('jwt_auth_mismatch_session_user_id', session_user_id)
set_custom_attribute('jwt_auth_mismatch_session_lms_user_id', session_lms_user_id)

return True

def _get_lms_user_id_from_user(self, user):
"""
Returns the lms_user_id from the user object if found, or None if not found.
This is intended for use only by LMS user id matching code, and thus will provide appropriate error
logs in the case of misconfiguration.
"""
# .. custom_attribute_name: jwt_auth_get_lms_user_id_status
# .. custom_attribute_description: This custom attribute is intended to be temporary. It will allow
# us visibility into when and how the LMS user id is being found from the session user, which
# allows us to check the session's LMS user id with the JWT's LMS user id.

lms_user_id_property_name = get_setting(VERIFY_LMS_USER_ID_PROPERTY_NAME)
if not lms_user_id_property_name:
set_custom_attribute('jwt_auth_get_lms_user_id_status', 'not-configured')
return None

if not hasattr(user, lms_user_id_property_name):
logger.error(f'Misconfigured VERIFY_LMS_USER_ID_PROPERTY_NAME. User object has no attribute with name'
f' [{lms_user_id_property_name}]. User id validation will be skipped.')
set_custom_attribute('jwt_auth_get_lms_user_id_status', 'misconfigured')
return None

# If the property is found, but returns None, validation will be skipped with no messaging.
lms_user_id = getattr(user, lms_user_id_property_name, None)
if lms_user_id:
set_custom_attribute('jwt_auth_get_lms_user_id_status', 'id-found')
else: # pragma: no cover
set_custom_attribute('jwt_auth_get_lms_user_id_status', 'id-not-found')

return lms_user_id


_IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY = '_is_request_user_for_jwt_set'

Expand Down
Loading

0 comments on commit a17fc69

Please sign in to comment.