-
Notifications
You must be signed in to change notification settings - Fork 16
MCKIN-22396 Fix samesite cookie issue for Scorm Shell login flow #1876
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.
👍
- I tested this: matches tested upstream code for the same purpose. Checked that the samesite attribute it set correctly.
- I read through the code
- [na] I checked for accessibility issues
- [na] Includes documentation
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.
please cherry pick commits form upstream instead of adding new commits. It would make our life easier in upcoming rebases.
@@ -73,6 +73,7 @@ django-babel-underscore==0.5.2 | |||
django-babel==0.6.2 # via django-babel-underscore | |||
django-classy-tags==0.8.0 # via django-sekizai | |||
django-config-models==0.2.2 | |||
django-cookies-samesite==0.6.6 |
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.
I noticed openedx is on 0.5.1
, not sure why are we on 0.6.6
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.
@ihtram I don't see a reason to pin this to 0.5.1 just because openedx has done so.
The newer version is still compatible but has a better logic for detecting compatible/incompatible browsers. Where 0.5.1 does a rough regex match 0.6.6 has a better mechanism that could work for more browsers in a more consistent way.
In fact if it's acceptable we should update the version for upstream ironwood as well.
Following edx team's request, opened another PR #1879 including cherry-picked commits from upstream, yet the same code. |
Ticket: https://edx-wiki.atlassian.net/browse/MCKIN-22396
In Scorm shell login flow, the platform is loaded in an iframe from a cross origin ancestor. Hence we need the
sessionid
andcsrftoken
cookies to have samesite='None' and secure=True (after chrome update).This has also been done for edx/edx-platform here