From 311e9a75dd4d31942cf7f2318b6c413abcdfac79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Ba=C4=8Do?= Date: Sat, 20 Jan 2024 09:43:31 +0000 Subject: [PATCH 1/5] Allow empty values in PATH user service --- app/controllers/api/users.php | 18 ++- src/Appwrite/Auth/Validator/Password.php | 11 ++ .../Auth/Validator/PasswordDictionary.php | 3 +- src/Appwrite/Auth/Validator/Phone.php | 17 ++- src/Appwrite/Network/Validator/Email.php | 11 ++ tests/e2e/Services/Users/UsersBase.php | 103 ++++++++++++++++++ 6 files changed, 157 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index e090b3230..176479107 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -1033,7 +1033,7 @@ App::patch('/v1/users/:userId/name') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_USER) ->param('userId', '', new UID(), 'User ID.') - ->param('name', '', new Text(128), 'User name. Max length: 128 chars.') + ->param('name', '', new Text(128, 0), 'User name. Max length: 128 chars.') ->inject('response') ->inject('dbForProject') ->inject('queueForEvents') @@ -1071,7 +1071,7 @@ App::patch('/v1/users/:userId/password') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_USER) ->param('userId', '', new UID(), 'User ID.') - ->param('password', '', fn ($project, $passwordsDictionary) => new PasswordDictionary($passwordsDictionary, $project->getAttribute('auths', [])['passwordDictionary'] ?? false), 'New user password. Must be at least 8 chars.', false, ['project', 'passwordsDictionary']) + ->param('password', '', fn ($project, $passwordsDictionary) => new PasswordDictionary($passwordsDictionary, enabled: $project->getAttribute('auths', [])['passwordDictionary'] ?? false, allowEmpty: true), 'New user password. Must be at least 8 chars.', false, ['project', 'passwordsDictionary']) ->inject('response') ->inject('project') ->inject('dbForProject') @@ -1091,6 +1091,16 @@ App::patch('/v1/users/:userId/password') } } + if (\strlen($password) === 0) { + $user + ->setAttribute('password', '') + ->setAttribute('passwordUpdate', DateTime::now()); + + $user = $dbForProject->updateDocument('users', $user->getId(), $user); + $queueForEvents->setParam('userId', $user->getId()); + $response->dynamic($user, Response::MODEL_USER); + } + $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS); $historyLimit = $project->getAttribute('auths', [])['passwordHistory'] ?? 0; @@ -1136,7 +1146,7 @@ App::patch('/v1/users/:userId/email') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_USER) ->param('userId', '', new UID(), 'User ID.') - ->param('email', '', new Email(), 'User email.') + ->param('email', '', new Email(allowEmpty: true), 'User email.') ->inject('response') ->inject('dbForProject') ->inject('queueForEvents') @@ -1211,7 +1221,7 @@ App::patch('/v1/users/:userId/phone') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_USER) ->param('userId', '', new UID(), 'User ID.') - ->param('number', '', new Phone(), 'User phone number.') + ->param('number', '', new Phone(allowEmpty: true), 'User phone number.') ->inject('response') ->inject('dbForProject') ->inject('queueForEvents') diff --git a/src/Appwrite/Auth/Validator/Password.php b/src/Appwrite/Auth/Validator/Password.php index ffb72467e..bfe557788 100644 --- a/src/Appwrite/Auth/Validator/Password.php +++ b/src/Appwrite/Auth/Validator/Password.php @@ -11,6 +11,13 @@ use Utopia\Validator; */ class Password extends Validator { + protected bool $allowEmpty; + + public function __construct(bool $allowEmpty = false) + { + $this->allowEmpty = $allowEmpty; + } + /** * Get Description. * @@ -36,6 +43,10 @@ class Password extends Validator return false; } + if ($this->allowEmpty && \strlen($value) === 0) { + return true; + } + if (\strlen($value) < 8) { return false; } diff --git a/src/Appwrite/Auth/Validator/PasswordDictionary.php b/src/Appwrite/Auth/Validator/PasswordDictionary.php index e128f497f..99a6c8152 100644 --- a/src/Appwrite/Auth/Validator/PasswordDictionary.php +++ b/src/Appwrite/Auth/Validator/PasswordDictionary.php @@ -12,8 +12,9 @@ class PasswordDictionary extends Password protected array $dictionary; protected bool $enabled; - public function __construct(array $dictionary, bool $enabled = false) + public function __construct(array $dictionary, bool $enabled = false, bool $allowEmpty = false) { + parent::__construct($allowEmpty); $this->dictionary = $dictionary; $this->enabled = $enabled; } diff --git a/src/Appwrite/Auth/Validator/Phone.php b/src/Appwrite/Auth/Validator/Phone.php index 32c3ca339..b8c66edd0 100644 --- a/src/Appwrite/Auth/Validator/Phone.php +++ b/src/Appwrite/Auth/Validator/Phone.php @@ -11,6 +11,13 @@ use Utopia\Validator; */ class Phone extends Validator { + protected bool $allowEmpty; + + public function __construct(bool $allowEmpty = false) + { + $this->allowEmpty = $allowEmpty; + } + /** * Get Description. * @@ -32,7 +39,15 @@ class Phone extends Validator */ public function isValid($value): bool { - return is_string($value) && !!\preg_match('/^\+[1-9]\d{1,14}$/', $value); + if (!is_string($value)) { + return false; + } + + if ($this->allowEmpty && \strlen($value) === 0) { + return true; + } + + return !!\preg_match('/^\+[1-9]\d{1,14}$/', $value); } /** diff --git a/src/Appwrite/Network/Validator/Email.php b/src/Appwrite/Network/Validator/Email.php index efc1a5d5b..3209a4aad 100644 --- a/src/Appwrite/Network/Validator/Email.php +++ b/src/Appwrite/Network/Validator/Email.php @@ -13,6 +13,13 @@ use Utopia\Validator; */ class Email extends Validator { + protected bool $allowEmpty; + + public function __construct(bool $allowEmpty = false) + { + $this->allowEmpty = $allowEmpty; + } + /** * Get Description * @@ -35,6 +42,10 @@ class Email extends Validator */ public function isValid($value): bool { + if ($this->allowEmpty && \strlen($value) === 0) { + return true; + } + if (!\filter_var($value, FILTER_VALIDATE_EMAIL)) { return false; } diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index ddd6e2247..13f010c16 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -742,6 +742,32 @@ trait UsersBase /** * Test for SUCCESS */ + $user = $this->client->call(Client::METHOD_GET, '/users/' . $data['userId'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['name'], 'Cristiano Ronaldo'); + + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/name', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'name' => '', + ]); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['name'], ''); + + $user = $this->client->call(Client::METHOD_GET, '/users/' . $data['userId'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['name'], ''); + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/name', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], @@ -809,6 +835,32 @@ trait UsersBase /** * Test for SUCCESS */ + $user = $this->client->call(Client::METHOD_GET, '/users/' . $data['userId'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['email'], 'cristiano.ronaldo@manchester-united.co.uk'); + + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/email', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'email' => '', + ]); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['email'], ''); + + $user = $this->client->call(Client::METHOD_GET, '/users/' . $data['userId'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['email'], ''); + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/email', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], @@ -876,6 +928,37 @@ trait UsersBase /** * Test for SUCCESS */ + $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], [ + 'email' => 'users.service@updated.com', + 'password' => 'password' + ]); + + $this->assertEquals($session['headers']['status-code'], 201); + + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/password', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'password' => '', + ]); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertNotEmpty($user['body']['$id']); + $this->assertEmpty($user['body']['password']); + + $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], [ + 'email' => 'users.service@updated.com', + 'password' => 'password' + ]); + + $this->assertEquals($session['headers']['status-code'], 401); + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/password', array_merge([ 'content-type' => 'application/json', 'x-appwrite-project' => $this->getProject()['$id'], @@ -885,6 +968,7 @@ trait UsersBase $this->assertEquals($user['headers']['status-code'], 200); $this->assertNotEmpty($user['body']['$id']); + $this->assertNotEmpty($user['body']['password']); $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [ 'content-type' => 'application/json', @@ -1022,6 +1106,25 @@ trait UsersBase /** * Test for SUCCESS */ + $updatedNumber = ""; + $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/phone', array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders()), [ + 'number' => $updatedNumber, + ]); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['phone'], $updatedNumber); + + $user = $this->client->call(Client::METHOD_GET, '/users/' . $data['userId'], array_merge([ + 'content-type' => 'application/json', + 'x-appwrite-project' => $this->getProject()['$id'], + ], $this->getHeaders())); + + $this->assertEquals($user['headers']['status-code'], 200); + $this->assertEquals($user['body']['phone'], $updatedNumber); + $updatedNumber = "+910000000000"; //dummy number $user = $this->client->call(Client::METHOD_PATCH, '/users/' . $data['userId'] . '/phone', array_merge([ 'content-type' => 'application/json', From 42eb051368134b23b75eac2689f59b2d1b4af01f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Ba=C4=8Do?= Date: Sat, 20 Jan 2024 10:06:30 +0000 Subject: [PATCH 2/5] QA bug fixes --- app/controllers/api/users.php | 53 +++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index 176479107..0f6241589 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -1160,21 +1160,23 @@ App::patch('/v1/users/:userId/email') $email = \strtolower($email); - // Makes sure this email is not already used in another identity - $identityWithMatchingEmail = $dbForProject->findOne('identities', [ - Query::equal('providerEmail', [$email]), - Query::notEqual('userId', $user->getId()), - ]); - if ($identityWithMatchingEmail !== false && !$identityWithMatchingEmail->isEmpty()) { - throw new Exception(Exception::USER_EMAIL_ALREADY_EXISTS); - } + if (\strlen($email) !== 0) { + // Makes sure this email is not already used in another identity + $identityWithMatchingEmail = $dbForProject->findOne('identities', [ + Query::equal('providerEmail', [$email]), + Query::notEqual('userId', $user->getId()), + ]); + if ($identityWithMatchingEmail !== false && !$identityWithMatchingEmail->isEmpty()) { + throw new Exception(Exception::USER_EMAIL_ALREADY_EXISTS); + } - $target = $dbForProject->findOne('targets', [ - Query::equal('identifier', [$email]), - ]); + $target = $dbForProject->findOne('targets', [ + Query::equal('identifier', [$email]), + ]); - if ($target instanceof Document && !$target->isEmpty()) { - throw new Exception(Exception::USER_TARGET_ALREADY_EXISTS); + if ($target instanceof Document && !$target->isEmpty()) { + throw new Exception(Exception::USER_TARGET_ALREADY_EXISTS); + } } $oldEmail = $user->getAttribute('email'); @@ -1184,7 +1186,6 @@ App::patch('/v1/users/:userId/email') ->setAttribute('emailVerification', false) ; - try { $user = $dbForProject->updateDocument('users', $user->getId(), $user); /** @@ -1193,7 +1194,11 @@ App::patch('/v1/users/:userId/email') $oldTarget = $user->find('identifier', $oldEmail, 'targets'); if ($oldTarget instanceof Document && !$oldTarget->isEmpty()) { - $dbForProject->updateDocument('targets', $oldTarget->getId(), $oldTarget->setAttribute('identifier', $email)); + if (\strlen($email) !== 0) { + $dbForProject->updateDocument('targets', $oldTarget->getId(), $oldTarget->setAttribute('identifier', $email)); + } else { + $dbForProject->deleteDocument('targets', $oldTarget->getId()); + } } $dbForProject->deleteCachedDocument('users', $user->getId()); } catch (Duplicate $th) { @@ -1240,12 +1245,14 @@ App::patch('/v1/users/:userId/phone') ->setAttribute('phoneVerification', false) ; - $target = $dbForProject->findOne('targets', [ - Query::equal('identifier', [$number]), - ]); + if (\strlen($number) !== 0) { + $target = $dbForProject->findOne('targets', [ + Query::equal('identifier', [$number]), + ]); - if ($target instanceof Document && !$target->isEmpty()) { - throw new Exception(Exception::USER_TARGET_ALREADY_EXISTS); + if ($target instanceof Document && !$target->isEmpty()) { + throw new Exception(Exception::USER_TARGET_ALREADY_EXISTS); + } } try { @@ -1256,7 +1263,11 @@ App::patch('/v1/users/:userId/phone') $oldTarget = $user->find('identifier', $oldPhone, 'targets'); if ($oldTarget instanceof Document && !$oldTarget->isEmpty()) { - $dbForProject->updateDocument('targets', $oldTarget->getId(), $oldTarget->setAttribute('identifier', $number)); + if (\strlen($number) !== 0) { + $dbForProject->updateDocument('targets', $oldTarget->getId(), $oldTarget->setAttribute('identifier', $number)); + } else { + $dbForProject->deleteDocument('targets', $oldTarget->getId()); + } } $dbForProject->deleteCachedDocument('users', $user->getId()); } catch (Duplicate $th) { From eb597b2bd27c715ff7f51298964cfe64918f9f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Ba=C4=8Do?= Date: Sat, 20 Jan 2024 13:07:53 +0000 Subject: [PATCH 3/5] Try to fix targets bug --- app/controllers/api/users.php | 20 +++++++++++++++++++ .../Auth/Validator/PasswordHistory.php | 2 ++ src/Appwrite/Auth/Validator/PersonalData.php | 1 + 3 files changed, 23 insertions(+) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index 0f6241589..04037acdc 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -1199,6 +1199,16 @@ App::patch('/v1/users/:userId/email') } else { $dbForProject->deleteDocument('targets', $oldTarget->getId()); } + } else { + if (\strlen($email) !== 0) { + $target = $dbForProject->createDocument('targets', new Document([ + 'userId' => $user->getId(), + 'userInternalId' => $user->getInternalId(), + 'providerType' => 'email', + 'identifier' => $email, + ])); + $user->setAttribute('targets', [...$user->getAttribute('targets', []), $target]); + } } $dbForProject->deleteCachedDocument('users', $user->getId()); } catch (Duplicate $th) { @@ -1268,6 +1278,16 @@ App::patch('/v1/users/:userId/phone') } else { $dbForProject->deleteDocument('targets', $oldTarget->getId()); } + } else { + if (\strlen($number) !== 0) { + $target = $dbForProject->createDocument('targets', new Document([ + 'userId' => $user->getId(), + 'userInternalId' => $user->getInternalId(), + 'providerType' => 'sms', + 'identifier' => $number, + ])); + $user->setAttribute('targets', [...$user->getAttribute('targets', []), $target]); + } } $dbForProject->deleteCachedDocument('users', $user->getId()); } catch (Duplicate $th) { diff --git a/src/Appwrite/Auth/Validator/PasswordHistory.php b/src/Appwrite/Auth/Validator/PasswordHistory.php index fb73ea6c9..f623ca180 100644 --- a/src/Appwrite/Auth/Validator/PasswordHistory.php +++ b/src/Appwrite/Auth/Validator/PasswordHistory.php @@ -17,6 +17,8 @@ class PasswordHistory extends Password public function __construct(array $history, string $algo, array $algoOptions = []) { + parent::__construct(); + $this->history = $history; $this->algo = $algo; $this->algoOptions = $algoOptions; diff --git a/src/Appwrite/Auth/Validator/PersonalData.php b/src/Appwrite/Auth/Validator/PersonalData.php index 6f8ed0a8c..dbfcdb6cd 100644 --- a/src/Appwrite/Auth/Validator/PersonalData.php +++ b/src/Appwrite/Auth/Validator/PersonalData.php @@ -14,6 +14,7 @@ class PersonalData extends Password protected ?string $phone = null, protected bool $strict = false ) { + parent::__construct(); } /** From 8cfd194cc0378165c371da3b108e7f5498b8bb83 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Mon, 22 Jan 2024 23:29:41 +1300 Subject: [PATCH 4/5] Fix user tests --- tests/e2e/Services/Users/UsersBase.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index 13f010c16..ba974270f 100644 --- a/tests/e2e/Services/Users/UsersBase.php +++ b/tests/e2e/Services/Users/UsersBase.php @@ -1148,7 +1148,7 @@ trait UsersBase * Test for FAILURE */ - $errorType = "user_phone_already_exists"; + $errorType = "user_target_already_exists"; $user1Id = "user1"; $statusCodeForUserPhoneAlredyExists = 409; @@ -1442,7 +1442,7 @@ trait UsersBase 'x-appwrite-project' => $this->getProject()['$id'], ], $this->getHeaders())); $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals(2, \count($response['body']['targets'])); + $this->assertEquals(3, \count($response['body']['targets'])); } /** @@ -1476,7 +1476,7 @@ trait UsersBase ], $this->getHeaders())); $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals(1, $response['body']['total']); + $this->assertEquals(2, $response['body']['total']); } /** From 25f8702429cdb71857d5d97d14ab4218cf2a2ba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Ba=C4=8Do?= Date: Mon, 5 Feb 2024 10:40:53 +0000 Subject: [PATCH 5/5] Fix formatting --- app/controllers/api/users.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index 794e0cd5d..267b8231b 100644 --- a/app/controllers/api/users.php +++ b/app/controllers/api/users.php @@ -1098,7 +1098,7 @@ App::patch('/v1/users/:userId/password') $queueForEvents->setParam('userId', $user->getId()); $response->dynamic($user, Response::MODEL_USER); } - + $hooks->trigger('passwordValidator', [$dbForProject, $project, $password, &$user, true]); $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);