aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorFerdinand Thiessen <opensource@fthiessen.de>2025-01-19 11:36:24 +0100
committerbackportbot[bot] <backportbot[bot]@users.noreply.github.com>2025-02-04 15:06:11 +0000
commit8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1 (patch)
treedeba33466ca11a4d8fc04fab80f3abd3a6292328
parentdd921969dafa64de916cd7f9daba6bf2e57f4525 (diff)
downloadnextcloud-server-8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1.tar.gz
nextcloud-server-8fc1b6f1cdec3cbfc8c3c3ba733fb441f4000ca1.zip
fix(sharing): Ensure download restrictions are not droppedbackport/50642/stable31
When a user receives a share with share-permissions but also with download restrictions (hide download or the modern download permission attribute), then re-shares of that share must always also include those restrictions. Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
-rw-r--r--apps/files_sharing/lib/Controller/ShareAPIController.php64
-rw-r--r--apps/files_sharing/tests/Controller/ShareAPIControllerTest.php148
-rw-r--r--build/integration/sharing_features/sharing-v1-part2.feature73
3 files changed, 193 insertions, 92 deletions
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 1fb62480049..9716e8f2772 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -654,7 +654,6 @@ class ShareAPIController extends OCSController {
}
$share->setSharedBy($this->userId);
- $this->checkInheritedAttributes($share);
// Handle mail send
if (is_null($sendMail)) {
@@ -787,6 +786,7 @@ class ShareAPIController extends OCSController {
}
$share->setShareType($shareType);
+ $this->checkInheritedAttributes($share);
if ($note !== '') {
$share->setNote($note);
@@ -1272,7 +1272,6 @@ class ShareAPIController extends OCSController {
if ($attributes !== null) {
$share = $this->setShareAttributes($share, $attributes);
}
- $this->checkInheritedAttributes($share);
// Handle mail send
if ($sendMail === 'true' || $sendMail === 'false') {
@@ -1359,6 +1358,7 @@ class ShareAPIController extends OCSController {
}
try {
+ $this->checkInheritedAttributes($share);
$share = $this->shareManager->updateShare($share);
} catch (HintException $e) {
$code = $e->getCode() === 0 ? 403 : $e->getCode();
@@ -2083,30 +2083,48 @@ class ShareAPIController extends OCSController {
if (!$share->getSharedBy()) {
return; // Probably in a test
}
+
+ $canDownload = false;
+ $hideDownload = true;
+
$userFolder = $this->rootFolder->getUserFolder($share->getSharedBy());
- $node = $userFolder->getFirstNodeById($share->getNodeId());
- if (!$node) {
- return;
- }
- if ($node->getStorage()->instanceOfStorage(SharedStorage::class)) {
- $storage = $node->getStorage();
- if ($storage instanceof Wrapper) {
- $storage = $storage->getInstanceOfStorage(SharedStorage::class);
- if ($storage === null) {
- throw new \RuntimeException('Should not happen, instanceOfStorage but getInstanceOfStorage return null');
- }
- } else {
- throw new \RuntimeException('Should not happen, instanceOfStorage but not a wrapper');
+ $nodes = $userFolder->getById($share->getNodeId());
+ foreach ($nodes as $node) {
+ // Owner always can download it - so allow it and break
+ if ($node->getOwner()?->getUID() === $share->getSharedBy()) {
+ $canDownload = true;
+ $hideDownload = false;
+ break;
}
- /** @var SharedStorage $storage */
- $inheritedAttributes = $storage->getShare()->getAttributes();
- if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) {
- $share->setHideDownload(true);
- $attributes = $share->getAttributes();
- if ($attributes) {
- $attributes->setAttribute('permissions', 'download', false);
- $share->setAttributes($attributes);
+
+ if ($node->getStorage()->instanceOfStorage(SharedStorage::class)) {
+ $storage = $node->getStorage();
+ if ($storage instanceof Wrapper) {
+ $storage = $storage->getInstanceOfStorage(SharedStorage::class);
+ if ($storage === null) {
+ throw new \RuntimeException('Should not happen, instanceOfStorage but getInstanceOfStorage return null');
+ }
+ } else {
+ throw new \RuntimeException('Should not happen, instanceOfStorage but not a wrapper');
}
+
+ /** @var SharedStorage $storage */
+ $originalShare = $storage->getShare();
+ $inheritedAttributes = $originalShare->getAttributes();
+ // hide if hidden and also the current share enforces hide (can only be false if one share is false or user is owner)
+ $hideDownload = $hideDownload && $originalShare->getHideDownload();
+ // allow download if already allowed by previous share or when the current share allows downloading
+ $canDownload = $canDownload || $inheritedAttributes === null || $inheritedAttributes->getAttribute('permissions', 'download') !== false;
+ }
+ }
+
+ if ($hideDownload || !$canDownload) {
+ $share->setHideDownload(true);
+
+ if (!$canDownload) {
+ $attributes = $share->getAttributes() ?? $share->newAttributes();
+ $attributes->setAttribute('permissions', 'download', false);
+ $share->setAttributes($attributes);
}
}
}
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
index f8c316cd2e9..ac788dd6b28 100644
--- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -157,7 +157,7 @@ class ShareAPIControllerTest extends TestCase {
$this->factory,
$this->mailer,
$this->currentUser,
- ])->setMethods(['formatShare'])
+ ])->onlyMethods(['formatShare'])
->getMock();
}
@@ -246,9 +246,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with($share->getNodeId())
- ->willReturn($node);
+ ->willReturn([$node]);
$this->shareManager
->expects($this->once())
@@ -411,9 +411,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with($share->getNodeId())
- ->willReturn($share->getNode());
+ ->willReturn([$share->getNode()]);
$this->shareManager->expects($this->once())
->method('deleteFromSelf')
@@ -474,9 +474,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with($share->getNodeId())
- ->willReturn($share->getNode());
+ ->willReturn([$share->getNode()]);
$this->shareManager->expects($this->never())
->method('deleteFromSelf');
@@ -506,9 +506,10 @@ class ShareAPIControllerTest extends TestCase {
->willReturn($mount);
$userFolder = $this->createMock(Folder::class);
- $userFolder
- ->expects($this->exactly(2))
- ->method('getFirstNodeById')
+ $userFolder->method('getById')
+ ->with(2)
+ ->willReturn([$file]);
+ $userFolder->method('getFirstNodeById')
->with(2)
->willReturn($file);
@@ -841,7 +842,8 @@ class ShareAPIControllerTest extends TestCase {
$this->mailer,
$this->currentUser,
- ])->setMethods(['canAccessShare'])
+ ])
+ ->onlyMethods(['canAccessShare'])
->getMock();
$ocs->expects($this->any())
@@ -862,7 +864,6 @@ class ShareAPIControllerTest extends TestCase {
$userFolder->method('getById')
->with($share->getNodeId())
->willReturn([$share->getNode()]);
-
$userFolder->method('getFirstNodeById')
->with($share->getNodeId())
->willReturn($share->getNode());
@@ -901,9 +902,8 @@ class ShareAPIControllerTest extends TestCase {
]);
$this->dateTimeZone->method('getTimezone')->willReturn(new \DateTimeZone('UTC'));
- $d = $ocs->getShare($share->getId())->getData()[0];
-
- $this->assertEquals($result, $ocs->getShare($share->getId())->getData()[0]);
+ $data = $ocs->getShare($share->getId())->getData()[0];
+ $this->assertEquals($result, $data);
}
@@ -1558,6 +1558,9 @@ class ShareAPIControllerTest extends TestCase {
$userFolder->method('getFirstNodeById')
->with($share->getNodeId())
->willReturn($file);
+ $userFolder->method('getById')
+ ->with($share->getNodeId())
+ ->willReturn([$file]);
$file->method('getPermissions')
->will($this->onConsecutiveCalls(Constants::PERMISSION_SHARE, Constants::PERMISSION_READ));
@@ -1651,9 +1654,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with($share->getNodeId())
- ->willReturn($share->getNode());
+ ->willReturn([$share->getNode()]);
if (!$helperAvailable) {
$this->appManager->method('isEnabledForUser')
@@ -1741,8 +1744,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('newShare')->willReturn($share);
[$userFolder, $path] = $this->getNonSharedUserFile();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -1769,8 +1771,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('newShare')->willReturn($share);
[$userFolder, $path] = $this->getNonSharedUserFile();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -1869,8 +1870,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('allowGroupSharing')->willReturn(true);
[$userFolder, $path] = $this->getNonSharedUserFile();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -1974,8 +1974,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('newShare')->willReturn($share);
[$userFolder, $path] = $this->getNonSharedUserFolder();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -2531,8 +2530,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('newShare')->willReturn($share);
[$userFolder, $path] = $this->getNonSharedUserFolder();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -2569,8 +2567,7 @@ class ShareAPIControllerTest extends TestCase {
$this->shareManager->method('newShare')->willReturn($share);
[$userFolder, $path] = $this->getNonSharedUserFile();
- $this->rootFolder->expects($this->exactly(2))
- ->method('getUserFolder')
+ $this->rootFolder->method('getUserFolder')
->with('currentUser')
->willReturn($userFolder);
@@ -2703,9 +2700,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with($share->getNodeId())
- ->willReturn($share->getNode());
+ ->willReturn([$share->getNode()]);
$this->ocs->updateShare(42);
}
@@ -2796,6 +2793,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
+ $userFolder->method('getById')
+ ->with(42)
+ ->willReturn([$node]);
$userFolder->method('getFirstNodeById')
->with(42)
->willReturn($node);
@@ -2850,9 +2850,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -2900,9 +2900,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -2958,9 +2958,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -3016,9 +3016,9 @@ class ShareAPIControllerTest extends TestCase {
$ocs = $this->mockFormatShare();
[$userFolder, $folder] = $this->getNonSharedUserFolder();
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3063,9 +3063,9 @@ class ShareAPIControllerTest extends TestCase {
$ocs = $this->mockFormatShare();
[$userFolder, $folder] = $this->getNonSharedUserFolder();
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3091,10 +3091,13 @@ class ShareAPIControllerTest extends TestCase {
$ocs = $this->mockFormatShare();
- [$userFolder, $file] = $this->getNonSharedUserFile();
- $userFolder->method('getFirstNodeById')
+ $file = $this->getMockBuilder(File::class)->getMock();
+ $file->method('getId')
+ ->willReturn(42);
+ [$userFolder, $folder] = $this->getNonSharedUserFolder();
+ $userFolder->method('getById')
->with(42)
- ->willReturn($file);
+ ->willReturn([$folder]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3128,9 +3131,9 @@ class ShareAPIControllerTest extends TestCase {
[$userFolder, $node] = $this->getNonSharedUserFolder();
$node->method('getId')->willReturn(42);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3179,9 +3182,9 @@ class ShareAPIControllerTest extends TestCase {
$date->setTime(0, 0, 0);
[$userFolder, $node] = $this->getNonSharedUserFolder();
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3237,9 +3240,9 @@ class ShareAPIControllerTest extends TestCase {
$date->setTime(0, 0, 0);
[$userFolder, $node] = $this->getNonSharedUserFolder();
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3277,9 +3280,9 @@ class ShareAPIControllerTest extends TestCase {
$date->setTime(0, 0, 0);
[$userFolder, $node] = $this->getNonSharedUserFolder();
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);
@@ -3371,9 +3374,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$mountPoint = $this->createMock(IMountPoint::class);
$node->method('getMountPoint')
@@ -3439,9 +3442,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($node);
+ ->willReturn([$node]);
$mountPoint = $this->createMock(IMountPoint::class);
$node->method('getMountPoint')
@@ -3500,9 +3503,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -3560,9 +3563,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -3620,9 +3623,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -3668,9 +3671,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($file);
+ ->willReturn([$file]);
$mountPoint = $this->createMock(IMountPoint::class);
$file->method('getMountPoint')
@@ -3734,6 +3737,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
+ $userFolder->method('getById')
+ ->with(42)
+ ->willReturn([$folder]);
$userFolder->method('getFirstNodeById')
->with(42)
->willReturn($folder);
@@ -3804,9 +3810,9 @@ class ShareAPIControllerTest extends TestCase {
->with($this->currentUser)
->willReturn($userFolder);
- $userFolder->method('getFirstNodeById')
+ $userFolder->method('getById')
->with(42)
- ->willReturn($folder);
+ ->willReturn([$folder]);
$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
@@ -3834,9 +3840,10 @@ class ShareAPIControllerTest extends TestCase {
->willReturn($mount);
$userFolder = $this->createMock(Folder::class);
- $userFolder
- ->expects($this->exactly(2))
- ->method('getFirstNodeById')
+ $userFolder->method('getById')
+ ->with(2)
+ ->willReturn([$file]);
+ $userFolder->method('getFirstNodeById')
->with(2)
->willReturn($file);
@@ -5107,6 +5114,9 @@ class ShareAPIControllerTest extends TestCase {
$userFolder->method('getStorage')->willReturn($storage);
$node->method('getStorage')->willReturn($storage);
$node->method('getId')->willReturn(42);
+ $user = $this->createMock(IUser::class);
+ $user->method('getUID')->willReturn($this->currentUser);
+ $node->method('getOwner')->willReturn($user);
return [$userFolder, $node];
}
diff --git a/build/integration/sharing_features/sharing-v1-part2.feature b/build/integration/sharing_features/sharing-v1-part2.feature
index 8cc97fe71ee..e4da8436508 100644
--- a/build/integration/sharing_features/sharing-v1-part2.feature
+++ b/build/integration/sharing_features/sharing-v1-part2.feature
@@ -701,6 +701,79 @@ Feature: sharing
Then the OCS status code should be "404"
And the HTTP status code should be "200"
+ Scenario: download restrictions can not be dropped
+ As an "admin"
+ Given user "user0" exists
+ And user "user1" exists
+ And user "user2" exists
+ And User "user0" uploads file with content "foo" to "/tmp.txt"
+ And As an "user0"
+ And creating a share with
+ | path | /tmp.txt |
+ | shareType | 0 |
+ | shareWith | user1 |
+ | permissions | 17 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+ And As an "user1"
+ And accepting last share
+ When Getting info of last share
+ Then Share fields of last share match with
+ | uid_owner | user0 |
+ | uid_file_owner | user0 |
+ | permissions | 17 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+ When creating a share with
+ | path | /tmp.txt |
+ | shareType | 0 |
+ | shareWith | user2 |
+ | permissions | 1 |
+ Then the OCS status code should be "100"
+ And the HTTP status code should be "200"
+ When As an "user2"
+ And accepting last share
+ And Getting info of last share
+ Then Share fields of last share match with
+ | share_type | 0 |
+ | permissions | 1 |
+ | uid_owner | user1 |
+ | uid_file_owner | user0 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+
+ Scenario: download restrictions can not be dropped when re-sharing even on link shares
+ As an "admin"
+ Given user "user0" exists
+ And user "user1" exists
+ And User "user0" uploads file with content "foo" to "/tmp.txt"
+ And As an "user0"
+ And creating a share with
+ | path | /tmp.txt |
+ | shareType | 0 |
+ | shareWith | user1 |
+ | permissions | 17 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+ And As an "user1"
+ And accepting last share
+ When Getting info of last share
+ Then Share fields of last share match with
+ | uid_owner | user0 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+ When creating a share with
+ | path | /tmp.txt |
+ | shareType | 3 |
+ | permissions | 1 |
+ And Getting info of last share
+ And Updating last share with
+ | hideDownload | false |
+ Then the OCS status code should be "100"
+ And the HTTP status code should be "200"
+ When Getting info of last share
+ Then Share fields of last share match with
+ | share_type | 3 |
+ | uid_owner | user1 |
+ | uid_file_owner | user0 |
+ | hide_download | 1 |
+ | attributes | [{"scope":"permissions","key":"download","value":false}] |
+
Scenario: User is not allowed to reshare file with additional delete permissions
As an "admin"
Given user "user0" exists