feat: handle post_max_size and upload_max_filesize

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Log the project folder setup and terms of service signed for the OpenProject user to the audit log
 - Log users connecting and disconnecting their OpenProject account to the audit log
 - Log files linked to and unlinked from OpenProject work packages to the audit log
+- Handle exceeding post_max_size and upload_max_filesize [#1190](https://github.com/nextcloud/integration_openproject/pull/1190)
 
 ### Fixed
 - Keep the admin settings when Nextcloud replays the migrations to check the database schema (`occ db:schema:check`, every `occ upgrade` on Nextcloud 35)

lib/Controller/DirectUploadController.php

@@ -8,10 +8,8 @@
 namespace OCA\OpenProject\Controller;
 
 use \OCP\AppFramework\ApiController;
-use DateTime;
 use InvalidArgumentException;
 use OC\Files\Filesystem;
-use OC\Files\Node\Folder;
 use OC\ForbiddenException;
 use OC\User\NoUserException;
 use OCA\OpenProject\Exception\OpenprojectFileNotUploadedException;
@@ -35,6 +33,7 @@
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\Lock\LockedException;
+use OCP\Util;
 use Sabre\DAV\Exception\Conflict;
 
 class DirectUploadController extends ApiController {
@@ -78,6 +77,17 @@ class DirectUploadController extends ApiController {
 
 	private $l;
 
+	private const PHP_FILE_UPLOAD_ERRORS = [
+		UPLOAD_ERR_OK => 'The file was uploaded',
+		UPLOAD_ERR_INI_SIZE => 'The uploaded file exceeds the upload_max_filesize directive in php.ini',
+		UPLOAD_ERR_FORM_SIZE => 'The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form',
+		UPLOAD_ERR_PARTIAL => 'The file was only partially uploaded',
+		UPLOAD_ERR_NO_FILE => 'No file was uploaded',
+		UPLOAD_ERR_NO_TMP_DIR => 'Missing a temporary folder',
+		UPLOAD_ERR_CANT_WRITE => 'Could not write file to disk',
+		UPLOAD_ERR_EXTENSION => 'A PHP extension stopped the file upload',
+	];
+
 	public function __construct(
 		string $appName,
 		IRequest $request,
@@ -154,93 +164,46 @@ public function directUpload(string $token):DataResponse {
 		try {
 			if (strlen($token) !== 64 || !preg_match('/^[a-zA-Z0-9]*/', $token)) {
 				$this->databaseService->deleteToken($token);
-				throw new NotFoundException('invalid token');
+				throw new OpenprojectUnauthorizedUserException('invalid token');
 			}
-			$tokenInfo = $this->directUploadService->getTokenInfo($token);
+
+			$postMaxSizeInBytes = (int)Util::computerFileSize(ini_get('post_max_size'));
+			$contentLength = (int)$this->request->getHeader('Content-Length');
+			if ($postMaxSizeInBytes > 0 && $contentLength > $postMaxSizeInBytes) {
+				throw new OpenprojectFileNotUploadedException(
+					'File was not uploaded. The request exceeds the maximum post size.',
+				);
+			}
+
 			$directUploadFile = $this->request->getUploadedFile('file');
 			if (empty($directUploadFile)) {
-				throw new OpenprojectFileNotUploadedException(
-					'File was not uploaded. post_max_size exceeded?'
+				throw new \InvalidArgumentException(
+					'No file is present to upload.'
 				);
 			}
-			$fileName = trim($directUploadFile['name']);
-			$this->scanForInvalidCharacters($fileName, "\\/");
-			if (empty($directUploadFile['tmp_name']) || $directUploadFile['error'] === 1) {
+
+			if (array_key_exists('error', $directUploadFile) && $directUploadFile['error'] !== UPLOAD_ERR_OK) {
 				throw new OpenprojectFileNotUploadedException(
-					'File was not uploaded. upload_max_filesize exceeded?'
+					self::PHP_FILE_UPLOAD_ERRORS[$directUploadFile['error']],
 				);
 			}
-			$tmpPath = $directUploadFile['tmp_name'];
+
+			$fileName = trim($directUploadFile['name']);
+			$this->scanForInvalidCharacters($fileName, "\\/");
 			if (Filesystem::isFileBlacklisted($fileName)) {
 				throw new ForbiddenException('invalid file name');
 			}
+
 			$overwrite = $this->request->getParam('overwrite');
 			if (isset($overwrite)) {
 				$acceptedOverwriteValues = ['true','false'];
 				$overwrite = strtolower($overwrite);
-				if (in_array($overwrite, $acceptedOverwriteValues)) {
-					$overwrite = $overwrite === 'true';
-				} else {
+				if (!in_array($overwrite, $acceptedOverwriteValues)) {
 					throw new \InvalidArgumentException('invalid overwrite value');
 				}
-			} else {
-				$overwrite = null;
-			}
-			$user = $this->userManager->get($tokenInfo['user_id']);
-			$this->userSession->setUser($user);
-			$userFolder = $this->rootFolder->getUserFolder($user->getUID());
-			$nodes = $userFolder->getById($tokenInfo['folder_id']);
-			if (empty($nodes)) {
-				throw new NotFoundException('folder not found or not enough permissions');
-			}
-			/**
-			 * @var Folder $folderNode
-			 */
-			$folderNode = array_shift($nodes);
-			if (!$folderNode->isCreatable() && !$overwrite) {
-				throw new ForbiddenException('not enough permissions');
-			}
-			$freeSpace = $folderNode->getFreeSpace();
-
-			// this is also true if we try to overwrite
-			// to overwrite a file we need enough free quota for the new data
-			// otherwise `putContent()` fails,
-			// if freeSpace is smaller than 0 then treat it as a unlimited space
-			// and don't throw an error
-			if ($directUploadFile['size'] > $freeSpace && $freeSpace >= 0) {
-				throw new NotEnoughSpaceException('insufficient quota');
+				$overwrite = $overwrite === 'true';
 			}
-			if ($folderNode->nodeExists($fileName) && $overwrite) {
-				/**
-				 * @var File $file
-				 */
-				$file = $folderNode->get($fileName);
-				if ($file->getType() === FileInfo::TYPE_FOLDER) {
-					throw new Conflict('overwrite is not allowed on non-files');
-				}
-				if (!$file->isUpdateable()) {
-					throw new ForbiddenException('not enough permissions');
-				}
-				// overwrite the file
-				$file->putContent(fopen($tmpPath, 'r'));
-				$fileId = $file->getId();
-				return new DataResponse([
-					'file_name' => $fileName,
-					'file_id' => $fileId
-				], Http::STATUS_OK);
-			} elseif ($folderNode->nodeExists($fileName) && $overwrite === false) {
-				// get unique name for duplicate file with number suffix
-				$fileName = $folderNode->getNonExistingName($fileName);
-			} elseif ($folderNode->nodeExists($fileName)) {
-				throw new Conflict('conflict, file name already exists');
-			}
-			$fileInfo = $folderNode->newFile($fileName, fopen($tmpPath, 'r'));
-			$fileId = $fileInfo->getId();
-			// setting the creation time for the uploaded file
-			$creationTime = (new DateTime())->getTimestamp();
-			$fileInfo->getStorage()->getCache()->update($fileId, [
-				'creation_time' => $creationTime
-			]);
+			$result = $this->directUploadService->uploadFile($token, $directUploadFile, $overwrite);
 		} catch (OpenprojectUnauthorizedUserException $e) {
 			return new DataResponse([
 				'error' => $this->l->t($e->getMessage())
@@ -269,8 +232,7 @@ public function directUpload(string $token):DataResponse {
 			], Http::STATUS_INSUFFICIENT_STORAGE);
 		} catch (OpenprojectFileNotUploadedException $e) {
 			return new DataResponse([
-				'error' => $this->l->t($e->getMessage()),
-				'upload_limit' => \OCP\Util::uploadLimit()
+				'error' => $this->l->t($e->getMessage())
 			], Http::STATUS_REQUEST_ENTITY_TOO_LARGE);
 		} catch (InvalidContentException $e) { // files_antivirus throws this exception
 			return new DataResponse([
@@ -285,10 +247,11 @@ public function directUpload(string $token):DataResponse {
 				'error' => $this->l->t($e->getMessage())
 			], Http::STATUS_INTERNAL_SERVER_ERROR);
 		}
+		$statusCode = $overwrite ? Http::STATUS_OK : Http::STATUS_CREATED;
 		return new DataResponse([
-			'file_name' => $fileName,
-			'file_id' => $fileId
-		], Http::STATUS_CREATED);
+			'file_name' => $result['file_name'],
+			'file_id' => $result['file_id'],
+		], $statusCode);
 	}
 
 	/**

lib/Service/DirectUploadService.php

@@ -8,12 +8,20 @@
 namespace OCA\OpenProject\Service;
 
 use DateTime;
+use OC\Files\Node\Folder;
+use OC\ForbiddenException;
 use OCA\OpenProject\Exception\OpenprojectUnauthorizedUserException;
 use OCP\DB\Exception;
+use OCP\Files\File;
+use OCP\Files\FileInfo;
+use OCP\Files\IRootFolder;
+use OCP\Files\NotEnoughSpaceException;
 use OCP\Files\NotFoundException;
 use OCP\IL10N;
 use OCP\IUserManager;
+use OCP\IUserSession;
 use OCP\Security\ISecureRandom;
+use Sabre\DAV\Exception\Conflict;
 
 class DirectUploadService {
 
@@ -40,16 +48,30 @@ class DirectUploadService {
 	/** @var string time of token expiration */
 	private string $expiryTime = '+1 hour';
 
+	/**
+	 * @var IUserSession
+	 */
+	private IUserSession $userSession;
+
+	/**
+	 * @var IRootFolder
+	 */
+	private IRootFolder $rootFolder;
+
 	public function __construct(
-		IUserManager $userManager,
-		IL10N $l,
-		ISecureRandom $secureRandom,
-		DatabaseService $databaseService
+		IUserManager    $userManager,
+		IL10N           $l,
+		ISecureRandom   $secureRandom,
+		DatabaseService $databaseService,
+		IUserSession $userSession,
+		IRootFolder $rootFolder,
 	) {
 		$this->l = $l;
 		$this->userManager = $userManager;
 		$this->secureRandom = $secureRandom;
 		$this->databaseService = $databaseService;
+		$this->userSession = $userSession;
+		$this->rootFolder = $rootFolder;
 	}
 
 	/**
@@ -100,4 +122,83 @@ public function getTokenInfo(string $token): ?array {
 		}
 		return $tokenInfo;
 	}
+
+	/**
+	 *
+	 * uploads a file
+	 *
+	 * @param string $token
+	 * @param mixed $directUploadFile
+	 * @param bool|null $overwrite
+	 *
+	 * @return array{file_id: int, file_name: string}
+	 *
+	 * @throws NotFoundException
+	 * @throws ForbiddenException
+	 * @throws NotEnoughSpaceException
+	 * @throws Conflict
+	 */
+	public function uploadFile(string $token, mixed $directUploadFile, ?bool $overwrite) : array {
+		$tokenInfo = $this->getTokenInfo($token);
+		$user = $this->userManager->get($tokenInfo['user_id']);
+		$this->userSession->setUser($user);
+		$userFolder = $this->rootFolder->getUserFolder($user->getUID());
+		$nodes = $userFolder->getById($tokenInfo['folder_id']);
+		if (empty($nodes)) {
+			throw new NotFoundException('folder not found or not enough permissions');
+		}
+		/**
+		 * @var Folder $folderNode
+		 */
+		$folderNode = array_shift($nodes);
+		if (!$folderNode->isCreatable() && !$overwrite) {
+			throw new ForbiddenException('not enough permissions');
+		}
+		$freeSpace = $folderNode->getFreeSpace();
+
+		// this is also true if we try to overwrite
+		// to overwrite a file we need enough free quota for the new data
+		// otherwise `putContent()` fails,
+		// if freeSpace is smaller than 0 then treat it as a unlimited space
+		// and don't throw an error
+		if ($directUploadFile['size'] > $freeSpace && $freeSpace >= 0) {
+			throw new NotEnoughSpaceException('insufficient quota');
+		}
+		$fileName = trim($directUploadFile['name']);
+		$tmpPath = $directUploadFile['tmp_name'];
+		if ($folderNode->nodeExists($fileName) && $overwrite) {
+			/**
+			 * @var File $file
+			 */
+			$file = $folderNode->get($fileName);
+			if ($file->getType() === FileInfo::TYPE_FOLDER) {
+				throw new Conflict('overwrite is not allowed on non-files');
+			}
+			if (!$file->isUpdateable()) {
+				throw new ForbiddenException('not enough permissions');
+			}
+			// overwrite the file
+			$file->putContent(fopen($tmpPath, 'r'));
+			return [
+				'file_id' => $file->getId(),
+				'file_name' => $fileName,
+			];
+		} elseif ($folderNode->nodeExists($fileName) && $overwrite === false) {
+			// get unique name for duplicate file with number suffix
+			$fileName = $folderNode->getNonExistingName($fileName);
+		} elseif ($folderNode->nodeExists($fileName)) {
+			throw new Conflict('conflict, file name already exists');
+		}
+		$fileInfo = $folderNode->newFile($fileName, fopen($tmpPath, 'r'));
+		$fileId = $fileInfo->getId();
+		// setting the creation time for the uploaded file
+		$creationTime = (new DateTime())->getTimestamp();
+		$fileInfo->getStorage()->getCache()->update($fileId, [
+			'creation_time' => $creationTime
+		]);
+		return [
+			'file_id' => $fileId,
+			'file_name' => $fileName,
+		];
+	}
 }

tests/acceptance/features/api/directUpload.feature

@@ -70,18 +70,17 @@ Feature: API endpoint for direct upload
     When an anonymous user sends a multipart form data POST request to the "direct-upload/%last-created-direct-upload-token%" endpoint with:
       | file_name | ""          |
       | data      | some data   |
-    Then the HTTP status code should be "413"
+	# An empty file name means no file is received, so this is a bad request (400), not content too large (413)
+    Then the HTTP status code should be "400"
     And the data of the response should match
     """"
     {
     "type": "object",
     "required": [
-        "error",
-        "upload_limit"
+        "error"
       ],
       "properties": {
-          "error": {"type": "string", "pattern": "^File was not uploaded\\. post_max_size exceeded\\?$"},
-          "upload_limit": {"type": "integer"}
+          "error": {"type": "string", "pattern": "No file is present to upload."}
       }
     }
     """
@@ -260,20 +259,22 @@ Feature: API endpoint for direct upload
     When an anonymous user sends a multipart form data POST request to the "direct-upload/%last-created-direct-upload-token%" endpoint with:
       | file_name | ""        |
       | data      | some data |
-    Then the HTTP status code should be "413"
+    Then the HTTP status code should be "400"
     When an anonymous user sends a multipart form data POST request to the "direct-upload/%last-created-direct-upload-token%" endpoint with:
       | file_name | file.txt  |
       | data      | some data |
-    Then the HTTP status code should be "401"
+    Then the HTTP status code should be "201"
     And the data of the response should match
     """"
     {
     "type": "object",
     "required": [
-        "error"
+        "file_name",
+        "file_id"
       ],
       "properties": {
-          "error": {"type": "string", "pattern": "^unauthorized$"}
+          "file_name": {"type": "string", "pattern": "^file\\.txt$"},
+          "file_id": {"type" : "integer"}
       }
     }
     """