Merge pull request #2729 from Budibase/fix/no-perms-hierarchy

Removing the concept of permissions hierarchy from backend for resources

packages/auth/src/security/permissions.js

@@ -139,8 +139,7 @@ exports.doesHaveResourcePermission = (
   // set foundSub to not subResourceId, incase there is no subResource
   let foundMain = false,
     foundSub = false
-  for (let [resource, level] of Object.entries(permissions)) {
-    const levels = getAllowedLevels(level)
+  for (let [resource, levels] of Object.entries(permissions)) {
     if (resource === resourceId && levels.indexOf(permLevel) !== -1) {
       foundMain = true
     }
@@ -177,10 +176,6 @@ exports.doesHaveBasePermission = (permType, permLevel, permissionIds) => {
   return false
 }
 
-exports.higherPermission = (perm1, perm2) => {
-  return levelToNumber(perm1) > levelToNumber(perm2) ? perm1 : perm2
-}
-
 exports.isPermissionLevelHigherThanRead = level => {
   return levelToNumber(level) > 1
 }

packages/auth/src/security/roles.js

@@ -1,6 +1,6 @@
 const { getDB } = require("../db")
 const { cloneDeep } = require("lodash/fp")
-const { BUILTIN_PERMISSION_IDS, higherPermission } = require("./permissions")
+const { BUILTIN_PERMISSION_IDS } = require("./permissions")
 const {
   generateRoleID,
   getRoleParams,
@@ -193,8 +193,17 @@ exports.getUserPermissions = async (appId, userRoleId) => {
   const permissions = {}
   for (let role of rolesHierarchy) {
     if (role.permissions) {
-      for (let [resource, level] of Object.entries(role.permissions)) {
-        permissions[resource] = higherPermission(permissions[resource], level)
+      for (let [resource, levels] of Object.entries(role.permissions)) {
+        if (!permissions[resource]) {
+          permissions[resource] = []
+        }
+        const permsSet = new Set(permissions[resource])
+        if (Array.isArray(levels)) {
+          levels.forEach(level => permsSet.add(level))
+        } else {
+          permsSet.add(levels)
+        }
+        permissions[resource] = [...permsSet]
       }
     }
   }

packages/server/src/api/controllers/permission.js

@@ -1,9 +1,4 @@
-const {
-  getBuiltinPermissions,
-  PermissionLevels,
-  isPermissionLevelHigherThanRead,
-  higherPermission,
-} = require("@budibase/auth/permissions")
+const { getBuiltinPermissions } = require("@budibase/auth/permissions")
 const {
   isBuiltin,
   getDBRoleID,
@@ -16,6 +11,7 @@ const {
   CURRENTLY_SUPPORTED_LEVELS,
   getBasePermissions,
 } = require("../../utilities/security")
+const { removeFromArray } = require("../../utilities")
 
 const PermissionUpdateType = {
   REMOVE: "remove",
@@ -24,22 +20,6 @@ const PermissionUpdateType = {
 
 const SUPPORTED_LEVELS = CURRENTLY_SUPPORTED_LEVELS
 
-// quick function to perform a bit of weird logic, make sure fetch calls
-// always say a write role also has read permission
-function fetchLevelPerms(permissions, level, roleId) {
-  if (!permissions) {
-    permissions = {}
-  }
-  permissions[level] = roleId
-  if (
-    isPermissionLevelHigherThanRead(level) &&
-    !permissions[PermissionLevels.READ]
-  ) {
-    permissions[PermissionLevels.READ] = roleId
-  }
-  return permissions
-}
-
 // utility function to stop this repetition - permissions always stored under roles
 async function getAllDBRoles(db) {
   const body = await db.allDocs(
@@ -74,23 +54,31 @@ async function updatePermissionOnRole(
   for (let role of dbRoles) {
     let updated = false
     const rolePermissions = role.permissions ? role.permissions : {}
+    // make sure its an array, also handle migrating
+    if (
+      !rolePermissions[resourceId] ||
+      !Array.isArray(rolePermissions[resourceId])
+    ) {
+      rolePermissions[resourceId] =
+        typeof rolePermissions[resourceId] === "string"
+          ? [rolePermissions[resourceId]]
+          : []
+    }
     // handle the removal/updating the role which has this permission first
     // the updating (role._id !== dbRoleId) is required because a resource/level can
     // only be permitted in a single role (this reduces hierarchy confusion and simplifies
     // the general UI for this, rather than needing to show everywhere it is used)
     if (
       (role._id !== dbRoleId || remove) &&
-      rolePermissions[resourceId] === level
+      rolePermissions[resourceId].indexOf(level) !== -1
     ) {
-      delete rolePermissions[resourceId]
+      removeFromArray(rolePermissions[resourceId], level)
       updated = true
     }
     // handle the adding, we're on the correct role, at it to this
     if (!remove && role._id === dbRoleId) {
-      rolePermissions[resourceId] = higherPermission(
-        rolePermissions[resourceId],
-        level
-      )
+      const set = new Set(rolePermissions[resourceId])
+      rolePermissions[resourceId] = [...set.add(level)]
       updated = true
     }
     // handle the update, add it to bulk docs to perform at end
@@ -127,12 +115,11 @@ exports.fetch = async function (ctx) {
       continue
     }
     const roleId = getExternalRoleID(role._id)
-    for (let [resource, level] of Object.entries(role.permissions)) {
-      permissions[resource] = fetchLevelPerms(
-        permissions[resource],
-        level,
-        roleId
-      )
+    for (let [resource, levelArr] of Object.entries(role.permissions)) {
+      const levels = Array.isArray(levelArr) ? [levelArr] : levelArr
+      const perms = {}
+      levels.forEach(level => (perms[level] = roleId))
+      permissions[resource] = perms
     }
   }
   // apply the base permissions
@@ -157,12 +144,13 @@ exports.getResourcePerms = async function (ctx) {
   for (let level of SUPPORTED_LEVELS) {
     // update the various roleIds in the resource permissions
     for (let role of roles) {
-      if (role.permissions && role.permissions[resourceId] === level) {
-        permissions = fetchLevelPerms(
-          permissions,
-          level,
-          getExternalRoleID(role._id)
-        )
+      const rolePerms = role.permissions
+      if (
+        rolePerms &&
+        (rolePerms[resourceId] === level ||
+          rolePerms[resourceId].indexOf(level) !== -1)
+      ) {
+        permissions[level] = getExternalRoleID(role._id)
       }
     }
   }

packages/server/src/api/routes/tests/role.spec.js

@@ -72,7 +72,7 @@ describe("/roles", () => {
         .expect(200)
       expect(res.body.length).toBeGreaterThan(0)
       const power = res.body.find(role => role._id === BUILTIN_ROLE_IDS.POWER)
-      expect(power.permissions[table._id]).toEqual("read")
+      expect(power.permissions[table._id]).toEqual(["read"])
     })
   })
 

packages/server/src/utilities/index.js

@@ -10,6 +10,14 @@ exports.wait = ms => new Promise(resolve => setTimeout(resolve, ms))
 
 exports.isDev = env.isDev
 
+exports.removeFromArray = (array, element) => {
+  const index = array.indexOf(element)
+  if (index !== -1) {
+    array.splice(index, 1)
+  }
+  return array
+}
+
 /**
  * Makes sure that a URL has the correct number of slashes, while maintaining the
  * http(s):// double slashes.