AssertionInterface is inconvenient

[olegkrivtsov]

Hi,

I'm trying to use your dev-develop version of zend-permissions-rbac with the latest fixes. I'm testing how to use dynamic assertions and found AssertionInterface inconvenient.

Currently, the AssertionInterface provides the assert(Rbac $rbac) method. The $rbac argument is not the primary data I need for assertion checking. Actually, I need the name of permission and some array of parameters. Permission name and additional parameters now have to be passed to assertion class via additional methods.

My suggestion: why not to introduce the following assert method: assert(Rbac $rbac, $permission, $params)? That would make it easier to implement dynamic assertions in my code.

P.S. Currently, assertion can be a class, a function, a closure, etc. The class is usable somehow, but the function/closure is absolutely not usable, because you pass only $rbac argument to it. How to pass the permission name and parameters to the function?

[olegkrivtsov]

What's most inconvenient with AssertionInterface is that it suggests to write a different assertion class per permission. It is very boring/unneeded work. My suggestion would make the single assertion class to handle all/several dynamic assertions.

Can you introduce another method in AssertionInterface, say checkAssertion(Rbac $rbac, $permission, $params)? This won't break BC, and make the interface more usable.

For example, one would write something like this:

class AssertionManager implements AssertionInterface
{
    //...

    public function checkAssertion(Rbac $rbac, $permission, $params) 
    {
        switch ($permission) {
            case 'profile.view.own' : return $params['user']==$this->authService->getIdentity(); 
        }

        return false;
    }
}

[olegkrivtsov]

I can provide a pull request if you approve this.

[ezimuel]

@olegkrivtsov I see your point and it makes sense to me. Since I'm planning to schedule a major version (3.0.0) of zend-permissions-rbac we can think to change the AssertionInterface. To reduce BC breaks, I was thinking to change it passing $permission and $role as optional parameters:

namespace Zend\Permissions\Rbac;

interface AssertionInterface
{
    /**
     * Assertion method - must return a boolean.
     *
     * @param Rbac $rbac
     * @param string $permission
     * @param RoleInterface $role
     * @return bool
     */
    public function assert(Rbac $rbac, $permission = null, $role = null);
}

Regarding your proposal for a $params, I don't think we need it. You can create a class that implements AssertionInterface passing all the dependencies (in constructor or using setter) and consuming it in the assert() function.

[PowerKiKi]

To reduce BC breaks

If you are already breaking things, please go all the way to have the best result possible, so make the new arguments mandatory. If they were optional that would suggest that we could assert something without any permission, which seems rather counter-intuitive.

[ezimuel]

@PowerKiKi the parameters of AssertionInterface::assert are all required. The new interface of v.3.0.0 will be as follows:

namespace Zend\Permissions\Rbac;

interface AssertionInterface
{
    /**
     * Assertion method - must return a boolean.
     */
    public function assert(Rbac $rbac, RoleInterface $role, string $permission) : bool;
}

Notice: I also changed the parameter order of $role and $permission. See #34 for more details.