Skip to content

Commit a3dcd62

Browse files
committed
* Ensure we delete boards owned by a circle when the circle gets deleted
following the same behavior for users * make sure findAllByOwner is explicit about the type of ownership * Move 'PERMISSION_OWNER' to a sane spot * Fix the annotateAclRetainedAccess missing the owning circle's ACL entry Signed-off-by: Jos Poortvliet <jospoortvliet@gmail.com>
1 parent 3baaeb9 commit a3dcd62

11 files changed

Lines changed: 271 additions & 22 deletions

File tree

lib/Command/TransferOwnership.php

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,12 @@ protected function configure() {
7070
InputOption::VALUE_NONE,
7171
'Treat <newOwner> as a team ID (internally stored as a circle ID) instead of a user UID'
7272
)
73+
->addOption(
74+
'from-team',
75+
null,
76+
InputOption::VALUE_NONE,
77+
'Treat <owner> as a team ID (internally stored as a circle ID) instead of a user UID'
78+
)
7379
;
7480
}
7581

@@ -79,8 +85,32 @@ protected function execute(InputInterface $input, OutputInterface $output): int
7985
$boardId = $input->getArgument('boardId');
8086
$remapAssignment = $input->getOption('remap');
8187
$toTeam = $input->getOption('to-team');
88+
$fromTeam = $input->getOption('from-team');
89+
$ownerType = Acl::PERMISSION_TYPE_USER;
8290
$newOwnerType = Acl::PERMISSION_TYPE_USER;
8391
$teamDisplayName = null;
92+
if ($fromTeam) {
93+
$ownerType = Acl::PERMISSION_TYPE_CIRCLE;
94+
} else {
95+
$ownerUserExists = $this->userManager->userExists($owner);
96+
$ownerCircleExists = false;
97+
if ($this->circlesService->isCirclesEnabled()) {
98+
try {
99+
$ownerCircleExists = $this->circlesService->getCircle($owner) !== null;
100+
} catch (\Throwable $e) {
101+
$ownerCircleExists = false;
102+
}
103+
}
104+
105+
if ($ownerUserExists && $ownerCircleExists) {
106+
$output->writeln('<error>Ambiguous source owner: ' . $owner . ' matches both a user and a team (circle ID). Use --from-team if you mean the team.</error>');
107+
return 1;
108+
}
109+
110+
if ($ownerCircleExists && !$ownerUserExists) {
111+
$ownerType = Acl::PERMISSION_TYPE_CIRCLE;
112+
}
113+
}
84114
if ($toTeam) {
85115
$newOwnerType = Acl::PERMISSION_TYPE_CIRCLE;
86116
if ($this->circlesService->isCirclesEnabled()) {
@@ -154,7 +184,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
154184
return 0;
155185
}
156186

157-
foreach ($this->boardService->transferOwnership($owner, $newOwner, $remapAssignment, $newOwnerType) as $board) {
187+
foreach ($this->boardService->transferOwnership($owner, $newOwner, $remapAssignment, $newOwnerType, $ownerType) as $board) {
158188
$output->writeln(' - ' . $board->getTitle() . ' transferred');
159189
}
160190
$output->writeln("<info>All boards from $owner transferred to $newOwnerLabel</info>");

lib/Db/Acl.php

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ class Acl extends RelationalEntity {
3030
public const PERMISSION_EDIT = 1;
3131
public const PERMISSION_SHARE = 2;
3232
public const PERMISSION_MANAGE = 3;
33-
public const PERMISSION_OWNER = 4;
3433

3534
public const PERMISSION_TYPE_USER = 0;
3635
public const PERMISSION_TYPE_GROUP = 1;

lib/Db/Board.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
* @method int | null getExternalId()
3333
*/
3434
class Board extends RelationalEntity {
35+
public const PERMISSION_OWNER = 4;
36+
3537
protected $title;
3638
protected $owner;
3739
protected $ownerType = 0;

lib/Db/BoardMapper.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,11 +295,12 @@ public function findAllByUser(string $userId, ?int $limit = null, ?int $offset =
295295
return $entries;
296296
}
297297

298-
public function findAllByOwner(string $userId, ?int $limit = null, ?int $offset = null) {
298+
public function findAllByOwner(string $ownerId, int $ownerType = Acl::PERMISSION_TYPE_USER, ?int $limit = null, ?int $offset = null): array {
299299
$qb = $this->db->getQueryBuilder();
300300
$qb->select('*')
301301
->from('deck_boards')
302-
->where($qb->expr()->eq('owner', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
302+
->where($qb->expr()->eq('owner', $qb->createNamedParameter($ownerId, IQueryBuilder::PARAM_STR)))
303+
->andWhere($qb->expr()->eq('owner_type', $qb->createNamedParameter($ownerType, IQueryBuilder::PARAM_INT)))
303304
->orderBy('id');
304305
if ($limit !== null) {
305306
$qb->setMaxResults($limit);

lib/Listeners/ParticipantCleanupListener.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ public function __construct(AclMapper $aclMapper, AssignmentMapper $assignmentMa
3030

3131
public function handle(Event $event): void {
3232
if ($event instanceof UserDeletedEvent) {
33-
$boards = $this->boardMapper->findAllByOwner($event->getUser()->getUID());
33+
$boards = $this->boardMapper->findAllByOwner($event->getUser()->getUID(), Acl::PERMISSION_TYPE_USER);
3434
foreach ($boards as $board) {
3535
$this->boardMapper->delete($board);
3636
}
@@ -44,6 +44,11 @@ public function handle(Event $event): void {
4444

4545
if ($event instanceof CircleDestroyedEvent) {
4646
$circleId = $event->getCircle()->getSingleId();
47+
$boards = $this->boardMapper->findAllByOwner($circleId, Acl::PERMISSION_TYPE_CIRCLE);
48+
foreach ($boards as $board) {
49+
$this->boardMapper->delete($board);
50+
}
51+
4752
$this->cleanupByParticipant(Acl::PERMISSION_TYPE_CIRCLE, $circleId);
4853
}
4954
}

lib/Service/BoardService.php

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ public function create(string $title, string $userId, string $color): Board {
223223
'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false,
224224
'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false,
225225
'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false,
226-
'PERMISSION_OWNER' => $permissions[Acl::PERMISSION_OWNER] ?? false,
226+
'PERMISSION_OWNER' => $permissions[Board::PERMISSION_OWNER] ?? false,
227227
]);
228228
$this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $board, ActivityManager::SUBJECT_BOARD_CREATE, [], $userId);
229229
$this->changeHelper->boardChanged($board->getId());
@@ -575,7 +575,7 @@ public function clone(
575575
'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false,
576576
'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false,
577577
'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false,
578-
'PERMISSION_OWNER' => $permissions[Acl::PERMISSION_OWNER] ?? false,
578+
'PERMISSION_OWNER' => $permissions[Board::PERMISSION_OWNER] ?? false,
579579
]);
580580
$this->boardMapper->insert($newBoard);
581581

@@ -673,10 +673,10 @@ public function transferBoardOwnership(int $boardId, string $newOwner, bool $cha
673673
}
674674
}
675675

676-
public function transferOwnership(string $owner, string $newOwner, bool $changeContent = false, int $newOwnerType = Acl::PERMISSION_TYPE_USER): \Generator {
676+
public function transferOwnership(string $owner, string $newOwner, bool $changeContent = false, int $newOwnerType = Acl::PERMISSION_TYPE_USER, int $ownerType = Acl::PERMISSION_TYPE_USER): \Generator {
677677
// Validate once up front so invalid targets fail even if no boards match.
678678
$this->validateTransferTarget($newOwner, $newOwnerType);
679-
$boards = $this->boardMapper->findAllByOwner($owner);
679+
$boards = $this->boardMapper->findAllByOwner($owner, $ownerType);
680680
foreach ($boards as $board) {
681681
yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent, $newOwnerType);
682682
}
@@ -735,7 +735,7 @@ private function enrichBoards(array $boards, bool $fullDetails = true): array {
735735
'PERMISSION_EDIT' => $permissions[Acl::PERMISSION_EDIT] ?? false,
736736
'PERMISSION_MANAGE' => $permissions[Acl::PERMISSION_MANAGE] ?? false,
737737
'PERMISSION_SHARE' => $permissions[Acl::PERMISSION_SHARE] ?? false,
738-
'PERMISSION_OWNER' => $permissions[Acl::PERMISSION_OWNER] ?? false,
738+
'PERMISSION_OWNER' => $permissions[Board::PERMISSION_OWNER] ?? false,
739739
]);
740740

741741
if ($fullDetails) {
@@ -760,6 +760,22 @@ private function enrichBoards(array $boards, bool $fullDetails = true): array {
760760
private function annotateAclRetainedAccess(Board $board): void {
761761
$acls = $board->getAcl() ?? [];
762762
foreach ($acls as $acl) {
763+
if ($acl->getType() === Acl::PERMISSION_TYPE_GROUP) {
764+
$acl->setRetainsAccessViaMembership(true);
765+
continue;
766+
}
767+
768+
if ($acl->getType() === Acl::PERMISSION_TYPE_CIRCLE) {
769+
if ($board->getOwnerType() === Acl::PERMISSION_TYPE_CIRCLE
770+
&& (string)$acl->getParticipant() === $board->getOwner()) {
771+
$acl->setRetainsAccessViaMembership(true);
772+
continue;
773+
}
774+
775+
$acl->setRetainsAccessViaMembership(true);
776+
continue;
777+
}
778+
763779
if ($acl->getType() !== Acl::PERMISSION_TYPE_USER) {
764780
$acl->setRetainsAccessViaMembership(false);
765781
continue;

lib/Service/PermissionService.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ public function getPermissions(int $boardId, ?string $userId = null, bool $allow
116116
* Get current user permissions for a board
117117
*
118118
* @param Board $board
119-
* @return array|bool
119+
* @return array<int, bool>
120120
* @internal param $boardId
121121
*/
122122
public function matchPermissions(Board $board) {
@@ -136,7 +136,7 @@ public function matchPermissions(Board $board) {
136136
Acl::PERMISSION_MANAGE => $owner || $this->userCan($acls, Acl::PERMISSION_MANAGE),
137137
Acl::PERMISSION_SHARE => ($owner || $this->userCan($acls, Acl::PERMISSION_SHARE))
138138
&& (!$this->shareManager->sharingDisabledForUser($this->userId)),
139-
Acl::PERMISSION_OWNER => $owner,
139+
Board::PERMISSION_OWNER => $owner,
140140
];
141141
}
142142

src/components/board/SharingTabSidebar.vue

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
</span>
2626
<NcActions v-if="board.ownerType === PERMISSION_TYPE_CIRCLE && isBoardOwner" :force-menu="true">
2727
<NcActionButton
28-
icon="icon-transfer-ownership"
28+
icon="icon-user-admin"
2929
data-cy="action:permission-owner-self"
3030
@click="clickTransferOwner(currentUserUid, PERMISSION_TYPE_USER)">
3131
{{ t('deck', 'Transfer ownership to myself') }}
@@ -64,19 +64,19 @@
6464
{{ t('deck', 'Can manage') }}
6565
</NcActionCheckbox>
6666
<NcActionButton v-if="canTransferTo(acl)"
67-
icon="icon-transfer-ownership"
67+
icon="icon-user-admin"
6868
data-cy="action:permission-owner"
6969
@click="clickTransferOwner(acl.participant.uid || acl.participant.id, acl.type, acl.participant.displayname || acl.participant.id || acl.participant.uid)">
7070
{{ t('deck', 'Transfer ownership') }}
7171
</NcActionButton>
7272
<NcActionButton v-if="canManage"
7373
icon="icon-delete"
7474
data-cy="action:acl-delete"
75-
:title="acl.type === PERMISSION_TYPE_USER && acl.retainsAccessViaMembership
75+
:title="acl.retainsAccessViaMembership
7676
? t('deck', 'Board access through group or team membership is retained.')
7777
: undefined"
7878
@click="clickDeleteAcl(acl)">
79-
{{ acl.type === PERMISSION_TYPE_USER && acl.retainsAccessViaMembership
79+
{{ acl.retainsAccessViaMembership
8080
? t('deck', 'Remove extra permissions')
8181
: t('deck', 'Delete') }}
8282
</NcActionButton>

tests/integration/import/ImportExportTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ public function assertDatabase(string $owner = 'admin') {
279279
$stackMapper = self::getFreshService(StackMapper::class);
280280
$cardMapper = self::getFreshService(CardMapper::class);
281281

282-
$boards = $boardMapper->findAllByOwner($owner);
282+
$boards = $boardMapper->findAllByOwner($owner, Acl::PERMISSION_TYPE_USER);
283283
$boardNames = array_map(fn ($board) => $board->getTitle(), $boards);
284284
self::assertEquals(2, count($boards));
285285

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
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\Deck\Listeners;
11+
12+
use OCA\Circles\Events\CircleDestroyedEvent;
13+
use OCA\Circles\Model\Circle;
14+
use OCA\Deck\Db\Acl;
15+
use OCA\Deck\Db\AclMapper;
16+
use OCA\Deck\Db\Assignment;
17+
use OCA\Deck\Db\AssignmentMapper;
18+
use OCA\Deck\Db\Board;
19+
use OCA\Deck\Db\BoardMapper;
20+
use PHPUnit\Framework\TestCase;
21+
22+
class ParticipantCleanupListenerTest extends TestCase {
23+
private AclMapper $aclMapper;
24+
private AssignmentMapper $assignmentMapper;
25+
private BoardMapper $boardMapper;
26+
private ParticipantCleanupListener $listener;
27+
28+
protected function setUp(): void {
29+
parent::setUp();
30+
$this->aclMapper = $this->createMock(AclMapper::class);
31+
$this->assignmentMapper = $this->createMock(AssignmentMapper::class);
32+
$this->boardMapper = $this->createMock(BoardMapper::class);
33+
$this->listener = new ParticipantCleanupListener($this->aclMapper, $this->assignmentMapper, $this->boardMapper);
34+
}
35+
36+
public function testCircleDestroyedDeletesCircleOwnedBoardsAndCleansParticipantData(): void {
37+
$circleId = 'circle-123';
38+
39+
$circle = $this->createMock(Circle::class);
40+
$circle->expects($this->once())
41+
->method('getSingleId')
42+
->willReturn($circleId);
43+
44+
$event = $this->createMock(CircleDestroyedEvent::class);
45+
$event->expects($this->once())
46+
->method('getCircle')
47+
->willReturn($circle);
48+
49+
$boardOne = new Board();
50+
$boardTwo = new Board();
51+
$this->boardMapper->expects($this->once())
52+
->method('findAllByOwner')
53+
->with($circleId, Acl::PERMISSION_TYPE_CIRCLE)
54+
->willReturn([$boardOne, $boardTwo]);
55+
$this->boardMapper->expects($this->exactly(2))
56+
->method('delete');
57+
58+
$acl = new Acl();
59+
$this->aclMapper->expects($this->once())
60+
->method('findByParticipant')
61+
->with(Acl::PERMISSION_TYPE_CIRCLE, $circleId)
62+
->willReturn([$acl]);
63+
$this->aclMapper->expects($this->once())
64+
->method('delete')
65+
->with($acl);
66+
67+
$assignment = new Assignment();
68+
$this->assignmentMapper->expects($this->once())
69+
->method('findByParticipant')
70+
->with($circleId, Acl::PERMISSION_TYPE_CIRCLE)
71+
->willReturn([$assignment]);
72+
$this->assignmentMapper->expects($this->once())
73+
->method('delete')
74+
->with($assignment);
75+
76+
$this->listener->handle($event);
77+
}
78+
}

0 commit comments

Comments
 (0)