archive/nextcloud_server
0
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2025-03-13 07:53:51 +00:00
Code Issues Projects Releases Wiki Activity

fix(sharing): Ensure download restrictions are not dropped

Browse source 
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>
This commit is contained in:
Ferdinand Thiessen 2025-01-19 11:36:24 +01:00
parent 83e35b6991
commit 73dc0f0f19
No known key found for this signature in database
GPG key ID: 45FAE7268762B400
3 changed files with 193 additions and 92 deletions
apps/files_sharing
lib/Controller
ShareAPIController.php
tests/Controller
ShareAPIControllerTest.php
build/integration/sharing_features
sharing-v1-part2.feature

64
apps/files_sharing/lib/Controller/ShareAPIController.php
View file

@ -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);
}
}
}

148
apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file

@ -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];
}

73
build/integration/sharing_features/sharing-v1-part2.feature
View file

@ -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