refactor(shares): use getOrCreateFolder and fix TOCTOU in folder lookup

-e
Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>

Author: pringelmann

lib/Controller/ShareApiController.php

@@ -31,6 +31,7 @@
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\Files\IRootFolder;
+use OCP\Files\NotFoundException;
 use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IRequest;
@@ -274,12 +275,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
 			if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) {
 				$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
 				$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
-				if ($userFolder->nodeExists($uploadedFilesFolderPath)) {
-					$folder = $userFolder->get($uploadedFilesFolderPath);
-				} else {
-					$folder = $userFolder->newFolder($uploadedFilesFolderPath);
-				}
-				/** @var \OCP\Files\Folder $folder */
+				$folder = $userFolder->getOrCreateFolder($uploadedFilesFolderPath);
 
 				$folderShare = $this->shareManager->newShare();
 				$folderShare->setShareType($formShare->getShareType());
@@ -359,11 +355,11 @@ private function removeUploadedFilesShare(Form $form, Share $formShare): void {
 
 		$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
 		$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
-		if (!$userFolder->nodeExists($uploadedFilesFolderPath)) {
+		try {
+			$folder = $userFolder->get($uploadedFilesFolderPath);
+		} catch (NotFoundException $e) {
 			return;
 		}
-
-		$folder = $userFolder->get($uploadedFilesFolderPath);
 		$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
 		foreach ($folderShares as $folderShare) {
 			if ($folderShare->getSharedWith() === $formShare->getShareWith()) {

tests/Unit/Controller/ShareApiControllerTest.php

@@ -566,9 +566,6 @@ public function testDeleteShare_cleansUpFileShare(): void {
 		// Mock the uploaded files folder lookup
 		$folder = $this->createMock(Folder::class);
 		$userFolder = $this->createMock(Folder::class);
-		$userFolder->expects($this->once())
-			->method('nodeExists')
-			->willReturn(true);
 		$userFolder->expects($this->once())
 			->method('get')
 			->willReturn($folder);
@@ -864,9 +861,6 @@ public function testUpdateShare(array $share, string $formOwner, array $keyValue
 			->willReturn($this->createMock(IUser::class));
 
 		$userFolder = $this->createMock(Folder::class);
-		$userFolder->expects($this->any())
-			->method('nodeExists')
-			->willReturn(true);
 
 		$file = $this->createMock(File::class);
 		$file->expects($this->any())
@@ -878,6 +872,9 @@ public function testUpdateShare(array $share, string $formOwner, array $keyValue
 			->method('newFile')
 			->willReturn($file);
 
+		$userFolder->expects($this->any())
+			->method('getOrCreateFolder')
+			->willReturn($folder);
 		$userFolder->expects($this->any())
 			->method('get')
 			->willReturn($folder);