Skip to content

[ZF3] Rbac component cleaning #5231

Closed
wants to merge 18 commits into from

7 participants

@bakura10
bakura10 commented Oct 5, 2013

ping @spiffyjr

Another PR for ZF3 as I have lot of time today :) !

This one could have been included in ZF2 but I think it would make a very very small BC so here it is. This component is pretty small and has nearly not evolved in ZF2 as it is nearly perfect (nice work @spiffyjr!):

  • Rbac class no longer extends AbstractIterator. In fact while workign on my InputFilter refacotr, I realized that implementing IteratorAggregate is MUCH faster than implementing all the recursive iterator methods (because the C code must call a lot of userland code, while when returning a RecursiveArrayIterator, all is done in C). Furthermore, it does not make a lot of sense for Rbac component to implement RecursiveIterator (in fact, the roles are iterated).
  • Roles no longer implement RecursiveIteraotr, but only the simpler IteratorAggregate that returns a RecursiveArrayIterator.
  • Variable "children" in rbac was renamed to "roles" to better inforce what it does.

Side note: it's not useful to create custom RecursiveIterator implementation unless you have specific and custom needs for the getChildren class.

All tests pass.

NOTE to spiffyjr: I checked your implementation, and the "setParent" method in the Role only accept a RoleInterface object, while the addChild can accept a string or a role (and create a new Role instance if it is a string). I think we should make this more coherent by either accepting string in setParent OR typehint RoleInterface for addChild. But as it is today it's a bit confusing.

Also: I don't like the fact that getRole method in Rbac throws an exception if it cannot found a role. Would not it make more sense to the getRole method returns "null" if no role can be found?

EDIT: also: shouldn't the AssertionInterface have the role and the permission in the assert method too?

@samsonasik samsonasik commented on an outdated diff Oct 5, 2013
library/Zend/Permissions/Rbac/AbstractRole.php
*/
public function addPermission($name)
{
- $this->permissions[$name] = true;
-
- return $this;
+ $this->permissions[(string)$name] = true;
@samsonasik
samsonasik added a note Oct 5, 2013

space after cast :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@spiffyjr
spiffyjr commented Oct 9, 2013

Looks pretty solid. The reason I have getRole() throwing an exception is that's what ACL does. I don't personally care one way or another which is implemented.

@bakura10
@spiffyjr

It does, but again, I have no opinion one way or another.

@texdc texdc commented on an outdated diff Nov 27, 2013
library/Zend/Permissions/Rbac/Rbac.php
}
/**
* Is a child with $name registered?
*
- * @param \Zend\Permissions\Rbac\RoleInterface|string $objectOrName
+ * @param RoleInterface|string $objectOrName
* @return bool
*/
public function hasRole($objectOrName)
@texdc
texdc added a note Nov 27, 2013

I prefer a more explicit interface:

public function hasRole(RoleInterface $role);
public function hasRoleName($roleName);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@texdc texdc and 1 other commented on an outdated diff Nov 27, 2013
library/Zend/Permissions/Rbac/Rbac.php
*
- * @param \Zend\Permissions\Rbac\RoleInterface|string $objectOrName
- * @return RoleInterface
+ * @param RoleInterface|string $objectOrName
+ * @return RoleInterface|null
* @throws Exception\InvalidArgumentException
*/
public function getRole($objectOrName)
@texdc
texdc added a note Nov 27, 2013

again, explicit:

public function getRole(RoleInterface $role);
public function getRoleByName($roleName);
@bakura10
bakura10 added a note Nov 27, 2013

No no no!!! This logic should be handled by the Rbac container. ZfcRbac calls this method a lot of time at different places. I should not do the test in ZfcRbac.

@texdc
texdc added a note Nov 27, 2013

It's not a test. The alternative is RoleInterface::__toString(). Ambiguity smells.

@texdc
texdc added a note Nov 27, 2013

Let's take a step back: why would we want to getRole(RoleInterface $role)? Why not just getRole($roleName) ? Or, better yet: getRole($roleId).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bakura10 bakura10 referenced this pull request in ZF-Commons/zfc-rbac Dec 11, 2013
Closed

Initial fix for broken things #114

@bakura10

ping @danizord @arekkas

I just rewrite the Rbac component completely. I'd like to have your feedback on this. It should be MAGNITUDE faster because each operation like "getRole" or "hasRole" no longer trigger a recursion.

The Rbac class has been renamed "RbacContainer" to outline more what it does: just storing role. As discussed @danizord, the RbacContainer is actually "optional" BUT it offers a nice way to store roles, do basic checking (for instance not adding the same role twice), and basic logic for isGranted with assertion. I really think we should keep it, as it's now only a simple container.

I've added a few methods to the RoleInterafce, based on @ocramius work on Service manager request. By having the __toString method on the RoleInterface and PermissionInterface, we can now simply cast everything to string, instead of having to check if instanceof RoleInterace blablabla...

@bakura10

I also removed the "AbstractRole" class and keeping only the "Role" class.

@danizord danizord and 1 other commented on an outdated diff Dec 11, 2013
library/Zend/Permissions/Rbac/RoleInterface.php
+
+ /**
+ * Add a child
+ *
+ * @param RoleInterface $child
+ * @return void
+ */
+ public function addChild(RoleInterface $child);
+
+ /**
+ * Remove a child
+ *
+ * @param RoleInterface $child
+ * @return void
+ */
+ public function removeChild(RoleInterface $child);
@danizord
danizord added a note Dec 11, 2013

This is really needed?

@bakura10
bakura10 added a note Dec 11, 2013

Same, without remove, you imply that your hierarchy is fixed at creation? This seems wrong to me.

@danizord
danizord added a note Dec 11, 2013

RoleInterface is the contract between RbacContainer and the userland Role.
RbacContainer dont need this methods. So do not force developers to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danizord danizord and 1 other commented on an outdated diff Dec 11, 2013
library/Zend/Permissions/Rbac/RoleInterface.php
*/
- public function hasPermission($name);
+ public function removePermission($permission);
@danizord
danizord added a note Dec 11, 2013

This is really needed?

@bakura10
bakura10 added a note Dec 11, 2013

I don't know, but I'm thinking of things like the AWS permission system. You have role and a bunch of permission that you can add/remove at runtime. I think remove is actually really useful.

@danizord
danizord added a note Dec 11, 2013

Interface segregation! It is useful for the AWS permission system, not for the Zend\Rbac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danizord danizord and 1 other commented on an outdated diff Dec 11, 2013
library/Zend/Permissions/Rbac/RoleInterface.php
/**
- * Add a child.
+ * Checks if a permission exists for this role or any child roles.
@danizord
danizord added a note Dec 11, 2013

This logic of recursion is the responsibility of Rbac, not of the role.

@bakura10
bakura10 added a note Dec 11, 2013

Make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@danizord danizord and 2 others commented on an outdated diff Dec 11, 2013
library/Zend/Permissions/Rbac/Role.php
+ {
+ unset($this->permissions[(string) $permission]);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function hasPermission($permission)
+ {
+ $name = (string) $permission;
+
+ if (isset($this->permissions[$name])) {
+ return true;
+ }
+
+ $iteratorIterator = new RecursiveIteratorIterator($this, RecursiveIteratorIterator::CHILD_FIRST);
@danizord
danizord added a note Dec 11, 2013

As said above, you should to move this logic out of the Role. Then a can be a value-object.

@bakura10
bakura10 added a note Dec 11, 2013

I'm not sure about this. This can lead to false negative:

If Role A has permission "foo" and is children of role B, I expect $roleB->hasPermission('foo') to return true. If I move the recursion logic to Rbac, I will have false.

@danizord
danizord added a note Dec 11, 2013

This is a lot problematic:

  • I dont want to make my MyApp\Entity\Role inherit Zend\Permissions\Rbac\Role just to handle that logic.
  • This way RbacContainer::isGranted() depends on the role implementation to work properly.
  • What if we want to cache this result? You will inject the cache component into each Role?
@bakura10
bakura10 added a note Dec 11, 2013

There is one big advantage is that we don't need to make RoleInterface extends IteratorAggregate. Having said this, my biggest problem in doing this is that "hasPermission" returning wrong result :/.

@bakura10
bakura10 added a note Dec 11, 2013

Or if we do this, we have to make it clear in the doc.

@danizord
danizord added a note Dec 11, 2013

You dont need to implement IteratorAggregate.

Simply remove the hasPermission(). :smiley:

getPermissions() and getChildren() is enough, the container can handle all the logic calling this two methods.

@bakura10
bakura10 added a note Dec 11, 2013

In fact, I think we actually need to implement the whole RecursiveIterator interface. The thing is that getChildren is not an array of array, but an array of other Role.

@arekkas
arekkas added a note Dec 11, 2013

I don't completely understand why

If Role A has permission "foo" and is children of role B, I expect $roleB->hasPermission('foo') to return true. If I move the recursion logic to Rbac, I will have false.

would fail? Couldn't the container iterate through the role's children just like the role itself does?

Because I like the idea of moving that logic into the rbaccontainer

@bakura10
bakura10 added a note Dec 11, 2013

It would fail if we move the logic of the recursive iteration to the Rbac, instead of the role.

@arekkas
arekkas added a note Dec 11, 2013

+1 on @danizord

getPermissions() and getChildren()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bakura10 bakura10 referenced this pull request in ZF-Commons/zfc-rbac Dec 11, 2013
Merged

Rewrite parts #116

@danizord

@bakura10 If the reason of having a container is preventing you to add the same role twice, then simply remove the container, then you will not need to add the role anymore. :smiley:

@bakura10

Ok, ping @arekkas @danizord , I've updated the code so now the logic is moved to the RbacContainer and not the role. However, please note that the Role HAVE to implement the RecursiveIterator. I first tried to simply implement getIterator and return a RecursiveArrayIterator but it does not work, because actually the method would return an array of objects, and that break the whole thing.

@arekkas
arekkas commented Dec 11, 2013

I haven't been working with RecursiveIteratorIterator a lot, but wouldn't it be possible to just return the RII from the role (getRecursiveIteratorIterator) so we don't have to implement RecursiveIterator?

edit:// I now see the problem with it being inconsistent

@bakura10

I think we don't have choice. RecursiveIteratorIterator work if you have a structure this way:

[
   'foo' => [
      'bar' => [
          'baz'
      ]
   ]
]

But here we have

[
    'children' => [
        object(RoleInterface)
    ]
]
@arekkas
arekkas commented Dec 11, 2013

Hm, that's really difficult, I really like the fact that you used IteratorAggregate but I also think that hasPermission should be in the rbac container.

But since you managed to decrease computing time I'd currently go with your original implementation. Thoughts?

@bakura10

Which one? Actually using IteratorAggregate. It can only work one level deep (so the immediate children, but not the grand children).

@bakura10

(implementing the RecursiveIterator is not difficult, and this is what is done in current implementation).

But current implementation ALSO have RecursiveIterator on the Rbac container!)

@arekkas
arekkas commented Dec 11, 2013

No, the decision is difficult ;)

@texdc texdc and 2 others commented on an outdated diff Dec 11, 2013
library/Zend/Permissions/Rbac/RbacContainer.php
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Permissions\Rbac;
+
+use RecursiveIteratorIterator;
+use Traversable;
+
+/**
+ * A RBAC container is just here to store roles and avoid any duplicate
+ */
+class RbacContainer
@texdc
texdc added a note Dec 11, 2013

A container of Rbacs or a container of Roles?

@danizord
danizord added a note Dec 11, 2013

RoleContainer makes sense :D

@bakura10
bakura10 added a note Dec 11, 2013
@texdc
texdc added a note Dec 11, 2013

That makes sense. But, isn't an Rbac inherently a container of roles? In other words, isn't Rbac sufficient to describe it's nature? (This might be an opportunity for a RoleContainerTrait)

To be absurd: you could go the route of RII with a RoleContainerContainerInterface.

@bakura10
bakura10 added a note Dec 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@texdc texdc commented on the diff Dec 11, 2013
library/Zend/Permissions/Rbac/Role.php
@@ -9,13 +9,168 @@
namespace Zend\Permissions\Rbac;
-class Role extends AbstractRole
+use RecursiveIterator;
+
+class Role implements RoleInterface
@texdc
texdc added a note Dec 11, 2013

I'd like to see a common Role (interface/trait) that is shared/extened by both ACL and Rbac.

@bakura10
bakura10 added a note Dec 11, 2013

I really can't see how. The concepts are completely different, it would just be a lot of work and naming trick for a completely useless purpose, just for the sake of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@arekkas
arekkas commented Dec 11, 2013

I think we should go with the new approach where the permission checking is done inside rbac (despite the downsides). What's everyone elses opinion on this?

@bakura10

After giving it more thought, I think it actually make sense. It allows cleaner and easier implementation on the Role side, and allow to efficiently filter permission (using Criteria API for Doctrine for instance).

@spiffyjr is the Rbac guy here :).

@bakura10

I still need to wait about @danizord feedback on Rbac container. He was against it. But cannot see how it could work without that :)

@arekkas
arekkas commented Dec 11, 2013

And developers can implement their own roles without having to extend from an abstractrole or without risking to break a crucial part of rbac (hasPermission). We could also implement something like flattenRole(RoleInterface $role) inside the rbac container, which returns a one-dimensional array (like it currently does in zfcrbac) - instead of doing this in hasPermission exclusively (and this allows us to cache the role hierarchy instead of building everything for every hasPermission request again and again)

@bakura10

The only constraint is that for it to work, we must be very rigorous in ZfcRbac, and correctly add all the roles to the container. Current problem is that, for instance, if we load from DB a role "foo" that has a child role "bar", and we add foo inside the container, $rbac->hasRole('foo') will return true BUT $rbac->hasRole('bar') will return false.

@arekkas
arekkas commented Dec 11, 2013

Shoudln't we check the children's roles of the ones added to rbac as well? This would prevent us from having to add all roles to the container. At least that is what happens atm when using hasPermission, isn't it?

@bakura10

You mean, whenever we add a role in the container, we recursively traverses all its children? What I'm afraid is that this would trigger database calls no?

@bakura10

The thing is that I start to understand why "getRole" in previous version triggers a recursion every time.. This is the only way to make sure that "getRole" work as expected.

@arekkas
arekkas commented Dec 11, 2013

Yes that would trigger database-calls, but roles are generally < 10 and any database should be able to handle that kind of load. And it's a better approach than iterating every time we do getRole or isGranted.

However it's still debatable if it's smart to "cache" stuff in a security suite, but I'm far from making calls on this one

@bakura10

Ok guys, I've updated tests.

@arekkas , right now, the issue is as follow:

So far, I thought about the following solutions:

  1. When adding the role (on addRole), we traverse it and add all its children and sub-children. The issue is that if sometime in the future a new role is added, it won't be taken into account, so that's problematic.

  2. We trigger a recursion on "hasRole" and "getRole", as before, but whenever we encounter a match, we add this to a local cache in the container, like $found[$roleName] = true. If it's not in the local cache, we trigger again a full recursion on the role. This way, a single call to "hasRole" for a given name won't trigger another recursion on subsequent hasRole or getRole calls.

@bakura10

@arekkas , just push an idea. What do you think?

@arekkas
arekkas commented Dec 11, 2013

That looks very promising in my opinion

@bakura10

Grrrr again this problem of not being able to recursively iterate when it encounters objects... GR. STUPID PHP.

@bakura10

Done!

A few note: we need to make a nested foreach because only role implement RecursiveIteratorIterator. If we want to do this in one for, we would need to make Rbac class implements RecursiveIterator also (after all, there was a reason behind all Spiffy choices...).

To me this is ready for review. After all implementation is not that different to original one. What we have new:

  • parent concept has been removed. Role have children, that's all.
  • smarter caching when getting role, so that any call to "getRole" or "hasRole" will put the result in cache and won't redo a new recursion if it's asked again
  • logic of traversing roles has been moved from role to the Rbac container.

Thoughts?

@bakura10

Ok, there is actually a problem with the fact that we check "hasRole" when adding a role.

Following case:

$role = new Role('bar');
$child = new Role('baz');
$role->addChild($child);

$this->rbac->addRole($role);
$this->rbac->addRole($child); // EXCEPTION!!

It triggers an exception because hasRole('baz') will evaluate to true.

This thing annoys me u_u.

@bakura10

Ok, added a new commit. I think we got the right approach (finally!). Now Rbac only have "isGranted". No more getRole, hasRole, addRole... this is handled "somewhere else" (likely in ZfcRbac). Here is a transcript of IRC before I forget:

[00:22:22] I can't find a clean way. The more I think, the more I feel that after all, when a permission is asked, it's always again given role
[00:22:26] those roles, we know them
[00:22:51] we have them in the authorization service, and those are from the identity
[00:23:13] so what we could do is simply inject the RoleProviderInterface INTO the authorizationservice
[00:23:28] removing the event system, and doing $this->roleProvider->getRoles($roles)
[00:23:57] If those are already instances of RoleInterface, we return them immediately
[00:24:11] so if an Entity contains RoleInterface, there isn't even a database call because it's already here
[00:24:35] otherwise, the InMemoryProvider will create only the asked role from config
[00:25:05] Bakura, aww yeah!
[00:25:14] does this sound stupid?
[00:25:18] now you got the spirit :D
[00:25:32] this sounds like the best approach
[00:25:37] This is what you had in mind from the beginning?
[00:25:47] yes
[00:25:54] I'm writing some code here
[00:25:56] lol
[00:26:17] I told you
[00:26:37] fetch from provider and call $rbac->isGranted()
[00:26:45] you dont need a container

@arekkas
arekkas commented Dec 12, 2013

I like the idea with the authorizationservice, it's pretty clean in my eyes and since we have only one provider, this could work pretty well

@bakura10

Hi,

I've pushed this component to a new repository: https://github.com/zf-fr/zfr-rbac

You can do PR on this new library, and I will back port feature here. ZfrRbac will be used in next version of ZfcRbac as the Rbac component.

@danizord

@arekkas We can aways create a ChainProvider that fetch roles from others providers.

@arekkas
arekkas commented Dec 12, 2013

We've already discussed extensively that ChainProvider do more harm than good, as it's hard to deal with conflicts. So those won't be included in future zfcrbac nor should they in rbac

@danizord

@arekkas agree, but the problem is that we was making the merge in RoleLoaderListener. If we handle all the logic inside the ChainProvider, then this becomes more easy ;)

@arekkas
arekkas commented Dec 12, 2013

From what I understood, listeners aren't going to be included / neccessary in future versions of zfcrbac, as the rbac container doesn't handle the role logic any more.

Additionally Chains are a security problem so I strongly advise not to use them, even if they could be useful

@arekkas
arekkas commented Dec 12, 2013

Here's why chains are a security issue:

ProviderA:
Role moderator inherits from login and specialrightsA

ProviderB:
Role moderator inherits from login and specialrightsB

Question:
Which roles does moderator inherit from? It could be all of them, some of them or none. There isn't a good strategy for solving this.

@danizord

@arekkas Yes, we'll get rid of listeners. Then handle all the logic inside the RoleProvider.

Your case will throw a RuntimeException. We should not allow a provider to overwrite others.

The chain is useful to get application built-in roles from InMemoryProvider and get the user-land roles from ObjectRepositoryProvider, for example.

Ok, this is a subject for other discussion :D

@danizord

@bakura10 Great idea to maintain a prototype until the release of ZF3. But this naming is really confusing though :confused:

@bakura10

You mean ZfrRbac and ZfcRbac ? Yeah I know... bad luck that my namespace looks the same :/. I could rename it ZfrPermissions maybe ?

@danizord

@bakura10 I'd sugest simply "Rbac".

@bakura10

Mmhh... that would be strange, among all my libraries that are "zfr"

@bakura10

Done. https://github.com/zf-fr/rbac

Could somebody read the code again (it's small) in the library, so I can tag for use in ZfcRbac

@weierophinney weierophinney added this to the 3.0.0 milestone Mar 3, 2014
@bakura10

I suppose ZF3 won't have anymore RBAC compoennt. ZfcRbac uses Zfr-Rbac as component.

@bakura10 bakura10 closed this Jan 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.