Fixing some issues with finding roles.

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

@@ -235,7 +235,7 @@ export function findRole(
   roleId: string,
   roles: RoleDoc[],
   opts?: { defaultPublic?: boolean }
-): RoleDoc {
+): RoleDoc | undefined {
   // built in roles mostly come from the in-code implementation,
   // but can be extended by a doc stored about them (e.g. permissions)
   let role: RoleDoc | undefined = getBuiltinRole(roleId)
@@ -249,13 +249,13 @@ export function findRole(
   if (!dbRole && !isBuiltin(roleId) && opts?.defaultPublic) {
     return cloneDeep(BUILTIN_ROLES.PUBLIC)
   }
-  if (!dbRole && (!role || Object.keys(role).length === 0)) {
-    throw new Error("Role could not be found")
-  }
+  // combine the roles
   role = Object.assign(role || {}, dbRole)
   // finalise the ID
-  role._id = getExternalRoleID(role._id!, role.version)
-  return role
+  if (role?._id) {
+    role._id = getExternalRoleID(role._id, role.version)
+  }
+  return Object.keys(role).length === 0 ? undefined : role
 }
 
 /**
@@ -268,7 +268,7 @@ export function findRole(
 export async function getRole(
   roleId: string,
   opts?: { defaultPublic?: boolean }
-): Promise<RoleDoc> {
+): Promise<RoleDoc | undefined> {
   const db = getAppDB()
   const roleList = []
   if (!isBuiltin(roleId)) {
@@ -305,7 +305,7 @@ async function getAllUserRoles(
 
   const roleIds = [userRoleId]
   const roles: RoleDoc[] = []
-  const iterateInherited = (role: RoleDoc) => {
+  const iterateInherited = (role: RoleDoc | undefined) => {
     if (!role || !role._id) {
       return
     }
@@ -335,7 +335,10 @@ async function getAllUserRoles(
   }
 
   // get all the inherited roles
-  iterateInherited(findRole(userRoleId, allRoles, opts))
+  const foundRole = findRole(userRoleId, allRoles, opts)
+  if (foundRole) {
+    iterateInherited(foundRole)
+  }
   const foundRoleIds: string[] = []
   return roles.filter(role => {
     if (role._id && !foundRoleIds.includes(role._id)) {

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

@@ -58,7 +58,12 @@ export async function fetch(ctx: UserCtx<void, FetchRolesResponse>) {
 }
 
 export async function find(ctx: UserCtx<void, FindRoleResponse>) {
-  ctx.body = await roles.getRole(ctx.params.roleId)
+  const role = await roles.getRole(ctx.params.roleId)
+  if (!role) {
+    ctx.throw(404, { message: "Role not found" })
+  } else {
+    ctx.body = role
+  }
 }
 
 export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
@@ -205,7 +210,8 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
   // 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 inherits = (await roles.getRole(roleHeader))?.inherits
+    const role = await roles.getRole(roleHeader)
+    const inherits = role?.inherits
     const orderedRoles = ctx.body.reverse()
     let filteredRoles = [roleHeader]
     for (let role of orderedRoles) {

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

@@ -6,6 +6,8 @@ const { basicRole } = setup.structures
 const { BUILTIN_ROLE_IDS } = roles
 const { BuiltinPermissionID } = permissions
 
+const LOOP_ERROR = "Role inheritance contains a loop, this is not supported"
+
 describe("/roles", () => {
   let config = setup.getConfig()
 
@@ -31,6 +33,11 @@ describe("/roles", () => {
   })
 
   describe("update", () => {
+    beforeEach(async () => {
+      // Recreate the app
+      await config.init()
+    })
+
     it("updates a role", async () => {
       const role = basicRole()
       let res = await config.api.roles.save(role, {
@@ -49,22 +56,59 @@ describe("/roles", () => {
     })
 
     it("disallow loops", async () => {
-      let role1 = basicRole()
-      role1 = await config.api.roles.save(role1, {
+      const role1 = await config.api.roles.save(basicRole(), {
         status: 200,
       })
-      let role2 = basicRole()
-      role2.inherits = [role1._id!]
-      role2 = await config.api.roles.save(role2, {
-        status: 200,
-      })
-      role1.inherits = [role2._id!]
-      await config.api.roles.save(role1, {
-        status: 400,
-        body: {
-          message: "Role inheritance contains a loop, this is not supported",
+      const role2 = await config.api.roles.save(
+        {
+          ...basicRole(),
+          inherits: [role1._id!],
         },
+        {
+          status: 200,
+        }
+      )
+      await config.api.roles.save(
+        {
+          ...role1,
+          inherits: [role2._id!],
+        },
+        { status: 400, body: { message: LOOP_ERROR } }
+      )
+    })
+
+    it("disallow more complex loops", async () => {
+      let role1 = await config.api.roles.save({
+        ...basicRole(),
+        name: "role1",
+        inherits: [BUILTIN_ROLE_IDS.POWER],
       })
+      let role2 = await config.api.roles.save({
+        ...basicRole(),
+        name: "role2",
+        inherits: [BUILTIN_ROLE_IDS.POWER, role1._id!],
+      })
+      let role3 = await config.api.roles.save({
+        ...basicRole(),
+        name: "role3",
+        inherits: [BUILTIN_ROLE_IDS.POWER, role1._id!, role2._id!],
+      })
+      // go back to role1
+      role1 = await config.api.roles.save(
+        {
+          ...role1,
+          inherits: [BUILTIN_ROLE_IDS.POWER, role2._id!, role3._id!],
+        },
+        { status: 400, body: { message: LOOP_ERROR } }
+      )
+      // go back to role2
+      role2 = await config.api.roles.save(
+        {
+          ...role2,
+          inherits: [BUILTIN_ROLE_IDS.POWER, role1._id!, role3._id!],
+        },
+        { status: 400, body: { message: LOOP_ERROR } }
+      )
     })
   })
 
@@ -134,6 +178,13 @@ describe("/roles", () => {
   })
 
   describe("accessible", () => {
+    beforeAll(async () => {
+      // new app, reset roles
+      await config.init()
+      // create one custom role
+      await config.createRole()
+    })
+
     it("should be able to fetch accessible roles (with builder)", async () => {
       const res = await config.api.roles.accessible(config.defaultHeaders(), {
         status: 200,

packages/server/src/middleware/currentapp.ts

@@ -59,11 +59,15 @@ export default async (ctx: UserCtx, next: any) => {
       // Ensure the role is valid by ensuring a definition exists
       try {
         if (roleHeader) {
-          await roles.getRole(roleHeader)
-          roleId = 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
+            // 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