Skip to content

fix: Use strict PHP comparaison #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
provokateurin merged 2 commits into from
Jan 26, 2026
Merged

fix: Use strict PHP comparaison #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c14b6f2
fix: Use strict PHP comparaison
CarlSchwan Jan 22, 2026
57ee6cd
fix: Use tag id as string only
CarlSchwan Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions lib/Service/ApprovalService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
use Sabre\DAV\INode;
use Sabre\DAV\PropFind;

/**
* @psalm-import-type Rule from RuleService
*/
class ApprovalService {

public function __construct(
Expand Down Expand Up @@ -164,7 +167,7 @@ public function getUserRules(string $userId, string $role = 'requesters', ?int $
* Check if a user is authorized to approve or request by a given rule
*
* @param string $userId
* @param array $rule
* @param Rule $rule
* @param string $role
* @return bool
*/
Expand All @@ -180,7 +183,7 @@ private function userIsAuthorizedByRule(string $userId, array $rule, string $rol
}));

// if user is in rule's user list
if (in_array($userId, $ruleUserIds)) {
if (in_array($userId, $ruleUserIds, true)) {
return true;
} else {
// if user is member of one rule's group list
Expand Down Expand Up @@ -285,15 +288,15 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
$rules = $this->ruleService->getRules();

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

// first check if it's approvable
foreach ($rules as $id => $rule) {
if (in_array($rule['tagPending'], $tags)
if (in_array($rule['tagPending'], $tags, true)
&& $this->userIsAuthorizedByRule($userId, $rule, 'approvers')) {
return [
'state' => Application::STATE_APPROVABLE,
Expand All @@ -304,7 +307,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce

// then check pending in priority
foreach ($rules as $id => $rule) {
if (in_array($rule['tagPending'], $tags)) {
if (in_array($rule['tagPending'], $tags, true)) {
return [
'state' => Application::STATE_PENDING,
'rule' => $rule,
Expand All @@ -313,7 +316,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
}
// then rejected
foreach ($rules as $id => $rule) {
if (in_array($rule['tagRejected'], $tags)) {
if (in_array($rule['tagRejected'], $tags, true)) {
return [
'state' => Application::STATE_REJECTED,
'rule' => $rule,
Expand All @@ -322,7 +325,7 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
}
// then approved
foreach ($rules as $id => $rule) {
if (in_array($rule['tagApproved'], $tags)) {
if (in_array($rule['tagApproved'], $tags, true)) {
return [
'state' => Application::STATE_APPROVED,
'rule' => $rule,
Expand Down Expand Up @@ -502,7 +505,7 @@ public function requestViaTagAssignment(int $fileId, int $ruleId, string $reques
* Share file with everybody who can approve with given rule and have no access yet
*
* @param int $fileId
* @param array $rule
* @param Rule $rule
* @param string $userId
* @return array list of created shares
* @throws \OCP\Files\NotPermittedException
Expand Down Expand Up @@ -626,15 +629,15 @@ public function getRuleAuthorizedUserIds(array $rule, string $role = 'approvers'
$ruleUserIds = [];
foreach ($rule[$role] as $approver) {
if ($approver['type'] === 'user') {
if (!in_array($approver['entityId'], $ruleUserIds)) {
if (!in_array($approver['entityId'], $ruleUserIds, true)) {
$ruleUserIds[] = $approver['entityId'];
}
} elseif ($approver['type'] === 'group') {
$groupId = $approver['entityId'];
if ($this->groupManager->groupExists($groupId)) {
$users = $this->groupManager->get($groupId)->getUsers();
foreach ($users as $user) {
if ($user instanceof IUser && !in_array($user->getUID(), $ruleUserIds)) {
if ($user instanceof IUser && !in_array($user->getUID(), $ruleUserIds, true)) {
$ruleUserIds[] = $user->getUID();
}
}
Expand All @@ -650,7 +653,7 @@ public function getRuleAuthorizedUserIds(array $rule, string $role = 'approvers'
continue;
}
$memberUserId = $member->getUserId();
if (!in_array($memberUserId, $ruleUserIds)) {
if (!in_array($memberUserId, $ruleUserIds, true)) {
$ruleUserIds[] = $memberUserId;
}
}
Expand Down Expand Up @@ -678,7 +681,7 @@ public function handleTagAssignmentEvent(int $fileId, array $tags): void {
$rules = $this->ruleService->getRules();
foreach ($rules as $id => $rule) {
// rule matches tags
if (in_array($rule['tagPending'], $tags)) {
if (in_array($rule['tagPending'], $tags, true)) {
$ruleInvolded = $rule;
break;
}
Expand Down Expand Up @@ -722,7 +725,7 @@ public function handleTagAssignmentEvent(int $fileId, array $tags): void {
* Send it to all users who are authorized to approve it
*
* @param int $fileId
* @param array $rule
* @param Rule $rule
* @param string $requestUserId
* @return void
*/
Expand All @@ -731,7 +734,7 @@ public function sendRequestNotification(int $fileId, array $rule, string $reques
$rulesUserIds = [];
$thisRuleUserIds = $this->getRuleAuthorizedUserIds($rule, 'approvers');
foreach ($thisRuleUserIds as $userId) {
if (!in_array($userId, $rulesUserIds)) {
if (!in_array($userId, $rulesUserIds, true)) {
$rulesUserIds[] = $userId;
}
}
Expand Down
28 changes: 20 additions & 8 deletions lib/Service/RuleService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@

use OCP\IUserManager;

/**
* @psalm-type Rule = array{
* id: int,
* tagPending: string,
* tagApproved: string,
* tagRejected: string,
* description: string,
* approvers: array,
* requesters: array,
* unapproveWhenModified: bool,
* }
*/
class RuleService {
/** @var array */
private $strTypeToInt;
Expand Down Expand Up @@ -318,7 +330,7 @@ public function deleteRule(int $id): array {
/**
* Get a rule by id
* @param int $id the rule id
* @return array|null the rule or null if not found
* @return Rule|null the rule or null if not found
*/
public function getRule(int $id): ?array {
$rule = null;
Expand All @@ -331,9 +343,9 @@ public function getRule(int $id): ?array {
);
$req = $qb->executeQuery();
while ($row = $req->fetch()) {
$tagPending = (int)$row['tag_pending'];
$tagApproved = (int)$row['tag_approved'];
$tagRejected = (int)$row['tag_rejected'];
$tagPending = (string)$row['tag_pending'];
$tagApproved = (string)$row['tag_approved'];
$tagRejected = (string)$row['tag_rejected'];
$description = $row['description'];
$rule = [
'id' => $id,
Expand Down Expand Up @@ -362,7 +374,7 @@ public function getRule(int $id): ?array {
/**
* Get all rules
*
* @return array
* @return array<int, Rule>
*/
public function getRules(): array {
if ($this->cachedRules === null) {
Expand All @@ -374,9 +386,9 @@ public function getRules(): array {
$req = $qb->executeQuery();
while ($row = $req->fetch()) {
$id = (int)$row['id'];
$tagPending = (int)$row['tag_pending'];
$tagApproved = (int)$row['tag_approved'];
$tagRejected = (int)$row['tag_rejected'];
$tagPending = (string)$row['tag_pending'];
$tagApproved = (string)$row['tag_approved'];
$tagRejected = (string)$row['tag_rejected'];
$description = $row['description'];
$rules[$id] = [
'id' => $id,
Expand Down
58 changes: 14 additions & 44 deletions tests/unit/Service/ApprovalServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,49 +28,19 @@
use Psr\Log\LoggerInterface;

class ApprovalServiceTest extends TestCase {

/**
* @var RuleService
*/
private $ruleService;
/**
* @var ApprovalService
*/
private $approvalService;
/**
* @var UtilsService
*/
private $utilsService;
/**
* @var IRootFolder
*/
private $root;
/**
* @var int
*/
private $idTagPending1;
/**
* @var int
*/
private $idTagApproved1;
/**
* @var int
*/
private $idTagRejected1;
/**
* @var int
*/
private $idRule1;
/**
* @var ISystemTagObjectMapper
*/
private $tagObjectMapper;
/**
* @var string[][]
*/
private $approvers1;
private $requesters1;
private $description1;
private RuleService $ruleService;
private ApprovalService $approvalService;
private UtilsService $utilsService;
private IRootFolder $root;
private int $idTagPending1;
private int $idTagApproved1;
private int $idTagRejected1;
private int $idRule1;
private ISystemTagObjectMapper $tagObjectMapper;
/** @var string[][] */
private array $approvers1;
private array $requesters1;
private string $description1;

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

public function testGetApprovalState() {
public function testGetApprovalState(): void {
$uf = $this->root->getUserFolder('user1');
$file1 = $uf->newFile('file1.txt', 'content');
$state = $this->approvalService->getApprovalState($file1->getId(), 'user1');
Expand Down
Loading