Added ability to set multiple assertions and their condition for permissions

[DavidHavl]

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

[bakura10]

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]

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

[lorenzoferrarajr]

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

[basz]

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

[DavidHavl]

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

[prolic]

Is there still progress on this?

[coveralls]

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

[coveralls]

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

[coveralls]

Coverage decreased (-4.9%) to 87.96% when pulling b06275fb99e2366d0c6d61e345ca192483eeb4ab on DavidHavl:master into 7344374417fe50c06ef596848c5813ed4a739ac3 on ZF-Commons:master.

[coveralls]

Coverage decreased (-4.9%) to 87.96% when pulling b06275fb99e2366d0c6d61e345ca192483eeb4ab on DavidHavl:master into 7344374417fe50c06ef596848c5813ed4a739ac3 on ZF-Commons:master.

[prolic]

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

[DavidHavl]

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

[basz]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[DavidHavl]

@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?

[svycka]

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]

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

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 https://github.com/ZF-Commons/zfc-rbac/pull/320#issuecomment-257191928, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNj_p69g-dUeeEEuPfUqI0_BxKzQiosks5q5TcdgaJpZM4G-YHO .

[DavidHavl]

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.

[basz]

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]

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.