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

Conditions for Guards #268

Merged
merged 8 commits into from
Mar 31, 2015
Merged

Conditions for Guards #268

merged 8 commits into from
Mar 31, 2015

Conversation

davidwindell
Copy link
Contributor

See #262.

So far this only applies to the RoutePermissionGuard as a proof of concept.

It's a shame that this guard specifically was created with an AND default whereas an OR would have made more sense (like the RouteGuard).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 1eae206 on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 1c55f88 on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

$condition = isset($allowedPermissions['condition']) ? $allowedPermissions['condition'] : GuardInterface::CONDITION_AND;

foreach ($permissions as $permission) {
if ($condition == GuardInterface::CONDITION_OR && $this->authorizationService->isGranted($permission)) {
Copy link
Member

Choose a reason for hiding this comment

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

Strict comparison please (===)

Copy link
Member

Choose a reason for hiding this comment

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

By the way, you are doing the check against condition inside the loop. I think the check can be move out the foreach.

@davidwindell
Copy link
Contributor Author

Ok, those are done - sorry about the delay.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling be64fe5 on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@bakura10
Copy link
Member

Tests do not pass, there are three small CS fixes (https://travis-ci.org/ZF-Commons/zfc-rbac/jobs/38596755).

Also, I'll ask you to update the doc too: https://github.com/ZF-Commons/zfc-rbac/blob/master/docs/04.%20Guards.md#routeguard

Once everything is done, I'll merge and tag ;).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.21%) when pulling 29b754f on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@davidwindell
Copy link
Contributor Author

There you go sir!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling c4e629d on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@davidwindell
Copy link
Contributor Author

@bakura10 Just noticed this one - can you get it merged now?

@@ -177,17 +177,34 @@ return [
'guards' => [
'ZfcRbac\Guard\RoutePermissionsGuard' => [
'admin*' => ['admin'],
'post/manage' => ['post.update', 'post.delete']
'post/manage' =>
Copy link
Member

Choose a reason for hiding this comment

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

Doc is wrong here :p.

@bakura10
Copy link
Member

Hi,

Sorry about this, we should definitely start to merge it :p. @danizord could you have your own quick review please ?

@davidwindell
Copy link
Contributor Author

Great - we have been using in production for some time now. Works grand.

@bakura10
Copy link
Member

Ok, I'm going to merge than Monday if nothing complain about it. Can you remind me if I forget?

@@ -326,6 +326,26 @@ public function routeDataProvider()
'isGranted' => true,
'policy' => GuardInterface::POLICY_DENY
],
[
'rules' => ['route' => [
Copy link
Member

Choose a reason for hiding this comment

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

No need to align when the value is array

@danizord
Copy link
Member

I think you can simplify the logic to something like that:

if (GuardInterface::CONDITION_AND === $condition) {
    foreach ($permissions as $permission) {
        if (!$this->authorizationService->isGranted($permission)) {
            return false;
        }
    }

    return true;
}    

if (GuardInterface::CONDITION_OR === $condition) {
    foreach ($permissions as $permission) {
        if ($this->authorizationService->isGranted($permission)) {
            return true;
        }
    }

    return false;
}    

throw new InvalidArgumentException(sprintf(
    'Condition must be either "AND" or "OR", %s given',
    is_object($condition) ? get_class($condition) : gettype($condition)
));

@bakura10
Copy link
Member

I agree with @danizord change, more efficient and way easier to read.

@danizord danizord added this to the 2.5 milestone Mar 27, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.82%) to 93.27% when pulling 41ce33f on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@bakura10
Copy link
Member

Thanks!

Just a few more CS fixes to make travis happy: https://travis-ci.org/ZF-Commons/zfc-rbac/jobs/56477072

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.82%) to 93.27% when pulling f56b0b8 on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.82%) to 93.27% when pulling f56b0b8 on davidwindell:issue-262 into 02a5593 on ZF-Commons:master.

@davidwindell
Copy link
Contributor Author

👍

@davidwindell davidwindell changed the title Conditions for Guards [WIP] Conditions for Guards Mar 30, 2015
bakura10 added a commit that referenced this pull request Mar 31, 2015
@bakura10 bakura10 merged commit e89cba0 into ZF-Commons:master Mar 31, 2015
@bakura10
Copy link
Member

Thanks! I've tagged ZfcRbac as 2.5.0 :).

Hopefully someone will be able to backport this feature for other guards !

@davidwindell davidwindell deleted the issue-262 branch March 31, 2015 09:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants