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 authentication method for the token endpoint #107

Merged
merged 1 commit into from
Mar 3, 2019

Conversation

RomanMinkin
Copy link

@RomanMinkin RomanMinkin commented Feb 22, 2019

Hi there,

This PR adds ability to set authentication method for the token endpoint for OAuth 2.0.

Reason
There is no way to set authentication method for the token endpoint.

Context

  1. Per RFC 6749 not recommended to send credentials in the request-body:

Including the client credentials in the request-body using the two
parameters is NOT RECOMMENDED and SHOULD be limited to clients unable
to directly utilize the HTTP Basic authentication scheme

  1. Per RFC 7591 there are 3 supported auth method none (is omitted in the PR since it's only for public clients), client_secret_post and client_secret_basic . With client_secret_basic as a default method

If unspecified or omitted, the default is "client_secret_basic"

  1. node-oauth has similar issuer unresolved since Jan 17, 2014 Pass Basic Authorization header to OAuth2 access token request ciaranj/node-oauth#316 , which is a dependency for passport-oauth2, so guess the internet is pretty broken ^^

I was trying to follow general code style at my best ;)

@simov
Copy link
Owner

simov commented Feb 24, 2019

Thanks @RomanMinkin, I was aware of this issue.

Such option was definitely needed. Also your excerpts from the spec are spot on!

However, there is an issue as well.

First of all it's a bit too late for me to change that default behavior without major release, or without re-testing each and every of the officially supported providers manually.

Second, the list of providers that was in lib/flow/oauth2, was there to document which providers do not support credentials posted in the request body.

I'm obviously not using the recommended defaults too, but these are the only 5 providers out of 180 that I've tested that do not support that.

As for the client, these days it doesn't really matter whether you post the credentials in the request body or not, since every connection is an HTTPS one, and Basic auth doesn't give you any additional protection anyway.

I want to include your contribution, but as it stands right now I'll have to remove most of the proposed changes from it.

I think you only need to add:

if (/ebay|fitbit2|homeaway|hootsuite|reddit/.test(provider.name)
  || provider.token_endpoint_auth_method === 'client_secret_basic'
)

in lib/flow/oauth2, and then in lib/config you can default to client_secret_post which won't be used for now. Then you'll have to fix a few tests back, and also revert the change in config/oauth.

If you make that change feel free to amend and force push, or rebase it to include only the last commit. After that I can review it again and probably make a few small changes to my liking 👍

@RomanMinkin
Copy link
Author

RomanMinkin commented Feb 25, 2019

@simov totally makes sense, I'll address you comments.

@RomanMinkin
Copy link
Author

RomanMinkin commented Feb 25, 2019

@simov, pushed new version, please check it out. This time it's much leaner, let me know if i need to fix anything else ;)

@simov
Copy link
Owner

simov commented Feb 28, 2019

Thanks @RomanMinkin it's looking great! I'll merge it shortly, probably during the weekend.

@simov simov merged commit 43c4fc4 into simov:master Mar 3, 2019
@simov
Copy link
Owner

simov commented Mar 3, 2019

Published in version 4.5.0

I made a few small changes, but I can get back to this PR at some point and revisit some of the details.

Thanks for the help!

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