From 821d96ad08bf5651ba1ecb4fe01b8ff2e6f11398 Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Tue, 13 Feb 2024 18:40:11 +0600 Subject: [PATCH 1/8] Add `SimpleRuleFactory` --- CHANGELOG.md | 1 + {tests/Support => src}/SimpleRuleFactory.php | 4 +--- tests/Common/ManagerConfigurationTestTrait.php | 2 +- tests/Common/ManagerLogicTestTrait.php | 4 ++-- tests/CompositeRuleTest.php | 2 +- tests/ConfigTest.php | 2 +- tests/RuleContextTest.php | 2 +- 7 files changed, 8 insertions(+), 9 deletions(-) rename {tests/Support => src}/SimpleRuleFactory.php (83%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0cc7d542..4a85a0f2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ - Bug #237: Handle not found base item in access tree (@arogachev) - Enh #245: Handle same names during renaming item in `AssignmentsStorage` (@arogachev) - Chg #208: Rename `getAccessTree()` to `getHierarchy()` in `ItemsStorageInterface` (@arogachev) +- Enh #248: Add `SimpleRuleFactory` (@arogachev) ## 1.0.2 April 20, 2023 diff --git a/tests/Support/SimpleRuleFactory.php b/src/SimpleRuleFactory.php similarity index 83% rename from tests/Support/SimpleRuleFactory.php rename to src/SimpleRuleFactory.php index 4e1751e3a..2af133c0f 100644 --- a/tests/Support/SimpleRuleFactory.php +++ b/src/SimpleRuleFactory.php @@ -2,11 +2,9 @@ declare(strict_types=1); -namespace Yiisoft\Rbac\Tests\Support; +namespace Yiisoft\Rbac; use Yiisoft\Rbac\Exception\RuleNotFoundException; -use Yiisoft\Rbac\RuleFactoryInterface; -use Yiisoft\Rbac\RuleInterface; use function array_key_exists; diff --git a/tests/Common/ManagerConfigurationTestTrait.php b/tests/Common/ManagerConfigurationTestTrait.php index 5e47809eb..5d853f36f 100644 --- a/tests/Common/ManagerConfigurationTestTrait.php +++ b/tests/Common/ManagerConfigurationTestTrait.php @@ -11,11 +11,11 @@ use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; use Yiisoft\Rbac\RuleFactoryInterface; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\AuthorRule; use Yiisoft\Rbac\Tests\Support\EasyRule; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; -use Yiisoft\Rbac\Tests\Support\SimpleRuleFactory; trait ManagerConfigurationTestTrait { diff --git a/tests/Common/ManagerLogicTestTrait.php b/tests/Common/ManagerLogicTestTrait.php index bb473897c..b23fc47e8 100644 --- a/tests/Common/ManagerLogicTestTrait.php +++ b/tests/Common/ManagerLogicTestTrait.php @@ -15,14 +15,14 @@ use Yiisoft\Rbac\Exception\RuleNotFoundException; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\AdsRule; -use Yiisoft\Rbac\Tests\Support\GuestRule; use Yiisoft\Rbac\Tests\Support\AuthorRule; use Yiisoft\Rbac\Tests\Support\BanRule; use Yiisoft\Rbac\Tests\Support\EasyRule; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; -use Yiisoft\Rbac\Tests\Support\SimpleRuleFactory; +use Yiisoft\Rbac\Tests\Support\GuestRule; use Yiisoft\Rbac\Tests\Support\SubscriptionRule; trait ManagerLogicTestTrait diff --git a/tests/CompositeRuleTest.php b/tests/CompositeRuleTest.php index 1f251f78a..334215151 100644 --- a/tests/CompositeRuleTest.php +++ b/tests/CompositeRuleTest.php @@ -9,8 +9,8 @@ use Yiisoft\Rbac\CompositeRule; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\RuleContext; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\EasyRule; -use Yiisoft\Rbac\Tests\Support\SimpleRuleFactory; final class CompositeRuleTest extends TestCase { diff --git a/tests/ConfigTest.php b/tests/ConfigTest.php index b05a9979a..f79912c48 100644 --- a/tests/ConfigTest.php +++ b/tests/ConfigTest.php @@ -13,9 +13,9 @@ use Yiisoft\Rbac\Manager; use Yiisoft\Rbac\ManagerInterface; use Yiisoft\Rbac\RuleFactoryInterface; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; -use Yiisoft\Rbac\Tests\Support\SimpleRuleFactory; final class ConfigTest extends TestCase { diff --git a/tests/RuleContextTest.php b/tests/RuleContextTest.php index 1399f8448..c27ea1877 100644 --- a/tests/RuleContextTest.php +++ b/tests/RuleContextTest.php @@ -6,8 +6,8 @@ use PHPUnit\Framework\TestCase; use Yiisoft\Rbac\RuleContext; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\EasyRule; -use Yiisoft\Rbac\Tests\Support\SimpleRuleFactory; final class RuleContextTest extends TestCase { From 6b65ca84b220dd5de16b0212d5b990e35768164b Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Wed, 14 Feb 2024 14:51:59 +0600 Subject: [PATCH 2/8] WIP --- CHANGELOG.md | 2 +- README.md | 33 +++---------------- src/Manager.php | 4 ++- src/RuleContext.php | 7 ++-- src/SimpleRuleFactory.php | 19 +++++------ .../Common/ManagerConfigurationTestTrait.php | 21 +++--------- tests/Common/ManagerLogicTestTrait.php | 28 ++++++---------- tests/CompositeRuleTest.php | 18 ++++------ tests/RuleContextTest.php | 13 ++++---- tests/Support/{EasyRule.php => FalseRule.php} | 13 ++------ tests/Support/TrueRule.php | 17 ++++++++++ 11 files changed, 69 insertions(+), 106 deletions(-) rename tests/Support/{EasyRule.php => FalseRule.php} (52%) create mode 100644 tests/Support/TrueRule.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c07509c9..162b79f09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,8 +56,8 @@ - Bug #237: Handle not found base item in access tree (@arogachev) - Enh #245: Handle same names during renaming item in `AssignmentsStorage` (@arogachev) - Chg #208: Rename `getAccessTree()` to `getHierarchy()` in `ItemsStorageInterface` (@arogachev) -- Enh #248: Add `SimpleRuleFactory` (@arogachev) - Enh #252: Return `$this` instead of throwing "already assigned" exception in `Manager::assign()` (@arogachev) +- Enh #248: Add `SimpleRuleFactory` (@arogachev) ## 1.0.2 April 20, 2023 diff --git a/README.md b/README.md index 6c666e7ba..968852ef5 100644 --- a/README.md +++ b/README.md @@ -70,38 +70,15 @@ use Yiisoft\Rbac\RuleFactoryInterface; $manager = new Manager($itemsStorage, $assignmentsStorage, $ruleFactory); ``` -It requires specifying the following dependencies: +It requires the following dependencies: - Items storage (hierarchy itself). - Assignments storage where user IDs are mapped to roles. -- Rule factory. Given a rule name stored in item storage it can create an instance of `Rule`. +- Rule factory. Creates a rule instance by a given name. -If you don't want to use [Rules Container](https://github.com/yiisoft/rbac-rules-container), here is an example of -simple self-contained rule factory: - -```php -use Yiisoft\Rbac\Exception\RuleNotFoundException; -use Yiisoft\Rbac\RuleFactoryInterface; -use Yiisoft\Rbac\RuleInterface; - -use function array_key_exists; - -final class SimpleRuleFactory implements RuleFactoryInterface -{ - public function __construct(private array $rules = []) - { - } - - public function create(string $name): RuleInterface - { - if (!array_key_exists($name, $this->rules)) { - throw new RuleNotFoundException($name); - } - - return $this->rules[$name]; - } -} -``` +While storages are required, rule factory is optional and, when omitted, `SimpleRuleFactory` will be used. For more +advanced usage, such as resolving rules by aliases and passing arguments in rules constructor, install +[Rules Container](https://github.com/yiisoft/rbac-rules-container) additionally or write your own implementation. A few tips for choosing storage backend: diff --git a/src/Manager.php b/src/Manager.php index fa70a7fc3..0ce04ca54 100644 --- a/src/Manager.php +++ b/src/Manager.php @@ -19,6 +19,7 @@ */ final class Manager implements ManagerInterface { + private readonly RuleFactoryInterface $ruleFactory; /** * @var string[] A list of role names that are assigned to every user automatically without calling {@see assign()}. * Note that these roles are applied to users, regardless of their state of authentication. @@ -36,9 +37,10 @@ final class Manager implements ManagerInterface public function __construct( private readonly ItemsStorageInterface $itemsStorage, private readonly AssignmentsStorageInterface $assignmentsStorage, - private readonly RuleFactoryInterface $ruleFactory, + ?RuleFactoryInterface $ruleFactory = null, private readonly bool $enableDirectPermissions = false, ) { + $this->ruleFactory = $ruleFactory ?? new SimpleRuleFactory(); } public function userHasPermission( diff --git a/src/RuleContext.php b/src/RuleContext.php index 9b042f33f..e248394ad 100644 --- a/src/RuleContext.php +++ b/src/RuleContext.php @@ -6,10 +6,13 @@ final class RuleContext { + private readonly RuleFactoryInterface $ruleFactory; + public function __construct( - private readonly RuleFactoryInterface $ruleFactory, - private readonly array $parameters, + ?RuleFactoryInterface $ruleFactory = null, + private readonly array $parameters = [], ) { + $this->ruleFactory = $ruleFactory ?? new SimpleRuleFactory(); } public function getParameters(): array diff --git a/src/SimpleRuleFactory.php b/src/SimpleRuleFactory.php index 2af133c0f..ec2fa5730 100644 --- a/src/SimpleRuleFactory.php +++ b/src/SimpleRuleFactory.php @@ -4,25 +4,22 @@ namespace Yiisoft\Rbac; +use Yiisoft\Rbac\Exception\RuleInterfaceNotImplementedException; use Yiisoft\Rbac\Exception\RuleNotFoundException; -use function array_key_exists; - final class SimpleRuleFactory implements RuleFactoryInterface { - /** - * @psalm-param array $rules - */ - public function __construct(private readonly array $rules = []) - { - } - public function create(string $name): RuleInterface { - if (!array_key_exists($name, $this->rules)) { + if (!class_exists($name)) { throw new RuleNotFoundException($name); } - return $this->rules[$name]; + $instance = new $name; + if (!$instance instanceof RuleInterface) { + throw new RuleInterfaceNotImplementedException($name); + } + + return $instance; } } diff --git a/tests/Common/ManagerConfigurationTestTrait.php b/tests/Common/ManagerConfigurationTestTrait.php index 5d853f36f..ffe8af3e5 100644 --- a/tests/Common/ManagerConfigurationTestTrait.php +++ b/tests/Common/ManagerConfigurationTestTrait.php @@ -10,10 +10,7 @@ use Yiisoft\Rbac\ManagerInterface; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; -use Yiisoft\Rbac\RuleFactoryInterface; -use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\AuthorRule; -use Yiisoft\Rbac\Tests\Support\EasyRule; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; @@ -22,16 +19,14 @@ trait ManagerConfigurationTestTrait protected function createManager( ?ItemsStorageInterface $itemsStorage = null, ?AssignmentsStorageInterface $assignmentsStorage = null, - ?RuleFactoryInterface $ruleFactory = null, - ?bool $enableDirectPermissions = false + ?bool $enableDirectPermissions = false, ): ManagerInterface { $arguments = [ - $itemsStorage ?? $this->createItemsStorage(), - $assignmentsStorage ?? $this->createAssignmentsStorage(), - $ruleFactory ?? new SimpleRuleFactory(), + 'itemsStorage' => $itemsStorage ?? $this->createItemsStorage(), + 'assignmentsStorage' => $assignmentsStorage ?? $this->createAssignmentsStorage(), ]; if ($enableDirectPermissions !== null) { - $arguments[] = $enableDirectPermissions; + $arguments['enableDirectPermissions'] = $enableDirectPermissions; } return new Manager(...$arguments); @@ -50,17 +45,11 @@ protected function createAssignmentsStorage(): AssignmentsStorageInterface protected function createFilledManager( ?ItemsStorageInterface $itemsStorage = null, ?AssignmentsStorageInterface $assignmentsStorage = null, - ?RuleFactoryInterface $ruleFactory = null, ): ManagerInterface { return $this ->createManager( $itemsStorage ?? $this->createItemsStorage(), $assignmentsStorage ?? $this->createAssignmentsStorage(), - $ruleFactory ?? new SimpleRuleFactory([ - 'isAuthor' => new AuthorRule(), - 'easyTrue' => new EasyRule(true), - 'easyFalse' => new EasyRule(false), - ]), enableDirectPermissions: true, ) ->addPermission(new Permission('Fast Metabolism')) @@ -68,7 +57,7 @@ protected function createFilledManager( ->addPermission(new Permission('publishPost')) ->addPermission(new Permission('readPost')) ->addPermission(new Permission('deletePost')) - ->addPermission((new Permission('updatePost'))->withRuleName('isAuthor')) + ->addPermission((new Permission('updatePost'))->withRuleName(AuthorRule::class)) ->addPermission(new Permission('updateAnyPost')) ->addRole(new Role('reader')) ->addRole(new Role('author')) diff --git a/tests/Common/ManagerLogicTestTrait.php b/tests/Common/ManagerLogicTestTrait.php index 76aa64417..2d6e6f8d1 100644 --- a/tests/Common/ManagerLogicTestTrait.php +++ b/tests/Common/ManagerLogicTestTrait.php @@ -15,15 +15,14 @@ use Yiisoft\Rbac\Exception\RuleNotFoundException; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; -use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\AdsRule; use Yiisoft\Rbac\Tests\Support\AuthorRule; use Yiisoft\Rbac\Tests\Support\BanRule; -use Yiisoft\Rbac\Tests\Support\EasyRule; use Yiisoft\Rbac\Tests\Support\FakeAssignmentsStorage; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; use Yiisoft\Rbac\Tests\Support\GuestRule; use Yiisoft\Rbac\Tests\Support\SubscriptionRule; +use Yiisoft\Rbac\Tests\Support\TrueRule; trait ManagerLogicTestTrait { @@ -175,30 +174,23 @@ public function testUserHasPermissionGuestOriented( ->createManager( $this->createItemsStorage(), $this->createAssignmentsStorage(), - new SimpleRuleFactory([ - 'subscription' => new SubscriptionRule(), - 'ads' => new AdsRule(), - 'author' => new AuthorRule(), - 'ban' => new BanRule(), - 'guest' => new GuestRule(), - ]), enableDirectPermissions: true, ) - ->addRole((new Role('guest'))->withRuleName('guest')) + ->addRole((new Role('guest'))->withRuleName(GuestRule::class)) ->setGuestRoleName('guest') ->addRole(new Role('news comment manager')) ->addRole(new Role('warned user')) ->addRole(new Role('trial user')) - ->addRole((new Role('subscribed user'))->withRuleName('subscription')) - ->addPermission((new Permission('view ads'))->withRuleName('ads')) - ->addPermission((new Permission('view ban warning'))->withRuleName('ban')) + ->addRole((new Role('subscribed user'))->withRuleName(SubscriptionRule::class)) + ->addPermission((new Permission('view ads'))->withRuleName(AdsRule::class)) + ->addPermission((new Permission('view ban warning'))->withRuleName(BanRule::class)) ->addPermission(new Permission('view content')) ->addPermission(new Permission('view regular content')) ->addPermission(new Permission('view news')) ->addPermission(new Permission('add news comment')) ->addPermission(new Permission('view news comment')) - ->addPermission((new Permission('edit news comment'))->withRuleName('author')) - ->addPermission((new Permission('remove news comment'))->withRuleName('author')) + ->addPermission((new Permission('edit news comment'))->withRuleName(AuthorRule::class)) + ->addPermission((new Permission('remove news comment'))->withRuleName(AuthorRule::class)) ->addPermission(new Permission('view wiki')) ->addPermission(new Permission('view exclusive content')) ->addChild('view content', 'view regular content') @@ -601,11 +593,11 @@ public function testAddRole(): void { $manager = $this->createFilledManager(); - $rule = new EasyRule(); + $rule = new TrueRule(); $role = (new Role('new role')) ->withDescription('new role description') - ->withRuleName($rule->getName()) + ->withRuleName(TrueRule::class) ->withCreatedAt(1_642_026_147) ->withUpdatedAt(1_642_026_148); @@ -621,7 +613,7 @@ public function testAddRole(): void [ 'name' => 'new role', 'description' => 'new role description', - 'rule_name' => EasyRule::class, + 'rule_name' => TrueRule::class, 'type' => 'role', 'updated_at' => 1_642_026_148, 'created_at' => 1_642_026_147, diff --git a/tests/CompositeRuleTest.php b/tests/CompositeRuleTest.php index 334215151..363a58dae 100644 --- a/tests/CompositeRuleTest.php +++ b/tests/CompositeRuleTest.php @@ -9,8 +9,8 @@ use Yiisoft\Rbac\CompositeRule; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\RuleContext; -use Yiisoft\Rbac\SimpleRuleFactory; -use Yiisoft\Rbac\Tests\Support\EasyRule; +use Yiisoft\Rbac\Tests\Support\FalseRule; +use Yiisoft\Rbac\Tests\Support\TrueRule; final class CompositeRuleTest extends TestCase { @@ -18,12 +18,12 @@ public static function dataCompositeRule(): array { return [ 'AND empty' => [CompositeRule::AND, [], true], - 'AND all true' => [CompositeRule::AND, ['easy_rule_true', 'easy_rule_true'], true], - 'AND last false' => [CompositeRule::AND, ['easy_rule_true', 'easy_rule_false'], false], + 'AND all true' => [CompositeRule::AND, [TrueRule::class, TrueRule::class], true], + 'AND last false' => [CompositeRule::AND, [TrueRule::class, FalseRule::class], false], 'OR empty' => [CompositeRule::OR, [], true], - 'OR all false' => [CompositeRule::OR, ['easy_rule_false', 'easy_rule_false'], false], - 'OR last true' => [CompositeRule::OR, ['easy_rule_false', 'easy_rule_true'], true], + 'OR all false' => [CompositeRule::OR, [FalseRule::class, FalseRule::class], false], + 'OR last true' => [CompositeRule::OR, [FalseRule::class, TrueRule::class], true], ]; } @@ -33,11 +33,7 @@ public static function dataCompositeRule(): array public function testCompositeRule(string $operator, array $rules, bool $expected): void { $rule = new CompositeRule($operator, $rules); - $ruleFactory = new SimpleRuleFactory([ - 'easy_rule_false' => new EasyRule(false), - 'easy_rule_true' => new EasyRule(true), - ]); - $result = $rule->execute('user', new Permission('permission'), new RuleContext($ruleFactory, [])); + $result = $rule->execute('user', new Permission('permission'), new RuleContext()); $this->assertSame($expected, $result); } diff --git a/tests/RuleContextTest.php b/tests/RuleContextTest.php index c27ea1877..fd80bba7c 100644 --- a/tests/RuleContextTest.php +++ b/tests/RuleContextTest.php @@ -6,34 +6,33 @@ use PHPUnit\Framework\TestCase; use Yiisoft\Rbac\RuleContext; -use Yiisoft\Rbac\SimpleRuleFactory; -use Yiisoft\Rbac\Tests\Support\EasyRule; +use Yiisoft\Rbac\Tests\Support\TrueRule; final class RuleContextTest extends TestCase { public function testGetParameters(): void { - $context = new RuleContext(new SimpleRuleFactory(['easy' => new EasyRule()]), ['a' => 1, 'b' => 2]); + $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); $this->assertSame(['a' => 1, 'b' => 2], $context->getParameters()); } public function testGetParameterValue(): void { - $context = new RuleContext(new SimpleRuleFactory(['easy' => new EasyRule()]), ['a' => 1, 'b' => 2]); + $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); $this->assertSame(1, $context->getParameterValue('a')); $this->assertNull($context->getParameterValue('c')); } public function testHasParameter(): void { - $context = new RuleContext(new SimpleRuleFactory(['easy' => new EasyRule()]), ['a' => 1, 'b' => 2]); + $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); $this->assertTrue($context->hasParameter('a')); $this->assertFalse($context->hasParameter('c')); } public function testCreateRule(): void { - $context = new RuleContext(new SimpleRuleFactory(['easy' => new EasyRule()]), ['a' => 1, 'b' => 2]); - $this->assertEquals(new EasyRule(), $context->createRule('easy')); + $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); + $this->assertEquals(new TrueRule(), $context->createRule(TrueRule::class)); } } diff --git a/tests/Support/EasyRule.php b/tests/Support/FalseRule.php similarity index 52% rename from tests/Support/EasyRule.php rename to tests/Support/FalseRule.php index 8ac9c6587..46d6cc56d 100644 --- a/tests/Support/EasyRule.php +++ b/tests/Support/FalseRule.php @@ -8,19 +8,10 @@ use Yiisoft\Rbac\RuleContext; use Yiisoft\Rbac\RuleInterface; -final class EasyRule implements RuleInterface +final class FalseRule implements RuleInterface { - public function __construct(private readonly bool $expected = true) - { - } - public function execute(?string $userId, Item $item, RuleContext $ruleContext): bool { - return $this->expected; - } - - public function getName(): string - { - return self::class; + return false; } } diff --git a/tests/Support/TrueRule.php b/tests/Support/TrueRule.php new file mode 100644 index 000000000..9ed061b29 --- /dev/null +++ b/tests/Support/TrueRule.php @@ -0,0 +1,17 @@ + Date: Wed, 14 Feb 2024 08:52:12 +0000 Subject: [PATCH 3/8] Apply fixes from StyleCI --- src/SimpleRuleFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SimpleRuleFactory.php b/src/SimpleRuleFactory.php index ec2fa5730..ad5ccd542 100644 --- a/src/SimpleRuleFactory.php +++ b/src/SimpleRuleFactory.php @@ -15,7 +15,7 @@ public function create(string $name): RuleInterface throw new RuleNotFoundException($name); } - $instance = new $name; + $instance = new $name(); if (!$instance instanceof RuleInterface) { throw new RuleInterfaceNotImplementedException($name); } From a22a47c716ab7436770a9c5c64f0dbaa29be0499 Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Wed, 14 Feb 2024 17:27:04 +0600 Subject: [PATCH 4/8] Fix failed workflows --- README.md | 4 ++-- src/SimpleRuleFactory.php | 5 ++--- tests/Common/ItemsStorageTestTrait.php | 5 +++-- tests/Common/ManagerLogicTestTrait.php | 19 +++++++++++++++++++ tests/PermissionTest.php | 3 ++- tests/RoleTest.php | 4 ++-- tests/Support/WannabeRule.php | 16 ++++++++++++++++ 7 files changed, 46 insertions(+), 10 deletions(-) create mode 100644 tests/Support/WannabeRule.php diff --git a/README.md b/README.md index 968852ef5..c10b932bd 100644 --- a/README.md +++ b/README.md @@ -175,13 +175,13 @@ use Yiisoft\Rbac\Permission; /** @var ManagerInterface $manager */ $manager->addPermission( - (new Permission('viewList'))->withRuleName('action_rule'), + (new Permission('viewList'))->withRuleName(ActionRule::class), ); // or $manager->addRole( - (new Role('NewYearMaintainer'))->withRuleName('new_year_only_rule') + (new Role('NewYearMaintainer'))->withRuleName(NewYearOnlyRule::class) ); ``` diff --git a/src/SimpleRuleFactory.php b/src/SimpleRuleFactory.php index ec2fa5730..cc8d28f43 100644 --- a/src/SimpleRuleFactory.php +++ b/src/SimpleRuleFactory.php @@ -15,11 +15,10 @@ public function create(string $name): RuleInterface throw new RuleNotFoundException($name); } - $instance = new $name; - if (!$instance instanceof RuleInterface) { + if (!is_a($name, RuleInterface::class, allow_string: true)) { throw new RuleInterfaceNotImplementedException($name); } - return $instance; + return new $name; } } diff --git a/tests/Common/ItemsStorageTestTrait.php b/tests/Common/ItemsStorageTestTrait.php index e434092be..6eb52b656 100644 --- a/tests/Common/ItemsStorageTestTrait.php +++ b/tests/Common/ItemsStorageTestTrait.php @@ -11,6 +11,7 @@ use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; use Yiisoft\Rbac\Tests\Support\FakeItemsStorage; +use Yiisoft\Rbac\Tests\Support\TrueRule; trait ItemsStorageTestTrait { @@ -66,7 +67,7 @@ public function testUpdate(string $itemName, string $parentNameForChildrenCheck, $item = $item ->withName('Super Admin') - ->withRuleName('super admin'); + ->withRuleName(TrueRule::class); $actionStorage->update($itemName, $item); $this->assertNull($testStorage->get($itemName)); @@ -75,7 +76,7 @@ public function testUpdate(string $itemName, string $parentNameForChildrenCheck, $this->assertNotNull($item); $this->assertSame('Super Admin', $item->getName()); - $this->assertSame('super admin', $item->getRuleName()); + $this->assertSame(TrueRule::class, $item->getRuleName()); $this->assertSame($expectedHasChildren, $testStorage->hasChildren($parentNameForChildrenCheck)); } diff --git a/tests/Common/ManagerLogicTestTrait.php b/tests/Common/ManagerLogicTestTrait.php index 2d6e6f8d1..bb5261a9f 100644 --- a/tests/Common/ManagerLogicTestTrait.php +++ b/tests/Common/ManagerLogicTestTrait.php @@ -12,9 +12,11 @@ use Yiisoft\Rbac\Assignment; use Yiisoft\Rbac\Exception\DefaultRolesNotFoundException; use Yiisoft\Rbac\Exception\ItemAlreadyExistsException; +use Yiisoft\Rbac\Exception\RuleInterfaceNotImplementedException; use Yiisoft\Rbac\Exception\RuleNotFoundException; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\Role; +use Yiisoft\Rbac\RuleInterface; use Yiisoft\Rbac\Tests\Support\AdsRule; use Yiisoft\Rbac\Tests\Support\AuthorRule; use Yiisoft\Rbac\Tests\Support\BanRule; @@ -23,6 +25,7 @@ use Yiisoft\Rbac\Tests\Support\GuestRule; use Yiisoft\Rbac\Tests\Support\SubscriptionRule; use Yiisoft\Rbac\Tests\Support\TrueRule; +use Yiisoft\Rbac\Tests\Support\WannabeRule; trait ManagerLogicTestTrait { @@ -254,6 +257,22 @@ public function testUserHasPermissionWithNonExistingRule(): void $manager->userHasPermission('reader A', 'test-permission'); } + public function testUserHasPermissionWithRuleMissingImplements(): void + { + $className = WannabeRule::class; + $interfaceName = RuleInterface::class; + $manager = $this + ->createFilledManager() + ->addPermission((new Permission('test-permission'))->withRuleName($className)) + ->addRole(new Role('test')) + ->addChild('test', 'test-permission') + ->assign('test-permission', 'reader A'); + + $this->expectException(RuleInterfaceNotImplementedException::class); + $this->expectExceptionMessage("Rule \"$className\" must implement \"$interfaceName\"."); + $manager->userHasPermission('reader A', 'test-permission'); + } + public function testCanAddExistingChild(): void { $manager = $this->createFilledManager(); diff --git a/tests/PermissionTest.php b/tests/PermissionTest.php index 050faa49f..e92b4fc7f 100644 --- a/tests/PermissionTest.php +++ b/tests/PermissionTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\TestCase; use Yiisoft\Rbac\Item; use Yiisoft\Rbac\Permission; +use Yiisoft\Rbac\Tests\Support\TrueRule; final class PermissionTest extends TestCase { @@ -17,7 +18,7 @@ public function testImmutability(): void $new2 = $original->withDescription('new description'); $new3 = $original->withUpdatedAt(1_642_029_084); $new4 = $original->withCreatedAt(1_642_029_084); - $new5 = $original->withRuleName('test'); + $new5 = $original->withRuleName(TrueRule::class); $this->assertNotSame($original, $new1); $this->assertNotSame($original, $new2); diff --git a/tests/RoleTest.php b/tests/RoleTest.php index 7e1a02fcb..cffdc303f 100644 --- a/tests/RoleTest.php +++ b/tests/RoleTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\TestCase; use Yiisoft\Rbac\Item; use Yiisoft\Rbac\Role; +use Yiisoft\Rbac\Tests\Support\TrueRule; final class RoleTest extends TestCase { @@ -17,8 +18,7 @@ public function testImmutability(): void $new2 = $original->withDescription('new description'); $new3 = $original->withUpdatedAt(1_642_029_084); $new4 = $original->withCreatedAt(1_642_029_084); - $new5 = $original->withRuleName('test'); - + $new5 = $original->withRuleName(TrueRule::class); $this->assertNotSame($original, $new1); $this->assertNotSame($original, $new2); diff --git a/tests/Support/WannabeRule.php b/tests/Support/WannabeRule.php new file mode 100644 index 000000000..c8888fb85 --- /dev/null +++ b/tests/Support/WannabeRule.php @@ -0,0 +1,16 @@ + Date: Wed, 14 Feb 2024 11:27:57 +0000 Subject: [PATCH 5/8] Apply fixes from StyleCI --- src/SimpleRuleFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SimpleRuleFactory.php b/src/SimpleRuleFactory.php index cc8d28f43..909212add 100644 --- a/src/SimpleRuleFactory.php +++ b/src/SimpleRuleFactory.php @@ -19,6 +19,6 @@ public function create(string $name): RuleInterface throw new RuleInterfaceNotImplementedException($name); } - return new $name; + return new $name(); } } From ccc84a48fe4a9dbabf5b2ee7254fcb7ccdd6b0a4 Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Wed, 14 Feb 2024 17:49:53 +0600 Subject: [PATCH 6/8] Add separate test --- tests/SimpleRuleFactoryTest.php | 38 +++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 tests/SimpleRuleFactoryTest.php diff --git a/tests/SimpleRuleFactoryTest.php b/tests/SimpleRuleFactoryTest.php new file mode 100644 index 000000000..ddd8a8a50 --- /dev/null +++ b/tests/SimpleRuleFactoryTest.php @@ -0,0 +1,38 @@ +assertEquals(new TrueRule(), (new SimpleRuleFactory())->create(TrueRule::class)); + } + + public function testCreateWithNonExistingRule(): void + { + $this->expectException(RuleNotFoundException::class); + $this->expectExceptionMessage('Rule "non-existing-rule" not found.'); + (new SimpleRuleFactory())->create('non-existing-rule'); + } + + public function testCreateWithRuleMissingImplements(): void + { + $className = WannabeRule::class; + $interfaceName = RuleInterface::class; + + $this->expectException(RuleInterfaceNotImplementedException::class); + $this->expectExceptionMessage("Rule \"$className\" must implement \"$interfaceName\"."); + (new SimpleRuleFactory())->create($className); + } +} From 161668952eb3070098b67a469c182f5fcff03f9e Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Thu, 15 Feb 2024 13:44:37 +0600 Subject: [PATCH 7/8] Review fix --- src/RuleContext.php | 5 +---- tests/CompositeRuleTest.php | 3 ++- tests/RuleContextTest.php | 9 +++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/RuleContext.php b/src/RuleContext.php index e248394ad..c92866e49 100644 --- a/src/RuleContext.php +++ b/src/RuleContext.php @@ -6,13 +6,10 @@ final class RuleContext { - private readonly RuleFactoryInterface $ruleFactory; - public function __construct( - ?RuleFactoryInterface $ruleFactory = null, + private readonly RuleFactoryInterface $ruleFactory, private readonly array $parameters = [], ) { - $this->ruleFactory = $ruleFactory ?? new SimpleRuleFactory(); } public function getParameters(): array diff --git a/tests/CompositeRuleTest.php b/tests/CompositeRuleTest.php index 363a58dae..800338dea 100644 --- a/tests/CompositeRuleTest.php +++ b/tests/CompositeRuleTest.php @@ -9,6 +9,7 @@ use Yiisoft\Rbac\CompositeRule; use Yiisoft\Rbac\Permission; use Yiisoft\Rbac\RuleContext; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\FalseRule; use Yiisoft\Rbac\Tests\Support\TrueRule; @@ -33,7 +34,7 @@ public static function dataCompositeRule(): array public function testCompositeRule(string $operator, array $rules, bool $expected): void { $rule = new CompositeRule($operator, $rules); - $result = $rule->execute('user', new Permission('permission'), new RuleContext()); + $result = $rule->execute('user', new Permission('permission'), new RuleContext(new SimpleRuleFactory())); $this->assertSame($expected, $result); } diff --git a/tests/RuleContextTest.php b/tests/RuleContextTest.php index fd80bba7c..28ceaaff1 100644 --- a/tests/RuleContextTest.php +++ b/tests/RuleContextTest.php @@ -6,33 +6,34 @@ use PHPUnit\Framework\TestCase; use Yiisoft\Rbac\RuleContext; +use Yiisoft\Rbac\SimpleRuleFactory; use Yiisoft\Rbac\Tests\Support\TrueRule; final class RuleContextTest extends TestCase { public function testGetParameters(): void { - $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); + $context = new RuleContext(new SimpleRuleFactory(), ['a' => 1, 'b' => 2]); $this->assertSame(['a' => 1, 'b' => 2], $context->getParameters()); } public function testGetParameterValue(): void { - $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); + $context = new RuleContext(new SimpleRuleFactory(), ['a' => 1, 'b' => 2]); $this->assertSame(1, $context->getParameterValue('a')); $this->assertNull($context->getParameterValue('c')); } public function testHasParameter(): void { - $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); + $context = new RuleContext(new SimpleRuleFactory(), ['a' => 1, 'b' => 2]); $this->assertTrue($context->hasParameter('a')); $this->assertFalse($context->hasParameter('c')); } public function testCreateRule(): void { - $context = new RuleContext(parameters: ['a' => 1, 'b' => 2]); + $context = new RuleContext(new SimpleRuleFactory(), ['a' => 1, 'b' => 2]); $this->assertEquals(new TrueRule(), $context->createRule(TrueRule::class)); } } From 50d8b2891efffa29dfac5739867aad0fea71eec0 Mon Sep 17 00:00:00 2001 From: Alexey Rogachev Date: Fri, 16 Feb 2024 13:12:42 +0600 Subject: [PATCH 8/8] Parameters are not optional too (review fix) --- src/RuleContext.php | 2 +- tests/CompositeRuleTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/RuleContext.php b/src/RuleContext.php index c92866e49..9b042f33f 100644 --- a/src/RuleContext.php +++ b/src/RuleContext.php @@ -8,7 +8,7 @@ final class RuleContext { public function __construct( private readonly RuleFactoryInterface $ruleFactory, - private readonly array $parameters = [], + private readonly array $parameters, ) { } diff --git a/tests/CompositeRuleTest.php b/tests/CompositeRuleTest.php index 800338dea..8ef87ee0c 100644 --- a/tests/CompositeRuleTest.php +++ b/tests/CompositeRuleTest.php @@ -34,7 +34,7 @@ public static function dataCompositeRule(): array public function testCompositeRule(string $operator, array $rules, bool $expected): void { $rule = new CompositeRule($operator, $rules); - $result = $rule->execute('user', new Permission('permission'), new RuleContext(new SimpleRuleFactory())); + $result = $rule->execute('user', new Permission('permission'), new RuleContext(new SimpleRuleFactory(), [])); $this->assertSame($expected, $result); }