diff --git a/dashboard/src/js/client.js b/dashboard/src/js/client.js index 22ae13f68..8c403f4df 100644 --- a/dashboard/src/js/client.js +++ b/dashboard/src/js/client.js @@ -2273,17 +2273,26 @@ angular.module('Application').service('Client', ['$http', '$interval', '$timeout }); }; - Client.prototype.updateUser = function (user, callback) { - var data = { - email: user.email, - displayName: user.displayName, - fallbackEmail: user.fallbackEmail, - active: user.active, - role: user.role - }; - if (user.username) data.username = user.username; + Client.prototype.updateUserProfile = function (userId, data, callback) { + post('/api/v1/users/' + userId + '/profile', data, null, function (error, data, status) { + if (error) return callback(error); + if (status !== 204) return callback(new ClientError(status, data)); - post('/api/v1/users/' + user.id, data, null, function (error, data, status) { + callback(null); + }); + }; + + Client.prototype.setRole = function (userId, role, callback) { + put('/api/v1/users/' + userId + '/role', { role: role }, null, function (error, data, status) { + if (error) return callback(error); + if (status !== 204) return callback(new ClientError(status, data)); + + callback(null); + }); + }; + + Client.prototype.setActive = function (userId, active, callback) { + put('/api/v1/users/' + userId + '/active', { active: active }, null, function (error, data, status) { if (error) return callback(error); if (status !== 204) return callback(new ClientError(status, data)); diff --git a/dashboard/src/views/users.js b/dashboard/src/views/users.js index 4818f1f13..7ccfb7999 100644 --- a/dashboard/src/views/users.js +++ b/dashboard/src/views/users.js @@ -186,7 +186,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio $scope.userImport.busy = false; $scope.userImport.done = true; if ($scope.userImport.success) { - refresh(); + refreshCurrentPage(); refreshAllUsers(); } }); @@ -254,7 +254,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio $scope.userRemove.userInfo = {}; - refresh(); + refreshCurrentPage(); refreshAllUsers(); $('#userRemoveModal').modal('hide'); @@ -353,7 +353,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio if ($scope.userAdd.sendInvite) Client.sendInviteEmail(userId, user.email, function (error) { if (error) console.error('Failed to send invite.', error); }); - refresh(); + refreshCurrentPage(); refreshAllUsers(); $('#userAddModal').modal('hide'); @@ -402,29 +402,36 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio $scope.userEdit.busy = true; var userId = $scope.userEdit.userInfo.id; - var data = { - id: userId - }; - // only send if not the current active user - if (userId !== $scope.userInfo.id) { - data.active = $scope.userEdit.active; - data.role = $scope.userEdit.role; - } + async.series([ + function setRole(next) { + if (userId === $scope.userInfo.id) return next(); // cannot set role on self + Client.setRole(userId, $scope.userEdit.role, next); + }, + function setActive(next) { + if (userId === $scope.userInfo.id) return next(); // cannot set role on self + Client.setActive(userId, $scope.userEdit.active, next); + }, + function updateUserProfile(next) { + if ($scope.userEdit.source) return next(); // cannot update profile of external user + // username is settable only if it was empty previously. it's editable for the "lock" profiles feature + var data = {}; + if (!$scope.userEdit.userInfo.username) data.username = $scope.userEdit.username; + data.email = $scope.userEdit.email; + data.displayName = $scope.userEdit.displayName; + data.fallbackEmail = $scope.userEdit.fallbackEmail; + Client.updateUserProfile(userId, data, next); + }, + function setGroups(next) { + if ($scope.config.ldapGroupsSynced) return next(); // cannot update user groups when syncing ldap groups - // only change those if it is a local user - if (!$scope.userEdit.source) { - // username is settable only if it was empty previously. it's editable for the "lock" profiles feature - if (!$scope.userEdit.userInfo.username) data.username = $scope.userEdit.username; - data.email = $scope.userEdit.email; - data.displayName = $scope.userEdit.displayName; - data.fallbackEmail = $scope.userEdit.fallbackEmail; - } + var groupIds = $scope.userEdit.selectedGroups.map(function (g) { return g.id; }); + Client.setGroups(userId, groupIds, next); + } + ], function (error) { + $scope.userEdit.busy = false; - Client.updateUser(data, function (error) { if (error) { - $scope.userEdit.busy = false; - if (error.statusCode === 409) { if (error.message.toLowerCase().indexOf('email') !== -1) { $scope.userEdit.error.email = 'Email already taken'; @@ -441,19 +448,9 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio return; } - if ($scope.config.ldapGroupsSynced) return; // cannot update user groups when syncing ldap groups - - var groupIds = $scope.userEdit.selectedGroups.map(function (g) { return g.id; }); - - Client.setGroups(data.id, groupIds, function (error) { - $scope.userEdit.busy = false; - - if (error) return console.error('Unable to update groups for user:', error); - - refreshUsers(false); - - $('#userEditModal').modal('hide'); - }); + refreshUsersCurrentPage(false /* busy indicator */); + if (!$scope.config.ldapGroupsSynced) refreshGroups(); + $('#userEditModal').modal('hide'); }); }, @@ -519,7 +516,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio if (error) return console.error('Unable to add memebers.', error.statusCode, error.message); - refresh(); + refreshCurrentPage(); $('#groupAddModal').modal('hide'); }); @@ -596,7 +593,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio $scope.groupEdit.busy = false; if (error) return console.error('Unable to set removed app access.', error.statusCode, error.message); - refresh(); + refreshCurrentPage(); // refresh apps to reflect change Client.refreshInstalledApps(); @@ -676,7 +673,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio if (error) return console.error('Unable to remove group.', error.statusCode, error.message); - refresh(); + refreshCurrentPage(); $('#groupRemoveModal').modal('hide'); }); } @@ -807,7 +804,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio } }; - function getUsers(callback) { + function getUsersCurrentPage(callback) { var users = []; Client.getUsers($scope.userSearchString, $scope.userStateFilter.value, $scope.currentPage, $scope.pageItems, function (error, results) { @@ -827,10 +824,10 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio }); } - function refreshUsers(showBusy) { // loads users on current page only + function refreshUsersCurrentPage(showBusy) { // loads users on current page only if (showBusy) $scope.userRefreshBusy = true; - getUsers(function (error, result) { + getUsersCurrentPage(function (error, result) { if (error) return console.error('Unable to get user listing.', error); angular.copy(result, $scope.users); @@ -857,26 +854,26 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio }); } - function refresh() { + function refreshCurrentPage() { refreshGroups(function (error) { if (error) return console.error('Unable to get group listing.', error); - refreshUsers(true); + refreshUsersCurrentPage(true /* busy indicator */); }); } $scope.showNextPage = function () { $scope.currentPage++; - refreshUsers(); + refreshUsersCurrentPage(false /* no busy indicator */); }; $scope.showPrevPage = function () { if ($scope.currentPage > 1) $scope.currentPage--; else $scope.currentPage = 1; - refreshUsers(); + refreshUsersCurrentPage(false /* no busy indicator */); }; $scope.updateFilter = function () { - refreshUsers(); + refreshUsersCurrentPage(false /* no busy indicator */); }; function refreshAllUsers() { // this loads all users on Cloudron, not just current page @@ -893,7 +890,7 @@ angular.module('Application').controller('UsersController', ['$scope', '$locatio } Client.onReady(function () { - refresh(); + refreshCurrentPage(); refreshAllUsers(); // Order matters for permissions used in canEdit diff --git a/src/routes/test/users-test.js b/src/routes/test/users-test.js index 74417dd34..744416296 100644 --- a/src/routes/test/users-test.js +++ b/src/routes/test/users-test.js @@ -213,7 +213,7 @@ describe('Users API', function () { describe('admin status', function () { it('set second user as admin succeeds', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_ADMIN }); @@ -229,7 +229,7 @@ describe('Users API', function () { }); it('make self as admin fails', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${owner.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${owner.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_ADMIN }) .ok(() => true); @@ -238,7 +238,7 @@ describe('Users API', function () { }); it('make self as normal user fails', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${owner.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${owner.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_USER }) .ok(() => true); @@ -247,7 +247,7 @@ describe('Users API', function () { }); it('remove second user as admin succeeds', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_USER }); @@ -255,7 +255,7 @@ describe('Users API', function () { }); it('normal user cannot change role of admin', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${owner.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${owner.id}/role`) .query({ access_token: user.token }) .send({ role: users.ROLE_USER }) .ok(() => true); @@ -307,9 +307,9 @@ describe('Users API', function () { }); }); - describe('update', function () { + describe('profile update', function () { it('change email fails due to missing token', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .send({ email: 'newemail@cloudron.local' }) .ok(() => true); @@ -317,7 +317,7 @@ describe('Users API', function () { }); it('change email fails due to invalid email', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({ email: 'newemail@cloudron' }) .ok(() => true); @@ -326,7 +326,7 @@ describe('Users API', function () { }); it('change fallbackEmail fails due to invalid email', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({ fallbackEmail: 'newemail@cloudron' }) .ok(() => true); @@ -335,7 +335,7 @@ describe('Users API', function () { }); it('change user succeeds without email nor displayName', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({}); @@ -344,7 +344,7 @@ describe('Users API', function () { it('change email succeeds', async function () { user2.email = 'NewEmail@cloudron.local'; - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({ email: user2.email }); @@ -360,7 +360,7 @@ describe('Users API', function () { }); it('cannot change email to existing one', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({ email: owner.email }) .ok(() => true); @@ -371,7 +371,7 @@ describe('Users API', function () { it('can change display name', async function () { const displayName = 'New name'; - const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}`) + const response = await superagent.post(`${serverUrl}/api/v1/users/${user2.id}/profile`) .query({ access_token: owner.token }) .send({ displayName: displayName }); @@ -416,9 +416,39 @@ describe('Users API', function () { }); }); + describe('active', function () { + it('can make user inactive', async function () { + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/active`) + .query({ access_token: owner.token }) + .send({ active: false }); + + expect(response.statusCode).to.equal(204); + + const response2 = await superagent.get(`${serverUrl}/api/v1/users/${user.id}`) + .query({ access_token: owner.token }); + + expect(response2.statusCode).to.equal(200); + expect(response2.body.active).to.equal(false); + }); + + it('can make user active', async function () { + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/active`) + .query({ access_token: owner.token }) + .send({ active: true }); + + expect(response.statusCode).to.equal(204); + + const response2 = await superagent.get(`${serverUrl}/api/v1/users/${user.id}`) + .query({ access_token: owner.token }); + + expect(response2.statusCode).to.equal(200); + expect(response2.body.active).to.equal(true); + }); + }); + describe('role - user manager', function () { it('can make normal user a usermanager', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_USER_MANAGER }); @@ -452,7 +482,7 @@ describe('Users API', function () { }); it('cannot change admin bit of another', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${owner.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${owner.id}/role`) .query({ access_token: user.token }) .send({ role: users.ROLE_ADMIN }) .ok(() => true); @@ -461,7 +491,7 @@ describe('Users API', function () { }); it('cannot change admin bit of self', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: user.token }) .send({ role: users.ROLE_ADMIN }) .ok(() => true); @@ -506,7 +536,7 @@ describe('Users API', function () { describe('role - mail manager', function () { it('can make normal user a usermanager', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: owner.token }) .send({ role: users.ROLE_MAIL_MANAGER }); @@ -523,7 +553,7 @@ describe('Users API', function () { }); it('cannot change admin bit of self', async function () { - const response = await superagent.post(`${serverUrl}/api/v1/users/${user.id}`) + const response = await superagent.put(`${serverUrl}/api/v1/users/${user.id}/role`) .query({ access_token: user.token }) .send({ role: users.ROLE_ADMIN }) .ok(() => true); diff --git a/src/routes/users.js b/src/routes/users.js index 3b97248b0..8178b923c 100644 --- a/src/routes/users.js +++ b/src/routes/users.js @@ -2,10 +2,14 @@ exports = module.exports = { get, - update, list, add, del, + + setRole, + setActive, + updateProfile, + setPassword, verifyPassword, setGroups, @@ -32,7 +36,8 @@ const assert = require('assert'), HttpError = require('connect-lastmile').HttpError, HttpSuccess = require('connect-lastmile').HttpSuccess, safe = require('safetydance'), - users = require('../users.js'); + users = require('../users.js'), + _ = require('underscore'); async function load(req, res, next) { assert.strictEqual(typeof req.params.userId, 'string'); @@ -70,7 +75,40 @@ async function add(req, res, next) { next(new HttpSuccess(201, { id })); } -async function update(req, res, next) { +async function setRole(req, res, next) { + assert.strictEqual(typeof req.resource, 'object'); + assert.strictEqual(typeof req.user, 'object'); + assert.strictEqual(typeof req.body, 'object'); + + if (typeof req.body.role !== 'string') return next(new HttpError(400, 'role must be a string')); + if (req.user.id === req.resource.id) return next(new HttpError(409, 'Cannot set role flag on self')); + + if (users.compareRoles(req.user.role, req.body.role) < 0) return next(new HttpError(403, `role '${req.body.role}' is required but you are only '${req.user.role}'`)); + if (users.compareRoles(req.user.role, req.resource.role) < 0) return next(new HttpError(403, `role '${req.resource.role}' is required but you are only '${req.user.role}'`)); + + const [error] = await safe(users.update(req.resource, { role: req.body.role }, AuditSource.fromRequest(req))); + if (error) return next(BoxError.toHttpError(error)); + + next(new HttpSuccess(204)); +} + +async function setActive(req, res, next) { + assert.strictEqual(typeof req.resource, 'object'); + assert.strictEqual(typeof req.user, 'object'); + assert.strictEqual(typeof req.body, 'object'); + + if (typeof req.body.active !== 'boolean') return next(new HttpError(400, 'active must be a boolean')); + if (req.user.id === req.resource.id) return next(new HttpError(409, 'Cannot set active flag on self')); + + if (users.compareRoles(req.user.role, req.resource.role) < 0) return next(new HttpError(403, `role '${req.resource.role}' is required but you are only '${req.user.role}'`)); + + const [error] = await safe(users.update(req.resource, { active: req.body.active }, AuditSource.fromRequest(req))); + if (error) return next(BoxError.toHttpError(error)); + + next(new HttpSuccess(204)); +} + +async function updateProfile(req, res, next) { assert.strictEqual(typeof req.resource, 'object'); assert.strictEqual(typeof req.user, 'object'); assert.strictEqual(typeof req.body, 'object'); @@ -79,23 +117,11 @@ async function update(req, res, next) { if ('email' in req.body && typeof req.body.email !== 'string') return next(new HttpError(400, 'email must be string')); if ('fallbackEmail' in req.body && typeof req.body.fallbackEmail !== 'string') return next(new HttpError(400, 'fallbackEmail must be string')); 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 ('role' in req.body) { - if (typeof req.body.role !== 'string') return next(new HttpError(400, 'role must be a string')); - if (req.user.id === req.resource.id) return next(new HttpError(409, 'Cannot set role flag on self')); - - if (users.compareRoles(req.user.role, req.body.role) < 0) return next(new HttpError(403, `role '${req.body.role}' is required but you are only '${req.user.role}'`)); - } - - if ('active' in req.body) { - if (typeof req.body.active !== 'boolean') return next(new HttpError(400, 'active must be a boolean')); - if (req.user.id === req.resource.id) return next(new HttpError(409, 'Cannot set active flag on self')); - } if (users.compareRoles(req.user.role, req.resource.role) < 0) return next(new HttpError(403, `role '${req.resource.role}' is required but you are only '${req.user.role}'`)); - const [error] = await safe(users.update(req.resource, req.body, AuditSource.fromRequest(req))); + const data = _.pick(req.body, 'username', 'email', 'fallbackEmail', 'displayName'); + const [error] = await safe(users.updateProfile(req.resource, data, AuditSource.fromRequest(req))); if (error) return next(BoxError.toHttpError(error)); next(new HttpSuccess(204)); diff --git a/src/server.js b/src/server.js index 220bf34e8..13398846a 100644 --- a/src/server.js +++ b/src/server.js @@ -189,7 +189,9 @@ async function initializeExpressSync() { router.post('/api/v1/users', json, token, authorizeUserManager, routes.users.add); 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.del); - router.post('/api/v1/users/:userId', json, token, authorizeUserManager, routes.users.load, routes.users.update); + router.put ('/api/v1/users/:userId/role', json, token, authorizeUserManager, routes.users.load, routes.users.setRole); + router.put ('/api/v1/users/:userId/active', json, token, authorizeUserManager, routes.users.load, routes.users.setActive); + 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); diff --git a/src/test/externalldap-test.js b/src/test/externalldap-test.js index e6d4ffc58..e02392c92 100644 --- a/src/test/externalldap-test.js +++ b/src/test/externalldap-test.js @@ -544,6 +544,12 @@ describe('External LDAP', function () { const [error] = await safe(groups.setMembership(user, [])); expect(error.reason).to.be(BoxError.BAD_STATE); }); + + it('cannot update profile of external user', async function () { + const user = await users.getByUsername(gLdapUsers[0].username); + const [error] = await safe(users.updateProfile(user, { displayName: 'another name'}, auditSource)); + expect(error.reason).to.be(BoxError.BAD_STATE); + }); }); describe('user auto creation', function () { diff --git a/src/users.js b/src/users.js index fb64d3461..f2ee10baa 100644 --- a/src/users.js +++ b/src/users.js @@ -24,6 +24,7 @@ exports = module.exports = { setPassword, setGhost, + updateProfile, update, del, @@ -633,6 +634,16 @@ async function update(user, data, auditSource) { }); } +async function updateProfile(user, profile, auditSource) { + assert.strictEqual(typeof user, 'object'); + assert.strictEqual(typeof profile, 'object'); + assert(auditSource && typeof auditSource === 'object'); + + if (user.source === 'ldap') throw new BoxError(BoxError.BAD_STATE, 'Cannot update profile of external auth user'); + + await update(user, profile, auditSource); +} + async function getOwner() { const owners = await getByRole(exports.ROLE_OWNER); if (owners.length === 0) return null;