mitodl / sga-lti

an LTI implementation of Staff Graded Assignments, for use with edX
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

LTI not working on Safari #56

Open sfrucht opened 8 years ago

sfrucht commented 8 years ago

It works on Chrome and Firefox, but not Safari.

alvinsiu commented 8 years ago

Can you give more information about what is not working? We just tried on Safari and it looks fine.

sfrucht commented 8 years ago

Unfortunately, I can't give many other details. When I go to the page I have set up, the page is empty and doesn't load.

screen shot 2016-08-01 at 4 00 11 pm
pdpinch commented 8 years ago

I just tried it and I get some errors in the Safari console:

[Error] Invalid 'X-Frame-Options' header encountered when loading 'https://edge.edx.org/courses/course-v1:MITx+SGA101+2T2016/xblock/block-v1:MITx+SGA101+2T2016+type@lti_consumer+block@9f899e4e3edb478480613f80525f8620/handler/lti_launch_handler': 'ALLOW' is not a recognized directive. The header will be ignored.
[Error] Failed to load resource: the server responded with a status of 403 (Forbidden) (3, line 0)

I'll see if I can get the corresponding log from Heroku.

pdpinch commented 8 years ago

@sfrucht which LTI server are you connecting to? (I've looked at the logs for mitodl-sga-lti-staging and mitodl-sga-lti and neither of them make sense)

sfrucht commented 8 years ago

@pdpinch https://mitodl-sga-lti.herokuapp.com/

pdpinch commented 8 years ago

It's working for me now, but I'm not sure what changed. Is it working for you @sfrucht ?

It may have been a start-up problem. I'll ask @mitodl/devops to set the production version to use a reserved Heroku dyno so it will stop going to sleep.

sfrucht commented 8 years ago

@pdpinch Unfortunately, it's still not working for me. Did you check my course? (You are added as staff)

pdpinch commented 8 years ago

Safari seems to be more strict in its cross-origin restrictions than Firefox or Chrome. If I turn on the developer tools in Safari and select "Disable Cross Origin Restrictions" then the LTI will work.

It seems like the easiest thing to do would be to add a Access-Control-Allow-Origin header to the app, where the value for the header is configurable by environment variable (defaulting to ). To securely deploy our production version, we'd set the header to `.edx.org` or similar.

@alvinsiu have you ever encountered something like this?

blarghmatey commented 8 years ago

So, to provide a bit more detail, a wildcard (*) is only allowed when applying to every domain. If we want to enable a wildcard for every subdomain then that logic would need to be implemented in the app. For instance:

allowed_origin = os.environ.get('ALLOWED_ORIGIN', '*')
if not allowed_origins == '*':
    client_origin = request.headers.get('Origin')
    if client_origin.endswith(ALLOWED_ORIGIN):
        origin = client_origin
response.headers['Access-Control-Allow-Origin'] = allowed_origin
pdpinch commented 8 years ago

Can we specify multiple domains, i.e. edge.edx.org <http://edge.edx.org/>, courses.edx.org ?

blarghmatey commented 8 years ago

Unfortunately the spec isn't very flexible in that regard. It only allows for a single value and it doesn't support subdomains nicely. I've successfully used the strategy highlighted above though. The code I put isn't necessarily 100% correct, but the underlying concept works.

If we wanted to support multiple domains then we can just modify the code to handle validating against a list.

pdpinch commented 8 years ago

@alvinsiu @zags have you guys encountered this before? If so, do you know what the best practice solution is?

zags commented 8 years ago

We haven't done much with allowed origins. The primary security defense on the LTI tool is supposed to be the oauth keys it gives to edX. The best solution would probably be a middleware (something along the lines of this https://pypi.python.org/pypi/django-cors-middleware; I haven't actually used this library before but it looks in the right vein)

pdpinch commented 8 years ago

@blarghmatey what do you think?

blarghmatey commented 8 years ago

That looks like it does what we need.

pdpinch commented 8 years ago

Adding the CORS library doesn't seem to have fixed this issue.

Steps to reproduce:

  1. Clear your browser history in Safari (or open a Private Window)
  2. Navigate to the LTI block (e.g. https://edge.edx.org/courses/MITx/TEST101/2013_Fall/courseware/487dae5b80404631a8b43475f3839cd5/3ed699ea56e344069ea06a2c74f4a125/)

Expected behavior: the LTI displays in an iFrame Actual behavior: the LTI returns a 403 and nothing is displayed at all

Here's the Heroku logs when Safari makes a request in this case:

2016-08-22T17:56:25.073945+00:00 app[web.1]: {address space usage: 483778560 bytes/461MB} {rss usage: 42815488 bytes/40MB} [pid: 12|app: 0|req: 2/32] 10.95.237.20 () {54 vars in 1101 bytes} [Mon Aug 22 17:56:24 2016] POST / => generated 0 bytes in 891 msecs (HTTP/1.1 302) 6 headers in 412 bytes (1 switches on core 0)
2016-08-22T17:56:25.171937+00:00 app[web.1]: Could not find LTI launch parameters
2016-08-22T17:56:25.206426+00:00 app[web.1]: {address space usage: 486416384 bytes/463MB} {rss usage: 43581440 bytes/41MB} [pid: 20|app: 0|req: 7/33] 10.51.167.27 () {50 vars in 1067 bytes} [Mon Aug 22 17:56:25 2016] GET /view-assignment/1/1 => generated 0 bytes in 36 msecs (HTTP/1.1 403) 4 headers in 262 bytes (1 switches on core 0)
2016-08-22T17:56:25.076863+00:00 heroku[router]: at=info method=POST path="/" host=mitodl-sga-lti-staging-pr-63.herokuapp.com request_id=8425aba8-5dca-49ff-bc3a-b3c44338a90a fwd="18.189.18.73" dyno=web.1 connect=0ms service=893ms status=302 bytes=412
2016-08-22T17:56:25.227218+00:00 heroku[router]: at=info method=GET path="/view-assignment/1/1" host=mitodl-sga-lti-staging-pr-63.herokuapp.com request_id=fe5b2275-db82-4b1c-a738-a0782133e258 fwd="18.189.18.73" dyno=web.1 connect=0ms service=37ms status=403 bytes=262

I appears Safari won't pass the LTI parameters, for some reason.

However, if I access the LTI directly (i.e. https://mitodl-sga-lti-staging-pr-63.herokuapp.com) and then reload, Safari passes the LTI parameters, and the contents of the iFrame load without issue.

No other browser we've tested exhibits this behavior.

(Has anyone tried IE 11 or Edge? @sfrucht @indagation @zags @alvinsiu ?)

indagation commented 8 years ago

I have not test in IE at all.

pdpinch commented 8 years ago

Ok, in Safari I feel confident now that it has to do with Cookie settings. The default privacy setting in Safari will not allow sites opened in iframes to set cookies. Because django uses cookies for sessionids, we can't establish a session and the oauth authentication fails.

If the user opens the LTI outside of an iframe, that allows the cookie to be set and everything works from then on. Or the user can change their cookie settings from "Allow from websites I visit" to "Always Allow"

Unfortunately, it doesn't look like there's an easy way to address this in the application. We're probably better off just asking Safari users to work around it themselves (or use another browser). The only suggested fix I've seen so far is this blog post, which suggests testing the cookie and then opening a new window if it fails.

indagation commented 8 years ago

That seems to make sense. I can confirm also that the error, Invalid 'X-Frame-Options' header encountered. Occurs on Chrome as well, but it doesn't cause the LTI to fail.