diff --git a/dashboard/src/js/client.js b/dashboard/src/js/client.js index d400bdc0d..22ae13f68 100644 --- a/dashboard/src/js/client.js +++ b/dashboard/src/js/client.js @@ -1763,12 +1763,12 @@ angular.module('Application').service('Client', ['$http', '$interval', '$timeout }); }; - Client.prototype.updateGroup = function (id, name, callback) { + Client.prototype.setGroupName = function (id, name, callback) { var data = { name: name }; - post('/api/v1/groups/' + id, data, null, function (error, data, status) { + put('/api/v1/groups/' + id + '/name', data, null, function (error, data, status) { if (error) return callback(error); if (status !== 200) return callback(new ClientError(status, data)); diff --git a/dashboard/src/views/users.js b/dashboard/src/views/users.js index 6c0ce05c0..c179276f1 100644 --- a/dashboard/src/views/users.js +++ b/dashboard/src/views/users.js @@ -610,7 +610,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio if ($scope.groupEdit.source) return $scope.groupEdit.updateAccessRestriction(); // cannot update name or members of external groups - Client.updateGroup($scope.groupEdit.groupInfo.id, $scope.groupEdit.name, function (error) { + Client.setGroupName($scope.groupEdit.groupInfo.id, $scope.groupEdit.name, function (error) { if (error) { $scope.groupEdit.busy = false; diff --git a/src/groups.js b/src/groups.js index 4849f712e..0dbde9de1 100644 --- a/src/groups.js +++ b/src/groups.js @@ -5,7 +5,9 @@ exports = module.exports = { remove, get, getByName, - update, + + setName, + getWithMembers, list, listWithMembers, @@ -73,7 +75,7 @@ async function add(group) { if (error && error.code === 'ER_DUP_ENTRY') throw new BoxError(BoxError.ALREADY_EXISTS, error); if (error) throw error; - return { id, name }; + return { id, name, source }; } async function remove(id) { @@ -200,34 +202,22 @@ async function isMember(groupId, userId) { return result.length !== 0; } -async function update(id, data) { - assert.strictEqual(typeof id, 'string'); - assert(data && typeof data === 'object'); +async function setName(group, name) { + assert.strictEqual(typeof group, 'object'); + assert.strictEqual(typeof name, 'string'); - if ('name' in data) { - assert.strictEqual(typeof data.name, 'string'); - const error = validateGroupname(data.name); - if (error) throw error; - } + if (group.source === 'ldap') throw new BoxError(BoxError.BAD_STATE, 'Cannot set name of external group'); - const args = []; - const fields = []; - for (const k in data) { - if (k === 'name') { - assert.strictEqual(typeof data.name, 'string'); - fields.push(k + ' = ?'); - args.push(data.name); - } - } - args.push(id); + 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 ' + fields.join(', ') + ' WHERE id = ?', args)); + 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'); } - async function resetSource() { await database.query('UPDATE userGroups SET source = ?', [ '' ]); } diff --git a/src/routes/groups.js b/src/routes/groups.js index ec74de054..396ead98c 100644 --- a/src/routes/groups.js +++ b/src/routes/groups.js @@ -6,7 +6,7 @@ exports = module.exports = { get, list, add, - update, + setName, remove, setMembers }; @@ -47,15 +47,12 @@ async function get(req, res, next) { next(new HttpSuccess(200, req.resource)); } -async function update(req, res, next) { - assert.strictEqual(typeof req.params.groupId, 'string'); +async function setName(req, res, next) { + assert.strictEqual(typeof req.resource, 'object'); assert.strictEqual(typeof req.body, 'object'); - if (req.resource.source === 'ldap') return next(new HttpError(409, 'external group cannot be updated')); - - if ('name' in req.body && typeof req.body.name !== 'string') return next(new HttpError(400, 'name must be a string')); - - const [error] = await safe(groups.update(req.params.groupId, req.body)); + if (typeof req.body.name !== 'string') return next(new HttpError(400, 'name must be a string')); + const [error] = await safe(groups.setName(req.resource, req.body.name)); if (error) return next(BoxError.toHttpError(error)); next(new HttpSuccess(200, { })); diff --git a/src/routes/test/groups-test.js b/src/routes/test/groups-test.js index 9eb58854f..ab3553335 100644 --- a/src/routes/test/groups-test.js +++ b/src/routes/test/groups-test.js @@ -150,6 +150,32 @@ describe('Groups API', function () { expect(response.body.groups[1].name).to.eql(group1Object.name); }); + it('cannot set missing group name', async function () { + const response = await superagent.put(`${serverUrl}/api/v1/groups/${group1Object.id}/name`) + .query({ access_token: owner.token }) + .send({ }) + .ok(() => true); + + expect(response.statusCode).to.equal(400); + }); + + it('can set invalid group name', async function () { + const response = await superagent.put(`${serverUrl}/api/v1/groups/${group1Object.id}/name`) + .query({ access_token: owner.token }) + .send({ name: '!group1-newname'}) + .ok(() => true); + + expect(response.statusCode).to.equal(400); + }); + + it('can set group name', async function () { + const response = await superagent.put(`${serverUrl}/api/v1/groups/${group1Object.id}/name`) + .query({ access_token: owner.token }) + .send({ name: 'group1-newname'}); + + expect(response.statusCode).to.equal(200); + }); + it('remove user from group', async function () { const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/groups`) .query({ access_token: owner.token }) diff --git a/src/server.js b/src/server.js index cb7ad2d0e..220bf34e8 100644 --- a/src/server.js +++ b/src/server.js @@ -204,7 +204,7 @@ async function initializeExpressSync() { router.post('/api/v1/groups', json, token, authorizeUserManager, routes.groups.add); router.get ('/api/v1/groups/:groupId', token, authorizeUserManager, routes.groups.load, routes.groups.get); router.put ('/api/v1/groups/:groupId/members', json, token, authorizeUserManager, routes.groups.load, routes.groups.setMembers); - router.post('/api/v1/groups/:groupId', json, token, authorizeUserManager, routes.groups.load, routes.groups.update); + router.put ('/api/v1/groups/:groupId/name', json, token, authorizeUserManager, routes.groups.load, routes.groups.setName); router.del ('/api/v1/groups/:groupId', token, authorizeUserManager, routes.groups.load, routes.groups.remove); // User directory diff --git a/src/test/groups-test.js b/src/test/groups-test.js index 77591c83f..386fd7355 100644 --- a/src/test/groups-test.js +++ b/src/test/groups-test.js @@ -57,7 +57,7 @@ describe('Groups', function () { expect(error).to.be(null); group0Object = result; - [error, result] = await safe(groups.add({ name: group1Name })); + [error, result] = await safe(groups.add({ name: group1Name, source: 'ldap' })); expect(error).to.be(null); group1Object = result; }); @@ -171,4 +171,36 @@ describe('Groups', function () { await groups.remove(group0Object.id); }); }); + + describe('update', function () { + let groupObject; + + before(async function () { + let [error, result] = await safe(groups.add({ name: 'kootam' })); + expect(error).to.be(null); + groupObject = result; + }); + + it('cannot set empty group name', async function () { + const [error] = await safe(groups.setName(groupObject, '')); + expect(error.reason).to.be(BoxError.BAD_FIELD); + }); + + it('cannot set bad group name', async function () { + const [error] = await safe(groups.setName(groupObject, '!kootam')); + expect(error.reason).to.be(BoxError.BAD_FIELD); + }); + + it('can set group name', async function () { + await groups.setName(groupObject, 'kootam2'); + groupObject = await groups.get(groupObject.id); + expect(groupObject.name).to.be('kootam2'); + }); + + it('cannot change ldap group name', async function () { + const result = await groups.add({ name: 'ldap-kootam', source: 'ldap' }); + const [error] = await safe(groups.setName(result, 'ldap-kootam2')); + expect(error.reason).to.be(BoxError.BAD_STATE); + }); + }); });