jupyterhub / ltiauthenticator

A JupyterHub authenticator for LTI
https://ltiauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
68 stars 54 forks source link

feat: Allow to manually specify URL scheme #166

Closed martinclaus closed 1 year ago

martinclaus commented 1 year ago

A robust mechanism to determine the protocol seen by the browser is available from jupyterhub >= 2.0.2 (see jupyterhub.utils.get_browser_protocol) . Jupyterhub's implementation preferably checks the standardised Forwarded header and falls back to X-Scheme and X-Forwarded-Proto.

Since we are currently depending on jupterhub >= 1.2.0, I am keeping our own implementation as a fallback.

Note, that I have added the X-Scheme header to the list of headers checked by our implementation.

minrk commented 1 year ago

The get_browser_prototocol is definitely not robust against complex multi-proxy cloud deployments that may use a mixture of several different versions of forwarding headers.

We use it very little in JupyterHub (even less in 4.0, where we dropped it from cross-site checks for causing loads of problems), and I believe the few places it's still used can be overridden by manual config in case it's wrong (usually hardcoding 'https' because every real deployment uses https).

While I think it's fine to use this, I'd probably allow an escape hatch for the cases when this might not detect correctly.

That should probably be a feature on JupyterHub (e.g. c.JupyterHub.public_url), but without it, you might want it here.

martinclaus commented 1 year ago

@minrk Thank you for your feedback and insight! At least, your implementation in jupyterhub.utlils is more robust then the one in this project. I agree that specifying the public url or scheme should be a configuration feature in jupyterhub since inferring it from the request seems impossible in all situations. So I will go for adding a configuration option for the scheme defaulting to https. If jupyterhub adds something like c.JupyterHub.public_url someday, we can easily infere the scheme from its value with minimal changes to the codebase.

martinclaus commented 1 year ago

I have added a configuration option as mentioned above which defaults to auto detection. IMO ready to merge.