Skip to content

Commit 4ae4de1

Browse files
Chartman123Chartman123
authored
Merge pull request #3294 from nextcloud/backport/3291-stable5.2
[stable5.2] fix: clean up file shares on collaborator removal
2 parents a74641f + e22ec1f commit 4ae4de1

File tree

2 files changed

+132
-18
lines changed

2 files changed

+132
-18
lines changed

‎lib/Controller/ShareApiController.php‎

Lines changed: 37 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;
@@ -30,6 +31,7 @@
3031
use OCP\AppFramework\OCS\OCSNotFoundException;
3132
use OCP\AppFramework\OCSController;
3233
use OCP\Files\IRootFolder;
34+
use OCP\Files\NotFoundException;
3335
use OCP\IGroup;
3436
use OCP\IGroupManager;
3537
use OCP\IRequest;
@@ -270,16 +272,16 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
270272
$formShare = $this->shareMapper->update($formShare);
271273

272274
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-
282275
if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) {
276+
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
277+
$uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form);
278+
try {
279+
/** @var \OCP\Files\Folder $folder */
280+
$folder = $userFolder->get($uploadedFilesFolderPath);
281+
} catch (NotFoundException $e) {
282+
$folder = $userFolder->newFolder($uploadedFilesFolderPath);
283+
}
284+
283285
$folderShare = $this->shareManager->newShare();
284286
$folderShare->setShareType($formShare->getShareType());
285287
$folderShare->setSharedWith($formShare->getShareWith());
@@ -290,12 +292,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
290292

291293
$this->shareManager->createShare($folderShare);
292294
} 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-
}
295+
$this->removeUploadedFilesShare($form, $formShare);
299296
}
300297
}
301298

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

346343
$this->formsService->obtainFormLock($form);
347344

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

351353
return new DataResponse($shareId);
352354
}
353355

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

‎tests/Unit/Controller/ShareApiControllerTest.php‎

Lines changed: 95 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,101 @@ 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('get')
571+
->willReturn($folder);
572+
$this->storage->expects($this->once())
573+
->method('getUserFolder')
574+
->with('currentUser')
575+
->willReturn($userFolder);
576+
$this->formsService->expects($this->once())
577+
->method('getFormUploadedFilesFolderPath')
578+
->with($form)
579+
->willReturn('Forms/my-form');
580+
581+
// Mock finding and deleting the matching Files share
582+
$fileShare = $this->createMock(IShare::class);
583+
$fileShare->method('getSharedWith')->willReturn('collaborator');
584+
$this->shareManager->expects($this->once())
585+
->method('getSharesBy')
586+
->with('currentUser', IShare::TYPE_USER, $folder)
587+
->willReturn([$fileShare]);
588+
$this->shareManager->expects($this->once())
589+
->method('deleteShare')
590+
->with($fileShare);
591+
592+
$this->shareMapper->expects($this->once())
593+
->method('delete')
594+
->with($share);
595+
596+
$response = new DataResponse(8);
597+
$this->assertEquals($response, $this->shareApiController->deleteShare(5, 8));
598+
}
599+
600+
/**
601+
* Delete share without results permission — no file share cleanup needed.
602+
*/
603+
public function testDeleteShare_noResultsPermission_skipsFileShareCleanup(): void {
604+
$share = new Share();
605+
$share->setId(8);
606+
$share->setFormId(5);
607+
$share->setShareType(IShare::TYPE_USER);
608+
$share->setShareWith('collaborator');
609+
$share->setPermissions([Constants::PERMISSION_SUBMIT]);
610+
611+
$this->shareMapper->expects($this->once())
612+
->method('findById')
613+
->with('8')
614+
->willReturn($share);
615+
616+
$form = new Form();
617+
$form->setId('5');
618+
$form->setOwnerId('currentUser');
619+
$this->formsService->expects($this->once())
620+
->method('getFormIfAllowed')
621+
->with('5')
622+
->willReturn($form);
623+
624+
// No file share interactions expected
625+
$this->storage->expects($this->never())->method('getUserFolder');
626+
$this->shareManager->expects($this->never())->method('getSharesBy');
627+
$this->shareManager->expects($this->never())->method('deleteShare');
628+
629+
$this->shareMapper->expects($this->once())
630+
->method('delete')
631+
->with($share);
632+
633+
$response = new DataResponse(8);
634+
$this->assertEquals($response, $this->shareApiController->deleteShare(5, 8));
635+
}
636+
542637
/**
543638
* Delete Non-existing share.
544639
*/
@@ -766,9 +861,6 @@ public function testUpdateShare(array $share, string $formOwner, array $keyValue
766861
->willReturn($this->createMock(IUser::class));
767862

768863
$userFolder = $this->createMock(Folder::class);
769-
$userFolder->expects($this->any())
770-
->method('nodeExists')
771-
->willReturn(true);
772864

773865
$file = $this->createMock(File::class);
774866
$file->expects($this->any())

0 commit comments

Comments
 (0)