Skip to content

Commit ab1e8c2

Browse files
provokateurinprovokateurin
authored
Merge pull request #362 from nextcloud/carl/strict-comparaison
2 parents ba77b9d + 57ee6cd commit ab1e8c2

File tree

3 files changed

+53
-68
lines changed

3 files changed

+53
-68
lines changed

‎lib/Service/ApprovalService.php‎

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@
2929
use Sabre\DAV\INode;
3030
use Sabre\DAV\PropFind;
3131

32+
/**
33+
* @psalm-import-type Rule from RuleService
34+
*/
3235
class ApprovalService {
3336

3437
public function __construct(
@@ -162,7 +165,7 @@ public function getUserRules(string $userId, string $role = 'requesters', ?int $
162165
* Check if a user is authorized to approve or request by a given rule
163166
*
164167
* @param string $userId
165-
* @param array $rule
168+
* @param Rule $rule
166169
* @param string $role
167170
* @return bool
168171
*/
@@ -178,7 +181,7 @@ private function userIsAuthorizedByRule(string $userId, array $rule, string $rol
178181
}));
179182

180183
// if user is in rule's user list
181-
if (in_array($userId, $ruleUserIds)) {
184+
if (in_array($userId, $ruleUserIds, true)) {
182185
return true;
183186
} else {
184187
// if user is member of one rule's group list
@@ -295,15 +298,15 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
295298
$rules = $this->ruleService->getRules();
296299

297300
// Get all tags a file has to prevent needing to check for each tag in every rule
298-
$tags = $this->tagObjectMapper->getTagIdsForObjects([$fileId], 'files');
299-
if (!array_key_exists($fileId, $tags)) {
301+
$tags = $this->tagObjectMapper->getTagIdsForObjects([(string)$fileId], 'files');
302+
if (!array_key_exists((string)$fileId, $tags)) {
300303
return ['state' => Application::STATE_NOTHING];
301304
}
302-
$tags = $tags[$fileId];
305+
$tags = array_map(static fn ($tag): string => (string)$tag, $tags[(string)$fileId]);
303306

304307
// first check if it's approvable
305308
foreach ($rules as $id => $rule) {
306-
if (in_array($rule['tagPending'], $tags)
309+
if (in_array($rule['tagPending'], $tags, true)
307310
&& $this->userIsAuthorizedByRule($userId, $rule, 'approvers')) {
308311
return [
309312
'state' => Application::STATE_APPROVABLE,
@@ -314,7 +317,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
314317

315318
// then check pending in priority
316319
foreach ($rules as $id => $rule) {
317-
if (in_array($rule['tagPending'], $tags)) {
320+
if (in_array($rule['tagPending'], $tags, true)) {
318321
return [
319322
'state' => Application::STATE_PENDING,
320323
'rule' => $rule,
@@ -323,7 +326,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
323326
}
324327
// then rejected
325328
foreach ($rules as $id => $rule) {
326-
if (in_array($rule['tagRejected'], $tags)) {
329+
if (in_array($rule['tagRejected'], $tags, true)) {
327330
return [
328331
'state' => Application::STATE_REJECTED,
329332
'rule' => $rule,
@@ -332,7 +335,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
332335
}
333336
// then approved
334337
foreach ($rules as $id => $rule) {
335-
if (in_array($rule['tagApproved'], $tags)) {
338+
if (in_array($rule['tagApproved'], $tags, true)) {
336339
return [
337340
'state' => Application::STATE_APPROVED,
338341
'rule' => $rule,
@@ -522,7 +525,7 @@ public function requestViaTagAssignment(int $fileId, int $ruleId, string $reques
522525
* Share file with everybody who can approve with given rule and have no access yet
523526
*
524527
* @param int $fileId
525-
* @param array $rule
528+
* @param Rule $rule
526529
* @param string $userId
527530
* @return array list of created shares
528531
* @throws \OCP\Files\NotPermittedException
@@ -646,15 +649,15 @@ public function getRuleAuthorizedUserIds(array $rule, string $role = 'approvers'
646649
$ruleUserIds = [];
647650
foreach ($rule[$role] as $approver) {
648651
if ($approver['type'] === 'user') {
649-
if (!in_array($approver['entityId'], $ruleUserIds)) {
652+
if (!in_array($approver['entityId'], $ruleUserIds, true)) {
650653
$ruleUserIds[] = $approver['entityId'];
651654
}
652655
} elseif ($approver['type'] === 'group') {
653656
$groupId = $approver['entityId'];
654657
if ($this->groupManager->groupExists($groupId)) {
655658
$users = $this->groupManager->get($groupId)->getUsers();
656659
foreach ($users as $user) {
657-
if ($user instanceof IUser && !in_array($user->getUID(), $ruleUserIds)) {
660+
if ($user instanceof IUser && !in_array($user->getUID(), $ruleUserIds, true)) {
658661
$ruleUserIds[] = $user->getUID();
659662
}
660663
}
@@ -670,7 +673,7 @@ public function getRuleAuthorizedUserIds(array $rule, string $role = 'approvers'
670673
continue;
671674
}
672675
$memberUserId = $member->getUserId();
673-
if (!in_array($memberUserId, $ruleUserIds)) {
676+
if (!in_array($memberUserId, $ruleUserIds, true)) {
674677
$ruleUserIds[] = $memberUserId;
675678
}
676679
}
@@ -698,7 +701,7 @@ public function handleTagAssignmentEvent(int $fileId, array $tags): void {
698701
$rules = $this->ruleService->getRules();
699702
foreach ($rules as $id => $rule) {
700703
// rule matches tags
701-
if (in_array($rule['tagPending'], $tags)) {
704+
if (in_array($rule['tagPending'], $tags, true)) {
702705
$ruleInvolded = $rule;
703706
break;
704707
}
@@ -742,7 +745,7 @@ public function handleTagAssignmentEvent(int $fileId, array $tags): void {
742745
* Send it to all users who are authorized to approve it
743746
*
744747
* @param int $fileId
745-
* @param array $rule
748+
* @param Rule $rule
746749
* @param string $requestUserId
747750
* @return void
748751
*/
@@ -751,7 +754,7 @@ public function sendRequestNotification(int $fileId, array $rule, string $reques
751754
$rulesUserIds = [];
752755
$thisRuleUserIds = $this->getRuleAuthorizedUserIds($rule, 'approvers');
753756
foreach ($thisRuleUserIds as $userId) {
754-
if (!in_array($userId, $rulesUserIds)) {
757+
if (!in_array($userId, $rulesUserIds, true)) {
755758
$rulesUserIds[] = $userId;
756759
}
757760
}

‎lib/Service/RuleService.php‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,18 @@
1616

1717
use OCP\IUserManager;
1818

19+
/**
20+
* @psalm-type Rule = array{
21+
* id: int,
22+
* tagPending: string,
23+
* tagApproved: string,
24+
* tagRejected: string,
25+
* description: string,
26+
* approvers: array,
27+
* requesters: array,
28+
* unapproveWhenModified: bool,
29+
* }
30+
*/
1931
class RuleService {
2032
/** @var array */
2133
private $strTypeToInt;
@@ -318,7 +330,7 @@ public function deleteRule(int $id): array {
318330
/**
319331
* Get a rule by id
320332
* @param int $id the rule id
321-
* @return array|null the rule or null if not found
333+
* @return Rule|null the rule or null if not found
322334
*/
323335
public function getRule(int $id): ?array {
324336
$rule = null;
@@ -331,9 +343,9 @@ public function getRule(int $id): ?array {
331343
);
332344
$req = $qb->executeQuery();
333345
while ($row = $req->fetch()) {
334-
$tagPending = (int)$row['tag_pending'];
335-
$tagApproved = (int)$row['tag_approved'];
336-
$tagRejected = (int)$row['tag_rejected'];
346+
$tagPending = (string)$row['tag_pending'];
347+
$tagApproved = (string)$row['tag_approved'];
348+
$tagRejected = (string)$row['tag_rejected'];
337349
$description = $row['description'];
338350
$rule = [
339351
'id' => $id,
@@ -362,7 +374,7 @@ public function getRule(int $id): ?array {
362374
/**
363375
* Get all rules
364376
*
365-
* @return array
377+
* @return array<int, Rule>
366378
*/
367379
public function getRules(): array {
368380
if ($this->cachedRules === null) {
@@ -374,9 +386,9 @@ public function getRules(): array {
374386
$req = $qb->executeQuery();
375387
while ($row = $req->fetch()) {
376388
$id = (int)$row['id'];
377-
$tagPending = (int)$row['tag_pending'];
378-
$tagApproved = (int)$row['tag_approved'];
379-
$tagRejected = (int)$row['tag_rejected'];
389+
$tagPending = (string)$row['tag_pending'];
390+
$tagApproved = (string)$row['tag_approved'];
391+
$tagRejected = (string)$row['tag_rejected'];
380392
$description = $row['description'];
381393
$rules[$id] = [
382394
'id' => $id,

‎tests/unit/Service/ApprovalServiceTest.php‎

Lines changed: 14 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -28,49 +28,19 @@
2828
use Psr\Log\LoggerInterface;
2929

3030
class ApprovalServiceTest extends TestCase {
31-
32-
/**
33-
* @var RuleService
34-
*/
35-
private $ruleService;
36-
/**
37-
* @var ApprovalService
38-
*/
39-
private $approvalService;
40-
/**
41-
* @var UtilsService
42-
*/
43-
private $utilsService;
44-
/**
45-
* @var IRootFolder
46-
*/
47-
private $root;
48-
/**
49-
* @var int
50-
*/
51-
private $idTagPending1;
52-
/**
53-
* @var int
54-
*/
55-
private $idTagApproved1;
56-
/**
57-
* @var int
58-
*/
59-
private $idTagRejected1;
60-
/**
61-
* @var int
62-
*/
63-
private $idRule1;
64-
/**
65-
* @var ISystemTagObjectMapper
66-
*/
67-
private $tagObjectMapper;
68-
/**
69-
* @var string[][]
70-
*/
71-
private $approvers1;
72-
private $requesters1;
73-
private $description1;
31+
private RuleService $ruleService;
32+
private ApprovalService $approvalService;
33+
private UtilsService $utilsService;
34+
private IRootFolder $root;
35+
private int $idTagPending1;
36+
private int $idTagApproved1;
37+
private int $idTagRejected1;
38+
private int $idRule1;
39+
private ISystemTagObjectMapper $tagObjectMapper;
40+
/** @var string[][] */
41+
private array $approvers1;
42+
private array $requesters1;
43+
private string $description1;
7444

7545
public static function setUpBeforeClass(): void {
7646
$app = new Application();
@@ -271,7 +241,7 @@ public function testGetRuleAuthorizedUserIds() {
271241
$this->assertEquals('user1', $uidRequesters[0]);
272242
}
273243

274-
public function testGetApprovalState() {
244+
public function testGetApprovalState(): void {
275245
$uf = $this->root->getUserFolder('user1');
276246
$file1 = $uf->newFile('file1.txt', 'content');
277247
$state = $this->approvalService->getApprovalState($file1->getId(), 'user1');

0 commit comments

Comments
 (0)