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

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

Open
7 of 21 tasks
robrap opened this issue Aug 14, 2023 · 6 comments
Open
7 of 21 tasks

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

robrap opened this issue Aug 14, 2023 · 6 comments
Assignees
Labels
depr Proposal for deprecation & removal per OEP-21

Comments

@robrap
Copy link
Contributor

robrap commented Aug 14, 2023

Proposal Date

2023-08-14

Target Ticket Acceptance Date

2023-08-18

Earliest Open edX Named Release Without This Functionality

Quince - 2023-10

Rationale

The USE-JWT-COOKIE header was used by MFE's to inform backends when JWT cookies should be used. It has some complexities as detailed in the following ADR proposing its removal. See docs/decisions/0002-remove-use-jwt-cookie-header.rst in #197 (which hasn't merged as-of this initial ticket write up).

The complexity causes confusion, and we'd like to simplify that.

Removal

This PR introduces the replacement and ability to disable the old behavior:

This ticket details some of the follow-up cleanup work for full removal:

Replacement

The PR #197 also introduces the replacement, which is something called "forgiving JWT cookies", where we accept JWT cookies on all requests (not just those with a special header), but if it fails authentication, we allow the endpoint to try other forms of authentication before giving up.

Deprecation

No response

Migration

No response

Additional Info

No response

Task list

Note: To make life simpler for me (@robrap), I left searches for org openedx and edx together on this ticket.

@github-actions github-actions bot added the depr Proposal for deprecation & removal per OEP-21 label Aug 14, 2023
@robrap robrap moved this from Proposed to Communicated in DEPR: Deprecation & Removal Aug 14, 2023
@robrap
Copy link
Contributor Author

robrap commented Aug 14, 2023

Note: This should move to "Removing" once accepted, since this work is in-progress.

robrap added a commit that referenced this issue Aug 14, 2023
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.

DEPR for Use-JWT-COOKIE header:
#371

Co-authored-by: Feanil Patel <[email protected]>
@dianakhuang dianakhuang moved this from Communicated to Removing in DEPR: Deprecation & Removal Aug 24, 2023
@robrap robrap self-assigned this Oct 19, 2023
robrap added a commit that referenced this issue Nov 20, 2023
Fixes a bug for any service other than the identity service
(LMS/CMS), where the session's local service user id would never
match the JWT LMS user id when compared.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
renamed to jwt_auth_mismatch_session_lms_user_id to make this more
clear.
- The setting EDX_DRF_EXTENSIONS[LMS_USER_ID_PROPERTY_NAME] was added
to enable choosing the user object property that contains the
lms_user_id, if one exists.

Part of DEPR:
#371
robrap added a commit that referenced this issue Nov 21, 2023
If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any
service other than the identity service(LMS/CMS), where the session's
local service user id would never match the JWT LMS user id when
compared. This has been fixed.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
  renamed to jwt_auth_mismatch_session_lms_user_id to make this more
  clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
  was added to enable choosing the user object property that contains
  the lms_user_id, if one exists. If this is unset, a service will
  simply skip this additional protection.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
  provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the
original Session vs JWT user id check.

Part of DEPR:
#371
robrap added a commit that referenced this issue Nov 21, 2023
If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any
service other than the identity service(LMS/CMS), where the session's
local service user id would never match the JWT LMS user id when
compared. This has been fixed.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
  renamed to jwt_auth_mismatch_session_lms_user_id to make this more
  clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
  was added to enable choosing the user object property that contains
  the lms_user_id, if one exists. If this is unset, a service will
  simply skip this additional protection.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
  provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the
original Session vs JWT user id check.

Part of DEPR:
#371
robrap added a commit that referenced this issue Nov 21, 2023
If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any
service other than the identity service(LMS/CMS), where the session's
local service user id would never match the JWT LMS user id when
compared. This has been fixed.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
  renamed to jwt_auth_mismatch_session_lms_user_id to make this more
  clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
  was added to enable choosing the user object property that contains
  the lms_user_id, if one exists. If this is unset, a service will
  simply skip this additional protection.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
  provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set in order to provide the
original Session vs JWT user id check.

Part of DEPR:
#371
robrap added a commit that referenced this issue Nov 22, 2023
When VERIFY_LMS_USER_ID_PROPERTY_NAME is None, uses lms_user_id
as a default (only if found). This is to ease deployment. Also
uses special value 'skip-check' to disable, in case of emergency.

See updated commit message below:
---------------------------------

Fixes a bug for any service other than the identity service
(LMS/CMS), where the session's local service user id would never
match the JWT LMS user id when compared.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
 renamed to jwt_auth_mismatch_session_lms_user_id to make this more
 clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
 was added to enable choosing the user object property that contains
 the LMS user id, if one exists. If this is set to None (the
 default), the check will use the lms_user_id property if it is
 found, and otherwise will skip this additional protection. In case
 of an unforeseen issue, use 'skip-check' to skip the check, even
 when there is an lms_user_id property.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
 provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set appropriately in order to
provide the existing Session vs JWT user id check. Note that only
LMS/CMS will likely need to set this value.

Part of DEPR:
#371
robrap added a commit that referenced this issue Nov 27, 2023
If ENABLE_FORGIVING_JWT_COOKIES was enabled, there was a bug for any
service other than the identity service(LMS/CMS), where the session's
local service user id would never match the JWT LMS user id when
compared. This has been fixed.

- The custom attribute jwt_auth_mismatch_session_lms_user_id was
 renamed to jwt_auth_mismatch_session_lms_user_id to make this more
 clear.
- The setting EDX_DRF_EXTENSIONS[VERIFY_LMS_USER_ID_PROPERTY_NAME]
 was added to enable choosing the user object property that contains
 the LMS user id, if one exists. If this is set to None (the
 default), the check will use the lms_user_id property if it is
 found, and otherwise will skip this additional protection. In case
 of an unforeseen issue, use 'skip-check' to skip the check, even
 when there is an lms_user_id property.
- The custom attribute jwt_auth_get_lms_user_id_status was added to
 provide observability into the new functionality.

BREAKING CHANGE: The breaking change only affects services with
ENABLE_FORGIVING_JWT_COOKIES enabled. It now requires the new setting
VERIFY_LMS_USER_ID_PROPERTY_NAME to be set appropriately in order to
provide the existing Session vs JWT user id check. Note that only
LMS/CMS will likely need to set this value, because the default is meant
to work in all other services.

Part of DEPR:
#371
@robrap
Copy link
Contributor Author

robrap commented Jan 25, 2024

A main part of this is complete. I updated the task list in the PR description regarding the long tail.

@dianakhuang dianakhuang moved this to In Progress in Arch-BOM Jan 26, 2024
@dianakhuang dianakhuang moved this from In Progress to Done in Arch-BOM Jan 26, 2024
@robrap robrap removed the status in Arch-BOM Jan 26, 2024
MichaelRoytman added a commit to edx/edx-exams that referenced this issue Jan 31, 2024
This commit removes references to the edx-drf-extensions USE-JWT-COOKIE constant. This constant was removed in version 10.0.0 of the edx-drf-extensions library. This constant is no longer necessary, because JWT authentication will now be attempted on all requests. This removes the need to set this constant explicitly inthe presence of a JWT cookie. This also removes the need to force an early return for lti endpoints, because edx-drf-extensions will fallback to other forms of authentication in the event that JWT authentication fails.

Please see the associated DEPR issue in the edx-drf-extensions library for more information: openedx/edx-drf-extensions#371.
@robrap
Copy link
Contributor Author

robrap commented Mar 9, 2024

Update: I'm working on the next task, trying to get services upgraded. Unfortunately this uncovered an issue that arose in one service (2U-specific), and we've ticketed and will look into this. Private ticket link.

@jristau1984
Copy link
Contributor

@robrap the internal ticket has been moved to Done.

@robrap
Copy link
Contributor Author

robrap commented Aug 28, 2024

Update: This ticket is mostly unblocked and the task list in this ticket has been updated to reflect next steps.

robrap added a commit to openedx/edx-platform that referenced this issue Aug 28, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/credentials that referenced this issue Aug 28, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to edx/edx-exams that referenced this issue Aug 28, 2024
his repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/enterprise-catalog that referenced this issue Aug 28, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to edx/configuration that referenced this issue Aug 29, 2024
Although we may no longer need the USE-JWT-COOKIE
header, it could break ecommerce if this were
removed at this time. So, we are leaving
a comment so we'll see this in any searches, and
avoid updating for now.

Once all backends, including ecommerce, have edx-drf-extensions>=10.2.0, this could be removed.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/edx-platform that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to edx/configuration that referenced this issue Aug 29, 2024
Although we may no longer need the USE-JWT-COOKIE
header, it could break ecommerce if this were
removed at this time. So, we are leaving
a comment so we'll see this in any searches, and
avoid updating for now.

Once all backends, including ecommerce, have edx-drf-extensions>=10.2.0, this could be removed.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/enterprise-catalog that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to edx/edx-exams that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/credentials that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This is final clean-up for this repo.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
@robrap
Copy link
Contributor Author

robrap commented Aug 29, 2024

UPDATE: After learning that you cannot drop use-jet-cookie from CORS_ALLOW_HEADERS in any backend before all the frontends stop sending this header, the rest of this removal got more complicated. The PR description task list has been updated appropriately. I'm not sure when and if this work will proceed.

robrap added a commit to openedx/edx-platform that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This removes all uses of the header, except updating
CORS_ALLOW_HEADERS, which can't be done before all
MFEs and other callers stop sending the header.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/enterprise-catalog that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This removes all uses of the header, except updating
CORS_ALLOW_HEADERS, which can't be done before all
MFEs and other callers stop sending the header.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/enterprise-catalog that referenced this issue Aug 29, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This removes all uses of the header, except updating
CORS_ALLOW_HEADERS, which can't be done before all
MFEs and other callers stop sending the header.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
robrap added a commit to openedx/edx-platform that referenced this issue Aug 30, 2024
This repo is no longer using USE-JWT-COOKIE header,
since it has the required edx-drf-extensions>10.2.0,
where it was fully removed.

This removes all uses of the header, except updating
CORS_ALLOW_HEADERS, which can't be done before all
MFEs and other callers stop sending the header.

See "[DEPR]: USE-JWT-COOKIE header" for more details:
- openedx/edx-drf-extensions#371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depr Proposal for deprecation & removal per OEP-21
Projects
Status: Removing
Development

No branches or pull requests

2 participants