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.

Sign up for GitHub

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

Jump to bottom

fix!: bug in JWT vs session user id compare #408

Merged
merged 2 commits into from
Nov 27, 2023
Merged

fix!: bug in JWT vs session user id compare #408

merged 2 commits into from
Nov 27, 2023

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Nov 20, 2023 •
edited
Loading

Description:

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

Before merge checklist:

  • For 2U: Set VERIFY_LMS_USER_ID_PROPERTY_NAME to 'id' in LMS.

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bump if needed
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

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 robrap requested a review from feanil November 20, 2023 22:45
@robrap robrap force-pushed the robrap/fix-lms-user-id-compare branch from 1ac7da0 to a17fc69 Compare November 21, 2023 17:14
@robrap robrap changed the title fix: bug in JWT vs session user id compare fix!: bug in JWT vs session user id compare Nov 21, 2023
@robrap robrap force-pushed the robrap/fix-lms-user-id-compare branch from a17fc69 to 3b26f05 Compare November 21, 2023 17:43
@robrap
fix!: bug in JWT vs session user id compare
90f1c73 
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 robrap force-pushed the robrap/fix-lms-user-id-compare branch from 3b26f05 to 90f1c73 Compare November 21, 2023 17:54
@robrap
squash! add lms_user_id attribute default
579cc9c 
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
robrap commented Nov 22, 2023
View reviewed changes
edx_rest_framework_extensions/auth/jwt/authentication.py
Comment on lines +393 to +398
# This special value acts like an emergency disable toggle in the event that the user object has an lms_user_id,
# but this LMS id check starts causing unforeseen issues and needs to be disabled.
skip_check_property_name = 'skip-check'
if lms_user_id_property_name == skip_check_property_name:
set_custom_attribute('jwt_auth_get_lms_user_id_status', skip_check_property_name)
return None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Author Note: It was taking me equally long trying to justify we we might not want this, and since I want to quickly get to removing the forgiven JWT toggle, I would have had no kill switch for this if it turns out to cause an issue. So, I decided to add this additional bit of complexity.

@feanil: Please mark as resolved if you have no response, or start a conversation. Thanks.

@robrap robrap merged commit 7998ec5 into master Nov 27, 2023
7 checks passed
@robrap robrap deleted the robrap/fix-lms-user-id-compare branch November 27, 2023 16:02
timmc-edx
timmc-edx reviewed Nov 27, 2023
View reviewed changes
CHANGELOG.rst
~~~~~
* **BREAKING CHANGE**: 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first copy here is supposed to be jwt_auth_mismatch_session_user_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I created #411.

robrap added a commit to openedx/edx-platform that referenced this pull request Nov 27, 2023
@robrap
fix: compatibility change for edx-drf-extensions 9.0.0
18cb734 
edx-drf-extensions 9.0.0 requires VERIFY_LMS_USER_ID_PROPERTY_NAME
to be properly set in LMS to get the appropriate verification when
forgiving JWTs is enabled (which will soon be by default).

See openedx/edx-drf-extensions#408 for details.

This is part of:
edx/edx-arch-experiments#429
robrap added a commit to openedx/edx-platform that referenced this pull request Nov 27, 2023
@github-actions @robrap
feat: Upgrade Python dependency edx-drf-extensions 9.0.0 (#33807)
3b902c8 
Upgrade edx-drf-extensions 9.0.0

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

edx-drf-extensions 9.0.0 requires VERIFY_LMS_USER_ID_PROPERTY_NAME
to be properly set in LMS to get the appropriate verification when
forgiving JWTs is enabled (which will soon be by default).

See openedx/edx-drf-extensions#408 for details.

This is part of:
edx/edx-arch-experiments#429

Co-authored-by: robrap <robrap@users.noreply.github.com>
robrap added a commit to robrap/ecommerce that referenced this pull request Nov 27, 2023
@robrap
feat: upgrade edx-drf-extensions to 9.0.0
618568b 
9.0.0 fixes ENABLE_FORGIVING_JWT_COOKIES bug.
The JWT's LMS user id will now be compared to the
user object's lms_user_id, rather than to its id.
For details, see:
openedx/edx-drf-extensions#408

Note that this won't be put into effect until
ENABLE_FORGIVING_JWT_COOKIES is toggled on
separately.

Also, although this is a major upgrade, it only
caused a backward-incompatible issue in edx-platform.
There are no other changes required for ecommerce.

This is part of the rollout of:
edx/edx-arch-experiments#429
@robrap robrap mentioned this pull request Nov 27, 2023
Merged
1 task
robrap added a commit to robrap/ecommerce that referenced this pull request Nov 28, 2023
@robrap
feat: upgrade edx-drf-extensions to 9.0.0
911ac71 
9.0.0 fixes ENABLE_FORGIVING_JWT_COOKIES bug.
The JWT's LMS user id will now be compared to the
user object's lms_user_id, rather than to its id.
For details, see:
openedx/edx-drf-extensions#408

Note that this won't be put into effect until
ENABLE_FORGIVING_JWT_COOKIES is toggled on
separately.

Also, although this is a major upgrade, it only
caused a backward-incompatible issue in edx-platform.
There are no other changes required for ecommerce.

This is part of the rollout of:
edx/edx-arch-experiments#429
christopappas pushed a commit to openedx-unsupported/ecommerce that referenced this pull request Nov 28, 2023
@robrap
feat: upgrade edx-drf-extensions to 9.0.0 (#4060)
59968f4 
9.0.0 fixes ENABLE_FORGIVING_JWT_COOKIES bug.
The JWT's LMS user id will now be compared to the
user object's lms_user_id, rather than to its id.
For details, see:
openedx/edx-drf-extensions#408

Note that this won't be put into effect until
ENABLE_FORGIVING_JWT_COOKIES is toggled on
separately.

Also, although this is a major upgrade, it only
caused a backward-incompatible issue in edx-platform.
There are no other changes required for ecommerce.

This is part of the rollout of:
edx/edx-arch-experiments#429
christopappas pushed a commit to openedx-unsupported/ecommerce that referenced this pull request Dec 4, 2023
@robrap @christopappas
feat: upgrade edx-drf-extensions to 9.0.0 (#4060)
ef2f253 
9.0.0 fixes ENABLE_FORGIVING_JWT_COOKIES bug.
The JWT's LMS user id will now be compared to the
user object's lms_user_id, rather than to its id.
For details, see:
openedx/edx-drf-extensions#408

Note that this won't be put into effect until
ENABLE_FORGIVING_JWT_COOKIES is toggled on
separately.

Also, although this is a major upgrade, it only
caused a backward-incompatible issue in edx-platform.
There are no other changes required for ecommerce.

This is part of the rollout of:
edx/edx-arch-experiments#429
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timmc-edx timmc-edx timmc-edx left review comments

@feanil feanil feanil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robrap @feanil @timmc-edx