Fix #45

[ezimuel]

This PR fixes #45 adding the children roles in Rbac::addRole().

[michalbundyra]

@ezimuel I think this is still not full solution. Please see my comment. I think we need iterator on roles as before. Also - documentation needs update.

[michalbundyra]

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).

[ezimuel]

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.

[michalbundyra]

@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.

[weierophinney]

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.

[weierophinney]

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

[weierophinney]

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.

[weierophinney]

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.

[weierophinney]

This repository has been closed and moved to laminas/laminas-permissions-rbac; a new issue has been opened at laminas/laminas-permissions-rbac#2.

[weierophinney]

This repository has been moved to laminas/laminas-permissions-rbac. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-permissions-rbac to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-permissions-rbac.
• In your clone of laminas/laminas-permissions-rbac, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.