Skip to content

Commit 050c2b9

Browse files
pringelmannpringelmann
committed
fix(shares): clean up file shares on collaborator removal
-e Signed-off-by: Peter Ringelmann <peter.ringelmann@nextcloud.com>
1 parent d237027 commit 050c2b9

File tree

2 files changed

+134
-15
lines changed

2 files changed

+134
-15
lines changed

‎lib/Controller/ShareApiController.php‎

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
namespace OCA\Forms\Controller;
1111

1212
use OCA\Forms\Constants;
13+
use OCA\Forms\Db\Form;
1314
use OCA\Forms\Db\FormMapper;
1415
use OCA\Forms\Db\Share;
1516
use OCA\Forms\Db\ShareMapper;
@@ -270,16 +271,16 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
270271
$formShare = $this->shareMapper->update($formShare);
271272

272273
if (in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) {
273-
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
274-
$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
275-
if ($userFolder->nodeExists($uploadedFilesFolderPath)) {
276-
$folder = $userFolder->get($uploadedFilesFolderPath);
277-
} else {
278-
$folder = $userFolder->newFolder($uploadedFilesFolderPath);
279-
}
280-
/** @var \OCP\Files\Folder $folder */
281-
282274
if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) {
275+
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
276+
$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
277+
if ($userFolder->nodeExists($uploadedFilesFolderPath)) {
278+
$folder = $userFolder->get($uploadedFilesFolderPath);
279+
} else {
280+
$folder = $userFolder->newFolder($uploadedFilesFolderPath);
281+
}
282+
/** @var \OCP\Files\Folder $folder */
283+
283284
$folderShare = $this->shareManager->newShare();
284285
$folderShare->setShareType($formShare->getShareType());
285286
$folderShare->setSharedWith($formShare->getShareWith());
@@ -290,12 +291,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
290291

291292
$this->shareManager->createShare($folderShare);
292293
} else {
293-
$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder);
294-
foreach ($folderShares as $folderShare) {
295-
if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
296-
$this->shareManager->deleteShare($folderShare);
297-
}
298-
}
294+
$this->removeUploadedFilesShare($form, $formShare);
299295
}
300296
}
301297

@@ -345,12 +341,37 @@ public function deleteShare(int $formId, int $shareId): DataResponse {
345341

346342
$this->formsService->obtainFormLock($form);
347343

344+
// Revoke any linked Files share before deleting the Forms share
345+
if (in_array(Constants::PERMISSION_RESULTS, $share->getPermissions(), true)) {
346+
$this->removeUploadedFilesShare($form, $share);
347+
}
348+
348349
$this->shareMapper->delete($share);
349350
$this->formMapper->update($form);
350351

351352
return new DataResponse($shareId);
352353
}
353354

355+
private function removeUploadedFilesShare(Form $form, Share $formShare): void {
356+
if (!in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) {
357+
return;
358+
}
359+
360+
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
361+
$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
362+
if (!$userFolder->nodeExists($uploadedFilesFolderPath)) {
363+
return;
364+
}
365+
366+
$folder = $userFolder->get($uploadedFilesFolderPath);
367+
$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
368+
foreach ($folderShares as $folderShare) {
369+
if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
370+
$this->shareManager->deleteShare($folderShare);
371+
}
372+
}
373+
}
374+
354375
/**
355376
* Validate user given permission array
356377
*

‎tests/Unit/Controller/ShareApiControllerTest.php‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,104 @@ public function testDeleteShare() {
539539
$this->assertEquals($response, $this->shareApiController->deleteShare(5, 8));
540540
}
541541

542+
/**
543+
* Delete share that had results permission — Files share must be cleaned up.
544+
*/
545+
public function testDeleteShare_cleansUpFileShare(): void {
546+
$share = new Share();
547+
$share->setId(8);
548+
$share->setFormId(5);
549+
$share->setShareType(IShare::TYPE_USER);
550+
$share->setShareWith('collaborator');
551+
$share->setPermissions([Constants::PERMISSION_SUBMIT, Constants::PERMISSION_RESULTS]);
552+
553+
$this->shareMapper->expects($this->once())
554+
->method('findById')
555+
->with('8')
556+
->willReturn($share);
557+
558+
$form = new Form();
559+
$form->setId('5');
560+
$form->setOwnerId('currentUser');
561+
$this->formsService->expects($this->once())
562+
->method('getFormIfAllowed')
563+
->with('5')
564+
->willReturn($form);
565+
566+
// Mock the uploaded files folder lookup
567+
$folder = $this->createMock(Folder::class);
568+
$userFolder = $this->createMock(Folder::class);
569+
$userFolder->expects($this->once())
570+
->method('nodeExists')
571+
->willReturn(true);
572+
$userFolder->expects($this->once())
573+
->method('get')
574+
->willReturn($folder);
575+
$this->storage->expects($this->once())
576+
->method('getUserFolder')
577+
->with('currentUser')
578+
->willReturn($userFolder);
579+
$this->formsService->expects($this->once())
580+
->method('getFormUploadedFilesFolderPath')
581+
->with($form)
582+
->willReturn('Forms/my-form');
583+
584+
// Mock finding and deleting the matching Files share
585+
$fileShare = $this->createMock(IShare::class);
586+
$fileShare->method('getSharedWith')->willReturn('collaborator');
587+
$this->shareManager->expects($this->once())
588+
->method('getSharesBy')
589+
->with('currentUser', IShare::TYPE_USER, $folder)
590+
->willReturn([$fileShare]);
591+
$this->shareManager->expects($this->once())
592+
->method('deleteShare')
593+
->with($fileShare);
594+
595+
$this->shareMapper->expects($this->once())
596+
->method('delete')
597+
->with($share);
598+
599+
$response = new DataResponse(8);
600+
$this->assertEquals($response, $this->shareApiController->deleteShare(5, 8));
601+
}
602+
603+
/**
604+
* Delete share without results permission — no file share cleanup needed.
605+
*/
606+
public function testDeleteShare_noResultsPermission_skipsFileShareCleanup(): void {
607+
$share = new Share();
608+
$share->setId(8);
609+
$share->setFormId(5);
610+
$share->setShareType(IShare::TYPE_USER);
611+
$share->setShareWith('collaborator');
612+
$share->setPermissions([Constants::PERMISSION_SUBMIT]);
613+
614+
$this->shareMapper->expects($this->once())
615+
->method('findById')
616+
->with('8')
617+
->willReturn($share);
618+
619+
$form = new Form();
620+
$form->setId('5');
621+
$form->setOwnerId('currentUser');
622+
$this->formsService->expects($this->once())
623+
->method('getFormIfAllowed')
624+
->with('5')
625+
->willReturn($form);
626+
627+
// No file share interactions expected
628+
$this->storage->expects($this->never())->method('getUserFolder');
629+
$this->shareManager->expects($this->never())->method('getSharesBy');
630+
$this->shareManager->expects($this->never())->method('deleteShare');
631+
632+
$this->shareMapper->expects($this->once())
633+
->method('delete')
634+
->with($share);
635+
636+
$response = new DataResponse(8);
637+
$this->assertEquals($response, $this->shareApiController->deleteShare(5, 8));
638+
}
639+
542640
/**
543641
* Delete Non-existing share.
544642
*/

0 commit comments

Comments
 (0)