Skip to content

Commit 27e7763

Browse files
susnuxsusnux
committed
fix(sharing): properly handle verification of copy/move actions
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
1 parent 5816a00 commit 27e7763

5 files changed

Lines changed: 154 additions & 6 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCP\EventDispatcher\IEventDispatcher;
1919
use OCP\Files\Folder;
2020
use OCP\Files\IFilenameValidator;
21+
use OCP\Files\IRootFolder;
2122
use OCP\Files\Mount\IMountManager;
2223
use OCP\IConfig;
2324
use OCP\IDateTimeZone;
@@ -146,7 +147,8 @@ public function createServer(string $baseUri,
146147
$objectTree,
147148
$this->userSession,
148149
$userFolder,
149-
\OC::$server->getShareManager()
150+
\OCP\Server::get(\OCP\Share\IManager::class),
151+
\OCP\Server::get(IRootFolder::class),
150152
));
151153
$server->addPlugin(new CommentPropertiesPlugin(\OC::$server->getCommentsManager(), $this->userSession));
152154
$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;
@@ -49,6 +50,7 @@ public function __construct(
4950
private IUserSession $userSession,
5051
private Folder $userFolder,
5152
private IManager $shareManager,
53+
private IRootFolder $rootFolder,
5254
) {
5355
$this->userId = $userSession->getUser()->getUID();
5456
}
@@ -79,7 +81,7 @@ public function initialize(Server $server) {
7981
* @param Node $node
8082
* @return IShare[]
8183
*/
82-
private function getShare(Node $node): array {
84+
private function getShare(Node $node, bool $includeIncoming = true): array {
8385
$result = [];
8486
$requestedShareTypes = [
8587
IShare::TYPE_USER,
@@ -102,6 +104,10 @@ private function getShare(Node $node): array {
102104
-1
103105
));
104106

107+
if (!$includeIncoming) {
108+
continue;
109+
}
110+
105111
// Also check for shares where the user is the recipient
106112
try {
107113
$result = array_merge($result, $this->shareManager->getSharedWith(
@@ -118,6 +124,24 @@ private function getShare(Node $node): array {
118124
return $result;
119125
}
120126

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

236-
$targetShares = $this->getShare($targetNode->getNode());
237-
if (empty($targetShares)) {
260+
$targetShares = $this->getSharesForTarget($targetNode->getNode());
261+
if ($targetShares === []) {
238262
// Target is not a share so no re-sharing inprogress
239263
return true;
240264
}
@@ -255,7 +279,7 @@ public function validateMoveOrCopy(string $source, string $target): bool {
255279
// the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share
256280
//
257281
// since the 2-step move is allowed, we also allow both steps at once
258-
if ($sourceNode->isDeletable()) {
282+
if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) {
259283
return true;
260284
}
261285
}

‎apps/dav/lib/Server.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
use OCP\Diagnostics\IEventLogger;
7373
use OCP\EventDispatcher\IEventDispatcher;
7474
use OCP\Files\IFilenameValidator;
75+
use OCP\Files\IRootFolder;
7576
use OCP\FilesMetadata\IFilesMetadataManager;
7677
use OCP\IAppConfig;
7778
use OCP\ICacheFactory;
@@ -316,6 +317,7 @@ public function __construct(
316317
$userSession,
317318
$userFolder,
318319
$shareManager,
320+
\OCP\Server::get(IRootFolder::class),
319321
));
320322
$this->server->addPlugin(new CommentPropertiesPlugin(
321323
\OC::$server->getCommentsManager(),

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@
1313
use OCA\DAV\Connector\Sabre\SharesPlugin;
1414
use OCA\DAV\Upload\UploadFile;
1515
use OCP\Files\Folder;
16+
use OCP\Files\IRootFolder;
1617
use OCP\IUser;
1718
use OCP\IUserSession;
1819
use OCP\Share\IManager;
1920
use OCP\Share\IShare;
21+
use PHPUnit\Framework\MockObject\MockObject;
2022
use Sabre\DAV\Tree;
2123

2224
class SharesPluginTest extends \Test\TestCase {
@@ -47,11 +49,17 @@ class SharesPluginTest extends \Test\TestCase {
4749
*/
4850
private $plugin;
4951

52+
/**
53+
* @var IRootFolder&MockObject
54+
*/
55+
private $rootFolder;
56+
5057
protected function setUp(): void {
5158
parent::setUp();
5259
$this->server = new \Sabre\DAV\Server();
5360
$this->tree = $this->createMock(Tree::class);
5461
$this->shareManager = $this->createMock(IManager::class);
62+
$this->rootFolder = $this->createMock(IRootFolder::class);
5563
$user = $this->createMock(IUser::class);
5664
$user->expects($this->once())
5765
->method('getUID')
@@ -66,7 +74,8 @@ protected function setUp(): void {
6674
$this->tree,
6775
$userSession,
6876
$this->userFolder,
69-
$this->shareManager
77+
$this->shareManager,
78+
$this->rootFolder,
7079
);
7180
$this->plugin->initialize($this->server);
7281
}

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