Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

MCKIN-22396 Fix samesite cookie issue for Scorm Shell login flow #1876

Closed
wants to merge 4 commits into from

Conversation

moeez96
Copy link

@moeez96 moeez96 commented Aug 19, 2020

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 and csrftoken cookies to have samesite='None' and secure=True (after chrome update).

This has also been done for edx/edx-platform here

@moeez96 moeez96 requested a review from xitij2000 August 19, 2020 13:24
@moeez96 moeez96 requested a review from naeem91 August 20, 2020 08:08
@naeem91 naeem91 requested a review from ihtram August 20, 2020 10:46
Copy link

@xitij2000 xitij2000 left a 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

Copy link

@ihtram ihtram left a 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
Copy link

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

Copy link

@xitij2000 xitij2000 Aug 21, 2020

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.

@moeez96
Copy link
Author

moeez96 commented Aug 24, 2020

Following edx team's request, opened another PR #1879 including cherry-picked commits from upstream, yet the same code.

@moeez96 moeez96 closed this Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants