Guards are not really aware of permissions

[jmleroux]

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]

ping @danizord , @ocramius ?

[danizord]

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

• 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]

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]

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]

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

[bakura10]

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]

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]

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]

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]

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]

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]

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]

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

[danizord]

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

I'm :+1: to ControllerRoleGuard, ControllerPermissionGuard, [...]

[jmleroux]

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]

:+1: 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]

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

[davidwindell]

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]

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]

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

[jmleroux]

Closed by #238