squash! add lms_user_id attribute default

When VERIFY_LMS_USER_ID_PROPERTY_NAME is None, uses lms_user_id
as a default (only if found). This is to ease deployment. Also
uses special value 'skip-check' to disable, in case of emergency.

See updated commit message below:
---------------------------------

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.

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.

Part of DEPR:
#371

CHANGELOG.rst

@@ -19,9 +19,9 @@ 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 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 in order to provide the existing Session vs JWT user id check.
+  * 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/auth/jwt/authentication.py

@@ -381,13 +381,30 @@ def _get_lms_user_id_from_user(self, user):
         # .. 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.
+        #      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)
-        if not lms_user_id_property_name:
-            set_custom_attribute('jwt_auth_get_lms_user_id_status', 'not-configured')
+
+        # 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.')

edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py

@@ -227,13 +227,105 @@ def test_authenticate_with_correct_jwt_cookie(self, mock_set_custom_attribute, m
         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
 
+    @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(
@@ -697,13 +789,11 @@ def test_authenticate_user_id_property_and_set_request_user(self, mock_set_custo
     @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 lms_user_id property and a request to set user.
+        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.

edx_rest_framework_extensions/config.py

@@ -28,9 +28,13 @@
 ENABLE_FORGIVING_JWT_COOKIES = 'ENABLE_FORGIVING_JWT_COOKIES'
 
 # .. setting_name: EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
-# .. setting_default: None
-# .. setting_description: This setting should be set to the name of the user attribute property containing the LMS
-#      user id. Examples might be `id` or `lms_user_id`. If there is no property, leave the default value of None.
-#      This is 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.
+# .. 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'