From 7a25572780c12187bea21290bda8055854a9e366 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:46:23 +0200 Subject: [PATCH 1/7] feat(teams): log team lifecycle and settings to the audit log Creating, renaming and deleting a team, changing its configuration flags (open, invite only, visible, federated, ...), setting or removing a team setting, and creating or revoking its invitation link are now written to the admin audit log. Every change of a team goes through a federated item, which is applied by its manage() method whether it comes from the web UI, the OCS API, occ or another app through the CirclesManager. The entries are written there, after the database change, by the new AuditLogService. The EventService hooks named in the plan are not used: the "...ed" result hooks only run for asynchronous items and only once every instance answered, so renames, configuration and invitation changes never reach them, and the deletion hook runs before the team is deleted. A federated team is changed on every instance it is shared with, so the entry is only written by the instance owning the team, not by the instances receiving the change. Changes are often applied in a background request without a user session, so the entries name the user who did the change. Teams mirroring a Nextcloud group are not logged, their changes come from the group, which is already audited. The value of a team setting is never logged, as it can be the password of the team shares, and neither is the invitation code. A change of the description only is not logged. The operations are collected in the new OCA\Circles\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 | 41 ++ lib/FederatedItems/CircleConfig.php | 15 + lib/FederatedItems/CircleCreate.php | 10 + lib/FederatedItems/CircleCreateInvitation.php | 11 + lib/FederatedItems/CircleDestroy.php | 10 + lib/FederatedItems/CircleEdit.php | 15 + lib/FederatedItems/CircleRevokeInvitation.php | 13 +- lib/FederatedItems/CircleSetting.php | 14 + lib/Service/AuditLogService.php | 92 +++++ tests/unit/lib/AuditLog/OperationTest.php | 26 ++ .../lib/FederatedItems/CircleAuditLogTest.php | 360 ++++++++++++++++++ .../unit/lib/Service/AuditLogServiceTest.php | 143 +++++++ 12 files changed, 749 insertions(+), 1 deletion(-) create mode 100644 lib/AuditLog/Operation.php create mode 100644 lib/Service/AuditLogService.php create mode 100644 tests/unit/lib/AuditLog/OperationTest.php create mode 100644 tests/unit/lib/FederatedItems/CircleAuditLogTest.php create mode 100644 tests/unit/lib/Service/AuditLogServiceTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php new file mode 100644 index 000000000..526417594 --- /dev/null +++ b/lib/AuditLog/Operation.php @@ -0,0 +1,41 @@ + 'A team was created', + self::TeamDeleted => 'A team was deleted, directly or because its last member left it', + self::TeamRenamed => 'A team was renamed', + self::TeamConfigUpdated => 'The configuration flags of a team (open, invite only, visible, federated, ...) were changed', + self::TeamSettingUpdated => 'A setting of a team, for example the password of its shares, was set or removed', + self::InvitationCreated => 'An invitation link to join a team was created', + self::InvitationRevoked => 'The invitation link to join a team was revoked', + }; + } +} diff --git a/lib/FederatedItems/CircleConfig.php b/lib/FederatedItems/CircleConfig.php index 3f0115193..b248dacf4 100644 --- a/lib/FederatedItems/CircleConfig.php +++ b/lib/FederatedItems/CircleConfig.php @@ -9,6 +9,7 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Exceptions\FederatedItemBadRequestException; use OCA\Circles\Exceptions\FederatedItemException; @@ -17,6 +18,7 @@ use OCA\Circles\Model\Circle; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\PermissionService; use OCA\Circles\Tools\Traits\TDeserialize; @@ -36,11 +38,13 @@ class CircleConfig implements * @param CircleRequest $circleRequest * @param PermissionService $permissionService * @param ConfigService $configService + * @param AuditLogService $auditLogService */ public function __construct( private CircleRequest $circleRequest, private PermissionService $permissionService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { } @@ -146,6 +150,17 @@ public function manage(FederatedEvent $event): void { // TODO: sync if it is to broadcast to Trusted RemoteInstance $this->circleRequest->updateConfig($circle); + + $oldConfig = $event->getCircle()->getConfig(); + if ($config !== $oldConfig) { + $this->auditLogService->logTeamChange($circle, Operation::TeamConfigUpdated, 'Configuration of team %s ("%s") was changed from %s to %s by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'oldConfig' => $oldConfig, + 'config' => $config, + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); + } } /** diff --git a/lib/FederatedItems/CircleCreate.php b/lib/FederatedItems/CircleCreate.php index 594544953..8cf9c510b 100644 --- a/lib/FederatedItems/CircleCreate.php +++ b/lib/FederatedItems/CircleCreate.php @@ -10,6 +10,7 @@ namespace OCA\Circles\FederatedItems; use OC\User\NoUserException; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\CircleNotFoundException; @@ -22,6 +23,7 @@ use OCA\Circles\IFederatedItemHighSeverity; use OCA\Circles\IFederatedItemMustBeInitializedLocally; use OCA\Circles\Model\Federated\FederatedEvent; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\EventService; use OCA\Circles\Service\MaintenanceService; @@ -47,6 +49,7 @@ public function __construct( private MembershipService $membershipService, private MaintenanceService $maintenanceService, private EventService $eventService, + private AuditLogService $auditLogService, ) { } @@ -101,6 +104,13 @@ public function manage(FederatedEvent $event): void { } $this->eventService->circleCreating($event); + + $this->auditLogService->logTeamChange($circle, Operation::TeamCreated, 'Team %s ("%s") was created with owner "%s" by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'owner' => $this->auditLogService->getUserId($owner), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/FederatedItems/CircleCreateInvitation.php b/lib/FederatedItems/CircleCreateInvitation.php index 62abceafc..e5e5eb8d5 100644 --- a/lib/FederatedItems/CircleCreateInvitation.php +++ b/lib/FederatedItems/CircleCreateInvitation.php @@ -9,11 +9,13 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Entity\CircleInvitation; use OCA\Circles\IFederatedItem; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; use OCA\Circles\Repository\CircleInvitationRepository; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\EventService; use OCA\Circles\Tools\Traits\TDeserialize; use OCP\Security\ISecureRandom; @@ -25,6 +27,7 @@ class CircleCreateInvitation implements IFederatedItem { public function __construct( private CircleInvitationRepository $circleInvitationRepository, private EventService $eventService, + private AuditLogService $auditLogService, ) { } @@ -56,6 +59,14 @@ public function manage(FederatedEvent $event): void { // todo: do we need separate event here? $this->eventService->circleEditing($event); + + // never logs the invitation code, it lets anyone join the team + $circle = $event->getCircle(); + $this->auditLogService->logTeamChange($circle, Operation::InvitationCreated, 'Invitation link of team %s ("%s") was created by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } public function result(FederatedEvent $event, array $results): void { diff --git a/lib/FederatedItems/CircleDestroy.php b/lib/FederatedItems/CircleDestroy.php index 6f9df62fb..e9508c5c2 100644 --- a/lib/FederatedItems/CircleDestroy.php +++ b/lib/FederatedItems/CircleDestroy.php @@ -9,6 +9,7 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\FederatedItemBadRequestException; @@ -20,6 +21,7 @@ use OCA\Circles\Model\Circle; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\EventService; use OCA\Circles\Service\MembershipService; use OCA\Circles\StatusCode; @@ -46,12 +48,14 @@ class CircleDestroy implements * @param MemberRequest $memberRequest * @param EventService $eventService * @param MembershipService $membershipService + * @param AuditLogService $auditLogService */ public function __construct( private CircleRequest $circleRequest, private MemberRequest $memberRequest, private EventService $eventService, private MembershipService $membershipService, + private AuditLogService $auditLogService, ) { } @@ -96,6 +100,12 @@ public function manage(FederatedEvent $event): void { foreach ($parentCirlces as $parentCircle) { $this->membershipService->updatePopulation($parentCircle); } + + $this->auditLogService->logTeamChange($circle, Operation::TeamDeleted, 'Team %s ("%s") was deleted by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/FederatedItems/CircleEdit.php b/lib/FederatedItems/CircleEdit.php index d6c43e143..f262d9cf2 100644 --- a/lib/FederatedItems/CircleEdit.php +++ b/lib/FederatedItems/CircleEdit.php @@ -9,6 +9,7 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\CircleNameTooLongException; @@ -17,6 +18,7 @@ use OCA\Circles\IFederatedItem; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\EventService; use OCA\Circles\Tools\Traits\TDeserialize; @@ -36,12 +38,14 @@ class CircleEdit implements IFederatedItem { * @param MemberRequest $memberRequest * @param CircleService $circleService * @param EventService $eventService + * @param AuditLogService $auditLogService */ public function __construct( private CircleRequest $circleRequest, private MemberRequest $memberRequest, private CircleService $circleService, private EventService $eventService, + private AuditLogService $auditLogService, ) { } @@ -114,6 +118,17 @@ public function manage(FederatedEvent $event): void { $this->circleRequest->edit($circle); $this->memberRequest->updateDisplayName($circle->getSingleId(), $circle->getDisplayName()); $this->eventService->circleEditing($event); + + $oldName = $this->auditLogService->getTeamName($event->getCircle()); + $name = $this->auditLogService->getTeamName($circle); + if ($name !== $oldName) { + $this->auditLogService->logTeamChange($circle, Operation::TeamRenamed, 'Team %s was renamed from "%s" to "%s" by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'oldName' => $oldName, + 'name' => $name, + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); + } } /** diff --git a/lib/FederatedItems/CircleRevokeInvitation.php b/lib/FederatedItems/CircleRevokeInvitation.php index a49f0d87e..c4a37b54b 100644 --- a/lib/FederatedItems/CircleRevokeInvitation.php +++ b/lib/FederatedItems/CircleRevokeInvitation.php @@ -9,10 +9,12 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\IFederatedItem; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; use OCA\Circles\Repository\CircleInvitationRepository; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\EventService; use OCA\Circles\Tools\Traits\TDeserialize; @@ -22,6 +24,7 @@ class CircleRevokeInvitation implements IFederatedItem { public function __construct( private CircleInvitationRepository $circleInvitationRepository, private EventService $eventService, + private AuditLogService $auditLogService, ) { } @@ -40,9 +43,17 @@ public function verify(FederatedEvent $event): void { public function manage(FederatedEvent $event): void { $circle = clone $event->getCircle(); - $this->circleInvitationRepository->deleteBy(['circleId' => $circle->getSingleId()]); + $deleted = $this->circleInvitationRepository->deleteBy(['circleId' => $circle->getSingleId()]); // todo: do we need separate event here? $this->eventService->circleEditing($event); + + if ($deleted > 0) { + $this->auditLogService->logTeamChange($circle, Operation::InvitationRevoked, 'Invitation link of team %s ("%s") was revoked by "%s"', [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); + } } public function result(FederatedEvent $event, array $results): void { diff --git a/lib/FederatedItems/CircleSetting.php b/lib/FederatedItems/CircleSetting.php index a20f02c79..6565aa5a7 100644 --- a/lib/FederatedItems/CircleSetting.php +++ b/lib/FederatedItems/CircleSetting.php @@ -9,12 +9,14 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\IFederatedItem; use OCA\Circles\IFederatedItemAsyncProcess; use OCA\Circles\IFederatedItemHighSeverity; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\ShareTokenService; use OCA\Circles\Tools\Traits\TDeserialize; @@ -29,6 +31,7 @@ public function __construct( private CircleRequest $circleRequest, private ShareTokenService $shareTokenService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { } @@ -108,6 +111,17 @@ public function manage(FederatedEvent $event): void { && !$this->configService->enforcePasswordOnSharedFile($circle)) { $this->shareTokenService->removeSharePassword($circle->getSingleId()); } + + // the value is never logged, it can be the password of the shares + $message = $event->getParams()->gBool('unset') + ? 'Setting "%s" of team %s ("%s") was removed by "%s"' + : 'Setting "%s" of team %s ("%s") was set by "%s"'; + $this->auditLogService->logTeamChange($circle, Operation::TeamSettingUpdated, $message, [ + 'setting' => $event->getParams()->g('setting'), + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/Service/AuditLogService.php b/lib/Service/AuditLogService.php new file mode 100644 index 000000000..6941f9404 --- /dev/null +++ b/lib/Service/AuditLogService.php @@ -0,0 +1,92 @@ + $parameters + */ + public function log(Operation $operation, string $message, array $parameters): void { + $this->eventDispatcher->dispatchTyped( + new CriticalActionPerformedEvent($message, $parameters, false, $operation->value) + ); + } + + /** + * Logs a change of a team, only on the instance owning the team: the other instances + * of a federated team apply the same change when it is broadcast to them. + * + * Teams mirroring a Nextcloud group are not logged, their changes come from the group + * which is already audited. + * + * @param array $parameters + */ + public function logTeamChange(Circle $circle, Operation $operation, string $message, array $parameters): void { + if ($circle->getSource() === Member::TYPE_GROUP || !$this->isOwnedLocally($circle)) { + return; + } + + $this->log($operation, $message, $parameters); + } + + /** + * The user who did the change. Changes are often applied in a background request + * without a user session, so the audit log cannot tell the user otherwise. + */ + public function getInitiator(Circle $circle): ?string { + if (!$circle->hasInitiator()) { + return null; + } + + return $this->getUserId($circle->getInitiator()); + } + + /** + * User, group, team or app ID of a member, with the instance when it is remote. + * Email addresses and contacts are not logged, the member ID identifies them. + */ + public function getUserId(IFederatedUser $user): ?string { + if ($user->getUserType() === Member::TYPE_MAIL || $user->getUserType() === Member::TYPE_CONTACT) { + return null; + } + + return $this->configService->displayFederatedUser($user); + } + + public function getTeamName(Circle $circle): string { + return $circle->getDisplayName() !== '' ? $circle->getDisplayName() : $circle->getName(); + } + + private function isOwnedLocally(Circle $circle): bool { + try { + return $this->configService->isLocalInstance($circle->getInstance()); + } catch (OwnerNotFoundException) { + return true; + } + } +} diff --git a/tests/unit/lib/AuditLog/OperationTest.php b/tests/unit/lib/AuditLog/OperationTest.php new file mode 100644 index 000000000..509e4371d --- /dev/null +++ b/tests/unit/lib/AuditLog/OperationTest.php @@ -0,0 +1,26 @@ + [$operation], Operation::cases()); + } + + #[DataProvider('dataOperations')] + public function testOperationFormat(Operation $operation): void { + $this->assertMatchesRegularExpression('/^circles\.[a-z][a-z0-9_]*\.[a-z][a-z0-9_]*$/', $operation->value); + $this->assertNotSame('', $operation->description()); + } +} diff --git a/tests/unit/lib/FederatedItems/CircleAuditLogTest.php b/tests/unit/lib/FederatedItems/CircleAuditLogTest.php new file mode 100644 index 000000000..65ad335b8 --- /dev/null +++ b/tests/unit/lib/FederatedItems/CircleAuditLogTest.php @@ -0,0 +1,360 @@ + */ + private array $auditEvents = []; + + protected function setUp(): void { + parent::setUp(); + + $this->auditEvents = []; + $eventDispatcher = $this->createMock(IEventDispatcher::class); + $eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event): void { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + } + }); + + $this->configService = $this->createMock(ConfigService::class); + $this->configService->method('isLocalInstance') + ->willReturnCallback(static fn (string $instance): bool => $instance === ''); + $this->configService->method('displayFederatedUser') + ->willReturnCallback(static fn (IFederatedUser $user): string => $user->getUserId() . ($user->getInstance() === '' ? '' : '@' . $user->getInstance())); + + $this->auditLogService = new AuditLogService($eventDispatcher, $this->configService); + } + + private function createEvent(string $class, string $instance = ''): FederatedEvent { + $owner = new Member(); + $owner->setId('ownerMember') + ->setUserId('owner') + ->setUserType(Member::TYPE_USER) + ->setInstance($instance) + ->setLevel(Member::LEVEL_OWNER); + + $initiator = new Member(); + $initiator->setId('initiatorMember') + ->setUserId('alice') + ->setUserType(Member::TYPE_USER) + ->setInstance('') + ->setLevel(Member::LEVEL_ADMIN); + + $circle = new Circle(); + $circle->setSingleId('circle1') + ->setName('Team') + ->setDisplayName('Team') + ->setSource(Member::TYPE_CIRCLE) + ->setConfig(Circle::CFG_OPEN) + ->setOwner($owner) + ->setInitiator($initiator); + + $event = new FederatedEvent($class); + $event->setCircle($circle); + + return $event; + } + + private function assertAuditEvent(Operation $operation, string $message, array $parameters): void { + $this->assertCount(1, $this->auditEvents); + $this->assertSame($operation->value, $this->auditEvents[0]->getOperation()); + $this->assertSame($message, $this->auditEvents[0]->getLogMessage()); + $this->assertSame($parameters, $this->auditEvents[0]->getParameters()); + } + + private function createCircleCreate(CircleRequest $circleRequest): CircleCreate { + $memberRequest = $this->createMock(MemberRequest::class); + $memberRequest->method('getMemberById') + ->willThrowException(new MemberNotFoundException()); + + return new CircleCreate( + $circleRequest, + $memberRequest, + $this->createMock(CircleService::class), + $this->createMock(MembershipService::class), + $this->createMock(MaintenanceService::class), + $this->createMock(EventService::class), + $this->auditLogService, + ); + } + + public function testCircleCreate(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->method('getCircle') + ->willThrowException(new CircleNotFoundException()); + $circleRequest->expects($this->once()) + ->method('save'); + + $this->createCircleCreate($circleRequest)->manage($this->createEvent(CircleCreate::class)); + + $this->assertAuditEvent(Operation::TeamCreated, 'Team %s ("%s") was created with owner "%s" by "%s"', [ + 'circleId' => 'circle1', + 'name' => 'Team', + 'owner' => 'owner', + 'initiator' => 'alice', + ]); + } + + public function testCircleCreateNotLoggedWhenTeamExists(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->method('getCircle') + ->willReturn(new Circle()); + $circleRequest->expects($this->never()) + ->method('save'); + + try { + $this->createCircleCreate($circleRequest)->manage($this->createEvent(CircleCreate::class)); + $this->fail('Expected FederatedEventDSyncException'); + } catch (FederatedEventDSyncException) { + } + + $this->assertSame([], $this->auditEvents); + } + + public function testCircleCreateNotLoggedForGroupMirror(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->method('getCircle') + ->willThrowException(new CircleNotFoundException()); + + $event = $this->createEvent(CircleCreate::class); + $event->getCircle()->setSource(Member::TYPE_GROUP); + $this->createCircleCreate($circleRequest)->manage($event); + + $this->assertSame([], $this->auditEvents); + } + + private function createCircleDestroy(CircleRequest $circleRequest): CircleDestroy { + return new CircleDestroy( + $circleRequest, + $this->createMock(MemberRequest::class), + $this->createMock(EventService::class), + $this->createMock(MembershipService::class), + $this->auditLogService, + ); + } + + public function testCircleDestroy(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->expects($this->once()) + ->method('delete'); + + $this->createCircleDestroy($circleRequest)->manage($this->createEvent(CircleDestroy::class)); + + $this->assertAuditEvent(Operation::TeamDeleted, 'Team %s ("%s") was deleted by "%s"', [ + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ]); + } + + public function testCircleDestroyNotLoggedOnOtherInstances(): void { + $this->createCircleDestroy($this->createMock(CircleRequest::class)) + ->manage($this->createEvent(CircleDestroy::class, 'remote.example.com')); + + $this->assertSame([], $this->auditEvents); + } + + private function createCircleEdit(): CircleEdit { + return new CircleEdit( + $this->createMock(CircleRequest::class), + $this->createMock(MemberRequest::class), + $this->createMock(CircleService::class), + $this->createMock(EventService::class), + $this->auditLogService, + ); + } + + public function testCircleEditRename(): void { + $event = $this->createEvent(CircleEdit::class); + $event->getData()->s('displayName', 'New name'); + + $this->createCircleEdit()->manage($event); + + $this->assertAuditEvent(Operation::TeamRenamed, 'Team %s was renamed from "%s" to "%s" by "%s"', [ + 'circleId' => 'circle1', + 'oldName' => 'Team', + 'name' => 'New name', + 'initiator' => 'alice', + ]); + } + + public function testCircleEditDescriptionNotLogged(): void { + $event = $this->createEvent(CircleEdit::class); + $event->getData()->s('description', 'Some description'); + + $this->createCircleEdit()->manage($event); + + $this->assertSame([], $this->auditEvents); + } + + public function testCircleConfig(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->expects($this->once()) + ->method('updateConfig'); + $item = new CircleConfig( + $circleRequest, + $this->createMock(PermissionService::class), + $this->configService, + $this->auditLogService, + ); + + $event = $this->createEvent(CircleConfig::class); + $event->getData()->sInt('config', Circle::CFG_OPEN | Circle::CFG_VISIBLE); + $item->manage($event); + + $this->assertAuditEvent(Operation::TeamConfigUpdated, 'Configuration of team %s ("%s") was changed from %s to %s by "%s"', [ + 'circleId' => 'circle1', + 'name' => 'Team', + 'oldConfig' => Circle::CFG_OPEN, + 'config' => Circle::CFG_OPEN | Circle::CFG_VISIBLE, + 'initiator' => 'alice', + ]); + } + + public function testCircleConfigUnchangedNotLogged(): void { + $item = new CircleConfig( + $this->createMock(CircleRequest::class), + $this->createMock(PermissionService::class), + $this->configService, + $this->auditLogService, + ); + + $event = $this->createEvent(CircleConfig::class); + $event->getData()->sInt('config', Circle::CFG_OPEN); + $item->manage($event); + + $this->assertSame([], $this->auditEvents); + } + + private function createCircleSetting(): CircleSetting { + return new CircleSetting( + $this->createMock(CircleRequest::class), + $this->createMock(ShareTokenService::class), + $this->configService, + $this->auditLogService, + ); + } + + public function testCircleSettingSet(): void { + $event = $this->createEvent(CircleSetting::class); + $event->setParams(new SimpleDataStore(['setting' => 'password_single', 'value' => 'hashed-secret', 'unset' => false])); + $event->getData()->sArray('settings', ['password_single' => 'hashed-secret']); + + $this->createCircleSetting()->manage($event); + + $this->assertAuditEvent(Operation::TeamSettingUpdated, 'Setting "%s" of team %s ("%s") was set by "%s"', [ + 'setting' => 'password_single', + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ]); + } + + public function testCircleSettingUnset(): void { + $event = $this->createEvent(CircleSetting::class); + $event->setParams(new SimpleDataStore(['setting' => 'enforce_password', 'value' => null, 'unset' => true])); + $event->getData()->sArray('settings', []); + + $this->createCircleSetting()->manage($event); + + $this->assertAuditEvent(Operation::TeamSettingUpdated, 'Setting "%s" of team %s ("%s") was removed by "%s"', [ + 'setting' => 'enforce_password', + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ]); + } + + public function testCircleCreateInvitation(): void { + $repository = $this->createMock(CircleInvitationRepository::class); + $repository->expects($this->once()) + ->method('replace') + ->willReturnArgument(0); + $item = new CircleCreateInvitation($repository, $this->createMock(EventService::class), $this->auditLogService); + + $invitation = new CircleInvitation(); + $invitation->circleId = 'circle1'; + $invitation->invitationCode = 'SECRETCODE'; + $event = $this->createEvent(CircleCreateInvitation::class); + $event->getData()->sObj('circle_invitation', $invitation); + $item->manage($event); + + $this->assertAuditEvent(Operation::InvitationCreated, 'Invitation link of team %s ("%s") was created by "%s"', [ + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ]); + } + + public function testCircleRevokeInvitation(): void { + $repository = $this->createMock(CircleInvitationRepository::class); + $repository->method('deleteBy') + ->with(['circleId' => 'circle1']) + ->willReturn(1); + $item = new CircleRevokeInvitation($repository, $this->createMock(EventService::class), $this->auditLogService); + + $item->manage($this->createEvent(CircleRevokeInvitation::class)); + + $this->assertAuditEvent(Operation::InvitationRevoked, 'Invitation link of team %s ("%s") was revoked by "%s"', [ + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ]); + } + + public function testCircleRevokeInvitationWithoutInvitationNotLogged(): void { + $repository = $this->createMock(CircleInvitationRepository::class); + $repository->method('deleteBy') + ->willReturn(0); + $item = new CircleRevokeInvitation($repository, $this->createMock(EventService::class), $this->auditLogService); + + $item->manage($this->createEvent(CircleRevokeInvitation::class)); + + $this->assertSame([], $this->auditEvents); + } +} diff --git a/tests/unit/lib/Service/AuditLogServiceTest.php b/tests/unit/lib/Service/AuditLogServiceTest.php new file mode 100644 index 000000000..7360a96a5 --- /dev/null +++ b/tests/unit/lib/Service/AuditLogServiceTest.php @@ -0,0 +1,143 @@ +eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->configService = $this->createMock(ConfigService::class); + $this->configService->method('isLocalInstance') + ->willReturnCallback(static fn (string $instance): bool => $instance === '' || $instance === 'cloud.example.com'); + $this->configService->method('displayFederatedUser') + ->willReturnCallback(static fn (IFederatedUser $user): string => $user->getUserId() . '@' . $user->getInstance()); + + $this->service = new AuditLogService($this->eventDispatcher, $this->configService); + } + + private function createCircle(string $instance, int $source = Member::TYPE_CIRCLE): Circle { + $owner = new Member(); + $owner->setUserId('owner') + ->setUserType(Member::TYPE_USER) + ->setInstance($instance); + + $circle = new Circle(); + $circle->setSingleId('circle1') + ->setSource($source) + ->setOwner($owner); + + return $circle; + } + + public function testLog(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function (CriticalActionPerformedEvent $event): bool { + $this->assertSame('Team %s was deleted', $event->getLogMessage()); + $this->assertSame(['circleId' => 'circle1'], $event->getParameters()); + $this->assertFalse($event->getObfuscateParameters()); + $this->assertSame(Operation::TeamDeleted->value, $event->getOperation()); + return true; + })); + + $this->service->log(Operation::TeamDeleted, 'Team %s was deleted', ['circleId' => 'circle1']); + } + + public function testLogTeamChangeOnOwningInstance(): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(CriticalActionPerformedEvent::class)); + + $this->service->logTeamChange($this->createCircle('cloud.example.com'), Operation::TeamDeleted, 'Team %s was deleted', ['circleId' => 'circle1']); + } + + public function testLogTeamChangeSkipsTeamOwnedByAnotherInstance(): void { + $this->eventDispatcher->expects($this->never()) + ->method('dispatchTyped'); + + $this->service->logTeamChange($this->createCircle('remote.example.com'), Operation::TeamDeleted, 'Team %s was deleted', ['circleId' => 'circle1']); + } + + public function testLogTeamChangeSkipsGroupMirror(): void { + $this->eventDispatcher->expects($this->never()) + ->method('dispatchTyped'); + + $this->service->logTeamChange($this->createCircle('', Member::TYPE_GROUP), Operation::TeamDeleted, 'Team %s was deleted', ['circleId' => 'circle1']); + } + + public function testLogTeamChangeWithoutOwner(): void { + $circle = new Circle(); + $circle->setSingleId('circle1'); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped'); + + $this->service->logTeamChange($circle, Operation::TeamDeleted, 'Team %s was deleted', ['circleId' => 'circle1']); + } + + public static function dataGetUserId(): array { + return [ + [Member::TYPE_USER, 'alice@cloud.example.com'], + [Member::TYPE_GROUP, 'alice@cloud.example.com'], + [Member::TYPE_CIRCLE, 'alice@cloud.example.com'], + [Member::TYPE_APP, 'alice@cloud.example.com'], + [Member::TYPE_MAIL, null], + [Member::TYPE_CONTACT, null], + ]; + } + + #[DataProvider('dataGetUserId')] + public function testGetUserId(int $userType, ?string $expected): void { + $member = new Member(); + $member->setUserId('alice') + ->setUserType($userType) + ->setInstance('cloud.example.com'); + + $this->assertSame($expected, $this->service->getUserId($member)); + } + + public function testGetInitiator(): void { + $initiator = new Member(); + $initiator->setUserId('alice') + ->setUserType(Member::TYPE_USER) + ->setInstance('cloud.example.com'); + $circle = new Circle(); + $circle->setInitiator($initiator); + + $this->assertSame('alice@cloud.example.com', $this->service->getInitiator($circle)); + $this->assertNull($this->service->getInitiator(new Circle())); + } + + public function testGetTeamName(): void { + $circle = new Circle(); + $circle->setName('name'); + $this->assertSame('name', $this->service->getTeamName($circle)); + + $circle->setDisplayName('Display name'); + $this->assertSame('Display name', $this->service->getTeamName($circle)); + } +} From 9363515ef2947313284b37f1212e712baae7300b Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:48:46 +0200 Subject: [PATCH 2/7] feat(members): log team membership changes to the audit log Adding members to a team (one or several at once), inviting them to an invite only team, removing them, changing their level (including ownership transfers), users joining a team or requesting to join it, and members leaving a team are now written to the admin audit log. The entries are written by the federated items applying the change, after the database change and only on the instance owning the team, like the team changes. When the owner leaves, the member becoming the new owner gets its own level change entry, and the team deletion when the last member leaves is logged as well. A transfer of the ownership also logs the previous owner becoming an admin. Members are identified by their member ID, type and user, group or team ID. Email addresses and contacts are only identified by their member ID, the address is never logged. Membership changes of teams mirroring a Nextcloud group are not logged, as the group changes are already audited, which also keeps the group sync out of the log. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 14 + lib/FederatedItems/CircleJoin.php | 15 + lib/FederatedItems/CircleLeave.php | 30 ++ lib/FederatedItems/MassiveMemberAdd.php | 2 + lib/FederatedItems/MemberLevel.php | 21 + lib/FederatedItems/MemberRemove.php | 10 + lib/FederatedItems/SingleMemberAdd.php | 24 ++ lib/Service/AuditLogService.php | 17 + .../lib/FederatedItems/MemberAuditLogTest.php | 395 ++++++++++++++++++ 9 files changed, 528 insertions(+) create mode 100644 tests/unit/lib/FederatedItems/MemberAuditLogTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 526417594..02a17147c 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -23,6 +23,13 @@ enum Operation: string { case TeamSettingUpdated = 'circles.team_setting.updated'; case InvitationCreated = 'circles.invitation.created'; case InvitationRevoked = 'circles.invitation.revoked'; + case MemberAdded = 'circles.member.added'; + case MemberInvited = 'circles.member.invited'; + case MemberRemoved = 'circles.member.removed'; + case MemberLevelUpdated = 'circles.member_level.updated'; + case MemberJoined = 'circles.member.joined'; + case MemberRequested = 'circles.member.requested'; + case MemberLeft = 'circles.member.left'; /** * Short description of when the operation is logged, used for the documentation @@ -36,6 +43,13 @@ public function description(): string { self::TeamSettingUpdated => 'A setting of a team, for example the password of its shares, was set or removed', self::InvitationCreated => 'An invitation link to join a team was created', self::InvitationRevoked => 'The invitation link to join a team was revoked', + self::MemberAdded => 'A user, group, team, email address or contact was added to a team, or a request to join it was accepted', + self::MemberInvited => 'A user, group or team was invited to join an invite only team', + self::MemberRemoved => 'A member was removed from a team', + self::MemberLevelUpdated => 'The level of a member of a team (member, moderator, admin, owner) was changed, including ownership transfers', + self::MemberJoined => 'A user joined an open team, through an invitation link or by accepting an invitation', + self::MemberRequested => 'A user requested to join a team that requires confirmation', + self::MemberLeft => 'A member left a team', }; } } diff --git a/lib/FederatedItems/CircleJoin.php b/lib/FederatedItems/CircleJoin.php index b12238d8d..e27d7c98f 100644 --- a/lib/FederatedItems/CircleJoin.php +++ b/lib/FederatedItems/CircleJoin.php @@ -10,6 +10,7 @@ namespace OCA\Circles\FederatedItems; use Exception; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\FederatedItemBadRequestException; use OCA\Circles\Exceptions\FederatedUserException; @@ -29,6 +30,7 @@ use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\ManagedModel; use OCA\Circles\Model\Member; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\EventService; @@ -61,6 +63,7 @@ class CircleJoin implements * @param MembershipService $membershipService * @param EventService $eventService * @param ConfigService $configService + * @param AuditLogService $auditLogService */ public function __construct( private IUserManager $userManager, @@ -71,6 +74,7 @@ public function __construct( private MembershipService $membershipService, private EventService $eventService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { } @@ -191,6 +195,17 @@ public function manage(FederatedEvent $event): void { } $this->membershipService->updatePopulation($event->getCircle()); + + $circle = $event->getCircle(); + $parameters = $this->auditLogService->getMemberParameters($member) + [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + ]; + if ($member->getStatus() === Member::STATUS_REQUEST) { + $this->auditLogService->logTeamChange($circle, Operation::MemberRequested, 'Member "%s" (%s, member %s) requested to join team %s ("%s")', $parameters); + } else { + $this->auditLogService->logTeamChange($circle, Operation::MemberJoined, 'Member "%s" (%s, member %s) joined team %s ("%s")', $parameters); + } } /** diff --git a/lib/FederatedItems/CircleLeave.php b/lib/FederatedItems/CircleLeave.php index b82ca073f..7e288a6c1 100644 --- a/lib/FederatedItems/CircleLeave.php +++ b/lib/FederatedItems/CircleLeave.php @@ -10,6 +10,7 @@ namespace OCA\Circles\FederatedItems; use OCA\Circles\AppInfo\Application; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\CircleNotFoundException; @@ -26,6 +27,7 @@ use OCA\Circles\Model\FederatedUser; use OCA\Circles\Model\Member; use OCA\Circles\Model\Probes\CircleProbe; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\EventService; use OCA\Circles\Service\MembershipService; @@ -62,6 +64,7 @@ public function __construct( private MembershipService $membershipService, private EventService $eventService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { $this->setup('app', Application::APP_ID); } @@ -128,8 +131,11 @@ public function verify(FederatedEvent $event): void { public function manage(FederatedEvent $event): void { $member = $event->getMember(); $newOwnerId = $event->getData()->g('newOwnerId'); + $newOwner = null; + $newOwnerOldLevel = Member::LEVEL_NONE; if ($newOwnerId !== '') { $newOwner = $this->memberRequest->getMemberById($newOwnerId); + $newOwnerOldLevel = $newOwner->getLevel(); $newOwner->setLevel(Member::LEVEL_OWNER); $this->memberRequest->updateLevel($newOwner); $this->membershipService->onUpdate($newOwner->getSingleId()); @@ -152,6 +158,30 @@ public function manage(FederatedEvent $event): void { } $this->membershipService->updatePopulation($event->getCircle()); + + $this->logLeave($event->getCircle(), $member, $newOwner, $newOwnerOldLevel, $destroyingCircle); + } + + private function logLeave(Circle $circle, Member $member, ?Member $newOwner, int $newOwnerOldLevel, bool $destroyingCircle): void { + $teamParameters = [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + ]; + + $this->auditLogService->logTeamChange($circle, Operation::MemberLeft, 'Member "%s" (%s, member %s) left team %s ("%s")', $this->auditLogService->getMemberParameters($member) + $teamParameters); + + if ($newOwner !== null) { + $this->auditLogService->logTeamChange($circle, Operation::MemberLevelUpdated, 'Level of member "%s" (%s, member %s) in team %s ("%s") was changed from %s to %s because the owner left', $this->auditLogService->getMemberParameters($newOwner) + $teamParameters + [ + 'oldLevel' => $this->auditLogService->getLevelName($newOwnerOldLevel), + 'level' => $this->auditLogService->getLevelName(Member::LEVEL_OWNER), + ]); + } + + if ($destroyingCircle) { + $this->auditLogService->logTeamChange($circle, Operation::TeamDeleted, 'Team %s ("%s") was deleted because its last member "%s" left', $teamParameters + [ + 'userId' => $this->auditLogService->getUserId($member), + ]); + } } /** diff --git a/lib/FederatedItems/MassiveMemberAdd.php b/lib/FederatedItems/MassiveMemberAdd.php index d12b07cf9..692688ebc 100644 --- a/lib/FederatedItems/MassiveMemberAdd.php +++ b/lib/FederatedItems/MassiveMemberAdd.php @@ -77,6 +77,8 @@ public function manage(FederatedEvent $event): void { } else { $this->eventService->memberAdding($event); } + + $this->logMemberAdded($event->getCircle(), $member); } catch (Exception) { } } diff --git a/lib/FederatedItems/MemberLevel.php b/lib/FederatedItems/MemberLevel.php index 888db5c83..d70e1ba66 100644 --- a/lib/FederatedItems/MemberLevel.php +++ b/lib/FederatedItems/MemberLevel.php @@ -9,6 +9,7 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\FederatedItemBadRequestException; use OCA\Circles\Exceptions\FederatedItemException; @@ -21,6 +22,7 @@ use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; use OCA\Circles\Model\Member; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\EventService; use OCA\Circles\Service\MembershipService; @@ -44,12 +46,14 @@ class MemberLevel implements * @param MembershipService $membershipService * @param EventService $eventService * @param ConfigService $configService + * @param AuditLogService $auditLogService */ public function __construct( private MemberRequest $memberRequest, private MembershipService $membershipService, private EventService $eventService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { } @@ -104,6 +108,7 @@ public function manage(FederatedEvent $event): void { $member = clone $event->getMember(); $member->setLevel($event->getData()->gInt('level')); + $oldOwner = null; if ($member->getLevel() === Member::LEVEL_OWNER) { $oldOwner = $this->memberRequest->promoteToOwner($member); if ($oldOwner !== null) { @@ -116,6 +121,22 @@ public function manage(FederatedEvent $event): void { $this->membershipService->onUpdate($member->getSingleId()); $this->eventService->memberLevelEditing($event); + + $circle = $event->getCircle(); + $this->logLevelChange($circle, $member, $event->getMember()->getLevel()); + if ($oldOwner !== null && $oldOwner->getId() !== $member->getId()) { + $this->logLevelChange($circle, $oldOwner, Member::LEVEL_OWNER); + } + } + + private function logLevelChange(Circle $circle, Member $member, int $oldLevel): void { + $this->auditLogService->logTeamChange($circle, Operation::MemberLevelUpdated, 'Level of member "%s" (%s, member %s) in team %s ("%s") was changed from %s to %s by "%s"', $this->auditLogService->getMemberParameters($member) + [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'oldLevel' => $this->auditLogService->getLevelName($oldLevel), + 'level' => $this->auditLogService->getLevelName($member->getLevel()), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/FederatedItems/MemberRemove.php b/lib/FederatedItems/MemberRemove.php index f3d4e0463..0f38bab27 100644 --- a/lib/FederatedItems/MemberRemove.php +++ b/lib/FederatedItems/MemberRemove.php @@ -9,6 +9,7 @@ namespace OCA\Circles\FederatedItems; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\FederatedItemException; use OCA\Circles\Exceptions\RequestBuilderException; @@ -18,6 +19,7 @@ use OCA\Circles\IFederatedItemMemberRequired; use OCA\Circles\Model\Federated\FederatedEvent; use OCA\Circles\Model\Helpers\MemberHelper; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\EventService; use OCA\Circles\Service\MembershipService; @@ -42,6 +44,7 @@ public function __construct( private readonly MemberRequest $memberRequest, private readonly MembershipService $membershipService, private readonly EventService $eventService, + private readonly AuditLogService $auditLogService, ) { } @@ -79,6 +82,13 @@ public function manage(FederatedEvent $event): void { $this->eventService->memberRemoving($event); $this->membershipService->updatePopulation($event->getCircle()); + + $circle = $event->getCircle(); + $this->auditLogService->logTeamChange($circle, Operation::MemberRemoved, 'Member "%s" (%s, member %s) was removed from team %s ("%s") by "%s"', $this->auditLogService->getMemberParameters($member) + [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/FederatedItems/SingleMemberAdd.php b/lib/FederatedItems/SingleMemberAdd.php index 7e122cd20..ec9147ec9 100644 --- a/lib/FederatedItems/SingleMemberAdd.php +++ b/lib/FederatedItems/SingleMemberAdd.php @@ -10,6 +10,7 @@ namespace OCA\Circles\FederatedItems; use OC\User\NoUserException; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; use OCA\Circles\Db\MemberRequest; use OCA\Circles\Exceptions\CircleNotFoundException; @@ -42,6 +43,7 @@ use OCA\Circles\Model\Helpers\MemberHelper; use OCA\Circles\Model\ManagedModel; use OCA\Circles\Model\Member; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\EventService; @@ -85,6 +87,7 @@ public function __construct( protected EventService $eventService, protected ConfigService $configService, protected MaintenanceService $maintenanceService, + protected AuditLogService $auditLogService, ) { } @@ -137,6 +140,27 @@ public function manage(FederatedEvent $event): void { } $this->membershipService->updatePopulation($event->getCircle()); + + $this->logMemberAdded($event->getCircle(), $member); + } + + protected function logMemberAdded(Circle $circle, Member $member): void { + $parameters = $this->auditLogService->getMemberParameters($member) + [ + 'circleId' => $circle->getSingleId(), + 'name' => $this->auditLogService->getTeamName($circle), + ]; + + if ($member->getStatus() === Member::STATUS_INVITED) { + $this->auditLogService->logTeamChange($circle, Operation::MemberInvited, 'Member "%s" (%s, member %s) was invited to team %s ("%s") by "%s"', $parameters + [ + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); + return; + } + + $this->auditLogService->logTeamChange($circle, Operation::MemberAdded, 'Member "%s" (%s, member %s) was added to team %s ("%s") as %s by "%s"', $parameters + [ + 'level' => $this->auditLogService->getLevelName($member->getLevel()), + 'initiator' => $this->auditLogService->getInitiator($circle), + ]); } /** diff --git a/lib/Service/AuditLogService.php b/lib/Service/AuditLogService.php index 6941f9404..a6ca4befe 100644 --- a/lib/Service/AuditLogService.php +++ b/lib/Service/AuditLogService.php @@ -78,6 +78,23 @@ public function getUserId(IFederatedUser $user): ?string { return $this->configService->displayFederatedUser($user); } + /** + * Parameters identifying a member, for messages starting with `Member "%s" (%s, member %s)` + * + * @return array{userId: ?string, memberType: string, memberId: string} + */ + public function getMemberParameters(Member $member): array { + return [ + 'userId' => $this->getUserId($member), + 'memberType' => Member::$TYPE[$member->getUserType()] ?? (string)$member->getUserType(), + 'memberId' => $member->getId(), + ]; + } + + public function getLevelName(int $level): string { + return Member::$DEF_LEVEL[$level] ?? (string)$level; + } + public function getTeamName(Circle $circle): string { return $circle->getDisplayName() !== '' ? $circle->getDisplayName() : $circle->getName(); } diff --git a/tests/unit/lib/FederatedItems/MemberAuditLogTest.php b/tests/unit/lib/FederatedItems/MemberAuditLogTest.php new file mode 100644 index 000000000..fb2d60e99 --- /dev/null +++ b/tests/unit/lib/FederatedItems/MemberAuditLogTest.php @@ -0,0 +1,395 @@ + */ + private array $auditEvents = []; + + protected function setUp(): void { + parent::setUp(); + + $this->auditEvents = []; + $eventDispatcher = $this->createMock(IEventDispatcher::class); + $eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (Event $event): void { + if ($event instanceof CriticalActionPerformedEvent) { + $this->auditEvents[] = $event; + } + }); + + $this->configService = $this->createMock(ConfigService::class); + $this->configService->method('isLocalInstance') + ->willReturnCallback(static fn (string $instance): bool => $instance === ''); + $this->configService->method('displayFederatedUser') + ->willReturnCallback(static fn (IFederatedUser $user): string => $user->getUserId() . ($user->getInstance() === '' ? '' : '@' . $user->getInstance())); + + $this->memberService = $this->createMock(MemberService::class); + $this->memberRequest = $this->createMock(MemberRequest::class); + $this->auditLogService = new AuditLogService($eventDispatcher, $this->configService); + } + + private function createMember(string $id, string $userId, int $userType = Member::TYPE_USER, int $level = Member::LEVEL_MEMBER, string $status = Member::STATUS_MEMBER, string $instance = ''): Member { + $member = new Member(); + $member->setId($id) + ->setUserId($userId) + ->setUserType($userType) + ->setInstance($instance) + ->setLevel($level) + ->setStatus($status); + + return $member; + } + + private function createEvent(string $class, ?Member $member = null, string $instance = ''): FederatedEvent { + $circle = new Circle(); + $circle->setSingleId('circle1') + ->setName('Team') + ->setDisplayName('Team') + ->setSource(Member::TYPE_CIRCLE) + ->setOwner($this->createMember('ownerMember', 'owner', level: Member::LEVEL_OWNER, instance: $instance)) + ->setInitiator($this->createMember('initiatorMember', 'alice', level: Member::LEVEL_ADMIN)); + + $event = new FederatedEvent($class); + $event->setCircle($circle); + $event->setMember($member); + + return $event; + } + + /** + * @param list $expected + */ + private function assertAuditEvents(array $expected): void { + $this->assertCount(count($expected), $this->auditEvents); + foreach ($expected as $i => [$operation, $message, $parameters]) { + $this->assertSame($operation->value, $this->auditEvents[$i]->getOperation()); + $this->assertSame($message, $this->auditEvents[$i]->getLogMessage()); + $this->assertSame($parameters, $this->auditEvents[$i]->getParameters()); + } + } + + /** + * @param class-string $class + */ + private function createMemberAdd(string $class): SingleMemberAdd { + return new $class( + $this->createMock(IUserManager::class), + $this->createMock(IAppConfig::class), + $this->memberRequest, + $this->createMock(FederatedUserService::class), + $this->createMock(RemoteStreamService::class), + $this->createMock(CircleService::class), + $this->memberService, + $this->createMock(MembershipService::class), + $this->createMock(EventService::class), + $this->configService, + $this->createMock(MaintenanceService::class), + $this->auditLogService, + ); + } + + public function testSingleMemberAdd(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + + $this->createMemberAdd(SingleMemberAdd::class) + ->manage($this->createEvent(SingleMemberAdd::class, $this->createMember('member1', 'bob'))); + + $this->assertAuditEvents([[ + Operation::MemberAdded, + 'Member "%s" (%s, member %s) was added to team %s ("%s") as %s by "%s"', + [ + 'userId' => 'bob', + 'memberType' => 'user', + 'memberId' => 'member1', + 'circleId' => 'circle1', + 'name' => 'Team', + 'level' => 'Member', + 'initiator' => 'alice', + ], + ]]); + } + + public function testSingleMemberAddInvitation(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + $member = $this->createMember('member1', 'bob', Member::TYPE_USER, Member::LEVEL_NONE, Member::STATUS_INVITED, 'remote.example.com'); + + $this->createMemberAdd(SingleMemberAdd::class) + ->manage($this->createEvent(SingleMemberAdd::class, $member)); + + $this->assertAuditEvents([[ + Operation::MemberInvited, + 'Member "%s" (%s, member %s) was invited to team %s ("%s") by "%s"', + [ + 'userId' => 'bob@remote.example.com', + 'memberType' => 'user', + 'memberId' => 'member1', + 'circleId' => 'circle1', + 'name' => 'Team', + 'initiator' => 'alice', + ], + ]]); + } + + public function testSingleMemberAddEmailAddressNotLogged(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + + $this->createMemberAdd(SingleMemberAdd::class) + ->manage($this->createEvent(SingleMemberAdd::class, $this->createMember('member1', 'bob@example.com', Member::TYPE_MAIL))); + + $this->assertCount(1, $this->auditEvents); + $this->assertNull($this->auditEvents[0]->getParameters()['userId']); + $this->assertSame('mail', $this->auditEvents[0]->getParameters()['memberType']); + } + + public function testSingleMemberAddFailedNotLogged(): void { + $this->memberService->method('insertOrUpdate')->willReturn(false); + + $this->createMemberAdd(SingleMemberAdd::class) + ->manage($this->createEvent(SingleMemberAdd::class, $this->createMember('member1', 'bob'))); + + $this->assertSame([], $this->auditEvents); + } + + public function testSingleMemberAddToGroupMirrorNotLogged(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + + $event = $this->createEvent(SingleMemberAdd::class, $this->createMember('member1', 'bob')); + $event->getCircle()->setSource(Member::TYPE_GROUP); + $this->createMemberAdd(SingleMemberAdd::class)->manage($event); + + $this->assertSame([], $this->auditEvents); + } + + public function testMassiveMemberAdd(): void { + $this->memberService->method('insertOrUpdate') + ->willReturnCallback(static fn (Member $member): bool => $member->getId() !== 'failed'); + + $event = $this->createEvent(MassiveMemberAdd::class); + $event->setMembers([ + $this->createMember('member1', 'bob'), + $this->createMember('failed', 'carol'), + $this->createMember('member2', 'group1', Member::TYPE_GROUP), + ]); + $this->createMemberAdd(MassiveMemberAdd::class)->manage($event); + + $message = 'Member "%s" (%s, member %s) was added to team %s ("%s") as %s by "%s"'; + $this->assertAuditEvents([ + [Operation::MemberAdded, $message, ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team', 'level' => 'Member', 'initiator' => 'alice']], + [Operation::MemberAdded, $message, ['userId' => 'group1', 'memberType' => 'group', 'memberId' => 'member2', 'circleId' => 'circle1', 'name' => 'Team', 'level' => 'Member', 'initiator' => 'alice']], + ]); + } + + private function createCircleJoin(): CircleJoin { + return new CircleJoin( + $this->createMock(IUserManager::class), + $this->memberRequest, + $this->createMock(FederatedUserService::class), + $this->createMock(CircleService::class), + $this->memberService, + $this->createMock(MembershipService::class), + $this->createMock(EventService::class), + $this->configService, + $this->auditLogService, + ); + } + + public function testCircleJoin(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + + $this->createCircleJoin()->manage($this->createEvent(CircleJoin::class, $this->createMember('member1', 'bob'))); + + $this->assertAuditEvents([[ + Operation::MemberJoined, + 'Member "%s" (%s, member %s) joined team %s ("%s")', + ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team'], + ]]); + } + + public function testCircleJoinRequest(): void { + $this->memberService->method('insertOrUpdate')->willReturn(true); + $member = $this->createMember('member1', 'bob', Member::TYPE_USER, Member::LEVEL_NONE, Member::STATUS_REQUEST); + + $this->createCircleJoin()->manage($this->createEvent(CircleJoin::class, $member)); + + $this->assertAuditEvents([[ + Operation::MemberRequested, + 'Member "%s" (%s, member %s) requested to join team %s ("%s")', + ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team'], + ]]); + } + + public function testCircleJoinFailedNotLogged(): void { + $this->memberService->method('insertOrUpdate')->willReturn(false); + + $this->createCircleJoin()->manage($this->createEvent(CircleJoin::class, $this->createMember('member1', 'bob'))); + + $this->assertSame([], $this->auditEvents); + } + + private function createCircleLeave(?CircleRequest $circleRequest = null): CircleLeave { + return new CircleLeave( + $this->memberRequest, + $circleRequest ?? $this->createMock(CircleRequest::class), + $this->createMock(MembershipService::class), + $this->createMock(EventService::class), + $this->configService, + $this->auditLogService, + ); + } + + public function testCircleLeave(): void { + $this->memberRequest->expects($this->once()) + ->method('delete'); + + $this->createCircleLeave()->manage($this->createEvent(CircleLeave::class, $this->createMember('member1', 'bob'))); + + $this->assertAuditEvents([[ + Operation::MemberLeft, + 'Member "%s" (%s, member %s) left team %s ("%s")', + ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team'], + ]]); + } + + public function testCircleLeaveOfOwner(): void { + $this->memberRequest->method('getMemberById') + ->with('member2') + ->willReturn($this->createMember('member2', 'carol', level: Member::LEVEL_ADMIN)); + + $event = $this->createEvent(CircleLeave::class, $this->createMember('ownerMember', 'owner', level: Member::LEVEL_OWNER)); + $event->getData()->s('newOwnerId', 'member2'); + $this->createCircleLeave()->manage($event); + + $this->assertAuditEvents([ + [ + Operation::MemberLeft, + 'Member "%s" (%s, member %s) left team %s ("%s")', + ['userId' => 'owner', 'memberType' => 'user', 'memberId' => 'ownerMember', 'circleId' => 'circle1', 'name' => 'Team'], + ], + [ + Operation::MemberLevelUpdated, + 'Level of member "%s" (%s, member %s) in team %s ("%s") was changed from %s to %s because the owner left', + ['userId' => 'carol', 'memberType' => 'user', 'memberId' => 'member2', 'circleId' => 'circle1', 'name' => 'Team', 'oldLevel' => 'Admin', 'level' => 'Owner'], + ], + ]); + } + + public function testCircleLeaveOfLastMember(): void { + $circleRequest = $this->createMock(CircleRequest::class); + $circleRequest->expects($this->once()) + ->method('delete'); + + $event = $this->createEvent(CircleLeave::class, $this->createMember('ownerMember', 'owner', level: Member::LEVEL_OWNER)); + $event->getData()->sBool('destroyCircle', true); + $this->createCircleLeave($circleRequest)->manage($event); + + $this->assertAuditEvents([ + [ + Operation::MemberLeft, + 'Member "%s" (%s, member %s) left team %s ("%s")', + ['userId' => 'owner', 'memberType' => 'user', 'memberId' => 'ownerMember', 'circleId' => 'circle1', 'name' => 'Team'], + ], + [ + Operation::TeamDeleted, + 'Team %s ("%s") was deleted because its last member "%s" left', + ['circleId' => 'circle1', 'name' => 'Team', 'userId' => 'owner'], + ], + ]); + } + + public function testCircleLeaveNotLoggedOnOtherInstances(): void { + $this->createCircleLeave()->manage($this->createEvent(CircleLeave::class, $this->createMember('member1', 'bob'), 'remote.example.com')); + + $this->assertSame([], $this->auditEvents); + } + + public function testMemberRemove(): void { + $this->memberRequest->expects($this->once()) + ->method('delete'); + $item = new MemberRemove($this->memberRequest, $this->createMock(MembershipService::class), $this->createMock(EventService::class), $this->auditLogService); + + $item->manage($this->createEvent(MemberRemove::class, $this->createMember('member1', 'bob'))); + + $this->assertAuditEvents([[ + Operation::MemberRemoved, + 'Member "%s" (%s, member %s) was removed from team %s ("%s") by "%s"', + ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team', 'initiator' => 'alice'], + ]]); + } + + private function createMemberLevel(): MemberLevel { + return new MemberLevel($this->memberRequest, $this->createMock(MembershipService::class), $this->createMock(EventService::class), $this->configService, $this->auditLogService); + } + + public function testMemberLevel(): void { + $this->memberRequest->expects($this->once()) + ->method('updateLevel'); + + $event = $this->createEvent(MemberLevel::class, $this->createMember('member1', 'bob')); + $event->getData()->sInt('level', Member::LEVEL_MODERATOR); + $this->createMemberLevel()->manage($event); + + $this->assertAuditEvents([[ + Operation::MemberLevelUpdated, + 'Level of member "%s" (%s, member %s) in team %s ("%s") was changed from %s to %s by "%s"', + ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team', 'oldLevel' => 'Member', 'level' => 'Moderator', 'initiator' => 'alice'], + ]]); + } + + public function testMemberLevelOwnershipTransfer(): void { + $this->memberRequest->expects($this->once()) + ->method('promoteToOwner') + ->willReturn($this->createMember('ownerMember', 'owner', level: Member::LEVEL_ADMIN)); + + $event = $this->createEvent(MemberLevel::class, $this->createMember('member1', 'bob', level: Member::LEVEL_ADMIN)); + $event->getData()->sInt('level', Member::LEVEL_OWNER); + $this->createMemberLevel()->manage($event); + + $message = 'Level of member "%s" (%s, member %s) in team %s ("%s") was changed from %s to %s by "%s"'; + $this->assertAuditEvents([ + [Operation::MemberLevelUpdated, $message, ['userId' => 'bob', 'memberType' => 'user', 'memberId' => 'member1', 'circleId' => 'circle1', 'name' => 'Team', 'oldLevel' => 'Admin', 'level' => 'Owner', 'initiator' => 'alice']], + [Operation::MemberLevelUpdated, $message, ['userId' => 'owner', 'memberType' => 'user', 'memberId' => 'ownerMember', 'circleId' => 'circle1', 'name' => 'Team', 'oldLevel' => 'Owner', 'level' => 'Admin', 'initiator' => 'alice']], + ]); + } +} From bbfb5875816a538a33a61c376a966e41acbff59d Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:50:25 +0200 Subject: [PATCH 3/7] feat(federation): log remote instance trust changes to the audit log Adding a remote instance for federated teams, changing the identity or the type of a known one, and removing one are now written to the admin audit log, as are allowing a remote instance to add members to teams managed by SCIM and revoking that again. Remote instances are added by occ circles:remote and by the sync of the trusted servers, which now both store them through the new RemoteStreamService::saveRemoteInstance(), and occ circles:remote --delete removes them through RemoteStreamService::removeRemoteInstance(), so each change is logged in one place. Deleting an unknown instance is not logged. Refreshing the cached data or the address of a known instance does not change its trust and is not logged. The SCIM instances are a list in the app config only changed by occ circles:scim:add-remote-instance and circles:scim:remove-remote-instance, so these commands log the change themselves. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 10 ++ lib/Command/CirclesRemote.php | 4 +- lib/Command/CirclesScimAddRemoteInstance.php | 6 + .../CirclesScimRemoveRemoteInstance.php | 6 + lib/Db/RemoteRequest.php | 5 +- lib/Service/RemoteStreamService.php | 40 +++++ .../Command/CirclesScimRemoteInstanceTest.php | 109 ++++++++++++++ .../lib/Service/RemoteStreamServiceTest.php | 142 ++++++++++++++++++ 8 files changed, 318 insertions(+), 4 deletions(-) create mode 100644 tests/unit/lib/Command/CirclesScimRemoteInstanceTest.php create mode 100644 tests/unit/lib/Service/RemoteStreamServiceTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 02a17147c..f68627423 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -30,6 +30,11 @@ enum Operation: string { case MemberJoined = 'circles.member.joined'; case MemberRequested = 'circles.member.requested'; case MemberLeft = 'circles.member.left'; + case RemoteInstanceAdded = 'circles.remote_instance.added'; + case RemoteInstanceUpdated = 'circles.remote_instance.updated'; + case RemoteInstanceRemoved = 'circles.remote_instance.removed'; + case ScimInstanceAdded = 'circles.scim_instance.added'; + case ScimInstanceRemoved = 'circles.scim_instance.removed'; /** * Short description of when the operation is logged, used for the documentation @@ -50,6 +55,11 @@ public function description(): string { self::MemberJoined => 'A user joined an open team, through an invitation link or by accepting an invitation', self::MemberRequested => 'A user requested to join a team that requires confirmation', self::MemberLeft => 'A member left a team', + self::RemoteInstanceAdded => 'A remote instance was added for federated teams, with the type defining how much it is trusted', + self::RemoteInstanceUpdated => 'The identity or the type of a known remote instance for federated teams was changed', + self::RemoteInstanceRemoved => 'A remote instance for federated teams was removed', + self::ScimInstanceAdded => 'A remote instance was allowed to add members to teams managed by SCIM', + self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM, removing the members it added', }; } } diff --git a/lib/Command/CirclesRemote.php b/lib/Command/CirclesRemote.php index 65aab420b..85cf13151 100644 --- a/lib/Command/CirclesRemote.php +++ b/lib/Command/CirclesRemote.php @@ -94,7 +94,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int if ($host) { if ($input->getOption('delete')) { - $this->remoteRequest->deleteInstance($host); + $this->remoteStreamService->removeRemoteInstance($host); return 0; } else { $this->requestInstance($host); @@ -299,7 +299,7 @@ private function saveRemote(RemoteInstance $remoteSignatory) { if (!$this->interfaceService->isInterfaceInternal($remoteSignatory->getInterface())) { $remoteSignatory->setAliases([]); } - $this->remoteRequest->save($remoteSignatory); + $this->remoteStreamService->saveRemoteInstance($remoteSignatory); $this->output->writeln('remote instance saved'); } } diff --git a/lib/Command/CirclesScimAddRemoteInstance.php b/lib/Command/CirclesScimAddRemoteInstance.php index 879d56644..a92eccafc 100644 --- a/lib/Command/CirclesScimAddRemoteInstance.php +++ b/lib/Command/CirclesScimAddRemoteInstance.php @@ -10,8 +10,10 @@ namespace OCA\Circles\Command; use OC\Core\Command\Base; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; use OCA\Circles\Db\RemoteRequest; +use OCA\Circles\Service\AuditLogService; use OCP\AppFramework\Services\IAppConfig; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -21,6 +23,7 @@ class CirclesScimAddRemoteInstance extends Base { public function __construct( private readonly IAppConfig $appConfig, private readonly RemoteRequest $remoteRequest, + private readonly AuditLogService $auditLogService, ) { parent::__construct(); } @@ -55,6 +58,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int $instances[] = $instance; $this->appConfig->setAppValueArray(ConfigLexicon::SCIM_REMOTE_INSTANCES, $instances); + $this->auditLogService->log(Operation::ScimInstanceAdded, 'Remote instance "%s" was allowed to add members to SCIM teams', [ + 'instance' => $instance, + ]); $output->writeln('done'); diff --git a/lib/Command/CirclesScimRemoveRemoteInstance.php b/lib/Command/CirclesScimRemoveRemoteInstance.php index 1bf3b3ba4..bb6af5fc0 100644 --- a/lib/Command/CirclesScimRemoveRemoteInstance.php +++ b/lib/Command/CirclesScimRemoveRemoteInstance.php @@ -10,7 +10,9 @@ namespace OCA\Circles\Command; use OC\Core\Command\Base; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ScimService; use OCP\AppFramework\Services\IAppConfig; use Symfony\Component\Console\Input\InputArgument; @@ -21,6 +23,7 @@ class CirclesScimRemoveRemoteInstance extends Base { public function __construct( private readonly IAppConfig $appConfig, private readonly ScimService $scimService, + private readonly AuditLogService $auditLogService, ) { parent::__construct(); } @@ -46,6 +49,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int unset($instances[$key]); $this->appConfig->setAppValueArray(ConfigLexicon::SCIM_REMOTE_INSTANCES, array_values($instances)); + $this->auditLogService->log(Operation::ScimInstanceRemoved, 'Remote instance "%s" is no longer allowed to add members to SCIM teams', [ + 'instance' => $instance, + ]); $this->scimService->removeRemoteInstanceMembers($instance); diff --git a/lib/Db/RemoteRequest.php b/lib/Db/RemoteRequest.php index a0f2f73f1..dda28e24b 100644 --- a/lib/Db/RemoteRequest.php +++ b/lib/Db/RemoteRequest.php @@ -242,10 +242,11 @@ public function searchDuplicate(RemoteInstance $remoteInstance): RemoteInstance /** * @param RemoteInstance $remoteInstance */ - public function deleteInstance(string $host) { + public function deleteInstance(string $host): int { $qb = $this->getRemoteDeleteSql(); $qb->limitToInstance($host); - $qb->executeStatement(); + + return $qb->executeStatement(); } /** diff --git a/lib/Service/RemoteStreamService.php b/lib/Service/RemoteStreamService.php index 485954d4a..9346d987a 100644 --- a/lib/Service/RemoteStreamService.php +++ b/lib/Service/RemoteStreamService.php @@ -11,6 +11,7 @@ use JsonSerializable; use OCA\Circles\AppInfo\Application; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Db\RemoteRequest; use OCA\Circles\Exceptions\FederatedItemException; use OCA\Circles\Exceptions\RemoteAlreadyExistsException; @@ -66,6 +67,7 @@ class RemoteStreamService extends NCSignature { * @param RemoteRequest $remoteRequest * @param InterfaceService $interfaceService * @param ConfigService $configService + * @param AuditLogService $auditLogService */ public function __construct( private readonly IConfig $config, @@ -73,6 +75,7 @@ public function __construct( private RemoteRequest $remoteRequest, private InterfaceService $interfaceService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { $this->setup('app', 'circles'); } @@ -392,7 +395,32 @@ public function addRemoteInstance( } catch (RemoteNotFoundException) { } + $this->saveRemoteInstance($remoteInstance); + } + + /** + * Stores a new remote instance, trusted according to its type + * + * @throws RemoteUidException + */ + public function saveRemoteInstance(RemoteInstance $remoteInstance): void { $this->remoteRequest->save($remoteInstance); + + $this->auditLogService->log(Operation::RemoteInstanceAdded, 'Remote instance "%s" was added as %s using the %s interface', [ + 'instance' => $remoteInstance->getInstance(), + 'type' => $remoteInstance->getType(), + 'interface' => $this->getInterfaceName($remoteInstance), + ]); + } + + public function removeRemoteInstance(string $instance): void { + if ($this->remoteRequest->deleteInstance($instance) === 0) { + return; + } + + $this->auditLogService->log(Operation::RemoteInstanceRemoved, 'Remote instance "%s" was removed', [ + 'instance' => $instance, + ]); } /** @@ -554,6 +582,10 @@ public function update(RemoteInstance $remote, string $update = self::UPDATE_DAT switch ($update) { case self::UPDATE_DATA: $this->remoteRequest->update($remote); + $this->auditLogService->log(Operation::RemoteInstanceUpdated, 'Identity of remote instance "%s" was changed, it is known as %s', [ + 'instance' => $remote->getInstance(), + 'type' => $remote->getType(), + ]); break; case self::UPDATE_ITEM: @@ -562,6 +594,10 @@ public function update(RemoteInstance $remote, string $update = self::UPDATE_DAT case self::UPDATE_TYPE: $this->remoteRequest->updateType($remote); + $this->auditLogService->log(Operation::RemoteInstanceUpdated, 'Type of remote instance "%s" was changed to %s', [ + 'instance' => $remote->getInstance(), + 'type' => $remote->getType(), + ]); break; case self::UPDATE_HREF: @@ -573,4 +609,8 @@ public function update(RemoteInstance $remote, string $update = self::UPDATE_DAT break; } } + + private function getInterfaceName(RemoteInstance $remoteInstance): string { + return InterfaceService::LIST_IFACE[$remoteInstance->getInterface()] ?? (string)$remoteInstance->getInterface(); + } } diff --git a/tests/unit/lib/Command/CirclesScimRemoteInstanceTest.php b/tests/unit/lib/Command/CirclesScimRemoteInstanceTest.php new file mode 100644 index 000000000..357909ce5 --- /dev/null +++ b/tests/unit/lib/Command/CirclesScimRemoteInstanceTest.php @@ -0,0 +1,109 @@ +appConfig = $this->createMock(IAppConfig::class); + $this->auditLogService = $this->createMock(AuditLogService::class); + } + + private function createAddCommand(): CirclesScimAddRemoteInstance { + $remoteInstance = new RemoteInstance(); + $remoteInstance->setInstance('remote.example.com'); + $remoteRequest = $this->createMock(RemoteRequest::class); + $remoteRequest->method('getKnownInstances') + ->willReturn([$remoteInstance]); + + return new CirclesScimAddRemoteInstance($this->appConfig, $remoteRequest, $this->auditLogService); + } + + public function testAdd(): void { + $this->appConfig->method('getAppValueArray') + ->willReturn([]); + $this->appConfig->expects($this->once()) + ->method('setAppValueArray') + ->with(ConfigLexicon::SCIM_REMOTE_INSTANCES, ['remote.example.com']); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::ScimInstanceAdded, 'Remote instance "%s" was allowed to add members to SCIM teams', [ + 'instance' => 'remote.example.com', + ]); + + $this->assertSame(0, $this->createAddCommand()->run(new ArrayInput(['instance' => 'remote.example.com']), new NullOutput())); + } + + public function testAddAlreadyAddedNotLogged(): void { + $this->appConfig->method('getAppValueArray') + ->willReturn(['remote.example.com']); + $this->appConfig->expects($this->never()) + ->method('setAppValueArray'); + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->assertSame(0, $this->createAddCommand()->run(new ArrayInput(['instance' => 'remote.example.com']), new NullOutput())); + } + + public function testAddUnknownInstanceNotLogged(): void { + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->assertSame(1, $this->createAddCommand()->run(new ArrayInput(['instance' => 'unknown.example.com']), new NullOutput())); + } + + public function testRemove(): void { + $scimService = $this->createMock(ScimService::class); + $scimService->expects($this->once()) + ->method('removeRemoteInstanceMembers') + ->with('remote.example.com'); + $this->appConfig->method('getAppValueArray') + ->willReturn(['other.example.com', 'remote.example.com']); + $this->appConfig->expects($this->once()) + ->method('setAppValueArray') + ->with(ConfigLexicon::SCIM_REMOTE_INSTANCES, ['other.example.com']); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::ScimInstanceRemoved, 'Remote instance "%s" is no longer allowed to add members to SCIM teams', [ + 'instance' => 'remote.example.com', + ]); + + $command = new CirclesScimRemoveRemoteInstance($this->appConfig, $scimService, $this->auditLogService); + $this->assertSame(0, $command->run(new ArrayInput(['instance' => 'remote.example.com']), new NullOutput())); + } + + public function testRemoveUnknownInstanceNotLogged(): void { + $this->appConfig->method('getAppValueArray') + ->willReturn(['other.example.com']); + $this->auditLogService->expects($this->never()) + ->method('log'); + + $command = new CirclesScimRemoveRemoteInstance($this->appConfig, $this->createMock(ScimService::class), $this->auditLogService); + $this->assertSame(0, $command->run(new ArrayInput(['instance' => 'remote.example.com']), new NullOutput())); + } +} diff --git a/tests/unit/lib/Service/RemoteStreamServiceTest.php b/tests/unit/lib/Service/RemoteStreamServiceTest.php new file mode 100644 index 000000000..408fea27a --- /dev/null +++ b/tests/unit/lib/Service/RemoteStreamServiceTest.php @@ -0,0 +1,142 @@ +remoteRequest = $this->createMock(RemoteRequest::class); + $this->configService = $this->createMock(ConfigService::class); + $this->auditLogService = $this->createMock(AuditLogService::class); + + $this->service = new RemoteStreamService( + $this->createMock(IConfig::class), + $this->createMock(IURLGenerator::class), + $this->remoteRequest, + $this->createMock(InterfaceService::class), + $this->configService, + $this->auditLogService, + ); + } + + private function createRemoteInstance(): RemoteInstance { + $remoteInstance = new RemoteInstance(); + $remoteInstance->setInstance('remote.example.com') + ->setType(RemoteInstance::TYPE_TRUSTED) + ->setInterface(InterfaceService::IFACE_FRONTAL); + + return $remoteInstance; + } + + public function testSaveRemoteInstance(): void { + $remoteInstance = $this->createRemoteInstance(); + $this->remoteRequest->expects($this->once()) + ->method('save') + ->with($remoteInstance); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::RemoteInstanceAdded, 'Remote instance "%s" was added as %s using the %s interface', [ + 'instance' => 'remote.example.com', + 'type' => 'Trusted', + 'interface' => 'frontal', + ]); + + $this->service->saveRemoteInstance($remoteInstance); + } + + public function testAddLocalInstanceNotLogged(): void { + $this->configService->method('isLocalInstance') + ->willReturn(true); + $this->remoteRequest->expects($this->never()) + ->method('save'); + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->expectException(RemoteAlreadyExistsException::class); + $this->service->addRemoteInstance('cloud.example.com'); + } + + public function testRemoveRemoteInstance(): void { + $this->remoteRequest->expects($this->once()) + ->method('deleteInstance') + ->with('remote.example.com') + ->willReturn(1); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::RemoteInstanceRemoved, 'Remote instance "%s" was removed', [ + 'instance' => 'remote.example.com', + ]); + + $this->service->removeRemoteInstance('remote.example.com'); + } + + public function testRemoveUnknownRemoteInstanceNotLogged(): void { + $this->remoteRequest->method('deleteInstance') + ->willReturn(0); + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->service->removeRemoteInstance('unknown.example.com'); + } + + public function testUpdateIdentity(): void { + $this->remoteRequest->expects($this->once()) + ->method('update'); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::RemoteInstanceUpdated, 'Identity of remote instance "%s" was changed, it is known as %s', [ + 'instance' => 'remote.example.com', + 'type' => 'Trusted', + ]); + + $this->service->update($this->createRemoteInstance()); + } + + public function testUpdateType(): void { + $this->remoteRequest->expects($this->once()) + ->method('updateType'); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::RemoteInstanceUpdated, 'Type of remote instance "%s" was changed to %s', [ + 'instance' => 'remote.example.com', + 'type' => 'Trusted', + ]); + + $this->service->update($this->createRemoteInstance(), RemoteStreamService::UPDATE_TYPE); + } + + public function testUpdateItemNotLogged(): void { + $this->remoteRequest->expects($this->once()) + ->method('updateItem'); + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->service->update($this->createRemoteInstance(), RemoteStreamService::UPDATE_ITEM); + } +} From 65d61f44d66b96297106c33521b911a5cefdaf72 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:51:10 +0200 Subject: [PATCH 4/7] feat(settings): log Teams admin settings to the audit log Changing an admin setting of Teams through the admin settings page (enabling federated teams, the frontal address of the instance, the default team folder quota and the team folder quotas per group) is now written to the admin audit log with the key and the new value. None of these values is secret. Rejected values are not logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 2 + lib/Controller/SettingsController.php | 14 +++ .../lib/Controller/SettingsControllerTest.php | 86 +++++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 tests/unit/lib/Controller/SettingsControllerTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index f68627423..9ff4e05cc 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -35,6 +35,7 @@ enum Operation: string { case RemoteInstanceRemoved = 'circles.remote_instance.removed'; case ScimInstanceAdded = 'circles.scim_instance.added'; case ScimInstanceRemoved = 'circles.scim_instance.removed'; + case ConfigUpdated = 'circles.config.updated'; /** * Short description of when the operation is logged, used for the documentation @@ -60,6 +61,7 @@ public function description(): string { self::RemoteInstanceRemoved => 'A remote instance for federated teams was removed', self::ScimInstanceAdded => 'A remote instance was allowed to add members to teams managed by SCIM', self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM, removing the members it added', + self::ConfigUpdated => 'An admin setting of Teams (federated teams, frontal address, team folder quotas) was changed', }; } } diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 84a066cc4..8d569b484 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -9,7 +9,9 @@ namespace OCA\Circles\Controller; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\ConfigService; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; @@ -24,6 +26,7 @@ public function __construct( IRequest $request, private readonly IAppConfig $appConfig, private readonly IGroupManager $groupManager, + private readonly AuditLogService $auditLogService, ) { parent::__construct($appName, $request); } @@ -31,6 +34,7 @@ public function __construct( public function setValue(string $key, string $value): DataResponse { if ($key === ConfigLexicon::FEDERATED_TEAMS_FRONTAL) { if ($this->setFrontalValue($value)) { + $this->logSetting($key, $value); return $this->getValues(); } @@ -39,6 +43,7 @@ public function setValue(string $key, string $value): DataResponse { if ($key === ConfigLexicon::FEDERATED_TEAMS_ENABLED) { $this->appConfig->setAppValueBool(ConfigLexicon::FEDERATED_TEAMS_ENABLED, $value === 'yes'); + $this->logSetting($key, $value === 'yes' ? 'yes' : 'no'); return $this->getValues(); } @@ -48,6 +53,7 @@ public function setValue(string $key, string $value): DataResponse { } $this->appConfig->setAppValueInt(ConfigLexicon::TEAM_FOLDER_DEFAULT_QUOTA, (int)$value); + $this->logSetting($key, (string)(int)$value); return $this->getValues(); } @@ -58,6 +64,7 @@ public function setValue(string $key, string $value): DataResponse { } $this->appConfig->setAppValueArray(ConfigLexicon::TEAM_FOLDER_GROUP_QUOTAS, $quotas); + $this->logSetting($key, (string)json_encode($quotas)); return $this->getValues(); } @@ -83,6 +90,13 @@ public function getGroups(string $search = ''): DataResponse { )); } + private function logSetting(string $key, string $value): void { + $this->auditLogService->log(Operation::ConfigUpdated, 'Teams setting "%s" was set to "%s"', [ + 'key' => $key, + 'value' => $value, + ]); + } + /** @param array $quotas */ private function areValidGroupQuotas(array $quotas): bool { foreach ($quotas as $groupId => $quota) { diff --git a/tests/unit/lib/Controller/SettingsControllerTest.php b/tests/unit/lib/Controller/SettingsControllerTest.php new file mode 100644 index 000000000..4a6f65c28 --- /dev/null +++ b/tests/unit/lib/Controller/SettingsControllerTest.php @@ -0,0 +1,86 @@ +appConfig = $this->createMock(IAppConfig::class); + $this->auditLogService = $this->createMock(AuditLogService::class); + + $this->controller = new SettingsController( + 'circles', + $this->createMock(IRequest::class), + $this->appConfig, + $this->createMock(IGroupManager::class), + $this->auditLogService, + ); + } + + public static function dataSetValue(): array { + return [ + [ConfigLexicon::FEDERATED_TEAMS_ENABLED, 'yes', 'yes'], + [ConfigLexicon::FEDERATED_TEAMS_ENABLED, 'anything', 'no'], + [ConfigLexicon::FEDERATED_TEAMS_FRONTAL, 'https://cloud.example.com/', 'https://cloud.example.com/'], + [ConfigLexicon::TEAM_FOLDER_DEFAULT_QUOTA, '1024', '1024'], + [ConfigLexicon::TEAM_FOLDER_GROUP_QUOTAS, '{"admin":2048}', '{"admin":2048}'], + ]; + } + + #[DataProvider('dataSetValue')] + public function testSetValue(string $key, string $value, string $loggedValue): void { + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::ConfigUpdated, 'Teams setting "%s" was set to "%s"', [ + 'key' => $key, + 'value' => $loggedValue, + ]); + + $this->assertSame(Http::STATUS_OK, $this->controller->setValue($key, $value)->getStatus()); + } + + public static function dataSetInvalidValue(): array { + return [ + [ConfigLexicon::FEDERATED_TEAMS_FRONTAL, 'not a url'], + [ConfigLexicon::TEAM_FOLDER_DEFAULT_QUOTA, '-1'], + [ConfigLexicon::TEAM_FOLDER_GROUP_QUOTAS, '{"admin":-1}'], + ['unknown_key', 'value'], + ]; + } + + #[DataProvider('dataSetInvalidValue')] + public function testSetInvalidValueNotLogged(string $key, string $value): void { + foreach (['setAppValueString', 'setAppValueBool', 'setAppValueInt', 'setAppValueArray'] as $method) { + $this->appConfig->expects($this->never()) + ->method($method); + } + $this->auditLogService->expects($this->never()) + ->method('log'); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $this->controller->setValue($key, $value)->getStatus()); + } +} From 6201f62065f6f93e783b20145b32e18202d92cea Mon Sep 17 00:00:00 2001 From: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> Date: Mon, 26 Oct 2026 13:22:05 +0100 Subject: [PATCH 5/7] Update lib/AuditLog/Operation.php Co-authored-by: Cristian Scheid <74515775+cristianscheid@users.noreply.github.com> Signed-off-by: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> --- lib/AuditLog/Operation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 9ff4e05cc..eefa60452 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -49,7 +49,7 @@ public function description(): string { self::TeamSettingUpdated => 'A setting of a team, for example the password of its shares, was set or removed', self::InvitationCreated => 'An invitation link to join a team was created', self::InvitationRevoked => 'The invitation link to join a team was revoked', - self::MemberAdded => 'A user, group, team, email address or contact was added to a team, or a request to join it was accepted', + self::MemberAdded => 'A member was added to a team, or a request to join it was accepted', self::MemberInvited => 'A user, group or team was invited to join an invite only team', self::MemberRemoved => 'A member was removed from a team', self::MemberLevelUpdated => 'The level of a member of a team (member, moderator, admin, owner) was changed, including ownership transfers', From 24b3e79b241f8640b98768b33ca0f8b408d3369b Mon Sep 17 00:00:00 2001 From: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> Date: Mon, 26 Oct 2026 13:22:37 +0100 Subject: [PATCH 6/7] Update lib/AuditLog/Operation.php Co-authored-by: Cristian Scheid <74515775+cristianscheid@users.noreply.github.com> Signed-off-by: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> --- lib/AuditLog/Operation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index eefa60452..1ca49bb82 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -43,7 +43,7 @@ enum Operation: string { public function description(): string { return match ($this) { self::TeamCreated => 'A team was created', - self::TeamDeleted => 'A team was deleted, directly or because its last member left it', + self::TeamDeleted => 'A team was deleted', self::TeamRenamed => 'A team was renamed', self::TeamConfigUpdated => 'The configuration flags of a team (open, invite only, visible, federated, ...) were changed', self::TeamSettingUpdated => 'A setting of a team, for example the password of its shares, was set or removed', From 93c96bd37df18eda0ecda517bb54b102299b3b7b Mon Sep 17 00:00:00 2001 From: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> Date: Mon, 26 Oct 2026 13:22:47 +0100 Subject: [PATCH 7/7] Update lib/AuditLog/Operation.php Co-authored-by: Cristian Scheid <74515775+cristianscheid@users.noreply.github.com> Signed-off-by: kefi ghassen <84786282+GhassenKefi@users.noreply.github.com> --- lib/AuditLog/Operation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index 1ca49bb82..ecb4c34e2 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -60,7 +60,7 @@ public function description(): string { self::RemoteInstanceUpdated => 'The identity or the type of a known remote instance for federated teams was changed', self::RemoteInstanceRemoved => 'A remote instance for federated teams was removed', self::ScimInstanceAdded => 'A remote instance was allowed to add members to teams managed by SCIM', - self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM, removing the members it added', + self::ScimInstanceRemoved => 'A remote instance was no longer allowed to add members to teams managed by SCIM', self::ConfigUpdated => 'An admin setting of Teams (federated teams, frontal address, team folder quotas) was changed', }; }