From e22ec1fb32705cb223b315a0a661c95d24a5cf83 Mon Sep 17 00:00:00 2001 From: "Peter R." Date: Tue, 14 Apr 2026 13:58:44 +0200 Subject: [PATCH] fix: clean up file shares on collaborator removal (#3291) * fix(shares): clean up file shares on collaborator removal -e Signed-off-by: Peter Ringelmann * refactor(shares): fix TOCTOU in folder lookup -e Signed-off-by: Peter Ringelmann --- lib/Controller/ShareApiController.php | 52 +++++++--- .../Controller/ShareApiControllerTest.php | 98 ++++++++++++++++++- 2 files changed, 132 insertions(+), 18 deletions(-) diff --git a/lib/Controller/ShareApiController.php b/lib/Controller/ShareApiController.php index 21b15753f..badf724f9 100644 --- a/lib/Controller/ShareApiController.php +++ b/lib/Controller/ShareApiController.php @@ -10,6 +10,7 @@ namespace OCA\Forms\Controller; use OCA\Forms\Constants; +use OCA\Forms\Db\Form; use OCA\Forms\Db\FormMapper; use OCA\Forms\Db\Share; use OCA\Forms\Db\ShareMapper; @@ -30,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; @@ -270,16 +272,16 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da $formShare = $this->shareMapper->update($formShare); if (in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], 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 */ - if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) { + $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); + $uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form); + try { + /** @var \OCP\Files\Folder $folder */ + $folder = $userFolder->get($uploadedFilesFolderPath); + } catch (NotFoundException $e) { + $folder = $userFolder->newFolder($uploadedFilesFolderPath); + } + $folderShare = $this->shareManager->newShare(); $folderShare->setShareType($formShare->getShareType()); $folderShare->setSharedWith($formShare->getShareWith()); @@ -290,12 +292,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da $this->shareManager->createShare($folderShare); } else { - $folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder); - foreach ($folderShares as $folderShare) { - if ($folderShare->getSharedWith() === $formShare->getShareWith()) { - $this->shareManager->deleteShare($folderShare); - } - } + $this->removeUploadedFilesShare($form, $formShare); } } @@ -345,12 +342,37 @@ public function deleteShare(int $formId, int $shareId): DataResponse { $this->formsService->obtainFormLock($form); + // Revoke any linked Files share before deleting the Forms share + if (in_array(Constants::PERMISSION_RESULTS, $share->getPermissions(), true)) { + $this->removeUploadedFilesShare($form, $share); + } + $this->shareMapper->delete($share); $this->formMapper->update($form); return new DataResponse($shareId); } + private function removeUploadedFilesShare(Form $form, Share $formShare): void { + if (!in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) { + return; + } + + $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); + $uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form); + try { + $folder = $userFolder->get($uploadedFilesFolderPath); + } catch (NotFoundException $e) { + 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); + } + } + } + /** * Validate user given permission array * diff --git a/tests/Unit/Controller/ShareApiControllerTest.php b/tests/Unit/Controller/ShareApiControllerTest.php index 806f96d57..0c4572815 100644 --- a/tests/Unit/Controller/ShareApiControllerTest.php +++ b/tests/Unit/Controller/ShareApiControllerTest.php @@ -539,6 +539,101 @@ public function testDeleteShare() { $this->assertEquals($response, $this->shareApiController->deleteShare(5, 8)); } + /** + * Delete share that had results permission — Files share must be cleaned up. + */ + public function testDeleteShare_cleansUpFileShare(): void { + $share = new Share(); + $share->setId(8); + $share->setFormId(5); + $share->setShareType(IShare::TYPE_USER); + $share->setShareWith('collaborator'); + $share->setPermissions([Constants::PERMISSION_SUBMIT, Constants::PERMISSION_RESULTS]); + + $this->shareMapper->expects($this->once()) + ->method('findById') + ->with('8') + ->willReturn($share); + + $form = new Form(); + $form->setId('5'); + $form->setOwnerId('currentUser'); + $this->formsService->expects($this->once()) + ->method('getFormIfAllowed') + ->with('5') + ->willReturn($form); + + // Mock the uploaded files folder lookup + $folder = $this->createMock(Folder::class); + $userFolder = $this->createMock(Folder::class); + $userFolder->expects($this->once()) + ->method('get') + ->willReturn($folder); + $this->storage->expects($this->once()) + ->method('getUserFolder') + ->with('currentUser') + ->willReturn($userFolder); + $this->formsService->expects($this->once()) + ->method('getFormUploadedFilesFolderPath') + ->with($form) + ->willReturn('Forms/my-form'); + + // Mock finding and deleting the matching Files share + $fileShare = $this->createMock(IShare::class); + $fileShare->method('getSharedWith')->willReturn('collaborator'); + $this->shareManager->expects($this->once()) + ->method('getSharesBy') + ->with('currentUser', IShare::TYPE_USER, $folder) + ->willReturn([$fileShare]); + $this->shareManager->expects($this->once()) + ->method('deleteShare') + ->with($fileShare); + + $this->shareMapper->expects($this->once()) + ->method('delete') + ->with($share); + + $response = new DataResponse(8); + $this->assertEquals($response, $this->shareApiController->deleteShare(5, 8)); + } + + /** + * Delete share without results permission — no file share cleanup needed. + */ + public function testDeleteShare_noResultsPermission_skipsFileShareCleanup(): void { + $share = new Share(); + $share->setId(8); + $share->setFormId(5); + $share->setShareType(IShare::TYPE_USER); + $share->setShareWith('collaborator'); + $share->setPermissions([Constants::PERMISSION_SUBMIT]); + + $this->shareMapper->expects($this->once()) + ->method('findById') + ->with('8') + ->willReturn($share); + + $form = new Form(); + $form->setId('5'); + $form->setOwnerId('currentUser'); + $this->formsService->expects($this->once()) + ->method('getFormIfAllowed') + ->with('5') + ->willReturn($form); + + // No file share interactions expected + $this->storage->expects($this->never())->method('getUserFolder'); + $this->shareManager->expects($this->never())->method('getSharesBy'); + $this->shareManager->expects($this->never())->method('deleteShare'); + + $this->shareMapper->expects($this->once()) + ->method('delete') + ->with($share); + + $response = new DataResponse(8); + $this->assertEquals($response, $this->shareApiController->deleteShare(5, 8)); + } + /** * Delete Non-existing share. */ @@ -766,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())