Skip to content

Commit 5360cfd

Browse files
kesselbkesselb
committed
fix(caldav): prevent unshare entry creation for owner unsharing
- Introduces a `unshare` method in `CalDavBackend` to handle user unshares. - Implements check to determine if unshare entry is needed based on group/circle membership. - Ensures `updateShares` is only used when the calendar owner manages shares. - Resolves issue where unsharing a calendar as owner created an unshare entry in `oc_dav_shares`. Related PRs: - #43117 - #47737 Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
1 parent ba2b778 commit 5360cfd

6 files changed

Lines changed: 316 additions & 24 deletions

File tree

‎apps/dav/lib/CalDAV/CalDavBackend.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3628,4 +3628,26 @@ protected function purgeObjectInvitations(string $eventId): void {
36283628
->where($cmd->expr()->eq('uid', $cmd->createNamedParameter($eventId, IQueryBuilder::PARAM_STR), IQueryBuilder::PARAM_STR));
36293629
$cmd->executeStatement();
36303630
}
3631+
3632+
public function unshare(IShareable $shareable, string $principal): void {
3633+
$this->atomic(function () use ($shareable, $principal): void {
3634+
$calendarData = $this->getCalendarById($shareable->getResourceId());
3635+
if ($calendarData === null) {
3636+
throw new \RuntimeException('Trying to update shares for non-existing calendar: ' . $shareable->getResourceId());
3637+
}
3638+
3639+
$oldShares = $this->getShares($shareable->getResourceId());
3640+
$unshare = $this->calendarSharingBackend->unshare($shareable, $principal);
3641+
3642+
if ($unshare) {
3643+
$this->dispatcher->dispatchTyped(new CalendarShareUpdatedEvent(
3644+
$shareable->getResourceId(),
3645+
$calendarData,
3646+
$oldShares,
3647+
[],
3648+
[$principal]
3649+
));
3650+
}
3651+
}, $this->db);
3652+
}
36313653
}

‎apps/dav/lib/CalDAV/Calendar.php‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,12 +214,8 @@ public function getOwner(): ?string {
214214
}
215215

216216
public function delete() {
217-
if (isset($this->calendarInfo['{http://owncloud.org/ns}owner-principal']) &&
218-
$this->calendarInfo['{http://owncloud.org/ns}owner-principal'] !== $this->calendarInfo['principaluri']) {
219-
$principal = 'principal:' . parent::getOwner();
220-
$this->caldavBackend->updateShares($this, [], [
221-
$principal
222-
]);
217+
if ($this->isShared()) {
218+
$this->caldavBackend->unshare($this, 'principal:' . $this->getPrincipalURI());
223219
return;
224220
}
225221

‎apps/dav/lib/DAV/Sharing/Backend.php‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -90,14 +90,6 @@ public function updateShares(IShareable $shareable, array $add, array $remove, a
9090

9191
// Delete any possible direct shares (since the frontend does not separate between them)
9292
$this->service->deleteShare($shareable->getResourceId(), $principal);
93-
94-
// Check if a user has a groupshare that they're trying to free themselves from
95-
// If so we need to add a self::ACCESS_UNSHARED row
96-
if (!str_contains($principal, 'group')
97-
&& $this->service->hasGroupShare($oldShares)
98-
) {
99-
$this->service->unshare($shareable->getResourceId(), $principal);
100-
}
10193
}
10294
}
10395

@@ -204,4 +196,44 @@ public function applyShareAcl(array $shares, array $acl): array {
204196
}
205197
return $acl;
206198
}
199+
200+
public function unshare(IShareable $shareable, string $principalUri): bool {
201+
$principal = $this->principalBackend->findByUri($principalUri, '');
202+
if (empty($principal)) {
203+
return false;
204+
}
205+
206+
if ($shareable->getOwner() === $principal) {
207+
return false;
208+
}
209+
210+
// Delete any possible direct shares (since the frontend does not separate between them)
211+
$this->service->deleteShare($shareable->getResourceId(), $principal);
212+
213+
$needsUnshare = $this->hasAccessByGroupOrCirclesMembership(
214+
$shareable->getResourceId(),
215+
$principal
216+
);
217+
218+
if ($needsUnshare) {
219+
$this->service->unshare($shareable->getResourceId(), $principal);
220+
}
221+
222+
$this->shareCache->clear();
223+
return true;
224+
}
225+
226+
private function hasAccessByGroupOrCirclesMembership(int $resourceId, string $principal) {
227+
$memberships = array_merge(
228+
$this->principalBackend->getGroupMembership($principal, true),
229+
$this->principalBackend->getCircleMembership($principal)
230+
);
231+
232+
$shares = array_column(
233+
$this->service->getShares($resourceId),
234+
'principaluri'
235+
);
236+
237+
return count(array_intersect($memberships, $shares)) > 0;
238+
}
207239
}

‎apps/dav/lib/DAV/Sharing/SharingService.php‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,14 +50,4 @@ public function getUnshares(int $resourceId): array {
5050
public function getSharesForIds(array $resourceIds): array {
5151
return $this->mapper->getSharesForIds($resourceIds, $this->getResourceType());
5252
}
53-
54-
/**
55-
* @param array $oldShares
56-
* @return bool
57-
*/
58-
public function hasGroupShare(array $oldShares): bool {
59-
return !empty(array_filter($oldShares, function (array $share) {
60-
return $share['{http://owncloud.org/ns}group-share'] === true;
61-
}));
62-
}
6353
}

‎apps/dav/tests/integration/DAV/Sharing/CalDavSharingBackendTest.php‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Tests\integration\DAV\Sharing;
11+
12+
use OCA\DAV\CalDAV\Calendar;
13+
use OCA\DAV\Connector\Sabre\Principal;
14+
use OCA\DAV\DAV\Sharing\Backend;
15+
use OCA\DAV\DAV\Sharing\SharingMapper;
16+
use OCA\DAV\DAV\Sharing\SharingService;
17+
use OCP\DB\QueryBuilder\IQueryBuilder;
18+
use OCP\ICacheFactory;
19+
use OCP\IDBConnection;
20+
use OCP\IGroupManager;
21+
use OCP\IUserManager;
22+
use OCP\Server;
23+
use Psr\Log\LoggerInterface;
24+
use Test\TestCase;
25+
26+
/**
27+
* @group DB
28+
*/
29+
class CalDavSharingBackendTest extends TestCase {
30+
31+
private IDBConnection $db;
32+
private IUserManager $userManager;
33+
private IGroupManager $groupManager;
34+
private Principal $principalBackend;
35+
private ICacheFactory $cacheFactory;
36+
private LoggerInterface $logger;
37+
private SharingMapper $sharingMapper;
38+
private SharingService $sharingService;
39+
private Backend $sharingBackend;
40+
41+
private $principalUris = ['principals/groups/alice_bob'];
42+
private $resourceIds = [10001];
43+
44+
protected function setUp(): void {
45+
parent::setUp();
46+
47+
$this->db = Server::get(IDBConnection::class);
48+
49+
$this->userManager = $this->createMock(IUserManager::class);
50+
$this->groupManager = $this->createMock(IGroupManager::class);
51+
$this->principalBackend = $this->createMock(Principal::class);
52+
$this->cacheFactory = $this->createMock(ICacheFactory::class);
53+
$this->cacheFactory->method('createInMemory')
54+
->willReturn(new \OC\Memcache\NullCache());
55+
$this->logger = new \Psr\Log\NullLogger();
56+
57+
$this->sharingMapper = new SharingMapper($this->db);
58+
$this->sharingService = new \OCA\DAV\CalDAV\Sharing\Service($this->sharingMapper);
59+
60+
$this->sharingBackend = new \OCA\DAV\CalDAV\Sharing\Backend(
61+
$this->userManager,
62+
$this->groupManager,
63+
$this->principalBackend,
64+
$this->cacheFactory,
65+
$this->sharingService,
66+
$this->logger
67+
);
68+
69+
$this->removeFixtures();
70+
}
71+
72+
protected function tearDown(): void {
73+
$this->removeFixtures();
74+
}
75+
76+
protected function removeFixtures(): void {
77+
$qb = $this->db->getQueryBuilder();
78+
$qb->delete('dav_shares')
79+
->where($qb->expr()->in('resourceid', $qb->createNamedParameter($this->resourceIds, IQueryBuilder::PARAM_INT_ARRAY)));
80+
$qb->executeStatement();
81+
}
82+
83+
public function testShareCalendarWithGroup(): void {
84+
$calendar = $this->createMock(Calendar::class);
85+
$calendar->method('getResourceId')
86+
->willReturn(10001);
87+
$calendar->method('getOwner')
88+
->willReturn('principals/users/admin');
89+
90+
$this->principalBackend->method('findByUri')
91+
->willReturn('principals/groups/alice_bob');
92+
93+
$this->groupManager->method('groupExists')
94+
->willReturn(true);
95+
96+
$this->sharingBackend->updateShares(
97+
$calendar,
98+
[['href' => 'principals/groups/alice_bob']],
99+
[],
100+
[]
101+
);
102+
103+
$this->assertCount(1, $this->sharingService->getShares(10001));
104+
}
105+
106+
public function testUnshareCalendarFromGroup(): void {
107+
$calendar = $this->createMock(Calendar::class);
108+
$calendar->method('getResourceId')
109+
->willReturn(10001);
110+
$calendar->method('getOwner')
111+
->willReturn('principals/users/admin');
112+
113+
$this->principalBackend->method('findByUri')
114+
->willReturn('principals/groups/alice_bob');
115+
116+
$this->groupManager->method('groupExists')
117+
->willReturn(true);
118+
119+
$this->sharingBackend->updateShares(
120+
shareable: $calendar,
121+
add: [['href' => 'principals/groups/alice_bob']],
122+
remove: [],
123+
);
124+
125+
$this->assertCount(1, $this->sharingService->getShares(10001));
126+
127+
$this->sharingBackend->updateShares(
128+
shareable: $calendar,
129+
add: [],
130+
remove: ['principals/groups/alice_bob'],
131+
);
132+
133+
$this->assertCount(0, $this->sharingService->getShares(10001));
134+
}
135+
136+
public function testShareCalendarWithGroupAndUnshareAsUser(): void {
137+
$calendar = $this->createMock(Calendar::class);
138+
$calendar->method('getResourceId')
139+
->willReturn(10001);
140+
$calendar->method('getOwner')
141+
->willReturn('principals/users/admin');
142+
143+
$this->principalBackend->method('findByUri')
144+
->willReturnMap([
145+
['principals/groups/alice_bob', '', 'principals/groups/alice_bob'],
146+
['principals/users/bob', '', 'principals/users/bob'],
147+
]);
148+
$this->principalBackend->method('getGroupMembership')
149+
->willReturn([
150+
'principals/groups/alice_bob',
151+
]);
152+
$this->principalBackend->method('getCircleMembership')
153+
->willReturn([]);
154+
155+
$this->groupManager->method('groupExists')
156+
->willReturn(true);
157+
158+
/*
159+
* Owner is sharing the calendar with a group.
160+
*/
161+
$this->sharingBackend->updateShares(
162+
shareable: $calendar,
163+
add: [['href' => 'principals/groups/alice_bob']],
164+
remove: [],
165+
);
166+
167+
$this->assertCount(1, $this->sharingService->getShares(10001));
168+
169+
/*
170+
* Member of the group unshares the calendar.
171+
*/
172+
$this->sharingBackend->unshare(
173+
shareable: $calendar,
174+
principalUri: 'principals/users/bob'
175+
);
176+
177+
$this->assertCount(1, $this->sharingService->getShares(10001));
178+
$this->assertCount(1, $this->sharingService->getUnshares(10001));
179+
}
180+
181+
/**
182+
* Tests the functionality of sharing a calendar with a user, then with a group (that includes the shared user),
183+
* and subsequently unsharing it from the individual user. Verifies that the unshare operation correctly removes the specific user share
184+
* without creating an additional unshare entry.
185+
*/
186+
public function testShareCalendarWithUserThenGroupThenUnshareUser(): void {
187+
$calendar = $this->createMock(Calendar::class);
188+
$calendar->method('getResourceId')
189+
->willReturn(10001);
190+
$calendar->method('getOwner')
191+
->willReturn('principals/users/admin');
192+
193+
$this->principalBackend->method('findByUri')
194+
->willReturnMap([
195+
['principals/groups/alice_bob', '', 'principals/groups/alice_bob'],
196+
['principals/users/bob', '', 'principals/users/bob'],
197+
]);
198+
$this->principalBackend->method('getGroupMembership')
199+
->willReturn([
200+
'principals/groups/alice_bob',
201+
]);
202+
$this->principalBackend->method('getCircleMembership')
203+
->willReturn([]);
204+
205+
$this->userManager->method('userExists')
206+
->willReturn(true);
207+
$this->groupManager->method('groupExists')
208+
->willReturn(true);
209+
210+
/*
211+
* Step 1) The owner shares the calendar with a user.
212+
*/
213+
$this->sharingBackend->updateShares(
214+
shareable: $calendar,
215+
add: [['href' => 'principals/users/bob']],
216+
remove: [],
217+
);
218+
219+
$this->assertCount(1, $this->sharingService->getShares(10001));
220+
221+
/*
222+
* Step 2) The owner shares the calendar with a group that includes the
223+
* user from step 1 as a member.
224+
*/
225+
$this->sharingBackend->updateShares(
226+
shareable: $calendar,
227+
add: [['href' => 'principals/groups/alice_bob']],
228+
remove: [],
229+
);
230+
231+
$this->assertCount(2, $this->sharingService->getShares(10001));
232+
233+
/*
234+
* Step 3) Unshare the calendar from user as owner.
235+
*/
236+
$this->sharingBackend->updateShares(
237+
shareable: $calendar,
238+
add: [],
239+
remove: ['principals/users/bob'],
240+
);
241+
242+
/*
243+
* The purpose of this test is to ensure that removing a user from a share, as the owner, does not result in an "unshare" row being added.
244+
* Instead, the actual user share should be removed.
245+
*/
246+
$this->assertCount(1, $this->sharingService->getShares(10001));
247+
$this->assertCount(0, $this->sharingService->getUnshares(10001));
248+
}
249+
250+
}

‎apps/dav/tests/integration/DAV/Sharing/SharingMapperTest.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
* SPDX-License-Identifier: AGPL-3.0-or-later
88
*/
99

10+
namespace OCA\DAV\Tests\integration\DAV\Sharing;
11+
1012
use OCA\DAV\DAV\Sharing\SharingMapper;
1113
use OCP\IDBConnection;
1214
use OCP\Server;

0 commit comments

Comments
 (0)