fix: prevent manage:users users from editing manage:system users

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
    *

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')

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' })
+        revokeTargetSessions(args.id)
-        WIKI.events.outbound.emit('addAuthRevoke', { id: args.id, kind: 'u' })
 
         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))) {
+        // Prevent locking out the root administrator
-          throw new Error('You are not authorized to modify / assign a user from / to an administrative group.')
+        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' })
+        revokeTargetSessions(args.id)
-        WIKI.events.outbound.emit('addAuthRevoke', { id: args.id, kind: 'u' })
 
         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')
         }

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()
     }