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
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7d5795e
Added ability to set multiple assertions and it's condition for permi…
DavidHavl Jan 4, 2016
9b5d507
Recoded the ability to set multiple assertions and it's condition for…
DavidHavl Oct 25, 2016
01e35ba
Merge remote-tracking branch 'remotes/upstream/master'
DavidHavl Oct 25, 2016
b06275f
Added PSR2 new line to end of file
DavidHavl Oct 25, 2016
0a3e9a7
empty lines are back
DavidHavl Oct 25, 2016
d2a7e27
Ability to define multiple assertions in assertion map simpler way.
DavidHavl Oct 28, 2016
8c6a9c3
PSR2 compliance fix
DavidHavl Oct 28, 2016
311b942
PSR2 compliance fix
DavidHavl Oct 28, 2016
d9a86ad
Adjusted unit tests
DavidHavl Oct 28, 2016
1a30f06
Adjusted unit tests
DavidHavl Oct 28, 2016
c695e41
Added check for valid condition when asserting
DavidHavl Oct 28, 2016
47e17f8
Rewrite of AssertionSet and moved dynamic creation of AssertionSet to…
DavidHavl Oct 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/ZfcRbac/Assertion/AssertionSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,5 @@ public function assert(AuthorizationService $authorizationService, $context = nu

return false;
}

}
}
6 changes: 3 additions & 3 deletions src/ZfcRbac/Service/AuthorizationService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,17 @@ 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 •
edited
Loading

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);
} else if (is_array($assertion)) { // else if multiple assertion definition, create assertion set.
} 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 •
edited
Loading

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;
} else if (!is_array($assertion['assertions'])) {
} elseif (!is_array($assertion['assertions'])) {
Copy link
Contributor

@svycka svycka Oct 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

remove else, should be:

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

Copy link
Contributor

@svycka svycka Oct 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

remove else, this should be:

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

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

// retrieve an actual instance from assertion plugin manager if necessary
foreach ($assertion['assertions'] as $key=>$value) {
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()

}
Expand Down
32 changes: 16 additions & 16 deletions tests/ZfcRbacTest/Service/AuthorizationServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,10 @@ public function testAssertionMapWithSingleAssertions()
$rbac = $this->getMock('Rbac\Rbac', [], [], '', false);
$roleService = $this->getMock('ZfcRbac\Service\RoleService', [], [], '', false);
$assertionPluginManager = $this->getMock('ZfcRbac\Assertion\AssertionPluginManager', [], [], '', false);
$assertionObject = $this->getMock('ZfcRbac\Assertion\AssertionInterface');
$assertionPluginManager->method('get')->will($this->returnValueMap([
['bar', null, $assertionObject],
['foo', null, $assertionObject]
$assertion = new SimpleAssertion();
$assertionPluginManager->expects($this->any())->method('get')->will($this->returnValueMap([
['bar', null, $assertion],
['foo', null, $assertion]
]));
$authorizationService = new AuthorizationService($rbac, $roleService, $assertionPluginManager);

Expand All @@ -259,10 +259,10 @@ public function testAssertionMapWithMultipleSimpleAssertions()
$rbac = $this->getMock('Rbac\Rbac', [], [], '', false);
$roleService = $this->getMock('ZfcRbac\Service\RoleService', [], [], '', false);
$assertionPluginManager = $this->getMock('ZfcRbac\Assertion\AssertionPluginManager', [], [], '', false);
$assertionObject = $this->getMock('ZfcRbac\Assertion\AssertionInterface');
$assertionPluginManager->method('get')->will($this->returnValueMap([
['assertion1', null, $assertionObject],
['assertion2', null, $assertionObject]
$assertion = new SimpleAssertion();
$assertionPluginManager->expects($this->any())->method('get')->will($this->returnValueMap([
['assertion1', null, $assertion],
['assertion2', null, $assertion]
]));
$authorizationService = new AuthorizationService($rbac, $roleService, $assertionPluginManager);

Expand All @@ -280,10 +280,10 @@ public function testAssertionMapWithMultipleAssertions()
$rbac = $this->getMock('Rbac\Rbac', [], [], '', false);
$roleService = $this->getMock('ZfcRbac\Service\RoleService', [], [], '', false);
$assertionPluginManager = $this->getMock('ZfcRbac\Assertion\AssertionPluginManager', [], [], '', false);
$assertionObject = $this->getMock('ZfcRbac\Assertion\AssertionInterface');
$assertionPluginManager->method('get')->will($this->returnValueMap([
['assertion1', null, $assertionObject],
['assertion2', null, $assertionObject]
$assertion = new SimpleAssertion();
$assertionPluginManager->expects($this->any())->method('get')->will($this->returnValueMap([
['assertion1', null, $assertion],
['assertion2', null, $assertion]
]));
$authorizationService = new AuthorizationService($rbac, $roleService, $assertionPluginManager);

Expand All @@ -302,10 +302,10 @@ public function testAssertionMapWithMultipleAssertionsWithCondition()
$rbac = $this->getMock('Rbac\Rbac', [], [], '', false);
$roleService = $this->getMock('ZfcRbac\Service\RoleService', [], [], '', false);
$assertionPluginManager = $this->getMock('ZfcRbac\Assertion\AssertionPluginManager', [], [], '', false);
$assertionObject = $this->getMock('ZfcRbac\Assertion\AssertionInterface');
$assertionPluginManager->method('get')->will($this->returnValueMap([
['assertion1', null, $assertionObject],
['assertion2', null, $assertionObject]
$assertion = new SimpleAssertion();
$assertionPluginManager->expects($this->any())->method('get')->will($this->returnValueMap([
['assertion1', null, $assertion],
['assertion2', null, $assertion]
]));
$authorizationService = new AuthorizationService($rbac, $roleService, $assertionPluginManager);

Expand Down