diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index b5416f665b..f7d45bf1db 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 as SignRequestEntity; 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,77 @@ 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(), but it only persists the activation and returns + * the signers it activated instead of notifying them. + * + * A caller running inside a transaction must collect the returned signers and + * call notifyActivatedSigners() only after the commit: a notification sent + * before the commit is already delivered when a rollback reverts the + * activation it announced. + * + * @return list The signers moved to 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); + } + + /** + * @param list $signers + */ + public function notifyActivatedSigners(array $signers): void { + foreach ($signers as $signer) { + $this->notifySigner($signer); + } + } + + /** + * Isolate recoverable lookup and delivery failures so other signers can proceed. + * Channel exceptions vary; programming errors must still propagate. + */ + private function notifySigner(SignRequestEntity $signer): void { + try { + $identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId()); + } catch (\Exception $e) { + $this->logger->error('Error loading identification methods for an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + ]); return; } - $this->activateSignersForOrder($allSignRequests, $nextOrder); + foreach ($identifyMethods as $methodGroup) { + foreach ($methodGroup as $identifyMethod) { + try { + $identifyMethod->willNotifyUser(true); + $identifyMethod->notify(); + } catch (\Exception $e) { + $this->logger->error('Error notifying an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + 'identifyMethod' => $identifyMethod->getName(), + ]); + } + } + } } /** @@ -111,7 +169,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,7 +209,12 @@ private function findNextOrder(array $signRequests, int $completedOrder): ?int { return null; } - private function activateSignersForOrder(array $signRequests, int $order): void { + /** + * @param list $signRequests + * @return list The signers moved to ABLE_TO_SIGN. + */ + private function activateSignersForOrder(array $signRequests, int $order): array { + $activated = []; $signersToActivate = array_filter( $signRequests, fn ($sr) => $this->isSigningParticipant($sr) @@ -162,16 +225,11 @@ private function activateSignersForOrder(array $signRequests, int $order): void 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(); - } - } + $activated[] = $signer; } } + + return $activated; } private function getSignatureFlow(): SignatureFlow { diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index c440c3992c..29f268cd78 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/lib/Service/SignatureRejection/SignatureRejectionService.php @@ -76,8 +76,10 @@ 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); + // Record the committed rejection before notification errors can interrupt this flow. $this->dispatchRejectedEvent($signRequest, $libreSignFile, $workflowCanceled); + $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); return $signRequest; } @@ -94,6 +96,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 Signers activated by the committed rejection. */ private function persistRejection( FileEntity $libreSignFile, @@ -101,7 +105,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(), @@ -122,8 +126,9 @@ private function persistRejection( if ($workflowCanceled) { $this->cancelWorkflow($libreSignFile); + $activatedSigners = []; } else { - $this->releaseNextSigningOrders($libreSignFile, $signRequests); + $activatedSigners = $this->releaseNextSigningOrders($libreSignFile, $signRequests); } $this->db->commit(); @@ -138,6 +143,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; } /** @@ -187,19 +194,26 @@ private function collectSignRequestsToReject(FileEntity $libreSignFile, SignRequ * forever. * * @param list $rejectedSignRequests + * @return list The signers activated on every document. */ - 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 + foreach ($this->sequentialSigningService ->setFile($document) - ->releaseNextOrder( + ->activateNextOrder( $rejected->getFileId(), $rejected->getSigningOrder() - ); + ) as $activated) { + $activatedSigners[] = $activated; + } } + + return $activatedSigners; } /** diff --git a/tests/php/Unit/Service/SequentialSigningServiceTest.php b/tests/php/Unit/Service/SequentialSigningServiceTest.php index fd617c0bdc..a56dfeb38c 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,150 @@ 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->expects($this->once()) + ->method('getByFileId') + ->with(99) + ->willReturn($signRequests); + + $this->signRequestMapper->expects($this->exactly(2)) + ->method('update') + ->with($this->callback(fn (SignRequest $request): bool => $request->getStatusEnum() === SignRequestStatus::ABLE_TO_SIGN)); + + $this->identifyMethodService->expects($this->never())->method('getIdentifyMethodsFromSignRequestId'); + + $activated = $this->service->activateNextOrder(99, 1); + + $this->assertCount(2, $activated); + $this->assertSame([2, 3], array_map(fn (SignRequest $request): int => $request->getId(), $activated)); + } + + public function testNotifyActivatedSignersNotifiesEverySignerItReceives(): void { + $this->service->setFile($this->createMock(FileEntity::class)); + + $identifyMethod = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $identifyMethod->expects($this->exactly(2)) + ->method('willNotifyUser') + ->with(true); + $identifyMethod->expects($this->exactly(2)) + ->method('notify'); + + $this->identifyMethodService->expects($this->exactly(2)) + ->method('getIdentifyMethodsFromSignRequestId') + ->willReturn([[$identifyMethod]]); + + $this->service->notifyActivatedSigners($this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ])); + } + + public function testNotifyActivatedSignersIsANoOpWithoutActivatedSigners(): void { + $this->service->setFile($this->createMock(FileEntity::class)); + + $this->identifyMethodService->expects($this->never())->method('getIdentifyMethodsFromSignRequestId'); + + $this->service->notifyActivatedSigners([]); + } + + public function testNotifyActivatedSignersKeepsNotifyingTheRemainingSignersWhenADeliveryFails(): void { + $this->service->setFile($this->createMock(FileEntity::class)); + + $failing = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $failing->method('getName')->willReturn('email'); + $failing->expects($this->once())->method('willNotifyUser')->with(true); + $failing->expects($this->once())->method('notify') + ->willThrowException(new \RuntimeException('the mail server is unreachable')); + + $delivered = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $delivered->expects($this->exactly(2))->method('willNotifyUser')->with(true); + $delivered->expects($this->exactly(2))->method('notify'); + + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(fn (int $signRequestId): array => $signRequestId === 2 + ? [[$failing, $delivered]] + : [[$delivered]]); + + $this->logger->expects($this->once())->method('error')->with( + $this->stringContains('the mail server is unreachable'), + $this->callback(fn (array $context): bool => $context['signRequestId'] === 2 + && $context['identifyMethod'] === 'email'), + ); + + $this->service->notifyActivatedSigners($this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ])); + } + + public function testNotifyActivatedSignersContinuesAfterAnIdentificationLookupFails(): void { + $delivered = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $delivered->expects($this->once())->method('willNotifyUser')->with(true); + $delivered->expects($this->once())->method('notify'); + + $this->identifyMethodService->expects($this->exactly(2)) + ->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(function (int $signRequestId) use ($delivered): array { + if ($signRequestId === 2) { + throw new \RuntimeException('identification lookup failed'); + } + $this->assertSame(3, $signRequestId); + return [[$delivered]]; + }); + $this->logger->expects($this->once())->method('error')->with( + $this->stringContains('identification lookup failed'), + $this->callback(fn (array $context): bool => $context['signRequestId'] === 2 + && $context['exception'] instanceof \RuntimeException), + ); + + $this->service->notifyActivatedSigners($this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + [3, SignRequestStatus::ABLE_TO_SIGN, 2], + ])); + } + + public function testNotifyActivatedSignersDoesNotSwallowIdentificationLookupProgrammingErrors(): void { + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willThrowException(new \TypeError('invalid identification lookup')); + $this->logger->expects($this->never())->method('error'); + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('invalid identification lookup'); + + $this->service->notifyActivatedSigners($this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + ])); + } + + public function testNotifyActivatedSignersDoesNotSwallowProgrammingErrors(): void { + $this->service->setFile($this->createMock(FileEntity::class)); + + $broken = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); + $broken->expects($this->once())->method('notify') + ->willThrowException(new \TypeError('a listener is calling the wrong method')); + + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturn([[$broken]]); + + $this->logger->expects($this->never())->method('error'); + + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('a listener is calling the wrong method'); + + $this->service->notifyActivatedSigners($this->buildSignRequests([ + [2, SignRequestStatus::ABLE_TO_SIGN, 2], + ])); + } + 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..b1bc63e1cc 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,7 @@ 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->getService()->reject($this->file(), $this->signRequest()); } @@ -351,9 +352,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,8 +367,9 @@ 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->sequentialSigningService->expects($this->never())->method('notifyActivatedSigners'); $this->db->expects($this->once())->method('rollBack'); $this->db->expects($this->never())->method('commit'); @@ -378,6 +381,126 @@ public function testAFailedReleaseOfTheNextOrderLeavesNoPartiallyUpdatedWorkflow } } + public function testTheReleasedOrderIsNotifiedOnlyAfterTheRejectionIsCommitted(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + $activated = $this->signRequest(id: 77); + $activated->setFileId(10); + + $this->sequentialSigningService->method('activateNextOrder') + ->willReturn([$activated]); + + $committed = false; + $eventDispatched = false; + $this->db->expects($this->once()) + ->method('commit') + ->willReturnCallback(function () use (&$committed): void { + $committed = true; + }); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with($this->isInstanceOf(SignatureRejectedEvent::class)) + ->willReturnCallback(function () use (&$committed, &$eventDispatched): void { + $this->assertTrue($committed, 'The rejection event must follow the commit.'); + $eventDispatched = true; + }); + $this->sequentialSigningService->expects($this->once()) + ->method('notifyActivatedSigners') + ->with([$activated]) + ->willReturnCallback(function () use (&$committed, &$eventDispatched): void { + $this->assertTrue($eventDispatched, 'The rejection event must precede signer notifications.'); + $this->assertTrue( + $committed, + 'The released signers must only be notified once the rejection is committed.', + ); + }); + + $this->getService()->reject($this->file(), $this->signRequest()); + } + + public function testARolledBackRejectionDoesNotNotifyAnOrderReleasedForAnEarlierDocument(): 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); + $onTheEnvelope = $this->signRequest(id: 13); + $onTheEnvelope->setFileId(1); + + $this->envelopeChildSignRequests = [$onDoc1, $onDoc2]; + $this->envelopeSignRequest = $onTheEnvelope; + + $releasedForDoc1 = $this->signRequest(id: 71); + $this->sequentialSigningService->method('activateNextOrder') + ->willReturnCallback(function (int $fileId) use ($releasedForDoc1): array { + if ($fileId === 21) { + return [$releasedForDoc1]; + } + if ($fileId === 22) { + throw new \RuntimeException('the second document could not be updated'); + } + + return []; + }); + + $this->db->expects($this->once())->method('rollBack'); + $this->db->expects($this->never())->method('commit'); + $this->sequentialSigningService->expects($this->never())->method('notifyActivatedSigners'); + + try { + $this->getService()->reject($envelope, $onDoc1); + $this->fail('The rejection must fail when a later document cannot be released.'); + } catch (LibresignException) { + $this->assertSame(SignRequestStatus::ABLE_TO_SIGN, $onDoc1->getStatusEnum()); + $this->assertSame(SignRequestStatus::ABLE_TO_SIGN, $onDoc2->getStatusEnum()); + $this->assertSame(SignRequestStatus::ABLE_TO_SIGN, $onTheEnvelope->getStatusEnum()); + } + } + + public function testAnUnexpectedNotificationExceptionDoesNotRollBackACommittedRejection(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + $signRequest = $this->signRequest(); + + $this->sequentialSigningService->method('activateNextOrder') + ->willReturn([$this->signRequest(id: 78)]); + $this->sequentialSigningService->method('notifyActivatedSigners') + ->willThrowException(new \RuntimeException('unexpected notification exception')); + $this->eventDispatcher->expects($this->once())->method('dispatchTyped') + ->with($this->isInstanceOf(SignatureRejectedEvent::class)); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollBack'); + $this->logger->expects($this->never())->method('error'); + + try { + $this->getService()->reject($this->file(), $signRequest); + $this->fail('Unexpected notification exceptions must propagate.'); + } catch (\RuntimeException $e) { + $this->assertSame('unexpected notification exception', $e->getMessage()); + $this->assertSame(SignRequestStatus::REJECTED, $signRequest->getStatusEnum()); + } + } + + public function testAProgrammingErrorWhileNotifyingIsNotConvertedIntoADeliveryFailure(): void { + $this->withPolicy(self::policy(behavior: 'continue')); + $signRequest = $this->signRequest(); + + $this->sequentialSigningService->method('activateNextOrder') + ->willReturn([$this->signRequest(id: 78)]); + $this->sequentialSigningService->method('notifyActivatedSigners') + ->willThrowException(new \TypeError('bug in the notification path')); + + $this->eventDispatcher->expects($this->once())->method('dispatchTyped') + ->with($this->isInstanceOf(SignatureRejectedEvent::class)); + $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->expectExceptionMessage('bug in the notification path'); + + $this->getService()->reject($this->file(), $signRequest); + } + public function testWorkflowIsClosedWhenThePolicyCancelsIt(): void { $this->withPolicy(self::policy(behavior: 'cancel')); $file = $this->file(FileStatus::PARTIAL_SIGNED->value);