diff options
3 files changed, 206 insertions, 96 deletions
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index cad0f542d53..d959fad7560 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -680,7 +680,6 @@ class ShareAPIController extends OCSController { } $share->setSharedBy($this->currentUser); - $this->checkInheritedAttributes($share); if ($shareType === IShare::TYPE_USER) { // Valid user is required to share @@ -792,6 +791,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); /** * expiration date, password and publicUpload only make sense for link shares @@ -1351,6 +1350,7 @@ class ShareAPIController extends OCSController { } try { + $this->checkInheritedAttributes($share); $share = $this->shareManager->updateShare($share); } catch (HintException $e) { $code = $e->getCode() === 0 ? 403 : $e->getCode(); @@ -2055,30 +2055,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 \OCA\Files_Sharing\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 72e4fb00f7a..02da6098d28 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -99,7 +99,7 @@ class ShareAPIControllerTest extends TestCase { protected function setUp(): void { /** @var IManager|MockObject */ - $this->shareManager = $this->createMock(IManager::class); + $this->shareManager = $this->createMock(\OC\Share20\Manager::class); $this->shareManager ->expects($this->any()) ->method('shareApiEnabled') @@ -152,7 +152,7 @@ class ShareAPIControllerTest extends TestCase { } /** - * @return ShareAPIController&MockObject + * @return ShareAPIController|MockObject */ private function mockFormatShare() { return $this->getMockBuilder(ShareAPIController::class) @@ -173,7 +173,8 @@ class ShareAPIControllerTest extends TestCase { $this->dateTimeZone, $this->logger, $this->currentUser, - ])->setMethods(['formatShare']) + ]) + ->onlyMethods(['formatShare']) ->getMock(); } @@ -252,10 +253,20 @@ class ShareAPIControllerTest extends TestCase { $this->expectExceptionMessage('Could not delete share'); $node = $this->getMockBuilder(File::class)->getMock(); + $node->method('getId')->willReturn(42); $share = $this->newShare(); $share->setNode($node); + $userFolder = $this->getMockBuilder(Folder::class)->getMock(); + $this->rootFolder->method('getUserFolder') + ->with($this->currentUser) + ->willReturn($userFolder); + + $userFolder->method('getById') + ->with($share->getNodeId()) + ->willReturn([$node]); + $this->shareManager ->expects($this->once()) ->method('getShareById') @@ -417,9 +428,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') @@ -480,9 +491,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'); @@ -769,7 +780,7 @@ class ShareAPIControllerTest extends TestCase { * @dataProvider dataGetShare */ public function testGetShare(IShare $share, array $result): void { - /** @var ShareAPIController&MockObject $ocs */ + /** @var ShareAPIController|MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ Application::APP_ID, @@ -789,8 +800,9 @@ class ShareAPIControllerTest extends TestCase { $this->logger, $this->currentUser, - ])->setMethods(['canAccessShare']) - ->getMock(); + ]) + ->onlyMethods(['canAccessShare']) + ->getMock(); $ocs->expects($this->any()) ->method('canAccessShare') @@ -810,7 +822,6 @@ class ShareAPIControllerTest extends TestCase { $userFolder->method('getById') ->with($share->getNodeId()) ->willReturn([$share->getNode()]); - $userFolder->method('getFirstNodeById') ->with($share->getNodeId()) ->willReturn($share->getNode()); @@ -849,9 +860,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); } @@ -1504,6 +1514,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(\OCP\Constants::PERMISSION_SHARE, \OCP\Constants::PERMISSION_READ)); @@ -1597,9 +1610,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') @@ -1687,8 +1700,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); @@ -1715,8 +1727,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); @@ -1813,10 +1824,9 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('allowGroupSharing')->willReturn(true); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') - ->with('currentUser') - ->willReturn($userFolder); + $this->rootFolder->method('getUserFolder') + ->with('currentUser') + ->willReturn($userFolder); $userFolder->expects($this->once()) ->method('get') @@ -1836,7 +1846,7 @@ class ShareAPIControllerTest extends TestCase { $share = $this->newShare(); $this->shareManager->method('newShare')->willReturn($share); - /** @var ShareAPIController&MockObject $ocs */ + /** @var ShareAPIController|MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ Application::APP_ID, @@ -1916,8 +1926,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); @@ -2469,10 +2478,9 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFolder(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') - ->with('currentUser') - ->willReturn($userFolder); + $this->rootFolder->method('getUserFolder') + ->with('currentUser') + ->willReturn($userFolder); $path->method('getPath')->willReturn('valid-path'); $userFolder->expects($this->once()) @@ -2507,10 +2515,9 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('newShare')->willReturn($share); [$userFolder, $path] = $this->getNonSharedUserFile(); - $this->rootFolder->expects($this->exactly(2)) - ->method('getUserFolder') - ->with('currentUser') - ->willReturn($userFolder); + $this->rootFolder->method('getUserFolder') + ->with('currentUser') + ->willReturn($userFolder); $userFolder->expects($this->once()) ->method('get') @@ -2559,7 +2566,7 @@ class ShareAPIControllerTest extends TestCase { $share = \OCP\Server::get(IManager::class)->newShare(); $this->shareManager->method('newShare')->willReturn($share); - /** @var ShareAPIController&MockObject $ocs */ + /** @var ShareAPIController|MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ Application::APP_ID, @@ -2639,9 +2646,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); } @@ -2732,6 +2739,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$node]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($node); @@ -2786,9 +2796,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') @@ -2836,9 +2846,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') @@ -2894,9 +2904,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') @@ -2952,9 +2962,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); @@ -2999,9 +3009,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); @@ -3027,10 +3037,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); @@ -3064,9 +3077,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); @@ -3115,9 +3128,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); @@ -3173,9 +3186,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); @@ -3213,9 +3226,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); @@ -3307,9 +3320,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') @@ -3375,9 +3388,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') @@ -3436,9 +3449,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') @@ -3496,9 +3509,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') @@ -3556,9 +3569,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') @@ -3604,9 +3617,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') @@ -3670,6 +3683,9 @@ class ShareAPIControllerTest extends TestCase { ->with($this->currentUser) ->willReturn($userFolder); + $userFolder->method('getById') + ->with(42) + ->willReturn([$folder]); $userFolder->method('getFirstNodeById') ->with(42) ->willReturn($folder); @@ -3740,9 +3756,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') @@ -4986,6 +5002,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 a9e2e50ce02..d7cc320aee6 100644 --- a/build/integration/sharing_features/sharing-v1-part2.feature +++ b/build/integration/sharing_features/sharing-v1-part2.feature @@ -699,6 +699,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","enabled":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","enabled":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","enabled":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","enabled":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","enabled":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","enabled":false}] | + Scenario: User is not allowed to reshare file with additional delete permissions As an "admin" Given user "user0" exists |