-
Notifications
You must be signed in to change notification settings - Fork 2
fix: add configuration to enable correct redirect_uri #36
fix: add configuration to enable correct redirect_uri #36
Conversation
Thanks for the pull request, @mariajgrimaldi! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
label: core contributor |
@mariajgrimaldi Thanks for these changes! Let us know when they're ready for review. |
Thanks! This is ready for review :) |
|
||
{% if ENABLE_HTTPS %} | ||
ENABLE_PROXY_FIX = True | ||
FEATURE_FLAGS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As written this will overwrite the FEATURE_FLAGS
from above, but I think this is a big enough change that we either want it on or off since it's not really related to HTTPS. Is there a plugin needed for the proxy fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this without the override and I think it works. So we're good. Now, regarding the https issue, I'm no expert so I'll go with what you suggest. Our setups (docker-compose, k8s) are behind a proxy (caddy, a lb) so we needed the change in both cases. Maybe someone won't. Should we use something like SUPERSET_ENABLE_PROXY_FIX
?
Is there a plugin needed for the proxy fix?
No, this is part of the superset config: https://github.com/apache/superset/blob/master/superset/config.py#L273-L277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably make it a flag, sadly. For instance I think that running tutor dev
doesn't use the caddy proxy. Otherwise looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running tutor dev
the setting ENABLE_HTTPS
should be False
so we're good as it is right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I'll follow these changes instead:
{% if not ENABLE_WEB_PROXY %}
# Caddy is running behind a proxy: Superset needs to handle x-forwarded-* headers
# https://flask.palletsprojects.com/en/latest/deploying/proxy_fix/
ENABLE_PROXY_FIX = True
{% endif %}
From the tutor-cairn plugin
Fix inspired by these changes: overhangio/tutor-cairn@4905653
@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR adds a configuration that enables the service provider (superset) to build the redirect_uri correctly matching the one created on the LMS site (identify provider).
For more information on this solution, please check these superset configurations: https://github.com/apache/superset/blob/master/superset/config.py#L273-L277
This PR solves #30
How to test
In a production environment (using https):
pip install git+https://github.com/eduNEXT/tutor-contrib-superset.git@MJG/redirect-uri-fix
tutor config save
tutor local start -d
Check the
redirect_uri
has the correct schema.References
dpgaspar/Flask-AppBuilder#1666
apache/superset#8117
apache/airflow#17536 (comment)