Skip to content

Commit 0719b49

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #62938 from nextcloud/backport/62871/stable33
[stable33] fix(sharing): properly handle verification of copy/move actions
2 parents 20b34a3 + ffefab5 commit 0719b49

6 files changed

Lines changed: 148 additions & 6 deletions

File tree

‎.github/workflows/integration-sqlite.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ jobs:
129129
extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, imagick, intl, json, ldap, libxml, mbstring, openssl, pcntl, posix, redis, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite
130130
coverage: none
131131
ini-file: development
132+
ini-values: disable_functions=""
132133
env:
133134
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
134135

‎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
}
@@ -82,7 +84,7 @@ public function initialize(Server $server) {
8284
* @param Node $node
8385
* @return IShare[]
8486
*/
85-
private function getShare(Node $node): array {
87+
private function getShare(Node $node, bool $includeIncoming = true): array {
8688
$result = [];
8789
$requestedShareTypes = [
8890
IShare::TYPE_USER,
@@ -104,6 +106,10 @@ private function getShare(Node $node): array {
104106
-1
105107
);
106108

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

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

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

‎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
$this->server->tree,
341341
$userSession,
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;
@@ -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)