Skip to content

Commit 9a4f926

Browse files
susnuxsusnux
authored
Merge pull request #62939 from nextcloud/backport/62871/stable32
[stable32] fix(sharing): properly handle verification of copy/move actions
2 parents fa5ae66 + 3051fc5 commit 9a4f926

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
@@ -181,7 +181,8 @@ public function createServer(
181181
$tree,
182182
$this->userSession,
183183
$userFolder,
184-
\OCP\Server::get(\OCP\Share\IManager::class)
184+
\OCP\Server::get(\OCP\Share\IManager::class),
185+
\OCP\Server::get(IRootFolder::class),
185186
));
186187
$server->addPlugin(new CommentPropertiesPlugin(\OCP\Server::get(ICommentsManager::class), $this->userSession));
187188
$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;
@@ -57,6 +58,7 @@ public function __construct(
5758
private IUserSession $userSession,
5859
private Folder $userFolder,
5960
private IManager $shareManager,
61+
private IRootFolder $rootFolder,
6062
) {
6163
$this->userId = $userSession->getUser()->getUID();
6264
}
@@ -88,7 +90,7 @@ public function initialize(Server $server) {
8890
* @param Node $node
8991
* @return IShare[]
9092
*/
91-
private function getShare(Node $node): array {
93+
private function getShare(Node $node, bool $includeIncoming = true): array {
9294
$result = [];
9395
$requestedShareTypes = [
9496
IShare::TYPE_USER,
@@ -111,6 +113,10 @@ private function getShare(Node $node): array {
111113
-1
112114
);
113115

116+
if (!$includeIncoming) {
117+
continue;
118+
}
119+
114120
// Also check for shares where the user is the recipient
115121
try {
116122
$result[] = $this->shareManager->getSharedWith(
@@ -127,6 +133,24 @@ private function getShare(Node $node): array {
127133
return array_merge(...$result);
128134
}
129135

136+
/**
137+
* @return IShare[]
138+
*/
139+
private function getSharesForTarget(Node $node): array {
140+
$shares = $this->getShare($node);
141+
if ($shares !== []) {
142+
return $shares;
143+
}
144+
145+
// also check the owner side
146+
$userRoot = $this->rootFolder->getUserFolder($this->userId);
147+
while (str_starts_with($node->getPath(), $userRoot->getPath() . '/')) {
148+
$shares = array_merge($shares, $this->getShare($node, false));
149+
$node = $node->getParent();
150+
}
151+
return $shares;
152+
}
153+
130154
/**
131155
* @param Folder $node
132156
* @return IShare[][]
@@ -245,8 +269,8 @@ public function validateMoveOrCopy(string $source, string $target): bool {
245269
return true;
246270
}
247271

248-
$targetShares = $this->getShare($targetNode->getNode());
249-
if (empty($targetShares)) {
272+
$targetShares = $this->getSharesForTarget($targetNode->getNode());
273+
if ($targetShares === []) {
250274
// Target is not a share so no re-sharing inprogress
251275
return true;
252276
}
@@ -267,7 +291,7 @@ public function validateMoveOrCopy(string $source, string $target): bool {
267291
// the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share
268292
//
269293
// since the 2-step move is allowed, we also allow both steps at once
270-
if ($sourceNode->isDeletable()) {
294+
if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) {
271295
return true;
272296
}
273297
}

‎apps/dav/lib/Server.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,7 @@ public function __construct(
340340
$userSession,
341341
$userFolder,
342342
$shareManager,
343+
\OCP\Server::get(IRootFolder::class),
343344
));
344345
$this->server->addPlugin(new CommentPropertiesPlugin(
345346
\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;
@@ -28,13 +29,15 @@ class SharesPluginTest extends \Test\TestCase {
2829
private \Sabre\DAV\Tree&MockObject $tree;
2930
private \OCP\Share\IManager&MockObject $shareManager;
3031
private Folder&MockObject $userFolder;
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')
@@ -49,7 +52,8 @@ protected function setUp(): void {
4952
$this->tree,
5053
$userSession,
5154
$this->userFolder,
52-
$this->shareManager
55+
$this->shareManager,
56+
$this->rootFolder,
5357
);
5458
$this->plugin->initialize($this->server);
5559
}

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

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,3 +315,114 @@ Scenario: Can copy file between shares if share permissions
315315
And the OCS status code should be "100"
316316
When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt"
317317
Then the HTTP status code should be "201"
318+
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"

0 commit comments

Comments
 (0)