diff --git a/.github/workflows/cypress.yml b/.github/workflows/cypress.yml index 54277ffef3fde..33c7c04b0a39f 100644 --- a/.github/workflows/cypress.yml +++ b/.github/workflows/cypress.yml @@ -112,7 +112,7 @@ jobs: CYPRESS_RECORD_KEY: ${{ secrets.CYPRESS_RECORD_KEY }} - name: Upload snapshots - uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 + uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 if: always() with: name: snapshots_${{ matrix.containers }} @@ -123,7 +123,7 @@ jobs: run: docker logs nextcloud-cypress-tests-${{ env.APP_NAME }} > nextcloud.log - name: Upload NC logs - uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 + uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 if: failure() && matrix.containers != 'component' with: name: nc_logs_${{ matrix.containers }} @@ -134,7 +134,7 @@ jobs: run: docker exec nextcloud-cypress-tests-server tar -cvjf - data > data.tar - name: Upload data dir archive - uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 + uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 if: failure() && matrix.containers != 'component' with: name: nc_data_${{ matrix.containers }} diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml index 3d63d5fa67189..987f8d070d06d 100644 --- a/.github/workflows/performance.yml +++ b/.github/workflows/performance.yml @@ -79,7 +79,7 @@ jobs: - name: Upload profiles if: always() - uses: actions/upload-artifact@v2 + uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0 with: name: profiles path: | diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 566b2fece5da0..1212e1b38d8e4 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -45,7 +45,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; @@ -60,6 +59,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; @@ -515,20 +515,21 @@ public function deleteShare(string $id): DataResponse { /** * @NoAdminRequired * - * @param string $path - * @param int $permissions - * @param int $shareType - * @param string $shareWith - * @param string $publicUpload - * @param string $password - * @param string $sendPasswordByTalk - * @param string $expireDate - * @param string $label - * @param string $attributes + * @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 '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|null $expireDate The expiry date of the share in the user's timezone at 00:00. + * If $expireDate is not supplied or set to `null`, the system default will be used. + * @param string $note Note for the share + * @param string $label Label for the share (only used in link and email) + * @param string|null $attributes Additional attributes for the share * * @return DataResponse - * @throws NotFoundException - * @throws OCSBadRequestException + * @throws OCSBadRequestException Unknown share type * @throws OCSException * @throws OCSForbiddenException * @throws OCSNotFoundException @@ -539,8 +540,8 @@ public function createShare( string $path = null, int $permissions = null, int $shareType = -1, - string $shareWith = null, - string $publicUpload = 'false', + ?string $shareWith = null, + ?string $publicUpload = null, string $password = '', string $sendPasswordByTalk = null, string $expireDate = null, @@ -549,17 +550,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, $permissions); // Verify path if ($path === null) { @@ -578,7 +569,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(); } @@ -591,17 +582,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); } /** @@ -661,28 +658,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 @@ -936,6 +912,73 @@ 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, ?int $permissions): ?bool { + if ($legacyPublicUpload === 'true') { + return true; + } elseif ($legacyPublicUpload === 'false') { + return false; + } elseif ($legacyPublicUpload === null && $permissions === null) { + 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 @@ -1125,7 +1168,6 @@ private function hasPermission(int $permissionsSet, int $permissionsToCheck): bo return ($permissionsSet & $permissionsToCheck) === $permissionsToCheck; } - /** * @NoAdminRequired * @@ -1198,7 +1240,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) { @@ -1222,58 +1264,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); + $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 e6518ca5e6412..0b6540c968348 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1536,10 +1536,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 e4035891384d0..12b20639aa77f 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -35,11 +35,14 @@ */ 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; @@ -63,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; @@ -75,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()) @@ -127,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, @@ -147,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, @@ -173,7 +178,7 @@ private function mockFormatShare() { } private function newShare() { - return \OC::$server->getShareManager()->newShare(); + return \OCP\Server::get(IManager::class)->newShare(); } @@ -738,11 +743,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, @@ -825,7 +830,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'); @@ -856,7 +861,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') @@ -870,7 +875,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') @@ -884,7 +889,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') @@ -898,7 +903,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') @@ -912,7 +917,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') @@ -926,7 +931,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') @@ -940,7 +945,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') @@ -949,7 +954,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') @@ -963,7 +968,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') @@ -977,7 +982,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') @@ -991,7 +996,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') @@ -1005,7 +1010,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') @@ -1020,7 +1025,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') @@ -1035,7 +1040,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') @@ -1366,11 +1371,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, @@ -1386,7 +1391,8 @@ public function testGetShares(array $getSharesParameters, array $shares, array $ $this->dateTimeZone, $this->logger, $this->currentUser, - ])->setMethods(['formatShare']) + ]) + ->onlyMethods(['formatShare']) ->getMock(); $ocs->method('formatShare') @@ -1617,38 +1623,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 user'); $share = $this->newShare(); @@ -1711,7 +1714,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, @@ -1804,10 +1807,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, @@ -1930,7 +1933,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); } @@ -1953,31 +1958,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(); - $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); @@ -2000,7 +2008,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); @@ -2038,23 +2046,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()); @@ -2076,7 +2084,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); @@ -2086,7 +2094,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 && @@ -2095,7 +2103,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()); @@ -2122,7 +2130,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); @@ -2159,7 +2167,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); @@ -2204,7 +2212,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); @@ -2218,7 +2226,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, @@ -2286,7 +2294,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, @@ -2388,11 +2396,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); } ); @@ -2401,16 +2405,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); @@ -2491,15 +2491,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) { @@ -2520,14 +2518,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, @@ -2719,8 +2717,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); @@ -2776,8 +2774,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') @@ -2837,8 +2835,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') @@ -2868,7 +2866,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()); @@ -2889,7 +2887,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() { @@ -2925,8 +2923,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); @@ -2971,8 +2969,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); @@ -2990,25 +2988,32 @@ 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(); + $file->method('getId')->willReturn(42); $userFolder->method('getById') ->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', ''); } @@ -3358,8 +3363,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') @@ -3419,8 +3424,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') @@ -3479,8 +3484,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') @@ -3495,17 +3500,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) @@ -3524,7 +3531,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()); @@ -3537,8 +3544,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); @@ -3583,7 +3590,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) @@ -3595,7 +3602,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) @@ -3655,7 +3662,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) @@ -3667,7 +3674,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) @@ -3753,7 +3760,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') @@ -3847,7 +3854,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') @@ -3897,7 +3904,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') @@ -3949,7 +3956,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') @@ -3998,7 +4005,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') @@ -4044,7 +4051,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') @@ -4098,7 +4105,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') @@ -4152,7 +4159,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') @@ -4200,7 +4207,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') @@ -4249,7 +4256,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]') @@ -4300,7 +4307,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]') @@ -4348,7 +4355,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)') @@ -4395,7 +4402,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') @@ -4410,7 +4417,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') @@ -4460,7 +4467,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') @@ -4512,7 +4519,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') @@ -4667,7 +4674,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') @@ -4714,7 +4721,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') @@ -4812,6 +4819,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(); @@ -4826,6 +4836,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 new file mode 100644 index 0000000000000..ff36858c6d7aa --- /dev/null +++ b/build/integration/sharing_features/sharing-v1-part4.feature @@ -0,0 +1,64 @@ +Feature: sharing + Background: + Given using api version "1" + Given using new dav path + +# See sharing-v1-part3.feature + +# 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 4743d2873bb5b..25b2e82417839 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -296,7 +296,6 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) { throw new \InvalidArgumentException('A share requires permissions'); } - $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); $permissions = 0; $isReshare = $share->getNode()->getOwner() && $share->getNode()->getOwner()->getUID() !== $share->getSharedBy(); @@ -305,6 +304,7 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) { $isReshare = $share->getShareOwner() !== $share->getSharedBy(); } + $isFederatedShare = $share->getNode()->getStorage()->instanceOfStorage('\OCA\Files_Sharing\External\Storage'); if (!$isFederatedShare && $isReshare) { $userMounts = array_filter($userFolder->getById($share->getNode()->getId()), function ($mount) { // We need to filter since there might be other mountpoints that contain the file @@ -349,6 +349,17 @@ protected function generalCreateChecks(IShare $share, bool $isUpdate = false) { } } + // 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')); + } + // Check that we do not share with more permissions than we have if ($share->getPermissions() & ~$permissions) { $path = $userFolder->getRelativePath($share->getNode()->getPath()); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 962db1dec8a9b..88d239e6d7e1a 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -690,10 +690,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]; $nonMoveableMountPermssions = $this->createMock(Folder::class); $nonMoveableMountPermssions->method('isShareable')->willReturn(true); @@ -718,6 +717,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); @@ -730,6 +743,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]; @@ -2185,9 +2204,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);