Skip to content

Commit fe1292b

Browse files
CarlSchwanbackportbot[bot]
CarlSchwan
authored and
backportbot[bot]
committed
perf: Perform share path validation early
Signed-off-by: Carl Schwan <carlschwan@kde.org> [skip ci]
1 parent 8d27686 commit fe1292b

2 files changed

Lines changed: 75 additions & 37 deletions

File tree

‎lib/private/Share20/Manager.php‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,7 +1072,7 @@ protected function promoteReshares(IShare $share): void {
10721072

10731073
// Figure out which users has some shares with which providers
10741074
$qb = $this->connection->getQueryBuilder();
1075-
$qb->select('uid_initiator', 'share_type')
1075+
$qb->select('uid_initiator', 'share_type', 'uid_owner', 'file_source')
10761076
->from('share')
10771077
->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY)))
10781078
->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter($shareTypes, IQueryBuilder::PARAM_INT_ARRAY)))
@@ -1094,47 +1094,52 @@ protected function promoteReshares(IShare $share): void {
10941094
$qb->orderBy('id');
10951095

10961096
$cursor = $qb->executeQuery();
1097+
/** @var array<string, list<array{IShare::TYPE_*, Node}>> $rawShare */
10971098
$rawShares = [];
10981099
while ($data = $cursor->fetch()) {
10991100
if (!isset($rawShares[$data['uid_initiator']])) {
11001101
$rawShares[$data['uid_initiator']] = [];
11011102
}
11021103
if (!in_array($data['share_type'], $rawShares[$data['uid_initiator']], true)) {
1103-
$rawShares[$data['uid_initiator']][] = $data['share_type'];
1104+
if ($node instanceof Folder) {
1105+
if ($data['file_source'] === null || $data['uid_owner'] === null) {
1106+
/* Ignore share of non-existing node */
1107+
continue;
1108+
}
1109+
1110+
// for federated shares the owner can be a remote user, in this
1111+
// case we use the initiator
1112+
if ($this->userManager->userExists($data['uid_owner'])) {
1113+
$userFolder = $this->rootFolder->getUserFolder($data['uid_owner']);
1114+
} else {
1115+
$userFolder = $this->rootFolder->getUserFolder($data['uid_initiator']);
1116+
}
1117+
$sharedNode = $userFolder->getFirstNodeById((int)$data['file_source']);
1118+
if (!$sharedNode) {
1119+
continue;
1120+
}
1121+
if ($node->getRelativePath($sharedNode->getPath()) !== null) {
1122+
$rawShares[$data['uid_initiator']][] = [(int)$data['share_type'], $sharedNode];
1123+
}
1124+
} elseif ($node instanceof File) {
1125+
$rawShares[$data['uid_initiator']][] = [(int)$data['share_type'], $node];
1126+
}
11041127
}
11051128
}
11061129
$cursor->closeCursor();
11071130

1108-
foreach ($rawShares as $userId => $shareTypes) {
1109-
foreach ($shareTypes as $shareType) {
1131+
foreach ($rawShares as $userId => $shareInfos) {
1132+
foreach ($shareInfos as $shareInfo) {
1133+
[$shareType, $sharedNode] = $shareInfo;
11101134
try {
11111135
$provider = $this->factory->getProviderForType($shareType);
11121136
} catch (ProviderException) {
11131137
continue;
11141138
}
11151139

1116-
1117-
if ($node instanceof Folder) {
1118-
/* We need to get all shares by this user to get subshares */
1119-
$shares = $provider->getSharesBy($userId, $shareType, null, false, -1, 0);
1120-
1121-
foreach ($shares as $share) {
1122-
try {
1123-
$path = $share->getNode()->getPath();
1124-
} catch (NotFoundException) {
1125-
/* Ignore share of non-existing node */
1126-
continue;
1127-
}
1128-
if ($node->getRelativePath($path) !== null) {
1129-
/* If relative path is not null it means the shared node is the same or in a subfolder */
1130-
$reshareRecords[] = $share;
1131-
}
1132-
}
1133-
} else {
1134-
$shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0);
1135-
foreach ($shares as $child) {
1136-
$reshareRecords[] = $child;
1137-
}
1140+
$shares = $provider->getSharesBy($userId, $shareType, $sharedNode, false, -1, 0);
1141+
foreach ($shares as $child) {
1142+
$reshareRecords[] = $child;
11381143
}
11391144
}
11401145
}

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,7 @@ public function testPromoteReshareFile(): void {
500500
->getMock();
501501

502502
$file = $this->createMock(File::class);
503+
$file->method('getId')->willReturn(42);
503504

504505
$share = $this->createMock(IShare::class);
505506
$share->method('getShareType')->willReturn(IShare::TYPE_USER);
@@ -528,6 +529,13 @@ public function testPromoteReshareFile(): void {
528529

529530
$manager->expects($this->exactly(1))->method('updateShare')->with($reShare);
530531

532+
$this->userManager->method('userExists')->willReturn(true);
533+
$userFolder = $this->createMock(Folder::class);
534+
$this->rootFolder->method('getUserFolder')->with('userA')->willReturn($userFolder);
535+
$userFolder->method('getFirstNodeById')
536+
->with(42)
537+
->willReturn($file);
538+
531539
$qb = $this->createMock(IQueryBuilder::class);
532540
$result = $this->createMock(IResult::class);
533541
$qb->method('select')
@@ -544,7 +552,7 @@ public function testPromoteReshareFile(): void {
544552
->willReturn($qb);
545553
$result->method('fetch')
546554
->willReturnOnConsecutiveCalls(
547-
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
555+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 42],
548556
false,
549557
);
550558

@@ -585,19 +593,35 @@ public function testPromoteReshare(): void {
585593
$reShareInOtherFolder->method('getNode')->willReturn($otherFolder);
586594

587595
$this->defaultProvider->method('getSharesBy')
588-
->willReturnCallback(function ($userId, $shareType, $node, $reshares, $limit, $offset) use ($reShare, $reShareInSubFolder, $reShareInOtherFolder) {
596+
->willReturnCallback(function ($userId, $shareType, $node, $reshares, $limit, $offset) use ($folder, $subFolder, $reShare, $reShareInSubFolder, $reShareInOtherFolder) {
589597
if ($shareType === IShare::TYPE_USER) {
590-
return match($userId) {
591-
'userB' => [$reShare,$reShareInSubFolder,$reShareInOtherFolder],
592-
};
593-
} else {
594-
return [];
598+
if ($userId === 'userB') {
599+
if ($node === $folder) {
600+
return [$reShare];
601+
}
602+
if ($node === $subFolder) {
603+
return [$reShareInSubFolder];
604+
}
605+
}
595606
}
607+
$this->fail();
596608
});
597609
$manager->method('generalCreateChecks')->willThrowException(new GenericShareException());
598610

599611
$manager->expects($this->exactly(2))->method('updateShare')->withConsecutive([$reShare], [$reShareInSubFolder]);
600612

613+
$this->userManager->method('userExists')->willReturn(true);
614+
$userFolder = $this->createMock(Folder::class);
615+
$this->rootFolder->method('getUserFolder')->with('userA')->willReturn($userFolder);
616+
$userFolder->method('getFirstNodeById')
617+
->willReturnCallback(function ($id) use ($subFolder, $otherFolder, $folder) {
618+
return match ($id) {
619+
41 => $subFolder,
620+
42 => $otherFolder,
621+
43 => $folder,
622+
};
623+
});
624+
601625
$qb = $this->createMock(IQueryBuilder::class);
602626
$result = $this->createMock(IResult::class);
603627
$qb->method('select')
@@ -614,7 +638,9 @@ public function testPromoteReshare(): void {
614638
->willReturn($qb);
615639
$result->method('fetch')
616640
->willReturnOnConsecutiveCalls(
617-
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
641+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 41],
642+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 42],
643+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 43],
618644
false,
619645
);
620646

@@ -646,6 +672,13 @@ public function testPromoteReshareWhenUserHasAnotherShare(): void {
646672
/* No share is promoted because generalCreateChecks does not throw */
647673
$manager->expects($this->never())->method('updateShare');
648674

675+
$this->userManager->method('userExists')->willReturn(true);
676+
$userFolder = $this->createMock(Folder::class);
677+
$this->rootFolder->method('getUserFolder')->with('userA')->willReturn($userFolder);
678+
$userFolder->method('getFirstNodeById')
679+
->with(42)
680+
->willReturn($folder);
681+
649682
$qb = $this->createMock(IQueryBuilder::class);
650683
$result = $this->createMock(IResult::class);
651684
$qb->method('select')
@@ -662,7 +695,7 @@ public function testPromoteReshareWhenUserHasAnotherShare(): void {
662695
->willReturn($qb);
663696
$result->method('fetch')
664697
->willReturnOnConsecutiveCalls(
665-
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
698+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 42],
666699
false,
667700
);
668701

@@ -739,8 +772,8 @@ public function testPromoteReshareOfUsersInGroupShare(): void {
739772
->willReturn($qb);
740773
$result->method('fetch')
741774
->willReturnOnConsecutiveCalls(
742-
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
743-
['uid_initiator' => 'userC', 'share_type' => IShare::TYPE_USER],
775+
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 42],
776+
['uid_initiator' => 'userC', 'share_type' => IShare::TYPE_USER, 'uid_owner' => 'userA', 'file_source' => 42],
744777
false,
745778
);
746779

0 commit comments

Comments
 (0)