Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authorize according to API key (if given) over cookies #3877

Merged
merged 7 commits into from
Jun 12, 2019

Conversation

rauchy
Copy link
Contributor

@rauchy rauchy commented Jun 4, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

Related Tickets & Documents

Fixes #3770

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@rauchy rauchy requested a review from arikfr June 4, 2019 10:04
@@ -41,6 +41,10 @@ def sign(key, path, expires):

@login_manager.user_loader
def load_user(user_id_with_identity):
user = api_key_load_user_from_request(request)
if user:
return user
Copy link
Member

Choose a reason for hiding this comment

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

This feels almost as too simple 😆

Copy link
Contributor Author

@rauchy rauchy Jun 4, 2019

Choose a reason for hiding this comment

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

I've battled flask_login for like 8 hours, trying to get it to cooperate with user_loader and request_loader until this hit me 🤦‍♂️

@arikfr
Copy link
Member

arikfr commented Jun 5, 2019

login_user doesn't have any side effects like setting a cookie or anything like this, right?

@rauchy
Copy link
Contributor Author

rauchy commented Jun 6, 2019

@arikfr I CURLed this around, and there are no Set-Cookie headers in the response when using an apikey querystring parameter, so I believe we're safe.

@arikfr arikfr merged commit 2af8b39 into master Jun 12, 2019
@arikfr arikfr deleted the favor-api-key-auth branch June 12, 2019 08:45
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
* remove legacy session identifier support

* remove redundant test

* redirect to login to support any invalid session identifiers

* be more specific with caught errors

* use authorization according to api_key (if provided) over session
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.

API Key authentication should take precedence over cookies
2 participants