From ff27e95196b440908882da89c0d80aac202f6b0e Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:40:48 +0200 Subject: [PATCH 1/4] feat(forms): log form lifecycle and access changes to the audit log Creating a form (empty or as a copy of another form, with the source form ID), deleting a form, transferring its ownership and the following form updates are now written to the admin audit log: - opening the form to or closing it for all users (permitAllUsers, showToAllUsers) - closing, archiving or reopening the form, with the new and previous state - changing the response settings isAnonymous, submitMultiple, allowEditSubmissions, expires or maxSubmissions; only the names of the changed settings are logged - linking the results to a spreadsheet file, with the file ID and format Updates are only logged when the value actually changed, after the form was saved. The title, description, submission message and confirmation email texts are never logged. Unlinking the results file and the short-lived editing lock are not logged. Form deletion is logged in the controller rather than in FormMapper::deleteForm(), which also runs from the cleanup job on user deletion, where there is no actor and admin_audit already logs the user deletion. The operations are collected in the new OCA\Forms\AuditLog\Operation enum, with a short description of each for the documentation. Entries are written through the new OCA\Forms\AuditLog\AuditLogger, which dispatches the public CriticalActionPerformedEvent. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/AuditLogger.php | 36 ++++ lib/AuditLog/Operation.php | 40 +++++ lib/Constants.php | 9 + lib/Controller/ApiController.php | 91 ++++++++++ tests/Unit/AuditLog/AuditEvents.php | 20 +++ tests/Unit/AuditLog/OperationTest.php | 25 +++ tests/Unit/Controller/ApiControllerTest.php | 181 ++++++++++++++++++++ 7 files changed, 402 insertions(+) create mode 100644 lib/AuditLog/AuditLogger.php create mode 100644 lib/AuditLog/Operation.php create mode 100644 tests/Unit/AuditLog/AuditEvents.php create mode 100644 tests/Unit/AuditLog/OperationTest.php diff --git a/lib/AuditLog/AuditLogger.php b/lib/AuditLog/AuditLogger.php new file mode 100644 index 000000000..8b95d8a48 --- /dev/null +++ b/lib/AuditLog/AuditLogger.php @@ -0,0 +1,36 @@ + $parameters named values for the placeholders of $message, in order, optionally followed by more context + */ + public function log(Operation $operation, string $message, array $parameters = []): void { + $arguments = [$message, $parameters, false]; + if (method_exists(CriticalActionPerformedEvent::class, 'getOperation')) { + $arguments[] = $operation->value; + } + $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent(...$arguments)); + } +} diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php new file mode 100644 index 000000000..a2c79c96b --- /dev/null +++ b/lib/AuditLog/Operation.php @@ -0,0 +1,40 @@ + 'A form was created, either empty or as a copy of another form', + self::FormDeleted => 'A form was deleted with its questions, shares, submissions and uploaded files', + self::FormOwnerTransferred => 'The ownership of a form was transferred to another user', + self::FormAccessUpdated => 'A form was opened to or closed for all users of the instance', + self::FormStateChanged => 'A form was closed, archived or reopened', + self::FormSettingsUpdated => 'The response settings of a form (anonymity, multiple or editable submissions, expiration, submission limit) were changed', + self::ResultsFileLinked => 'The results of a form were linked to a spreadsheet file, which is then kept in sync with the submissions', + }; + } +} diff --git a/lib/Constants.php b/lib/Constants.php index 3a8f2e5fa..21bed641a 100644 --- a/lib/Constants.php +++ b/lib/Constants.php @@ -71,6 +71,15 @@ class Constants { public const FORM_STATE_CLOSED = 1; public const FORM_STATE_ARCHIVED = 2; + /** + * Names of the form states, used in the audit log + */ + public const FORM_STATE_NAMES = [ + self::FORM_STATE_ACTIVE => 'active', + self::FORM_STATE_CLOSED => 'closed', + self::FORM_STATE_ARCHIVED => 'archived', + ]; + /** * Access flags of a form */ diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index db27cc45b..8df66fc48 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -7,6 +7,8 @@ namespace OCA\Forms\Controller; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\BackgroundJob\DeleteQuestionFoldersJob; use OCA\Forms\BackgroundJob\SyncSubmissionsWithLinkedFileJob; use OCA\Forms\Constants; @@ -92,6 +94,7 @@ public function __construct( private readonly IMimeTypeDetector $mimeTypeDetector, private readonly IJobList $jobList, private readonly ISecureRandom $secureRandom, + private readonly AuditLogger $auditLogger, ) { parent::__construct($appName, $request); $this->currentUser = $userSession->getUser(); @@ -178,6 +181,11 @@ public function newForm(?int $fromId = null): DataResponse { $form->setIsAnonymous(false); $this->formMapper->insert($form); + $this->auditLogger->log(Operation::FormCreated, 'Form %s was created by "%s"', [ + 'formId' => $form->getId(), + 'ownerId' => $form->getOwnerId(), + 'sourceFormId' => null, + ]); } else { $oldForm = $this->formsService->getFormIfAllowed($fromId, Constants::PERMISSION_EDIT); @@ -238,6 +246,11 @@ public function newForm(?int $fromId = null): DataResponse { } $this->formMapper->update($form); + $this->auditLogger->log(Operation::FormCreated, 'Form %s was created by "%s" as a copy of form %s', [ + 'formId' => $form->getId(), + 'ownerId' => $form->getOwnerId(), + 'sourceFormId' => $oldForm->getId(), + ]); } return new DataResponse($this->formsService->getForm($form), Http::STATUS_CREATED); @@ -323,11 +336,17 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { // Do not allow changing showToAllUsers or permitAllUsers if disabled $this->checkAccessUpdate($keyValuePairs); + $previousAccess = $form->getAccess(); + $previousState = $form->getState(); + $previousSettings = $this->getResponseSettings($form); + // Process file linking + $fileLinked = false; if (isset($keyValuePairs['path']) && isset($keyValuePairs['fileFormat'])) { $file = $this->submissionService->writeFileToCloud($form, $keyValuePairs['path'], $keyValuePairs['fileFormat']); $form->setFileId($file->getId()); $form->setFileFormat($keyValuePairs['fileFormat']); + $fileLinked = true; } // Process file unlinking @@ -365,6 +384,8 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse { // Update changed Columns in Db. $this->formMapper->update($form); + $this->logFormUpdate($form, $previousAccess, $previousState, $previousSettings, $fileLinked); + return new DataResponse($form->getId()); } @@ -389,6 +410,10 @@ public function deleteForm(int $formId): DataResponse { $form = $this->formsService->getFormIfAllowed($formId); $this->formMapper->deleteForm($form); + $this->auditLogger->log(Operation::FormDeleted, 'Form %s of "%s" was deleted', [ + 'formId' => $form->getId(), + 'ownerId' => $form->getOwnerId(), + ]); return new DataResponse($formId); } @@ -2019,13 +2044,79 @@ private function handleOwnerTransfer(Form $form, int $formId, string $currentUse $this->logger->debug('Could not find new form owner'); throw new OCSBadRequestException('Could not find new form owner'); } + $previousOwnerId = $form->getOwnerId(); $form->setOwnerId($keyValuePairs['ownerId']); $form->setLockedBy(null); $form->setLockedUntil(null); $this->formMapper->update($form); + $this->auditLogger->log(Operation::FormOwnerTransferred, 'Ownership of form %s was transferred from "%s" to "%s"', [ + 'formId' => $form->getId(), + 'oldOwnerId' => $previousOwnerId, + 'newOwnerId' => $form->getOwnerId(), + ]); return new DataResponse($form->getOwnerId()); } + /** + * Settings that decide how responses are collected and handled + * + * @return array + */ + private function getResponseSettings(Form $form): array { + return [ + 'isAnonymous' => $form->getIsAnonymous(), + 'submitMultiple' => $form->getSubmitMultiple(), + 'allowEditSubmissions' => $form->getAllowEditSubmissions(), + 'expires' => $form->getExpires(), + 'maxSubmissions' => $form->getMaxSubmissions(), + ]; + } + + /** + * Only the names of changed settings are logged, never texts like the title, + * description or confirmation email + * + * @param array $previousSettings + */ + private function logFormUpdate(Form $form, array $previousAccess, ?int $previousState, array $previousSettings, bool $fileLinked): void { + $access = $form->getAccess(); + if ($access !== $previousAccess) { + $this->auditLogger->log(Operation::FormAccessUpdated, 'Access to form %s was changed', [ + 'formId' => $form->getId(), + 'permitAllUsers' => $access['permitAllUsers'], + 'showToAllUsers' => $access['showToAllUsers'], + ]); + } + + if ($form->getState() !== $previousState) { + $this->auditLogger->log(Operation::FormStateChanged, 'State of form %s was changed to %s', [ + 'formId' => $form->getId(), + 'state' => Constants::FORM_STATE_NAMES[$form->getState()] ?? $form->getState(), + 'previousState' => Constants::FORM_STATE_NAMES[$previousState] ?? $previousState, + ]); + } + + $changedKeys = array_keys(array_filter( + $this->getResponseSettings($form), + static fn (mixed $value, string $key): bool => $value !== $previousSettings[$key], + ARRAY_FILTER_USE_BOTH, + )); + if ($changedKeys !== []) { + $this->auditLogger->log(Operation::FormSettingsUpdated, 'Response settings of form %s were changed', [ + 'formId' => $form->getId(), + 'changedKeys' => $changedKeys, + ]); + } + + if ($fileLinked) { + $this->auditLogger->log(Operation::ResultsFileLinked, 'Results of form %s were linked to file %s', [ + 'formId' => $form->getId(), + 'fileId' => $form->getFileId(), + 'fileFormat' => $form->getFileFormat(), + ]); + } + } + /** * Shift existing question orders to make room for an insertion at $position. * Normalizes the given $position to the valid range and updates all questions diff --git a/tests/Unit/AuditLog/AuditEvents.php b/tests/Unit/AuditLog/AuditEvents.php new file mode 100644 index 000000000..0a7faf12c --- /dev/null +++ b/tests/Unit/AuditLog/AuditEvents.php @@ -0,0 +1,20 @@ +getOperation() === $operation; + } +} diff --git a/tests/Unit/AuditLog/OperationTest.php b/tests/Unit/AuditLog/OperationTest.php new file mode 100644 index 000000000..1e0e474b8 --- /dev/null +++ b/tests/Unit/AuditLog/OperationTest.php @@ -0,0 +1,25 @@ + [$operation], Operation::cases()); + } + + #[DataProvider('dataOperations')] + public function testOperationFormat(Operation $operation): void { + $this->assertMatchesRegularExpression('/^forms\.[a-z][a-z0-9_]*\.[a-z][a-z0-9_]*$/', $operation->value); + $this->assertNotSame('', $operation->description()); + } +} diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index c3c0bbde1..c9525ac47 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -29,6 +29,8 @@ function is_uploaded_file(string|bool|null $filename) { namespace OCA\Forms\Tests\Unit\Controller; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\BackgroundJob\SyncSubmissionsWithLinkedFileJob; use OCA\Forms\Constants; use OCA\Forms\Controller\ApiController; @@ -47,6 +49,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCA\Forms\Service\ConfirmationEmailService; use OCA\Forms\Service\FormsService; use OCA\Forms\Service\SubmissionService; +use OCA\Forms\Tests\Unit\AuditLog\AuditEvents; use OCA\Forms\Tests\Unit\UserFolderMockTrait; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; @@ -56,6 +59,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\BackgroundJob\IJobList; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IMimeTypeDetector; @@ -66,6 +70,7 @@ function is_uploaded_file(string|bool|null $filename) { use OCP\IUser; use OCP\IUserManager; use OCP\IUserSession; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ISecureRandom; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -93,6 +98,9 @@ class ApiControllerTest extends TestCase { private IMimeTypeDetector|MockObject $mimeTypeDetector; private IJobList|MockObject $jobList; private ISecureRandom|MockObject $secureRandom; + private IEventDispatcher|MockObject $eventDispatcher; + /** @var list */ + private array $dispatchedEvents = []; public function setUp(): void { $this->answerMapper = $this->createMock(AnswerMapper::class); @@ -117,6 +125,12 @@ public function setUp(): void { $this->jobList = $this->createMock(IJobList::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->secureRandom->method('generate')->willReturn('abcdefghijklmnopqrstuvwxyz012345'); + $this->dispatchedEvents = []; + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (object $event): void { + $this->dispatchedEvents[] = $event; + }); $this->apiController = new ApiController( 'forms', @@ -139,9 +153,28 @@ public function setUp(): void { $this->mimeTypeDetector, $this->jobList, $this->secureRandom, + new AuditLogger($this->eventDispatcher), ); } + private function expectNoAuditLog(): void { + $this->eventDispatcher->expects($this->never())->method('dispatchTyped'); + } + + /** + * @param list}> $expected operation, message and parameters of each audit log entry, in order + */ + private function assertAuditLogged(array $expected): void { + $events = array_values(array_filter($this->dispatchedEvents, static fn (object $event): bool => $event instanceof CriticalActionPerformedEvent)); + $this->assertSame( + array_map(static fn (array $entry): array => [$entry[1], $entry[2]], $expected), + array_map(static fn (CriticalActionPerformedEvent $event): array => [$event->getLogMessage(), $event->getParameters()], $events), + ); + foreach ($events as $i => $event) { + $this->assertTrue(AuditEvents::hasOperation($event, $expected[$i][0]->value)); + } + } + /** * Helper factory to prevent duplicated code */ @@ -556,6 +589,9 @@ public function testCreateNewForm($expectedForm) { return $form; }); $this->assertEquals(new DataResponse([], Http::STATUS_CREATED), $this->apiController->newForm()); + $this->assertAuditLogged([ + [Operation::FormCreated, 'Form %s was created by "%s"', ['formId' => 7, 'ownerId' => 'currentUser', 'sourceFormId' => null]], + ]); } public static function dataCloneForm_exceptions() { @@ -589,6 +625,7 @@ public function testCloneForm_exceptions(bool $canCreate, $callback, string $exc ->method('getFormIfAllowed') ->with(7, Constants::PERMISSION_EDIT) ->willReturnCallback($callback); + $this->expectNoAuditLog(); $this->expectException($exception); $this->apiController->newForm(7); } @@ -667,6 +704,9 @@ public function testCloneForm($old, $new) { ->willReturn([]); $this->assertEquals(new DataResponse([], Http::STATUS_CREATED), $this->apiController->newForm(7)); + $this->assertAuditLogged([ + [Operation::FormCreated, 'Form %s was created by "%s" as a copy of form %s', ['formId' => 14, 'ownerId' => 'currentUser', 'sourceFormId' => 7]], + ]); } private function formAccess(bool $hasUserAccess = true, bool $hasFormExpired = false, bool $canSubmit = true) { @@ -1202,6 +1242,7 @@ public function testUpdateFormAllowsValidConfirmationEmailQuestionId(): void { $this->assertEquals(new DataResponse(1), $this->apiController->updateForm(1, ['confirmationEmailQuestionId' => 7])); $this->assertSame(7, $form->getConfirmationEmailQuestionId()); + $this->assertAuditLogged([]); } public static function dataUpdateFormRejectsForbiddenKeys(): array { @@ -1231,6 +1272,7 @@ public function testUpdateFormRejectsForbiddenKeys(array $keyValuePairs): void { ->willReturn($form); $this->formMapper->expects($this->never())->method('update'); + $this->expectNoAuditLog(); $this->expectException(OCSForbiddenException::class); $this->apiController->updateForm(1, $keyValuePairs); @@ -1403,6 +1445,7 @@ public function testTransferOwnerNotOwner() { ->with(1, Constants::PERMISSION_EDIT) ->willReturn($form); + $this->expectNoAuditLog(); $this->expectException(OCSForbiddenException::class); $this->apiController->updateForm(1, ['ownerId' => 'newOwner']); } @@ -1428,6 +1471,7 @@ public function testTransferNewOwnerNotFound() { ->with('newOwner') ->willReturn(null); + $this->expectNoAuditLog(); $this->expectException(OCSBadRequestException::class); $this->apiController->updateForm(1, ['ownerId' => 'newOwner']); } @@ -1456,6 +1500,143 @@ public function testTransferOwner() { $this->assertEquals(new DataResponse('newOwner'), $this->apiController->updateForm(1, ['ownerId' => 'newOwner'])); $this->assertEquals('newOwner', $form->getOwnerId()); + $this->assertAuditLogged([ + [Operation::FormOwnerTransferred, 'Ownership of form %s was transferred from "%s" to "%s"', ['formId' => 1, 'oldOwnerId' => 'currentUser', 'newOwnerId' => 'newOwner']], + ]); + } + + public function testDeleteForm(): void { + $form = new Form(); + $form->setId(1); + $form->setOwnerId('formOwner'); + + $this->formsService->expects($this->once()) + ->method('getFormIfAllowed') + ->with(1) + ->willReturn($form); + $this->formMapper->expects($this->once()) + ->method('deleteForm') + ->with($form); + + $this->assertEquals(new DataResponse(1), $this->apiController->deleteForm(1)); + $this->assertAuditLogged([ + [Operation::FormDeleted, 'Form %s of "%s" was deleted', ['formId' => 1, 'ownerId' => 'formOwner']], + ]); + } + + public function testDeleteFormNotAllowed(): void { + $this->formsService->expects($this->once()) + ->method('getFormIfAllowed') + ->with(1) + ->willThrowException(new NoSuchFormException('This form is not owned by the current user', Http::STATUS_FORBIDDEN)); + $this->formMapper->expects($this->never()) + ->method('deleteForm'); + $this->expectNoAuditLog(); + + $this->expectException(NoSuchFormException::class); + $this->apiController->deleteForm(1); + } + + private function createUpdatableForm(): Form { + $form = new Form(); + $form->setId(1); + $form->setHash('hash'); + $form->setOwnerId('currentUser'); + $form->setState(Constants::FORM_STATE_ACTIVE); + $form->setAccess(['permitAllUsers' => false, 'showToAllUsers' => false]); + $form->setIsAnonymous(false); + $form->setSubmitMultiple(false); + $form->setAllowEditSubmissions(false); + $form->setExpires(0); + + $this->formsService + ->method('getFormIfAllowed') + ->with(1, Constants::PERMISSION_EDIT) + ->willReturn($form); + + return $form; + } + + public function testUpdateFormLogsAccessChange(): void { + $this->createUpdatableForm(); + $this->configService->method('getAllowPermitAll')->willReturn(true); + + $this->apiController->updateForm(1, ['access' => ['permitAllUsers' => true, 'showToAllUsers' => false]]); + + $this->assertAuditLogged([ + [Operation::FormAccessUpdated, 'Access to form %s was changed', ['formId' => 1, 'permitAllUsers' => true, 'showToAllUsers' => false]], + ]); + } + + public function testUpdateFormDoesNotLogUnchangedAccess(): void { + $this->createUpdatableForm(); + + $this->apiController->updateForm(1, ['access' => ['permitAllUsers' => false, 'showToAllUsers' => false]]); + + $this->assertAuditLogged([]); + } + + public function testUpdateFormNotAllowedAccessChangeIsNotLogged(): void { + $this->createUpdatableForm(); + $this->configService->method('getAllowShowToAll')->willReturn(false); + $this->formMapper->expects($this->never())->method('update'); + $this->expectNoAuditLog(); + + $this->expectException(OCSForbiddenException::class); + $this->apiController->updateForm(1, ['access' => ['permitAllUsers' => true, 'showToAllUsers' => true]]); + } + + public function testUpdateFormLogsStateChange(): void { + $this->createUpdatableForm(); + $this->formsService->method('isFormArchived')->willReturn(false); + + $this->apiController->updateForm(1, ['state' => Constants::FORM_STATE_ARCHIVED]); + + $this->assertAuditLogged([ + [Operation::FormStateChanged, 'State of form %s was changed to %s', ['formId' => 1, 'state' => 'archived', 'previousState' => 'active']], + ]); + } + + public function testUpdateFormLogsOnlyNamesOfChangedResponseSettings(): void { + $this->createUpdatableForm(); + + $this->apiController->updateForm(1, [ + 'title' => 'New title', + 'isAnonymous' => true, + 'expires' => 0, + 'maxSubmissions' => 5, + ]); + + $this->assertAuditLogged([ + [Operation::FormSettingsUpdated, 'Response settings of form %s were changed', ['formId' => 1, 'changedKeys' => ['isAnonymous', 'maxSubmissions']]], + ]); + } + + public function testUpdateFormLogsLinkedResultsFile(): void { + $form = $this->createUpdatableForm(); + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(42); + $this->submissionService->expects($this->once()) + ->method('writeFileToCloud') + ->with($form, '/Results', 'csv') + ->willReturn($file); + + $this->apiController->updateForm(1, ['path' => '/Results', 'fileFormat' => 'csv']); + + $this->assertAuditLogged([ + [Operation::ResultsFileLinked, 'Results of form %s were linked to file %s', ['formId' => 1, 'fileId' => 42, 'fileFormat' => 'csv']], + ]); + } + + public function testUpdateFormDoesNotLogUnlinkedResultsFile(): void { + $form = $this->createUpdatableForm(); + $form->setFileId(42); + $form->setFileFormat('csv'); + + $this->apiController->updateForm(1, ['fileId' => null, 'fileFormat' => null]); + + $this->assertNull($form->getFileId()); + $this->assertAuditLogged([]); } public function testGetSubmission_invalidForm() { From 7734e8ca16493020ea050ad3364fa76539ee3408 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:43:29 +0200 Subject: [PATCH 2/4] feat(sharing): log Forms shares to the audit log Creating a share of a form (with a user, group or team or by public link), changing the permissions or the custom token of a share and deleting a share are now written to the admin audit log, with the share ID, form ID, share type, recipient and permissions. Forms shares are stored in the Forms share table, not as core shares, so admin_audit did not see them. Deleting a share through the share review is logged with its own message, after the share was deleted. The token of a link share is stored as its recipient and is never logged; a share update only logs whether the token changed. The recipient of user, group and team shares is also logged on updates and deletions, so that a revoked grant can be attributed. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/AuditLogger.php | 18 ++++ lib/AuditLog/Operation.php | 6 ++ lib/Controller/ShareApiController.php | 9 ++ lib/ShareReview/ShareReviewSource.php | 7 +- tests/Unit/AuditLog/AuditLoggerTest.php | 86 +++++++++++++++++++ .../Controller/ShareApiControllerTest.php | 85 ++++++++++++++++++ .../ShareReview/ShareReviewSourceTest.php | 23 +++++ 7 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/AuditLog/AuditLoggerTest.php diff --git a/lib/AuditLog/AuditLogger.php b/lib/AuditLog/AuditLogger.php index 8b95d8a48..a540305b1 100644 --- a/lib/AuditLog/AuditLogger.php +++ b/lib/AuditLog/AuditLogger.php @@ -8,8 +8,10 @@ namespace OCA\Forms\AuditLog; +use OCA\Forms\Db\Share; use OCP\EventDispatcher\IEventDispatcher; use OCP\Log\Audit\CriticalActionPerformedEvent; +use OCP\Share\IShare; /** * Writes entries to the admin audit log @@ -33,4 +35,20 @@ public function log(Operation $operation, string $message, array $parameters = [ } $this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent(...$arguments)); } + + /** + * Logs a share with its share ID and form ID as the first parameters. + * The token of a link share is stored as its shareWith and is never logged. + * + * @param array $extraParameters + */ + public function logShare(Operation $operation, string $message, Share $share, array $extraParameters = []): void { + $this->log($operation, $message, [ + 'shareId' => $share->getId(), + 'formId' => $share->getFormId(), + 'shareType' => $share->getShareType(), + 'shareWith' => $share->getShareType() === IShare::TYPE_LINK ? null : $share->getShareWith(), + 'permissions' => $share->getPermissions(), + ] + $extraParameters); + } } diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index a2c79c96b..fd57714b1 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -22,6 +22,9 @@ enum Operation: string { case FormStateChanged = 'forms.form_state.changed'; case FormSettingsUpdated = 'forms.form_settings.updated'; case ResultsFileLinked = 'forms.results_file.linked'; + case ShareCreated = 'forms.share.created'; + case ShareUpdated = 'forms.share.updated'; + case ShareDeleted = 'forms.share.deleted'; /** * Short description of when the operation is logged, used for the documentation @@ -35,6 +38,9 @@ public function description(): string { self::FormStateChanged => 'A form was closed, archived or reopened', self::FormSettingsUpdated => 'The response settings of a form (anonymity, multiple or editable submissions, expiration, submission limit) were changed', self::ResultsFileLinked => 'The results of a form were linked to a spreadsheet file, which is then kept in sync with the submissions', + self::ShareCreated => 'A form was shared with a user, group or team or by public link, with its permissions', + self::ShareUpdated => 'The permissions or the custom public link token of a form share were changed', + self::ShareDeleted => 'A form share was deleted, by someone managing the form or through the share review', }; } } diff --git a/lib/Controller/ShareApiController.php b/lib/Controller/ShareApiController.php index fde50da43..b1771c3fa 100644 --- a/lib/Controller/ShareApiController.php +++ b/lib/Controller/ShareApiController.php @@ -9,6 +9,8 @@ namespace OCA\Forms\Controller; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Constants; use OCA\Forms\Db\Form; use OCA\Forms\Db\FormMapper; @@ -64,6 +66,7 @@ public function __construct( private readonly FilePathHelper $filePathHelper, private readonly IManager $shareManager, private readonly UploadedFilesShareService $uploadedFilesShareService, + private readonly AuditLogger $auditLogger, ) { parent::__construct($appName, $request); } @@ -194,6 +197,7 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar /** @var Share */ $share = $this->shareMapper->insert($share); $this->formMapper->update($form); + $this->auditLogger->logShare(Operation::ShareCreated, 'Share %s of form %s was created', $share); // Create share-notifications (activity) $this->formsService->notifyNewShares($form, $share); @@ -270,6 +274,7 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da throw new OCSBadRequestException('Invalid permission given'); } + $previousShareWith = $formShare->getShareWith(); if (array_key_exists('token', $keyValuePairs)) { if (!$this->configService->getAllowCustomPublicToken()) { $this->logger->debug('Custom public share tokens are not allowed.'); @@ -341,6 +346,9 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da } $this->formMapper->update($form); + $this->auditLogger->logShare(Operation::ShareUpdated, 'Share %s of form %s was updated', $formShare, [ + 'tokenChanged' => $formShare->getShareType() === IShare::TYPE_LINK && $formShare->getShareWith() !== $previousShareWith, + ]); return new DataResponse($formShare->getId()); } @@ -393,6 +401,7 @@ public function deleteShare(int $formId, int $shareId): DataResponse { $this->shareMapper->delete($share); $this->formMapper->update($form); + $this->auditLogger->logShare(Operation::ShareDeleted, 'Share %s of form %s was deleted', $share); return new DataResponse($shareId); } diff --git a/lib/ShareReview/ShareReviewSource.php b/lib/ShareReview/ShareReviewSource.php index 40b1b8b4f..d4c9b91b7 100644 --- a/lib/ShareReview/ShareReviewSource.php +++ b/lib/ShareReview/ShareReviewSource.php @@ -9,6 +9,8 @@ namespace OCA\Forms\ShareReview; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Constants; use OCA\Forms\Db\FormMapper; use OCA\Forms\Db\ShareMapper; @@ -43,6 +45,7 @@ public function __construct( private readonly IEventDispatcher $eventDispatcher, private readonly LoggerInterface $logger, private readonly IL10N $l10n, + private readonly AuditLogger $auditLogger, ) { } @@ -96,11 +99,13 @@ public function deleteShare(string $shareId): bool { $this->shareMapper->delete($share); // Bump the form's last_updated timestamp, matching the regular deletion flow $this->formMapper->update($form); - return true; } catch (\Exception $e) { $this->logger->error('Forms ShareReview: failed to delete share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]); return false; } + + $this->auditLogger->logShare(Operation::ShareDeleted, 'Share %s of form %s was deleted through the share review', $share); + return true; } /** @param array $share */ diff --git a/tests/Unit/AuditLog/AuditLoggerTest.php b/tests/Unit/AuditLog/AuditLoggerTest.php new file mode 100644 index 000000000..00b5c39a6 --- /dev/null +++ b/tests/Unit/AuditLog/AuditLoggerTest.php @@ -0,0 +1,86 @@ +eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->auditLogger = new AuditLogger($this->eventDispatcher); + } + + private function expectAuditEvent(string $operation, string $message, array $parameters): void { + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function (CriticalActionPerformedEvent $event) use ($operation, $message, $parameters): bool { + $this->assertTrue(AuditEvents::hasOperation($event, $operation)); + $this->assertSame($message, $event->getLogMessage()); + $this->assertSame($parameters, $event->getParameters()); + $this->assertFalse($event->getObfuscateParameters()); + return true; + })); + } + + public function testLog(): void { + $this->expectAuditEvent('forms.form.deleted', 'Form %s of "%s" was deleted', ['formId' => 1, 'ownerId' => 'alice']); + + $this->auditLogger->log(Operation::FormDeleted, 'Form %s of "%s" was deleted', ['formId' => 1, 'ownerId' => 'alice']); + } + + public function testLogShare(): void { + $share = new Share(); + $share->setId(8); + $share->setFormId(5); + $share->setShareType(IShare::TYPE_GROUP); + $share->setShareWith('staff'); + $share->setPermissions(['submit', 'results']); + + $this->expectAuditEvent('forms.share.updated', 'Share %s of form %s was updated', [ + 'shareId' => 8, + 'formId' => 5, + 'shareType' => IShare::TYPE_GROUP, + 'shareWith' => 'staff', + 'permissions' => ['submit', 'results'], + 'tokenChanged' => false, + ]); + + $this->auditLogger->logShare(Operation::ShareUpdated, 'Share %s of form %s was updated', $share, ['tokenChanged' => false]); + } + + public function testLogShareNeverLogsLinkShareToken(): void { + $share = new Share(); + $share->setId(9); + $share->setFormId(5); + $share->setShareType(IShare::TYPE_LINK); + $share->setShareWith('secretToken123'); + $share->setPermissions(['submit']); + + $this->expectAuditEvent('forms.share.created', 'Share %s of form %s was created', [ + 'shareId' => 9, + 'formId' => 5, + 'shareType' => IShare::TYPE_LINK, + 'shareWith' => null, + 'permissions' => ['submit'], + ]); + + $this->auditLogger->logShare(Operation::ShareCreated, 'Share %s of form %s was created', $share); + } +} diff --git a/tests/Unit/Controller/ShareApiControllerTest.php b/tests/Unit/Controller/ShareApiControllerTest.php index 42099d651..636bb3bd7 100644 --- a/tests/Unit/Controller/ShareApiControllerTest.php +++ b/tests/Unit/Controller/ShareApiControllerTest.php @@ -9,6 +9,8 @@ namespace OCA\Forms\Tests\Unit\Controller; use OCA\Circles\Model\Circle; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Constants; use OCA\Forms\Controller\ShareApiController; use OCA\Forms\Db\Form; @@ -21,6 +23,7 @@ use OCA\Forms\Service\ConfigService; use OCA\Forms\Service\FormsService; use OCA\Forms\Service\UploadedFilesShareService; +use OCA\Forms\Tests\Unit\AuditLog\AuditEvents; use OCA\Forms\Tests\Unit\UserFolderMockTrait; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\IMapperException; @@ -29,6 +32,7 @@ use OCP\AppFramework\OCS\OCSBadRequestException; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCS\OCSNotFoundException; +use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IFilenameValidator; @@ -38,6 +42,7 @@ use OCP\IRequest; use OCP\IUser; use OCP\IUserManager; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Security\ISecureRandom; use OCP\Share\IManager; use OCP\Share\IShare; @@ -68,6 +73,9 @@ class ShareApiControllerTest extends TestCase { private IFilenameValidator|MockObject $filenameValidator; private IManager|MockObject $shareManager; private UploadedFilesShareService $uploadedFilesShareService; + private IEventDispatcher|MockObject $eventDispatcher; + /** @var list */ + private array $dispatchedEvents = []; public function setUp(): void { $this->formMapper = $this->createMock(FormMapper::class); @@ -95,6 +103,13 @@ public function setUp(): void { $this->logger, ); + $this->dispatchedEvents = []; + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (object $event): void { + $this->dispatchedEvents[] = $event; + }); + $this->shareApiController = new ShareApiController( 'forms', $this->request, @@ -111,9 +126,28 @@ public function setUp(): void { $this->filePathHelper, $this->shareManager, $this->uploadedFilesShareService, + new AuditLogger($this->eventDispatcher), ); } + private function expectNoAuditLog(): void { + $this->eventDispatcher->expects($this->never())->method('dispatchTyped'); + } + + /** + * @param list}> $expected operation, message and parameters of each audit log entry, in order + */ + private function assertAuditLogged(array $expected): void { + $events = array_values(array_filter($this->dispatchedEvents, static fn (object $event): bool => $event instanceof CriticalActionPerformedEvent)); + $this->assertSame( + array_map(static fn (array $entry): array => [$entry[1], $entry[2]], $expected), + array_map(static fn (CriticalActionPerformedEvent $event): array => [$event->getLogMessage(), $event->getParameters()], $events), + ); + foreach ($events as $i => $event) { + $this->assertTrue(AuditEvents::hasOperation($event, $expected[$i][0]->value)); + } + } + public static function dataValidNewShare() { return [ 'newUserShare' => [ @@ -213,6 +247,15 @@ public function testValidNewShare(int $shareType, string $shareWith, array $perm // Share Form '5' to 'user1' of share-type 'user=0' $expectedResponse = new DataResponse($expected, Http::STATUS_CREATED); $this->assertEquals($expectedResponse, $this->shareApiController->newShare(5, $shareType, $shareWith, $permissions)); + $this->assertAuditLogged([ + [Operation::ShareCreated, 'Share %s of form %s was created', [ + 'shareId' => 13, + 'formId' => 5, + 'shareType' => $shareType, + 'shareWith' => $shareWith, + 'permissions' => $permissions, + ]], + ]); } public static function dataNewLinkShare() { @@ -296,7 +339,17 @@ public function testNewLinkShare(int $shareType, string $shareWith, array $permi // Share the form. $expectedResponse = new DataResponse($expected, Http::STATUS_CREATED); $this->assertEquals($expectedResponse, $this->shareApiController->newShare(5, $shareType, $shareWith, $permissions)); + $this->assertAuditLogged([ + [Operation::ShareCreated, 'Share %s of form %s was created', [ + 'shareId' => 13, + 'formId' => 5, + 'shareType' => IShare::TYPE_LINK, + 'shareWith' => null, + 'permissions' => $permissions, + ]], + ]); } else { + $this->expectNoAuditLog(); $this->expectException($exception); $this->shareApiController->newShare(5, $shareType, $shareWith, $permissions); } @@ -519,6 +572,15 @@ public function testDeleteShare() { $response = new DataResponse(8); $this->assertEquals($response, $this->shareApiController->deleteShare(5, 8)); + $this->assertAuditLogged([ + [Operation::ShareDeleted, 'Share %s of form %s was deleted', [ + 'shareId' => 8, + 'formId' => 5, + 'shareType' => null, + 'shareWith' => null, + 'permissions' => [Constants::PERMISSION_SUBMIT], + ]], + ]); } /** @@ -644,6 +706,7 @@ public function testDeleteForeignShare() { ->with('5') ->willThrowException(new NoSuchFormException('This form is not owned by the current user')); + $this->expectNoAuditLog(); $this->expectException(NoSuchFormException::class); $this->shareApiController->deleteShare(5, 8); } @@ -887,7 +950,18 @@ public function testUpdateShare(array $share, string $formOwner, array $keyValue if ($exception === null) { $expectedResponse = new DataResponse($expected); $this->assertEquals($expectedResponse, $this->shareApiController->updateShare($share['formId'], $share['id'], $keyValuePairs)); + $this->assertAuditLogged([ + [Operation::ShareUpdated, 'Share %s of form %s was updated', [ + 'shareId' => $share['id'], + 'formId' => $share['formId'], + 'shareType' => $share['shareType'], + 'shareWith' => $share['shareType'] === IShare::TYPE_LINK ? null : $share['shareWith'], + 'permissions' => $keyValuePairs['permissions'], + 'tokenChanged' => false, + ]], + ]); } else { + $this->expectNoAuditLog(); $this->expectException($exception); $this->shareApiController->updateShare($share['formId'], $share['id'], $keyValuePairs); } @@ -970,6 +1044,16 @@ public function testUpdateShareToken() { }); $this->assertEquals(new DataResponse(8), $this->shareApiController->updateShare(5, 8, ['token' => 'tokenabcd'])); + $this->assertAuditLogged([ + [Operation::ShareUpdated, 'Share %s of form %s was updated', [ + 'shareId' => 8, + 'formId' => 5, + 'shareType' => IShare::TYPE_LINK, + 'shareWith' => null, + 'permissions' => [Constants::PERMISSION_SUBMIT], + 'tokenChanged' => true, + ]], + ]); } public function testUpdateShareToken_CustomTokensDisabled() { @@ -1197,6 +1281,7 @@ public function testUpdateShareToken_SameTokenReturnsEarly() { ->method('update'); $this->formsService->expects($this->never()) ->method('obtainFormLock'); + $this->expectNoAuditLog(); $this->assertEquals(new DataResponse(8), $this->shareApiController->updateShare(5, 8, ['token' => 'sameToken123'])); } diff --git a/tests/Unit/ShareReview/ShareReviewSourceTest.php b/tests/Unit/ShareReview/ShareReviewSourceTest.php index f262b1d2d..2189a57fd 100644 --- a/tests/Unit/ShareReview/ShareReviewSourceTest.php +++ b/tests/Unit/ShareReview/ShareReviewSourceTest.php @@ -9,16 +9,20 @@ namespace OCA\Forms\Tests\Unit\ShareReview; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Db\Form; use OCA\Forms\Db\FormMapper; use OCA\Forms\Db\Share; use OCA\Forms\Db\ShareMapper; use OCA\Forms\Service\UploadedFilesShareService; use OCA\Forms\ShareReview\ShareReviewSource; +use OCA\Forms\Tests\Unit\AuditLog\AuditEvents; use OCP\AppFramework\Db\DoesNotExistException; use OCP\DB\Exception; use OCP\EventDispatcher\IEventDispatcher; use OCP\IL10N; +use OCP\Log\Audit\CriticalActionPerformedEvent; use OCP\Share\IShare; use OCP\Share\ShareReview\Events\ShareReviewAccessCheckEvent; use OCP\Share\ShareReview\ShareReviewEntry; @@ -34,6 +38,7 @@ final class ShareReviewSourceTest extends TestCase { private IEventDispatcher|MockObject $eventDispatcher; private LoggerInterface|MockObject $logger; private IL10N|MockObject $l10n; + private IEventDispatcher|MockObject $auditEventDispatcher; private ShareReviewSource $source; protected function setUp(): void { @@ -43,6 +48,7 @@ protected function setUp(): void { $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->logger = $this->createMock(LoggerInterface::class); $this->l10n = $this->createMock(IL10N::class); + $this->auditEventDispatcher = $this->createMock(IEventDispatcher::class); $this->l10n->method('t')->willReturnCallback( fn (string $text, array $params = []): string => empty($params) ? $text : vsprintf($text, $params) ); @@ -53,6 +59,7 @@ protected function setUp(): void { $this->eventDispatcher, $this->logger, $this->l10n, + new AuditLogger($this->auditEventDispatcher), ); } @@ -339,6 +346,7 @@ public function testDeleteShareEventDeniedReturnsFalse(): void { $event->denyAccess('not in group'); }); $this->shareMapper->expects($this->never())->method('delete'); + $this->auditEventDispatcher->expects($this->never())->method('dispatchTyped'); $this->assertFalse($this->source->deleteShare('7')); } @@ -369,6 +377,20 @@ public function testDeleteShareGrantedDeletesShareAndBumpsForm(): void { $this->formMapper->expects($this->once())->method('update')->with($form); // No results permission — the uploaded-files share must not be touched $this->uploadedFilesShareService->expects($this->never())->method('removeForCollaborator'); + $this->auditEventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function (CriticalActionPerformedEvent $event): bool { + $this->assertTrue(AuditEvents::hasOperation($event, Operation::ShareDeleted->value)); + $this->assertSame('Share %s of form %s was deleted through the share review', $event->getLogMessage()); + $this->assertSame([ + 'shareId' => 7, + 'formId' => 10, + 'shareType' => IShare::TYPE_USER, + 'shareWith' => 'bob', + 'permissions' => ['submit'], + ], $event->getParameters()); + return true; + })); $this->assertTrue($this->source->deleteShare('7')); } @@ -401,6 +423,7 @@ public function testDeleteShareDbErrorReturnsFalse(): void { $this->formMapper->method('findById')->willReturn($form); $this->shareMapper->method('delete')->willThrowException($this->createMock(Exception::class)); $this->logger->expects($this->once())->method('error'); + $this->auditEventDispatcher->expects($this->never())->method('dispatchTyped'); $this->assertFalse($this->source->deleteShare('7')); } From 583c6ab55c094757d47aac02f43c09d22f103c88 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:44:38 +0200 Subject: [PATCH 3/4] feat(results): log results exports and submission changes to the audit log Downloading all results as a CSV, ODS or XLSX file, exporting them to a file in the cloud, deleting all submissions of a form, and deleting or updating a single submission are now written to the admin audit log. The results download is not a file access, so admin_audit did not see it. Submission entries carry the submission ID, the form ID and the user who submitted it, which is often not the user doing the change. The answers are never logged. The cloud export is logged in the controller, not in SubmissionService::writeFileToCloud(), which also runs from the background job keeping a linked results file in sync. The export entry also carries the file ID besides the file name and format. New submissions and uploaded answer files are not logged: they are frequent, often anonymous, and the file writes are already logged by admin_audit. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 10 ++++ lib/Controller/ApiController.php | 24 +++++++++ tests/Unit/Controller/ApiControllerTest.php | 58 +++++++++++++++++++++ 3 files changed, 92 insertions(+) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index fd57714b1..ed55d86a0 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -25,6 +25,11 @@ enum Operation: string { case ShareCreated = 'forms.share.created'; case ShareUpdated = 'forms.share.updated'; case ShareDeleted = 'forms.share.deleted'; + case ResultsDownloaded = 'forms.results.downloaded'; + case ResultsExported = 'forms.results.exported'; + case SubmissionUpdated = 'forms.submission.updated'; + case SubmissionDeleted = 'forms.submission.deleted'; + case SubmissionsDeletedAll = 'forms.submissions.deleted_all'; /** * Short description of when the operation is logged, used for the documentation @@ -41,6 +46,11 @@ public function description(): string { self::ShareCreated => 'A form was shared with a user, group or team or by public link, with its permissions', self::ShareUpdated => 'The permissions or the custom public link token of a form share were changed', self::ShareDeleted => 'A form share was deleted, by someone managing the form or through the share review', + self::ResultsDownloaded => 'All submissions of a form were downloaded as a CSV, ODS or XLSX file', + self::ResultsExported => 'All submissions of a form were exported to a file in the cloud', + self::SubmissionUpdated => 'The answers of a submission were replaced', + self::SubmissionDeleted => 'A submission was deleted', + self::SubmissionsDeletedAll => 'All submissions of a form were deleted', }; } } diff --git a/lib/Controller/ApiController.php b/lib/Controller/ApiController.php index 8df66fc48..85485d2b1 100644 --- a/lib/Controller/ApiController.php +++ b/lib/Controller/ApiController.php @@ -1264,6 +1264,10 @@ public function getSubmissions(int $formId, ?string $query = null, ?int $limit = $submissionsData = $this->submissionService->getSubmissionsData($form, $fileFormat); $fileName = $this->formsService->getFileName($form, $fileFormat); + $this->auditLogger->log(Operation::ResultsDownloaded, 'Results of form %s were downloaded as %s', [ + 'formId' => $form->getId(), + 'fileFormat' => $fileFormat, + ]); return new DataDownloadResponse($submissionsData, $fileName, Constants::SUPPORTED_EXPORT_FORMATS[$fileFormat]); } @@ -1402,6 +1406,9 @@ public function deleteAllSubmissions(int $formId): DataResponse { // Delete all submissions (incl. Answers) $this->submissionMapper->deleteByForm($formId); $this->formMapper->update($form); + $this->auditLogger->log(Operation::SubmissionsDeletedAll, 'All submissions of form %s were deleted', [ + 'formId' => $form->getId(), + ]); $this->queueLinkedFileSync($form); @@ -1570,6 +1577,12 @@ public function updateSubmission(int $formId, int $submissionId, array $answers) $this->storeAnswersForQuestion($form, $submission->getId(), $question, $answerArray); } + $this->auditLogger->log(Operation::SubmissionUpdated, 'Submission %s of form %s by "%s" was updated', [ + 'submissionId' => $submissionId, + 'formId' => $form->getId(), + 'submissionUserId' => $submission->getUserId(), + ]); + //Create Activity $this->formsService->notifyNewSubmission($form, $submission); @@ -1622,6 +1635,11 @@ public function deleteSubmission(int $formId, int $submissionId): DataResponse { // Delete submission (incl. Answers) $this->submissionMapper->deleteById($form, $submissionId); $this->formMapper->update($form); + $this->auditLogger->log(Operation::SubmissionDeleted, 'Submission %s of form %s by "%s" was deleted', [ + 'submissionId' => $submissionId, + 'formId' => $form->getId(), + 'submissionUserId' => $submission->getUserId(), + ]); $this->queueLinkedFileSync($form); @@ -1655,6 +1673,12 @@ public function exportSubmissionsToCloud(int $formId, string $path, string $file } $file = $this->submissionService->writeFileToCloud($form, $path, $fileFormat); + $this->auditLogger->log(Operation::ResultsExported, 'Results of form %s were exported to file %s', [ + 'formId' => $form->getId(), + 'fileId' => $file->getId(), + 'fileName' => $file->getName(), + 'fileFormat' => $fileFormat, + ]); return new DataResponse($file->getName()); } diff --git a/tests/Unit/Controller/ApiControllerTest.php b/tests/Unit/Controller/ApiControllerTest.php index c9525ac47..b67d6bdbf 100644 --- a/tests/Unit/Controller/ApiControllerTest.php +++ b/tests/Unit/Controller/ApiControllerTest.php @@ -458,6 +458,7 @@ public function testExportSubmissions_noExportPermissions() { ->with($form) ->willReturn([Constants::PERMISSION_SUBMIT]); + $this->expectNoAuditLog(); $this->expectException(NoSuchFormException::class); $this->apiController->getSubmissions(1, fileFormat: 'csv'); } @@ -490,6 +491,9 @@ public function testExportSubmissions() { ->willReturn($fileName); $this->assertEquals(new DataDownloadResponse($csv, $fileName, 'text/csv'), $this->apiController->getSubmissions(1, fileFormat: 'csv')); + $this->assertAuditLogged([ + [Operation::ResultsDownloaded, 'Results of form %s were downloaded as %s', ['formId' => 1, 'fileFormat' => 'csv']], + ]); } public function testExportSubmissionsToCloud_invalidForm() { @@ -518,11 +522,45 @@ public function testExportSubmissionsToCloud_noExportPermissions() { $this->submissionService->expects($this->never()) ->method('writeFileToCloud'); + $this->expectNoAuditLog(); $this->expectException(OCSForbiddenException::class); $this->apiController->exportSubmissionsToCloud(1, '/', 'csv'); } + public function testExportSubmissionsToCloud(): void { + $form = new Form(); + $form->setId(1); + $form->setOwnerId('currentUser'); + + $this->formsService->expects($this->once()) + ->method('getFormIfAllowed') + ->with(1, Constants::PERMISSION_RESULTS) + ->willReturn($form); + $this->formsService->expects($this->once()) + ->method('getPermissions') + ->with($form) + ->willReturn([Constants::PERMISSION_SUBMIT, Constants::PERMISSION_RESULTS]); + + $file = $this->createMock(File::class); + $file->method('getId')->willReturn(42); + $file->method('getName')->willReturn('Form 1 (responses).xlsx'); + $this->submissionService->expects($this->once()) + ->method('writeFileToCloud') + ->with($form, '/Exports', 'xlsx') + ->willReturn($file); + + $this->assertEquals(new DataResponse('Form 1 (responses).xlsx'), $this->apiController->exportSubmissionsToCloud(1, '/Exports', 'xlsx')); + $this->assertAuditLogged([ + [Operation::ResultsExported, 'Results of form %s were exported to file %s', [ + 'formId' => 1, + 'fileId' => 42, + 'fileName' => 'Form 1 (responses).xlsx', + 'fileFormat' => 'xlsx', + ]], + ]); + } + public function testCreateNewForm_notAllowed() { $this->configService->expects($this->once()) ->method('canCreateForms') @@ -1030,6 +1068,10 @@ public function testDeleteSubmissionNoPermission($submissionData, $formData) { ->with(1, Constants::PERMISSION_RESULTS_DELETE) ->willReturn($form); + $this->submissionMapper->expects($this->never()) + ->method('deleteById'); + $this->expectNoAuditLog(); + $this->expectException(OCSForbiddenException::class); $this->apiController->deleteSubmission(1, 42); } @@ -1067,13 +1109,18 @@ public function testDeleteSubmission($submissionData, $formData) { ->with(SyncSubmissionsWithLinkedFileJob::class, ['form_id' => 1]); $this->assertEquals(new DataResponse(42), $this->apiController->deleteSubmission(1, 42)); + $this->assertAuditLogged([ + [Operation::SubmissionDeleted, 'Submission %s of form %s by "%s" was deleted', ['submissionId' => 42, 'formId' => 1, 'submissionUserId' => 'respondent']], + ]); } public static function dataTestDeletePermission() { return [ [ [ + 'id' => 42, 'formId' => 1, + 'userId' => 'respondent', ], [ 'id' => 1, @@ -1132,6 +1179,7 @@ public function testDeleteAllSubmissionsNoPermission(): void { $this->submissionMapper ->expects($this->never()) ->method('deleteByForm'); + $this->expectNoAuditLog(); $this->expectException(OCSForbiddenException::class); $this->apiController->deleteAllSubmissions(1); @@ -1178,6 +1226,9 @@ public function testDeleteAllSubmissionsAsOwner(): void { ->with(SyncSubmissionsWithLinkedFileJob::class, ['form_id' => 1]); $this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1)); + $this->assertAuditLogged([ + [Operation::SubmissionsDeletedAll, 'All submissions of form %s were deleted', ['formId' => 1]], + ]); } /** @@ -1220,6 +1271,9 @@ public function testDeleteAllSubmissionsWithResultsDeletePermission(): void { ->method('add'); $this->assertEquals(new DataResponse(1), $this->apiController->deleteAllSubmissions(1)); + $this->assertAuditLogged([ + [Operation::SubmissionsDeletedAll, 'All submissions of form %s were deleted', ['formId' => 1]], + ]); } public function testUpdateFormAllowsValidConfirmationEmailQuestionId(): void { @@ -1889,6 +1943,9 @@ public function testUpdateSubmission_success() { $response = $this->apiController->updateSubmission($formId, $submissionId, $answers); $this->assertEquals(new DataResponse($submissionId), $response); + $this->assertAuditLogged([ + [Operation::SubmissionUpdated, 'Submission %s of form %s by "%s" was updated', ['submissionId' => 42, 'formId' => 1, 'submissionUserId' => 'currentUser']], + ]); } public function testUpdateSubmission_noPermission() { @@ -1947,6 +2004,7 @@ public function testUpdateSubmission_invalidAnswers() { ->method('validateSubmission') ->willThrowException(new \InvalidArgumentException('Invalid answers')); + $this->expectNoAuditLog(); $this->expectException(OCSBadRequestException::class); $this->expectExceptionMessage('Invalid answers'); $this->apiController->updateSubmission($formId, $submissionId, $answers); From 13a1de293d51d0be6711891bd422d8e636013b48 Mon Sep 17 00:00:00 2001 From: Ghassen kefi Date: Mon, 19 Oct 2026 15:45:22 +0200 Subject: [PATCH 4/4] feat(settings): log changes to the Forms admin settings to the audit log Changing one of the Forms admin settings (permitting forms for all users, public links, custom public link tokens, showing forms to all users, restricting form creation and the groups allowed to create forms, confirmation emails and their rate limit, comments) is now written to the admin audit log with the setting name and its new value, after the value was saved. All of these settings are booleans, integers or group IDs. Rejected values are not logged. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ghassen kefi --- lib/AuditLog/Operation.php | 2 ++ lib/Controller/ConfigController.php | 9 +++++ .../Unit/Controller/ConfigControllerTest.php | 34 ++++++++++++++++++- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index ed55d86a0..d791f42f8 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -30,6 +30,7 @@ enum Operation: string { case SubmissionUpdated = 'forms.submission.updated'; case SubmissionDeleted = 'forms.submission.deleted'; case SubmissionsDeletedAll = 'forms.submissions.deleted_all'; + case ConfigUpdated = 'forms.config.updated'; /** * Short description of when the operation is logged, used for the documentation @@ -51,6 +52,7 @@ public function description(): string { self::SubmissionUpdated => 'The answers of a submission were replaced', self::SubmissionDeleted => 'A submission was deleted', self::SubmissionsDeletedAll => 'All submissions of a form were deleted', + self::ConfigUpdated => 'An admin setting of Forms (sharing, form creation, confirmation emails, comments) was changed', }; } } diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 7eee3e403..e314db2d3 100644 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -8,6 +8,8 @@ namespace OCA\Forms\Controller; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Constants; use OCA\Forms\Service\ConfigService; use OCP\AppFramework\ApiController; @@ -27,6 +29,7 @@ public function __construct( private readonly IAppConfig $appConfig, private readonly LoggerInterface $logger, IRequest $request, + private readonly AuditLogger $auditLogger, ) { parent::__construct($appName, $request); } @@ -83,6 +86,12 @@ public function updateAppConfig(string $configKey, mixed $configValue): DataResp return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST); } + // All settings are booleans, integers or group IDs + $this->auditLogger->log(Operation::ConfigUpdated, 'Forms setting %s was changed', [ + 'configKey' => $configKey, + 'configValue' => $configValue, + ]); + return new DataResponse(); } } diff --git a/tests/Unit/Controller/ConfigControllerTest.php b/tests/Unit/Controller/ConfigControllerTest.php index 128e35049..f05f7d18a 100644 --- a/tests/Unit/Controller/ConfigControllerTest.php +++ b/tests/Unit/Controller/ConfigControllerTest.php @@ -8,11 +8,16 @@ namespace OCA\Forms\Tests\Unit\Controller; +use OCA\Forms\AuditLog\AuditLogger; +use OCA\Forms\AuditLog\Operation; use OCA\Forms\Controller\ConfigController; use OCA\Forms\Service\ConfigService; +use OCA\Forms\Tests\Unit\AuditLog\AuditEvents; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Services\IAppConfig; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IRequest; +use OCP\Log\Audit\CriticalActionPerformedEvent; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -24,6 +29,7 @@ class ConfigControllerTest extends TestCase { private IAppConfig|MockObject $config; private LoggerInterface|MockObject $logger; private IRequest|MockObject $request; + private IEventDispatcher|MockObject $eventDispatcher; public function setUp(): void { parent::setUp(); @@ -32,13 +38,15 @@ public function setUp(): void { $this->config = $this->createMock(IAppConfig::class); $this->logger = $this->createMock(LoggerInterface::class); $this->request = $this->createMock(IRequest::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->configController = new ConfigController( 'forms', $this->configService, $this->config, $this->logger, - $this->request + $this->request, + new AuditLogger($this->eventDispatcher), ); } @@ -99,9 +107,31 @@ public function testUpdateAppConfig(string $configKey, $configValue) { ->with($configKey, $configValue); } + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->callback(function (CriticalActionPerformedEvent $event) use ($configKey, $configValue): bool { + $this->assertTrue(AuditEvents::hasOperation($event, Operation::ConfigUpdated->value)); + $this->assertSame('Forms setting %s was changed', $event->getLogMessage()); + $this->assertSame(['configKey' => $configKey, 'configValue' => $configValue], $event->getParameters()); + return true; + })); + $this->assertEquals(new DataResponse(), $this->configController->updateAppConfig($configKey, $configValue)); } + public function testUpdateAppConfigInvalidValueIsNotLogged(): void { + $this->config->expects($this->once()) + ->method('setAppValueInt') + ->willThrowException(new \InvalidArgumentException()); + $this->eventDispatcher->expects($this->never()) + ->method('dispatchTyped'); + + $this->assertEquals( + new DataResponse('Invalid value for confirmationEmailRateLimit', 400), + $this->configController->updateAppConfig('confirmationEmailRateLimit', -1) + ); + } + public function testUpdateAppConfig_unknownKey() { $this->logger->expects($this->once()) ->method('debug'); @@ -110,6 +140,8 @@ public function testUpdateAppConfig_unknownKey() { ->method('setAppValueBool'); $this->config->expects($this->never()) ->method('setAppValueArray'); + $this->eventDispatcher->expects($this->never()) + ->method('dispatchTyped'); $this->assertEquals(new DataResponse('Unknown appConfig key: someUnknownKey', 400), $this->configController->updateAppConfig('someUnknownKey', 'storeThisValue!')); }