Skip to content

Commit e8dc75a

Browse files
AndyScherzingerAndyScherzinger
committed
feat(sharereview): gate ACL deletion behind event-based access check
Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
1 parent bec4321 commit e8dc75a

6 files changed

Lines changed: 247 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/ShareReviewAccessCheckEvent.php‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
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\Tables\ShareReview;
11+
12+
use OCP\EventDispatcher\Event;
13+
14+
class ShareReviewAccessCheckEvent extends Event {
15+
16+
private bool $handled = false;
17+
private bool $granted = false;
18+
private ?string $reason = null;
19+
20+
public function grantAccess(): void {
21+
if ($this->handled && !$this->granted) {
22+
return; // a prior denyAccess() cannot be escalated to a grant — deny wins
23+
}
24+
$this->handled = true;
25+
$this->granted = true;
26+
}
27+
28+
public function denyAccess(string $reason): void {
29+
$this->handled = true;
30+
$this->granted = false;
31+
$this->reason = $reason;
32+
}
33+
34+
public function isHandled(): bool {
35+
return $this->handled;
36+
}
37+
public function isGranted(): bool {
38+
return $this->granted;
39+
}
40+
public function getReason(): ?string {
41+
return $this->reason;
42+
}
43+
}

‎lib/ShareReview/ShareReviewSource.php‎

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

1212
use OCA\ShareReview\Sources\ISource;
1313
use OCA\Tables\Db\ContextMapper;
14-
use OCA\Tables\Db\ContextNavigationMapper;
1514
use OCA\Tables\Db\ShareMapper;
1615
use OCA\Tables\Db\TableMapper;
1716
use OCA\Tables\Db\ViewMapper;
17+
use OCA\Tables\Service\ShareService;
1818
use OCP\AppFramework\Db\DoesNotExistException;
19-
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
2019
use OCP\Constants;
2120
use OCP\DB\Exception;
21+
use OCP\EventDispatcher\IEventDispatcher;
2222
use OCP\IL10N;
2323
use OCP\Share\IShare;
2424
use Psr\Log\LoggerInterface;
@@ -33,12 +33,13 @@ class ShareReviewSource implements ISource {
3333

3434
public function __construct(
3535
private ShareMapper $shareMapper,
36-
private ContextNavigationMapper $contextNavigationMapper,
3736
private TableMapper $tableMapper,
3837
private ViewMapper $viewMapper,
3938
private ContextMapper $contextMapper,
4039
private IL10N $l10n,
4140
private LoggerInterface $logger,
41+
private readonly ShareService $shareService,
42+
private readonly IEventDispatcher $eventDispatcher,
4243
) {
4344
}
4445

@@ -88,33 +89,26 @@ public function getShares(): array {
8889
}
8990

9091
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]);
92+
if (!is_numeric($shareId)) {
9793
return false;
98-
} catch (MultipleObjectsReturnedException|Exception $e) {
99-
$this->logger->error('Tables ShareReview: failed to find share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
94+
}
95+
96+
$event = new ShareReviewAccessCheckEvent();
97+
$this->eventDispatcher->dispatchTyped($event);
98+
99+
if (!$event->isHandled() || !$event->isGranted()) {
100100
return false;
101101
}
102102

103103
try {
104-
$this->shareMapper->delete($share);
104+
$this->shareService->deleteForShareReview((int)$shareId);
105+
return true;
106+
} catch (DoesNotExistException) {
107+
return false;
105108
} catch (Exception $e) {
106109
$this->logger->error('Tables ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);
107110
return false;
108111
}
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;
118112
}
119113

120114
/**

‎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/ShareReviewAccessCheckEventTest.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
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\Tables\Tests\Unit\ShareReview;
11+
12+
use OCA\Tables\ShareReview\ShareReviewAccessCheckEvent;
13+
use PHPUnit\Framework\TestCase;
14+
15+
final class ShareReviewAccessCheckEventTest extends TestCase {
16+
17+
public function testInitialState(): void {
18+
$event = new ShareReviewAccessCheckEvent();
19+
20+
$this->assertFalse($event->isHandled());
21+
$this->assertFalse($event->isGranted());
22+
$this->assertNull($event->getReason());
23+
}
24+
25+
public function testGrantAccess(): void {
26+
$event = new ShareReviewAccessCheckEvent();
27+
$event->grantAccess();
28+
29+
$this->assertTrue($event->isHandled());
30+
$this->assertTrue($event->isGranted());
31+
}
32+
33+
public function testDenyAccess(): void {
34+
$event = new ShareReviewAccessCheckEvent();
35+
$event->denyAccess('not in group');
36+
37+
$this->assertTrue($event->isHandled());
38+
$this->assertFalse($event->isGranted());
39+
$this->assertSame('not in group', $event->getReason());
40+
}
41+
42+
public function testGrantThenDenyIsGrantedFalse(): void {
43+
$event = new ShareReviewAccessCheckEvent();
44+
$event->grantAccess();
45+
$event->denyAccess('revoked');
46+
47+
$this->assertFalse($event->isGranted());
48+
$this->assertSame('revoked', $event->getReason());
49+
}
50+
51+
public function testDenyThenGrantIsGrantedFalse(): void {
52+
$event = new ShareReviewAccessCheckEvent();
53+
$event->denyAccess('not allowed');
54+
$event->grantAccess();
55+
56+
$this->assertFalse($event->isGranted());
57+
}
58+
}

0 commit comments

Comments
 (0)