diff --git a/server/core/auth.js b/server/core/auth.js index ec0dd720b..5242b86b6 100644 --- a/server/core/auth.js +++ b/server/core/auth.js @@ -360,6 +360,54 @@ module.exports = { }) }, + /** + * Check if user (requester) can manage an existing user (target) + * + * Prevents a delegated user manager from taking over / tampering with an + * account holding higher or equal administrative privileges. + * + * @param {User} requester The user attempting to manage the target user + * @param {Number} targetId The ID of the user being managed + * @returns {Boolean} + */ + async checkManageUserTargetAccess(requester, targetId) { + const requesterPermissions = requester.permissions ? requester.permissions : requester.getGlobalPermissions() + + // System Admin + if (requesterPermissions.includes('manage:system')) { + return true + } + + const target = await WIKI.models.users.query().findById(targetId).withGraphJoined('groups').modifyGraph('groups', builder => { + builder.select('groups.id', 'permissions') + }) + if (!target) { + return false + } + + // Protect system accounts (root administrator + guest), even if their groups were tampered with + if (target.id <= 2 || target.isSystem) { + return false + } + + const targetPermissions = target.getGlobalPermissions() + + // Check target for manage:system permission + if (targetPermissions.includes('manage:system')) { + return false + } + + // Check target for administrative permissions + if (targetPermissions.some(p => { + const permType = _.last(p.split(':')) + return ['users', 'groups', 'navigation', 'theme', 'api'].includes(permType) + }) && !requesterPermissions.includes('manage:groups')) { + return false + } + + return true + }, + /** * Check and apply Page Rule specificity * diff --git a/server/graph/resolvers/group.js b/server/graph/resolvers/group.js index 8cc847c16..1a1a972ae 100644 --- a/server/graph/resolvers/group.js +++ b/server/graph/resolvers/group.js @@ -130,13 +130,19 @@ module.exports = { /** * UNASSIGN USER FROM GROUP */ - async unassignUser (obj, args) { + async unassignUser (obj, args, { req }) { if (args.userId === 2) { throw new gql.GraphQLError('Cannot unassign Guest user') } if (args.userId === 1 && args.groupId === 1) { throw new gql.GraphQLError('Cannot unassign Administrator user from Administrators group.') } + + // Check that the requester is allowed to manage the target user + if (!(await WIKI.auth.checkManageUserTargetAccess(req.user, args.userId))) { + throw new gql.GraphQLError('You are not authorized to unassign this user from a group.') + } + const grp = await WIKI.models.groups.query().findById(args.groupId) if (!grp) { throw new gql.GraphQLError('Invalid Group ID') diff --git a/server/graph/resolvers/user.js b/server/graph/resolvers/user.js index 19b9f42ba..30511d52b 100644 --- a/server/graph/resolvers/user.js +++ b/server/graph/resolvers/user.js @@ -3,6 +3,28 @@ const _ = require('lodash') /* global WIKI */ +/** + * Ensure the requester is allowed to manage the target user and, optionally, + * to assign it to the requested groups. + */ +const assertCanManageTarget = async (requester, targetId, groups) => { + if (!(await WIKI.auth.checkManageUserTargetAccess(requester, targetId))) { + throw new Error('You are not authorized to manage this user.') + } + + if (_.isArray(groups) && !(await WIKI.auth.checkAssignUserToGroupAccess(requester, groups))) { + throw new Error('You are not authorized to modify / assign a user from / to an administrative group.') + } +} + +/** + * Invalidate all active sessions of the target user + */ +const revokeTargetSessions = targetId => { + WIKI.auth.revokeUserTokens({ id: targetId, kind: 'u' }) + WIKI.events.outbound.emit('addAuthRevoke', { id: targetId, kind: 'u' }) +} + module.exports = { Query: { async users() { return {} } @@ -77,15 +99,16 @@ module.exports = { return graphHelper.generateError(err) } }, - async delete (obj, args) { + async delete (obj, args, context) { try { if (args.id <= 2) { throw new WIKI.Error.UserDeleteProtected() } + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.deleteUser(args.id, args.replaceId) - WIKI.auth.revokeUserTokens({ id: args.id, kind: 'u' }) - WIKI.events.outbound.emit('addAuthRevoke', { id: args.id, kind: 'u' }) + revokeTargetSessions(args.id) return { responseResult: graphHelper.generateSuccess('User deleted successfully') @@ -100,11 +123,19 @@ module.exports = { }, async update (obj, args, context) { try { - if (!(await WIKI.auth.checkAssignUserToGroupAccess(context.req.user, args.groups))) { - throw new Error('You are not authorized to modify / assign a user from / to an administrative group.') + // Prevent locking out the root administrator + if (args.id === 1 && _.isArray(args.groups) && !args.groups.includes(1)) { + throw new Error('Cannot unassign the root administrator from the Administrators group.') } - await WIKI.models.users.updateUser(args) + await assertCanManageTarget(context.req.user, args.id, args.groups) + + const changes = await WIKI.models.users.updateUser(args) + + // Invalidate active sessions on security-sensitive changes + if (changes.passwordChanged || changes.groupsChanged) { + revokeTargetSessions(args.id) + } return { responseResult: graphHelper.generateSuccess('User updated successfully') @@ -113,8 +144,10 @@ module.exports = { return graphHelper.generateError(err) } }, - async verify (obj, args) { + async verify (obj, args, context) { try { + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.query().patch({ isVerified: true }).findById(args.id) return { @@ -124,8 +157,10 @@ module.exports = { return graphHelper.generateError(err) } }, - async activate (obj, args) { + async activate (obj, args, context) { try { + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.query().patch({ isActive: true }).findById(args.id) return { @@ -135,15 +170,16 @@ module.exports = { return graphHelper.generateError(err) } }, - async deactivate (obj, args) { + async deactivate (obj, args, context) { try { if (args.id <= 2) { throw new Error('Cannot deactivate system accounts.') } + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.query().patch({ isActive: false }).findById(args.id) - WIKI.auth.revokeUserTokens({ id: args.id, kind: 'u' }) - WIKI.events.outbound.emit('addAuthRevoke', { id: args.id, kind: 'u' }) + revokeTargetSessions(args.id) return { responseResult: graphHelper.generateSuccess('User deactivated successfully') @@ -152,8 +188,10 @@ module.exports = { return graphHelper.generateError(err) } }, - async enableTFA (obj, args) { + async enableTFA (obj, args, context) { try { + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.query().patch({ tfaIsActive: true, tfaSecret: null }).findById(args.id) return { @@ -163,10 +201,14 @@ module.exports = { return graphHelper.generateError(err) } }, - async disableTFA (obj, args) { + async disableTFA (obj, args, context) { try { + await assertCanManageTarget(context.req.user, args.id) + await WIKI.models.users.query().patch({ tfaIsActive: false, tfaSecret: null }).findById(args.id) + revokeTargetSessions(args.id) + return { responseResult: graphHelper.generateSuccess('User 2FA disabled successfully') } diff --git a/server/models/users.js b/server/models/users.js index 8996206d6..50f91819c 100644 --- a/server/models/users.js +++ b/server/models/users.js @@ -677,11 +677,16 @@ module.exports = class User extends Model { * Update an existing user * * @param {Object} param0 User ID and fields to update + * @returns {Object} Security-sensitive changes applied to the user */ static async updateUser ({ id, email, name, newPassword, groups, location, jobTitle, timezone, dateFormat, appearance }) { const usr = await WIKI.models.users.query().findById(id) if (usr) { let usrData = {} + const changes = { + passwordChanged: false, + groupsChanged: false + } if (!_.isEmpty(email) && email !== usr.email) { const dupUsr = await WIKI.models.users.query().select('id').where({ email, @@ -700,6 +705,7 @@ module.exports = class User extends Model { throw new WIKI.Error.InputInvalid('Password must be at least 6 characters!') } usrData.password = newPassword + changes.passwordChanged = true } if (_.isArray(groups)) { const usrGroupsRaw = await usr.$relatedQuery('groups') @@ -714,6 +720,7 @@ module.exports = class User extends Model { for (const grp of remUsrGroups) { await usr.$relatedQuery('groups').unrelate().where('groupId', grp) } + changes.groupsChanged = addUsrGroups.length > 0 || remUsrGroups.length > 0 } if (!_.isEmpty(location) && location !== usr.location) { usrData.location = _.trim(location) @@ -731,6 +738,8 @@ module.exports = class User extends Model { usrData.appearance = appearance } await WIKI.models.users.query().patch(usrData).findById(id) + + return changes } else { throw new WIKI.Error.UserNotFound() }