From d052c4f17df891e2fe9d0b5c75e0dfebd9d71875 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Thu, 15 Oct 2026 12:09:17 +0200 Subject: [PATCH 1/3] feat(core): log app passwords, password reset requests and finished remote wipes App passwords created through login flow v1 and v2 and the app password API, and deleting an app password through the API, are now written to the audit log with the same operations the settings app uses (auth.app_password.created and auth.app_password.deleted). A password reset request is logged with the resolved user ID once the reset token has been created, and a finished remote wipe is logged when the device confirms it. The token and the password are never logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- apps/admin_audit/lib/Operation.php | 2 + core/Controller/AppPasswordController.php | 19 ++++++ core/Controller/ClientFlowLoginController.php | 7 +++ core/Controller/LostController.php | 8 +++ core/Service/LoginFlowV2Service.php | 11 +++- .../Authentication/Token/RemoteWipe.php | 7 +++ .../Controller/AppPasswordControllerTest.php | 63 +++++++++++++++++-- .../ClientFlowLoginControllerTest.php | 34 +++++++--- tests/Core/Controller/LostControllerTest.php | 9 +++ .../Service/LoginFlowV2ServiceUnitTest.php | 19 +++++- .../Authentication/Token/RemoteWipeTest.php | 23 +++++++ 11 files changed, 187 insertions(+), 15 deletions(-) diff --git a/apps/admin_audit/lib/Operation.php b/apps/admin_audit/lib/Operation.php index c88bffdff2178..f3a604042abde 100644 --- a/apps/admin_audit/lib/Operation.php +++ b/apps/admin_audit/lib/Operation.php @@ -49,6 +49,8 @@ enum Operation: string { case LogoutPerformed = 'auth.logout.performed'; case TwoFactorPassed = 'auth.twofactor.passed'; case TwoFactorFailed = 'auth.twofactor.failed'; + case PasswordResetRequested = 'auth.password_reset.requested'; + case RemoteWipeFinished = 'auth.remote_wipe.finished'; // Users case UserCreated = 'users.user.created'; diff --git a/core/Controller/AppPasswordController.php b/core/Controller/AppPasswordController.php index faa26a31913f7..f2835b1cbe0eb 100644 --- a/core/Controller/AppPasswordController.php +++ b/core/Controller/AppPasswordController.php @@ -33,6 +33,7 @@ use OCP\IRequest; use OCP\ISession; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\ISecureRandom; @@ -104,6 +105,12 @@ public function getAppPassword(): DataResponse { $this->eventDispatcher->dispatchTyped( new AppPasswordCreatedEvent($generatedToken) ); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'App password with ID "%s" and name "%s" was created', + ['tokenId' => $generatedToken->getId(), 'tokenName' => $generatedToken->getName()], + false, + 'auth.app_password.created', + )); return new DataResponse([ 'apppassword' => $token @@ -134,6 +141,12 @@ public function deleteAppPassword(): DataResponse { } $this->tokenProvider->invalidateTokenById($token->getUID(), $token->getId()); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'App password with ID "%s" was deleted', + ['tokenId' => $token->getId()], + false, + 'auth.app_password.deleted', + )); return new DataResponse(); } @@ -242,6 +255,12 @@ public function getAppPasswordWithOneTimePassword(): DataResponse { $this->eventDispatcher->dispatchTyped( new AppPasswordCreatedEvent($generatedToken) ); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'App password with ID "%s" and name "%s" was created', + ['tokenId' => $generatedToken->getId(), 'tokenName' => $generatedToken->getName()], + false, + 'auth.app_password.created', + )); return new DataResponse([ 'apppassword' => $token diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index dbf1570e476eb..06a6db515a123 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -39,6 +39,7 @@ use OCP\ISession; use OCP\IURLGenerator; use OCP\IUserSession; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use OCP\Session\Exceptions\SessionNotAvailableException; @@ -337,6 +338,12 @@ public function generateAppPassword( $this->eventDispatcher->dispatchTyped( new AppPasswordCreatedEvent($generatedToken) ); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'App password with ID "%s" and name "%s" was created', + ['tokenId' => $generatedToken->getId(), 'tokenName' => $generatedToken->getName()], + false, + 'auth.app_password.created', + )); return new RedirectResponse($redirectUri); } diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 184af1dee9126..b25052306ce77 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -37,6 +37,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Mail\IMailer; use OCP\PreConditionNotMetException; use OCP\Security\VerificationToken\InvalidTokenException; @@ -283,6 +284,13 @@ protected function sendEmail(string $input): void { // Log the exception and continue $this->logger->error($e->getMessage(), ['app' => 'core', 'exception' => $e]); } + + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Password reset was requested for user "%s"', + ['userId' => $user->getUID()], + false, + 'auth.password_reset.requested', + )); } /** diff --git a/core/Service/LoginFlowV2Service.php b/core/Service/LoginFlowV2Service.php index 5d6048230c638..47707b909ecf6 100644 --- a/core/Service/LoginFlowV2Service.php +++ b/core/Service/LoginFlowV2Service.php @@ -21,7 +21,9 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\Exceptions\InvalidTokenException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; @@ -35,6 +37,7 @@ public function __construct( private ICrypto $crypto, private LoggerInterface $logger, private IProvider $tokenProvider, + private IEventDispatcher $eventDispatcher, ) { } @@ -153,7 +156,7 @@ public function flowDone(string $loginToken, string $sessionId, string $server, } $appPassword = $this->random->generate(72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS); - $this->tokenProvider->generateToken( + $generatedToken = $this->tokenProvider->generateToken( $appPassword, $userId, $loginName, @@ -162,6 +165,12 @@ public function flowDone(string $loginToken, string $sessionId, string $server, IToken::PERMANENT_TOKEN, IToken::DO_NOT_REMEMBER ); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'App password with ID "%s" and name "%s" was created', + ['tokenId' => $generatedToken->getId(), 'tokenName' => $generatedToken->getName()], + false, + 'auth.app_password.created', + )); $data->setLoginName($loginName); $data->setServer($server); diff --git a/lib/private/Authentication/Token/RemoteWipe.php b/lib/private/Authentication/Token/RemoteWipe.php index 08dce6fa5a1fb..48ec8a7639715 100644 --- a/lib/private/Authentication/Token/RemoteWipe.php +++ b/lib/private/Authentication/Token/RemoteWipe.php @@ -15,6 +15,7 @@ use OCP\Authentication\Exceptions\WipeTokenException; use OCP\EventDispatcher\IEventDispatcher; use OCP\IUser; +use OCP\Log\Audit\CriticalActionPerformedEvent; use Psr\Log\LoggerInterface; use function array_filter; @@ -119,6 +120,12 @@ public function finish(string $token): bool { $this->logger->info('user ' . $dbToken->getUID() . ' finished a remote wipe'); $this->eventDispatcher->dispatch(RemoteWipeFinished::class, new RemoteWipeFinished($dbToken)); + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( + 'Remote wipe of app password with ID "%s" of user "%s" was finished', + ['tokenId' => $dbToken->getId(), 'userId' => $dbToken->getUID()], + false, + 'auth.remote_wipe.finished', + )); return true; } diff --git a/tests/Core/Controller/AppPasswordControllerTest.php b/tests/Core/Controller/AppPasswordControllerTest.php index 123a61efa119b..c4e05ea63f9f4 100644 --- a/tests/Core/Controller/AppPasswordControllerTest.php +++ b/tests/Core/Controller/AppPasswordControllerTest.php @@ -8,6 +8,7 @@ namespace Tests\Core\Controller; +use OC\Authentication\Events\AppPasswordCreatedEvent; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; @@ -19,11 +20,13 @@ use OCP\Authentication\Exceptions\PasswordUnavailableException; use OCP\Authentication\LoginCredentials\ICredentials; use OCP\Authentication\LoginCredentials\IStore; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\Bruteforce\IThrottler; use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; @@ -170,8 +173,7 @@ public function testGetAppPassword(): void { IToken::DO_NOT_REMEMBER ); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $this->controller->getAppPassword(); } @@ -215,8 +217,7 @@ public function testGetAppPasswordNoPassword(): void { IToken::DO_NOT_REMEMBER ); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $this->controller->getAppPassword(); } @@ -289,8 +290,62 @@ public function testDeleteAppPasswordSuccess(): void { 42 ); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'auth.app_password.deleted' + && $event->getParameters() === ['tokenId' => 42] + && $event->getLogMessage() === 'App password with ID "%s" was deleted')); + $result = $this->controller->deleteAppPassword(); $this->assertEquals(new DataResponse(), $result); } + + public function testGetAppPasswordWithOneTimePassword(): void { + $credentials = $this->createMock(ICredentials::class); + + $this->session->method('exists') + ->with('one_time_token') + ->willReturn(true); + $this->credentialStore->method('getLoginCredentials') + ->willReturn($credentials); + $credentials->method('getUid') + ->willReturn('myUID'); + $credentials->method('getPassword') + ->willReturn('myPassword'); + $credentials->method('getLoginName') + ->willReturn('myLoginName'); + $this->request->method('getHeader') + ->with('user-agent') + ->willReturn('myUA'); + $this->random->method('generate') + ->willReturn('myToken'); + + $this->tokenProvider->expects($this->once()) + ->method('generateToken') + ->with('myToken', 'myUID', 'myLoginName', 'myPassword', 'myUA', IToken::PERMANENT_TOKEN); + + $this->expectAppPasswordCreatedEvents(); + + $this->assertEquals(new DataResponse(['apppassword' => 'myToken']), $this->controller->getAppPasswordWithOneTimePassword()); + } + + /** + * Expects the AppPasswordCreatedEvent followed by its audit log entry. + */ + private function expectAppPasswordCreatedEvents(): void { + $calls = 0; + $this->eventDispatcher->expects($this->exactly(2)) + ->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use (&$calls): void { + if (++$calls === 1) { + $this->assertInstanceOf(AppPasswordCreatedEvent::class, $event); + return; + } + $this->assertInstanceOf(CriticalActionPerformedEvent::class, $event); + $this->assertSame('auth.app_password.created', $event->getOperation()); + $this->assertSame('App password with ID "%s" and name "%s" was created', $event->getLogMessage()); + $this->assertSame(['tokenId', 'tokenName'], array_keys($event->getParameters())); + }); + } } diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 772562507429c..bcd34c6c462fa 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -9,6 +9,7 @@ namespace Tests\Core\Controller; +use OC\Authentication\Events\AppPasswordCreatedEvent; use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Token\IProvider; @@ -25,6 +26,7 @@ use OCP\AppFramework\Services\IInitialState; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; +use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IL10N; @@ -33,6 +35,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use OCP\Session\Exceptions\SessionNotAvailableException; @@ -409,8 +412,7 @@ public function testGeneratePasswordWithPassword(): void { ->method('getHeader') ->willReturn(''); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $expected = new RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken'); $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); @@ -500,8 +502,7 @@ public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $re ->with('MyClientIdentifier') ->willReturn($client); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $expected = new RedirectResponse($redirectUrl); $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken', 'MyClientIdentifier')); @@ -575,8 +576,7 @@ public function testGeneratePasswordWithoutPassword(): void { ->method('getHeader') ->willReturn(''); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $expected = new RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken'); $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); @@ -706,10 +706,28 @@ public function testGeneratePasswordWithHttpsProxy(array $headers, $protocol, $e ->method('getHeader') ->willReturnMap($headers); - $this->eventDispatcher->expects($this->once()) - ->method('dispatchTyped'); + $this->expectAppPasswordCreatedEvents(); $expected = new RedirectResponse('nc://login/server:' . $expected . '://example.com&user:MyLoginName&password:MyGeneratedToken'); $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken')); } + + /** + * Expects the AppPasswordCreatedEvent followed by its audit log entry. + */ + private function expectAppPasswordCreatedEvents(): void { + $calls = 0; + $this->eventDispatcher->expects($this->exactly(2)) + ->method('dispatchTyped') + ->willReturnCallback(function (Event $event) use (&$calls): void { + if (++$calls === 1) { + $this->assertInstanceOf(AppPasswordCreatedEvent::class, $event); + return; + } + $this->assertInstanceOf(CriticalActionPerformedEvent::class, $event); + $this->assertSame('auth.app_password.created', $event->getOperation()); + $this->assertSame('App password with ID "%s" and name "%s" was created', $event->getLogMessage()); + $this->assertSame(['tokenId', 'tokenName'], array_keys($event->getParameters())); + }); + } } diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index dc033d983f921..63a3ffdd2c5b3 100644 --- a/tests/Core/Controller/LostControllerTest.php +++ b/tests/Core/Controller/LostControllerTest.php @@ -26,6 +26,7 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; use OCP\Security\VerificationToken\InvalidTokenException; @@ -238,6 +239,8 @@ public function testEmailUnsuccessful(): void { $this->logger->expects($this->exactly(0)) ->method('error'); + $this->eventDispatcher->expects($this->never()) + ->method('dispatchTyped'); $this->logger->expects($this->exactly(2)) ->method('warning'); @@ -321,6 +324,12 @@ public function testEmailSuccessful(): void { ->method('send') ->with($message); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'auth.password_reset.requested' + && $event->getParameters() === ['userId' => 'ExistingUser'] + && $event->getLogMessage() === 'Password reset was requested for user "%s"')); + $response = $this->lostController->email('ExistingUser'); $expectedResponse = new JSONResponse(['status' => 'success']); $expectedResponse->throttle(); diff --git a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php index 35f939a275eeb..300626b9bc5d4 100644 --- a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php +++ b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php @@ -20,7 +20,9 @@ use OC\Core\Service\LoginFlowV2Service; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; @@ -41,6 +43,7 @@ class LoginFlowV2ServiceUnitTest extends TestCase { private ISecureRandom&MockObject $secureRandom; private ITimeFactory&MockObject $timeFactory; private IProvider&MockObject $tokenProvider; + private IEventDispatcher&MockObject $eventDispatcher; #[\Override] public function setUp(): void { @@ -60,6 +63,7 @@ private function setupSubjectUnderTest(): void { $this->mapper = $this->createMock(LoginFlowV2Mapper::class); $this->logger = $this->createMock(LoggerInterface::class); $this->tokenProvider = $this->createMock(IProvider::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->timeFactory = $this->createMock(ITimeFactory::class); @@ -70,7 +74,8 @@ private function setupSubjectUnderTest(): void { $this->config, $this->crypto, $this->logger, - $this->tokenProvider + $this->tokenProvider, + $this->eventDispatcher, ); } @@ -370,6 +375,10 @@ public function testFlowDone(): void { ->method('getToken') ->willReturn($sessionToken); + $appToken = $this->createMock(IToken::class); + $appToken->method('getId')->willReturn(7); + $appToken->method('getName')->willReturn('client_name'); + $this->tokenProvider->expects($this->once()) ->method('generateToken') ->with( @@ -380,7 +389,13 @@ public function testFlowDone(): void { 'client_name', IToken::PERMANENT_TOKEN, IToken::DO_NOT_REMEMBER - ); + ) + ->willReturn($appToken); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'auth.app_password.created' + && $event->getParameters() === ['tokenId' => 7, 'tokenName' => 'client_name'])); $result = $this->subjectUnderTest->flowDone( 'login_token', diff --git a/tests/lib/Authentication/Token/RemoteWipeTest.php b/tests/lib/Authentication/Token/RemoteWipeTest.php index a256c9d609456..4d2b2e2e2cd2a 100644 --- a/tests/lib/Authentication/Token/RemoteWipeTest.php +++ b/tests/lib/Authentication/Token/RemoteWipeTest.php @@ -18,6 +18,7 @@ use OC\Authentication\Token\RemoteWipe; use OCP\EventDispatcher\IEventDispatcher; use OCP\IUser; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -152,4 +153,26 @@ public function startFinishWiping() { $this->assertTrue($result); } + + public function testFinishWiping(): void { + $token = $this->createMock(IToken::class); + $token->method('getId')->willReturn(42); + $token->method('getUID')->willReturn('user1'); + $this->getAutoMock(ITokenProvider::class)->expects($this->once()) + ->method('getToken') + ->with('tk1') + ->willThrowException(new WipeTokenException($token)); + $this->getAutoMock(ITokenProvider::class)->expects($this->once()) + ->method('invalidateToken') + ->with('tk1'); + $this->getAutoMock(IEventDispatcher::class)->expects($this->once()) + ->method('dispatch') + ->with(RemoteWipeFinished::class, $this->equalTo(new RemoteWipeFinished($token))); + $this->getAutoMock(IEventDispatcher::class)->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(fn (CriticalActionPerformedEvent $event): bool => $event->getOperation() === 'auth.remote_wipe.finished' + && $event->getParameters() === ['tokenId' => 42, 'userId' => 'user1'])); + + $this->assertTrue($this->remoteWipe->finish('tk1')); + } } From 9883a71e8a5f3f53a972df135910dfd42e694d4d Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Thu, 15 Oct 2026 12:16:24 +0200 Subject: [PATCH 2/3] feat(sharing): log changes made through the unified sharing API The unified sharing API (behind sharing.unified_api_enable) changed shares, their sources, recipients, permissions, properties and recipient secrets without any audit entry. SharingManager now writes one entry per change, with unified_sharing.* operations, so they can be told apart from the legacy sharing.share.* entries, which carry different parameters. Recipient secrets (the public link token) and property values are never logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- apps/admin_audit/lib/Operation.php | 16 ++++ lib/private/Sharing/SharingManager.php | 100 ++++++++++++++++++++++- tests/lib/Sharing/SharingManagerTest.php | 71 ++++++++++++++++ 3 files changed, 185 insertions(+), 2 deletions(-) diff --git a/apps/admin_audit/lib/Operation.php b/apps/admin_audit/lib/Operation.php index f3a604042abde..7da7e737a50b2 100644 --- a/apps/admin_audit/lib/Operation.php +++ b/apps/admin_audit/lib/Operation.php @@ -36,6 +36,22 @@ enum Operation: string { case ShareExpirationRemoved = 'sharing.share.expiration_removed'; case ShareLinkAccessed = 'sharing.share.link_accessed'; + // Unified sharing API + // TODO: Merge with the legacy sharing.share.* operations above once the legacy sharing API is removed + case UnifiedShareCreated = 'unified_sharing.share.created'; + case UnifiedShareDeleted = 'unified_sharing.share.deleted'; + case UnifiedShareStateUpdated = 'unified_sharing.share.state_updated'; + case UnifiedShareUserStatusUpdated = 'unified_sharing.share.user_status_updated'; + case UnifiedShareSourceAdded = 'unified_sharing.source.added'; + case UnifiedShareSourceRemoved = 'unified_sharing.source.removed'; + case UnifiedShareRecipientAdded = 'unified_sharing.recipient.added'; + case UnifiedShareRecipientRemoved = 'unified_sharing.recipient.removed'; + case UnifiedShareRecipientSecretUpdated = 'unified_sharing.recipient_secret.updated'; + case UnifiedSharePropertyUpdated = 'unified_sharing.property.updated'; + case UnifiedSharePermissionUpdated = 'unified_sharing.permission.updated'; + case UnifiedShareRecipientPermissionUpdated = 'unified_sharing.recipient_permission.updated'; + case UnifiedSharePermissionPresetSelected = 'unified_sharing.permission_preset.selected'; + // Trash bin and versions case TrashbinFileDeleted = 'trashbin.file.deleted'; case TrashbinFileRestored = 'trashbin.file.restored'; diff --git a/lib/private/Sharing/SharingManager.php b/lib/private/Sharing/SharingManager.php index 0190d1408864a..a6d36ca28e782 100644 --- a/lib/private/Sharing/SharingManager.php +++ b/lib/private/Sharing/SharingManager.php @@ -42,6 +42,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\L10N\IFactory; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ISecureRandom; use OCP\Share\IManager; use OCP\Snowflake\ISnowflakeGenerator; @@ -64,7 +65,7 @@ private IL10N $l10n; public function __construct( - IEventDispatcher $eventDispatcher, + private IEventDispatcher $eventDispatcher, private IUserManager $userManager, private IFactory $l10nFactory, private ISnowflakeGenerator $snowflakeGenerator, @@ -78,7 +79,7 @@ public function __construct( $this->randomizer = new Randomizer(); $this->l10n = $l10nFactory->get('sharing'); - $eventDispatcher->addServiceListener(BeforeUserDeletedEvent::class, self::class); + $this->eventDispatcher->addServiceListener(BeforeUserDeletedEvent::class, self::class); } #[\Override] @@ -175,6 +176,11 @@ public function createShare(ShareAccessContext $accessContext): Share { $share = $this->backend->getShare($accessContext, $id); [$share] = $this->processShareUpdates([$share]); + $this->audit('Share "%s" was created by "%s"', [ + 'shareId' => $share->id, + 'owner' => $share->owner->userId, + ], 'unified_sharing.share.created'); + return $share; } @@ -215,6 +221,12 @@ public function updateShareState(ShareAccessContext $accessContext, Share $share ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('State of share "%s" was set to "%s"', [ + 'shareId' => $share->id, + 'state' => $state->value, + ], 'unified_sharing.share.state_updated'); + return $share; } @@ -245,6 +257,13 @@ public function updateShareUserStatus(ShareAccessContext $accessContext, Share $ ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s" was set to "%s" by recipient "%s"', [ + 'shareId' => $share->id, + 'userStatus' => $userStatus->value, + 'user' => $currentUser->getUID(), + ], 'unified_sharing.share.user_status_updated'); + return $share; } @@ -297,6 +316,13 @@ public function addShareSource(ShareAccessContext $accessContext, Share $share, [$share] = $this->backend->ensureDefaults([$share]); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": source %s "%s" was added', [ + 'shareId' => $share->id, + 'sourceClass' => $source->class, + 'sourceValue' => $source->value, + ], 'unified_sharing.source.added'); + return $share; } @@ -333,6 +359,13 @@ public function removeShareSource(ShareAccessContext $accessContext, Share $shar ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": source %s "%s" was removed', [ + 'shareId' => $share->id, + 'sourceClass' => $source->class, + 'sourceValue' => $source->value, + ], 'unified_sharing.source.removed'); + return $share; } @@ -431,6 +464,14 @@ public function addShareRecipient(ShareAccessContext $accessContext, Share $shar [$share] = $this->backend->ensureDefaults([$share]); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": recipient %s "%s" on "%s" was added', [ + 'shareId' => $share->id, + 'recipientClass' => $recipient->class, + 'recipientValue' => $recipient->value, + 'instance' => $recipient->instance ?? 'local', + ], 'unified_sharing.recipient.added'); + return $share; } @@ -469,6 +510,14 @@ public function removeShareRecipient(ShareAccessContext $accessContext, Share $s ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": recipient %s "%s" on "%s" was removed', [ + 'shareId' => $share->id, + 'recipientClass' => $recipient->class, + 'recipientValue' => $recipient->value, + 'instance' => $recipient->instance ?? 'local', + ], 'unified_sharing.recipient.removed'); + return $share; } @@ -568,6 +617,13 @@ public function updateShareRecipientSecret(ShareAccessContext $accessContext, Sh ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": the secret of recipient %s "%s" was changed', [ + 'shareId' => $share->id, + 'recipientClass' => $recipient->class, + 'recipientValue' => $recipient->value, + ], 'unified_sharing.recipient_secret.updated'); + return $share; } @@ -612,6 +668,12 @@ public function updateShareProperty(ShareAccessContext $accessContext, Share $sh ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": property %s was changed', [ + 'shareId' => $share->id, + 'propertyClass' => $property->class, + ], 'unified_sharing.property.updated'); + return $share; } @@ -648,6 +710,13 @@ public function updateSharePermission(ShareAccessContext $accessContext, Share $ $this->backend->updateSharePermission($share->id, $permission); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": permission %s was %s', [ + 'shareId' => $share->id, + 'permissionClass' => $permission->class, + 'enabled' => $permission->enabled ? 'enabled' : 'disabled', + ], 'unified_sharing.permission.updated'); + return $share; } @@ -703,6 +772,15 @@ public function updateShareRecipientPermission(ShareAccessContext $accessContext ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": permission %s of recipient %s "%s" was %s', [ + 'shareId' => $share->id, + 'permissionClass' => $permission->class, + 'recipientClass' => $recipient->class, + 'recipientValue' => $recipient->value, + 'enabled' => $permission->enabled ? 'enabled' : 'disabled', + ], 'unified_sharing.recipient_permission.updated'); + return $share; } @@ -740,6 +818,12 @@ public function selectSharePermissionPreset(ShareAccessContext $accessContext, S ); [$share] = $this->processShareUpdates([$share]); + + $this->audit('Share "%s": permission preset %s was selected', [ + 'shareId' => $share->id, + 'presetClass' => $permissionPresetClass, + ], 'unified_sharing.permission_preset.selected'); + return $share; } @@ -750,6 +834,18 @@ public function deleteShare(ShareAccessContext $accessContext, Share $share): vo $this->validateShareEditPermissions($accessContext, $share); $this->backend->deleteShare($share->id); + + $this->audit('Share "%s" of "%s" was deleted', [ + 'shareId' => $share->id, + 'owner' => $share->owner->userId, + ], 'unified_sharing.share.deleted'); + } + + /** + * @param array $params + */ + private function audit(string $message, array $params, string $operation): void { + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent($message, $params, false, $operation)); } #[\Override] diff --git a/tests/lib/Sharing/SharingManagerTest.php b/tests/lib/Sharing/SharingManagerTest.php index 2cdfeb6ec3102..7fb600c9825bc 100644 --- a/tests/lib/Sharing/SharingManagerTest.php +++ b/tests/lib/Sharing/SharingManagerTest.php @@ -19,9 +19,11 @@ use NCU\Sharing\ShareUserStatus; use NCU\Sharing\Source\ShareSource; use OC\Sharing\SharingManager; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\L10N\IFactory; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Server; use PHPUnit\Framework\Attributes\Group; @@ -303,4 +305,73 @@ protected function getShares(ShareAccessContext $accessContext, ?string $filterS throw $exception; } } + + public function testAuditLog(): void { + $this->registry->registerRecipientType(new TestShareRecipientTypePublicSecret(['recipient1' => 'Recipient 1'], [], true, true)); + + $auditEvents = []; + $listener = static function (CriticalActionPerformedEvent $event) use (&$auditEvents): void { + if (str_starts_with($event->getOperation() ?? '', 'unified_sharing.')) { + $auditEvents[] = $event; + } + }; + $eventDispatcher = Server::get(IEventDispatcher::class); + $eventDispatcher->addListener(CriticalActionPerformedEvent::class, $listener); + + $accessContext = new ShareAccessContext($this->owner); + $recipient = new ShareRecipient(TestShareRecipientType1::class, 'recipient1', null); + $secretRecipient = new ShareRecipient(TestShareRecipientTypePublicSecret::class, 'recipient1', null); + + try { + $this->dbConnection->beginTransaction(); + $share = $this->manager->createShare($accessContext); + $share = $this->manager->addShareSource($accessContext, $share, new ShareSource(TestShareSourceType1::class, 'source1')); + $share = $this->manager->addShareRecipient($accessContext, $share, $recipient); + $share = $this->manager->updateSharePermission($accessContext, $share, new SharePermission(TestSharePermissionType1::class, true)); + $share = $this->manager->updateShareProperty($accessContext, $share, new ShareProperty(TestSharePropertyType1::class, 'valid1')); + $share = $this->manager->updateShareState($accessContext, $share, ShareState::Active); + $share = $this->manager->addShareRecipient($accessContext, $share, $secretRecipient); + $share = $this->manager->updateShareRecipientSecret($accessContext, $share, $secretRecipient, 'mysecret'); + $share = $this->manager->updateShareRecipientPermission($accessContext, $share, $recipient, new SharePermission(TestSharePermissionType1::class, false)); + $share = $this->manager->selectSharePermissionPreset($accessContext, $share, TestSharePermissionPreset1::class); + $recipientContext = new ShareAccessContext($this->user1); + $this->manager->updateShareUserStatus($recipientContext, $this->manager->getShare($recipientContext, $share->id), ShareUserStatus::Accepted); + $share = $this->manager->removeShareRecipient($accessContext, $share, $secretRecipient); + $share = $this->manager->removeShareSource($accessContext, $share, new ShareSource(TestShareSourceType1::class, 'source1')); + $this->manager->deleteShare($accessContext, $share); + $this->dbConnection->commit(); + } finally { + $eventDispatcher->removeListener(CriticalActionPerformedEvent::class, $listener); + } + + $this->assertSame([ + 'unified_sharing.share.created', + 'unified_sharing.source.added', + 'unified_sharing.recipient.added', + 'unified_sharing.permission.updated', + 'unified_sharing.property.updated', + 'unified_sharing.share.state_updated', + 'unified_sharing.recipient.added', + 'unified_sharing.recipient_secret.updated', + 'unified_sharing.recipient_permission.updated', + 'unified_sharing.permission_preset.selected', + 'unified_sharing.share.user_status_updated', + 'unified_sharing.recipient.removed', + 'unified_sharing.source.removed', + 'unified_sharing.share.deleted', + ], array_map(static fn (CriticalActionPerformedEvent $event): ?string => $event->getOperation(), $auditEvents)); + + $this->assertSame([ + 'shareId' => $share->id, + 'recipientClass' => TestShareRecipientType1::class, + 'recipientValue' => 'recipient1', + 'instance' => 'local', + ], $auditEvents[2]->getParameters()); + $this->assertSame(['shareId' => $share->id, 'userStatus' => 'accepted', 'user' => 'user1'], $auditEvents[10]->getParameters()); + $this->assertSame(['shareId' => $share->id, 'owner' => 'owner'], $auditEvents[13]->getParameters()); + + foreach ($auditEvents as $event) { + $this->assertNotContains('mysecret', $event->getParameters(), 'The recipient secret must not be logged'); + } + } } From dc9896f54c66bdae61725746aba3267510e0bd2b Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Fri, 23 Oct 2026 11:09:46 +0200 Subject: [PATCH 3/3] feat(core): add operation identifiers to brute force and interaction audit entries The audit entries for brute force attempts from allow-listed addresses and for interaction checks had no operation identifier, so they could only be filtered by their message. They now use auth.bruteforce.bypassed, users.interaction.allowed and users.interaction.restricted. The interaction parameters are now named, so data.params is written as an object instead of a list. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ghassen kefi --- lib/private/Security/Bruteforce/Throttler.php | 2 + .../Interaction/RestrictInteractionEvent.php | 12 ++- .../RestrictInteractionEventTest.php | 10 ++- .../lib/Security/Bruteforce/ThrottlerTest.php | 81 +++++++++++++++++++ 4 files changed, 97 insertions(+), 8 deletions(-) create mode 100644 tests/lib/Security/Bruteforce/ThrottlerTest.php diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index e975399845aa4..4eb9373232200 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -65,6 +65,8 @@ public function registerAttempt(string $action, new CriticalActionPerformedEvent( 'Bruteforce attempt from "{ip}" detected for action "{action}" not throttled due to allow-list.', ['ip' => $ip, 'action' => $action], + false, + 'auth.bruteforce.bypassed', ) ); return; diff --git a/lib/public/Interaction/RestrictInteractionEvent.php b/lib/public/Interaction/RestrictInteractionEvent.php index 3fe4f0ab8bfee..db97d33167cb9 100644 --- a/lib/public/Interaction/RestrictInteractionEvent.php +++ b/lib/public/Interaction/RestrictInteractionEvent.php @@ -69,10 +69,10 @@ public function isInteractionRestricted(): false|string { $eventDispatcher = Server::get(IEventDispatcher::class); $params = [ - $this->action instanceof InteractionAction ? $this->action::class : '?', - $this->userId, - $this->resources === [] ? '?' : implode(', ', array_map(static fn (InteractionResource $resource): string => $resource->getID(), $this->resources)), - $this->receivers === [] ? '?' : implode(', ', array_map(static fn (InteractionReceiver $receiver): string => $receiver->getID() ?? $receiver::class, $this->receivers)), + 'action' => $this->action instanceof InteractionAction ? $this->action::class : '?', + 'userId' => $this->userId, + 'resources' => $this->resources === [] ? '?' : implode(', ', array_map(static fn (InteractionResource $resource): string => $resource->getID(), $this->resources)), + 'receivers' => $this->receivers === [] ? '?' : implode(', ', array_map(static fn (InteractionReceiver $receiver): string => $receiver->getID() ?? $receiver::class, $this->receivers)), ]; try { @@ -81,6 +81,8 @@ public function isInteractionRestricted(): false|string { $eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( 'Interaction %s from user %s on %s to %s is allowed.', $params, + false, + 'users.interaction.allowed', )); return false; @@ -88,6 +90,8 @@ public function isInteractionRestricted(): false|string { $eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent( 'Interaction %s from user %s on %s to %s is restricted: ' . $interactionRestrictedException->getMessage(), $params, + false, + 'users.interaction.restricted', )); return $interactionRestrictedException->getHint(); diff --git a/tests/lib/Interaction/RestrictInteractionEventTest.php b/tests/lib/Interaction/RestrictInteractionEventTest.php index 0b30db0566627..61edf969bd888 100644 --- a/tests/lib/Interaction/RestrictInteractionEventTest.php +++ b/tests/lib/Interaction/RestrictInteractionEventTest.php @@ -97,11 +97,13 @@ public function testIsInteractionRestricted(bool $isRestricted): void { ? 'Interaction %s from user %s on %s to %s is restricted: my restriction message' : 'Interaction %s from user %s on %s to %s is allowed.', [ - $action::class, - 'my-uid', - 'my-resource1, my-resource2', - 'my-receiver1, my-receiver2', + 'action' => $action::class, + 'userId' => 'my-uid', + 'resources' => 'my-resource1, my-resource2', + 'receivers' => 'my-receiver1, my-receiver2', ], + false, + $isRestricted ? 'users.interaction.restricted' : 'users.interaction.allowed', ), ], $auditEvents); diff --git a/tests/lib/Security/Bruteforce/ThrottlerTest.php b/tests/lib/Security/Bruteforce/ThrottlerTest.php new file mode 100644 index 0000000000000..a9411ed986014 --- /dev/null +++ b/tests/lib/Security/Bruteforce/ThrottlerTest.php @@ -0,0 +1,81 @@ +throttler = $this->createInstanceWithMocks(Throttler::class); + } + + public function testRegisterAttemptFromAllowListedIp(): void { + $this->getAutoMock(IConfig::class)->method('getSystemValueBool') + ->with('auth.bruteforce.protection.enabled', true) + ->willReturn(true); + $this->getAutoMock(BruteforceAllowList::class)->method('isBypassListed') + ->with('10.10.10.10') + ->willReturn(true); + + $this->getAutoMock(IEventDispatcher::class)->expects($this->once()) + ->method('dispatchTyped') + ->with(new CriticalActionPerformedEvent( + 'Bruteforce attempt from "{ip}" detected for action "{action}" not throttled due to allow-list.', + ['ip' => '10.10.10.10', 'action' => 'login'], + false, + 'auth.bruteforce.bypassed', + )); + $this->getAutoMock(IBackend::class)->expects($this->never()) + ->method('registerAttempt'); + + $this->throttler->registerAttempt('login', '10.10.10.10'); + } + + public function testRegisterAttempt(): void { + $this->getAutoMock(IConfig::class)->method('getSystemValueBool') + ->with('auth.bruteforce.protection.enabled', true) + ->willReturn(true); + $this->getAutoMock(BruteforceAllowList::class)->method('isBypassListed') + ->with('10.10.10.10') + ->willReturn(false); + + $this->getAutoMock(IEventDispatcher::class)->expects($this->never()) + ->method('dispatchTyped'); + $this->getAutoMock(IBackend::class)->expects($this->once()) + ->method('registerAttempt') + ->with('10.10.10.10', '10.10.10.10/32', $this->anything(), 'login', []); + + $this->throttler->registerAttempt('login', '10.10.10.10'); + } + + public function testRegisterAttemptWithProtectionDisabled(): void { + $this->getAutoMock(IConfig::class)->method('getSystemValueBool') + ->with('auth.bruteforce.protection.enabled', true) + ->willReturn(false); + + $this->getAutoMock(IEventDispatcher::class)->expects($this->never()) + ->method('dispatchTyped'); + $this->getAutoMock(IBackend::class)->expects($this->never()) + ->method('registerAttempt'); + + $this->throttler->registerAttempt('login', '10.10.10.10'); + } +}