fix: use forgiving jwt authentication #197

robrap · 2021-08-13T14:02:49Z

Description:

The idea behind this work is to use "forgiving" jwt authentication
when working with JWT cookies, which means that if jwt cookie
authentication fails, we would try other authentication mechanisms,
rather than failing and halting the authentication attempt.

This solution would replace an earlier solution to issues around
jwt cookies that introduced an HTTP_USE_JWT_COOKIE header.

For more background and details, see the new ADR:
0002-remove-use-jwt-cookie-header.rst.

Also contains:

add custom attributes:
- is_forgiving_jwt_cookies_enabled
- use_jwt_cookie_requested
- has_jwt_cookie
- jwt_auth_result
ENABLE_FORGIVING_JWT_COOKIES toggle for rollout of what would
be a future breaking change, when we have fully switched to
forgiving jwt cookies.

JIRA:

DEPR: [DEPR]: USE-JWT-COOKIE header #371
Legacy ticket: ARCHBOM-1218

Author concerns:

Ecommerce would be the riskiest rollout since it is using all affected Middleware.

Reviewers:

tag reviewer

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPi after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

robrap

Notes for my future self or future reviewers. Also see "Author Concerns" of the PR description.

edx_rest_framework_extensions/auth/jwt/authentication.py

edx_rest_framework_extensions/auth/jwt/middleware.py

edx_rest_framework_extensions/middleware.py

requirements/base.in

robrap · 2021-08-25T13:14:34Z

Closing this draft PR with comments for ARCHBOM-1218 until the ticket is picked up again.

feanil · 2023-05-09T15:36:53Z

Re-opening this because I think it's still relevant once we update it a bit. I'll take a stab at re-basing it off of the latest main branch.

edx_rest_framework_extensions/auth/jwt/middleware.py

edx_rest_framework_extensions/config.py

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

feanil

Just a few comments from a more thorough review, next I'm gonna try to get this going locally and see what the test coverage looks like.

edx_rest_framework_extensions/auth/jwt/authentication.py

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

edx_rest_framework_extensions/auth/jwt/middleware.py

edx_rest_framework_extensions/config.py

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

robrap · 2023-07-10T21:51:46Z

@feanil: What's your plan for this?

feanil · 2023-07-11T18:43:53Z

@robrap I had to backburner this a bit to get some other work around documentation moving. I think I'll be able to finish that up this week and then I can come back to this next week.

The idea behind this work is to use "forgiving" jwt authentication when working with JWT cookies, which means that if jwt cookie authentication fails, we would try other authentication mechanisms, rather than failing and halting the authentication attempt. This solution would replace an earlier solution to issues around jwt cookies that introduced an HTTP_USE_JWT_COOKIE header. For more background and details, see the new ADR: 0002-remove-use-jwt-cookie-header.rst. Also contains: * add custom attributes `jwt_auth_has_jwt_cookie` for debugging. * ENABLE_FORGIVING_JWT_COOKIES toggle for rollout of what would be a future breaking change, when we have fully switched to forgiving jwt cookies. ARCHBOM-1218

By default it will be off so the release should be safe to push out and then we can enable as needed to test the new code paths.

We now set multiple attribuets as a part of authentication so change how we validate our mock calls.

Run all the existing tests with both forgiving and original JWT Auth behaviors. Only one test needed to be modified. Previously we were raising an exception when CSRF checks failed within the authentication process but with forgiving auth, we no longer rais an exception so we update the one test to handle both cases for now. This will get cleaned when we hopefully move forward with only having the forgiving JWT auth flow in the future.

This test was missing an assert to ensure that authentication succeeded. However when we add the assert, I learned that the test was failing because the Authorization header was not being submitted correctly into the initial request. I updated the test so that it would pass as expected.

.editorconfig

robrap

Thanks @feanil. I'm going to discuss monitoring with someone on my team late tomorrow, and will come back with a proposal around that. In the meantime, I've noted other things.

Also, when done, please check the Conversations tab to ensure you caught all the comments. Some of my comments are not new, but just pings on old comments. Thanks!

edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py

edx_rest_framework_extensions/config.py

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

.editorconfig

edx_rest_framework_extensions/auth/jwt/authentication.py

edx_rest_framework_extensions/auth/jwt/tests/test_middleware.py

edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py

Co-authored-by: Robert Raposa <[email protected]>

edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py

edx_rest_framework_extensions/config.py

Co-authored-by: Robert Raposa <[email protected]>

edx_rest_framework_extensions/auth/jwt/middleware.py

robrap

@feanil: Confirming that I will be updating the refactor PR and fixing some of the monitoring there, and then we can merge back in to this PR and finish any remaining issues here.

edx_rest_framework_extensions/auth/jwt/authentication.py

edx_rest_framework_extensions/middleware.py

This initial refactor recombines the original and forgiving versions of process_view, and tries to keep as close to the original as possible. Changes to original include: - reconstitute cookie if is_forgiving_jwt_cookies_enabled - move setting request user to the bottom of the method - for forgiving cookies, user is set if jwt cookie is found

Monitoring shows that missing JWT cookies is already a common case, and will be even more common once this is enabled. Therefore, there is no reason to log this as a warning, nor even to log it at all.

- Add new custom attribute use_jwt_cookie_requested - Remove custom attribute request_jwt_cookie Notes: - request_jwt_cookie had a confusing name - the new temporary attribute use_jwt_cookie_requested provides more specific help for the overall removal of this header. - the new attribute jwt_auth_has_jwt_cookie (separate commit) can be used instead to determine if we reconstituted.

Simplify middleware code for reconstituting cookies by simply returning early if USE_JWT_COOKIE_HEADER is not supplied, and we haven't yet enabled ENABLE_FORGIVING_JWT_COOKIES. To accomplish this, one minor change was made that should be fine, but it does affect the USE_JWT_COOKIE_HEADER case. Rather than setting request user for all scenarios where USE_JWT_COOKIE_HEADER is supplied, including when there is no JWT cookie, we only set the request user if the JWT cookie has been reconstituted.

Updates docs for request_auth_type_guess custom attribute using annotations.

Updates docs for jwt_auth_failed custom attribute using annotations.

Made the following custom attribute changes: * replace jwt_auth_has_jwt_cookie (was in JwtAuthentication) with new has_jwt_cookie in JwtAuthCookieMiddleware * refactor and rename jwt_auth_forgiving_jwt_cookies to is_forgiving_jwt_cookies_enabled * annotate is_forgiving_jwt_cookies_enabled * add and annotate jwt_auth_failure_forgiven

Combine _authenticate_original and _authenticate_forgiving_jwt_cookies into one method. Note: The refactor makes a change to the original, where we now use 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.

Configure test settings to enable more complete JwtAuthetication testing with JWT cookies. Note: A new test_authenticate_with_correct_jwt_cookie will be added in a separate commit because it relies on other changes.

* Add jwt_auth_result with various values for the different possible outcomes of JwtAuthentication. * Remove jwt_auth_failure_forgiven custom attribute, which was replaced by jwt_auth_result value 'forgiven-failure'. Configure test settings to enable more complete JwtAuthetication testing with JWT cookies. Note: A new test_authenticate_with_correct_jwt_cookie will be added in a separate commit because it relies on other changes.

robrap

@timmc-edx @feanil: You may want to review the Aug 8th and Aug 10th commits by commit.

Here is a summary of changes:

docs: update annotations for request_auth_type_guess
docs: add annotations for jwt_auth_failed
fixup! forgiving jwt custom attribute updates
- replace jwt_auth_has_jwt_cookie (was in JwtAuthentication) with new has_jwt_cookie in JwtAuthCookieMiddleware
- refactor and rename jwt_auth_forgiving_jwt_cookies to is_forgiving_jwt_cookies_enabled
- annotate is_forgiving_jwt_cookies_enabled
- add and annotate jwt_auth_failure_forgiven
  - Note: This is replaced by jwt_auth_result in a future commit.
fixup! refactor JwtAuthentication authenticate (combined orginal and forgiving_jwt_cookies)
- Note: this was another refactor like the one from the separate PR
feat: enable JWT cookie testing
- adds a test setting used in a follow-up commit
fixup! switch to jwt_auth_result custom attribute
- replaces jwt_auth_failure_forgiven

UPDATE: Also added a changelog entry and bumped the version.

robrap · 2023-08-10T21:47:28Z

edx_rest_framework_extensions/auth/jwt/authentication.py

-            # 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


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.

feanil

One question, otherwise looks good to me.

feanil · 2023-08-14T15:11:07Z

edx_rest_framework_extensions/auth/jwt/authentication.py

@@ -142,9 +139,15 @@ 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)))
+            # .. 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


What's Only JWT cookie failures will be forgiven. meant to communicate? It suggests that maybe JWT auth where you use a JWT Authorization header is not forgiven? Is that right?

@feanil: You are correct. Also, this comment went away when this attribute was removed in favor of jwt_auth_result, so there is no doc updates needed.

robrap · 2023-08-14T15:47:16Z

@feanil @timmc-edx: [inform] I just added a changelog entry as well.

robrap commented Aug 13, 2021

View reviewed changes

robrap mentioned this pull request Aug 13, 2021

ARCHBOM-1218: Proposal: use forgiving jwt authentication with jwt cookies #129

Closed

robrap closed this Aug 25, 2021

robrap deleted the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch August 25, 2021 13:14

feanil restored the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch May 9, 2023 15:36

feanil reopened this May 9, 2023

feanil force-pushed the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch from 1de9346 to bcab347 Compare May 9, 2023 15:38

robrap commented May 9, 2023

View reviewed changes

edx_rest_framework_extensions/auth/jwt/middleware.py Outdated Show resolved Hide resolved

robrap commented May 10, 2023

View reviewed changes

edx_rest_framework_extensions/config.py Outdated Show resolved Hide resolved

robrap commented May 10, 2023

View reviewed changes

edx_rest_framework_extensions/config.py Outdated Show resolved Hide resolved

robrap commented May 10, 2023

View reviewed changes

docs/decisions/0002-remove-use-jwt-cookie-header.rst Outdated Show resolved Hide resolved

feanil reviewed May 17, 2023

View reviewed changes

robrap commented May 17, 2023

View reviewed changes

docs/decisions/0002-remove-use-jwt-cookie-header.rst Show resolved Hide resolved

feanil force-pushed the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch from 35f3baf to b567e94 Compare May 23, 2023 19:07

robrap mentioned this pull request Jul 20, 2023

feat: Add JwtAuthentication as a default DRF auth class. openedx/edx-platform#32802

Merged

robrap and others added 5 commits July 21, 2023 12:51

fix: Add a default value for ENABLE_FORGIVING_JWT_COOKIES.

0dbad93

By default it will be off so the release should be safe to push out and then we can enable as needed to test the new code paths.

test: Update jwt auth tests due to a code change.

e0d9fae

We now set multiple attribuets as a part of authentication so change how we validate our mock calls.

style: Fix various pylint violations.

07ffa5e

style: Update pylintrc and add an editorconfig from edx-lint.

4fce295

feanil force-pushed the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch 2 times, most recently from 02a6bd9 to 88f7879 Compare July 21, 2023 17:00

feanil added 2 commits July 21, 2023 13:04

feanil force-pushed the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch from 88f7879 to 65919e4 Compare July 21, 2023 17:04

robrap commented Jul 21, 2023

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

robrap commented Jul 25, 2023

View reviewed changes

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

50092b4

Co-authored-by: Robert Raposa <[email protected]>

feanil mentioned this pull request Jul 26, 2023

Cleanup ENABLE_FORGIVING_JWT_COOKIES toggle and related code #365

Closed

feanil reviewed Jul 26, 2023

View reviewed changes

edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py Show resolved Hide resolved

edx_rest_framework_extensions/config.py Outdated Show resolved Hide resolved

docs: Apply suggestions from code review

f80ad42

Co-authored-by: Robert Raposa <[email protected]>

timmc-edx reviewed Jul 26, 2023

View reviewed changes

edx_rest_framework_extensions/auth/jwt/middleware.py Outdated Show resolved Hide resolved

robrap commented Jul 26, 2023

View reviewed changes

edx_rest_framework_extensions/auth/jwt/authentication.py Outdated Show resolved Hide resolved

edx_rest_framework_extensions/middleware.py Outdated Show resolved Hide resolved

robrap added 12 commits August 2, 2023 11:42

refactor: simplify conditional code

bd3af42

fix: drop warning for missing cookies

74a45d1

Monitoring shows that missing JWT cookies is already a common case, and will be even more common once this is enabled. Therefore, there is no reason to log this as a warning, nor even to log it at all.

docs: update annotations for request_auth_type_guess

1e9f31a

Updates docs for request_auth_type_guess custom attribute using annotations.

docs: add annotations for jwt_auth_failed

bfd163e

Updates docs for jwt_auth_failed custom attribute using annotations.

feat: enable JWT cookie testing

ef040c6

Configure test settings to enable more complete JwtAuthetication testing with JWT cookies. Note: A new test_authenticate_with_correct_jwt_cookie will be added in a separate commit because it relies on other changes.

fixup! fix quality

b84dd92

robrap commented Aug 10, 2023

View reviewed changes

fixup! fix quality

f85fbf1

robrap mentioned this pull request Aug 14, 2023

[DEPR]: USE-JWT-COOKIE header #371

Open

21 tasks

fixup! switch to new DEPR ticket link

1524623

feanil approved these changes Aug 14, 2023

View reviewed changes

robrap added 2 commits August 14, 2023 11:21

fixup! update changelog and version

8394d2b

fixup! update changelog formatting

81afc8f

robrap merged commit e8c7e0c into master Aug 14, 2023

robrap deleted the robrap/ARCHBOM-1218-drop-use-jwt-cookie-header branch August 14, 2023 18:10

robrap mentioned this pull request Nov 6, 2023

Support DEPR of USE-JWT-COOKIE with forgiving JWTs edx/edx-arch-experiments#429

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use forgiving jwt authentication #197

fix: use forgiving jwt authentication #197

robrap commented Aug 13, 2021 •

edited

Loading

robrap left a comment

robrap commented Aug 25, 2021

feanil commented May 9, 2023

feanil left a comment

robrap commented Jul 10, 2023

feanil commented Jul 11, 2023

robrap left a comment

robrap left a comment

robrap left a comment •

edited

Loading

robrap Aug 10, 2023

feanil left a comment

feanil Aug 14, 2023

robrap Aug 14, 2023

robrap commented Aug 14, 2023

fix: use forgiving jwt authentication #197

fix: use forgiving jwt authentication #197

Conversation

robrap commented Aug 13, 2021 • edited Loading

robrap left a comment

Choose a reason for hiding this comment

robrap commented Aug 25, 2021

feanil commented May 9, 2023

feanil left a comment

Choose a reason for hiding this comment

robrap commented Jul 10, 2023

feanil commented Jul 11, 2023

robrap left a comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

robrap left a comment • edited Loading

Choose a reason for hiding this comment

robrap Aug 10, 2023

Choose a reason for hiding this comment

feanil left a comment

Choose a reason for hiding this comment

feanil Aug 14, 2023

Choose a reason for hiding this comment

robrap Aug 14, 2023

Choose a reason for hiding this comment

robrap commented Aug 14, 2023

robrap commented Aug 13, 2021 •

edited

Loading

robrap left a comment •

edited

Loading