-
Notifications
You must be signed in to change notification settings - Fork 16
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: add check for enforcing jwt and lms user email match #419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Minor issue with the custom attribute. I know you plan to add tests. Thanks and good luck.
29154d4
to
c74c3e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Thanks.
- I'm still reviewing tests.
- The changelog and version need to be updated.
- The base branch will be updated over the next couple of days. Sorry about that. :)
I will update the changelog and version once my branch is rebased on the master. If it's okay. |
c82ad02
to
7d931d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py
Outdated
Show resolved
Hide resolved
edx_rest_framework_extensions/auth/jwt/tests/test_authentication.py
Outdated
Show resolved
Hide resolved
19fc77c
to
c15bd24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work.
- My change was merged, so you can rebase against master.
- I just looked at my final review comments and marked resolved. I did not re-review all the tests, etc.
- I give this a 👍, except it is difficult to review until you rebase. Feel free to have someone on your team do the final review after rebase (mostly to make sure the rebase looks good), or I can do that if you need me to.
- Remember to add CHANGELOG and version update for 10.1.0.
fe968a4
to
5709f13
Compare
Thank you for merging your changes. I have rebased the branch against the master and added CHANGELOG and version updates. If possible, could you please review now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor request to update the changelog. Thank you.
This is the final step (in this library) of the rollout of forgiving JWTs as a replacement for the USE-JWT-COOKIE header. **BREAKING CHANGE:** Removed ENABLE_FORGIVING_JWT_COOKIES toggle. It is now permanently enabled. - The header USE-JWT-COOKIE was removed because it has been fully replaced by forgiving JWTs. - Removed temporary rollout custom attributes: use_jwt_cookie_requested, jwt_auth_request_user_not_found, and skip_jwt_vs_session_check. See ADR 0002-remove-use-jwt-cookie-header.rst for details.
5709f13
to
ae2968d
Compare
Description:
Unauthenticate in case of mismatch between JWT email and lms user email
JIRA:
VAN-1694