From 53987b23f842e3c1d31618438975793776651f36 Mon Sep 17 00:00:00 2001 From: Ashim Shrestha Date: Wed, 7 Oct 2026 09:43:13 +0545 Subject: [PATCH 1/5] feat: handle post_max_size and upload_max_filesize Signed-off-by: Ashim Shrestha --- lib/Controller/DirectUploadController.php | 53 +++++++---- .../features/api/directUpload.feature | 18 ++-- .../Controller/DirectUploadControllerTest.php | 91 +++++++++++++++---- 3 files changed, 120 insertions(+), 42 deletions(-) diff --git a/lib/Controller/DirectUploadController.php b/lib/Controller/DirectUploadController.php index d0c7bef49..02d377c34 100644 --- a/lib/Controller/DirectUploadController.php +++ b/lib/Controller/DirectUploadController.php @@ -154,38 +154,60 @@ 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); + + $postMaxSize = ini_get('post_max_size'); + $postMaxSizeInBytes = (int)\OCP\Util::computerFileSize($postMaxSize); + $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) { + + $phpFileUploadErrors = [ + 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', + ]; + + if (array_key_exists('error', $directUploadFile) && $directUploadFile['error'] !== UPLOAD_ERR_OK) { throw new OpenprojectFileNotUploadedException( - 'File was not uploaded. upload_max_filesize exceeded?' + $phpFileUploadErrors[$directUploadFile['error']], ); } - $tmpPath = $directUploadFile['tmp_name']; + + $fileName = trim($directUploadFile['name']); + $this->scanForInvalidCharacters($fileName, "\\/"); if (Filesystem::isFileBlacklisted($fileName)) { throw new ForbiddenException('invalid file name'); } + $tmpPath = $directUploadFile['tmp_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; + $overwrite = $overwrite === 'true'; } + + $tokenInfo = $this->directUploadService->getTokenInfo($token); $user = $this->userManager->get($tokenInfo['user_id']); $this->userSession->setUser($user); $userFolder = $this->rootFolder->getUserFolder($user->getUID()); @@ -269,8 +291,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([ diff --git a/tests/acceptance/features/api/directUpload.feature b/tests/acceptance/features/api/directUpload.feature index 78233d8da..1449cf95b 100644 --- a/tests/acceptance/features/api/directUpload.feature +++ b/tests/acceptance/features/api/directUpload.feature @@ -70,18 +70,16 @@ 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" 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 +258,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"} } } """ diff --git a/tests/lib/Controller/DirectUploadControllerTest.php b/tests/lib/Controller/DirectUploadControllerTest.php index c0c25701e..998b8cf22 100644 --- a/tests/lib/Controller/DirectUploadControllerTest.php +++ b/tests/lib/Controller/DirectUploadControllerTest.php @@ -22,6 +22,7 @@ class DirectUploadControllerTest extends TestCase { * @var IL10N */ private $l; + private const TOKEN = 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3'; /** * @return mixed @@ -43,7 +44,7 @@ public function testprepareDirectUpload() { $result = $directUploadController->prepareDirectUpload(123); assertSame( [ - 'token' => 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3', + 'token' => self::TOKEN, 'expires_on' => 1671537939 ], $result->getData() @@ -113,7 +114,7 @@ public function testDirectUploadInvalidToken(string $token):void { ], $result->getData() ); - assertSame(404, $result->getStatus()); + assertSame(401, $result->getStatus()); } public function testDirectUploadNotEnoughSpace():void { @@ -126,7 +127,7 @@ public function testDirectUploadNotEnoughSpace():void { $userFolderMock, 101 ); $result = $directUploadController->directUpload( - 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3' + self::TOKEN, ); assertSame( [ @@ -142,34 +143,81 @@ public function testDirectUploadNotEnoughSpace():void { */ public function fileNotUploadedDataProvider() { return [ - ['', 1], - ['some name', 1], - ['', 0], + [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'], ]; } /** - * @param string $tmpName * @param int $error + * @param string $expectedErrorMessage * @return void * @dataProvider fileNotUploadedDataProvider */ - public function testDirectUploadFileNotUploaded(string $tmpName, int $error):void { + public function testDirectUploadFileNotUploaded(int $error, string $expectedErrorMessage):void { + $nodeMock = $this->getNodeMock('folder'); + + $userFolderMock = $this->getFolderMock(); + $userFolderMock->method('getById')->willReturn($nodeMock); + $directUploadController = $this->createDirectUploadController( + $userFolderMock, 100, '', $error + ); + $result = $directUploadController->directUpload( + self::TOKEN, + ); + $resultArray = $result->getData(); + assertSame( + $expectedErrorMessage, + $resultArray['error'] + ); + assertSame(413, $result->getStatus()); + } + + /** + * @return void + */ + public function testDirectUploadNoFile():void { $nodeMock = $this->getNodeMock('folder'); $userFolderMock = $this->getFolderMock(); $userFolderMock->method('getById')->willReturn($nodeMock); $directUploadController = $this->createDirectUploadController( - $userFolderMock, 100, $tmpName, $error + folderMock: $userFolderMock, noUploadedFile: true ); $result = $directUploadController->directUpload( - 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3' + self::TOKEN, ); $resultArray = $result->getData(); assertSame( - 'File was not uploaded. upload_max_filesize exceeded?', + 'No file is present to upload.', + $resultArray['error'] + ); + assertSame(400, $result->getStatus()); + } + + /** + * @return void + */ + public function testDirectUploadPostMaxSizeExceeded():void { + $nodeMock = $this->getNodeMock('folder'); + + $userFolderMock = $this->getFolderMock(); + $userFolderMock->method('getById')->willReturn($nodeMock); + $directUploadController = $this->createDirectUploadController( + folderMock: $userFolderMock, contentLength: (string)PHP_INT_MAX + ); + $result = $directUploadController->directUpload( + self::TOKEN, + ); + $resultArray = $result->getData(); + assertSame( + 'File was not uploaded. The request exceeds the maximum post size.', $resultArray['error'] ); - self::assertIsNumeric($resultArray['upload_limit']); assertSame(413, $result->getStatus()); } @@ -200,7 +248,7 @@ public function testDirectUploadException( $directUploadController = $this->createDirectUploadController( $userFolderMock, 0, $tmpFileName); $result = $directUploadController->directUpload( - 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3' + self::TOKEN, ); $resultArray = $result->getData(); assertSame( @@ -230,7 +278,7 @@ public function testNegativeFreeSpace(): void { $userFolderMock, 101, $tmpFileName ); $result = $directUploadController->directUpload( - 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3' + self::TOKEN, ); $resultArray = $result->getData(); assertSame( @@ -248,13 +296,17 @@ public function testNegativeFreeSpace(): void { * @param int $uploadedFileSize * @param string $uploadedFileTmpName * @param int $uploadedFileError + * @param string $contentLength + * @param bool $noUploadedFile * @return DirectUploadController */ private function createDirectUploadController( mixed $folderMock, int $uploadedFileSize = 9999, string $uploadedFileTmpName = '/tmp/andjashd', - int $uploadedFileError = 0 + int $uploadedFileError = 0, + string $contentLength = '100', + bool $noUploadedFile = false ): DirectUploadController { $storageMock = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock(); $storageMock->method('getUserFolder')->willReturn($folderMock); @@ -271,7 +323,7 @@ private function createDirectUploadController( $directUploadServiceMock->method('getTokenForDirectUpload') ->willReturn([ - 'token' => 'WampxL5Z97CndGwB7qLPfotosDT5mXk7oFyGLa64nmY35ANtkzT7zDQwYyXrbdC3', + 'token' => self::TOKEN, 'expires_on' => 1671537939 ]); @@ -293,8 +345,13 @@ private function createDirectUploadController( $userManagerMock->method('get')->willReturn($userMock); $requestMock = $this->getMockBuilder(IRequest::class)->disableOriginalConstructor()->getMock(); + $requestMock->method('getHeader')->willReturnCallback( + function (string $name) use ($contentLength) { + return $name === 'Content-Length' ? $contentLength : ''; + } + ); - $requestMock->method('getUploadedFile')->willReturn([ + $requestMock->method('getUploadedFile')->willReturn($noUploadedFile ? null : [ 'name' => 'file.txt', 'tmp_name' => $uploadedFileTmpName, 'size' => $uploadedFileSize, From a8049bac203f9166e597398d1b19b917f5f76545 Mon Sep 17 00:00:00 2001 From: Ashim Shrestha Date: Mon, 12 Oct 2026 12:24:54 +0545 Subject: [PATCH 2/5] refactor: move file upload functionality to service from controller Signed-off-by: Ashim Shrestha --- lib/Controller/DirectUploadController.php | 68 +-------- lib/Service/DirectUploadService.php | 96 +++++++++++- .../Controller/DirectUploadControllerTest.php | 117 ++------------ tests/lib/Service/DirectUploadServiceTest.php | 143 +++++++++++++++++- 4 files changed, 249 insertions(+), 175 deletions(-) diff --git a/lib/Controller/DirectUploadController.php b/lib/Controller/DirectUploadController.php index 02d377c34..23a751255 100644 --- a/lib/Controller/DirectUploadController.php +++ b/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; @@ -195,7 +193,6 @@ public function directUpload(string $token):DataResponse { if (Filesystem::isFileBlacklisted($fileName)) { throw new ForbiddenException('invalid file name'); } - $tmpPath = $directUploadFile['tmp_name']; $overwrite = $this->request->getParam('overwrite'); if (isset($overwrite)) { @@ -206,63 +203,7 @@ public function directUpload(string $token):DataResponse { } $overwrite = $overwrite === 'true'; } - - $tokenInfo = $this->directUploadService->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'); - } - 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()) @@ -306,10 +247,11 @@ public function directUpload(string $token):DataResponse { 'error' => $this->l->t($e->getMessage()) ], Http::STATUS_INTERNAL_SERVER_ERROR); } + $statusCode = $result['overwritten'] ? 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); } /** diff --git a/lib/Service/DirectUploadService.php b/lib/Service/DirectUploadService.php index fb319f52e..32d9434e8 100644 --- a/lib/Service/DirectUploadService.php +++ b/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,70 @@ public function getTokenInfo(string $token): ?array { } return $tokenInfo; } + + 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, + 'overwritten' => true, + ]; + } 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, + 'overwritten' => false, + ]; + } } diff --git a/tests/lib/Controller/DirectUploadControllerTest.php b/tests/lib/Controller/DirectUploadControllerTest.php index 998b8cf22..b62ec6351 100644 --- a/tests/lib/Controller/DirectUploadControllerTest.php +++ b/tests/lib/Controller/DirectUploadControllerTest.php @@ -7,12 +7,16 @@ namespace OCA\OpenProject\Controller; +use OCA\OpenProject\Service\DatabaseService; +use OCA\OpenProject\Service\DirectUploadService; use OCP\Files\Folder; -use OCP\Files\ForbiddenException as FileAccessForbiddenException; -use OCP\Files\InvalidContentException; +use OCP\Files\IRootFolder; use OCP\Files\IUserFolder; use OCP\IL10N; use OCP\IRequest; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; use PHPUnit\Framework\TestCase; use function PHPUnit\Framework\assertSame; @@ -117,27 +121,6 @@ public function testDirectUploadInvalidToken(string $token):void { assertSame(401, $result->getStatus()); } - public function testDirectUploadNotEnoughSpace():void { - $nodeMock = $this->getNodeMock('folder'); - $nodeMock[0]->method('getFreeSpace')->willReturn(100); - - $userFolderMock = $this->getFolderMock(); - $userFolderMock->method('getById')->willReturn($nodeMock); - $directUploadController = $this->createDirectUploadController( - $userFolderMock, 101 - ); - $result = $directUploadController->directUpload( - self::TOKEN, - ); - assertSame( - [ - 'error' => 'insufficient quota' - ], - $result->getData() - ); - assertSame(507, $result->getStatus()); - } - /** * @return array> */ @@ -221,75 +204,6 @@ public function testDirectUploadPostMaxSizeExceeded():void { assertSame(413, $result->getStatus()); } - /** - * @return array> - */ - public function newFileExceptionsDataProvider() { - return [ - [new InvalidContentException('Virus detected'), 'Virus detected', 415], - [new FileAccessForbiddenException('Access denied by the access control', false), 'Access denied by the access control', 403], - [new \Exception('could not upload'), 'could not upload', 500], - ]; - } - - /** - * @return void - * @dataProvider newFileExceptionsDataProvider - */ - public function testDirectUploadException( - \Exception $exception, string $expectedErrorMessage, int $expectedStatusCode - ):void { - $nodeMock = $this->getNodeMock('folder'); - $tmpFileName = '/tmp/integration_openproject_unit_test'; - touch($tmpFileName); - $nodeMock[0]->method('newFile')->will($this->throwException($exception)); - $userFolderMock = $this->getFolderMock(); - $userFolderMock->method('getById')->willReturn($nodeMock); - $directUploadController = $this->createDirectUploadController( - $userFolderMock, 0, $tmpFileName); - $result = $directUploadController->directUpload( - self::TOKEN, - ); - $resultArray = $result->getData(); - assertSame( - $expectedErrorMessage, - $resultArray['error'] - ); - assertSame($expectedStatusCode, $result->getStatus()); - } - - public function testNegativeFreeSpace(): void { - $cacheMock = $this->getMockBuilder('\OCP\Files\Cache\ICache')->getMock(); - $cacheMock->method('update')->willReturn(true); - $storageMock = $this->getMockBuilder('\OCP\Files\Storage\IStorage')->disableOriginalConstructor()->getMock(); - $storageMock->method('getCache')->willReturn($cacheMock); - - $fileMock = $this->getMockBuilder('\OC\Files\Node\File')->disableOriginalConstructor()->getMock(); - $fileMock->method('getId')->willReturn(123); - $fileMock->method('getStorage')->willReturn($storageMock); - $nodeMock = $this->getNodeMock('folder'); - $tmpFileName = '/tmp/integration_openproject_unit_test'; - touch($tmpFileName); - $nodeMock[0]->method('getFreeSpace')->willReturn(-3); - $nodeMock[0]->method('newFile')->willReturn($fileMock); - $userFolderMock = $this->getFolderMock(); - $userFolderMock->method('getById')->willReturn($nodeMock); - $directUploadController = $this->createDirectUploadController( - $userFolderMock, 101, $tmpFileName - ); - $result = $directUploadController->directUpload( - self::TOKEN, - ); - $resultArray = $result->getData(); - assertSame( - [ - 'file_name' => 'file.txt', - 'file_id' => 123 - ], - $resultArray - ); - } - /** * @param mixed $folderMock @@ -306,20 +220,19 @@ private function createDirectUploadController( string $uploadedFileTmpName = '/tmp/andjashd', int $uploadedFileError = 0, string $contentLength = '100', - bool $noUploadedFile = false + bool $noUploadedFile = false, ): DirectUploadController { - $storageMock = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock(); + $storageMock = $this->createMock(IRootFolder::class); $storageMock->method('getUserFolder')->willReturn($folderMock); - $userMock = $this->getMockBuilder('\OCP\IUser')->getMock(); + $userMock = $this->createMock(IUser::class); $userMock->method('getUID')->willReturn('testUser'); - $userSessionMock = $this->getMockBuilder('\OCP\IUserSession')->getMock(); + $userSessionMock = $this->createMock(IUserSession::class); $userSessionMock->method('getUser')->willReturn($userMock); - $directUploadServiceMock = $this->getMockBuilder( - 'OCA\OpenProject\Service\DirectUploadService' - )->disableOriginalConstructor()->getMock(); + $directUploadServiceMock = $this->createMock( + DirectUploadService::class); $directUploadServiceMock->method('getTokenForDirectUpload') ->willReturn([ @@ -341,10 +254,10 @@ private function createDirectUploadController( 'folder_id' => 123 ] ); - $userManagerMock = $this->getMockBuilder('OCP\IUserManager')->disableOriginalConstructor()->getMock(); + $userManagerMock = $this->createMock(IUserManager::class); $userManagerMock->method('get')->willReturn($userMock); - $requestMock = $this->getMockBuilder(IRequest::class)->disableOriginalConstructor()->getMock(); + $requestMock = $this->createMock(IRequest::class); $requestMock->method('getHeader')->willReturnCallback( function (string $name) use ($contentLength) { return $name === 'Content-Length' ? $contentLength : ''; @@ -364,7 +277,7 @@ function (string $name) use ($contentLength) { $userSessionMock, $userManagerMock, $directUploadServiceMock, - $this->getMockBuilder('OCA\OpenProject\Service\DatabaseService')->disableOriginalConstructor()->getMock(), + $this->createMock(DatabaseService::class), $this->l, 'testUser', ); diff --git a/tests/lib/Service/DirectUploadServiceTest.php b/tests/lib/Service/DirectUploadServiceTest.php index 630b784cf..18b80716a 100644 --- a/tests/lib/Service/DirectUploadServiceTest.php +++ b/tests/lib/Service/DirectUploadServiceTest.php @@ -7,34 +7,65 @@ namespace OCA\OpenProject\Service; +use OC\Files\Node\File; +use OC\Files\Node\Folder; +use OCP\Files\Cache\ICache; +use OCP\Files\ForbiddenException as FileAccessForbiddenException; +use OCP\Files\InvalidContentException; +use OCP\Files\IRootFolder; +use OCP\Files\IUserFolder; +use OCP\Files\NotEnoughSpaceException; use OCP\Files\NotFoundException; +use OCP\Files\Storage\IStorage; use OCP\IL10N; use OCP\IUser; use OCP\IUserManager; +use OCP\IUserSession; use OCP\Security\ISecureRandom; use PHPUnit\Framework\TestCase; +use function PHPUnit\Framework\assertSame; class DirectUploadServiceTest extends TestCase { - protected function getDirectUploadService(int $expiresOn): DirectUploadService { - $databaseServiceMock = $this->getMockBuilder(DatabaseService::class)->disableOriginalConstructor()->getMock(); + + private const TOKEN = 'ziPpdeFW4qoTg7AzEc4E9bnREkF97f5B2q65M4t3iex58E7ENZK4GomwEZCPjeNa'; + private const TMP_NAME = '/tmp/integration_openproject_unit_test'; + + /** + * @return mixed + */ + public function getFolderMock(): mixed { + if (interface_exists(IUserFolder::class)) { + return $this->createMock(IUserFolder::class); + } + return $this->createMock(Folder::class); + } + + protected function getDirectUploadService(int $expiresOn, mixed $userFolderMock = null): DirectUploadService { + $databaseServiceMock = $this->createMock(DatabaseService::class); $databaseServiceMock->method('getTokenInfoFromDB')->willReturn([ 'user_id' => 'testUser', 'expires_on' => $expiresOn, 'folder_id' => '123' ]); - $userMock = $this->getMockBuilder(IUser::class)->disableOriginalConstructor()->getMock(); + $userMock = $this->createMock(IUser::class); $userMock->method('isEnabled')->willReturn(true); - $userManagerMock = $this->getMockBuilder(IUserManager::class)->disableOriginalConstructor()->getMock(); + $userManagerMock = $this->createMock(IUserManager::class); $userManagerMock->method('userExists')->willReturn(true); $userManagerMock->method('get')->willReturn($userMock); + $userSessionMock = $this->createMock(IUserSession::class); + $userSessionMock->method('getUser')->willReturn($userMock); + $storageMock = $this->createMock(IRootFolder::class); + $storageMock->method('getUserFolder')->willReturn($userFolderMock ?? $this->getFolderMock()); return new DirectUploadService( $userManagerMock, $this->createMock(IL10N::class), $this->createMock(ISecureRandom::class), - $databaseServiceMock + $databaseServiceMock, + $userSessionMock, + $storageMock ); } @@ -47,6 +78,106 @@ public function testGetTokenInfoExpiredTime() { //send an already expired time $direcUploadService = $this->getDirectUploadService(1671537979); $this->expectException(NotFoundException::class); - $direcUploadService->getTokenInfo('ziPpdeFW4qoTg7AzEc4E9bnREkF97f5B2q65M4t3iex58E7ENZK4GomwEZCPjeNa'); + $direcUploadService->getTokenInfo(self::TOKEN); + } + + /** + * + * @param string $type + * @param int $id + * @return array + */ + private function getNodeMock(string $type, int $id = 123): array { + $ownerMock = $this->createMock(IUser::class); + $ownerMock->method('getDisplayName')->willReturn('Test User'); + $ownerMock->method('getUID')->willReturn('3df8ff78-49cb-4d60-8d8b-171b29591fd3'); + + $cacheMock = $this->createMock(ICache::class); + $cacheMock->method('update')->willReturn(true); + $storageMock = $this->createMock(IStorage::class); + $storageMock->method('getCache')->willReturn($cacheMock); + + $fileMock = $this->createMock(File::class); + $fileMock->method('getId')->willReturn(123); + $fileMock->method('getStorage')->willReturn($storageMock); + + $folderMock = $this->getFolderMock(); + $folderMock->method('getId')->willReturn($id); + $folderMock->method('getType')->willReturn($type); + $folderMock->method('isCreatable')->willReturn(true); + $folderMock->method('nodeExists')->willReturn(false); + $folderMock->method('newFile')->willReturn($fileMock); + return [$folderMock]; + } + + public function testDirectUploadNotEnoughSpace(): void { + $nodeMock = $this->getNodeMock('folder'); + $nodeMock[0]->method('getFreeSpace')->willReturn(100); + + $userFolderMock = $this->getFolderMock(); + $userFolderMock->method('getById')->willReturn($nodeMock); + + $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); + $this->expectException(NotEnoughSpaceException::class); + $this->expectExceptionMessage('insufficient quota'); + $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], false); + } + + public function testNegativeFreeSpace(): void { + $cacheMock = $this->createMock(ICache::class); + $cacheMock->method('update')->willReturn(true); + $storageMock = $this->createMock(IStorage::class); + $storageMock->method('getCache')->willReturn($cacheMock); + + $fileMock = $this->createMock(File::class); + $fileMock->method('getId')->willReturn(123); + $fileMock->method('getStorage')->willReturn($storageMock); + $nodeMock = $this->getNodeMock('folder'); + touch(self::TMP_NAME); + $nodeMock[0]->method('getFreeSpace')->willReturn(-3); + $nodeMock[0]->method('newFile')->willReturn($fileMock); + $userFolderMock = $this->getFolderMock(); + $userFolderMock->method('getById')->willReturn($nodeMock); + + $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); + $result = $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], false); + assertSame( + [ + 'file_id' => 123, + 'file_name' => 'file.txt', + 'overwritten' => false + ], + $result + ); + } + + /** + * @return array + */ + public function newFileExceptionsDataProvider() { + return [ + [new InvalidContentException('Virus detected'), 'Virus detected'], + [new FileAccessForbiddenException('Access denied by the access control', false), 'Access denied by the access control'], + [new \Exception('could not upload'), 'could not upload'], + ]; + } + + /** + * @return void + * @dataProvider newFileExceptionsDataProvider + */ + public function testDirectUploadException( + \Exception $exception, string $expectedErrorMessage + ):void { + $nodeMock = $this->getNodeMock('folder'); + touch(self::TMP_NAME); + $nodeMock[0]->method('newFile')->willThrowException($exception); + $userFolderMock = $this->getFolderMock(); + $userFolderMock->method('getById')->willReturn($nodeMock); + + $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); + $this->expectException($exception::class); + $this->expectExceptionMessage($expectedErrorMessage); + $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 0], false); } } From 88603f193fbf43b6580f39035f3a64802c2dd7e7 Mon Sep 17 00:00:00 2001 From: Ashim Shrestha Date: Tue, 27 Oct 2026 10:09:34 +0545 Subject: [PATCH 3/5] chore: fix minor typo Signed-off-by: Ashim Shrestha --- tests/lib/Controller/DirectUploadControllerTest.php | 6 +++--- tests/lib/Service/DirectUploadServiceTest.php | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/lib/Controller/DirectUploadControllerTest.php b/tests/lib/Controller/DirectUploadControllerTest.php index b62ec6351..a0c995076 100644 --- a/tests/lib/Controller/DirectUploadControllerTest.php +++ b/tests/lib/Controller/DirectUploadControllerTest.php @@ -41,7 +41,7 @@ public function getFolderMock(): mixed { /** * @return void */ - public function testprepareDirectUpload() { + public function testPrepareDirectUpload() { $folderMock = $this->getFolderMock(); $folderMock->method('getById')->willReturn($this->getNodeMock('dir')); $directUploadController = $this->createDirectUploadController($folderMock); @@ -59,7 +59,7 @@ public function testprepareDirectUpload() { /** * @return void */ - public function testprepareDirectUploadTypeFile(): void { + public function testPrepareDirectUploadTypeFile(): void { $folderMock = $this->getFolderMock(); $folderMock->method('getById')->willReturn($this->getNodeMock('file')); $directUploadController = $this->createDirectUploadController($folderMock); @@ -73,7 +73,7 @@ public function testprepareDirectUploadTypeFile(): void { assertSame(404, $result->getStatus()); } - public function testprepareDirectUploadException(): void { + public function testPrepareDirectUploadException(): void { $folderMock = $this->getFolderMock(); $folderMock->method('getById') ->will($this->throwException(new \Exception('something bad happened'))); diff --git a/tests/lib/Service/DirectUploadServiceTest.php b/tests/lib/Service/DirectUploadServiceTest.php index 18b80716a..8a70ec995 100644 --- a/tests/lib/Service/DirectUploadServiceTest.php +++ b/tests/lib/Service/DirectUploadServiceTest.php @@ -76,9 +76,9 @@ protected function getDirectUploadService(int $expiresOn, mixed $userFolderMock */ public function testGetTokenInfoExpiredTime() { //send an already expired time - $direcUploadService = $this->getDirectUploadService(1671537979); + $directUploadService = $this->getDirectUploadService(1671537979); $this->expectException(NotFoundException::class); - $direcUploadService->getTokenInfo(self::TOKEN); + $directUploadService->getTokenInfo(self::TOKEN); } /** From 1d16eef38b46c329ed628ce3638c6a915f08a27c Mon Sep 17 00:00:00 2001 From: Ashim Shrestha Date: Tue, 27 Oct 2026 15:22:23 +0545 Subject: [PATCH 4/5] chore: add changelog entry Signed-off-by: Ashim Shrestha --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bb803342..426e7738e 100644 --- a/CHANGELOG.md +++ b/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) From f7556a2ce1f682e09c9c31933e68104f2a521f0e Mon Sep 17 00:00:00 2001 From: Ashim Shrestha Date: Tue, 27 Oct 2026 15:29:37 +0545 Subject: [PATCH 5/5] address reviews Signed-off-by: Ashim Shrestha --- lib/Controller/DirectUploadController.php | 38 +++--- lib/Service/DirectUploadService.php | 111 ++++++++++++------ .../features/api/directUpload.feature | 1 + tests/lib/Service/DirectUploadServiceTest.php | 8 +- 4 files changed, 102 insertions(+), 56 deletions(-) diff --git a/lib/Controller/DirectUploadController.php b/lib/Controller/DirectUploadController.php index 23a751255..6e94bed7b 100644 --- a/lib/Controller/DirectUploadController.php +++ b/lib/Controller/DirectUploadController.php @@ -16,6 +16,7 @@ use OCA\OpenProject\Exception\OpenprojectUnauthorizedUserException; use OCA\OpenProject\Service\DatabaseService; use OCA\OpenProject\Service\DirectUploadService; +use OCA\OpenProject\Service\FileAction; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; use OCP\Files\File; @@ -33,6 +34,7 @@ use OCP\IUserManager; use OCP\IUserSession; use OCP\Lock\LockedException; +use OCP\Util; use Sabre\DAV\Exception\Conflict; class DirectUploadController extends ApiController { @@ -76,6 +78,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, @@ -155,8 +168,7 @@ public function directUpload(string $token):DataResponse { throw new OpenprojectUnauthorizedUserException('invalid token'); } - $postMaxSize = ini_get('post_max_size'); - $postMaxSizeInBytes = (int)\OCP\Util::computerFileSize($postMaxSize); + $postMaxSizeInBytes = (int)Util::computerFileSize(ini_get('post_max_size')); $contentLength = (int)$this->request->getHeader('Content-Length'); if ($postMaxSizeInBytes > 0 && $contentLength > $postMaxSizeInBytes) { throw new OpenprojectFileNotUploadedException( @@ -171,20 +183,9 @@ public function directUpload(string $token):DataResponse { ); } - $phpFileUploadErrors = [ - 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', - ]; - if (array_key_exists('error', $directUploadFile) && $directUploadFile['error'] !== UPLOAD_ERR_OK) { throw new OpenprojectFileNotUploadedException( - $phpFileUploadErrors[$directUploadFile['error']], + self::PHP_FILE_UPLOAD_ERRORS[$directUploadFile['error']], ); } @@ -203,7 +204,12 @@ public function directUpload(string $token):DataResponse { } $overwrite = $overwrite === 'true'; } - $result = $this->directUploadService->uploadFile($token, $directUploadFile, $overwrite); + + $fileAction = $overwrite === null + ? FileAction::Upload + : ($overwrite ? FileAction::Replace : FileAction::KeepBoth); + $result = $this->directUploadService->handleUploadFile($token, $directUploadFile, $fileAction); + } catch (OpenprojectUnauthorizedUserException $e) { return new DataResponse([ 'error' => $this->l->t($e->getMessage()) @@ -247,7 +253,7 @@ public function directUpload(string $token):DataResponse { 'error' => $this->l->t($e->getMessage()) ], Http::STATUS_INTERNAL_SERVER_ERROR); } - $statusCode = $result['overwritten'] ? Http::STATUS_OK : Http::STATUS_CREATED; + $statusCode = $result['overwrite'] ? Http::STATUS_OK : Http::STATUS_CREATED; return new DataResponse([ 'file_name' => $result['file_name'], 'file_id' => $result['file_id'], diff --git a/lib/Service/DirectUploadService.php b/lib/Service/DirectUploadService.php index 32d9434e8..9de3534d0 100644 --- a/lib/Service/DirectUploadService.php +++ b/lib/Service/DirectUploadService.php @@ -23,6 +23,12 @@ use OCP\Security\ISecureRandom; use Sabre\DAV\Exception\Conflict; +enum FileAction: string { + case Replace = 'replace'; + case KeepBoth = 'keep_both'; + case Upload = 'upload'; +} + class DirectUploadService { /** @@ -123,7 +129,47 @@ public function getTokenInfo(string $token): ?array { return $tokenInfo; } - public function uploadFile(string $token, mixed $directUploadFile, ?bool $overwrite) : array { + /** + * + * uploads a file + * + * @param mixed $folderNode + * @param string $fileName + * @param string $tmpPath + * + * @return array{file_id: int, file_name: string, overwrite: bool} + */ + private function uploadFile(mixed $folderNode, string $fileName, string $tmpPath) : array { + $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, + 'overwrite' => false, + ]; + } + + /** + * + * handles uploading a file + * + * @param string $token + * @param mixed $directUploadFile + * @param FileAction $fileAction + * + * @return array{file_id: int, file_name: string, overwrite: bool} + * + * @throws NotFoundException + * @throws ForbiddenException + * @throws NotEnoughSpaceException + * @throws Conflict + */ + public function handleUploadFile(string $token, mixed $directUploadFile, FileAction $fileAction) : array { $tokenInfo = $this->getTokenInfo($token); $user = $this->userManager->get($tokenInfo['user_id']); $this->userSession->setUser($user); @@ -136,7 +182,7 @@ public function uploadFile(string $token, mixed $directUploadFile, ?bool $overwr * @var Folder $folderNode */ $folderNode = array_shift($nodes); - if (!$folderNode->isCreatable() && !$overwrite) { + if (!$folderNode->isCreatable() && $fileAction != FileAction::Replace) { throw new ForbiddenException('not enough permissions'); } $freeSpace = $folderNode->getFreeSpace(); @@ -151,41 +197,34 @@ public function uploadFile(string $token, mixed $directUploadFile, ?bool $overwr } $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'); + + if ($folderNode->nodeExists($fileName)) { + if ($fileAction === FileAction::Replace) { + /** + * @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, + 'overwrite' => true, + ]; + } elseif ($fileAction === FileAction::KeepBoth) { + $fileName = $folderNode->getNonExistingName($fileName); + return $this->uploadFile($folderNode, $fileName, $tmpPath); + } elseif ($fileAction === FileAction::Upload) { + throw new Conflict('conflict, file name already exists'); } - // overwrite the file - $file->putContent(fopen($tmpPath, 'r')); - return [ - 'file_id' => $file->getId(), - 'file_name' => $fileName, - 'overwritten' => true, - ]; - } 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, - 'overwritten' => false, - ]; + + return $this->uploadFile($folderNode, $fileName, $tmpPath); } } diff --git a/tests/acceptance/features/api/directUpload.feature b/tests/acceptance/features/api/directUpload.feature index 1449cf95b..00f99e32a 100644 --- a/tests/acceptance/features/api/directUpload.feature +++ b/tests/acceptance/features/api/directUpload.feature @@ -70,6 +70,7 @@ 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 | + # 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 """" diff --git a/tests/lib/Service/DirectUploadServiceTest.php b/tests/lib/Service/DirectUploadServiceTest.php index 8a70ec995..98cec9024 100644 --- a/tests/lib/Service/DirectUploadServiceTest.php +++ b/tests/lib/Service/DirectUploadServiceTest.php @@ -120,7 +120,7 @@ public function testDirectUploadNotEnoughSpace(): void { $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); $this->expectException(NotEnoughSpaceException::class); $this->expectExceptionMessage('insufficient quota'); - $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], false); + $directUploadServiceMock->handleUploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], FileAction::Upload); } public function testNegativeFreeSpace(): void { @@ -140,12 +140,12 @@ public function testNegativeFreeSpace(): void { $userFolderMock->method('getById')->willReturn($nodeMock); $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); - $result = $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], false); + $result = $directUploadServiceMock->handleUploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 101], FileAction::Upload); assertSame( [ 'file_id' => 123, 'file_name' => 'file.txt', - 'overwritten' => false + 'overwrite' => false, ], $result ); @@ -178,6 +178,6 @@ public function testDirectUploadException( $directUploadServiceMock = $this->getDirectUploadService(time() + 1000, $userFolderMock); $this->expectException($exception::class); $this->expectExceptionMessage($expectedErrorMessage); - $directUploadServiceMock->uploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 0], false); + $directUploadServiceMock->handleUploadFile(self::TOKEN, [ 'name' => 'file.txt', 'tmp_name' => self::TMP_NAME, 'size' => 0], FileAction::Upload); } }