diff --git a/src/externalldap.js b/src/externalldap.js index f19356cc4..32ed6c656 100644 --- a/src/externalldap.js +++ b/src/externalldap.js @@ -396,14 +396,11 @@ async function syncGroups(config, progressCallback) { let percent = 40; let step = 30/(ldapGroups.length+1); // ensure no divide by 0 - // we ignore all non internal errors here and just log them for now for (const ldapGroup of ldapGroups) { let groupName = ldapGroup[config.groupnameField]; if (!groupName) return; - // some servers return empty array for unknown properties :-/ - if (typeof groupName !== 'string') return; + if (typeof groupName !== 'string') return; // some servers return empty array for unknown properties :-/ - // groups are lowercase groupName = groupName.toLowerCase(); percent += step; @@ -413,10 +410,13 @@ async function syncGroups(config, progressCallback) { if (!result) { debug(`syncGroups: [adding group] groupname=${groupName}`); - const [error] = await safe(groups.add({ name: groupName, source: 'ldap' })); if (error) debug('syncGroups: Failed to create group', groupName, error); } else { + // convert local group to ldap group. 2 reasons: + // 1. we reset source flag when externalldap is disabled. if we renable, it automatically coverts + // 2. externalldap connector usually implies user wants to user external users/groups. + groups.update(result.id, { source: 'ldap' }); debug(`syncGroups: [up-to-date group] groupname=${groupName}`); } } @@ -486,7 +486,7 @@ async function syncGroupMembers(config, progressCallback) { userIds.push(userObject.id); } - const [setError] = await safe(groups.setMembers(group.id, userIds)); + const [setError] = await safe(groups.setMembers(group, userIds, { skipSourceCheck: true })); if (setError) debug(`syncGroupMembers: Failed to set members of group ${group.name}. %o`, setError); } diff --git a/src/groups.js b/src/groups.js index 0dbde9de1..39f1c25c9 100644 --- a/src/groups.js +++ b/src/groups.js @@ -6,6 +6,7 @@ exports = module.exports = { get, getByName, + update, setName, getWithMembers, @@ -27,13 +28,14 @@ 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'); const GROUPS_FIELDS = [ 'id', 'name', 'source' ].join(','); // keep this in sync with validateUsername -function validateGroupname(name) { +function validateName(name) { assert.strictEqual(typeof name, 'string'); if (name.length < 1) return new BoxError(BoxError.BAD_FIELD, 'name must be atleast 1 char'); @@ -47,7 +49,7 @@ function validateGroupname(name) { return null; } -function validateGroupSource(source) { +function validateSource(source) { assert.strictEqual(typeof source, 'string'); if (source !== '' && source !== 'ldap') return new BoxError(BoxError.BAD_FIELD, 'source must be "" or "ldap"'); @@ -63,10 +65,10 @@ async function add(group) { name = name.toLowerCase(); // we store names in lowercase source = source || ''; - let error = validateGroupname(name); + let error = validateName(name); if (error) throw error; - error = validateGroupSource(source); + error = validateSource(source); if (error) throw error; const id = `gid-${uuid.v4()}`; @@ -154,14 +156,19 @@ async function getMembership(userId) { return result.map(function (r) { return r.groupId; }); } -async function setMembership(userId, groupIds) { - assert.strictEqual(typeof userId, 'string'); +async function setMembership(user, groupIds) { + assert.strictEqual(typeof user, 'object'); assert(Array.isArray(groupIds)); + 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: [ userId ] }); + queries.push({ query: 'DELETE from groupMembers WHERE userId = ?', args: [ user.id ] }); groupIds.forEach(function (gid) { - queries.push({ query: 'INSERT INTO groupMembers (groupId, userId) VALUES (? , ?)', args: [ gid, userId ] }); + queries.push({ query: 'INSERT INTO groupMembers (groupId, userId) VALUES (? , ?)', args: [ gid, user.id ] }); }); const [error] = await safe(database.transaction(queries)); @@ -170,14 +177,17 @@ async function setMembership(userId, groupIds) { if (error) throw error; } -async function setMembers(groupId, userIds) { - assert.strictEqual(typeof groupId, 'string'); +async function setMembers(group, userIds, options) { + assert.strictEqual(typeof group, 'object'); assert(Array.isArray(userIds)); + assert.strictEqual(typeof options, 'object'); - let queries = []; - queries.push({ query: 'DELETE FROM groupMembers WHERE groupId = ?', args: [ groupId ] }); + if (!options.skipSourceCheck && group.source === 'ldap') throw new BoxError(BoxError.BAD_STATE, 'Cannot set members of external group'); + + const queries = []; + queries.push({ query: 'DELETE FROM groupMembers WHERE groupId = ?', args: [ group.id ] }); for (let i = 0; i < userIds.length; i++) { - queries.push({ query: 'INSERT INTO groupMembers (groupId, userId) VALUES (?, ?)', args: [ groupId, userIds[i] ] }); + queries.push({ query: 'INSERT INTO groupMembers (groupId, userId) VALUES (?, ?)', args: [ group.id, userIds[i] ] }); } const [error] = await safe(database.transaction(queries)); @@ -202,20 +212,46 @@ async function isMember(groupId, userId) { return result.length !== 0; } +async function update(id, data) { + assert.strictEqual(typeof id, 'string'); + assert(data && typeof data === 'object'); + + if ('name' in data) { + assert.strictEqual(typeof data.name, 'string'); + data.name = data.name.toLowerCase(); + const error = validateName(data.name); + if (error) throw error; + } + + if ('source' in data) { + assert.strictEqual(typeof data.source, 'string'); + const error = validateSource(data.source); + if (error) throw error; + } + + const args = []; + const fields = []; + for (const k in data) { + if (k === 'name' || k === 'source') { + fields.push(k + ' = ?'); + args.push(data[k]); + } + } + args.push(id); + + const [updateError, result] = await safe(database.query('UPDATE userGroups SET ' + fields.join(', ') + ' WHERE id = ?', args)); + if (updateError && updateError.code === 'ER_DUP_ENTRY' && updateError.sqlMessage.indexOf('userGroups_name') !== -1) throw new BoxError(BoxError.ALREADY_EXISTS, 'name already exists'); + if (updateError) throw updateError; + if (result.affectedRows !== 1) throw new BoxError(BoxError.NOT_FOUND, 'Group not found'); +} + async function setName(group, name) { assert.strictEqual(typeof group, 'object'); assert.strictEqual(typeof name, 'string'); if (group.source === 'ldap') throw new BoxError(BoxError.BAD_STATE, 'Cannot set name of external group'); - name = name.toLowerCase(); // we store names in lowercase - const error = validateGroupname(name); - if (error) throw error; - - const [updateError, result] = await safe(database.query('UPDATE userGroups SET name = ? WHERE id = ?', [ name, group.id ])); - if (updateError && updateError.code === 'ER_DUP_ENTRY' && updateError.sqlMessage.indexOf('userGroups_name') !== -1) throw new BoxError(BoxError.ALREADY_EXISTS, 'name already exists'); - if (updateError) throw updateError; - if (result.affectedRows !== 1) throw new BoxError(BoxError.NOT_FOUND, 'Group not found'); + await update(group.id, { name }); } async function resetSource() { diff --git a/src/routes/groups.js b/src/routes/groups.js index 396ead98c..56a73d764 100644 --- a/src/routes/groups.js +++ b/src/routes/groups.js @@ -59,7 +59,7 @@ async function setName(req, res, next) { } async function setMembers(req, res, next) { - assert.strictEqual(typeof req.params.groupId, 'string'); + assert.strictEqual(typeof req.resource, 'object'); if (req.resource.source === 'ldap') return next(new HttpError(409, 'members of an external group cannot be set')); @@ -67,7 +67,7 @@ async function setMembers(req, res, next) { if (!Array.isArray(req.body.userIds)) return next(new HttpError(404, 'userIds must be an array')); if (req.body.userIds.some((u) => typeof u !== 'string')) return next(new HttpError(400, 'userIds array must contain strings')); - const [error] = await safe(groups.setMembers(req.params.groupId, req.body.userIds)); + const [error] = await safe(groups.setMembers(req.resource, req.body.userIds, { skipSourceCheck: false })); if (error) return next(BoxError.toHttpError(error)); next(new HttpSuccess(200, { })); diff --git a/src/routes/users.js b/src/routes/users.js index ac0e25235..3b97248b0 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -170,7 +170,7 @@ async function setGroups(req, res, next) { 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.id, req.body.groupIds)); + const [error] = await safe(groups.setMembership(req.resource, req.body.groupIds)); if (error) return next(BoxError.toHttpError(error)); next(new HttpSuccess(204)); diff --git a/src/test/directoryserver-test.js b/src/test/directoryserver-test.js index 58e382e4f..4cf1fe188 100644 --- a/src/test/directoryserver-test.js +++ b/src/test/directoryserver-test.js @@ -85,11 +85,11 @@ describe('Directory Server (LDAP)', function () { directoryServer.setConfig.bind(null, { enabled: true, secret: auth.secret, allowlist: '127.0.0.1' }, auditSource), async () => { group = await groups.add({ name: 'ldap-test-1' }); - await groups.setMembers(group.id, [ admin.id, user.id ]); + await groups.setMembers(group, [ admin.id, user.id ], {}); }, async () => { group2 = await groups.add({ name: 'ldap-test-2' }); - await groups.setMembers(group2.id, [ admin.id ]); + await groups.setMembers(group2, [ admin.id ], {}); } ], done); }); diff --git a/src/test/externalldap-test.js b/src/test/externalldap-test.js index 965a624aa..e6d4ffc58 100644 --- a/src/test/externalldap-test.js +++ b/src/test/externalldap-test.js @@ -397,7 +397,7 @@ describe('External LDAP', function () { await externalLdap.sync(function progress() {}); const result = await users.list(); expect(result.find(function (u) { - return u.username === 'firstuser' && u.email === 'first@user.com' && u.displayName === 'First User'; + return u.username === 'firstuser' && u.email === 'first@user.com' && u.displayName === 'First User' && u.source === 'ldap'; })).to.be.ok(); }); @@ -408,11 +408,11 @@ describe('External LDAP', function () { await externalLdap.sync(function progress() {}); const result = await users.list(); expect(result.find(function (u) { - return u.username === 'firstuser' && u.email === 'first@changed.com' && u.displayName === 'User First'; + return u.username === 'firstuser' && u.email === 'first@changed.com' && u.displayName === 'User First' && u.source === 'ldap'; })).to.be.ok(); }); - it('mapps already existing users with same username', async function () { + it('maps already existing users with same username', async function () { gLdapUsers.push({ username: admin.username, displayName: 'Something Else', @@ -436,6 +436,11 @@ describe('External LDAP', function () { expect(result.length).to.equal(0); }); + it('can set groups of external user when group sync is disabled', async function () { + const user = await users.getByUsername(gLdapUsers[0].username); + await groups.setMembership(user, []); + }); + it('enable with groupSync', async function () { let conf = Object.assign({}, LDAP_CONFIG); conf.syncGroups = true; @@ -458,7 +463,7 @@ describe('External LDAP', function () { await externalLdap.sync(function progress() {}); const result = await groups.list(); expect(result.find(function (g) { - return g.name === 'extgroup1'; + return g.name === 'extgroup1' && g.source === 'ldap'; })).to.be.ok(); }); @@ -471,11 +476,11 @@ describe('External LDAP', function () { const result = await groups.list(); expect(result.length).to.be(2); expect(result.find(function (g) { - return g.name === 'extgroup2'; + return g.name === 'extgroup2' && g.source === 'ldap'; })).to.be.ok(); }); - it('does not create already existing group', async function () { + it('does not create or change already existing group', async function () { gLdapGroups.push({ groupname: 'INTERNALgroup' // also tests lowercasing }); @@ -485,6 +490,9 @@ describe('External LDAP', function () { const result = await groups.list(); expect(result.length).to.equal(3); + expect(result.find(function (g) { + return g.name === 'internalgroup' && g.source === 'ldap'; // source is updated to LDAP + })).to.be.ok(); }); it('adds users of groups', async function () { @@ -530,6 +538,12 @@ describe('External LDAP', function () { const u = await users.get(result2[0]); expect(u.username).to.equal(gLdapUsers[0].username); }); + + it('cannot set groups of external user when group sync is disabled', async function () { + const user = await users.getByUsername(gLdapUsers[0].username); + const [error] = await safe(groups.setMembership(user, [])); + expect(error.reason).to.be(BoxError.BAD_STATE); + }); }); describe('user auto creation', function () { diff --git a/src/test/groups-test.js b/src/test/groups-test.js index 386fd7355..98f07e43e 100644 --- a/src/test/groups-test.js +++ b/src/test/groups-test.js @@ -18,10 +18,10 @@ describe('Groups', function () { before(setup); after(cleanup); - describe('add/get/del', function () { - let group0Name = 'administrators', group0Object; - let group1Name = 'externs', group1Object; + let group0Name = 'administrators', group0Object; + let group1Name = 'externs', group1Object; + describe('add', function () { it('cannot add group - too small', async function () { const [error] = await safe(groups.add({ name: '' })); expect(error.reason).to.be(BoxError.BAD_FIELD); @@ -72,7 +72,9 @@ describe('Groups', function () { const [error] = await safe(groups.add({name: group0Name, source: 'ldap' })); expect(error.reason).to.be(BoxError.ALREADY_EXISTS); }); + }); + describe('get', function () { it('cannot get invalid group', async function () { const result = await groups.get('sometrandom'); expect(result).to.be(null); @@ -82,18 +84,20 @@ describe('Groups', function () { const result = await groups.get(group0Object.id); expect(result.name).to.equal(group0Name); }); + }); + describe('members', function () { it('isMember returns false', async function () { const isMember = await groups.isMember(group0Object.id, admin.id); expect(isMember).to.be(false); }); it('can set members', async function () { - await groups.setMembers(group0Object.id, [ admin.id, user.id ]); + await groups.setMembers(group0Object, [ admin.id, user.id ], {}); }); it('cannot set duplicate members', async function () { - const [error] = await safe(groups.setMembers(group0Object.id, [ admin.id, user.id, admin.id ])); + const [error] = await safe(groups.setMembers(group0Object, [ admin.id, user.id, admin.id ], {})); expect(error.reason).to.be(BoxError.CONFLICT); }); @@ -127,17 +131,17 @@ describe('Groups', function () { expect(result.userIds).to.eql([ admin.id ]); }); - it('can set groups', async function () { - await groups.setMembership(admin.id, [ group0Object.id ]); + it('can set group membership', async function () { + await groups.setMembership(admin, [ group0Object.id ]); }); it('cannot set user to same group twice', async function () { - const [error] = await safe(groups.setMembership(admin.id, [ group0Object.id, group0Object.id ])); + const [error] = await safe(groups.setMembership(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.id, [ group0Object.id, group1Object.id ]); + await groups.setMembership(admin, [ group0Object.id, group1Object.id ]); }); it('can get groups membership', async function () { @@ -145,7 +149,9 @@ describe('Groups', function () { expect(groupIds.length).to.be(2); expect(groupIds.sort()).to.eql([ group0Object.id, group1Object.id ].sort()); }); + }); + describe('list', function () { it('can list', async function () { const result = await groups.list(); expect(result.length).to.be(2); @@ -160,14 +166,16 @@ describe('Groups', function () { expect(result[1].userIds).to.eql([ admin.id ]); expect(result[1].name).to.be(group1Name); }); + }); + describe('delete', function () { it('cannot delete invalid group', async function () { const [error] = await safe(groups.remove('random')); expect(error.reason).to.be(BoxError.NOT_FOUND); }); it('can delete valid group', async function () { - await groups.setMembers(group0Object.id, [ admin.id, user.id ]); // ensure group has some members + await groups.setMembers(group0Object, [ admin.id, user.id ], {}); // ensure group has some members await groups.remove(group0Object.id); }); }); diff --git a/src/test/ldapserver-test.js b/src/test/ldapserver-test.js index 928bf7d8d..a96c85374 100644 --- a/src/test/ldapserver-test.js +++ b/src/test/ldapserver-test.js @@ -78,11 +78,11 @@ describe('Ldap', function () { ldapServer.start.bind(null), async () => { group = await groups.add({ name: 'ldap-test-1' }); - await groups.setMembers(group.id, [ admin.id, user.id ]); + await groups.setMembers(group, [ admin.id, user.id ], {}); }, async () => { group2 = await groups.add({ name: 'ldap-test-2' }); - await groups.setMembers(group2.id, [ admin.id ]); + await groups.setMembers(group2, [ admin.id ], {}); } ], done);