diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index a88395cd13a8b..cad0f542d53ef 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -47,7 +47,6 @@ namespace OCA\Files_Sharing\Controller; use Exception; -use OC\Files\FileInfo; use OC\Files\Storage\Wrapper\Wrapper; use OCA\Files\Helper; use OCA\Files_Sharing\Exceptions\SharingRightsException; @@ -64,6 +63,7 @@ use OCP\AppFramework\OCSController; use OCP\AppFramework\QueryException; use OCP\Constants; +use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\InvalidPathException; use OCP\Files\IRootFolder; @@ -566,8 +566,8 @@ public function deleteShare(string $id): DataResponse { * @param string|null $path Path of the share * @param int|null $permissions Permissions for the share * @param int $shareType Type of the share - * @param string|null $shareWith The entity this should be shared with - * @param string $publicUpload If public uploading is allowed + * @param ?string $shareWith The entity this should be shared with + * @param 'true'|'false'|null $publicUpload If public uploading is allowed (deprecated) * @param string $password Password for the share * @param string|null $sendPasswordByTalk Send the password for the share over Talk * @param ?string $expireDate The expiry date of the share in the user's timezone at 00:00. @@ -590,7 +590,7 @@ public function createShare( ?int $permissions = null, int $shareType = -1, ?string $shareWith = null, - string $publicUpload = 'false', + ?string $publicUpload = null, string $password = '', ?string $sendPasswordByTalk = null, ?string $expireDate = null, @@ -599,17 +599,7 @@ public function createShare( ?string $attributes = null ): DataResponse { $share = $this->shareManager->newShare(); - - if ($permissions === null) { - if ($shareType === IShare::TYPE_LINK - || $shareType === IShare::TYPE_EMAIL) { - - // to keep legacy default behaviour, we ignore the setting below for link shares - $permissions = Constants::PERMISSION_READ; - } else { - $permissions = (int)$this->config->getAppValue('core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL); - } - } + $hasPublicUpload = $this->getLegacyPublicUpload($publicUpload); // Verify path if ($path === null) { @@ -628,7 +618,7 @@ public function createShare( // combine all permissions to determine if the user can share this file $nodes = $userFolder->getById($node->getId()); foreach ($nodes as $nodeById) { - /** @var FileInfo $fileInfo */ + /** @var \OC\Files\FileInfo $fileInfo */ $fileInfo = $node->getFileInfo(); $fileInfo['permissions'] |= $nodeById->getPermissions(); } @@ -641,17 +631,23 @@ public function createShare( throw new OCSNotFoundException($this->l->t('Could not create share')); } - if ($permissions < 0 || $permissions > Constants::PERMISSION_ALL) { - throw new OCSNotFoundException($this->l->t('Invalid permissions')); + // Set permissions + if ($shareType === IShare::TYPE_LINK || $shareType === IShare::TYPE_EMAIL) { + $permissions = $this->getLinkSharePermissions($permissions, $hasPublicUpload); + $this->validateLinkSharePermissions($node, $permissions, $hasPublicUpload); + } else { + // Use default permissions only for non-link shares to keep legacy behavior + if ($permissions === null) { + $permissions = (int)$this->config->getAppValue('core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL); + } + // Non-link shares always require read permissions (link shares could be file drop) + $permissions |= Constants::PERMISSION_READ; } - // Shares always require read permissions - $permissions |= Constants::PERMISSION_READ; - - if ($node instanceof \OCP\Files\File) { - // Single file shares should never have delete or create permissions - $permissions &= ~Constants::PERMISSION_DELETE; - $permissions &= ~Constants::PERMISSION_CREATE; + // For legacy reasons the API allows to pass PERMISSIONS_ALL even for single file shares (I look at you Talk) + if ($node instanceof File) { + // if this is a single file share we remove the DELETE and CREATE permissions + $permissions = $permissions & ~(Constants::PERMISSION_DELETE | Constants::PERMISSION_CREATE); } /** @@ -712,28 +708,7 @@ public function createShare( throw new OCSNotFoundException($this->l->t('Public link sharing is disabled by the administrator')); } - if ($publicUpload === 'true') { - // Check if public upload is allowed - if (!$this->shareManager->shareApiLinkAllowPublicUpload()) { - throw new OCSForbiddenException($this->l->t('Public upload disabled by the administrator')); - } - - // Public upload can only be set for folders - if ($node instanceof \OCP\Files\File) { - throw new OCSNotFoundException($this->l->t('Public upload is only possible for publicly shared folders')); - } - - $permissions = Constants::PERMISSION_READ | - Constants::PERMISSION_CREATE | - Constants::PERMISSION_UPDATE | - Constants::PERMISSION_DELETE; - } - - // TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones - if ($this->shareManager->outgoingServer2ServerSharesAllowed()) { - $permissions |= Constants::PERMISSION_SHARE; - } - + $this->validateLinkSharePermissions($node, $permissions, $hasPublicUpload); $share->setPermissions($permissions); // Set password @@ -979,6 +954,71 @@ public function getShares( return new DataResponse($shares); } + private function getLinkSharePermissions(?int $permissions, ?bool $legacyPublicUpload): int { + $permissions = $permissions ?? Constants::PERMISSION_READ; + + // Legacy option handling + if ($legacyPublicUpload !== null) { + $permissions = $legacyPublicUpload + ? (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE) + : Constants::PERMISSION_READ; + } + + // TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones + if ($this->hasPermission($permissions, Constants::PERMISSION_READ) + && $this->shareManager->outgoingServer2ServerSharesAllowed()) { + $permissions |= Constants::PERMISSION_SHARE; + } + + return $permissions; + } + + /** + * Helper to check for legacy "publicUpload" handling. + * If the value is set to `true` or `false` then true or false are returned. + * Otherwise null is returned to indicate that the option was not (or wrong) set. + * + * @param null|string $legacyPublicUpload The value of `publicUpload` + */ + private function getLegacyPublicUpload(?string $legacyPublicUpload): ?bool { + if ($legacyPublicUpload === 'true') { + return true; + } elseif ($legacyPublicUpload === 'false') { + return false; + } + // Not set at all + return null; + } + + /** + * For link and email shares validate that only allowed combinations are set. + * + * @throw OCSBadRequestException If permission combination is invalid. + * @throw OCSForbiddenException If public upload was forbidden by the administrator. + */ + private function validateLinkSharePermissions(Node $node, int $permissions, ?bool $legacyPublicUpload): void { + if ($legacyPublicUpload && ($node instanceof File)) { + throw new OCSBadRequestException($this->l->t('Public upload is only possible for publicly shared folders')); + } + + // We need at least READ or CREATE (file drop) + if (!$this->hasPermission($permissions, Constants::PERMISSION_READ) + && !$this->hasPermission($permissions, Constants::PERMISSION_CREATE)) { + throw new OCSBadRequestException($this->l->t('Share must at least have READ or CREATE permissions')); + } + + // UPDATE and DELETE require a READ permission + if (!$this->hasPermission($permissions, Constants::PERMISSION_READ) + && ($this->hasPermission($permissions, Constants::PERMISSION_UPDATE) || $this->hasPermission($permissions, Constants::PERMISSION_DELETE))) { + throw new OCSBadRequestException($this->l->t('Share must have READ permission if UPDATE or DELETE permission is set')); + } + + // Check if public uploading was disabled + if ($this->hasPermission($permissions, Constants::PERMISSION_CREATE) + && !$this->shareManager->shareApiLinkAllowPublicUpload()) { + throw new OCSForbiddenException($this->l->t('Public upload disabled by the administrator')); + } + } /** * @param string $viewer @@ -1161,7 +1201,6 @@ private function hasPermission(int $permissionsSet, int $permissionsToCheck): bo return ($permissionsSet & $permissionsToCheck) === $permissionsToCheck; } - /** * @NoAdminRequired * @@ -1236,7 +1275,7 @@ public function updateShare( $this->checkInheritedAttributes($share); /** - * expirationdate, password and publicUpload only make sense for link shares + * expiration date, password and publicUpload only make sense for link shares */ if ($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) { @@ -1260,58 +1299,13 @@ public function updateShare( $share->setHideDownload(false); } - $newPermissions = null; - if ($publicUpload === 'true') { - $newPermissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE; - } elseif ($publicUpload === 'false') { - $newPermissions = Constants::PERMISSION_READ; - } - - if ($permissions !== null) { - $newPermissions = $permissions; - $newPermissions = $newPermissions & ~Constants::PERMISSION_SHARE; - } - - if ($newPermissions !== null) { - if (!$this->hasPermission($newPermissions, Constants::PERMISSION_READ) && !$this->hasPermission($newPermissions, Constants::PERMISSION_CREATE)) { - throw new OCSBadRequestException($this->l->t('Share must at least have READ or CREATE permissions')); - } - - if (!$this->hasPermission($newPermissions, Constants::PERMISSION_READ) && ( - $this->hasPermission($newPermissions, Constants::PERMISSION_UPDATE) || $this->hasPermission($newPermissions, Constants::PERMISSION_DELETE) - )) { - throw new OCSBadRequestException($this->l->t('Share must have READ permission if UPDATE or DELETE permission is set')); - } - } - - if ( - // legacy - $newPermissions === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE) || - // correct - $newPermissions === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE) - ) { - if (!$this->shareManager->shareApiLinkAllowPublicUpload()) { - throw new OCSForbiddenException($this->l->t('Public upload disabled by the administrator')); - } - - if (!($share->getNode() instanceof \OCP\Files\Folder)) { - throw new OCSBadRequestException($this->l->t('Public upload is only possible for publicly shared folders')); - } - - // normalize to correct public upload permissions - if ($publicUpload === 'true') { - $newPermissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE; - } - } - - if ($newPermissions !== null) { - // TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones - if (($newPermissions & Constants::PERMISSION_READ) && $this->shareManager->outgoingServer2ServerSharesAllowed()) { - $newPermissions |= Constants::PERMISSION_SHARE; - } - - $share->setPermissions($newPermissions); - $permissions = $newPermissions; + // If either manual permissions are specified or publicUpload + // then we need to also update the permissions of the share + if ($permissions !== null || $publicUpload !== null) { + $hasPublicUpload = $this->getLegacyPublicUpload($publicUpload); + $permissions = $this->getLinkSharePermissions($permissions ?? Constants::PERMISSION_READ, $hasPublicUpload); + $this->validateLinkSharePermissions($share->getNode(), $permissions, $hasPublicUpload); + $share->setPermissions($permissions); } if ($password === '') { diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index e0f3a1397e51c..7238a3d5207f7 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1762,10 +1762,14 @@ { "name": "publicUpload", "in": "query", - "description": "If public uploading is allowed", + "description": "If public uploading is allowed (deprecated)", "schema": { "type": "string", - "default": "false" + "nullable": true, + "enum": [ + "true", + "false" + ] } }, { diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index f909cf9753823..72e4fb00f7a87 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -35,17 +35,21 @@ */ namespace OCA\Files_Sharing\Tests\Controller; +use OCA\Files_Sharing\AppInfo\Application; use OCA\Files_Sharing\Controller\ShareAPIController; use OCP\App\IAppManager; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\OCS\OCSBadRequestException; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSNotFoundException; +use OCP\Constants; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountPoint; use OCP\Files\NotFoundException; use OCP\Files\Storage; +use OCP\Files\Storage\IStorage; use OCP\IConfig; use OCP\IDateTimeZone; use OCP\IGroup; @@ -62,6 +66,7 @@ use OCP\Share\IManager; use OCP\Share\IShare; use OCP\UserStatus\IManager as IUserStatusManager; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -74,25 +79,26 @@ */ class ShareAPIControllerTest extends TestCase { - private string $appName = 'files_sharing'; - private \OC\Share20\Manager|\PHPUnit\Framework\MockObject\MockObject $shareManager; - private IGroupManager|\PHPUnit\Framework\MockObject\MockObject $groupManager; - private IUserManager|\PHPUnit\Framework\MockObject\MockObject $userManager; - private IRequest|\PHPUnit\Framework\MockObject\MockObject $request; - private IRootFolder|\PHPUnit\Framework\MockObject\MockObject $rootFolder; - private IURLGenerator|\PHPUnit\Framework\MockObject\MockObject $urlGenerator; - private string|\PHPUnit\Framework\MockObject\MockObject $currentUser; private ShareAPIController $ocs; - private IL10N|\PHPUnit\Framework\MockObject\MockObject $l; - private IConfig|\PHPUnit\Framework\MockObject\MockObject $config; - private IAppManager|\PHPUnit\Framework\MockObject\MockObject $appManager; - private IServerContainer|\PHPUnit\Framework\MockObject\MockObject $serverContainer; - private IUserStatusManager|\PHPUnit\Framework\MockObject\MockObject $userStatusManager; - private IPreview|\PHPUnit\Framework\MockObject\MockObject $previewManager; - private IDateTimeZone|\PHPUnit\Framework\MockObject\MockObject $dateTimeZone; - private LoggerInterface $logger; + private string $currentUser; + + private \OC\Share20\Manager|MockObject $shareManager; + private IGroupManager|MockObject $groupManager; + private IUserManager|MockObject $userManager; + private IRequest|MockObject $request; + private IRootFolder|MockObject $rootFolder; + private IURLGenerator|MockObject $urlGenerator; + private IL10N|MockObject $l; + private IConfig|MockObject $config; + private IAppManager|MockObject $appManager; + private ContainerInterface|MockObject $serverContainer; + private IUserStatusManager|MockObject $userStatusManager; + private IPreview|MockObject $previewManager; + private IDateTimeZone|MockObject $dateTimeZone; + private LoggerInterface|MockObject $logger; protected function setUp(): void { + /** @var IManager|MockObject */ $this->shareManager = $this->createMock(IManager::class); $this->shareManager ->expects($this->any()) @@ -126,7 +132,7 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->ocs = new ShareAPIController( - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -146,12 +152,12 @@ protected function setUp(): void { } /** - * @return ShareAPIController|\PHPUnit\Framework\MockObject\MockObject + * @return ShareAPIController&MockObject */ private function mockFormatShare() { return $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -172,7 +178,7 @@ private function mockFormatShare() { } private function newShare() { - return \OC::$server->getShareManager()->newShare(); + return \OCP\Server::get(IManager::class)->newShare(); } @@ -762,11 +768,11 @@ public function dataGetShare() { /** * @dataProvider dataGetShare */ - public function testGetShare(\OCP\Share\IShare $share, array $result) { - /** @var ShareAPIController|\PHPUnit\Framework\MockObject\MockObject $ocs */ + public function testGetShare(IShare $share, array $result): void { + /** @var ShareAPIController&MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -853,7 +859,7 @@ public function testGetShareInvalidNode() { $this->expectException(\OCP\AppFramework\OCS\OCSNotFoundException::class); $this->expectExceptionMessage('Wrong share ID, share does not exist'); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setSharedBy('initiator') ->setSharedWith('recipient') ->setShareOwner('owner'); @@ -884,7 +890,7 @@ public function dataGetShares() { $folder->method('getDirectoryListing') ->willReturn([$file1, $file2]); - $file1UserShareOwner = \OC::$server->getShareManager()->newShare(); + $file1UserShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1UserShareOwner->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -898,7 +904,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_USER, ]; - $file1UserShareInitiator = \OC::$server->getShareManager()->newShare(); + $file1UserShareInitiator = \OCP\Server::get(IManager::class)->newShare(); $file1UserShareInitiator->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('currentUser') @@ -912,7 +918,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_USER, ]; - $file1UserShareRecipient = \OC::$server->getShareManager()->newShare(); + $file1UserShareRecipient = \OCP\Server::get(IManager::class)->newShare(); $file1UserShareRecipient->setShareType(IShare::TYPE_USER) ->setSharedWith('currentUser') ->setSharedBy('initiator') @@ -926,7 +932,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_USER, ]; - $file1UserShareOther = \OC::$server->getShareManager()->newShare(); + $file1UserShareOther = \OCP\Server::get(IManager::class)->newShare(); $file1UserShareOther->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -940,7 +946,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_USER, ]; - $file1GroupShareOwner = \OC::$server->getShareManager()->newShare(); + $file1GroupShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1GroupShareOwner->setShareType(IShare::TYPE_GROUP) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -954,7 +960,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_GROUP, ]; - $file1GroupShareRecipient = \OC::$server->getShareManager()->newShare(); + $file1GroupShareRecipient = \OCP\Server::get(IManager::class)->newShare(); $file1GroupShareRecipient->setShareType(IShare::TYPE_GROUP) ->setSharedWith('currentUserGroup') ->setSharedBy('initiator') @@ -968,7 +974,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_GROUP, ]; - $file1GroupShareOther = \OC::$server->getShareManager()->newShare(); + $file1GroupShareOther = \OCP\Server::get(IManager::class)->newShare(); $file1GroupShareOther->setShareType(IShare::TYPE_GROUP) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -977,7 +983,7 @@ public function dataGetShares() { ->setNode($file1) ->setId(108); - $file1LinkShareOwner = \OC::$server->getShareManager()->newShare(); + $file1LinkShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1LinkShareOwner->setShareType(IShare::TYPE_LINK) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -991,7 +997,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_LINK, ]; - $file1EmailShareOwner = \OC::$server->getShareManager()->newShare(); + $file1EmailShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1EmailShareOwner->setShareType(IShare::TYPE_EMAIL) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1005,7 +1011,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_EMAIL, ]; - $file1CircleShareOwner = \OC::$server->getShareManager()->newShare(); + $file1CircleShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1CircleShareOwner->setShareType(IShare::TYPE_CIRCLE) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1019,7 +1025,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_CIRCLE, ]; - $file1RoomShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RoomShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1RoomShareOwner->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1033,7 +1039,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_ROOM, ]; - $file1RemoteShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RemoteShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1RemoteShareOwner->setShareType(IShare::TYPE_REMOTE) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1048,7 +1054,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_REMOTE, ]; - $file1RemoteGroupShareOwner = \OC::$server->getShareManager()->newShare(); + $file1RemoteGroupShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file1RemoteGroupShareOwner->setShareType(IShare::TYPE_REMOTE_GROUP) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1063,7 +1069,7 @@ public function dataGetShares() { 'share_type' => IShare::TYPE_REMOTE_GROUP, ]; - $file2UserShareOwner = \OC::$server->getShareManager()->newShare(); + $file2UserShareOwner = \OCP\Server::get(IManager::class)->newShare(); $file2UserShareOwner->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -1394,11 +1400,11 @@ public function dataGetShares() { /** * @dataProvider dataGetShares */ - public function testGetShares(array $getSharesParameters, array $shares, array $extraShareTypes, array $expected) { - /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ + public function testGetShares(array $getSharesParameters, array $shares, array $extraShareTypes, array $expected): void { + /** @var ShareAPIController|MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -1414,7 +1420,8 @@ public function testGetShares(array $getSharesParameters, array $shares, array $ $this->dateTimeZone, $this->logger, $this->currentUser, - ])->setMethods(['formatShare']) + ]) + ->onlyMethods(['formatShare']) ->getMock(); $ocs->method('formatShare') @@ -1645,38 +1652,35 @@ public function testCreateShareInvalidPath() { $this->ocs->createShare('invalid-path'); } - - public function testCreateShareInvalidPermissions() { - $this->expectException(\OCP\AppFramework\OCS\OCSNotFoundException::class); - $this->expectExceptionMessage('Invalid permissions'); + public function testCreateShareInvalidShareType(): void { + $this->expectException(OCSBadRequestException::class); + $this->expectExceptionMessage('Unknown share type'); $share = $this->newShare(); $this->shareManager->method('newShare')->willReturn($share); - $userFolder = $this->getMockBuilder(Folder::class)->getMock(); - $this->rootFolder->expects($this->once()) - ->method('getUserFolder') - ->with('currentUser') - ->willReturn($userFolder); + [$userFolder, $file] = $this->getNonSharedUserFile(); + $this->rootFolder->expects($this->atLeastOnce()) + ->method('getUserFolder') + ->with('currentUser') + ->willReturn($userFolder); - $path = $this->getMockBuilder(File::class)->getMock(); - $userFolder->expects($this->once()) - ->method('get') - ->with('valid-path') - ->willReturn($path); + $userFolder->expects($this->atLeastOnce()) + ->method('get') + ->with('valid-path') + ->willReturn($file); $userFolder->method('getById') ->willReturn([]); - $path->expects($this->once()) + $file->expects($this->once()) ->method('lock') ->with(\OCP\Lock\ILockingProvider::LOCK_SHARED); - $this->ocs->createShare('valid-path', 32); + $this->ocs->createShare('valid-path', 31); } - - public function testCreateShareUserNoShareWith() { - $this->expectException(\OCP\AppFramework\OCS\OCSNotFoundException::class); + public function testCreateShareUserNoShareWith(): void { + $this->expectException(OCSNotFoundException::class); $this->expectExceptionMessage('Please specify a valid account to share with'); $share = $this->newShare(); @@ -1739,7 +1743,7 @@ public function testCreateShareUser() { /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -1832,10 +1836,10 @@ public function testCreateShareGroup() { $share = $this->newShare(); $this->shareManager->method('newShare')->willReturn($share); - /** @var ShareAPIController|\PHPUnit\Framework\MockObject\MockObject $ocs */ + /** @var ShareAPIController&MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -1959,7 +1963,9 @@ public function testCreateShareLinkNoLinksAllowed() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); + $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); + $this->shareManager->method('shareApiAllowLinks')->willReturn(false); $this->ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, IShare::TYPE_LINK); } @@ -1983,32 +1989,34 @@ public function testCreateShareLinkNoPublicUpload() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, IShare::TYPE_LINK, null, 'true'); } - public function testCreateShareLinkPublicUploadFile() { - $this->expectException(\OCP\AppFramework\OCS\OCSNotFoundException::class); + public function testCreateShareLinkPublicUploadFile(): void { + $this->expectException(OCSBadRequestException::class); $this->expectExceptionMessage('Public upload is only possible for publicly shared folders'); - $path = $this->getMockBuilder(File::class)->getMock(); - $path->method('getId')->willReturn(42); - $storage = $this->createMock(Storage::class); + $storage = $this->createMock(IStorage::class); $storage->method('instanceOfStorage') ->willReturnMap([ ['OCA\Files_Sharing\External\Storage', false], ['OCA\Files_Sharing\SharedStorage', false], ]); - $path->method('getStorage')->willReturn($storage); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(42); + $file->method('getStorage')->willReturn($storage); + $this->rootFolder->method('getUserFolder')->with($this->currentUser)->willReturnSelf(); - $this->rootFolder->method('get')->with('valid-path')->willReturn($path); + $this->rootFolder->method('get')->with('valid-path')->willReturn($file); $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2032,7 +2040,7 @@ public function testCreateShareLinkPublicUploadFolder() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2071,23 +2079,23 @@ public function testCreateShareLinkPassword() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); $this->shareManager->expects($this->once())->method('createShare')->with( - $this->callback(function (\OCP\Share\IShare $share) use ($path) { - return $share->getNode() === $path && - $share->getShareType() === IShare::TYPE_LINK && - $share->getPermissions() === \OCP\Constants::PERMISSION_ALL && - $share->getSharedBy() === 'currentUser' && - $share->getPassword() === 'password' && - $share->getExpirationDate() === null; + $this->callback(function (IShare $share) use ($path) { + return $share->getNode() === $path + && $share->getShareType() === IShare::TYPE_LINK + && $share->getPermissions() === Constants::PERMISSION_READ // publicUpload was set to false + && $share->getSharedBy() === 'currentUser' + && $share->getPassword() === 'password' + && $share->getExpirationDate() === null; }) )->willReturnArgument(0); $expected = new DataResponse([]); - $result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, IShare::TYPE_LINK, null, 'false', 'password', null, ''); + $result = $ocs->createShare('valid-path', Constants::PERMISSION_READ, IShare::TYPE_LINK, null, 'false', 'password', null, ''); $this->assertInstanceOf(get_class($expected), $result); $this->assertEquals($expected->getData(), $result->getData()); @@ -2110,7 +2118,7 @@ public function testCreateShareLinkSendPasswordByTalk() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2120,7 +2128,7 @@ public function testCreateShareLinkSendPasswordByTalk() { $this->callback(function (\OCP\Share\IShare $share) use ($path) { return $share->getNode() === $path && $share->getShareType() === IShare::TYPE_LINK && - $share->getPermissions() === \OCP\Constants::PERMISSION_ALL && + $share->getPermissions() === (Constants::PERMISSION_ALL & ~(Constants::PERMISSION_SHARE)) && $share->getSharedBy() === 'currentUser' && $share->getPassword() === 'password' && $share->getSendPasswordByTalk() === true && @@ -2129,7 +2137,7 @@ public function testCreateShareLinkSendPasswordByTalk() { )->willReturnArgument(0); $expected = new DataResponse([]); - $result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, IShare::TYPE_LINK, null, 'false', 'password', 'true', ''); + $result = $ocs->createShare('valid-path', Constants::PERMISSION_ALL, IShare::TYPE_LINK, null, 'true', 'password', 'true', ''); $this->assertInstanceOf(get_class($expected), $result); $this->assertEquals($expected->getData(), $result->getData()); @@ -2157,7 +2165,7 @@ public function testCreateShareLinkSendPasswordByTalkWithTalkDisabled() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2195,7 +2203,7 @@ public function testCreateShareValidExpireDate() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2241,7 +2249,7 @@ public function testCreateShareInvalidExpireDate() { $this->rootFolder->method('getById') ->willReturn([]); - $this->shareManager->method('newShare')->willReturn(\OC::$server->getShareManager()->newShare()); + $this->shareManager->method('newShare')->willReturn(\OCP\Server::get(IManager::class)->newShare()); $this->shareManager->method('shareApiAllowLinks')->willReturn(true); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); @@ -2255,7 +2263,7 @@ public function testCreateShareRemote() { /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -2323,7 +2331,7 @@ public function testCreateShareRemoteGroup() { /** @var \OCA\Files_Sharing\Controller\ShareAPIController $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -2425,11 +2433,7 @@ public function testCreateShareRoom() { )->willReturnCallback( function ($share) { $share->setSharedWith('recipientRoom'); - $share->setPermissions( - \OCP\Constants::PERMISSION_ALL & - ~\OCP\Constants::PERMISSION_DELETE & - ~\OCP\Constants::PERMISSION_CREATE - ); + $share->setPermissions(Constants::PERMISSION_ALL); } ); @@ -2438,16 +2442,12 @@ function ($share) { ->willReturn($helper); $this->shareManager->method('createShare') - ->with($this->callback(function (\OCP\Share\IShare $share) use ($path) { - return $share->getNode() === $path && - $share->getPermissions() === ( - \OCP\Constants::PERMISSION_ALL & - ~\OCP\Constants::PERMISSION_DELETE & - ~\OCP\Constants::PERMISSION_CREATE - ) && - $share->getShareType() === IShare::TYPE_ROOM && - $share->getSharedWith() === 'recipientRoom' && - $share->getSharedBy() === 'currentUser'; + ->with($this->callback(function (IShare $share) use ($path) { + return $share->getNode() === $path + && $share->getPermissions() === Constants::PERMISSION_ALL + && $share->getShareType() === IShare::TYPE_ROOM + && $share->getSharedWith() === 'recipientRoom' + && $share->getSharedBy() === 'currentUser'; })) ->willReturnArgument(0); @@ -2528,15 +2528,13 @@ public function testCreateShareRoomHelperThrowException() { ->willReturn(true); $helper = $this->getMockBuilder('\OCA\Talk\Share\Helper\ShareAPIController') - ->setMethods(['createShare']) + ->addMethods(['createShare']) ->getMock(); $helper->method('createShare') ->with( $share, 'recipientRoom', - \OCP\Constants::PERMISSION_ALL & - ~\OCP\Constants::PERMISSION_DELETE & - ~\OCP\Constants::PERMISSION_CREATE, + Constants::PERMISSION_ALL & ~(Constants::PERMISSION_CREATE | Constants::PERMISSION_DELETE), '' )->willReturnCallback( function ($share) { @@ -2557,14 +2555,14 @@ function ($share) { * Test for https://github.com/owncloud/core/issues/22587 * TODO: Remove once proper solution is in place */ - public function testCreateReshareOfFederatedMountNoDeletePermissions() { - $share = \OC::$server->getShareManager()->newShare(); + public function testCreateReshareOfFederatedMountNoDeletePermissions(): void { + $share = \OCP\Server::get(IManager::class)->newShare(); $this->shareManager->method('newShare')->willReturn($share); - /** @var ShareAPIController|\PHPUnit\Framework\MockObject\MockObject $ocs */ + /** @var ShareAPIController&MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -2758,8 +2756,8 @@ public function testUpdateLinkShareSet() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setNode($folder); @@ -2815,8 +2813,8 @@ public function testUpdateLinkShareEnablePublicUpload($permissions, $publicUploa $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setPassword('password') @@ -2876,8 +2874,8 @@ public function testUpdateLinkShareSetCRUDPermissions($permissions) { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setPassword('password') @@ -2907,7 +2905,7 @@ public function testUpdateLinkShareSetCRUDPermissions($permissions) { ->willReturn(42); $expected = new DataResponse([]); - $result = $ocs->updateShare(42, $permissions, 'password', null, 'true', null); + $result = $ocs->updateShare(42, $permissions, 'password', null, null, null); $this->assertInstanceOf(get_class($expected), $result); $this->assertEquals($expected->getData(), $result->getData()); @@ -2928,7 +2926,7 @@ public function testUpdateLinkShareSetInvalidCRUDPermissions1($permissions) { $this->expectException(\OCP\AppFramework\OCS\OCSBadRequestException::class); $this->expectExceptionMessage('Share must at least have READ or CREATE permissions'); - $this->testUpdateLinkShareSetCRUDPermissions($permissions); + $this->testUpdateLinkShareSetCRUDPermissions($permissions, null); } public function publicLinkInvalidPermissionsProvider2() { @@ -2964,8 +2962,8 @@ public function testUpdateLinkShareInvalidDate() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setNode($folder); @@ -3010,8 +3008,8 @@ public function testUpdateLinkSharePublicUploadNotAllowed($permissions, $publicU $folder->method('getId')->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setNode($folder); @@ -3029,25 +3027,31 @@ public function testUpdateLinkSharePublicUploadOnFile() { $ocs = $this->mockFormatShare(); - $file = $this->getMockBuilder(File::class)->getMock(); - $file->method('getId') - ->willReturn(42); - [$userFolder, $folder] = $this->getNonSharedUserFolder(); + [$userFolder, $file] = $this->getNonSharedUserFile(); $userFolder->method('getFirstNodeById') ->with(42) - ->willReturn($folder); + ->willReturn($file); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturn($userFolder); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setNode($file); - $this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share); - $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); + $this->shareManager + ->method('getShareById') + ->with('ocinternal:42') + ->willReturn($share); + $this->shareManager + ->method('shareApiLinkAllowPublicUpload') + ->willReturn(true); + $this->shareManager + ->method('updateShare') + ->with($share) + ->willThrowException(new \InvalidArgumentException('File shares cannot have create or delete permissions')); $ocs->updateShare(42, null, 'password', null, 'true', ''); } @@ -3397,8 +3401,8 @@ public function testUpdateLinkSharePublicUploadDoesNotChangeOther() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setPassword('password') @@ -3458,8 +3462,8 @@ public function testUpdateLinkSharePermissions() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setPassword('password') @@ -3518,8 +3522,8 @@ public function testUpdateLinkSharePermissionsShare() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) ->setPassword('password') @@ -3534,17 +3538,19 @@ public function testUpdateLinkSharePermissionsShare() { $this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true); - $this->shareManager->expects($this->once())->method('updateShare')->with( - $this->callback(function (\OCP\Share\IShare $share) use ($date) { - return $share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) && - $share->getPassword() === 'password' && - $share->getSendPasswordByTalk() === true && - $share->getExpirationDate() === $date && - $share->getNote() === 'note' && - $share->getLabel() === 'label' && - $share->getHideDownload() === true; - }) - )->willReturnArgument(0); + $this->shareManager->expects($this->once()) + ->method('updateShare') + ->with( + $this->callback(function (IShare $share) use ($date) { + return $share->getPermissions() === Constants::PERMISSION_ALL && + $share->getPassword() === 'password' && + $share->getSendPasswordByTalk() === true && + $share->getExpirationDate() === $date && + $share->getNote() === 'note' && + $share->getLabel() === 'label' && + $share->getHideDownload() === true; + }) + )->willReturnArgument(0); $this->rootFolder->method('getUserFolder') ->with($this->currentUser) @@ -3563,7 +3569,7 @@ public function testUpdateLinkSharePermissionsShare() { $this->shareManager->method('getSharedWith')->willReturn([]); $expected = new DataResponse([]); - $result = $ocs->updateShare(42, 31, null, null, null, null, null, null, null); + $result = $ocs->updateShare(42, Constants::PERMISSION_ALL, null, null, null, null, null, null, null); $this->assertInstanceOf(get_class($expected), $result); $this->assertEquals($expected->getData(), $result->getData()); @@ -3576,8 +3582,8 @@ public function testUpdateOtherPermissions() { $file->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); - $share->setPermissions(\OCP\Constants::PERMISSION_ALL) + $share = \OCP\Server::get(IManager::class)->newShare(); + $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_USER) ->setNode($file); @@ -3622,7 +3628,7 @@ public function testUpdateShareCannotIncreasePermissions() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share ->setId(42) ->setSharedBy($this->currentUser) @@ -3634,7 +3640,7 @@ public function testUpdateShareCannotIncreasePermissions() { // note: updateShare will modify the received instance but getSharedWith will reread from the database, // so their values will be different - $incomingShare = \OC::$server->getShareManager()->newShare(); + $incomingShare = \OCP\Server::get(IManager::class)->newShare(); $incomingShare ->setId(42) ->setSharedBy($this->currentUser) @@ -3694,7 +3700,7 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share ->setId(42) ->setSharedBy($this->currentUser) @@ -3706,7 +3712,7 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() { // note: updateShare will modify the received instance but getSharedWith will reread from the database, // so their values will be different - $incomingShare = \OC::$server->getShareManager()->newShare(); + $incomingShare = \OCP\Server::get(IManager::class)->newShare(); $incomingShare ->setId(42) ->setSharedBy($this->currentUser) @@ -3811,7 +3817,7 @@ public function dataFormatShare() { $result = []; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -3913,7 +3919,7 @@ public function dataFormatShare() { ], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -3967,7 +3973,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4023,7 +4029,7 @@ public function dataFormatShare() { // with existing group - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_GROUP) ->setSharedWith('recipientGroup') ->setSharedBy('initiator') @@ -4077,7 +4083,7 @@ public function dataFormatShare() { ]; // with unknown group / no group backend - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_GROUP) ->setSharedWith('recipientGroup2') ->setSharedBy('initiator') @@ -4128,7 +4134,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_LINK) ->setSharedBy('initiator') ->setShareOwner('owner') @@ -4187,7 +4193,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_LINK) ->setSharedBy('initiator') ->setShareOwner('owner') @@ -4246,7 +4252,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_REMOTE) ->setSharedBy('initiator') ->setSharedWith('user@server.com') @@ -4299,7 +4305,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_REMOTE_GROUP) ->setSharedBy('initiator') ->setSharedWith('user@server.com') @@ -4353,7 +4359,7 @@ public function dataFormatShare() { ]; // Circle with id, display name and avatar set by the Circles app - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_CIRCLE) ->setSharedBy('initiator') ->setSharedWith('Circle (Public circle, circleOwner) [4815162342]') @@ -4409,7 +4415,7 @@ public function dataFormatShare() { ]; // Circle with id set by the Circles app - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_CIRCLE) ->setSharedBy('initiator') ->setSharedWith('Circle (Public circle, circleOwner) [4815162342]') @@ -4462,7 +4468,7 @@ public function dataFormatShare() { ]; // Circle with id not set by the Circles app - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_CIRCLE) ->setSharedBy('initiator') ->setSharedWith('Circle (Public circle, circleOwner)') @@ -4514,7 +4520,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedBy('initiator') ->setSharedWith('recipient') @@ -4529,7 +4535,7 @@ public function dataFormatShare() { [], $share, [], true ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_EMAIL) ->setSharedBy('initiator') ->setSharedWith('user@server.com') @@ -4584,7 +4590,7 @@ public function dataFormatShare() { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_EMAIL) ->setSharedBy('initiator') ->setSharedWith('user@server.com') @@ -4641,7 +4647,7 @@ public function dataFormatShare() { ]; // Preview is available - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4807,7 +4813,7 @@ public function dataFormatRoomShare() { $result = []; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipientRoom') ->setSharedBy('initiator') @@ -4859,7 +4865,7 @@ public function dataFormatRoomShare() { ], $share, false, [] ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipientRoom') ->setSharedBy('initiator') @@ -4965,6 +4971,9 @@ public function testFormatRoomShare(array $expects, \OCP\Share\IShare $share, bo $this->assertEquals($expects, $result); } + /** + * @return list{Folder, Folder} + */ private function getNonSharedUserFolder(): array { $node = $this->getMockBuilder(Folder::class)->getMock(); $userFolder = $this->getMockBuilder(Folder::class)->getMock(); @@ -4980,6 +4989,9 @@ private function getNonSharedUserFolder(): array { return [$userFolder, $node]; } + /** + * @return list{Folder, File} + */ private function getNonSharedUserFile(): array { $node = $this->getMockBuilder(File::class)->getMock(); $userFolder = $this->getMockBuilder(Folder::class)->getMock(); diff --git a/build/integration/features/bootstrap/SharingContext.php b/build/integration/features/bootstrap/SharingContext.php index f187e89f08fac..4267cedd9b107 100644 --- a/build/integration/features/bootstrap/SharingContext.php +++ b/build/integration/features/bootstrap/SharingContext.php @@ -46,6 +46,7 @@ protected function resetAppConfigs() { $this->deleteServerConfig('core', 'shareapi_default_expire_date'); $this->deleteServerConfig('core', 'shareapi_expire_after_n_days'); $this->deleteServerConfig('core', 'link_defaultExpDays'); + $this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled'); $this->runOcc(['config:system:delete', 'share_folder']); } diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index b180ad0807271..baffe0eca0130 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -122,3 +122,61 @@ Scenario: Receiving a share of a file without delete permission gives delete per | path | /welcome (2).txt | | permissions | 19 | | item_permissions | 27 | + +# This is a regression test as in the past creating a file drop required creating with permissions=5 +# and then afterwards update the share to permissions=4 +Scenario: Directly create link share with CREATE only permissions (file drop) + Given user "user0" exists + And As an "user0" + And user "user0" created a folder "/TMP" + When creating a share with + | path | TMP | + | shareType | 3 | + | permissions | 4 | + And Getting info of last share + Then Share fields of last share match with + | uid_file_owner | user0 | + | share_type | 3 | + | permissions | 4 | + +Scenario: Directly create email share with CREATE only permissions (file drop) + Given user "user0" exists + And As an "user0" + And user "user0" created a folder "/TMP" + When creating a share with + | path | TMP | + | shareType | 4 | + | shareWith | j.doe@example.com | + | permissions | 4 | + And Getting info of last share + Then Share fields of last share match with + | uid_file_owner | user0 | + | share_type | 4 | + | permissions | 4 | + +# This ensures the legacy behavior of sharing v1 is kept +Scenario: publicUpload overrides permissions + Given user "user0" exists + And As an "user0" + And parameter "outgoing_server2server_share_enabled" of app "files_sharing" is set to "no" + And user "user0" created a folder "/TMP" + When creating a share with + | path | TMP | + | shareType | 3 | + | permissions | 4 | + | publicUpload | true | + And Getting info of last share + Then Share fields of last share match with + | uid_file_owner | user0 | + | share_type | 3 | + | permissions | 15 | + When creating a share with + | path | TMP | + | shareType | 3 | + | permissions | 4 | + | publicUpload | false | + And Getting info of last share + Then Share fields of last share match with + | uid_file_owner | user0 | + | share_type | 3 | + | permissions | 1 | diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 4f02e45c73588..92eeaa2b83b9c 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -244,6 +244,17 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) { throw new \InvalidArgumentException('A share requires permissions'); } + // Permissions must be valid + if ($share->getPermissions() < 0 || $share->getPermissions() > \OCP\Constants::PERMISSION_ALL) { + throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); + } + + // Single file shares should never have delete or create permissions + if (($share->getNode() instanceof File) + && (($share->getPermissions() & (\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE)) !== 0)) { + throw new \InvalidArgumentException($this->l->t('File shares cannot have create or delete permissions')); + } + $permissions = 0; $nodesForUser = $userFolder->getById($share->getNodeId()); foreach ($nodesForUser as $node) { diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index edc072f4695b9..461bddf3db68e 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -902,10 +902,9 @@ public function dataGeneralChecks() { $mount = $this->createMock(MoveableMount::class); $limitedPermssions->method('getMountPoint')->willReturn($mount); - - $data[] = [$this->createShare(null, IShare::TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 31, null, null), 'Cannot increase permissions of path', true]; + // increase permissions of a re-share $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, 17, null, null), 'Cannot increase permissions of path', true]; - $data[] = [$this->createShare(null, IShare::TYPE_LINK, $limitedPermssions, null, $user0, $user0, 3, null, null), 'Cannot increase permissions of path', true]; + $data[] = [$this->createShare(null, IShare::TYPE_USER, $limitedPermssions, $user2, $user0, $user0, 3, null, null), 'Cannot increase permissions of path', true]; $nonMovableStorage = $this->createMock(Storage\IStorage::class); $nonMovableStorage->method('instanceOfStorage') @@ -936,6 +935,20 @@ public function dataGeneralChecks() { $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $rootFolder, $group0, $user0, $user0, 2, null, null), 'You cannot share your root folder', true]; $data[] = [$this->createShare(null, IShare::TYPE_LINK, $rootFolder, null, $user0, $user0, 16, null, null), 'You cannot share your root folder', true]; + $allPermssionsFiles = $this->createMock(File::class); + $allPermssionsFiles->method('isShareable')->willReturn(true); + $allPermssionsFiles->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); + $allPermssionsFiles->method('getId')->willReturn(187); + $allPermssionsFiles->method('getOwner') + ->willReturn($owner); + $allPermssionsFiles->method('getStorage') + ->willReturn($storage); + + // test invalid CREATE or DELETE permissions + $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssionsFiles, $user2, $user0, $user0, \OCP\Constants::PERMISSION_ALL, null, null), 'File shares cannot have create or delete permissions', true]; + $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssionsFiles, $group0, $user0, $user0, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE, null, null), 'File shares cannot have create or delete permissions', true]; + $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssionsFiles, null, $user0, $user0, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_DELETE, null, null), 'File shares cannot have create or delete permissions', true]; + $allPermssions = $this->createMock(Folder::class); $allPermssions->method('isShareable')->willReturn(true); $allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL); @@ -948,6 +961,12 @@ public function dataGeneralChecks() { $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 30, null, null), 'Shares need at least read permissions', true]; $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 2, null, null), 'Shares need at least read permissions', true]; + // test invalid permissions + $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 32, null, null), 'Valid permissions are required for sharing', true]; + $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 63, null, null), 'Valid permissions are required for sharing', true]; + $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssions, null, $user0, $user0, -1, null, null), 'Valid permissions are required for sharing', true]; + + // working shares $data[] = [$this->createShare(null, IShare::TYPE_USER, $allPermssions, $user2, $user0, $user0, 31, null, null), null, false]; $data[] = [$this->createShare(null, IShare::TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 3, null, null), null, false]; $data[] = [$this->createShare(null, IShare::TYPE_LINK, $allPermssions, null, $user0, $user0, 17, null, null), null, false]; @@ -2414,9 +2433,10 @@ public function testCanShare($expected, $sharingEnabled, $disabledForUser) { $this->assertEquals($expected, !$exception); } - public function testCreateShareUser() { + public function testCreateShareUser(): void { + /** @var Manager|MockObject $manager */ $manager = $this->createManagerMock() - ->setMethods(['canShare', 'generalCreateChecks', 'userCreateChecks', 'pathCreateChecks']) + ->onlyMethods(['canShare', 'generalCreateChecks', 'userCreateChecks', 'pathCreateChecks']) ->getMock(); $shareOwner = $this->createMock(IUser::class);