fix!: bug in JWT vs session user id compare (#408)

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 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.

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 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, because the default is meant
to work in all other services.

Part of DEPR:
#371

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
 ---------------------
 

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

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'