requested changes

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>

apps/cloud_federation_api/lib/Capabilities.php

@@ -31,51 +31,57 @@
 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 {
 
 	public const API_VERSION = '1.0-proposal1';
 
 	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');
 
 		$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()];
 	}
 }

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) {

build/psalm-baseline.xml

@@ -43,6 +43,11 @@
       <code>IEventListener</code>
     </MissingTemplateParam>
   </file>
+  <file src="apps/cloud_federation_api/lib/Capabilities.php">
+    <LessSpecificImplementedReturnType>
+      <code>array</code>
+    </LessSpecificImplementedReturnType>
+  </file>
   <file src="apps/comments/lib/Listener/CommentsEntityEventListener.php">
     <MissingTemplateParam>
       <code>IEventListener</code>

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
 			);
 		}
 	}

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,

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(),

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<string, string> */
 	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<string, string> $protocols
 	 *
 	 * @return $this
 	 */
@@ -85,7 +87,7 @@ public function setProtocols(array $protocols): self {
 	}
 
 	/**
-	 * @return array
+	 * @return array<string, string>
 	 */
 	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'] ?? []);
 	}

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;

lib/public/OCM/Exceptions/OCMArgumentException.php

@@ -27,5 +27,8 @@
 
 use Exception;
 
+/**
+ * @since 28.0.0
+ */
 class OCMArgumentException extends Exception {
 }