[stable5.4] Fix/shared file access

lib/Controller/ApiController.php

@@ -361,11 +361,16 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
 		}
 
 		// Create FormEntity with given Params & Id.
+		$titleChanged = array_key_exists('title', $keyValuePairs);
 		foreach ($keyValuePairs as $key => $value) {
 			$method = 'set' . ucfirst($key);
 			$form->$method($value);
 		}
 
+		if ($titleChanged) {
+			$this->formsService->syncFormUploadFolderName($form);
+		}
+
 		// Update changed Columns in Db.
 		$this->formMapper->update($form);
 

lib/Helper/FilePathHelper.php

@@ -75,6 +75,56 @@ public function getAllFormFoldersById(int $formId, string $ownerId): array {
 		return $matchingFolders;
 	}
 
+	/**
+	 * Rename the upload folder to the form's current title.
+	 * If that name already contains files, move the other folder's contents into it and delete the other folder.
+	 */
+	public function syncFormUploadFolderName(Form $form): void {
+		$formsFolder = $this->getFormsFolder($form->getOwnerId());
+		if ($formsFolder === null) {
+			return;
+		}
+
+		$newName = $this->normalizeFileName($form->getId() . ' - ' . $form->getTitle());
+		$folders = $this->getAllFormFoldersById($form->getId(), $form->getOwnerId());
+		if ($folders === []) {
+			return;
+		}
+
+		$source = $folders[0];
+		foreach ($folders as $candidate) {
+			if ($candidate->getDirectoryListing() !== []) {
+				$source = $candidate;
+				break;
+			}
+		}
+
+		if ($source->getName() !== $newName) {
+			if ($formsFolder->nodeExists($newName)) {
+				$existing = $formsFolder->get($newName);
+				if ($existing instanceof Folder && $existing->getId() !== $source->getId()) {
+					if ($existing->getDirectoryListing() === []) {
+						$existing->delete();
+					} else {
+						foreach ($source->getDirectoryListing() as $node) {
+							$node->move($existing->getPath() . '/' . $node->getName());
+						}
+						$source->delete();
+					}
+				}
+			}
+			if (!$formsFolder->nodeExists($newName)) {
+				$source->move($formsFolder->getPath() . '/' . $newName);
+			}
+		}
+
+		foreach ($this->getAllFormFoldersById($form->getId(), $form->getOwnerId()) as $leftover) {
+			if ($leftover->getName() !== $newName && $leftover->getDirectoryListing() === []) {
+				$leftover->delete();
+			}
+		}
+	}
+
 	/**
 	 * Get the forms folder for a user
 	 */

lib/Service/FormsService.php

@@ -1059,6 +1059,17 @@ public function getFormUploadedFilesFolderPath(Form $form): string {
 		return $this->filePathHelper->getFormUploadedFilesFolderPath($form);
 	}
 
+	/**
+	 * Keep Forms upload folder name in sync with the form title.
+	 */
+	public function syncFormUploadFolderName(Form $form): void {
+		try {
+			$this->filePathHelper->syncFormUploadFolderName($form);
+		} catch (\Throwable $e) {
+			$this->logger->warning('Failed to sync form upload folder name', ['exception' => $e]);
+		}
+	}
+
 	public function getUploadedFilePath(Form $form, int $submissionId, int $questionId, ?string $questionName, string $questionText): string {
 		return $this->filePathHelper->getUploadedFilePath($form, $submissionId, $questionId, $questionName, $questionText);
 	}

lib/Service/UploadedFilesShareService.php

@@ -14,15 +14,12 @@
 use OCA\Forms\Db\Share;
 use OCA\Forms\Db\ShareMapper;
 use OCA\Forms\Helper\FilePathHelper;
-use OCP\Files\IRootFolder;
-use OCP\Files\NotFoundException;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
 use Psr\Log\LoggerInterface;
 
 class UploadedFilesShareService {
 	public function __construct(
-		private readonly IRootFolder $rootFolder,
 		private readonly FilePathHelper $filePathHelper,
 		private readonly IManager $shareManager,
 		private readonly ShareMapper $shareMapper,
@@ -51,18 +48,13 @@ public function removeForCollaborator(Form $form, Share $formShare): void {
 		}
 
 		try {
-			$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
-			$uploadedFilesFolderPath = $this->filePathHelper->getFormUploadedFilesFolderPath($form);
-			try {
-				$folder = $userFolder->get($uploadedFilesFolderPath);
-			} catch (NotFoundException) {
-				return;
-			}
-
-			$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
-			foreach ($folderShares as $folderShare) {
-				if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
-					$this->shareManager->deleteShare($folderShare);
+			$folders = $this->filePathHelper->getAllFormFoldersById($form->getId(), $form->getOwnerId());
+			foreach ($folders as $folder) {
+				$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
+				foreach ($folderShares as $folderShare) {
+					if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
+						$this->shareManager->deleteShare($folderShare);
+					}
 				}
 			}
 		} catch (\Throwable $e) {

tests/Unit/Controller/ShareApiControllerTest.php

@@ -88,7 +88,6 @@ public function setUp(): void {
 		$this->filePathHelper = new FilePathHelper($this->rootFolder, $this->filenameValidator);
 		$this->shareManager = $this->createMock(IManager::class);
 		$this->uploadedFilesShareService = new UploadedFilesShareService(
-			$this->rootFolder,
 			$this->filePathHelper,
 			$this->shareManager,
 			$this->shareMapper,
@@ -545,14 +544,18 @@ public function testDeleteShare_cleansUpFileShare(): void {
 			->with('5')
 			->willReturn($form);
 
-		// Mock the uploaded files folder lookup
+		// Mock the uploaded files folder lookup by form id
 		$folder = $this->createMock(Folder::class);
+		$folder->method('getName')->willReturn('5 - form');
+		$formsFolder = $this->createMock(Folder::class);
+		$formsFolder->method('getDirectoryListing')->willReturn([$folder]);
+
 		$userFolder = $this->createUserFolderMock();
 		$userFolder->expects($this->once())
 			->method('get')
-			->willReturn($folder);
+			->with(Constants::FILES_FOLDER)
+			->willReturn($formsFolder);
 
-		// Set expectations on the rootFolder
 		$this->rootFolder->expects($this->once())
 			->method('getUserFolder')
 			->with('currentUser')

tests/Unit/Service/UploadedFilesShareServiceTest.php

@@ -15,20 +15,16 @@
 use OCA\Forms\Db\ShareMapper;
 use OCA\Forms\Helper\FilePathHelper;
 use OCA\Forms\Service\UploadedFilesShareService;
-use OCA\Forms\Tests\Unit\UserFolderMockTrait;
 use OCP\Files\Folder;
 use OCP\Files\IFilenameValidator;
 use OCP\Files\IRootFolder;
-use OCP\Files\NotFoundException;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 class UploadedFilesShareServiceTest extends TestCase {
-	use UserFolderMockTrait;
-
 	private UploadedFilesShareService $service;
 	private ShareMapper|MockObject $shareMapper;
 	private IRootFolder|MockObject $rootFolder;
@@ -48,14 +44,28 @@ protected function setUp(): void {
 		);
 
 		$this->service = new UploadedFilesShareService(
-			$this->rootFolder,
 			$filePathHelper,
 			$this->shareManager,
 			$this->shareMapper,
 			$this->createMock(LoggerInterface::class),
 		);
 	}
 
+	/**
+	 * @return array{0: Folder&MockObject, 1: Folder&MockObject}
+	 */
+	private function mockFormsFolderWithChildren(string $ownerId, array $children): array {
+		$formsFolder = $this->createMock(Folder::class);
+		$formsFolder->method('getDirectoryListing')->willReturn($children);
+
+		$userFolder = $this->createMock(Folder::class);
+		$userFolder->method('get')->with(Constants::FILES_FOLDER)->willReturn($formsFolder);
+
+		$this->rootFolder->method('getUserFolder')->with($ownerId)->willReturn($userFolder);
+
+		return [$userFolder, $formsFolder];
+	}
+
 	public function testRemoveAllForFormCleansUpResultsShares(): void {
 		$form = new Form();
 		$form->setId(2);
@@ -78,15 +88,8 @@ public function testRemoveAllForFormCleansUpResultsShares(): void {
 			->willReturn([$resultsShare, $submitOnlyShare]);
 
 		$folder = $this->createMock(Folder::class);
-		$userFolder = $this->createUserFolderMock();
-		$userFolder->expects($this->once())
-			->method('get')
-			->with('Forms/2 - test')
-			->willReturn($folder);
-		$this->rootFolder->expects($this->once())
-			->method('getUserFolder')
-			->with('alice')
-			->willReturn($userFolder);
+		$folder->method('getName')->willReturn('2 - test');
+		$this->mockFormsFolderWithChildren('alice', [$folder]);
 
 		$fileShare = $this->createMock(IShare::class);
 		$fileShare->method('getSharedWith')->willReturn('bob');
@@ -101,6 +104,34 @@ public function testRemoveAllForFormCleansUpResultsShares(): void {
 		$this->service->removeAllForForm($form);
 	}
 
+	public function testRemoveForCollaboratorRevokesSharesOnRenamedFormFolder(): void {
+		$form = new Form();
+		$form->setId(19);
+		$form->setTitle('T2');
+		$form->setOwnerId('alice');
+
+		$share = new Share();
+		$share->setShareType(IShare::TYPE_USER);
+		$share->setShareWith('carol');
+		$share->setPermissions([Constants::PERMISSION_RESULTS]);
+
+		$oldTitleFolder = $this->createMock(Folder::class);
+		$oldTitleFolder->method('getName')->willReturn('19 - T1');
+		$this->mockFormsFolderWithChildren('alice', [$oldTitleFolder]);
+
+		$fileShare = $this->createMock(IShare::class);
+		$fileShare->method('getSharedWith')->willReturn('carol');
+		$this->shareManager->expects($this->once())
+			->method('getSharesBy')
+			->with('alice', IShare::TYPE_USER, $oldTitleFolder, false, -1)
+			->willReturn([$fileShare]);
+		$this->shareManager->expects($this->once())
+			->method('deleteShare')
+			->with($fileShare);
+
+		$this->service->removeForCollaborator($form, $share);
+	}
+
 	public function testRemoveForCollaboratorSkipsShareTypesWithoutFilesShare(): void {
 		$form = new Form();
 		$form->setId(2);
@@ -128,9 +159,7 @@ public function testRemoveForCollaboratorIgnoresMissingUploadedFilesFolder(): vo
 		$share->setShareWith('bob');
 		$share->setPermissions([Constants::PERMISSION_RESULTS]);
 
-		$userFolder = $this->createUserFolderMock();
-		$userFolder->method('get')->willThrowException(new NotFoundException());
-		$this->rootFolder->method('getUserFolder')->with('alice')->willReturn($userFolder);
+		$this->mockFormsFolderWithChildren('alice', []);
 		$this->shareManager->expects($this->never())->method('getSharesBy');
 
 		$this->service->removeForCollaborator($form, $share);