Merge pull request #3 from dicoding-dev/improvements/refactor-unset-a…

…bility

[Improvements] Refactor the unset abilities

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.1-stable",
+    "version": "0.6.2-stable",
     "scripts": {
         "test": "./vendor/bin/pest"
     },

src/Abilities/Core/Comparator/AbilityCheckerImpl.php

@@ -74,7 +74,7 @@ public function getExactRuleOf(string|Rule $ruleOrSyntax): ?Rule
 
         $queriedRules = $this->compiledRules->queryRule(
             $ruleOrSyntax->getScope()->get(),
-            $ruleOrSyntax->getResource()->getResource(),
+            $ruleOrSyntax->getResource()->getResourceString(),
             $ruleOrSyntax->getAction()->get()
         );
 

src/Abilities/Core/Objects/Action.php

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

src/Abilities/Core/Objects/CompiledRules.php

@@ -28,12 +28,34 @@ public function queryRule(string $scope, string $resource, string $action): arra
             return [];
         }
 
+        if ($this->isWhole($resource)) {
+            $result = [];
+            $isWholeAction = $this->isWhole($action);
+            foreach ($this->compiledRules[$scope] as $actions) {
+                foreach ($actions as $arrayOfRule) {
+                    /** @var Rule $rule */
+                    foreach ($arrayOfRule as $rule) {
+                        if ($isWholeAction) {
+                            $result[] = $rule;
+                            continue;
+                        }
+
+                        if ($this->matchAction($rule->getAction(), $action)) {
+                            $result[] = $rule;
+                        }
+                    }
+                }
+            }
+
+            return $result;
+        }
+
         if (!array_key_exists($resource, $this->compiledRules[$scope])) {
             return [];
         }
 
         // if the action not specific, it will retrieve all actions (include global too)
-        if (empty(trim($action))) {
+        if ($this->isWhole($action)) {
             $unspecifiedActions = $this->compiledRules[$scope][$resource];
             $result = [];
             array_walk_recursive($unspecifiedActions, function($a) use (&$result) { $result[] = $a; });
@@ -47,14 +69,23 @@ public function queryRule(string $scope, string $resource, string $action): arra
         return $this->compiledRules[$scope][$resource][$action];
     }
 
+    private function matchAction(Action $action, string $checkedAction): bool
+    {
+        if ($checkedAction === '*' || $checkedAction === '') {
+            return true;
+        }
+
+        return $action->get() === $checkedAction;
+    }
+
     private function compile(): void
     {
         foreach ($this->rules as $rule) {
             $compiledRule = RuleCompiler::compile($rule->rule);
             $compiledRule->setRuleId($rule->id);
 
             $scope = $compiledRule->getScope()->get();
-            $resource = $compiledRule->getResource()->getResource();
+            $resource = $compiledRule->getResource()->getResourceString();
             $action = $compiledRule->getAction()->get();
 
             if (!array_key_exists($scope, $this->compiledRules)) {
@@ -76,4 +107,9 @@ private function compile(): void
             }
         }
     }
+
+    private function isWhole(string $str): bool
+    {
+        return empty($str) || $str === '*';
+    }
 }

src/Abilities/Core/Objects/Resource.php

@@ -54,7 +54,7 @@ private function processField(): void
         );
     }
 
-    public function getResource(): string
+    public function getResourceString(): string
     {
         return $this->resource;
     }
@@ -114,7 +114,7 @@ public function getFieldType(): FieldType
 
     public function isEqualWith(self $other): bool
     {
-        if ($other->getResource() !== $this->getResource()) {
+        if ($other->getResourceString() !== $this->getResourceString()) {
             return false;
         }
 
@@ -137,13 +137,13 @@ private function isSingularField(mixed $field): bool
     public function __toString(): string
     {
         if ($this->allField()) {
-            return $this->getResource() . "/*";
+            return $this->getResourceString() . "/*";
         }
 
         if ($this->fieldType === FieldType::SINGULAR_FIELD) {
-            return $this->getResource() . "/" . $this->getField();
+            return $this->getResourceString() . "/" . $this->getField();
         }
 
-        return $this->getResource() . "/" . json_encode($this->getField());
+        return $this->getResourceString() . "/" . json_encode($this->getField());
     }
 }

src/Abilities/Core/Objects/Rule.php

@@ -57,4 +57,13 @@ public function __toString(): string
     {
         return ($this->isInverted() ? '!' : '') . $this->getScope() . ':' . $this->getResource() . ':' . $this->getAction();
     }
-}
+
+    public function withNewField(mixed $field): self
+    {
+        return new self(
+            $this->getScope(),
+            new Resource($this->getResource()->getResourceString(), $field),
+            $this->getAction()
+        );
+    }
+}

src/Abilities/Core/Repository/MutableUserAbilityRepository.php

@@ -6,6 +6,7 @@
 use Abilities\Core\Comparator\AbilityCheckerImpl;
 use Abilities\Core\Objects\Action;
 use Abilities\Core\Objects\CompiledRules;
+use Abilities\Core\Objects\Enums\FieldType;
 use Abilities\Core\Objects\Resource;
 use Abilities\Core\Objects\Rule;
 use Abilities\Core\Objects\Scope;
@@ -107,25 +108,79 @@ public function unsetAbility(string $action, string $resource, string $scope, mi
             throw new \Exception('Empty compiled rules');
         }
 
-        $composedNewRule = new Rule(
-            new Scope($scope),
-            new Resource($resource, $field),
-            new Action($action),
-            $inverted
-        );
-
         $queriedRules = $this->compiledRules->queryRule($scope, $resource, $action);
         foreach ($queriedRules as $rule) {
-            if ($composedNewRule->getResource()->isEqualWith($rule->getResource()) &&
-                $composedNewRule->isInverted() === $rule->isInverted()) {
+
+            if ($this->matchResource($rule->getResource(), $resource, $field) &&
+                $this->matchAction($rule->getAction(), $action) &&
+                $rule->isInverted() === $inverted
+            ) {
+                if ($rule->getResource()->getFieldType() === FieldType::ARRAY) {
+                    $newRule = $this->removeItemRuleFromArray($rule, $field);
+                    if (!empty($newRule)) {
+                        $this->storage->onUpdateRule($rule->getRuleId(), $this->currentUserId, "$newRule");
+                        continue;
+                    }
+                }
+
                 $this->storage->onDeleteSpecificRule($rule->getRuleId(), $this->currentUserId);
-                break;
             }
         }
 
         $this->refresh();
     }
 
+    private function removeItemRuleFromArray(Rule $rule, int|array|string $fields): ?Rule
+    {
+        $newFields = [];
+        if (!is_array($fields)) {
+            $fields = [$fields];
+        }
+
+        $steadyFieldCount = 0;
+        foreach ($rule->getResource()->getField() as $oldField) {
+            if (!in_array($oldField, $fields)) {
+                $newFields[] = $oldField;
+                $steadyFieldCount++;
+            }
+        }
+
+        if ($steadyFieldCount === 0) {
+            return null;
+        }
+
+        if ($steadyFieldCount === 1) {
+            return $rule->withNewField($newFields[0]);
+        }
+
+        return $rule->withNewField($newFields);
+    }
+
+    private function matchResource(
+        Resource $resource,
+        string $checkedResource,
+        mixed $checkedResourceField
+    ): bool {
+        if ($checkedResource === '*' ) {
+            return $checkedResourceField === '*' || $resource->matchField($checkedResourceField);
+        }
+
+        if ($resource->getResourceString() !== $checkedResource) {
+            return false;
+        }
+
+        return $resource->matchField($checkedResourceField);
+    }
+
+    private function matchAction(Action $action, string $checkedAction): bool
+    {
+        if ($checkedAction === '*') {
+            return true;
+        }
+
+        return $action->match($checkedAction);
+    }
+
     /**
      * @inheritDoc
      */

tests/Feature/Core/Repository/AbilityRepositoryImplTest.php

@@ -156,6 +156,24 @@
             ]);
     });
 
+    it('must delete the ability when the rule is matched', function () {
+        $repository = new MutableUserAbilityRepository(
+            1,
+            $storage = new StorageFixture([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource2/123:read'
+            ])
+        );
+
+        $repository->unsetAbility('read', '*', 'scope', 123);
+
+        expect($storage->getRules())
+            ->toEqual([
+                2 => 'scope:resource/4:*'
+            ]);
+    });
+
     it('must not remove the ability when the rule is unmatched', function () {
         $repository = new MutableUserAbilityRepository(
             1,
@@ -165,7 +183,66 @@
             ])
         );
 
-        $repository->unsetAbility('*', 'resource', 'scope', 123);
+        $repository->unsetAbility('update', 'resource', 'scope', 123);
+
+        expect($storage->getRules())
+            ->toEqual([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*'
+            ]);
+    });
+
+    it('must only remove expected field when unset one item from arrayable field', function () {
+        $repository = new MutableUserAbilityRepository(
+            1,
+            $storage = new StorageFixture([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource/[6, 7, 8]:read'
+            ])
+        );
+
+        $repository->unsetAbility('read', 'resource', 'scope', 7);
+
+        expect($storage->getRules())
+            ->toEqual([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource/[6,8]:read'
+            ]);
+    });
+
+    it('must correctly remove 2 item from 3 item arrayable fields', function () {
+        $repository = new MutableUserAbilityRepository(
+            1,
+            $storage = new StorageFixture([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource/[6, 7, 8]:read'
+            ])
+        );
+
+        $repository->unsetAbility('read', 'resource', 'scope', [6, 8]);
+
+        expect($storage->getRules())
+            ->toEqual([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource/7:read'
+            ]);
+    });
+
+    it('must correctly remove the rule when unset all items from arrayable fields', function () {
+        $repository = new MutableUserAbilityRepository(
+            1,
+            $storage = new StorageFixture([
+                1 => 'scope:resource/123:read',
+                2 => 'scope:resource/4:*',
+                3 => 'scope:resource/[6, 7, 8]:read'
+            ])
+        );
+
+        $repository->unsetAbility('read', 'resource', 'scope', [6, 8, 7]);
 
         expect($storage->getRules())
             ->toEqual([

tests/Unit/Core/Objects/CompiledRulesTest.php

@@ -73,13 +73,27 @@
 
         $rules2 = $compiledRules->queryRule('scope1', 'resource1', '*');
         expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules2))
-            ->toHaveCount(1)
-            ->toContain(1);
+            ->toHaveCount(4)
+            ->toContain(1, 2, 3, 4);
 
 
         $rules3 = $compiledRules->queryRule('scope2', 'resource1', 'read');
         expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules3))
             ->toHaveCount(2)
             ->toContain(5, 7);
     })->with([$compiledRules]);
-});
+
+    it('must return all rules inside the scope whatever resource and actions', function(CompiledRules $compiledRules) {
+        $rules = $compiledRules->queryRule('scope1', '*', '*');
+        expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules))
+            ->toHaveCount(5)
+            ->toContain(1, 2, 3, 4, 6);
+    })->with([$compiledRules]);
+
+    it('must return all rules inside the scope with specific action and whatever resource', function(CompiledRules $compiledRules) {
+        $rules = $compiledRules->queryRule('scope1', '*', 'update');
+        expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules))
+            ->toHaveCount(1)
+            ->toContain(6);
+    })->with([$compiledRules]);
+});