Merge pull request #2 from dicoding-dev/fix/issue-check-with-inverted-whole-action-rule

[Bugfix] Comparison issue when facing with inverted (negated) rule that have whole field with specific action

Author: AlexzPurewoko

composer.json

@@ -3,7 +3,7 @@
     "description": "A core package for managing the authorization through CanCanCan style. With supports of granularity based on Attributes Based Access Control.",
     "type": "library",
     "license": "MIT",
-    "version": "0.6.0-stable",
+    "version": "0.6.1-stable",
     "scripts": {
         "test": "./vendor/bin/pest"
     },

src/Abilities/Core/Comparator/AbilityCheckerImpl.php

@@ -24,7 +24,10 @@ public function can(string $action, string $resource, string $scope, mixed $fiel
         $starActionRules = [];
         foreach ($unspecifiedActionRules as $unspecifiedActionRule) {
             /** 1. Checking on specific inverted rules */
-            if ($unspecifiedActionRule->isInverted() && $unspecifiedActionRule->getResource()->matchField($field)) {
+            if ($unspecifiedActionRule->isInverted() &&
+                $unspecifiedActionRule->getResource()->matchField($field) &&
+                $unspecifiedActionRule->getAction()->match($action)
+            ) {
                 return false; // as the correspondent user is prohibited access resource
             } elseif ($unspecifiedActionRule->getAction()->wholeAction()) {
                 $starActionRules[] = $unspecifiedActionRule;

src/Abilities/Core/Objects/Action.php

@@ -37,4 +37,13 @@ public function __toString(): string
     {
         return $this->get();
     }
+
+    public function match(string $action): bool
+    {
+        if ($this->wholeAction()) {
+            return true;
+        }
+
+        return $this->get() === $action;
+    }
 }

tests/Feature/Core/AbilityCheckerImplTest.php

@@ -157,6 +157,33 @@
         expect($abilityChecker->can('update', 'resource1', 'scope1', (object) ['author' => 666]))
             ->toBeFalse();
     });
+
+    it('must return as expected when has inverted specific action and whole field rule compared with rule that has specific field with whole action', function () {
+        $compiledRules = new CompiledRules([
+            (object) [
+                'id' => 2,
+                'rule' => '!scope1:resource1/*:review'
+            ],
+            (object) [
+                'id' => 3,
+                'rule' => 'scope1:resource1/{"author": 667}:*'
+            ]
+        ]);
+
+        $abilityChecker = new AbilityCheckerImpl($compiledRules);
+        expect($abilityChecker->can('update', 'resource1', 'scope1', (object) ['author' => 667]))
+            ->toBeTrue();
+        expect($abilityChecker->can('update', 'resource1', 'scope1'))
+            ->toBeFalse();
+        expect($abilityChecker->can('*', 'resource1', 'scope1'))
+            ->toBeFalse();
+        expect($abilityChecker->can('review', 'resource1', 'scope1', (object) ['author' => 667]))
+            ->toBeFalse();
+        expect($abilityChecker->can('review', 'resource1', 'scope1'))
+            ->toBeFalse();
+        expect($abilityChecker->can('review', 'resource1', 'scope1', (object) ['author' => 666]))
+            ->toBeFalse();
+    });
 });
 
 describe('cannot() feature function test', function () {

tests/Unit/Core/Objects/ActionTest.php

@@ -34,4 +34,23 @@
 it('can know if the action is whole action (star)', function () {
     expect((new Action('*'))->wholeAction())->toBeTrue();
 
+});
+
+describe('match() function test', function () {
+   it('must return true when the current action is whole action', function () {
+       $currentAction = new Action();
+
+       expect($currentAction->match('create'))->toBeTrue();
+       expect($currentAction->match('*'))->toBeTrue();
+   });
+
+   it ('must return true when the match with specific action', function () {
+       $currentAction = new Action('create');
+       expect($currentAction->match('create'))->toBeTrue();
+   });
+
+   it('must return false when not match with specific action', function () {
+       $currentAction = new Action('create');
+       expect($currentAction->match('update'))->toBeFalse();
+   });
 });