From b242ff3664c20c40a2757d8b3933d1fe9b77b6d7 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 7c3398890f..5eed585a89 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -170,4 +170,66 @@ private function getSignatureFlow(): SignatureFlow { 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 8c38e9678c44a6a0e8c8e13f0ebdb02e7c0a58d9 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 2ae89cf223..e63e4811ae 100644 --- a/lib/Service/RequestSignatureService.php +++ b/lib/Service/RequestSignatureService.php @@ -208,6 +208,7 @@ private function associateToSigners(array $data, int $fileId): array { 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 @@ private function associateToSigners(array $data, int $fileId): array { fileId: $fileId, signingOrder: $signingOrder, fileStatus: $fileStatus, + signerStatus: $signerStatus, ); } } else { @@ -232,6 +234,7 @@ private function associateToSigners(array $data, int $fileId): array { fileId: $fileId, signingOrder: $signingOrder, fileStatus: $fileStatus, + signerStatus: $signerStatus, ); } } @@ -254,6 +257,7 @@ private function associateToSigner( int $fileId, int $signingOrder = 0, ?int $fileStatus = null, + ?int $signerStatus = null, ): SignRequestEntity { $identifyMethodsIncances = $this->identifyMethod->getByUserData($identifyMethods); if (empty($identifyMethodsIncances)) { @@ -272,8 +276,8 @@ private function associateToSigner( $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 @@ private function associateToSigner( 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 ed21b5c38cedded85d8ce011e39f8638e278cd91 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 e63e4811ae..2da4aa1514 100644 --- a/lib/Service/RequestSignatureService.php +++ b/lib/Service/RequestSignatureService.php @@ -296,7 +296,7 @@ private function updateStatusIfAllowed( 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 @@ private function determineInitialStatus( ?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 5eed585a89..a639cd00cc 100644 --- a/lib/Service/SequentialSigningService.php +++ b/lib/Service/SequentialSigningService.php @@ -196,7 +196,7 @@ public function hasPendingLowerOrderSigners(int $fileId, int $currentOrder): boo */ public function isStatusUpgrade( SignRequestStatus $currentStatus, - SignRequestStatus $desiredStatus + SignRequestStatus $desiredStatus, ): bool { return $desiredStatus->value >= $currentStatus->value; } @@ -213,7 +213,7 @@ public function isStatusUpgrade( public function validateStatusByOrder( SignRequestStatus $desiredStatus, int $signingOrder, - int $fileId + int $fileId, ): SignRequestStatus { // Only validate for ordered numeric flow if (!$this->isOrderedNumericFlow()) {