diff options
20 files changed, 537 insertions, 452 deletions
diff --git a/apps/dav/lib/CalDAV/Schedule/IMipService.php b/apps/dav/lib/CalDAV/Schedule/IMipService.php index 2ff097bb1b7..69c9e774042 100644 --- a/apps/dav/lib/CalDAV/Schedule/IMipService.php +++ b/apps/dav/lib/CalDAV/Schedule/IMipService.php @@ -1240,12 +1240,12 @@ class IMipService { 'Second' => $this->l10n->t('Second'), 'Third' => $this->l10n->t('Third'), 'Fourth' => $this->l10n->t('Fourth'), - 'Fifty' => $this->l10n->t('Fifty'), + 'Fifth' => $this->l10n->t('Fifth'), 'Last' => $this->l10n->t('Last'), 'Second Last' => $this->l10n->t('Second Last'), 'Third Last' => $this->l10n->t('Third Last'), 'Fourth Last' => $this->l10n->t('Fourth Last'), - 'Fifty Last' => $this->l10n->t('Fifty Last'), + 'Fifth Last' => $this->l10n->t('Fifth Last'), }; } } diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php index 372dde198a0..350f4718712 100644 --- a/apps/files_sharing/lib/Cache.php +++ b/apps/files_sharing/lib/Cache.php @@ -63,12 +63,12 @@ class Cache extends CacheJail { /** @var Jail $currentStorage */ $absoluteRoot = $currentStorage->getJailedPath($absoluteRoot); } - $this->root = $absoluteRoot; + $this->root = $absoluteRoot ?? ''; } return $this->root; } - protected function getGetUnjailedRoot() { + protected function getGetUnjailedRoot(): string { return $this->sourceRootInfo->getPath(); } diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index e7b4a8d6bb8..1fb62480049 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -10,7 +10,6 @@ declare(strict_types=1); namespace OCA\Files_Sharing\Controller; use Exception; -use OC\Files\FileInfo; use OC\Files\Storage\Wrapper\Wrapper; use OCA\Circles\Api\v1\Circles; use OCA\Files\Helper; @@ -536,8 +535,8 @@ class ShareAPIController extends OCSController { * @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. @@ -562,7 +561,7 @@ class ShareAPIController extends OCSController { ?int $permissions = null, int $shareType = -1, ?string $shareWith = null, - string $publicUpload = 'false', + ?string $publicUpload = null, string $password = '', ?string $sendPasswordByTalk = null, ?string $expireDate = null, @@ -571,18 +570,10 @@ class ShareAPIController extends OCSController { ?string $attributes = null, ?string $sendMail = null, ): DataResponse { - $share = $this->shareManager->newShare(); - - if ($permissions === null) { - if ($shareType === IShare::TYPE_LINK - || $shareType === IShare::TYPE_EMAIL) { + assert($this->userId !== null); - // 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); - } - } + $share = $this->shareManager->newShare(); + $hasPublicUpload = $this->getLegacyPublicUpload($publicUpload); // Verify path if ($path === null) { @@ -601,7 +592,7 @@ class ShareAPIController extends OCSController { // 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(); } @@ -614,19 +605,23 @@ class ShareAPIController extends OCSController { 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; } + // For legacy reasons the API allows to pass PERMISSIONS_ALL even for single file shares (I look at you Talk) if ($node instanceof File) { - // Single file shares should never have delete or create permissions - $permissions &= ~Constants::PERMISSION_DELETE; - $permissions &= ~Constants::PERMISSION_CREATE; + // if this is a single file share we remove the DELETE and CREATE permissions + $permissions = $permissions & ~(Constants::PERMISSION_DELETE | Constants::PERMISSION_CREATE); } /** @@ -701,28 +696,7 @@ class ShareAPIController extends OCSController { 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 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 @@ -974,6 +948,71 @@ class ShareAPIController extends OCSController { 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 @@ -1154,7 +1193,6 @@ class ShareAPIController extends OCSController { return ($permissionsSet & $permissionsToCheck) === $permissionsToCheck; } - /** * Update a share * @@ -1242,7 +1280,7 @@ class ShareAPIController extends OCSController { } /** - * 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) { @@ -1259,58 +1297,13 @@ class ShareAPIController extends OCSController { $share->setAttributes($attributes); - $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 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/lib/External/Scanner.php b/apps/files_sharing/lib/External/Scanner.php index d3b54e0f0f6..e45d3723923 100644 --- a/apps/files_sharing/lib/External/Scanner.php +++ b/apps/files_sharing/lib/External/Scanner.php @@ -29,9 +29,10 @@ class Scanner extends \OC\Files\Cache\Scanner { * @param string $file file to scan * @param int $reuseExisting * @param int $parentId - * @param array | null $cacheData existing data in the cache for the file to be scanned + * @param \OC\Files\Cache\CacheEntry|array|null|false $cacheData existing data in the cache for the file to be scanned * @param bool $lock set to false to disable getting an additional read lock during scanning - * @return array | null an array of metadata of the scanned file + * @param array|null $data the metadata for the file, as returned by the storage + * @return array|null an array of metadata of the scanned file */ public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = null, $lock = true, $data = null) { try { diff --git a/apps/files_sharing/lib/Scanner.php b/apps/files_sharing/lib/Scanner.php index d346d34cb03..8a695ce9539 100644 --- a/apps/files_sharing/lib/Scanner.php +++ b/apps/files_sharing/lib/Scanner.php @@ -57,7 +57,7 @@ class Scanner extends \OC\Files\Cache\Scanner { $sourceScanner = $this->getSourceScanner(); if ($sourceScanner instanceof ObjectStoreScanner) { // ObjectStoreScanner doesn't scan - return []; + return null; } else { return parent::scanFile($file, $reuseExisting, $parentId, $cacheData, $lock); } diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index 145847b9550..e70764a59f2 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -1780,8 +1780,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 abb98826ab5..f8c316cd2e9 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -41,6 +41,7 @@ use OCP\Share\IManager; 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; @@ -54,24 +55,26 @@ use Test\TestCase; 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 string $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 IManager&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 { $this->shareManager = $this->createMock(IManager::class); @@ -131,7 +134,7 @@ class ShareAPIControllerTest extends TestCase { } /** - * @return ShareAPIController|\PHPUnit\Framework\MockObject\MockObject + * @return ShareAPIController&MockObject */ private function mockFormatShare() { return $this->getMockBuilder(ShareAPIController::class) @@ -159,7 +162,7 @@ class ShareAPIControllerTest extends TestCase { } private function newShare() { - return \OC::$server->getShareManager()->newShare(); + return \OCP\Server::get(IManager::class)->newShare(); } @@ -816,7 +819,7 @@ class ShareAPIControllerTest extends TestCase { * @dataProvider dataGetShare */ public function testGetShare(IShare $share, array $result): void { - /** @var ShareAPIController|\PHPUnit\Framework\MockObject\MockObject $ocs */ + /** @var ShareAPIController&MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ $this->appName, @@ -908,7 +911,7 @@ class ShareAPIControllerTest extends TestCase { $this->expectException(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'); @@ -939,7 +942,7 @@ class ShareAPIControllerTest extends TestCase { $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') @@ -953,7 +956,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -967,7 +970,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -981,7 +984,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -995,7 +998,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1009,7 +1012,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1023,7 +1026,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1032,7 +1035,7 @@ class ShareAPIControllerTest extends TestCase { ->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') @@ -1046,7 +1049,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1060,7 +1063,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1074,7 +1077,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1088,7 +1091,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1103,7 +1106,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1118,7 +1121,7 @@ class ShareAPIControllerTest extends TestCase { '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') @@ -1450,7 +1453,7 @@ class ShareAPIControllerTest extends TestCase { * @dataProvider dataGetShares */ public function testGetShares(array $getSharesParameters, array $shares, array $extraShareTypes, array $expected): void { - /** @var ShareAPIController $ocs */ + /** @var ShareAPIController&MockObject $ocs */ $ocs = $this->getMockBuilder(ShareAPIController::class) ->setConstructorArgs([ $this->appName, @@ -1471,7 +1474,8 @@ class ShareAPIControllerTest extends TestCase { $this->factory, $this->mailer, $this->currentUser, - ])->setMethods(['formatShare']) + ]) + ->onlyMethods(['formatShare']) ->getMock(); $ocs->method('formatShare') @@ -1702,36 +1706,33 @@ class ShareAPIControllerTest extends TestCase { $this->ocs->createShare('invalid-path'); } - - public function testCreateShareInvalidPermissions(): void { - $this->expectException(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()) + [$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()) + $userFolder->expects($this->atLeastOnce()) ->method('get') ->with('valid-path') - ->willReturn($path); + ->willReturn($file); $userFolder->method('getById') ->willReturn([]); - $path->expects($this->once()) + $file->expects($this->once()) ->method('lock') ->with(ILockingProvider::LOCK_SHARED); - $this->ocs->createShare('valid-path', 32); + $this->ocs->createShare('valid-path', 31); } - public function testCreateShareUserNoShareWith(): void { $this->expectException(OCSNotFoundException::class); $this->expectExceptionMessage('Please specify a valid account to share with'); @@ -1891,7 +1892,7 @@ class ShareAPIControllerTest extends TestCase { $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, @@ -2020,7 +2021,9 @@ class ShareAPIControllerTest extends TestCase { $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', Constants::PERMISSION_ALL, IShare::TYPE_LINK); } @@ -2044,7 +2047,7 @@ class ShareAPIControllerTest extends TestCase { $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', Constants::PERMISSION_ALL, IShare::TYPE_LINK, null, 'true'); @@ -2052,24 +2055,26 @@ class ShareAPIControllerTest extends TestCase { public function testCreateShareLinkPublicUploadFile(): void { - $this->expectException(OCSNotFoundException::class); + $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(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); @@ -2093,7 +2098,7 @@ class ShareAPIControllerTest extends TestCase { $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); @@ -2132,23 +2137,23 @@ class ShareAPIControllerTest extends TestCase { $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 (IShare $share) use ($path) { - return $share->getNode() === $path && - $share->getShareType() === IShare::TYPE_LINK && - $share->getPermissions() === Constants::PERMISSION_ALL && - $share->getSharedBy() === 'currentUser' && - $share->getPassword() === 'password' && - $share->getExpirationDate() === null; + 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', 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()); @@ -2171,7 +2176,7 @@ class ShareAPIControllerTest extends TestCase { $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); @@ -2181,7 +2186,7 @@ class ShareAPIControllerTest extends TestCase { $this->callback(function (IShare $share) use ($path) { return $share->getNode() === $path && $share->getShareType() === IShare::TYPE_LINK && - $share->getPermissions() === Constants::PERMISSION_ALL && + $share->getPermissions() === (Constants::PERMISSION_ALL & ~(Constants::PERMISSION_SHARE)) && $share->getSharedBy() === 'currentUser' && $share->getPassword() === 'password' && $share->getSendPasswordByTalk() === true && @@ -2190,7 +2195,7 @@ class ShareAPIControllerTest extends TestCase { )->willReturnArgument(0); $expected = new DataResponse([]); - $result = $ocs->createShare('valid-path', 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()); @@ -2218,7 +2223,7 @@ class ShareAPIControllerTest extends TestCase { $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); @@ -2256,7 +2261,7 @@ class ShareAPIControllerTest extends TestCase { $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); @@ -2302,7 +2307,7 @@ class ShareAPIControllerTest extends TestCase { $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); @@ -2490,11 +2495,7 @@ class ShareAPIControllerTest extends TestCase { )->willReturnCallback( function ($share): void { $share->setSharedWith('recipientRoom'); - $share->setPermissions( - Constants::PERMISSION_ALL & - ~Constants::PERMISSION_DELETE & - ~Constants::PERMISSION_CREATE - ); + $share->setPermissions(Constants::PERMISSION_ALL); } ); @@ -2504,15 +2505,11 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager->method('createShare') ->with($this->callback(function (IShare $share) use ($path) { - return $share->getNode() === $path && - $share->getPermissions() === ( - Constants::PERMISSION_ALL & - ~Constants::PERMISSION_DELETE & - ~Constants::PERMISSION_CREATE - ) && - $share->getShareType() === IShare::TYPE_ROOM && - $share->getSharedWith() === 'recipientRoom' && - $share->getSharedBy() === 'currentUser'; + return $share->getNode() === $path + && $share->getPermissions() === Constants::PERMISSION_ALL + && $share->getShareType() === IShare::TYPE_ROOM + && $share->getSharedWith() === 'recipientRoom' + && $share->getSharedBy() === 'currentUser'; })) ->willReturnArgument(0); @@ -2593,15 +2590,13 @@ class ShareAPIControllerTest extends TestCase { ->willReturn(true); $helper = $this->getMockBuilder('\OCA\Talk\Share\Helper\ShareAPIController') - ->setMethods(['createShare']) + ->addMethods(['createShare']) ->getMock(); $helper->method('createShare') ->with( $share, 'recipientRoom', - Constants::PERMISSION_ALL & - ~Constants::PERMISSION_DELETE & - ~Constants::PERMISSION_CREATE, + Constants::PERMISSION_ALL & ~(Constants::PERMISSION_CREATE | Constants::PERMISSION_DELETE), '' )->willReturnCallback( function ($share): void { @@ -2623,10 +2618,10 @@ class ShareAPIControllerTest extends TestCase { * TODO: Remove once proper solution is in place */ public function testCreateReshareOfFederatedMountNoDeletePermissions(): void { - $share = \OC::$server->getShareManager()->newShare(); + $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, @@ -2825,7 +2820,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -2882,7 +2877,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -2943,7 +2938,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -2974,7 +2969,7 @@ class ShareAPIControllerTest extends TestCase { ->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()); @@ -2995,7 +2990,7 @@ class ShareAPIControllerTest extends TestCase { $this->expectException(OCSBadRequestException::class); $this->expectExceptionMessage('Share must at least have READ or CREATE permissions'); - $this->testUpdateLinkShareSetCRUDPermissions($permissions); + $this->testUpdateLinkShareSetCRUDPermissions($permissions, null); } public function publicLinkInvalidPermissionsProvider2() { @@ -3031,7 +3026,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -3077,7 +3072,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId')->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -3096,25 +3091,31 @@ class ShareAPIControllerTest extends TestCase { $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 = \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', ''); } @@ -3464,7 +3465,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -3525,7 +3526,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -3585,7 +3586,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_LINK) @@ -3601,17 +3602,19 @@ class ShareAPIControllerTest extends TestCase { $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 (IShare $share) use ($date) { - return $share->getPermissions() === (Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | 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) @@ -3630,7 +3633,7 @@ class ShareAPIControllerTest extends TestCase { $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()); @@ -3643,7 +3646,7 @@ class ShareAPIControllerTest extends TestCase { $file->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setPermissions(Constants::PERMISSION_ALL) ->setSharedBy($this->currentUser) ->setShareType(IShare::TYPE_USER) @@ -3689,7 +3692,7 @@ class ShareAPIControllerTest extends TestCase { $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 +3704,7 @@ class ShareAPIControllerTest extends TestCase { // 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) @@ -3761,7 +3764,7 @@ class ShareAPIControllerTest extends TestCase { $folder->method('getId') ->willReturn(42); - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share ->setId(42) ->setSharedBy($this->currentUser) @@ -3773,7 +3776,7 @@ class ShareAPIControllerTest extends TestCase { // 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) @@ -3935,7 +3938,7 @@ class ShareAPIControllerTest extends TestCase { $result = []; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4037,7 +4040,7 @@ class ShareAPIControllerTest extends TestCase { ], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4091,7 +4094,7 @@ class ShareAPIControllerTest extends TestCase { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4147,7 +4150,7 @@ class ShareAPIControllerTest extends TestCase { // with existing group - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_GROUP) ->setSharedWith('recipientGroup') ->setSharedBy('initiator') @@ -4201,7 +4204,7 @@ class ShareAPIControllerTest extends TestCase { ]; // 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') @@ -4252,7 +4255,7 @@ class ShareAPIControllerTest extends TestCase { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_LINK) ->setSharedBy('initiator') ->setShareOwner('owner') @@ -4311,7 +4314,7 @@ class ShareAPIControllerTest extends TestCase { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_LINK) ->setSharedBy('initiator') ->setShareOwner('owner') @@ -4370,7 +4373,7 @@ class ShareAPIControllerTest extends TestCase { ], $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') @@ -4423,7 +4426,7 @@ class ShareAPIControllerTest extends TestCase { ], $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') @@ -4477,7 +4480,7 @@ class ShareAPIControllerTest extends TestCase { ]; // 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]') @@ -4533,7 +4536,7 @@ class ShareAPIControllerTest extends TestCase { ]; // 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]') @@ -4586,7 +4589,7 @@ class ShareAPIControllerTest extends TestCase { ]; // 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)') @@ -4638,7 +4641,7 @@ class ShareAPIControllerTest extends TestCase { ], $share, [], false ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedBy('initiator') ->setSharedWith('recipient') @@ -4653,7 +4656,7 @@ class ShareAPIControllerTest extends TestCase { [], $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') @@ -4708,7 +4711,7 @@ class ShareAPIControllerTest extends TestCase { ], $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') @@ -4765,7 +4768,7 @@ class ShareAPIControllerTest extends TestCase { ]; // Preview is available - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_USER) ->setSharedWith('recipient') ->setSharedBy('initiator') @@ -4931,7 +4934,7 @@ class ShareAPIControllerTest extends TestCase { $result = []; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipientRoom') ->setSharedBy('initiator') @@ -4983,7 +4986,7 @@ class ShareAPIControllerTest extends TestCase { ], $share, false, [] ]; - $share = \OC::$server->getShareManager()->newShare(); + $share = \OCP\Server::get(IManager::class)->newShare(); $share->setShareType(IShare::TYPE_ROOM) ->setSharedWith('recipientRoom') ->setSharedBy('initiator') @@ -5089,6 +5092,9 @@ class ShareAPIControllerTest extends TestCase { $this->assertEquals($expects, $result); } + /** + * @return list{Folder, Folder} + */ private function getNonSharedUserFolder(): array { $node = $this->getMockBuilder(Folder::class)->getMock(); $userFolder = $this->getMockBuilder(Folder::class)->getMock(); @@ -5104,6 +5110,9 @@ class ShareAPIControllerTest extends TestCase { 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 97c2a35ad84..d2f1a2446ae 100644 --- a/build/integration/features/bootstrap/SharingContext.php +++ b/build/integration/features/bootstrap/SharingContext.php @@ -27,6 +27,7 @@ class SharingContext implements Context, SnippetAcceptingContext { $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 1b32f99a0a2..d138f0a1769 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/build/psalm-baseline.xml b/build/psalm-baseline.xml index fe3a17ab896..5cf823f9864 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -873,11 +873,6 @@ <code><![CDATA[Mount]]></code> </MoreSpecificReturnType> </file> - <file src="apps/files_sharing/lib/External/Scanner.php"> - <MoreSpecificImplementedParamType> - <code><![CDATA[$cacheData]]></code> - </MoreSpecificImplementedParamType> - </file> <file src="apps/files_sharing/lib/MountProvider.php"> <RedundantFunctionCall> <code><![CDATA[array_values]]></code> @@ -1758,14 +1753,10 @@ </MoreSpecificReturnType> </file> <file src="lib/private/Files/Cache/Cache.php"> - <InvalidArgument> - <code><![CDATA[$parentData]]></code> - </InvalidArgument> <InvalidNullableReturnType> <code><![CDATA[array]]></code> </InvalidNullableReturnType> <InvalidScalarArgument> - <code><![CDATA[$path]]></code> <code><![CDATA[\OC_Util::normalizeUnicode($path)]]></code> </InvalidScalarArgument> <NullableReturnStatement> @@ -1796,12 +1787,6 @@ <InvalidArgument> <code><![CDATA[self::SCAN_RECURSIVE_INCOMPLETE]]></code> </InvalidArgument> - <InvalidReturnStatement> - <code><![CDATA[$existingChildren]]></code> - </InvalidReturnStatement> - <InvalidReturnType> - <code><![CDATA[array[]]]></code> - </InvalidReturnType> </file> <file src="lib/private/Files/Cache/Storage.php"> <InvalidNullableReturnType> diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index cb841755efd..7fbb625050b 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -109,7 +109,7 @@ class Cache implements ICache { /** * get the stored metadata of a file or folder * - * @param string | int $file either the path of a file or folder or the file id for a file or folder + * @param string|int $file either the path of a file or folder or the file id for a file or folder * @return ICacheEntry|false the cache entry as array or false if the file is not found in the cache */ public function get($file) { @@ -131,15 +131,17 @@ class Cache implements ICache { $data = $result->fetch(); $result->closeCursor(); - //merge partial data - if (!$data && is_string($file) && isset($this->partial[$file])) { - return $this->partial[$file]; - } elseif (!$data) { - return $data; - } else { + if ($data !== false) { $data['metadata'] = $metadataQuery->extractMetadata($data)->asArray(); return self::cacheEntryFromData($data, $this->mimetypeLoader); + } else { + //merge partial data + if (is_string($file) && isset($this->partial[$file])) { + return $this->partial[$file]; + } } + + return false; } /** @@ -886,19 +888,23 @@ class Cache implements ICache { /** * Re-calculate the folder size and the size of all parent folders * - * @param string|boolean $path - * @param array $data (optional) meta data of the folder + * @param array|ICacheEntry|null $data (optional) meta data of the folder */ - public function correctFolderSize($path, $data = null, $isBackgroundScan = false) { + public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void { $this->calculateFolderSize($path, $data); + if ($path !== '') { $parent = dirname($path); if ($parent === '.' || $parent === '/') { $parent = ''; } + if ($isBackgroundScan) { $parentData = $this->get($parent); - if ($parentData['size'] !== -1 && $this->getIncompleteChildrenCount($parentData['fileid']) === 0) { + if ($parentData !== false + && $parentData['size'] !== -1 + && $this->getIncompleteChildrenCount($parentData['fileid']) === 0 + ) { $this->correctFolderSize($parent, $parentData, $isBackgroundScan); } } else { @@ -1009,8 +1015,8 @@ class Cache implements ICache { } // only set unencrypted size for a folder if any child entries have it set, or the folder is empty - $shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || $entry['unencrypted_size'] > 0; - if ($entry['size'] !== $totalSize || ($entry['unencrypted_size'] !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) { + $shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || ($entry['unencrypted_size'] ?? 0) > 0; + if ($entry['size'] !== $totalSize || (($entry['unencrypted_size'] ?? 0) !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) { if ($shouldWriteUnEncryptedSize) { // if all children have an unencrypted size of 0, just set the folder unencrypted size to 0 instead of summing the sizes if ($unencryptedMax === 0) { diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php index 3bd674f79e2..1fb408a0655 100644 --- a/lib/private/Files/Cache/Scanner.php +++ b/lib/private/Files/Cache/Scanner.php @@ -83,7 +83,7 @@ class Scanner extends BasicEmitter implements IScanner { * * @param bool $useTransactions */ - public function setUseTransactions($useTransactions) { + public function setUseTransactions($useTransactions): void { $this->useTransactions = $useTransactions; } @@ -108,9 +108,9 @@ class Scanner extends BasicEmitter implements IScanner { * @param string $file * @param int $reuseExisting * @param int $parentId - * @param array|null|false $cacheData existing data in the cache for the file to be scanned + * @param array|CacheEntry|null|false $cacheData existing data in the cache for the file to be scanned * @param bool $lock set to false to disable getting an additional read lock during scanning - * @param null $data the metadata for the file, as returned by the storage + * @param array|null $data the metadata for the file, as returned by the storage * @return array|null an array of metadata of the scanned file * @throws \OCP\Lock\LockedException */ @@ -122,139 +122,130 @@ class Scanner extends BasicEmitter implements IScanner { return null; } } + // only proceed if $file is not a partial file, blacklist is handled by the storage - if (!self::isPartialFile($file)) { - // acquire a lock + if (self::isPartialFile($file)) { + return null; + } + + // acquire a lock + if ($lock) { + if ($this->storage->instanceOfStorage(ILockingStorage::class)) { + $this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); + } + } + + try { + $data = $data ?? $this->getData($file); + } catch (ForbiddenException $e) { if ($lock) { if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); + $this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); } } - try { - $data = $data ?? $this->getData($file); - } catch (ForbiddenException $e) { - if ($lock) { - if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); - } + return null; + } + + try { + if ($data === null) { + $this->removeFromCache($file); + } else { + // pre-emit only if it was a file. By that we avoid counting/treating folders as files + if ($data['mimetype'] !== 'httpd/unix-directory') { + $this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]); + \OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]); } - return null; - } + $parent = dirname($file); + if ($parent === '.' || $parent === '/') { + $parent = ''; + } + if ($parentId === -1) { + $parentId = $this->cache->getParentId($file); + } - try { - if ($data) { - // pre-emit only if it was a file. By that we avoid counting/treating folders as files - if ($data['mimetype'] !== 'httpd/unix-directory') { - $this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]); - \OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]); + // scan the parent if it's not in the cache (id -1) and the current file is not the root folder + if ($file && $parentId === -1) { + $parentData = $this->scanFile($parent); + if ($parentData === null) { + return null; } - $parent = dirname($file); - if ($parent === '.' || $parent === '/') { - $parent = ''; - } - if ($parentId === -1) { - $parentId = $this->cache->getParentId($file); - } + $parentId = $parentData['fileid']; + } + if ($parent) { + $data['parent'] = $parentId; + } - // scan the parent if it's not in the cache (id -1) and the current file is not the root folder - if ($file && $parentId === -1) { - $parentData = $this->scanFile($parent); - if (!$parentData) { - return null; - } - $parentId = $parentData['fileid']; - } - if ($parent) { - $data['parent'] = $parentId; - } - if (is_null($cacheData)) { - /** @var CacheEntry $cacheData */ - $cacheData = $this->cache->get($file); - } - if ($cacheData && $reuseExisting && isset($cacheData['fileid'])) { - // prevent empty etag - $etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag']; - $fileId = $cacheData['fileid']; - $data['fileid'] = $fileId; - // only reuse data if the file hasn't explicitly changed - $mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime']; - // if the folder is marked as unscanned, never reuse etags - if ($mtimeUnchanged && $cacheData['size'] !== -1) { - $data['mtime'] = $cacheData['mtime']; - if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) { - $data['size'] = $cacheData['size']; - } - if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) { - $data['etag'] = $etag; - } + $cacheData = $cacheData ?? $this->cache->get($file); + if ($reuseExisting && $cacheData !== false && isset($cacheData['fileid'])) { + // prevent empty etag + $etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag']; + $fileId = $cacheData['fileid']; + $data['fileid'] = $fileId; + // only reuse data if the file hasn't explicitly changed + $mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime']; + // if the folder is marked as unscanned, never reuse etags + if ($mtimeUnchanged && $cacheData['size'] !== -1) { + $data['mtime'] = $cacheData['mtime']; + if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) { + $data['size'] = $cacheData['size']; } - - // we only updated unencrypted_size if it's already set - if ($cacheData['unencrypted_size'] === 0) { - unset($data['unencrypted_size']); + if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) { + $data['etag'] = $etag; } - - // Only update metadata that has changed - // i.e. get all the values in $data that are not present in the cache already - $newData = $this->array_diff_assoc_multi($data, $cacheData->getData()); - - // make it known to the caller that etag has been changed and needs propagation - if (isset($newData['etag'])) { - $data['etag_changed'] = true; - } - } else { - // we only updated unencrypted_size if it's already set - unset($data['unencrypted_size']); - $newData = $data; - $fileId = -1; } - if (!empty($newData)) { - // Reset the checksum if the data has changed - $newData['checksum'] = ''; - $newData['parent'] = $parentId; - $data['fileid'] = $this->addToCache($file, $newData, $fileId); - } - - $data['oldSize'] = ($cacheData && isset($cacheData['size'])) ? $cacheData['size'] : 0; - if ($cacheData && isset($cacheData['encrypted'])) { - $data['encrypted'] = $cacheData['encrypted']; + // we only updated unencrypted_size if it's already set + if (isset($cacheData['unencrypted_size']) && $cacheData['unencrypted_size'] === 0) { + unset($data['unencrypted_size']); } - // post-emit only if it was a file. By that we avoid counting/treating folders as files - if ($data['mimetype'] !== 'httpd/unix-directory') { - $this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]); - \OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]); + /** + * Only update metadata that has changed. + * i.e. get all the values in $data that are not present in the cache already + * + * We need the OC implementation for usage of "getData" method below. + * @var \OC\Files\Cache\CacheEntry $cacheData + */ + $newData = $this->array_diff_assoc_multi($data, $cacheData->getData()); + + // make it known to the caller that etag has been changed and needs propagation + if (isset($newData['etag'])) { + $data['etag_changed'] = true; } } else { - $this->removeFromCache($file); + unset($data['unencrypted_size']); + $newData = $data; + $fileId = -1; } - } catch (\Exception $e) { - if ($lock) { - if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); - } + if (!empty($newData)) { + // Reset the checksum if the data has changed + $newData['checksum'] = ''; + $newData['parent'] = $parentId; + $data['fileid'] = $this->addToCache($file, $newData, $fileId); } - throw $e; - } - // release the acquired lock - if ($lock) { - if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); + if ($cacheData !== false) { + $data['oldSize'] = $cacheData['size'] ?? 0; + $data['encrypted'] = $cacheData['encrypted'] ?? false; } - } - if ($data && !isset($data['encrypted'])) { - $data['encrypted'] = false; + // post-emit only if it was a file. By that we avoid counting/treating folders as files + if ($data['mimetype'] !== 'httpd/unix-directory') { + $this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]); + \OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]); + } + } + } finally { + // release the acquired lock + if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) { + $this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider); } - return $data; } - return null; + return $data; } protected function removeFromCache($path) { @@ -319,29 +310,26 @@ class Scanner extends BasicEmitter implements IScanner { if ($reuse === -1) { $reuse = ($recursive === self::SCAN_SHALLOW) ? self::REUSE_ETAG | self::REUSE_SIZE : self::REUSE_ETAG; } - if ($lock) { - if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider); - $this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider); - } + + if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) { + $this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider); + $this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider); } + try { - try { - $data = $this->scanFile($path, $reuse, -1, null, $lock); - if ($data && $data['mimetype'] === 'httpd/unix-directory') { - $size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']); - $data['size'] = $size; - } - } catch (NotFoundException $e) { - $this->removeFromCache($path); - return null; + $data = $this->scanFile($path, $reuse, -1, lock: $lock); + + if ($data !== null && $data['mimetype'] === 'httpd/unix-directory') { + $size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']); + $data['size'] = $size; } + } catch (NotFoundException $e) { + $this->removeFromCache($path); + return null; } finally { - if ($lock) { - if ($this->storage->instanceOfStorage(ILockingStorage::class)) { - $this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider); - $this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider); - } + if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) { + $this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider); + $this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider); } } return $data; @@ -395,9 +383,9 @@ class Scanner extends BasicEmitter implements IScanner { * Get the children currently in the cache * * @param int $folderId - * @return array[] + * @return array<string, \OCP\Files\Cache\ICacheEntry> */ - protected function getExistingChildren($folderId) { + protected function getExistingChildren($folderId): array { $existingChildren = []; $children = $this->cache->getFolderContentsById($folderId); foreach ($children as $child) { diff --git a/lib/private/Files/Cache/Wrapper/CacheJail.php b/lib/private/Files/Cache/Wrapper/CacheJail.php index ea0f992114a..5c7bd4334f3 100644 --- a/lib/private/Files/Cache/Wrapper/CacheJail.php +++ b/lib/private/Files/Cache/Wrapper/CacheJail.php @@ -21,19 +21,15 @@ use OCP\Files\Search\ISearchOperator; * Jail to a subdirectory of the wrapped cache */ class CacheJail extends CacheWrapper { - /** - * @var string - */ - protected $root; - protected $unjailedRoot; + + protected string $unjailedRoot; public function __construct( ?ICache $cache, - string $root, + protected string $root, ?CacheDependencies $dependencies = null, ) { parent::__construct($cache, $dependencies); - $this->root = $root; if ($cache instanceof CacheJail) { $this->unjailedRoot = $cache->getSourcePath($root); @@ -42,6 +38,9 @@ class CacheJail extends CacheWrapper { } } + /** + * @return string + */ protected function getRoot() { return $this->root; } @@ -55,7 +54,10 @@ class CacheJail extends CacheWrapper { return $this->unjailedRoot; } - protected function getSourcePath($path) { + /** + * @return string + */ + protected function getSourcePath(string $path) { if ($path === '') { return $this->getRoot(); } else { @@ -95,7 +97,7 @@ class CacheJail extends CacheWrapper { /** * get the stored metadata of a file or folder * - * @param string /int $file + * @param string|int $file * @return ICacheEntry|false */ public function get($file) { @@ -206,12 +208,12 @@ class CacheJail extends CacheWrapper { /** * update the folder size and the size of all parent folders * - * @param string|boolean $path - * @param array $data (optional) meta data of the folder + * @param array|ICacheEntry|null $data (optional) meta data of the folder */ - public function correctFolderSize($path, $data = null, $isBackgroundScan = false) { - if ($this->getCache() instanceof Cache) { - $this->getCache()->correctFolderSize($this->getSourcePath($path), $data, $isBackgroundScan); + public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void { + $cache = $this->getCache(); + if ($cache instanceof Cache) { + $cache->correctFolderSize($this->getSourcePath($path), $data, $isBackgroundScan); } } @@ -223,8 +225,9 @@ class CacheJail extends CacheWrapper { * @return int|float */ public function calculateFolderSize($path, $entry = null) { - if ($this->getCache() instanceof Cache) { - return $this->getCache()->calculateFolderSize($this->getSourcePath($path), $entry); + $cache = $this->getCache(); + if ($cache instanceof Cache) { + return $cache->calculateFolderSize($this->getSourcePath($path), $entry); } else { return 0; } diff --git a/lib/private/Files/Cache/Wrapper/CacheWrapper.php b/lib/private/Files/Cache/Wrapper/CacheWrapper.php index fdaa2cf4b7a..f2f1036d6a3 100644 --- a/lib/private/Files/Cache/Wrapper/CacheWrapper.php +++ b/lib/private/Files/Cache/Wrapper/CacheWrapper.php @@ -221,12 +221,12 @@ class CacheWrapper extends Cache { /** * update the folder size and the size of all parent folders * - * @param string|boolean $path - * @param array $data (optional) meta data of the folder + * @param array|ICacheEntry|null $data (optional) meta data of the folder */ - public function correctFolderSize($path, $data = null, $isBackgroundScan = false) { - if ($this->getCache() instanceof Cache) { - $this->getCache()->correctFolderSize($path, $data, $isBackgroundScan); + public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void { + $cache = $this->getCache(); + if ($cache instanceof Cache) { + $cache->correctFolderSize($path, $data, $isBackgroundScan); } } @@ -238,8 +238,9 @@ class CacheWrapper extends Cache { * @return int|float */ public function calculateFolderSize($path, $entry = null) { - if ($this->getCache() instanceof Cache) { - return $this->getCache()->calculateFolderSize($path, $entry); + $cache = $this->getCache(); + if ($cache instanceof Cache) { + return $cache->calculateFolderSize($path, $entry); } else { return 0; } diff --git a/lib/private/Files/ObjectStore/ObjectStoreScanner.php b/lib/private/Files/ObjectStore/ObjectStoreScanner.php index d8a77d36dee..05929a49aab 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreScanner.php +++ b/lib/private/Files/ObjectStore/ObjectStoreScanner.php @@ -13,11 +13,11 @@ use OCP\Files\FileInfo; class ObjectStoreScanner extends Scanner { public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = null, $lock = true, $data = null) { - return []; + return null; } public function scan($path, $recursive = self::SCAN_RECURSIVE, $reuse = -1, $lock = true) { - return []; + return null; } protected function scanChildren(string $path, $recursive, int $reuse, int $folderId, bool $lock, int|float $oldSize, &$etagChanged = false) { diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php index 072d3520ae9..5e09ce69fb2 100644 --- a/lib/private/Files/View.php +++ b/lib/private/Files/View.php @@ -610,13 +610,13 @@ class View { $this->lockFile($path, ILockingProvider::LOCK_SHARED); $exists = $this->file_exists($path); - $run = true; if ($this->shouldEmitHooks($path)) { + $run = true; $this->emit_file_hooks_pre($exists, $path, $run); - } - if (!$run) { - $this->unlockFile($path, ILockingProvider::LOCK_SHARED); - return false; + if (!$run) { + $this->unlockFile($path, ILockingProvider::LOCK_SHARED); + return false; + } } try { diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 35166c37ee3..a84481730c4 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -217,6 +217,17 @@ class Manager implements IManager { throw new \InvalidArgumentException($this->l->t('Valid permissions are required for sharing')); } + // 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/Files/ObjectStore/ObjectStoreScannerTest.php b/tests/lib/Files/ObjectStore/ObjectStoreScannerTest.php index 4f6254cdb94..11c913cd232 100644 --- a/tests/lib/Files/ObjectStore/ObjectStoreScannerTest.php +++ b/tests/lib/Files/ObjectStore/ObjectStoreScannerTest.php @@ -35,9 +35,7 @@ class ObjectStoreScannerTest extends TestCase { $data = "dummy file data\n"; $this->storage->file_put_contents('foo.txt', $data); - $this->assertEquals( - [], - $this->scanner->scanFile('foo.txt'), + $this->assertNull($this->scanner->scanFile('foo.txt'), 'Asserting that no error occurred while scanFile()' ); } @@ -54,8 +52,7 @@ class ObjectStoreScannerTest extends TestCase { public function testFolder(): void { $this->fillTestFolders(); - $this->assertEquals( - [], + $this->assertNull( $this->scanner->scan(''), 'Asserting that no error occurred while scan()' ); diff --git a/tests/lib/Files/Storage/Wrapper/EncodingTest.php b/tests/lib/Files/Storage/Wrapper/EncodingTest.php index f52e3689155..d8b03a891c2 100644 --- a/tests/lib/Files/Storage/Wrapper/EncodingTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncodingTest.php @@ -240,4 +240,12 @@ class EncodingTest extends \Test\Files\Storage\Storage { $entry = $this->instance->getMetaData('/test/' . self::NFD_NAME); $this->assertEquals(self::NFC_NAME, $entry['name']); } + + /** + * Regression test of https://github.com/nextcloud/server/issues/50431 + */ + public function testNoMetadata() { + $this->assertNull($this->instance->getMetaData('/test/null')); + } + } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 6df97423989..8b8d6ea4e65 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -893,10 +893,9 @@ class ManagerTest extends \Test\TestCase { $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(IStorage::class); $nonMovableStorage->method('instanceOfStorage') @@ -927,6 +926,20 @@ class ManagerTest extends \Test\TestCase { $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); @@ -939,6 +952,12 @@ class ManagerTest extends \Test\TestCase { $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]; @@ -2406,8 +2425,9 @@ class ManagerTest extends \Test\TestCase { } 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); |