fix: update jwt vs session user monitoring #398
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
robrap
commented
Oct 25, 2023
There was a problem hiding this comment.
@feanil: Here is a first pass that has existing tests passing (locally at least). I did not yet add new tests for the new functionality. I wanted some feedback before either of us invested in that next level of effort. I may do some local ecommerce testing as well. Thanks.
robrap
commented
Oct 25, 2023
feanil
approved these changes
Oct 26, 2023
|
@feanil: This is ready for re-review. You can review the three newest commits one-by-one. I made some minor changes and added tests. Thanks. |
|
@feanil: This is ready for re-re-review. I want to see how many approvals I can get for the same PR. :) Please see the latest 3 commits, and reviewing them separately may make things simpler for you. |
feanil
approved these changes
Oct 31, 2023
1. An issue was found when both ENABLE_JWT_VS_SESSION_USER_CHECK and ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE were enabled at the same time in an ecommerce stage environment for edx.org, that is not reproducible locally. Additionally, the issue killed the requests without providing errors, so potentially caused an infinite loop or some such issue. The guess is that the code for setting the user behind ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, and the code to get the user in the new ENABLE_JWT_VS_SESSION_USER_CHECK check, were somehow in conflict. This update skips the JWT vs Session user check in the case that we have set the user based on the JWT, in which case it would be a JWT vs JWT check, which not only is unnecessary, but also may be the cause of the issue. 2. Also adds custom attributes - set_user_from_jwt_status - skip_jwt_vs_session_check 3. Additionally, enforces JWT and session user matching when the toggle ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE is enabled, because otherwise the user in the middleware might not match the user during JWT authentication. 4. Since the JWT cookie vs session user check is an intimate part of how ENABLE_FORGIVING_JWT_COOKIES should function appropriately, this removes ENABLE_JWT_VS_SESSION_USER_CHECK as a toggle and these checks will be performed whenever ENABLE_FORGIVING_JWT_COOKIES is enabled. This makes the ENABLE_FORGIVING_JWT_COOKIES toggle more safe, because it can no longer be enabled without this required functionality. Note that the earlier tests did not cover all combinations of ENABLE_FORGIVING_JWT_COOKIES and ENABLE_JWT_VS_SESSION_USER_CHECK being disabled and enabled, and the updated tests better cover both cases for the remaining toggle. 5. Lastly, the ADR was updated to explain that various cases regarding handling of a JWT cookie user and session user mismatch.
robrap
force-pushed
the
robrap/fix-jwt-session-monitoring
branch
from
e9d3ad1 to
efa2c4e
Compare
20 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
An issue was found when both ENABLE_JWT_VS_SESSION_USER_CHECK and ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE were enabled at the same time in an ecommerce stage environment for edx.org, that is not reproducible locally. Additionally, the issue killed the requests without providing errors, so potentially caused an infinite loop or some such issue.
The guess is that the code for setting the user behind ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE, and the code to get the user in the new ENABLE_JWT_VS_SESSION_USER_CHECK check, were somehow in conflict. This update attempts to skip the JWT vs Session check in the case that we have set the user based on the JWT, in which case it would be a JWT vs JWT check, which not only is unnecessary, but also may be the cause of the issue.
Adds custom attributes:
Issue:
edx/edx-arch-experiments#429
Merge checklist:
Post merge:
finished.