From aeea3fcedd0bed67d55d598cf63d99106d5503c0 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:03:51 +0200 Subject: [PATCH 1/6] feat(boards): log board lifecycle to the audit log Creating, cloning, importing, archiving and unarchiving boards, moving them to the trash and restoring them, permanently deleting them (after the trash retention or together with their team) and transferring their ownership (directly, through occ deck:transfer-ownership or when the owner leaves the team) are now written to the admin audit log. The operations are collected in the new OCA\Deck\AuditLog\Operation enum, with a short description of each for the documentation. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 45 +++++++ lib/Cron/DeleteCron.php | 13 ++ lib/Service/BoardService.php | 57 +++++++++ lib/Service/Importer/BoardImportService.php | 12 ++ lib/Service/TeamBoardService.php | 34 ++++++ tests/unit/AuditLog/OperationTest.php | 25 ++++ tests/unit/Cron/DeleteCronTest.php | 19 +++ tests/unit/Service/BoardServiceTest.php | 112 ++++++++++++++++++ .../Importer/BoardImportServiceTest.php | 18 ++- tests/unit/Service/TeamBoardServiceTest.php | 31 +++++ 10 files changed, 364 insertions(+), 2 deletions(-) create mode 100644 lib/AuditLog/Operation.php create mode 100644 tests/unit/AuditLog/OperationTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php new file mode 100644 index 0000000000..7ddb568197 --- /dev/null +++ b/lib/AuditLog/Operation.php @@ -0,0 +1,45 @@ + 'A board was created', + self::BoardDeleted => 'A board was moved to the trash', + self::BoardRestored => 'A board was restored from the trash', + self::BoardPurged => 'A board was permanently deleted, after the trash retention or together with its team', + self::BoardArchived => 'A board was archived', + self::BoardUnarchived => 'A board was unarchived', + self::BoardCloned => 'A board was cloned into a new board', + self::BoardImported => 'A board was imported from another system', + self::BoardOwnershipTransferred => 'The ownership of a board was transferred to another user', + }; + } +} diff --git a/lib/Cron/DeleteCron.php b/lib/Cron/DeleteCron.php index 0496db6e87..9ae63793ad 100644 --- a/lib/Cron/DeleteCron.php +++ b/lib/Cron/DeleteCron.php @@ -7,6 +7,7 @@ namespace OCA\Deck\Cron; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\Db\AttachmentMapper; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\CardMapper; @@ -18,6 +19,8 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\IJob; use OCP\BackgroundJob\TimedJob; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; class DeleteCron extends TimedJob { @@ -43,6 +46,7 @@ public function __construct( AttachmentMapper $attachmentMapper, StackMapper $stackMapper, DeckShareProvider $deckShareProvider, + private readonly IEventDispatcher $eventDispatcher, ) { parent::__construct($time); $this->boardMapper = $boardMapper; @@ -66,6 +70,15 @@ protected function run($argument) { $boards = $this->boardMapper->findToDelete($timeLimit); foreach ($boards as $board) { $this->boardMapper->delete($board); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s of "%s" was permanently deleted from the trash', + [ + 'boardId' => $board->getId(), + 'owner' => $board->getOwner(), + ], + false, + Operation::BoardPurged->value, + )); } $cards = $this->cardMapper->findToDelete($timeLimit, 500); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 2edb86a0b4..c84a340d0c 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -11,6 +11,7 @@ use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; use OCA\Deck\AppInfo\Application; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; @@ -48,6 +49,7 @@ use OCP\IL10N; use OCP\IURLGenerator; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ISecureRandom; use OCP\Server; use Psr\Container\ContainerExceptionInterface; @@ -225,6 +227,15 @@ public function create(string $title, string $userId, string $color): Board { ]); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $board, ActivityManager::SUBJECT_BOARD_CREATE, [], $userId); $this->changeHelper->boardChanged($board->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s of "%s" was created', + [ + 'boardId' => $board->getId(), + 'owner' => $userId, + ], + false, + Operation::BoardCreated->value, + )); return $board; } @@ -297,6 +308,14 @@ public function delete(int $id): Board { $board = $this->boardMapper->update($board); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $board, ActivityManager::SUBJECT_BOARD_DELETE); $this->changeHelper->boardChanged($board->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was moved to the trash', + [ + 'boardId' => $board->getId(), + ], + false, + Operation::BoardDeleted->value, + )); if (class_exists(UserShareAccessUpdatedEvent::class)) { $acls = $this->aclMapper->findAll($id); @@ -324,6 +343,14 @@ public function deleteUndo(int $id): Board { $board = $this->boardMapper->update($board); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $board, ActivityManager::SUBJECT_BOARD_RESTORE); $this->changeHelper->boardChanged($board->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was restored from the trash', + [ + 'boardId' => $board->getId(), + ], + false, + Operation::BoardRestored->value, + )); if (class_exists(UserShareAccessUpdatedEvent::class)) { $acls = $this->aclMapper->findAll($id); @@ -365,6 +392,7 @@ public function update(int $id, string $title, string $color, bool $archived): B $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE); $board = $this->find($id); $changes = new ChangeSet($board); + $wasArchived = $board->getArchived(); $board->setTitle($title); $board->setColor($color); $board->setArchived($archived); @@ -374,6 +402,14 @@ public function update(int $id, string $title, string $color, bool $archived): B $this->activityManager->triggerUpdateEvents(ActivityManager::DECK_OBJECT_BOARD, $changes, ActivityManager::SUBJECT_BOARD_UPDATE); $this->changeHelper->boardChanged($board->getId()); $this->eventDispatcher->dispatchTyped(new BoardUpdatedEvent($board->getId())); + if ($wasArchived !== $archived) { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + $archived ? 'Board %s was archived' : 'Board %s was unarchived', + ['boardId' => $board->getId()], + false, + ($archived ? Operation::BoardArchived : Operation::BoardUnarchived)->value, + )); + } return $board; } @@ -664,6 +700,16 @@ public function clone( if ($withCards) { $this->cloneCards($board, $newBoard, $withAssignments, $withLabels, $withDueDate, $moveCardsToLeftStack, $restoreArchivedCards); } + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was cloned into board %s of "%s"', + [ + 'sourceBoardId' => $board->getId(), + 'boardId' => $newBoard->getId(), + 'owner' => $userId, + ], + false, + Operation::BoardCloned->value, + )); return $this->find($newBoard->getId()); } @@ -692,6 +738,17 @@ public function transferBoardOwnership(int $boardId, string $newOwner, bool $cha $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); } $this->connection->commit(); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Ownership of board %s was transferred from "%s" to "%s"', + [ + 'boardId' => $boardId, + 'previousOwner' => $previousOwner, + 'newOwner' => $newOwner, + 'changeContent' => $changeContent, + ], + false, + Operation::BoardOwnershipTransferred->value, + )); return $this->boardMapper->find($boardId); } catch (\Throwable $e) { $this->connection->rollBack(); diff --git a/lib/Service/Importer/BoardImportService.php b/lib/Service/Importer/BoardImportService.php index 2bb17dee55..41ebf6522b 100644 --- a/lib/Service/Importer/BoardImportService.php +++ b/lib/Service/Importer/BoardImportService.php @@ -10,6 +10,7 @@ use JsonSchema\Constraints\Constraint; use JsonSchema\Validator; use OCA\Deck\AppInfo\Application; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\AssignmentMapper; @@ -33,6 +34,7 @@ use OCP\Comments\NotFoundException as CommentNotFoundException; use OCP\EventDispatcher\IEventDispatcher; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Server; use Psr\Log\LoggerInterface; @@ -222,6 +224,16 @@ public function importBoard(): void { $this->boardMapper->insert($board); $this->board = $board; + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s of "%s" was imported from %s', + [ + 'boardId' => $board->getId(), + 'owner' => $board->getOwner(), + 'importSystem' => $this->getSystem(), + ], + false, + Operation::BoardImported->value, + )); } public function getBoard(bool $reset = false): Board { diff --git a/lib/Service/TeamBoardService.php b/lib/Service/TeamBoardService.php index b52f74df5b..2191271312 100644 --- a/lib/Service/TeamBoardService.php +++ b/lib/Service/TeamBoardService.php @@ -9,12 +9,16 @@ namespace OCA\Deck\Service; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\Db\BoardMapper; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; class TeamBoardService { public function __construct( private BoardMapper $boardMapper, private CirclesService $circlesService, + private IEventDispatcher $eventDispatcher, ) { } @@ -30,10 +34,31 @@ public function handleMemberLeftTeam(string $teamId, string $userId): void { $nextOwner = $this->circlesService->findNextMemberUserId($teamId, $userId); if ($nextOwner === null) { $this->boardMapper->delete($board); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was permanently deleted because its owner "%s" left team "%s"', + [ + 'boardId' => $board->getId(), + 'owner' => $userId, + 'teamId' => $teamId, + ], + false, + Operation::BoardPurged->value, + )); continue; } $this->boardMapper->transferOwnership($userId, $nextOwner, $board->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Ownership of board %s was transferred from "%s" to "%s" after leaving team "%s"', + [ + 'boardId' => $board->getId(), + 'previousOwner' => $userId, + 'newOwner' => $nextOwner, + 'teamId' => $teamId, + ], + false, + Operation::BoardOwnershipTransferred->value, + )); } } @@ -43,6 +68,15 @@ public function handleMemberLeftTeam(string $teamId, string $userId): void { public function deleteBoardsAttachedToTeam(string $teamId): void { foreach ($this->boardMapper->findAllAttachedToTeam($teamId) as $board) { $this->boardMapper->delete($board); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was permanently deleted together with team "%s"', + [ + 'boardId' => $board->getId(), + 'teamId' => $teamId, + ], + false, + Operation::BoardPurged->value, + )); } } } diff --git a/tests/unit/AuditLog/OperationTest.php b/tests/unit/AuditLog/OperationTest.php new file mode 100644 index 0000000000..246004b7e1 --- /dev/null +++ b/tests/unit/AuditLog/OperationTest.php @@ -0,0 +1,25 @@ + [$operation], Operation::cases()); + } + + /** + * @dataProvider dataOperations + */ + public function testOperationFormat(Operation $operation): void { + $this->assertMatchesRegularExpression('/^deck\.[a-z][a-z0-9_]*\.[a-z][a-z0-9_]*$/', $operation->value); + $this->assertNotSame('', $operation->description()); + } +} diff --git a/tests/unit/Cron/DeleteCronTest.php b/tests/unit/Cron/DeleteCronTest.php index 9834798fc6..893196b38d 100644 --- a/tests/unit/Cron/DeleteCronTest.php +++ b/tests/unit/Cron/DeleteCronTest.php @@ -38,6 +38,9 @@ use OCA\Deck\Service\IAttachmentService; use OCA\Deck\Sharing\DeckShareProvider; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -59,6 +62,9 @@ class DeleteCronTest extends TestCase { private $stackMapper; /** @var DeckShareProvider */ private $deckShareProvider; + private IEventDispatcher&MockObject $eventDispatcher; + /** @var list */ + private array $auditEvents = []; /** @var DeleteCron */ protected $deleteCron; @@ -72,6 +78,14 @@ public function setUp(): void { $this->attachmentMapper = $this->createMock(AttachmentMapper::class); $this->stackMapper = $this->createMock(StackMapper::class); $this->deckShareProvider = $this->createMock(DeckShareProvider::class); + $this->auditEvents = []; + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event): void { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + } + }); $this->deleteCron = new DeleteCron( $this->timeFactory, $this->configService, @@ -81,6 +95,7 @@ public function setUp(): void { $this->attachmentMapper, $this->stackMapper, $this->deckShareProvider, + $this->eventDispatcher, ); } @@ -162,6 +177,10 @@ public function testDeleteCron() { ); $this->invokePrivate($this->deleteCron, 'run', [null]); + + $this->assertCount(count($boards), $this->auditEvents); + $this->assertSame('deck.board.purged', $this->auditEvents[0]->getOperation()); + $this->assertSame(['boardId' => 1, 'owner' => null], $this->auditEvents[0]->getParameters()); } public function testDeleteCronInvalidAttachment() { diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index c51a049d99..039b72213f 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -56,12 +56,14 @@ use OCA\Deck\NoPermissionException; use OCA\Deck\Notification\NotificationHelper; use OCA\Deck\Validators\BoardServiceValidator; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -636,4 +638,114 @@ public function testDeleteAcl() { ->with(new AclDeletedEvent($acl)); $this->assertEquals($acl, $this->service->deleteAcl(123)); } + + /** + * @return \ArrayObject + */ + private function collectAuditEvents(): \ArrayObject { + $auditEvents = new \ArrayObject(); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use ($auditEvents): void { + if ($event instanceof CriticalActionPerformedEvent) { + $auditEvents[] = $event; + } + }); + return $auditEvents; + } + + public function testCreateLogsCriticalAction(): void { + $board = new Board(); + $board->setId(5); + $board->setOwner('admin'); + $this->boardMapper->method('insert')->willReturn($board); + $this->permissionService->method('canCreate')->willReturn(true); + $auditEvents = $this->collectAuditEvents(); + + $this->service->create('MyBoard', 'admin', '00ff00'); + + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board.created', $auditEvents[0]->getOperation()); + $this->assertSame('Board %s of "%s" was created', $auditEvents[0]->getLogMessage()); + $this->assertSame(['boardId' => 5, 'owner' => 'admin'], $auditEvents[0]->getParameters()); + } + + public function testDeleteAndRestoreLogCriticalAction(): void { + $board = new Board(); + $board->setId(42); + $board->setOwner('admin'); + $board->setDeletedAt(0); + $this->boardMapper->method('find')->willReturn($board); + $this->boardMapper->method('update')->willReturnArgument(0); + $this->permissionService->method('findUsers')->willReturn(['admin' => 'admin']); + $this->sessionMapper->method('findAllActive')->willReturn([]); + $this->aclMapper->method('findAll')->willReturn([]); + $auditEvents = $this->collectAuditEvents(); + + $this->service->delete(42); + $this->service->deleteUndo(42); + + $this->assertSame(['deck.board.deleted', 'deck.board.restored'], array_map(static fn ($event) => $event->getOperation(), $auditEvents->getArrayCopy())); + $this->assertSame(['boardId' => 42], $auditEvents[1]->getParameters()); + } + + public function testUpdateArchivedLogsCriticalAction(): void { + $board = new Board(); + $board->setId(123); + $board->setOwner('admin'); + $board->setArchived(false); + $this->boardMapper->method('find')->willReturn($board); + $this->boardMapper->method('update')->willReturnArgument(0); + $this->permissionService->method('findUsers')->willReturn(['admin' => 'admin']); + $this->sessionMapper->method('findAllActive')->willReturn([]); + $auditEvents = $this->collectAuditEvents(); + + $this->service->update(123, 'MyBoard', 'ffffff', true); + $this->service->update(123, 'MyBoard', 'ffffff', true); + + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board.archived', $auditEvents[0]->getOperation()); + $this->assertSame(['boardId' => 123], $auditEvents[0]->getParameters()); + } + + public function testCloneLogsCriticalAction(): void { + $board = new Board(); + $board->setId(1); + $board->setOwner('admin'); + $board->setTitle('Board'); + $board->setColor('ffffff'); + $this->permissionService->method('canCreate')->willReturn(true); + $this->permissionService->method('findUsers')->willReturn(['admin' => 'admin']); + $this->boardMapper->method('find')->willReturn($board); + $this->boardMapper->method('insert') + ->willReturnCallback(static function (Board $newBoard): Board { + $newBoard->setId(2); + return $newBoard; + }); + $this->aclMapper->method('findAll')->willReturn([]); + $this->labelMapper->method('findAll')->willReturn([]); + $this->stackMapper->method('findAll')->willReturn([]); + $this->sessionMapper->method('findAllActive')->willReturn([]); + $auditEvents = $this->collectAuditEvents(); + + $this->service->clone(1, 'admin'); + + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board.cloned', $auditEvents[0]->getOperation()); + $this->assertSame(['sourceBoardId' => 1, 'boardId' => 2, 'owner' => 'admin'], $auditEvents[0]->getParameters()); + } + + public function testTransferBoardOwnershipLogsCriticalAction(): void { + $board = new Board(); + $board->setId(7); + $board->setOwner('admin'); + $this->boardMapper->method('find')->willReturn($board); + $auditEvents = $this->collectAuditEvents(); + + $this->service->transferBoardOwnership(7, 'newowner', true); + + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board.ownership_transferred', $auditEvents[0]->getOperation()); + $this->assertSame('Ownership of board %s was transferred from "%s" to "%s"', $auditEvents[0]->getLogMessage()); + $this->assertSame(['boardId' => 7, 'previousOwner' => 'admin', 'newOwner' => 'newowner', 'changeContent' => true], $auditEvents[0]->getParameters()); + } } diff --git a/tests/unit/Service/Importer/BoardImportServiceTest.php b/tests/unit/Service/Importer/BoardImportServiceTest.php index 5941a855bc..d3ddabc7e9 100644 --- a/tests/unit/Service/Importer/BoardImportServiceTest.php +++ b/tests/unit/Service/Importer/BoardImportServiceTest.php @@ -42,14 +42,18 @@ use OCA\Deck\Service\AttachmentService; use OCA\Deck\Service\Importer\Systems\TrelloJsonService; use OCP\Comments\ICommentsManager; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; class BoardImportServiceTest extends \Test\TestCase { + /** @var list */ + private array $auditEvents = []; /** @var IDBConnection|MockObject */ protected $dbConn; /** @var IUserManager|MockObject */ @@ -109,7 +113,14 @@ public function setUp(): void { $this->eventDispatcher ->method('dispatchTyped') - ->willReturnCallback(function (BoardImportGetAllowedEvent $event) { + ->willReturnCallback(function (Event $event) { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + return; + } + if (!$event instanceof BoardImportGetAllowedEvent) { + return; + } $event->getService()->addAllowedImportSystem([ 'name' => TrelloJsonService::$name, 'class' => TrelloJsonService::class, @@ -218,6 +229,9 @@ public function testImportSuccess() { ->willReturn($assignment); $this->boardImportService->import(); - self::assertTrue(true); + + $auditOperations = array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $this->auditEvents); + $this->assertSame(['deck.board.imported'], $auditOperations); + $this->assertSame('trelloJson', $this->auditEvents[0]->getParameters()['importSystem']); } } diff --git a/tests/unit/Service/TeamBoardServiceTest.php b/tests/unit/Service/TeamBoardServiceTest.php index 4100e196f7..e648a2b2ff 100644 --- a/tests/unit/Service/TeamBoardServiceTest.php +++ b/tests/unit/Service/TeamBoardServiceTest.php @@ -11,6 +11,9 @@ use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -20,6 +23,9 @@ class TeamBoardServiceTest extends TestCase { /** @var CirclesService|MockObject */ private $circlesService; private TeamBoardService $service; + private IEventDispatcher&MockObject $eventDispatcher; + /** @var list */ + private array $auditEvents = []; private $userId1 = 'user1'; private $userId2 = 'user2'; @@ -27,12 +33,28 @@ public function setUp(): void { parent::setUp(); $this->boardMapper = $this->createMock(BoardMapper::class); $this->circlesService = $this->createMock(CirclesService::class); + $this->auditEvents = []; + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event): void { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + } + }); $this->service = new TeamBoardService( $this->boardMapper, $this->circlesService, + $this->eventDispatcher, ); } + /** + * @return list + */ + private function getAuditOperations(): array { + return array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $this->auditEvents); + } + public function testHandleMemberLeftTeamTransfersOwnership(): void { $board = new Board(); $board->setId(10); @@ -54,6 +76,9 @@ public function testHandleMemberLeftTeamTransfersOwnership(): void { ->method('delete'); $this->service->handleMemberLeftTeam('team-a', $this->userId1); + + $this->assertSame(['deck.board.ownership_transferred'], $this->getAuditOperations()); + $this->assertSame(['boardId' => 10, 'previousOwner' => $this->userId1, 'newOwner' => $this->userId2, 'teamId' => 'team-a'], $this->auditEvents[0]->getParameters()); } public function testHandleMemberLeftTeamDeletesWhenNoNextMember(): void { @@ -77,6 +102,9 @@ public function testHandleMemberLeftTeamDeletesWhenNoNextMember(): void { ->method('transferOwnership'); $this->service->handleMemberLeftTeam('team-a', $this->userId1); + + $this->assertSame(['deck.board.purged'], $this->getAuditOperations()); + $this->assertSame(['boardId' => 11, 'owner' => $this->userId1, 'teamId' => 'team-a'], $this->auditEvents[0]->getParameters()); } public function testHandleMemberLeftTeamSkipsNonOwnerBoards(): void { @@ -114,5 +142,8 @@ public function testDeleteBoardsAttachedToTeam(): void { ->withConsecutive([$boardA], [$boardB]); $this->service->deleteBoardsAttachedToTeam('team-a'); + + $this->assertSame(['deck.board.purged', 'deck.board.purged'], $this->getAuditOperations()); + $this->assertSame(['boardId' => 21, 'teamId' => 'team-a'], $this->auditEvents[1]->getParameters()); } } From af9ee97e3755dc03458fc09e2dfd145121337240 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:06:50 +0200 Subject: [PATCH 2/6] feat(sharing): log board shares to the audit log Sharing a board with a user, group, team or remote user, changing the permissions of a share, removing a share, leaving a shared board and receiving a board share from another server are now written to the admin audit log. Deck shares are not core shares, so admin_audit did not see any of them before. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 12 +++ lib/Federation/DeckFederationProvider.php | 16 ++++ lib/Service/BoardService.php | 36 +++++++++ .../Federation/DeckFederationProviderTest.php | 73 +++++++++++++++++++ tests/unit/Service/BoardServiceTest.php | 39 ++++++++-- 5 files changed, 171 insertions(+), 5 deletions(-) create mode 100644 tests/unit/Federation/DeckFederationProviderTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 7ddb568197..b713b3ce1f 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -26,6 +26,13 @@ enum Operation: string { case BoardImported = 'deck.board.imported'; case BoardOwnershipTransferred = 'deck.board.ownership_transferred'; + // Sharing + case BoardShareCreated = 'deck.board_share.created'; + case BoardShareUpdated = 'deck.board_share.updated'; + case BoardShareDeleted = 'deck.board_share.deleted'; + case BoardShareLeft = 'deck.board_share.left'; + case BoardShareReceived = 'deck.board_share.received'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -40,6 +47,11 @@ public function description(): string { self::BoardCloned => 'A board was cloned into a new board', self::BoardImported => 'A board was imported from another system', self::BoardOwnershipTransferred => 'The ownership of a board was transferred to another user', + self::BoardShareCreated => 'A board was shared with a user, group, team or remote user', + self::BoardShareUpdated => 'The permissions of a board share were changed', + self::BoardShareDeleted => 'A board share was removed', + self::BoardShareLeft => 'A user left a board shared with them', + self::BoardShareReceived => 'A board was shared with a local user from another server', }; } } diff --git a/lib/Federation/DeckFederationProvider.php b/lib/Federation/DeckFederationProvider.php index 558b0d38cb..f05629badb 100644 --- a/lib/Federation/DeckFederationProvider.php +++ b/lib/Federation/DeckFederationProvider.php @@ -8,6 +8,7 @@ namespace OCA\Deck\Federation; use Exception; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\Board; @@ -17,9 +18,11 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\Common\Exception\NotFoundException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Federation\ICloudFederationShare; use OCP\Federation\ICloudIdManager; use OCP\Federation\ISignedCloudFederationProvider; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Notification\IManager as INotificationManager; class DeckFederationProvider implements ISignedCloudFederationProvider { @@ -32,6 +35,7 @@ public function __construct( private AclMapper $aclMapper, private ChangeHelper $changeHelper, private ConfigService $configService, + private IEventDispatcher $eventDispatcher, ) { } @@ -60,6 +64,18 @@ public function shareReceived(ICloudFederationShare $share): string { $this->changeHelper->boardChanged($insertedBoard->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Board %s was shared by "%s" with "%s" from the remote board %s', + [ + 'boardId' => $insertedBoard->getId(), + 'sharedBy' => $share->getSharedBy(), + 'sharedWith' => $share->getShareWith(), + 'remoteBoardId' => $share->getProviderId(), + ], + false, + Operation::BoardShareReceived->value, + )); + $notification = $this->notificationManager->createNotification(); $notification->setApp('deck'); $notification->setUser($share->getShareWith()); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index c84a340d0c..cb38c8d4b4 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -508,6 +508,7 @@ public function addAcl(int $boardId, int $type, $participant, bool $edit, bool $ } $this->eventDispatcher->dispatchTyped(new AclCreatedEvent($acl)); + $this->logBoardShare($newAcl, 'created', Operation::BoardShareCreated); // Sync permissions to remote server since shareReceived() creates ACL with no permissions if ($type === Acl::PERMISSION_TYPE_REMOTE && ($edit || $share || $manage)) { @@ -562,6 +563,7 @@ public function updateAcl(int $id, bool $edit, bool $share, bool $manage): Acl { } $this->eventDispatcher->dispatchTyped(new AclUpdatedEvent($acl)); + $this->logBoardShare($acl, 'updated', Operation::BoardShareUpdated); return $acl; } @@ -598,6 +600,7 @@ public function deleteAcl(int $id): ?Acl { $deletedAcl = $this->aclMapper->delete($acl); $this->eventDispatcher->dispatchTyped(new AclDeletedEvent($acl)); + $this->logBoardShare($acl, 'removed', Operation::BoardShareDeleted); return $deletedAcl; } @@ -629,6 +632,7 @@ public function leave(int $boardId): ?Acl { $deletedAcl = $this->aclMapper->delete($acl); $this->eventDispatcher->dispatchTyped(new AclDeletedEvent($acl)); + $this->logBoardShare($acl, 'left', Operation::BoardShareLeft); return $deletedAcl; } @@ -928,6 +932,38 @@ private function enrichWithUsers(Board $board): void { /** * Clean a given board data from the Cache */ + private function logBoardShare(Acl $acl, string $action, Operation $operation): void { + $permissions = ['read']; + if ($acl->getPermissionEdit()) { + $permissions[] = 'edit'; + } + if ($acl->getPermissionShare()) { + $permissions[] = 'share'; + } + if ($acl->getPermissionManage()) { + $permissions[] = 'manage'; + } + + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Share %s of board %s with %s "%s" was ' . $action . ' (permissions: %s)', + [ + 'aclId' => $acl->getId(), + 'boardId' => $acl->getBoardId(), + 'shareType' => match ($acl->getType()) { + Acl::PERMISSION_TYPE_USER => 'user', + Acl::PERMISSION_TYPE_GROUP => 'group', + Acl::PERMISSION_TYPE_REMOTE => 'remote user', + Acl::PERMISSION_TYPE_CIRCLE => 'team', + default => 'unknown', + }, + 'sharedWith' => $acl->getParticipant(), + 'permissions' => implode(', ', $permissions), + ], + false, + $operation->value, + )); + } + private function clearBoardFromCache(Board $board): void { $boardId = $board->getId(); $boardOwnerId = $board->getOwner(); diff --git a/tests/unit/Federation/DeckFederationProviderTest.php b/tests/unit/Federation/DeckFederationProviderTest.php new file mode 100644 index 0000000000..fe3fe69faf --- /dev/null +++ b/tests/unit/Federation/DeckFederationProviderTest.php @@ -0,0 +1,73 @@ +boardMapper = $this->createMock(BoardMapper::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $notification = $this->createMock(INotification::class); + foreach (['setApp', 'setUser', 'setDateTime', 'setObject', 'setSubject'] as $method) { + $notification->method($method)->willReturnSelf(); + } + $notificationManager = $this->createMock(INotificationManager::class); + $notificationManager->method('createNotification')->willReturn($notification); + + $this->provider = new DeckFederationProvider( + $this->createMock(ICloudIdManager::class), + $notificationManager, + $this->boardMapper, + $this->createMock(AclMapper::class), + $this->createMock(ChangeHelper::class), + $this->createMock(ConfigService::class), + $this->eventDispatcher, + ); + } + + public function testShareReceivedLogsCriticalAction(): void { + $share = $this->createMock(ICloudFederationShare::class); + $share->method('getResourceName')->willReturn('Remote board'); + $share->method('getProviderId')->willReturn('77'); + $share->method('getSharedBy')->willReturn('alice@remote.example'); + $share->method('getShareWith')->willReturn('bob'); + $this->boardMapper->method('insert') + ->willReturnCallback(static function (Board $board): Board { + $board->setId(5); + return $board; + }); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.board_share.received' + && $event->getLogMessage() === 'Board %s was shared by "%s" with "%s" from the remote board %s' + && $event->getParameters() === ['boardId' => 5, 'sharedBy' => 'alice@remote.example', 'sharedWith' => 'bob', 'remoteBoardId' => '77'])); + + $this->assertSame('5', $this->provider->shareReceived($share)); + } +} diff --git a/tests/unit/Service/BoardServiceTest.php b/tests/unit/Service/BoardServiceTest.php index 039b72213f..7e52be8312 100644 --- a/tests/unit/Service/BoardServiceTest.php +++ b/tests/unit/Service/BoardServiceTest.php @@ -563,9 +563,9 @@ public function testAddAclExtendPermission($currentUserAcl, $providedAcl, $resul && $actual->getLastModifiedAt() > 0; })) ->willReturn($acl); - $this->eventDispatcher->expects(self::once()) + $this->eventDispatcher->expects(self::exactly(2)) ->method('dispatchTyped') - ->with($this->callback(function (AclCreatedEvent $event) use ($acl) { + ->withConsecutive([$this->callback(function (AclCreatedEvent $event) use ($acl) { $eventAcl = $event->getAcl(); return $eventAcl->getBoardId() === $acl->getBoardId() && $eventAcl->getType() === $acl->getType() @@ -575,7 +575,10 @@ public function testAddAclExtendPermission($currentUserAcl, $providedAcl, $resul && $eventAcl->getPermissionManage() === $acl->getPermissionManage() && $eventAcl->getCreatedAt() > 0 && $eventAcl->getLastModifiedAt() > 0; - })); + })], [$this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.board_share.created' + && $event->getParameters()['boardId'] === 123 + && $event->getParameters()['sharedWith'] === 'admin' + && $event->getParameters()['shareType'] === 'user')]); $this->assertEquals($expected, $this->service->addAcl( 123, Acl::PERMISSION_TYPE_USER, 'admin', $providedAcl[0], $providedAcl[1], $providedAcl[2] )); @@ -599,10 +602,15 @@ public function testUpdateAcl() { ->with($acl) ->willReturn($acl); + $auditEvents = $this->collectAuditEvents(); + $result = $this->service->updateAcl( 123, false, false, false ); + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board_share.updated', $auditEvents[0]->getOperation()); + $this->assertSame('read', $auditEvents[0]->getParameters()['permissions']); $this->assertFalse($result->getPermissionEdit()); $this->assertFalse($result->getPermissionShare()); $this->assertFalse($result->getPermissionManage()); @@ -633,12 +641,33 @@ public function testDeleteAcl() { ->method('delete') ->with($acl) ->willReturn($acl); - $this->eventDispatcher->expects(self::once()) + $this->eventDispatcher->expects(self::exactly(2)) ->method('dispatchTyped') - ->with(new AclDeletedEvent($acl)); + ->withConsecutive([new AclDeletedEvent($acl)], [$this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.board_share.deleted' + && $event->getLogMessage() === 'Share %s of board %s with %s "%s" was removed (permissions: %s)' + && $event->getParameters() === ['aclId' => null, 'boardId' => 123, 'shareType' => 'user', 'sharedWith' => 'admin', 'permissions' => 'read, edit, share, manage'])]); $this->assertEquals($acl, $this->service->deleteAcl(123)); } + public function testLeaveLogsCriticalAction(): void { + $acl = new Acl(); + $acl->setId(9); + $acl->setBoardId(123); + $acl->setType(Acl::PERMISSION_TYPE_USER); + $acl->setParticipant('admin'); + $acl->setPermissionEdit(true); + $this->permissionService->method('userIsBoardOwner')->willReturn(false); + $this->aclMapper->method('findParticipantFromBoard')->willReturn($acl); + $this->aclMapper->method('delete')->willReturn($acl); + $auditEvents = $this->collectAuditEvents(); + + $this->service->leave(123); + + $this->assertCount(1, $auditEvents); + $this->assertSame('deck.board_share.left', $auditEvents[0]->getOperation()); + $this->assertSame(['aclId' => 9, 'boardId' => 123, 'shareType' => 'user', 'sharedWith' => 'admin', 'permissions' => 'read, edit'], $auditEvents[0]->getParameters()); + } + /** * @return \ArrayObject */ From b6cdf484728214749d25919d41ca964c14dd7848 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:10:20 +0200 Subject: [PATCH 3/6] feat(cards): log card, stack, assignment and label changes to the audit log Creating cards, moving them to the trash and restoring them, moving them to another board (through an update or drag and drop), archiving and unarchiving them, assigning and unassigning users, groups and teams, moving stacks to the trash and restoring them, and deleting labels are now written to the admin audit log. Card titles and descriptions are never logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 24 +++++++ lib/Service/AssignmentService.php | 23 +++++++ lib/Service/CardService.php | 57 ++++++++++++++++ lib/Service/LabelService.php | 11 +++ lib/Service/StackService.php | 17 +++++ tests/unit/Service/AssignmentServiceTest.php | 29 ++++++++ tests/unit/Service/CardServiceTest.php | 72 ++++++++++++++++++++ tests/unit/Service/LabelServiceTest.php | 10 +++ tests/unit/Service/StackServiceTest.php | 42 ++++++++++++ 9 files changed, 285 insertions(+) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index b713b3ce1f..85ff5f45e8 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -33,6 +33,19 @@ enum Operation: string { case BoardShareLeft = 'deck.board_share.left'; case BoardShareReceived = 'deck.board_share.received'; + // Stacks, cards and labels + case StackDeleted = 'deck.stack.deleted'; + case StackRestored = 'deck.stack.restored'; + case CardCreated = 'deck.card.created'; + case CardDeleted = 'deck.card.deleted'; + case CardRestored = 'deck.card.restored'; + case CardMoved = 'deck.card.moved'; + case CardArchived = 'deck.card.archived'; + case CardUnarchived = 'deck.card.unarchived'; + case CardAssignmentCreated = 'deck.card_assignment.created'; + case CardAssignmentDeleted = 'deck.card_assignment.deleted'; + case LabelDeleted = 'deck.label.deleted'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -52,6 +65,17 @@ public function description(): string { self::BoardShareDeleted => 'A board share was removed', self::BoardShareLeft => 'A user left a board shared with them', self::BoardShareReceived => 'A board was shared with a local user from another server', + self::StackDeleted => 'A stack was moved to the trash with all its cards', + self::StackRestored => 'A stack was restored from the trash', + self::CardCreated => 'A card was created', + self::CardDeleted => 'A card was moved to the trash', + self::CardRestored => 'A card was restored from the trash', + self::CardMoved => 'A card was moved to another board', + self::CardArchived => 'A card was archived', + self::CardUnarchived => 'A card was unarchived', + self::CardAssignmentCreated => 'A user, group or team was assigned to a card', + self::CardAssignmentDeleted => 'A user, group or team was unassigned from a card', + self::LabelDeleted => 'A label was deleted and removed from all cards of its board', }; } } diff --git a/lib/Service/AssignmentService.php b/lib/Service/AssignmentService.php index 292aa672ab..32010f77fd 100644 --- a/lib/Service/AssignmentService.php +++ b/lib/Service/AssignmentService.php @@ -8,6 +8,7 @@ namespace OCA\Deck\Service; use OCA\Deck\Activity\ActivityManager; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; @@ -23,6 +24,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; class AssignmentService { public function __construct( @@ -87,6 +89,7 @@ public function assignUser(int $cardId, string $userId, int $type = Assignment:: $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_CARD_USER_ASSIGN, ['assigneduser' => $userId]); $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $this->logAssignment($cardId, $card->getStackId(), $userId, $type, 'assigned to', Operation::CardAssignmentCreated); return $assignment; } @@ -114,10 +117,30 @@ public function unassignUser(int $cardId, string $userId, int $type = 0): Assign $this->changeHelper->cardChanged($cardId); $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card)); + $this->logAssignment($cardId, $card->getStackId(), $userId, $type, 'unassigned from', Operation::CardAssignmentDeleted); return $assignment; } } throw new NotFoundException('No assignment for ' . $userId . 'found.'); } + + private function logAssignment(int $cardId, ?int $stackId, string $assignee, int $type, string $action, Operation $operation): void { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + '%s "%s" was ' . $action . ' card %s of stack %s', + [ + 'assigneeType' => match ($type) { + Assignment::TYPE_GROUP => 'group', + Assignment::TYPE_CIRCLE => 'team', + Assignment::TYPE_REMOTE => 'remote user', + default => 'user', + }, + 'assignee' => $assignee, + 'cardId' => $cardId, + 'stackId' => $stackId, + ], + false, + $operation->value, + )); + } } diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 7ebc8d8a6f..c04d65f9cb 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -9,6 +9,7 @@ use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AssignmentMapper; @@ -34,6 +35,7 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use Psr\Log\LoggerInterface; class CardService { @@ -225,6 +227,10 @@ public function create(string $title, int $stackId, string $type, int $order, st $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_CARD_CREATE, [], $card->getOwner()); $this->changeHelper->cardChanged($card->getId(), false); $this->eventDispatcher->dispatchTyped(new CardCreatedEvent($card)); + $this->logCardAction('Card %s was created in stack %s', [ + 'cardId' => $card->getId(), + 'stackId' => $card->getStackId(), + ], Operation::CardCreated); [$card] = $this->enrichCards([$card]); @@ -251,6 +257,10 @@ public function delete(int $id): Card { $this->notificationHelper->markDuedateAsRead($card); $this->changeHelper->cardChanged($card->getId(), false); $this->eventDispatcher->dispatchTyped(new CardDeletedEvent($card)); + $this->logCardAction('Card %s of stack %s was moved to the trash', [ + 'cardId' => $id, + 'stackId' => $card->getStackId(), + ], Operation::CardDeleted); return $card; } @@ -374,6 +384,20 @@ public function update(int $id, string $title, int $stackId, string $type, strin $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $before = $changes->getBefore(); + if ($before->getDeletedAt() > 0 && $card->getDeletedAt() === 0) { + $this->logCardAction('Card %s of stack %s was restored from the trash', [ + 'cardId' => $card->getId(), + 'stackId' => $card->getStackId(), + ], Operation::CardRestored); + } + if ($boardId !== $oldBoardId) { + $this->logCardMoved($card->getId(), $oldBoardId, $boardId); + } + if ($before->getArchived() !== $card->getArchived()) { + $this->logCardArchived($card->getId(), $card->getStackId(), $card->getArchived()); + } + [$card] = $this->enrichCards([$card]); return $card; @@ -466,6 +490,8 @@ public function reorder(int $id, int $stackId, int $order): array { $changes = new ChangeSet($card); $oldStackId = $card->getStackId(); $card->setStackId($stackId); + $newStack = null; + $oldStack = null; if ($stackId !== $oldStackId) { $newStack = $this->stackMapper->find($stackId); @@ -487,9 +513,38 @@ public function reorder(int $id, int $stackId, int $order): array { $this->changeHelper->cardChanged($id, false); $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + if ($newStack !== null) { + $oldStack ??= $this->stackMapper->find($oldStackId); + if ($newStack->getBoardId() !== $oldStack->getBoardId()) { + $this->logCardMoved($id, $oldStack->getBoardId(), $newStack->getBoardId()); + } + } + return $result; } + private function logCardMoved(int $cardId, ?int $fromBoardId, ?int $toBoardId): void { + $this->logCardAction('Card %s was moved from board %s to board %s', [ + 'cardId' => $cardId, + 'fromBoardId' => $fromBoardId, + 'toBoardId' => $toBoardId, + ], Operation::CardMoved); + } + + private function logCardArchived(int $cardId, ?int $stackId, bool $archived): void { + $this->logCardAction($archived ? 'Card %s of stack %s was archived' : 'Card %s of stack %s was unarchived', [ + 'cardId' => $cardId, + 'stackId' => $stackId, + ], $archived ? Operation::CardArchived : Operation::CardUnarchived); + } + + /** + * @param array $parameters + */ + private function logCardAction(string $message, array $parameters, Operation $operation): void { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent($message, $parameters, false, $operation->value)); + } + /** * @return list */ @@ -543,6 +598,7 @@ public function archive(int $id): Card { $this->changeHelper->cardChanged($id, false); $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->logCardArchived($id, $card->getStackId(), true); return $newCard; } @@ -569,6 +625,7 @@ public function unarchive(int $id): Card { $this->changeHelper->cardChanged($id, false); $this->eventDispatcher->dispatchTyped(new CardUpdatedEvent($card, $changes->getBefore())); + $this->logCardArchived($id, $card->getStackId(), false); return $newCard; } diff --git a/lib/Service/LabelService.php b/lib/Service/LabelService.php index 625ef8a442..a22ccbe922 100644 --- a/lib/Service/LabelService.php +++ b/lib/Service/LabelService.php @@ -7,6 +7,7 @@ namespace OCA\Deck\Service; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\ChangeHelper; @@ -14,6 +15,8 @@ use OCA\Deck\Db\LabelMapper; use OCA\Deck\StatusException; use OCA\Deck\Validators\LabelServiceValidator; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; class LabelService { @@ -23,6 +26,7 @@ public function __construct( private BoardService $boardService, private ChangeHelper $changeHelper, private LabelServiceValidator $labelServiceValidator, + private IEventDispatcher $eventDispatcher, ) { } @@ -94,6 +98,13 @@ public function delete(int $id): Label { $label = $this->labelMapper->delete($this->find($id)); $this->changeHelper->boardChanged($label->getBoardId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Label %s of board %s was deleted', + ['labelId' => $label->getId(), 'boardId' => $label->getBoardId()], + false, + Operation::LabelDeleted->value, + )); + return $label; } diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php index 16e8d8b415..0e40e7cc6a 100644 --- a/lib/Service/StackService.php +++ b/lib/Service/StackService.php @@ -9,6 +9,7 @@ use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\AssignmentMapper; @@ -25,6 +26,7 @@ use OCA\Deck\StatusException; use OCA\Deck\Validators\StackServiceValidator; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use Psr\Log\LoggerInterface; class StackService { @@ -219,6 +221,12 @@ public function delete(int $id): Stack { ); $this->changeHelper->boardChanged($stack->getBoardId()); $this->eventDispatcher->dispatchTyped(new BoardUpdatedEvent($stack->getBoardId())); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Stack %s of board %s was moved to the trash', + ['stackId' => $stack->getId(), 'boardId' => $stack->getBoardId()], + false, + Operation::StackDeleted->value, + )); $this->enrichStacksWithCards([$stack]); return $stack; @@ -243,6 +251,7 @@ public function update(int $id, string $title, int $boardId, int $order, ?int $d $stack = $this->stackMapper->find($id); $changes = new ChangeSet($stack); + $wasDeleted = $stack->getDeletedAt() > 0; $stack->setTitle($title); $stack->setBoardId($boardId); $stack->setOrder($order); @@ -254,6 +263,14 @@ public function update(int $id, string $title, int $boardId, int $order, ?int $d ); $this->changeHelper->boardChanged($stack->getBoardId()); $this->eventDispatcher->dispatchTyped(new BoardUpdatedEvent($stack->getBoardId())); + if ($wasDeleted && $stack->getDeletedAt() === 0) { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Stack %s of board %s was restored from the trash', + ['stackId' => $stack->getId(), 'boardId' => $stack->getBoardId()], + false, + Operation::StackRestored->value, + )); + } return $stack; } diff --git a/tests/unit/Service/AssignmentServiceTest.php b/tests/unit/Service/AssignmentServiceTest.php index 01254380d3..e99eab18a8 100644 --- a/tests/unit/Service/AssignmentServiceTest.php +++ b/tests/unit/Service/AssignmentServiceTest.php @@ -35,7 +35,9 @@ use OCA\Deck\Notification\NotificationHelper; use OCA\Deck\Validators\AssignmentServiceValidator; use OCP\Activity\IEvent; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -143,8 +145,11 @@ public function testAssignUser() { ->method('insert') ->with($assignment) ->willReturn($assignment); + $auditEvents = $this->collectAuditEvents(); $actual = $this->assignmentService->assignUser(123, 'admin'); $this->assertEquals($assignment, $actual); + $this->assertSame(['deck.card_assignment.created'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['assigneeType' => 'user', 'assignee' => 'admin', 'cardId' => 123, 'stackId' => null], $auditEvents[0]->getParameters()); } public function testAssignUserNoParticipant() { @@ -206,8 +211,10 @@ public function testUnassignUserExisting() { ->method('delete') ->with($assignment) ->willReturn($assignment); + $auditEvents = $this->collectAuditEvents(); $actual = $this->assignmentService->unassignUser(123, 'admin'); $this->assertEquals($assignment, $actual); + $this->assertSame(['deck.card_assignment.deleted'], $this->getAuditOperations($auditEvents)); } public function testUnassignUserNotExisting() { @@ -226,4 +233,26 @@ public function testUnassignUserNotExisting() { $this->expectException(NotFoundException::class); $actual = $this->assignmentService->unassignUser(123, 'user'); } + + /** + * @return \ArrayObject + */ + private function collectAuditEvents(): \ArrayObject { + $auditEvents = new \ArrayObject(); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use ($auditEvents): void { + if ($event instanceof CriticalActionPerformedEvent) { + $auditEvents[] = $event; + } + }); + return $auditEvents; + } + + /** + * @param \ArrayObject $auditEvents + * @return list + */ + private function getAuditOperations(\ArrayObject $auditEvents): array { + return array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $auditEvents->getArrayCopy()); + } } diff --git a/tests/unit/Service/CardServiceTest.php b/tests/unit/Service/CardServiceTest.php index 94a96a7a98..ca76e085fc 100644 --- a/tests/unit/Service/CardServiceTest.php +++ b/tests/unit/Service/CardServiceTest.php @@ -44,12 +44,14 @@ use OCP\Activity\IEvent; use OCP\Collaboration\Reference\IReferenceManager; use OCP\Comments\ICommentsManager; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -243,8 +245,11 @@ public function testCreate() { ->method('find') ->with(123) ->willReturn($stack); + $auditEvents = $this->collectAuditEvents(); $b = $this->cardService->create('Card title', 123, 'text', 999, 'admin', '', null, null, '00ff00'); + $this->assertSame(['deck.card.created'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['cardId' => 0, 'stackId' => 123], $auditEvents[0]->getParameters()); $this->assertEquals($b->getTitle(), 'Card title'); $this->assertEquals($b->getOwner(), 'admin'); $this->assertEquals($b->getType(), 'text'); @@ -415,7 +420,10 @@ public function testDelete() { $this->cardMapper->expects($this->once()) ->method('update') ->willReturn($cardToBeDeleted); + $auditEvents = $this->collectAuditEvents(); $this->cardService->delete(123); + $this->assertSame(['deck.card.deleted'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['cardId' => 123, 'stackId' => null], $auditEvents[0]->getParameters()); $this->assertTrue($cardToBeDeleted->getDeletedAt() <= time(), 'deletedAt is in the past'); } @@ -656,7 +664,9 @@ public function testArchive() { $this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) { return $c; }); + $auditEvents = $this->collectAuditEvents(); $this->assertTrue($this->cardService->archive(123)->getArchived()); + $this->assertSame(['deck.card.archived'], $this->getAuditOperations($auditEvents)); } public function testUnarchive() { $card = new Card(); @@ -666,7 +676,9 @@ public function testUnarchive() { $this->cardMapper->expects($this->once())->method('update')->willReturnCallback(function ($c) { return $c; }); + $auditEvents = $this->collectAuditEvents(); $this->assertFalse($this->cardService->unarchive(123)->getArchived()); + $this->assertSame(['deck.card.unarchived'], $this->getAuditOperations($auditEvents)); } public function testAssignLabel() { @@ -845,4 +857,64 @@ public function testRemoveDependentCard() { $result = $this->cardService->removeDependentCard(42, 43); $this->assertEquals([44], $result->getDependentCards()); } + + /** + * @return \ArrayObject + */ + private function collectAuditEvents(): \ArrayObject { + $auditEvents = new \ArrayObject(); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use ($auditEvents): void { + if ($event instanceof CriticalActionPerformedEvent) { + $auditEvents[] = $event; + } + }); + return $auditEvents; + } + + /** + * @param \ArrayObject $auditEvents + * @return list + */ + private function getAuditOperations(\ArrayObject $auditEvents): array { + return array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $auditEvents->getArrayCopy()); + } + + public function testUpdateRestoreAndArchiveLogCriticalAction(): void { + $card = Card::fromParams([ + 'title' => 'Card title', + 'stackId' => 234, + 'deletedAt' => 100, + 'archived' => false, + ]); + $this->cardMapper->method('find')->willReturn($card); + $this->cardMapper->method('update')->willReturnCallback(function ($c) { + $c->setId(1); + return $c; + }); + $this->stackMapper->method('find')->willReturn(Stack::fromParams(['id' => 234, 'boardId' => 1337])); + $this->stackMapper->method('findBoardId')->willReturn(1337); + $this->cardMapper->method('findBoardId')->willReturn(1337); + $auditEvents = $this->collectAuditEvents(); + + $this->cardService->update(123, 'Card title', 234, 'text', 'admin', '', 0, null, 0, true); + + $this->assertSame(['deck.card.restored', 'deck.card.archived'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['cardId' => 1, 'stackId' => 234], $auditEvents[0]->getParameters()); + } + + public function testReorderToAnotherBoardLogsCriticalAction(): void { + $card = new Card(); + $card->setId(5); + $card->setStackId(123); + $this->cardMapper->method('find')->willReturn($card); + $this->cardMapper->method('findAll')->willReturn([$card]); + $this->stackMapper->method('find')->willReturnCallback(fn (int $id): Stack => Stack::fromParams(['id' => $id, 'boardId' => $id === 123 ? 1 : 2])); + $auditEvents = $this->collectAuditEvents(); + + $this->cardService->reorder(5, 234, 0); + + $this->assertSame(['deck.card.moved'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['cardId' => 5, 'fromBoardId' => 1, 'toBoardId' => 2], $auditEvents[0]->getParameters()); + } } diff --git a/tests/unit/Service/LabelServiceTest.php b/tests/unit/Service/LabelServiceTest.php index 038f4b8a8d..d96c40ba8a 100644 --- a/tests/unit/Service/LabelServiceTest.php +++ b/tests/unit/Service/LabelServiceTest.php @@ -31,6 +31,8 @@ use OCA\Deck\Db\Label; use OCA\Deck\Db\LabelMapper; use OCA\Deck\Validators\LabelServiceValidator; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -47,6 +49,7 @@ class LabelServiceTest extends TestCase { /** @var ChangeHelper|\PHPUnit\Framework\MockObject\MockObject */ private ChangeHelper&MockObject $changeHelper; private LabelServiceValidator&MockObject $labelServiceValidator; + private IEventDispatcher&MockObject $eventDispatcher; public function setUp(): void { parent::setUp(); @@ -57,6 +60,7 @@ public function setUp(): void { $this->boardService = $this->createMock(BoardService::class); $this->changeHelper = $this->createMock(ChangeHelper::class); $this->labelServiceValidator = $this->createMock(LabelServiceValidator::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->labelService = new LabelService( $this->labelMapper, @@ -64,6 +68,7 @@ public function setUp(): void { $this->boardService, $this->changeHelper, $this->labelServiceValidator, + $this->eventDispatcher, ); } @@ -164,6 +169,11 @@ public function testDelete() { $this->labelMapper->expects($this->once()) ->method('delete') ->willReturn($label); + $label->setBoardId(9); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.label.deleted' + && $event->getParameters() === ['labelId' => 1, 'boardId' => 9])); $this->assertEquals($label, $this->labelService->delete(1)); } } diff --git a/tests/unit/Service/StackServiceTest.php b/tests/unit/Service/StackServiceTest.php index fde58b709f..def224f8e8 100644 --- a/tests/unit/Service/StackServiceTest.php +++ b/tests/unit/Service/StackServiceTest.php @@ -38,7 +38,9 @@ use OCA\Deck\Db\StackMapper; use OCA\Deck\Model\CardDetails; use OCA\Deck\Validators\StackServiceValidator; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Log\Audit\CriticalActionPerformedEvent; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -221,7 +223,10 @@ public function testDelete() { $this->stackMapper->expects($this->once())->method('find')->willReturn($stackToBeDeleted); $this->stackMapper->expects($this->once())->method('update')->willReturn($stackToBeDeleted); $this->cardMapper->expects($this->once())->method('findAllForStacks')->willReturn([]); + $auditEvents = $this->collectAuditEvents(); $this->stackService->delete(123); + $this->assertSame(['deck.stack.deleted'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['stackId' => 1, 'boardId' => 1], $auditEvents[0]->getParameters()); $this->assertTrue($stackToBeDeleted->getDeletedAt() <= time(), 'deletedAt is in the past'); $this->assertTrue($stackToBeDeleted->getDeletedAt() > 0, 'deletedAt is set'); } @@ -342,4 +347,41 @@ public function testSetDoneStackThrowsOnMismatchBoardId(): void { $this->expectException(\OCA\Deck\BadRequestException::class); $this->stackService->setDoneStack(5, 2, true); } + + /** + * @return \ArrayObject + */ + private function collectAuditEvents(): \ArrayObject { + $auditEvents = new \ArrayObject(); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use ($auditEvents): void { + if ($event instanceof CriticalActionPerformedEvent) { + $auditEvents[] = $event; + } + }); + return $auditEvents; + } + + /** + * @param \ArrayObject $auditEvents + * @return list + */ + private function getAuditOperations(\ArrayObject $auditEvents): array { + return array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $auditEvents->getArrayCopy()); + } + + public function testUpdateRestoreLogsCriticalAction(): void { + $stack = new Stack(); + $stack->setId(123); + $stack->setBoardId(2); + $stack->setDeletedAt(100); + $this->stackMapper->method('find')->willReturn($stack); + $this->stackMapper->method('update')->willReturnArgument(0); + $auditEvents = $this->collectAuditEvents(); + + $this->stackService->update(123, 'Foo', 2, 1, 0); + + $this->assertSame(['deck.stack.restored'], $this->getAuditOperations($auditEvents)); + $this->assertSame(['stackId' => 123, 'boardId' => 2], $auditEvents[0]->getParameters()); + } } From db65e305822d66a3827930a8bcb8be71235a6614 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:11:47 +0200 Subject: [PATCH 4/6] feat(attachments): log card attachments stored by Deck to the audit log Attaching a file stored by Deck to a card, deleting it and restoring it are now written to the admin audit log. These files live in the Deck app data, so the core file events do not record them. Attachments of the type "file" are Nextcloud files shared with the card and are already logged as shares, so they are skipped. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 8 ++++ lib/Service/AttachmentService.php | 28 ++++++++++++ tests/unit/Service/AttachmentServiceTest.php | 45 +++++++++++++++++-- .../Service/BatchQueryPerformanceTest.php | 1 + 4 files changed, 79 insertions(+), 3 deletions(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 85ff5f45e8..f66dc3009a 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -46,6 +46,11 @@ enum Operation: string { case CardAssignmentDeleted = 'deck.card_assignment.deleted'; case LabelDeleted = 'deck.label.deleted'; + // Attachments + case AttachmentCreated = 'deck.attachment.created'; + case AttachmentDeleted = 'deck.attachment.deleted'; + case AttachmentRestored = 'deck.attachment.restored'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -76,6 +81,9 @@ public function description(): string { self::CardAssignmentCreated => 'A user, group or team was assigned to a card', self::CardAssignmentDeleted => 'A user, group or team was unassigned from a card', self::LabelDeleted => 'A label was deleted and removed from all cards of its board', + self::AttachmentCreated => 'A file stored by Deck was attached to a card', + self::AttachmentDeleted => 'A file stored by Deck was deleted from a card', + self::AttachmentRestored => 'A deleted file stored by Deck was restored on a card', }; } } diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index a871119f22..8c3f85ff25 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -9,6 +9,7 @@ use OCA\Deck\Activity\ActivityManager; use OCA\Deck\AppInfo\Application; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Cache\AttachmentCacheHelper; use OCA\Deck\Db\Acl; @@ -23,8 +24,10 @@ use OCA\Deck\Validators\AttachmentServiceValidator; use OCP\AppFramework\Db\IMapperException; use OCP\AppFramework\Http\Response; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use Psr\Container\ContainerExceptionInterface; class AttachmentService { @@ -44,6 +47,7 @@ public function __construct( private readonly IL10N $l10n, private readonly ActivityManager $activityManager, private readonly AttachmentServiceValidator $attachmentServiceValidator, + private readonly IEventDispatcher $eventDispatcher, ) { // Register shipped attachment services // TODO: move this to a plugin based approach once we have different types of attachments @@ -202,6 +206,7 @@ public function create(int $cardId, string $type, string $data = '') { $this->changeHelper->cardChanged($attachment->getCardId()); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_CREATE); + $this->logAttachment($attachment, 'added to', Operation::AttachmentCreated); return $attachment; } @@ -355,6 +360,7 @@ public function delete(int $cardId, int $attachmentId, string $type = 'deck_file $this->attachmentCacheHelper->clearAttachmentCount($cardId); $this->changeHelper->cardChanged($attachment->getCardId()); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_DELETE); + $this->logAttachment($attachment, 'deleted from', Operation::AttachmentDeleted); return $attachment; } @@ -375,6 +381,7 @@ public function restore(int $cardId, int $attachmentId, string $type = 'deck_fil $attachment = $this->attachmentMapper->update($attachment); $this->changeHelper->cardChanged($attachment->getCardId()); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $attachment, ActivityManager::SUBJECT_ATTACHMENT_RESTORE); + $this->logAttachment($attachment, 'restored on', Operation::AttachmentRestored); return $attachment; } } catch (InvalidAttachmentType $e) { @@ -403,4 +410,25 @@ private function addCreator(Attachment $attachment): Attachment { return $attachment; } + + /** + * Attachments of the type "file" are Nextcloud files shared with the card, + * which the audit log already records as shares + */ + private function logAttachment(Attachment $attachment, string $action, Operation $operation): void { + if ($attachment->getType() === 'file') { + return; + } + + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Attachment %s (%s) was ' . $action . ' card %s', + [ + 'attachmentId' => $attachment->getId(), + 'attachmentType' => $attachment->getType(), + 'cardId' => $attachment->getCardId(), + ], + false, + $operation->value, + )); + } } diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index c7c940c9bf..d5210d61be 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -39,8 +39,11 @@ use OCA\Deck\NotFoundException; use OCA\Deck\Validators\AttachmentServiceValidator; use OCP\AppFramework\Http\Response; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; use Test\TestCase; @@ -99,6 +102,9 @@ class AttachmentServiceTest extends TestCase { * @var AttachmentServiceValidator */ private $attachmentServiceValidator; + private IEventDispatcher&MockObject $eventDispatcher; + /** @var list */ + private array $auditEvents = []; public function setUp(): void { parent::setUp(); @@ -132,6 +138,14 @@ public function setUp(): void { $this->l10n = $this->createMock(IL10N::class); $this->changeHelper = $this->createMock(ChangeHelper::class); $this->attachmentServiceValidator = $this->createMock(AttachmentServiceValidator::class); + $this->auditEvents = []; + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event): void { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + } + }); $this->attachmentService = new AttachmentService( $this->configService, @@ -145,7 +159,8 @@ public function setUp(): void { $this->userId, $this->l10n, $this->activityManager, - $this->attachmentServiceValidator + $this->attachmentServiceValidator, + $this->eventDispatcher, ); } @@ -171,7 +186,7 @@ public function testRegisterAttachmentService() { $application->expects($this->any()) ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); + $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator, $this->eventDispatcher); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $this->assertEquals($fileServiceMock, $attachmentService->getService('deck_file')); $this->assertEquals(MyAttachmentService::class, get_class($attachmentService->getService('custom'))); @@ -196,7 +211,7 @@ public function testRegisterAttachmentServiceNotExisting() { ->method('getContainer') ->willReturn($appContainer); - $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator); + $attachmentService = new AttachmentService($this->configService, $this->attachmentMapper, $this->cardMapper, $this->userManager, $this->changeHelper, $this->permissionService, $application, $this->attachmentCacheHelper, $this->userId, $this->l10n, $this->activityManager, $this->attachmentServiceValidator, $this->eventDispatcher); $attachmentService->registerAttachmentService('custom', MyAttachmentService::class); $attachmentService->getService('deck_file_invalid'); } @@ -316,6 +331,7 @@ public function testCreate() { ], ]); $this->assertEquals($expected, $actual); + $this->assertAuditOperations(['deck.attachment.created']); } public function testDisplay() { @@ -402,6 +418,7 @@ public function testDelete() { ->willReturn($attachment); $actual = $this->attachmentService->delete(1, 1); $this->assertEquals($expected, $actual); + $this->assertAuditOperations(['deck.attachment.deleted']); } public function testDeleteWithUndo() { @@ -427,6 +444,7 @@ public function testDeleteWithUndo() { $expected->setDeletedAt(23); $actual = $this->attachmentService->delete(1, 1); $this->assertEquals($expected, $actual); + $this->assertAuditOperations(['deck.attachment.deleted']); } public function testRestore() { @@ -447,6 +465,8 @@ public function testRestore() { $expected->setDeletedAt(0); $actual = $this->attachmentService->restore(1, 1); $this->assertEquals($expected, $actual); + $this->assertAuditOperations(['deck.attachment.restored']); + $this->assertSame(['attachmentId' => null, 'attachmentType' => 'deck_file', 'cardId' => 123], $this->auditEvents[0]->getParameters()); } public function testRestoreNotAllowed() { @@ -465,4 +485,23 @@ public function testRestoreNotAllowed() { $actual = $this->attachmentService->restore(1, 1); $this->assertEquals($expected, $actual); } + + public function testFileAttachmentIsNotLogged(): void { + $attachment = $this->createAttachment('file', 'file_name.jpg'); + $this->attachmentMapper->method('find')->willReturn($attachment); + $this->attachmentServiceImpl->method('allowUndo')->willReturn(false); + $this->attachmentMapper->method('delete')->willReturn($attachment); + $this->mockPermission(Acl::PERMISSION_EDIT); + + $this->attachmentService->delete(1, 1, 'file'); + + $this->assertSame([], $this->auditEvents); + } + + /** + * @param list $operations + */ + private function assertAuditOperations(array $operations): void { + $this->assertSame($operations, array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $this->auditEvents)); + } } diff --git a/tests/unit/Service/BatchQueryPerformanceTest.php b/tests/unit/Service/BatchQueryPerformanceTest.php index 76873614f8..6ed201af24 100644 --- a/tests/unit/Service/BatchQueryPerformanceTest.php +++ b/tests/unit/Service/BatchQueryPerformanceTest.php @@ -345,6 +345,7 @@ private function setUpAttachmentService(): void { $this->createMock(IL10N::class), $this->createMock(ActivityManager::class), $this->createMock(AttachmentServiceValidator::class), + $this->createMock(IEventDispatcher::class), ); } From 111ebd98b16f9236c5aae7c6b89e7d6162061175 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:13:07 +0200 Subject: [PATCH 5/6] feat(comments): log card comments to the audit log Adding, editing and deleting comments on cards are now written to the admin audit log. Only the fact is logged with the comment and card ID, never the comment text. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 8 +++ lib/Service/CommentService.php | 19 +++++ tests/unit/Service/CommentServiceTest.php | 85 +++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 tests/unit/Service/CommentServiceTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index f66dc3009a..44092ef57c 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -51,6 +51,11 @@ enum Operation: string { case AttachmentDeleted = 'deck.attachment.deleted'; case AttachmentRestored = 'deck.attachment.restored'; + // Comments + case CommentCreated = 'deck.comment.created'; + case CommentUpdated = 'deck.comment.updated'; + case CommentDeleted = 'deck.comment.deleted'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -84,6 +89,9 @@ public function description(): string { self::AttachmentCreated => 'A file stored by Deck was attached to a card', self::AttachmentDeleted => 'A file stored by Deck was deleted from a card', self::AttachmentRestored => 'A deleted file stored by Deck was restored on a card', + self::CommentCreated => 'A comment was added to a card', + self::CommentUpdated => 'A comment on a card was edited by its author', + self::CommentDeleted => 'A comment was deleted from a card by its author', }; } } diff --git a/lib/Service/CommentService.php b/lib/Service/CommentService.php index e5d4bc140e..9d7cf47697 100644 --- a/lib/Service/CommentService.php +++ b/lib/Service/CommentService.php @@ -8,6 +8,7 @@ namespace OCA\Deck\Service; use OCA\Deck\AppInfo\Application; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Db\Acl; use OCA\Deck\Db\CardMapper; @@ -18,7 +19,9 @@ use OCP\Comments\ICommentsManager; use OCP\Comments\MessageTooLongException; use OCP\Comments\NotFoundException as CommentNotFoundException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OutOfBoundsException; use Psr\Log\LoggerInterface; @@ -31,6 +34,7 @@ public function __construct( private IUserManager $userManager, private LoggerInterface $logger, private ?string $userId, + private IEventDispatcher $eventDispatcher, ) { } @@ -115,6 +119,7 @@ public function create(int $cardId, string $message, int $replyTo = 0): DataResp $comment->setVerb('comment'); $comment->setParentId((string)$replyTo); $this->commentsManager->save($comment); + $this->logComment($cardId, $comment->getId(), 'added to', Operation::CommentCreated); return new DataResponse($this->formatComment($comment, true)); } catch (\InvalidArgumentException $e) { throw new BadRequestException('Invalid input values'); @@ -134,6 +139,7 @@ public function update(int $cardId, int $commentId, string $message): DataRespon $comment->setMessage($message); $this->commentsManager->save($comment); + $this->logComment($cardId, $comment->getId(), 'edited on', Operation::CommentUpdated); return new DataResponse($this->formatComment($comment)); } @@ -152,6 +158,7 @@ public function delete(int $cardId, int $commentId): DataResponse { throw new NoPermissionException('Only authors are allowed to edit their comment.'); } $this->commentsManager->delete((string)$commentId); + $this->logComment($cardId, (string)$commentId, 'deleted from', Operation::CommentDeleted); return new DataResponse([]); } @@ -191,4 +198,16 @@ private function formatComment(IComment $comment, bool $addReplyTo = false): arr } return $formattedComment; } + + /** + * Only the fact is logged, never the comment text + */ + private function logComment(int $cardId, string $commentId, string $action, Operation $operation): void { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Comment %s was ' . $action . ' card %s', + ['commentId' => $commentId, 'cardId' => $cardId], + false, + $operation->value, + )); + } } diff --git a/tests/unit/Service/CommentServiceTest.php b/tests/unit/Service/CommentServiceTest.php new file mode 100644 index 0000000000..45d3d8f43d --- /dev/null +++ b/tests/unit/Service/CommentServiceTest.php @@ -0,0 +1,85 @@ +commentsManager = $this->createMock(ICommentsManager::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->service = new CommentService( + $this->commentsManager, + $this->createMock(PermissionService::class), + $this->createMock(CardMapper::class), + $this->createMock(IUserManager::class), + $this->createMock(LoggerInterface::class), + 'admin', + $this->eventDispatcher, + ); + } + + private function createComment(): IComment&MockObject { + $comment = $this->createMock(IComment::class); + $comment->method('getId')->willReturn('42'); + $comment->method('getObjectType')->willReturn(Application::COMMENT_ENTITY_TYPE); + $comment->method('getObjectId')->willReturn('7'); + $comment->method('getActorType')->willReturn('users'); + $comment->method('getActorId')->willReturn('admin'); + $comment->method('getMessage')->willReturn('Secret message text'); + $comment->method('getCreationDateTime')->willReturn(new \DateTime()); + $comment->method('getMentions')->willReturn([]); + $comment->method('getParentId')->willReturn('0'); + return $comment; + } + + private function expectAuditEvent(string $operation, string $message): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === $operation + && $event->getLogMessage() === $message + && $event->getParameters() === ['commentId' => '42', 'cardId' => 7])); + } + + public function testCreateLogsCriticalActionWithoutMessage(): void { + $this->commentsManager->method('create')->willReturn($this->createComment()); + $this->expectAuditEvent('deck.comment.created', 'Comment %s was added to card %s'); + + $this->service->create(7, 'Secret message text'); + } + + public function testUpdateLogsCriticalAction(): void { + $this->commentsManager->method('get')->willReturn($this->createComment()); + $this->expectAuditEvent('deck.comment.updated', 'Comment %s was edited on card %s'); + + $this->service->update(7, 42, 'New text'); + } + + public function testDeleteLogsCriticalAction(): void { + $this->commentsManager->method('get')->willReturn($this->createComment()); + $this->expectAuditEvent('deck.comment.deleted', 'Comment %s was deleted from card %s'); + + $this->service->delete(7, 42); + } +} From 2eb7266f3950a707993ddcdfcfb6f8f0f8449501 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 16 Oct 2026 17:14:05 +0200 Subject: [PATCH 6/6] feat(settings): log Deck admin settings to the audit log Limiting Deck to certain groups and turning board federation on or off are now written to the admin audit log. Personal preferences such as the calendar or notification settings are not logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 6 +++ lib/Service/ConfigService.php | 16 +++++++ tests/unit/Service/ConfigServiceTest.php | 60 ++++++++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 tests/unit/Service/ConfigServiceTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 44092ef57c..686b89b2c6 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -56,6 +56,10 @@ enum Operation: string { case CommentUpdated = 'deck.comment.updated'; case CommentDeleted = 'deck.comment.deleted'; + // Administration + case ConfigGroupLimitUpdated = 'deck.config.group_limit_updated'; + case ConfigFederationUpdated = 'deck.config.federation_updated'; + /** * Short description of when the operation is logged, used for the documentation */ @@ -92,6 +96,8 @@ public function description(): string { self::CommentCreated => 'A comment was added to a card', self::CommentUpdated => 'A comment on a card was edited by its author', self::CommentDeleted => 'A comment was deleted from a card by its author', + self::ConfigGroupLimitUpdated => 'The groups allowed to use Deck were changed', + self::ConfigFederationUpdated => 'Sharing boards with other servers was turned on or off', }; } } diff --git a/lib/Service/ConfigService.php b/lib/Service/ConfigService.php index 5a59890527..5e579c07f8 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -10,13 +10,16 @@ namespace OCA\Deck\Service; use OCA\Deck\AppInfo\Application; +use OCA\Deck\AuditLog\Operation; use OCA\Deck\BadRequestException; use OCA\Deck\Exceptions\FederationDisabledException; use OCA\Deck\NoPermissionException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUserSession; +use OCP\Log\Audit\CriticalActionPerformedEvent; class ConfigService { public const SETTING_BOARD_NOTIFICATION_DUE_OFF = 'off'; @@ -29,6 +32,7 @@ class ConfigService { public function __construct( private readonly IConfig $config, private readonly IGroupManager $groupManager, + private readonly IEventDispatcher $eventDispatcher, ) { } @@ -171,6 +175,12 @@ public function set($key, $value) { throw new NoPermissionException('You must be admin to set the group limit'); } $result = $this->setGroupLimit($value); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Deck usage was limited to the groups "%s"', + ['groups' => implode(', ', $result)], + false, + Operation::ConfigGroupLimitUpdated->value, + )); break; case 'federationEnabled': if (!$this->groupManager->isAdmin($userId)) { @@ -178,6 +188,12 @@ public function set($key, $value) { } $this->config->setAppValue(Application::APP_ID, 'federationEnabled', (string)$value); $result = $value; + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Federation of boards was %s', + ['federation' => $value ? 'enabled' : 'disabled'], + false, + Operation::ConfigFederationUpdated->value, + )); break; case 'calendar': $this->config->setUserValue($userId, Application::APP_ID, 'calendar', (string)$value); diff --git a/tests/unit/Service/ConfigServiceTest.php b/tests/unit/Service/ConfigServiceTest.php new file mode 100644 index 0000000000..a5e2aa22b0 --- /dev/null +++ b/tests/unit/Service/ConfigServiceTest.php @@ -0,0 +1,60 @@ +groupManager = $this->createMock(IGroupManager::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->service = new ConfigService( + $this->createMock(IConfig::class), + $this->groupManager, + $this->eventDispatcher, + ); + self::invokePrivate($this->service, 'userId', ['admin']); + $this->groupManager->method('isAdmin')->willReturn(true); + } + + public function testSetGroupLimitLogsCriticalAction(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.config.group_limit_updated' + && $event->getParameters() === ['groups' => 'admin, staff'])); + + $this->service->set('groupLimit', [['id' => 'admin'], ['id' => 'staff']]); + } + + public function testSetFederationEnabledLogsCriticalAction(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'deck.config.federation_updated' + && $event->getParameters() === ['federation' => 'enabled'])); + + $this->service->set('federationEnabled', true); + } + + public function testPersonalSettingIsNotLogged(): void { + $this->eventDispatcher->expects($this->never())->method('dispatchTyped'); + + $this->service->set('calendar', true); + } +}