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

Add external OAuth case to browser check #396

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

MarvinSchenkel
Copy link
Contributor

@MarvinSchenkel MarvinSchenkel commented May 28, 2023

Hi 👋 ,

I use this project as a dependency for a project where I retrieve an OAuth token myself and set it on the headers to authenticate. Since version 1.0.1 this started failing, because this method is not recognized as either browser_auth or oauth.

This PR extends the browser check to make sure that the Authorization value is an OAuth token in case no Cookie is supplied.

Regards,
Marvin

@sigma67
Copy link
Owner

sigma67 commented May 29, 2023

Just to make sure I'm understanding this correctly. You're using browser auth with an OAuth Bearer token? And in that case you're not passing the cookie attribute?

That seems like a bit of an edge case, although I wouldn't want to prevent it if it works for you.

Either way I'd rather modify the outer scope to prevent breaking is_browser for anyone else. I.e. add another elif after this:

elif is_browser(input_dict):
headers = input_dict

@MarvinSchenkel
Copy link
Contributor Author

Just to make sure I'm understanding this correctly. You're using browser auth with an OAuth Bearer token? And in that case you're not passing the cookie attribute?

That seems like a bit of an edge case, although I wouldn't want to prevent it if it works for you.

Either way I'd rather modify the outer scope to prevent breaking is_browser for anyone else. I.e. add another elif after this:

elif is_browser(input_dict):
headers = input_dict

Yep that's correct. The reason is that I do not want to use the manual CLI approach to obtain an OAuth token, since I want to redirect my users to the login page automatically.

The reason I picked is_browser is because I am actually using a browser. I extended the check and made sure it only passes when a Bearer token is passed when the cookie is omitted. This should not break any existing functionality.

Of course, if you'd rather have a new elif, then I will be happy to adjust my PR.

@sigma67
Copy link
Owner

sigma67 commented Jun 3, 2023

Yes, I'd prefer to create it separate. You could also add another method is_browser_oauth in browser.py, which you then add to the elif in headers.py

@MarvinSchenkel
Copy link
Contributor Author

@sigma67 Fixed! :)

@sigma67 sigma67 merged commit 89c6a25 into sigma67:master Jun 15, 2023
@sigma67 sigma67 mentioned this pull request Dec 17, 2023
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.

2 participants