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

Hotfix/45 #49

Closed
wants to merge 4 commits into from
Closed

Hotfix/45 #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8a28fa
[hotfix/45] - recursive check for roles
traviskentbeste Aug 6, 2019
e6dbec9
getRole is now searching recursively
traviskentbeste Aug 8, 2019
10ffa7c
fixing object comparison in needle for hasRole
traviskentbeste Aug 8, 2019
125d819
formatting adjustments, now clean cs-check
traviskentbeste Aug 8, 2019
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
106 changes: 94 additions & 12 deletions src/Rbac.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

namespace Zend\Permissions\Rbac;

use ZendTest\Permissions\Rbac\RoleTest;

class Rbac
{
/**
Expand Down Expand Up @@ -82,29 +84,109 @@ public function hasRole($role) : bool
);
}

if (is_string($role)) {
return isset($this->roles[$role]);
return $this->roleSearchIncludingChildren($this->roles, $role);
}

/**
* @param $obj|Array
* @param $needle|String
* @return bool
*/
private function roleSearchIncludingChildren($obj, $needle) : bool
{
$rv = 0;

if (is_array($obj)) {
foreach ($obj as $role) {
$roleName = $role->getName();
if (is_string($needle)) {
if ($roleName === $needle) {
$rv++;
}
} elseif (is_object($needle) && (get_class($needle) == 'Zend\Permissions\Rbac\Role')) {
if ($roleName == $needle->getName()) {
$rv++;
}
}
$rv += $this->roleSearchIncludingChildren($role, $needle);
Copy link
Member

Choose a reason for hiding this comment

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

This section could be condensed dramatically:

if ((is_string($needle) && $roleName === $needle)
    || (is_object($needle) && $needle instanceof Role && $roleName === $needle->getName())
) {
    return true; // no need to do anything else, because method returns based on non-zero $rv value
}
return $this->roleSearchIncludingChildren($role, $needle); // Same rationale here

}
} else {
$children = $obj->getChildren();

// need to make sure the children are arrays (meaning they are added correctly)
if (! is_array($children)) {
return $rv ? true : false;
Copy link
Member

Choose a reason for hiding this comment

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

There is zero possibility of $rv being anything but 0 in this scenario.

} elseif (! count($children)) {
if (is_object($needle) && (get_class($needle) == 'Zend\Permissions\Rbac\Role')) {
if ($obj->getName() == $needle->getName()) {
$rv++;
}
}
} else {
foreach ($children as $child) {
$roleName = $child->getName();
if (is_string($needle)) {
if ($roleName === $needle) {
$rv++;
}
} elseif (is_object($needle) && (get_class($needle) == 'Zend\Permissions\Rbac\Role')) {
if ($roleName == $needle->getName()) {
$rv++;
}
}
$rv += $this->roleSearchIncludingChildren($child, $needle);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Just like the other block, we could do the following:

if (! is_array($children)) {
    return false;
}
if (! count($children)) {
    return is_object($needle) && $needle instanceof Role && $obje->getName() === $needle->getName();
}

foreach ($children as $child) {
    $roleName = $hcild->getName();
    if (is_string($needle) && $roleName === $needle) {
        return true;
    }
    if (is_object($needle) && $needle instanceof Role && $roleName === $needle->getName()) {
        return true;
    }
    if ($this->roleSearchIncludingChildren($child, $needle)) {
        return true;
    }
}
return false;

Making these changes and the previous ones I suggested would eliminate unnecessary cycles when we've already determined a matching child is present.

}

$roleName = $role->getName();
return isset($this->roles[$roleName])
&& $this->roles[$roleName] === $role;
return $rv ? true : false;
}

/**
* Get a registered role by name
*
* @throws Exception\InvalidArgumentException if role is not found.
*/
public function getRole(string $roleName) : RoleInterface
public function getRole(string $needle) : RoleInterface
{
if (! isset($this->roles[$roleName])) {
throw new Exception\InvalidArgumentException(sprintf(
'No role with name "%s" could be found',
$roleName
));
// tricky thing here is that $this->roles are an array of RoleInterface objects
foreach ($this->roles as $role) {
if ($role->getName() == $needle) {
return $role;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Since your if block always returns, there's no need for an else; if the condition does not match, the next code must always execute. Promote the else block:

if ($role->getName() === $needle) {
   return $role;
}
$role = $this->getRoleSearchingChildren($role, $needle);
if ($role !== null) {
    return $role;
}

$role = $this->getRoleSearchingChildren($role, $needle);
if ($role != null) {
return $role;
}
}
}

throw new Exception\InvalidArgumentException(sprintf(
'No role with name "%s" could be found',
$needle
));
}

/**
* @param $obj RoleInterface
* @param $needle String
* @return null|RoleInterface
*/
private function getRoleSearchingChildren($obj, $needle)
{
if (($obj instanceof RoleInterface) && ($obj->getName() == $needle)) {
return $obj;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for getRole() - promote the body of your else block, as you always return from the if block.

$children = $obj->getChildren();
if (is_array($children) && ($children != null)) {
$result = '';
foreach ($children as $child) {
$result = $this->getRoleSearchingChildren($child, $needle);
}
return $result;
Copy link
Member

Choose a reason for hiding this comment

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

This part is confusing - do you want to return the first match, or the last? If the last, why?

}
}
return $this->roles[$roleName];
return null;
}

/**
Expand Down
7 changes: 7 additions & 0 deletions test/RbacTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ public function testHasRole()
$this->assertNotEquals($snafu, $this->rbac->getRole('snafu'));
$this->assertTrue($this->rbac->hasRole('snafu'));
$this->assertFalse($this->rbac->hasRole($snafu));

// check child is found
$parent = new TestAsset\RoleTest('parent');
$child = new TestAsset\RoleTest('child');
$parent->addChild($child);
$this->rbac->addRole($parent);
$this->assertTrue($this->rbac->hasRole('child'));
Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate test entirely.

}

public function testHasRoleWithInvalidElement()
Expand Down