fix: password comparison

2024-06-28 19:20:25 +12:00 · 2022-12-18 06:27:41 +00:00 · 2022-12-18 06:27:41 +00:00 · 0eae73e7b8
parent 51ad9c6479
commit 0eae73e7b8
7 changed files with 175 additions and 19 deletions
--- a/app/config/collections.php
+++ b/app/config/collections.php
@ -1243,7 +1243,7 @@ $collections = [
                'required' => false,
                'default' => null,
                'array' => true,
-                'filters' => ['json', 'encrypt'],
+                'filters' => [],
            ],
            [
                '$id' => ID::custom('password'),
--- a/app/controllers/api/account.php
+++ b/app/controllers/api/account.php
@ -99,6 +99,7 @@ App::post('/v1/account')

        $passwordHistory = $project->getAttribute('auths',[])['passwordHistory'] ?? 0;

+        $password = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);
        try {
            $userId = $userId == 'unique()' ? ID::unique() : $userId;
            $user = Authorization::skip(fn() => $dbForProject->createDocument('users', new Document([
@ -111,8 +112,8 @@ App::post('/v1/account')
                'email' => $email,
                'emailVerification' => false,
                'status' => true,
-                'passwordHistory' => $passwordHistory > 0 ? [Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS)] : [],
-                'password' => Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS),
+                'passwordHistory' => $passwordHistory > 0 ? [$password] : [],
+                'password' => $password,
                'hash' => Auth::DEFAULT_ALGO,
                'hashOptions' => Auth::DEFAULT_ALGO_OPTIONS,
                'passwordUpdate' => DateTime::now(),
@ -1523,17 +1524,20 @@ App::patch('/v1/account/password')
            throw new Exception(Exception::USER_INVALID_CREDENTIALS);
        }

-        $historyLimit = $project->getAttribute('auths', [])['passwordHistory'] ?? 0;
+        $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);

+        $historyLimit = $project->getAttribute('auths', [])['passwordHistory'] ?? 0;
        $history = [];
        if($historyLimit > 0) {
            $history = $user->getAttribute('passwordHistory', []);
-            $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);
-    
-            if(in_array($newPassword, $history)) {
-                throw new Exception(Exception::USER_PASSWORD_RECENTLY_USED, 'The password was recently used', 409);
+            
+            foreach($history as $hash) {
+                if(Auth::passwordVerify($password, $hash, $user->getAttribute('hash'), $user->getAttribute('hashOptions')))
+                {
+                    throw new Exception(Exception::USER_PASSWORD_RECENTLY_USED, 'The password was recently used', 409);
+                }
            }
-    
+
            $history[] = $newPassword;
            while(count($history) > $historyLimit) {
                array_pop($history);
--- a/app/controllers/api/projects.php
+++ b/app/controllers/api/projects.php
@ -587,7 +587,7 @@ App::patch('/v1/projects/:projectId/auth/password-history')
    ->label('sdk.response.type', Response::CONTENT_TYPE_JSON)
    ->label('sdk.response.model', Response::MODEL_PROJECT)
    ->param('projectId', '', new UID(), 'Project unique ID.')
-    ->param('limit', false, new Range(1, APP_LIMIT_USER_SESSIONS_MAX), 'Set the max number of password history to keep for users. Value allowed is between 0-' . APP_LIMIT_USER_PASSWORD_HISTORY . '. Default is 0')
+    ->param('limit', false, new Range(0, APP_LIMIT_USER_SESSIONS_MAX), 'Set the max number of password history to keep for users. Value allowed is between 0-' . APP_LIMIT_USER_PASSWORD_HISTORY . '. Default is 0')
    ->inject('response')
    ->inject('dbForConsole')
    ->action(function (string $projectId, int $limit, Response $response, Database $dbForConsole) {
--- a/app/controllers/api/users.php
+++ b/app/controllers/api/users.php
@ -803,12 +803,12 @@ App::patch('/v1/users/:userId/password')
            throw new Exception(Exception::USER_NOT_FOUND);
        }

+        $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);
+        
        $historyLimit = $project->getAttribute('auths', [])['passwordHistory'] ?? 0;
-
        $history = [];
        if($historyLimit > 0) {
            $history = $user->getAttribute('passwordHistory', []);
-            $newPassword = Auth::passwordHash($password, Auth::DEFAULT_ALGO, Auth::DEFAULT_ALGO_OPTIONS);
    
            if(in_array($newPassword, $history)) {
                throw new Exception(Exception::USER_PASSWORD_RECENTLY_USED, 'The password was recently used', 409);
--- a/src/Appwrite/Utopia/Response/Model/Project.php
+++ b/src/Appwrite/Utopia/Response/Model/Project.php
@ -120,6 +120,12 @@ class Project extends Model
                'default' => 10,
                'example' => 10,
            ])
+            ->addRule('authPasswordHistory', [
+                'type' => self::TYPE_INTEGER,
+                'description' => 'Max password history to save per user. 20 maximum. 0 for ignoring the password history.',
+                'default' => 0,
+                'example' => 5,
+            ])
            ->addRule('providers', [
                'type' => Response::MODEL_PROVIDER,
                'description' => 'List of Providers.',
@ -240,6 +246,7 @@ class Project extends Model
        $document->setAttribute('authLimit', $authValues['limit'] ?? 0);
        $document->setAttribute('authDuration', $authValues['duration'] ?? Auth::TOKEN_EXPIRATION_LOGIN_LONG);
        $document->setAttribute('authSessionLimit', $authValues['maxSessions'] ?? APP_LIMIT_USER_SESSIONS_DEFAULT);
+        $document->setAttribute('authPasswordHistory', $authValues['passwordHistory'] ?? 0);

        foreach ($auth as $index => $method) {
            $key = $method['key'];
--- a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
+++ b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php
@ -992,6 +992,91 @@ class ProjectsConsoleClientTest extends Scope
        return $data;
    }

+
+    /**
+     * @depends testUpdateProjectAuthLimit
+     */
+    public function testUpdateProjectAuthPasswordHistory($data): array
+    {
+        $id = $data['projectId'] ?? '';
+
+        /**
+         * Test for Success
+         */
+        $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/password-history', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()), [
+            'limit' => 1,
+        ]);
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertEquals(1, $response['body']['authPasswordHistory']);
+
+
+        $email = uniqid() . 'user@localhost.test';
+        $password = 'password';
+        $name = 'User Name';
+
+        /**
+         * Create new user
+         */
+        $response = $this->client->call(Client::METHOD_POST, '/account', array_merge([
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $id,
+        ]), [
+            'userId' => ID::unique(),
+            'email' => $email,
+            'password' => $password,
+            'name' => $name,
+        ]);
+
+        $this->assertEquals(201, $response['headers']['status-code']);
+
+        // create session
+        $session = $this->client->call(Client::METHOD_POST, '/account/sessions/email', [
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $id,
+        ], [
+            'email' => $email,
+            'password' => $password,
+        ]);
+        $this->assertEquals(201, $session['headers']['status-code']);
+        $session = $this->client->parseCookie((string)$session['headers']['set-cookie'])['a_session_' . $id];
+
+        $response = $this->client->call(Client::METHOD_PATCH, '/account/password', array_merge([
+            'origin' => 'http://localhost',
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $id,
+            'cookie' => 'a_session_' . $id . '=' . $session,
+        ]), [
+            'oldPassword' => $password,
+            'password' => $password,
+        ]);
+
+        var_dump($response['body']);
+
+        $this->assertEquals(409, $response['headers']['status-code']);
+
+        
+         /**
+         * Reset
+         */
+        $response = $this->client->call(Client::METHOD_PATCH, '/projects/' . $id . '/auth/password-history', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => $this->getProject()['$id'],
+        ], $this->getHeaders()), [
+            'limit' => 0,
+        ]);
+
+        $this->assertEquals(200, $response['headers']['status-code']);
+        $this->assertEquals(0, $response['body']['authPasswordHistory']);
+        return $data;
+    }
+
+
    public function testUpdateProjectServiceStatusAdmin(): array
    {
        $team = $this->client->call(Client::METHOD_POST, '/teams', array_merge([
--- a/tests/e2e/Services/Users/UsersBase.php
+++ b/tests/e2e/Services/Users/UsersBase.php
@ -56,7 +56,7 @@ trait UsersBase
        $this->assertEquals(true, $res['body']['status']);
        $this->assertGreaterThan('2000-01-01 00:00:00', $res['body']['registration']);

-         /**
+        /**
         * Test Create with hashed passwords
         */
        $res = $this->client->call(Client::METHOD_POST, '/users/md5', array_merge([
@ -180,7 +180,7 @@ trait UsersBase
     */
    public function testCreateUserSessionHashed(array $data): void
    {
-        $userIds = [ 'md5', 'bcrypt', 'argon2', 'sha512', 'scrypt', 'phpass', 'scrypt-modified' ];
+        $userIds = ['md5', 'bcrypt', 'argon2', 'sha512', 'scrypt', 'phpass', 'scrypt-modified'];

        foreach ($userIds as $userId) {
            // Ensure sessions can be created with hashed passwords
@ -236,7 +236,7 @@ trait UsersBase
    {
        /**
         * Test for SUCCESS
-        */
+         */

        // Email + password
        $response = $this->client->call(Client::METHOD_POST, '/users', array_merge([
@ -812,6 +812,66 @@ trait UsersBase
        return $data;
    }

+    /**
+     * @depends testUpdateUserPassword
+     */
+    public function testUserPasswordHistory(array $data): array
+    {
+        $userId = $data['userId'];
+        $projectId = $this->getProject()['$id'];
+        /**
+         * Test for SUCCESS
+         */
+        $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' => 'password2',
+        ]);
+        $this->assertEquals($user['headers']['status-code'], 200);
+        $this->assertNotEmpty($user['body']['$id']);
+
+        $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' => 'password2',
+        ]);
+        $this->assertEquals($user['headers']['status-code'], 200);
+        $this->assertNotEmpty($user['body']['$id']);
+
+        $project = $this->client->call(Client::METHOD_PATCH, '/projects/' . $projectId . '/auth/password-history', array_merge([
+            'content-type' => 'application/json',
+            'x-appwrite-project' => 'console',
+            'cookie' => 'a_session_console=' . $this->getRoot()['session'],
+            'x-appwrite-mode' => 'admin'
+        ]), [
+            'limit' => 2
+        ]);
+        $this->assertEquals(200, $project['headers']['status-code']);
+        $this->assertEquals(2, $project['body']['passwordHistory']);
+
+        $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' => 'password2',
+        ]);
+        $this->assertEquals($user['headers']['status-code'], 200);
+        $this->assertNotEmpty($user['body']['$id']);
+
+        $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' => 'password2',
+        ]);
+        $this->assertEquals(409, $user['headers']['status-code']);
+        $this->assertNotEmpty($user['body']['$id']);
+
+        return $data;
+    }
+
    /**
     * @depends testGetUser
     */
@ -953,7 +1013,7 @@ trait UsersBase
        $this->assertEquals($user['headers']['status-code'], 200);
        $this->assertEquals($user['body']['phone'], $updatedNumber);

-         /**
+        /**
         * Test for FAILURE
         */

@ -1023,7 +1083,7 @@ trait UsersBase
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
-            'queries' => [ 'limit(1)' ],
+            'queries' => ['limit(1)'],
        ]);

        $this->assertEquals($logs['headers']['status-code'], 200);
@ -1035,7 +1095,7 @@ trait UsersBase
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
-            'queries' => [ 'offset(1)' ],
+            'queries' => ['offset(1)'],
        ]);

        $this->assertEquals($logs['headers']['status-code'], 200);
@ -1046,7 +1106,7 @@ trait UsersBase
            'content-type' => 'application/json',
            'x-appwrite-project' => $this->getProject()['$id'],
        ], $this->getHeaders()), [
-            'queries' => [ 'limit(1)', 'offset(1)' ],
+            'queries' => ['limit(1)', 'offset(1)'],
        ]);

        $this->assertEquals($logs['headers']['status-code'], 200);