Skip to content

Commit 3a343ff

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

5 files changed

Lines changed: 160 additions & 14 deletions

File tree

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCP\EventDispatcher\IEventDispatcher;
1717
use OCP\Files\Folder;
1818
use OCP\Files\IFilenameValidator;
19+
use OCP\Files\IRootFolder;
1920
use OCP\Files\Mount\IMountManager;
2021
use OCP\IConfig;
2122
use OCP\IDBConnection;
@@ -152,8 +153,8 @@ public function createServer(string $baseUri,
152153
$server->addPlugin(new \OCA\DAV\Connector\Sabre\SharesPlugin(
153154
$objectTree,
154155
$this->userSession,
155-
$userFolder,
156-
\OC::$server->getShareManager()
156+
\OCP\Server::get(\OCP\Share\IManager::class),
157+
\OCP\Server::get(IRootFolder::class),
157158
));
158159
$server->addPlugin(new \OCA\DAV\Connector\Sabre\CommentPropertiesPlugin(\OC::$server->getCommentsManager(), $this->userSession));
159160
$server->addPlugin(new \OCA\DAV\Connector\Sabre\FilesReportPlugin(

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

Lines changed: 35 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;
@@ -47,8 +48,8 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin {
4748
public function __construct(
4849
private Tree $tree,
4950
private IUserSession $userSession,
50-
private Folder $userFolder,
5151
private IManager $shareManager,
52+
private IRootFolder $rootFolder,
5253
) {
5354
$this->userId = $userSession->getUser()->getUID();
5455
}
@@ -79,7 +80,7 @@ public function initialize(Server $server) {
7980
* @param Node $node
8081
* @return IShare[]
8182
*/
82-
private function getShare(Node $node): array {
83+
private function getShare(Node $node, bool $includeIncoming = true): array {
8384
$result = [];
8485
$requestedShareTypes = [
8586
IShare::TYPE_USER,
@@ -102,6 +103,10 @@ private function getShare(Node $node): array {
102103
-1
103104
));
104105

106+
if (!$includeIncoming) {
107+
continue;
108+
}
109+
105110
// Also check for shares where the user is the recipient
106111
try {
107112
$result = array_merge($result, $this->shareManager->getSharedWith(
@@ -118,6 +123,24 @@ private function getShare(Node $node): array {
118123
return $result;
119124
}
120125

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

236-
$targetShares = $this->getShare($targetNode->getNode());
237-
if (empty($targetShares)) {
259+
$targetShares = $this->getSharesForTarget($targetNode->getNode());
260+
if ($targetShares === []) {
238261
// Target is not a share so no re-sharing inprogress
239262
return true;
240263
}
@@ -250,6 +273,14 @@ public function validateMoveOrCopy(string $source, string $target): bool {
250273
return true;
251274
}
252275
}
276+
277+
// if the share recipient is allow to delete from the share, they are allowed to move the file out of the share
278+
// the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share
279+
//
280+
// since the 2-step move is allowed, we also allow both steps at once
281+
if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) {
282+
return true;
283+
}
253284
}
254285

255286
throw new Forbidden('You cannot move a non-shareable node into a share');

‎apps/dav/lib/Server.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
use OCP\Diagnostics\IEventLogger;
5959
use OCP\EventDispatcher\IEventDispatcher;
6060
use OCP\Files\IFilenameValidator;
61+
use OCP\Files\IRootFolder;
6162
use OCP\FilesMetadata\IFilesMetadataManager;
6263
use OCP\IAppConfig;
6364
use OCP\ICacheFactory;
@@ -290,8 +291,8 @@ public function __construct(IRequest $request, string $baseUri) {
290291
$this->server->addPlugin(new SharesPlugin(
291292
$this->server->tree,
292293
$userSession,
293-
$userFolder,
294294
$shareManager,
295+
\OCP\Server::get(IRootFolder::class),
295296
));
296297
$this->server->addPlugin(new CommentPropertiesPlugin(
297298
\OC::$server->getCommentsManager(),

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

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,12 @@
1212
use OCA\DAV\Connector\Sabre\Node;
1313
use OCA\DAV\Upload\UploadFile;
1414
use OCP\Files\Folder;
15+
use OCP\Files\IRootFolder;
1516
use OCP\IUser;
1617
use OCP\IUserSession;
1718
use OCP\Share\IManager;
1819
use OCP\Share\IShare;
20+
use PHPUnit\Framework\MockObject\MockObject;
1921
use Sabre\DAV\Tree;
2022

2123
class SharesPluginTest extends \Test\TestCase {
@@ -37,20 +39,21 @@ class SharesPluginTest extends \Test\TestCase {
3739
private $shareManager;
3840

3941
/**
40-
* @var \OCP\Files\Folder
42+
* @var \OCA\DAV\Connector\Sabre\SharesPlugin
4143
*/
42-
private $userFolder;
44+
private $plugin;
4345

4446
/**
45-
* @var \OCA\DAV\Connector\Sabre\SharesPlugin
47+
* @var IRootFolder&MockObject
4648
*/
47-
private $plugin;
49+
private $rootFolder;
4850

4951
protected function setUp(): void {
5052
parent::setUp();
5153
$this->server = new \Sabre\DAV\Server();
5254
$this->tree = $this->createMock(Tree::class);
5355
$this->shareManager = $this->createMock(IManager::class);
56+
$this->rootFolder = $this->createMock(IRootFolder::class);
5457
$user = $this->createMock(IUser::class);
5558
$user->expects($this->once())
5659
->method('getUID')
@@ -59,13 +62,12 @@ protected function setUp(): void {
5962
$userSession->expects($this->once())
6063
->method('getUser')
6164
->willReturn($user);
62-
$this->userFolder = $this->createMock(Folder::class);
6365

6466
$this->plugin = new \OCA\DAV\Connector\Sabre\SharesPlugin(
6567
$this->tree,
6668
$userSession,
67-
$this->userFolder,
68-
$this->shareManager
69+
$this->shareManager,
70+
$this->rootFolder,
6971
);
7072
$this->plugin->initialize($this->server);
7173
}

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

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,3 +289,114 @@ Scenario: Can copy file between shares if share permissions
289289
And the OCS status code should be "100"
290290
When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt"
291291
Then the HTTP status code should be "201"
292+
293+
Scenario: Cannot copy files from share without share permission into subfolder of other share
294+
Given user "user0" exists
295+
Given user "user1" exists
296+
Given user "user2" exists
297+
And As an "user0"
298+
And user "user0" created a folder "/share"
299+
When creating a share with
300+
| path | share |
301+
| shareType | 0 |
302+
| shareWith | user1 |
303+
| permissions | 7 |
304+
Then the HTTP status code should be "200"
305+
And the OCS status code should be "100"
306+
And User "user0" uploads file with content "test" to "/share/test.txt"
307+
And As an "user1"
308+
And user "user1" created a folder "/re-share"
309+
And user "user1" created a folder "/re-share/subfolder"
310+
When creating a share with
311+
| path | re-share |
312+
| shareType | 0 |
313+
| shareWith | user2 |
314+
| permissions | 31 |
315+
Then the HTTP status code should be "200"
316+
And the OCS status code should be "100"
317+
When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
318+
Then the HTTP status code should be "403"
319+
320+
Scenario: Cannot move files from share without share permission into subfolder of other share
321+
Given user "user0" exists
322+
Given user "user1" exists
323+
Given user "user2" exists
324+
And As an "user0"
325+
And user "user0" created a folder "/share"
326+
When creating a share with
327+
| path | share |
328+
| shareType | 0 |
329+
| shareWith | user1 |
330+
| permissions | 7 |
331+
Then the HTTP status code should be "200"
332+
And the OCS status code should be "100"
333+
And User "user0" uploads file with content "test" to "/share/test.txt"
334+
And As an "user1"
335+
And user "user1" created a folder "/re-share"
336+
And user "user1" created a folder "/re-share/subfolder"
337+
When creating a share with
338+
| path | re-share |
339+
| shareType | 0 |
340+
| shareWith | user2 |
341+
| permissions | 31 |
342+
Then the HTTP status code should be "200"
343+
And the OCS status code should be "100"
344+
When User "user1" moves file "/share/test.txt" to "/re-share/subfolder/movetest.txt"
345+
Then the HTTP status code should be "403"
346+
347+
Scenario: Cannot move folder containing share without share permission into subfolder of other share
348+
Given user "user0" exists
349+
Given user "user1" exists
350+
Given user "user2" exists
351+
And As an "user0"
352+
And user "user0" created a folder "/share"
353+
When creating a share with
354+
| path | share |
355+
| shareType | 0 |
356+
| shareWith | user1 |
357+
| permissions | 7 |
358+
Then the HTTP status code should be "200"
359+
And the OCS status code should be "100"
360+
And User "user0" uploads file with content "test" to "/share/test.txt"
361+
And As an "user1"
362+
And user "user1" created a folder "/contains-share"
363+
When User "user1" moves file "/share" to "/contains-share/share"
364+
Then the HTTP status code should be "201"
365+
And user "user1" created a folder "/re-share"
366+
And user "user1" created a folder "/re-share/subfolder"
367+
When creating a share with
368+
| path | re-share |
369+
| shareType | 0 |
370+
| shareWith | user2 |
371+
| permissions | 31 |
372+
Then the HTTP status code should be "200"
373+
And the OCS status code should be "100"
374+
When User "user1" moves file "/contains-share" to "/re-share/subfolder/movetest"
375+
Then the HTTP status code should be "403"
376+
377+
Scenario: Can copy file between shares into subfolder if share permissions
378+
Given user "user0" exists
379+
Given user "user1" exists
380+
Given user "user2" exists
381+
And As an "user0"
382+
And user "user0" created a folder "/share"
383+
When creating a share with
384+
| path | share |
385+
| shareType | 0 |
386+
| shareWith | user1 |
387+
| permissions | 31 |
388+
Then the HTTP status code should be "200"
389+
And the OCS status code should be "100"
390+
And User "user0" uploads file with content "test" to "/share/test.txt"
391+
And As an "user1"
392+
And user "user1" created a folder "/re-share"
393+
And user "user1" created a folder "/re-share/subfolder"
394+
When creating a share with
395+
| path | re-share |
396+
| shareType | 0 |
397+
| shareWith | user2 |
398+
| permissions | 31 |
399+
Then the HTTP status code should be "200"
400+
And the OCS status code should be "100"
401+
When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
402+
Then the HTTP status code should be "201"

0 commit comments

Comments
 (0)