search

laravel / socialite

Laravel wrapper around OAuth 1 & OAuth 2 libraries.
https://laravel.com/docs/socialite
MIT License
5.58k stars 940 forks source link

[5.x] Adding basic authentication by default on signed routes #684

Closed moufmouf closed 10 months ago

moufmouf commented 10 months ago

According to RFC-6749 clients can choose from a number of authentication methods to authenticate with the authorization server.

Section 2.3.1 states that clients can put the credentials either as a Basic authorization header or passing the credentials in the body of the POST.

Right now, the default method for Socialite (in AbstractProvider) is to pass the credentials in the body of the POST.

However, the spec states this:

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 (or other password-based HTTP authentication schemes).

So Socialite passes the credentials using the "non recommended" way.

Furthermore, this way of passing the credentials in NOT supported by all servers. However, the Basic authentication method is mandated to be compulsory per the spec:

The authorization server MUST support the HTTP Basic authentication scheme for authenticating clients that were issued a client password.

As a result, a number of providers need to manually add the Basic authentication as you can see from a simple Github search on the "Providers" Github repository:

https://github.com/search?q=repo%3ASocialiteProviders%2FProviders%20Basic&type=code

It would be better to use by default the only authentication scheme that we know (for sure) is supported by all servers.

This commit adds Basic authentication header to the requests created by the AbstractProvider.

It does not remove the parameters in the body in order to limit breaking changes.

driesvints commented 10 months ago

Could you fix the tests?

moufmouf commented 10 months ago

Could you fix the tests?

Done!

Flightfreak commented 9 months ago

Not disputing it being part of the RFC, but this is a breaking change. Did this belong in a minor?

driesvints commented 9 months ago

@moufmouf I've reverted this again because it seems it's too much of a breaking change for third party providers. We can reconsider for the next major release maybe.

@Flightfreak reverted

moufmouf commented 9 months ago

No problem :+1: Next major is fine.