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

add permissions capacity to RouteGuard and ControllerGuard #243

Closed
wants to merge 5 commits into from

Conversation

jmleroux
Copy link
Contributor

Another way to manage permissions checks for RouteGuard as an alternative to #238

Discussion is open. ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.24%) when pulling d580bff on jmleroux:permissions-guards into 397c676 on ZF-Commons:master.

@jmleroux jmleroux changed the title add permissions capacity to RouteGuard add permissions capacity to RouteGuard and ControllerGuard Jun 16, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 2819ed0 on jmleroux:permissions-guards into 397c676 on ZF-Commons:master.

@jmleroux
Copy link
Contributor Author

I prefer this way for checking permissions.

@arekkas, @danizord, @bakura10, @Ocramius ?

return $this->protectionPolicy === self::POLICY_ALLOW;
}

if (in_array('*', (array)$allowedPermissions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear to me, that 'roles' => 'foo' override (AND operation) 'permission' => 'bar' you would actually need a setting for that. Take this config for example:

'My\Controller' => [
    'roles' => ['admin'],
    'permission' => ['mayAccess']
]

I think it is preferable to do something like this:

'My\Controller' => [
    'roles' => ['admin'],
    'permission' => ['mayAccess'],
    'operator' => GuardInterface::OR_OPERATOR
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like discussed in #238, it will be another PR.
I keep it simple first.

@aeneasr
Copy link
Contributor

aeneasr commented Jun 16, 2014

@bakura10 and me hat a discussion some while back where we talked about this exact thing. He pointed out, that Guards should not be the single point of authorization but rather an additional security measure in your logic. Allowing programmers to add permissions could result in the neglect of implementing security measures in their respective services.

However it seems as if #238 changed his mind. I think checking for permissions in controllers is a very convenient feature and I do approve of that.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.23%) when pulling 2b40a1b on jmleroux:permissions-guards into 397c676 on ZF-Commons:master.

@jmleroux
Copy link
Contributor Author

For me, not being able to check for permissions in route guards make it impossible to use ZfcRbac 2.x in lots of applications. I kept stuck with 0.2.
And i think i'm not the only one...

@coveralls
Copy link

Coverage Status

Coverage increased (+0.23%) when pulling 68d4dce on jmleroux:permissions-guards into 397c676 on ZF-Commons:master.

@davidwindell
Copy link
Contributor

@jmleroux so I don't have to look through the commits, can you summarise what is different between this and #238?

@jmleroux
Copy link
Contributor Author

@davidwindell
#238 uses a dedicated RoutePermissionsGuard. This PR add the permissions capabilities directly to RouteGuard.

@davidwindell
Copy link
Contributor

@jmleroux how does this work if you have a ROLE and a PERMISSION, is it an or between these just as multiple roles/permissions would be an OR (for now)?

@jmleroux
Copy link
Contributor Author

For now, if there is a 'roles' key in configuration, then access is verified against roles. Permissions will not be tested.
Maybe i will throw an exception if both keys are present.
I don't think it's a good idea to mix roles and permissions. Do you have an example for mxing the two ?

@davidwindell
Copy link
Contributor

I think it would be better kept separate tbh as in #238, it would make configuration files cleaner when there are lots of routes.

@jmleroux
Copy link
Contributor Author

I think it would be better kept separate tbh as in #238, it would make configuration files cleaner when there are lots of routes.

It's a choice.
For my part, I have not yet chosen. 😄

Others ?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) when pulling 74e84a7 on jmleroux:permissions-guards into 397c676 on ZF-Commons:master.

@aeneasr
Copy link
Contributor

aeneasr commented Jun 16, 2014

I think it would be better kept separate tbh as in #238, it would make configuration files cleaner when there are lots of routes.

agreed!

@jmleroux
Copy link
Contributor Author

AAArrrggghhh :)

So this PR is unnecessary. 😬 ?

@jmleroux
Copy link
Contributor Author

Closing this one, as it seems the separation between guards is the preferred one.

@jmleroux jmleroux closed this Jun 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants