-
Notifications
You must be signed in to change notification settings - Fork 111
Added ability to set multiple assertions and their condition for permissions #320
base: master
Are you sure you want to change the base?
Changes from 4 commits
7d5795e
9b5d507
01e35ba
b06275f
0a3e9a7
d2a7e27
8c6a9c3
311b942
d9a86ad
1a30f06
c695e41
47e17f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,183 @@ | ||
<?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 | ||
{ | ||
/** | ||
* Condition constants | ||
*/ | ||
const CONDITION_OR = 'OR'; | ||
const CONDITION_AND = 'AND'; | ||
|
||
/** | ||
* @var $assertions array | ||
*/ | ||
protected $assertions = []; | ||
|
||
/** | ||
* @var $condition string | ||
*/ | ||
protected $condition = 'AND'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
/** | ||
* Constructor. | ||
* | ||
* @param AssertionInterface[] $assertions An array of assertions. | ||
*/ | ||
public function __construct(array $assertions = array()) | ||
{ | ||
foreach ($assertions as $name => $assertion) { | ||
$this->setAssertion($assertion, is_int($name) ? null : $name); | ||
} | ||
} | ||
|
||
/** | ||
* Set assertions. | ||
* | ||
* @param AssertionInterface[] $assertions The assertions to set | ||
* | ||
* @return $this | ||
*/ | ||
public function setAssertions($assertions) | ||
{ | ||
foreach ($assertions as $name => $assertion) { | ||
$this->setAssertion($assertion, is_int($name) ? null : $name); | ||
} | ||
return $this; | ||
} | ||
|
||
/** | ||
* Set an assertion. | ||
* | ||
* @param AssertionInterface $assertion The assertion instance | ||
* | ||
* @param string $name A name/alias | ||
* | ||
* @return $this | ||
*/ | ||
public function setAssertion(AssertionInterface $assertion, $name = null) | ||
{ | ||
if (null !== $name) { | ||
$this->assertions[$name] = $assertion; | ||
} | ||
$this->assertions[] = $assertion; | ||
return $this; | ||
} | ||
|
||
/** | ||
* 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure but do we use this somewhere outside this class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes people can when they use the class directly (see docs). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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
]
]
]; There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. That is a good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
]); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
} | ||
|
||
/** | ||
* @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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Thanx. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
{ | ||
if (self::CONDITION_AND === $this->condition) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this class not a final then maybe |
||
foreach ($this->assertions as $assertion) { | ||
if (!$assertion->assert($authorizationService, $context)) { | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
if (self::CONDITION_OR === $this->condition) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this class not a final then maybe |
||
foreach ($this->assertions as $assertion) { | ||
if ($assertion->assert($authorizationService, $context)) { | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
throw new InvalidArgumentException(sprintf( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,19 +122,15 @@ public function getIdentity() | |
public function isGranted($permission, $context = null) | ||
{ | ||
$roles = $this->roleService->getIdentityRoles(); | ||
|
||
if (empty($roles)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to remove these empty lines |
||
return false; | ||
} | ||
|
||
if (!$this->rbac->isGranted($roles, $permission)) { | ||
return false; | ||
} | ||
|
||
if ($this->hasAssertion($permission)) { | ||
return $this->assert($this->assertions[(string) $permission], $context); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
<?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 ZfcRbacTest\Asset; | ||
|
||
use ZfcRbac\Assertion\AssertionInterface; | ||
use ZfcRbac\Service\AuthorizationService; | ||
|
||
class SimpleFalseAssertion implements AssertionInterface | ||
{ | ||
/** | ||
* @var bool | ||
*/ | ||
protected $called = false; | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function assert(AuthorizationService $authorization, $context = null) | ||
{ | ||
$this->called = true; | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* @return bool | ||
*/ | ||
public function getCalled() | ||
{ | ||
return $this->called; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
<?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 ZfcRbacTest\Asset; | ||
|
||
use ZfcRbac\Assertion\AssertionInterface; | ||
use ZfcRbac\Service\AuthorizationService; | ||
|
||
class SimpleTrueAssertion implements AssertionInterface | ||
{ | ||
/** | ||
* @var bool | ||
*/ | ||
protected $called = false; | ||
|
||
/** | ||
* {@inheritDoc} | ||
*/ | ||
public function assert(AuthorizationService $authorization, $context = null) | ||
{ | ||
$this->called = true; | ||
|
||
return true; | ||
} | ||
|
||
/** | ||
* @return bool | ||
*/ | ||
public function getCalled() | ||
{ | ||
return $this->called; | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.