Merge pull request #7471 from appwrite/fix-empty-user-updates

Fix: Empty values in PATCH of users
2024-06-27 02:31:04 +12:00 · 2024-02-05 15:54:40 +04:00 · 2024-02-05 15:54:40 +04:00 · ad9cc748a5
parent 7ce01deec3 25f8702429
commit ad9cc748a5
8 changed files with 215 additions and 30 deletions
--- a/app/controllers/api/users.php
+++ b/app/controllers/api/users.php
@ -1031,7 +1031,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')
@ -1068,7 +1068,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')
@ -1089,6 +1089,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);
+        }
+
        $hooks->trigger('passwordValidator', [$dbForProject, $project, $password, &$user, true]);

        $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);
@ -1135,7 +1145,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')
@ -1149,21 +1159,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');
@ -1173,7 +1185,6 @@ App::patch('/v1/users/:userId/email')
            ->setAttribute('emailVerification', false)
        ;

-
        try {
            $user = $dbForProject->updateDocument('users', $user->getId(), $user);
            /**
@ -1182,7 +1193,21 @@ 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());
+                }
+            } 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->purgeCachedDocument('users', $user->getId());
        } catch (Duplicate $th) {
@ -1209,7 +1234,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')
@ -1228,12 +1253,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 {
@ -1244,7 +1271,21 @@ 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());
+                }
+            } 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->purgeCachedDocument('users', $user->getId());
        } catch (Duplicate $th) {
--- 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;
        }
--- 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;
    }
--- 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;
--- 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();
    }

    /**
--- 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);
    }

    /**
--- 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;
        }
--- a/tests/e2e/Services/Users/UsersBase.php
+++ b/tests/e2e/Services/Users/UsersBase.php
@ -771,6 +771,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'],
@ -838,6 +864,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'],
@ -905,6 +957,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'],
@ -914,6 +997,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',
@ -1051,6 +1135,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',
@ -1074,7 +1177,7 @@ trait UsersBase
         * Test for FAILURE
         */

-        $errorType = "user_phone_already_exists";
+        $errorType = "user_target_already_exists";
        $user1Id = "user1";
        $statusCodeForUserPhoneAlredyExists = 409;

@ -1385,7 +1488,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']));
    }

    /**
@ -1419,7 +1522,7 @@ trait UsersBase
        ], $this->getHeaders()));

        $this->assertEquals(200, $response['headers']['status-code']);
-        $this->assertEquals(1, $response['body']['total']);
+        $this->assertEquals(2, $response['body']['total']);
    }

    /**