Skip to content

Commit

Permalink
fix: exceptional case with JwtAuthentication
Browse files Browse the repository at this point in the history
Fixes exceptional case where JwtAuthentication should not CSRF
protect a request that has both a JWT token in the authorization
header and a JWT cookie, since the cookie should be ignored.
  • Loading branch information
robrap committed Aug 31, 2023
1 parent 64c91c0 commit a519606
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 7 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ Change Log
Unreleased
----------

[8.9.2] - 2023-08-31
--------------------

Fixed
~~~~~
* Fixes exceptional case where JwtAuthentication should not CSRF protect a request that has both a JWT token in the authorization header and a JWT cookie, since the cookie should be ignored.

[8.9.1] - 2023-08-22
--------------------

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.9.1' # pragma: no cover
__version__ = '8.9.2' # pragma: no cover
24 changes: 19 additions & 5 deletions edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from rest_framework import exceptions
from rest_framework_jwt.authentication import JSONWebTokenAuthentication

from edx_rest_framework_extensions.auth.jwt.cookies import jwt_cookie_name
from edx_rest_framework_extensions.auth.jwt.decoder import configured_jwt_decode_handler
from edx_rest_framework_extensions.config import ENABLE_FORGIVING_JWT_COOKIES
from edx_rest_framework_extensions.settings import get_setting
Expand Down Expand Up @@ -82,7 +81,7 @@ def authenticate(self, request):
# 'failed-cookie': JWT cookie authentication failed. This prevents other
# authentication classes from attempting authentication.

has_jwt_cookie = jwt_cookie_name() in request.COOKIES
is_authenticating_with_jwt_cookie = self.is_authenticating_with_jwt_cookie(request)
try:
user_and_auth = super().authenticate(request)

Expand All @@ -92,7 +91,7 @@ def authenticate(self, request):
return user_and_auth

# Not using JWT cookie, CSRF validation not required
if not has_jwt_cookie:
if not is_authenticating_with_jwt_cookie:
set_custom_attribute('jwt_auth_result', 'success-auth-header')
return user_and_auth

Expand All @@ -111,11 +110,11 @@ def authenticate(self, request):
# for debugging.
set_custom_attribute('jwt_auth_failed', 'Exception:{}'.format(repr(exception)))

is_jwt_failure_forgiven = is_forgiving_jwt_cookies_enabled and has_jwt_cookie
is_jwt_failure_forgiven = is_forgiving_jwt_cookies_enabled and is_authenticating_with_jwt_cookie
if is_jwt_failure_forgiven:
set_custom_attribute('jwt_auth_result', 'forgiven-failure')
return None
if has_jwt_cookie:
if is_authenticating_with_jwt_cookie:
set_custom_attribute('jwt_auth_result', 'failed-cookie')
else:
set_custom_attribute('jwt_auth_result', 'failed-auth-header')
Expand Down Expand Up @@ -200,6 +199,21 @@ def enforce_csrf(self, request):
# CSRF failed, bail with explicit error message
raise exceptions.PermissionDenied('CSRF Failed: %s' % reason)

@classmethod
def is_authenticating_with_jwt_cookie(cls, request):
"""
Returns True if authenticating with a JWT cookie, and False otherwise.
"""
try:
# If there is a token in the authorization header, it takes precedence in
# get_token_from_request. This ensures that not only is a JWT cookie found,
# but that it was actually used for authentication.
request_token = JSONWebTokenAuthentication.get_token_from_request(request)
cookie_token = JSONWebTokenAuthentication.get_token_from_cookies(request.COOKIES)
return cookie_token and (request_token == cookie_token)
except Exception: # pylint: disable=broad-exception-caught
return False


def is_jwt_authenticated(request):
successful_authenticator = getattr(request, 'successful_authenticator', None)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ def test_get_decoded_jwt_from_auth(self, is_jwt_authentication):
@mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute')
def test_authenticate_with_correct_jwt_authorization(self, mock_set_custom_attribute):
"""
With JWT header it continues and validates the credentials and throws error.
With JWT header it continues and validates the credentials.
Note: CSRF protection should be skipped for this case, with no PermissionDenied.
"""
Expand All @@ -266,6 +266,19 @@ def test_authenticate_with_incorrect_jwt_authorization(self, mock_set_custom_att
JwtAuthentication().authenticate(request)
mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'failed-auth-header')

@mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute')
def test_authenticate_with_correct_jwt_authorization_and_bad_cookie(self, mock_set_custom_attribute):
"""
With JWT header it continues and validates the credentials and ignores the invalid cookie.
Note: CSRF protection should be skipped for this case, with no PermissionDenied.
"""
jwt_token = self._get_test_jwt_token()
request = RequestFactory().get('/', HTTP_AUTHORIZATION=f"JWT {jwt_token}")
request.COOKIES[jwt_cookie_name()] = 'foo'
assert JwtAuthentication().authenticate(request)
mock_set_custom_attribute.assert_any_call('jwt_auth_result', 'success-auth-header')

@mock.patch('edx_rest_framework_extensions.auth.jwt.authentication.set_custom_attribute')
def test_authenticate_with_bearer_token(self, mock_set_custom_attribute):
""" Returns a None for bearer header request. """
Expand Down

0 comments on commit a519606

Please sign in to comment.