Skip to content

[WIP][Security] OAuth2 component #31952

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

Closed
wants to merge 2 commits into from
Closed

[WIP][Security] OAuth2 component #31952

wants to merge 2 commits into from

Conversation

Guikingone
Copy link
Contributor

@Guikingone Guikingone commented Jun 8, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR To add

Hi everyone 👋

Here's my first "big" PR on Symfony, recently, I've decided to implement OAuth authentication in a side-project application, in order to do it, I've used HttpClient (thanks to Nicolas Grekas) and I've been amazed by the simplicity of the implementation but something was weird ... Why Symfony doesn't have a "native" OAuth support/Client/Server/Component?
Then I saw that the hackaton highlighted the idea of having a native support: #30914

So, I've decided to take time to work on a strict implementation (well, as strict as I could do) thanks to the RFC and HttpClient and here's the first result, a new "OAuth component" (actually, that's an experimentation 😄), for now, this experimentation only support the "client" aspect (the server has been started) and that I'm aware that a lot of feature can be lacking or to update.

So, why opening this PR now ?

First, I'm aware that this is a WIP and that a lot of feedback can be linked to some choices that I've made, please, do not hesitate to explain me if there's a better option/way to do it.

Second, I think that it's a good idea to discuss about this idea and explain why it's not a good idea/option to do it as a component (or even as a feature?) before going further on the development 🙂

So here's the PR with the first parts of the components, feel free to comments and have a great day 🙂

This PR replace the following: #31937

EDIT 13/06/2019:

Here's the actual documentation of this "component":

A set of class which allows to use OAuth thanks to HttpClient and OptionsResolver.

Usage

Using symfony/security-oauth-client outside of the framework is pretty straight forward.

Let's assume that we need to use the Github OAuth API.

First, let's instantiate the provider:

<?php

require __DIR__ . 'vendor/autoload.php';

use Symfony\Components\HttpClient\HttpClient;
use Symfony\Components\OAuth\Provider\AuthorizationCodeProvider;

$client = HttpClient::create();

// The provider requires the main configuration keys
$provider = new AuthorizationCodeProvider($client, [
    'client_id' => 'foo',
    'client_secret' => 'bar',
    'redirect_uri' => 'http://foo.com/',
    'authorization_url' => 'https://github.com/login/oauth/authorize',
    'accessToken_url' => 'https://github.com/login/oauth/access_token',
    'userDetails_url' => 'https://api.github.com/user',
]);

// In the case that a provider cannot fetch an authorization code, a \RuntimeException is thrown
$authenticationCode = $provider->fetchAuthorizationCode([
    'scope' => 'public', 
    'state' => 'randomstring',
]);

// if you need it, you can use the `AuthorizationCodeResponse::get*` methods to access both code and state
// Once the authentication code is fetched, time to receive the access_token

$accessToken = $provider->fetchAccessToken(['code' => $authenticationCode->getCode()]); 

// Now, we can use use the received `AuthorizationCodeGrantAccessToken` object value to fetch the user details
// The $clientProfileLoader is prepared by the GenericProvider
$clientProfileLoader = $provider->prepareClientProfileLoader();

// The $userDate is a `ClientProfile` object
$userData = $clientProfileLoader->fetchClientProfile($accessToken->getTokenValue('access_token'));

echo $userData->get('login'); // Display Bob Foo

Informations

  • The constructor arguments order of GenericProvider can probably be improved.
  • The AuthorizationCodeGrantAccessToken::getOptionValue() is similar to ParameterBag::get().
  • The AuthorizationCodeResponse isn't critical as it's a simple "DTO", we can
    easily return an array.
  • For now, the Symfony\Components\OAuth\Provider\GenericProvider doesn't allow
    to fetch the user details directly (as it's not part of the RFC),
    the final request is up to the user.
  • If an error occurs, no message are returned for now, it could be a good idea
    to return the error key returned by the API.
  • No implementation is available on the full-stack framework for now.
  • The Symfony\Component\OAuth\Provider\ProviderInterface can benefit of
    having the fetchAuthorizationCode and fetchAccessToken methods if a
    contract is created.
  • The \RuntimeException which is thrown during the $provider->fetchAuthenticationCode call (if the provider can't use it)
    can be moved to a trigger_error.

Supported providers

For now, the component deliver the main providers described in the RFC: https://tools.ietf.org/html/rfc6749

Tests

The component isn't fully tested as it's considered as a POC,
the main providers are tested and validated.
The tokens should be fully tested.

Framework integration (WIP)

This part isn't ready to use, it's just an idea

As an application can use multiples providers, it could be a good idea to
allows to use a factory (like HttpClient?):

framework:
    oauth:
        redirect_uri: '%env(REDIRECT_URI)%' # Can be useful in order to share the configuration?
        providers:
            github:
                type: 'authorization_code'
                    client_id: '%env(CLIENT_ID)%'
                    client_secret: '%env(CLIENT_SECRET)%'
                    authorization_url: '%env(API_AUTHORIZATION_URL)%'
                    access_token_url: '%env(API_ACCESS_TOKEN_URL)%'
            google: 
                type: 'authorization_code' # Can be authorization_code, implicit, client_credentials or resource_owner

This way, a newly created service can be accessed via @oauth.github,
time to configure the security.yaml:

security:
    providers:
        github_oauth:
            oauth:
                provider: '@oauth.github' # Or maybe '@oauth.google'? 
    
    firewalls:
        main:
            provider: github_oauth
            

Let's imagine that we need to inject the provider in a service:

<?php

namespace App\Services;

use Symfony\Component\OAuth\Provider\ProviderInterface; // Maybe a contract?

class Foo 
{
    private $oauthGithub;
    
    public function __construct(ProviderInterface $oauthGithub) 
    {
        $this->oauthGithub = $oauthGithub;
    }
    
    // ...
}

Creating a custom provider

As the component allows to use a ProviderInterface, creating a custom provider
is as easy as it sounds:

<?php

use Symfony\Component\OAuth\Provider\ProviderInterface;

class FooProvider implements ProviderInterface 
{
    public function fetchAccessToken(array $options,array $headers = [],string $method = 'GET')
    {
        // ...
    }
    
    // ...
}

Or if needed, we can directly extends the GenericProvider class.

@Guikingone
Copy link
Contributor Author

@Taluu

Here's the default value that I've defined, not sure about it:

->scalarNode('client_id')->defaultValue('12345678')->end()
->scalarNode('client_secret')->defaultValue('12345678')->end()
->scalarNode('authorization_url')->defaultValue('https://foo.com/authenticate')->end()
->scalarNode('redirect_uri')->defaultValue('https://myapp.com/oauth')->end()
->scalarNode('access_token_url')->defaultValue('https://foo.com/token')->end()

Any recommendation? 🤔

@Guikingone
Copy link
Contributor Author

@Koc Configuration keys name fixed 🙂

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 8, 2019
@Taluu
Copy link
Contributor

Taluu commented Jun 8, 2019

My point was to remove these options from the default options. I don't see any case where something could be generic for these options (and merged into the options of the provider), and keep the "required" argument for them in the provider section.

@chalasr
Copy link
Member

chalasr commented Jun 8, 2019

Looks promising. Should be a sub component of the security one IMO (i.e. moved under the Security\ namespace and renamed to security-oauth)

@Guikingone
Copy link
Contributor Author

@chalasr Thanks for the feedback, yes, I totally agree with this idea, as OAuth is totally linked to the security.

Plus, it make no sense at all to use the default framework configuration node as the parent node of OAuth.

@Guikingone
Copy link
Contributor Author

Guikingone commented Jun 8, 2019

@Taluu Oh ok, sorry, I've missed the point the first time 😄

That's right, there's no case where an option can be "shared" with many providers, nice one 👍

@Guikingone
Copy link
Contributor Author

@chalasr About the configuration, this is more a DX question but after thinking about it, here's my idea, what about moving this configuration:

framework:
    oauth:
        redirect_uri: '%env(REDIRECT_URI)%' # Can be useful in order to share the configuration?
        providers:
            github:
                type: 'authorization_code'
                    client_id: '%env(CLIENT_ID)%'
                    client_secret: '%env(CLIENT_SECRET)%'
                    authorization_url: '%env(API_AUTHORIZATION_URL)%'
                    access_token_url: '%env(API_ACCESS_TOKEN_URL)%'
            google: 
                type: 'authorization_code'

Here:

security:
    providers:
        github_oauth:
            oauth:
                type: 'authorization_code'
                client_id: '%env(CLIENT_ID)%'
                client_secret: '%env(CLIENT_SECRET)%'
                authorization_url: '%env(API_AUTHORIZATION_URL)%'
                access_token_url: '%env(API_ACCESS_TOKEN_URL)%'
    
        google_oauth:
            # ... 

    firewalls:
        main:
            provider: github_oauth

I'm not a fan of the first one now as it sounds like the OAuth configuration is linked to the framework one (which is not now as it's been moved as a sub-component), plus, this make the configuration similar to the entity, id, ldap ones 🤔

Any suggestion?

@dunglas
Copy link
Member

dunglas commented Jun 10, 2019

Couldn't we promote @weaverryan's oauth2-client-bundle or HWIOAuthBundle as an official Symfony package instead of starting a new component from scratch?

@ajgarlag
Copy link
Contributor

I think that any official Symfony package should use symfony/http-client to send http requests.

@dunglas
Copy link
Member

dunglas commented Jun 10, 2019

I think that any official Symfony package should use symfony/http-client to send http requests.

Of course, but patching the two existing packages to use it is easy. Way easier than re-creating something from scratch.

@Guikingone
Copy link
Contributor Author

@dunglas I don't know if it's easier to patch the existing packages (and hoping that nothing breaks) rather than adding this package, I've used Ryan's package and it's great, don't get me wrong, problem is, it's not "native" on Symfony and it force us to use an external package rather than using an "official" one which use the latest tools provided by Sf.

I could be wrong but (and if it's the case, just stop me) for me, it's important that Sf provide this kind of feature with an official package (which is not fragmented/splitted in sub-packages) that helps every developers to implement OAuth with a standard API and a "top-class" (not actually, I agree but I'm working on it 😄) integration on the framework.

I agree with @ajgarlag for HttpClient usages.

Actually, the package take 3-4 days to be created so it's not a big work for now, I see it as an experimentation and if it's clear that Sf doesn't have any gains on using it and prefer to promote @weaverryan's oauth2-client-bundle or HWIOAuthBundle as officials packages, it's okay for me 🙂

@Taluu
Copy link
Contributor

Taluu commented Jun 10, 2019

I don't know about @weaverryan's bundle, but for HWIO, it's a mess in there and something that is (almost) unmaintainable because of the providers which makes it really hard to use and implement a custom provider (hard-coded config).

@dunglas
Copy link
Member

dunglas commented Jun 10, 2019

Ryan's bundle is simpler, and more modern, but doesn't support OAuth 1.

@dunglas
Copy link
Member

dunglas commented Jun 10, 2019

@Guikingone I agree with the rationale of having more things, including an OAuth implementation, in the symfony namespace. My point is why not simply renaming one of the existing battle-tested solutions (OAuth isn't easy) instead of starting from scratch? Switching from anything to HttpClient is straightforward, we've already done it for the MercureComponent, Panther, and the Symfony website ; and we're doing it for API Platform, it's just a matter of changing some method calls.

@Guikingone
Copy link
Contributor Author

My point was that using the KNP client is simple, I agree with it but what if you need to add a new provider? And what about the maintainability?

On the current "component", it's simple, you have a default provider which is linked to the RFC requirements, you only need to instantiate it and configure the options (without creating a dedicated provider thanks to the Security configuration but you can if you need, the default classes are here to guide you and provide a RFC compliant implementation).
For now, the component doesn't rely on external component (except the HttpClient one, I agree), but KNP bundle depends on the PHPLeague library, which can change its API from one day to another (even if it's rare, it can happen), on Sf side and thanks to the BC promises, we can improve the component and maintain it without relying on external librairies (except HttpClient).

When it comes to maintainability, simple example, if you take a look at https://github.com/knpuniversity/oauth2-client-bundle/blob/master/src/Client/Provider/GitlabClient.php and https://github.com/knpuniversity/oauth2-client-bundle/blob/master/src/Client/Provider/GoogleClient.php, where are the differences that requires you to create a dedicated provider?

For me, there's no reason to have dedicated provider as you don't override the arguments, don't change the $token class and so on.

The component in this PR can get rid of this code duplication and improve the DX as the don't need to use multiples classes to do the same things.

As always, I can get wrong and if it's the case, just stop me 🙂

@Guikingone Guikingone changed the title [FEAT] [SECURITY] OAuth component [WIP][SECURITY] OAuth component Jun 12, 2019
@stof
Copy link
Member

stof commented Jun 12, 2019

When it comes to maintainability, simple example, if you take a look at https://github.com/knpuniversity/oauth2-client-bundle/blob/master/src/Client/Provider/GitlabClient.php and https://github.com/knpuniversity/oauth2-client-bundle/blob/master/src/Client/Provider/GoogleClient.php, where are the differences that requires you to create a dedicated provider?

@return GitlabResourceOwner vs @return GoogleUser. They override the method to override the phpdoc with a specific child class to give better autocompletion when you work specifically with one provider.
And they do that because of their own API wrapping the PHPLeague providers. the OAuth2 implementation is actually in PHPLeague, where specific providers are implemented (either in core or third-party) to preconfigure options for ease of use (similar to what we did for mailer, where we have some pre-configured Smtp transports).

KNP bundle depends on the PHPLeague library, which can change its API from one day to another (even if it's rare, it can happen)

Well, PHPLeague also follows semver, so that would still not break your app. Symfony's BC policy is "we follow semver" + "we always provide deprecation warnings before doing BC breaks" (+ a time-based release schedule so that you know when you can expect a major version, but that's not necessary for such good experience, especially for smaller packages like the ones from PHPLeague which may not have a need for major versions as often)

/**
* @author Guillaume Loulier <contact@guillaumeloulier.fr>
*/
trait Psr18ProviderTrait
Copy link
Member

Choose a reason for hiding this comment

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

this empty trait is useless

$query['scope'] = $options['scope'];
} else {
if ($this->logger) {
$this->logger->warning('The scope key is optional, the expected behaviour can vary.');
Copy link
Member

Choose a reason for hiding this comment

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

the message should be about The scope is not provided, not The scope key is optional. If you provide it explicitly, it is still optional.

/**
* @author Guillaume Loulier <contact@guillaumeloulier.fr>
*/
trait Psr7Trait
Copy link
Member

Choose a reason for hiding this comment

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

Why is there some PSR-7 here ? HttpClient is used for requests.

* file that was distributed with this source code.
*/

namespace Symfony\Component\Security\OAuth\Server;
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a Server namespace when the readme says we don't handle the server-side protocol ?

/**
* @author Guillaume Loulier <contact@guillaumeloulier.fr>
*/
abstract class AbstractGrantType implements GrantTypeInterface
Copy link
Member

Choose a reason for hiding this comment

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

such empty abstract class is useless.


public function handleRefreshTokenRequest(AuthorizationRequest $request);

public function returnResponsePayload(): array;
Copy link
Member

Choose a reason for hiding this comment

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

this looks like an interface enforcing to make the service stateful. That's a bad idea. Stateful services are a huge pain (as we then need to reset the state at appropriate time)

"symfony/http-foundation": "~4.3"
},
"require-dev": {
"psr/http-message": "~1.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why would you need PSR-7 interfaces ?

@kralos
Copy link

kralos commented Jun 13, 2019

@Guikingone I think this should all be named such that it's clearly part of OAuth 2.0 as opposed to OAuth 1.0 / 1.0a

@Guikingone
Copy link
Contributor Author

@kralos Hey 👋

Sorry for the delay, yes, it could be a good idea, maybe naming it as OAuth2Client and OAuth2Server can be a solution? 🤔

@Guikingone Guikingone changed the title [WIP][SECURITY] OAuth component [WIP][SECURITY] OAuth2 component Jul 3, 2019
@kralos
Copy link

kralos commented Jul 3, 2019

@Guikingone JWT's are not part of OAuth2's specification, they are supported via an OAuth2 Extension https://tools.ietf.org/html/rfc7519#page-19

[Opinion] Do you think we should solve core vanilla OAuth2, then add Extensions as optional pluggable add ons (Security\Core\Authentication\Provider\AuthenticationProviderInterface + Security\Http\Firewall\ListenerInterface etc)?

Developers might want to plug their own OAuth2 Extensions into the Symfony core OAuth2 implementation.

If you start implementing extensions in Symfony Core, people might expect you to implement all extensions.

@Guikingone
Copy link
Contributor Author

Hi @kralos 👋

Yes, in fact, I was experimenting on this one, the core idea is to solve core OAuth2 specifications then if developers wants to add extensions, providing a set of interfaces (like contracts?) to do it.

That's more or less the idea behind ProviderInterface for now 🙂

As long as you implement this interface, you can add every provider that you need.

For now, I'm focusing on resolving core issues when it comes to authorization attacks as described here:

I was also thinking about implementing Dynamic Client registration as described in 7591 but I don't know if it's really needed for now (as the Client is almost ready for usage).

@Guikingone Guikingone changed the title [WIP][SECURITY] OAuth2 component [WIP][Security] OAuth2 component Aug 14, 2019
@manu-sparheld
Copy link

When can we start using it ?

@Guikingone
Copy link
Contributor Author

When can we start using it?

Hi @manu-sparheld, for now, the client is available and open for alpha testing using the repository, I'm currently working on the security aspect of the component and polishing the DX, feel free to give feedback 🙂

*/
abstract class GenericProvider implements ProviderInterface
{
private const DEFAULT_OPTIONS = [

Choose a reason for hiding this comment

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

I think that some of those keys are not always required including redirect_uri according the use case (client credentials flow for example).

Unless I miss something, it may concern the option userDetails_url too as a user is not always involved.

@Guikingone Guikingone changed the base branch from 4.4 to master September 28, 2019 17:03
@jamalo
Copy link

jamalo commented Nov 7, 2019

This looks very promising. What is the progress? =) @Guikingone

@Guikingone
Copy link
Contributor Author

Hi @j-mywheels,

Thanks for the interest, the component is almost done for the OAuth2Client part of it, the OAuth2Server is for now "paused" and moved to a later release on my side.

I'm currently working on the Symfony integration (mostly Security component and DIC usages) 🙂

@stof
Copy link
Member

stof commented Nov 7, 2019

And given we are already past the feature freeze (way past it), this component won't be part of 4.4 and 5.0. It will have to wait for 5.1

@jamalo
Copy link

jamalo commented Nov 11, 2019

Also a couple questions, will the AccessToken, RefreshToken, Client, Scope.. etc. be linked to Doctrine entities?

Would it be possible to create extension grants (https://tools.ietf.org/html/rfc6749#section-4.5)? it might be useful to see how we can implement two factor authentication in the early stage of this component (using the extension grant for example)?

@Guikingone
Copy link
Contributor Author

Hi @jamalo 👋

For now, the OAuth2Client isn't linked to Doctrine, the "bridge" with the framework can bring a relation due to security internal logic (mostly when it comes to providers) but the core classes aren't linked, the only "relations" are HttpClient and OptionsResolver.

When it comes to Extension Grant, for now, it's not planned as the main requirements are related to the endpoint that you're using (SAML, etc) but it can easily added thanks to GenericProvider (which is the parent provider) 🙂

@jamalo
Copy link

jamalo commented Nov 12, 2019

Hi @jamalo

For now, the OAuth2Client isn't linked to Doctrine, the "bridge" with the framework can bring a relation due to security internal logic (mostly when it comes to providers) but the core classes aren't linked, the only "relations" are HttpClient and OptionsResolver.

When it comes to Extension Grant, for now, it's not planned as the main requirements are related to the endpoint that you're using (SAML, etc) but it can easily added thanks to GenericProvider (which is the parent provider)

Hopefully this will make FOSOAuthServerBundle past time. Good luck @Guikingone

@fabpot
Copy link
Member

fabpot commented Jun 25, 2020

I have not looked at the code. This is interesting as OAuth is everywhere nowadays.

Before going further, I'd like to get thoughts from @weaverryan (as it maintains a similar bundle) and @wouterj as he is actively working on modernizing the Symfony security system (and we talked about OAuth support recently).

@wouterj
Copy link
Member

wouterj commented Jun 25, 2020

I've never used OAuth, everything below this paragraph is based on a few google searches. I apologize if it doesn't make any sense, in that case feel free to ignore it.


I'm a bit on the same track as @dunglas in the comments above.

I feel like OAuth consists of 2 completely saturated PHP communities: OAuth and Http clients. I don't think it makes sense to oversaturate it by creating yet another OAuth component. Apart from that, this also means someone has to maintain the Symfony OAuth component and keep it up to date with the specs, etc.

From what I read here, it seems like the lack of HTTP Client support is the biggest "issue" with the League OAuth package (which is used by the Knp bundle). Can't we discuss with the PHP League maintainers to find out if they are open for building against a standard interface (which they already did before 1.0)? Symfony's HTTP Client and Guzzle are implementing tons of interopability interfaces, it must be possible to find one that can be used imho.

Also, PRs like symfonycorp/connect#95 makes me think there might not be need for a full-blown OAuth component and a couple abstract authenticators might be enough of a help? If someone wants multiple social logins, they can use the popular OAuth bundles already.

@fabpot
Copy link
Member

fabpot commented Sep 24, 2020

I agree with @wouterj that having support for HttpClient in League OAuth package would be much better (https://github.com/thephpleague/oauth2-client).

@fabpot fabpot closed this Sep 24, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.