diff --git a/src/groups.js b/src/groups.js index 8d966c760..addea4d70 100644 --- a/src/groups.js +++ b/src/groups.js @@ -18,17 +18,17 @@ exports = module.exports = { removeMember, isMember, - setMembership, - getMembership, + setLocalMembership, + resetSource, - resetSource + // exported for testing + _getMembership: getMembership }; const assert = require('assert'), BoxError = require('./boxerror.js'), constants = require('./constants.js'), database = require('./database.js'), - externalLdap = require('./externalldap.js'), safe = require('safetydance'), uuid = require('uuid'); @@ -156,26 +156,23 @@ async function getMembership(userId) { return result.map(function (r) { return r.groupId; }); } -async function setMembership(user, groupIds) { - assert.strictEqual(typeof user, 'object'); - assert(Array.isArray(groupIds)); +async function setLocalMembership(user, localGroupIds) { + assert.strictEqual(typeof user, 'object'); // can be local or external + assert(Array.isArray(localGroupIds)); - for (const groupId of groupIds) { + // ensure groups are actually local + for (const groupId of localGroupIds) { const group = await get(groupId); if (!group) throw new BoxError(BoxError.NOT_FOUND, `Group ${groupId} not found`); if (group.source) throw new BoxError(BoxError.BAD_STATE, 'Cannot set members of external group'); } - if (user.source === 'ldap') { - const config = await externalLdap.getConfig(); - if (config.syncGroups) throw new BoxError(BoxError.BAD_STATE, 'Cannot set groups of external user when syncing groups'); - } - - let queries = [ ]; - queries.push({ query: 'DELETE from groupMembers WHERE userId = ?', args: [ user.id ] }); - groupIds.forEach(function (gid) { + const queries = []; + // a remote user may already be part of some external groups. do not clear those because remote groups are non-editable + queries.push({ query: 'DELETE FROM groupMembers WHERE userId = ? AND groupId IN (SELECT id FROM userGroups WHERE source = ?)', args: [ user.id, '' ] }); + for (const gid of localGroupIds) { queries.push({ query: 'INSERT INTO groupMembers (groupId, userId) VALUES (? , ?)', args: [ gid, user.id ] }); - }); + } const [error] = await safe(database.transaction(queries)); if (error && error.code === 'ER_NO_REFERENCED_ROW_2') throw new BoxError(BoxError.NOT_FOUND, 'Group not found'); diff --git a/src/routes/users.js b/src/routes/users.js index b23b91f1d..455b3a3bd 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -12,7 +12,7 @@ exports = module.exports = { setPassword, verifyPassword, - setGroups, + setLocalGroups, setGhost, getPasswordResetLink, @@ -190,14 +190,14 @@ async function disableTwoFactorAuthentication(req, res, next) { next(new HttpSuccess(200, {})); } -async function setGroups(req, res, next) { +async function setLocalGroups(req, res, next) { assert.strictEqual(typeof req.body, 'object'); assert.strictEqual(typeof req.resource, 'object'); if (!Array.isArray(req.body.groupIds)) return next(new HttpError(400, 'API call requires a groups array.')); 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}'`)); - const [error] = await safe(groups.setMembership(req.resource, req.body.groupIds)); + const [error] = await safe(groups.setLocalMembership(req.resource, req.body.groupIds)); if (error) return next(BoxError.toHttpError(error)); next(new HttpSuccess(204)); diff --git a/src/server.js b/src/server.js index fd2cb57f8..87032f900 100644 --- a/src/server.js +++ b/src/server.js @@ -195,7 +195,7 @@ async function initializeExpressSync() { router.post('/api/v1/users/:userId/profile', json, token, authorizeUserManager, routes.users.load, routes.users.updateProfile); router.post('/api/v1/users/:userId/password', json, token, authorizeUserManager, routes.users.load, routes.users.setPassword); router.post('/api/v1/users/:userId/ghost', json, token, authorizeAdmin, routes.users.load, routes.users.setGhost); - router.put ('/api/v1/users/:userId/groups', json, token, authorizeUserManager, routes.users.load, routes.users.setGroups); + router.put ('/api/v1/users/:userId/groups', json, token, authorizeUserManager, routes.users.load, routes.users.setLocalGroups); router.get ('/api/v1/users/:userId/password_reset_link', json, token, authorizeUserManager, routes.users.load, routes.users.getPasswordResetLink); router.post('/api/v1/users/:userId/send_password_reset_email', json, token, authorizeUserManager, routes.users.load, routes.users.sendPasswordResetEmail); router.get ('/api/v1/users/:userId/invite_link', json, token, authorizeUserManager, routes.users.load, routes.users.getInviteLink); diff --git a/src/test/groups-test.js b/src/test/groups-test.js index 7e550d746..a74cf51b1 100644 --- a/src/test/groups-test.js +++ b/src/test/groups-test.js @@ -132,20 +132,22 @@ describe('Groups', function () { }); it('can set group membership', async function () { - await groups.setMembership(admin, [ group0Object.id ]); + await groups.setLocalMembership(admin, [ group0Object.id ]); + const groupIds = await groups._getMembership(admin.id); + expect(groupIds.length).to.be(1); }); it('cannot set user to same group twice', async function () { - const [error] = await safe(groups.setMembership(admin, [ group0Object.id, group0Object.id ])); + const [error] = await safe(groups.setLocalMembership(admin, [ group0Object.id, group0Object.id ])); expect(error.reason).to.be(BoxError.CONFLICT); }); it('can set user to multiple groups', async function () { - await groups.setMembership(admin, [ group0Object.id, group1Object.id ]); + await groups.setLocalMembership(admin, [ group0Object.id, group1Object.id ]); }); it('can get groups membership', async function () { - const groupIds = await groups.getMembership(admin.id); + const groupIds = await groups._getMembership(admin.id); expect(groupIds.length).to.be(2); expect(groupIds.sort()).to.eql([ group0Object.id, group1Object.id ].sort()); }); @@ -224,8 +226,17 @@ describe('Groups', function () { }); it('cannot set membership', async function () { - const [error] = await safe(groups.setMembership(admin, [ ldapGroup.id ])); + const [error] = await safe(groups.setLocalMembership(admin, [ ldapGroup.id ])); expect(error.reason).to.be(BoxError.BAD_STATE); }); + + it('does not clear remote membership', async function () { + await groups.setMembers(ldapGroup, [ admin.id ], { skipSourceCheck: true }); // would be called by ldap syncer + await groups.setLocalMembership(admin, [ group1Object.id ]); + + const groupIds = await groups._getMembership(admin.id); + expect(groupIds.length).to.be(2); + expect(groupIds.sort()).to.eql([ group1Object.id, ldapGroup.id ].sort()); + }); }); });