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: unblock edx-drf-extensions upgrade #267

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Conversation

zacharis278
Copy link
Contributor

@zacharis278 zacharis278 commented Apr 2, 2024

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.

@zacharis278 zacharis278 force-pushed the zhancock/drf-ext-upgrade branch from eaaccc9 to 042e5c2 Compare April 2, 2024 14:46
@zacharis278
Copy link
Contributor Author

@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 ENABLE_SET_REQUEST_USER_FOR_JWT_COOKIE off that I might be missing?

Copy link
Member

@alangsto alangsto left a comment

Choose a reason for hiding this comment

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

👍

@robrap
Copy link
Contributor

robrap commented Apr 3, 2024

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.

@MichaelRoytman
Copy link
Member

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?

@zacharis278
Copy link
Contributor Author

zacharis278 commented Apr 9, 2024

@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?

@zacharis278
Copy link
Contributor Author

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?

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.

@robrap
Copy link
Contributor

robrap commented Apr 9, 2024

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?

Maybe explicitly use JwtAuthentication and not SessionAuthentication on the endpoints, and comment that it is intentional, and then SessionAuthentication wouldn't be a backup.

@zacharis278
Copy link
Contributor Author

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?

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

@zacharis278 zacharis278 force-pushed the zhancock/drf-ext-upgrade branch from f8c841d to 881900d Compare April 15, 2024 18:36
@zacharis278 zacharis278 merged commit f47e877 into main Apr 15, 2024
4 of 5 checks passed
@zacharis278 zacharis278 deleted the zhancock/drf-ext-upgrade branch April 15, 2024 19:36
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.

4 participants