-
Notifications
You must be signed in to change notification settings - Fork 2
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
LTI not working on Safari #56
Comments
Can you give more information about what is not working? We just tried on Safari and it looks fine. |
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. |
I just tried it and I get some errors in the Safari console:
I'll see if I can get the corresponding log from Heroku. |
@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) |
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. |
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 @alvinsiu have you ever encountered something like this? |
So, to provide a bit more detail, a wildcard ( 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 |
Can we specify multiple domains, i.e. |
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. |
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) |
@blarghmatey what do you think? |
That looks like it does what we need. |
Adding the CORS library doesn't seem to have fixed this issue. Steps to reproduce:
Expected behavior: the LTI displays in an iFrame Here's the Heroku logs when Safari makes a request in this case:
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 ?) |
I have not test in IE at all. |
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. |
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. |
It works on Chrome and Firefox, but not Safari.
The text was updated successfully, but these errors were encountered: