Skip to content

Commit 988cd7f

Browse files
susnuxsusnux
authored
Merge pull request #62871 from nextcloud/fix/sharing-copy-move
fix(sharing): properly handle verification of copy/move actions
2 parents bbcab34 + 133b3e5 commit 988cd7f

5 files changed

Lines changed: 147 additions & 6 deletions

File tree

‎apps/dav/lib/Connector/Sabre/ServerFactory.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,8 @@ public function createServer(
186186
$server->addPlugin(new SharesPlugin(
187187
$tree,
188188
$this->userSession,
189-
\OCP\Server::get(\OCP\Share\IManager::class)
189+
\OCP\Server::get(\OCP\Share\IManager::class),
190+
\OCP\Server::get(IRootFolder::class),
190191
));
191192
$server->addPlugin(new CommentPropertiesPlugin(\OCP\Server::get(ICommentsManager::class), $this->userSession));
192193
$server->addPlugin(new FilesReportPlugin(

‎apps/dav/lib/Connector/Sabre/SharesPlugin.php‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OCA\DAV\Connector\Sabre\Exception\Forbidden;
1313
use OCA\DAV\Connector\Sabre\Node as DavNode;
1414
use OCP\Files\Folder;
15+
use OCP\Files\IRootFolder;
1516
use OCP\Files\Node;
1617
use OCP\Files\NotFoundException;
1718
use OCP\Files\Storage\ISharedStorage;
@@ -52,6 +53,7 @@ public function __construct(
5253
private Tree $tree,
5354
IUserSession $userSession,
5455
private IManager $shareManager,
56+
private IRootFolder $rootFolder,
5557
) {
5658
$this->userId = $userSession->getUser()->getUID();
5759
}
@@ -84,7 +86,7 @@ public function initialize(Server $server) {
8486
* @param Node $node
8587
* @return IShare[]
8688
*/
87-
private function getShare(Node $node): array {
89+
private function getShare(Node $node, bool $includeIncoming = true): array {
8890
$result = [];
8991
$requestedShareTypes = [
9092
IShare::TYPE_USER,
@@ -106,6 +108,10 @@ private function getShare(Node $node): array {
106108
-1
107109
);
108110

111+
if (!$includeIncoming) {
112+
continue;
113+
}
114+
109115
// Also check for shares where the user is the recipient
110116
try {
111117
$result[] = $this->shareManager->getSharedWith(
@@ -122,6 +128,24 @@ private function getShare(Node $node): array {
122128
return array_merge(...$result);
123129
}
124130

131+
/**
132+
* @return IShare[]
133+
*/
134+
private function getSharesForTarget(Node $node): array {
135+
$shares = $this->getShare($node);
136+
if ($shares !== []) {
137+
return $shares;
138+
}
139+
140+
// also check the owner side
141+
$userRoot = $this->rootFolder->getUserFolder($this->userId);
142+
while (str_starts_with($node->getPath(), $userRoot->getPath() . '/')) {
143+
$shares = array_merge($shares, $this->getShare($node, false));
144+
$node = $node->getParent();
145+
}
146+
return $shares;
147+
}
148+
125149
/**
126150
* @param Folder $node
127151
* @return IShare[][]
@@ -237,8 +261,8 @@ public function validateMoveOrCopy(string $source, string $target): bool {
237261
return true;
238262
}
239263

240-
$targetShares = $this->getShare($targetNode->getNode());
241-
if (empty($targetShares)) {
264+
$targetShares = $this->getSharesForTarget($targetNode->getNode());
265+
if ($targetShares === []) {
242266
// Target is not a share so no re-sharing inprogress
243267
return true;
244268
}
@@ -259,7 +283,7 @@ public function validateMoveOrCopy(string $source, string $target): bool {
259283
// the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share
260284
//
261285
// since the 2-step move is allowed, we also allow both steps at once
262-
if ($sourceNode->isDeletable()) {
286+
if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) {
263287
return true;
264288
}
265289
}

‎apps/dav/lib/Server.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,7 @@ public function __construct(
343343
$this->server->tree,
344344
$userSession,
345345
$shareManager,
346+
\OCP\Server::get(IRootFolder::class),
346347
));
347348
$this->server->addPlugin(new CommentPropertiesPlugin(
348349
\OCP\Server::get(ICommentsManager::class),

‎apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use OCA\DAV\Connector\Sabre\SharesPlugin;
1616
use OCA\DAV\Upload\UploadFile;
1717
use OCP\Files\Folder;
18+
use OCP\Files\IRootFolder;
1819
use OCP\IUser;
1920
use OCP\IUserSession;
2021
use OCP\Share\IManager;
@@ -28,13 +29,15 @@ class SharesPluginTest extends \Test\TestCase {
2829
private \Sabre\DAV\Server $server;
2930
private \Sabre\DAV\Tree&MockObject $tree;
3031
private \OCP\Share\IManager&MockObject $shareManager;
32+
private IRootFolder&MockObject $rootFolder;
3133
private SharesPlugin $plugin;
3234

3335
protected function setUp(): void {
3436
parent::setUp();
3537
$this->server = new \Sabre\DAV\Server();
3638
$this->tree = $this->createMock(Tree::class);
3739
$this->shareManager = $this->createMock(IManager::class);
40+
$this->rootFolder = $this->createMock(IRootFolder::class);
3841
$user = $this->createMock(IUser::class);
3942
$user->expects($this->once())
4043
->method('getUID')
@@ -47,7 +50,8 @@ protected function setUp(): void {
4750
$this->plugin = new SharesPlugin(
4851
$this->tree,
4952
$userSession,
50-
$this->shareManager
53+
$this->shareManager,
54+
$this->rootFolder,
5155
);
5256
$this->plugin->initialize($this->server);
5357
}

‎build/integration/sharing_features/sharing-v1-part4.feature‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,117 @@ Scenario: Can copy file between shares if share permissions
316316
When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt"
317317
Then the HTTP status code should be "201"
318318

319+
Scenario: Cannot copy files from share without share permission into subfolder of other share
320+
Given user "user0" exists
321+
Given user "user1" exists
322+
Given user "user2" exists
323+
And As an "user0"
324+
And user "user0" created a folder "/share"
325+
When creating a share with
326+
| path | share |
327+
| shareType | 0 |
328+
| shareWith | user1 |
329+
| permissions | 7 |
330+
Then the HTTP status code should be "200"
331+
And the OCS status code should be "100"
332+
And User "user0" uploads file with content "test" to "/share/test.txt"
333+
And As an "user1"
334+
And user "user1" created a folder "/re-share"
335+
And user "user1" created a folder "/re-share/subfolder"
336+
When creating a share with
337+
| path | re-share |
338+
| shareType | 0 |
339+
| shareWith | user2 |
340+
| permissions | 31 |
341+
Then the HTTP status code should be "200"
342+
And the OCS status code should be "100"
343+
When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
344+
Then the HTTP status code should be "403"
345+
346+
Scenario: Cannot move files from share without share permission into subfolder of other share
347+
Given user "user0" exists
348+
Given user "user1" exists
349+
Given user "user2" exists
350+
And As an "user0"
351+
And user "user0" created a folder "/share"
352+
When creating a share with
353+
| path | share |
354+
| shareType | 0 |
355+
| shareWith | user1 |
356+
| permissions | 7 |
357+
Then the HTTP status code should be "200"
358+
And the OCS status code should be "100"
359+
And User "user0" uploads file with content "test" to "/share/test.txt"
360+
And As an "user1"
361+
And user "user1" created a folder "/re-share"
362+
And user "user1" created a folder "/re-share/subfolder"
363+
When creating a share with
364+
| path | re-share |
365+
| shareType | 0 |
366+
| shareWith | user2 |
367+
| permissions | 31 |
368+
Then the HTTP status code should be "200"
369+
And the OCS status code should be "100"
370+
When User "user1" moves file "/share/test.txt" to "/re-share/subfolder/movetest.txt"
371+
Then the HTTP status code should be "403"
372+
373+
Scenario: Cannot move folder containing share without share permission into subfolder of other share
374+
Given user "user0" exists
375+
Given user "user1" exists
376+
Given user "user2" exists
377+
And As an "user0"
378+
And user "user0" created a folder "/share"
379+
When creating a share with
380+
| path | share |
381+
| shareType | 0 |
382+
| shareWith | user1 |
383+
| permissions | 7 |
384+
Then the HTTP status code should be "200"
385+
And the OCS status code should be "100"
386+
And User "user0" uploads file with content "test" to "/share/test.txt"
387+
And As an "user1"
388+
And user "user1" created a folder "/contains-share"
389+
When User "user1" moves file "/share" to "/contains-share/share"
390+
Then the HTTP status code should be "201"
391+
And user "user1" created a folder "/re-share"
392+
And user "user1" created a folder "/re-share/subfolder"
393+
When creating a share with
394+
| path | re-share |
395+
| shareType | 0 |
396+
| shareWith | user2 |
397+
| permissions | 31 |
398+
Then the HTTP status code should be "200"
399+
And the OCS status code should be "100"
400+
When User "user1" moves file "/contains-share" to "/re-share/subfolder/movetest"
401+
Then the HTTP status code should be "403"
402+
403+
Scenario: Can copy file between shares into subfolder if share permissions
404+
Given user "user0" exists
405+
Given user "user1" exists
406+
Given user "user2" exists
407+
And As an "user0"
408+
And user "user0" created a folder "/share"
409+
When creating a share with
410+
| path | share |
411+
| shareType | 0 |
412+
| shareWith | user1 |
413+
| permissions | 31 |
414+
Then the HTTP status code should be "200"
415+
And the OCS status code should be "100"
416+
And User "user0" uploads file with content "test" to "/share/test.txt"
417+
And As an "user1"
418+
And user "user1" created a folder "/re-share"
419+
And user "user1" created a folder "/re-share/subfolder"
420+
When creating a share with
421+
| path | re-share |
422+
| shareType | 0 |
423+
| shareWith | user2 |
424+
| permissions | 31 |
425+
Then the HTTP status code should be "200"
426+
And the OCS status code should be "100"
427+
When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
428+
Then the HTTP status code should be "201"
429+
319430
Scenario: Group deletes removes mount without marking
320431
Given As an "admin"
321432
And user "user0" exists

0 commit comments

Comments
 (0)