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

Guards are not really aware of permissions #182

Closed
jmleroux opened this issue Jan 31, 2014 · 21 comments
Closed

Guards are not really aware of permissions #182

jmleroux opened this issue Jan 31, 2014 · 21 comments

Comments

@jmleroux
Copy link
Contributor

Guards are not really aware of permissions (it does not make any sense) but rather only think about "roles".

Permissions are often the cornerstone of the previous authorzation systems i made with ZfcRbac.
At first glance, i thought that Guards "role only aware" would be fine, but finally I have a doubt.

I build an application with a classic Rbac system (even without role hierachy).
The administrators must be able to create roles and set permissions on this roles.
Current ZfcRbac can't handle the new roles out of the box, because roles are hard-coded in Guards configuration.

In old ZfcRbac (<1.0), i could set a route firewall with a permission condition. If a new role is created, it don't breaks the permission system.

With current ZfcRbac, roles AND permissions are doomed to be immutable.

@bakura10 and others, what is your opinion on this ?

@jmleroux
Copy link
Contributor Author

ping @danizord , @Ocramius ?

@danizord
Copy link
Member

Here's my opinion on this one: Guards aware of roles is that does not make any sense to me. 😄

  • Permissions are meant to be hardcoded
  • Roles are not meant to be hardcoded
  • All authorization stuff should be done in AuthorizationService
  • 2 authorization models in same namespace makes things hard to understand
  • Developers will check authorization twice anyway (guards and service layer), so why we don't use same config for guards and service? (avoid having 2 places to config/mantain)

However, I know that the current implementation has some benefits

  • Allow admin/* routes only to admin role :)
  • Some people use guards and guest role stuff to check authentication (bad design detected)
  • Some small sites don't need permissions at all, so they use only guards to do a simple authorization.

Finally, I think we can mantain the 2 approaches (role guards and permission guards).

@jmleroux
Copy link
Contributor Author

Allow admin/* routes only to admin role :)
Some people use guards and guest role stuff to check authentication (bad design detected)

We should keep the capability to check role (for roles that are read-only, as admin is quite sure to be) but we must permit Guards to check against permissions.

@aeneasr
Copy link
Contributor

aeneasr commented Feb 1, 2014

Yeah I'm missing the permission check in guards as well - that makes them actually quite unusable for me. However it should be considered, that assertions won't work with guards and probably will throw an exception, if one checks a permission from a guard without context

@aeneasr
Copy link
Contributor

aeneasr commented Feb 1, 2014

However, it is pretty easy to implement your own guard, that is aware of permissions and add it to the guardpm!

@bakura10
Copy link
Member

bakura10 commented Feb 2, 2014

The fact that a guard could be like "don't let any role that has the permission foo" is not really a problem to me. However I feel it really wrong about what we had before, that both roles AND permissions were evaluated.

Maybe we could have a RoutePermissionGuard/ControllerPermissionGuard and RouteRoleGuard/ControllerRoleGuard. Thoughts?

@jmleroux
Copy link
Contributor Author

jmleroux commented Feb 2, 2014

In fact I don't think so. How will you handle a route where you need the edit permission or admin role ?
For me, you should be able to specify a permission OR a role in the guard configuration.

@jmleroux
Copy link
Contributor Author

jmleroux commented Feb 2, 2014

However I feel it really wrong about what we had before, that both roles AND permissions were evaluated

Say we can specify role and/or permission configuration. We should be able to optimize the test against role or permission.

@bakura10
Copy link
Member

bakura10 commented Feb 2, 2014

Okey, so basically, if we have no permissions set in the guards config, it uses the current mechanism, otherwise it do a full call to "isGranted" ?

Please make a PR if you feel so. However I will be super strict regarding BC! :)

@jmleroux
Copy link
Contributor Author

jmleroux commented Feb 2, 2014

Okey, so basically, if we have no permissions set in the guards config, it uses the current mechanism, otherwise it do a full call to "isGranted" ?

almost... Does it work for "edit permission OR admin" ? => i'll have both permission and role in config.

@bakura10
Copy link
Member

bakura10 commented Feb 2, 2014

Mmhh this is pretty complicated if we need to take into account AND, OR... It would complicate quite a bit the guards implementation:

'ZfcRbac\Guard\RouteGuard' => [
   'roles' => 'admin',
   'permissions' => 'edit',
   'expr' => RouteGuard::AND
]

Something like that. But I really not sure if I want guards to be so complex. Once again my fear with that is that people will tend to use only guards then :(.

@jmleroux
Copy link
Contributor Author

jmleroux commented Feb 2, 2014

Just OR : this is the simplest and most frequent.
I want to keep it simple and stupid too. But usable... ;)
A guard like "edit or admin" is a classic.

@bakura10
Copy link
Member

bakura10 commented Feb 2, 2014

So try to do it and let's see the impl :).

@danizord
Copy link
Member

danizord commented Feb 3, 2014

That feels wrong to me. Just assign edit to admin and it should work.

I'm 👍 to ControllerRoleGuard, ControllerPermissionGuard, [...]

@jmleroux
Copy link
Contributor Author

jmleroux commented Feb 3, 2014

almost... Does it work for "edit permission OR admin" ? => i'll have both permission and role in config.

Maybe the solution is to add a RouteRoleGuard for admin before a RoutePermissionGuard if we can control the order of guards tests

@davidwindell
Copy link
Contributor

👍 I'm just upgrading from 0.2.3 and can't without this feature. We have a PROJECT_CREATE permission that should also apply to the project/create route.

@bakura10
Copy link
Member

I'm still a bit reluctant to include permissions into the guard logic. iirc someone (@jmleroux) needed to give it a try.

@davidwindell
Copy link
Contributor

Here's just a small snippet of our 0.2 setup, how else can we achieve this without permission support (besides a custom guard of course);

array('route' => 'app/contact/create', 'permissions' => array(Permission::CONTACT_EDIT)),
array('route' => 'app/contact/*', 'permissions' => array(Permission::CONTACT_VIEW_ALL)),
array('route' => 'app/invoice/create', 'permissions' => array(Permission::INVOICE_EDIT)),
array('route' => 'app/invoice/*', 'permissions' => array(Permission::INVOICE_VIEW_ALL)),
array('route' => 'app/api/contact-note', 'roles' => array(Role::ROLE_USER)),

@bakura10
Copy link
Member

Can't you role have the permissions instead ?

Anyway... seing the config like this, maybe it makes sense... so if you want to try a PR, go on: ).

@davidwindell
Copy link
Contributor

It's probably worth mentioning that users create their own roles in our app with any number of different permissions.

@jmleroux
Copy link
Contributor Author

Closed by #238

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants