diff --git a/CHANGELOG.md b/CHANGELOG.md index e0cb9b2b..248250cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,7 +59,9 @@ - Enh #252: Return `$this` instead of throwing "already assigned" exception in `Manager::assign()` (@arogachev) - Enh #248: Add `SimpleRuleFactory` (@arogachev) - Enh #251: Allow checking for user's roles in `ManagerInterface::userHasPermission()` (@arogachev) -- Chg #259: Rename `$ruleContext` argument to `$context` in `RuleInterface::execute()` (@arogachev) +- Chg #259: Rename `$ruleContext` argument to `$context` in `RuleInterface::execute()` (@arogachev) +- Bug #260: Fix `Manager::userHasPermission()` to return `true` for the case when a user have access via at least one + hierarchy branch (@arogachev) ## 1.0.2 April 20, 2023 diff --git a/src/Manager.php b/src/Manager.php index 3631eb82..9a02cc59 100644 --- a/src/Manager.php +++ b/src/Manager.php @@ -70,37 +70,44 @@ public function userHasPermission( } $hierarchy = $this->itemsStorage->getHierarchy($item->getName()); - if ($guestRole !== null && !array_key_exists($guestRole->getName(), $hierarchy)) { - $hierarchy[$guestRole->getName()] = ['item' => $guestRole, 'children' => []]; - } - $itemNames = array_map(static fn (array $treeItem): string => $treeItem['item']->getName(), $hierarchy); $userItemNames = $guestRole !== null ? [$guestRole->getName()] : $this->assignmentsStorage->filterUserItemNames((string) $userId, $itemNames); + $userItemNamesMap = []; + foreach ($userItemNames as $userItemName) { + $userItemNamesMap[$userItemName] = null; + } - $userItems = []; - foreach ($userItemNames as $itemName) { - $userItems[$itemName] = $hierarchy[$itemName]['item']; - foreach ($hierarchy[$itemName]['children'] ?? [] as $item) { - $userItems[$item->getName()] = $item; + foreach ($hierarchy as $data) { + if ( + !array_key_exists($data['item']->getName(), $userItemNamesMap) || + !$this->executeRule($userId === null ? $userId : (string) $userId, $data['item'], $parameters) + ) { + continue; } - } - if ( - empty($userItems) || - ($guestRole !== null && count($userItems) === 1 && array_key_exists('guest', $userItems)) - ) { - return false; - } + $hasPermission = true; + foreach ($data['children'] as $childItem) { + if (!$this->executeRule($userId === null ? $userId : (string) $userId, $childItem, $parameters)) { + $hasPermission = false; + + /** + * @infection-ignore-all Break_ + * Replacing with `continue` works as well, but there is no point in further checks, because at + * least one failed rule execution means access is not granted via current iterated hierarchy + * branch. + */ + break; + } + } - foreach ($userItems as $item) { - if (!$this->executeRule($userId === null ? $userId : (string) $userId, $item, $parameters)) { - return false; + if ($hasPermission) { + return true; } } - return true; + return false; } public function canAddChild(string $parentName, string $childName): bool diff --git a/tests/Common/ManagerLogicTestTrait.php b/tests/Common/ManagerLogicTestTrait.php index 80e3c5b2..9c2a95e3 100644 --- a/tests/Common/ManagerLogicTestTrait.php +++ b/tests/Common/ManagerLogicTestTrait.php @@ -22,6 +22,7 @@ use Yiisoft\Rbac\Tests\Support\BanRule; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; +use Yiisoft\Rbac\Tests\Support\FalseRule; use Yiisoft\Rbac\Tests\Support\GuestRule; use Yiisoft\Rbac\Tests\Support\SubscriptionRule; use Yiisoft\Rbac\Tests\Support\TrueRule; @@ -297,6 +298,21 @@ public function testUserHasPermissionWithRolesAllowed( ); } + public function testUserHasPermissionWithOneHierarchyBranch(): void + { + $manager = $this + ->createFilledManager() + ->addPermission(new Permission('Permission')) + ->addRole(new Role('Role 1')) + ->addRole((new Role('Role 2'))->withRuleName(FalseRule::class)) + ->addChild('Role 1', 'Permission') + ->addChild('Role 2', 'Permission') + ->assign(itemName: 'Role 1', userId: 'User') + ->assign(itemName: 'Role 2', userId: 'User'); + + $this->assertTrue($manager->userHasPermission('User', 'Permission')); + } + public function testCanAddExistingChild(): void { $manager = $this->createFilledManager();