From ed8d8335d7acf8b09b8c14fb6374bae715ede1c9 Mon Sep 17 00:00:00 2001 From: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:33:58 +0300 Subject: [PATCH 1/4] fix(rejection): notify the next signing order only after the commit A rejection that lets the workflow continue releases the next signing order inside its transaction. releaseNextOrder() persisted ABLE_TO_SIGN and dispatched the notification in the same method, so on an envelope with several documents the signers released for an earlier document were already notified when a later document failed and rolled the transaction back: the message was delivered for an activation that no longer existed. activateSignersForOrder() now only persists the state and returns the signers it activated. activateNextOrder() exposes that for callers that run inside a transaction and notifyActivatedSigners() dispatches the notifications, which SignatureRejectionService calls after commit(). releaseNextOrder() keeps activating and notifying in one step, so the non-transactional callers are unchanged, and a failing listener can no longer undo a recorded rejection. Signed-off-by: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> --- lib/Service/SequentialSigningService.php | 61 +++++++++--- .../SignatureRejectionService.php | 28 +++++- .../Service/SequentialSigningServiceTest.php | 56 +++++++++++ .../SignatureRejectionServiceTest.php | 97 +++++++++++++++++-- 4 files changed, 217 insertions(+), 25 deletions(-) diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index b5416f665b..60cf0b115a 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -9,6 +9,7 @@ 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; @@ -69,22 +70,54 @@ 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); + return $this->activateSignersForOrder($allSignRequests, $nextOrder); + } + + /** + * Notify the signers returned by activateNextOrder(). + * + * @param list $signers + */ + public function notifyActivatedSigners(array $signers): void { + foreach ($signers as $signer) { + $identifyMethods = $this->identifyMethodService->getIdentifyMethodsFromSignRequestId($signer->getId()); + foreach ($identifyMethods as $methodGroup) { + foreach ($methodGroup as $identifyMethod) { + $identifyMethod->willNotifyUser(true); + $identifyMethod->notify(); + } + } + } } /** @@ -111,7 +144,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 +184,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 +200,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..dbddbb7998 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/lib/Service/SignatureRejection/SignatureRejectionService.php @@ -122,8 +122,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 +139,16 @@ 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.')); } + + // The activation is committed at this point, so the signers it released + // are only told about it now. Notifying inside the transaction would + // deliver a message for an activation that a later failure rolls back, + // and a failing listener must not undo a recorded rejection. + try { + $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); + } catch (\Throwable $e) { + $this->logger->error('Error notifying the signers released by the rejection: ' . $e->getMessage(), ['exception' => $e]); + } } /** @@ -187,19 +198,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..abe38b2576 100644 --- a/tests/php/Unit/Service/SequentialSigningServiceTest.php +++ b/tests/php/Unit/Service/SequentialSigningServiceTest.php @@ -213,6 +213,62 @@ 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 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..e8aaeeb564 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,88 @@ 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; + $this->db->expects($this->once()) + ->method('commit') + ->willReturnCallback(function () use (&$committed): void { + $committed = true; + }); + $this->sequentialSigningService->expects($this->once()) + ->method('notifyActivatedSigners') + ->with([$activated]) + ->willReturnCallback(function () use (&$committed): void { + $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()); + } + } + + public function testAFailingNotificationDoesNotUndoACommittedRejection(): 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('the mail server is unreachable')); + $this->db->expects($this->once())->method('commit'); + $this->db->expects($this->never())->method('rollBack'); + $this->logger->expects($this->atLeastOnce())->method('error'); + + $result = $this->getService()->reject($this->file(), $signRequest); + + $this->assertSame(SignRequestStatus::REJECTED, $result->getStatusEnum()); + } + public function testWorkflowIsClosedWhenThePolicyCancelsIt(): void { $this->withPolicy(self::policy(behavior: 'cancel')); $file = $this->file(FileStatus::PARTIAL_SIGNED->value); From d4fe1be5cb56ec1795a016fb1a975deb689ab7ad Mon Sep 17 00:00:00 2001 From: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> Date: Wed, 30 Sep 2026 20:53:38 +0300 Subject: [PATCH 2/4] fix: isolate signer notification failures Every signer/identify-method notification is isolated: a failed delivery is logged with the sign request id and the identify method, and the remaining activated signers are still notified instead of being skipped. The post-commit catch in SignatureRejectionService now catches recoverable exceptions (\Exception) instead of \Throwable, so programming errors are not converted into a logged delivery failure; the log includes the sign request id. Adds regression tests for both behaviours. Signed-off-by: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> --- lib/Service/SequentialSigningService.php | 22 +++++++- .../SignatureRejectionService.php | 14 ++++- .../Service/SequentialSigningServiceTest.php | 56 ++++++++++++++++++- .../SignatureRejectionServiceTest.php | 18 ++++++ 4 files changed, 106 insertions(+), 4 deletions(-) diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index 60cf0b115a..d7be60d544 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -13,6 +13,7 @@ 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; @@ -21,6 +22,7 @@ class SequentialSigningService { public function __construct( private SignRequestMapper $signRequestMapper, private IdentifyMethodService $identifyMethodService, + private LoggerInterface $logger, ) { } @@ -106,6 +108,16 @@ public function activateNextOrder(int $fileId, int $completedOrder): array { /** * Notify the signers returned by activateNextOrder(). * + * A delivery is best effort: one failing notification must not stop the + * remaining signers from being told that they can sign now, so every + * identify method is isolated from the others. + * + * Only recoverable failures (\Exception) are isolated. The concrete + * exception classes depend on the installed notification channels (mail, + * notifications, gateways), so the recoverable family is caught here per + * signer and identify method. A programming error (\Error) is not a + * delivery failure and stays visible instead of being logged as one. + * * @param list $signers */ public function notifyActivatedSigners(array $signers): void { @@ -114,7 +126,15 @@ public function notifyActivatedSigners(array $signers): void { foreach ($identifyMethods as $methodGroup) { foreach ($methodGroup as $identifyMethod) { $identifyMethod->willNotifyUser(true); - $identifyMethod->notify(); + try { + $identifyMethod->notify(); + } catch (\Exception $e) { + $this->logger->error('Error notifying an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + 'identifyMethod' => $identifyMethod->getName(), + ]); + } } } } diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index dbddbb7998..aa61d50449 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/lib/Service/SignatureRejection/SignatureRejectionService.php @@ -144,10 +144,20 @@ private function persistRejection( // are only told about it now. Notifying inside the transaction would // deliver a message for an activation that a later failure rolls back, // and a failing listener must not undo a recorded rejection. + // + // Sending a notification is a post-commit side effect: a recoverable + // delivery failure (\Exception) is logged instead of making the + // recorded rejection look failed for the caller. Programming errors + // (\Error) are not delivery failures and deliberately stay visible; + // failing notifications are already isolated per signer by + // SequentialSigningService::notifyActivatedSigners(). try { $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); - } catch (\Throwable $e) { - $this->logger->error('Error notifying the signers released by the rejection: ' . $e->getMessage(), ['exception' => $e]); + } catch (\Exception $e) { + $this->logger->error('Error notifying the signers released by the rejection: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signRequest->getId(), + ]); } } diff --git a/tests/php/Unit/Service/SequentialSigningServiceTest.php b/tests/php/Unit/Service/SequentialSigningServiceTest.php index abe38b2576..ececb3d4d7 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 ); } @@ -269,6 +273,56 @@ public function testNotifyActivatedSignersIsANoOpWithoutActivatedSigners(): void $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->once())->method('willNotifyUser')->with(true); + $delivered->expects($this->once())->method('notify'); + + $this->identifyMethodService->method('getIdentifyMethodsFromSignRequestId') + ->willReturnCallback(fn (int $signRequestId): array => $signRequestId === 2 + ? [[$failing]] + : [[$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 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 e8aaeeb564..d57daf9f5a 100644 --- a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php +++ b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php @@ -463,6 +463,24 @@ public function testAFailingNotificationDoesNotUndoACommittedRejection(): void { $this->assertSame(SignRequestStatus::REJECTED, $result->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->db->expects($this->once())->method('commit'); + $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); From fb8ad1a45384234b92b82e8d2919a7315f2907b1 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Tue, 6 Oct 2026 00:13:20 +0000 Subject: [PATCH 3/4] fix(rejection): address notification isolation review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep recoverable notification failures in SequentialSigningService, extract per-signer delivery, and preserve rollback assertions. Adapt identification lookup isolation from PR #8948 while keeping rejection event ordering for that follow-up PR. Co-authored-by: André Maia Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/SequentialSigningService.php | 51 ++++++++++--------- .../SignatureRejectionService.php | 20 +------- .../Service/SequentialSigningServiceTest.php | 44 ++++++++++++++-- .../SignatureRejectionServiceTest.php | 18 ++++--- 4 files changed, 82 insertions(+), 51 deletions(-) diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index d7be60d544..f7d45bf1db 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -106,35 +106,40 @@ public function activateNextOrder(int $fileId, int $completedOrder): array { } /** - * Notify the signers returned by activateNextOrder(). - * - * A delivery is best effort: one failing notification must not stop the - * remaining signers from being told that they can sign now, so every - * identify method is isolated from the others. - * - * Only recoverable failures (\Exception) are isolated. The concrete - * exception classes depend on the installed notification channels (mail, - * notifications, gateways), so the recoverable family is caught here per - * signer and identify method. A programming error (\Error) is not a - * delivery failure and stays visible instead of being logged as one. - * * @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()); - foreach ($identifyMethods as $methodGroup) { - foreach ($methodGroup as $identifyMethod) { + } catch (\Exception $e) { + $this->logger->error('Error loading identification methods for an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + ]); + return; + } + + foreach ($identifyMethods as $methodGroup) { + foreach ($methodGroup as $identifyMethod) { + try { $identifyMethod->willNotifyUser(true); - try { - $identifyMethod->notify(); - } catch (\Exception $e) { - $this->logger->error('Error notifying an activated signer: ' . $e->getMessage(), [ - 'exception' => $e, - 'signRequestId' => $signer->getId(), - 'identifyMethod' => $identifyMethod->getName(), - ]); - } + $identifyMethod->notify(); + } catch (\Exception $e) { + $this->logger->error('Error notifying an activated signer: ' . $e->getMessage(), [ + 'exception' => $e, + 'signRequestId' => $signer->getId(), + 'identifyMethod' => $identifyMethod->getName(), + ]); } } } diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index aa61d50449..2e80122772 100644 --- a/lib/Service/SignatureRejection/SignatureRejectionService.php +++ b/lib/Service/SignatureRejection/SignatureRejectionService.php @@ -140,25 +140,7 @@ private function persistRejection( throw new LibresignException($this->l10n->t('It was not possible to register the rejection. Nothing was changed.')); } - // The activation is committed at this point, so the signers it released - // are only told about it now. Notifying inside the transaction would - // deliver a message for an activation that a later failure rolls back, - // and a failing listener must not undo a recorded rejection. - // - // Sending a notification is a post-commit side effect: a recoverable - // delivery failure (\Exception) is logged instead of making the - // recorded rejection look failed for the caller. Programming errors - // (\Error) are not delivery failures and deliberately stay visible; - // failing notifications are already isolated per signer by - // SequentialSigningService::notifyActivatedSigners(). - try { - $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); - } catch (\Exception $e) { - $this->logger->error('Error notifying the signers released by the rejection: ' . $e->getMessage(), [ - 'exception' => $e, - 'signRequestId' => $signRequest->getId(), - ]); - } + $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); } /** diff --git a/tests/php/Unit/Service/SequentialSigningServiceTest.php b/tests/php/Unit/Service/SequentialSigningServiceTest.php index ececb3d4d7..a56dfeb38c 100644 --- a/tests/php/Unit/Service/SequentialSigningServiceTest.php +++ b/tests/php/Unit/Service/SequentialSigningServiceTest.php @@ -283,12 +283,12 @@ public function testNotifyActivatedSignersKeepsNotifyingTheRemainingSignersWhenA ->willThrowException(new \RuntimeException('the mail server is unreachable')); $delivered = $this->createMock(\OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod::class); - $delivered->expects($this->once())->method('willNotifyUser')->with(true); - $delivered->expects($this->once())->method('notify'); + $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]] + ? [[$failing, $delivered]] : [[$delivered]]); $this->logger->expects($this->once())->method('error')->with( @@ -303,6 +303,44 @@ public function testNotifyActivatedSignersKeepsNotifyingTheRemainingSignersWhenA ])); } + 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)); diff --git a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php index d57daf9f5a..ddf84146f7 100644 --- a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php +++ b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php @@ -443,24 +443,30 @@ public function testARolledBackRejectionDoesNotNotifyAnOrderReleasedForAnEarlier $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 testAFailingNotificationDoesNotUndoACommittedRejection(): void { + 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('the mail server is unreachable')); + ->willThrowException(new \RuntimeException('unexpected notification exception')); $this->db->expects($this->once())->method('commit'); $this->db->expects($this->never())->method('rollBack'); - $this->logger->expects($this->atLeastOnce())->method('error'); - - $result = $this->getService()->reject($this->file(), $signRequest); + $this->logger->expects($this->never())->method('error'); - $this->assertSame(SignRequestStatus::REJECTED, $result->getStatusEnum()); + 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 { From f067b5598618db096b273e057c14c3b3a0656944 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Tue, 6 Oct 2026 00:29:46 +0000 Subject: [PATCH 4/4] fix(rejection): dispatch rejection event before signer notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Return activated signers after committing the rejection and dispatch SignatureRejectedEvent before attempting their notifications. Preserve event delivery when a post-commit notification error propagates. Adapt the event-ordering correction from PR #8948 and strengthen the successful-order and notification-error regression assertions. Co-authored-by: André Maia Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- .../SignatureRejectionService.php | 10 +++++++--- .../SignatureRejectionServiceTest.php | 16 +++++++++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/Service/SignatureRejection/SignatureRejectionService.php b/lib/Service/SignatureRejection/SignatureRejectionService.php index 2e80122772..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(), @@ -140,7 +144,7 @@ private function persistRejection( throw new LibresignException($this->l10n->t('It was not possible to register the rejection. Nothing was changed.')); } - $this->sequentialSigningService->notifyActivatedSigners($activatedSigners); + return $activatedSigners; } /** diff --git a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php index ddf84146f7..b1bc63e1cc 100644 --- a/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php +++ b/tests/php/Unit/Service/SignatureRejection/SignatureRejectionServiceTest.php @@ -390,15 +390,24 @@ public function testTheReleasedOrderIsNotifiedOnlyAfterTheRejectionIsCommitted() ->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): void { + ->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.', @@ -456,6 +465,8 @@ public function testAnUnexpectedNotificationExceptionDoesNotRollBackACommittedRe ->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'); @@ -478,7 +489,10 @@ public function testAProgrammingErrorWhileNotifyingIsNotConvertedIntoADeliveryFa $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);