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

DavidHavl · 2016-01-04T21:24:19Z

An update to V2 to allow set multiple assertions as well as their condition for permissions.

…ssions.

bakura10 · 2016-01-04T21:43:58Z

I like the idea. What about bringing a new "AssertionSet" class that would contain the two constants, and abstract the logic of checking all the assertions and returning a boolean, so the isGranted can stay simple?

DavidHavl · 2016-01-05T15:17:37Z

That actually sounds good. I'll work on it within next few days.

lorenzoferrarajr · 2016-03-07T13:48:42Z

This is a very good idea! It would help us keep things better organized.

basz · 2016-09-04T15:46:29Z

@DavidHavl I like this idea! Now work on V3 is progressing it would be bring this feature there too.

DavidHavl · 2016-09-13T13:27:14Z

Thanks @basz, I have been extremely busy with work past few months but I should have more time now for this.

prolic · 2016-10-24T08:10:59Z

Is there still progress on this?

… permissions to use new AssertionSet.

coveralls · 2016-10-25T15:07:04Z

Coverage decreased (-4.9%) to 87.96% when pulling 01e35ba on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-25T15:07:04Z

Coverage decreased (-4.9%) to 87.96% when pulling 01e35ba on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-25T15:19:12Z

Coverage decreased (-4.9%) to 87.96% when pulling b06275f on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-25T15:19:12Z

Coverage decreased (-4.9%) to 87.96% when pulling b06275f on DavidHavl:master into 7344374 on ZF-Commons:master.

basz

Looks very solid.

Perhaps the way an AssertionSet is created could be automated. Look at 'RoutePermissionsGuard' configuration for inspiration if you're up to it. cc @prolic

basz · 2016-10-25T15:40:48Z

src/ZfcRbac/Service/AuthorizationService.php

@@ -122,19 +122,15 @@ public function getIdentity()
    public function isGranted($permission, $context = null)
    {
        $roles = $this->roleService->getIdentityRoles();
-


no need to remove these empty lines

prolic · 2016-10-25T15:46:33Z

Also coverage decreased, can you add some more tests, please?

DavidHavl · 2016-10-25T15:50:28Z

@basz that was my initial idea behind this but it was not accepted.
I'll work on it.

basz · 2016-10-25T18:20:29Z

Ok, remember why? Perhaps was it for good reason that I am unfamiliar with?

coveralls · 2016-10-28T00:45:12Z

Coverage increased (+0.5%) to 93.361% when pulling d2a7e27 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T00:45:12Z

Coverage increased (+0.5%) to 93.361% when pulling d2a7e27 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T00:58:18Z

Coverage increased (+0.5%) to 93.361% when pulling 8c6a9c3 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T00:58:18Z

Coverage increased (+0.5%) to 93.361% when pulling 8c6a9c3 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T01:06:19Z

Coverage increased (+0.5%) to 93.361% when pulling 311b942 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T01:35:35Z

Coverage increased (+0.5%) to 93.361% when pulling d9a86ad on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T02:15:58Z

Coverage increased (+0.5%) to 93.361% when pulling 1a30f06 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T02:15:58Z

Coverage increased (+0.5%) to 93.361% when pulling 1a30f06 on DavidHavl:master into 7344374 on ZF-Commons:master.

svycka

I haven't tested locally just on github, but seems good feature just dont't like that it can have big performance change. Also I guess not everything tested, if I found a bug and tests did not.

svycka · 2016-10-28T05:12:56Z

src/ZfcRbac/Service/AuthorizationService.php


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


remove else, should be:

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

svycka · 2016-10-28T05:24:04Z

src/ZfcRbac/Assertion/AssertionSet.php

+     * @param  mixed                $context
+     * @return bool
+     */
+    public function assert(AuthorizationService $authorizationService, $context = null)


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

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

Better to have defensive defaults.

Good point. Thanx.

svycka · 2016-10-28T05:25:26Z

src/ZfcRbac/Service/AuthorizationService.php

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


remove else, this should be:

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

svycka · 2016-10-28T05:49:05Z

src/ZfcRbac/Assertion/AssertionSet.php

+ * @licence MIT
+ */
+
+class AssertionSet implements AssertionInterface, \IteratorAggregate


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?

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).

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?

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

svycka · 2016-10-28T05:55:15Z

src/ZfcRbac/Assertion/AssertionSet.php

+     *
+     * @return bool true if the assertion is defined, false otherwise
+     */
+    public function hasAssertion($name)


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

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

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?

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 ] ] ];

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.

+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.

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

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

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

@svycka great, I am working on it.

svycka · 2016-10-28T06:18:38Z

src/ZfcRbac/Service/AuthorizationService.php

+            // 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);


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

svycka · 2016-10-28T06:20:49Z

src/ZfcRbac/Service/AuthorizationService.php

@@ -88,7 +112,10 @@ public function setAssertion($permission, $assertion)
     */
    public function setAssertions(array $assertions)
    {
-        $this->assertions = $assertions;


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

svycka · 2016-10-28T06:26:26Z

src/ZfcRbac/Service/AuthorizationService.php

-        } elseif (is_string($assertion)) {
-            $assertion = $this->assertionPluginManager->get($assertion);
-
-            return $assertion->assert($this, $context);
        }


maybe add this:

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

see my above comments why

svycka · 2016-10-28T06:29:16Z

src/ZfcRbac/Assertion/AssertionSet.php

+     */
+    public function assert(AuthorizationService $authorizationService, $context = null)
+    {
+        if (self::CONDITION_AND === $this->condition) {


if this class not a final then maybe AssertionSet::CONDITION_AND

svycka · 2016-10-28T06:29:28Z

src/ZfcRbac/Assertion/AssertionSet.php

+            return true;
+        }
+
+        if (self::CONDITION_OR === $this->condition) {


if this class not a final then maybe AssertionSet::CONDITION_OR

svycka · 2016-10-28T09:01:11Z

src/ZfcRbac/Assertion/AssertionSet.php

+    /**
+     * @var $condition string
+     */
+    protected $condition = 'AND';


AssertionSet::CONDITION_AND instead of AND

coveralls · 2016-10-28T10:04:59Z

Coverage increased (+0.7%) to 93.535% when pulling c695e41 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-28T10:04:59Z

Coverage increased (+0.7%) to 93.535% when pulling c695e41 on DavidHavl:master into 7344374 on ZF-Commons:master.

danizord

Just left some suggestions

danizord · 2016-10-28T11:54:11Z

src/ZfcRbac/Assertion/AssertionSet.php

+            return false;
+        }
+
+        throw new InvalidArgumentException(sprintf(


Should probably be checked at construction time

danizord · 2016-10-28T11:55:36Z

src/ZfcRbac/Assertion/AssertionSet.php

+     * Retrieve an external iterator
+     * @return \ArrayIterator
+     */
+    public function getIterator()


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

danizord · 2016-10-28T12:12:56Z

src/ZfcRbac/Service/AuthorizationService.php

+        // if is name of the assertion, retrieve an actual instance from assertion plugin manager
+        if (is_string($assertion)) {
+            $assertion = $this->assertionPluginManager->get($assertion);
+        } elseif (is_array($assertion)) { // else if multiple assertion definition, create assertion set.


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

… assert method of AuthorizationService.

coveralls · 2016-10-29T15:48:29Z

Coverage increased (+0.4%) to 93.258% when pulling 47e17f8 on DavidHavl:master into 7344374 on ZF-Commons:master.

coveralls · 2016-10-29T15:48:29Z

Coverage increased (+0.4%) to 93.258% when pulling 47e17f8 on DavidHavl:master into 7344374 on ZF-Commons:master.

DavidHavl · 2016-10-29T15:48:30Z

@danizord the approach you describe is interesting, but I have a concern from usability point of view. I mean, I think it may be more confusing and less intuitive for people wanting to use the module to have yet another separate config for sets. Plus I am not sure about performance implications either.
What do you think?

DavidHavl

Recoded with most of the suggestions and changes implemented. Please review and let me know if there is anything else that is an issue.
Thank you.

DavidHavl · 2016-10-29T21:43:14Z

src/ZfcRbac/Assertion/AssertionSet.php

+     * @param  mixed                $context
+     * @return bool
+     */
+    public function assert(AuthorizationService $authorizationService, $context = null)


DavidHavl · 2016-10-29T21:46:17Z

src/ZfcRbac/Service/AuthorizationService.php

@@ -150,10 +177,6 @@ protected function assert($assertion, $context = null)
            return $assertion($this, $context);
        } elseif ($assertion instanceof AssertionInterface) {
            return $assertion->assert($this, $context);
-        } elseif (is_string($assertion)) {


This is now back, you are right, it makes it more performant to have the retrieval only when it's called/needed.

svycka · 2016-10-30T08:19:03Z

First yes you moved AssertionSet creation to assert() but this still not optimal you still creating all assertions even if they are not required. Imagine you have 20 assertions with or condition so you have to create all 20 when in reality you should have created just first one and all others are not required.

I don't like that AssertionSet does not support assertions as string or callback while supported are string|callable|array|AssertionInterface and only allows AssertionInterface at the same time does not check if $assertion is AssertionInterface. Also do we really need names for assertions in config?

And I still would like to change AuthorizatrionService::assert() to something like:

protected function assert($assertion, $context = null)
{
    $assertion = new AssertionSet($assertion, $this->assertionPluginManager);
    return $assertion->assert($this, $context);
}

this is also AssertionSet and if I understand correctly should work no? haven't tested

return [
    'zfc_rbac' => [
        'assertion_map' => [
             // single assertion
            'myPermission'  => 'myAssertion',
            // assertion set with default condition `and`
            'myPermission2' => [ 
                'myAssertion',
                'myAssertion2',
            ]
        ]
    ]
];

or if you don't like adding AssertionPluginManager to AssertionSet then maybe introduce new method in AuthorizationService:

public function getAssertion(string $assertion)
{
    return $this->assertionPluginManager->get($assertion);
}

DavidHavl · 2016-10-31T00:32:27Z

@svycka I see your point in case of OR condition where it is redundant to create rest of the assertions.
However, the simplified AuthorizatrionService::assert() method you propose has a flow. It would have to create an instance of AssertionSet every time even for single standalone assertions or callables or named assertion which is redundant.

svycka · 2016-10-31T05:46:25Z

It is not just or condition with and this also valid if first fail all
others does not mater and will not be created. Also if you have concerns
about creating this object then ok you can create it if assertion is array.
But if everyone agrees with you then ok let's leave it this way.

On Oct 31, 2016 02:32, "David Havl" notifications@github.com wrote:

@svycka https://github.com/svycka I see your point in case of OR
condition where it is redundant to create rest of the assertions.
However, the simplified AuthorizatrionService::assert() method you
propose has a flow. It would have to create an instance of AssertionSet
every time even for single standalone assertions or callables or named
assertion which is redundant.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#320 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABNj_p69g-dUeeEEuPfUqI0_BxKzQiosks5q5TcdgaJpZM4G-YHO
.

DavidHavl · 2017-01-02T15:36:11Z

Hey guys, I don't think I will have enough time to do more changes on this in next few months (got an urgent project that doesn't use this), so feel free to do a pull
and then if you think it needs more refinements feel free to adjust it.

prolic · 2018-02-20T16:15:19Z

src/ZfcRbac/Assertion/AssertionSet.php

+ * @author  David Havl
+ * @licence MIT
+ */
+class AssertionSet implements AssertionInterface


let's mark final

basz · 2018-02-22T08:00:05Z

the author expressed he didn't have time to continue it. We now have this functionality in the develop branch and #379 could be backported to master if anyone needs it. (hence the label change)

DavidHavl · 2018-02-22T11:21:33Z

Great, thanks for finishing it up @basz ! I indeed was not able to work on it as much as I would like to any more and it was good enough for the project I was working on at that time.
I am glad some of it was helpful.

Added ability to set multiple assertions and it's condition for permi…

7d5795e

…ssions.

danizord added the awaiting author updates label Jul 13, 2016

DavidHavl added 2 commits October 25, 2016 15:54

Recoded the ability to set multiple assertions and it's condition for…

9b5d507

… permissions to use new AssertionSet.

Merge remote-tracking branch 'remotes/upstream/master'

01e35ba

Added PSR2 new line to end of file

b06275f

basz reviewed Oct 25, 2016

View reviewed changes

empty lines are back

0a3e9a7

Ability to define multiple assertions in assertion map simpler way.

d2a7e27

PSR2 compliance fix

8c6a9c3

PSR2 compliance fix

311b942

Adjusted unit tests

d9a86ad

Adjusted unit tests

1a30f06

svycka suggested changes Oct 28, 2016

View reviewed changes

svycka reviewed Oct 28, 2016

View reviewed changes

Added check for valid condition when asserting

c695e41

danizord reviewed Oct 28, 2016

View reviewed changes

Rewrite of AssertionSet and moved dynamic creation of AssertionSet to…

47e17f8

… assert method of AuthorizationService.

DavidHavl commented Oct 29, 2016

View reviewed changes

This was referenced Feb 18, 2018

Multiple assertions develop #379

Merged

Future #367

Open

prolic reviewed Feb 20, 2018

View reviewed changes

basz added low hanging fruit and removed awaiting author updates labels Feb 22, 2018

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

Are you sure you want to change the base?

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

Conversation

DavidHavl commented Jan 4, 2016

bakura10 commented Jan 4, 2016

DavidHavl commented Jan 5, 2016

lorenzoferrarajr commented Mar 7, 2016

basz commented Sep 4, 2016

DavidHavl commented Sep 13, 2016

prolic commented Oct 24, 2016

coveralls commented Oct 25, 2016

coveralls commented Oct 25, 2016

coveralls commented Oct 25, 2016

coveralls commented Oct 25, 2016

basz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic commented Oct 25, 2016

DavidHavl commented Oct 25, 2016 • edited

basz commented Oct 25, 2016 • edited

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016 • edited

coveralls commented Oct 28, 2016 • edited

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016

svycka left a comment

Choose a reason for hiding this comment

svycka Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svycka Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidHavl Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 28, 2016

coveralls commented Oct 28, 2016

danizord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 29, 2016

coveralls commented Oct 29, 2016

DavidHavl commented Oct 29, 2016 • edited

DavidHavl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svycka commented Oct 30, 2016

DavidHavl commented Oct 31, 2016

svycka commented Oct 31, 2016

DavidHavl commented Jan 2, 2017

Choose a reason for hiding this comment

basz commented Feb 22, 2018

DavidHavl commented Feb 22, 2018 • edited

DavidHavl commented Oct 25, 2016 •

edited

basz commented Oct 25, 2016 •

edited

coveralls commented Oct 28, 2016 •

edited

coveralls commented Oct 28, 2016 •

edited

svycka Oct 28, 2016 •

edited

svycka Oct 28, 2016 •

edited

DavidHavl Oct 28, 2016 •

edited

DavidHavl commented Oct 29, 2016 •

edited

DavidHavl commented Feb 22, 2018 •

edited