From 76c143e8b09f812562caf71d44d4780598d47035 Mon Sep 17 00:00:00 2001
From: Laurent Cozic <laurent@cozic.net>
Date: Sun, 14 Mar 2021 18:43:20 +0000
Subject: [PATCH] Server: Prevent new user password from being hashed twice

---
 packages/server/src/models/UserModel.ts        | 9 ++++++++-
 packages/server/src/routes/index/users.test.ts | 9 +++++++++
 packages/server/src/routes/index/users.ts      | 3 +--
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/packages/server/src/models/UserModel.ts b/packages/server/src/models/UserModel.ts
index 0c62d3ed82..a97e22124e 100644
--- a/packages/server/src/models/UserModel.ts
+++ b/packages/server/src/models/UserModel.ts
@@ -100,12 +100,19 @@ export default class UserModel extends BaseModel<User> {
 		}, 'UserModel::delete');
 	}
 
+	// Note that when the "password" property is provided, it is going to be
+	// hashed automatically. It means that it is not safe to do:
+	//
+	//     const user = await model.load(id);
+	//     await model.save(user);
+	//
+	// Because the password would be hashed twice.
 	public async save(object: User, options: SaveOptions = {}): Promise<User> {
 		const isNew = await this.isNew(object, options);
 
 		let newUser = { ...object };
 
-		if (isNew && newUser.password) newUser.password = auth.hashPassword(newUser.password);
+		if (newUser.password) newUser.password = auth.hashPassword(newUser.password);
 
 		await this.withTransaction(async () => {
 			newUser = await super.save(newUser, options);
diff --git a/packages/server/src/routes/index/users.test.ts b/packages/server/src/routes/index/users.test.ts
index b94c974aee..67bed05f85 100644
--- a/packages/server/src/routes/index/users.test.ts
+++ b/packages/server/src/routes/index/users.test.ts
@@ -92,6 +92,15 @@ describe('index_users', function() {
 		expect(!!rootFile.id).toBe(true);
 	});
 
+	test('new user should be able to login', async function() {
+		const { session } = await createUserAndSession(1, true);
+
+		await postUser(session.id, 'test@example.com', '123456');
+		const loggedInUser = await models().user().login('test@example.com', '123456');
+		expect(!!loggedInUser).toBe(true);
+		expect(loggedInUser.email).toBe('test@example.com');
+	});
+
 	test('should not create anything, neither user, root file nor permissions, if user creation fail', async function() {
 		const { user, session } = await createUserAndSession(1, true);
 
diff --git a/packages/server/src/routes/index/users.ts b/packages/server/src/routes/index/users.ts
index 44387a61c6..5703ffcc57 100644
--- a/packages/server/src/routes/index/users.ts
+++ b/packages/server/src/routes/index/users.ts
@@ -7,7 +7,6 @@ import { User } from '../../db';
 import config from '../../config';
 import { View } from '../../services/MustacheService';
 import defaultView from '../../utils/defaultView';
-import { hashPassword } from '../../utils/auth';
 
 function makeUser(isNew: boolean, fields: any): User {
 	const user: User = {};
@@ -17,7 +16,7 @@ function makeUser(isNew: boolean, fields: any): User {
 
 	if (fields.password) {
 		if (fields.password !== fields.password2) throw new ErrorUnprocessableEntity('Passwords do not match');
-		user.password = hashPassword(fields.password);
+		user.password = fields.password;
 	}
 
 	if (!isNew) user.id = fields.id;