Skip to content

Commit 029e2a1

Browse files
susnuxsusnux
authored
Merge pull request #62937 from nextcloud/backport/62871/stable34
[stable34] fix(sharing): properly handle verification of copy/move actions
2 parents c97ed8d + ac5b432 commit 029e2a1

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
@@ -185,7 +185,8 @@ public function createServer(
185185
$server->addPlugin(new SharesPlugin(
186186
$tree,
187187
$this->userSession,
188-
\OCP\Server::get(\OCP\Share\IManager::class)
188+
\OCP\Server::get(\OCP\Share\IManager::class),
189+
\OCP\Server::get(IRootFolder::class),
189190
));
190191
$server->addPlugin(new CommentPropertiesPlugin(\OCP\Server::get(ICommentsManager::class), $this->userSession));
191192
$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
@@ -11,6 +11,7 @@
1111
use OCA\DAV\Connector\Sabre\Exception\Forbidden;
1212
use OCA\DAV\Connector\Sabre\Node as DavNode;
1313
use OCP\Files\Folder;
14+
use OCP\Files\IRootFolder;
1415
use OCP\Files\Node;
1516
use OCP\Files\NotFoundException;
1617
use OCP\Files\Storage\ISharedStorage;
@@ -51,6 +52,7 @@ public function __construct(
5152
private Tree $tree,
5253
IUserSession $userSession,
5354
private IManager $shareManager,
55+
private IRootFolder $rootFolder,
5456
) {
5557
$this->userId = $userSession->getUser()->getUID();
5658
}
@@ -83,7 +85,7 @@ public function initialize(Server $server) {
8385
* @param Node $node
8486
* @return IShare[]
8587
*/
86-
private function getShare(Node $node): array {
88+
private function getShare(Node $node, bool $includeIncoming = true): array {
8789
$result = [];
8890
$requestedShareTypes = [
8991
IShare::TYPE_USER,
@@ -105,6 +107,10 @@ private function getShare(Node $node): array {
105107
-1
106108
);
107109

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

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

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

‎apps/dav/lib/Server.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,7 @@ public function __construct(
341341
$this->server->tree,
342342
$userSession,
343343
$shareManager,
344+
\OCP\Server::get(IRootFolder::class),
344345
));
345346
$this->server->addPlugin(new CommentPropertiesPlugin(
346347
\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
@@ -14,6 +14,7 @@
1414
use OCA\DAV\Connector\Sabre\SharesPlugin;
1515
use OCA\DAV\Upload\UploadFile;
1616
use OCP\Files\Folder;
17+
use OCP\Files\IRootFolder;
1718
use OCP\IUser;
1819
use OCP\IUserSession;
1920
use OCP\Share\IManager;
@@ -27,13 +28,15 @@ class SharesPluginTest extends \Test\TestCase {
2728
private \Sabre\DAV\Server $server;
2829
private \Sabre\DAV\Tree&MockObject $tree;
2930
private \OCP\Share\IManager&MockObject $shareManager;
31+
private IRootFolder&MockObject $rootFolder;
3032
private SharesPlugin $plugin;
3133

3234
protected function setUp(): void {
3335
parent::setUp();
3436
$this->server = new \Sabre\DAV\Server();
3537
$this->tree = $this->createMock(Tree::class);
3638
$this->shareManager = $this->createMock(IManager::class);
39+
$this->rootFolder = $this->createMock(IRootFolder::class);
3740
$user = $this->createMock(IUser::class);
3841
$user->expects($this->once())
3942
->method('getUID')
@@ -46,7 +49,8 @@ protected function setUp(): void {
4649
$this->plugin = new SharesPlugin(
4750
$this->tree,
4851
$userSession,
49-
$this->shareManager
52+
$this->shareManager,
53+
$this->rootFolder,
5054
);
5155
$this->plugin->initialize($this->server);
5256
}

‎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)