From 19138c2d466b71cfe6050b34f8d43c04c80a4db2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Salgado?= Date: Mon, 19 Apr 2021 15:46:16 +0100 Subject: [PATCH] fix: redundant authentication code --- .../graphql/modules/authentication.js | 168 +++++++++--------- lib/new-admin/services/login.js | 15 +- lib/users.js | 12 +- 3 files changed, 100 insertions(+), 95 deletions(-) diff --git a/lib/new-admin/graphql/modules/authentication.js b/lib/new-admin/graphql/modules/authentication.js index f65aad3f..a02be3a4 100644 --- a/lib/new-admin/graphql/modules/authentication.js +++ b/lib/new-admin/graphql/modules/authentication.js @@ -9,7 +9,7 @@ const authErrors = require('../errors/authentication') const REMEMBER_ME_AGE = 90 * T.day -function authenticateUser(username, password) { +const authenticateUser = (username, password) => { return users.getUserByUsername(username) .then(user => { const hashedPassword = user.password @@ -26,26 +26,50 @@ function authenticateUser(username, password) { }) } +const destroySessionIfSameUser = (context, user) => { + const sessionUser = getUserFromCookie(context) + if (sessionUser && user.id === sessionUser.id) + context.req.session.destroy() +} + +const destroySessionIfBeingUsed = (sessID, context) => { + if (sessID === context.req.session.id) { + context.req.session.destroy() + } +} + +const getUserFromCookie = context => { + return context.req.session.user +} + +const getLamassuCookie = context => { + return context.req.cookies && context.req.cookies.lid +} + +const initializeSession = (context, user, rememberMe) => { + const finalUser = { id: user.id, username: user.username, role: user.role } + context.req.session.user = finalUser + if (rememberMe) context.req.session.cookie.maxAge = REMEMBER_ME_AGE +} + const getUserData = context => { - const lidCookie = context.req.cookies && context.req.cookies.lid + const lidCookie = getLamassuCookie(context) if (!lidCookie) return - const user = context.req.session.user + const user = getUserFromCookie(context) return user } const get2FASecret = (username, password) => { return authenticateUser(username, password).then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - const secret = otplib.authenticator.generateSecret() - const otpauth = otplib.authenticator.keyuri(username, 'Lamassu Industries', secret) + const otpauth = otplib.authenticator.keyuri(user.username, 'Lamassu Industries', secret) return { secret, otpauth } }) } const confirm2FA = (token, context) => { - const requestingUser = context.req.session.user + const requestingUser = getUserFromCookie(context) if (!requestingUser) throw new authErrors.InvalidCredentialsError() @@ -94,56 +118,38 @@ const validateReset2FALink = token => { } const deleteSession = (sessionID, context) => { - if (sessionID === context.req.session.id) { - context.req.session.destroy() - } + destroySessionIfBeingUsed(sessionID, context) return sessionManager.deleteSessionById(sessionID) } const login = (username, password) => { return authenticateUser(username, password).then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - return users.getUserById(user.id).then(user => { - const twoFASecret = user.twofa_code - return twoFASecret ? 'INPUT2FA' : 'SETUP2FA' - }) + const twoFASecret = user.twofa_code + return twoFASecret ? 'INPUT2FA' : 'SETUP2FA' }) } const input2FA = (username, password, rememberMe, code, context) => { return authenticateUser(username, password) - .then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - return users.getUserById(user.id) - }) .then(user => { const secret = user.twofa_code const isCodeValid = otplib.authenticator.verify({ token: code, secret: secret }) if (!isCodeValid) throw new authErrors.InvalidTwoFactorError() - const finalUser = { id: user.id, username: user.username, role: user.role } - context.req.session.user = finalUser - if (rememberMe) context.req.session.cookie.maxAge = REMEMBER_ME_AGE - + initializeSession(context, user, rememberMe) return true }) } const setup2FA = (username, password, rememberMe, secret, codeConfirmation, context) => { + if (!secret) throw new authErrors.InvalidTwoFactorError() + + const isCodeValid = otplib.authenticator.verify({ token: codeConfirmation, secret: secret }) + if (!isCodeValid) throw new authErrors.InvalidTwoFactorError() + return authenticateUser(username, password) .then(user => { - if (!user || !secret) throw new authErrors.InvalidCredentialsError() - - const isCodeValid = otplib.authenticator.verify({ token: codeConfirmation, secret: secret }) - if (!isCodeValid) throw new authErrors.InvalidTwoFactorError() - - return users.getUserById(user.id) - }) - .then(user => { - const finalUser = { id: user.id, username: user.username, role: user.role } - context.req.session.user = finalUser - if (rememberMe) context.req.session.cookie.maxAge = REMEMBER_ME_AGE - + initializeSession(context, user, rememberMe) return users.save2FASecret(user.id, secret) }) .then(() => true) @@ -152,70 +158,71 @@ const setup2FA = (username, password, rememberMe, secret, codeConfirmation, cont const changeUserRole = (code, id, newRole, context) => { const action = (id, newRole) => users.changeUserRole(id, newRole) - if (!code) { - return action(id, newRole) - } - - return confirm2FA(code, context) + return users.getUserById(id) + .then(user => { + if (user.role !== 'superuser') { + return action(id, newRole) + } + + return confirm2FA(code, context) + }) .then(() => action(id, newRole)) } const enableUser = (code, id, context) => { const action = id => users.enableUser(id) - if (!code) { - return action(id) - } - - return confirm2FA(code, context) + return users.getUserById(id) + .then(user => { + if (user.role !== 'superuser') { + return action(id) + } + + return confirm2FA(code, context) + }) .then(() => action(id)) } const disableUser = (code, id, context) => { const action = id => users.disableUser(id) - if (!code) { - return action(id) - } - - return confirm2FA(code, context) + return users.getUserById(id) + .then(user => { + if (user.role !== 'superuser') { + return action(id) + } + + return confirm2FA(code, context) + }) .then(() => action(id)) } const createResetPasswordToken = (code, userID, context) => { - const action = userID => { - return users.getUserById(userID) - .then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - return users.createAuthToken(user.id, 'reset_password') - }) - .catch(err => console.error(err)) - } + const action = user => users.createAuthToken(user.id, 'reset_password') - if (!code) { - return action(userID) - } - - return confirm2FA(code, context) - .then(() => action(userID)) + return users.getUserById(userID) + .then(user => { + if (user.role !== 'superuser') { + return action(user.id) + } + + return confirm2FA(code, context) + .then(() => action(user)) + }) } const createReset2FAToken = (code, userID, context) => { - const action = userID => { - return users.getUserById(userID) - .then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - return users.createAuthToken(user.id, 'reset_twofa') - }) - .catch(err => console.error(err)) - } + const action = user => users.createAuthToken(user.id, 'reset_twofa') - if (!code) { - return action(userID) - } - - return confirm2FA(code, context) - .then(() => action(userID)) + return users.getUserById(userID) + .then(user => { + if (user.role !== 'superuser') { + return action(user) + } + + return confirm2FA(code, context) + .then(() => action(user)) + }) } const createRegisterToken = (username, role) => { @@ -240,8 +247,7 @@ const register = (token, username, password, role) => { const resetPassword = (token, userID, newPassword, context) => { return users.getUserById(userID) .then(user => { - if (!user) throw new authErrors.InvalidCredentialsError() - if (context.req.session.user && user.id === context.req.session.user.id) context.req.session.destroy() + destroySessionIfSameUser(context, user) return users.updatePassword(token, user.id, newPassword) }) .then(() => true) @@ -254,7 +260,7 @@ const reset2FA = (token, userID, code, secret, context) => { return users.getUserById(userID) .then(user => { - if (context.req.session.user && user.id === context.req.session.user.id) context.req.session.destroy() + destroySessionIfSameUser(context, user) return users.reset2FASecret(token, user.id, secret).then(() => true) }) .catch(err => console.error(err)) diff --git a/lib/new-admin/services/login.js b/lib/new-admin/services/login.js index 6d43462a..4895997c 100644 --- a/lib/new-admin/services/login.js +++ b/lib/new-admin/services/login.js @@ -1,15 +1,14 @@ const db = require('../../db') function validateUser (username, password) { - const sql = 'SELECT id, username FROM users WHERE username=$1 AND password=$2' - const sqlUpdateLastAccessed = 'UPDATE users SET last_accessed = now() WHERE username=$1' + return db.tx(t => { + const q1 = t.one('SELECT * FROM users WHERE username=$1 AND password=$2', [username, password]) + const q2 = t.none('UPDATE users SET last_accessed = now() WHERE username=$1', [username]) - return db.one(sql, [username, password]) - .then(user => { - return db.none(sqlUpdateLastAccessed, [user.username]) - .then(() => user) - }) - .catch(() => false) + return t.batch([q1, q2]) + .then(([user, _]) => user) + .catch(() => false) + }) } module.exports = { diff --git a/lib/users.js b/lib/users.js index bfb29c48..0fe7796a 100644 --- a/lib/users.js +++ b/lib/users.js @@ -54,14 +54,14 @@ function getUsers () { function verifyAndUpdateUser (id, ua, ip) { const sql = `SELECT id, username, role, enabled FROM users WHERE id=$1 limit 1` - return db.oneOrNone(sql, [id]).then(user => { - if (!user) return null + return db.oneOrNone(sql, [id]) + .then(user => { + if (!user) return null - const sql2 = `UPDATE users SET last_accessed=now(), last_accessed_from=$1, last_accessed_address=$2 WHERE id=$3 RETURNING id, role, enabled` - return db.one(sql2, [ua, ip, id]).then(user => { - return user + const sql2 = `UPDATE users SET last_accessed=now(), last_accessed_from=$1, last_accessed_address=$2 WHERE id=$3 RETURNING id, role, enabled` + return db.one(sql2, [ua, ip, id]) }) - }) + .then(user => user) } function save2FASecret (id, secret) {