From 3bdb3442242aba93d0fae6181861baeb60b1514d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 29 Oct 2025 16:50:50 +0100 Subject: [PATCH] perf(external-sharing): Port to Entity and SnowflakeId This removes all the read after write and we don't need to queries all the time the same share in the same request anymore. Signed-off-by: Carl Schwan --- .../Controller/RequestHandlerController.php | 3 +- .../lib/FederatedShareProvider.php | 7 +- .../lib/OCM/CloudFederationProviderFiles.php | 50 +- .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + .../Controller/ExternalSharesController.php | 27 +- .../lib/Controller/RemoteController.php | 112 ++-- .../lib/Controller/ShareInfoController.php | 2 +- .../lib/External/ExternalShare.php | 139 ++++ .../lib/External/ExternalShareMapper.php | 204 ++++++ apps/files_sharing/lib/External/Manager.php | 633 ++++++------------ apps/files_sharing/lib/External/Mount.php | 5 +- .../files_sharing/lib/ResponseDefinitions.php | 4 +- apps/files_sharing/openapi.json | 22 +- .../Command/CleanupRemoteStoragesTest.php | 31 +- .../ExternalShareControllerTest.php | 37 +- .../tests/External/CacheTest.php | 28 +- .../tests/External/ManagerTest.php | 403 +++++------ .../features/bootstrap/Sharing.php | 12 + build/psalm-baseline.xml | 38 -- lib/private/Activity/Event.php | 9 +- lib/private/Share20/Manager.php | 19 +- lib/public/Activity/IEvent.php | 8 +- openapi.json | 22 +- 24 files changed, 941 insertions(+), 878 deletions(-) create mode 100644 apps/files_sharing/lib/External/ExternalShare.php create mode 100644 apps/files_sharing/lib/External/ExternalShareMapper.php diff --git a/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php b/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php index e15d57181a3..eb8718e83da 100644 --- a/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php +++ b/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php @@ -500,7 +500,6 @@ class RequestHandlerController extends Controller { * * @param IIncomingSignedRequest|null $signedRequest * @param string $resourceType - * @param string $sharedSecret * * @throws IncomingRequestException * @throws BadRequestException @@ -524,7 +523,7 @@ class RequestHandlerController extends Controller { return; } } catch (\Exception $e) { - throw new IncomingRequestException($e->getMessage()); + throw new IncomingRequestException($e->getMessage(), previous: $e); } $this->confirmNotificationEntry($signedRequest, $identity); diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 47ed3e893e9..e2708ddd4c7 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -752,10 +752,9 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl * Get a share by token * * @param string $token - * @return IShare * @throws ShareNotFound */ - public function getShareByToken($token) { + public function getShareByToken($token): IShare { $qb = $this->dbConnection->getQueryBuilder(); $cursor = $qb->select('*') @@ -812,9 +811,9 @@ class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAl * @throws InvalidShare * @throws ShareNotFound */ - private function createShareObject($data) { + private function createShareObject($data): IShare { $share = new Share($this->rootFolder, $this->userManager); - $share->setId((int)$data['id']) + $share->setId((string)$data['id']) ->setShareType((int)$data['share_type']) ->setPermissions((int)$data['permissions']) ->setTarget($data['file_target']) diff --git a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php index 9555a13fa20..6fb8a60a1e4 100644 --- a/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php +++ b/apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php @@ -14,8 +14,10 @@ use OCA\FederatedFileSharing\AddressHandler; use OCA\FederatedFileSharing\FederatedShareProvider; use OCA\Federation\TrustedServers; use OCA\Files_Sharing\Activity\Providers\RemoteShares; +use OCA\Files_Sharing\External\ExternalShare; use OCA\Files_Sharing\External\Manager; use OCA\GlobalSiteSelector\Service\SlaveService; +use OCA\Polls\Db\Share; use OCP\Activity\IManager as IActivityManager; use OCP\App\IAppManager; use OCP\AppFramework\QueryException; @@ -44,6 +46,7 @@ use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IProviderFactory; use OCP\Share\IShare; +use OCP\Snowflake\IGenerator; use OCP\Util; use Psr\Log\LoggerInterface; use SensitiveParameter; @@ -72,13 +75,11 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { private IFilenameValidator $filenameValidator, private readonly IProviderFactory $shareProviderFactory, private readonly SetupManager $setupManager, + private readonly IGenerator $snowflakeGenerator, ) { } - /** - * @return string - */ - public function getShareType() { + public function getShareType(): string { return 'file'; } @@ -93,7 +94,7 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { * @throws HintException * @since 14.0.0 */ - public function shareReceived(ICloudFederationShare $share) { + public function shareReceived(ICloudFederationShare $share): string { if (!$this->isS2SEnabled(true)) { throw new ProviderCouldNotAddShareException('Server does not support federated cloud sharing', '', Http::STATUS_SERVICE_UNAVAILABLE); } @@ -103,7 +104,8 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { throw new ProviderCouldNotAddShareException('Unsupported protocol for data exchange.', '', Http::STATUS_NOT_IMPLEMENTED); } - [$ownerUid, $remote] = $this->addressHandler->splitUserRemote($share->getOwner()); + [, $remote] = $this->addressHandler->splitUserRemote($share->getOwner()); + // for backward compatibility make sure that the remote url stored in the // database ends with a trailing slash if (!str_ends_with($remote, '/')) { @@ -113,17 +115,15 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { $token = $share->getShareSecret(); $name = $share->getResourceName(); $owner = $share->getOwnerDisplayName() ?: $share->getOwner(); - $sharedBy = $share->getSharedByDisplayName(); $shareWith = $share->getShareWith(); $remoteId = $share->getProviderId(); $sharedByFederatedId = $share->getSharedBy(); $ownerFederatedId = $share->getOwner(); $shareType = $this->mapShareTypeToNextcloud($share->getShareType()); - // if no explicit information about the person who created the share was send + // if no explicit information about the person who created the share was sent // we assume that the share comes from the owner if ($sharedByFederatedId === null) { - $sharedBy = $owner; $sharedByFederatedId = $ownerFederatedId; } @@ -159,9 +159,19 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { } } + $externalShare = new ExternalShare(); + $externalShare->setId($this->snowflakeGenerator->nextId()); + $externalShare->setRemote($remote); + $externalShare->setRemoteId($remoteId); + $externalShare->setShareToken($token); + $externalShare->setPassword(''); + $externalShare->setName($name); + $externalShare->setOwner($owner); + $externalShare->setShareType($shareType); + $externalShare->setAccepted(IShare::STATUS_PENDING); + try { - $this->externalShareManager->addShare($remote, $token, '', $name, $owner, $shareType, false, $userOrGroup, $remoteId); - $shareId = Server::get(IDBConnection::class)->lastInsertId('*PREFIX*share_external'); + $this->externalShareManager->addShare($externalShare, $userOrGroup); // get DisplayName about the owner of the share $ownerDisplayName = $this->getUserDisplayName($ownerFederatedId); @@ -184,14 +194,14 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { ->setType('remote_share') ->setSubject(RemoteShares::SUBJECT_REMOTE_SHARE_RECEIVED, [$ownerFederatedId, trim($name, '/'), $ownerDisplayName]) ->setAffectedUser($shareWith) - ->setObject('remote_share', $shareId, $name); + ->setObject('remote_share', $externalShare->getId(), $name); Server::get(IActivityManager::class)->publish($event); - $this->notifyAboutNewShare($shareWith, $shareId, $ownerFederatedId, $sharedByFederatedId, $name, $ownerDisplayName); + $this->notifyAboutNewShare($shareWith, $externalShare->getId(), $ownerFederatedId, $sharedByFederatedId, $name, $ownerDisplayName); // If auto-accept is enabled, accept the share if ($this->federatedShareProvider->isFederatedTrustedShareAutoAccept() && $trustedServers?->isTrustedServer($remote) === true) { /** @var IUser $userOrGroup */ - $this->externalShareManager->acceptShare($shareId, $userOrGroup); + $this->externalShareManager->acceptShare($externalShare, $userOrGroup); } } else { /** @var IGroup $userOrGroup */ @@ -202,18 +212,18 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { ->setType('remote_share') ->setSubject(RemoteShares::SUBJECT_REMOTE_SHARE_RECEIVED, [$ownerFederatedId, trim($name, '/'), $ownerDisplayName]) ->setAffectedUser($user->getUID()) - ->setObject('remote_share', $shareId, $name); + ->setObject('remote_share', $externalShare->getId(), $name); Server::get(IActivityManager::class)->publish($event); - $this->notifyAboutNewShare($user->getUID(), $shareId, $ownerFederatedId, $sharedByFederatedId, $name, $ownerDisplayName); + $this->notifyAboutNewShare($user->getUID(), $externalShare->getId(), $ownerFederatedId, $sharedByFederatedId, $name, $ownerDisplayName); // If auto-accept is enabled, accept the share if ($this->federatedShareProvider->isFederatedTrustedShareAutoAccept() && $trustedServers?->isTrustedServer($remote) === true) { - $this->externalShareManager->acceptShare($shareId, $user); + $this->externalShareManager->acceptShare($externalShare, $user); } } } - return $shareId; + return $externalShare->getId(); } catch (\Exception $e) { $this->logger->error('Server can not add remote share.', [ 'app' => 'files_sharing', @@ -275,7 +285,7 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { return $result; } - private function notifyAboutNewShare($shareWith, $shareId, $ownerFederatedId, $sharedByFederatedId, $name, $displayName): void { + private function notifyAboutNewShare($shareWith, string $shareId, $ownerFederatedId, $sharedByFederatedId, string $name, string $displayName): void { $notification = $this->notificationManager->createNotification(); $notification->setApp('files_sharing') ->setUser($shareWith) @@ -813,7 +823,7 @@ class CloudFederationProviderFiles implements ISignedCloudFederationProvider { return ''; } - return $share['user'] . '@' . $share['remote']; + return $share->getUser() . '@' . $share->getRemote(); } // if uid_owner is a local account, the request comes from the recipient diff --git a/apps/files_sharing/composer/composer/autoload_classmap.php b/apps/files_sharing/composer/composer/autoload_classmap.php index dda5b5c7f77..61100bd8747 100644 --- a/apps/files_sharing/composer/composer/autoload_classmap.php +++ b/apps/files_sharing/composer/composer/autoload_classmap.php @@ -50,6 +50,8 @@ return array( 'OCA\\Files_Sharing\\Exceptions\\SharingRightsException' => $baseDir . '/../lib/Exceptions/SharingRightsException.php', 'OCA\\Files_Sharing\\ExpireSharesJob' => $baseDir . '/../lib/ExpireSharesJob.php', 'OCA\\Files_Sharing\\External\\Cache' => $baseDir . '/../lib/External/Cache.php', + 'OCA\\Files_Sharing\\External\\ExternalShare' => $baseDir . '/../lib/External/ExternalShare.php', + 'OCA\\Files_Sharing\\External\\ExternalShareMapper' => $baseDir . '/../lib/External/ExternalShareMapper.php', 'OCA\\Files_Sharing\\External\\Manager' => $baseDir . '/../lib/External/Manager.php', 'OCA\\Files_Sharing\\External\\Mount' => $baseDir . '/../lib/External/Mount.php', 'OCA\\Files_Sharing\\External\\MountProvider' => $baseDir . '/../lib/External/MountProvider.php', diff --git a/apps/files_sharing/composer/composer/autoload_static.php b/apps/files_sharing/composer/composer/autoload_static.php index 4f65cfe1105..617a674dad4 100644 --- a/apps/files_sharing/composer/composer/autoload_static.php +++ b/apps/files_sharing/composer/composer/autoload_static.php @@ -65,6 +65,8 @@ class ComposerStaticInitFiles_Sharing 'OCA\\Files_Sharing\\Exceptions\\SharingRightsException' => __DIR__ . '/..' . '/../lib/Exceptions/SharingRightsException.php', 'OCA\\Files_Sharing\\ExpireSharesJob' => __DIR__ . '/..' . '/../lib/ExpireSharesJob.php', 'OCA\\Files_Sharing\\External\\Cache' => __DIR__ . '/..' . '/../lib/External/Cache.php', + 'OCA\\Files_Sharing\\External\\ExternalShare' => __DIR__ . '/..' . '/../lib/External/ExternalShare.php', + 'OCA\\Files_Sharing\\External\\ExternalShareMapper' => __DIR__ . '/..' . '/../lib/External/ExternalShareMapper.php', 'OCA\\Files_Sharing\\External\\Manager' => __DIR__ . '/..' . '/../lib/External/Manager.php', 'OCA\\Files_Sharing\\External\\Mount' => __DIR__ . '/..' . '/../lib/External/Mount.php', 'OCA\\Files_Sharing\\External\\MountProvider' => __DIR__ . '/..' . '/../lib/External/MountProvider.php', diff --git a/apps/files_sharing/lib/Controller/ExternalSharesController.php b/apps/files_sharing/lib/Controller/ExternalSharesController.php index fa828a9d2c2..7f266ea53a3 100644 --- a/apps/files_sharing/lib/Controller/ExternalSharesController.php +++ b/apps/files_sharing/lib/Controller/ExternalSharesController.php @@ -7,6 +7,7 @@ */ namespace OCA\Files_Sharing\Controller; +use OCA\Files_Sharing\External\Manager; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\JSONResponse; @@ -21,42 +22,40 @@ class ExternalSharesController extends Controller { public function __construct( string $appName, IRequest $request, - private \OCA\Files_Sharing\External\Manager $externalManager, + private readonly Manager $externalManager, ) { parent::__construct($appName, $request); } /** * @NoOutgoingFederatedSharingRequired - * - * @return JSONResponse */ #[NoAdminRequired] - public function index() { + public function index(): JSONResponse { return new JSONResponse($this->externalManager->getOpenShares()); } /** * @NoOutgoingFederatedSharingRequired - * - * @param int $id - * @return JSONResponse */ #[NoAdminRequired] - public function create($id) { - $this->externalManager->acceptShare($id); + public function create(string $id): JSONResponse { + $externalShare = $this->externalManager->getShare($id); + if ($externalShare !== false) { + $this->externalManager->acceptShare($externalShare); + } return new JSONResponse(); } /** * @NoOutgoingFederatedSharingRequired - * - * @param integer $id - * @return JSONResponse */ #[NoAdminRequired] - public function destroy($id) { - $this->externalManager->declineShare($id); + public function destroy(string $id): JSONResponse { + $externalShare = $this->externalManager->getShare($id); + if ($externalShare !== false) { + $this->externalManager->declineShare($externalShare); + } return new JSONResponse(); } } diff --git a/apps/files_sharing/lib/Controller/RemoteController.php b/apps/files_sharing/lib/Controller/RemoteController.php index 8c15cd8463e..baff94a9db4 100644 --- a/apps/files_sharing/lib/Controller/RemoteController.php +++ b/apps/files_sharing/lib/Controller/RemoteController.php @@ -7,7 +7,7 @@ */ namespace OCA\Files_Sharing\Controller; -use OC\Files\View; +use OCA\Files_Sharing\External\ExternalShare; use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\ResponseDefinitions; use OCP\AppFramework\Http; @@ -16,25 +16,27 @@ use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; +use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; +use OCP\Files\NotPermittedException; use OCP\IRequest; use Psr\Log\LoggerInterface; /** * @psalm-import-type Files_SharingRemoteShare from ResponseDefinitions + * @api */ class RemoteController extends OCSController { /** - * Remote constructor. - * - * @param string $appName - * @param IRequest $request - * @param Manager $externalManager + * Remote controller constructor. */ public function __construct( - $appName, + string $appName, IRequest $request, - private Manager $externalManager, - private LoggerInterface $logger, + private readonly Manager $externalManager, + private readonly LoggerInterface $logger, + private readonly ?string $userId, + private readonly IRootFolder $rootFolder, ) { parent::__construct($appName, $request); } @@ -47,71 +49,84 @@ class RemoteController extends OCSController { * 200: Pending remote shares returned */ #[NoAdminRequired] - public function getOpenShares() { - return new DataResponse($this->externalManager->getOpenShares()); + public function getOpenShares(): DataResponse { + $shares = $this->externalManager->getOpenShares(); + $shares = array_map($this->extendShareInfo(...), $shares); + return new DataResponse($shares); } /** - * Accept a remote share + * Accept a remote share. * - * @param int $id ID of the share + * @param string $id ID of the share * @return DataResponse, array{}> * @throws OCSNotFoundException Share not found * * 200: Share accepted successfully */ #[NoAdminRequired] - public function acceptShare($id) { - if ($this->externalManager->acceptShare($id)) { - return new DataResponse(); + public function acceptShare(string $id): DataResponse { + $externalShare = $this->externalManager->getShare($id); + if ($externalShare === false) { + $this->logger->error('Could not accept federated share with id: ' . $id . ' Share not found.', ['app' => 'files_sharing']); + throw new OCSNotFoundException('Wrong share ID, share does not exist.'); } - $this->logger->error('Could not accept federated share with id: ' . $id, - ['app' => 'files_sharing']); + if (!$this->externalManager->acceptShare($externalShare)) { + $this->logger->error('Could not accept federated share with id: ' . $id, ['app' => 'files_sharing']); + throw new OCSNotFoundException('Wrong share ID, share does not exist.'); + } - throw new OCSNotFoundException('wrong share ID, share does not exist.'); + return new DataResponse(); } /** - * Decline a remote share + * Decline a remote share. * - * @param int $id ID of the share + * @param string $id ID of the share * @return DataResponse, array{}> * @throws OCSNotFoundException Share not found * * 200: Share declined successfully */ #[NoAdminRequired] - public function declineShare($id) { - if ($this->externalManager->declineShare($id)) { - return new DataResponse(); + public function declineShare(string $id): DataResponse { + $externalShare = $this->externalManager->getShare($id); + if ($externalShare === false) { + $this->logger->error('Could not decline federated share with id: ' . $id . ' Share not found.', ['app' => 'files_sharing']); + throw new OCSNotFoundException('Wrong share ID, share does not exist.'); } - // Make sure the user has no notification for something that does not exist anymore. - $this->externalManager->processNotification($id); + if (!$this->externalManager->declineShare($externalShare)) { + $this->logger->error('Could not decline federated share with id: ' . $id, ['app' => 'files_sharing']); + throw new OCSNotFoundException('Wrong share ID, share does not exist.'); + } - throw new OCSNotFoundException('wrong share ID, share does not exist.'); + return new DataResponse(); } /** - * @param array $share Share with info from the share_external table - * @return array enriched share info with data from the filecache + * @param ExternalShare $share Share with info from the share_external table + * @return Files_SharingRemoteShare Enriched share info with data from the filecache */ - private static function extendShareInfo($share) { - $view = new View('/' . \OC_User::getUser() . '/files/'); - $info = $view->getFileInfo($share['mountpoint']); + private function extendShareInfo(ExternalShare $share): array { + $userFolder = $this->rootFolder->getUserFolder($this->userId); - if ($info === false) { - return $share; + try { + $mountPointNode = $userFolder->get($share->getMountpoint()); + } catch (NotPermittedException|NotFoundException) { + return $share->jsonSerialize(); } - $share['mimetype'] = $info->getMimetype(); - $share['mtime'] = $info->getMTime(); - $share['permissions'] = $info->getPermissions(); - $share['type'] = $info->getType(); - $share['file_id'] = $info->getId(); + $shareData = $share->jsonSerialize(); - return $share; + $shareData['mimetype'] = $mountPointNode->getMimetype(); + $shareData['mtime'] = $mountPointNode->getMTime(); + $shareData['permissions'] = $mountPointNode->getPermissions(); + $shareData['type'] = $mountPointNode->getType(); + $shareData['file_id'] = $mountPointNode->getId(); + + return $shareData; } /** @@ -122,30 +137,29 @@ class RemoteController extends OCSController { * 200: Accepted remote shares returned */ #[NoAdminRequired] - public function getShares() { + public function getShares(): DataResponse { $shares = $this->externalManager->getAcceptedShares(); - $shares = array_map(self::extendShareInfo(...), $shares); - + $shares = array_map(fn (ExternalShare $share) => $this->extendShareInfo($share), $shares); return new DataResponse($shares); } /** * Get info of a remote share * - * @param int $id ID of the share + * @param string $id ID of the share * @return DataResponse * @throws OCSNotFoundException Share not found * * 200: Share returned */ #[NoAdminRequired] - public function getShare($id) { + public function getShare(string $id): DataResponse { $shareInfo = $this->externalManager->getShare($id); if ($shareInfo === false) { throw new OCSNotFoundException('share does not exist'); } else { - $shareInfo = self::extendShareInfo($shareInfo); + $shareInfo = $this->extendShareInfo($shareInfo); return new DataResponse($shareInfo); } } @@ -153,7 +167,7 @@ class RemoteController extends OCSController { /** * Unshare a remote share * - * @param int $id ID of the share + * @param string $id ID of the share * @return DataResponse, array{}> * @throws OCSNotFoundException Share not found * @throws OCSForbiddenException Unsharing is not possible @@ -161,14 +175,14 @@ class RemoteController extends OCSController { * 200: Share unshared successfully */ #[NoAdminRequired] - public function unshare($id) { + public function unshare(string $id): DataResponse { $shareInfo = $this->externalManager->getShare($id); if ($shareInfo === false) { throw new OCSNotFoundException('Share does not exist'); } - $mountPoint = '/' . \OC_User::getUser() . '/files' . $shareInfo['mountpoint']; + $mountPoint = '/' . $this->userId . '/files' . $shareInfo->getMountPoint(); if ($this->externalManager->removeShare($mountPoint) === true) { return new DataResponse(); diff --git a/apps/files_sharing/lib/Controller/ShareInfoController.php b/apps/files_sharing/lib/Controller/ShareInfoController.php index b7e79aec830..27cef7bff52 100644 --- a/apps/files_sharing/lib/Controller/ShareInfoController.php +++ b/apps/files_sharing/lib/Controller/ShareInfoController.php @@ -64,7 +64,7 @@ class ShareInfoController extends ApiController { try { $share = $this->shareManager->getShareByToken($t); } catch (ShareNotFound $e) { - $response = new JSONResponse([], Http::STATUS_NOT_FOUND); + $response = new JSONResponse(["message" => "Not found " . $t . " "], Http::STATUS_NOT_FOUND); $response->throttle(['token' => $t]); return $response; } diff --git a/apps/files_sharing/lib/External/ExternalShare.php b/apps/files_sharing/lib/External/ExternalShare.php new file mode 100644 index 00000000000..94dbc0abbf7 --- /dev/null +++ b/apps/files_sharing/lib/External/ExternalShare.php @@ -0,0 +1,139 @@ +addType('id', Types::STRING); // Stored as a bigint + $this->addType('parent', Types::STRING); // Stored as a bigint + $this->addType('shareType', Types::INTEGER); + $this->addType('remote', Types::STRING); + $this->addType('remoteId', Types::STRING); + $this->addType('shareToken', Types::STRING); + $this->addType('password', Types::STRING); + $this->addType('name', Types::STRING); + $this->addType('owner', Types::STRING); + $this->addType('user', Types::STRING); + $this->addType('mountpoint', Types::STRING); + $this->addType('mountpointHash', Types::STRING); + $this->addType('accepted', Types::INTEGER); + } + + public function setMountpoint(string $mountPoint): void { + $this->setter('mountpoint', [$mountPoint]); + $this->setMountpointHash(md5($mountPoint)); + } + + public function setName(string $name): void { + $name = Filesystem::normalizePath('/' . $name); + $this->setter('name', [$name]); + } + + public function setUserOrGroup(IUser|IGroup|null $userOrGroup): void { + $this->setUser($userOrGroup instanceof IGroup ? $userOrGroup->getGID() : $userOrGroup->getUID()); + } + + /** + * @return Files_SharingRemoteShare + */ + public function jsonSerialize(): array { + $parent = $this->getParent(); + return [ + 'id' => $this->getId(), + 'parent' => $parent === '-1' ? null : $parent, + 'share_type' => $this->getShareType() ?? IShare::TYPE_USER, // unfortunately nullable on the DB level, but never null. + 'remote' => $this->getRemote(), + 'remote_id' => $this->getRemoteId(), + 'share_token' => $this->getShareToken(), + 'name' => $this->getName(), + 'owner' => $this->getOwner(), + 'user' => $this->getUser(), + 'mountpoint' => $this->getMountpoint(), + 'accepted' => $this->getAccepted(), + + // Added later on + 'file_id' => null, + 'mimetype' => null, + 'permissions' => null, + 'mtime' => null, + 'type' => null, + ]; + } + + /** + * @internal For unit tests + * @return ExternalShare + */ + public function clone(): self { + $newShare = new ExternalShare(); + $newShare->setParent($this->getParent()); + $newShare->setShareType($this->getShareType()); + $newShare->setRemote($this->getRemote()); + $newShare->setRemoteId($this->getRemoteId()); + $newShare->setShareToken($this->getShareToken()); + $newShare->setPassword($this->getPassword()); + $newShare->setName($this->getName()); + $newShare->setOwner($this->getOwner()); + $newShare->setMountpoint($this->getMountpoint()); + $newShare->setAccepted($this->getAccepted()); + $newShare->setPassword($this->getPassword()); + return $newShare; + } +} diff --git a/apps/files_sharing/lib/External/ExternalShareMapper.php b/apps/files_sharing/lib/External/ExternalShareMapper.php new file mode 100644 index 00000000000..b19ca7950da --- /dev/null +++ b/apps/files_sharing/lib/External/ExternalShareMapper.php @@ -0,0 +1,204 @@ + + */ +class ExternalShareMapper extends QBMapper { + private const TABLE_NAME = 'share_external'; + + public function __construct( + IDBConnection $db, + private readonly IGroupManager $groupManager, + ) { + parent::__construct($db, self::TABLE_NAME); + } + + /** + * @throws DoesNotExistException + */ + public function getById(string $id): ExternalShare { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($id))) + ->setMaxResults(1); + return $this->findEntity($qb); + } + + /** + * Get share by token. + * + * @throws DoesNotExistException + */ + public function getShareByToken(string $token): ExternalShare { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->eq('share_token', $qb->createNamedParameter($token, IQueryBuilder::PARAM_STR))) + ->setMaxResults(1); + return $this->findEntity($qb); + } + + /** + * Get share by parent id and user. + * + * @throws DoesNotExistException + */ + public function getUserShare(ExternalShare $parentShare, IUser $user): ExternalShare { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->andX( + $qb->expr()->eq('parent', $qb->createNamedParameter($parentShare->getId())), + $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR)), + )); + return $this->findEntity($qb); + } + + public function get() { + + } + + public function getByMountPointAndUser(string $mountPoint, IUser $user) { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->andX( + $qb->expr()->eq('mountpoint_hash', $qb->createNamedParameter(md5($mountPoint))), + $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR)), + )); + return $this->findEntity($qb); + } + + /** + * @return \Generator + * @throws Exception + */ + public function getUserShares(IUser $user): \Generator { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER, IQueryBuilder::PARAM_INT))); + + return $this->yieldEntities($qb); + } + + public function deleteUserShares(IUser $user): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete(self::TABLE_NAME) + // user field can specify a user or a group + ->where($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID()))) + ->andWhere( + $qb->expr()->orX( + // delete direct shares + $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)), + // delete sub-shares of group shares for that user + $qb->expr()->andX( + $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_GROUP)), + $qb->expr()->neq('parent', $qb->expr()->literal(-1)), + ) + ) + ); + $qb->executeStatement(); + } + + /** + * @throws Exception + */ + public function deleteGroupShares(IGroup $group): void { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from(self::TABLE_NAME) + ->where($qb->expr()->eq('user', $qb->createNamedParameter($group->getGID()))) + ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))); + + $this->yieldEntities($qb); + + $delete = $this->db->getQueryBuilder(); + $delete->delete(self::TABLE_NAME) + ->where( + $qb->expr()->orX( + $qb->expr()->eq('id', $qb->createParameter('share_id')), + $qb->expr()->eq('parent', $qb->createParameter('share_parent_id')) + ) + ); + + foreach ($this->yieldEntities($qb) as $share) { + $delete->setParameter('share_id', $share->getId()); + $delete->setParameter('share_parent_id', $share->getId()); + $delete->executeStatement(); + } + } + + /** + * Return a list of shares for the user. + * + * @psalm-param IShare::STATUS_PENDING|IShare::STATUS_ACCEPTED|null $status Filter share by their status or return all shares of the user if null. + * @return list list of open server-to-server shares + * @throws Exception + */ + public function getShares(IUser $user, ?int $status): array { + // Not allowing providing a user here, + // as we only want to retrieve shares for the current user. + $groups = $this->groupManager->getUserGroups($user); + $userGroups = []; + foreach ($groups as $group) { + $userGroups[] = $group->getGID(); + } + + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from('share_external') + ->where( + $qb->expr()->orX( + $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID())), + $qb->expr()->in( + 'user', + $qb->createNamedParameter($userGroups, IQueryBuilder::PARAM_STR_ARRAY) + ) + ) + ) + ->orderBy('id', 'ASC'); + + $shares = $this->findEntities($qb); + + // remove parent group share entry if we have a specific user share entry for the user + $toRemove = []; + foreach ($shares as $share) { + if ($share->getShareType() === IShare::TYPE_GROUP && $share->getParent() !== '-1') { + $toRemove[] = $share->getParent(); + } + } + $shares = array_filter($shares, function (ExternalShare $share) use ($toRemove): bool { + return !in_array($share->getId(), $toRemove, true); + }); + + if (!is_null($status)) { + $shares = array_filter($shares, function (ExternalShare $share) use ($status): bool { + return $share->getAccepted() === $status; + }); + } + return array_values($shares); + } +} diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php index db881b06ec7..c2b26ccbff0 100644 --- a/apps/files_sharing/lib/External/Manager.php +++ b/apps/files_sharing/lib/External/Manager.php @@ -13,9 +13,8 @@ use OC\Files\SetupManager; use OC\User\NoUserException; use OCA\FederatedFileSharing\Events\FederatedShareAddedEvent; use OCA\Files_Sharing\Helper; -use OCA\Files_Sharing\ResponseDefinitions; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\DB\Exception; -use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\ICloudFederationFactory; use OCP\Federation\ICloudFederationProviderManager; @@ -34,29 +33,10 @@ use OCP\IUser; use OCP\IUserSession; use OCP\Notification\IManager; use OCP\OCS\IDiscoveryService; -use OCP\Server; -use OCP\Share; use OCP\Share\IShare; +use OCP\Snowflake\IGenerator; use Psr\Log\LoggerInterface; -/** - * @psalm-import-type Files_SharingRemoteShare from ResponseDefinitions - * @psalm-type ExternalShare = array{ - * id: int, - * remote: string, - * remote_id: string, - * parent: int, - * share_token: string, - * name: string, - * owner: string, - * user: string, - * mountpoint: string, - * accepted: bool, - * share_type:int, - * password: string, - * mountpoint_hash: string - * } - */ class Manager { public const STORAGE = '\OCA\Files_Sharing\External\Storage'; @@ -78,6 +58,8 @@ class Manager { private IRootFolder $rootFolder, private SetupManager $setupManager, private ICertificateManager $certificateManager, + private ExternalShareMapper $externalShareMapper, + private IGenerator $snowflakeGenerator, ) { $this->user = $userSession->getUser(); } @@ -89,181 +71,95 @@ class Manager { * @throws NotPermittedException * @throws NoUserException */ - public function addShare(string $remote, string $token, string $password, string $name, string $owner, int $shareType, bool $accepted = false, IUser|IGroup|null $userOrGroup = null, string $remoteId = '', int $parent = -1): ?Mount { + public function addShare(ExternalShare $shareExternal, IUser|IGroup|null $userOrGroup = null): ?Mount { $userOrGroup = $userOrGroup ?? $this->user; - $accepted = $accepted ? IShare::STATUS_ACCEPTED : IShare::STATUS_PENDING; - $name = Filesystem::normalizePath('/' . $name); - if ($accepted !== IShare::STATUS_ACCEPTED) { + if ($shareExternal->getAccepted() !== IShare::STATUS_ACCEPTED) { // To avoid conflicts with the mount point generation later, // we only use a temporary mount point name here. The real // mount point name will be generated when accepting the share, // using the original share item name. - $tmpMountPointName = '{{TemporaryMountPointName#' . $name . '}}'; - $mountPoint = $tmpMountPointName; - $hash = md5($tmpMountPointName); - $data = [ - 'remote' => $remote, - 'share_token' => $token, - 'password' => $password, - 'name' => $name, - 'owner' => $owner, - 'user' => $userOrGroup instanceof IGroup ? $userOrGroup->getGID() : $userOrGroup->getUID(), - 'mountpoint' => $mountPoint, - 'mountpoint_hash' => $hash, - 'accepted' => $accepted, - 'remote_id' => $remoteId, - 'share_type' => $shareType, - ]; + $tmpMountPointName = '{{TemporaryMountPointName#' . $shareExternal->getName() . '}}'; + $shareExternal->setMountpoint($tmpMountPointName); + $shareExternal->setUserOrGroup($userOrGroup); $i = 1; - while (!$this->connection->insertIfNotExist('*PREFIX*share_external', $data, ['user', 'mountpoint_hash'])) { - // The external share already exists for the user - $data['mountpoint'] = $tmpMountPointName . '-' . $i; - $data['mountpoint_hash'] = md5($data['mountpoint']); - $i++; + while (true) { + try { + $this->externalShareMapper->insert($shareExternal); + break; + } catch (Exception $e) { + if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + $shareExternal->setMountpoint($tmpMountPointName . '-' . $i); + $i++; + } else { + throw $e; + } + } } + return null; } $user = $userOrGroup instanceof IUser ? $userOrGroup : $this->user; $userFolder = $this->rootFolder->getUserFolder($user->getUID()); - $mountPoint = $userFolder->getNonExistingName($name); + $mountPoint = $userFolder->getNonExistingName($shareExternal->getName()); $mountPoint = Filesystem::normalizePath('/' . $mountPoint); - $hash = md5($mountPoint); - - $this->writeShareToDb($remote, $token, $password, $name, $owner, $userOrGroup, $mountPoint, $hash, $accepted, $remoteId, $parent, $shareType); + $shareExternal->setMountpoint($mountPoint); + $shareExternal->setUserOrGroup($user); + $this->externalShareMapper->insert($shareExternal); $options = [ - 'remote' => $remote, - 'token' => $token, - 'password' => $password, - 'mountpoint' => $mountPoint, - 'owner' => $owner + 'remote' => $shareExternal->getRemote(), + 'token' => $shareExternal->getShareToken(), + 'password' => $shareExternal->getPassword(), + 'mountpoint' => $shareExternal->getMountpoint(), + 'owner' => $shareExternal->getOwner(), ]; return $this->mountShare($options, $user); } - /** - * Write remote share to the database. - * - * @throws Exception - */ - private function writeShareToDb(string $remote, string $token, ?string $password, string $name, string $owner, IUser|IGroup $userOrGroup, string $mountPoint, string $hash, int $accepted, string $remoteId, int $parent, int $shareType): void { - $qb = $this->connection->getQueryBuilder(); - $qb->insert('share_external') - ->values([ - 'remote' => $qb->createNamedParameter($remote, IQueryBuilder::PARAM_STR), - 'share_token' => $qb->createNamedParameter($token, IQueryBuilder::PARAM_STR), - 'password' => $qb->createNamedParameter($password, IQueryBuilder::PARAM_STR), - 'name' => $qb->createNamedParameter($name, IQueryBuilder::PARAM_STR), - 'owner' => $qb->createNamedParameter($owner, IQueryBuilder::PARAM_STR), - 'user' => $qb->createNamedParameter($userOrGroup instanceof IGroup ? $userOrGroup->getGID() : $userOrGroup->getUID(), IQueryBuilder::PARAM_STR), - 'mountpoint' => $qb->createNamedParameter($mountPoint, IQueryBuilder::PARAM_STR), - 'mountpoint_hash' => $qb->createNamedParameter($hash, IQueryBuilder::PARAM_STR), - 'accepted' => $qb->createNamedParameter($accepted, IQueryBuilder::PARAM_INT), - 'remote_id' => $qb->createNamedParameter($remoteId, IQueryBuilder::PARAM_STR), - 'parent' => $qb->createNamedParameter($parent, IQueryBuilder::PARAM_INT), - 'share_type' => $qb->createNamedParameter($shareType, IQueryBuilder::PARAM_INT), - ]) - ->executeStatement(); - } - - /** - * Get share by id. - * - * @return ExternalShare|false - * @throws Exception - */ - private function fetchShare(int $id): array|false { - $qb = $this->connection->getQueryBuilder(); - $result = $qb->select('id', 'remote', 'remote_id', 'share_token', 'name', 'owner', 'user', 'mountpoint', 'accepted', 'parent', 'share_type', 'password', 'mountpoint_hash') - ->from('share_external') - ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))) - ->executeQuery(); - $share = $result->fetchAssociative(); - $result->closeCursor(); - return $share; - } - - /** - * Get share by token. - * - * @return ExternalShare|false - * @throws Exception - */ - public function getShareByToken(string $token): array|false { - $qb = $this->connection->getQueryBuilder(); - $result = $qb->select('id', 'remote', 'remote_id', 'share_token', 'name', 'owner', 'user', 'mountpoint', 'accepted', 'parent', 'share_type', 'password', 'mountpoint_hash') - ->from('share_external') - ->where($qb->expr()->eq('share_token', $qb->createNamedParameter($token, IQueryBuilder::PARAM_STR))) - ->executeQuery(); - $share = $result->fetchAssociative(); - $result->closeCursor(); - return $share; - } - - /** - * Get share by parent id and user. - * - * @return ExternalShare|false - * @throws Exception - */ - private function getUserShare(int $parentId, IUser $user): array|false { - $qb = $this->connection->getQueryBuilder(); - $result = $qb->select('id', 'remote', 'remote_id', 'share_token', 'name', 'owner', 'user', 'mountpoint', 'accepted', 'parent', 'share_type', 'password', 'mountpoint_hash') - ->from('share_external') - ->where($qb->expr()->andX( - $qb->expr()->eq('parent', $qb->createNamedParameter($parentId, IQueryBuilder::PARAM_INT)), - $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR)), - )) - ->executeQuery(); - $share = $result->fetchAssociative(); - $result->closeCursor(); - return $share; - } - - public function getShare(int $id, ?IUser $user = null): array|false { + public function getShare(string $id, ?IUser $user = null): ExternalShare|false { $user = $user ?? $this->user; - $share = $this->fetchShare($id); - if ($share === false) { + try { + $externalShare = $this->externalShareMapper->getById($id); + } catch (DoesNotExistException $e) { return false; } // check if the user is allowed to access it - if ($this->canAccessShare($share, $user)) { - return $share; + if ($this->canAccessShare($externalShare, $user)) { + return $externalShare; } return false; } - private function canAccessShare(array $share, IUser $user): bool { - $validShare = isset($share['share_type']) && isset($share['user']); - - if (!$validShare) { + private function canAccessShare(ExternalShare $share, IUser $user): bool { + $isValid = $share->getShareType() === null; + if ($isValid) { + // Invalid share type return false; } // If the share is a user share, check if the user is the recipient - if ((int)$share['share_type'] === IShare::TYPE_USER - && $share['user'] === $user->getUID()) { + if ($share->getShareType() === IShare::TYPE_USER && $share->getUser() === $user->getUID()) { return true; } // If the share is a group share, check if the user is in the group - if ((int)$share['share_type'] === IShare::TYPE_GROUP) { - $parentId = (int)$share['parent']; - if ($parentId !== -1) { + if ($share->getShareType() === IShare::TYPE_GROUP) { + $parentId = $share->getParent(); + if ($parentId !== '-1') { // we just retrieved a sub-share, switch to the parent entry for verification - $groupShare = $this->fetchShare($parentId); + $groupShare = $this->externalShareMapper->getById($parentId); } else { $groupShare = $share; } - if ($this->groupManager->get($groupShare['user'])->inGroup($user)) { + if ($this->groupManager->get($groupShare->getUser())->inGroup($user)) { return true; } } @@ -271,17 +167,52 @@ class Manager { return false; } + public function getShareByToken(string $token): ExternalShare|false { + try { + return $this->externalShareMapper->getShareByToken($token); + } catch (DoesNotExistException $e) { + return false; + } + } + /** - * Updates accepted flag in the database. - * * @throws Exception */ - private function updateAccepted(int $shareId, bool $accepted): void { - $qb = $this->connection->getQueryBuilder(); - $qb->update('share_external') - ->set('accepted', $qb->createNamedParameter($accepted ? 1 : 0, IQueryBuilder::PARAM_INT)) - ->where($qb->expr()->eq('id', $qb->createNamedParameter($shareId, IQueryBuilder::PARAM_INT))) - ->executeStatement(); + private function updateSubShare(ExternalShare $externalShare, IUser $user, ?string $mountPoint, int $accepted): ExternalShare { + $parentId = $externalShare->getParent(); + if ($parentId !== '-1') { + // this is the sub-share + $subShare = $externalShare; + } else { + try { + $subShare = $this->externalShareMapper->getUserShare($externalShare, $user); + } catch (DoesNotExistException $e) { + $subShare = new ExternalShare(); + $subShare->setId($this->snowflakeGenerator->nextId()); + $subShare->setRemote($externalShare->getRemote()); + $subShare->setPassword($externalShare->getPassword()); + $subShare->setName($externalShare->getName()); + $subShare->setOwner($externalShare->getOwner()); + $subShare->setUser($user->getUID()); + $subShare->setMountpoint($mountPoint ?? $externalShare->getMountpoint()); + $subShare->setAccepted($accepted); + $subShare->setRemoteId($externalShare->getRemoteId()); + $subShare->setParent($externalShare->getId()); + $subShare->setShareType($externalShare->getShareType()); + $subShare->setShareToken($externalShare->getShareToken()); + $this->externalShareMapper->insert($subShare); + } + } + + if ($subShare->getAccepted() !== $accepted) { + $subShare->setAccepted($accepted); + if ($mountPoint !== null) { + $subShare->setMountpoint($mountPoint); + } + $this->externalShareMapper->update($subShare); + } + + return $subShare; } /** @@ -289,7 +220,7 @@ class Manager { * * @return bool True if the share could be accepted, false otherwise */ - public function acceptShare(int $id, ?IUser $user = null): bool { + public function acceptShare(ExternalShare $externalShare, ?IUser $user = null): bool { // If we're auto-accepting a share, we need to know the user id // as there is no session available while processing the share // from the remote server request. @@ -299,91 +230,45 @@ class Manager { return false; } - $share = $this->getShare($id, $user); $result = false; - - if ($share) { - $this->setupManager->setupForUser($user); - $folder = $this->rootFolder->getUserFolder($user->getUID()); - - $shareFolder = Helper::getShareFolder(null, $user->getUID()); - $shareFolder = $folder->get($shareFolder); - /** @var Folder $shareFolder */ - $mountPoint = $shareFolder->getNonExistingName($share['name']); - $mountPoint = Filesystem::normalizePath($mountPoint); - $hash = md5($mountPoint); - $userShareAccepted = false; - - if ((int)$share['share_type'] === IShare::TYPE_USER) { - $qb = $this->connection->getQueryBuilder(); - $qb->update('share_external') - ->set('accepted', $qb->createNamedParameter(1)) - ->set('mountpoint', $qb->createNamedParameter($mountPoint)) - ->set('mountpoint_hash', $qb->createNamedParameter($hash)) - ->where($qb->expr()->andX( - $qb->expr()->eq('id', $qb->createNamedParameter($id)), - $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID())) - )); - $userShareAccepted = $qb->executeStatement(); - } else { - $parentId = (int)$share['parent']; - if ($parentId !== -1) { - // this is the sub-share - $subshare = $share; - } else { - $subshare = $this->getUserShare($id, $user); - } - - if ($subshare !== false) { - try { - $qb = $this->connection->getQueryBuilder(); - $qb->update('share_external') - ->set('accepted', $qb->createNamedParameter(1)) - ->set('mountpoint', $qb->createNamedParameter($mountPoint)) - ->set('mountpoint_hash', $qb->createNamedParameter($hash)) - ->where($qb->expr()->andX( - $qb->expr()->eq('id', $qb->createNamedParameter($subshare['id'])), - $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID())) - )) - ->executeStatement(); - $result = true; - } catch (Exception $e) { - $this->logger->emergency('Could not update share', ['exception' => $e]); - $result = false; - } - } else { - try { - $this->writeShareToDb( - $share['remote'], - $share['share_token'], - $share['password'], - $share['name'], - $share['owner'], - $user, - $mountPoint, $hash, 1, - $share['remote_id'], - $id, - $share['share_type']); - $result = true; - } catch (Exception $e) { - $this->logger->emergency('Could not create share', ['exception' => $e]); - $result = false; - } - } + $this->setupManager->setupForUser($user); + $folder = $this->rootFolder->getUserFolder($user->getUID()); + + $shareFolder = Helper::getShareFolder(null, $user->getUID()); + $shareFolder = $folder->get($shareFolder); + /** @var Folder $shareFolder */ + $mountPoint = $shareFolder->getNonExistingName($externalShare->getName()); + $mountPoint = Filesystem::normalizePath($mountPoint); + $userShareAccepted = false; + + if ($externalShare->getShareType() === IShare::TYPE_USER) { + if ($externalShare->getUser() === $user->getUID()) { + $externalShare->setAccepted(IShare::STATUS_ACCEPTED); + $externalShare->setMountpoint($mountPoint); + $this->externalShareMapper->update($externalShare); + $userShareAccepted = true; } - - if ($userShareAccepted !== false) { - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'accept'); - $event = new FederatedShareAddedEvent($share['remote']); - $this->eventDispatcher->dispatchTyped($event); - $this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($user)); + } else { + try { + $this->updateSubShare($externalShare, $user, $mountPoint, IShare::STATUS_ACCEPTED); $result = true; + } catch (Exception $e) { + $this->logger->emergency('Could not create sub-share', ['exception' => $e]); + $this->processNotification($externalShare, $user); + return false; } } - // Make sure the user has no notification for something that does not exist anymore. - $this->processNotification($id, $user); + if ($userShareAccepted !== false) { + $this->sendFeedbackToRemote($externalShare, 'accept'); + $event = new FederatedShareAddedEvent($externalShare->getRemote()); + $this->eventDispatcher->dispatchTyped($event); + $this->eventDispatcher->dispatchTyped(new InvalidateMountCacheEvent($user)); + $result = true; + } + // Make sure the user has no notification for something that does not exist anymore. + $this->processNotification($externalShare, $user); return $result; } @@ -392,108 +277,68 @@ class Manager { * * @return bool True if the share could be declined, false otherwise */ - public function declineShare(int $id, ?Iuser $user = null): bool { + public function declineShare(ExternalShare $externalShare, ?Iuser $user = null): bool { $user = $user ?? $this->user; if ($user === null) { $this->logger->error('No user specified for declining share'); return false; } - $share = $this->getShare($id, $user); $result = false; - if ($share && (int)$share['share_type'] === IShare::TYPE_USER) { - $qb = $this->connection->getQueryBuilder(); - $qb->delete('share_external') - ->where($qb->expr()->andX( - $qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)), - $qb->expr()->eq('user', $qb->createNamedParameter($user->getUID(), IQueryBuilder::PARAM_STR)) - )) - ->executeStatement(); - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline'); - - $this->processNotification($id, $user); - $result = true; - } elseif ($share && (int)$share['share_type'] === IShare::TYPE_GROUP) { - $parentId = (int)$share['parent']; - if ($parentId !== -1) { - // this is the sub-share - $subshare = $share; - } else { - $subshare = $this->getUserShare($id, $user); + if ($externalShare->getShareType() === IShare::TYPE_USER) { + if ($externalShare->getUser() === $user->getUID()) { + $this->externalShareMapper->delete($externalShare); + $this->sendFeedbackToRemote($externalShare, 'decline'); + $this->processNotification($externalShare, $user); + $result = true; } - - if ($subshare !== false) { - try { - $this->updateAccepted((int)$subshare['id'], false); - $result = true; - } catch (Exception $e) { - $this->logger->emergency('Could not update share', ['exception' => $e]); - $result = false; - } - } else { - try { - $this->writeShareToDb( - $share['remote'], - $share['share_token'], - $share['password'], - $share['name'], - $share['owner'], - $user, - $share['mountpoint'], - $share['mountpoint_hash'], - 0, - $share['remote_id'], - $id, - $share['share_type']); - $result = true; - } catch (Exception $e) { - $this->logger->emergency('Could not create share', ['exception' => $e]); - $result = false; - } + } elseif ($externalShare->getShareType() === IShare::TYPE_GROUP) { + try { + $this->updateSubShare($externalShare, $user, null, IShare::STATUS_PENDING); + $result = true; + } catch (Exception $e) { + $this->logger->emergency('Could not create sub-share', ['exception' => $e]); + $this->processNotification($externalShare, $user); + return false; } - $this->processNotification($id, $user); } + // Make sure the user has no notification for something that does not exist anymore. + $this->processNotification($externalShare, $user); return $result; } - public function processNotification(int $remoteShare, ?IUser $user = null): void { + public function processNotification(ExternalShare $remoteShare, ?IUser $user = null): void { $user = $user ?? $this->user; if ($user === null) { $this->logger->error('No user specified for processing notification'); return; } - $share = $this->fetchShare($remoteShare); - if ($share === false) { - return; - } - $filter = $this->notificationManager->createNotification(); $filter->setApp('files_sharing') ->setUser($user->getUID()) - ->setObject('remote_share', (string)$remoteShare); + ->setObject('remote_share', $remoteShare->getId()); $this->notificationManager->markProcessed($filter); } /** * Inform remote server whether server-to-server share was accepted/declined * - * @param string $remoteId Share id on the remote host * @param 'accept'|'decline' $feedback */ - private function sendFeedbackToRemote(string $remote, string $token, string $remoteId, string $feedback): bool { - $result = $this->tryOCMEndPoint($remote, $token, $remoteId, $feedback); + private function sendFeedbackToRemote(ExternalShare $externalShare, string $feedback): bool { + $result = $this->tryOCMEndPoint($externalShare, $feedback); if (is_array($result)) { return true; } - $federationEndpoints = $this->discoveryService->discover($remote, 'FEDERATED_SHARING'); + $federationEndpoints = $this->discoveryService->discover($externalShare->getRemote(), 'FEDERATED_SHARING'); $endpoint = $federationEndpoints['share'] ?? '/ocs/v2.php/cloud/shares'; - $url = rtrim($remote, '/') . $endpoint . '/' . $remoteId . '/' . $feedback . '?format=json'; - $fields = ['token' => $token]; + $url = rtrim($externalShare->getRemote(), '/') . $endpoint . '/' . $externalShare->getRemoteId() . '/' . $feedback . '?format=json'; + $fields = ['token' => $externalShare->getShareToken()]; $client = $this->clientService->newClient(); @@ -517,37 +362,36 @@ class Manager { /** * Try to send accept message to ocm end-point * - * @param string $remoteId id of the share * @param 'accept'|'decline' $feedback * @return array|false */ - protected function tryOCMEndPoint(string $remoteDomain, string $token, string $remoteId, string $feedback) { + protected function tryOCMEndPoint(ExternalShare $externalShare, string $feedback) { switch ($feedback) { case 'accept': $notification = $this->cloudFederationFactory->getCloudFederationNotification(); $notification->setMessage( 'SHARE_ACCEPTED', 'file', - $remoteId, + $externalShare->getRemoteId(), [ - 'sharedSecret' => $token, + 'sharedSecret' => $externalShare->getShareToken(), 'message' => 'Recipient accept the share' ] ); - return $this->cloudFederationProviderManager->sendNotification($remoteDomain, $notification); + return $this->cloudFederationProviderManager->sendNotification($externalShare->getRemote(), $notification); case 'decline': $notification = $this->cloudFederationFactory->getCloudFederationNotification(); $notification->setMessage( 'SHARE_DECLINED', 'file', - $remoteId, + $externalShare->getRemoteId(), [ - 'sharedSecret' => $token, + 'sharedSecret' => $externalShare->getShareToken(), 'message' => 'Recipient declined the share' ] ); - return $this->cloudFederationProviderManager->sendNotification($remoteDomain, $notification); + return $this->cloudFederationProviderManager->sendNotification($externalShare->getRemote(), $notification); } return false; } @@ -560,7 +404,7 @@ class Manager { return rtrim(substr($path, strlen($prefix)), '/'); } - public function getMount(array $data, ?IUser $user = null) { + public function getMount(array $data, ?IUser $user = null): Mount { $user = $user ?? $this->user; $data['manager'] = $this; $mountPoint = '/' . $user->getUID() . '/files' . $data['mountpoint']; @@ -599,7 +443,7 @@ class Manager { return $result; } - public function removeShare($mountPoint): bool { + public function removeShare(string $mountPoint): bool { try { $mountPointObj = $this->mountManager->find($mountPoint); } catch (NotFoundException $e) { @@ -613,31 +457,28 @@ class Manager { $id = $mountPointObj->getStorage()->getCache()->getId(''); $mountPoint = $this->stripPath($mountPoint); - $hash = md5($mountPoint); try { - $qb = $this->connection->getQueryBuilder(); - $qb->select('remote', 'share_token', 'remote_id', 'share_type', 'id') - ->from('share_external') - ->where($qb->expr()->eq('mountpoint_hash', $qb->createNamedParameter($hash))) - ->andWhere($qb->expr()->eq('user', $qb->createNamedParameter($this->user->getUID()))); - $result = $qb->executeQuery(); - $share = $result->fetchAssociative(); - $result->closeCursor(); - if ($share !== false && (int)$share['share_type'] === IShare::TYPE_USER) { + try { + $externalShare = $this->externalShareMapper->getByMountPointAndUser($mountPoint, $this->user); + } catch (DoesNotExistException $e) { + // ignore + $this->removeReShares((string)$id); + return true; + } + + if ($externalShare->getShareType() === IShare::TYPE_USER) { try { - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline'); + $this->sendFeedbackToRemote($externalShare, 'decline'); } catch (\Throwable $e) { // if we fail to notify the remote (probably cause the remote is down) // we still want the share to be gone to prevent undeletable remotes } - $qb = $this->connection->getQueryBuilder(); - $qb->delete('share_external') - ->where('id', $qb->createNamedParameter((int)$share['id'])) - ->executeStatement(); - } elseif ($share !== false && (int)$share['share_type'] === IShare::TYPE_GROUP) { - $this->updateAccepted((int)$share['id'], false); + $this->externalShareMapper->delete($externalShare); + } elseif ($externalShare->getShareType() === IShare::TYPE_GROUP) { + $externalShare->setAccepted(IShare::STATUS_PENDING); + $this->externalShareMapper->update($externalShare); } $this->removeReShares((string)$id); @@ -670,39 +511,16 @@ class Manager { } /** - * remove all shares for user $uid if the user was deleted + * Remove all shares for user $uid if the user was deleted. */ public function removeUserShares(IUser $user): bool { try { - $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'remote', 'share_type', 'share_token', 'remote_id') - ->from('share_external') - ->where($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID()))) - ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER))); - $result = $qb->executeQuery(); - $shares = $result->fetchAllAssociative(); - $result->closeCursor(); - + $shares = $this->externalShareMapper->getUserShares($user); foreach ($shares as $share) { - $this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline'); + $this->sendFeedbackToRemote($share, 'decline'); } - $qb = $this->connection->getQueryBuilder(); - $qb->delete('share_external') - // user field can specify a user or a group - ->where($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID()))) - ->andWhere( - $qb->expr()->orX( - // delete direct shares - $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)), - // delete sub-shares of group shares for that user - $qb->expr()->andX( - $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_GROUP)), - $qb->expr()->neq('parent', $qb->expr()->literal(-1)), - ) - ) - ); - $qb->executeStatement(); + $this->externalShareMapper->deleteUserShares($user); } catch (Exception $ex) { $this->logger->emergency('Could not delete user shares', ['exception' => $ex]); return false; @@ -711,32 +529,9 @@ class Manager { return true; } - public function removeGroupShares($gid): bool { + public function removeGroupShares(IGroup $group): bool { try { - $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'remote', 'share_type', 'share_token', 'remote_id') - ->from('share_external') - ->where($qb->expr()->eq('user', $qb->createNamedParameter($gid))) - ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))); - $result = $qb->executeQuery(); - $shares = $result->fetchAllAssociative(); - $result->closeCursor(); - - $qb = $this->connection->getQueryBuilder(); - // delete group share entry and matching sub-entries - $qb->delete('share_external') - ->where( - $qb->expr()->orX( - $qb->expr()->eq('id', $qb->createParameter('share_id')), - $qb->expr()->eq('parent', $qb->createParameter('share_parent_id')) - ) - ); - - foreach ($shares as $share) { - $qb->setParameter('share_id', $share['id']); - $qb->setParameter('share_parent_id', $share['id']); - $qb->executeStatement(); - } + $this->externalShareMapper->deleteGroupShares($group); } catch (Exception $ex) { $this->logger->emergency('Could not delete user shares', ['exception' => $ex]); return false; @@ -746,77 +541,27 @@ class Manager { } /** - * return a list of shares which are not yet accepted by the user + * Return a list of shares which are not yet accepted by the user. * - * @return list list of open server-to-server shares + * @return list list of open server-to-server shares */ - public function getOpenShares() { - return $this->getShares(false); - } - - /** - * return a list of shares which are accepted by the user - * - * @return list list of accepted server-to-server shares - */ - public function getAcceptedShares() { - return $this->getShares(true); + public function getOpenShares(): array { + try { + return $this->externalShareMapper->getShares($this->user, IShare::STATUS_PENDING); + } catch (Exception $e) { + $this->logger->emergency('Error when retrieving shares', ['exception' => $e]); + return []; + } } /** - * return a list of shares for the user + * Return a list of shares which are accepted by the user. * - * @param bool|null $accepted True for accepted only, - * false for not accepted, - * null for all shares of the user - * @return list list of open server-to-server shares + * @return list list of accepted server-to-server shares */ - private function getShares($accepted) { - // Not allowing providing a user here, - // as we only want to retrieve shares for the current user. - $groups = $this->groupManager->getUserGroups($this->user); - $userGroups = []; - foreach ($groups as $group) { - $userGroups[] = $group->getGID(); - } - - $qb = $this->connection->getQueryBuilder(); - $qb->select('id', 'share_type', 'parent', 'remote', 'remote_id', 'share_token', 'name', 'owner', 'user', 'mountpoint', 'accepted') - ->from('share_external') - ->where( - $qb->expr()->orX( - $qb->expr()->eq('user', $qb->createNamedParameter($this->user->getUID())), - $qb->expr()->in( - 'user', - $qb->createNamedParameter($userGroups, IQueryBuilder::PARAM_STR_ARRAY) - ) - ) - ) - ->orderBy('id', 'ASC'); - + public function getAcceptedShares(): array { try { - $result = $qb->executeQuery(); - /** @var list $shares */ - $shares = $result->fetchAllAssociative(); - $result->closeCursor(); - - // remove parent group share entry if we have a specific user share entry for the user - $toRemove = []; - foreach ($shares as $share) { - if ((int)$share['share_type'] === IShare::TYPE_GROUP && (int)$share['parent'] > 0) { - $toRemove[] = $share['parent']; - } - } - $shares = array_filter($shares, function ($share) use ($toRemove) { - return !in_array($share['id'], $toRemove, true); - }); - - if (!is_null($accepted)) { - $shares = array_filter($shares, function ($share) use ($accepted) { - return (bool)$share['accepted'] === $accepted; - }); - } - return array_values($shares); + return $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED); } catch (Exception $e) { $this->logger->emergency('Error when retrieving shares', ['exception' => $e]); return []; diff --git a/apps/files_sharing/lib/External/Mount.php b/apps/files_sharing/lib/External/Mount.php index f50c379f85f..6741270ec35 100644 --- a/apps/files_sharing/lib/External/Mount.php +++ b/apps/files_sharing/lib/External/Mount.php @@ -35,9 +35,8 @@ class Mount extends MountPoint implements MoveableMount, ISharedMountPoint { * Move the mount point to $target * * @param string $target the target mount point - * @return bool */ - public function moveMount($target) { + public function moveMount($target): bool { $result = $this->manager->setMountPoint($this->mountPoint, $target); $this->setMountPoint($target); @@ -57,7 +56,7 @@ class Mount extends MountPoint implements MoveableMount, ISharedMountPoint { * * @return string */ - public function getMountType() { + public function getMountType(): string { return 'shared'; } } diff --git a/apps/files_sharing/lib/ResponseDefinitions.php b/apps/files_sharing/lib/ResponseDefinitions.php index 71a2b25a70c..502931bf688 100644 --- a/apps/files_sharing/lib/ResponseDefinitions.php +++ b/apps/files_sharing/lib/ResponseDefinitions.php @@ -83,13 +83,13 @@ namespace OCA\Files_Sharing; * @psalm-type Files_SharingRemoteShare = array{ * accepted: bool, * file_id: int|null, - * id: int, + * id: string, * mimetype: string|null, * mountpoint: string, * mtime: int|null, * name: string, * owner: string, - * parent: int|null, + * parent: string|null, * permissions: int|null, * remote: string, * remote_id: string, diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json index 380a7194188..d2e80654ce3 100644 --- a/apps/files_sharing/openapi.json +++ b/apps/files_sharing/openapi.json @@ -416,8 +416,7 @@ "nullable": true }, "id": { - "type": "integer", - "format": "int64" + "type": "string" }, "mimetype": { "type": "string", @@ -438,8 +437,7 @@ "type": "string" }, "parent": { - "type": "integer", - "format": "int64", + "type": "string", "nullable": true }, "permissions": { @@ -3932,7 +3930,7 @@ "/ocs/v2.php/apps/files_sharing/api/v1/remote_shares/pending/{id}": { "post": { "operationId": "remote-accept-share", - "summary": "Accept a remote share", + "summary": "Accept a remote share.", "tags": [ "remote" ], @@ -3951,8 +3949,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -4055,7 +4052,7 @@ }, "delete": { "operationId": "remote-decline-share", - "summary": "Decline a remote share", + "summary": "Decline a remote share.", "tags": [ "remote" ], @@ -4074,8 +4071,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -4199,8 +4195,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -4324,8 +4319,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { diff --git a/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php b/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php index d9c832a3185..2ce427c8097 100644 --- a/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php +++ b/apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php @@ -8,10 +8,13 @@ namespace OCA\Files_Sharing\Tests\Command; use OCA\Files_Sharing\Command\CleanupRemoteStorages; +use OCA\Files_Sharing\External\ExternalShare; +use OCA\Files_Sharing\External\ExternalShareMapper; use OCP\Federation\ICloudId; use OCP\Federation\ICloudIdManager; use OCP\IDBConnection; use OCP\Server; +use OCP\Snowflake\IGenerator; use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -49,16 +52,6 @@ class CleanupRemoteStoragesTest extends TestCase { $storageQuery->insert('storages') ->setValue('id', $storageQuery->createParameter('id')); - $shareExternalQuery = Server::get(IDBConnection::class)->getQueryBuilder(); - $shareExternalQuery->insert('share_external') - ->setValue('share_token', $shareExternalQuery->createParameter('share_token')) - ->setValue('remote', $shareExternalQuery->createParameter('remote')) - ->setValue('name', $shareExternalQuery->createParameter('name')) - ->setValue('owner', $shareExternalQuery->createParameter('owner')) - ->setValue('user', $shareExternalQuery->createParameter('user')) - ->setValue('mountpoint', $shareExternalQuery->createParameter('mountpoint')) - ->setValue('mountpoint_hash', $shareExternalQuery->createParameter('mountpoint_hash')); - $filesQuery = Server::get(IDBConnection::class)->getQueryBuilder(); $filesQuery->insert('filecache') ->setValue('storage', $filesQuery->createParameter('storage')) @@ -73,15 +66,15 @@ class CleanupRemoteStoragesTest extends TestCase { } if (isset($storage['share_token'])) { - $shareExternalQuery - ->setParameter('share_token', $storage['share_token']) - ->setParameter('remote', $storage['remote']) - ->setParameter('name', 'irrelevant') - ->setParameter('owner', 'irrelevant') - ->setParameter('user', $storage['user']) - ->setParameter('mountpoint', 'irrelevant') - ->setParameter('mountpoint_hash', 'irrelevant'); - $shareExternalQuery->executeStatement(); + $externalShare = new ExternalShare(); + $externalShare->setId(Server::get(IGenerator::class)->nextId()); + $externalShare->setShareToken($storage['share_token']); + $externalShare->setRemote($storage['remote']); + $externalShare->setName('irrelevant'); + $externalShare->setOwner('irrelevant'); + $externalShare->setUser($storage['user']); + $externalShare->setMountpoint('irrelevant'); + Server::get(ExternalShareMapper::class)->insert($externalShare); } if (isset($storage['files_count'])) { diff --git a/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php b/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php index 7e054d9a6dc..74978984fd5 100644 --- a/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php +++ b/apps/files_sharing/tests/Controller/ExternalShareControllerTest.php @@ -8,10 +8,9 @@ namespace OCA\Files_Sharing\Tests\Controllers; use OCA\Files_Sharing\Controller\ExternalSharesController; +use OCA\Files_Sharing\External\ExternalShare; use OCA\Files_Sharing\External\Manager; use OCP\AppFramework\Http\JSONResponse; -use OCP\Http\Client\IClientService; -use OCP\IConfig; use OCP\IRequest; use PHPUnit\Framework\MockObject\MockObject; @@ -21,21 +20,13 @@ use PHPUnit\Framework\MockObject\MockObject; * @package OCA\Files_Sharing\Controllers */ class ExternalShareControllerTest extends \Test\TestCase { - /** @var IRequest */ - private $request; - /** @var \OCA\Files_Sharing\External\Manager */ - private $externalManager; - /** @var IConfig|MockObject */ - private $config; - /** @var IClientService */ - private $clientService; + private IRequest&MockObject $request; + private Manager $externalManager; protected function setUp(): void { parent::setUp(); $this->request = $this->createMock(IRequest::class); $this->externalManager = $this->createMock(Manager::class); - $this->clientService = $this->createMock(IClientService::class); - $this->config = $this->createMock(IConfig::class); } /** @@ -46,8 +37,6 @@ class ExternalShareControllerTest extends \Test\TestCase { 'files_sharing', $this->request, $this->externalManager, - $this->clientService, - $this->config, ); } @@ -61,20 +50,32 @@ class ExternalShareControllerTest extends \Test\TestCase { } public function testCreate(): void { + $share = $this->createMock(ExternalShare::class); + $this->externalManager + ->expects($this->once()) + ->method('getShare') + ->with('4') + ->willReturn($share); $this->externalManager ->expects($this->once()) ->method('acceptShare') - ->with(4); + ->with($share); - $this->assertEquals(new JSONResponse(), $this->getExternalShareController()->create(4)); + $this->assertEquals(new JSONResponse(), $this->getExternalShareController()->create('4')); } public function testDestroy(): void { + $share = $this->createMock(ExternalShare::class); + $this->externalManager + ->expects($this->once()) + ->method('getShare') + ->with('4') + ->willReturn($share); $this->externalManager ->expects($this->once()) ->method('declineShare') - ->with(4); + ->with($share); - $this->assertEquals(new JSONResponse(), $this->getExternalShareController()->destroy(4)); + $this->assertEquals(new JSONResponse(), $this->getExternalShareController()->destroy('4')); } } diff --git a/apps/files_sharing/tests/External/CacheTest.php b/apps/files_sharing/tests/External/CacheTest.php index c23101ea49e..7af8af4fb5f 100644 --- a/apps/files_sharing/tests/External/CacheTest.php +++ b/apps/files_sharing/tests/External/CacheTest.php @@ -18,6 +18,7 @@ use OCP\Files\Cache\ICacheEntry; use OCP\ICacheFactory; use OCP\IURLGenerator; use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; /** * Class Cache @@ -27,26 +28,11 @@ use OCP\IUserManager; */ #[\PHPUnit\Framework\Attributes\Group('DB')] class CacheTest extends TestCase { - /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ - protected $contactsManager; - - /** - * @var Storage - **/ - private $storage; - - /** - * @var Cache - */ - private $cache; - - /** - * @var string - */ - private $remoteUser; - - /** @var ICloudIdManager */ - private $cloudIdManager; + protected IManager&MockObject $contactsManager; + private Storage&MockObject $storage; + private Cache $cache; + private string $remoteUser; + private ICloudIdManager $cloudIdManager; protected function setUp(): void { parent::setUp(); @@ -62,7 +48,7 @@ class CacheTest extends TestCase { ); $this->remoteUser = $this->getUniqueID('remoteuser'); - $this->storage = $this->getMockBuilder('\OCA\Files_Sharing\External\Storage') + $this->storage = $this->getMockBuilder(\OCA\Files_Sharing\External\Storage::class) ->disableOriginalConstructor() ->getMock(); $this->storage diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php index 50c57f6a08a..64afb91b080 100644 --- a/apps/files_sharing/tests/External/ManagerTest.php +++ b/apps/files_sharing/tests/External/ManagerTest.php @@ -13,6 +13,8 @@ use OC\Files\SetupManager; use OC\Files\SetupManagerFactory; use OC\Files\Storage\StorageFactory; use OC\Files\Storage\Temporary; +use OCA\Files_Sharing\External\ExternalShare; +use OCA\Files_Sharing\External\ExternalShareMapper; use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\External\MountProvider; use OCA\Files_Sharing\Tests\TestCase; @@ -38,6 +40,7 @@ use OCP\IUserSession; use OCP\OCS\IDiscoveryService; use OCP\Server; use OCP\Share\IShare; +use OCP\Snowflake\IGenerator; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\Traits\UserTrait; @@ -68,6 +71,7 @@ class ManagerTest extends TestCase { protected IUserManager&MockObject $userManager; protected SetupManager&MockObject $setupManager; protected ICertificateManager&MockObject $certificateManager; + private ExternalShareMapper $externalShareMapper; protected function setUp(): void { parent::setUp(); @@ -93,6 +97,8 @@ class ManagerTest extends TestCase { return $folder; }); + $this->externalShareMapper = new ExternalShareMapper(Server::get(IDBConnection::class), $this->groupManager); + $this->contactsManager = $this->createMock(IManager::class); // needed for MountProvider() initialization $this->contactsManager->expects($this->any()) @@ -163,6 +169,8 @@ class ManagerTest extends TestCase { $this->rootFolder, $this->setupManager, $this->certificateManager, + $this->externalShareMapper, + Server::get(IGenerator::class), ] )->onlyMethods(['tryOCMEndPoint'])->getMock(); } @@ -181,39 +189,35 @@ class ManagerTest extends TestCase { } public function testAddUserShare(): void { - $this->doTestAddShare([ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_USER, - 'accepted' => false, - 'userOrGroup' => $this->user, - 'remoteId' => '2342' - ], false); + $userShare = new ExternalShare(); + $userShare->setId(Server::get(IGenerator::class)->nextId()); + $userShare->setRemote('http://localhost'); + $userShare->setShareToken('token1'); + $userShare->setPassword(''); + $userShare->setName('/SharedFolder'); + $userShare->setOwner('foobar'); + $userShare->setShareType(IShare::TYPE_USER); + $userShare->setAccepted(IShare::STATUS_PENDING); + $userShare->setRemoteId('2342'); + + $this->doTestAddShare($userShare, $this->user); } public function testAddGroupShare(): void { - $this->doTestAddShare([ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_GROUP, - 'accepted' => false, - 'userOrGroup' => $this->group1, - 'remoteId' => '2342' - ], true); + $groupShare = new ExternalShare(); + $groupShare->setId(Server::get(IGenerator::class)->nextId()); + $groupShare->setRemote('http://localhost'); + $groupShare->setOwner('foobar'); + $groupShare->setShareType(IShare::TYPE_GROUP); + $groupShare->setAccepted(IShare::STATUS_PENDING); + $groupShare->setRemoteId('2342'); + $groupShare->setShareToken('token1'); + $groupShare->setPassword(''); + $groupShare->setName('/SharedFolder'); + $this->doTestAddShare($groupShare, $this->group1, isGroup: true); } - public function doTestAddShare(array $shareData1, bool $isGroup = false): void { - $shareData2 = $shareData1; - $shareData2['token'] = 'token2'; - $shareData3 = $shareData1; - $shareData3['token'] = 'token3'; - + public function doTestAddShare(ExternalShare $shareData1, IUser|IGroup $userOrGroup, bool $isGroup = false): void { if ($isGroup) { $this->manager->expects($this->never())->method('tryOCMEndPoint')->willReturn(false); } else { @@ -226,27 +230,34 @@ class ManagerTest extends TestCase { } // Add a share for "user" - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData1)); + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], [$shareData1, $userOrGroup])); $openShares = $this->manager->getOpenShares(); $this->assertCount(1, $openShares); - $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}', $shareData1['userOrGroup']); + $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1->getName() . '}}', $userOrGroup); + + $shareData2 = $shareData1->clone(); + $shareData2->setShareToken('token2'); + $shareData2->setId(\OCP\Server::get(IGenerator::class)->nextId()); + $shareData3 = $shareData1->clone(); + $shareData3->setShareToken('token3'); + $shareData3->setId(\OCP\Server::get(IGenerator::class)->nextId()); $this->setupMounts(); $this->assertNotMount('SharedFolder'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); // Add a second share for "user" with the same name - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData2)); + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], [$shareData2, $userOrGroup])); $openShares = $this->manager->getOpenShares(); $this->assertCount(2, $openShares); - $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1['name'] . '}}', $shareData1['userOrGroup']); + $this->assertExternalShareEntry($shareData1, $openShares[0], 1, '{{TemporaryMountPointName#' . $shareData1->getName() . '}}', $userOrGroup); // New share falls back to "-1" appendix, because the name is already taken - $this->assertExternalShareEntry($shareData2, $openShares[1], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1', $shareData2['userOrGroup']); + $this->assertExternalShareEntry($shareData2, $openShares[1], 2, '{{TemporaryMountPointName#' . $shareData2->getName() . '}}-1', $userOrGroup); $this->setupMounts(); $this->assertNotMount('SharedFolder'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); $newClientCalls = []; $this->clientService @@ -271,44 +282,44 @@ class ManagerTest extends TestCase { ])); $client->expects($this->once()) ->method('post') - ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[0]['remote_id']), $this->anything()) + ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[0]->getRemoteId()), $this->anything()) ->willReturn($response); } // Accept the first share - $this->assertTrue($this->manager->acceptShare($openShares[0]['id'])); + $this->assertTrue($this->manager->acceptShare($openShares[0])); // Check remaining shares - Accepted - $acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]); + $acceptedShares = $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED); $this->assertCount(1, $acceptedShares); - $shareData1['accepted'] = true; - $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1['name'], $this->user); + $shareData1->setAccepted(true); + $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1->getName(), $this->user); // Check remaining shares - Open $openShares = $this->manager->getOpenShares(); $this->assertCount(1, $openShares); - $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1', $shareData2['userOrGroup']); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2->getName() . '}}-1', $userOrGroup); $this->setupMounts(); - $this->assertMount($shareData1['name']); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertMount($shareData1->getName()); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); // Add another share for "user" with the same name - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData3)); + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], [$shareData3, $userOrGroup])); $openShares = $this->manager->getOpenShares(); $this->assertCount(2, $openShares); - $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1', $shareData2['userOrGroup']); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2->getName() . '}}-1', $userOrGroup); if (!$isGroup) { // New share falls back to the original name (no "-\d", because the name is not taken) - $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}', $shareData3['userOrGroup']); + $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3->getName() . '}}', $userOrGroup); } else { - $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}-2', $shareData3['userOrGroup']); + $this->assertExternalShareEntry($shareData3, $openShares[1], 3, '{{TemporaryMountPointName#' . $shareData3->getName() . '}}-2', $userOrGroup); } $this->setupMounts(); - $this->assertMount($shareData1['name']); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertMount($shareData1->getName()); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); if (!$isGroup) { $client = $this->createMock(IClient::class); @@ -324,45 +335,45 @@ class ManagerTest extends TestCase { ])); $client->expects($this->once()) ->method('post') - ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[1]['remote_id'] . '/decline'), $this->anything()) + ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[1]->getRemoteId() . '/decline'), $this->anything()) ->willReturn($response); } // Decline the third share - $this->assertTrue($this->manager->declineShare($openShares[1]['id'])); + $this->assertTrue($this->manager->declineShare($openShares[1])); $this->setupMounts(); - $this->assertMount($shareData1['name']); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertMount($shareData1->getName()); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); // Check remaining shares - Accepted - $acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]); + $acceptedShares = $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED); $this->assertCount(1, $acceptedShares); - $shareData1['accepted'] = true; - $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1['name'], $this->user); + $shareData1->setAccepted(true); + $this->assertExternalShareEntry($shareData1, $acceptedShares[0], 1, $shareData1->getName(), $this->user); // Check remaining shares - Open $openShares = $this->manager->getOpenShares(); if ($isGroup) { // declining a group share adds it back to pending instead of deleting it $this->assertCount(2, $openShares); // this is a group share that is still open - $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1', $shareData2['userOrGroup']); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2->getName() . '}}-1', $userOrGroup); // this is the user share sub-entry matching the group share which got declined - $this->assertExternalShareEntry($shareData3, $openShares[1], 2, '{{TemporaryMountPointName#' . $shareData3['name'] . '}}-2', $this->user); + $this->assertExternalShareEntry($shareData3, $openShares[1], 2, '{{TemporaryMountPointName#' . $shareData3->getName() . '}}-2', $this->user); } else { $this->assertCount(1, $openShares); - $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2['name'] . '}}-1', $this->user); + $this->assertExternalShareEntry($shareData2, $openShares[0], 2, '{{TemporaryMountPointName#' . $shareData2->getName() . '}}-1', $this->user); } $this->setupMounts(); - $this->assertMount($shareData1['name']); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertMount($shareData1->getName()); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); if ($isGroup) { // no http requests here - $this->manager->removeGroupShares('group1'); + $this->manager->removeGroupShares($this->group1); } else { $client1 = $this->createMock(IClient::class); $client2 = $this->createMock(IClient::class); @@ -380,113 +391,119 @@ class ManagerTest extends TestCase { $client1->expects($this->once()) ->method('post') - ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[0]['remote_id'] . '/decline'), $this->anything()) + ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $openShares[0]->getRemoteId() . '/decline'), $this->anything()) ->willReturn($response); $client2->expects($this->once()) ->method('post') - ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $acceptedShares[0]['remote_id'] . '/decline'), $this->anything()) + ->with($this->stringStartsWith('http://localhost/ocs/v2.php/cloud/shares/' . $acceptedShares[0]->getRemoteId() . '/decline'), $this->anything()) ->willReturn($response); $this->manager->removeUserShares($this->user); } - $this->assertEmpty(self::invokePrivate($this->manager, 'getShares', [null]), 'Asserting all shares for the user have been deleted'); + $this->assertEmpty($this->externalShareMapper->getShares($this->user, null), 'Asserting all shares for the user have been deleted'); $this->clearMounts(); self::invokePrivate($this->manager, 'setupMounts'); - $this->assertNotMount($shareData1['name']); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}'); - $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1'); + $this->assertNotMount($shareData1->getName()); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}'); + $this->assertNotMount('{{TemporaryMountPointName#' . $shareData1->getName() . '}}-1'); } - private function verifyAcceptedGroupShare(array $shareData): void { + private function verifyAcceptedGroupShare(ExternalShare $share): void { $openShares = $this->manager->getOpenShares(); $this->assertCount(0, $openShares); - $acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]); + $acceptedShares = $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED); $this->assertCount(1, $acceptedShares); - $shareData['accepted'] = true; - $this->assertExternalShareEntry($shareData, $acceptedShares[0], 0, $shareData['name'], $this->user); + $share->setAccepted(IShare::STATUS_ACCEPTED); + $this->assertExternalShareEntry($share, $acceptedShares[0], 0, $share->getName(), $this->user); $this->setupMounts(); - $this->assertMount($shareData['name']); + $this->assertMount($share->getName()); } - private function verifyDeclinedGroupShare(array $shareData, ?string $tempMount = null): void { + private function verifyDeclinedGroupShare(ExternalShare $share, ?string $tempMount = null): void { if ($tempMount === null) { $tempMount = '{{TemporaryMountPointName#/SharedFolder}}'; } $openShares = $this->manager->getOpenShares(); $this->assertCount(1, $openShares); - $acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]); + $acceptedShares = $this->externalShareMapper->getShares($this->user, IShare::STATUS_ACCEPTED); $this->assertCount(0, $acceptedShares); - $this->assertExternalShareEntry($shareData, $openShares[0], 0, $tempMount, $this->user); + $share->setAccepted(IShare::STATUS_PENDING); + $this->assertExternalShareEntry($share, $openShares[0], 0, $tempMount, $this->user); $this->setupMounts(); - $this->assertNotMount($shareData['name']); + $this->assertNotMount($share->getName()); $this->assertNotMount($tempMount); } - private function createTestUserShare(string $userId = 'user1'): array { + private function createTestUserShare(string $userId = 'user1'): ExternalShare { $user = $this->createMock(IUser::class); $user->expects($this->any())->method('getUID')->willReturn($userId); - $shareData = [ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_USER, - 'accepted' => false, - 'userOrGroup' => $user, - 'remoteId' => '2342' - ]; - - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData)); - - return $shareData; + $share = new ExternalShare(); + $share->setId(Server::get(IGenerator::class)->nextId()); + $share->setRemote('http://localhost'); + $share->setShareToken('token1'); + $share->setPassword(''); + $share->setName('/SharedFolder'); + $share->setOwner('foobar'); + $share->setShareType(IShare::TYPE_USER); + $share->setAccepted(IShare::STATUS_PENDING); + $share->setRemoteId('2346'); + + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], [$share, $user])); + + return $share; } + + /** + * @return array{0: ExternalShare, 1: ExternalShare} + */ private function createTestGroupShare(string $groupId = 'group1'): array { - $shareData = [ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_GROUP, - 'accepted' => false, - 'userOrGroup' => $groupId === 'group1' ? $this->group1 : $this->group2, - 'remoteId' => '2342' - ]; - - $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData)); - - $allShares = self::invokePrivate($this->manager, 'getShares', [null]); + $share = new ExternalShare(); + $share->setId(Server::get(IGenerator::class)->nextId()); + $share->setRemote('http://localhost'); + $share->setShareToken('token1'); + $share->setPassword(''); + $share->setName('/SharedFolder'); + $share->setOwner('foobar'); + $share->setShareType(IShare::TYPE_GROUP); + $share->setAccepted(IShare::STATUS_PENDING); + $share->setRemoteId('2342'); + + $this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], [$share, $groupId === 'group1' ? $this->group1 : $this->group2])); + + $allShares = $this->externalShareMapper->getShares($this->user, null); + $groupShare = null; foreach ($allShares as $share) { - if ($share['user'] === $groupId) { + if ($share->getUser() === $groupId) { // this will hold the main group entry $groupShare = $share; break; } } - return [$shareData, $groupShare]; + $this->assertEquals($share->getId(), $groupShare->getId()); + + return [$share, $groupShare]; } public function testAcceptOriginalGroupShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // a second time - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); } public function testAcceptGroupShareAgainThroughGroupShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // decline again, this keeps the sub-share - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); // this will return sub-entries @@ -494,21 +511,21 @@ class ManagerTest extends TestCase { $this->assertCount(1, $openShares); // accept through group share - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData, '/SharedFolder'); // accept a second time - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData, '/SharedFolder'); } public function testAcceptGroupShareAgainThroughSubShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // decline again, this keeps the sub-share - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); // this will return sub-entries @@ -516,152 +533,150 @@ class ManagerTest extends TestCase { $this->assertCount(1, $openShares); // accept through sub-share - $this->assertTrue($this->manager->acceptShare($openShares[0]['id'])); + $this->assertTrue($this->manager->acceptShare($openShares[0])); $this->verifyAcceptedGroupShare($shareData); // accept a second time - $this->assertTrue($this->manager->acceptShare($openShares[0]['id'])); + $this->assertTrue($this->manager->acceptShare($openShares[0])); $this->verifyAcceptedGroupShare($shareData); } public function testDeclineOriginalGroupShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData); // a second time - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData); } public function testDeclineGroupShareAgainThroughGroupShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // decline again, this keeps the sub-share - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); // a second time - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); } public function testDeclineGroupShareAgainThroughSubshare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // this will return sub-entries - $allShares = self::invokePrivate($this->manager, 'getShares', [null]); + $allShares = $this->externalShareMapper->getShares($this->user, null); $this->assertCount(1, $allShares); // decline again through sub-share - $this->assertTrue($this->manager->declineShare($allShares[0]['id'])); + $this->assertTrue($this->manager->declineShare($allShares[0])); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); // a second time - $this->assertTrue($this->manager->declineShare($allShares[0]['id'])); + $this->assertTrue($this->manager->declineShare($allShares[0])); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); } public function testDeclineGroupShareAgainThroughMountPoint(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData); // decline through mount point name - $this->assertTrue($this->manager->removeShare($this->user->getUID() . '/files/' . $shareData['name'])); + $this->assertTrue($this->manager->removeShare($this->user->getUID() . '/files/' . $shareData->getName())); $this->verifyDeclinedGroupShare($shareData, '/SharedFolder'); // second time must fail as the mount point is gone - $this->assertFalse($this->manager->removeShare($this->user->getUID() . '/files/' . $shareData['name'])); + $this->assertFalse($this->manager->removeShare($this->user->getUID() . '/files/' . $shareData->getName())); } public function testDeclineThenAcceptGroupShareAgainThroughGroupShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); // decline, this creates a declined sub-share - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData); - // this will return sub-entries - $openShares = $this->manager->getOpenShares(); - // accept through sub-share - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData, '/SharedFolder'); // accept a second time - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $this->verifyAcceptedGroupShare($shareData, '/SharedFolder'); } public function testDeclineThenAcceptGroupShareAgainThroughSubShare(): void { [$shareData, $groupShare] = $this->createTestGroupShare(); // decline, this creates a declined sub-share - $this->assertTrue($this->manager->declineShare($groupShare['id'])); + $this->assertTrue($this->manager->declineShare($groupShare)); $this->verifyDeclinedGroupShare($shareData); // this will return sub-entries $openShares = $this->manager->getOpenShares(); // accept through sub-share - $this->assertTrue($this->manager->acceptShare($openShares[0]['id'])); + $this->assertTrue($this->manager->acceptShare($openShares[0])); $this->verifyAcceptedGroupShare($shareData); // accept a second time - $this->assertTrue($this->manager->acceptShare($openShares[0]['id'])); + $this->assertTrue($this->manager->acceptShare($openShares[0])); $this->verifyAcceptedGroupShare($shareData); } public function testDeleteUserShares(): void { // user 1 shares - $shareData = $this->createTestUserShare($this->user->getUID()); + $userShare = $this->createTestUserShare($this->user->getUID()); [$shareData, $groupShare] = $this->createTestGroupShare(); $shares = $this->manager->getOpenShares(); $this->assertCount(2, $shares); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); - $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('user2'); + $this->assertTrue($this->manager->acceptShare($groupShare)); + $user2 = $this->createMock(IUser::class); + $user2->method('getUID')->willReturn('user2'); // user 2 shares - $manager2 = $this->createManagerForUser($user); - $shareData2 = [ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_USER, - 'accepted' => false, - 'userOrGroup' => $user, - 'remoteId' => '2342' - ]; + $manager2 = $this->createManagerForUser($user2); + $share = new ExternalShare(); + $share->setId(Server::get(IGenerator::class)->nextId()); + $share->setRemote('http://localhost'); + $share->setShareToken('token1'); + $share->setPassword(''); + $share->setName('/SharedFolder'); + $share->setOwner('foobar'); + $share->setShareType(IShare::TYPE_USER); + $share->setAccepted(IShare::STATUS_PENDING); + $share->setRemoteId('2342'); $this->assertCount(1, $manager2->getOpenShares()); - $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2)); + $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], [$share, $user2])); $this->assertCount(2, $manager2->getOpenShares()); - $this->manager->expects($this->once())->method('tryOCMEndPoint')->with('http://localhost', 'token1', '2342', 'decline')->willReturn([]); + $userShare = $this->externalShareMapper->getById($userShare->getId()); // Simpler to compare + + $this->manager->expects($this->once())->method('tryOCMEndPoint')->with($userShare, 'decline')->willReturn([]); $this->manager->removeUserShares($this->user); $user1Shares = $this->manager->getOpenShares(); // user share is gone, group is still there $this->assertCount(1, $user1Shares); - $this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_GROUP); + $this->assertEquals($user1Shares[0]->getShareType(), IShare::TYPE_GROUP); // user 2 shares untouched $user2Shares = $manager2->getOpenShares(); $this->assertCount(2, $user2Shares); - $this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_GROUP); - $this->assertEquals($user2Shares[0]['user'], 'group1'); - $this->assertEquals($user2Shares[1]['share_type'], IShare::TYPE_USER); - $this->assertEquals($user2Shares[1]['user'], 'user2'); + $this->assertEquals($user2Shares[0]->getShareType(), IShare::TYPE_GROUP); + $this->assertEquals($user2Shares[0]->getUser(), 'group1'); + $this->assertEquals($user2Shares[1]->getShareType(), IShare::TYPE_USER); + $this->assertEquals($user2Shares[1]->getUser(), 'user2'); } public function testDeleteGroupShares(): void { @@ -672,52 +687,52 @@ class ManagerTest extends TestCase { $shares = $this->manager->getOpenShares(); $this->assertCount(2, $shares); - $this->assertTrue($this->manager->acceptShare($groupShare['id'])); + $this->assertTrue($this->manager->acceptShare($groupShare)); $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user2'); // user 2 shares $manager2 = $this->createManagerForUser($user); - $shareData2 = [ - 'remote' => 'http://localhost', - 'token' => 'token1', - 'password' => '', - 'name' => '/SharedFolder', - 'owner' => 'foobar', - 'shareType' => IShare::TYPE_USER, - 'accepted' => false, - 'userOrGroup' => $user, - 'remoteId' => '2342' - ]; + + $share = new ExternalShare(); + $share->setId(Server::get(IGenerator::class)->nextId()); + $share->setRemote('http://localhost'); + $share->setShareToken('token1'); + $share->setPassword(''); + $share->setName('/SharedFolder'); + $share->setOwner('foobar'); + $share->setShareType(IShare::TYPE_USER); + $share->setAccepted(IShare::STATUS_PENDING); + $share->setRemoteId('2343'); $this->assertCount(1, $manager2->getOpenShares()); - $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2)); + $this->assertSame(null, call_user_func_array([$manager2, 'addShare'], [$share, $user])); $this->assertCount(2, $manager2->getOpenShares()); $this->manager->expects($this->never())->method('tryOCMEndPoint'); - $this->manager->removeGroupShares('group1'); + $this->manager->removeGroupShares($this->group1); $user1Shares = $this->manager->getOpenShares(); // user share is gone, group is still there $this->assertCount(1, $user1Shares); - $this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_USER); + $this->assertEquals($user1Shares[0]->getShareType(), IShare::TYPE_USER); // user 2 shares untouched $user2Shares = $manager2->getOpenShares(); $this->assertCount(1, $user2Shares); - $this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_USER); - $this->assertEquals($user2Shares[0]['user'], 'user2'); + $this->assertEquals($user2Shares[0]->getShareType(), IShare::TYPE_USER); + $this->assertEquals($user2Shares[0]->getUser(), 'user2'); } - protected function assertExternalShareEntry(array $expected, array $actual, int $share, string $mountPoint, IUser|IGroup $targetEntity): void { - $this->assertEquals($expected['remote'], $actual['remote'], 'Asserting remote of a share #' . $share); - $this->assertEquals($expected['token'], $actual['share_token'], 'Asserting token of a share #' . $share); - $this->assertEquals($expected['name'], $actual['name'], 'Asserting name of a share #' . $share); - $this->assertEquals($expected['owner'], $actual['owner'], 'Asserting owner of a share #' . $share); - $this->assertEquals($expected['accepted'], (int)$actual['accepted'], 'Asserting accept of a share #' . $share); - $this->assertEquals($targetEntity instanceof IGroup ? $targetEntity->getGID() : $targetEntity->getUID(), $actual['user'], 'Asserting user of a share #' . $share); - $this->assertEquals($mountPoint, $actual['mountpoint'], 'Asserting mountpoint of a share #' . $share); + protected function assertExternalShareEntry(ExternalShare $expected, ExternalShare $actual, int $share, string $mountPoint, IUser|IGroup $targetEntity): void { + $this->assertEquals($expected->getRemote(), $actual->getRemote(), 'Asserting remote of a share #' . $share); + $this->assertEquals($expected->getShareToken(), $actual->getShareToken(), 'Asserting token of a share #' . $share); + $this->assertEquals($expected->getName(), $actual->getName(), 'Asserting name of a share #' . $share); + $this->assertEquals($expected->getOwner(), $actual->getOwner(), 'Asserting owner of a share #' . $share); + $this->assertEquals($expected->getAccepted(), $actual->getAccepted(), 'Asserting accept of a share #' . $share); + $this->assertEquals($targetEntity instanceof IGroup ? $targetEntity->getGID() : $targetEntity->getUID(), $actual->getUser(), 'Asserting user of a share #' . $share); + $this->assertEquals($mountPoint, $actual->getMountpoint(), 'Asserting mountpoint of a share #' . $share); } private function assertMount(string $mountPoint): void { diff --git a/build/integration/features/bootstrap/Sharing.php b/build/integration/features/bootstrap/Sharing.php index c4ca33618d7..17747570b41 100644 --- a/build/integration/features/bootstrap/Sharing.php +++ b/build/integration/features/bootstrap/Sharing.php @@ -305,6 +305,18 @@ trait Sharing { } } + public function getFieldValueInResponse($field) { + $data = simplexml_load_string($this->response->getBody())->data[0]; + if (count($data->element) > 0) { + foreach ($data as $element) { + return (string)$element->$field; + } + + return false; + } + return $data->$field; + } + public function isFieldInResponse($field, $contentExpected) { $data = simplexml_load_string($this->response->getBody())->data[0]; if ((string)$field == 'expiration') { diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 0e03b79bb18..ebeea6d8467 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1305,7 +1305,6 @@ - @@ -1315,7 +1314,6 @@ ['uid' => &$shareWith] )]]> - @@ -1325,12 +1323,6 @@ - - - - - - @@ -1626,15 +1618,6 @@ - - - - - - - - - @@ -1682,25 +1665,10 @@ - - - - - - - - - - - - - - - @@ -4420,14 +4388,8 @@ - - - - - - diff --git a/lib/private/Activity/Event.php b/lib/private/Activity/Event.php index 0ccad1d0a4e..5ca326fbdd6 100644 --- a/lib/private/Activity/Event.php +++ b/lib/private/Activity/Event.php @@ -48,8 +48,7 @@ class Event implements IEvent { protected $messageRichParameters = []; /** @var string */ protected $objectType = ''; - /** @var int */ - protected $objectId = 0; + protected string|int $objectId = 0; /** @var string */ protected $objectName = ''; /** @var string */ @@ -319,7 +318,7 @@ class Event implements IEvent { /** * {@inheritDoc} */ - public function setObject(string $objectType, int $objectId, string $objectName = ''): IEvent { + public function setObject(string $objectType, string|int $objectId, string $objectName = ''): IEvent { if (isset($objectType[255])) { throw new InvalidValueException('objectType'); } @@ -340,9 +339,9 @@ class Event implements IEvent { } /** - * @return int + * @return int|string */ - public function getObjectId(): int { + public function getObjectId(): string|int { return $this->objectId; } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 93f3fa44b9d..2722ba59e62 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1406,8 +1406,8 @@ class Manager implements IManager { * * @throws ShareNotFound */ - public function getShareByToken($token) { - // tokens cannot be valid local user names + public function getShareByToken($token): IShare { + // tokens cannot be valid local usernames if ($this->userManager->userExists($token)) { throw new ShareNotFound(); } @@ -1417,8 +1417,7 @@ class Manager implements IManager { $provider = $this->factory->getProviderForType(IShare::TYPE_LINK); $share = $provider->getShareByToken($token); } - } catch (ProviderException $e) { - } catch (ShareNotFound $e) { + } catch (ProviderException|ShareNotFound) { } @@ -1427,8 +1426,7 @@ class Manager implements IManager { try { $provider = $this->factory->getProviderForType(IShare::TYPE_REMOTE); $share = $provider->getShareByToken($token); - } catch (ProviderException $e) { - } catch (ShareNotFound $e) { + } catch (ProviderException|ShareNotFound) { } } @@ -1437,8 +1435,7 @@ class Manager implements IManager { try { $provider = $this->factory->getProviderForType(IShare::TYPE_EMAIL); $share = $provider->getShareByToken($token); - } catch (ProviderException $e) { - } catch (ShareNotFound $e) { + } catch (ProviderException|ShareNotFound) { } } @@ -1446,8 +1443,7 @@ class Manager implements IManager { try { $provider = $this->factory->getProviderForType(IShare::TYPE_CIRCLE); $share = $provider->getShareByToken($token); - } catch (ProviderException $e) { - } catch (ShareNotFound $e) { + } catch (ProviderException|ShareNotFound) { } } @@ -1455,8 +1451,7 @@ class Manager implements IManager { try { $provider = $this->factory->getProviderForType(IShare::TYPE_ROOM); $share = $provider->getShareByToken($token); - } catch (ProviderException $e) { - } catch (ShareNotFound $e) { + } catch (ProviderException|ShareNotFound) { } } diff --git a/lib/public/Activity/IEvent.php b/lib/public/Activity/IEvent.php index 6014b75123c..898eebac4b5 100644 --- a/lib/public/Activity/IEvent.php +++ b/lib/public/Activity/IEvent.php @@ -211,14 +211,14 @@ interface IEvent { * Set the object of the activity * * @param string $objectType - * @param int $objectId + * @param string|int $objectId * @param string $objectName * @return IEvent * @throws InvalidValueException if the object is invalid * @since 8.2.0 * @since 30.0.0 throws {@see InvalidValueException} instead of \InvalidArgumentException */ - public function setObject(string $objectType, int $objectId, string $objectName = ''): self; + public function setObject(string $objectType, string|int $objectId, string $objectName = ''): self; /** * Set the link of the activity @@ -292,10 +292,10 @@ interface IEvent { public function getObjectType(): string; /** - * @return int + * @return string|int * @since 8.2.0 */ - public function getObjectId(): int; + public function getObjectId(): string|int; /** * @return string diff --git a/openapi.json b/openapi.json index 24f56c78288..1a4f3fd4be5 100644 --- a/openapi.json +++ b/openapi.json @@ -2523,8 +2523,7 @@ "nullable": true }, "id": { - "type": "integer", - "format": "int64" + "type": "string" }, "mimetype": { "type": "string", @@ -2545,8 +2544,7 @@ "type": "string" }, "parent": { - "type": "integer", - "format": "int64", + "type": "string", "nullable": true }, "permissions": { @@ -25527,7 +25525,7 @@ "/ocs/v2.php/apps/files_sharing/api/v1/remote_shares/pending/{id}": { "post": { "operationId": "files_sharing-remote-accept-share", - "summary": "Accept a remote share", + "summary": "Accept a remote share.", "tags": [ "files_sharing/remote" ], @@ -25546,8 +25544,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -25650,7 +25647,7 @@ }, "delete": { "operationId": "files_sharing-remote-decline-share", - "summary": "Decline a remote share", + "summary": "Decline a remote share.", "tags": [ "files_sharing/remote" ], @@ -25669,8 +25666,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -25794,8 +25790,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, { @@ -25919,8 +25914,7 @@ "description": "ID of the share", "required": true, "schema": { - "type": "integer", - "format": "int64" + "type": "string" } }, {