search

ccnmtl / django-lti-provider

django-lti-provider adds LTI functionality for the Django web framework. This work began as a port of MIT's LTI Flask Sample, which demonstrates a sample LTI provider for the Flask Framework based on the Python LTI library, PyLTI.
Other
47 stars 28 forks source link

request scheme set incorrectly for launch_url in a load balancer scenario #279

Closed wgwz closed 3 years ago

wgwz commented 4 years ago

Hi folks 👋

I just wanted to open a small tracking issue for something I came across today. While working on an LTI that is being deployed behind an AWS ALB, I found that in the rendered /lti/config.xml, I was getting http://mydomain.net/lti/ instead of what I expected https://mydomain.net/lti/".

I mention the ALB because it's relevant to why this was happening. I wanted to use SSL termination at the load balancer. And then have the traffic from the load balancer to the application as http traffic.

Taking a look at the code, I realized that the scheme is getting set by request.scheme.

https://github.com/ccnmtl/django-lti-provider/blob/5e0927054c866ec6b998ca31bd8144bbe12aa5fe/lti_provider/views.py#L30-L32

I was puzzled by how I could get around this. I think all that's is required in this scenario is to make use of the following:

https://docs.djangoproject.com/en/3.1/ref/settings/#secure-proxy-ssl-header

Initially I was considering implement a PR that would allow for you to override the url scheme, by adding a new config item in the LTI_TOOL_CONFIGURATION called use_https_for_launch (or something). But now I would like just propose that we could add some documentation into the README (If the secure-proxy-ssl-header approach does indeed work).

sdreher commented 4 years ago

Hi @wgwz! Thanks for all the contributions, and I'm sorry about the slow response. I'm wondering if the right answer here is to simple use https always. It seems silly to think it might be anything different ever right?

wgwz commented 4 years ago

Hey @sdreher 👋🏻 No worries, I just thought I’d write these up while it was all fresh in my mind. I was looking at this stuff again today and dropped another comment in another issue.

I think what you are doing by setting the value based off of the request is fine, 100%. It’s not all that uncommon to have a situation where your load balancer listens on https and the target nodes underneath are listening on http. It turns out that modifying the secure proxy header setting in Django does resolve the issue for these scenarios.

I think that modifying the readme to include this bit of information is sufficient. I actually think the way you are choosing the scheme is the best option for all scenarios.