chore: Address review comments

Signed-off-by: Julius Härtl <jus@bitgrid.net>

apps/dav/lib/Server.php

@@ -220,7 +220,7 @@ public function __construct(IRequest $request, string $baseUri) {
 
 		$this->server->addPlugin(new CopyEtagHeaderPlugin());
 		$this->server->addPlugin(new RequestIdHeaderPlugin(\OC::$server->get(IRequest::class)));
-		$this->server->addPlugin(new ChunkingV2Plugin(\OCP\Server::get(ICacheFactory::class)));
+		$this->server->addPlugin(new ChunkingV2Plugin(\OCP\Server::get(ICacheFactory::class), \OCP\Server::get(LoggerInterface::class)));
 		$this->server->addPlugin(new ChunkingPlugin());
 
 		// allow setup of additional plugins

apps/dav/lib/Upload/ChunkingV2Plugin.php

@@ -2,7 +2,7 @@
 
 declare(strict_types=1);
 /*
- * @copyright Copyright (c) 2021 Julius Härtl <jus@bitgrid.net>
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
  * @author Julius Härtl <jus@bitgrid.net>
  *
@@ -44,25 +44,23 @@
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\Lock\ILockingProvider;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\NotFoundExceptionInterface;
+use Psr\Log\LoggerInterface;
 use Sabre\DAV\Exception\BadRequest;
 use Sabre\DAV\Exception\InsufficientStorage;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\DAV\Exception\PreconditionFailed;
-use Sabre\DAV\ICollection;
-use Sabre\DAV\INode;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
 use Sabre\HTTP\ResponseInterface;
 use Sabre\Uri;
 
 class ChunkingV2Plugin extends ServerPlugin {
-	/** @var Server */
-	private $server;
-	/** @var UploadFolder */
-	private $uploadFolder;
-	/** @var ICache */
-	private $cache;
+	private ?Server $server = null;
+	private ?UploadFolder $uploadFolder;
+	private ICache $cache;
 
 	private ?string $uploadId = null;
 	private ?string $uploadPath = null;
@@ -76,14 +74,14 @@ class ChunkingV2Plugin extends ServerPlugin {
 
 	private const DESTINATION_HEADER = 'Destination';
 
-	public function __construct(ICacheFactory $cacheFactory) {
+	public function __construct(ICacheFactory $cacheFactory, private LoggerInterface $logger) {
 		$this->cache = $cacheFactory->createDistributed(self::CACHE_KEY);
 	}
 
 	/**
 	 * @inheritdoc
 	 */
-	public function initialize(Server $server) {
+	public function initialize(Server $server): void {
 		$server->on('afterMethod:MKCOL', [$this, 'afterMkcol']);
 		$server->on('beforeMethod:PUT', [$this, 'beforePut']);
 		$server->on('beforeMethod:DELETE', [$this, 'beforeDelete']);
@@ -92,12 +90,7 @@ public function initialize(Server $server) {
 		$this->server = $server;
 	}
 
-	/**
-	 * @param string $path
-	 * @param bool $createIfNotExists
-	 * @return FutureFile|UploadFile|ICollection|INode
-	 */
-	private function getUploadFile(string $path, bool $createIfNotExists = false) {
+	private function getUploadFile(string $path, bool $createIfNotExists = false): UploadFile|File {
 		try {
 			$actualFile = $this->server->tree->getNodeForPath($path);
 			// Only directly upload to the target file if it is on the same storage
@@ -126,6 +119,7 @@ public function afterMkcol(RequestInterface $request, ResponseInterface $respons
 			$this->prepareUpload($request->getPath());
 			$this->checkPrerequisites(false);
 		} catch (BadRequest|StorageInvalidException|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
@@ -150,12 +144,14 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 			$this->prepareUpload(dirname($request->getPath()));
 			$this->checkPrerequisites();
 		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
 		[$storage, $storagePath] = $this->getUploadStorage($this->uploadPath);
 
 		$chunkName = basename($request->getPath());
+		// S3 Multipart upload allows up to 10000 chunks at maximum and required them to be numeric
 		$partId = is_numeric($chunkName) ? (int)$chunkName : -1;
 		if (!($partId >= 1 && $partId <= 10000)) {
 			throw new BadRequest('Invalid chunk name, must be numeric between 1 and 10000');
@@ -168,7 +164,7 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 		if ($this->uploadFolder->childExists(self::TEMP_TARGET) && $this->uploadPath) {
 			/** @var UploadFile $tempTargetFile */
 			$tempTargetFile = $this->uploadFolder->getChild(self::TEMP_TARGET);
-			[$destinationDir, $destinationName] = Uri\split($this->uploadPath);
+			[$destinationDir, ] = Uri\split($this->uploadPath);
 			/** @var Directory $destinationParent */
 			$destinationParent = $this->server->tree->getNodeForPath($destinationDir);
 			$free = $destinationParent->getNode()->getFreeSpace();
@@ -190,14 +186,15 @@ public function beforePut(RequestInterface $request, ResponseInterface $response
 		return false;
 	}
 
-	public function beforeMove($sourcePath, $destination): bool {
+	public function beforeMove(string $sourcePath, string $destination): bool {
 		try {
 			$this->prepareUpload(dirname($sourcePath));
 			$this->checkPrerequisites();
-		} catch (StorageInvalidException|BadRequest|NotFound|PreconditionFailed $e) {
+		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
-		[$storage, $storagePath] = $this->getUploadStorage($this->uploadPath);
+		[$storage, ] = $this->getUploadStorage($this->uploadPath);
 
 		$targetFile = $this->getUploadFile($this->uploadPath);
 
@@ -256,11 +253,12 @@ public function beforeMove($sourcePath, $destination): bool {
 		return false;
 	}
 
-	public function beforeDelete(RequestInterface $request, ResponseInterface $response) {
+	public function beforeDelete(RequestInterface $request, ResponseInterface $response): bool {
 		try {
 			$this->prepareUpload(dirname($request->getPath()));
 			$this->checkPrerequisites();
 		} catch (StorageInvalidException|BadRequest|NotFound $e) {
+			$this->logger->debug('Preconditions for chunking v2 not matched, fallback to v1', ['exception' => $e]);
 			return true;
 		}
 
@@ -273,6 +271,8 @@ public function beforeDelete(RequestInterface $request, ResponseInterface $respo
 	 * @throws BadRequest
 	 * @throws PreconditionFailed
 	 * @throws StorageInvalidException
+	 * @throws ContainerExceptionInterface
+	 * @throws NotFoundExceptionInterface
 	 */
 	private function checkPrerequisites(bool $checkUploadMetadata = true): void {
 		$distributedCacheConfig = \OCP\Server::get(IConfig::class)->getSystemValue('memcache.distributed', null);
@@ -283,10 +283,12 @@ private function checkPrerequisites(bool $checkUploadMetadata = true): void {
 		if (!$this->uploadFolder instanceof UploadFolder || empty($this->server->httpRequest->getHeader(self::DESTINATION_HEADER))) {
 			throw new BadRequest('Skipping chunked file writing as the destination header was not passed');
 		}
-		if (!$this->uploadFolder->getStorage()->instanceOfStorage(IChunkedFileWrite::class)) {
+		/** @var ObjectStoreStorage $storage */
+		$storage = $this->uploadFolder->getStorage();
+		if (!$storage->instanceOfStorage(IChunkedFileWrite::class)) {
 			throw new StorageInvalidException('Storage does not support chunked file writing');
 		}
-		if ($this->uploadFolder->getStorage()->instanceOfStorage(ObjectStoreStorage::class) && !$this->uploadFolder->getStorage()->getObjectStore() instanceof IObjectStoreMultiPartUpload) {
+		if ($storage->instanceOfStorage(ObjectStoreStorage::class) && !$storage->getObjectStore() instanceof IObjectStoreMultiPartUpload) {
 			throw new StorageInvalidException('Storage does not support multi part uploads');
 		}
 
@@ -317,8 +319,12 @@ protected function sanitizeMtime(string $mtimeFromRequest): int {
 	/**
 	 * @throws NotFound
 	 */
-	public function prepareUpload($path): void {
-		$this->uploadFolder = $this->server->tree->getNodeForPath($path);
+	public function prepareUpload(string $path): void {
+		$uploadFolder = $this->server->tree->getNodeForPath($path);
+		if (!$uploadFolder instanceof UploadFolder) {
+			throw new \RuntimeException('Unexpected node returned, should be the upload folder');
+		}
+		$this->uploadFolder = $uploadFolder;
 		$uploadMetadata = $this->cache->get($this->uploadFolder->getName());
 		$this->uploadId = $uploadMetadata[self::UPLOAD_ID] ?? null;
 		$this->uploadPath = $uploadMetadata[self::UPLOAD_TARGET_PATH] ?? null;
@@ -346,7 +352,7 @@ private function completeChunkedWrite(string $targetAbsolutePath): void {
 		try {
 			$uploadFileAbsolutePath = Filesystem::getRoot() . $uploadFile->getPath();
 			if ($uploadFileAbsolutePath !== $targetAbsolutePath) {
-				$uploadFile = $rootFolder->get($uploadFile->getFileInfo()->getPath());
+				$uploadFile = $rootFolder->get($uploadFile->getPath());
 				if ($exists) {
 					$uploadFile->copy($targetAbsolutePath);
 				} else {

apps/dav/lib/Upload/FutureFile.php

@@ -65,7 +65,7 @@ public function get() {
 		return AssemblyStream::wrap($nodes);
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/.file';
 	}
 

apps/dav/lib/Upload/PartFile.php

@@ -1,26 +1,28 @@
 <?php
-/**
- * @copyright Copyright (c) 2016, ownCloud, Inc.
+
+declare(strict_types=1);
+/*
+ * @copyright Copyright (c) 2023 Julius Härtl <jus@bitgrid.net>
  *
- * @author Christoph Wurst <christoph@winzerhof-wurst.at>
- * @author Lukas Reschke <lukas@statuscode.ch>
- * @author Thomas Müller <thomas.mueller@tmit.eu>
+ * @author Julius Härtl <jus@bitgrid.net>
  *
- * @license AGPL-3.0
+ * @license GNU AGPL version 3 or any later version
  *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
  *
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
  * GNU Affero General Public License for more details.
  *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see <http://www.gnu.org/licenses/>
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
  *
  */
+
 namespace OCA\DAV\Upload;
 
 use OCA\DAV\Connector\Sabre\Directory;
@@ -32,10 +34,8 @@
  * but handled directly by external storage services like S3 with Multipart Upload
  */
 class PartFile implements IFile {
-	/** @var Directory */
-	private $root;
-	/** @var array */
-	private $partInfo;
+	private Directory $root;
+	private array $partInfo;
 
 	public function __construct(Directory $root, array $partInfo) {
 		$this->root = $root;
@@ -56,7 +56,7 @@ public function get() {
 		throw new Forbidden('Permission denied to get this file');
 	}
 
-	public function getPath() {
+	public function getPath(): string {
 		return $this->root->getFileInfo()->getInternalPath() . '/' . $this->partInfo['PartNumber'];
 	}
 

apps/dav/lib/Upload/UploadFile.php

@@ -44,7 +44,7 @@ public function get() {
 		return $this->file->get();
 	}
 
-	public function getId() {
+	public function getId(): int {
 		return $this->file->getId();
 	}
 
@@ -88,7 +88,7 @@ public function getFile(): File {
 		return $this->file;
 	}
 
-	public function getNode() {
+	public function getNode(): \OCP\Files\File {
 		return $this->file->getNode();
 	}
 }

apps/dav/lib/Upload/UploadFolder.php

@@ -28,16 +28,14 @@
 use OCA\DAV\Connector\Sabre\Directory;
 use OCP\Files\ObjectStore\IObjectStoreMultiPartUpload;
 use OCP\Files\Storage\IStorage;
+use OCP\ICacheFactory;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\ICollection;
 
 class UploadFolder implements ICollection {
-	/** @var Directory */
-	private $node;
-	/** @var CleanupService */
-	private $cleanupService;
-	/** @var IStorage */
-	private $storage;
+	private Directory $node;
+	private CleanupService $cleanupService;
+	private IStorage $storage;
 
 	public function __construct(Directory $node, CleanupService $cleanupService, IStorage $storage) {
 		$this->node = $node;
@@ -83,7 +81,7 @@ public function getChildren() {
 			/** @var ObjectStoreStorage $storage */
 			$objectStore = $this->storage->getObjectStore();
 			if ($objectStore instanceof IObjectStoreMultiPartUpload) {
-				$cache = \OC::$server->getMemCacheFactory()->createDistributed(ChunkingV2Plugin::CACHE_KEY);
+				$cache = \OCP\Server::get(ICacheFactory::class)->createDistributed(ChunkingV2Plugin::CACHE_KEY);
 				$uploadSession = $cache->get($this->getName());
 				if ($uploadSession) {
 					$uploadId = $uploadSession[ChunkingV2Plugin::UPLOAD_ID];
@@ -125,7 +123,7 @@ public function getLastModified() {
 		return $this->node->getLastModified();
 	}
 
-	public function getStorage() {
+	public function getStorage(): IStorage {
 		return $this->storage;
 	}
 }

apps/dav/lib/Upload/UploadHome.php

@@ -28,6 +28,7 @@
 use OC\Files\Filesystem;
 use OC\Files\View;
 use OCA\DAV\Connector\Sabre\Directory;
+use OCP\Files\Storage\IStorage;
 use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\ICollection;
 
@@ -87,13 +88,13 @@ public function getLastModified() {
 	/**
 	 * @return Directory
 	 */
-	private function impl() {
+	private function impl(): Directory {
 		$view = $this->getView();
 		$rootInfo = $view->getFileInfo('');
 		return new Directory($view, $rootInfo);
 	}
 
-	private function getView() {
+	private function getView(): View {
 		$rootView = new View();
 		$user = \OC::$server->getUserSession()->getUser();
 		Filesystem::initMountPoints($user->getUID());
@@ -103,7 +104,7 @@ private function getView() {
 		return new View('/' . $user->getUID() . '/uploads');
 	}
 
-	private function getStorage() {
+	private function getStorage(): IStorage {
 		$view = $this->getView();
 		$storage = $view->getFileInfo('')->getStorage();
 		return $storage;

lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -674,10 +674,6 @@ public function startChunkedWrite(string $targetPath): string {
 		return $this->objectStore->initiateMultipartUpload($urn);
 	}
 
-	/**
-	 *
-	 * @throws GenericFileException
-	 */
 	public function putChunkedWritePart(
 		string $targetPath,
 		string $writeToken,
@@ -688,6 +684,11 @@ public function putChunkedWritePart(
 		if (!$this->objectStore instanceof IObjectStoreMultiPartUpload) {
 			throw new GenericFileException('Object store does not support multipart upload');
 		}
+
+		if (!is_numeric($chunkId)) {
+			throw new GenericFileException('Chunk ID must be numeric for S3 multipart upload');
+		}
+
 		$cacheEntry = $this->getCache()->get($targetPath);
 		$urn = $this->getURN($cacheEntry->getId());