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

feat: handle jwt cookie vs session user mismatch #388

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Oct 5, 2023

Description:

Adds toggle
EDX_DRF_EXTENSIONS[ENABLE_JWT_VS_SESSION_USER_MONITORING]
to enable the following features:

  • New custom attributes is_jwt_vs_session_user_check_enabled,
    jwt_auth_session_user_id, jwt_auth_and_session_user_mismatch,
    and invalid_jwt_cookie_user_id for monitoring and debugging.
  • When forgiving JWT cookies are also enabled, user mismatches
    will now result in a failure, rather than a forgiving JWT.

This implements #381 (comment).

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 October 5, 2023 04:23
@robrap robrap force-pushed the robrap/jwt-vs-session-monitoring branch 4 times, most recently from 66b4b86 to b7a5229 Compare October 5, 2023 05:21
Adds toggle
EDX_DRF_EXTENSIONS[ENABLE_JWT_VS_SESSION_USER_MONITORING]
to enable the following features:

- New custom attributes is_jwt_vs_session_user_check_enabled,
 jwt_auth_session_user_id, jwt_auth_and_session_user_mismatch,
 and invalid_jwt_cookie_user_id for monitoring and debugging.
- When forgiving JWT cookies are also enabled, user mismatches
 will now result in a failure, rather than a forgiving JWT.
@robrap robrap force-pushed the robrap/jwt-vs-session-monitoring branch from b7a5229 to d41ac06 Compare October 5, 2023 09:12
@robrap robrap changed the title feat: add monitoring for jwt vs session user feat: handle jwt cookie vs session user mismatch Oct 5, 2023
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Generally looks good, I had a couple of questions about error handling.

edx_rest_framework_extensions/auth/jwt/authentication.py Outdated Show resolved Hide resolved
edx_rest_framework_extensions/auth/jwt/authentication.py Outdated Show resolved Hide resolved
edx_rest_framework_extensions/auth/jwt/authentication.py Outdated Show resolved Hide resolved
edx_rest_framework_extensions/auth/jwt/authentication.py Outdated Show resolved Hide resolved
- add separate methods to find mismatch with successful and
 failed JWT, to make differences more clear.
- add additional values for ``failed_jwt_cookie_user_id`` in
 the case that the user_id is not found, or the JWT can't be
 decoded.
- typo fix.
@robrap
Copy link
Contributor Author

robrap commented Oct 5, 2023

@feanil: This is ready for re-review.

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

New error checking looks great, thanks for making the update. This looks good to me to land.

@robrap robrap merged commit e3ac799 into master Oct 10, 2023
7 checks passed
@robrap robrap deleted the robrap/jwt-vs-session-monitoring branch October 10, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants