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

Switch authentication method precedence #164

Merged
merged 1 commit into from May 5, 2020
Merged

Conversation

@davidkokkelink
Copy link
Member

@davidkokkelink davidkokkelink commented May 5, 2020

This changes the behavior of the auth strategies:

  • the bearer token strategy is evaluated first
  • then, the cookie strategy is evaluated

The only practical difference is that if both methods are passed, the token takes precedence. This does not have any impact for requests that have no Authorization header set, performance or otherwise, as the entire strategy is not executed in that case (which I verified).

This aligns behavior with our prior PHP API and also makes more sense in my opinion, as one is way more likely to accidentally send a cookie than an API token.

@davidkokkelink davidkokkelink requested a review from gka May 5, 2020
@davidkokkelink davidkokkelink requested a review from sto3psl as a code owner May 5, 2020
@davidkokkelink
Copy link
Member Author

@davidkokkelink davidkokkelink commented May 5, 2020

This is particularly necessary because the PHP API always returns a guest session cookie, even when authenticated with an API token. This means that if you make an API call with token to PHP first, and then to Node, and your client saves the cookie (as many do), the second request will fail with a 403

@gka
gka approved these changes May 5, 2020
Copy link
Member

@gka gka left a comment

makes sense!

@davidkokkelink davidkokkelink merged commit 17f86da into master May 5, 2020
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@davidkokkelink davidkokkelink deleted the switch-auth-precedence branch May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.