diff --git a/src/routes/users.js b/src/routes/users.js index 4752c92f5..b30c394d8 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -5,7 +5,7 @@ exports = module.exports = { update, list, create, - remove, + del, changePassword, verifyPassword, createInvite, @@ -18,11 +18,12 @@ exports = module.exports = { load }; -var assert = require('assert'), +const assert = require('assert'), auditSource = require('../auditsource.js'), BoxError = require('../boxerror.js'), HttpError = require('connect-lastmile').HttpError, HttpSuccess = require('connect-lastmile').HttpSuccess, + safe = require('safetydance'), users = require('../users.js'); function load(req, res, next) { @@ -117,17 +118,16 @@ function get(req, res, next) { next(new HttpSuccess(200, users.removePrivateFields(req.resource))); } -function remove(req, res, next) { +async function del(req, res, next) { assert.strictEqual(typeof req.resource, 'object'); if (req.user.id === req.resource.id) return next(new HttpError(409, 'Not allowed to remove yourself.')); if (users.compareRoles(req.user.role, req.resource.role) < 0) return next(new HttpError(403, `role '${req.resource.role}' is required but user has only '${req.user.role}'`)); - users.remove(req.resource, auditSource.fromRequest(req), function (error) { - if (error) return next(BoxError.toHttpError(error)); + const [error] = await safe(users.remove(req.resource, auditSource.fromRequest(req))); + if (error) return next(BoxError.toHttpError(error)); - next(new HttpSuccess(204)); - }); + next(new HttpSuccess(204)); } function verifyPassword(req, res, next) { diff --git a/src/server.js b/src/server.js index 95423e6b8..e87a1cd0e 100644 --- a/src/server.js +++ b/src/server.js @@ -171,7 +171,7 @@ function initializeExpressSync() { router.get ('/api/v1/users', token, authorizeUserManager, routes.users.list); router.post('/api/v1/users', json, token, authorizeUserManager, routes.users.create); router.get ('/api/v1/users/:userId', token, authorizeUserManager, routes.users.load, routes.users.get); // this is manage scope because it returns non-restricted fields - router.del ('/api/v1/users/:userId', token, authorizeUserManager, routes.users.load, routes.users.remove); + router.del ('/api/v1/users/:userId', token, authorizeUserManager, routes.users.load, routes.users.del); router.post('/api/v1/users/:userId', json, token, authorizeUserManager, routes.users.load, routes.users.update); router.post('/api/v1/users/:userId/password', json, token, authorizeUserManager, routes.users.load, routes.users.changePassword); router.put ('/api/v1/users/:userId/groups', json, token, authorizeUserManager, routes.users.load, routes.users.setGroups); diff --git a/src/test/apppasswords-test.js b/src/test/apppasswords-test.js index a889261a1..53b574a9c 100644 --- a/src/test/apppasswords-test.js +++ b/src/test/apppasswords-test.js @@ -10,7 +10,8 @@ const appPasswords = require('../apppasswords.js'), BoxError = require('../boxerror.js'), common = require('./common.js'), expect = require('expect.js'), - safe = require('safetydance'); + safe = require('safetydance'), + users = require('../users.js'); describe('App passwords', function () { const { setup, cleanup, ADMIN } = common; @@ -18,7 +19,7 @@ describe('App passwords', function () { before(setup); after(cleanup); - let id; + let id, password; it('cannot add bad app password', async function () { const [error] = await safe(appPasswords.add(ADMIN.id, 'appid', 'x'.repeat(201))); expect(error.reason).to.be(BoxError.BAD_FIELD); @@ -29,6 +30,7 @@ describe('App passwords', function () { expect(result.id).to.be.a('string'); expect(result.password).to.be.a('string'); id = result.id; + password = result.password; }); it('can get app password', async function () { @@ -51,10 +53,58 @@ describe('App passwords', function () { expect(results[0].identifier).to.be('appid'); }); + it('can verify app password', function (done) { + users.verify(ADMIN.id, password, 'appid', function (error, result) { + expect(error).to.not.be.ok(); + expect(result).to.be.ok(); + expect(result.appPassword).to.be(true); + + done(); + }); + }); + + it('can verify non-app password', function (done) { + users.verify(ADMIN.id, ADMIN.password, 'appid', function (error, result) { + expect(error).to.not.be.ok(); + expect(result).to.be.ok(); + expect(result.appPassword).to.be(undefined); + + done(); + }); + }); + + it('cannot verify bad password', function (done) { + users.verify(ADMIN.id, 'bad', 'appid', function (error, result) { + expect(error).to.be.ok(); + expect(result).to.not.be.ok(); + expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); + + done(); + }); + }); + + it('cannot verify password for another app', function (done) { + users.verify(ADMIN.id, password, 'appid2', function (error, result) { + expect(error).to.be.ok(); + expect(result).to.not.be.ok(); + expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); + + done(); + }); + }); + it('can del app password', async function () { await appPasswords.del(id); }); + it('cannot verify deleted app password', function (done) { + users.verify(ADMIN.id, password, 'appid', function (error) { + expect(error).to.be.ok(); + + done(); + }); + }); + it('cannot del random app password', async function () { const [error] = await safe(appPasswords.del('random')); expect(error.reason).to.be(BoxError.NOT_FOUND); diff --git a/src/test/users-test.js b/src/test/users-test.js index f6df12bbe..810abd132 100644 --- a/src/test/users-test.js +++ b/src/test/users-test.js @@ -5,7 +5,7 @@ 'use strict'; -var async = require('async'), +const async = require('async'), BoxError = require('../boxerror.js'), database = require('../database.js'), expect = require('expect.js'), @@ -16,6 +16,7 @@ var async = require('async'), mailer = require('../mailer.js'), paths = require('../paths.js'), provision = require('../provision.js'), + safe = require('safetydance'), userdb = require('../userdb.js'), users = require('../users.js'), _ = require('underscore'); @@ -530,94 +531,6 @@ describe('User', function () { }); }); - describe('appPasswords', function () { - before(createOwner); - after(cleanupUsers); - let pwd; - - it('can add app password', function (done) { - users.addAppPassword(userObject.id, 'appid', 'rpi', function (error, result) { - expect(error).to.be(null); - pwd = result; - done(); - }); - }); - - it('can get app passwords', function (done) { - users.getAppPasswords(userObject.id, function (error, result) { - expect(error).to.be(null); - expect(result.length).to.be(1); - expect(result[0].name).to.be('rpi'); - expect(result[0].identifier).to.be('appid'); - expect(result[0].hashedPassword).to.be(undefined); - done(); - }); - }); - - it('can get app password', function (done) { - users.getAppPassword(pwd.id, function (error, result) { - expect(error).to.be(null); - expect(result.name).to.be('rpi'); - expect(result.identifier).to.be('appid'); - expect(result.hashedPassword).to.be(undefined); - done(); - }); - }); - - it('can verify app password', function (done) { - users.verify(userObject.id, pwd.password, 'appid', function (error, result) { - expect(error).to.not.be.ok(); - expect(result).to.be.ok(); - expect(result.appPassword).to.be(true); - - done(); - }); - }); - - it('can verify non-app password', function (done) { - users.verify(userObject.id, PASSWORD, 'appid', function (error, result) { - expect(error).to.not.be.ok(); - expect(result).to.be.ok(); - expect(result.appPassword).to.be(undefined); - - done(); - }); - }); - - it('cannot verify bad password', function (done) { - users.verify(userObject.id, 'bad', 'appid', function (error, result) { - expect(error).to.be.ok(); - expect(result).to.not.be.ok(); - expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); - - done(); - }); - }); - - it('cannot verify password for another app', function (done) { - users.verify(userObject.id, pwd.password, 'appid2', function (error, result) { - expect(error).to.be.ok(); - expect(result).to.not.be.ok(); - expect(error.reason).to.be(BoxError.INVALID_CREDENTIALS); - - done(); - }); - }); - - it('can del app password', function (done) { - users.delAppPassword(pwd.id, function (error) { - if (error) return done(error); - - // cannot verify anymore - users.verify(userObject.id, pwd.password, 'appid', function (error) { - expect(error).to.be.ok(); - - done(); - }); - }); - }); - }); - describe('retrieving', function () { before(createOwner); after(cleanupUsers); @@ -909,18 +822,13 @@ describe('User', function () { before(createOwner); after(cleanupUsers); - it('fails for unknown user', function (done) { - users.remove(_.extend({}, userObject, { id: 'unknown' }), AUDIT_SOURCE, function (error) { - expect(error.reason).to.be(BoxError.NOT_FOUND); - done(); - }); + it('fails for unknown user', async function () { + const [error] = await safe(users.del(_.extend({}, userObject, { id: 'unknown' }), AUDIT_SOURCE)); + expect(error.reason).to.be(BoxError.NOT_FOUND); }); - it('can remove valid user', function (done) { - users.remove(userObject, AUDIT_SOURCE, function (error) { - expect(!error).to.be.ok(); - done(); - }); + it('can remove valid user', async function () { + await users.del(userObject, AUDIT_SOURCE); }); it('can re-create user after user was removed', createOwner); diff --git a/src/userdb.js b/src/userdb.js index 85f76f2a8..5c3f0b3e6 100644 --- a/src/userdb.js +++ b/src/userdb.js @@ -10,7 +10,6 @@ exports = module.exports = { getAllWithGroupIdsPaged, getByRole, add, - del, update, count, @@ -182,26 +181,6 @@ function add(userId, user, callback) { }); } -function del(userId, callback) { - assert.strictEqual(typeof userId, 'string'); - assert.strictEqual(typeof callback, 'function'); - - // also cleanup the groupMembers table - var queries = []; - queries.push({ query: 'DELETE FROM groupMembers WHERE userId = ?', args: [ userId ] }); - queries.push({ query: 'DELETE FROM tokens WHERE identifier = ?', args: [ userId ] }); - queries.push({ query: 'DELETE FROM appPasswords WHERE userId = ?', args: [ userId ] }); - queries.push({ query: 'DELETE FROM users WHERE id = ?', args: [ userId ] }); - - database.transaction(queries, function (error, result) { - if (error && error.code === 'ER_NO_REFERENCED_ROW_2') return callback(new BoxError(BoxError.NOT_FOUND, error)); - if (error) return callback(new BoxError(BoxError.DATABASE_ERROR, error)); - if (result[3].affectedRows !== 1) return callback(new BoxError(BoxError.NOT_FOUND, 'User not found')); - - callback(error); - }); -} - function getByAccessToken(accessToken, callback) { assert.strictEqual(typeof accessToken, 'string'); assert.strictEqual(typeof callback, 'function'); diff --git a/src/users.js b/src/users.js index 4fc39b674..9d4d4aaf1 100644 --- a/src/users.js +++ b/src/users.js @@ -11,7 +11,7 @@ exports = module.exports = { verify, verifyWithUsername, verifyWithEmail, - remove, + del, get, getByResetToken, getByUsername, @@ -314,20 +314,24 @@ function verifyWithEmail(email, password, identifier, callback) { }); } -function remove(user, auditSource, callback) { +async function del(user, auditSource) { assert.strictEqual(typeof user, 'object'); assert(auditSource && typeof auditSource === 'object'); - assert.strictEqual(typeof callback, 'function'); - if (settings.isDemo() && user.username === constants.DEMO_USERNAME) return callback(new BoxError(BoxError.BAD_FIELD, 'Not allowed in demo mode')); + if (settings.isDemo() && user.username === constants.DEMO_USERNAME) throw new BoxError(BoxError.BAD_FIELD, 'Not allowed in demo mode'); - userdb.del(user.id, function (error) { - if (error) return callback(error); + const queries = []; + queries.push({ query: 'DELETE FROM groupMembers WHERE userId = ?', args: [ user.id ] }); + queries.push({ query: 'DELETE FROM tokens WHERE identifier = ?', args: [ user.id ] }); + queries.push({ query: 'DELETE FROM appPasswords WHERE userId = ?', args: [ user.id ] }); + queries.push({ query: 'DELETE FROM users WHERE id = ?', args: [ user.id ] }); - eventlog.add(eventlog.ACTION_USER_REMOVE, auditSource, { userId: user.id, user: removePrivateFields(user) }); + const [error, result] = await safe(database.transaction(queries)); + if (error && error.code === 'ER_NO_REFERENCED_ROW_2') throw new BoxError(BoxError.NOT_FOUND, error); + if (error) throw error; + if (result[3].affectedRows !== 1) throw new BoxError(BoxError.NOT_FOUND, 'User not found'); - callback(); - }); + await safe(eventlog.add(eventlog.ACTION_USER_REMOVE, auditSource, { userId: user.id, user: removePrivateFields(user) })); } function getAll(callback) {