From 711ca8801a3c0a21429ebc354462b725e5ee85a8 Mon Sep 17 00:00:00 2001 From: Christy Jacob Date: Sun, 21 Jul 2024 23:30:06 +0400 Subject: [PATCH 1/4] feat: address review comments --- app/controllers/api/projects.php | 20 ++++++++++ src/Appwrite/Auth/Validator/MockNumber.php | 4 +- .../Projects/ProjectsConsoleClientTest.php | 38 ++++++++++++++++++- 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/projects.php b/app/controllers/api/projects.php index af588fffc4..1569b4685e 100644 --- a/app/controllers/api/projects.php +++ b/app/controllers/api/projects.php @@ -882,6 +882,26 @@ App::patch('/v1/projects/:projectId/auth/mock-numbers') throw new Exception(Exception::PROJECT_NOT_FOUND); } + /* Ensure there are no duplicate numbers . Numbers have a structure. Throw an exception if there are duplicate numbers + [ + { + "number": "+1234567890", + "code": "123456" + }, + { + "number": "+1234567891", + "code": "123456" + } + ] + */ + $uniqueNumbers = []; + foreach ($numbers as $number) { + if (isset($uniqueNumbers[$number['number']])) { + throw new Exception(Exception::GENERAL_BAD_REQUEST, 'Duplicate numbers are not allowed.'); + } + $uniqueNumbers[$number['number']] = $number['code']; + } + $auths = $project->getAttribute('auths', []); $auths['mockNumbers'] = $numbers; diff --git a/src/Appwrite/Auth/Validator/MockNumber.php b/src/Appwrite/Auth/Validator/MockNumber.php index 2c1c81f863..6f1cf81cae 100644 --- a/src/Appwrite/Auth/Validator/MockNumber.php +++ b/src/Appwrite/Auth/Validator/MockNumber.php @@ -47,8 +47,8 @@ class MockNumber extends Validator } $otp = new Text(6, 6); - if (!$otp->isValid($value['otp'])) { - $this->message = 'OTP must be a valid string and exactly 6 characters.'; + if (!$otp->isValid($value['otp']) || !\ctype_digit($value['otp'])) { + $this->message = 'Invalid OTP. Please make sure the OTP is a 6 digit number'; return false; } diff --git a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php index 753322f168..7be97e2136 100644 --- a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php +++ b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php @@ -1592,7 +1592,7 @@ class ProjectsConsoleClientTest extends Scope ] ]); $this->assertEquals(400, $response['headers']['status-code']); - $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and OTP must be a valid string and exactly 6 characters.', $response['body']['message']); + $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Invalid OTP. Please make sure the OTP is a 6 digit number', $response['body']['message']); /** Trying to pass an OTP shorter than 6 characters*/ $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/mock-numbers', array_merge([ @@ -1607,7 +1607,22 @@ class ProjectsConsoleClientTest extends Scope ] ]); $this->assertEquals(400, $response['headers']['status-code']); - $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and OTP must be a valid string and exactly 6 characters.', $response['body']['message']); + $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Invalid OTP. Please make sure the OTP is a 6 digit number', $response['body']['message']); + + /** Trying to pass an OTP with non numeric characters */ + $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/mock-numbers', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'numbers' => [ + [ + 'phone' => '+1655513432', + 'otp' => '123re2' + ] + ] + ]); + $this->assertEquals(400, $response['headers']['status-code']); + $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Invalid OTP. Please make sure the OTP is a 6 digit number', $response['body']['message']); /** Trying to pass an invalid phone number */ $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/mock-numbers', array_merge([ @@ -1639,6 +1654,25 @@ class ProjectsConsoleClientTest extends Scope $this->assertEquals(400, $response['headers']['status-code']); $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Phone number must start with a \'+\' can have a maximum of fifteen digits.', $response['body']['message']); + /** Trying to pass duplicate numbers */ + $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/mock-numbers', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'numbers' => [ + [ + 'phone' => '+1655513432', + 'otp' => '123456' + ], + [ + 'phone' => '+1655513432', + 'otp' => '123456' + ] + ] + ]); + $this->assertEquals(400, $response['headers']['status-code']); + $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Phone number must start with a \'+\' can have a maximum of fifteen digits.', $response['body']['message']); + $numbers = []; for ($i = 0; $i < 11; $i++) { $numbers[] = [ From 98b8e13add3c6e42fafd8685106738058621398d Mon Sep 17 00:00:00 2001 From: Christy Jacob Date: Sun, 21 Jul 2024 23:45:39 +0400 Subject: [PATCH 2/4] feat: address review comments --- app/controllers/api/projects.php | 18 +++--------------- .../Projects/ProjectsConsoleClientTest.php | 2 +- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/app/controllers/api/projects.php b/app/controllers/api/projects.php index 1569b4685e..e934a2799c 100644 --- a/app/controllers/api/projects.php +++ b/app/controllers/api/projects.php @@ -882,24 +882,12 @@ App::patch('/v1/projects/:projectId/auth/mock-numbers') throw new Exception(Exception::PROJECT_NOT_FOUND); } - /* Ensure there are no duplicate numbers . Numbers have a structure. Throw an exception if there are duplicate numbers - [ - { - "number": "+1234567890", - "code": "123456" - }, - { - "number": "+1234567891", - "code": "123456" - } - ] - */ $uniqueNumbers = []; foreach ($numbers as $number) { - if (isset($uniqueNumbers[$number['number']])) { - throw new Exception(Exception::GENERAL_BAD_REQUEST, 'Duplicate numbers are not allowed.'); + if (isset($uniqueNumbers[$number['phone']])) { + throw new Exception(Exception::GENERAL_BAD_REQUEST, 'Duplicate phone numbers are not allowed.'); } - $uniqueNumbers[$number['number']] = $number['code']; + $uniqueNumbers[$number['phone']] = $number['otp']; } $auths = $project->getAttribute('auths', []); diff --git a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php index 7be97e2136..dd0c8420d5 100644 --- a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php +++ b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php @@ -1671,7 +1671,7 @@ class ProjectsConsoleClientTest extends Scope ] ]); $this->assertEquals(400, $response['headers']['status-code']); - $this->assertEquals('Invalid `numbers` param: Value must a valid array no longer than 10 items and Phone number must start with a \'+\' can have a maximum of fifteen digits.', $response['body']['message']); + $this->assertEquals('Duplicate phone numbers are not allowed.', $response['body']['message']); $numbers = []; for ($i = 0; $i < 11; $i++) { From 7614a356a2fa7628812841d0c79f827596c39fbc Mon Sep 17 00:00:00 2001 From: Christy Jacob Date: Mon, 22 Jul 2024 12:55:15 +0400 Subject: [PATCH 3/4] feat: address review comments --- app/controllers/api/projects.php | 12 ++++++------ src/Appwrite/Auth/Validator/MockNumber.php | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/projects.php b/app/controllers/api/projects.php index e934a2799c..f9b2753eb6 100644 --- a/app/controllers/api/projects.php +++ b/app/controllers/api/projects.php @@ -876,12 +876,6 @@ App::patch('/v1/projects/:projectId/auth/mock-numbers') ->inject('dbForConsole') ->action(function (string $projectId, array $numbers, Response $response, Database $dbForConsole) { - $project = $dbForConsole->getDocument('projects', $projectId); - - if ($project->isEmpty()) { - throw new Exception(Exception::PROJECT_NOT_FOUND); - } - $uniqueNumbers = []; foreach ($numbers as $number) { if (isset($uniqueNumbers[$number['phone']])) { @@ -889,6 +883,12 @@ App::patch('/v1/projects/:projectId/auth/mock-numbers') } $uniqueNumbers[$number['phone']] = $number['otp']; } + + $project = $dbForConsole->getDocument('projects', $projectId); + + if ($project->isEmpty()) { + throw new Exception(Exception::PROJECT_NOT_FOUND); + } $auths = $project->getAttribute('auths', []); diff --git a/src/Appwrite/Auth/Validator/MockNumber.php b/src/Appwrite/Auth/Validator/MockNumber.php index 6f1cf81cae..ac5ba89fc5 100644 --- a/src/Appwrite/Auth/Validator/MockNumber.php +++ b/src/Appwrite/Auth/Validator/MockNumber.php @@ -46,8 +46,8 @@ class MockNumber extends Validator return false; } - $otp = new Text(6, 6); - if (!$otp->isValid($value['otp']) || !\ctype_digit($value['otp'])) { + $otp = new Text(6, 6, Text::NUMBERS); + if (!$otp->isValid($value['otp'])) { $this->message = 'Invalid OTP. Please make sure the OTP is a 6 digit number'; return false; } From eb4e259be5d0c277457b4a51a5e0441cdc83751c Mon Sep 17 00:00:00 2001 From: Christy Jacob Date: Mon, 22 Jul 2024 13:08:02 +0400 Subject: [PATCH 4/4] chore: linter --- app/controllers/api/projects.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/projects.php b/app/controllers/api/projects.php index f9b2753eb6..cdb1575d76 100644 --- a/app/controllers/api/projects.php +++ b/app/controllers/api/projects.php @@ -883,7 +883,7 @@ App::patch('/v1/projects/:projectId/auth/mock-numbers') } $uniqueNumbers[$number['phone']] = $number['otp']; } - + $project = $dbForConsole->getDocument('projects', $projectId); if ($project->isEmpty()) {