Mirrors
/
nextcloud-server
mirror of https://github.com/nextcloud/server
3
0
Fork 0
Code Releases Activity
Browse Source

Merge pull request #50642 from nextcloud/fix/proper-download-check 

fix(sharing): better handle file share attributes
pull/50653/head
Ferdinand Thiessen 9 months ago
committed by GitHub
parent
83e35b6991 73dc0f0f19
commit
c82bc0a5a3
No known key found for this signature in database GPG Key ID: B5690EEEBB952194
3 changed files with 193 additions and 92 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 64
      apps/files_sharing/lib/Controller/ShareAPIController.php
  2. 148
      apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
  3. 73
      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

Write Preview
Loading…
Cancel
Save