-
Notifications
You must be signed in to change notification settings - Fork 3
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: unblock edx-drf-extensions upgrade #267
Conversation
eaaccc9
to
042e5c2
Compare
@robrap I think you're the expert on the JWT auth changes. Does this assessment of this problem sound correct to you? Any consequences of |
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.
👍
DRF does not set the user until middleware completes. This toggle makes the JWT user available to middleware. However, if a session user is found, it is used in place of the JWT user. The reason that a user mismatch between the JWT and the session results in this new error, is because the middleware would have used the session user, but the rest of the request would have used the JWT user, which was found to be a different user, and this seems a recipe for trouble. When this toggle is disabled, the mismatched user is treated as a warning only, although it still may be possible that the session user was used? Although it’s only a warning, it’s still a confusing and bad state, since there is still a session user and a JWT user that don’t agree. If you proceed as if there is no issue, it should use the JWT user. The question is what the expectation of the user is for that request? Does the user think it is working with the session user or the JWT user. Is this a normally occurring case, and is there some way to resolve by actually getting the two users to match? Let me know if this helps clarify the situation. |
I can see that we leverage sessions to authenticate users in the core LTI implementation. If we still use session authentication in the core LTI implementation, will this request in the incorrect user information being pulled from the session data during launch, because the old session is still there? |
@robrap thanks that is helpful. I'm not sure if this setting was ever included intentionally, it might just have been part of pulling config from somewhere else? We do always want to use to JWT user in cases where the session might mismatch. It sounds to me like that puts us in 'this should work but it's not ideal' territory. To be more certain about behavior would it make sense to write a middleware here or elsewhere that does enforce the JWT user is used when it does not match the session? |
I don't think that would be an issue because when we initialize the launch we login a new session. So long as that request is correctly using the JWT a mismatch would get fixed at that point. |
Maybe explicitly use JwtAuthentication and not SessionAuthentication on the endpoints, and comment that it is intentional, and then SessionAuthentication wouldn't be a backup. |
I believe all of our endpoints do explicitly use JwtAuthentication so I think we're good there |
f8c841d
to
881900d
Compare
JIRA: COSMO-215
Description:
After pulling in the v10 release of edx-drf-extensions we noticed some course staff receiving authentication errors when testing LTI based proctored exams. This was caused by users swapping between two accounts to test the student versus instructor experience with exams.
The core implementation of LTI (shared here and in edx-platform) uses the django session rather than a JWT to authenticate callbacks/redirects from the tool during the launch flow. Because exams is an IDA, we produce a edx_exams specific session cookie that is not cleared when the user switches to a new account since the logout action only cleans up LMS cookies. When the user logs in with a different account that session cookie no longer matches the JWT and produces an error. Prior to v10 this was allowed and the mismatched session was simply overridden the next time and LTI flow is kicked off.
Removing
ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE
gets us back to where we were before v10 at the cost of having no fix for jpadilla/django-rest-framework-jwt#45. But, we don't have any middleware that requires a request user at this time.Long term we could maybe rewrite the auth for LTI to use the JWT. I think it was only built that way originally since JWT auth required an edx specific header that we could not force tools to use.
Hard to test this locally since we no longer have access to the 1EdTech testing tools. I plan to pause this in stage and make sure everything actually works as expected before releasing.