From a9c93d4876b3f7dd75d7240e41cc12cd795cb8ab Mon Sep 17 00:00:00 2001 From: Maxence Lange Date: Thu, 10 Aug 2023 23:15:59 -0100 Subject: [PATCH] requested changes Signed-off-by: Maxence Lange --- .../cloud_federation_api/lib/Capabilities.php | 32 ++++---- apps/files_sharing/lib/External/Storage.php | 2 +- build/psalm-baseline.xml | 5 ++ core/Controller/OCMController.php | 5 +- .../CloudFederationProviderManager.php | 6 -- lib/private/OCM/Model/OCMProvider.php | 77 +++++++------------ lib/private/OCM/Model/OCMResource.php | 12 +-- lib/private/OCM/OCMDiscoveryService.php | 27 +++++-- .../OCM/Exceptions/OCMArgumentException.php | 3 + .../OCM/Exceptions/OCMProviderException.php | 3 + lib/public/OCM/IOCMProvider.php | 25 +----- lib/public/OCM/IOCMResource.php | 10 +-- 12 files changed, 96 insertions(+), 111 deletions(-) diff --git a/apps/cloud_federation_api/lib/Capabilities.php b/apps/cloud_federation_api/lib/Capabilities.php index e17374c08f739..9c94aa68a8cec 100644 --- a/apps/cloud_federation_api/lib/Capabilities.php +++ b/apps/cloud_federation_api/lib/Capabilities.php @@ -31,7 +31,7 @@ use OC\OCM\Model\OCMResource; use OCP\Capabilities\ICapability; use OCP\IURLGenerator; -use OCP\OCM\IOCMDiscoveryService; +use OCP\OCM\Exceptions\OCMArgumentException; class Capabilities implements ICapability { @@ -39,27 +39,27 @@ class Capabilities implements ICapability { public function __construct( private IURLGenerator $urlGenerator, - private IOCMDiscoveryService $discoveryService ) { } /** * Function an app uses to return the capabilities * - * @return array{ - * ocm: array{ + * @return array { + * ocm: array { * enabled: bool, * apiVersion: string, * endPoint: string, - * resourceTypes: array{ + * resourceTypes: array { * name: string, * shareTypes: string[], - * protocols: array{ + * protocols: array { * webdav: string, - * }, - * }[], - * }, + * }, + * }[], + * }, * } + * @throws OCMArgumentException */ public function getCapabilities() { $url = $this->urlGenerator->linkToRouteAbsolute('cloud_federation_api.requesthandlercontroller.addShare'); @@ -67,15 +67,21 @@ public function getCapabilities() { $provider = new OCMProvider(); $provider->setEnabled(true); $provider->setApiVersion(self::API_VERSION); - $provider->setEndPoint(substr($url, 0, strrpos($url, '/'))); + + $pos = strrpos($url, '/'); + if (false === $pos) { + throw new OCMArgumentException('generated route should contains a slash character'); + } + + $provider->setEndPoint(substr($url, 0, $pos)); $resource = new OCMResource(); $resource->setName('file') - ->setShareTypes(['user', 'group']) - ->setProtocols(['webdav' => '/public.php/webdav/']); + ->setShareTypes(['user', 'group']) + ->setProtocols(['webdav' => '/public.php/webdav/']); $provider->setResourceTypes([$resource]); - return ['ocm' => $provider]; + return ['ocm' => $provider->jsonSerialize()]; } } diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index b750e2b76cffc..735f74b0a6067 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -82,7 +82,7 @@ public function __construct($options) { // use default path to webdav if not found on discovery try { $ocmProvider = $discoveryService->discover($this->cloudId->getRemote()); - $webDavEndpoint = $ocmProvider->extractProtocolUrl('file', 'webdav'); + $webDavEndpoint = $ocmProvider->extractProtocolEntry('file', 'webdav'); $secure = (parse_url($ocmProvider->getEndPoint(), PHP_URL_SCHEME) === 'https'); $host = parse_url($ocmProvider->getEndPoint(), PHP_URL_HOST); } catch (OCMProviderException|OCMArgumentException $e) { diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 3d56f710237c1..7be4bc475623b 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -43,6 +43,11 @@ IEventListener + + + array + + IEventListener diff --git a/core/Controller/OCMController.php b/core/Controller/OCMController.php index fcfaadc87ab1c..04983d21c6817 100644 --- a/core/Controller/OCMController.php +++ b/core/Controller/OCMController.php @@ -34,8 +34,6 @@ use OCP\IConfig; use OCP\IRequest; use OCP\Server; -use Psr\Container\ContainerExceptionInterface; -use Psr\Container\NotFoundExceptionInterface; use Psr\Log\LoggerInterface; /** @@ -44,7 +42,6 @@ * @since 28.0.0 */ class OCMController extends Controller { - public function __construct( IRequest $request, private IConfig $config, @@ -85,7 +82,7 @@ public function discovery(): Response { return new DataResponse( ['message' => '/ocm-provider/ not supported'], - Http::STATUS_NOT_FOUND + Http::STATUS_INTERNAL_SERVER_ERROR ); } } diff --git a/lib/private/Federation/CloudFederationProviderManager.php b/lib/private/Federation/CloudFederationProviderManager.php index db6c67fb2f0ad..ea2f0dd7575f0 100644 --- a/lib/private/Federation/CloudFederationProviderManager.php +++ b/lib/private/Federation/CloudFederationProviderManager.php @@ -49,14 +49,8 @@ * @package OC\Federation */ class CloudFederationProviderManager implements ICloudFederationProviderManager { - /** @var array list of available cloud federation providers */ private array $cloudFederationProvider = []; - private array $ocmEndPoints = []; - private array $supportedAPIVersion = [ - '1.0-proposal1', - '1.0', - ]; public function __construct( private IConfig $config, diff --git a/lib/private/OCM/Model/OCMProvider.php b/lib/private/OCM/Model/OCMProvider.php index 50854b1b6e241..8f78ebf8cc591 100644 --- a/lib/private/OCM/Model/OCMProvider.php +++ b/lib/private/OCM/Model/OCMProvider.php @@ -28,6 +28,7 @@ use JsonSerializable; use OCP\OCM\Exceptions\OCMArgumentException; +use OCP\OCM\Exceptions\OCMProviderException; use OCP\OCM\IOCMProvider; /** @@ -87,9 +88,7 @@ public function getApiVersion(): string { * @return OCMProvider */ public function setEndPoint(string $endPoint): self { - if ($this->isSafeUrl(parse_url($endPoint, PHP_URL_PATH) ?? '')) { - $this->endPoint = $endPoint; - } + $this->endPoint = $endPoint; return $this; } @@ -106,7 +105,7 @@ public function getEndPoint(): string { * * @return $this */ - public function addResource(OCMResource $resource): self { + public function addResourceType(OCMResource $resource): self { $this->resourceTypes[] = $resource; return $this; @@ -130,44 +129,6 @@ public function getResourceTypes(): array { return $this->resourceTypes; } - /** - * @return bool - */ - public function looksValid(): bool { - if ($this->url !== '' - && parse_url($this->url, PHP_URL_HOST) !== - parse_url($this->getEndPoint(), PHP_URL_HOST)) { - return false; - } - - return ($this->getApiVersion() !== '' && $this->getEndPoint() !== ''); - } - - /** - * @param string $url - * - * @return bool - */ - protected function isSafeUrl(string $url): bool { - return (bool)preg_match('/^[\/\.\-A-Za-z0-9]+$/', $url); - } - - /** - * @param string $resourceName - * @param string $protocol - * - * @return string - * @throws OCMArgumentException - */ - public function extractProtocolUrl(string $resourceName, string $protocol): string { - $url = $this->extractProtocolEntry($resourceName, $protocol); - if (!$this->isSafeUrl($url)) { - throw new OCMArgumentException('url does not looks safe'); - } - - return $url; - } - /** * @param string $resourceName * @param string $protocol @@ -193,18 +154,15 @@ public function extractProtocolEntry(string $resourceName, string $protocol): st /** * import data from an array * - * @param array|null $data + * @param array $data * * @return self + * @throws OCMProviderException in case a descent provider cannot be generated from data * @see self::jsonSerialize() */ - public function import(?array $data): self { - if (is_null($data)) { - return $this; - } - + public function import(array $data): self { $this->setEnabled(is_bool($data['enabled'] ?? '') ? $data['enabled'] : false) - ->setApiVersion((string)$data['apiVersion'] ?? '') + ->setApiVersion((string)($data['apiVersion'] ?? '')) ->setEndPoint($data['endPoint'] ?? ''); $resources = []; @@ -214,9 +172,30 @@ public function import(?array $data): self { } $this->setResourceTypes($resources); + if (!$this->looksValid()) { + throw new OCMProviderException('remote provider does not look valid'); + } + return $this; } + + /** + * @return bool + */ + private function looksValid(): bool { + if ($this->url !== '' + && parse_url($this->url, PHP_URL_HOST) !== + parse_url($this->getEndPoint(), PHP_URL_HOST)) { + return false; + } + + return ($this->getApiVersion() !== '' && $this->getEndPoint() !== ''); + } + + + + public function jsonSerialize(): array { return [ 'enabled' => $this->isEnabled(), diff --git a/lib/private/OCM/Model/OCMResource.php b/lib/private/OCM/Model/OCMResource.php index 99200403d14c1..77e997924a5c6 100644 --- a/lib/private/OCM/Model/OCMResource.php +++ b/lib/private/OCM/Model/OCMResource.php @@ -34,7 +34,9 @@ */ class OCMResource implements IOCMResource, JsonSerializable { private string $name = ''; + /** @var string[] */ private array $shareTypes = []; + /** @var array */ private array $protocols = []; /** @@ -56,7 +58,7 @@ public function getName(): string { } /** - * @param array $shareTypes + * @param string[] $shareTypes * * @return OCMResource */ @@ -67,14 +69,14 @@ public function setShareTypes(array $shareTypes): self { } /** - * @return array + * @return string[] */ public function getShareTypes(): array { return $this->shareTypes; } /** - * @param array $protocols + * @param array $protocols * * @return $this */ @@ -85,7 +87,7 @@ public function setProtocols(array $protocols): self { } /** - * @return array + * @return array */ public function getProtocols(): array { return $this->protocols; @@ -100,7 +102,7 @@ public function getProtocols(): array { * @return self */ public function import(array $data): self { - return $this->setName((string)$data['name'] ?? '') + return $this->setName((string)($data['name'] ?? '')) ->setShareTypes($data['shareTypes'] ?? []) ->setProtocols($data['protocols'] ?? []); } diff --git a/lib/private/OCM/OCMDiscoveryService.php b/lib/private/OCM/OCMDiscoveryService.php index 0fa760bf4a562..92f394cf4aed2 100644 --- a/lib/private/OCM/OCMDiscoveryService.php +++ b/lib/private/OCM/OCMDiscoveryService.php @@ -26,12 +26,14 @@ namespace OC\OCM; +use JsonException; use OC\OCM\Model\OCMProvider; use OCP\AppFramework\Http; use OCP\Http\Client\IClientService; use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; +use OCP\IL10N; use OCP\OCM\Exceptions\OCMProviderException; use OCP\OCM\IOCMDiscoveryService; use OCP\OCM\IOCMProvider; @@ -42,11 +44,17 @@ */ class OCMDiscoveryService implements IOCMDiscoveryService { private ICache $cache; + private array $supportedAPIVersion = + [ + '1.0-proposal1', + '1.0', + ]; public function __construct( ICacheFactory $cacheFactory, private IClientService $clientService, private IConfig $config, + private IL10N $l10n, private LoggerInterface $logger ) { $this->cache = $cacheFactory->createDistributed('ocm-discovery'); @@ -65,9 +73,13 @@ public function discover(string $remote, bool $skipCache = false): IOCMProvider $provider = new OCMProvider($remote); if (!$skipCache) { - $provider->import(json_decode($this->cache->get($remote) ?? '', true)); - if ($provider->looksValid()) { - return $provider; // if cache looks valid, we use it + try { + $provider->import(json_decode($this->cache->get($remote) ?? '', true, 8, JSON_THROW_ON_ERROR) ?? []); + if (in_array($provider->getApiVersion(), $this->supportedAPIVersion)) { + return $provider; // if cache looks valid, we use it + } + } catch (JsonException|OCMProviderException $e) { + // we ignore cache on issues } } @@ -85,18 +97,21 @@ public function discover(string $remote, bool $skipCache = false): IOCMProvider if ($response->getStatusCode() === Http::STATUS_OK) { $body = $response->getBody(); // update provider with data returned by the request - $provider->import(json_decode($body, true)); + $provider->import(json_decode($body, true, 8, JSON_THROW_ON_ERROR) ?? []); $this->cache->set($remote, $body, 60 * 60 * 24); } + } catch (JsonException|OCMProviderException $e) { + throw new OCMProviderException('data returned by remote seems invalid - ' . ($body ?? '')); } catch (\Exception $e) { $this->logger->warning('error while discovering ocm provider', [ 'exception' => $e, 'remote' => $remote ]); + throw new OCMProviderException('error while requesting remote ocm provider'); } - if (!$provider->looksValid()) { - throw new OCMProviderException('remote provider does not look valid'); + if (!in_array($provider->getApiVersion(), $this->supportedAPIVersion)) { + throw new OCMProviderException($this->l10n->t('unsupported version (%1)', [$provider->getApiVersion()])); } return $provider; diff --git a/lib/public/OCM/Exceptions/OCMArgumentException.php b/lib/public/OCM/Exceptions/OCMArgumentException.php index 8a3f1c03fee82..e3abd7bf26bab 100644 --- a/lib/public/OCM/Exceptions/OCMArgumentException.php +++ b/lib/public/OCM/Exceptions/OCMArgumentException.php @@ -27,5 +27,8 @@ use Exception; +/** + * @since 28.0.0 + */ class OCMArgumentException extends Exception { } diff --git a/lib/public/OCM/Exceptions/OCMProviderException.php b/lib/public/OCM/Exceptions/OCMProviderException.php index 3257177d0049e..32dab10dc68d6 100644 --- a/lib/public/OCM/Exceptions/OCMProviderException.php +++ b/lib/public/OCM/Exceptions/OCMProviderException.php @@ -27,5 +27,8 @@ use Exception; +/** + * @since 28.0.0 + */ class OCMProviderException extends Exception { } diff --git a/lib/public/OCM/IOCMProvider.php b/lib/public/OCM/IOCMProvider.php index e289fef144b22..f99ccf1cd238e 100644 --- a/lib/public/OCM/IOCMProvider.php +++ b/lib/public/OCM/IOCMProvider.php @@ -28,6 +28,7 @@ use OC\OCM\Model\OCMResource; use OCP\OCM\Exceptions\OCMArgumentException; +use OCP\OCM\Exceptions\OCMProviderException; /** * Model based on the Open Cloud Mesh Discovery API @@ -35,7 +36,6 @@ * @since 28.0.0 */ interface IOCMProvider { - /** * enable OCM * @@ -98,7 +98,7 @@ public function getEndPoint(): string; * @return self * @since 28.0.0 */ - public function addResource(OCMResource $resource): self; + public function addResourceType(OCMResource $resource): self; /** * set resources @@ -118,26 +118,6 @@ public function setResourceTypes(array $resourceTypes): self; */ public function getResourceTypes(): array; - /** - * returns if object have its required parameters well set - * - * @return bool - * @since 28.0.0 - */ - public function looksValid(): bool; - - /** - * extract a safe url from the listing of protocols, based on resource-name and protocol-name - * - * @param string $resourceName - * @param string $protocol - * - * @return string - * @throws OCMArgumentException - * @since 28.0.0 - */ - public function extractProtocolUrl(string $resourceName, string $protocol): string; - /** * extract a specific string value from the listing of protocols, based on resource-name and protocol-name * @@ -156,6 +136,7 @@ public function extractProtocolEntry(string $resourceName, string $protocol): st * @param array $data * * @return self + * @throws OCMProviderException in case a descent provider cannot be generated from data * @since 28.0.0 */ public function import(array $data): self; diff --git a/lib/public/OCM/IOCMResource.php b/lib/public/OCM/IOCMResource.php index 2ed46c2b9c705..381af61cecc4c 100644 --- a/lib/public/OCM/IOCMResource.php +++ b/lib/public/OCM/IOCMResource.php @@ -33,7 +33,6 @@ * @since 28.0.0 */ interface IOCMResource { - /** * set name of the resource * @@ -48,13 +47,14 @@ public function setName(string $name): self; * get name of the resource * * @return string + * @since 28.0.0 */ public function getName(): string; /** * set share types * - * @param array $shareTypes + * @param string[] $shareTypes * * @return self * @since 28.0.0 @@ -64,7 +64,7 @@ public function setShareTypes(array $shareTypes): self; /** * get share types * - * @return array + * @return string[] * @since 28.0.0 */ public function getShareTypes(): array; @@ -72,7 +72,7 @@ public function getShareTypes(): array; /** * set available protocols * - * @param array $protocols + * @param array $protocols * * @return self * @since 28.0.0 @@ -82,7 +82,7 @@ public function setProtocols(array $protocols): self; /** * get configured protocols * - * @return array + * @return array * @since 28.0.0 */ public function getProtocols(): array;