feat!: make forgiving JWTs the default (#404)

This is the final step (in this library) of the rollout of forgiving JWTs as a replacement for the USE-JWT-COOKIE header. **BREAKING CHANGE:** Removed ENABLE_FORGIVING_JWT_COOKIES toggle. It is now permanently enabled. - The header USE-JWT-COOKIE was removed because it has been fully replaced by forgiving JWTs. - Removed temporary rollout custom attributes: use_jwt_cookie_requested, jwt_auth_request_user_not_found, and skip_jwt_vs_session_check. See ADR 0002-remove-use-jwt-cookie-header.rst for details.
openedx · Jan 25, 2024 · 9bd04b3 · 9bd04b3
1 parent 8e045d8
commit 9bd04b3
Show file tree

Hide file tree

Showing 11 changed files with 92 additions and 378 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -12,6 +12,16 @@ Change Log
 Unreleased
 ----------
 
+[10.0.0] - 2023-11-30
+---------------------
+
+Removed
+~~~~~~~
+* **BREAKING CHANGE:** Removed ENABLE_FORGIVING_JWT_COOKIES toggle. It is now permanently enabled.
+
+  * The header USE-JWT-COOKIE was removed because it has been fully replaced by forgiving JWTs.
+  * Removed temporary rollout custom attributes: use_jwt_cookie_requested, jwt_auth_request_user_not_found, and skip_jwt_vs_session_check.
+
 [9.1.2] - 2024-01-07
 --------------------
 Updated

diff --git a/docs/decisions/0002-remove-use-jwt-cookie-header.rst b/docs/decisions/0002-remove-use-jwt-cookie-header.rst
@@ -1,5 +1,5 @@
-2. Remove HTTP_USE_JWT_COOKIE Header
-====================================
+2. Replace HTTP_USE_JWT_COOKIE Header
+=====================================
 
 Status
 ------
@@ -21,14 +21,16 @@ Use of this header has several problems:
 Decision
 --------
 
-Replace the `HTTP_USE_JWT_COOKIE` header with forgiving authentication when using JWT cookies. By "forgiving", we mean that JWT authentication would no longer raise exceptions for failed authentication when using JWT cookies, but instead would simply return None.
+Replace the ``HTTP_USE_JWT_COOKIE`` header with forgiving authentication when using JWT cookies. By "forgiving", we mean that JWT authentication would no longer raise exceptions for failed authentication when using JWT cookies, but instead would simply return None.
 
-By returning None from JwtAuthentication, rather than raising an authentication failure, we enable services to move on to other classes, like SessionAuthentication, rather than aborting the authentication process. Failure messages could still be surfaced using `set_custom_metric` for debugging purposes.
+By returning None from JwtAuthentication, rather than raising an authentication failure, we enable services to move on to other classes, like SessionAuthentication, rather than aborting the authentication process. Failure messages could still be surfaced using ``set_custom_attribute`` for debugging purposes.
 
-Rather than checking for the `HTTP_USE_JWT_COOKIE`, the `JwtAuthCookieMiddleware`_ would always reconstitute the JWT cookie if the parts were available.
+Rather than checking for the ``HTTP_USE_JWT_COOKIE``, the `JwtAuthCookieMiddleware`_ would always reconstitute the JWT cookie if the parts were available.
 
 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.
 
+**Update:** As of Nov-2023, the ``ENABLE_FORGIVING_JWT_COOKIES`` toggle and ``HTTP_USE_JWT_COOKIE`` have been fully removed, and forgiving JWTs is the default and only implementation remaining.
+
 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:
@@ -47,20 +49,24 @@ Consequences
 
   * For example, local testing of endpoints outside of MFEs will use JWT cookies rather than failing, which has been misleading for engineers.
 
-* Greatly simplifies features like `JwtRedirectToLoginIfUnauthenticatedMiddleware`_.
+* Simplifies features like `JwtRedirectToLoginIfUnauthenticatedMiddleware`_.
 * Service authentication can take advantage of JWT cookies more often.
 * Services can more consistently take advantage of the JWT payload of the JWT cookie.
 * Additional clean-up when retiring the ``HTTP_USE_JWT_COOKIE`` header will be needed:
 
   * ``HTTP_USE_JWT_COOKIE`` should be removed from frontend-platform auth code when ready.
-  * ADR that explains `why the request header HTTP_USE_JWT_COOKIE`_ was created should be updated to point to this ADR.
+  * ADR that explains `why the request header HTTP_USE_JWT_COOKIE`_ was updated in https://github.com/openedx/edx-platform/pull/33680.
 
 .. _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-11-08
+~~~~~~~~~~
+* Updated implementation status, since forgiving JWTs has been rolled out and the ENABLE_FORGIVING_JWT_COOKIES toggle and HTTP_USE_JWT_COOKIE header have been fully removed.
+
 2023-10-30
 ~~~~~~~~~~
 * Details added for handling of a variety of situations when the JWT cookie user and the session user do not match.

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__ = '9.1.2'  # pragma: no cover
+__version__ = '10.0.0'  # pragma: no cover
diff --git a/edx_rest_framework_extensions/auth/jwt/authentication.py b/edx_rest_framework_extensions/auth/jwt/authentication.py
@@ -14,10 +14,7 @@
     configured_jwt_decode_handler,
     unsafe_jwt_decode_handler,
 )
-from edx_rest_framework_extensions.config import (
-    ENABLE_FORGIVING_JWT_COOKIES,
-    ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE,
-)
+from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
 from edx_rest_framework_extensions.settings import get_setting
 
 
@@ -77,13 +74,6 @@ def get_jwt_claim_mergeable_attributes(self):
         return get_setting('JWT_PAYLOAD_MERGEABLE_USER_ATTRIBUTES')
 
     def authenticate(self, request):
-        is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES)
-        # .. custom_attribute_name: is_forgiving_jwt_cookies_enabled
-        # .. custom_attribute_description: This is temporary custom attribute to show
-        #      whether ENABLE_FORGIVING_JWT_COOKIES is toggled on or off.
-        #      See docs/decisions/0002-remove-use-jwt-cookie-header.rst
-        set_custom_attribute('is_forgiving_jwt_cookies_enabled', is_forgiving_jwt_cookies_enabled)
-
         # .. custom_attribute_name: jwt_auth_result
         # .. custom_attribute_description: The result of the JWT authenticate process,
         #      which can having the following values:
@@ -154,14 +144,11 @@ def authenticate(self, request):
             if is_authenticating_with_jwt_cookie:
                 # This check also adds monitoring details
                 is_user_mismatch = self._is_jwt_cookie_and_session_user_mismatch(request)
-                if is_forgiving_jwt_cookies_enabled:
-                    if is_user_mismatch:
-                        set_custom_attribute('jwt_auth_result', 'user-mismatch-failure')
-                        raise
-                    set_custom_attribute('jwt_auth_result', 'forgiven-failure')
-                    return None
-                set_custom_attribute('jwt_auth_result', 'failed-cookie')
-                raise
+                if is_user_mismatch:
+                    set_custom_attribute('jwt_auth_result', 'user-mismatch-failure')
+                    raise
+                set_custom_attribute('jwt_auth_result', 'forgiven-failure')
+                return None
 
             set_custom_attribute('jwt_auth_result', 'failed-auth-header')
             raise
@@ -268,15 +255,9 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request):
             request: The request.
 
         Other notes:
-        - 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_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
-
         jwt_username, jwt_lms_user_id = self._get_unsafe_jwt_cookie_username_and_lms_user_id(request)
 
         # add early monitoring for the JWT LMS user_id for observability for a variety of user cases
@@ -291,11 +272,6 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request):
         # 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
 
         wsgi_request = getattr(request, '_request', request)
@@ -311,11 +287,6 @@ def _is_jwt_cookie_and_session_user_mismatch(self, request):
         # This line taken from DRF SessionAuthentication.
         session_user = getattr(wsgi_request, 'user', None)
         if not session_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
 
         if not session_user.is_authenticated or not session_user.username or session_user.username == jwt_username:

diff --git a/edx_rest_framework_extensions/auth/jwt/constants.py b/edx_rest_framework_extensions/auth/jwt/constants.py
@@ -3,4 +3,3 @@
 """
 
 JWT_DELIMITER = '.'
-USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE'
diff --git a/edx_rest_framework_extensions/auth/jwt/middleware.py b/edx_rest_framework_extensions/auth/jwt/middleware.py
@@ -18,19 +18,13 @@
 from edx_rest_framework_extensions.auth.jwt.authentication import (
     set_flag_is_request_user_set_for_jwt_auth,
 )
-from edx_rest_framework_extensions.auth.jwt.constants import (
-    JWT_DELIMITER,
-    USE_JWT_COOKIE_HEADER,
-)
+from edx_rest_framework_extensions.auth.jwt.constants import JWT_DELIMITER
 from edx_rest_framework_extensions.auth.jwt.cookies import (
     jwt_cookie_header_payload_name,
     jwt_cookie_name,
     jwt_cookie_signature_name,
 )
-from edx_rest_framework_extensions.config import (
-    ENABLE_FORGIVING_JWT_COOKIES,
-    ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE,
-)
+from edx_rest_framework_extensions.config import ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
 from edx_rest_framework_extensions.permissions import (
     LoginRedirectIfUnauthenticated,
     NotJwtRestrictedApplication,
@@ -106,9 +100,6 @@ class JwtRedirectToLoginIfUnauthenticatedMiddleware(MiddlewareMixin):
     using the LoginRedirectIfUnauthenticated permission class.
 
     Enables a DRF view to redirect the user to login when they are unauthenticated.
-    It automatically enables JWT-cookie-based authentication by setting the
-    `USE_JWT_COOKIE_HEADER` for endpoints using the LoginRedirectIfUnauthenticated
-    permission.
 
     This can be used to convert a plain Django view using @login_required into a
     DRF APIView, which is useful to enable our DRF JwtAuthentication class.
@@ -139,18 +130,10 @@ def process_view(self, request, view_func, view_args, view_kwargs):  # pylint: d
         """
         Enables Jwt Authentication for endpoints using the LoginRedirectIfUnauthenticated permission class.
         """
+        # Note: Rather than caching here, this could be called directly in process_response based on the request,
+        # which would require using reverse to determine the view.
         self._check_and_cache_login_required_found(view_func)
 
-        # Forgiving JWT cookies is an alternative to the older USE_JWT_COOKIE_HEADER.
-        # If the rollout for forgiving JWT cookies succeeds, we would need to see if
-        #   this middleware could be simplified or replaced by a simpler solution, because
-        #   at least one part of the original solution required this middleware to insert
-        #   the USE_JWT_COOKIE_HEADER.
-        is_forgiving_jwt_cookies_enabled = get_setting(ENABLE_FORGIVING_JWT_COOKIES)
-        if not is_forgiving_jwt_cookies_enabled:
-            if self.is_jwt_auth_enabled_with_login_required(request, view_func):
-                request.META[USE_JWT_COOKIE_HEADER] = 'true'
-
     def process_response(self, request, response):
         """
         Redirects unauthenticated users to login when LoginRedirectIfUnauthenticated permission class was used.
@@ -230,15 +213,6 @@ def process_view(self, request, view_func, view_args, view_kwargs):  # pylint: d
         """
         assert hasattr(request, 'session'), "The Django authentication middleware requires session middleware to be installed. Edit your MIDDLEWARE setting to insert 'django.contrib.sessions.middleware.SessionMiddleware'."  # noqa E501 line too long
 
-        # .. custom_attribute_name: use_jwt_cookie_requested
-        # .. custom_attribute_description: True if USE_JWT_COOKIE_HEADER was found.
-        #   This is a temporary attribute, because this header is being deprecated/removed.
-        set_custom_attribute('use_jwt_cookie_requested', bool(request.META.get(USE_JWT_COOKIE_HEADER)))
-
-        if not get_setting(ENABLE_FORGIVING_JWT_COOKIES):
-            if not request.META.get(USE_JWT_COOKIE_HEADER):
-                return
-
         header_payload_cookie = request.COOKIES.get(jwt_cookie_header_payload_name())
         signature_cookie = request.COOKIES.get(jwt_cookie_signature_name())
 
@@ -275,18 +249,12 @@ def process_view(self, request, view_func, view_args, view_kwargs):  # pylint: d
         # handled through a more traditional AuthenticationMiddleware that handles both JWT cookies and sessions in
         # the future.
         if has_reconstituted_jwt_cookie and get_setting(ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE):
-            if get_setting(ENABLE_FORGIVING_JWT_COOKIES):
-                # Since this call to the user is not made lazily, and has the potential to cause issues, we
-                # ensure it is only used in the case of ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE.
-                if not get_user(request).is_authenticated:
-                    # Similar to django/contrib/auth/middleware.py AuthenticationMiddleware.
-                    set_flag_is_request_user_set_for_jwt_auth()
-                    request.user = SimpleLazyObject(lambda: _get_cached_user_from_jwt(request, view_func))
-            else:
-                # Disabling ENABLE_FORGIVING_JWT_COOKIES will provide the original implementation,
-                # without checking the user outside of the lazy call (and without caching). This
-                # ensures there is no change in behavior until we test ENABLE_FORGIVING_JWT_COOKIES.
-                request.user = SimpleLazyObject(lambda: _get_user_from_jwt_original(request, view_func))
+            # Since this call to the user is not made lazily, and has the potential to cause issues, we
+            # ensure it is only used in the case of ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE.
+            if not get_user(request).is_authenticated:
+                # Similar to django/contrib/auth/middleware.py AuthenticationMiddleware.
+                set_flag_is_request_user_set_for_jwt_auth()
+                request.user = SimpleLazyObject(lambda: _get_cached_user_from_jwt(request, view_func))
 
 
 def _get_module_request_cache():
@@ -307,22 +275,6 @@ def _get_cached_user_from_jwt(request, view_func):
     return _get_module_request_cache()[_JWT_USER_CACHE_KEY]
 
 
-def _get_user_from_jwt_original(request, view_func):
-    """
-    Returns user from JWT authentication using the original implementation.
-
-    Although this implementation is not ideal, it is temporarily being left in place
-    for backward-compatibility when ENABLE_FORGIVING_JWT_COOKIES is disabled.
-    """
-    # The original implementation first returned the result of the authenticated session
-    # user if it were available, which is not really what this method says it will do.
-    user = get_user(request)
-    if user.is_authenticated:
-        return user
-
-    return _get_user_from_jwt(request, view_func)
-
-
 def _get_user_from_jwt(request, view_func):
     """
     Performs JWT Authentication and returns the user, or AnonymousUser if the user is