From 0eae73e7b8c67095844c2abf419dacdd19454595 Mon Sep 17 00:00:00 2001 From: Damodar Lohani Date: Sun, 18 Dec 2022 06:27:41 +0000 Subject: [PATCH] fix: password comparison --- app/config/collections.php | 2 +- app/controllers/api/account.php | 20 +++-- app/controllers/api/projects.php | 2 +- app/controllers/api/users.php | 4 +- .../Utopia/Response/Model/Project.php | 7 ++ .../Projects/ProjectsConsoleClientTest.php | 85 +++++++++++++++++++ tests/e2e/Services/Users/UsersBase.php | 74 ++++++++++++++-- 7 files changed, 175 insertions(+), 19 deletions(-) diff --git a/app/config/collections.php b/app/config/collections.php index 16c0f32c4..cb4d6adf3 100644 --- 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'), diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 235c2ffbf..078d3ce0c 100644 --- 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); diff --git a/app/controllers/api/projects.php b/app/controllers/api/projects.php index ab741a797..f86374176 100644 --- 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) { diff --git a/app/controllers/api/users.php b/app/controllers/api/users.php index 33336c1ad..72e4b594f 100644 --- 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); diff --git a/src/Appwrite/Utopia/Response/Model/Project.php b/src/Appwrite/Utopia/Response/Model/Project.php index 28a9c86ff..a9e809c0a 100644 --- 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']; diff --git a/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php b/tests/e2e/Services/Projects/ProjectsConsoleClientTest.php index 889b73d61..504643f61 100644 --- 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([ diff --git a/tests/e2e/Services/Users/UsersBase.php b/tests/e2e/Services/Users/UsersBase.php index ac3b116b3..457b63a45 100644 --- 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);