Skip to content

Commit 08f03dd

Browse files
samin-zsamin-z
committed
add logic to remove dangling file shares after permission update
Signed-off-by: samin-z <samin.zavarkesh@gmail.com>
1 parent 236ad38 commit 08f03dd

6 files changed

Lines changed: 248 additions & 22 deletions

File tree

‎lib/Controller/ShareApiController.php‎

Lines changed: 4 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCA\Forms\Service\CirclesService;
2020
use OCA\Forms\Service\ConfigService;
2121
use OCA\Forms\Service\FormsService;
22+
use OCA\Forms\Service\UploadedFilesShareService;
2223
use OCP\AppFramework\Db\DoesNotExistException;
2324
use OCP\AppFramework\Db\IMapperException;
2425
use OCP\AppFramework\Http;
@@ -62,6 +63,7 @@ public function __construct(
6263
private readonly IRootFolder $rootFolder,
6364
private readonly FilePathHelper $filePathHelper,
6465
private readonly IManager $shareManager,
66+
private readonly UploadedFilesShareService $uploadedFilesShareService,
6567
) {
6668
parent::__construct($appName, $request);
6769
}
@@ -334,7 +336,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da
334336

335337
$this->shareManager->createShare($folderShare);
336338
} else {
337-
$this->removeUploadedFilesShare($form, $formShare);
339+
$this->uploadedFilesShareService->removeForCollaborator($form, $formShare);
338340
}
339341
}
340342

@@ -386,7 +388,7 @@ public function deleteShare(int $formId, int $shareId): DataResponse {
386388

387389
// Revoke any linked Files share before deleting the Forms share
388390
if (in_array(Constants::PERMISSION_RESULTS, $share->getPermissions(), true)) {
389-
$this->removeUploadedFilesShare($form, $share);
391+
$this->uploadedFilesShareService->removeForCollaborator($form, $share);
390392
}
391393

392394
$this->shareMapper->delete($share);
@@ -421,26 +423,6 @@ public function generateToken(): DataResponse {
421423
]);
422424
}
423425

424-
private function removeUploadedFilesShare(Form $form, Share $formShare): void {
425-
if (!in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) {
426-
return;
427-
}
428-
429-
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
430-
$uploadedFilesFolderPath = $this->filePathHelper->getFormUploadedFilesFolderPath($form);
431-
try {
432-
$folder = $userFolder->get($uploadedFilesFolderPath);
433-
} catch (NotFoundException) {
434-
return;
435-
}
436-
$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
437-
foreach ($folderShares as $folderShare) {
438-
if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
439-
$this->shareManager->deleteShare($folderShare);
440-
}
441-
}
442-
}
443-
444426
/**
445427
* Validate user given permission array
446428
*

‎lib/Db/FormMapper.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use OCA\Forms\Constants;
1111
use OCA\Forms\Helper\FilePathHelper;
1212
use OCA\Forms\Service\ConfigService;
13+
use OCA\Forms\Service\UploadedFilesShareService;
1314
use OCP\AppFramework\Db\Entity;
1415
use OCP\AppFramework\Db\QBMapper;
1516
use OCP\Comments\ICommentsManager;
@@ -39,6 +40,7 @@ public function __construct(
3940
private readonly FilePathHelper $filePathHelper,
4041
private readonly UploadedFileMapper $uploadedFileMapper,
4142
private readonly IRootFolder $rootFolder,
43+
private readonly UploadedFilesShareService $uploadedFilesShareService,
4244
private readonly LoggerInterface $logger,
4345
) {
4446
parent::__construct($db, 'forms_v2_forms', Form::class);
@@ -228,6 +230,7 @@ public function findAllByOwnerId(string $ownerId, ?string $queryTerm = null): ar
228230
public function deleteForm(Form $form): void {
229231
// Delete Submissions(incl. Answers), Questions(incl. Options), Shares and Form.
230232
$formId = $form->getId();
233+
$this->uploadedFilesShareService->removeAllForForm($form);
231234
$this->submissionMapper->deleteByForm($formId);
232235
$this->shareMapper->deleteByForm($formId);
233236
$this->questionMapper->deleteByForm($formId);

‎lib/Service/UploadedFilesShareService.php‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Forms\Service;
11+
12+
use OCA\Forms\Constants;
13+
use OCA\Forms\Db\Form;
14+
use OCA\Forms\Db\Share;
15+
use OCA\Forms\Db\ShareMapper;
16+
use OCA\Forms\Helper\FilePathHelper;
17+
use OCP\Files\IRootFolder;
18+
use OCP\Files\NotFoundException;
19+
use OCP\Share\IManager;
20+
use OCP\Share\IShare;
21+
use Psr\Log\LoggerInterface;
22+
23+
class UploadedFilesShareService {
24+
public function __construct(
25+
private readonly IRootFolder $rootFolder,
26+
private readonly FilePathHelper $filePathHelper,
27+
private readonly IManager $shareManager,
28+
private readonly ShareMapper $shareMapper,
29+
private readonly LoggerInterface $logger,
30+
) {
31+
}
32+
33+
/**
34+
* remove all linked files shares (oc_share) for collaborators who have 'view responses' on the given form.
35+
*/
36+
public function removeAllForForm(Form $form): void {
37+
$shares = $this->shareMapper->findByForm($form->getId());
38+
foreach ($shares as $share) {
39+
if (in_array(Constants::PERMISSION_RESULTS, $share->getPermissions(), true)) {
40+
$this->removeForCollaborator($form, $share);
41+
}
42+
}
43+
}
44+
45+
/**
46+
* remove the linked files share (oc_share) for one collaborator (either user, group, or team) on the given form.
47+
*/
48+
public function removeForCollaborator(Form $form, Share $formShare): void {
49+
if (!in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) {
50+
return;
51+
}
52+
53+
try {
54+
$userFolder = $this->rootFolder->getUserFolder($form->getOwnerId());
55+
$uploadedFilesFolderPath = $this->filePathHelper->getFormUploadedFilesFolderPath($form);
56+
try {
57+
$folder = $userFolder->get($uploadedFilesFolderPath);
58+
} catch (NotFoundException) {
59+
return;
60+
}
61+
62+
$folderShares = $this->shareManager->getSharesBy($form->getOwnerId(), $formShare->getShareType(), $folder, false, -1);
63+
foreach ($folderShares as $folderShare) {
64+
if ($folderShare->getSharedWith() === $formShare->getShareWith()) {
65+
$this->shareManager->deleteShare($folderShare);
66+
}
67+
}
68+
} catch (\Throwable $e) {
69+
$this->logger->warning('Failed to remove uploaded files share for form', ['exception' => $e]);
70+
}
71+
}
72+
}

‎tests/Unit/Controller/ShareApiControllerTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCA\Forms\Service\CirclesService;
2121
use OCA\Forms\Service\ConfigService;
2222
use OCA\Forms\Service\FormsService;
23+
use OCA\Forms\Service\UploadedFilesShareService;
2324
use OCP\AppFramework\Db\DoesNotExistException;
2425
use OCP\AppFramework\Db\IMapperException;
2526
use OCP\AppFramework\Http;
@@ -64,6 +65,7 @@ class ShareApiControllerTest extends TestCase {
6465
private IRootFolder|MockObject $rootFolder;
6566
private IFilenameValidator|MockObject $filenameValidator;
6667
private IManager|MockObject $shareManager;
68+
private UploadedFilesShareService $uploadedFilesShareService;
6769

6870
public function setUp(): void {
6971
$this->formMapper = $this->createMock(FormMapper::class);
@@ -83,6 +85,13 @@ public function setUp(): void {
8385
->willReturnCallback(static fn (string $fileName, string $replacement): string => str_replace(['/', '\\'], $replacement, trim($fileName)));
8486
$this->filePathHelper = new FilePathHelper($this->rootFolder, $this->filenameValidator);
8587
$this->shareManager = $this->createMock(IManager::class);
88+
$this->uploadedFilesShareService = new UploadedFilesShareService(
89+
$this->rootFolder,
90+
$this->filePathHelper,
91+
$this->shareManager,
92+
$this->shareMapper,
93+
$this->logger,
94+
);
8695

8796
$this->shareApiController = new ShareApiController(
8897
'forms',
@@ -99,6 +108,7 @@ public function setUp(): void {
99108
$this->rootFolder,
100109
$this->filePathHelper,
101110
$this->shareManager,
111+
$this->uploadedFilesShareService,
102112
);
103113
}
104114

‎tests/Unit/Db/FormMapperTest.php‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Forms\Tests\Unit\Db;
11+
12+
use OCA\Forms\Db\Form;
13+
use OCA\Forms\Db\FormMapper;
14+
use OCA\Forms\Db\QuestionMapper;
15+
use OCA\Forms\Db\ShareMapper;
16+
use OCA\Forms\Db\SubmissionMapper;
17+
use OCA\Forms\Db\UploadedFileMapper;
18+
use OCA\Forms\Helper\FilePathHelper;
19+
use OCA\Forms\Service\ConfigService;
20+
use OCA\Forms\Service\UploadedFilesShareService;
21+
use OCP\Comments\ICommentsManager;
22+
use OCP\Files\IRootFolder;
23+
use OCP\IDBConnection;
24+
use PHPUnit\Framework\MockObject\MockObject;
25+
use Psr\Log\LoggerInterface;
26+
use Test\TestCase;
27+
28+
class FormMapperTest extends TestCase {
29+
private FormMapper $formMapper;
30+
private UploadedFilesShareService|MockObject $uploadedFilesShareService;
31+
32+
protected function setUp(): void {
33+
parent::setUp();
34+
35+
$this->uploadedFilesShareService = $this->createMock(UploadedFilesShareService::class);
36+
37+
$this->formMapper = $this->getMockBuilder(FormMapper::class)
38+
->setConstructorArgs([
39+
$this->createMock(IDBConnection::class),
40+
$this->createMock(QuestionMapper::class),
41+
$this->createMock(ShareMapper::class),
42+
$this->createMock(SubmissionMapper::class),
43+
$this->createMock(ConfigService::class),
44+
$this->createMock(ICommentsManager::class),
45+
$this->createMock(FilePathHelper::class),
46+
$this->createMock(UploadedFileMapper::class),
47+
$this->createMock(IRootFolder::class),
48+
$this->uploadedFilesShareService,
49+
$this->createMock(LoggerInterface::class),
50+
])
51+
->onlyMethods(['delete'])
52+
->getMock();
53+
}
54+
55+
public function testDeleteFormCleansUpUploadedFilesShares(): void {
56+
$form = new Form();
57+
$form->setId(2);
58+
$form->setOwnerId('alice');
59+
60+
$this->uploadedFilesShareService->expects($this->once())
61+
->method('removeAllForForm')
62+
->with($form);
63+
64+
$this->formMapper->deleteForm($form);
65+
}
66+
}

‎tests/Unit/Service/UploadedFilesShareServiceTest.php‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Forms\Tests\Unit\Service;
11+
12+
use OCA\Forms\Constants;
13+
use OCA\Forms\Db\Form;
14+
use OCA\Forms\Db\Share;
15+
use OCA\Forms\Db\ShareMapper;
16+
use OCA\Forms\Helper\FilePathHelper;
17+
use OCA\Forms\Service\UploadedFilesShareService;
18+
use OCP\Files\Folder;
19+
use OCP\Files\IRootFolder;
20+
use OCP\Share\IManager;
21+
use OCP\Share\IShare;
22+
use PHPUnit\Framework\MockObject\MockObject;
23+
use Psr\Log\LoggerInterface;
24+
use Test\TestCase;
25+
26+
class UploadedFilesShareServiceTest extends TestCase {
27+
private UploadedFilesShareService $service;
28+
private ShareMapper|MockObject $shareMapper;
29+
private IRootFolder|MockObject $rootFolder;
30+
private IManager|MockObject $shareManager;
31+
32+
protected function setUp(): void {
33+
parent::setUp();
34+
35+
$this->shareMapper = $this->createMock(ShareMapper::class);
36+
$this->rootFolder = $this->createMock(IRootFolder::class);
37+
$this->shareManager = $this->createMock(IManager::class);
38+
$filePathHelper = new FilePathHelper($this->rootFolder);
39+
40+
$this->service = new UploadedFilesShareService(
41+
$this->rootFolder,
42+
$filePathHelper,
43+
$this->shareManager,
44+
$this->shareMapper,
45+
$this->createMock(LoggerInterface::class),
46+
);
47+
}
48+
49+
public function testRemoveAllForFormCleansUpResultsShares(): void {
50+
$form = new Form();
51+
$form->setId(2);
52+
$form->setTitle('test');
53+
$form->setOwnerId('alice');
54+
55+
$resultsShare = new Share();
56+
$resultsShare->setShareType(IShare::TYPE_USER);
57+
$resultsShare->setShareWith('bob');
58+
$resultsShare->setPermissions([Constants::PERMISSION_SUBMIT, Constants::PERMISSION_RESULTS]);
59+
60+
$submitOnlyShare = new Share();
61+
$submitOnlyShare->setShareType(IShare::TYPE_USER);
62+
$submitOnlyShare->setShareWith('carol');
63+
$submitOnlyShare->setPermissions([Constants::PERMISSION_SUBMIT]);
64+
65+
$this->shareMapper->expects($this->once())
66+
->method('findByForm')
67+
->with(2)
68+
->willReturn([$resultsShare, $submitOnlyShare]);
69+
70+
$folder = $this->createMock(Folder::class);
71+
$userFolder = $this->createMock(Folder::class);
72+
$userFolder->expects($this->once())
73+
->method('get')
74+
->with('Forms/2 - test')
75+
->willReturn($folder);
76+
$this->rootFolder->expects($this->once())
77+
->method('getUserFolder')
78+
->with('alice')
79+
->willReturn($userFolder);
80+
81+
$fileShare = $this->createMock(IShare::class);
82+
$fileShare->method('getSharedWith')->willReturn('bob');
83+
$this->shareManager->expects($this->once())
84+
->method('getSharesBy')
85+
->with('alice', IShare::TYPE_USER, $folder, false, -1)
86+
->willReturn([$fileShare]);
87+
$this->shareManager->expects($this->once())
88+
->method('deleteShare')
89+
->with($fileShare);
90+
91+
$this->service->removeAllForForm($form);
92+
}
93+
}

0 commit comments

Comments
 (0)