diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 591ebb3f9645b..f54d419d90b1c 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -10,7 +10,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; @@ -29,6 +28,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; @@ -532,8 +532,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. @@ -558,7 +558,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, @@ -568,17 +568,7 @@ public function createShare( ?string $sendMail = 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) { @@ -597,7 +587,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(); } @@ -610,19 +600,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')); - } - - // Shares always require read permissions OR create permissions - if (($permissions & Constants::PERMISSION_READ) === 0 && ($permissions & Constants::PERMISSION_CREATE) === 0) { + // 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; } - 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); } /** @@ -694,28 +688,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 @@ -967,6 +940,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 @@ -1147,7 +1185,6 @@ private function hasPermission(int $permissionsSet, int $permissionsToCheck): bo return ($permissionsSet & $permissionsToCheck) === $permissionsToCheck; } - /** * Update a share * @@ -1232,7 +1269,7 @@ public function updateShare( } /** - * 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) { @@ -1256,58 +1293,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 4932a7e14e447..098a769c52f88 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1747,8 +1747,12 @@ }, "publicUpload": { "type": "string", - "default": "false", - "description": "If public uploading is allowed" + "nullable": true, + "enum": [ + "true", + "false" + ], + "description": "If public uploading is allowed (deprecated)" }, "password": { "type": "string", diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index cdf38d66e0f3e..e04b492925916 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -6,17 +6,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; @@ -35,6 +39,7 @@ use OCP\Share\IProviderFactory; 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; @@ -47,27 +52,28 @@ */ 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 IProviderFactory|\PHPUnit\Framework\MockObject\MockObject $factory; - private IMailer|\PHPUnit\Framework\MockObject\MockObject $mailer; + 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; + private IProviderFactory|MockObject $factory; + private IMailer|MockObject $mailer; protected function setUp(): void { + /** @var IManager|MockObject */ $this->shareManager = $this->createMock(IManager::class); $this->shareManager ->expects($this->any()) @@ -103,7 +109,7 @@ protected function setUp(): void { $this->mailer = $this->createMock(IMailer::class); $this->ocs = new ShareAPIController( - $this->appName, + Application::APP_ID, $this->request, $this->shareManager, $this->groupManager, @@ -125,12 +131,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, @@ -153,7 +159,7 @@ private function mockFormatShare() { } private function newShare() { - return \OC::$server->getShareManager()->newShare(); + return \OCP\Server::get(IManager::class)->newShare(); } @@ -743,11 +749,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, @@ -836,7 +842,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'); @@ -867,7 +873,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') @@ -881,7 +887,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') @@ -895,7 +901,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') @@ -909,7 +915,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') @@ -923,7 +929,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') @@ -937,7 +943,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') @@ -951,7 +957,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') @@ -960,7 +966,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') @@ -974,7 +980,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') @@ -988,7 +994,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') @@ -1002,7 +1008,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') @@ -1016,7 +1022,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') @@ -1031,7 +1037,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') @@ -1046,7 +1052,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') @@ -1377,11 +1383,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, @@ -1399,7 +1405,8 @@ public function testGetShares(array $getSharesParameters, array $shares, array $ $this->factory, $this->mailer, $this->currentUser, - ])->setMethods(['formatShare']) + ]) + ->onlyMethods(['formatShare']) ->getMock(); $ocs->method('formatShare') @@ -1630,38 +1637,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(); @@ -1724,7 +1728,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, @@ -1819,10 +1823,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, @@ -1948,7 +1952,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); } @@ -1972,32 +1978,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); @@ -2021,7 +2029,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); @@ -2060,23 +2068,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()); @@ -2099,7 +2107,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); @@ -2109,7 +2117,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 && @@ -2118,7 +2126,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()); @@ -2146,7 +2154,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); @@ -2184,7 +2192,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); @@ -2230,7 +2238,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); @@ -2244,7 +2252,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, @@ -2314,7 +2322,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, @@ -2418,11 +2426,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); } ); @@ -2431,16 +2435,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); @@ -2521,15 +2521,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) { @@ -2550,14 +2548,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, @@ -2753,8 +2751,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); @@ -2810,8 +2808,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') @@ -2871,8 +2869,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') @@ -2902,7 +2900,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()); @@ -2923,7 +2921,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() { @@ -2959,8 +2957,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); @@ -3005,8 +3003,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); @@ -3024,25 +3022,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', ''); } @@ -3392,8 +3396,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') @@ -3453,8 +3457,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') @@ -3513,8 +3517,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') @@ -3529,17 +3533,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) @@ -3558,7 +3564,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()); @@ -3571,8 +3577,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); @@ -3617,7 +3623,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) @@ -3629,7 +3635,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) @@ -3689,7 +3695,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) @@ -3701,7 +3707,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) @@ -3806,7 +3812,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') @@ -3908,7 +3914,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') @@ -3962,7 +3968,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') @@ -4018,7 +4024,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') @@ -4072,7 +4078,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') @@ -4123,7 +4129,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') @@ -4182,7 +4188,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') @@ -4241,7 +4247,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') @@ -4294,7 +4300,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') @@ -4348,7 +4354,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]') @@ -4404,7 +4410,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]') @@ -4457,7 +4463,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)') @@ -4509,7 +4515,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') @@ -4524,7 +4530,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') @@ -4579,7 +4585,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') @@ -4636,7 +4642,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') @@ -4802,7 +4808,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') @@ -4854,7 +4860,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') @@ -4960,6 +4966,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(); @@ -4975,6 +4984,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 97c2a35ad8461..d2f1a2446aee2 100644 --- a/build/integration/features/bootstrap/SharingContext.php +++ b/build/integration/features/bootstrap/SharingContext.php @@ -27,6 +27,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 1b32f99a0a283..d138f0a176964 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -124,3 +124,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 1006a0f24bf76..4f3b638507475 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -212,6 +212,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 c780d29bfed66..d639b57f285b3 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -885,10 +885,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') @@ -919,6 +918,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); @@ -931,6 +944,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]; @@ -2397,9 +2416,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);