Skip to content

Commit

Permalink
feat!: make forgiving JWTs the default
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
robrap committed Nov 8, 2023
1 parent 11acf2e commit bf5ae92
Show file tree
Hide file tree
Showing 11 changed files with 63 additions and 314 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ Change Log
Unreleased
----------

[9.0.0] - 2023-11-08
--------------------

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.

[8.13.0] - 2023-10-30
---------------------

Expand Down
16 changes: 11 additions & 5 deletions docs/decisions/0002-remove-use-jwt-cookie-header.rst
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
2. Remove HTTP_USE_JWT_COOKIE Header
2. Replace HTTP_USE_JWT_COOKIE Header
====================================

Status
Expand All @@ -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.

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:
Expand All @@ -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.
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.0' # pragma: no cover
__version__ = '9.0.0' # pragma: no cover
43 changes: 7 additions & 36 deletions edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -150,14 +140,11 @@ def authenticate(self, request):
if is_authenticating_with_jwt_cookie:
# This check also adds monitoring details for all failed JWT cookies
is_user_mismatch = self._is_failed_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
Expand Down Expand Up @@ -310,35 +297,19 @@ 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_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

# 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)
if not user:
return False

if user.is_authenticated:
Expand Down
1 change: 0 additions & 1 deletion edx_rest_framework_extensions/auth/jwt/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,3 @@
"""

JWT_DELIMITER = '.'
USE_JWT_COOKIE_HEADER = 'HTTP_USE_JWT_COOKIE'
68 changes: 10 additions & 58 deletions edx_rest_framework_extensions/auth/jwt/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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():
Expand All @@ -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
Expand Down
Loading

0 comments on commit bf5ae92

Please sign in to comment.