diff --git a/src/directoryserver.js b/src/directoryserver.js index f64953e2b..69e277254 100644 --- a/src/directoryserver.js +++ b/src/directoryserver.js @@ -314,7 +314,7 @@ async function userAuth(req, res, next) { } else if (commonName.indexOf('@') !== -1) { // if mail is specified, enforce mail check verifyFunc = users.verifyWithEmail; } else if (commonName.indexOf('uid-') === 0) { - verifyFunc = users.verify; + verifyFunc = users.verifyWithId; } else { verifyFunc = users.verifyWithUsername; } diff --git a/src/ldapserver.js b/src/ldapserver.js index 623508bfa..3109077aa 100644 --- a/src/ldapserver.js +++ b/src/ldapserver.js @@ -59,7 +59,7 @@ async function userAuthInternal(appId, req, res, next) { } else if (commonName.indexOf('@') !== -1) { // if mail is specified, enforce mail check verifyFunc = users.verifyWithEmail; } else if (commonName.indexOf('uid-') === 0) { - verifyFunc = users.verify; + verifyFunc = users.verifyWithId; } else { verifyFunc = users.verifyWithUsername; } @@ -458,13 +458,13 @@ async function verifyMailboxPassword(mailbox, password) { assert.strictEqual(typeof password, 'string'); if (mailbox.ownerType === mail.OWNERTYPE_USER) { - return await users.verify(mailbox.ownerId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true }); + return await users.verifyWithId(mailbox.ownerId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true }); } else if (mailbox.ownerType === mail.OWNERTYPE_GROUP) { const userIds = await groups.getMemberIds(mailbox.ownerId); let verifiedUser = null; for (const userId of userIds) { - const [error, result] = await safe(users.verify(userId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true })); + const [error, result] = await safe(users.verifyWithId(userId, password, users.AP_MAIL /* identifier */, { skipTotpCheck: true })); if (error) continue; // try the next user verifiedUser = result; break; // found a matching validated user diff --git a/src/routes/accesscontrol.js b/src/routes/accesscontrol.js index 6fd30efbb..df0198fd5 100644 --- a/src/routes/accesscontrol.js +++ b/src/routes/accesscontrol.js @@ -9,11 +9,12 @@ exports = module.exports = { }; const apps = require('../apps.js'), - tokens = require('../tokens.js'), assert = require('assert'), BoxError = require('../boxerror.js'), + debug = require('debug')('box:routes/accesscontrol'), HttpError = require('@cloudron/connect-lastmile').HttpError, safe = require('safetydance'), + tokens = require('../tokens.js'), users = require('../users.js'); async function passwordAuth(req, res, next) { @@ -60,7 +61,10 @@ async function tokenAuth(req, res, next) { const user = await users.get(token.identifier); if (!user) return next(new HttpError(401, 'User not found')); - if (!user.active) return next(new HttpError(401, 'User not active')); + if (!user.active) { + debug(`tokenAuth: ${user.username || user.id} is not active`); + return next(new HttpError(401, 'User not active')); + } const remoteAddress = req.headers['x-forwarded-for'] || req.socket.remoteAddress; if (!tokens.isIpAllowedSync(token, remoteAddress)) return next(new HttpError(401, 'Token not allowed from this IP')); diff --git a/src/routes/test/users-test.js b/src/routes/test/users-test.js index f39d14500..9a4f5df6e 100644 --- a/src/routes/test/users-test.js +++ b/src/routes/test/users-test.js @@ -175,7 +175,7 @@ describe('Users API', function () { }); it('did set password of created user', async function () { - await users.verify(userWithPassword.id, userWithPassword.password, users.AP_WEBADMIN, {}); + await users.verifyWithId(userWithPassword.id, userWithPassword.password, users.AP_WEBADMIN, {}); }); }); @@ -410,7 +410,7 @@ describe('Users API', function () { }); it('did change the user password', async function () { - await users.verify(user.id, 'bigenough', users.AP_WEBADMIN, {}); + await users.verifyWithId(user.id, 'bigenough', users.AP_WEBADMIN, {}); }); }); diff --git a/src/routes/users.js b/src/routes/users.js index 42b3df412..763089e0f 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -196,7 +196,7 @@ async function verifyPassword(req, res, next) { if (typeof req.body.password !== 'string') return next(new HttpError(400, 'API call requires user password')); - const [error] = await safe(users.verify(req.user.id, req.body.password, users.AP_WEBADMIN, { skipTotpCheck: true })); + const [error] = await safe(users.verifyWithId(req.user.id, req.body.password, users.AP_WEBADMIN, { skipTotpCheck: true })); if (error) return next(BoxError.toHttpError(error)); req.body.password = ''; // this will prevent logs from displaying plain text password diff --git a/src/test/apppasswords-test.js b/src/test/apppasswords-test.js index b2fa21a82..6c0d9c597 100644 --- a/src/test/apppasswords-test.js +++ b/src/test/apppasswords-test.js @@ -54,25 +54,25 @@ describe('App passwords', function () { }); it('can verify app password', async function () { - const result = await users.verify(admin.id, password, 'appid', {}); + const result = await users.verifyWithId(admin.id, password, 'appid', {}); expect(result).to.be.ok(); expect(result.appPassword).to.be(true); }); it('can verify non-app password', async function () { - const result = await users.verify(admin.id, admin.password, 'appid', {}); + const result = await users.verifyWithId(admin.id, admin.password, 'appid', {}); expect(result).to.be.ok(); expect(result.appPassword).to.be(undefined); }); it('cannot verify bad password', async function () { - const [error, result] = await safe(users.verify(admin.id, 'bad', 'appid', {})); + const [error, result] = await safe(users.verifyWithId(admin.id, 'bad', 'appid', {})); expect(result).to.not.be.ok(); expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); }); it('cannot verify password for another app', async function () { - const [error, result] = await safe(users.verify(admin.id, password, 'appid2', {})); + const [error, result] = await safe(users.verifyWithId(admin.id, password, 'appid2', {})); expect(result).to.not.be.ok(); expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); }); @@ -82,7 +82,7 @@ describe('App passwords', function () { }); it('cannot verify deleted app password', async function () { - const [error] = await safe(users.verify(admin.id, password, 'appid', {})); + const [error] = await safe(users.verifyWithId(admin.id, password, 'appid', {})); expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); }); diff --git a/src/test/users-test.js b/src/test/users-test.js index 7447f3c60..5e68daebf 100644 --- a/src/test/users-test.js +++ b/src/test/users-test.js @@ -245,43 +245,43 @@ describe('User', function () { before(createOwner); it('fails due to non existing user', async function () { - const [error] = await safe(users.verify('somerandomid', 'somepassword', users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId('somerandomid', 'somepassword', users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.NOT_FOUND); }); it('fails due to empty password', async function () { - const [error] = await safe(users.verify(admin.id, '', users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, '', users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); }); it('fails due to wrong password', async function () { - const [error] = await safe(users.verify(admin.id, admin.password+'x', users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, admin.password+'x', users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); }); it('succeeds', async function () { - const result = await users.verify(admin.id, admin.password, users.AP_WEBADMIN, {}); + const result = await users.verifyWithId(admin.id, admin.password, users.AP_WEBADMIN, {}); expect(result).to.be.ok(); expect(result.appPassword).to.not.be.ok(); expect(result.ghost).to.not.be.ok(); }); it('fails for ghost if not enabled', async function () { - const [error] = await safe(users.verify(admin.id, 'foobar', users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, 'foobar', users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); }); it('fails for ghost with wrong password', async function () { await users.setGhost(admin, 'testpassword', 0); - const [error] = await safe(users.verify(admin.id, 'foobar', users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, 'foobar', users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); }); it('succeeds for ghost', async function () { await users.setGhost(admin, 'testpassword', 0); - const result = await users.verify(admin.id, 'testpassword', users.AP_WEBADMIN, {}); + const result = await users.verifyWithId(admin.id, 'testpassword', users.AP_WEBADMIN, {}); expect(result.id).to.equal(admin.id); expect(result.ghost).to.be(true); }); @@ -289,7 +289,7 @@ describe('User', function () { it('succeeds for normal user password when ghost file exists', async function () { await users.setGhost(admin, 'testpassword', 0); - const result = await users.verify(admin.id, admin.password, users.AP_WEBADMIN, {}); + const result = await users.verifyWithId(admin.id, admin.password, users.AP_WEBADMIN, {}); expect(result.id).to.equal(admin.id); expect(result.ghost).to.not.be.ok(); }); @@ -449,13 +449,13 @@ describe('User', function () { it('verify fails for inactive user', async function () { await users.update(admin, { active: false }, auditSource); - const [error] = await safe(users.verify(admin.id, admin.password, users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, admin.password, users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.NOT_FOUND); }); it('verify succeeds for inactive user', async function () { await users.update(admin, { active: true }, auditSource); - await users.verify(admin.id, admin.password, users.AP_WEBADMIN, {}); + await users.verifyWithId(admin.id, admin.password, users.AP_WEBADMIN, {}); }); }); @@ -517,12 +517,12 @@ describe('User', function () { }); it('actually changed the password (unable to login with old pasword)', async function () { - const [error] = await safe(users.verify(admin.id, admin.password, users.AP_WEBADMIN, {})); + const [error] = await safe(users.verifyWithId(admin.id, admin.password, users.AP_WEBADMIN, {})); expect(error.reason).to.equal(BoxError.INVALID_CREDENTIALS); }); it('actually changed the password (login with new password)', async function () { - await users.verify(admin.id, 'ThisIsNew1Password', users.AP_WEBADMIN, {}); + await users.verifyWithId(admin.id, 'ThisIsNew1Password', users.AP_WEBADMIN, {}); }); }); diff --git a/src/users.js b/src/users.js index 7d4d65be1..3eafdcf9a 100644 --- a/src/users.js +++ b/src/users.js @@ -18,7 +18,7 @@ exports = module.exports = { getAdmins, getSuperadmins, - verify, + verifyWithId, verifyWithUsername, verifyWithEmail, @@ -357,21 +357,23 @@ async function verifyAppPassword(userId, password, identifier) { } // identifier is only used to check if password is valid for a specific app -async function verify(userId, password, identifier, options) { - assert.strictEqual(typeof userId, 'string'); +async function verify(user, password, identifier, options) { + assert.strictEqual(typeof user, 'object'); assert.strictEqual(typeof password, 'string'); assert.strictEqual(typeof identifier, 'string'); assert.strictEqual(typeof options, 'object'); - const user = await get(userId); - if (!user) throw new BoxError(BoxError.NOT_FOUND, 'User not found'); - if (!user.active) throw new BoxError(BoxError.NOT_FOUND, 'User not active'); + if (!user.active) { + debug(`verify: ${user.username} is not active`); + throw new BoxError(BoxError.NOT_FOUND, 'User not active'); + } // for just invited users the username may be still null if (user.username) { const valid = await verifyGhost(user.username, password); if (valid) { + debug(`verify: ${user.username} authenticated via impersonation`); user.ghost = true; return user; } @@ -379,6 +381,7 @@ async function verify(userId, password, identifier, options) { const [error] = await safe(verifyAppPassword(user.id, password, identifier)); if (!error) { // matched app password + debug(`verify: ${user.username || user.id} matched app password`); user.appPassword = true; return user; } @@ -394,7 +397,10 @@ async function verify(userId, password, identifier, options) { if (error) throw new BoxError(BoxError.CRYPTO_ERROR, error); const derivedKeyHex = Buffer.from(derivedKey, 'binary').toString('hex'); - if (derivedKeyHex !== user.password) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Wrong password'); + if (derivedKeyHex !== user.password) { + debug(`verify: ${user.username || user.id} provided incorrect password`); + throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Wrong password'); + } localTotpCheck = user.twoFactorAuthenticationEnabled; } @@ -405,9 +411,25 @@ async function verify(userId, password, identifier, options) { if (!verified) throw new BoxError(BoxError.INVALID_CREDENTIALS, 'Invalid totpToken'); } + debug(`verify: ${user.username || user.id} authenticated`); return user; } +async function verifyWithId(id, password, identifier, options) { + assert.strictEqual(typeof id, 'string'); + assert.strictEqual(typeof password, 'string'); + assert.strictEqual(typeof identifier, 'string'); + assert.strictEqual(typeof options, 'object'); + + const user = await get(id); + if (!user) { + debug(`verifyWithId: ${id} not found`); + throw new BoxError(BoxError.NOT_FOUND, 'User not found'); + } + + return await verify(user, password, identifier, options); +} + async function verifyWithUsername(username, password, identifier, options) { assert.strictEqual(typeof username, 'string'); assert.strictEqual(typeof password, 'string'); @@ -415,16 +437,19 @@ async function verifyWithUsername(username, password, identifier, options) { assert.strictEqual(typeof options, 'object'); const user = await getByUsername(username.toLowerCase()); - if (user) return await verify(user.id, password, identifier, options); + if (user) return await verify(user, password, identifier, options); const [error, newUserId] = await safe(externalLdap.maybeCreateUser(username.toLowerCase())); - if (error && error.reason === BoxError.BAD_STATE) throw new BoxError(BoxError.NOT_FOUND, 'User not found'); // no external ldap or no auto create + if (error && error.reason === BoxError.BAD_STATE) { + debug(`verifyWithUsername: ${username} not found`); + throw new BoxError(BoxError.NOT_FOUND, 'User not found'); // no external ldap or no auto create + } if (error) { debug(`verifyWithUsername: failed to auto create user ${username}. %o`, error); throw new BoxError(BoxError.NOT_FOUND, 'User not found'); } - return await verify(newUserId, password, identifier, options); + return await verifyWithId(newUserId, password, identifier, options); } async function verifyWithEmail(email, password, identifier, options) { @@ -434,9 +459,12 @@ async function verifyWithEmail(email, password, identifier, options) { assert.strictEqual(typeof options, 'object'); const user = await getByEmail(email.toLowerCase()); - if (!user) throw new BoxError(BoxError.NOT_FOUND, 'User not found'); + if (!user) { + debug(`verifyWithEmail: ${email} no such user`); + throw new BoxError(BoxError.NOT_FOUND, 'User not found'); + } - return await verify(user.id, password, identifier, options); + return await verify(user, password, identifier, options); } async function del(user, auditSource) {