Fix SecurityServiceProviders voters

[hkdobrev]

Follow-up to #1614.

See https://github.com/silexphp/Silex/pull/1614#discussion_r170567411

[hkdobrev]

@stof I realise where you're coming from, but I think we could have a discussion whether that should be a deprecation here and that should be merged as a fix as the current master is broken after #1614 :( This way more things could be released as patch versions. What do you think?

[stof]

This PR is still broken. If you extend both the security.voter_services and security.voters arrays (which could happen in different providers), only extensions of the security.voter_services service are taken into account, as explained in my previous review.

Breaking code using the old extension as soon as any provider starts using the new one means that the BC layer is broken (as soon as someone starts migrating, there is no BC layer anymore).

[hkdobrev]

I have 2 suggestions to fix it so far:

We can make it work in parallel if we register services on the fly based on the voters objects and extend the services with them, but this seems quite hacky.

A simpler implementation could be achieved if the Pimple\ServiceIterator class accepts both service ID and instances

[hkdobrev]

@fabpot Would you have a moment to chime in on this? How do you think BC should be handled here? What's actually a breaking change?

[skalpa]

You can solve the BC problem by creating a custom iterator that is able to iterate over an array of service ids and an array of objects in sequence.

namespace Silex;

/** @internal **/
final class ServiceIterator implements \Iterator
{
    private $app;
    private $iterated;
    private $idsCount;
    private $index = 0;

    public function __construct(Application $app, array $serviceIds, array $serviceObjects)
    {
        $this->app = $app;
        $this->idsCount = count($serviceIds);
        $this->iterated = array_merge(array_values($serviceIds), array_values($serviceObjects));
    }

    public function current()
    {
        $current = $this->iterated[$this->index++];

        return $this->index > $this->idsCount ? $current : $this->app[$current];
    }
    // .......
}

Then make sure users get a deprecation notice if $app['security_voters'] is not empty, and inject new ServiceIterator($app, $app['security.voter_services'], $app['security.voters']).

At a later stage you'll be able to get rid of security.voters and inject a plain Pimple\ServiceIterator instance in place of your Silex\ServiceIterator.

Another remark: security.voter_services should be renamed to security.voter_service_ids as it is not an array of services, but an array of service ids (and that would be consistent with validator.validator_service_ids for instance).

[hkdobrev]

You can solve the BC problem by creating a custom iterator that is able to iterate over an array of service ids and an array of objects in sequence.

👍 That's what I've suggested above in https://github.com/silexphp/Silex/pull/1619#discussion_r170618144

At a later stage you'll be able to get rid of security.voters and inject a plain Pimple\ServiceIterator instance in place of your Silex\ServiceIterator.

Do you mean that we should implement the service iterator in Silex, but later move the implementation to Pimple when we upgrade to require minimum next minor release of Pimple?

Another remark: security.voter_services should be renamed to security.voter_service_ids as it is not an array of services, but an array of service ids (and that would be consistent with validator.validator_service_ids for instance).

👍 Totally with you here, couldn't find what the convention for these is.

[hkdobrev]

Then make sure users get a deprecation notice if $app['security_voters'] is not empty

That's not that easy unfortunately. To preserve BC we need this to continue to work:

$app->extend('security.voters`, function(array $voters) {
    $voters[] = new MyCustomVoter();

    return $voters;
});

And we need to keep the two default voters inside that voters array. We can easily check if security.voter_service_ids was modified as we can just diff the strings inside. For the voter services we may end up instantiating the default voters even if the users have used the new way and overridden the defaults.

[skalpa]

No, I don't think this belong in Pimple. I mean just get rid of it because it won't be needed it anymore:

• Tell users that using security.voters is deprecated and that it will be removed in the next major version of Silex
• In the next major Silex, remove security.voters
• At that point you'll just need an iterator that can new ServiceIterator($app, $app['security.voter_service_ids']), and that's exactly how the Pimple iterator works. Thus you'll be able to remove the temporary Silex\ServiceIterator and use the Pimple class instead (note that to be able to do this and just remove the class you have to add a docblock to mark it as @internal so users know they should not rely on it).

[hkdobrev]

On the service container, I've done it like you describe, I just thought Pimple could use that capability in general. But anyway, this is not the blocker here. What do you think about my BC comment?

[skalpa]

Regarding BC, I just realized that what I advised covered the users can add voters to the defaults use case but not users can replace the list of voters completely .

If I'm correct, this should be 100% BC instead:

$app['security.voter_service_ids'] = function () { return array(); };
$app['security.__default_voters'] = function () {
    return array(new Voter1(), new Voter2());
}; 
$app['security.voters'] = function () { return $app['security.__default_voters']; };

$app['security.access_manager'] = function ($app) {
    if ($app['security.voters'] !== $app['security.__default_voters']) {
        @trigger_error('Blah', E_USER_DEPRECATED);
    }

    return new AccessDecisionManager(new ServiceIterator($app, $app['security.voter_service_ids'], $app['security.voters']));
};

[hkdobrev]

Let's list all cases:

1. User hasn't modified any voters and access the security.access_manager service with the defaults.

• ServiceIterator is passed to AccessDecisionManager with service IDs only
• No deprecation warning is triggered
• RoleHierarchyVoter and AuthenticatedVoter voters are used

2. User has modified the security.voters service, but has not modified the security.voter_service_ids

• ServiceIterator is passed to AccessDecisionManager with instances only
• Deprecation warning is triggered
• The modified voters are used.

3. User has modified the security.voter_service_ids service, but has not modified the security.voters.

• ServiceIterator is passed to AccessDecisionManager with service IDs only
• No deprecation warning is triggered
• The modified voters are used lazily

4. User has modified both the security.voter_service_ids and security.voters services.

• ServiceIterator is passed to AccessDecisionManager with both service IDs and instances
• Deprecation warning is triggered
• All of the modified voters are used

[hkdobrev]

I've implemented the ServiceIterator supporting both cases in one argument as I think it's more useful for its purpose.

[fabpot]

@hkdobrev You need to get rid of the merge commit. We do not merge pull request with merge commit. Instead, you can rebase on current master (that will automatically remove the merge commit).

[hkdobrev]

@fabpot Done. Thought I'd use the GitHub conflict resolution for once as it was just for the changelog, but unfortunately it doesn't support rebasing.

On a related note, I'll send a PR telling Git to choose both sides when there's a conflict from additions in the changelog reducing conflicts at least locally via .gitattributes.

[hkdobrev]

✅ Fixed tests ❌ Triggered deprecation warning 📝 Updated changelog 📜 Updated docs

@stof @skalpa @fabpot Any input?

[skalpa]

Your latest version still breaks BC. Users are currently able to remove the default voters completely by using:

$app->extend('security.voters', function () {
    return array();
});

But that wouldn't work anymore...

Also, the way default voters are added to both security.voters and security.voter_service_ids is confusing and will lead to the registration of the same voter multiple times if custom voters are added to both parameters (ie: if a users chooses to use security.voter_service_ids while a 3rd-party service providers adds its voters to security.voters).

Honestly, while I obviously see the benefits of using iterators more globally, I think this hasn't been thought through well enough, and I'd recommend to just revert #1614 and fix master for the time being, so a better solution can be designed later.

The circular dependencies problem it was supposed to fix can be avoided easily anyway, so there's no rush.

[hkdobrev]

The circular dependencies problem it was supposed to fix can be avoided easily anyway

How could it be avoided?

[skalpa]

You can avoid circular dependencies by injecting the application instead of the AccessDecisionManager, as per my comment in the original issue.

[hkdobrev]

Closing in favour of #1634.