From e557674ef0d9315d920e8e424b1f82b5961b850d Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Fri, 12 Dec 2025 15:03:32 -0300 Subject: [PATCH 1/3] refactor: add status validation methods to SequentialSigningService Move status-related validation logic to SequentialSigningService where it belongs, improving code cohesion and testability. Changes: - Add hasPendingLowerOrderSigners() method to check for incomplete lower-order signers - Add isStatusUpgrade() method to validate status transitions - Add validateStatusByOrder() method to encapsulate ordering validation logic for status transitions These methods are now public and easily testable, centralizing all sequential signing validation logic in a single specialized service. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/SequentialSigningService.php | 62 ++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index 7c3398890..5eed585a8 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -170,4 +170,66 @@ class SequentialSigningService { return SignatureFlow::from($value); } + + /** + * Check if there are signers with lower signing order that haven't signed yet + */ + public function hasPendingLowerOrderSigners(int $fileId, int $currentOrder): bool { + $signRequests = $this->signRequestMapper->getByFileId($fileId); + + foreach ($signRequests as $signRequest) { + $order = $signRequest->getSigningOrder(); + $status = $signRequest->getStatusEnum(); + + // If a signer with lower order hasn't signed yet, return true + if ($order < $currentOrder && $status !== SignRequestStatus::SIGNED) { + return true; + } + } + + return false; + } + + /** + * Check if changing from currentStatus to desiredStatus is an upgrade (or same level) + * Status hierarchy: DRAFT (0) < ABLE_TO_SIGN (1) < SIGNED (2) + */ + public function isStatusUpgrade( + SignRequestStatus $currentStatus, + SignRequestStatus $desiredStatus + ): bool { + return $desiredStatus->value >= $currentStatus->value; + } + + /** + * Validate if a signer can transition to ABLE_TO_SIGN status based on signing order + * In ordered numeric flow, prevents skipping ahead if lower-order signers haven't signed + * + * @param SignRequestStatus $desiredStatus The status being requested + * @param int $signingOrder The signer's order + * @param int $fileId The file ID + * @return SignRequestStatus The validated status (may return DRAFT if validation fails) + */ + public function validateStatusByOrder( + SignRequestStatus $desiredStatus, + int $signingOrder, + int $fileId + ): SignRequestStatus { + // Only validate for ordered numeric flow + if (!$this->isOrderedNumericFlow()) { + return $desiredStatus; + } + + // Only validate when trying to set ABLE_TO_SIGN and not the first signer + if ($desiredStatus !== SignRequestStatus::ABLE_TO_SIGN || $signingOrder <= 1) { + return $desiredStatus; + } + + // Check if any lower order signers haven't signed yet + if ($this->hasPendingLowerOrderSigners($fileId, $signingOrder)) { + return SignRequestStatus::DRAFT; + } + + return $desiredStatus; + } } From 48915dca4bdab006e5566e162e9f62918c12004f Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Fri, 12 Dec 2025 15:03:52 -0300 Subject: [PATCH 2/3] refactor: delegate status validation to SequentialSigningService Remove internal validation methods from RequestSignatureService and delegate to SequentialSigningService for better separation of concerns. Changes: - Remove hasPendingLowerOrderSigners() private method - Remove isStatusUpgrade() private method - Replace inline ordering validation with call to validateStatusByOrder() - Simplify determineInitialStatus() by delegating validation logic This reduces complexity in RequestSignatureService and makes the code more maintainable by following single responsibility principle. All sequential signing logic is now centralized in the specialized service. Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/RequestSignatureService.php | 53 +++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/lib/Service/RequestSignatureService.php b/lib/Service/RequestSignatureService.php index 2ae89cf22..e63e4811a 100644 --- a/lib/Service/RequestSignatureService.php +++ b/lib/Service/RequestSignatureService.php @@ -208,6 +208,7 @@ class RequestSignatureService { foreach ($data['users'] as $user) { $userProvidedOrder = isset($user['signingOrder']) ? (int)$user['signingOrder'] : null; $signingOrder = $this->sequentialSigningService->determineSigningOrder($userProvidedOrder); + $signerStatus = $user['status'] ?? null; if (isset($user['identifyMethods'])) { foreach ($user['identifyMethods'] as $identifyMethod) { @@ -221,6 +222,7 @@ class RequestSignatureService { fileId: $fileId, signingOrder: $signingOrder, fileStatus: $fileStatus, + signerStatus: $signerStatus, ); } } else { @@ -232,6 +234,7 @@ class RequestSignatureService { fileId: $fileId, signingOrder: $signingOrder, fileStatus: $fileStatus, + signerStatus: $signerStatus, ); } } @@ -254,6 +257,7 @@ class RequestSignatureService { int $fileId, int $signingOrder = 0, ?int $fileStatus = null, + ?int $signerStatus = null, ): SignRequestEntity { $identifyMethodsIncances = $this->identifyMethod->getByUserData($identifyMethods); if (empty($identifyMethodsIncances)) { @@ -272,8 +276,8 @@ class RequestSignatureService { $currentStatus = $signRequest->getStatusEnum(); if ($isNewSignRequest || $currentStatus === \OCA\Libresign\Enum\SignRequestStatus::DRAFT) { - $initialStatus = $this->determineInitialStatus($signingOrder, $fileStatus); - $signRequest->setStatusEnum($initialStatus); + $desiredStatus = $this->determineInitialStatus($signingOrder, $fileStatus, $signerStatus, $currentStatus, $fileId); + $this->updateStatusIfAllowed($signRequest, $currentStatus, $desiredStatus, $isNewSignRequest); } $this->saveSignRequest($signRequest); @@ -288,13 +292,56 @@ class RequestSignatureService { return $signRequest; } - private function determineInitialStatus(int $signingOrder, ?int $fileStatus = null): \OCA\Libresign\Enum\SignRequestStatus { + private function updateStatusIfAllowed( + SignRequestEntity $signRequest, + \OCA\Libresign\Enum\SignRequestStatus $currentStatus, + \OCA\Libresign\Enum\SignRequestStatus $desiredStatus, + bool $isNewSignRequest + ): void { + if ($isNewSignRequest || $this->sequentialSigningService->isStatusUpgrade($currentStatus, $desiredStatus)) { + $signRequest->setStatusEnum($desiredStatus); + } + } + + private function determineInitialStatus( + int $signingOrder, + ?int $fileStatus = null, + ?int $signerStatus = null, + ?\OCA\Libresign\Enum\SignRequestStatus $currentStatus = null, + ?int $fileId = null + ): \OCA\Libresign\Enum\SignRequestStatus { + if ($signerStatus !== null) { + $desiredStatus = \OCA\Libresign\Enum\SignRequestStatus::from($signerStatus); + if ($currentStatus !== null && !$this->sequentialSigningService->isStatusUpgrade($currentStatus, $desiredStatus)) { + return $currentStatus; + } + + // Validate status transition based on signing order + if ($fileId !== null) { + return $this->sequentialSigningService->validateStatusByOrder($desiredStatus, $signingOrder, $fileId); + } + + return $desiredStatus; + } + // If fileStatus is explicitly DRAFT (0), keep signer as DRAFT // This allows adding new signers in DRAFT mode even when file is not in DRAFT status if ($fileStatus === FileEntity::STATUS_DRAFT) { return \OCA\Libresign\Enum\SignRequestStatus::DRAFT; } + if ($fileStatus === FileEntity::STATUS_ABLE_TO_SIGN) { + if ($this->sequentialSigningService->isOrderedNumericFlow()) { + // In ordered flow, only first signer (order 1) should be ABLE_TO_SIGN + // Others remain DRAFT until their turn + return $signingOrder === 1 + ? \OCA\Libresign\Enum\SignRequestStatus::ABLE_TO_SIGN + : \OCA\Libresign\Enum\SignRequestStatus::DRAFT; + } + // In parallel flow, all can sign + return \OCA\Libresign\Enum\SignRequestStatus::ABLE_TO_SIGN; + } + if (!$this->sequentialSigningService->isOrderedNumericFlow()) { return \OCA\Libresign\Enum\SignRequestStatus::ABLE_TO_SIGN; } From ede3fe8b9b61f57c635077f71d9f34ec565e87e2 Mon Sep 17 00:00:00 2001 From: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> Date: Fri, 12 Dec 2025 15:07:42 -0300 Subject: [PATCH 3/3] fix: cs Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com> --- lib/Service/RequestSignatureService.php | 4 ++-- lib/Service/SequentialSigningService.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Service/RequestSignatureService.php b/lib/Service/RequestSignatureService.php index e63e4811a..2da4aa151 100644 --- a/lib/Service/RequestSignatureService.php +++ b/lib/Service/RequestSignatureService.php @@ -296,7 +296,7 @@ class RequestSignatureService { SignRequestEntity $signRequest, \OCA\Libresign\Enum\SignRequestStatus $currentStatus, \OCA\Libresign\Enum\SignRequestStatus $desiredStatus, - bool $isNewSignRequest + bool $isNewSignRequest, ): void { if ($isNewSignRequest || $this->sequentialSigningService->isStatusUpgrade($currentStatus, $desiredStatus)) { $signRequest->setStatusEnum($desiredStatus); @@ -308,7 +308,7 @@ class RequestSignatureService { ?int $fileStatus = null, ?int $signerStatus = null, ?\OCA\Libresign\Enum\SignRequestStatus $currentStatus = null, - ?int $fileId = null + ?int $fileId = null, ): \OCA\Libresign\Enum\SignRequestStatus { if ($signerStatus !== null) { $desiredStatus = \OCA\Libresign\Enum\SignRequestStatus::from($signerStatus); diff --git a/lib/Service/SequentialSigningService.php b/lib/Service/SequentialSigningService.php index 5eed585a8..a639cd00c 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -196,7 +196,7 @@ class SequentialSigningService { */ public function isStatusUpgrade( SignRequestStatus $currentStatus, - SignRequestStatus $desiredStatus + SignRequestStatus $desiredStatus, ): bool { return $desiredStatus->value >= $currentStatus->value; } @@ -213,7 +213,7 @@ class SequentialSigningService { public function validateStatusByOrder( SignRequestStatus $desiredStatus, int $signingOrder, - int $fileId + int $fileId, ): SignRequestStatus { // Only validate for ordered numeric flow if (!$this->isOrderedNumericFlow()) {