fixing rbac

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

@@ -63,6 +63,12 @@ export class Role implements RoleDoc {
   }
 
   addInheritance(inherits?: string | string[]) {
+    // make sure IDs are correct format
+    if (inherits && typeof inherits === "string") {
+      inherits = prefixRoleIDNoBuiltin(inherits)
+    } else if (inherits && Array.isArray(inherits)) {
+      inherits = inherits.map(inherit => prefixRoleIDNoBuiltin(inherit))
+    }
     if (inherits) {
       this.inherits = inherits
     }
@@ -128,7 +134,15 @@ export function getBuiltinRoles(): { [key: string]: RoleDoc } {
 }
 
 export function isBuiltin(role: string) {
-  return getBuiltinRole(role) !== undefined
+  return Object.values(BUILTIN_ROLE_IDS).includes(role)
+}
+
+export function prefixRoleIDNoBuiltin(roleId: string) {
+  if (isBuiltin(roleId)) {
+    return roleId
+  } else {
+    return prefixRoleID(roleId)
+  }
 }
 
 export function getBuiltinRole(roleId: string): Role | undefined {
@@ -536,3 +550,16 @@ export function getExternalRoleID(roleId: string, version?: string) {
   }
   return roleId
 }
+
+export function getExternalRoleIDs(
+  roleIds: string | string[] | undefined,
+  version?: string
+) {
+  if (!roleIds) {
+    return roleIds
+  } else if (typeof roleIds === "string") {
+    return getExternalRoleID(roleIds, version)
+  } else {
+    return roleIds.map(roleId => getExternalRoleID(roleId, version))
+  }
+}

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

@@ -27,6 +27,18 @@ const UpdateRolesOptions = {
   REMOVED: "removed",
 }
 
+function externalRole(role: Role): Role {
+  let _id: string | undefined
+  if (role._id) {
+    _id = roles.getExternalRoleID(role._id)
+  }
+  return {
+    ...role,
+    _id,
+    inherits: roles.getExternalRoleIDs(role.inherits, role.version),
+  }
+}
+
 async function updateRolesOnUserTable(
   db: Database,
   roleId: string,
@@ -53,7 +65,7 @@ async function updateRolesOnUserTable(
 }
 
 export async function fetch(ctx: UserCtx<void, FetchRolesResponse>) {
-  ctx.body = await roles.getAllRoles()
+  ctx.body = (await roles.getAllRoles()).map(role => externalRole(role))
 }
 
 export async function find(ctx: UserCtx<void, FindRoleResponse>) {
@@ -61,7 +73,7 @@ export async function find(ctx: UserCtx<void, FindRoleResponse>) {
   if (!role) {
     ctx.throw(404, { message: "Role not found" })
   } else {
-    ctx.body = role
+    ctx.body = externalRole(role)
   }
 }
 
@@ -135,10 +147,7 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     role.version
   )
   role._rev = result.rev
-  ctx.body = {
+  ctx.body = externalRole(role)
-    ...role,
-    _id: roles.getExternalRoleID(role._id!, role.version),
-  }
 
   const devDb = context.getDevAppDB()
   const prodDb = context.getProdAppDB()
@@ -196,15 +205,14 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
   if (!roleId) {
     roleId = roles.BUILTIN_ROLE_IDS.PUBLIC
   }
+  let roleIds: string[] = []
   if (ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user)) {
     const appId = context.getAppId()
-    if (!appId) {
+    if (appId) {
-      ctx.body = []
+      roleIds = await roles.getAllRoleIds(appId)
-    } else {
-      ctx.body = await roles.getAllRoleIds(appId)
     }
   } else {
-    ctx.body = await roles.getUserRoleIdHierarchy(roleId!)
+    roleIds = await roles.getUserRoleIdHierarchy(roleId!)
   }
 
   // If a custom role is provided in the header, filter out higher level roles
@@ -212,7 +220,7 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
   if (roleHeader && !Object.keys(roles.BUILTIN_ROLE_IDS).includes(roleHeader)) {
     const role = await roles.getRole(roleHeader)
     const inherits = role?.inherits
-    const orderedRoles = ctx.body.reverse()
+    const orderedRoles = roleIds.reverse()
     let filteredRoles = [roleHeader]
     for (let role of orderedRoles) {
       filteredRoles = [role, ...filteredRoles]
@@ -224,6 +232,8 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
       }
     }
     filteredRoles.pop()
-    ctx.body = [roleHeader, ...filteredRoles]
+    roleIds = [roleHeader, ...filteredRoles]
   }
+
+  ctx.body = roleIds.map(roleId => roles.getExternalRoleID(roleId))
 }

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

@@ -105,7 +105,7 @@ describe("/roles", () => {
         inherits: [BUILTIN_ROLE_IDS.POWER, role1._id!, role2._id!],
       })
       // go back to role1
-      role1 = await config.api.roles.save(
+      await config.api.roles.save(
         {
           ...role1,
           inherits: [BUILTIN_ROLE_IDS.POWER, role2._id!, role3._id!],
@@ -113,7 +113,7 @@ describe("/roles", () => {
         { status: 400, body: { message: LOOP_ERROR } }
       )
       // go back to role2
-      role2 = await config.api.roles.save(
+      await config.api.roles.save(
         {
           ...role2,
           inherits: [BUILTIN_ROLE_IDS.POWER, role1._id!, role3._id!],

packages/shared-core/src/helpers/roles.ts

@@ -1,4 +1,4 @@
-import { Role } from "@budibase/types"
+import { Role, SEPARATOR, DocumentType } from "@budibase/types"
 
 // Function to detect loops in roles
 export function checkForRoleInheritanceLoops(roles: Role[]): boolean {