Skip to content

Commit 48f1cb8

Browse files
AlexzPurewokoAlexzPurewoko
authored
Merge pull request #3 from dicoding-dev/improvements/refactor-unset-ability
[Improvements] Refactor the unset abilities
2 parents 9e7edf6 + 15866fb commit 48f1cb8

File tree

10 files changed

+222
-31
lines changed

10 files changed

+222
-31
lines changed

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"description": "A core package for managing the authorization through CanCanCan style. With supports of granularity based on Attributes Based Access Control.",
44
"type": "library",
55
"license": "MIT",
6-
"version": "0.6.1-stable",
6+
"version": "0.6.2-stable",
77
"scripts": {
88
"test": "./vendor/bin/pest"
99
},

‎src/Abilities/Core/Comparator/AbilityCheckerImpl.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public function getExactRuleOf(string|Rule $ruleOrSyntax): ?Rule
7474

7575
$queriedRules = $this->compiledRules->queryRule(
7676
$ruleOrSyntax->getScope()->get(),
77-
$ruleOrSyntax->getResource()->getResource(),
77+
$ruleOrSyntax->getResource()->getResourceString(),
7878
$ruleOrSyntax->getAction()->get()
7979
);
8080

‎src/Abilities/Core/Objects/Action.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ public function __toString(): string
3838
return $this->get();
3939
}
4040

41-
public function match(string $action): bool
41+
public function match(string $otherAction): bool
4242
{
4343
if ($this->wholeAction()) {
4444
return true;
4545
}
4646

47-
return $this->get() === $action;
47+
return $this->get() === $otherAction;
4848
}
4949
}

‎src/Abilities/Core/Objects/CompiledRules.php‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,34 @@ public function queryRule(string $scope, string $resource, string $action): arra
2828
return [];
2929
}
3030

31+
if ($this->isWhole($resource)) {
32+
$result = [];
33+
$isWholeAction = $this->isWhole($action);
34+
foreach ($this->compiledRules[$scope] as $actions) {
35+
foreach ($actions as $arrayOfRule) {
36+
/** @var Rule $rule */
37+
foreach ($arrayOfRule as $rule) {
38+
if ($isWholeAction) {
39+
$result[] = $rule;
40+
continue;
41+
}
42+
43+
if ($this->matchAction($rule->getAction(), $action)) {
44+
$result[] = $rule;
45+
}
46+
}
47+
}
48+
}
49+
50+
return $result;
51+
}
52+
3153
if (!array_key_exists($resource, $this->compiledRules[$scope])) {
3254
return [];
3355
}
3456

3557
// if the action not specific, it will retrieve all actions (include global too)
36-
if (empty(trim($action))) {
58+
if ($this->isWhole($action)) {
3759
$unspecifiedActions = $this->compiledRules[$scope][$resource];
3860
$result = [];
3961
array_walk_recursive($unspecifiedActions, function($a) use (&$result) { $result[] = $a; });
@@ -47,14 +69,23 @@ public function queryRule(string $scope, string $resource, string $action): arra
4769
return $this->compiledRules[$scope][$resource][$action];
4870
}
4971

72+
private function matchAction(Action $action, string $checkedAction): bool
73+
{
74+
if ($checkedAction === '*' || $checkedAction === '') {
75+
return true;
76+
}
77+
78+
return $action->get() === $checkedAction;
79+
}
80+
5081
private function compile(): void
5182
{
5283
foreach ($this->rules as $rule) {
5384
$compiledRule = RuleCompiler::compile($rule->rule);
5485
$compiledRule->setRuleId($rule->id);
5586

5687
$scope = $compiledRule->getScope()->get();
57-
$resource = $compiledRule->getResource()->getResource();
88+
$resource = $compiledRule->getResource()->getResourceString();
5889
$action = $compiledRule->getAction()->get();
5990

6091
if (!array_key_exists($scope, $this->compiledRules)) {
@@ -76,4 +107,9 @@ private function compile(): void
76107
}
77108
}
78109
}
110+
111+
private function isWhole(string $str): bool
112+
{
113+
return empty($str) || $str === '*';
114+
}
79115
}

‎src/Abilities/Core/Objects/Resource.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ private function processField(): void
5454
);
5555
}
5656

57-
public function getResource(): string
57+
public function getResourceString(): string
5858
{
5959
return $this->resource;
6060
}
@@ -114,7 +114,7 @@ public function getFieldType(): FieldType
114114

115115
public function isEqualWith(self $other): bool
116116
{
117-
if ($other->getResource() !== $this->getResource()) {
117+
if ($other->getResourceString() !== $this->getResourceString()) {
118118
return false;
119119
}
120120

@@ -137,13 +137,13 @@ private function isSingularField(mixed $field): bool
137137
public function __toString(): string
138138
{
139139
if ($this->allField()) {
140-
return $this->getResource() . "/*";
140+
return $this->getResourceString() . "/*";
141141
}
142142

143143
if ($this->fieldType === FieldType::SINGULAR_FIELD) {
144-
return $this->getResource() . "/" . $this->getField();
144+
return $this->getResourceString() . "/" . $this->getField();
145145
}
146146

147-
return $this->getResource() . "/" . json_encode($this->getField());
147+
return $this->getResourceString() . "/" . json_encode($this->getField());
148148
}
149149
}

‎src/Abilities/Core/Objects/Rule.php‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,13 @@ public function __toString(): string
5757
{
5858
return ($this->isInverted() ? '!' : '') . $this->getScope() . ':' . $this->getResource() . ':' . $this->getAction();
5959
}
60-
}
60+
61+
public function withNewField(mixed $field): self
62+
{
63+
return new self(
64+
$this->getScope(),
65+
new Resource($this->getResource()->getResourceString(), $field),
66+
$this->getAction()
67+
);
68+
}
69+
}

‎src/Abilities/Core/Repository/MutableUserAbilityRepository.php‎

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use Abilities\Core\Comparator\AbilityCheckerImpl;
77
use Abilities\Core\Objects\Action;
88
use Abilities\Core\Objects\CompiledRules;
9+
use Abilities\Core\Objects\Enums\FieldType;
910
use Abilities\Core\Objects\Resource;
1011
use Abilities\Core\Objects\Rule;
1112
use Abilities\Core\Objects\Scope;
@@ -107,25 +108,79 @@ public function unsetAbility(string $action, string $resource, string $scope, mi
107108
throw new \Exception('Empty compiled rules');
108109
}
109110

110-
$composedNewRule = new Rule(
111-
new Scope($scope),
112-
new Resource($resource, $field),
113-
new Action($action),
114-
$inverted
115-
);
116-
117111
$queriedRules = $this->compiledRules->queryRule($scope, $resource, $action);
118112
foreach ($queriedRules as $rule) {
119-
if ($composedNewRule->getResource()->isEqualWith($rule->getResource()) &&
120-
$composedNewRule->isInverted() === $rule->isInverted()) {
113+
114+
if ($this->matchResource($rule->getResource(), $resource, $field) &&
115+
$this->matchAction($rule->getAction(), $action) &&
116+
$rule->isInverted() === $inverted
117+
) {
118+
if ($rule->getResource()->getFieldType() === FieldType::ARRAY) {
119+
$newRule = $this->removeItemRuleFromArray($rule, $field);
120+
if (!empty($newRule)) {
121+
$this->storage->onUpdateRule($rule->getRuleId(), $this->currentUserId, "$newRule");
122+
continue;
123+
}
124+
}
125+
121126
$this->storage->onDeleteSpecificRule($rule->getRuleId(), $this->currentUserId);
122-
break;
123127
}
124128
}
125129

126130
$this->refresh();
127131
}
128132

133+
private function removeItemRuleFromArray(Rule $rule, int|array|string $fields): ?Rule
134+
{
135+
$newFields = [];
136+
if (!is_array($fields)) {
137+
$fields = [$fields];
138+
}
139+
140+
$steadyFieldCount = 0;
141+
foreach ($rule->getResource()->getField() as $oldField) {
142+
if (!in_array($oldField, $fields)) {
143+
$newFields[] = $oldField;
144+
$steadyFieldCount++;
145+
}
146+
}
147+
148+
if ($steadyFieldCount === 0) {
149+
return null;
150+
}
151+
152+
if ($steadyFieldCount === 1) {
153+
return $rule->withNewField($newFields[0]);
154+
}
155+
156+
return $rule->withNewField($newFields);
157+
}
158+
159+
private function matchResource(
160+
Resource $resource,
161+
string $checkedResource,
162+
mixed $checkedResourceField
163+
): bool {
164+
if ($checkedResource === '*' ) {
165+
return $checkedResourceField === '*' || $resource->matchField($checkedResourceField);
166+
}
167+
168+
if ($resource->getResourceString() !== $checkedResource) {
169+
return false;
170+
}
171+
172+
return $resource->matchField($checkedResourceField);
173+
}
174+
175+
private function matchAction(Action $action, string $checkedAction): bool
176+
{
177+
if ($checkedAction === '*') {
178+
return true;
179+
}
180+
181+
return $action->match($checkedAction);
182+
}
183+
129184
/**
130185
* @inheritDoc
131186
*/

‎tests/Feature/Core/Repository/AbilityRepositoryImplTest.php‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,24 @@
156156
]);
157157
});
158158

159+
it('must delete the ability when the rule is matched', function () {
160+
$repository = new MutableUserAbilityRepository(
161+
1,
162+
$storage = new StorageFixture([
163+
1 => 'scope:resource/123:read',
164+
2 => 'scope:resource/4:*',
165+
3 => 'scope:resource2/123:read'
166+
])
167+
);
168+
169+
$repository->unsetAbility('read', '*', 'scope', 123);
170+
171+
expect($storage->getRules())
172+
->toEqual([
173+
2 => 'scope:resource/4:*'
174+
]);
175+
});
176+
159177
it('must not remove the ability when the rule is unmatched', function () {
160178
$repository = new MutableUserAbilityRepository(
161179
1,
@@ -165,7 +183,66 @@
165183
])
166184
);
167185

168-
$repository->unsetAbility('*', 'resource', 'scope', 123);
186+
$repository->unsetAbility('update', 'resource', 'scope', 123);
187+
188+
expect($storage->getRules())
189+
->toEqual([
190+
1 => 'scope:resource/123:read',
191+
2 => 'scope:resource/4:*'
192+
]);
193+
});
194+
195+
it('must only remove expected field when unset one item from arrayable field', function () {
196+
$repository = new MutableUserAbilityRepository(
197+
1,
198+
$storage = new StorageFixture([
199+
1 => 'scope:resource/123:read',
200+
2 => 'scope:resource/4:*',
201+
3 => 'scope:resource/[6, 7, 8]:read'
202+
])
203+
);
204+
205+
$repository->unsetAbility('read', 'resource', 'scope', 7);
206+
207+
expect($storage->getRules())
208+
->toEqual([
209+
1 => 'scope:resource/123:read',
210+
2 => 'scope:resource/4:*',
211+
3 => 'scope:resource/[6,8]:read'
212+
]);
213+
});
214+
215+
it('must correctly remove 2 item from 3 item arrayable fields', function () {
216+
$repository = new MutableUserAbilityRepository(
217+
1,
218+
$storage = new StorageFixture([
219+
1 => 'scope:resource/123:read',
220+
2 => 'scope:resource/4:*',
221+
3 => 'scope:resource/[6, 7, 8]:read'
222+
])
223+
);
224+
225+
$repository->unsetAbility('read', 'resource', 'scope', [6, 8]);
226+
227+
expect($storage->getRules())
228+
->toEqual([
229+
1 => 'scope:resource/123:read',
230+
2 => 'scope:resource/4:*',
231+
3 => 'scope:resource/7:read'
232+
]);
233+
});
234+
235+
it('must correctly remove the rule when unset all items from arrayable fields', function () {
236+
$repository = new MutableUserAbilityRepository(
237+
1,
238+
$storage = new StorageFixture([
239+
1 => 'scope:resource/123:read',
240+
2 => 'scope:resource/4:*',
241+
3 => 'scope:resource/[6, 7, 8]:read'
242+
])
243+
);
244+
245+
$repository->unsetAbility('read', 'resource', 'scope', [6, 8, 7]);
169246

170247
expect($storage->getRules())
171248
->toEqual([

‎tests/Unit/Core/Objects/CompiledRulesTest.php‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,27 @@
7373

7474
$rules2 = $compiledRules->queryRule('scope1', 'resource1', '*');
7575
expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules2))
76-
->toHaveCount(1)
77-
->toContain(1);
76+
->toHaveCount(4)
77+
->toContain(1, 2, 3, 4);
7878

7979

8080
$rules3 = $compiledRules->queryRule('scope2', 'resource1', 'read');
8181
expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules3))
8282
->toHaveCount(2)
8383
->toContain(5, 7);
8484
})->with([$compiledRules]);
85-
});
85+
86+
it('must return all rules inside the scope whatever resource and actions', function(CompiledRules $compiledRules) {
87+
$rules = $compiledRules->queryRule('scope1', '*', '*');
88+
expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules))
89+
->toHaveCount(5)
90+
->toContain(1, 2, 3, 4, 6);
91+
})->with([$compiledRules]);
92+
93+
it('must return all rules inside the scope with specific action and whatever resource', function(CompiledRules $compiledRules) {
94+
$rules = $compiledRules->queryRule('scope1', '*', 'update');
95+
expect(array_map(fn (Rule $rule) => $rule->getRuleId(), $rules))
96+
->toHaveCount(1)
97+
->toContain(6);
98+
})->with([$compiledRules]);
99+
});

0 commit comments

Comments
 (0)