Skip to content

Commit cc67fd6

Browse files
susnuxsusnux
authored
Merge pull request #66395 from nextcloud/backport/66375/stable35
[stable35] Fix: sharing attribute resolution
2 parents eb94b77 + cc55a77 commit cc67fd6

5 files changed

Lines changed: 420 additions & 16 deletions

File tree

‎apps/files_sharing/lib/Updater.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OC\Files\Cache\FileAccess;
1212
use OC\Files\Filesystem;
1313
use OC\Files\Mount\MountPoint;
14+
use OC\Share20\Manager;
1415
use OCP\Constants;
1516
use OCP\Files\Folder;
1617
use OCP\Files\Mount\IMountManager;
@@ -64,6 +65,10 @@ private static function moveShareInOrOutOfShare($path): void {
6465

6566
$shareManager = Server::get(\OCP\Share\IManager::class);
6667

68+
if ($shareManager instanceof Manager) {
69+
$shareManager->restrictDownloadOfReshares($src);
70+
}
71+
6772
// We intentionally include invalid shares, as they have been automatically invalidated due to the node no longer
6873
// being accessible for the user. Only in this case where we adjust the share after it was moved we want to ignore
6974
// this to be able to still adjust it.

‎apps/files_sharing/tests/ReshareAttributesTest.php‎

Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Files_Sharing\Tests;
11+
12+
use OC\Files\View;
13+
use OCA\Files_Sharing\Helper;
14+
use OCP\Constants;
15+
use OCP\Files\NotFoundException;
16+
use OCP\IDBConnection;
17+
use OCP\Server;
18+
use OCP\Share\IShare;
19+
use PHPUnit\Framework\Attributes\Group;
20+
21+
#[Group(name: 'DB')]
22+
class ReshareAttributesTest extends TestCase {
23+
public static function setUpBeforeClass(): void {
24+
parent::setUpBeforeClass();
25+
Helper::registerHooks();
26+
}
27+
28+
protected function setUp(): void {
29+
parent::setUp();
30+
31+
$this->view->mkdir('folder');
32+
$this->view->mkdir('folder/sub');
33+
$this->view->file_put_contents('folder/sub/file.txt', $this->data);
34+
}
35+
36+
private function createShare(int $type, string $path, string $initiator, ?string $recipient = null): IShare {
37+
$this->loginHelper($initiator);
38+
if ($recipient !== null) {
39+
return $this->share($type, $path, $initiator, $recipient, Constants::PERMISSION_ALL);
40+
}
41+
42+
$share = $this->shareManager->newShare()
43+
->setShareType($type)
44+
->setSharedBy($initiator)
45+
->setNode($this->rootFolder->getUserFolder($initiator)->get($path))
46+
->setPermissions(Constants::PERMISSION_READ);
47+
return $this->shareManager->createShare($share);
48+
}
49+
50+
private function setDownload(IShare $share, bool $canDownload): void {
51+
$this->loginHelper($share->getSharedBy());
52+
$share = $this->shareManager->getShareById($share->getFullId());
53+
$attributes = $share->getAttributes() ?? $share->newAttributes();
54+
$attributes->setAttribute('permissions', 'download', $canDownload);
55+
$share->setAttributes($attributes);
56+
$this->shareManager->updateShare($share);
57+
}
58+
59+
private function canDownload(IShare $share): bool {
60+
return $this->shareManager->getShareById($share->getFullId())->canDownload();
61+
}
62+
63+
/**
64+
* Moving between storages on object store primary storage creates new
65+
* file ids, so the shares of the moved node no longer resolve.
66+
*/
67+
private function assertCannotServeDownloads(IShare $share): void {
68+
$share = $this->shareManager->getShareById($share->getFullId());
69+
try {
70+
$share->getNode();
71+
$nodeExists = true;
72+
} catch (NotFoundException) {
73+
$nodeExists = false;
74+
}
75+
$this->assertFalse($nodeExists && $share->canDownload());
76+
}
77+
78+
public function testRestrictingDownloadRestrictsReshareChain(): void {
79+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
80+
$reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
81+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
82+
$fileLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub/file.txt', self::TEST_FILES_SHARING_API_USER2);
83+
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder/sub', self::TEST_FILES_SHARING_API_USER3);
84+
85+
$this->setDownload($share, false);
86+
87+
$this->assertFalse($this->canDownload($reshare));
88+
$this->assertFalse($this->canDownload($link));
89+
$this->assertFalse($this->canDownload($fileLink));
90+
$this->assertFalse($this->canDownload($chainedLink));
91+
}
92+
93+
public function testRestrictingDownloadKeepsSharesOfTheOwner(): void {
94+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
95+
$ownerShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3);
96+
$ownerLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER1);
97+
98+
$this->setDownload($share, false);
99+
100+
$this->assertTrue($this->canDownload($ownerShare));
101+
$this->assertTrue($this->canDownload($ownerLink));
102+
}
103+
104+
public function testRestrictingDownloadKeepsResharesWithAnotherAllowingShare(): void {
105+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
106+
$groupShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER1, 'group1');
107+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
108+
109+
$this->setDownload($share, false);
110+
$this->assertTrue($this->canDownload($link));
111+
112+
$this->setDownload($groupShare, false);
113+
$this->assertFalse($this->canDownload($link));
114+
}
115+
116+
public function testRestrictingDownloadRestrictsCyclicReshares(): void {
117+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
118+
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
119+
$cyclicShare = $this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1');
120+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
121+
122+
$this->setDownload($share, false);
123+
124+
$this->assertFalse($this->canDownload($cyclicShare));
125+
$this->assertFalse($this->canDownload($link));
126+
}
127+
128+
public function testRestrictingDownloadKeepsCyclicResharesReachableFromTheOwner(): void {
129+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
130+
$otherShare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER3);
131+
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER4);
132+
$this->createShare(IShare::TYPE_GROUP, 'folder', self::TEST_FILES_SHARING_API_USER3, 'group1');
133+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
134+
135+
$this->setDownload($share, false);
136+
$this->assertTrue($this->canDownload($link));
137+
138+
$this->setDownload($otherShare, false);
139+
$this->assertFalse($this->canDownload($link));
140+
}
141+
142+
public function testRestrictingDownloadOfReshareRestrictsItsReshares(): void {
143+
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
144+
$reshare = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
145+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
146+
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER3);
147+
148+
$this->setDownload($reshare, false);
149+
150+
$this->assertTrue($this->canDownload($link));
151+
$this->assertFalse($this->canDownload($chainedLink));
152+
}
153+
154+
public function testAllowingDownloadAgainKeepsResharesRestricted(): void {
155+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
156+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
157+
158+
$this->setDownload($share, false);
159+
$this->setDownload($share, true);
160+
161+
$this->assertFalse($this->canDownload($link));
162+
}
163+
164+
public function testUpdatingPermissionsKeepsResharesUnchanged(): void {
165+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
166+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
167+
168+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER1);
169+
$share = $this->shareManager->getShareById($share->getFullId());
170+
$share->setPermissions(Constants::PERMISSION_READ | Constants::PERMISSION_SHARE);
171+
$this->shareManager->updateShare($share);
172+
173+
$this->assertNull($this->shareManager->getShareById($link->getFullId())->getAttributes());
174+
}
175+
176+
public function testRestrictingDownloadRestrictsResharesOfUnresolvableSharers(): void {
177+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
178+
$link = $this->createShare(IShare::TYPE_LINK, 'folder', self::TEST_FILES_SHARING_API_USER2);
179+
180+
$qb = Server::get(IDBConnection::class)->getQueryBuilder();
181+
$qb->update('share')
182+
->set('uid_initiator', $qb->createNamedParameter('non-existing-user'))
183+
->where($qb->expr()->eq('id', $qb->createNamedParameter($link->getId())))
184+
->executeStatement();
185+
186+
$this->setDownload($share, false);
187+
188+
$this->assertFalse($this->canDownload($link));
189+
}
190+
191+
public function testMovingIntoRestrictedShareRestrictsReshares(): void {
192+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
193+
$this->setDownload($share, false);
194+
195+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
196+
$view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
197+
$view->mkdir('own');
198+
$view->file_put_contents('own/file.txt', $this->data);
199+
$link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2);
200+
$reshare = $this->createShare(IShare::TYPE_USER, 'own', self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3);
201+
$chainedLink = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER3);
202+
203+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
204+
$view->rename('own', 'folder/own');
205+
206+
$this->assertCannotServeDownloads($link);
207+
$this->assertCannotServeDownloads($reshare);
208+
$this->assertCannotServeDownloads($chainedLink);
209+
}
210+
211+
public function testMovingByOwnerIntoRestrictedShareRestrictsReshares(): void {
212+
$this->view->mkdir('open');
213+
$this->view->mkdir('open/moved');
214+
$share = $this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
215+
$this->createShare(IShare::TYPE_USER, 'open', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
216+
$this->setDownload($share, false);
217+
$link = $this->createShare(IShare::TYPE_LINK, 'open/moved', self::TEST_FILES_SHARING_API_USER2);
218+
$this->assertTrue($this->canDownload($link));
219+
220+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER1);
221+
$this->view->rename('open/moved', 'folder/moved');
222+
223+
$this->assertFalse($this->canDownload($link));
224+
}
225+
226+
public function testMovingIntoAllowingShareKeepsReshares(): void {
227+
$this->createShare(IShare::TYPE_USER, 'folder', self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2);
228+
229+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
230+
$view = new View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
231+
$view->mkdir('own');
232+
$link = $this->createShare(IShare::TYPE_LINK, 'own', self::TEST_FILES_SHARING_API_USER2);
233+
234+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
235+
$view->rename('own', 'folder/own');
236+
237+
$this->assertTrue($this->canDownload($link));
238+
}
239+
}

‎lib/private/Files/View.php‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1902,32 +1902,29 @@ private function targetIsNotShared(string $user, string $targetPath): bool {
19021902
$providers = [
19031903
IShare::TYPE_USER,
19041904
IShare::TYPE_GROUP,
1905+
IShare::TYPE_LINK,
19051906
IShare::TYPE_EMAIL,
19061907
IShare::TYPE_CIRCLE,
19071908
IShare::TYPE_ROOM,
19081909
IShare::TYPE_DECK,
19091910
];
19101911
$shareManager = Server::get(IManager::class);
1911-
/** @var IShare[] $shares */
1912-
$shares = array_merge(...array_map(function (int $type) use ($shareManager, $user) {
1913-
return $shareManager->getSharesBy($user, $type);
1914-
}, $providers));
1912+
$rootFolder = Server::get(IRootFolder::class);
1913+
$userFolderPath = '/' . $user . '/files';
19151914

1916-
foreach ($shares as $share) {
1915+
for ($path = $targetPath; str_starts_with($path, $userFolderPath . '/'); $path = dirname($path)) {
19171916
try {
1918-
$sharedPath = $share->getNode()->getPath();
1919-
} catch (NotFoundException $e) {
1920-
// node is not found, ignoring
1921-
$this->logger->debug(
1922-
'Could not find the node linked to a share',
1923-
['app' => 'files', 'exception' => $e]);
1917+
$node = $rootFolder->get($path);
1918+
} catch (NotFoundException) {
19241919
continue;
19251920
}
1926-
if ($targetPath === $sharedPath || str_starts_with($targetPath, $sharedPath . '/')) {
1927-
$this->logger->debug(
1928-
'It is not allowed to move one mount point into a shared folder',
1929-
['app' => 'files']);
1930-
return false;
1921+
foreach ($providers as $type) {
1922+
if ($shareManager->getSharesBy($user, $type, $node, false, 1) !== []) {
1923+
$this->logger->debug(
1924+
'It is not allowed to move one mount point into a shared folder',
1925+
['app' => 'files']);
1926+
return false;
1927+
}
19311928
}
19321929
}
19331930

0 commit comments

Comments
 (0)