Skip to content

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

Merged
douglashall merged 1 commit into
openedx:masterfrom
weaveeducation:invalid-display-courseware-through-lti-iframe
Mar 4, 2016
Merged

Invalid display courseware through the LTI iframe in IE 10+#11607
douglashall merged 1 commit into
openedx:masterfrom
weaveeducation:invalid-display-courseware-through-lti-iframe

Conversation

@dmitry-viskov

Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor Author

jenkins run bokchoy

Comment thread lms/djangoapps/lti_provider/views.py Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

👍

@douglashall

Copy link
Copy Markdown
Contributor

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

Comment thread cms/envs/common.py
X_FRAME_OPTIONS = 'ALLOW'

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

douglashall pushed a commit that referenced this pull request Mar 4, 2016
…are-through-lti-iframe

Invalid display courseware through the LTI iframe in IE 10+
@douglashall
douglashall merged commit 56ecc66 into openedx:master Mar 4, 2016
@dmitry-viskov
dmitry-viskov deleted the invalid-display-courseware-through-lti-iframe branch May 19, 2017 12:53
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