From 086520cef3955db64219aa45cca161ebedcea9ae Mon Sep 17 00:00:00 2001 From: Cristian Scheid Date: Thu, 29 Oct 2026 07:28:19 -0300 Subject: [PATCH] feat: add audit on relevant places Signed-off-by: Cristian Scheid --- lib/AuditLog/Operation.php | 8 + lib/Controller/LocalController.php | 11 ++ lib/Controller/OidcController.php | 20 ++- lib/Controller/RemoteController.php | 13 ++ .../lib/Controller/LocalControllerTest.php | 17 +++ .../lib/Controller/OidcControllerTest.php | 137 ++++++++++++++++++ .../lib/Controller/RemoteControllerTest.php | 119 +++++++++++++++ 7 files changed, 324 insertions(+), 1 deletion(-) create mode 100644 tests/unit/lib/Controller/OidcControllerTest.php create mode 100644 tests/unit/lib/Controller/RemoteControllerTest.php diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index ecb4c34e2..6fbf60cd0 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -23,6 +23,7 @@ enum Operation: string { case TeamSettingUpdated = 'circles.team_setting.updated'; case InvitationCreated = 'circles.invitation.created'; case InvitationRevoked = 'circles.invitation.revoked'; + case InvitationCodeRejected = 'circles.invitation_code.rejected'; case MemberAdded = 'circles.member.added'; case MemberInvited = 'circles.member.invited'; case MemberRemoved = 'circles.member.removed'; @@ -33,8 +34,11 @@ enum Operation: string { case RemoteInstanceAdded = 'circles.remote_instance.added'; case RemoteInstanceUpdated = 'circles.remote_instance.updated'; case RemoteInstanceRemoved = 'circles.remote_instance.removed'; + case RemoteRequestRejected = 'circles.remote_request.rejected'; case ScimInstanceAdded = 'circles.scim_instance.added'; case ScimInstanceRemoved = 'circles.scim_instance.removed'; + case OidcAccountLinked = 'circles.oidc_account.linked'; + case OidcAccountLinkFailed = 'circles.oidc_account.link_failed'; case ConfigUpdated = 'circles.config.updated'; /** @@ -49,6 +53,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::InvitationCodeRejected => 'An invitation code that matches no team was used to look up or join a team', 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', @@ -59,8 +64,11 @@ public function description(): string { 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::RemoteRequestRejected => 'A federated teams request was rejected', 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', + self::OidcAccountLinked => 'A user linked their OIDC account', + self::OidcAccountLinkFailed => 'Linking an OIDC account failed', self::ConfigUpdated => 'An admin setting of Teams (federated teams, frontal address, team folder quotas) was changed', }; } diff --git a/lib/Controller/LocalController.php b/lib/Controller/LocalController.php index 5c2125f9c..06d7022bc 100644 --- a/lib/Controller/LocalController.php +++ b/lib/Controller/LocalController.php @@ -10,6 +10,7 @@ namespace OCA\Circles\Controller; use Exception; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Exceptions\FederatedUserException; use OCA\Circles\Exceptions\FederatedUserNotFoundException; use OCA\Circles\Exceptions\FrontendException; @@ -23,6 +24,7 @@ use OCA\Circles\Model\Member; use OCA\Circles\Model\Probes\BasicProbe; use OCA\Circles\Model\Probes\CircleProbe; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\AvatarService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\ConfigService; @@ -66,6 +68,7 @@ public function __construct( private SearchService $searchService, private AvatarService $avatarService, private ConfigService $configService, + private AuditLogService $auditLogService, ) { parent::__construct($appName, $request); $this->setup('app', 'circles'); @@ -662,6 +665,7 @@ public function getInvitation(string $invitationCode): DataResponse { $circles = $this->circleService->getCircles($circleProbe); if (empty($circles)) { + $this->logRejectedInvitationCode(); return new DataResponse([], Http::STATUS_NOT_FOUND); } $circle = reset($circles); @@ -699,6 +703,7 @@ public function joinInvitation(string $invitationCode): DataResponse { $circles = $this->circleService->getCircles($circleProbe); if (empty($circles)) { + $this->logRejectedInvitationCode(); return new DataResponse([], Http::STATUS_NOT_FOUND); } $circle = reset($circles); @@ -728,4 +733,10 @@ private function setCurrentFederatedUser(): void { $user = $this->userSession->getUser(); $this->federatedUserService->setLocalCurrentUser($user); } + + private function logRejectedInvitationCode(): void { + $this->auditLogService->log(Operation::InvitationCodeRejected, 'Invalid team invitation code used by "%s"', [ + 'userId' => $this->userSession->getUser()?->getUID(), + ]); + } } diff --git a/lib/Controller/OidcController.php b/lib/Controller/OidcController.php index fc84ae749..2dad2df67 100644 --- a/lib/Controller/OidcController.php +++ b/lib/Controller/OidcController.php @@ -10,7 +10,9 @@ namespace OCA\Circles\Controller; use OCA\Circles\AppInfo\Application; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\OidcService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; @@ -40,6 +42,7 @@ public function __construct( private readonly IClientService $clientService, private readonly ICredentialsManager $credentialsManager, private readonly OidcService $oidcService, + private readonly AuditLogService $auditLogService, private readonly LoggerInterface $logger, ) { parent::__construct(Application::APP_ID, $request); @@ -83,18 +86,21 @@ public function connect(): RedirectResponse { #[NoAdminRequired] #[NoCSRFRequired] public function callback(string $state = '', string $code = '', string $error = '', string $error_description = ''): RedirectResponse { + $userId = $this->userSession->getUser()?->getUID(); if ($error !== '') { $this->logger->error('OIDC provider returned an error: ' . $error . ' - ' . $error_description); + $this->logFailedLink($userId, 'provider_error'); return $this->redirectToPersonalSettings('error'); } if ($state === '' || $state !== $this->session->get(self::SESSION_STATE)) { $this->logger->error('OIDC callback state mismatch'); + $this->logFailedLink($userId, 'state_mismatch'); return $this->redirectToPersonalSettings('error'); } - $userId = $this->userSession->getUser()?->getUID(); if ($userId === null || $userId !== $this->session->get(self::SESSION_USER_ID)) { $this->logger->error('OIDC callback user mismatch: started as ' . $this->session->get(self::SESSION_USER_ID) . ', completed as ' . $userId); + $this->logFailedLink($userId, 'user_mismatch'); return $this->redirectToPersonalSettings('error'); } $this->session->remove(self::SESSION_STATE); @@ -117,6 +123,7 @@ public function callback(string $state = '', string $code = '', string $error = ]); } catch (\Exception $e) { $this->logger->error('OIDC token exchange failed', ['exception' => $e]); + $this->logFailedLink($userId, 'token_exchange_failed'); return $this->redirectToPersonalSettings('error'); } @@ -124,10 +131,14 @@ public function callback(string $state = '', string $code = '', string $error = if (empty($data['refresh_token'])) { $this->logger->error('OIDC provider did not return a refresh_token for user ' . $userId); + $this->logFailedLink($userId, 'missing_refresh_token'); return $this->redirectToPersonalSettings('error'); } $this->credentialsManager->store($userId, OidcService::CREDENTIAL_REFRESH_TOKEN, $data['refresh_token']); + $this->auditLogService->log(Operation::OidcAccountLinked, 'OIDC account of "%s" linked to synchronize team memberships', [ + 'userId' => $userId, + ]); // initial sync if (!empty($data['access_token'])) { @@ -137,6 +148,13 @@ public function callback(string $state = '', string $code = '', string $error = return $this->redirectToPersonalSettings('success'); } + private function logFailedLink(?string $userId, string $reason): void { + $this->auditLogService->log(Operation::OidcAccountLinkFailed, 'Linking the OIDC account of "%s" failed: %s', [ + 'userId' => $userId, + 'reason' => $reason, + ]); + } + private function redirectToPersonalSettings(string $result): RedirectResponse { $url = $this->urlGenerator->linkToRoute('settings.PersonalSettings.index', ['section' => 'circles']); diff --git a/lib/Controller/RemoteController.php b/lib/Controller/RemoteController.php index c750fccec..b90f3e009 100644 --- a/lib/Controller/RemoteController.php +++ b/lib/Controller/RemoteController.php @@ -11,6 +11,7 @@ use Exception; use OC\AppFramework\Middleware\Security\Exceptions\NotLoggedInException; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\ConfigLexicon; use OCA\Circles\Db\CircleRequest; use OCA\Circles\Exceptions\FederatedEventException; @@ -26,6 +27,7 @@ use OCA\Circles\Model\Member; use OCA\Circles\Model\Probes\BasicProbe; use OCA\Circles\Model\Probes\CircleProbe; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\FederatedUserService; @@ -78,6 +80,7 @@ public function __construct( private readonly InterfaceService $interfaceService, private readonly ConfigService $configService, private readonly IUserSession $userSession, + private readonly AuditLogService $auditLogService, ) { parent::__construct($appName, $request); @@ -397,6 +400,7 @@ private function extractDataFromFromRequest(): SimpleDataStore { /** @var FederatedUser $initiator */ $initiator = $store->gObj('initiator', FederatedUser::class); if ($initiator->getInstance() !== $remoteInstance->getInstance() && !in_array($initiator->getInstance(), $remoteInstance->getAliases(), true)) { + $this->logRejectedRequest($remoteInstance->getInstance(), 'initiator_mismatch'); throw new InvalidRemoteInstanceException('Initiator instance does not match remote instance'); } $this->federatedUserService->setCurrentUser($initiator); @@ -432,12 +436,14 @@ private function confirmRemoteInstance(NCSignedRequest $signedRequest): RemoteIn if (!$signatory instanceof RemoteInstance) { $this->debug('Signatory is not a known RemoteInstance', ['signedRequest' => $signedRequest]); + $this->logRejectedRequest($signedRequest->getOrigin(), 'unknown_signatory'); throw new SignatoryException('Could not confirm identity'); } if (!$this->configService->isLocalInstance($signedRequest->getOrigin()) && $signatory->getType() === RemoteInstance::TYPE_UNKNOWN) { $this->debug('Could not confirm identity', ['signedRequest' => $signedRequest]); + $this->logRejectedRequest($signedRequest->getOrigin(), 'untrusted_instance'); throw new SignatoryException('Could not confirm identity'); } @@ -446,6 +452,13 @@ private function confirmRemoteInstance(NCSignedRequest $signedRequest): RemoteIn return $signatory; } + private function logRejectedRequest(?string $instance, string $reason): void { + $this->auditLogService->log(Operation::RemoteRequestRejected, 'Federated request from instance "%s" rejected: %s', [ + 'instance' => $instance, + 'reason' => $reason, + ]); + } + /** * @param Exception $e * @param int $httpErrorCode diff --git a/tests/unit/lib/Controller/LocalControllerTest.php b/tests/unit/lib/Controller/LocalControllerTest.php index 528be6c25..84c993a6e 100644 --- a/tests/unit/lib/Controller/LocalControllerTest.php +++ b/tests/unit/lib/Controller/LocalControllerTest.php @@ -8,9 +8,11 @@ namespace OCA\Circles\Tests\Controller; use OCA\Circles\AppInfo\Application; +use OCA\Circles\AuditLog\Operation; use OCA\Circles\Controller\LocalController; use OCA\Circles\Model\Circle; use OCA\Circles\Model\Member; +use OCA\Circles\Service\AuditLogService; use OCA\Circles\Service\AvatarService; use OCA\Circles\Service\CircleService; use OCA\Circles\Service\ConfigService; @@ -25,6 +27,7 @@ use OCP\IRequest; use OCP\IUserManager; use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Container\ContainerInterface; use Test\TestCase; @@ -45,6 +48,7 @@ class LocalControllerTest extends TestCase { private LocalController $localController; private IUserManager $userManager; private IUserSession $userSession; + private AuditLogService&MockObject $auditLogService; private string $circleId = ''; private array $circlesToCleanup = []; private static array $usersToCleanup = []; @@ -74,6 +78,7 @@ public function setUp(): void { $user1 = $this->userManager->get(self::TEST_USER_1); $this->userSession->setUser($user1); + $this->auditLogService = $this->createMock(AuditLogService::class); $this->localController = new LocalController( Application::APP_ID, @@ -87,6 +92,7 @@ public function setUp(): void { $this->container->get(SearchService::class), $this->container->get(AvatarService::class), $this->container->get(ConfigService::class), + $this->auditLogService, ); // Create a circle as TEST_USER_1 (owner) @@ -397,4 +403,15 @@ public function testMembersAddPermissionDenied(): void { $response = $this->localController->membersAdd($this->circleId, $members); $this->assertEquals($response->getData()['message'], 'Insufficient permissions to perform this action'); } + + public function testInvalidInvitationCodeWritesAuditLog(): void { + $this->auditLogService->expects($this->exactly(2)) + ->method('log') + ->with(Operation::InvitationCodeRejected, 'Invalid team invitation code used by "%s"', [ + 'userId' => self::TEST_USER_1, + ]); + + $this->assertSame(Http::STATUS_NOT_FOUND, $this->localController->getInvitation('invalid-code')->getStatus()); + $this->assertSame(Http::STATUS_NOT_FOUND, $this->localController->joinInvitation('invalid-code')->getStatus()); + } } diff --git a/tests/unit/lib/Controller/OidcControllerTest.php b/tests/unit/lib/Controller/OidcControllerTest.php new file mode 100644 index 000000000..cdc49a6db --- /dev/null +++ b/tests/unit/lib/Controller/OidcControllerTest.php @@ -0,0 +1,137 @@ +userSession = $this->createMock(IUserSession::class); + $this->session = $this->createMock(ISession::class); + $this->clientService = $this->createMock(IClientService::class); + $this->credentialsManager = $this->createMock(ICredentialsManager::class); + $this->auditLogService = $this->createMock(AuditLogService::class); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userSession->method('getUser')->willReturn($user); + } + + private function createController(string $sessionUserId = 'alice'): OidcController { + $this->session->method('get')->willReturnMap([ + ['circles.oidc.state', 'state'], + ['circles.oidc.user_id', $sessionUserId], + ]); + + return new OidcController( + $this->createMock(IRequest::class), + $this->createMock(IAppConfig::class), + $this->userSession, + $this->session, + $this->createMock(IURLGenerator::class), + $this->clientService, + $this->credentialsManager, + $this->createMock(OidcService::class), + $this->auditLogService, + $this->createMock(LoggerInterface::class), + ); + } + + private function expectFailedLink(string $reason): void { + $this->credentialsManager->expects($this->never())->method('store'); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::OidcAccountLinkFailed, 'Linking the OIDC account of "%s" failed: %s', [ + 'userId' => 'alice', + 'reason' => $reason, + ]); + } + + private function mockTokenResponse(array $data): void { + $response = $this->createMock(IResponse::class); + $response->method('getBody')->willReturn(json_encode($data)); + $client = $this->createMock(IClient::class); + $client->method('post')->willReturn($response); + $this->clientService->method('newClient')->willReturn($client); + } + + public function testCallbackWritesAuditLog(): void { + $this->mockTokenResponse(['refresh_token' => 'refresh']); + $this->credentialsManager->expects($this->once())->method('store'); + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::OidcAccountLinked, 'OIDC account of "%s" linked to synchronize team memberships', [ + 'userId' => 'alice', + ]); + + $this->createController()->callback('state', 'code'); + } + + public static function dataCallbackFailed(): array { + return [ + 'provider error' => ['state', 'access_denied', 'provider_error'], + 'state mismatch' => ['other', '', 'state_mismatch'], + ]; + } + + #[DataProvider('dataCallbackFailed')] + public function testCallbackFailedWritesAuditLog(string $state, string $error, string $reason): void { + $this->expectFailedLink($reason); + + $this->createController()->callback($state, 'code', $error); + } + + public function testCallbackUserMismatchWritesAuditLog(): void { + $this->expectFailedLink('user_mismatch'); + + $this->createController('bob')->callback('state', 'code'); + } + + public function testCallbackTokenExchangeFailedWritesAuditLog(): void { + $client = $this->createMock(IClient::class); + $client->method('post')->willThrowException(new \Exception('connection refused')); + $this->clientService->method('newClient')->willReturn($client); + $this->expectFailedLink('token_exchange_failed'); + + $this->createController()->callback('state', 'code'); + } + + public function testCallbackWithoutRefreshTokenWritesAuditLog(): void { + $this->mockTokenResponse([]); + $this->expectFailedLink('missing_refresh_token'); + + $this->createController()->callback('state', 'code'); + } +} diff --git a/tests/unit/lib/Controller/RemoteControllerTest.php b/tests/unit/lib/Controller/RemoteControllerTest.php new file mode 100644 index 000000000..8d303acb7 --- /dev/null +++ b/tests/unit/lib/Controller/RemoteControllerTest.php @@ -0,0 +1,119 @@ +remoteStreamService = $this->createMock(RemoteStreamService::class); + $this->auditLogService = $this->createMock(AuditLogService::class); + + $interfaceService = $this->createMock(InterfaceService::class); + $interfaceService->method('getCurrentInterface')->willReturn(InterfaceService::IFACE_INTERNAL); + + $this->controller = new RemoteController( + 'circles', + $this->createMock(IRequest::class), + $this->createMock(CircleRequest::class), + $this->createMock(IAppConfig::class), + $this->remoteStreamService, + $this->createMock(RemoteDownstreamService::class), + $this->createMock(FederatedUserService::class), + $this->createMock(CircleService::class), + $this->createMock(MemberService::class), + $this->createMock(MembershipService::class), + $interfaceService, + $this->createMock(ConfigService::class), + $this->createMock(IUserSession::class), + $this->auditLogService, + ); + } + + private function expectRejectedRequest(?string $instance, string $reason): void { + $this->auditLogService->expects($this->once()) + ->method('log') + ->with(Operation::RemoteRequestRejected, 'Federated request from instance "%s" rejected: %s', [ + 'instance' => $instance, + 'reason' => $reason, + ]); + } + + private function mockSignedRequest(NCSignatory $signatory, string $body = '{}'): void { + $signedRequest = $this->createMock(NCSignedRequest::class); + $signedRequest->method('getSignatory')->willReturn($signatory); + $signedRequest->method('getOrigin')->willReturn('remote.example'); + $signedRequest->method('getBody')->willReturn($body); + $this->remoteStreamService->method('incomingSignedRequest')->willReturn($signedRequest); + } + + private function createRemoteInstance(string $type): RemoteInstance { + $remoteInstance = new RemoteInstance(); + $remoteInstance->setType($type); + $remoteInstance->setInstance('remote.example'); + + return $remoteInstance; + } + + public function testUnknownSignatoryWritesAuditLog(): void { + $this->mockSignedRequest(new NCSignatory()); + $this->expectRejectedRequest('remote.example', 'unknown_signatory'); + + $this->assertSame(Http::STATUS_UNAUTHORIZED, $this->controller->event()->getStatus()); + } + + public function testUntrustedInstanceWritesAuditLog(): void { + $this->mockSignedRequest($this->createRemoteInstance(RemoteInstance::TYPE_UNKNOWN)); + $this->expectRejectedRequest('remote.example', 'untrusted_instance'); + + $this->assertSame(Http::STATUS_UNAUTHORIZED, $this->controller->event()->getStatus()); + } + + public function testInitiatorMismatchWritesAuditLog(): void { + $this->mockSignedRequest($this->createRemoteInstance(RemoteInstance::TYPE_TRUSTED), json_encode([ + 'initiator' => [ + 'id' => 'singleId', + 'userId' => 'alice', + 'userType' => Member::TYPE_USER, + 'instance' => 'other.example', + ], + ])); + $this->expectRejectedRequest('remote.example', 'initiator_mismatch'); + + $this->assertSame(Http::STATUS_UNAUTHORIZED, $this->controller->circles()->getStatus()); + } +}