Skip to content

Allow generic before_request#3970

Closed
timifasubaa wants to merge 2 commits into
apache:masterfrom
timifasubaa:add_header_level_request_filtering
Closed

Allow generic before_request#3970
timifasubaa wants to merge 2 commits into
apache:masterfrom
timifasubaa:add_header_level_request_filtering

Conversation

@timifasubaa
Copy link
Copy Markdown
Contributor

@timifasubaa timifasubaa commented Dec 1, 2017

This PR allows users specify functions that will be run before every request in their internal config file. These functions tend to be very specific so this approach allows each deployment have filters that don't interfere with other user deployments.

Could help with #3623

@john-bodley @mistercrunch

@timifasubaa timifasubaa force-pushed the add_header_level_request_filtering branch 3 times, most recently from 8f26bdc to cd3415d Compare December 1, 2017 19:44
Copy link
Copy Markdown
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a more generic approach of a flask_app_mutator configuration hook work?

flask_app_mutator would be a configuration function that can be provided in your environment, receives the app object and can alter it in any way.

There's a fair amount of flask related hook earlier and it shows that a more generic one might be the one answer to a family of needs.

Comment thread superset/__init__.py Outdated
Copy link
Copy Markdown
Member

@mistercrunch mistercrunch Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something specific to Airbnb's internal_auth or a certain flavor of proxies? Googling "X-CLIENT-SSL-CERT" doesn't give out much result so that doesn't seem like a standard.

Comment thread setup.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should tax the package with this even though some folks may not need this or even use SSL to run Superset (most likely the proxy/LB is taking care of that)

Comment thread superset/config.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably need to resync your repo as this change has been previously resolved.

@timifasubaa timifasubaa force-pushed the add_header_level_request_filtering branch from cb0cef8 to 6d83570 Compare December 1, 2017 22:51
@timifasubaa timifasubaa force-pushed the add_header_level_request_filtering branch 3 times, most recently from a8c9788 to 48d1375 Compare December 1, 2017 23:05
@timifasubaa timifasubaa force-pushed the add_header_level_request_filtering branch from 48d1375 to 69d5e5e Compare December 1, 2017 23:19
@timifasubaa timifasubaa changed the title Enable request filtering by cert common name Allow generic before_request Dec 1, 2017
@mistercrunch
Copy link
Copy Markdown
Member

From reading your earlier code it seems like your goal was to raise if some SSL cert related condition wasn't met. Now it looks like you'd be swallowing these errors making the hook useless.

I'm curious why you disregarded my suggestion around flask_app_mutator which seem like it would fit your use case as well a many others.

@john-bodley
Copy link
Copy Markdown
Member

@mistercrunch can you be more specific about what you mean by a flask_app_mutator configuration hook? A Google search for the term only returns one result; something you referenced in a prior Superset issue.

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Dec 4, 2017
@mistercrunch
Copy link
Copy Markdown
Member

Here: #3997

@timifasubaa
Copy link
Copy Markdown
Contributor Author

I went ahead with this approach because the other issue where you mentioned flask_app_mutator was also trying to use before_request and this approach solves both problems (once I remove the try catch). But now I see how your even more general solution solves other problems too. We can proceed with #3997 .

mistercrunch added a commit that referenced this pull request Dec 4, 2017
@mistercrunch
Copy link
Copy Markdown
Member

Superseeded by #3997

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mapto mapto mentioned this pull request Mar 22, 2019
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants