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

Fix #45 #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #45 #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Rbac.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ public function addRole($role, $parents = null) : void
);
}

if ($this->createMissingRoles) {
foreach ($role->getChildren() as $child) {
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this solution as well, but it is gonna work different way that it was in v2:

  • when we add child role to the given role after it is attached to rbac:
$rbac = ...
$role1 = new Role('role1');
$rbac->addRole($role1);

$child = new Role('role2');
$role1->addChild($child);

self::assertFalse($rbac->hasRole('role2'));

but in v2 it will be true.

  • createMissingRoles is default false, so it's not gonna add the child roles by default (it was also not the case in v2, as iterator worked there on all roles and all children).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know that there's a difference between v2 but this is ok, we are on v3 and the scope of my refactoring was about consistence (see #34).
Regarding your example, role2 is not added automatically to $rbac and I think is correct. If you want to have also role2 you need to add it as child to $role1 and finally assign it to $rbac using addRole. I don't see any issue here.

createMissingRoles was also false in v2. Maybe, we need to be more explicit in the documentation about that.

Copy link
Member

Choose a reason for hiding this comment

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

@ezimuel If you think the case I've provided above is desired then it should be also included in tests and described in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Since $createMissingRoles is currently false by default, changing its value to true would be a BC break.

That said, I think user expectations are that if you add a child role to a role, but not directly to the RBAC, it would still be considered a role in the RBAC. This was the behavior in v2, and the problem reported in #45. In looking through the 3.0.0 CHANGELOG, there is no reference to this change in behavior, which means it is unexpected.

What the CHANGELOG does note is that there is now support for detecting circular references in the role hierarchy. What this patch doesn't do is include a check to see if the RBAC instance has the child role before attempting to add it, which makes this a potentially destructive process.

My gut take is that we should:

  • Make $createMissingRoles be true be default, so that the behavior matches user expectations, as well as the existing documentation.
  • Add tests to ensure that when we add a role, we do not overwrite existing roles.
  • Modify the patch to only add a child role when it does not already exist in the RBAC.

I'll work on those momentarily.

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, I just switched the default value of $createMissingRoles, and tests continued to pass. Which indicates none of these scenarios were tested fully previously.

Copy link
Member

Choose a reason for hiding this comment

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

I started by adding tests that would not add a role if it already exists in the RBAC, by testing against hasRole(). Unfortunately, this failed, because hasRole() has logic that checks if the $role implements RoleInterface, and, if so, it does a strict equality check against the internal instance and the instance passed.

I considered removing that strict equality check, but that leaves us in a situation where we could have two different RoleInterface implementations, and these would be considered equivalent in the RBAC.

But this also means we have a scenario where one RoleInterface implementation could be registered with its own set of parents and/or children, and then another entirely different RoleInterface instance using the same role added later. In this scenario, the latter would overwrite the original, and we would potentially have completely different inheritance trees within the RBAC using the same role name, but with different instances.

What's really crazy is that if $parents are specified as strings, these end up being new Role instances, and overwriting any previous versions of those roles in the RBAC.

This feels really confusing to me, and I'm not sure it's something we want to support.

So, my question here is: what should the behavior be? Silently overwriting feels like it has huge potential for misuse and WTF moments. Doing strict checks in hasRole() seems like it has potential for problems, as it can return a false negative when a role of the same name exists, but it is a different instance than that provided to the method (as the method takes either the string name or a RoleInterface instance) — and even more so, as we might want to toggle that strictness off when building the RBAC, but on again later, or even vary based on the role we're adding.

Basically, we need a list of scenarios and how they should behave before we can continue with any of this.

$this->addRole($child);
}
}

if ($parents) {
$parents = is_array($parents) ? $parents : [$parents];
foreach ($parents as $parent) {
Expand Down
19 changes: 19 additions & 0 deletions test/RbacTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@ public function testAddCustomChildRole()
{
$role = $this->getMockForAbstractClass(Rbac\RoleInterface::class);
$this->rbac->setCreateMissingRoles(true);
$role->expects($this->any())
->method('getChildren')
->will($this->returnValue([]));

$this->rbac->addRole($role, 'parent');

$role->expects($this->any())
Expand Down Expand Up @@ -303,4 +307,19 @@ public function testEmptyRoles()
{
$this->assertEquals([], $this->rbac->getRoles());
}

/**
* @see https://github.com/zendframework/zend-permissions-rbac/issues/45
*/
public function testHasRoleWithChildrenRoles()
{
$foo = new Rbac\Role('foo');
$bar = new Rbac\Role('bar');

$this->rbac->setCreateMissingRoles(true);
$foo->addChild($bar);
$this->rbac->addRole($foo);

$this->assertTrue($this->rbac->hasRole('bar'));
}
}