silexphp / Silex

[DEPRECATED -- Use Symfony instead] The PHP micro-framework based on the Symfony Components
https://silex.symfony.com
MIT License
3.58k stars 718 forks source link

Injecting AccessDecisionManager in to Voter breaks application and kills PHP. #1530

Closed amustill closed 6 years ago

amustill commented 7 years ago

I have a requirement within my application to check user access based on particular roles and attributes, and so it seemed logical to implement a Voter to do this.

However, there appears to be an issue when injecting Symfony\Component\Security\Core\Authorization\AccessDecisionManager in to a Voter, as per the documentation.

Upon injecting the AccessDecisionManager via the $app['access_manager'] service, the application simply returns ERR_EMPTY_RESPONSE.

Furthermore, running the same application via PHP's development server, the server exits with Segmentation fault: 11.

php -S localhost:8000 web/app.php
PHP 7.0.18 Development Server started at Wed Jun  7 12:46:24 2017
Listening on http://localhost:8000
Document root is /Users/adam/Desktop/silex-voter
Press Ctrl-C to quit.
Segmentation fault: 11

I have tested this with symfony/security versions 2.8 through 3.3.

A repository showing the issue is available here: https://github.com/amustill/silex-voter-issue-demo

Line 25 or web/app.php is the offender. Commenting out this line and thus removing the dependency injection of Symfony\Component\Security\Core\Authorization\AccessDecisionManager makes the application load.

skalpa commented 7 years ago

The documentation is misleading. This still creates a circular reference with Silex, even if you use a recent version of the security component.

Using something like silexphp/Pimple#224 to inject the voters into the decision manager will allow us to prevent this, but in the meantime you'll have to resort to the inject the container instead of the decision manager workaround:

$app->extend('security.voters', function($voters) use ($app) {
    $voters[] = new TestVoter($app);

    return $voters;
});

use Pimple\Container;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\AccessDecisionManagerInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;

class TestVoter extends Voter
{
    private $container;

    public function __construct(Container $container)
    {
        $this->container = $container;
    }

    protected function getDecisionManager()
    {
        return $this->container['security.access_manager'];
    }

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
    {
        if ($this->getDecisionManager()->decide($token, array('ROLE_SUPER_ADMIN'))) {
            return true;
        }
        // ...
    }
}
stof commented 7 years ago

@amustill the Symfony 3.3+ wiring performs lazy-loading of voters, which does not create a circular instantiation between the voter and the decision manager. This is why direct injection works for Symfony (the lazy-loading breaks the circular reference). In the case of pimple, the lazy-loading of voters inside the decision manager is not implemented, so such injection creates a circular injection, which leads to an infinite loop (instantiating the access manager first requires instantiating the voter, which ask for instantiating the access manager again, and so on). Note that Symfony 3.2 and older don't allow such direct injection either, due to the circular reference (but the circular reference is detected during the compile-time of the Symfony container instead of triggering an infinite loop at runtime)

amustill commented 7 years ago

@skalpa @stof Many thanks for a quick response and explanation. I now see the limitation in implementing it that way.

I had already reverted to injecting the container as @skalpa suggested.

HeahDude commented 7 years ago

Can we close here?

hkdobrev commented 6 years ago

I've tried to tackle this in #1614 using what @skalpa suggested about authentication providers in https://github.com/silexphp/Pimple/pull/224#issue-123640267.

It proved to be a bit tricky to do for the authentication providers (help appreciated), but it was quite easy for the voters.

fabpot commented 6 years ago

Closing as Silex is not maintained anymore.