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

Invalid display courseware through the LTI iframe in IE 10+ #11607

Conversation

dmitry-viskov
Copy link
Contributor

When using in-frame LTI navigation in IE 10 & 11, problems are not rendering properly. The problems render properly in IE 10 & 11 when using edX directly, or when opening LTI in a new tab. This is reproducible in Canvas and D2L:
ie_bug1
This problem happens because in iframe JS couldn't get Cookie for the CSRF protection and sends always null:
ie_bug2
So per every such question LTI server returns HTTP 403:

"POST /courses/course-v1:OrgX+CS101+qwerty1/xblock/block-v1:OrgX+CS101+qwerty1+type@problem+block@5932e9abc0224b04af13c048a174553f/handler/xmodule_handler/problem_get HTTP/1.0" 200 3847

instead of:

"POST /courses/course-v1:OrgX+CS101+qwerty1/xblock/block-v1:OrgX+CS101+qwerty1+type@problem+block@5932e9abc0224b04af13c048a174553f/handler/xmodule_handler/problem_get HTTP/1.0" 403 9976

It is connected with the cookie-restricting privacy feature called P3P. More details can be found here: http://blogs.msdn.com/b/ieinternals/archive/2013/09/17/simple-introduction-to-p3p-cookie-blocking-frame.aspx
I'v created a little fix. You could see it in this pull request. I know only one case of using iframe to display courseware so I'm not sure that this fix should be applied only for the LTI views. May be it should be used globally for all other entry points.
Display questions after the fix:
ie_bug3

@openedx-webhooks
Copy link

Thanks for the pull request, @strannikk! I've created OSPR-1153 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Feb 19, 2016
@dmitry-viskov
Copy link
Contributor Author

jenkins run bokchoy

# this header should be used to save CSRF cookies in IE 10+ browser
# in case of display courseware through the iframe
# http://blogs.msdn.com/b/ieinternals/archive/2013/09/17/simple-introduction-to-p3p-cookie-blocking-frame.aspx
resp['P3P'] = 'CP="IDC DSP COR ADM DEVi TAIi PSA PSD IVAi IVDi CONi HIS OUR IND CNT"'
Copy link
Contributor

Choose a reason for hiding this comment

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

@strannikk How did you come to define this P3P policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@douglashall What do you mean? If you asking about the P3P value I've taken it from the blog article where this problem was described. This issue is very popular and could be found in many internet articles (for example on stackoverflow).

Copy link
Contributor

Choose a reason for hiding this comment

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

@strannikk Sorry, I am not entirely familiar with P3P and I am wondering if there is a good resource which describes each of the policy values you have listed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@strannikk Can you move the addition of this header to a Python decorator and apply the decorator to this function? Also, the policy value should be moved to a Django setting in lms/envs/common.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

@strannikk I think we should do something similar to what Facebook and Google are doing for the P3P policy, a plain-text explanation that we do not have a P3P policy. See http://stackoverflow.com/questions/8048306/what-is-the-most-broad-p3p-header-that-will-work-with-ie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@douglashall Not sure. I believe in this case LTI in IE will not work properly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@douglashall Sorry. I've read the solutions on the stackoverflow. Ok. I'll try to use the facebook approach. Let's see how it turns out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @strannikk. I have tried the following locally with a Canvas consumer and it seems to work.

resp['P3P'] = 'CP="Open EdX does not have a P3P policy."'

@openedx-webhooks openedx-webhooks added engineering review waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization engineering review labels Mar 1, 2016
@dmitry-viskov dmitry-viskov force-pushed the invalid-display-courseware-through-lti-iframe branch from c04ff38 to be2df68 Compare March 2, 2016 22:56
@dmitry-viskov
Copy link
Contributor Author

@douglashall I've updated the PR according your comments

…ndering properly.

The problems render properly in IE 10 & 11 when using edX directly, or when opening LTI in a new tab.
This is reproducible in Canvas and D2L
@dmitry-viskov dmitry-viskov force-pushed the invalid-display-courseware-through-lti-iframe branch from be2df68 to ca82f14 Compare March 3, 2016 09:27
@douglashall
Copy link
Contributor

👍

@douglashall
Copy link
Contributor

@robrap Can you give this a second thumb? I have tested this locally and it looks good.

@@ -358,6 +358,9 @@
# Clickjacking protection can be enabled by setting this to 'DENY'
X_FRAME_OPTIONS = 'ALLOW'

# Platform for Privacy Preferences header
P3P_HEADER = 'CP="Open EdX does not have a P3P policy."'
Copy link
Contributor

Choose a reason for hiding this comment

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

@douglashall I think we should tag someone (who?) on legal based on the following blog article.
https://blogs.msdn.microsoft.com/ie/2012/02/20/google-bypassing-user-privacy-settings/

Thoughts?

Other than that, it looks fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW that article was written in 2012 and I think things have evolved since then. From the research I have done, P3P seems to be a dead standard that no one except for IE implements. @smagoun Is there someone in legal who can/should look at this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@douglashall I did a quick search/read on my commute. If what you say is true, I'd say we shouldn't waste much time on this and I would give it a 👍 . What do you think?

@dmitry-viskov
Copy link
Contributor Author

@douglashall Do I need to move decorator to lms/djangoapps/lti_provider/decorators.py?
Seems that the comment with this request was removed. Am I wrong?

@douglashall
Copy link
Contributor

@strannikk Yes, I removed that comment after thinking about it some more. I think the location you chose is the right one.

FYI, we need to run the P3P policy by our legal team here. More to come soon.

Thanks again for raising this issue and putting in the fix.

@douglashall
Copy link
Contributor

Microsoft has dropped support for P3P on all versions of IE on Windows 10.

https://msdn.microsoft.com/en-us/library/mt146424(v=vs.85).aspx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants