Skip to content

Commit a32de93

Browse files
artongeartonge
authored
Merge pull request #31728 from nextcloud/bugfix/30791/update-subshare-owner-on-move
Update owner of subdir on move into/out of share
2 parents b4353c4 + f525067 commit a32de93

9 files changed

Lines changed: 240 additions & 31 deletions

File tree

‎apps/federatedfilesharing/lib/FederatedShareProvider.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -638,7 +638,7 @@ public function restore(IShare $share, string $recipient): IShare {
638638
}
639639

640640

641-
public function getSharesInFolder($userId, Folder $node, $reshares) {
641+
public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = true) {
642642
$qb = $this->dbConnection->getQueryBuilder();
643643
$qb->select('*')
644644
->from('share', 's')
@@ -664,8 +664,13 @@ public function getSharesInFolder($userId, Folder $node, $reshares) {
664664
);
665665
}
666666

667-
$qb->innerJoin('s', 'filecache' ,'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
668-
$qb->andWhere($qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())));
667+
$qb->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
668+
669+
if ($shallow) {
670+
$qb->andWhere($qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())));
671+
} else {
672+
$qb->andWhere($qb->expr()->like('f.path', $qb->createNamedParameter($this->dbConnection->escapeLikeParameter($node->getInternalPath()) . '/%')));
673+
}
669674

670675
$qb->orderBy('id');
671676

‎apps/files_sharing/lib/Updater.php‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
use OC\Files\Mount\MountPoint;
3030
use OCP\Constants;
3131
use OCP\Share\IShare;
32+
use OCP\Files\Folder;
3233

3334
class Updater {
3435

@@ -37,7 +38,7 @@ class Updater {
3738
*/
3839
public static function renameHook($params) {
3940
self::renameChildren($params['oldpath'], $params['newpath']);
40-
self::moveShareToShare($params['newpath']);
41+
self::moveShareInOrOutOfShare($params['newpath']);
4142
}
4243

4344
/**
@@ -50,7 +51,7 @@ public static function renameHook($params) {
5051
*
5152
* @param string $path
5253
*/
53-
private static function moveShareToShare($path) {
54+
private static function moveShareInOrOutOfShare($path): void {
5455
$userFolder = \OC::$server->getUserFolder();
5556

5657
// If the user folder can't be constructed (e.g. link share) just return.
@@ -62,10 +63,18 @@ private static function moveShareToShare($path) {
6263

6364
$shareManager = \OC::$server->getShareManager();
6465

66+
// FIXME: should CIRCLES be included here ??
6567
$shares = $shareManager->getSharesBy($userFolder->getOwner()->getUID(), IShare::TYPE_USER, $src, false, -1);
6668
$shares = array_merge($shares, $shareManager->getSharesBy($userFolder->getOwner()->getUID(), IShare::TYPE_GROUP, $src, false, -1));
6769
$shares = array_merge($shares, $shareManager->getSharesBy($userFolder->getOwner()->getUID(), IShare::TYPE_ROOM, $src, false, -1));
6870

71+
if ($src instanceof Folder) {
72+
$subShares = $shareManager->getSharesInFolder($userFolder->getOwner()->getUID(), $src, false, false);
73+
foreach ($subShares as $subShare) {
74+
$shares = array_merge($shares, array_values($subShare));
75+
}
76+
}
77+
6978
// If the path we move is not a share we don't care
7079
if (empty($shares)) {
7180
return;
@@ -74,21 +83,31 @@ private static function moveShareToShare($path) {
7483
// Check if the destination is inside a share
7584
$mountManager = \OC::$server->getMountManager();
7685
$dstMount = $mountManager->find($src->getPath());
77-
if (!($dstMount instanceof \OCA\Files_Sharing\SharedMount)) {
78-
return;
79-
}
80-
81-
$newOwner = $dstMount->getShare()->getShareOwner();
8286

8387
//Ownership is moved over
8488
foreach ($shares as $share) {
85-
/** @var IShare $share */
86-
if (!($dstMount->getShare()->getPermissions() & Constants::PERMISSION_SHARE)) {
87-
$shareManager->deleteShare($share);
89+
if (
90+
$share->getShareType() !== IShare::TYPE_USER &&
91+
$share->getShareType() !== IShare::TYPE_GROUP &&
92+
$share->getShareType() !== IShare::TYPE_ROOM
93+
) {
8894
continue;
8995
}
96+
97+
if ($dstMount instanceof \OCA\Files_Sharing\SharedMount) {
98+
if (!($dstMount->getShare()->getPermissions() & Constants::PERMISSION_SHARE)) {
99+
$shareManager->deleteShare($share);
100+
continue;
101+
}
102+
$newOwner = $dstMount->getShare()->getShareOwner();
103+
$newPermissions = $share->getPermissions() & $dstMount->getShare()->getPermissions();
104+
} else {
105+
$newOwner = $userFolder->getOwner()->getUID();
106+
$newPermissions = $share->getPermissions();
107+
}
108+
90109
$share->setShareOwner($newOwner);
91-
$share->setPermissions($share->getPermissions() & $dstMount->getShare()->getPermissions());
110+
$share->setPermissions($newPermissions);
92111
$shareManager->updateShare($share);
93112
}
94113
}

‎apps/files_sharing/tests/UpdaterTest.php‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,4 +237,123 @@ public function testRename() {
237237
// cleanup
238238
$this->shareManager->deleteShare($share);
239239
}
240+
241+
/**
242+
* If a folder gets moved into shared folder, children shares should have their uid_owner and permissions adjusted
243+
* user1
244+
* |-folder1 --> shared with user2
245+
* user2
246+
* |-folder2 --> shared with user3 and moved into folder1
247+
* |-subfolder1 --> shared with user3
248+
* |-file1.txt --> shared with user3
249+
* |-subfolder2
250+
* |-file2.txt --> shared with user3
251+
*/
252+
public function testMovedIntoShareChangeOwner() {
253+
$this->markTestSkipped('Skipped because this is failing with S3 as primary as file id are change when moved.');
254+
255+
// user1 creates folder1
256+
$viewUser1 = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER1 . '/files');
257+
$folder1 = 'folder1';
258+
$viewUser1->mkdir($folder1);
259+
260+
// user1 shares folder1 to user2
261+
$folder1Share = $this->share(
262+
IShare::TYPE_USER,
263+
$folder1,
264+
self::TEST_FILES_SHARING_API_USER1,
265+
self::TEST_FILES_SHARING_API_USER2,
266+
\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE
267+
);
268+
269+
$this->loginHelper(self::TEST_FILES_SHARING_API_USER2);
270+
$viewUser2 = new \OC\Files\View('/' . self::TEST_FILES_SHARING_API_USER2 . '/files');
271+
// Create user2 files
272+
$folder2 = 'folder2';
273+
$viewUser2->mkdir($folder2);
274+
$file1 = 'folder2/file1.txt';
275+
$viewUser2->touch($file1);
276+
$subfolder1 = 'folder2/subfolder1';
277+
$viewUser2->mkdir($subfolder1);
278+
$subfolder2 = 'folder2/subfolder2';
279+
$viewUser2->mkdir($subfolder2);
280+
$file2 = 'folder2/subfolder2/file2.txt';
281+
$viewUser2->touch($file2);
282+
283+
// user2 shares folder2 to user3
284+
$folder2Share = $this->share(
285+
IShare::TYPE_USER,
286+
$folder2,
287+
self::TEST_FILES_SHARING_API_USER2,
288+
self::TEST_FILES_SHARING_API_USER3,
289+
\OCP\Constants::PERMISSION_ALL
290+
);
291+
// user2 shares folder2/file1 to user3
292+
$file1Share = $this->share(
293+
IShare::TYPE_USER,
294+
$file1,
295+
self::TEST_FILES_SHARING_API_USER2,
296+
self::TEST_FILES_SHARING_API_USER3,
297+
\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE
298+
);
299+
// user2 shares subfolder1 to user3
300+
$subfolder1Share = $this->share(
301+
IShare::TYPE_USER,
302+
$subfolder1,
303+
self::TEST_FILES_SHARING_API_USER2,
304+
self::TEST_FILES_SHARING_API_USER3,
305+
\OCP\Constants::PERMISSION_ALL
306+
);
307+
// user2 shares subfolder2/file2.txt to user3
308+
$file2Share = $this->share(
309+
IShare::TYPE_USER,
310+
$file2,
311+
self::TEST_FILES_SHARING_API_USER2,
312+
self::TEST_FILES_SHARING_API_USER3,
313+
\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE
314+
);
315+
316+
// user2 moves folder2 into folder1
317+
$viewUser2->rename($folder2, $folder1.'/'.$folder2);
318+
$folder2Share = $this->shareManager->getShareById($folder2Share->getFullId());
319+
$file1Share = $this->shareManager->getShareById($file1Share->getFullId());
320+
$subfolder1Share = $this->shareManager->getShareById($subfolder1Share->getFullId());
321+
$file2Share = $this->shareManager->getShareById($file2Share->getFullId());
322+
323+
// Expect uid_owner of both shares to be user1
324+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER1, $folder2Share->getShareOwner());
325+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER1, $file1Share->getShareOwner());
326+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER1, $subfolder1Share->getShareOwner());
327+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER1, $file2Share->getShareOwner());
328+
// Expect permissions to be limited by the permissions of the destination share
329+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $folder2Share->getPermissions());
330+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $file1Share->getPermissions());
331+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $subfolder1Share->getPermissions());
332+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $file2Share->getPermissions());
333+
334+
// user2 moves folder2 out of folder1
335+
$viewUser2->rename($folder1.'/'.$folder2, $folder2);
336+
$folder2Share = $this->shareManager->getShareById($folder2Share->getFullId());
337+
$file1Share = $this->shareManager->getShareById($file1Share->getFullId());
338+
$subfolder1Share = $this->shareManager->getShareById($subfolder1Share->getFullId());
339+
$file2Share = $this->shareManager->getShareById($file2Share->getFullId());
340+
341+
// Expect uid_owner of both shares to be user2
342+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER2, $folder2Share->getShareOwner());
343+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER2, $file1Share->getShareOwner());
344+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER2, $subfolder1Share->getShareOwner());
345+
$this->assertEquals(self::TEST_FILES_SHARING_API_USER2, $file2Share->getShareOwner());
346+
// Expect permissions to not change
347+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $folder2Share->getPermissions());
348+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $file1Share->getPermissions());
349+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $subfolder1Share->getPermissions());
350+
$this->assertEquals(\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_SHARE, $file2Share->getPermissions());
351+
352+
// cleanup
353+
$this->shareManager->deleteShare($folder1Share);
354+
$this->shareManager->deleteShare($folder2Share);
355+
$this->shareManager->deleteShare($file1Share);
356+
$this->shareManager->deleteShare($subfolder1Share);
357+
$this->shareManager->deleteShare($file2Share);
358+
}
240359
}

‎apps/sharebymail/lib/ShareByMailProvider.php‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@
7575
* @package OCA\ShareByMail
7676
*/
7777
class ShareByMailProvider implements IShareProvider {
78-
7978
private IConfig $config;
8079

8180
/** @var IDBConnection */
@@ -1159,7 +1158,7 @@ protected function getRawShare($id) {
11591158
return $data;
11601159
}
11611160

1162-
public function getSharesInFolder($userId, Folder $node, $reshares) {
1161+
public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = true) {
11631162
$qb = $this->dbConnection->getQueryBuilder();
11641163
$qb->select('*')
11651164
->from('share', 's')
@@ -1185,8 +1184,13 @@ public function getSharesInFolder($userId, Folder $node, $reshares) {
11851184
);
11861185
}
11871186

1188-
$qb->innerJoin('s', 'filecache' ,'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
1189-
$qb->andWhere($qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())));
1187+
$qb->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
1188+
1189+
if ($shallow) {
1190+
$qb->andWhere($qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())));
1191+
} else {
1192+
$qb->andWhere($qb->expr()->like('f.path', $qb->createNamedParameter($this->dbConnection->escapeLikeParameter($node->getInternalPath()) . '/%')));
1193+
}
11901194

11911195
$qb->orderBy('id');
11921196

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,6 +514,54 @@ Feature: sharing
514514
Then as "user1" the file "/shared/shared_file.txt" exists
515515
And as "user0" the file "/shared/shared_file.txt" exists
516516

517+
Scenario: Owner of subshares is adjusted after moving into received share
518+
Given As an "admin"
519+
And user "user0" exists
520+
And user "user1" exists
521+
And user "user2" exists
522+
And user "user0" created a folder "/shared"
523+
And folder "/shared" of user "user0" is shared with user "user1"
524+
And user "user1" accepts last share
525+
And user "user1" created a folder "/movein"
526+
And user "user1" created a folder "/movein/subshare"
527+
When As an "user1"
528+
And folder "/movein" of user "user1" is shared with user "user2"
529+
And save last share id
530+
Then Getting info of last share
531+
And Share fields of last share match with
532+
| uid_file_owner | user1 |
533+
| share_with | user2 |
534+
When User "user1" moved file "/movein" to "/shared/movein"
535+
Then As an "user0"
536+
And Getting info of last share
537+
And Share fields of last share match with
538+
| uid_file_owner | user0 |
539+
| share_with | user2 |
540+
541+
Scenario: Owner of subshares is adjusted after moving out of received share
542+
Given As an "admin"
543+
And user "user0" exists
544+
And user "user1" exists
545+
And user "user2" exists
546+
And user "user0" created a folder "/shared"
547+
And user "user0" created a folder "/shared/moveout"
548+
And user "user0" created a folder "/shared/moveout/subshare"
549+
And folder "/shared" of user "user0" is shared with user "user1"
550+
And user "user1" accepts last share
551+
And As an "user1"
552+
And folder "/shared/moveout/subshare" of user "user1" is shared with user "user2"
553+
And save last share id
554+
When As an "user1"
555+
Then Getting info of last share
556+
And Share fields of last share match with
557+
| uid_file_owner | user0 |
558+
| share_with | user2 |
559+
When User "user1" moved file "/shared/moveout" to "/moveout"
560+
Then Getting info of last share
561+
And Share fields of last share match with
562+
| uid_file_owner | user1 |
563+
| share_with | user2 |
564+
517565
Scenario: Link shares inside of group shares keep their original data when the root share is updated
518566
Given As an "admin"
519567
And user "user0" exists

‎lib/private/Share20/DefaultShareProvider.php‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -641,9 +641,12 @@ public function move(\OCP\Share\IShare $share, $recipient) {
641641
return $share;
642642
}
643643

644-
public function getSharesInFolder($userId, Folder $node, $reshares) {
644+
public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = true) {
645645
$qb = $this->dbConn->getQueryBuilder();
646-
$qb->select('*')
646+
$qb->select('s.*',
647+
'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash',
648+
'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime',
649+
'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum')
647650
->from('share', 's')
648651
->andWhere($qb->expr()->orX(
649652
$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
@@ -679,12 +682,21 @@ public function getSharesInFolder($userId, Folder $node, $reshares) {
679682
}, $childMountNodes);
680683

681684
$qb->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
682-
$qb->andWhere(
683-
$qb->expr()->orX(
684-
$qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())),
685-
$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
686-
)
687-
);
685+
if ($shallow) {
686+
$qb->andWhere(
687+
$qb->expr()->orX(
688+
$qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())),
689+
$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
690+
)
691+
);
692+
} else {
693+
$qb->andWhere(
694+
$qb->expr()->orX(
695+
$qb->expr()->like('f.path', $qb->createNamedParameter($this->dbConn->escapeLikeParameter($node->getInternalPath()) . '/%')),
696+
$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
697+
)
698+
);
699+
}
688700

689701
$qb->orderBy('id');
690702

‎lib/private/Share20/Manager.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1303,11 +1303,11 @@ public function moveShare(IShare $share, $recipientId) {
13031303
return $provider->move($share, $recipientId);
13041304
}
13051305

1306-
public function getSharesInFolder($userId, Folder $node, $reshares = false) {
1306+
public function getSharesInFolder($userId, Folder $node, $reshares = false, $shallow = true) {
13071307
$providers = $this->factory->getAllProviders();
13081308

1309-
return array_reduce($providers, function ($shares, IShareProvider $provider) use ($userId, $node, $reshares) {
1310-
$newShares = $provider->getSharesInFolder($userId, $node, $reshares);
1309+
return array_reduce($providers, function ($shares, IShareProvider $provider) use ($userId, $node, $reshares, $shallow) {
1310+
$newShares = $provider->getSharesInFolder($userId, $node, $reshares, $shallow);
13111311
foreach ($newShares as $fid => $data) {
13121312
if (!isset($shares[$fid])) {
13131313
$shares[$fid] = [];

‎lib/public/Share/IManager.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,11 @@ public function moveShare(IShare $share, $recipientId);
135135
* @param string $userId
136136
* @param Folder $node
137137
* @param bool $reshares
138+
* @param bool $shallow Whether the method should stop at the first level, or look into sub-folders.
138139
* @return IShare[][] [$fileId => IShare[], ...]
139140
* @since 11.0.0
140141
*/
141-
public function getSharesInFolder($userId, Folder $node, $reshares = false);
142+
public function getSharesInFolder($userId, Folder $node, $reshares = false, $shallow = true);
142143

143144
/**
144145
* Get shares shared by (initiated) by the provided user.

0 commit comments

Comments
 (0)