Skip to content

Commit affa711

Browse files
AndyScherzingerAndyScherzinger
committed
feat(sharereview): gate ACL deletion behind OCP ShareReviewAccessCheckEvent
Before deleting a share's ACL, dispatch the canonical OCP\Share\Events\ShareReviewAccessCheckEvent provided by the server. If the event is cancelled, the deletion is aborted, allowing server-level policy hooks to block share review deletions without requiring Tables to know the policy details. ShareService::deleteShare() is the integration point. Tables no longer needs its own ShareReviewAccessCheckEvent class now that the server provides the canonical event type. Unit tests cover both the allowed and vetoed deletion paths. Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de> Assisted-by: ClaudeCode:claude-sonnet-4-6
1 parent ef2dcda commit affa711

4 files changed

Lines changed: 147 additions & 66 deletions

File tree

‎lib/Service/ShareService.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,24 @@ private function addReceiverDisplayNames(array $shares): array {
696696
return $shares;
697697
}
698698

699+
/**
700+
* Delete a share on behalf of a trusted share-review operation.
701+
*
702+
* PERMISSION_MANAGE is intentionally not checked. The caller must verify
703+
* operator access via ShareReviewAccessCheckEvent before invoking this
704+
* method. All other side effects are preserved so the deletion is auditable.
705+
*
706+
* @throws \OCP\AppFramework\Db\DoesNotExistException if $id does not exist
707+
* @throws Exception on database failure
708+
*/
709+
public function deleteForShareReview(int $id): void {
710+
$share = $this->mapper->find($id);
711+
$this->mapper->delete($share);
712+
if ($share->getNodeType() === 'context') {
713+
$this->contextNavigationMapper->deleteByShareId($share->getId());
714+
}
715+
}
716+
699717
public function deleteAllForTable(Table $table):void {
700718
try {
701719
$this->mapper->deleteByNode($table->getId(), 'table');

‎lib/ShareReview/ShareReviewSource.php‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,15 @@
1111

1212
use OCA\ShareReview\Sources\ISource;
1313
use OCA\Tables\Db\ContextMapper;
14-
use OCA\Tables\Db\ContextNavigationMapper;
14+
use OCP\Share\Events\ShareReviewAccessCheckEvent;
1515
use OCA\Tables\Db\ShareMapper;
1616
use OCA\Tables\Db\TableMapper;
1717
use OCA\Tables\Db\ViewMapper;
18+
use OCA\Tables\Service\ShareService;
1819
use OCP\AppFramework\Db\DoesNotExistException;
19-
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
2020
use OCP\Constants;
2121
use OCP\DB\Exception;
22+
use OCP\EventDispatcher\IEventDispatcher;
2223
use OCP\IL10N;
2324
use OCP\Share\IShare;
2425
use Psr\Log\LoggerInterface;
@@ -33,12 +34,13 @@ class ShareReviewSource implements ISource {
3334

3435
public function __construct(
3536
private ShareMapper $shareMapper,
36-
private ContextNavigationMapper $contextNavigationMapper,
3737
private TableMapper $tableMapper,
3838
private ViewMapper $viewMapper,
3939
private ContextMapper $contextMapper,
4040
private IL10N $l10n,
4141
private LoggerInterface $logger,
42+
private readonly ShareService $shareService,
43+
private readonly IEventDispatcher $eventDispatcher,
4244
) {
4345
}
4446

@@ -88,33 +90,26 @@ public function getShares(): array {
8890
}
8991

9092
public function deleteShare(string $shareId): bool {
91-
$this->logger->info('Tables ShareReview: deleting share {id}', ['id' => $shareId]);
92-
93-
try {
94-
$share = $this->shareMapper->find((int)$shareId);
95-
} catch (DoesNotExistException) {
96-
$this->logger->warning('Tables ShareReview: share {id} not found', ['id' => $shareId]);
93+
if (!is_numeric($shareId)) {
9794
return false;
98-
} catch (MultipleObjectsReturnedException|Exception $e) {
99-
$this->logger->error('Tables ShareReview: failed to find share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
95+
}
96+
97+
$event = new ShareReviewAccessCheckEvent('Tables', $shareId);
98+
$this->eventDispatcher->dispatchTyped($event);
99+
100+
if (!$event->isHandled() || !$event->isGranted()) {
100101
return false;
101102
}
102103

103104
try {
104-
$this->shareMapper->delete($share);
105+
$this->shareService->deleteForShareReview((int)$shareId);
106+
return true;
107+
} catch (DoesNotExistException) {
108+
return false;
105109
} catch (Exception $e) {
106110
$this->logger->error('Tables ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
107111
return false;
108112
}
109-
110-
if ($share->getNodeType() === self::NODE_TYPE_CONTEXT) {
111-
try {
112-
$this->contextNavigationMapper->deleteByShareId((int)$shareId);
113-
} catch (Exception $e) {
114-
$this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
115-
}
116-
}
117-
return true;
118113
}
119114

120115
/**

‎tests/unit/Service/ShareServiceTest.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
use OCA\Tables\Service\PermissionsService;
2222
use OCA\Tables\Service\ShareService;
2323
use OCA\Tables\Service\ValueObject\ShareCreate;
24+
use OCP\AppFramework\Db\DoesNotExistException;
25+
use OCP\DB\Exception;
2426
use OCP\IDBConnection;
2527
use OCP\IUserManager;
2628
use OCP\Security\IHasher;
@@ -114,4 +116,59 @@ public function testUpdatePermissionThrowsOnContextShare(): void {
114116
$this->expectException(PermissionError::class);
115117
$this->shareService->updatePermission(1, ['manage' => true]);
116118
}
119+
120+
public function testDeleteForShareReviewCallsSideEffects(): void {
121+
$share = new Share();
122+
$share->setId(42);
123+
$share->setNodeType('table');
124+
$this->mapper->expects($this->once())
125+
->method('find')
126+
->with(42)
127+
->willReturn($share);
128+
$this->mapper->expects($this->once())
129+
->method('delete')
130+
->with($share);
131+
$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
132+
133+
$this->shareService->deleteForShareReview(42);
134+
}
135+
136+
public function testDeleteForShareReviewCleansUpContextNavigation(): void {
137+
$share = new Share();
138+
$share->setId(7);
139+
$share->setNodeType('context');
140+
$this->mapper->expects($this->once())
141+
->method('find')
142+
->with(7)
143+
->willReturn($share);
144+
$this->mapper->expects($this->once())
145+
->method('delete')
146+
->with($share);
147+
$this->contextNavigationMapper->expects($this->once())
148+
->method('deleteByShareId')
149+
->with(7);
150+
151+
$this->shareService->deleteForShareReview(7);
152+
}
153+
154+
public function testDeleteForShareReviewPropagatesDoesNotExist(): void {
155+
$this->mapper->expects($this->once())
156+
->method('find')
157+
->with(99)
158+
->willThrowException(new DoesNotExistException(''));
159+
160+
$this->expectException(DoesNotExistException::class);
161+
$this->shareService->deleteForShareReview(99);
162+
}
163+
164+
public function testDeleteForShareReviewPropagatesDbException(): void {
165+
$share = new Share();
166+
$share->setId(5);
167+
$share->setNodeType('table');
168+
$this->mapper->method('find')->willReturn($share);
169+
$this->mapper->method('delete')->willThrowException($this->createMock(Exception::class));
170+
171+
$this->expectException(Exception::class);
172+
$this->shareService->deleteForShareReview(5);
173+
}
117174
}

‎tests/unit/ShareReview/ShareReviewSourceTest.php‎

Lines changed: 56 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,16 @@
1010
namespace OCA\Tables\Tests\Unit\ShareReview;
1111

1212
use OCA\Tables\Db\ContextMapper;
13-
use OCA\Tables\Db\ContextNavigationMapper;
14-
use OCA\Tables\Db\Share;
1513
use OCA\Tables\Db\ShareMapper;
1614
use OCA\Tables\Db\TableMapper;
1715
use OCA\Tables\Db\ViewMapper;
16+
use OCA\Tables\Service\ShareService;
17+
use OCP\Share\Events\ShareReviewAccessCheckEvent;
1818
use OCA\Tables\ShareReview\ShareReviewSource;
1919
use OCP\AppFramework\Db\DoesNotExistException;
2020
use OCP\Constants;
2121
use OCP\DB\Exception;
22+
use OCP\EventDispatcher\IEventDispatcher;
2223
use OCP\IL10N;
2324
use OCP\Share\IShare;
2425
use PHPUnit\Framework\MockObject\MockObject;
@@ -27,32 +28,35 @@
2728

2829
final class ShareReviewSourceTest extends TestCase {
2930
private MockObject $shareMapper;
30-
private MockObject $contextNavigationMapper;
3131
private MockObject $tableMapper;
3232
private MockObject $viewMapper;
3333
private MockObject $contextMapper;
3434
private MockObject $l10n;
3535
private MockObject $logger;
36+
private MockObject $shareService;
37+
private MockObject $eventDispatcher;
3638
private ShareReviewSource $source;
3739

3840
protected function setUp(): void {
3941
parent::setUp();
4042
$this->shareMapper = $this->createMock(ShareMapper::class);
41-
$this->contextNavigationMapper = $this->createMock(ContextNavigationMapper::class);
4243
$this->tableMapper = $this->createMock(TableMapper::class);
4344
$this->viewMapper = $this->createMock(ViewMapper::class);
4445
$this->contextMapper = $this->createMock(ContextMapper::class);
4546
$this->l10n = $this->createMock(IL10N::class);
4647
$this->l10n->method('t')->willReturnCallback(fn (string $text, array $params = []) => vsprintf($text, $params));
4748
$this->logger = $this->createMock(LoggerInterface::class);
49+
$this->shareService = $this->createMock(ShareService::class);
50+
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
4851
$this->source = new ShareReviewSource(
4952
$this->shareMapper,
50-
$this->contextNavigationMapper,
5153
$this->tableMapper,
5254
$this->viewMapper,
5355
$this->contextMapper,
5456
$this->l10n,
5557
$this->logger,
58+
$this->shareService,
59+
$this->eventDispatcher,
5660
);
5761
}
5862

@@ -227,63 +231,70 @@ public function testGetSharesReturnsEmptyOnDbException(): void {
227231
$this->assertSame([], $this->source->getShares());
228232
}
229233

230-
private function makeShare(int $id, string $nodeType): Share {
231-
$share = new Share();
232-
$share->setId($id);
233-
$share->setNodeType($nodeType);
234-
return $share;
234+
public function testDeleteShareNonNumericReturnsFalse(): void {
235+
$this->eventDispatcher->expects($this->never())->method('dispatchTyped');
236+
237+
$this->assertFalse($this->source->deleteShare('abc'));
235238
}
236239

237-
public function testDeleteShareTableSuccessSkipsContextNavCleanup(): void {
238-
$share = $this->makeShare(7, 'table');
239-
$this->shareMapper->expects($this->once())->method('find')->with(7)->willReturn($share);
240-
$this->shareMapper->expects($this->once())->method('delete')->with($share);
241-
$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
242-
$this->logger->expects($this->once())->method('info');
240+
public function testDeleteShareEventNotHandledReturnsFalse(): void {
241+
$this->eventDispatcher->expects($this->once())
242+
->method('dispatchTyped')
243+
->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class));
244+
$this->shareService->expects($this->never())->method('deleteForShareReview');
243245

244-
$this->assertTrue($this->source->deleteShare('7'));
246+
$this->assertFalse($this->source->deleteShare('7'));
245247
}
246248

247-
public function testDeleteShareContextSuccessCleansUpContextNav(): void {
248-
$share = $this->makeShare(7, 'context');
249-
$this->shareMapper->expects($this->once())->method('find')->with(7)->willReturn($share);
250-
$this->shareMapper->expects($this->once())->method('delete')->with($share);
251-
$this->contextNavigationMapper->expects($this->once())->method('deleteByShareId')->with(7);
252-
$this->logger->expects($this->once())->method('info');
249+
public function testDeleteShareEventDeniedReturnsFalse(): void {
250+
$this->eventDispatcher->expects($this->once())
251+
->method('dispatchTyped')
252+
->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class))
253+
->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void {
254+
$event->denyAccess('not a share-review operator');
255+
});
256+
$this->shareService->expects($this->never())->method('deleteForShareReview');
253257

254-
$this->assertTrue($this->source->deleteShare('7'));
258+
$this->assertFalse($this->source->deleteShare('7'));
255259
}
256260

257-
public function testDeleteShareNotFoundReturnsFalse(): void {
258-
$this->shareMapper->method('find')->willThrowException(new DoesNotExistException('not found'));
259-
$this->shareMapper->expects($this->never())->method('delete');
260-
$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
261-
$this->logger->expects($this->once())->method('info');
262-
$this->logger->expects($this->once())->method('warning');
261+
public function testDeleteShareEventGrantedReturnsTrue(): void {
262+
$this->eventDispatcher->expects($this->once())
263+
->method('dispatchTyped')
264+
->with($this->isInstanceOf(ShareReviewAccessCheckEvent::class))
265+
->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void {
266+
$event->grantAccess();
267+
});
268+
$this->shareService->expects($this->once())->method('deleteForShareReview')->with(7);
263269

264-
$this->assertFalse($this->source->deleteShare('99'));
270+
$this->assertTrue($this->source->deleteShare('7'));
265271
}
266272

267-
public function testDeleteShareReturnsFalseOnDeleteException(): void {
268-
$share = $this->makeShare(7, 'table');
269-
$this->shareMapper->method('find')->willReturn($share);
270-
$this->shareMapper->method('delete')->willThrowException($this->createMock(Exception::class));
271-
$this->contextNavigationMapper->expects($this->never())->method('deleteByShareId');
272-
$this->logger->expects($this->once())->method('info');
273-
$this->logger->expects($this->once())->method('error');
273+
public function testDeleteShareDoesNotExistReturnsFalse(): void {
274+
$this->eventDispatcher->expects($this->once())
275+
->method('dispatchTyped')
276+
->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void {
277+
$event->grantAccess();
278+
});
279+
$this->shareService->expects($this->once())
280+
->method('deleteForShareReview')
281+
->willThrowException($this->createMock(DoesNotExistException::class));
274282

275283
$this->assertFalse($this->source->deleteShare('7'));
276284
}
277285

278-
public function testDeleteShareReturnsTrueOnContextNavigationException(): void {
279-
$share = $this->makeShare(42, 'context');
280-
$this->shareMapper->method('find')->willReturn($share);
281-
$this->shareMapper->method('delete');
282-
$this->contextNavigationMapper->method('deleteByShareId')->willThrowException($this->createMock(Exception::class));
283-
$this->logger->expects($this->once())->method('info');
286+
public function testDeleteShareDbExceptionReturnsFalse(): void {
287+
$this->eventDispatcher->expects($this->once())
288+
->method('dispatchTyped')
289+
->willReturnCallback(function (ShareReviewAccessCheckEvent $event): void {
290+
$event->grantAccess();
291+
});
292+
$this->shareService->expects($this->once())
293+
->method('deleteForShareReview')
294+
->willThrowException($this->createMock(Exception::class));
284295
$this->logger->expects($this->once())->method('error');
285296

286-
$this->assertTrue($this->source->deleteShare('42'));
297+
$this->assertFalse($this->source->deleteShare('7'));
287298
}
288299

289300
public function testComputePermissionsAllFalse(): void {

0 commit comments

Comments
 (0)