Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117

ericandrewmeadows · 2019-08-27T00:34:19Z

SUMMARY

Custom Security Manager appeared to be broken when behind an AWS ALB, but when I dug further, the Werkzeug version was bumped. This caused an invalid configuration for the proxy in that the host was not passed on. Because of this, along with the change of the library path, and the inclusion of the additional library bump in accordance with apache/airflow#5571, I have validated that the regression that prevented CSM from working in v0.33.0rc1 -> v0.34.0rc [1 & 2] has been resolved.

TEST PLAN

I deployed internally the fix, and validated that the port is not being returned in the header for the destination after hitting the root IP of the Superset instance.

ADDITIONAL INFORMATION

Has associated issue:
- Fixes CUSTOM_SECURITY_MANAGER not working in 0.34.0rc1 #8062
- Fixes Investigate broken Custom Security Manager #8110
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

REVIEWERS

setup.py

superset/__init__.py

ericandrewmeadows · 2019-08-27T00:37:24Z

superset/config.py

@@ -111,6 +111,13 @@

 # Extract and use X-Forwarded-For/X-Forwarded-Proto headers?
 ENABLE_PROXY_FIX = False
+PROXY_FIX_CONFIG = {


This was validated to fix the issue with CSM, when ENABLE_PROXY_FIX is set to True.

dpgaspar · 2019-08-27T11:08:08Z

black config.py

FYI Airflow followup fix on: apache/airflow#5563
Docs: https://werkzeug.palletsprojects.com/en/0.15.x/middleware/proxy_fix/?highlight=proxyfix#werkzeug.middleware.proxy_fix.ProxyFix

villebro · 2019-08-27T16:29:04Z

Thanks for putting in the work to fix this @ericandrewmeadows . Once this gets merged I think we can start picking cherries for a 0.34.1.

ericandrewmeadows · 2019-08-27T16:46:12Z

👍 - fair to enforce all on since the side effect is minimal.

An aside: why don't we force line length to 79 max? I know it's an outdated standard set by punch cards...but most linters complain.

ericandrewmeadows · 2019-08-27T16:47:08Z

@villebro - you're welcome. I just happened to come across the Airflow item after I solved it. I updated the docs - thanks again!

ericandrewmeadows · 2019-08-29T20:06:02Z

@dpgaspar / @villebro - what's the process for merging this?

villebro · 2019-08-29T20:26:20Z

LGTM, but I don't feel comfortable merging before @dpgaspar approves this, as he is more knowledgeable on this topic than I am.

ericandrewmeadows · 2019-08-29T21:04:47Z

I figured - yeah - it may have just slipped under his radar.

ericandrewmeadows · 2019-09-03T16:49:27Z

@dpgaspar - can this be merged?

mistercrunch · 2019-09-03T17:19:51Z

Few notes:

do we know how this maps to previous default settings from the previous implementation?
can we add an entry in UPDATING.md to tell people "Changes around a new version of ProxyFix in Werkzeuf, if you use config ENABLE_PROXY_FIX = True, you'll want to review the newly introduced PROXY_FIX_CONFIG config key and make sure it's set as expected for your environment"

ericandrewmeadows · 2019-09-03T17:26:13Z

I first fixed my instance, then @dpgaspar pointed out the defaults from Airflow so I followed that convention. For me, I use the following settings:

ENABLE_PROXY_FIX = True
PROXY_FIX_CONFIG = {
    "x_for": 1,
    "x_proto": 0,
    "x_host": 1,
    "x_port": 0,
    "x_prefix": 0,
}

I'll add that to UPDATING.md

mistercrunch

Otherwise LGTM

Untitled.ipynb

codecov-io · 2019-09-03T18:21:10Z

Codecov Report

Merging #8117 into master will increase coverage by 0.13%.
The diff coverage is 33.33%.

@@            Coverage Diff            @@
##           master   #8117      +/-   ##
=========================================
+ Coverage   66.06%   66.2%   +0.13%     
=========================================
  Files         479     479              
  Lines       22930   22930              
  Branches     2524    2524              
=========================================
+ Hits        15148   15180      +32     
+ Misses       7648    7616      -32     
  Partials      134     134

Impacted Files	Coverage Δ
superset/__init__.py	`75% <0%> (-0.79%)`	⬇️
superset/config.py	`88.7% <100%> (+0.06%)`	⬆️
superset/assets/src/chart/chartAction.js	`49.63% <0%> (ø)`	⬆️
superset/sql_lab.py	`77.51% <0%> (+0.04%)`	⬆️
superset/views/core.py	`71.53% <0%> (+0.27%)`	⬆️
superset/models/core.py	`81.76% <0%> (+0.64%)`	⬆️
superset/db_engine_specs/mysql.py	`92.15% <0%> (+49.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee24539...cc556e6. Read the comment docs.

ericandrewmeadows

@mistercrunch - Should be good to be merged.

…#8117) * Fix to werkzeug proxy; expose additional configuration items * Forced to all x-forwarded configurations ON; black done * added comments related to x_port after testing * Updated UPDATING.md * Removed accidental notebook; added *.ipynb to gitignore * Delete Untitled-checkpoint.ipynb

Fix to werkzeug proxy; expose additional configuration items

4cfa014

pull-request-size bot added the size/S label Aug 27, 2019

ericandrewmeadows commented Aug 27, 2019

View reviewed changes

setup.py Show resolved Hide resolved

ericandrewmeadows commented Aug 27, 2019

View reviewed changes

superset/__init__.py Show resolved Hide resolved

ericandrewmeadows commented Aug 27, 2019

View reviewed changes

ericandrewmeadows changed the title ~~Fix to werkzeug proxy; expose additional configuration items~~ Fix to Werkzeug ProxyFix; expose ProxyFix configuration items Aug 27, 2019

Forced to all x-forwarded configurations ON; black done

57afa0a

added comments related to x_port after testing

51ec31a

villebro added the v0.34 label Aug 31, 2019

Eric Andrew Meadows added 2 commits September 3, 2019 10:32

Updated UPDATING.md

c7d20b8

Merge branch 'master' into eric/fix_to_werkzeug_proxy

954b728

mistercrunch requested changes Sep 3, 2019

View reviewed changes

Untitled.ipynb Outdated Show resolved Hide resolved

Removed accidental notebook; added *.ipynb to gitignore

4273d1f

Delete Untitled-checkpoint.ipynb

cc556e6

ericandrewmeadows commented Sep 4, 2019

View reviewed changes

mistercrunch approved these changes Sep 4, 2019

View reviewed changes

mistercrunch merged commit ae0dc30 into apache:master Sep 4, 2019

maltoze mentioned this pull request Nov 22, 2019

In HTTPS Change of data source not working on Table and Dashboards #8160

Closed

3 tasks

oofnikj mentioned this pull request Sep 9, 2021

Redirecting to http instead of https dpgaspar/Flask-AppBuilder#1666

Closed

This was referenced May 12, 2023

When running on HTTPS the sso doesn't work due to redirect URL openedx-unsupported/tutor-contrib-superset#30

Closed

fix: add configuration to enable correct redirect_uri openedx-unsupported/tutor-contrib-superset#36

Merged

mistercrunch added 🍒 0.34.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117

Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117

ericandrewmeadows commented Aug 27, 2019 •

edited

Loading

ericandrewmeadows Aug 27, 2019

dpgaspar commented Aug 27, 2019 •

edited

Loading

villebro commented Aug 27, 2019

ericandrewmeadows commented Aug 27, 2019

ericandrewmeadows commented Aug 27, 2019

ericandrewmeadows commented Aug 29, 2019

villebro commented Aug 29, 2019

ericandrewmeadows commented Aug 29, 2019

ericandrewmeadows commented Sep 3, 2019

mistercrunch commented Sep 3, 2019

ericandrewmeadows commented Sep 3, 2019

mistercrunch left a comment

codecov-io commented Sep 3, 2019 •

edited

Loading

ericandrewmeadows left a comment •

edited

Loading

Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117

Fix to Werkzeug ProxyFix; expose ProxyFix configuration items #8117

Conversation

ericandrewmeadows commented Aug 27, 2019 • edited Loading

CATEGORY

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

ericandrewmeadows Aug 27, 2019

Choose a reason for hiding this comment

dpgaspar commented Aug 27, 2019 • edited Loading

villebro commented Aug 27, 2019

ericandrewmeadows commented Aug 27, 2019

ericandrewmeadows commented Aug 27, 2019

ericandrewmeadows commented Aug 29, 2019

villebro commented Aug 29, 2019

ericandrewmeadows commented Aug 29, 2019

ericandrewmeadows commented Sep 3, 2019

mistercrunch commented Sep 3, 2019

ericandrewmeadows commented Sep 3, 2019

mistercrunch left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 3, 2019 • edited Loading

Codecov Report

ericandrewmeadows left a comment • edited Loading

Choose a reason for hiding this comment

ericandrewmeadows commented Aug 27, 2019 •

edited

Loading

dpgaspar commented Aug 27, 2019 •

edited

Loading

codecov-io commented Sep 3, 2019 •

edited

Loading

ericandrewmeadows left a comment •

edited

Loading