Merge pull request #1646 from appwrite/feat-database-user-deprecation

Feat: Deprecate user for security reasons

app/config/collections2.php

@@ -578,7 +578,7 @@ $collections = [
                 '$id' => 'email',
                 'type' => Database::VAR_STRING,
                 'format' => '',
-                'size' => 1024,
+                'size' => 320,
                 'signed' => true,
                 'required' => false,
                 'default' => null,
@@ -695,15 +695,33 @@ $collections = [
                 'array' => true,
                 'filters' => ['json'],
             ],
+            [
+                '$id' => 'deleted',
+                'type' => Database::VAR_BOOLEAN,
+                'format' => '',
+                'size' => 0,
+                'signed' => true,
+                'required' => false,
+                'default' => null,
+                'array' => false,
+                'filters' => [],
+            ],
         ],
         'indexes' => [
             [
                 '$id' => '_key_email',
                 'type' => Database::INDEX_UNIQUE,
                 'attributes' => ['email'],
-                'lengths' => [1024],
+                'lengths' => [320],
                 'orders' => [Database::ORDER_ASC],
             ],
+            [
+                '$id' => '_key_deleted_email',
+                'type' => Database::INDEX_KEY,
+                'attributes' => ['deleted', 'email'],
+                'lengths' => [0, 320],
+                'orders' => [Database::ORDER_ASC, Database::ORDER_ASC],
+            ],
         ],
     ],
 

app/controllers/api/account.php

@@ -78,7 +78,9 @@ App::post('/v1/account')
         $limit = $project->getAttribute('auths', [])['limit'] ?? 0;
 
         if ($limit !== 0) {
-            $sum = $dbForInternal->count('users', [], APP_LIMIT_USERS);
+            $sum = $dbForInternal->count('users', [
+                new Query('deleted', Query::TYPE_EQUAL, [false]),
+            ], APP_LIMIT_USERS);
 
             if ($sum >= $limit) {
                 throw new Exception('Project registration is restricted. Contact your administrator for more information.', 501);
@@ -105,6 +107,7 @@ App::post('/v1/account')
                 'sessions' => [],
                 'tokens' => [],
                 'memberships' => [],
+                'deleted' => false
             ]));
         } catch (Duplicate $th) {
             throw new Exception('Account already exists', 409);
@@ -165,7 +168,7 @@ App::post('/v1/account/sessions')
         $email = \strtolower($email);
         $protocol = $request->getProtocol();
 
-        $profile = $dbForInternal->findOne('users', [new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
+        $profile = $dbForInternal->findOne('users', [new Query('deleted', Query::TYPE_EQUAL, [false]), new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
 
         if (!$profile || !Auth::passwordVerify($password, $profile->getAttribute('password'))) {
             $audits
@@ -462,13 +465,13 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
             $name = $oauth2->getUserName($accessToken);
             $email = $oauth2->getUserEmail($accessToken);
 
-            $user = $dbForInternal->findOne('users', [new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
+            $user = $dbForInternal->findOne('users', [new Query('deleted', Query::TYPE_EQUAL, [false]), new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
 
             if ($user === false || $user->isEmpty()) { // Last option -> create the user, generate random password
                 $limit = $project->getAttribute('auths', [])['limit'] ?? 0;
 
                 if ($limit !== 0) {
-                    $sum = $dbForInternal->count('users', [], APP_LIMIT_COUNT);
+                    $sum = $dbForInternal->count('users', [ new Query('deleted', Query::TYPE_EQUAL, [false]),], APP_LIMIT_COUNT);
 
                     if ($sum >= $limit) {
                         throw new Exception('Project registration is restricted. Contact your administrator for more information.', 501);
@@ -495,6 +498,7 @@ App::get('/v1/account/sessions/oauth2/:provider/redirect')
                         'sessions' => [],
                         'tokens' => [],
                         'memberships' => [],
+                        'deleted' => false
                     ]));
                 } catch (Duplicate $th) {
                     throw new Exception('Account already exists', 409);
@@ -639,7 +643,9 @@ App::post('/v1/account/sessions/anonymous')
         $limit = $project->getAttribute('auths', [])['limit'] ?? 0;
 
         if ($limit !== 0) {
-            $sum = $dbForInternal->count('users', [], APP_LIMIT_COUNT);
+            $sum = $dbForInternal->count('users', [
+                new Query('deleted', Query::TYPE_EQUAL, [false]),
+            ], APP_LIMIT_COUNT);
 
             if ($sum >= $limit) {
                 throw new Exception('Project registration is restricted. Contact your administrator for more information.', 501);
@@ -665,6 +671,7 @@ App::post('/v1/account/sessions/anonymous')
             'sessions' => [],
             'tokens' => [],
             'memberships' => [],
+            'deleted' => false
         ]));
 
         Authorization::reset();
@@ -1221,6 +1228,8 @@ App::delete('/v1/account')
         $protocol = $request->getProtocol();
         $user = $dbForInternal->updateDocument('users', $user->getId(), $user->setAttribute('status', false));
 
+        // TODO Seems to be related to users.php/App::delete('/v1/users/:userId'). Can we share code between these two? Do todos below apply to users.php?
+
         // TODO delete all tokens or only current session?
         // TODO delete all user data according to GDPR. Make sure everything is backed up and backups are deleted later
         /*
@@ -1463,7 +1472,7 @@ App::post('/v1/account/recovery')
         $isAppUser = Auth::isAppUser(Authorization::$roles);
 
         $email = \strtolower($email);
-        $profile = $dbForInternal->findOne('users', [new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
+        $profile = $dbForInternal->findOne('users', [new Query('deleted', Query::TYPE_EQUAL, [false]), new Query('email', Query::TYPE_EQUAL, [$email])]); // Get user by email address
 
         if (!$profile) {
             throw new Exception('User not found', 404);
@@ -1566,7 +1575,7 @@ App::put('/v1/account/recovery')
 
         $profile = $dbForInternal->getDocument('users', $userId);
 
-        if ($profile->isEmpty()) {
+        if ($profile->isEmpty() || $profile->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 

app/controllers/api/users.php

@@ -66,6 +66,7 @@ App::post('/v1/users')
                 'sessions' => [],
                 'tokens' => [],
                 'memberships' => [],
+                'deleted' => false
             ]));
         } catch (Duplicate $th) {
             throw new Exception('Account already exists', 409);
@@ -106,13 +107,17 @@ App::get('/v1/users')
         if (!empty($after)) {
             $afterUser = $dbForInternal->getDocument('users', $after);
 
-            if ($afterUser->isEmpty()) {
+            if ($afterUser->isEmpty() || $afterUser->getAttribute('deleted')) {
                 throw new Exception('User for after not found', 400);
             }
         }
 
-        $results = $dbForInternal->find('users', [], $limit, $offset, [], [$orderType], $afterUser ?? null);
-        $sum = $dbForInternal->count('users', [], APP_LIMIT_COUNT);
+        $results = $dbForInternal->find('users', [
+            new Query('deleted', Query::TYPE_EQUAL, [false]),
+        ], $limit, $offset, [], [$orderType], $afterUser ?? null);
+        $sum = $dbForInternal->count('users', [
+            new Query('deleted', Query::TYPE_EQUAL, [false]),
+        ], APP_LIMIT_COUNT);
         
         $usage
             ->setParam('users.read', 1)
@@ -146,7 +151,7 @@ App::get('/v1/users/:userId')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -178,7 +183,7 @@ App::get('/v1/users/:userId/prefs')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -214,7 +219,7 @@ App::get('/v1/users/:userId/sessions')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -266,7 +271,7 @@ App::get('/v1/users/:userId/logs')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -374,7 +379,7 @@ App::patch('/v1/users/:userId/status')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -410,7 +415,7 @@ App::patch('/v1/users/:userId/verification')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -446,7 +451,7 @@ App::patch('/v1/users/:userId/name')
         
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -485,7 +490,7 @@ App::patch('/v1/users/:userId/password')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -525,7 +530,7 @@ App::patch('/v1/users/:userId/email')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -569,7 +574,7 @@ App::patch('/v1/users/:userId/prefs')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -606,7 +611,7 @@ App::delete('/v1/users/:userId/sessions/:sessionId')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -661,7 +666,7 @@ App::delete('/v1/users/:userId/sessions')
 
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
@@ -710,26 +715,35 @@ App::delete('/v1/users/:userId')
         
         $user = $dbForInternal->getDocument('users', $userId);
 
-        if ($user->isEmpty()) {
+        if ($user->isEmpty() || $user->getAttribute('deleted')) {
             throw new Exception('User not found', 404);
         }
 
-        if (!$dbForInternal->deleteDocument('users', $userId)) {
-            throw new Exception('Failed to remove user from DB', 500);
-        }
+        // clone user object to send to workers
+        $clone = clone $user;
+
+        $user
+            ->setAttribute("name", null)
+            ->setAttribute("email", null)
+            ->setAttribute("password", null)
+            ->setAttribute("deleted", true)
+        ;
+
+        $dbForInternal->updateDocument('users', $userId, $user);
 
         $deletes
             ->setParam('type', DELETE_TYPE_DOCUMENT)
-            ->setParam('document', $user)
+            ->setParam('document', $clone)
         ;
 
         $events
-            ->setParam('eventData', $response->output($user, Response::MODEL_USER))
+            ->setParam('eventData', $response->output($clone, Response::MODEL_USER))
         ;
 
         $usage
             ->setParam('users.delete', 1)
         ;
+
         $response->noContent();
     });
 

docker-compose.yml

@@ -63,7 +63,7 @@ services:
       - ./psalm.xml:/usr/src/code/psalm.xml
       - ./tests:/usr/src/code/tests
       - ./app:/usr/src/code/app
-      # - ./vendor/utopia-php/database:/usr/src/code/vendor/utopia-php/database
+      # - ./vendor:/usr/src/code/vendor
       - ./docs:/usr/src/code/docs
       - ./public:/usr/src/code/public
       - ./src:/usr/src/code/src

tests/e2e/Services/Users/UsersCustomServerTest.php

@@ -2,6 +2,7 @@
 
 namespace Tests\E2E\Services\Users;
 
+use Tests\E2E\Client;
 use Tests\E2E\Scopes\ProjectCustom;
 use Tests\E2E\Scopes\Scope;
 use Tests\E2E\Scopes\SideServer;
@@ -11,4 +12,49 @@ class UsersCustomServerTest extends Scope
     use UsersBase;
     use ProjectCustom;
     use SideServer;
+
+    public function testDeprecatedUsers():array
+    {
+        /**
+         * Test for FAILURE (don't allow recreating account with same custom ID)
+         */
+
+        // Create user with custom ID 'meldiron'
+        $response = $this->client->call(Client::METHOD_POST, '/users', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()), [
+            'userId' => 'meldiron',
+            'email' => 'matej@appwrite.io',
+            'password' => 'my-superstr0ng-password',
+            'name' => 'Matej Bačo'
+        ]);
+
+        $this->assertEquals(201, $response['headers']['status-code']);
+
+        // Delete user with custom ID 'meldiron'
+        $response = $this->client->call(Client::METHOD_DELETE, '/users/meldiron', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()));
+
+        $this->assertEquals(204, $response['headers']['status-code']);
+
+        // Try to create user with custom ID 'meldiron' again, but now it should fail
+        $response1 = $this->client->call(Client::METHOD_POST, '/users', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()), [
+            'userId' => 'meldiron',
+            'email' => 'matej2@appwrite.io',
+            'password' => 'someones-superstr0ng-password',
+            'name' => 'Matej Bačo Second'
+        ]);
+
+        $this->assertEquals(409, $response1['headers']['status-code']);
+        $this->assertEquals('Account already exists', $response1['body']['message']);
+
+        return [];
+    }
+
 }