Skip to content
This repository has been archived by the owner on Jul 3, 2020. It is now read-only.

Added ability to set multiple assertions and their condition for permissions #320

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
71 changes: 71 additions & 0 deletions docs/06. Using the Authorization Service.md
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,77 @@ return [

Now, every time you check for `myPermission`, `myAssertion` will be checked as well.



### Multiple assertions

The assertion map also accepts multiple assertions as a simple array:

```php
return [
'zfc_rbac' => [
'assertion_map' => [
'myPermission' => 'myAssertion', // single assertion
'myPermission2' => [ // multiple assertions
'myAssertion',
'myAssertion2'
]
]
]
];
```

Or with an additional condition definition:

```php
return [
'zfc_rbac' => [
'assertion_map' => [
// single assertion
'myPermission' => 'myAssertion',
'myPermission2' => [
// multiple assertions
'assertions' => [
'myAssertion',
'myAssertion2'
],
// condition
'condition' => \ZfcRbac\Assertion\AssertionSet::CONDITION_AND
]
]
]
];
```

If 'AND' condition is specified (this is default) all of the assertions must pass the check.
If 'OR' condition is specified at least one of the assertions must pass the check.
This in the background will create an instance of ZfcRbac\Assertion\AssertionSet and adds the given assertions to it.

### Assertion Set

ZfcRbac\Assertion\AssertionSet class is basically a container for multiple assertions as well as assertion condition.
An instance of the class get's actually created automatically when you specify multiple assertions (see above)
in the background, but you can also extend the class or create your own instance containing your custom assertions
and specify that in assertion map instead.

```php
return [
'zfc_rbac' => [
'assertion_manager' => [
'factories' => [
'myAssertionSet' => MyAssertionSetFactory::class
]
],

'assertion_map' => [
'myPermission' => 'myAssertion', // single assertion
'myPermission2' => 'myAssertionSet' // multiple assertions in set
]
]
];
```


### Checking permissions in a service

So let's check for a permission, shall we?
Expand Down
205 changes: 205 additions & 0 deletions src/ZfcRbac/Assertion/AssertionSet.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license.
*/
namespace ZfcRbac\Assertion;

use ZfcRbac\Exception\InvalidArgumentException;
use ZfcRbac\Service\AuthorizationService;

/**
* Assertion set to hold and process multiple assertions
*
* @author David Havl
* @licence MIT
*/

class AssertionSet implements AssertionInterface, \IteratorAggregate
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most of the setters/getters are not required and/or can be private or removed only assert() method is required by interface so why expose those while we can build AssertionSet thought constructor?

Copy link
Author

Choose a reason for hiding this comment

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

They are not required but I think it may be a good idea to have them, in case people want to use the class their way (see docs).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think anyone would want to modify this after it is added to AuthorizationService and even if they want this is not a good practice.
@basz @prolic @bakura10 what you think?

Copy link
Author

Choose a reason for hiding this comment

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

Of course not after it is added to AuthorizationService, but when specifying assertion map. See my other comment bellow.

{
/**
* Condition constants
*/
const CONDITION_OR = 'OR';
const CONDITION_AND = 'AND';

/**
* @var $assertions array
*/
protected $assertions = [];

/**
* @var $condition string
*/
protected $condition = AssertionSet::CONDITION_AND;

/**
* Constructor.
*
* @param array|AssertionInterface[] $assertions An array of assertions.
*/
public function __construct(array $assertions = array())
{
$this->setAssertions($assertions);
}

/**
* Set assertions.
*
* @param AssertionInterface[] $assertions The assertions to set
*
* @return $this
*/
public function setAssertions(array $assertions)
{
$this->assertions = [];

// if definition contains condition, set it.
if (isset($assertions['condition'])) {
if ($assertions['condition'] != self::CONDITION_AND && $assertions['condition'] != self::CONDITION_OR) {
throw new InvalidArgumentException('Invalid assertion condition given.');
}
$this->setCondition($assertions['condition']);
}

// if there are multiple assertions under a key 'assertions', get them.
if (isset($assertions['assertions']) && is_array($assertions['assertions'])) {
$assertions = $assertions['assertions'];
}

// set each assertion
foreach ($assertions as $name => $assertion) {
$this->setAssertion($assertion, is_int($name) ? null : $name);
}
return $this;
}

/**
* Set an assertion.
*
* @param string|AssertionInterface $assertion The assertion instance or it's name
* @param string $name A name/alias
* @return $this
*/
public function setAssertion(AssertionInterface $assertion, $name = null)
{
if (null !== $name) {
$this->assertions[$name] = $assertion;
} else {
$this->assertions[] = $assertion;
}
}

/**
* Returns true if the assertion if defined.
*
* @param string $name The assertion name
*
* @return bool true if the assertion is defined, false otherwise
*/
public function hasAssertion($name)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure but do we use this somewhere outside this class?

Copy link
Author

Choose a reason for hiding this comment

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

Yes people can when they use the class directly (see docs).

Copy link
Contributor

Choose a reason for hiding this comment

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

okey maybe they could but still not convinced. Even if they want to modify it what is bad I think they could create new one instead. because reusing same set on different permissions and randomly modify them at the same time can lead to big problems. And if users would want those evil methods they could extend the class anyway no? so why add those we don't want to maintain +6 methods for just in case users would need them.
but again @basz @prolic @bakura10 @danizord what you think?

Copy link
Author

@DavidHavl DavidHavl Oct 28, 2016

Choose a reason for hiding this comment

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

Not sure you understand. This is to be used for example when creating assertion map. I was imagining usage of it for example like this:

Creating a factory where one can do anything they want

use Interop\Container\ContainerInterface;
use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;
use ZfcRbac\Assertion\AssertionSet;

class MyAssertionSetFactory implements FactoryInterface
{
    /**
     * {@inheritDoc}
     *
     * @return AssertionSet
     */
    public function __invoke(ContainerInterface $container, $name, array $options = null)
    {
        $assertionManager = $container->get('ZfcRbac\Assertion\AssertionPluginManager');
        $assertion1 = $assertionManager->get('myAssertion1');
        $assertion2 = $assertionManager->get('myAssertion2');

        // create instance, set condition and add assertions
        $assertionSet = new AssertionSet();
        $assertionSet->setCondition(AssertionSet::CONDITION_OR);
        $assertionSet->setAssertions([$assertion1, $assertion2]);
        return $assertionSet;
    }

    /**
     * {@inheritDoc}
     *
     * For use with zend-servicemanager v2; proxies to __invoke().
     *
     * @param ServiceLocatorInterface $container
     * @return \ZfcRbac\Assertion\AssertionSet
     */
    public function createService(ServiceLocatorInterface $container)
    {
        // Retrieve the parent container when under zend-servicemanager v2
        if (method_exists($container, 'getServiceLocator')) {
            $container = $container->getServiceLocator() ?: $container;
        }

        return $this($container, AssertionSet::class);
    }
}

And then add it to assertion manager and assertion map config:

return [
    'zfc_rbac' => [
        'assertion_manager' => [
            'factories' => [
                'myAssertionSet' => MyAssertionSetFactory::class
            ]
        ],

        'assertion_map' => [
            'myPermission'  => 'myAssertion', // single assertion
            'myPermission2' => 'myAssertionSet' // multiple assertions in set
        ]
    ]
];

Copy link
Contributor

Choose a reason for hiding this comment

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

you could do the same without those methods:

$assertionManager = $container->get('ZfcRbac\Assertion\AssertionPluginManager');
$assertion1 = $assertionManager->get('myAssertion1');
$assertion2 = $assertionManager->get('myAssertion2');

// create instance, set condition and add assertions
 $assertionSet = new AssertionSet([
    'assertions' => [$assertion1, $assertion2],
    'condition' => AssertionSet::CONDITION_OR
]);

and this would prevent from changing later.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for immutable approach. That makes unit testing way easier and confident since you don't need to test a lot of possible different states.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough. That is a good point.
Will rewrite.
Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I said but will repeat: not good that we can't do this:

$assertionSet = new AssertionSet([
    'assertions' => ['myAssertion1', 'myAssertion2'],
    'condition' => AssertionSet::CONDITION_OR
]);

Copy link
Author

Choose a reason for hiding this comment

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

@svycka great, I am working on it.

{
return isset($this->assertions[$name]);
}

/**
* Gets a assertion value.
*
* @param string $name The assertion name
*
* @return AssertionInterface The assertion instance
*
* @throws InvalidArgumentException if the assertion is not defined
*/
public function getAssertion($name)
{
if (!$this->hasAssertion($name)) {
throw new InvalidArgumentException(sprintf('The assertion "%s" is not defined.', $name));
}
return $this->assertions[$name];
}

/**
* Gets all assertions.
*
* @return AssertionInterface[] The assertion instance
*/
public function getAssertions()
{
return $this->assertions;
}

/**
* @return string
*/
public function getCondition()
{
return $this->condition;
}

/**
* Set condition
*
* @param string $condition
*/
public function setCondition($condition)
{
$this->condition = $condition;
}

/**
* Retrieve an external iterator
* @return \ArrayIterator
*/
public function getIterator()
Copy link
Member

Choose a reason for hiding this comment

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

I think there's no reason to expose this method and other getters?

{
return new \ArrayIterator($this->assertions);
}

/**
* Check if assertions are successful
*
* @param AuthorizationService $authorizationService
* @param mixed $context
* @return bool
*/
public function assert(AuthorizationService $authorizationService, $context = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe not logical but it's possible to have zero assertions here then maybe return true by default or throw exception?

Copy link
Collaborator

Choose a reason for hiding this comment

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

An empty assertion is not logical indeed and should either return FALSE or an exception.

Better to have defensive defaults.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Thanx.

Copy link
Author

Choose a reason for hiding this comment

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

Done

{
if (AssertionSet::CONDITION_AND === $this->condition) {
foreach ($this->assertions as $assertion) {
if (!$assertion->assert($authorizationService, $context)) {
return false;
}
}

return true;
}

if (AssertionSet::CONDITION_OR === $this->condition) {
foreach ($this->assertions as $assertion) {
if ($assertion->assert($authorizationService, $context)) {
return true;
}
}

return false;
}

throw new InvalidArgumentException(sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be checked at construction time

'Condition must be either "AND" or "OR", %s given',
is_object($this->condition) ? get_class($this->condition) : gettype($this->condition)
));
}
}
37 changes: 31 additions & 6 deletions src/ZfcRbac/Service/AuthorizationService.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Rbac\Permission\PermissionInterface;
use ZfcRbac\Assertion\AssertionPluginManager;
use ZfcRbac\Assertion\AssertionInterface;
use ZfcRbac\Assertion\AssertionSet;
use ZfcRbac\Exception;
use ZfcRbac\Identity\IdentityInterface;

Expand Down Expand Up @@ -72,11 +73,36 @@ public function __construct(Rbac $rbac, RoleService $roleService, AssertionPlugi
* Set an assertion
*
* @param string|PermissionInterface $permission
* @param string|callable|AssertionInterface $assertion
* @param string|callable|array|AssertionInterface $assertion
* @return void
*/
public function setAssertion($permission, $assertion)
{
// if is name of the assertion, retrieve an actual instance from assertion plugin manager
if (is_string($assertion)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it's slower to do it here instead of where it was in assert() method because it will be created on every request even if this is not used.

Copy link
Author

@DavidHavl DavidHavl Oct 28, 2016

Choose a reason for hiding this comment

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

Good, point. Will move it to assert method.

$assertion = $this->assertionPluginManager->get($assertion);
} elseif (is_array($assertion)) { // else if multiple assertion definition, create assertion set.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this whole block here? can't we move it to AssertionSet? something like:

if (is_array($assertion)) {
   $assertion = new AssertionSet($assertion, $this->assertionPluginManager);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

one more thought. Maybe when moved to AssertionSet we could have nested assertions something like this:

return [
    'zfc_rbac' => [
        'assertion_map' => [
             // single assertion
            'myPermission'  => 'myAssertion',
            'myPermission2' => [ 
                // multiple assertions
                'assertions' => [
                    'myAssertion',
                    'myAssertion2',
                    [ 
                      // multiple assertions
                      'assertions' => [
                          'myAssertion3',
                          'myAssertion4'
                      ], 
                      // condition
                      'condition' => \ZfcRbac\Assertion\AssertionSet::CONDITION_OR
                  ]
                ], 
                // condition
                'condition' => \ZfcRbac\Assertion\AssertionSet::CONDITION_AND
            ]
        ]
    ]
];

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure I like moving the retrieval of assertions to AssertionSet class. Main idea behind the AssertionSet is to be a somewhat simple reusable container for assertions and specifically to have the ability to specify the condition.
Thoughts anyone?

Copy link
Contributor

Choose a reason for hiding this comment

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

and I don't like idea adding AssertionSet logic inside AuthorizationService while it is just simple custom assertion.
but again @bakura10 @prolic @basz @danizord

Copy link
Author

@DavidHavl DavidHavl Oct 28, 2016

Choose a reason for hiding this comment

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

I understand your concern, but look at the contradicting response/suggestion from bakura10 few months ago... this was the initial reasoning behind adding it and I am very fond of it since then. In fact I am using the concept now on several projects (on tweaked ZfcRbac module) and I like the versatility of that solution. But I am opened to suggestions of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry but I don't understand I just say this block is almost same as AssertionSet::setAssertions() so why duplicate it here. Also I understand why you don't want AssertionPluginManager in AssertionSet but thats also not so logical because of this you can't use assertion names while creating AssertionSet you have to first fetch all required assertions then add to set this is performance hungry if you have a lot of assertions.

Copy link
Member

Choose a reason for hiding this comment

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

What about the following approach?

  • assertion_map config key accepts only string => string (permission name => assertion name)
  • Another config key for assertion_sets that accepts string => array (assertion name => assertion config)
  • A default AssertionSetFactory implementation that reads the assertion_sets config key and builds AssertionSet
  • AuthorizationService always fetch assertions from AssertionPluginManager

Copy link
Contributor

Choose a reason for hiding this comment

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

@danizord maybe, but not sure I like it :) also have concerns about performance.


// move assertion definition under a key 'assertions'.
if (!isset($assertion['assertions'])) {
$assertion['assertions'] = (array)$assertion;
}

if (!is_array($assertion['assertions'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't know if this is good creating assertions while setting them. Maybe we could create AssertionSet only if we need in assert() method?

$assertion['assertions'] = (array)$assertion['assertions'];
}

// retrieve an actual instance from assertion plugin manager if necessary
foreach ($assertion['assertions'] as $key => $value) {
if (is_string($value)) {
$assertion['assertions'][$key] = $this->assertionPluginManager->get($value);
Copy link
Contributor

Choose a reason for hiding this comment

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

fetching assertions would be better when they are really needed in assert() method in this case AssertionSet::assert()

}
}

// create assertion set
$assertion = new AssertionSet($assertion);
}

$this->assertions[(string) $permission] = $assertion;
}

Expand All @@ -88,7 +114,10 @@ public function setAssertion($permission, $assertion)
*/
public function setAssertions(array $assertions)
{
$this->assertions = $assertions;
$this->assertions = [];
foreach ($assertions as $permissionName => $assertionData) {
$this->setAssertion($permissionName, $assertionData);
}
}

/**
Expand Down Expand Up @@ -149,10 +178,6 @@ protected function assert($assertion, $context = null)
if (is_callable($assertion)) {
return $assertion($this, $context);
} elseif ($assertion instanceof AssertionInterface) {
return $assertion->assert($this, $context);
} elseif (is_string($assertion)) {
$assertion = $this->assertionPluginManager->get($assertion);

return $assertion->assert($this, $context);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add this:

elseif (is_array($assertion)) {
    $assertionSet = new AssertionSet($assertion, $this->assertionPluginManager);
    return $assertionSet->assert($this, $context);
}

see my above comments why


Expand Down