Merge pull request #110 from vatesfr/julien-f-fix-password-rehash

Fix user password rehash.

src/api/user.js

@@ -73,9 +73,11 @@ set.params = {
   permission: { type: 'string', optional: true }
 }
 
+// -------------------------------------------------------------------
+
 export async function changePassword ({oldPassword, newPassword}) {
   const id = this.session.get('user_id')
-  await this.changePassword(id, oldPassword, newPassword)
+  await this.changeUserPassword(id, oldPassword, newPassword)
 }
 
 changePassword.description = 'change password after checking old password (user function)'

src/index.js

@@ -35,7 +35,6 @@ import Api from './api'
 import JobExecutor from './job-executor'
 import RemoteHandler from './remote-handler'
 import Scheduler from './scheduler'
-import User from './models/user'
 import WebServer from 'http-server-plus'
 import wsProxy from './ws-proxy'
 import Xo from './xo'
@@ -510,11 +509,9 @@ const registerPasswordAuthenticationProvider = xo => {
     }
 
     const user = await xo.getUserByName(username, true)
-    if (!user || !(await User.checkPassword(user, password))) {
-      return
+    if (user && await xo.checkUserPassword(user.id, password)) {
+      return user.id
     }
-
-    return user.id
   }
 
   xo.registerAuthenticationProvider(passwordAuthenticationProvider)

src/models/user.js

@@ -1,5 +1,5 @@
 import forEach from 'lodash.foreach'
-import {hash, needsRehash, verify} from 'hashy'
+import { hash } from 'hashy'
 
 import Collection from '../collection/redis'
 import Model from '../model'
@@ -16,26 +16,6 @@ const PERMISSIONS = {
 // ===================================================================
 
 export default class User extends Model {
-  static async checkPassword (user, password) {
-    const hash = user.pw_hash
-
-    if (!(hash && await verify(password, hash))) {
-      return false
-    }
-
-    // There might be no hash if the user authenticate with another
-    // method (e.g. LDAP).
-    if (needsRehash(hash)) {
-      await this.setPassword(password)
-    }
-
-    return true
-  }
-
-  async checkPassword (password) {
-    return await User.checkPassword(this.properties, password)
-  }
-
   hasPermission (permission) {
     return PERMISSIONS[this.get('permission')] >= PERMISSIONS[permission]
   }

src/xo.js

@@ -18,6 +18,10 @@ import XoCollection from 'xo-collection'
 import XoUniqueIndex from 'xo-collection/unique-index'
 import {createClient as createRedisClient} from 'redis'
 import {EventEmitter} from 'events'
+import {
+  needsRehash,
+  verify
+} from 'hashy'
 
 import * as xapiObjectsToXo from './xapi-objects-to-xo'
 import checkAuthorization from './acl'
@@ -371,19 +375,28 @@ export default class Xo extends EventEmitter {
     })
   }
 
-  async changePassword (id, oldPassword, newPassword) {
-    const user = await this._getUser(id)
-
-    if (user.get('provider')) {
-      throw new Error('Password change is only for locally created users')
-    }
-
-    const auth = await user.checkPassword(oldPassword)
-    if (!auth) {
+  async changeUserPassword (userId, oldPassword, newPassword) {
+    if (!(await this.checkUserPassword(userId, oldPassword, false))) {
       throw new InvalidCredential()
     }
-    await user.setPassword(newPassword)
-    await this._users.save(user.properties)
+
+    await this.updateUser(userId, { password: newPassword })
+  }
+
+  async checkUserPassword (userId, password, updateIfNecessary = true) {
+    const { pw_hash: hash } = await this.getUser(userId)
+    if (!(
+      hash &&
+      await verify(password, hash)
+    )) {
+      return false
+    }
+
+    if (updateIfNecessary && needsRehash(hash)) {
+      await this.updateUser(userId, { password })
+    }
+
+    return true
   }
 
   // -----------------------------------------------------------------