Merge pull request #14844 from Budibase/fix/multi-inheritance-preview-role

Preview role - multi-inheritance fixes

packages/backend-core/src/security/roles.ts

@@ -213,6 +213,22 @@ export function getBuiltinRole(roleId: string): Role | undefined {
   return cloneDeep(role)
 }
 
+export function validInherits(
+  allRoles: RoleDoc[],
+  inherits?: string | string[]
+): boolean {
+  if (!inherits) {
+    return false
+  }
+  const find = (id: string) => allRoles.find(r => roleIDsAreEqual(r._id!, id))
+  if (Array.isArray(inherits)) {
+    const filtered = inherits.filter(roleId => find(roleId))
+    return inherits.length !== 0 && filtered.length === inherits.length
+  } else {
+    return !!find(inherits)
+  }
+}
+
 /**
  * Works through the inheritance ranks to see how far up the builtin stack this ID is.
  */
@@ -290,7 +306,7 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string {
     : roleId1
 }
 
-export function compareRoleIds(roleId1: string, roleId2: string) {
+export function roleIDsAreEqual(roleId1: string, roleId2: string) {
   // make sure both role IDs are prefixed correctly
   return prefixRoleID(roleId1) === prefixRoleID(roleId2)
 }
@@ -323,7 +339,7 @@ export function findRole(
     roleId = prefixRoleID(roleId)
   }
   const dbRole = roles.find(
-    role => role._id && compareRoleIds(role._id, roleId)
+    role => role._id && roleIDsAreEqual(role._id, roleId)
   )
   if (!dbRole && !isBuiltin(roleId) && opts?.defaultPublic) {
     return cloneDeep(BUILTIN_ROLES.PUBLIC)
@@ -557,7 +573,7 @@ export class AccessController {
     }
 
     return (
-      roleIds?.find(roleId => compareRoleIds(roleId, tryingRoleId)) !==
+      roleIds?.find(roleId => roleIDsAreEqual(roleId, tryingRoleId)) !==
       undefined
     )
   }

packages/server/src/api/controllers/role.ts

@@ -18,6 +18,7 @@ import {
   UserCtx,
   UserMetadata,
   DocumentType,
+  PermissionLevel,
 } from "@budibase/types"
 import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core"
 import sdk from "../../sdk"
@@ -35,11 +36,11 @@ async function removeRoleFromOthers(roleId: string) {
     let changed = false
     if (Array.isArray(role.inherits)) {
       const newInherits = role.inherits.filter(
-        id => !roles.compareRoleIds(id, roleId)
+        id => !roles.roleIDsAreEqual(id, roleId)
       )
       changed = role.inherits.length !== newInherits.length
       role.inherits = newInherits
-    } else if (role.inherits && roles.compareRoleIds(role.inherits, roleId)) {
+    } else if (role.inherits && roles.roleIDsAreEqual(role.inherits, roleId)) {
       role.inherits = roles.BUILTIN_ROLE_IDS.PUBLIC
       changed = true
     }
@@ -125,6 +126,17 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     ctx.throw(400, "Cannot change custom role name")
   }
 
+  // custom roles should always inherit basic - if they don't inherit anything else
+  if (!inherits && roles.validInherits(allRoles, dbRole?.inherits)) {
+    inherits = dbRole?.inherits
+  } else if (!roles.validInherits(allRoles, inherits)) {
+    inherits = [roles.BUILTIN_ROLE_IDS.BASIC]
+  }
+  // assume write permission level for newly created roles
+  if (isCreate && !permissionId) {
+    permissionId = PermissionLevel.WRITE
+  }
+
   const role = new roles.Role(_id, name, permissionId, {
     displayName: uiMetadata?.displayName || name,
     description: uiMetadata?.description || "Custom role",
@@ -226,35 +238,23 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
   if (!roleId) {
     roleId = roles.BUILTIN_ROLE_IDS.PUBLIC
   }
+  // If a custom role is provided in the header, filter out higher level roles
+  const roleHeader = ctx.header[Header.PREVIEW_ROLE]
+  if (Array.isArray(roleHeader)) {
+    ctx.throw(400, `Too many roles specified in ${Header.PREVIEW_ROLE} header`)
+  }
+  const isBuilder = ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user)
   let roleIds: string[] = []
-  if (ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user)) {
+  if (!roleHeader && isBuilder) {
     const appId = context.getAppId()
     if (appId) {
       roleIds = await roles.getAllRoleIds(appId)
     }
+  } else if (isBuilder && roleHeader) {
+    roleIds = await roles.getUserRoleIdHierarchy(roleHeader)
   } else {
     roleIds = await roles.getUserRoleIdHierarchy(roleId!)
   }
 
-  // If a custom role is provided in the header, filter out higher level roles
-  const roleHeader = ctx.header?.[Header.PREVIEW_ROLE] as string
-  if (roleHeader && !Object.keys(roles.BUILTIN_ROLE_IDS).includes(roleHeader)) {
-    const role = await roles.getRole(roleHeader)
-    const inherits = role?.inherits
-    const orderedRoles = roleIds.reverse()
-    let filteredRoles = [roleHeader]
-    for (let role of orderedRoles) {
-      filteredRoles = [role, ...filteredRoles]
-      if (
-        (Array.isArray(inherits) && inherits.includes(role)) ||
-        role === inherits
-      ) {
-        break
-      }
-    }
-    filteredRoles.pop()
-    roleIds = [roleHeader, ...filteredRoles]
-  }
-
   ctx.body = roleIds.map(roleId => roles.getExternalRoleID(roleId))
 }

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

@@ -38,6 +38,26 @@ describe("/roles", () => {
         _id: dbCore.prefixRoleID(res._id!),
       })
     })
+
+    it("handle a role with invalid inherits", async () => {
+      const role = basicRole()
+      role.inherits = ["not_real", "some_other_not_real"]
+
+      const res = await config.api.roles.save(role, {
+        status: 200,
+      })
+      expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC])
+    })
+
+    it("handle a role with no inherits", async () => {
+      const role = basicRole()
+      role.inherits = []
+
+      const res = await config.api.roles.save(role, {
+        status: 200,
+      })
+      expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC])
+    })
   })
 
   describe("update", () => {
@@ -149,6 +169,17 @@ describe("/roles", () => {
         { status: 400, body: { message: LOOP_ERROR } }
       )
     })
+
+    it("handle updating a role, without its inherits", async () => {
+      const res = await config.api.roles.save({
+        ...basicRole(),
+        inherits: [BUILTIN_ROLE_IDS.ADMIN],
+      })
+      // remove the roles so that it will default back to DB roles, then save again
+      delete res.inherits
+      const updatedRes = await config.api.roles.save(res)
+      expect(updatedRes.inherits).toEqual([BUILTIN_ROLE_IDS.ADMIN])
+    })
   })
 
   describe("fetch", () => {
@@ -298,6 +329,23 @@ describe("/roles", () => {
         }
       )
     })
+
+    it("should fetch preview role correctly even without basic specified", async () => {
+      const role = await config.api.roles.save(basicRole())
+      // have to forcefully delete the inherits from DB - technically can't
+      // happen anymore - but good test case
+      await dbCore.getDB(config.appId!).put({
+        ...role,
+        _id: dbCore.prefixRoleID(role._id!),
+        inherits: [],
+      })
+      await config.withHeaders({ "x-budibase-role": role.name }, async () => {
+        const res = await config.api.roles.accessible({
+          status: 200,
+        })
+        expect(res).toEqual([role.name])
+      })
+    })
   })
 
   describe("accessible - multi-inheritance", () => {

packages/server/src/api/routes/tests/screen.spec.ts

@@ -86,7 +86,6 @@ describe("/screens", () => {
             status: 200,
           }
         )
-        // basic and role1 screen
         expect(res.screens.length).toEqual(screenIds.length)
         expect(res.screens.map(s => s._id).sort()).toEqual(screenIds.sort())
       })
@@ -107,6 +106,25 @@ describe("/screens", () => {
         screen2._id!,
       ])
     })
+
+    it("should be able to fetch basic and screen 1 with role1 in role header", async () => {
+      await config.withHeaders(
+        {
+          "x-budibase-role": role1._id!,
+        },
+        async () => {
+          const res = await config.api.application.getDefinition(
+            config.prodAppId!,
+            {
+              status: 200,
+            }
+          )
+          const screenIds = [screen._id!, screen1._id!]
+          expect(res.screens.length).toEqual(screenIds.length)
+          expect(res.screens.map(s => s._id).sort()).toEqual(screenIds.sort())
+        }
+      )
+    })
   })
 
   describe("save", () => {

packages/server/src/middleware/currentapp.ts

@@ -56,22 +56,9 @@ export default async (ctx: UserCtx, next: any) => {
       ctx.request &&
       (ctx.request.headers[constants.Header.PREVIEW_ROLE] as string)
     if (isBuilder && isDevApp && roleHeader) {
-      // Ensure the role is valid by ensuring a definition exists
-      try {
-        if (roleHeader) {
-          const role = await roles.getRole(roleHeader)
-          if (role) {
-            roleId = roleHeader
-
-            // Delete admin and builder flags so that the specified role is honoured
-            ctx.user = users.removePortalUserPermissions(
-              ctx.user
-            ) as ContextUser
-          }
-        }
-      } catch (error) {
-        // Swallow error and do nothing
-      }
+      roleId = roleHeader
+      // Delete admin and builder flags so that the specified role is honoured
+      ctx.user = users.removePortalUserPermissions(ctx.user) as ContextUser
     }
   }
 

packages/server/src/tests/utilities/structures.ts

@@ -8,31 +8,31 @@ import {
 } from "../../automations"
 import {
   AIOperationEnum,
+  AutoFieldSubType,
   Automation,
   AutomationActionStepId,
+  AutomationEventType,
   AutomationResults,
   AutomationStatus,
   AutomationStep,
   AutomationStepType,
   AutomationTrigger,
   AutomationTriggerStepId,
+  BBReferenceFieldSubType,
+  CreateViewRequest,
   Datasource,
+  FieldSchema,
   FieldType,
+  INTERNAL_TABLE_SOURCE_ID,
+  JsonFieldSubType,
+  LoopStepType,
+  Query,
+  Role,
   SourceName,
   Table,
-  INTERNAL_TABLE_SOURCE_ID,
   TableSourceType,
-  Query,
   Webhook,
   WebhookActionType,
-  AutomationEventType,
-  LoopStepType,
-  FieldSchema,
-  BBReferenceFieldSubType,
-  JsonFieldSubType,
-  AutoFieldSubType,
-  Role,
-  CreateViewRequest,
 } from "@budibase/types"
 import { LoopInput } from "../../definitions/automations"
 import { merge } from "lodash"
@@ -439,7 +439,7 @@ export function updateRowAutomationWithFilters(
   appId: string,
   tableId: string
 ): Automation {
-  const automation: Automation = {
+  return {
     name: "updateRowWithFilters",
     type: "automation",
     appId,
@@ -472,7 +472,6 @@ export function updateRowAutomationWithFilters(
       },
     },
   }
-  return automation
 }
 
 export function basicAutomationResults(