zendframework / zend-permissions-rbac

Create and query role-based access controls.
BSD 3-Clause "New" or "Revised" License
44 stars 23 forks source link

Inheritance problems and invalid documentation #30

Closed codeaid closed 6 years ago

codeaid commented 7 years ago

I've been struggling with a role inheritance issue for the last hour or so and I've come to a conclusion that there's either a bug or missing functionality in the library

This issue is about the way inheritance functionality is implemented in the library, as it assumes that every child role will be populated with permissions originally assigned to that role. Let me try to explain that.

Let's assume the following code:

$guest = new Role('guest');
$guest->addPermission('read');

$editor = new Role('editor');
$editor->addChild('guest');
$editor->addPermission('write');

$rbac = new Rbac();
$rbac->addRole($guest);
$rbac->addRole($editor);
var_dump($rbac);

Here's the output:

object(AppBundle\Permissions\Rbac\Rbac)#301 (3) {
  ["createMissingRoles":protected]=>
  bool(false)
  ["index":protected]=>
  int(0)
  ["children":protected]=>
  array(2) {
    [0]=>
    object(AppBundle\Permissions\Rbac\Role)#315 (6) {
      ["description":"AppBundle\Permissions\Rbac\Role":private]=>
      NULL
      ["parent":protected]=>
      NULL
      ["name":protected]=>
      string(5) "guest"
      ["permissions":protected]=>
      array(1) {
        ["read"]=>
        bool(true)
      }
      ["index":protected]=>
      int(0)
      ["children":protected]=>
      array(0) {
      }
    }
    [1]=>
    object(AppBundle\Permissions\Rbac\Role)#316 (6) {
      ["description":"AppBundle\Permissions\Rbac\Role":private]=>
      NULL
      ["parent":protected]=>
      NULL
      ["name":protected]=>
      string(6) "editor"
      ["permissions":protected]=>
      array(1) {
        ["write"]=>
        bool(true)
      }
      ["index":protected]=>
      int(0)
      ["children":protected]=>
      array(1) {
        [0]=>
        object(Zend\Permissions\Rbac\Role)#317 (5) {
          ["parent":protected]=>
          *RECURSION*
          ["name":protected]=>
          string(5) "guest"
          ["permissions":protected]=>
          array(0) {
          }
          ["index":protected]=>
          int(0)
          ["children":protected]=>
          array(0) {
          }
        }
      }
    }
  }
}

Notice value of the permissions property on both guest roles. The one that was added directly to the RBAC container is [ "read": true ], while the one added as a child (using the string notation) to the editor role has it as an empty array.

When a check is performed if the role editor has read permission by calling $rbac->isGranted('editor', 'read') it starts recursively traversing children of children and looking for the read permission, which obviously does not exist anywhere in the editor branch.

It works if the guest role is added as an object rather than a string - $editor->addChild($guest);. This poses a problem of dynamically generating RBAC containers, because of potential circular references.

Imagine role A, which is a child of B, which is a child of C, which is a child of the initial A. This is what happened/happens in my case, as users are free to edit roles and their permissions as they wish in the CMS system. I had a recursive method in my code fetching all roles from the database as a flat list and attempting to build a tree.

Here is some sample data I'm storing in my MongoDB database:

{
    "_id" : "f2a8ae60-893a-40df-8548-9817828ea4b6",
    "description" : "Permissions viewer",
    "permissions" : [
        "cms.permissions.list"
    ],
    "children" : [ ]
}
{
    "_id" : "d8cf9632-5f30-4ade-8663-f0140ce7e762",
    "description" : "Permissions admin",
    "permissions" : [ ],
    "children" : [
        "f2a8ae60-893a-40df-8548-9817828ea4b6"
    ]
}
{
    "_id" : "9949e3a5-9fc5-42ec-89ea-402da7446ea2",
    "description" : "CMS admin",
    "permissions" : [ ],
    "children" : [
        "d8cf9632-5f30-4ade-8663-f0140ce7e762"
    ]
}

If I fetch all documents as a flat list and just add them to an RBAC container, the CMS amdin user ends up not having the cms.permissions.list permission, which is obviously wrong. To fix that I need to replace child UUIDs in each role with the actual instance of a Role object, which in itself is doable and works, but ends up in an endless circular reference at one point.

So the question is - what is the solution to this problem?

One solution I see is passing the parent Rbac container into the role when making the hasPermission($name) call (as more roles can be added later). That would allow the parent role to fetch the actual child role object from the root of the container (if available) rather than having to use a blank Role.

Currently hasPermission only traverses into the role children, and, if they were specified (injected) as strings, list of permissions on them end up being empty, meaning recursion is completely pointless.

I hope this helps!

froschdesign commented 7 years ago

@codeaid

This issue is not about the documentation though.

The first part is a problem of the blog post and should be moved to the website issue tracker. This will break "the wall of text". Thanks – good catch!

codeaid commented 7 years ago

@froschdesign Should I copy/paste the first bit into a new website issue?

froschdesign commented 7 years ago

Cut and paste!

codeaid commented 7 years ago

Done: https://github.com/zendframework/zf3-web/issues/69

ezimuel commented 6 years ago

@codeaid regarding the support of string as role name I think we should avoid this feature because, as you noticed in your example, it can be confusing, referencing to different role objects with same name. I think the right approach will be to create roles first, assign permissions after, and define inheritance structure as last step, using object references.

Finally, regarding the question to prevent circular references I think we can accomplish this when iterating over the roles. I'm working on a potential refactor of zend-permissions-rbac to accomplish this. I'm also considering to remove the usage of RecursiveIteratorIterator since the performance are not so good, in our case.