diff --git a/migrations/20180726213634-users-add-admin.js b/migrations/20180726213634-users-add-admin.js new file mode 100644 index 000000000..8dcd89e2d --- /dev/null +++ b/migrations/20180726213634-users-add-admin.js @@ -0,0 +1,32 @@ +'use strict'; + +exports.up = function(db, callback) { + db.runSql('ALTER TABLE users ADD COLUMN admin BOOLEAN DEFAULT 0', function (error) { + if (error) return callback(error); + + db.all('SELECT userId FROM groupMembers WHERE groupId=?', [ 'admin' ], function (error, results) { + if (error) return callback(error); + + if (results.length === 0) return callback(); + + async.eachSeries(results, function (userId, iteratorDone) { + db.runSql('UPDATE users SET admin=1 WHERE id=?', [ userId ], iteratorDone); + }, function (error) { + if (error) return callback(error); + + async.series([ + db.runSql.bind(db, 'DELETE FROM groupMembers WHERE groupId=?', [ 'admin' ]), + db.runSql.bind(db, 'DELETE FROM groups WHERE id=?', [ 'admin' ]) + ], callback); + }); + }); + }); +}; + +exports.down = function(db, callback) { + db.runSql('ALTER TABLE users DROP COLUMN admin', function (error) { + if (error) console.error(error); + callback(error); + }); +}; + diff --git a/migrations/schema.sql b/migrations/schema.sql index d1e48be4f..9984ea989 100644 --- a/migrations/schema.sql +++ b/migrations/schema.sql @@ -25,6 +25,7 @@ CREATE TABLE IF NOT EXISTS users( fallbackEmail VARCHAR(512) DEFAULT "", twoFactorAuthenticationSecret VARCHAR(128) DEFAULT "", twoFactorAuthenticationEnabled BOOLEAN DEFAULT false, + admin BOOLEAN DEFAULT false, PRIMARY KEY(id)); diff --git a/src/accesscontrol.js b/src/accesscontrol.js index 06702bc7d..befaebc94 100644 --- a/src/accesscontrol.js +++ b/src/accesscontrol.js @@ -108,7 +108,7 @@ function hasScopes(authorizedScopes, requiredScopes) { } function scopesForUser(user) { - return users.isAdmin(user) ? exports.VALID_SCOPES : [ 'profile', 'apps:read' ]; + return user.admin ? exports.VALID_SCOPES : [ 'profile', 'apps:read' ]; } function validateToken(accessToken, callback) { diff --git a/src/apps.js b/src/apps.js index 2fb7072aa..edc2f87ba 100644 --- a/src/apps.js +++ b/src/apps.js @@ -85,7 +85,6 @@ var appdb = require('./appdb.js'), TransformStream = require('stream').Transform, updateChecker = require('./updatechecker.js'), url = require('url'), - users = require('./users.js'), util = require('util'), uuid = require('uuid'), validator = require('validator'), @@ -358,9 +357,7 @@ function hasAccessTo(app, user, callback) { // check user access if (app.accessRestriction.users.some(function (e) { return e === user.id; })) return callback(null, true); - const isAdmin = users.isAdmin(user); - - if (isAdmin) return callback(null, true); // admins can always access any app + if (user.admin) return callback(null, true); // admins can always access any app if (!app.accessRestriction.groups) return callback(null, false); diff --git a/src/constants.js b/src/constants.js index 834339000..3be8dd9da 100644 --- a/src/constants.js +++ b/src/constants.js @@ -21,9 +21,6 @@ exports = module.exports = { ADMIN_CLIENT_ID: 'webadmin', // oauth client id - ADMIN_GROUP_NAME: 'admin', - ADMIN_GROUP_ID: 'admin', - NGINX_ADMIN_CONFIG_FILE_NAME: 'admin.conf', GHOST_USER_FILE: '/tmp/cloudron_ghost.json', diff --git a/src/groupdb.js b/src/groupdb.js index 61d083c04..dac976071 100644 --- a/src/groupdb.js +++ b/src/groupdb.js @@ -152,7 +152,7 @@ function clear(callback) { database.query('DELETE FROM groupMembers', function (error) { if (error) return callback(new DatabaseError(DatabaseError.INTERNAL_ERROR, error)); - database.query('DELETE FROM groups WHERE id != ?', [ 'admin' ], function (error) { + database.query('DELETE FROM groups', function (error) { if (error) return callback(new DatabaseError(DatabaseError.INTERNAL_ERROR, error)); callback(error); diff --git a/src/groups.js b/src/groups.js index 83cda61dc..51e03d45a 100644 --- a/src/groups.js +++ b/src/groups.js @@ -3,8 +3,6 @@ exports = module.exports = { GroupsError: GroupsError, - addOwnerGroup: addOwnerGroup, - create: create, remove: remove, get: get, @@ -25,8 +23,7 @@ exports = module.exports = { getMembership: getMembership }; -var accesscontrol = require('./accesscontrol.js'), - assert = require('assert'), +var assert = require('assert'), constants = require('./constants.js'), DatabaseError = require('./databaseerror.js'), groupdb = require('./groupdb.js'), @@ -100,9 +97,6 @@ function remove(id, callback) { assert.strictEqual(typeof id, 'string'); assert.strictEqual(typeof callback, 'function'); - // never allow admin group to be deleted - if (id === constants.ADMIN_GROUP_ID) return callback(new GroupsError(GroupsError.NOT_ALLOWED)); - groupdb.del(id, function (error) { if (error && error.reason === DatabaseError.NOT_FOUND) return callback(new GroupsError(GroupsError.NOT_FOUND)); if (error) return callback(new GroupsError(GroupsError.INTERNAL_ERROR, error)); @@ -244,12 +238,6 @@ function isMember(groupId, userId, callback) { }); } -function addOwnerGroup(callback) { - assert.strictEqual(typeof callback, 'function'); - - groupdb.add(constants.ADMIN_GROUP_ID, constants.ADMIN_GROUP_NAME, callback); -} - function update(groupId, data, callback) { assert.strictEqual(typeof groupId, 'string'); assert(data && typeof data === 'object'); diff --git a/src/ldap.js b/src/ldap.js index 1b9d56fd2..855113d62 100644 --- a/src/ldap.js +++ b/src/ldap.js @@ -58,9 +58,6 @@ function getUsersWithAccessToApp(req, callback) { async.filter(result, apps.hasAccessTo.bind(null, app), function (error, allowedUsers) { if (error) return callback(new ldap.OperationsError(error.toString())); - // TODO: in the long run, we should probably get rid of this "admin" integration altogether - allowedUsers.forEach(function (r) { r.admin = users.isAdmin(r); }); - callback(null, allowedUsers); }); }); diff --git a/src/routes/test/groups-test.js b/src/routes/test/groups-test.js index 38bbd8452..e2ad2a523 100644 --- a/src/routes/test/groups-test.js +++ b/src/routes/test/groups-test.js @@ -24,7 +24,7 @@ var token, token_1 = null; var userId, userId_1 = null; var GROUP_NAME = 'externals'; -var groupObject; +var groupObject, group1Object; function setup(done) { config._reset(); @@ -88,218 +88,183 @@ describe('Groups API', function () { before(setup); after(cleanup); - describe('list', function () { - it('cannot get groups without token', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups') - .end(function (err, res) { - expect(res.statusCode).to.equal(401); - done(); - }); - }); - - it('cannot get groups as normal user', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups') - .query({ access_token: token_1 }) - .end(function (err, res) { - expect(res.statusCode).to.equal(403); - done(); - }); - }); - - it('can get groups', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups') - .query({ access_token: token }) - .end(function (err, res) { - expect(res.statusCode).to.equal(200); - expect(res.body.groups).to.be.an(Array); - expect(res.body.groups.length).to.be(1); - expect(res.body.groups[0].name).to.eql('admin'); - expect(res.body.groups[0].userIds).to.not.be.ok(); - done(); - }); - }); - }); - - describe('create', function () { - it('fails due to mising token', function (done) { - superagent.post(SERVER_URL + '/api/v1/groups') - .send({ name: GROUP_NAME}) - .end(function (error, result) { - expect(result.statusCode).to.equal(401); - done(); - }); - }); - - it('succeeds', function (done) { - superagent.post(SERVER_URL + '/api/v1/groups') - .query({ access_token: token }) - .send({ name: GROUP_NAME}) - .end(function (error, result) { - expect(result.statusCode).to.equal(201); - groupObject = result.body; - done(); - }); - }); - - it('fails for already exists', function (done) { - superagent.post(SERVER_URL + '/api/v1/groups') - .query({ access_token: token }) - .send({ name: GROUP_NAME}) - .end(function (error, result) { - expect(result.statusCode).to.equal(409); - done(); - }); - }); - }); - - describe('get', function () { - it('cannot get non-existing group', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups/nope') - .query({ access_token: token }) - .end(function (error, result) { - expect(result.statusCode).to.equal(404); - done(); - }); - }); - - it('cannot get existing group with normal user', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups/admin') - .query({ access_token: token_1 }) - .end(function (error, result) { - expect(result.statusCode).to.equal(403); - done(); - }); - }); - - it('can get existing group', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups/admin') - .query({ access_token: token }) - .end(function (error, result) { - expect(result.statusCode).to.equal(200); - expect(result.body.name).to.be('admin'); - expect(result.body.userIds.length).to.be(1); - expect(result.body.userIds[0]).to.be(userId); - done(); - }); - }); - }); - - describe('remove', function () { - it('cannot remove without token', function (done) { - superagent.del(SERVER_URL + '/api/v1/groups/externals') - .end(function (error, result) { - expect(result.statusCode).to.equal(401); - done(); - }); - }); - - it('can remove empty group', function (done) { - superagent.del(SERVER_URL + '/api/v1/groups/' + groupObject.id) - .send({ password: PASSWORD }) - .query({ access_token: token }) - .end(function (error, result) { - expect(result.statusCode).to.equal(204); - done(); - }); - }); - - it('cannot remove non-empty group', function (done) { - superagent.del(SERVER_URL + '/api/v1/groups/admin') - .send({ password: PASSWORD }) - .query({ access_token: token }) - .end(function (error, result) { - expect(result.statusCode).to.equal(409); - done(); - }); - }); - }); - - describe('Set groups', function () { - var group0Object, group1Object; - before(function (done) { - groups.create('group0', function (e, r) { - group0Object = r; - groups.create('group1', function (e, r) { - group1Object = r; - done(); - }); + it('create fails due to mising token', function (done) { + superagent.post(SERVER_URL + '/api/v1/groups') + .send({ name: GROUP_NAME}) + .end(function (error, result) { + expect(result.statusCode).to.equal(401); + done(); }); - }); + }); - it('cannot add user to invalid group', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') - .query({ access_token: token }) - .send({ groupIds: [ 'admin', 'something' ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(404); - done(); - }); - }); + it('create succeeds', function (done) { + superagent.post(SERVER_URL + '/api/v1/groups') + .query({ access_token: token }) + .send({ name: GROUP_NAME}) + .end(function (error, result) { + expect(result.statusCode).to.equal(201); + groupObject = result.body; + done(); + }); + }); - it('can add user to valid group', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') - .query({ access_token: token }) - .send({ groupIds: [ 'admin', group0Object.id, group1Object.id ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(204); - done(); - }); - }); + it('create fails for already exists', function (done) { + superagent.post(SERVER_URL + '/api/v1/groups') + .query({ access_token: token }) + .send({ name: GROUP_NAME}) + .end(function (error, result) { + expect(result.statusCode).to.equal(409); + done(); + }); + }); - it('cannot remove self from admin', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') - .query({ access_token: token }) - .send({ groupIds: [ group0Object.id, group1Object.id ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(409); // not allowed - done(); - }); - }); + it('can create another group', function (done) { + superagent.post(SERVER_URL + '/api/v1/groups') + .query({ access_token: token }) + .send({ name: 'group1'}) + .end(function (error, result) { + expect(result.statusCode).to.equal(201); + group1Object = result.body; + done(); + }); + }) - it('can add another user to admin', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId_1 + '/groups') - .query({ access_token: token }) - .send({ groupIds: [ 'admin' ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(204); - done(); - }); - }); + it('cannot add user to invalid group', function (done) { + superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') + .query({ access_token: token }) + .send({ groupIds: [ groupObject.id, 'something' ]}) + .end(function (error, result) { + expect(result.statusCode).to.equal(404); + done(); + }); + }); - it('lists members of admin group', function (done) { - superagent.get(SERVER_URL + '/api/v1/groups/admin') - .query({ access_token: token }) - .end(function (error, result) { - expect(result.statusCode).to.equal(200); - expect(result.body.userIds.length).to.be(2); - expect(result.body.userIds[0]).to.be(userId); - expect(result.body.userIds[1]).to.be(userId_1); - done(); - }); - }); + it('can set groups of a user', function (done) { + superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') + .query({ access_token: token }) + .send({ groupIds: [ groupObject.id, group1Object.id ]}) + .end(function (error, result) { + expect(result.statusCode).to.equal(204); + done(); + }); + }); - it('can add user_1 to admin', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId_1 + '/groups') - .query({ access_token: token }) - .send({ groupIds: [ 'admin' ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(204); + it('can set users of a group', function (done) { + superagent.put(SERVER_URL + '/api/v1/groups/' + groupObject.id + '/members') + .query({ access_token: token }) + .send({ userIds: [ userId, userId_1 ]}) + .end(function (error, result) { + expect(result.statusCode).to.equal(200); + done(); + }); + }); - token_1 = tokendb.generateToken(); + it('cannot get non-existing group', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups/nope') + .query({ access_token: token }) + .end(function (error, result) { + expect(result.statusCode).to.equal(404); + done(); + }); + }); - // HACK to get a token for second user (passwords are generated and the user should have gotten a password setup link...) - tokendb.add(token_1, userId_1, 'test-client-id', Date.now() + 100000, 'users', done); - }); - }); + it('cannot get existing group with normal user', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups/' + groupObject.id) + .query({ access_token: token_1 }) + .end(function (error, result) { + expect(result.statusCode).to.equal(403); + done(); + }); + }); - it('remove activation user from admin', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') - .query({ access_token: token_1 }) - .send({ groupIds: [ group1Object.id, group0Object.id ]}) - .end(function (error, result) { - expect(result.statusCode).to.equal(204); // user_1 is still admin, so we can remove the other person - done(); - }); - }); + it('can get existing group', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups/' + groupObject.id) + .query({ access_token: token }) + .end(function (error, result) { + expect(result.statusCode).to.equal(200); + expect(result.body.name).to.be(groupObject.name); + expect(result.body.userIds.length).to.be(2); + expect(result.body.userIds[0]).to.be(userId); + expect(result.body.userIds[1]).to.be(userId_1); + done(); + }); + }); + + it('cannot list groups without token', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups') + .end(function (err, res) { + expect(res.statusCode).to.equal(401); + done(); + }); + }); + + it('cannot list groups as normal user', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups') + .query({ access_token: token_1 }) + .end(function (err, res) { + expect(res.statusCode).to.equal(403); + done(); + }); + }); + + it('can list groups', function (done) { + superagent.get(SERVER_URL + '/api/v1/groups') + .query({ access_token: token }) + .end(function (err, res) { + expect(res.statusCode).to.equal(200); + expect(res.body.groups).to.be.an(Array); + expect(res.body.groups.length).to.be(2); + expect(res.body.groups[0].name).to.eql(groupObject.name); + expect(res.body.groups[1].name).to.eql(group1Object.name); + done(); + }); + }); + + it('remove user from group', function (done) { + superagent.put(SERVER_URL + '/api/v1/users/' + userId + '/groups') + .query({ access_token: token }) + .send({ groupIds: [ groupObject.id ]}) + .end(function (error, result) { + expect(result.statusCode).to.equal(204); + done(); + }); + }); + + it('cannot remove without token', function (done) { + superagent.del(SERVER_URL + '/api/v1/groups/externals') + .end(function (error, result) { + expect(result.statusCode).to.equal(401); + done(); + }); + }); + + it('can clear users of a group', function (done) { + superagent.put(SERVER_URL + '/api/v1/groups/' + group1Object.id + '/members') + .query({ access_token: token }) + .send({ userIds: [ ]}) + .end(function (error, result) { + expect(result.statusCode).to.equal(200); + done(); + }); + }); + + it('can remove empty group', function (done) { + superagent.del(SERVER_URL + '/api/v1/groups/' + group1Object.id) + .send({ password: PASSWORD }) + .query({ access_token: token }) + .end(function (error, result) { + expect(result.statusCode).to.equal(204); + done(); + }); + }); + + it('can remove non-empty group', function (done) { + superagent.del(SERVER_URL + '/api/v1/groups/' + groupObject.id) + .send({ password: PASSWORD }) + .query({ access_token: token }) + .end(function (error, result) { + expect(result.statusCode).to.equal(204); + done(); + }); }); }); diff --git a/src/routes/test/users-test.js b/src/routes/test/users-test.js index b0946f085..336c9aff4 100644 --- a/src/routes/test/users-test.js +++ b/src/routes/test/users-test.js @@ -165,7 +165,8 @@ describe('Users API', function () { expect(res.statusCode).to.equal(200); expect(res.body.username).to.equal(USERNAME_0.toLowerCase()); expect(res.body.email).to.equal(EMAIL_0.toLowerCase()); - expect(res.body.groupIds).to.eql(['admin']); + expect(res.body.groupIds).to.eql([]); + expect(res.body.admin).to.be(true); done(); }); @@ -196,7 +197,8 @@ describe('Users API', function () { expect(res.statusCode).to.equal(200); expect(res.body.username).to.equal(USERNAME_0.toLowerCase()); expect(res.body.email).to.equal(EMAIL_0.toLowerCase()); - expect(res.body.groupIds).to.eql(['admin']); + expect(res.body.groupIds).to.eql([]); + expect(res.body.admin).to.be(true); done(); }); @@ -236,7 +238,8 @@ describe('Users API', function () { expect(res.statusCode).to.equal(200); expect(res.body.username).to.equal(USERNAME_0.toLowerCase()); expect(res.body.email).to.equal(EMAIL_0.toLowerCase()); - expect(res.body.groupIds).to.eql(['admin']); + expect(res.body.groupIds).to.eql([]); + expect(res.body.admin).to.be(true); expect(res.body.displayName).to.be.a('string'); expect(res.body.password).to.not.be.ok(); expect(res.body.salt).to.not.be.ok(); @@ -321,9 +324,9 @@ describe('Users API', function () { }); it('set second user as admin succeeds', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + user_1.id + '/groups') + superagent.post(SERVER_URL + '/api/v1/users/' + user_1.id) .query({ access_token: token }) - .send({ groupIds: [ constants.ADMIN_GROUP_ID ] }) + .send({ admin: true }) .end(function (err, res) { expect(res.statusCode).to.equal(204); @@ -331,7 +334,7 @@ describe('Users API', function () { .query({ access_token: token }) .end(function (err, res) { expect(res.statusCode).to.equal(200); - expect(res.body.groupIds).to.eql(['admin']); + expect(res.body.admin).to.be(true); done(); }); @@ -353,10 +356,10 @@ describe('Users API', function () { }); }); - it('remove itself from admins fails', function (done) { - superagent.put(SERVER_URL + '/api/v1/users/' + user_0.id + '/groups') + it('remove self as admin fails', function (done) { + superagent.post(SERVER_URL + '/api/v1/users/' + user_0.id) .query({ access_token: token }) - .send({ groupIds: [ groupObject.id ] }) + .send({ admin: false }) .end(function (err, res) { expect(res.statusCode).to.equal(409); done(); diff --git a/src/routes/users.js b/src/routes/users.js index b9896cbdd..951f554ce 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -13,7 +13,6 @@ exports = module.exports = { }; var assert = require('assert'), - constants = require('../constants.js'), HttpError = require('connect-lastmile').HttpError, HttpSuccess = require('connect-lastmile').HttpSuccess, users = require('../users.js'), @@ -68,6 +67,12 @@ function update(req, res, next) { if ('displayName' in req.body && typeof req.body.displayName !== 'string') return next(new HttpError(400, 'displayName must be string')); if ('username' in req.body && typeof req.body.username !== 'string') return next(new HttpError(400, 'username must be a string')); + if ('admin' in req.body) { + if (typeof req.body.admin !== 'boolean') return next(new HttpError(400, 'admin must be a boolean')); + // this route is only allowed for admins, so req.user has to be an admin + if (req.user.id === req.params.userId) return next(new HttpError(409, 'Cannot change admin flag on self')); + } + users.update(req.params.userId, req.body, auditSource(req), function (error) { if (error && error.reason === UsersError.BAD_FIELD) return next(new HttpError(400, error.message)); if (error && error.reason === UsersError.ALREADY_EXISTS) return next(new HttpError(409, error.message)); @@ -149,9 +154,6 @@ function setGroups(req, res, next) { if (!Array.isArray(req.body.groupIds)) return next(new HttpError(400, 'API call requires a groups array.')); - // this route is only allowed for admins, so req.user has to be an admin - if (req.user.id === req.params.userId && req.body.groupIds.indexOf(constants.ADMIN_GROUP_ID) === -1) return next(new HttpError(409, 'Admin removing itself from admins is not allowed')); - users.setMembership(req.params.userId, req.body.groupIds, function (error) { if (error && error.reason === UsersError.NOT_FOUND) return next(new HttpError(404, 'One or more groups not found')); if (error) return next(new HttpError(500, error)); diff --git a/src/test/apps-test.js b/src/test/apps-test.js index 71062562c..7cb47cfa9 100644 --- a/src/test/apps-test.js +++ b/src/test/apps-test.js @@ -10,7 +10,6 @@ var appdb = require('../appdb.js'), AppsError = apps.AppsError, async = require('async'), config = require('../config.js'), - constants = require('../constants.js'), database = require('../database.js'), domains = require('../domains.js'), expect = require('expect.js'), @@ -33,7 +32,8 @@ describe('Apps', function () { modifiedAt: 'now', resetToken: hat(256), displayName: '', - groupIds: [ 'admin' ] + groupIds: [], + admin: true }; var USER_0 = { @@ -47,7 +47,8 @@ describe('Apps', function () { modifiedAt: 'now', resetToken: hat(256), displayName: '', - groupIds: [] + groupIds: [], + admin: false }; var USER_1 = { @@ -61,7 +62,8 @@ describe('Apps', function () { modifiedAt: 'now', resetToken: hat(256), displayName: '', - groupIds: [ 'somegroup' ] + groupIds: [ 'somegroup' ], + admin: false }; var GROUP_0 = { @@ -154,13 +156,11 @@ describe('Apps', function () { database._clear, domains.add.bind(null, DOMAIN_0.domain, DOMAIN_0.zoneName, DOMAIN_0.provider, DOMAIN_0.config, null, DOMAIN_0.tlsConfig), domains.add.bind(null, DOMAIN_1.domain, DOMAIN_1.zoneName, DOMAIN_1.provider, DOMAIN_1.config, null, DOMAIN_1.tlsConfig), - groups.addOwnerGroup, userdb.add.bind(null, ADMIN_0.id, ADMIN_0), userdb.add.bind(null, USER_0.id, USER_0), userdb.add.bind(null, USER_1.id, USER_1), groupdb.add.bind(null, GROUP_0.id, GROUP_0.name), groupdb.add.bind(null, GROUP_1.id, GROUP_1.name), - groups.addMember.bind(null, constants.ADMIN_GROUP_ID, ADMIN_0.id), groups.addMember.bind(null, GROUP_0.id, USER_1.id), appdb.add.bind(null, APP_0.id, APP_0.appStoreId, APP_0.manifest, APP_0.location, APP_0.domain, APP_0.ownerId, APP_0.portBindings, APP_0), appdb.add.bind(null, APP_1.id, APP_1.appStoreId, APP_1.manifest, APP_1.location, APP_1.domain, APP_1.ownerId, APP_1.portBindings, APP_1), @@ -290,8 +290,8 @@ describe('Apps', function () { }); describe('hasAccessTo', function () { - const someuser = { id: 'someuser', groupIds: [] }; - const adminuser = { id: 'adminuser', groupIds: [ 'admin' ] }; + const someuser = { id: 'someuser', groupIds: [], admin: false }; + const adminuser = { id: 'adminuser', groupIds: [ 'groupie' ], admin: true }; it('returns true for unrestricted access', function (done) { apps.hasAccessTo({ accessRestriction: null }, someuser, function (error, access) { diff --git a/src/test/apptask-test.js b/src/test/apptask-test.js index c1fe1a630..9518b7c29 100644 --- a/src/test/apptask-test.js +++ b/src/test/apptask-test.js @@ -72,7 +72,8 @@ var ADMIN = { createdAt: 'sometime back', modifiedAt: 'now', resetToken: '', - displayName: '' + displayName: '', + admin: true }; var APP = { diff --git a/src/test/database-test.js b/src/test/database-test.js index 6784f4764..7718c711c 100644 --- a/src/test/database-test.js +++ b/src/test/database-test.js @@ -11,7 +11,6 @@ var appdb = require('../appdb.js'), backupdb = require('../backupdb.js'), clientdb = require('../clientdb.js'), config = require('../config.js'), - constants = require('../constants.js'), database = require('../database'), DatabaseError = require('../databaseerror.js'), domaindb = require('../domaindb'), @@ -38,7 +37,8 @@ var USER_0 = { resetToken: hat(256), displayName: '', twoFactorAuthenticationEnabled: false, - twoFactorAuthenticationSecret: '' + twoFactorAuthenticationSecret: '', + admin: false }; var USER_1 = { @@ -53,7 +53,8 @@ var USER_1 = { resetToken: '', displayName: 'Herbert 1', twoFactorAuthenticationEnabled: false, - twoFactorAuthenticationSecret: '' + twoFactorAuthenticationSecret: '', + admin: false }; var USER_2 = { @@ -68,7 +69,8 @@ var USER_2 = { resetToken: '', displayName: 'Herbert 2', twoFactorAuthenticationEnabled: false, - twoFactorAuthenticationSecret: '' + twoFactorAuthenticationSecret: '', + admin: false }; const DOMAIN_0 = { @@ -1465,7 +1467,6 @@ describe('database', function () { async.series([ database.initialize, database._clear, - groupdb.add.bind(null, constants.ADMIN_GROUP_ID, constants.ADMIN_GROUP_NAME), userdb.add.bind(null, USER_0.id, USER_0), userdb.add.bind(null, USER_1.id, USER_1), userdb.add.bind(null, USER_2.id, USER_2) @@ -1544,9 +1545,8 @@ describe('database', function () { it('can getAll', function (done) { groupdb.getAll(function (error, result) { expect(error).to.be(null); - expect(result.length).to.be(2); // admin! - expect(result[0].name).to.be('admin'); - expect(result[1].name).to.be('founders'); + expect(result.length).to.be(1); + expect(result[0].name).to.be('founders'); done(); }); }); @@ -1554,19 +1554,17 @@ describe('database', function () { it('can getAllWithMembers', function (done) { groupdb.getAllWithMembers(function (error, result) { expect(error).to.be(null); - expect(result.length).to.be(2); // admin! - expect(result[0].name).to.be('admin'); - expect(result[0].userIds).to.eql([ ]); + expect(result.length).to.be(1); - expect(result[1].name).to.be('founders'); - expect(result[1].userIds).to.eql([ USER_2.id ]); + expect(result[0].name).to.be('founders'); + expect(result[0].userIds).to.eql([ USER_2.id ]); done(); }); }); it('can set groups', function (done) { - groupdb.setMembership(USER_0.id, [ 'admin', GROUP_ID_1 ], function (error) { + groupdb.setMembership(USER_0.id, [ GROUP_ID_1 ], function (error) { expect(error).to.be(null); done(); }); @@ -1575,7 +1573,7 @@ describe('database', function () { it('can get groups', function (done) { groupdb.getMembership(USER_0.id, function (error, result) { expect(error).to.be(null); - expect(result).to.eql([ 'admin', GROUP_ID_1 ]); + expect(result).to.eql([ GROUP_ID_1 ]); done(); }); }); diff --git a/src/test/groups-test.js b/src/test/groups-test.js index 7676ba18e..296b500aa 100644 --- a/src/test/groups-test.js +++ b/src/test/groups-test.js @@ -7,9 +7,7 @@ 'use strict'; var async = require('async'), - accesscontrol = require('../accesscontrol.js'), config = require('../config.js'), - constants = require('../constants.js'), database = require('../database.js'), DatabaseError = require('../databaseerror.js'), expect = require('expect.js'), @@ -362,21 +360,3 @@ describe('Set user groups', function () { }); }); }); - -describe('Admin group', function () { - before(function (done) { - async.series([ - setup, - userdb.add.bind(null, USER_0.id, USER_0) - ], done); - }); - after(cleanup); - - it('cannot delete admin group ever', function (done) { - groups.remove(constants.ADMIN_GROUP_ID, function (error) { - expect(error.reason).to.equal(GroupsError.NOT_ALLOWED); - - done(); - }); - }); -}); diff --git a/src/test/users-test.js b/src/test/users-test.js index efc0473e1..d9bd5e9a3 100644 --- a/src/test/users-test.js +++ b/src/test/users-test.js @@ -58,15 +58,13 @@ function cleanupUsers(done) { } function createOwner(done) { - groups.addOwnerGroup(function () { // ignore error since it might already exist - users.createOwner(USERNAME, PASSWORD, EMAIL, DISPLAY_NAME, AUDIT_SOURCE, function (error, result) { - expect(error).to.not.be.ok(); - expect(result).to.be.ok(); + users.createOwner(USERNAME, PASSWORD, EMAIL, DISPLAY_NAME, AUDIT_SOURCE, function (error, result) { + expect(error).to.not.be.ok(); + expect(result).to.be.ok(); - userObject = result; + userObject = result; - done(); - }); + done(); }); } @@ -673,7 +671,7 @@ describe('User', function () { user1.id = result.id; - users.setMembership(user1.id, [ constants.ADMIN_GROUP_ID ], function (error) { + users.update(user1.id, { admin: true }, { ip: '1.2.3.4' }, function (error) { expect(error).to.not.be.ok(); // one mail for user creation, one mail for admin change @@ -682,16 +680,8 @@ describe('User', function () { }); }); - it('add user to non admin group does not trigger admin mail', function (done) { - users.setMembership(user1.id, [ constants.ADMIN_GROUP_ID, groupObject.id ], function (error) { - expect(error).to.equal(null); - - checkMails(0, done); - }); - }); - it('succeeds to remove admin flag', function (done) { - users.setMembership(user1.id, [ groupObject.id ], function (error) { + users.update(user1.id, { admin: false }, { ip: '1.2.3.4' }, function (error) { expect(error).to.eql(null); checkMails(1, done); @@ -726,7 +716,7 @@ describe('User', function () { user1.id = result.id; - groups.setMembership(user1.id, [ constants.ADMIN_GROUP_ID ], function (error) { + users.update(user1.id, { admin: true }, { ip: '1.2.3.4' }, function (error) { expect(error).to.eql(null); users.getAllAdmins(function (error, admins) { @@ -735,8 +725,7 @@ describe('User', function () { expect(admins[0].username).to.equal(USERNAME.toLowerCase()); expect(admins[1].username).to.equal(user1.username.toLowerCase()); - // one mail for user creation one mail for admin change - checkMails(1, done); // FIXME should be 2 for admin change + checkMails(2, done); // one mail for user creation one mail for admin change }); }); }); diff --git a/src/userdb.js b/src/userdb.js index 441ad570c..48a95ef4d 100644 --- a/src/userdb.js +++ b/src/userdb.js @@ -18,17 +18,18 @@ exports = module.exports = { }; var assert = require('assert'), - constants = require('./constants.js'), database = require('./database.js'), debug = require('debug')('box:userdb'), DatabaseError = require('./databaseerror'); -var USERS_FIELDS = [ 'id', 'username', 'email', 'fallbackEmail', 'password', 'salt', 'createdAt', 'modifiedAt', 'resetToken', 'displayName', 'twoFactorAuthenticationEnabled', 'twoFactorAuthenticationSecret' ].join(','); +var USERS_FIELDS = [ 'id', 'username', 'email', 'fallbackEmail', 'password', 'salt', 'createdAt', 'modifiedAt', 'resetToken', 'displayName', + 'twoFactorAuthenticationEnabled', 'twoFactorAuthenticationSecret', 'admin' ].join(','); function postProcess(result) { assert.strictEqual(typeof result, 'object'); result.twoFactorAuthenticationEnabled = !!result.twoFactorAuthenticationEnabled; + result.admin = !!result.admin; return result; } @@ -73,8 +74,7 @@ function getOwner(callback) { assert.strictEqual(typeof callback, 'function'); // the first created user it the 'owner' - database.query('SELECT ' + USERS_FIELDS + ' FROM users, groupMembers WHERE groupMembers.groupId = ? AND users.id = groupMembers.userId ORDER BY createdAt LIMIT 1', - [ constants.ADMIN_GROUP_ID ], function (error, result) { + database.query('SELECT ' + USERS_FIELDS + ' FROM users WHERE admin=1 ORDER BY createdAt LIMIT 1', function (error, result) { if (error) return callback(new DatabaseError(DatabaseError.INTERNAL_ERROR, error)); if (result.length === 0) return callback(new DatabaseError(DatabaseError.NOT_FOUND)); @@ -119,7 +119,7 @@ function getAllAdmins(callback) { assert.strictEqual(typeof callback, 'function'); // the mailer code relies on the first object being the 'owner' (thus the ORDER) - database.query('SELECT ' + USERS_FIELDS + ' FROM users, groupMembers WHERE groupMembers.groupId = ? AND users.id = groupMembers.userId ORDER BY createdAt', [ constants.ADMIN_GROUP_ID ], function (error, results) { + database.query('SELECT ' + USERS_FIELDS + ' FROM users WHERE admin=1 ORDER BY createdAt', function (error, results) { if (error) return callback(new DatabaseError(DatabaseError.INTERNAL_ERROR, error)); results.forEach(postProcess); @@ -139,10 +139,11 @@ function add(userId, user, callback) { assert.strictEqual(typeof user.modifiedAt, 'string'); assert.strictEqual(typeof user.resetToken, 'string'); assert.strictEqual(typeof user.displayName, 'string'); + assert.strictEqual(typeof user.admin, 'boolean'); assert.strictEqual(typeof callback, 'function'); - const query = 'INSERT INTO users (id, username, password, email, fallbackEmail, salt, createdAt, modifiedAt, resetToken, displayName) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)'; - const args = [ userId, user.username, user.password, user.email, user.fallbackEmail, user.salt, user.createdAt, user.modifiedAt, user.resetToken, user.displayName ]; + const query = 'INSERT INTO users (id, username, password, email, fallbackEmail, salt, createdAt, modifiedAt, resetToken, displayName, admin) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)'; + const args = [ userId, user.username, user.password, user.email, user.fallbackEmail, user.salt, user.createdAt, user.modifiedAt, user.resetToken, user.displayName, user.admin ]; database.query(query, args, function (error) { if (error && error.code === 'ER_DUP_ENTRY' && error.sqlMessage.indexOf('users_email') !== -1) return callback(new DatabaseError(DatabaseError.ALREADY_EXISTS, 'email already exists')); @@ -210,7 +211,7 @@ function update(userId, user, callback) { } else if (k === 'email' || k === 'fallbackEmail') { assert.strictEqual(typeof user[k], 'string'); args.push(user[k]); - } else if (k === 'twoFactorAuthenticationEnabled') { + } else if (k === 'twoFactorAuthenticationEnabled' || k === 'admin') { assert.strictEqual(typeof user[k], 'boolean'); args.push(user[k] ? 1 : 0); } else { diff --git a/src/users.js b/src/users.js index 287c08d9d..987dab6f1 100644 --- a/src/users.js +++ b/src/users.js @@ -26,9 +26,7 @@ exports = module.exports = { setTwoFactorAuthenticationSecret: setTwoFactorAuthenticationSecret, enableTwoFactorAuthentication: enableTwoFactorAuthentication, disableTwoFactorAuthentication: disableTwoFactorAuthentication, - transferOwnership: transferOwnership, - - isAdmin: isAdmin + transferOwnership: transferOwnership }; var apps = require('./apps.js'), @@ -195,7 +193,8 @@ function create(username, password, email, displayName, options, auditSource, ca createdAt: now, modifiedAt: now, resetToken: hat(256), - displayName: displayName + displayName: displayName, + admin: owner }; userdb.add(user.id, user, function (error) { @@ -324,10 +323,6 @@ function count(callback) { }); } -function isAdmin(user) { - return user.groupIds.indexOf(constants.ADMIN_GROUP_ID) !== -1; -} - function get(userId, callback) { assert.strictEqual(typeof userId, 'string'); assert.strictEqual(typeof callback, 'function'); @@ -372,7 +367,7 @@ function updateUser(userId, data, auditSource, callback) { assert.strictEqual(typeof callback, 'function'); var error; - data = _.pick(data, 'email', 'fallbackEmail', 'displayName', 'username'); + data = _.pick(data, 'email', 'fallbackEmail', 'displayName', 'username', 'admin'); if (_.isEmpty(data)) return callback(); @@ -394,7 +389,7 @@ function updateUser(userId, data, auditSource, callback) { if (error) return callback(error); } - userdb.get(userId, function (error) { + userdb.get(userId, function (error, oldUser) { if (error && error.reason === DatabaseError.NOT_FOUND) return callback(new UsersError(UsersError.NOT_FOUND)); if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); @@ -403,12 +398,14 @@ function updateUser(userId, data, auditSource, callback) { if (error && error.reason === DatabaseError.NOT_FOUND) return callback(new UsersError(UsersError.NOT_FOUND, error)); if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); - callback(); + callback(null); get(userId, function (error, result) { if (error) return console.error(error); eventlog.add(eventlog.ACTION_USER_UPDATE, auditSource, { userId: userId, user: removePrivateFields(result) }); + + if ((result.admin && !oldUser.admin) || (!result.admin && oldUser.admin)) mailer.adminChanged(result, result.admin); }); }); }); @@ -419,28 +416,11 @@ function setMembership(userId, groupIds, callback) { assert(Array.isArray(groupIds)); assert.strictEqual(typeof callback, 'function'); - groups.getMembership(userId, function (error, oldGroupIds) { - if (error && error.reason !== GroupsError.NOT_FOUND) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); + groups.setMembership(userId, groupIds, function (error) { + if (error && error.reason === GroupsError.NOT_FOUND) return callback(new UsersError(UsersError.NOT_FOUND, 'One or more groups not found')); + if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); - oldGroupIds = oldGroupIds || []; - - groups.setMembership(userId, groupIds, function (error) { - if (error && error.reason === GroupsError.NOT_FOUND) return callback(new UsersError(UsersError.NOT_FOUND, 'One or more groups not found')); - if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); - - var isAdmin = groupIds.some(function (g) { return g === constants.ADMIN_GROUP_ID; }); - var wasAdmin = oldGroupIds.some(function (g) { return g === constants.ADMIN_GROUP_ID; }); - - if ((isAdmin && !wasAdmin) || (!isAdmin && wasAdmin)) { - get(userId, function (error, result) { - if (error) return debug('Failed to send admin change mail.', error); - - mailer.adminChanged(result, isAdmin); - }); - } - - callback(null); - }); + callback(null); }); } @@ -526,20 +506,10 @@ function createOwner(username, password, email, displayName, auditSource, callba if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); if (count !== 0) return callback(new UsersError(UsersError.ALREADY_EXISTS, 'Owner already exists')); - // have to provide the group id explicitly so using db layer directly - groups.addOwnerGroup(function (error) { - // we proceed if it already exists so we can re-create the owner if need be - if (error && error.reason !== DatabaseError.ALREADY_EXISTS) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); + create(username, password, email, displayName, { owner: true }, auditSource, function (error, user) { + if (error) return callback(error); - create(username, password, email, displayName, { owner: true }, auditSource, function (error, user) { - if (error) return callback(error); - - groups.addMember(constants.ADMIN_GROUP_ID, user.id, function (error) { - if (error) return callback(new UsersError(UsersError.INTERNAL_ERROR, error)); - - callback(null, user); - }); - }); + callback(null, user); }); }); }