Skip to content

Commit 8dd450c

Browse files
vitormattosvitormattos
authored
Merge pull request #5544 from LibreSign/backport/5533/stable32
[stable32] fix: make the error message more specific
2 parents ef94b29 + a9c6e1e commit 8dd450c

File tree

9 files changed

+458
-57
lines changed

9 files changed

+458
-57
lines changed

‎lib/Controller/RequestSignatureController.php‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
namespace OCA\Libresign\Controller;
1010

1111
use OCA\Libresign\AppInfo\Application;
12+
use OCA\Libresign\Exception\LibresignException;
1213
use OCA\Libresign\Helper\ValidateHelper;
1314
use OCA\Libresign\Middleware\Attribute\RequireManager;
1415
use OCA\Libresign\ResponseDefinitions;
@@ -91,10 +92,23 @@ public function request(array $file, array $users, string $name, ?string $callba
9192
],
9293
Http::STATUS_OK
9394
);
95+
} catch (LibresignException $e) {
96+
$errorMessage = $e->getMessage();
97+
$decoded = json_decode($errorMessage, true);
98+
if (json_last_error() === JSON_ERROR_NONE && isset($decoded['errors'])) {
99+
$errorMessage = $decoded['errors'][0]['message'] ?? $errorMessage;
100+
}
101+
return new DataResponse(
102+
[
103+
'message' => $errorMessage,
104+
],
105+
Http::STATUS_UNPROCESSABLE_ENTITY
106+
);
94107
} catch (\Throwable $th) {
108+
$errorMessage = $th->getMessage();
95109
return new DataResponse(
96110
[
97-
'message' => $th->getMessage(),
111+
'message' => $errorMessage,
98112
],
99113
Http::STATUS_UNPROCESSABLE_ENTITY
100114
);
@@ -133,6 +147,7 @@ public function updateSign(?array $users = [], ?string $uuid = null, ?array $vis
133147
try {
134148
$this->validateHelper->validateExistingFile($data);
135149
$this->validateHelper->validateFileStatus($data);
150+
$this->validateHelper->validateIdentifySigners($data);
136151
if (!empty($data['visibleElements'])) {
137152
$this->validateHelper->validateVisibleElements($data['visibleElements'], $this->validateHelper::TYPE_VISIBLE_ELEMENT_PDF);
138153
}

‎lib/Helper/ValidateHelper.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -503,6 +503,50 @@ public function validateFileStatus(array $data): void {
503503
}
504504
}
505505

506+
public function validateIdentifySigners(array $data): void {
507+
$this->validateSignersDataStructure($data);
508+
509+
foreach ($data['users'] as $signer) {
510+
$this->validateSignerData($signer);
511+
}
512+
}
513+
514+
private function validateSignersDataStructure(array $data): void {
515+
if (empty($data) || !array_key_exists('users', $data) || !is_array($data['users'])) {
516+
throw new LibresignException($this->l10n->t('No signers'));
517+
}
518+
}
519+
520+
private function validateSignerData(mixed $signer): void {
521+
if (!is_array($signer) || empty($signer)) {
522+
throw new LibresignException($this->l10n->t('No signers'));
523+
}
524+
525+
$this->validateSignerIdentifyMethods($signer);
526+
}
527+
528+
private function validateSignerIdentifyMethods(array $signer): void {
529+
if (empty($signer['identify']) || !is_array($signer['identify'])) {
530+
// It's an api error, don't translate
531+
throw new LibresignException('No identify methods for signer');
532+
}
533+
534+
foreach ($signer['identify'] as $name => $identifyValue) {
535+
$this->validateIdentifyMethodForRequest($name, $identifyValue);
536+
}
537+
}
538+
539+
private function validateIdentifyMethodForRequest(string $name, string $identifyValue): void {
540+
$identifyMethod = $this->identifyMethodService->getInstanceOfIdentifyMethod($name, $identifyValue);
541+
$identifyMethod->validateToRequest();
542+
543+
$signatureMethods = $identifyMethod->getSignatureMethods();
544+
if (empty($signatureMethods)) {
545+
// It's an api error, don't translate
546+
throw new LibresignException('No signature methods for identify method ' . $name);
547+
}
548+
}
549+
506550
public function validateExistingFile(array $data): void {
507551
if (isset($data['uuid'])) {
508552
$this->validateFileUuid($data);

‎lib/Service/IdentifyMethod/Account.php‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,27 @@ public function __construct(
5454
}
5555

5656
public function validateToRequest(): void {
57-
$signer = $this->userManager->get($this->entity->getIdentifierValue());
58-
if (!$signer) {
59-
throw new LibresignException($this->identifyService->getL10n()->t('User not found.'));
57+
$signer = $this->getSigner();
58+
$this->validateSignatureMethodsForRequest($signer);
59+
}
60+
61+
private function validateSignatureMethodsForRequest(IUser $signer): void {
62+
foreach ($this->getSignatureMethods() as $signatureMethod) {
63+
if (!$signatureMethod->isEnabled()) {
64+
continue;
65+
}
66+
if ($signatureMethod->getName() === ISignatureMethod::SIGNATURE_METHOD_EMAIL_TOKEN) {
67+
$this->validateEmailForEmailToken($signer);
68+
}
69+
}
70+
}
71+
72+
private function validateEmailForEmailToken(IUser $signer): void {
73+
$email = $signer->getEMailAddress();
74+
if (empty($email) || !filter_var($email, FILTER_VALIDATE_EMAIL)) {
75+
throw new LibresignException(
76+
$this->identifyService->getL10n()->t('Signer without valid email address')
77+
);
6078
}
6179
}
6280

‎lib/Service/IdentifyMethod/SignatureMethod/EmailToken.php‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@
88

99
namespace OCA\Libresign\Service\IdentifyMethod\SignatureMethod;
1010

11-
use OCA\Libresign\Exception\LibresignException;
12-
use OCA\Libresign\Helper\JSActions;
1311
use OCA\Libresign\Service\IdentifyMethod\IdentifyService;
1412
use OCA\Libresign\Vendor\Wobeto\EmailBlur\Blur;
1513

@@ -39,13 +37,6 @@ public function toArray(): array {
3937
default => '',
4038
};
4139

42-
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
43-
throw new LibresignException(json_encode([
44-
'action' => JSActions::ACTION_DO_NOTHING,
45-
'errors' => [['message' => $this->identifyService->getL10n()->t('Invalid email')]],
46-
]));
47-
}
48-
4940
$emailLowercase = strtolower($email);
5041

5142
$code = $entity->getCode();
@@ -61,8 +52,8 @@ public function toArray(): array {
6152
$return['identifyMethod'] = $entity->getIdentifierKey();
6253
$return['needCode'] = $needCode;
6354
$return['hasConfirmCode'] = $hasConfirmCode;
64-
$return['blurredEmail'] = $this->blurEmail($emailLowercase);
65-
$return['hashOfEmail'] = md5($emailLowercase);
55+
$return['blurredEmail'] = $emailLowercase ? $this->blurEmail($emailLowercase) : '';
56+
$return['hashOfEmail'] = $emailLowercase ? md5($emailLowercase) : '';
6657
return $return;
6758
}
6859

‎lib/Service/IdentifyMethodService.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ public function getInstanceOfIdentifyMethod(string $name, ?string $identifyValue
7575
if ($identifyValue && isset($this->identifyMethods[$name])) {
7676
foreach ($this->identifyMethods[$name] as $identifyMethod) {
7777
if ($identifyMethod->getEntity()->getIdentifierValue() === $identifyValue) {
78+
$identifyMethod = $this->mergeWithCurrentIdentifyMethod($identifyMethod);
7879
return $identifyMethod;
7980
}
8081
}
@@ -95,6 +96,18 @@ public function getInstanceOfIdentifyMethod(string $name, ?string $identifyValue
9596
return $identifyMethod;
9697
}
9798

99+
private function mergeWithCurrentIdentifyMethod(IIdentifyMethod $identifyMethod): IIdentifyMethod {
100+
if ($this->currentIdentifyMethod === null) {
101+
return $identifyMethod;
102+
}
103+
if ($this->currentIdentifyMethod->getIdentifierKey() === $identifyMethod->getEntity()->getIdentifierKey()
104+
&& $this->currentIdentifyMethod->getIdentifierValue() === $identifyMethod->getEntity()->getIdentifierValue()
105+
) {
106+
$identifyMethod->setEntity($this->currentIdentifyMethod);
107+
}
108+
return $identifyMethod;
109+
}
110+
98111
private function getNewInstanceOfMethod(string $name): IIdentifyMethod {
99112
$className = 'OCA\Libresign\Service\IdentifyMethod\\' . ucfirst($name);
100113
if (!class_exists($className)) {

‎tests/integration/features/sign/request.feature‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -222,8 +222,8 @@ Feature: request-signature
222222
| users | [{"identify":{"account":"signer2"}}] |
223223
| name | document |
224224
Then the response should be a JSON array with the following mandatory values
225-
| key | value |
226-
| (jq).ocs.data.message | User not found. |
225+
| key | value |
226+
| (jq).ocs.data.message | Invalid user |
227227

228228
Scenario: Request to sign with success using account as identifier
229229
Given as user "admin"
@@ -273,8 +273,8 @@ Feature: request-signature
273273
| name | document |
274274
Then the response should have a status code 422
275275
Then the response should be a JSON array with the following mandatory values
276-
| key | value |
277-
| (jq).ocs.data.message | User not found. |
276+
| key | value |
277+
| (jq).ocs.data.message | Invalid user |
278278

279279
Scenario: Request to sign with error using email as account identifier
280280
Given as user "admin"
@@ -285,8 +285,8 @@ Feature: request-signature
285285
| name | document |
286286
Then the response should have a status code 422
287287
Then the response should be a JSON array with the following mandatory values
288-
| key | value |
289-
| (jq).ocs.data.message | User not found. |
288+
| key | value |
289+
| (jq).ocs.data.message | Invalid user |
290290

291291
Scenario: Request to sign with success using email as identifier and URL as file
292292
Given as user "admin"

‎tests/php/Unit/Helper/ValidateHelperTest.php‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
use OCA\Libresign\Db\UserElementMapper;
2020
use OCA\Libresign\Exception\LibresignException;
2121
use OCA\Libresign\Helper\ValidateHelper;
22+
use OCA\Libresign\Service\IdentifyMethod\IIdentifyMethod;
23+
use OCA\Libresign\Service\IdentifyMethod\SignatureMethod\ISignatureMethod;
2224
use OCA\Libresign\Service\IdentifyMethodService;
2325
use OCA\Libresign\Service\SignerElementsService;
2426
use OCP\Files\IMimeTypeDetector;
@@ -30,6 +32,7 @@
3032
use OCP\IUser;
3133
use OCP\IUserManager;
3234
use OCP\Security\IHasher;
35+
use PHPUnit\Framework\Attributes\DataProvider;
3336
use PHPUnit\Framework\MockObject\MockObject;
3437

3538
final class ValidateHelperTest extends \OCA\Libresign\Tests\Unit\TestCase {
@@ -621,4 +624,119 @@ public static function datavalidateIfIdentifyMethodExists(): array {
621624
['telegram', false],
622625
];
623626
}
627+
628+
#[DataProvider('providerValidateSignersDataStructure')]
629+
public function testValidateSignersDataStructure(array $data, ?string $expectedException, ?string $expectedMessage): void {
630+
$validateHelper = $this->getValidateHelper();
631+
632+
if ($expectedException) {
633+
$this->expectException($expectedException);
634+
if ($expectedMessage) {
635+
$this->expectExceptionMessage($expectedMessage);
636+
}
637+
}
638+
639+
// Use reflection to test private method
640+
$method = new \ReflectionMethod($validateHelper, 'validateSignersDataStructure');
641+
$method->invoke($validateHelper, $data);
642+
643+
if (!$expectedException) {
644+
$this->assertTrue(true);
645+
}
646+
}
647+
648+
public static function providerValidateSignersDataStructure(): array {
649+
return [
650+
'Empty data' => [[], LibresignException::class, 'No signers'],
651+
'No users key' => [['invalid' => 'data'], LibresignException::class, 'No signers'],
652+
'Users is not array' => [['users' => 'invalid'], LibresignException::class, 'No signers'],
653+
'Valid structure' => [['users' => []], null, null],
654+
];
655+
}
656+
657+
#[DataProvider('providerValidateSignerData')]
658+
public function testValidateSignerData($signer, ?string $expectedException, ?string $expectedMessage): void {
659+
$validateHelper = $this->getValidateHelper();
660+
661+
if ($expectedException) {
662+
$this->expectException($expectedException);
663+
if ($expectedMessage) {
664+
$this->expectExceptionMessage($expectedMessage);
665+
}
666+
}
667+
668+
$method = new \ReflectionMethod($validateHelper, 'validateSignerData');
669+
$method->invoke($validateHelper, $signer);
670+
671+
if (!$expectedException) {
672+
$this->assertTrue(true);
673+
}
674+
}
675+
676+
public static function providerValidateSignerData(): array {
677+
return [
678+
'Signer is not array' => ['invalid', LibresignException::class, 'No signers'],
679+
'Empty signer' => [[], LibresignException::class, 'No signers'],
680+
'No identify methods' => [['name' => 'User'], LibresignException::class, 'No identify methods for signer'],
681+
'Identify is not array' => [['identify' => 'invalid'], LibresignException::class, 'No identify methods for signer'],
682+
];
683+
}
684+
685+
public function testValidateIdentifyMethodForRequestWithNoSignatureMethods(): void {
686+
$identifyMethod = $this->createMock(IIdentifyMethod::class);
687+
$identifyMethod->method('getSignatureMethods')->willReturn([]);
688+
$identifyMethod->method('validateToRequest');
689+
690+
$this->identifyMethodService
691+
->method('getInstanceOfIdentifyMethod')
692+
->willReturn($identifyMethod);
693+
694+
$validateHelper = $this->getValidateHelper();
695+
696+
$this->expectException(LibresignException::class);
697+
$this->expectExceptionMessage('No signature methods for identify method account');
698+
699+
$method = new \ReflectionMethod($validateHelper, 'validateIdentifyMethodForRequest');
700+
$method->invoke($validateHelper, 'account', '[email protected]');
701+
}
702+
703+
public function testValidateIdentifyMethodForRequestWithValidSignatureMethods(): void {
704+
$signatureMethod = $this->createMock(ISignatureMethod::class);
705+
$identifyMethod = $this->createMock(IIdentifyMethod::class);
706+
$identifyMethod->method('getSignatureMethods')->willReturn([$signatureMethod]);
707+
$identifyMethod->method('validateToRequest');
708+
709+
$this->identifyMethodService
710+
->method('getInstanceOfIdentifyMethod')
711+
->willReturn($identifyMethod);
712+
713+
$validateHelper = $this->getValidateHelper();
714+
715+
$method = new \ReflectionMethod($validateHelper, 'validateIdentifyMethodForRequest');
716+
$result = $method->invoke($validateHelper, 'account', '[email protected]');
717+
718+
$this->assertNull($result);
719+
}
720+
721+
public function testValidateIdentifySignersIntegration(): void {
722+
$signatureMethod = $this->createMock(ISignatureMethod::class);
723+
$identifyMethod = $this->createMock(IIdentifyMethod::class);
724+
$identifyMethod->method('getSignatureMethods')->willReturn([$signatureMethod]);
725+
$identifyMethod->method('validateToRequest');
726+
727+
$this->identifyMethodService
728+
->method('getInstanceOfIdentifyMethod')
729+
->willReturn($identifyMethod);
730+
731+
$data = [
732+
'users' => [
733+
['identify' => ['account' => '[email protected]']]
734+
]
735+
];
736+
737+
$validateHelper = $this->getValidateHelper();
738+
$result = $validateHelper->validateIdentifySigners($data);
739+
740+
$this->assertNull($result);
741+
}
624742
}

0 commit comments

Comments
 (0)