diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index b5416f665b..e8a3fdd48c 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -9,9 +9,11 @@ namespace OCA\Libresign\Service; use OCA\Libresign\Db\File as FileEntity; +use OCA\Libresign\Db\SignRequest; use OCA\Libresign\Db\SignRequestMapper; use OCA\Libresign\Enum\SignatureFlow; use OCA\Libresign\Enum\SignRequestStatus; +use Psr\Log\LoggerInterface; class SequentialSigningService { private int $currentOrder = 1; @@ -20,6 +22,7 @@ class SequentialSigningService { public function __construct( private SignRequestMapper $signRequestMapper, private IdentifyMethodService $identifyMethodService, + private LoggerInterface $logger, ) { } @@ -69,22 +72,76 @@ public function determineSigningOrder(?int $userProvidedOrder): int { * @param int $completedOrder The order that was just completed */ public function releaseNextOrder(int $fileId, int $completedOrder): void { + $this->notifyActivatedSigners($this->activateNextOrder($fileId, $completedOrder)); + } + + /** + * Same as releaseNextOrder() without notifying anyone, for callers inside a + * database transaction: a notification cannot be taken back if the + * transaction is rolled back, so they notify with notifyActivatedSigners() + * once it is committed. + * + * @return list the signers that became able to sign + */ + public function activateNextOrder(int $fileId, int $completedOrder): array { if (!$this->isOrderedNumericFlow()) { - return; + return []; } $allSignRequests = $this->signRequestMapper->getByFileId($fileId); if (!$this->isOrderFullyCompleted($allSignRequests, $completedOrder)) { - return; + return []; } $nextOrder = $this->findNextOrder($allSignRequests, $completedOrder); if ($nextOrder === null) { - return; + return []; } - $this->activateSignersForOrder($allSignRequests, $nextOrder); + return $this->activateSignersForOrder($allSignRequests, $nextOrder); + } + + /** + * @param list $signers + */ + public function notifyActivatedSigners(array $signers): void { + foreach ($signers as $signer) { + $this->notifySigner($signer); + } + } + + /** + * The signer is already able to sign when this runs, so a failed delivery + * must not keep the remaining identify methods and signers from being told. + * Only \Exception is isolated: the channels behind notify() (mail, + * notifications, gateways) raise different exception types, while an + * \Error is a programming error and must stay visible. + */ + private function notifySigner(SignRequest $signer): void { + try { + $identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId()); + } catch (\Exception $e) { + $this->logger->error('Failed to load the identify methods of an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + ]); + return; + } + foreach ($identifyMethods as $methodGroup) { + foreach ($methodGroup as $identifyMethod) { + $identifyMethod->willNotifyUser(true); + try { + $identifyMethod->notify(); + } catch (\Exception $e) { + $this->logger->error('Failed to notify an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + 'identifyMethod' => $identifyMethod->getName(), + ]); + } + } + } } /** @@ -111,7 +168,7 @@ public function reorderAfterDeletion(int $fileId, int $deletedOrder): void { if ($previousOrder > 0 && $this->isOrderFullyCompleted($allSignRequests, $previousOrder)) { $nextOrder = $this->findNextOrder($allSignRequests, $deletedOrder); if ($nextOrder !== null) { - $this->activateSignersForOrder($allSignRequests, $nextOrder); + $this->notifyActivatedSigners($this->activateSignersForOrder($allSignRequests, $nextOrder)); } } } @@ -151,27 +208,23 @@ private function findNextOrder(array $signRequests, int $completedOrder): ?int { return null; } - private function activateSignersForOrder(array $signRequests, int $order): void { + /** + * @return list + */ + private function activateSignersForOrder(array $signRequests, int $order): array { $signersToActivate = array_filter( $signRequests, fn ($sr) => $this->isSigningParticipant($sr) && $sr->getSigningOrder() === $order + && $sr->getStatusEnum() === SignRequestStatus::DRAFT ); foreach ($signersToActivate as $signer) { - if ($signer->getStatusEnum() === SignRequestStatus::DRAFT) { - $signer->setStatusEnum(SignRequestStatus::ABLE_TO_SIGN); - $this->signRequestMapper->update($signer); - - $identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId()); - foreach ($identifyMethods as $methodGroup) { - foreach ($methodGroup as $identifyMethod) { - $identifyMethod->willNotifyUser(true); - $identifyMethod->notify(); - } - } - } + $signer->setStatusEnum(SignRequestStatus::ABLE_TO_SIGN); + $this->signRequestMapper->update($signer); } + + return array_values($signersToActivate); } private function getSignatureFlow(): SignatureFlow { diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index c440c3992c..29b0c848ad 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/lib/Service/SignatureRejection/SignatureRejectionService.php @@ -76,8 +76,11 @@ public function reject( $commentIsPrivate = $normalizedComment !== null && $privateComment; $workflowCanceled = $config->cancelsWorkflow(); - $this->persistRejection($libreSignFile, $signRequest, $normalizedComment, $commentIsPrivate, $workflowCanceled); + $activatedSigners = $this->persistRejection($libreSignFile, $signRequest, $normalizedComment, $commentIsPrivate, $workflowCanceled); + // The event goes first so that nothing raised while notifying the released + // signers can keep the listeners from hearing about a committed rejection. $this->dispatchRejectedEvent($signRequest, $libreSignFile, $workflowCanceled); + $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); return $signRequest; } @@ -94,6 +97,8 @@ public function isWorkflowCanceled(FileEntity $libreSignFile): bool { * Marking the signer as rejected and closing the workflow describe a single * decision, so they are written together: a failure must never leave a signer * who already rejected on a document that stays open for everybody else. + * + * @return list the signers released by the rejection, to be notified after the commit */ private function persistRejection( FileEntity $libreSignFile, @@ -101,7 +106,7 @@ private function persistRejection( ?string $comment, bool $commentIsPrivate, bool $workflowCanceled, - ): void { + ): array { $signRequests = $this->collectSignRequestsToReject($libreSignFile, $signRequest); $previousSignerStatuses = array_map( static fn (SignRequestEntity $each): int => $each->getStatus(), @@ -110,6 +115,7 @@ private function persistRejection( $previousFileStatus = $libreSignFile->getStatus(); $rejectedAt = $this->timeFactory->getDateTime(); + $activatedSigners = []; $this->db->beginTransaction(); try { foreach ($signRequests as $each) { @@ -123,7 +129,7 @@ private function persistRejection( if ($workflowCanceled) { $this->cancelWorkflow($libreSignFile); } else { - $this->releaseNextSigningOrders($libreSignFile, $signRequests); + $activatedSigners = $this->releaseNextSigningOrders($libreSignFile, $signRequests); } $this->db->commit(); @@ -138,6 +144,8 @@ private function persistRejection( // TRANSLATORS Error shown when the rejection could not be saved and nothing was changed. throw new LibresignException($this->l10n->t('It was not possible to register the rejection. Nothing was changed.')); } + + return $activatedSigners; } /** @@ -184,22 +192,28 @@ private function collectSignRequestsToReject(FileEntity $libreSignFile, SignRequ * When the workflow continues, a rejection ends the signer's turn just like a * signature does, so in a sequential flow the next order must be released on * every document the signer rejected, or the signers after them would wait - * forever. + * forever. The released signers are returned to be notified after the commit. * * @param list $rejectedSignRequests + * @return list the signers that became able to sign */ - private function releaseNextSigningOrders(FileEntity $libreSignFile, array $rejectedSignRequests): void { + private function releaseNextSigningOrders(FileEntity $libreSignFile, array $rejectedSignRequests): array { + $activatedSigners = []; foreach ($rejectedSignRequests as $rejected) { $document = $rejected->getFileId() === $libreSignFile->getId() ? $libreSignFile : $this->fileMapper->getById($rejected->getFileId()); - $this->sequentialSigningService - ->setFile($document) - ->releaseNextOrder( - $rejected->getFileId(), - $rejected->getSigningOrder() - ); + $activatedSigners = [ + ...$activatedSigners, + ...$this->sequentialSigningService + ->setFile($document) + ->activateNextOrder( + $rejected->getFileId(), + $rejected->getSigningOrder() + ), + ]; } + return $activatedSigners; } /** diff --git a/tests/php/Unit/Service/SequentialSigningServiceTest.php b/tests/php/Unit/Service/SequentialSigningServiceTest.php index fd617c0bdc..59babfe6c3 100644 --- a/tests/php/Unit/Service/SequentialSigningServiceTest.php +++ b/tests/php/Unit/Service/SequentialSigningServiceTest.php @@ -19,20 +19,24 @@ use OCA\Libresign\Tests\Unit\TestCase; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; final class SequentialSigningServiceTest extends TestCase { private SignRequestMapper&MockObject $signRequestMapper; private IdentifyMethodService&MockObject $identifyMethodService; + private LoggerInterface&MockObject $logger; private SequentialSigningService $service; public function setUp(): void { parent::setUp(); $this->signRequestMapper = $this->createMock(SignRequestMapper::class); $this->identifyMethodService = $this->createMock(IdentifyMethodService::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->service = new SequentialSigningService( $this->signRequestMapper, - $this->identifyMethodService + $this->identifyMethodService, + $this->logger, ); } @@ -213,6 +217,135 @@ public function testReleaseNextOrderOrderedFlow(array $requests, int $completedO $this->service->releaseNextOrder(99, $completedOrder); } + public function testActivateNextOrderReturnsTheActivatedSignersWithoutNotifyingThem(): void { + $file = $this->createMock(FileEntity::class); + $file->method('getSignatureFlowEnum') + ->willReturn(SignatureFlow::ORDERED_NUMERIC); + $this->service->setFile($file); + + $signRequests = $this->buildSignRequests([ + [1, SignRequestStatus::SIGNED, 1], + [2, SignRequestStatus::DRAFT, 2], + [3, SignRequestStatus::DRAFT, 2], + ]); + $this->signRequestMapper->method('getByFileId')->willReturn($signRequests); + $this->signRequestMapper->expects($this->exactly(2))->method('update'); + $this->identifyMethodService->expects($this->never()) + ->method('getIdentifyMethodsFromSignRequestId'); + + $activated = $this->service->activateNextOrder(99, 1); + + $this->assertSame([2, 3], array_map(fn (SignRequest $signer): int => $signer->getId(), $activated)); + foreach ($activated as $signer) { + $this->assertSame(SignRequestStatus::ABLE_TO_SIGN, $signer->getStatusEnum()); + } + } + + public function testActivateNextOrderReturnsNobodyWhenNotOrdered(): void { + $file = $this->createMock(FileEntity::class); + $file->method('getSignatureFlowEnum') + ->willReturn(SignatureFlow::PARALLEL); + $this->service->setFile($file); + + $this->assertSame([], $this->service->activateNextOrder(99, 1)); + } + + public function testNotifyActivatedSignersNotifiesEveryIdentifyMethodOfEverySigner(): void { + $signers = $this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ]); + + $email = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $email->expects($this->exactly(2))->method('willNotifyUser')->with(true); + $email->expects($this->exactly(2))->method('notify'); + $account = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $account->expects($this->exactly(2))->method('notify'); + + $requestedIds = []; + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(function (int $signRequestId) use (&$requestedIds, $email, $account): array { + $requestedIds[] = $signRequestId; + return ['email' => [$email], 'account' => [$account]]; + }); + $this->signRequestMapper->expects($this->never())->method('update'); + + $this->service->notifyActivatedSigners($signers); + + $this->assertSame([2, 3], $requestedIds); + } + + public function testAFailedNotificationDoesNotStopTheOtherActivatedSigners(): void { + $signers = $this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ]); + + $failing = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $failing->method('getName')->willReturn('email'); + $failing->expects($this->once())->method('notify') + ->willThrowException(new \RuntimeException('mail server is down')); + $next = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $next->expects($this->once())->method('notify'); + + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(fn (int $signRequestId): array => $signRequestId === 2 + ? ['email' => [$failing]] + : ['account' => [$next]]); + $this->logger->expects($this->once()) + ->method('error') + ->with( + $this->stringContains('mail server is down'), + $this->callback(fn (array $context): bool => $context['signRequestId'] === 2 + && $context['identifyMethod'] === 'email' + && $context['exception'] instanceof \RuntimeException), + ); + + $this->service->notifyActivatedSigners($signers); + } + + public function testAFailedIdentifyMethodLookupDoesNotStopTheOtherActivatedSigners(): void { + $signers = $this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ]); + + $next = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $next->expects($this->once())->method('notify'); + + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(function (int $signRequestId) use ($next): array { + if ($signRequestId === 2) { + throw new \RuntimeException('lost connection to the database'); + } + return ['account' => [$next]]; + }); + $this->logger->expects($this->once()) + ->method('error') + ->with( + $this->stringContains('lost connection to the database'), + $this->callback(fn (array $context): bool => $context['signRequestId'] === 2 + && $context['exception'] instanceof \RuntimeException), + ); + + $this->service->notifyActivatedSigners($signers); + } + + public function testAProgrammingErrorWhileNotifyingIsNotHidden(): void { + $signers = $this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + ]); + + $broken = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $broken->method('notify')->willThrowException(new \TypeError('a bug, not a delivery failure')); + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturn(['email' => [$broken]]); + $this->logger->expects($this->never())->method('error'); + + $this->expectException(\TypeError::class); + $this->service->notifyActivatedSigners($signers); + } + public function testReorderAfterDeletionSkipsWhenNotOrdered(): void { $file = $this->createMock(FileEntity::class); $file->method('getSignatureFlowEnum') diff --git a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php index ecf7787d36..b6e762281c 100644 --- a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php +++ b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php @@ -323,8 +323,9 @@ public function testAContinuingRejectionReleasesTheNextSigningOrder(): void { ->with($file) ->willReturnSelf(); $this->sequentialSigningService->expects($this->once()) - ->method('releaseNextOrder') - ->with(10, 2); + ->method('activateNextOrder') + ->with(10, 2) + ->willReturn([]); $this->getService()->reject($file, $signRequest); } @@ -332,7 +333,10 @@ public function testAContinuingRejectionReleasesTheNextSigningOrder(): void { public function testACancellingRejectionReleasesNobody(): void { $this->withPolicy(self::policy(behavior: 'cancel')); - $this->sequentialSigningService->expects($this->never())->method('releaseNextOrder'); + $this->sequentialSigningService->expects($this->never())->method('activateNextOrder'); + $this->sequentialSigningService->expects($this->once()) + ->method('notifyActivatedSigners') + ->with([]); $this->getService()->reject($this->file(), $this->signRequest()); } @@ -351,9 +355,10 @@ public function testRejectingAnEnvelopeReleasesTheNextOrderOfEveryDocument(): vo $this->envelopeSignRequest = $onTheEnvelope; $released = []; - $this->sequentialSigningService->method('releaseNextOrder') - ->willReturnCallback(function (int $fileId) use (&$released): void { + $this->sequentialSigningService->method('activateNextOrder') + ->willReturnCallback(function (int $fileId) use (&$released): array { $released[] = $fileId; + return []; }); $this->getService()->reject($envelope, $onDoc1); @@ -365,7 +370,7 @@ public function testAFailedReleaseOfTheNextOrderLeavesNoPartiallyUpdatedWorkflow $this->withPolicy(self::policy(behavior: 'continue')); $signRequest = $this->signRequest(); - $this->sequentialSigningService->method('releaseNextOrder') + $this->sequentialSigningService->method('activateNextOrder') ->willThrowException(new \RuntimeException('database is down')); $this->db->expects($this->once())->method('rollBack'); $this->db->expects($this->never())->method('commit'); @@ -378,6 +383,100 @@ public function testAFailedReleaseOfTheNextOrderLeavesNoPartiallyUpdatedWorkflow } } + public function testSignersReleasedByARejectionAreNotifiedOnlyAfterTheCommit(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + $nextSigner = $this->signRequest(id: 2); + + $calls = []; + $this->sequentialSigningService->method('activateNextOrder') + ->willReturnCallback(function () use (&$calls, $nextSigner): array { + $calls[] = 'activate'; + return [$nextSigner]; + }); + $this->db->method('commit') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'commit'; + }); + $this->sequentialSigningService->expects($this->once()) + ->method('notifyActivatedSigners') + ->with([$nextSigner]) + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'notify'; + }); + + $this->getService()->reject($this->file(), $this->signRequest()); + + $this->assertSame(['activate', 'commit', 'notify'], $calls); + } + + public function testTheRejectionEventIsDispatchedBeforeTheReleasedSignersAreNotified(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + + $calls = []; + $this->sequentialSigningService->method('activateNextOrder') + ->willReturn([$this->signRequest(id: 2)]); + $this->db->method('commit') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'commit'; + }); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'dispatch'; + }); + $this->sequentialSigningService->expects($this->once()) + ->method('notifyActivatedSigners') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'notify'; + }); + + $this->getService()->reject($this->file(), $this->signRequest()); + + $this->assertSame(['commit', 'dispatch', 'notify'], $calls); + } + + public function testARolledBackRejectionNotifiesNobodyItHadReleased(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + $envelope = $this->envelope(); + $onDoc1 = $this->signRequest(id: 11); + $onDoc1->setFileId(21); + $onDoc2 = $this->signRequest(id: 12); + $onDoc2->setFileId(22); + $this->envelopeChildSignRequests = [$onDoc1, $onDoc2]; + + $nextSignerOnDoc1 = $this->signRequest(id: 31); + $this->sequentialSigningService->method('activateNextOrder') + ->willReturnCallback(function (int $fileId) use ($nextSignerOnDoc1): array { + if ($fileId === 22) { + throw new \RuntimeException('database is down'); + } + return [$nextSignerOnDoc1]; + }); + $this->db->expects($this->once())->method('rollBack'); + $this->db->expects($this->never())->method('commit'); + $this->sequentialSigningService->expects($this->never())->method('notifyActivatedSigners'); + $this->eventDispatcher->expects($this->never())->method('dispatchTyped'); + + $this->expectException(LibresignException::class); + $this->getService()->reject($envelope, $onDoc1); + } + + public function testAnErrorWhileNotifyingAfterTheCommitIsNotHidden(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + + $this->sequentialSigningService->method('activateNextOrder') + ->willReturn([$this->signRequest(id: 2)]); + $this->sequentialSigningService->method('notifyActivatedSigners') + ->willThrowException(new \TypeError('a bug, not a delivery failure')); + $this->eventDispatcher->expects($this->once())->method('dispatchTyped'); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollBack'); + $this->logger->expects($this->never())->method('error'); + + $this->expectException(\TypeError::class); + $this->getService()->reject($this->file(), $this->signRequest()); + } + public function testWorkflowIsClosedWhenThePolicyCancelsIt(): void { $this->withPolicy(self::policy(behavior: 'cancel')); $file = $this->file(FileStatus::PARTIAL_SIGNED->value);