Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use forgiving jwt authentication #197

Merged
merged 30 commits into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c3621ed
fix: use forgiving jwt authentication
robrap Aug 13, 2021
0dbad93
fix: Add a default value for ENABLE_FORGIVING_JWT_COOKIES.
feanil May 23, 2023
e0d9fae
test: Update jwt auth tests due to a code change.
feanil May 23, 2023
07ffa5e
style: Fix various pylint violations.
feanil May 23, 2023
4fce295
style: Update pylintrc and add an editorconfig from edx-lint.
feanil May 23, 2023
5b54d90
test: Update tests to also test forgiving JWT Auth.
feanil Jul 21, 2023
65919e4
test: Fix a test that didn't run correctly.
feanil Jul 21, 2023
cbe9c2d
fix: Don't always run the original middleware.
feanil Jul 22, 2023
ab7b16d
test: Add testing for forgiving JWT Auth.
feanil Jul 22, 2023
37deb3c
test: Add tests for the redirect middleware.
feanil Jul 24, 2023
f38cb1d
fixup! remove indents
robrap Jul 25, 2023
d0b2c49
fixup! fix typo
robrap Jul 25, 2023
50092b4
docs: Update docs/decisions/0002-remove-use-jwt-cookie-header.rst
Jul 26, 2023
f80ad42
docs: Apply suggestions from code review
Jul 26, 2023
58cd8ab
refactor: recombine original/forgiving process_view
robrap Jul 28, 2023
bd3af42
refactor: simplify conditional code
robrap Jul 28, 2023
74a45d1
fix: drop warning for missing cookies
robrap Jul 28, 2023
a35b32c
feat!: replace custom attribute request_jwt_cookie
robrap Jul 28, 2023
57ad929
refactor: simplify USE_JWT_COOKIE_HEADER case
robrap Jul 28, 2023
1e9f31a
docs: update annotations for request_auth_type_guess
robrap Aug 8, 2023
bfd163e
docs: add annotations for jwt_auth_failed
robrap Aug 8, 2023
b55d33a
fixup! forgiving jwt custom attribute updates
robrap Aug 8, 2023
b982fbd
fixup! refactor JwtAuthentication authenticate
robrap Aug 8, 2023
ef040c6
feat: enable JWT cookie testing
robrap Aug 10, 2023
a130e34
fixup! switch to jwt_auth_result custom attribute
robrap Aug 10, 2023
b84dd92
fixup! fix quality
robrap Aug 10, 2023
f85fbf1
fixup! fix quality
robrap Aug 10, 2023
1524623
fixup! switch to new DEPR ticket link
robrap Aug 14, 2023
8394d2b
fixup! update changelog and version
robrap Aug 14, 2023
81afc8f
fixup! update changelog formatting
robrap Aug 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 10 additions & 53 deletions edx_rest_framework_extensions/auth/jwt/authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,55 +65,9 @@ def authenticate(self, request):
# .. 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)

# TODO: Robert: Refactor back into this single method in a separate commit
if is_forgiving_jwt_cookies_enabled:
return self._authenticate_forgiving_jwt_cookies(request)
return self._authenticate_original(request)

def _authenticate_original(self, request):
try:
user_and_auth = super().authenticate(request)

# Unauthenticated, CSRF validation not required
if not user_and_auth:
return user_and_auth

# Not using JWT cookies, CSRF validation not required
use_jwt_cookie_requested = request.META.get(USE_JWT_COOKIE_HEADER)
if not use_jwt_cookie_requested:
return user_and_auth
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: The refactor makes a change to this original code (not seen in the changed code) where
we now check to see if there is JWT cookie, rather than checking if the USE_JWT_COOKIE header exists, to determine whether to check for CSRF protection.


self.enforce_csrf(request)

# CSRF passed validation with authenticated user
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)
# Useful monitoring for debugging various types of failures.
set_custom_attribute('jwt_auth_failed', 'Exception:{}'.format(repr(exception)))
raise

def _authenticate_forgiving_jwt_cookies(self, request):
"""
Authenticate using a JWT token.

If the JWT is in the authorization header, and authentication fails, we will raise
an exception which will halt additional authentication methods, and is the default
behavior of DRF authentication classes.

If the JWT token is found in the JWT cookie, we've had issues with the cookie sometimes
containing an expired token. For failed authentication, instead of raising an exception,
we will return None which will enable other authentication classes to be attempted where
defined (e.g. SessionAuthentication).

See docs/decisions/0002-remove-use-jwt-cookie-header.rst

"""
has_jwt_cookie = jwt_cookie_name() in request.COOKIES
try:
user_and_auth = super().authenticate(request)
Expand All @@ -139,15 +93,18 @@ def _authenticate_forgiving_jwt_cookies(self, request):
# .. custom_attribute_description: Includes a summary of the JWT failure exception
# 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
# .. custom_attribute_name: jwt_auth_failure_forgiven
# .. custom_attribute_description: This attribute will be True if the JWT failure
# is forgiven. Only JWT cookie failures will be forgiven. In the case of a
# forgiven failure, authenticate will return None rather than raise an
# exception, allowing other authentication classes to process. This attribute
# will be False for failures that are not forgiven.
# is forgiven, and False if it was not forgiven. We will forgive JWT cookie
# failures to handle the case where expired cookies would prevent
# authentication in a case where another authentication class, like
# SessionAuthentication, would have succeeded. In this case we return None
# instead of raising an exception to allow authentication to continue.
# See docs/decisions/0002-remove-use-jwt-cookie-header.rst for details.
set_custom_attribute('jwt_auth_failure_forgiven', has_jwt_cookie)
if has_jwt_cookie:
set_custom_attribute('jwt_auth_failure_forgiven', is_jwt_failure_forgiven)
if is_jwt_failure_forgiven:
return None
raise

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ def test_authenticate_csrf_protected(self, mock_set_custom_attribute):
with self.assertRaises(PermissionDenied) as context_manager:
JwtAuthentication().authenticate(request)
assert context_manager.exception.detail.startswith('CSRF Failed')
mock_set_custom_attribute.assert_any_call('jwt_auth_failure_forgiven', False)

mock_set_custom_attribute.assert_any_call(
'jwt_auth_failed',
Expand Down