feat: update jwt vs session user monitoring

1. An issue was found when both ENABLE_JWT_VS_SESSION_USER_CHECK and ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE were enabled at the same time in an ecommerce stage environment for edx.org, that is not reproducible locally. Additionally, the issue killed the requests without providing errors, so potentially caused an infinite loop or some such issue. The guess is that the code for setting the user behind ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, and the code to get the user in the new ENABLE_JWT_VS_SESSION_USER_CHECK check, were somehow in conflict. This update skips the JWT vs Session user check in the case that we have set the user based on the JWT, in which case it would be a JWT vs JWT check, which not only is unnecessary, but also may be the cause of the issue. 2. Also adds custom attributes - set_user_from_jwt_status - skip_jwt_vs_session_check 3. Additionally, enforces JWT and session user matching when the toggle ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE is enabled, because otherwise the user in the middleware might not match the user during JWT authentication. 4. Since the JWT cookie vs session user check is an intimate part of how ENABLE_FORGIVING_JWT_COOKIES should function appropriately, this removes ENABLE_JWT_VS_SESSION_USER_CHECK as a toggle and these checks will be performed whenever ENABLE_FORGIVING_JWT_COOKIES is enabled. This makes the ENABLE_FORGIVING_JWT_COOKIES toggle more safe, because it can no longer be enabled without this required functionality. Note that the earlier tests did not cover all combinations of ENABLE_FORGIVING_JWT_COOKIES and ENABLE_JWT_VS_SESSION_USER_CHECK being disabled and enabled, and the updated tests better cover both cases for the remaining toggle. 5. Lastly, the ADR was updated to explain that various cases regarding handling of a JWT cookie user and session user mismatch.
openedx · Oct 31, 2023 · cb84bbd · cb84bbd
1 parent 1b1700b
commit cb84bbd
Show file tree

Hide file tree

Showing 9 changed files with 412 additions and 133 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -12,6 +12,25 @@ Change Log
 Unreleased
 ----------
 
+[8.13.0] - 2023-10-30
+---------------------
+
+Fixed
+~~~~~
+* Bug fix for when both ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE and the JWT cookie user vs session user check behind ENABLE_FORGIVING_JWT_COOKIES were enabled at the same time.
+
+Added
+~~~~~
+* Added custom attributes set_user_from_jwt_status and skip_jwt_vs_session_check.
+
+Updated
+~~~~~~~
+* ADR for removing HTTP_USE_JWT_COOKIE, which explains forgiven JWT cookies, was updated to explain the cases where the JWT cookie user and session user do not match.
+
+Removed
+~~~~~~~
+* Toggle EDX_DRF_EXTENSIONS[ENABLE_JWT_VS_SESSION_USER_CHECK] has been removed. This check is now a default part of the ENABLE_FORGIVING_JWT_COOKIES functionality. ENABLE_JWT_VS_SESSION_USER_CHECK was just a temporary roll-out toggle that was already proven out everywhere ENABLE_FORGIVING_JWT_COOKIES was already enabled.
+
 [8.12.0] - 2023-10-16
 ---------------------
 

diff --git a/docs/decisions/0002-remove-use-jwt-cookie-header.rst b/docs/decisions/0002-remove-use-jwt-cookie-header.rst
@@ -29,6 +29,15 @@ Rather than checking for the `HTTP_USE_JWT_COOKIE`, the `JwtAuthCookieMiddleware
 
 The proposal includes protecting all changes with a temporary rollout feature toggle ``ENABLE_FORGIVING_JWT_COOKIES``. This can be used to ensure no harm is done for each service before cleaning up the old header.
 
+Unfortunately, there are certain rare cases where the user inside the JWT and the session user do not match:
+
+- If the JWT cookie succeeds authentication, and:
+
+    - If ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE is enabled to make the JWT user available to middleware, then we also enforce the that the JWT user and session user match. If they do not, we will fail authentication instead.
+    - If ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE is disabled, we allow the successful JWT cookie authentication to proceed, even though the session user does not match. We will monitor this situation and may choose to enforce the match and fail instead.
+
+- If the JWT cookie fails authentication, but the failed JWT contains a user that does not match the session user, authentication will be failed, rather than moving on to SessionAuthentication which would have resulted in authentication for a different user.
+
 .. _JwtAuthCookieMiddleware: https://github.com/edx/edx-drf-extensions/blob/270cf521a72b506d7df595c4c479c7ca232b4bec/edx_rest_framework_extensions/auth/jwt/middleware.py#L164
 
 Consequences
@@ -48,3 +57,14 @@ Consequences
 
 .. _why the request header HTTP_USE_JWT_COOKIE: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/oauth_dispatch/docs/decisions/0009-jwt-in-session-cookie.rst#login---cookie---api
 .. _JwtRedirectToLoginIfUnauthenticatedMiddleware: https://github.com/edx/edx-drf-extensions/blob/270cf521a72b506d7df595c4c479c7ca232b4bec/edx_rest_framework_extensions/auth/jwt/middleware.py#L87
+
+Change History
+--------------
+
+2023-10-30
+~~~~~~~~~~
+* Details added for handling of a variety of situations when the JWT cookie user and the session user do not match.
+
+2023-08-14
+~~~~~~~~~~
+* Merged original ADR
diff --git a/edx_rest_framework_extensions/__init__.py b/edx_rest_framework_extensions/__init__.py
@@ -1,3 +1,3 @@
 """ edx Django REST Framework extensions. """
 
-__version__ = '8.12.0'  # pragma: no cover
+__version__ = '8.13.0'  # pragma: no cover
diff --git a/edx_rest_framework_extensions/auth/jwt/authentication.py b/edx_rest_framework_extensions/auth/jwt/authentication.py
@@ -4,6 +4,7 @@
 
 from django.contrib.auth import get_user_model
 from django.middleware.csrf import CsrfViewMiddleware
+from edx_django_utils.cache import RequestCache
 from edx_django_utils.monitoring import set_custom_attribute
 from jwt import exceptions as jwt_exceptions
 from rest_framework import exceptions
@@ -15,14 +16,24 @@
 )
 from edx_rest_framework_extensions.config import (
     ENABLE_FORGIVING_JWT_COOKIES,
-    ENABLE_JWT_VS_SESSION_USER_CHECK,
+    ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE,
 )
 from edx_rest_framework_extensions.settings import get_setting
 
 
 logger = logging.getLogger(__name__)
 
 
+class JwtAuthenticationError(Exception):
+    """
+    Custom base class for all exceptions
+    """
+
+
+class JwtSessionUserMismatchError(JwtAuthenticationError):
+    pass
+
+
 class CSRFCheck(CsrfViewMiddleware):
     def _reject(self, request, reason):
         # Return the failure reason instead of an HttpResponse
@@ -87,6 +98,11 @@ def authenticate(self, request):
         #          other authentication classes from attempting authentication.
         #        'failed-cookie': JWT cookie authentication failed. This prevents other
         #          authentication classes from attempting authentication.
+        #        'user-mismatch-failure': JWT vs session user mismatch found for what would have been
+        #          a forgiven-failure, but instead, the JWT failure will be final.
+        #        'user-mismatch-enforced-failure': JWT vs session user mismatch found for what would
+        #          have been a successful JWT authentication, but we are enforcing a match, and thus
+        #          we fail authentication.
 
         is_authenticating_with_jwt_cookie = self.is_authenticating_with_jwt_cookie(request)
         try:
@@ -105,18 +121,29 @@ def authenticate(self, request):
             self.enforce_csrf(request)
 
             # CSRF passed validation with authenticated user
+
+            # adds additional monitoring for mismatches; and raises errors in certain cases
+            self._monitor_or_enforce_successful_jwt_cookie_and_session_user_mismatch(
+                request, jwt_user_id=user_and_auth[0].id
+            )
             set_custom_attribute('jwt_auth_result', 'success-cookie')
-            # adds additional monitoring for mismatches
-            self._monitor_successful_jwt_cookie_and_session_user_mismatch(request, jwt_user_id=user_and_auth[0].id)
             return user_and_auth
 
-        except Exception as exception:
-            # Errors in production do not need to be logged (as they may be noisy),
-            # but debug logging can help quickly resolve issues during development.
-            logger.debug('Failed JWT Authentication,', exc_info=exception)
+        except JwtSessionUserMismatchError as exception:
+            # Warn against these errors because JWT vs session user should not be happening.
+            logger.warning('Failed JWT Authentication due to session user mismatch.')
             # .. custom_attribute_name: jwt_auth_failed
             # .. custom_attribute_description: Includes a summary of the JWT failure exception
             #       for debugging.
+            set_custom_attribute('jwt_auth_failed', 'Exception:{}'.format(repr(exception)))
+            set_custom_attribute('jwt_auth_result', 'user-mismatch-enforced-failure')
+            raise
+
+        except Exception as exception:
+            # Errors in production do not need to be logged (as they may be noisy),
+            # but debug logging can help quickly resolve issues during development.
+            logger.debug('Failed JWT Authentication.', exc_info=exception)
+
             exception_to_report = _deepest_jwt_exception(exception)
             set_custom_attribute('jwt_auth_failed', 'Exception:{}'.format(repr(exception_to_report)))
 
@@ -256,15 +283,23 @@ def _is_failed_jwt_cookie_and_session_user_mismatch(self, request):
 
         return self._is_jwt_cookie_and_session_user_mismatch(request, jwt_user_id)
 
-    def _monitor_successful_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
+    def _monitor_or_enforce_successful_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
         """
-        Provides monitoring when a successful JWT cookie and session user do not match.
+        Provides monitoring and possible enforcement when a successful JWT cookie and session user do not match.
 
         Notes:
         - Must only be called in the case of a successful JWT cookie.
         - Also provides monitoring details for mismatches.
+        - In the case where ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE is being used, we trigger a failure for
+            what otherwise would have been a successful authentication.
         """
-        self._is_jwt_cookie_and_session_user_mismatch(request, jwt_user_id)
+        is_mismatch = self._is_jwt_cookie_and_session_user_mismatch(request, jwt_user_id)
+        if is_mismatch and get_setting(ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE):
+            # For failed_jwt_cookie_user_id docs, see custom attribute annotations elsewhere
+            set_custom_attribute('failed_jwt_cookie_user_id', jwt_user_id)
+            raise JwtSessionUserMismatchError(
+                'Failing otherwise successful JWT authentication due to session user mismatch with set request user.'
+            )
 
     def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
         """
@@ -275,32 +310,39 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
             jwt_user_id (int): The user_id of the JWT, None if not found.
 
         Other notes:
-        - If ENABLE_JWT_VS_SESSION_USER_CHECK is toggled off, always return False.
+        - If ENABLE_FORGIVING_JWT_COOKIES is toggled off, always return False.
         - Also adds monitoring details for mismatches.
         - Should only be called for JWT cookies.
         """
-        is_jwt_vs_session_user_check_enabled = get_setting(ENABLE_JWT_VS_SESSION_USER_CHECK)
-        # .. custom_attribute_name: is_jwt_vs_session_user_check_enabled
-        # .. custom_attribute_description: This is temporary custom attribute to show
-        #      whether ENABLE_JWT_VS_SESSION_USER_CHECK is toggled on or off.
-        set_custom_attribute('is_jwt_vs_session_user_check_enabled', is_jwt_vs_session_user_check_enabled)
-        if not is_jwt_vs_session_user_check_enabled:
+        is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES)
+        # This toggle provides a temporary safety valve for rollout.
+        if not is_forgiving_jwt_cookies_enabled:
             return False
 
-        has_request_user = (
-            hasattr(request, '_request') and hasattr(request._request, 'user')  # pylint: disable=protected-access
-        )
-        if not has_request_user:  # pragma: no cover
+        # If we set the request user in middleware for JWT auth, then we'd actually be checking JWT vs JWT user id.
+        # Additionally, somehow the setting of request.user and the retrieving of request.user below causes some
+        # unknown issue in production-like environments, and this allows us to skip that case.
+        if _is_request_user_set_for_jwt_auth():
+            # .. custom_attribute_name: skip_jwt_vs_session_check
+            # .. custom_attribute_description: This is temporary custom attribute to show that we skipped the check.
+            #      This is probably redundant with the custom attribute set_user_from_jwt_status, but temporarily
+            #      adding during initial rollout.
+            set_custom_attribute('skip_jwt_vs_session_check', True)
+            return False
+
+        # Get the session-based user from the underlying HttpRequest object.
+        # This line taken from DRF SessionAuthentication.
+        user = getattr(request._request, 'user', None)  # pylint: disable=protected-access
+        if not user:  # pragma: no cover
             # .. custom_attribute_name: jwt_auth_request_user_not_found
             # .. custom_attribute_description: This custom attribute shows when a
             #      session user was not found during JWT cookie authentication. This
             #      attribute will not exist if the session user is found.
             set_custom_attribute('jwt_auth_request_user_not_found', True)
             return False
 
-        wsgi_request_user = request._request.user  # pylint: disable=protected-access
-        if wsgi_request_user and wsgi_request_user.is_authenticated:
-            session_user_id = wsgi_request_user.id
+        if user.is_authenticated:
+            session_user_id = user.id
         else:
             session_user_id = None
 
@@ -320,6 +362,19 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request, jwt_user_id):
         return True
 
 
+_IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY = '_is_request_user_for_jwt_set'
+
+
+def set_flag_is_request_user_set_for_jwt_auth():
+    """
+    Sets a flag that the shows the request user was set to be based on JWT auth.
+
+    Used to coordinate between middleware and JwtAuthentication. Note that the flag
+    is stored in this module to avoid circular dependencies.
+    """
+    _get_module_request_cache()[_IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY] = True
+
+
 def is_jwt_authenticated(request):
     successful_authenticator = getattr(request, 'successful_authenticator', None)
     if not isinstance(successful_authenticator, JSONWebTokenAuthentication):
@@ -364,3 +419,16 @@ def _deepest_jwt_exception(exception):
             relevant_exception = cur_exception
 
     return relevant_exception
+
+
+def _get_module_request_cache():
+    return RequestCache(__name__).data
+
+
+def _is_request_user_set_for_jwt_auth():
+    """
+    Returns whether the request user was set to be based on JWT auth in JwtAuthCookieMiddleware.
+
+    This is a public method to enable coordination with the JwtAuthentication class.
+    """
+    return _get_module_request_cache().get(_IS_REQUEST_USER_SET_FOR_JWT_AUTH_CACHE_KEY, False)