Skip to content
This repository has been archived by the owner. It is now read-only.

Add support for the Guard component to the SecurityServiceProvider #1296

Closed
wants to merge 8 commits into from

Conversation

@GromNaN
Copy link
Contributor

commented Dec 17, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1260
License MIT
Doc PR included

Services configuration are extracted from the SecurityBundle guard.xml and GuardAuthenticationFactory

Usage is quite simple, the guard type can be configured in the firewall like other ones.

$app['app.authenticator'] = function ($app) {
    return new Authenticator();
};

$app->register(new Silex\Provider\SecurityServiceProvider(), [
    'security.firewalls' => [
        'main' => [
            'pattern' => '^/admin',
            'guard' => [
                'authenticators' => [
                    'app.authenticator'
                ]
            ]
        ]
    ]
]);

@GromNaN GromNaN changed the title [WIP] Add support for the Guard component to the SecurityServiceProvider Add support for the Guard component to the SecurityServiceProvider Dec 30, 2015

@GromNaN

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2015

This PR is ready to be reviewed.

);
.. note::
You can use many authenticators, they are executed by if the order they are configured.

This comment has been minimized.

Copy link
@davedevelopment

davedevelopment Jan 4, 2016

Contributor

by if

s/if//

This comment has been minimized.

Copy link
@weaverryan
@davedevelopment

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2016

A tentative 👍 from me, I've only read the code, not run anything :)

@GromNaN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2016

Maybe @weaverryan could have a look.

$entryPoint = null;
if ('http' === $type) {
$entryPoint = 'http';
} elseif ('form' === $type) {
$entryPoint = 'form';
} elseif ('guard' === $type) {
$entryPoint = 'guard';
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jan 27, 2016

👍 this is consistent with the full-stack: we don't replace the entry point of other auth methods, just to be safe

Step 1) Create the Authenticator Class
--------------------------------------

Suppose you have an API where your clients will send an X-AUTH-TOKEN header on each request. This token is composed of the username followed by a password, separated by a colon. Your job is to read this, find the associated user (if any) and check the password.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jan 27, 2016

I think the standard is to add line breaks after about the 72nd character.

Also, you should give an example if the header:

the username followed by a password, separated by a colon (e.g. ``X-AUTH-TOKEN: coolguy:awesomepassword``)
// no credential check is needed in this case
// return true to cause authentication success
return $user->getPassword() === $credentials['secret'];

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jan 27, 2016

Is the secret (the password) supposed to be encoded when the user sends it? If it's plain-text, then we would need to encode that secret to compare it with the encoded password, right?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

This should be:

        $encoder = $this->encoderFactory->getEncoder($user);

        return $encoder->isPasswordValid(
            $user->getPassword(),
            $credentials['secret'],
            $user->getSalt()
        );

With a constructor:

    private $encoderFactory;

    public function __construct(EncoderFactoryInterface $encoderFactory)
    {
        $this->encoderFactory = $encoderFactory;
    }

And the service registration needs to be updated:

$app['app.token_authenticator'] = function ($app) {
    return new App\Security\TokenAuthenticator($app['security.encoder_factory']);
};

This comment has been minimized.

Copy link
@GromNaN

GromNaN Apr 26, 2016

Author Contributor

What about password_verify ? That's a lot simpler.

@weaverryan

This comment has been minimized.

Copy link

commented Jan 27, 2016

Thanks @GromNaN for this (and the ping!). I left some comments, but it looks great. I also didn't try it locally... but your functional test speaks for itself.

Let's get those tweaks made and get this guy merged finally :)

public function checkCredentials($credentials, UserInterface $user)
{
return $user->getPassword() === $credentials['secret'];

This comment has been minimized.

Copy link
@GromNaN

GromNaN Feb 19, 2016

Author Contributor

I'll add a comment tailling this is not a safe way of checking password.

@vicmosin

This comment has been minimized.

Copy link

commented Apr 11, 2016

Have anyone an idea when this feature comes to release?

@weaverryan

This comment has been minimized.

Copy link

commented Apr 15, 2016

I'll review this PR again soon, including testing it locally.

$app['app.token_authenticator'] = function ($app) {
return new App\Security\TokenAuthenticator();
}

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

missing ; at the end - };

return;
}
list($username, $secret) = explode(':', $token, 2);

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

I know it's just an example, but we need to check for the presence of a : and fail auth if it's not there. The curl examples below try with a token of alan first (no colon), and it causes an undefined offset.

public function checkCredentials($credentials, UserInterface $user)
{
// check credentials - e.g. make sure the password is valid
// no credential check is needed in this case

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

probably this second bullet should be removed :)

// which one is used as entry point.
// 'entry_point' => 'app.token_authenticator',
),
'users' => array(

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

I'd like to add a comment above this:

// configure where your users come from. Hardcode them, or load them from somewhere
// http://silex.sensiolabs.org/doc/providers/security.html#defining-a-custom-user-provider
),
'users' => array(
'victoria' => array('ROLE_USER', 'randomsecret'),
),

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 26, 2016

I'd also like:

// 'anonymous' => true
@weaverryan

This comment has been minimized.

Copy link

commented Apr 26, 2016

@GromNaN I just tested this locally and added some corrections so we can get this guy merged! Also:

A) We should link to the cookbook entry form the security provider chapter
B) Is this meant to be only for Silex 2? The PR is against the master branch - if we want to be able to merge into Silex 1, it should be rebased against 1.3. That can be done on merge, I just don't know if there are actual code changes that would need to happen (there probably are) to be compatible with 1.3.

Also, in case it helps, I have a working project using your code here: https://github.com/weaverryan/silex-guard-test-example/tree/finished. You just need to delete vendor/silex/silex and replace it with your fork on the guard branch.

Thanks!

@GromNaN

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2016

Thanks, I'll check all that later today.

@GromNaN GromNaN force-pushed the GromNaN:guard branch from 1711fc9 to 5c599ab Apr 26, 2016

@GromNaN

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2016

@weaverryan I patched the doc.

Silex 1 and 2 have strong incompatibilities (pimple version, service provider definition) ...
I can backport this PR to Silex 1.3, but the code and the doc will be different between Silex 1.3 and 2.

$authenticators[] = $app[$authenticatorId];
}
$class = isset($options['listener_class']) ? $options['listener_class'] : 'Symfony\\Component\\Security\\Guard\\Firewall\\GuardAuthenticationListener';

This comment has been minimized.

Copy link
@stof

stof Apr 26, 2016

Contributor

does the listener class need to be configurable here ?

This comment has been minimized.

Copy link
@GromNaN

GromNaN Apr 26, 2016

Author Contributor

Not really. I replicated what has been done on others.

$authenticatorIds = $options['authenticators'];
if (count($authenticatorIds) == 1) {
// if there is only one authenticator, use that as the entry point
return $app[array_shift($authenticatorIds)];

This comment has been minimized.

Copy link
@stof

stof Apr 26, 2016

Contributor

using array_shift to get the first argument creates a reference mismatch here on PHP 5.x (PHP 7 does not have reference mismatches anymore) because the function takes a reference as argument. This forces to duplicate the array.
PHP 7 will also need to duplicate the array as you alter it.

Getting the first element without changing the array would be better.

This comment has been minimized.

Copy link
@GromNaN

GromNaN Apr 26, 2016

Author Contributor

I copied this code from GuardAuthenticationFactory

What about using current($authenticatorIds) instead ?

This comment has been minimized.

Copy link
@SpacePossum

SpacePossum Apr 26, 2016

Contributor

current might not work if the pointer is pointing to the end of the array. Since we know the count of the array is one wouldn't reset be safe and fast (i.e. no copy, no risk and no changes to the array)?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 28, 2016

In practice, the way it's written should be fine, right? Specifically, $authenticatorIds = $options['authenticators']; makes a copy of the array, and then this new copy is only used for this one purpose.

But, +1 also for reset() as a solution.

This comment has been minimized.

Copy link
@stof

stof Apr 29, 2016

Contributor

@weaverryan $authenticatorIds = $options['authenticators'] does not copy the array, as PHP performs copy-on-write.

reset would cause the same issue, as it also takes a reference, and so causes a reference mismatch.

This comment has been minimized.

Copy link
@SpacePossum

SpacePossum Apr 29, 2016

Contributor

since the key of the first (and only in this case) element is unknown, how to get the first element without the mismatch?

@mathroc

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2016

FWIW, I've been using in it in a some projects for a few weeks to implements token based and session authentication with success :)

thx @GromNaN !

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

Thank you @GromNaN.

fabpot added a commit that referenced this pull request Apr 29, 2016
feature #1296 Add support for the Guard component to the SecurityServ…
…iceProvider (GromNaN)

This PR was squashed before being merged into the 2.0.x-dev branch (closes #1296).

Discussion
----------

Add support for the Guard component to the SecurityServiceProvider

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #1260
| License       | MIT
| Doc PR        | included

Services configuration are extracted from the SecurityBundle [`guard.xml`](https://github.com/symfony/symfony/blob/3.0/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.xml) and [`GuardAuthenticationFactory`](https://github.com/symfony/symfony/blob/3.0/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/GuardAuthenticationFactory.php)

Usage is quite simple, the `guard` type can be configured in the firewall like other ones.

```php
$app['app.authenticator'] = function ($app) {
    return new Authenticator();
};

$app->register(new Silex\Provider\SecurityServiceProvider(), [
    'security.firewalls' => [
        'main' => [
            'pattern' => '^/admin',
            'guard' => [
                'authenticators' => [
                    'app.authenticator'
                ]
            ]
        ]
    ]
]);
```

Commits
-------

4b5ccc9 Add support for the Guard component to the SecurityServiceProvider

@fabpot fabpot closed this Apr 29, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.