From 0c8228edad3b1a288c5232e891ed0745f4774a53 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 5 Aug 2024 15:45:49 +0100
Subject: [PATCH 01/28] Initial work - some re-typing and updating the role
 tests to typescript - using role test API to make this a bit easier to adjust
 going forward.

---
 packages/backend-core/src/security/roles.ts   |  42 +++-
 packages/server/src/api/controllers/role.ts   |   5 +-
 .../server/src/api/routes/tests/role.spec.js  | 182 ------------------
 .../server/src/api/routes/tests/role.spec.ts  | 164 ++++++++++++++++
 .../src/tests/utilities/TestConfiguration.ts  |   1 +
 .../server/src/tests/utilities/api/role.ts    |  11 +-
 .../server/src/tests/utilities/structures.ts  |   4 +-
 packages/types/src/api/web/role.ts            |   4 +-
 packages/types/src/documents/app/role.ts      |   2 +-
 packages/types/src/sdk/events/role.ts         |   6 +-
 10 files changed, 218 insertions(+), 203 deletions(-)
 delete mode 100644 packages/server/src/api/routes/tests/role.spec.js
 create mode 100644 packages/server/src/api/routes/tests/role.spec.ts

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index a64be6b319..61d9786d32 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -42,7 +42,7 @@ export class Role implements RoleDoc {
   _rev?: string
   name: string
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
   version?: string
   permissions = {}
 
@@ -54,8 +54,10 @@ export class Role implements RoleDoc {
     this.version = RoleIDVersion.NAME
   }
 
-  addInheritance(inherits: string) {
-    this.inherits = inherits
+  addInheritance(inherits?: string | string[]) {
+    if (inherits) {
+      this.inherits = inherits
+    }
     return this
   }
 }
@@ -113,7 +115,11 @@ export function builtinRoleToNumber(id: string) {
     if (!role) {
       break
     }
-    role = builtins[role.inherits!]
+    if (Array.isArray(role.inherits)) {
+      // TODO: role inheritance
+    } else {
+      role = builtins[role.inherits!]
+    }
     count++
   } while (role !== null)
   return count
@@ -130,7 +136,12 @@ export async function roleToNumber(id: string) {
     defaultPublic: true,
   })) as RoleDoc[]
   for (let role of hierarchy) {
-    if (role?.inherits && isBuiltin(role.inherits)) {
+    if (!role.inherits) {
+      continue
+    }
+    if (Array.isArray(role.inherits)) {
+      // TODO: role inheritance
+    } else if (isBuiltin(role.inherits)) {
       return builtinRoleToNumber(role.inherits) + 1
     }
   }
@@ -202,16 +213,27 @@ async function getAllUserRoles(
   let currentRole = await getRole(userRoleId, opts)
   let roles = currentRole ? [currentRole] : []
   let roleIds = [userRoleId]
+  const rolesFound = (ids: string | string[]) => {
+    if (Array.isArray(ids)) {
+      return ids.filter(id => roleIds.includes(id)).length === ids.length
+    } else {
+      return roleIds.includes(ids)
+    }
+  }
   // get all the inherited roles
   while (
     currentRole &&
     currentRole.inherits &&
-    roleIds.indexOf(currentRole.inherits) === -1
+    !rolesFound(currentRole.inherits)
   ) {
-    roleIds.push(currentRole.inherits)
-    currentRole = await getRole(currentRole.inherits)
-    if (currentRole) {
-      roles.push(currentRole)
+    if (Array.isArray(currentRole.inherits)) {
+      // TODO: role inheritance
+    } else {
+      roleIds.push(currentRole.inherits)
+      currentRole = await getRole(currentRole.inherits)
+      if (currentRole) {
+        roles.push(currentRole)
+      }
     }
   }
   return roles
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 3398c8102c..3431724d7d 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -182,7 +182,10 @@ export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
     let filteredRoles = [roleHeader]
     for (let role of orderedRoles) {
       filteredRoles = [role, ...filteredRoles]
-      if (role === inherits) {
+      if (
+        (Array.isArray(inherits) && inherits.includes(role)) ||
+        role === inherits
+      ) {
         break
       }
     }
diff --git a/packages/server/src/api/routes/tests/role.spec.js b/packages/server/src/api/routes/tests/role.spec.js
deleted file mode 100644
index 4575f9b213..0000000000
--- a/packages/server/src/api/routes/tests/role.spec.js
+++ /dev/null
@@ -1,182 +0,0 @@
-const { roles, events, permissions } = require("@budibase/backend-core")
-const setup = require("./utilities")
-const { PermissionLevel } = require("@budibase/types")
-const { basicRole } = setup.structures
-const { BUILTIN_ROLE_IDS } = roles
-const { BuiltinPermissionID } = permissions
-
-describe("/roles", () => {
-  let request = setup.getRequest()
-  let config = setup.getConfig()
-
-  afterAll(setup.afterAll)
-
-  beforeAll(async () => {
-    await config.init()
-  })
-
-  const createRole = async role => {
-    if (!role) {
-      role = basicRole()
-    }
-
-    return request
-      .post(`/api/roles`)
-      .send(role)
-      .set(config.defaultHeaders())
-      .expect("Content-Type", /json/)
-      .expect(200)
-  }
-
-  describe("create", () => {
-    it("returns a success message when role is successfully created", async () => {
-      const role = basicRole()
-      const res = await createRole(role)
-
-      expect(res.body._id).toBeDefined()
-      expect(res.body._rev).toBeDefined()
-      expect(events.role.updated).not.toBeCalled()
-      expect(events.role.created).toBeCalledTimes(1)
-      expect(events.role.created).toBeCalledWith(res.body)
-    })
-  })
-
-  describe("update", () => {
-    it("updates a role", async () => {
-      const role = basicRole()
-      let res = await createRole(role)
-      jest.clearAllMocks()
-      res = await createRole(res.body)
-
-      expect(res.body._id).toBeDefined()
-      expect(res.body._rev).toBeDefined()
-      expect(events.role.created).not.toBeCalled()
-      expect(events.role.updated).toBeCalledTimes(1)
-      expect(events.role.updated).toBeCalledWith(res.body)
-    })
-  })
-
-  describe("fetch", () => {
-    beforeAll(async () => {
-      // Recreate the app
-      await config.init()
-    })
-
-    it("should list custom roles, plus 2 default roles", async () => {
-      const customRole = await config.createRole()
-
-      const res = await request
-        .get(`/api/roles`)
-        .set(config.defaultHeaders())
-        .expect("Content-Type", /json/)
-        .expect(200)
-
-      expect(res.body.length).toBe(5)
-
-      const adminRole = res.body.find(r => r._id === BUILTIN_ROLE_IDS.ADMIN)
-      expect(adminRole).toBeDefined()
-      expect(adminRole.inherits).toEqual(BUILTIN_ROLE_IDS.POWER)
-      expect(adminRole.permissionId).toEqual(BuiltinPermissionID.ADMIN)
-
-      const powerUserRole = res.body.find(r => r._id === BUILTIN_ROLE_IDS.POWER)
-      expect(powerUserRole).toBeDefined()
-      expect(powerUserRole.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
-      expect(powerUserRole.permissionId).toEqual(BuiltinPermissionID.POWER)
-
-      const customRoleFetched = res.body.find(r => r._id === customRole.name)
-      expect(customRoleFetched).toBeDefined()
-      expect(customRoleFetched.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
-      expect(customRoleFetched.permissionId).toEqual(
-        BuiltinPermissionID.READ_ONLY
-      )
-    })
-
-    it("should be able to get the role with a permission added", async () => {
-      const table = await config.createTable()
-      await config.api.permission.add({
-        roleId: BUILTIN_ROLE_IDS.POWER,
-        resourceId: table._id,
-        level: PermissionLevel.READ,
-      })
-      const res = await request
-        .get(`/api/roles`)
-        .set(config.defaultHeaders())
-        .expect("Content-Type", /json/)
-        .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"])
-    })
-  })
-
-  describe("destroy", () => {
-    it("should delete custom roles", async () => {
-      const customRole = await config.createRole({
-        name: "user",
-        permissionId: BuiltinPermissionID.READ_ONLY,
-        inherits: BUILTIN_ROLE_IDS.BASIC,
-      })
-      delete customRole._rev_tree
-      await request
-        .delete(`/api/roles/${customRole._id}/${customRole._rev}`)
-        .set(config.defaultHeaders())
-        .expect(200)
-      await request
-        .get(`/api/roles/${customRole._id}`)
-        .set(config.defaultHeaders())
-        .expect(404)
-      expect(events.role.deleted).toBeCalledTimes(1)
-      expect(events.role.deleted).toBeCalledWith(customRole)
-    })
-  })
-
-  describe("accessible", () => {
-    it("should be able to fetch accessible roles (with builder)", async () => {
-      const res = await request
-        .get("/api/roles/accessible")
-        .set(config.defaultHeaders())
-        .expect(200)
-      expect(res.body.length).toBe(5)
-      expect(typeof res.body[0]).toBe("string")
-    })
-
-    it("should be able to fetch accessible roles (basic user)", async () => {
-      const res = await request
-        .get("/api/roles/accessible")
-        .set(await config.basicRoleHeaders())
-        .expect(200)
-      expect(res.body.length).toBe(2)
-      expect(res.body[0]).toBe("BASIC")
-      expect(res.body[1]).toBe("PUBLIC")
-    })
-
-    it("should be able to fetch accessible roles (no user)", async () => {
-      const res = await request
-        .get("/api/roles/accessible")
-        .set(config.publicHeaders())
-        .expect(200)
-      expect(res.body.length).toBe(1)
-      expect(res.body[0]).toBe("PUBLIC")
-    })
-
-    it("should not fetch higher level accessible roles when a custom role header is provided", async () => {
-      await createRole({
-        name: `CUSTOM_ROLE`,
-        inherits: roles.BUILTIN_ROLE_IDS.BASIC,
-        permissionId: permissions.BuiltinPermissionID.READ_ONLY,
-        version: "name",
-      })
-      const res = await request
-        .get("/api/roles/accessible")
-        .set({
-          ...config.defaultHeaders(),
-          "x-budibase-role": "CUSTOM_ROLE",
-        })
-        .expect(200)
-      expect(res.body.length).toBe(3)
-      expect(res.body[0]).toBe("CUSTOM_ROLE")
-      expect(res.body[1]).toBe("BASIC")
-      expect(res.body[2]).toBe("PUBLIC")
-    })
-  })
-})
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
new file mode 100644
index 0000000000..127be789b9
--- /dev/null
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -0,0 +1,164 @@
+import { roles, events, permissions } from "@budibase/backend-core"
+import * as setup from "./utilities"
+import { PermissionLevel } from "@budibase/types"
+
+const { basicRole } = setup.structures
+const { BUILTIN_ROLE_IDS } = roles
+const { BuiltinPermissionID } = permissions
+
+describe("/roles", () => {
+  let config = setup.getConfig()
+
+  afterAll(setup.afterAll)
+
+  beforeAll(async () => {
+    await config.init()
+  })
+
+  describe("create", () => {
+    it("returns a success message when role is successfully created", async () => {
+      const role = basicRole()
+      const res = await config.api.roles.save(role, {
+        status: 200,
+      })
+
+      expect(res._id).toBeDefined()
+      expect(res._rev).toBeDefined()
+      expect(events.role.updated).not.toHaveBeenCalled()
+      expect(events.role.created).toHaveBeenCalledTimes(1)
+      expect(events.role.created).toHaveBeenCalledWith(res)
+    })
+  })
+
+  describe("update", () => {
+    it("updates a role", async () => {
+      const role = basicRole()
+      let res = await config.api.roles.save(role, {
+        status: 200,
+      })
+      jest.clearAllMocks()
+      res = await config.api.roles.save(res, {
+        status: 200,
+      })
+
+      expect(res._id).toBeDefined()
+      expect(res._rev).toBeDefined()
+      expect(events.role.created).not.toHaveBeenCalled()
+      expect(events.role.updated).toHaveBeenCalledTimes(1)
+      expect(events.role.updated).toHaveBeenCalledWith(res)
+    })
+  })
+
+  describe("fetch", () => {
+    beforeAll(async () => {
+      // Recreate the app
+      await config.init()
+    })
+
+    it("should list custom roles, plus 2 default roles", async () => {
+      const customRole = await config.createRole()
+
+      const res = await config.api.roles.fetch({
+        status: 200,
+      })
+
+      expect(res.length).toBe(5)
+
+      const adminRole = res.find(r => r._id === BUILTIN_ROLE_IDS.ADMIN)
+      expect(adminRole).toBeDefined()
+      expect(adminRole!.inherits).toEqual(BUILTIN_ROLE_IDS.POWER)
+      expect(adminRole!.permissionId).toEqual(BuiltinPermissionID.ADMIN)
+
+      const powerUserRole = res.find(r => r._id === BUILTIN_ROLE_IDS.POWER)
+      expect(powerUserRole).toBeDefined()
+      expect(powerUserRole!.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
+      expect(powerUserRole!.permissionId).toEqual(BuiltinPermissionID.POWER)
+
+      const customRoleFetched = res.find(r => r._id === customRole.name)
+      expect(customRoleFetched).toBeDefined()
+      expect(customRoleFetched!.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
+      expect(customRoleFetched!.permissionId).toEqual(
+        BuiltinPermissionID.READ_ONLY
+      )
+    })
+
+    it("should be able to get the role with a permission added", async () => {
+      const table = await config.createTable()
+      await config.api.permission.add({
+        roleId: BUILTIN_ROLE_IDS.POWER,
+        resourceId: table._id!,
+        level: PermissionLevel.READ,
+      })
+      const res = await config.api.roles.fetch()
+      expect(res.length).toBeGreaterThan(0)
+      const power = res.find(role => role._id === BUILTIN_ROLE_IDS.POWER)
+      expect(power?.permissions[table._id!]).toEqual(["read"])
+    })
+  })
+
+  describe("destroy", () => {
+    it("should delete custom roles", async () => {
+      const customRole = await config.createRole({
+        name: "user",
+        permissionId: BuiltinPermissionID.READ_ONLY,
+        inherits: BUILTIN_ROLE_IDS.BASIC,
+      })
+      await config.api.roles.destroy(customRole, {
+        status: 200,
+      })
+      await config.api.roles.find(customRole._id!, {
+        status: 404,
+      })
+      expect(events.role.deleted).toHaveBeenCalledTimes(1)
+      expect(events.role.deleted).toHaveBeenCalledWith(customRole)
+    })
+  })
+
+  describe("accessible", () => {
+    it("should be able to fetch accessible roles (with builder)", async () => {
+      const res = await config.api.roles.accessible(config.defaultHeaders(), {
+        status: 200,
+      })
+      expect(res.length).toBe(5)
+      expect(typeof res[0]).toBe("string")
+    })
+
+    it("should be able to fetch accessible roles (basic user)", async () => {
+      const headers = await config.basicRoleHeaders()
+      const res = await config.api.roles.accessible(headers, {
+        status: 200,
+      })
+      expect(res.length).toBe(2)
+      expect(res[0]).toBe("BASIC")
+      expect(res[1]).toBe("PUBLIC")
+    })
+
+    it("should be able to fetch accessible roles (no user)", async () => {
+      const res = await config.api.roles.accessible(config.publicHeaders(), {
+        status: 200,
+      })
+      expect(res.length).toBe(1)
+      expect(res[0]).toBe("PUBLIC")
+    })
+
+    it("should not fetch higher level accessible roles when a custom role header is provided", async () => {
+      const customRoleName = "CUSTOM_ROLE"
+      await config.api.roles.save({
+        name: customRoleName,
+        inherits: roles.BUILTIN_ROLE_IDS.BASIC,
+        permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+        version: "name",
+      })
+      const res = await config.api.roles.accessible(
+        { "x-budibase-role": customRoleName },
+        {
+          status: 200,
+        }
+      )
+      expect(res.length).toBe(3)
+      expect(res[0]).toBe(customRoleName)
+      expect(res[1]).toBe("BASIC")
+      expect(res[2]).toBe("PUBLIC")
+    })
+  })
+})
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index 3d53149385..0255268097 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -517,6 +517,7 @@ export default class TestConfiguration {
 
     const headers: any = {
       Accept: "application/json",
+      Cookie: "",
     }
     if (appId) {
       headers[constants.Header.APP_ID] = appId
diff --git a/packages/server/src/tests/utilities/api/role.ts b/packages/server/src/tests/utilities/api/role.ts
index 4defbc1220..31bffc6f85 100644
--- a/packages/server/src/tests/utilities/api/role.ts
+++ b/packages/server/src/tests/utilities/api/role.ts
@@ -4,6 +4,7 @@ import {
   FindRoleResponse,
   SaveRoleRequest,
   SaveRoleResponse,
+  Role,
 } from "@budibase/types"
 import { Expectations, TestAPI } from "./base"
 
@@ -27,14 +28,18 @@ export class RoleAPI extends TestAPI {
     })
   }
 
-  destroy = async (roleId: string, expectations?: Expectations) => {
-    return await this._delete(`/api/roles/${roleId}`, {
+  destroy = async (role: Role, expectations?: Expectations) => {
+    return await this._delete(`/api/roles/${role._id}/${role._rev}`, {
       expectations,
     })
   }
 
-  accesssible = async (expectations?: Expectations) => {
+  accessible = async (
+    headers: Record<string, string | string[]>,
+    expectations?: Expectations
+  ) => {
     return await this._get<AccessibleRolesResponse>(`/api/roles/accessible`, {
+      headers,
       expectations,
     })
   }
diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts
index 698f6d8236..e572447ab4 100644
--- a/packages/server/src/tests/utilities/structures.ts
+++ b/packages/server/src/tests/utilities/structures.ts
@@ -30,6 +30,7 @@ import {
   BBReferenceFieldSubType,
   JsonFieldSubType,
   AutoFieldSubType,
+  Role,
 } from "@budibase/types"
 import { LoopInput } from "../../definitions/automations"
 import { merge } from "lodash"
@@ -492,11 +493,12 @@ export function basicLinkedRow(
   }
 }
 
-export function basicRole() {
+export function basicRole(): Role {
   return {
     name: `NewRole_${utils.newid()}`,
     inherits: roles.BUILTIN_ROLE_IDS.BASIC,
     permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+    permissions: {},
     version: "name",
   }
 }
diff --git a/packages/types/src/api/web/role.ts b/packages/types/src/api/web/role.ts
index c37dee60e0..642f815cc4 100644
--- a/packages/types/src/api/web/role.ts
+++ b/packages/types/src/api/web/role.ts
@@ -4,9 +4,9 @@ export interface SaveRoleRequest {
   _id?: string
   _rev?: string
   name: string
-  inherits: string
+  inherits?: string | string[]
   permissionId: string
-  version: string
+  version?: string
 }
 
 export interface SaveRoleResponse extends Role {}
diff --git a/packages/types/src/documents/app/role.ts b/packages/types/src/documents/app/role.ts
index f32ba810b0..669e8f523c 100644
--- a/packages/types/src/documents/app/role.ts
+++ b/packages/types/src/documents/app/role.ts
@@ -2,7 +2,7 @@ import { Document } from "../document"
 
 export interface Role extends Document {
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
   permissions: { [key: string]: string[] }
   version?: string
   name: string
diff --git a/packages/types/src/sdk/events/role.ts b/packages/types/src/sdk/events/role.ts
index b04b9b8ee5..ce17b34dc4 100644
--- a/packages/types/src/sdk/events/role.ts
+++ b/packages/types/src/sdk/events/role.ts
@@ -3,19 +3,19 @@ import { BaseEvent } from "./event"
 export interface RoleCreatedEvent extends BaseEvent {
   roleId: string
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
 }
 
 export interface RoleUpdatedEvent extends BaseEvent {
   roleId: string
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
 }
 
 export interface RoleDeletedEvent extends BaseEvent {
   roleId: string
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
 }
 
 export interface RoleAssignedEvent extends BaseEvent {

From 10e187014a30a6b5b3703a90735f1984f0d5f632 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 26 Sep 2024 18:02:32 +0100
Subject: [PATCH 02/28] Fixing type issues.

---
 packages/types/src/api/web/role.ts       | 2 +-
 packages/types/src/documents/app/role.ts | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/types/src/api/web/role.ts b/packages/types/src/api/web/role.ts
index 9a768e3ac3..4e56f6cd14 100644
--- a/packages/types/src/api/web/role.ts
+++ b/packages/types/src/api/web/role.ts
@@ -6,7 +6,7 @@ export interface SaveRoleRequest {
   name: string
   inherits?: string | string[]
   permissionId: string
-  version: string
+  version?: string
   uiMetadata?: RoleUIMetadata
 }
 
diff --git a/packages/types/src/documents/app/role.ts b/packages/types/src/documents/app/role.ts
index 6557b7e19d..22f4ab9cd3 100644
--- a/packages/types/src/documents/app/role.ts
+++ b/packages/types/src/documents/app/role.ts
@@ -9,7 +9,7 @@ export interface RoleUIMetadata {
 
 export interface Role extends Document {
   permissionId: string
-  inherits?: string
+  inherits?: string | string[]
   permissions: Record<string, PermissionLevel[]>
   version?: string
   name: string

From fa9bb030c923dd62561388e2e3fb4000d8c952d3 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 26 Sep 2024 18:26:35 +0100
Subject: [PATCH 03/28] Utility for detecting loops in a list of roles.

---
 packages/shared-core/src/helpers/roles.ts     | 47 ++++++++++++++
 .../src/helpers/tests/roles.spec.ts           | 61 +++++++++++++++++++
 2 files changed, 108 insertions(+)
 create mode 100644 packages/shared-core/src/helpers/roles.ts
 create mode 100644 packages/shared-core/src/helpers/tests/roles.spec.ts

diff --git a/packages/shared-core/src/helpers/roles.ts b/packages/shared-core/src/helpers/roles.ts
new file mode 100644
index 0000000000..27f28cff82
--- /dev/null
+++ b/packages/shared-core/src/helpers/roles.ts
@@ -0,0 +1,47 @@
+import { Role } from "@budibase/types"
+
+// Function to detect loops in roles
+export function checkForRoleInheritanceLoops(roles: Role[]): boolean {
+  const roleMap = new Map<string, Role>()
+  roles.forEach(role => {
+    roleMap.set(role._id!, role)
+  })
+
+  const checked = new Set<string>()
+  const checking = new Set<string>()
+
+  function hasLoop(roleId: string): boolean {
+    if (checking.has(roleId)) {
+      return true
+    }
+    if (checked.has(roleId)) {
+      return false
+    }
+
+    checking.add(roleId)
+
+    const role = roleMap.get(roleId)
+    if (!role) {
+      // role not found - ignore
+      checking.delete(roleId)
+      return false
+    }
+
+    const inherits = Array.isArray(role.inherits)
+      ? role.inherits
+      : [role.inherits]
+    for (const inheritedId of inherits) {
+      if (inheritedId && hasLoop(inheritedId)) {
+        return true
+      }
+    }
+
+    // mark this role has been fully checked
+    checking.delete(roleId)
+    checked.add(roleId)
+
+    return false
+  }
+
+  return !!roles.find(role => hasLoop(role._id!))
+}
diff --git a/packages/shared-core/src/helpers/tests/roles.spec.ts b/packages/shared-core/src/helpers/tests/roles.spec.ts
new file mode 100644
index 0000000000..7e52ded3f6
--- /dev/null
+++ b/packages/shared-core/src/helpers/tests/roles.spec.ts
@@ -0,0 +1,61 @@
+import { checkForRoleInheritanceLoops } from "../roles"
+import { Role } from "@budibase/types"
+
+/**
+ * This unit test exists as this utility will be used in the frontend and backend, confirmation
+ * of its API and expected results is useful since the backend tests won't confirm it works
+ * exactly as the frontend needs it to - easy to add specific test cases here that the frontend
+ * might need to check/cover.
+ */
+
+interface TestRole extends Omit<Role, "_id"> {
+  _id: string
+}
+
+let allRoles: TestRole[] = []
+
+function role(id: string, inherits: string | string[]): TestRole {
+  const role = {
+    _id: id,
+    inherits: inherits,
+    name: "ROLE",
+    permissionId: "PERMISSION",
+    permissions: {}, // not needed for this test
+  }
+  allRoles.push(role)
+  return role
+}
+
+describe("role utilities", () => {
+  let role1: TestRole, role2: TestRole
+
+  beforeEach(() => {
+    role1 = role("role_1", [])
+    role2 = role("role_2", [role1._id])
+  })
+
+  afterEach(() => {
+    allRoles = []
+  })
+
+  function check(hasLoop: boolean) {
+    const result = checkForRoleInheritanceLoops(allRoles)
+    expect(result).toBe(hasLoop)
+  }
+
+  describe("checkForRoleInheritanceLoops", () => {
+    it("should confirm no loops", () => {
+      check(false)
+    })
+
+    it("should confirm there is a loop", () => {
+      const role3 = role("role_3", [role2._id])
+      const role4 = role("role_4", [role3._id, role2._id, role1._id])
+      role3.inherits = [
+        ...(Array.isArray(role3.inherits) ? role3.inherits : []),
+        role4._id,
+      ]
+      check(true)
+    })
+  })
+})

From d6d4da221da18029f635ab5baf6d9728e9fa9fba Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Fri, 27 Sep 2024 17:05:03 +0100
Subject: [PATCH 04/28] Updating role validator.

---
 packages/server/src/api/routes/utils/validators.ts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/packages/server/src/api/routes/utils/validators.ts b/packages/server/src/api/routes/utils/validators.ts
index b589d44b31..925593a8cd 100644
--- a/packages/server/src/api/routes/utils/validators.ts
+++ b/packages/server/src/api/routes/utils/validators.ts
@@ -226,7 +226,10 @@ export function roleValidator() {
           )
         )
         .optional(),
-      inherits: OPTIONAL_STRING,
+      inherits: Joi.alternatives().try(
+        OPTIONAL_STRING,
+        Joi.array().items(OPTIONAL_STRING)
+      ),
     }).unknown(true)
   )
 }

From 5f91c7d8da4960b7fd45d8999000e4f8bc836749 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 10 Oct 2024 16:11:03 +0100
Subject: [PATCH 05/28] new test case.

---
 .../server/src/api/routes/tests/role.spec.ts  | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 127be789b9..6531461e43 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -161,4 +161,37 @@ describe("/roles", () => {
       expect(res[2]).toBe("PUBLIC")
     })
   })
+
+  describe("accessible - multi-inheritance", () => {
+    it("should list access correctly for multi-inheritance role", async () => {
+      const role1 = "custom_role_1",
+        role2 = "custom_role_2",
+        role3 = "custom_role_3"
+      const { _id: roleId1 } = await config.api.roles.save({
+        name: role1,
+        inherits: roles.BUILTIN_ROLE_IDS.BASIC,
+        permissionId: permissions.BuiltinPermissionID.WRITE,
+        version: "name",
+      })
+      const { _id: roleId2 } = await config.api.roles.save({
+        name: role2,
+        inherits: roles.BUILTIN_ROLE_IDS.POWER,
+        permissionId: permissions.BuiltinPermissionID.POWER,
+        version: "name",
+      })
+      await config.api.roles.save({
+        name: role3,
+        inherits: role1,
+        permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+        version: "name",
+      })
+      const headers = await config.roleHeaders({
+        roleId: role3,
+      })
+      const res = await config.api.roles.accessible(headers, {
+        status: 200,
+      })
+      expect(res.length).toBe(4)
+    })
+  })
 })

From 324616be59e4832b6026edbeae73fff0d9b9d853 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 10 Oct 2024 18:15:23 +0100
Subject: [PATCH 06/28] Finishing multi-inheritance test case and getting
 accessibility to be detected correctly.

---
 packages/backend-core/src/security/roles.ts   | 121 ++++++++++++------
 .../server/src/api/routes/tests/role.spec.ts  |   9 +-
 packages/shared-core/src/helpers/index.ts     |   1 +
 3 files changed, 86 insertions(+), 45 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 108bc0414c..9d23463fa3 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -9,7 +9,7 @@ import {
 import { getAppDB } from "../context"
 import { Screen, Role as RoleDoc, RoleUIMetadata } from "@budibase/types"
 import cloneDeep from "lodash/fp/cloneDeep"
-import { RoleColor } from "@budibase/shared-core"
+import { RoleColor, helpers } from "@budibase/shared-core"
 
 export const BUILTIN_ROLE_IDS = {
   ADMIN: "ADMIN",
@@ -204,6 +204,36 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string {
     : roleId1
 }
 
+/**
+ * Given a list of roles, this will pick the role out, accounting for built ins.
+ */
+export function findRole(
+  roleId: string,
+  roles: RoleDoc[],
+  opts?: { defaultPublic?: boolean }
+): RoleDoc {
+  // 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)
+  if (!role) {
+    // make sure has the prefix (if it has it then it won't be added)
+    roleId = prefixRoleID(roleId)
+  }
+  const dbRole = roles.find(
+    role => role._id && role._id === getExternalRoleID(roleId, role.version)
+  )
+  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")
+  }
+  role = Object.assign(role || {}, dbRole)
+  // finalise the ID
+  role._id = getExternalRoleID(role._id!, role.version)
+  return role
+}
+
 /**
  * Gets the role object, this is mainly useful for two purposes, to check if the level exists and
  * to check if the role inherits any others.
@@ -215,29 +245,15 @@ export async function getRole(
   roleId: string,
   opts?: { defaultPublic?: boolean }
 ): Promise<RoleDoc> {
-  // 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)
-  if (!role) {
-    // make sure has the prefix (if it has it then it won't be added)
-    roleId = prefixRoleID(roleId)
-  }
-  try {
-    const db = getAppDB()
-    const dbRole = await db.get<RoleDoc>(getDBRoleID(roleId))
-    role = Object.assign(role || {}, dbRole)
-    // finalise the ID
-    role._id = getExternalRoleID(role._id!, role.version)
-  } catch (err) {
-    if (!isBuiltin(roleId) && opts?.defaultPublic) {
-      return cloneDeep(BUILTIN_ROLES.PUBLIC)
-    }
-    // only throw an error if there is no role at all
-    if (!role || Object.keys(role).length === 0) {
-      throw err
+  const db = getAppDB()
+  const roleList = []
+  if (!isBuiltin(roleId)) {
+    const role = await db.tryGet<RoleDoc>(getDBRoleID(roleId))
+    if (role) {
+      roleList.push(role)
     }
   }
-  return role
+  return findRole(roleId, roleList, opts)
 }
 
 /**
@@ -247,13 +263,14 @@ async function getAllUserRoles(
   userRoleId: string,
   opts?: { defaultPublic?: boolean }
 ): Promise<RoleDoc[]> {
+  const allRoles = await getAllRoles()
+  if (helpers.roles.checkForRoleInheritanceLoops(allRoles)) {
+    throw new Error("Loop detected in roles - cannot list roles")
+  }
   // admins have access to all roles
   if (userRoleId === BUILTIN_IDS.ADMIN) {
-    return getAllRoles()
+    return allRoles
   }
-  let currentRole = await getRole(userRoleId, opts)
-  let roles = currentRole ? [currentRole] : []
-  let roleIds = [userRoleId]
   const rolesFound = (ids: string | string[]) => {
     if (Array.isArray(ids)) {
       return ids.filter(id => roleIds.includes(id)).length === ids.length
@@ -261,23 +278,49 @@ async function getAllUserRoles(
       return roleIds.includes(ids)
     }
   }
-  // get all the inherited roles
-  while (
-    currentRole &&
-    currentRole.inherits &&
-    !rolesFound(currentRole.inherits)
-  ) {
-    if (Array.isArray(currentRole.inherits)) {
-      // TODO: role inheritance
+
+  const roleIds = [userRoleId]
+  const roles: RoleDoc[] = []
+  const iterateInherited = (role: RoleDoc) => {
+    if (!role || !role._id) {
+      return
+    }
+    roleIds.push(role._id)
+    roles.push(role)
+    if (Array.isArray(role.inherits)) {
+      role.inherits.forEach(roleId => {
+        const foundRole = findRole(roleId, allRoles, opts)
+        if (foundRole) {
+          iterateInherited(foundRole)
+        }
+      })
     } else {
-      roleIds.push(currentRole.inherits)
-      currentRole = await getRole(currentRole.inherits)
-      if (currentRole) {
-        roles.push(currentRole)
+      while (role && role.inherits && !rolesFound(role.inherits)) {
+        if (Array.isArray(role.inherits)) {
+          iterateInherited(role)
+          break
+        } else {
+          roleIds.push(role.inherits)
+          role = findRole(role.inherits, allRoles, opts)
+          if (role) {
+            roles.push(role)
+          }
+        }
       }
     }
   }
-  return roles
+
+  // get all the inherited roles
+  iterateInherited(findRole(userRoleId, allRoles, opts))
+  const foundRoleIds: string[] = []
+  return roles.filter(role => {
+    if (role._id && !foundRoleIds.includes(role._id)) {
+      foundRoleIds.push(role._id)
+      return true
+    } else {
+      return false
+    }
+  })
 }
 
 export async function getUserRoleIdHierarchy(
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index f9eb1c3651..682ebf2f7a 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -155,10 +155,7 @@ describe("/roles", () => {
           status: 200,
         }
       )
-      expect(res.length).toBe(3)
-      expect(res[0]).toBe(customRoleName)
-      expect(res[1]).toBe("BASIC")
-      expect(res[2]).toBe("PUBLIC")
+      expect(res).toEqual([customRoleName, "BASIC", "PUBLIC"])
     })
   })
 
@@ -181,7 +178,7 @@ describe("/roles", () => {
       })
       await config.api.roles.save({
         name: role3,
-        inherits: role1,
+        inherits: [roleId1!, roleId2!],
         permissionId: permissions.BuiltinPermissionID.READ_ONLY,
         version: "name",
       })
@@ -191,7 +188,7 @@ describe("/roles", () => {
       const res = await config.api.roles.accessible(headers, {
         status: 200,
       })
-      expect(res.length).toBe(4)
+      expect(res).toEqual([role3, role1, "BASIC", "PUBLIC", role2, "POWER"])
     })
   })
 })
diff --git a/packages/shared-core/src/helpers/index.ts b/packages/shared-core/src/helpers/index.ts
index 503f71e4eb..7603a9b88b 100644
--- a/packages/shared-core/src/helpers/index.ts
+++ b/packages/shared-core/src/helpers/index.ts
@@ -3,3 +3,4 @@ export * from "./integrations"
 export * as cron from "./cron"
 export * as schema from "./schema"
 export * as views from "./views"
+export * as roles from "./roles"

From 676cb3f92e9dcc49b7f2a5de1177d34217bf06f2 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 14 Oct 2024 18:00:41 +0100
Subject: [PATCH 07/28] Handling role numbering.

---
 packages/backend-core/src/security/roles.ts | 29 +++++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 9d23463fa3..3c36a91c48 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -157,7 +157,7 @@ export function builtinRoleToNumber(id: string) {
       break
     }
     if (Array.isArray(role.inherits)) {
-      // TODO: role inheritance
+      throw new Error("Built-in roles don't support multi-inheritance")
     } else {
       role = builtins[role.inherits!]
     }
@@ -176,17 +176,36 @@ export async function roleToNumber(id: string) {
   const hierarchy = (await getUserRoleHierarchy(id, {
     defaultPublic: true,
   })) as RoleDoc[]
-  for (let role of hierarchy) {
+  const findNumber = (role: RoleDoc): number => {
     if (!role.inherits) {
-      continue
+      return 0
     }
     if (Array.isArray(role.inherits)) {
-      // TODO: role inheritance
+      // find the built-in roles, get their number, sort it, then get the last one
+      const highestBuiltin: number | undefined = role.inherits
+        .map(roleId => {
+          const foundRole = hierarchy.find(role => role._id === roleId)
+          if (foundRole) {
+            return findNumber(foundRole) + 1
+          }
+        })
+        .filter(number => !!number)
+        .sort()
+        .pop()
+      if (highestBuiltin != undefined) {
+        return highestBuiltin
+      }
     } else if (isBuiltin(role.inherits)) {
       return builtinRoleToNumber(role.inherits) + 1
     }
+    return 0
   }
-  return 0
+  let highest = 0
+  for (let role of hierarchy) {
+    const roleNumber = findNumber(role)
+    highest = Math.max(roleNumber, highest)
+  }
+  return highest
 }
 
 /**

From 26ee50b10b54534022c63262b948fbcbafca57f1 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 14 Oct 2024 18:57:46 +0100
Subject: [PATCH 08/28] Adding test case for multi-inheritance

---
 .../src/api/routes/tests/permissions.spec.ts  | 82 ++++++++++++++++++-
 .../src/tests/utilities/TestConfiguration.ts  | 28 +++++++
 2 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts
index a479adb4cf..46ce656459 100644
--- a/packages/server/src/api/routes/tests/permissions.spec.ts
+++ b/packages/server/src/api/routes/tests/permissions.spec.ts
@@ -1,5 +1,5 @@
 import { roles } from "@budibase/backend-core"
-import { Document, PermissionLevel, Row } from "@budibase/types"
+import { Document, PermissionLevel, Role, Row, Table } from "@budibase/types"
 import * as setup from "./utilities"
 import { generator, mocks } from "@budibase/backend-core/tests"
 
@@ -288,6 +288,86 @@ describe("/permission", () => {
     })
   })
 
+  describe("multi-inheritance permissions", () => {
+    let table1: Table, table2: Table, role1: Role, role2: Role
+    beforeEach(async () => {
+      table1 = await config.createTable()
+      table2 = await config.createTable()
+      await config.api.row.save(table1._id!, {
+        name: "a",
+      })
+      await config.api.row.save(table2._id!, {
+        name: "b",
+      })
+      role1 = await config.api.roles.save(
+        {
+          name: "role1",
+          permissionId: PermissionLevel.WRITE,
+          inherits: BUILTIN_ROLE_IDS.BASIC,
+        },
+        { status: 200 }
+      )
+      role2 = await config.api.roles.save(
+        {
+          name: "role2",
+          permissionId: PermissionLevel.WRITE,
+          inherits: BUILTIN_ROLE_IDS.BASIC,
+        },
+        { status: 200 }
+      )
+      await config.api.permission.add({
+        roleId: role1._id!,
+        level: PermissionLevel.READ,
+        resourceId: table1._id!,
+      })
+      await config.api.permission.add({
+        roleId: role2._id!,
+        level: PermissionLevel.READ,
+        resourceId: table2._id!,
+      })
+    })
+
+    it("should be unable to search for table 2 using role 1", async () => {
+      await config.setRole(role1._id!, async () => {
+        const response2 = await config.api.row.search(
+          table2._id!,
+          {
+            query: {},
+          },
+          { status: 403 }
+        )
+        expect(response2.rows).toBeUndefined()
+      })
+    })
+
+    it("should be able to fetch two tables, with different roles, using multi-inheritance", async () => {
+      const role3 = await config.api.roles.save({
+        name: "role3",
+        permissionId: PermissionLevel.WRITE,
+        inherits: [role1._id!, role2._id!],
+      })
+
+      await config.setRole(role3._id!, async () => {
+        const response1 = await config.api.row.search(
+          table1._id!,
+          {
+            query: {},
+          },
+          { status: 200 }
+        )
+        const response2 = await config.api.row.search(
+          table2._id!,
+          {
+            query: {},
+          },
+          { status: 200 }
+        )
+        expect(response1.rows[0].name).toEqual("a")
+        expect(response2.rows[0].name).toEqual("b")
+      })
+    })
+  })
+
   describe("fetch builtins", () => {
     it("should be able to fetch builtin definitions", async () => {
       const res = await request
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index f320df2ff8..a3f2fb7adc 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -428,6 +428,34 @@ export default class TestConfiguration {
 
   // HEADERS
 
+  // sets the role for the headers, for the period of a callback
+  async setRole(roleId: string, cb: () => Promise<unknown>) {
+    const roleUser = await this.createUser({
+      roles: {
+        [this.prodAppId!]: roleId,
+      },
+      builder: { global: false },
+      admin: { global: false },
+    })
+    await this.login({
+      roleId,
+      userId: roleUser._id!,
+      builder: false,
+      prodApp: true,
+    })
+    const temp = this.user
+    this.user = roleUser
+    await cb()
+    if (temp) {
+      this.user = temp
+      await this.login({
+        userId: temp._id!,
+        builder: true,
+        prodApp: false,
+      })
+    }
+  }
+
   defaultHeaders(extras = {}, prodApp = false) {
     const tenantId = this.getTenantId()
     const user = this.getUser()

From a56a22804240e973d8c46f93b7a2e327b83b7136 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 14 Oct 2024 18:57:54 +0100
Subject: [PATCH 09/28] Fixes based on test case.

---
 packages/backend-core/src/security/roles.ts     | 2 +-
 packages/server/src/tests/utilities/api/role.ts | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 3c36a91c48..cd55e2f728 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -516,7 +516,7 @@ export function getDBRoleID(roleName: string) {
 export function getExternalRoleID(roleId: string, version?: string) {
   // for built-in roles we want to remove the DB role ID element (role_)
   if (
-    roleId.startsWith(DocumentType.ROLE) &&
+    roleId.startsWith(`${DocumentType.ROLE}${SEPARATOR}`) &&
     (isBuiltin(roleId) || version === RoleIDVersion.NAME)
   ) {
     const parts = roleId.split(SEPARATOR)
diff --git a/packages/server/src/tests/utilities/api/role.ts b/packages/server/src/tests/utilities/api/role.ts
index 31bffc6f85..05165cd38e 100644
--- a/packages/server/src/tests/utilities/api/role.ts
+++ b/packages/server/src/tests/utilities/api/role.ts
@@ -22,6 +22,10 @@ export class RoleAPI extends TestAPI {
   }
 
   save = async (body: SaveRoleRequest, expectations?: Expectations) => {
+    // the tests should always be creating the "new" version of roles
+    if (body.version === undefined) {
+      body.version = "name"
+    }
     return await this._post<SaveRoleResponse>(`/api/roles`, {
       body,
       expectations,

From bb43049d55e5a43d9019ca0d6dae54d6552ed1a6 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 15 Oct 2024 12:07:31 +0100
Subject: [PATCH 10/28] Adding loop protection.

---
 packages/server/src/api/controllers/role.ts | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index f3fd5d7b46..76638eea23 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -19,7 +19,7 @@ import {
   UserMetadata,
   DocumentType,
 } from "@budibase/types"
-import { RoleColor, sdk as sharedSdk } from "@budibase/shared-core"
+import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core"
 import sdk from "../../sdk"
 
 const UpdateRolesOptions = {
@@ -81,9 +81,10 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     _id = dbCore.prefixRoleID(_id)
   }
 
+  const allRoles = await roles.getAllRoles()
   let dbRole: Role | undefined
   if (!isCreate && _id?.startsWith(DocumentType.ROLE)) {
-    dbRole = await db.get<Role>(_id)
+    dbRole = allRoles.find(role => role._id === _id)
   }
   if (dbRole && dbRole.name !== name && isNewVersion) {
     ctx.throw(400, "Cannot change custom role name")
@@ -97,6 +98,18 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
   if (dbRole?.permissions && !role.permissions) {
     role.permissions = dbRole.permissions
   }
+
+  // add the new role to the list and check for loops
+  const index = allRoles.findIndex(r => r._id === role._id)
+  if (index === -1) {
+    allRoles.push(role)
+  } else {
+    allRoles[index] = role
+  }
+  if (helpers.roles.checkForRoleInheritanceLoops(allRoles)) {
+    ctx.throw(400, "Role inheritance contains a loop, this is not supported")
+  }
+
   const foundRev = ctx.request.body._rev || dbRole?._rev
   if (foundRev) {
     role._rev = foundRev

From 808096f31b8e0cc96ea186b8baaa76fb8fe24483 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 15 Oct 2024 14:34:32 +0100
Subject: [PATCH 11/28] Updating screen test case.

---
 .../src/api/routes/tests/screen.spec.js       | 103 ------------------
 .../src/api/routes/tests/screen.spec.ts       |  94 ++++++++++++++++
 .../server/src/tests/utilities/api/screen.ts  |  23 ++++
 3 files changed, 117 insertions(+), 103 deletions(-)
 delete mode 100644 packages/server/src/api/routes/tests/screen.spec.js
 create mode 100644 packages/server/src/api/routes/tests/screen.spec.ts

diff --git a/packages/server/src/api/routes/tests/screen.spec.js b/packages/server/src/api/routes/tests/screen.spec.js
deleted file mode 100644
index 481baac703..0000000000
--- a/packages/server/src/api/routes/tests/screen.spec.js
+++ /dev/null
@@ -1,103 +0,0 @@
-const { checkBuilderEndpoint } = require("./utilities/TestFunctions")
-const setup = require("./utilities")
-const { basicScreen } = setup.structures
-const { events } = require("@budibase/backend-core")
-
-describe("/screens", () => {
-  let request = setup.getRequest()
-  let config = setup.getConfig()
-  let screen
-
-  afterAll(setup.afterAll)
-
-  beforeAll(async () => {
-    await config.init()
-    screen = await config.createScreen()
-  })
-
-  describe("fetch", () => {
-    it("should be able to create a layout", async () => {
-      const res = await request
-        .get(`/api/screens`)
-        .set(config.defaultHeaders())
-        .expect("Content-Type", /json/)
-        .expect(200)
-      expect(res.body.length).toEqual(1)
-      expect(res.body.some(s => s._id === screen._id)).toEqual(true)
-    })
-
-    it("should apply authorization to endpoint", async () => {
-      await checkBuilderEndpoint({
-        config,
-        method: "GET",
-        url: `/api/screens`,
-      })
-    })
-  })
-
-  describe("save", () => {
-    const saveScreen = async screen => {
-      const res = await request
-        .post(`/api/screens`)
-        .send(screen)
-        .set(config.defaultHeaders())
-        .expect("Content-Type", /json/)
-        .expect(200)
-      return res
-    }
-
-    it("should be able to create a screen", async () => {
-      jest.clearAllMocks()
-
-      const screen = basicScreen()
-      const res = await saveScreen(screen)
-
-      expect(res.body._rev).toBeDefined()
-      expect(res.body.name).toEqual(screen.name)
-      expect(events.screen.created).toBeCalledTimes(1)
-    })
-
-    it("should be able to update a screen", async () => {
-      const screen = basicScreen()
-      let res = await saveScreen(screen)
-      screen._id = res.body._id
-      screen._rev = res.body._rev
-      screen.name = "edit"
-      jest.clearAllMocks()
-
-      res = await saveScreen(screen)
-
-      expect(res.body._rev).toBeDefined()
-      expect(res.body.name).toEqual(screen.name)
-      expect(events.screen.created).not.toBeCalled()
-    })
-
-    it("should apply authorization to endpoint", async () => {
-      await checkBuilderEndpoint({
-        config,
-        method: "POST",
-        url: `/api/screens`,
-      })
-    })
-  })
-
-  describe("destroy", () => {
-    it("should be able to delete the screen", async () => {
-      const res = await request
-        .delete(`/api/screens/${screen._id}/${screen._rev}`)
-        .set(config.defaultHeaders())
-        .expect("Content-Type", /json/)
-        .expect(200)
-      expect(res.body.message).toBeDefined()
-      expect(events.screen.deleted).toBeCalledTimes(1)
-    })
-
-    it("should apply authorization to endpoint", async () => {
-      await checkBuilderEndpoint({
-        config,
-        method: "DELETE",
-        url: `/api/screens/${screen._id}/${screen._rev}`,
-      })
-    })
-  })
-})
diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts
new file mode 100644
index 0000000000..73c3062058
--- /dev/null
+++ b/packages/server/src/api/routes/tests/screen.spec.ts
@@ -0,0 +1,94 @@
+import { checkBuilderEndpoint } from "./utilities/TestFunctions"
+import * as setup from "./utilities"
+import { events } from "@budibase/backend-core"
+import { Screen } from "@budibase/types"
+
+const { basicScreen } = setup.structures
+
+describe("/screens", () => {
+  let config = setup.getConfig()
+  let screen: Screen
+
+  afterAll(setup.afterAll)
+
+  beforeAll(async () => {
+    await config.init()
+    screen = await config.createScreen()
+  })
+
+  describe("fetch", () => {
+    it("should be able to create a layout", async () => {
+      const screens = await config.api.screen.list({ status: 200 })
+      expect(screens.length).toEqual(1)
+      expect(screens.some(s => s._id === screen._id)).toEqual(true)
+    })
+
+    it("should apply authorization to endpoint", async () => {
+      await checkBuilderEndpoint({
+        config,
+        method: "GET",
+        url: `/api/screens`,
+      })
+    })
+  })
+
+  describe("save", () => {
+    const saveScreen = async (screen: Screen) => {
+      return await config.api.screen.save(screen, { status: 200 })
+    }
+
+    it("should be able to create a screen", async () => {
+      jest.clearAllMocks()
+
+      const screen = basicScreen()
+      const responseScreen = await saveScreen(screen)
+
+      expect(responseScreen._rev).toBeDefined()
+      expect(responseScreen.name).toEqual(screen.name)
+      expect(events.screen.created).toHaveBeenCalledTimes(1)
+    })
+
+    it("should be able to update a screen", async () => {
+      const screen = basicScreen()
+      let responseScreen = await saveScreen(screen)
+      screen._id = responseScreen._id
+      screen._rev = responseScreen._rev
+      screen.name = "edit"
+      jest.clearAllMocks()
+
+      responseScreen = await saveScreen(screen)
+
+      expect(responseScreen._rev).toBeDefined()
+      expect(responseScreen.name).toEqual(screen.name)
+      expect(events.screen.created).not.toHaveBeenCalled()
+    })
+
+    it("should apply authorization to endpoint", async () => {
+      await checkBuilderEndpoint({
+        config,
+        method: "POST",
+        url: `/api/screens`,
+      })
+    })
+  })
+
+  describe("destroy", () => {
+    it("should be able to delete the screen", async () => {
+      const response = await config.api.screen.destroy(
+        screen._id!,
+        screen._rev!,
+        { status: 200 }
+      )
+      expect(response.message).toBeDefined()
+      expect(events.screen.deleted).toHaveBeenCalledTimes(1)
+    })
+
+    it("should apply authorization to endpoint", async () => {
+      await checkBuilderEndpoint({
+        config,
+        method: "DELETE",
+        url: `/api/screens/${screen._id}/${screen._rev}`,
+      })
+    })
+  })
+})
diff --git a/packages/server/src/tests/utilities/api/screen.ts b/packages/server/src/tests/utilities/api/screen.ts
index c8d3e647d8..8b7d7f92c1 100644
--- a/packages/server/src/tests/utilities/api/screen.ts
+++ b/packages/server/src/tests/utilities/api/screen.ts
@@ -5,4 +5,27 @@ export class ScreenAPI extends TestAPI {
   list = async (expectations?: Expectations): Promise<Screen[]> => {
     return await this._get<Screen[]>(`/api/screens`, { expectations })
   }
+
+  save = async (
+    screen: Screen,
+    expectations?: Expectations
+  ): Promise<Screen> => {
+    return await this._post<Screen>(`/api/screens`, {
+      expectations,
+      body: screen,
+    })
+  }
+
+  destroy = async (
+    screenId: string,
+    screenRev: string,
+    expectations?: Expectations
+  ): Promise<{ message: string }> => {
+    return this._delete<{ message: string }>(
+      `/api/screens/${screenId}/${screenRev}`,
+      {
+        expectations,
+      }
+    )
+  }
 }

From 9fceef0fc26c43090e98a52f9d0ccc08d32e3578 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 15 Oct 2024 17:53:48 +0100
Subject: [PATCH 12/28] Some more fixes and test case for screen access.

---
 packages/backend-core/src/security/roles.ts   | 12 ++-
 .../src/api/routes/tests/screen.spec.ts       | 94 ++++++++++++++++---
 2 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index cd55e2f728..41a55c55bd 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -223,6 +223,11 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string {
     : roleId1
 }
 
+function compareRoleIds(roleId1: string, roleId2: string) {
+  // make sure both role IDs are prefixed correctly
+  return prefixRoleID(roleId1) === prefixRoleID(roleId2)
+}
+
 /**
  * Given a list of roles, this will pick the role out, accounting for built ins.
  */
@@ -239,7 +244,7 @@ export function findRole(
     roleId = prefixRoleID(roleId)
   }
   const dbRole = roles.find(
-    role => role._id && role._id === getExternalRoleID(roleId, role.version)
+    role => role._id && compareRoleIds(role._id, roleId)
   )
   if (!dbRole && !isBuiltin(roleId) && opts?.defaultPublic) {
     return cloneDeep(BUILTIN_ROLES.PUBLIC)
@@ -474,7 +479,10 @@ export class AccessController {
       this.userHierarchies[userRoleId] = roleIds
     }
 
-    return roleIds?.indexOf(tryingRoleId) !== -1
+    return (
+      roleIds?.find(roleId => compareRoleIds(roleId, tryingRoleId)) !==
+      undefined
+    )
   }
 
   async checkScreensAccess(screens: Screen[], userRoleId: string) {
diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts
index 73c3062058..ff13d669ad 100644
--- a/packages/server/src/api/routes/tests/screen.spec.ts
+++ b/packages/server/src/api/routes/tests/screen.spec.ts
@@ -1,7 +1,7 @@
 import { checkBuilderEndpoint } from "./utilities/TestFunctions"
 import * as setup from "./utilities"
-import { events } from "@budibase/backend-core"
-import { Screen } from "@budibase/types"
+import { events, roles } from "@budibase/backend-core"
+import { Screen, PermissionLevel, Role } from "@budibase/types"
 
 const { basicScreen } = setup.structures
 
@@ -32,16 +32,89 @@ describe("/screens", () => {
     })
   })
 
-  describe("save", () => {
-    const saveScreen = async (screen: Screen) => {
-      return await config.api.screen.save(screen, { status: 200 })
+  describe("permissions", () => {
+    let screen1: Screen, screen2: Screen
+    let role1: Role, role2: Role, multiRole: Role
+
+    beforeAll(async () => {
+      role1 = await config.api.roles.save({
+        name: "role1",
+        inherits: roles.BUILTIN_ROLE_IDS.BASIC,
+        permissionId: PermissionLevel.WRITE,
+      })
+      role2 = await config.api.roles.save({
+        name: "role2",
+        inherits: roles.BUILTIN_ROLE_IDS.BASIC,
+        permissionId: PermissionLevel.WRITE,
+      })
+      multiRole = await config.api.roles.save({
+        name: "multiRole",
+        inherits: [role1._id!, role2._id!],
+        permissionId: PermissionLevel.WRITE,
+      })
+      screen1 = await config.api.screen.save(
+        {
+          ...basicScreen(),
+          routing: {
+            roleId: role1._id!,
+            route: "/foo",
+            homeScreen: false,
+          },
+        },
+        { status: 200 }
+      )
+      screen2 = await config.api.screen.save(
+        {
+          ...basicScreen(),
+          routing: {
+            roleId: role2._id!,
+            route: "/bar",
+            homeScreen: false,
+          },
+        },
+        { status: 200 }
+      )
+      // get into prod app
+      await config.publish()
+    })
+
+    async function checkScreens(roleId: string, screenIds: string[]) {
+      await config.setRole(roleId, async () => {
+        const res = await config.api.application.getDefinition(
+          config.prodAppId!,
+          {
+            status: 200,
+          }
+        )
+        // basic and role1 screen
+        expect(res.screens.length).toEqual(screenIds.length)
+        expect(res.screens.map(s => s._id).sort()).toEqual(screenIds.sort())
+      })
     }
 
-    it("should be able to create a screen", async () => {
-      jest.clearAllMocks()
+    it("should be able to fetch only screen1 with role1", async () => {
+      await checkScreens(role1._id!, [screen._id!, screen1._id!])
+    })
 
+    it("should be able to fetch only screen2 with role2", async () => {
+      await checkScreens(role2._id!, [screen._id!, screen2._id!])
+    })
+
+    it("should be able to fetch all three screens with multi-inheritance role", async () => {
+      await checkScreens(multiRole._id!, [
+        screen._id!,
+        screen1._id!,
+        screen2._id!,
+      ])
+    })
+  })
+
+  describe("save", () => {
+    it("should be able to create a screen", async () => {
       const screen = basicScreen()
-      const responseScreen = await saveScreen(screen)
+      const responseScreen = await config.api.screen.save(screen, {
+        status: 200,
+      })
 
       expect(responseScreen._rev).toBeDefined()
       expect(responseScreen.name).toEqual(screen.name)
@@ -50,13 +123,12 @@ describe("/screens", () => {
 
     it("should be able to update a screen", async () => {
       const screen = basicScreen()
-      let responseScreen = await saveScreen(screen)
+      let responseScreen = await config.api.screen.save(screen, { status: 200 })
       screen._id = responseScreen._id
       screen._rev = responseScreen._rev
       screen.name = "edit"
-      jest.clearAllMocks()
 
-      responseScreen = await saveScreen(screen)
+      responseScreen = await config.api.screen.save(screen, { status: 200 })
 
       expect(responseScreen._rev).toBeDefined()
       expect(responseScreen.name).toEqual(screen.name)

From 171ffd8aa33893b62913a221d745df3e217c0687 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 15 Oct 2024 17:54:34 +0100
Subject: [PATCH 13/28] Update test name.

---
 packages/server/src/api/routes/tests/screen.spec.ts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts
index ff13d669ad..f660589951 100644
--- a/packages/server/src/api/routes/tests/screen.spec.ts
+++ b/packages/server/src/api/routes/tests/screen.spec.ts
@@ -92,15 +92,15 @@ describe("/screens", () => {
       })
     }
 
-    it("should be able to fetch only screen1 with role1", async () => {
+    it("should be able to fetch basic and screen1 with role1", async () => {
       await checkScreens(role1._id!, [screen._id!, screen1._id!])
     })
 
-    it("should be able to fetch only screen2 with role2", async () => {
+    it("should be able to fetch basic and screen2 with role2", async () => {
       await checkScreens(role2._id!, [screen._id!, screen2._id!])
     })
 
-    it("should be able to fetch all three screens with multi-inheritance role", async () => {
+    it("should be able to fetch basic, screen1 and screen2 with multi-inheritance role", async () => {
       await checkScreens(multiRole._id!, [
         screen._id!,
         screen1._id!,

From 189b17606083e58157091300044fb9c1a0122d6e Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 12:27:15 +0100
Subject: [PATCH 14/28] Test for loops in role save API.

---
 packages/server/src/api/controllers/role.ts   |  5 ++++-
 .../server/src/api/routes/tests/role.spec.ts  | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 76638eea23..1e7f58e566 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -81,7 +81,10 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     _id = dbCore.prefixRoleID(_id)
   }
 
-  const allRoles = await roles.getAllRoles()
+  const allRoles = (await roles.getAllRoles()).map(role => ({
+    ...role,
+    _id: dbCore.prefixRoleID(role._id!),
+  }))
   let dbRole: Role | undefined
   if (!isCreate && _id?.startsWith(DocumentType.ROLE)) {
     dbRole = allRoles.find(role => role._id === _id)
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 682ebf2f7a..396afeb236 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -47,6 +47,25 @@ describe("/roles", () => {
       expect(events.role.updated).toHaveBeenCalledTimes(1)
       expect(events.role.updated).toHaveBeenCalledWith(res)
     })
+
+    it("disallow loops", async () => {
+      let role1 = basicRole()
+      role1 = await config.api.roles.save(role1, {
+        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",
+        },
+      })
+    })
   })
 
   describe("fetch", () => {

From 9c92288f7f94932698ff09e31de2012ee6f5d459 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 15:18:37 +0100
Subject: [PATCH 15/28] Fixing some issues with finding roles.

---
 packages/backend-core/src/security/roles.ts   | 21 +++---
 packages/server/src/api/controllers/role.ts   | 10 ++-
 .../server/src/api/routes/tests/role.spec.ts  | 75 ++++++++++++++++---
 packages/server/src/middleware/currentapp.ts  | 12 ++-
 4 files changed, 91 insertions(+), 27 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 41a55c55bd..03660b313d 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/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)) {
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 1e7f58e566..d2f15b84c2 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -57,7 +57,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>) {
@@ -202,7 +207,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) {
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 396afeb236..3a2ba3ce9d 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/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,
diff --git a/packages/server/src/middleware/currentapp.ts b/packages/server/src/middleware/currentapp.ts
index ad6f2afa18..d616377298 100644
--- a/packages/server/src/middleware/currentapp.ts
+++ b/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

From 33990931631e96cc0bb633f38a8c701c857f8087 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 15:46:03 +0100
Subject: [PATCH 16/28] Role save API returns role ID in correct format.

---
 packages/server/src/api/controllers/role.ts   |  5 ++++-
 .../server/src/api/routes/tests/role.spec.ts  | 22 +++++++++++++++----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index d2f15b84c2..8a838543b0 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -135,7 +135,10 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     role.version
   )
   role._rev = result.rev
-  ctx.body = role
+  ctx.body = {
+    ...role,
+    _id: roles.getExternalRoleID(role._id!, role.version),
+  }
 
   const devDb = context.getDevAppDB()
   const prodDb = context.getProdAppDB()
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 3a2ba3ce9d..ef3a919173 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -1,4 +1,9 @@
-import { roles, events, permissions } from "@budibase/backend-core"
+import {
+  roles,
+  events,
+  permissions,
+  db as dbCore,
+} from "@budibase/backend-core"
 import * as setup from "./utilities"
 import { PermissionLevel } from "@budibase/types"
 
@@ -28,7 +33,10 @@ describe("/roles", () => {
       expect(res._rev).toBeDefined()
       expect(events.role.updated).not.toHaveBeenCalled()
       expect(events.role.created).toHaveBeenCalledTimes(1)
-      expect(events.role.created).toHaveBeenCalledWith(res)
+      expect(events.role.created).toHaveBeenCalledWith({
+        ...res,
+        _id: dbCore.prefixRoleID(res._id!),
+      })
     })
   })
 
@@ -52,7 +60,10 @@ describe("/roles", () => {
       expect(res._rev).toBeDefined()
       expect(events.role.created).not.toHaveBeenCalled()
       expect(events.role.updated).toHaveBeenCalledTimes(1)
-      expect(events.role.updated).toHaveBeenCalledWith(res)
+      expect(events.role.updated).toHaveBeenCalledWith({
+        ...res,
+        _id: dbCore.prefixRoleID(res._id!),
+      })
     })
 
     it("disallow loops", async () => {
@@ -173,7 +184,10 @@ describe("/roles", () => {
         status: 404,
       })
       expect(events.role.deleted).toHaveBeenCalledTimes(1)
-      expect(events.role.deleted).toHaveBeenCalledWith(customRole)
+      expect(events.role.deleted).toHaveBeenCalledWith({
+        ...customRole,
+        _id: dbCore.prefixRoleID(customRole._id!),
+      })
     })
   })
 

From c40e4a72885bd551b5923c6afea11955cfcd4079 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 17:26:48 +0100
Subject: [PATCH 17/28] fixing rbac

---
 packages/backend-core/src/security/roles.ts   | 29 ++++++++++++++-
 packages/server/src/api/controllers/role.ts   | 36 ++++++++++++-------
 .../server/src/api/routes/tests/role.spec.ts  |  4 +--
 packages/shared-core/src/helpers/roles.ts     |  2 +-
 4 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 03660b313d..fdea09944a 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/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))
+  }
+}
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 8a838543b0..7b9f2d7658 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/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 = {
-    ...role,
-    _id: roles.getExternalRoleID(role._id!, role.version),
-  }
+  ctx.body = externalRole(role)
 
   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) {
-      ctx.body = []
-    } else {
-      ctx.body = await roles.getAllRoleIds(appId)
+    if (appId) {
+      roleIds = 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))
 }
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index ef3a919173..6ed5dfd30f 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/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!],
diff --git a/packages/shared-core/src/helpers/roles.ts b/packages/shared-core/src/helpers/roles.ts
index 27f28cff82..24b65501ce 100644
--- a/packages/shared-core/src/helpers/roles.ts
+++ b/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 {

From 1f9c33c53a6bb12a8b50ab7db5d862f6ea33226d Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 21:29:33 +0100
Subject: [PATCH 18/28] Test cases based on frontend.

---
 .../server/src/api/routes/tests/role.spec.ts  | 28 +++++++++++++++++++
 packages/types/src/api/web/role.ts            |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 6ed5dfd30f..134078e6bf 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -121,6 +121,34 @@ describe("/roles", () => {
         { status: 400, body: { message: LOOP_ERROR } }
       )
     })
+
+    it("frontend example - should deny", async () => {
+      const id1 = "cb27c4ec9415042f4800411adb346fb7c",
+        id2 = "cbc72a9d61ab64d49b31d90d1df4c1fdb"
+      const role1 = await config.api.roles.save({
+        _id: id1,
+        name: id1,
+        permissions: {},
+        permissionId: "write",
+        version: "name",
+        inherits: ["POWER"],
+      })
+      await config.api.roles.save({
+        _id: id2,
+        permissions: {},
+        name: id2,
+        permissionId: "write",
+        version: "name",
+        inherits: [id1],
+      })
+      await config.api.roles.save(
+        {
+          ...role1,
+          inherits: [BUILTIN_ROLE_IDS.POWER, id2],
+        },
+        { status: 400, body: { message: LOOP_ERROR } }
+      )
+    })
   })
 
   describe("fetch", () => {
diff --git a/packages/types/src/api/web/role.ts b/packages/types/src/api/web/role.ts
index 4e56f6cd14..df439e84e7 100644
--- a/packages/types/src/api/web/role.ts
+++ b/packages/types/src/api/web/role.ts
@@ -1,4 +1,5 @@
 import { Role, RoleUIMetadata } from "../../documents"
+import { PermissionLevel } from "../../sdk"
 
 export interface SaveRoleRequest {
   _id?: string
@@ -6,6 +7,7 @@ export interface SaveRoleRequest {
   name: string
   inherits?: string | string[]
   permissionId: string
+  permissions?: Record<string, PermissionLevel[]>
   version?: string
   uiMetadata?: RoleUIMetadata
 }

From 97b1c52f70c318323f74ad481326196770f30f04 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 16 Oct 2024 22:08:07 +0100
Subject: [PATCH 19/28] Protect against old roles with differences when it
 comes to loops.

---
 packages/shared-core/src/helpers/roles.ts        | 16 ++++++++++++----
 .../shared-core/src/helpers/tests/roles.spec.ts  |  7 +++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/packages/shared-core/src/helpers/roles.ts b/packages/shared-core/src/helpers/roles.ts
index 24b65501ce..8dee928edd 100644
--- a/packages/shared-core/src/helpers/roles.ts
+++ b/packages/shared-core/src/helpers/roles.ts
@@ -1,4 +1,11 @@
-import { Role, SEPARATOR, DocumentType } from "@budibase/types"
+import { Role, DocumentType, SEPARATOR } from "@budibase/types"
+
+// need to have a way to prefix, so we can check if the ID has its prefix or not
+// all new IDs should be the same in the future, but old roles they are never prefixed
+// while the role IDs always are - best to check both, also we can't access backend-core here
+function prefixForCheck(id: string) {
+  return `${DocumentType.ROLE}${SEPARATOR}${id}`
+}
 
 // Function to detect loops in roles
 export function checkForRoleInheritanceLoops(roles: Role[]): boolean {
@@ -11,16 +18,17 @@ export function checkForRoleInheritanceLoops(roles: Role[]): boolean {
   const checking = new Set<string>()
 
   function hasLoop(roleId: string): boolean {
-    if (checking.has(roleId)) {
+    const prefixed = prefixForCheck(roleId)
+    if (checking.has(roleId) || checking.has(prefixed)) {
       return true
     }
-    if (checked.has(roleId)) {
+    if (checked.has(roleId) || checked.has(prefixed)) {
       return false
     }
 
     checking.add(roleId)
 
-    const role = roleMap.get(roleId)
+    const role = roleMap.get(prefixed) || roleMap.get(roleId)
     if (!role) {
       // role not found - ignore
       checking.delete(roleId)
diff --git a/packages/shared-core/src/helpers/tests/roles.spec.ts b/packages/shared-core/src/helpers/tests/roles.spec.ts
index 7e52ded3f6..7b67e4e9e9 100644
--- a/packages/shared-core/src/helpers/tests/roles.spec.ts
+++ b/packages/shared-core/src/helpers/tests/roles.spec.ts
@@ -57,5 +57,12 @@ describe("role utilities", () => {
       ]
       check(true)
     })
+
+    it("should handle new and old inherits structure", () => {
+      const role1 = role("role_role_1", "role_1")
+      role("role_role_2", ["role_1"])
+      role1.inherits = "role_2"
+      check(true)
+    })
   })
 })

From b2e718504c90dda132f1a5ac714bfbaeb7a551b4 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 15:17:36 +0100
Subject: [PATCH 20/28] Adri PR comments.

---
 packages/server/src/api/controllers/role.ts              | 6 ++++--
 packages/server/src/api/routes/tests/permissions.spec.ts | 4 ++--
 packages/server/src/api/routes/tests/screen.spec.ts      | 2 +-
 packages/server/src/tests/utilities/TestConfiguration.ts | 2 +-
 packages/server/src/tests/utilities/api/role.ts          | 4 ----
 5 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 7b9f2d7658..fef68a650c 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -72,9 +72,8 @@ export async function find(ctx: UserCtx<void, FindRoleResponse>) {
   const role = await roles.getRole(ctx.params.roleId)
   if (!role) {
     ctx.throw(404, { message: "Role not found" })
-  } else {
-    ctx.body = externalRole(role)
   }
+  ctx.body = externalRole(role)
 }
 
 export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
@@ -82,6 +81,9 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
   let { _id, name, inherits, permissionId, version, uiMetadata } =
     ctx.request.body
   let isCreate = false
+  if (!ctx.request.body._rev && !version) {
+    version = roles.RoleIDVersion.NAME
+  }
   const isNewVersion = version === roles.RoleIDVersion.NAME
 
   if (_id && roles.isBuiltin(_id)) {
diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts
index 46ce656459..90060eef81 100644
--- a/packages/server/src/api/routes/tests/permissions.spec.ts
+++ b/packages/server/src/api/routes/tests/permissions.spec.ts
@@ -328,7 +328,7 @@ describe("/permission", () => {
     })
 
     it("should be unable to search for table 2 using role 1", async () => {
-      await config.setRole(role1._id!, async () => {
+      await config.loginAsRole(role1._id!, async () => {
         const response2 = await config.api.row.search(
           table2._id!,
           {
@@ -347,7 +347,7 @@ describe("/permission", () => {
         inherits: [role1._id!, role2._id!],
       })
 
-      await config.setRole(role3._id!, async () => {
+      await config.loginAsRole(role3._id!, async () => {
         const response1 = await config.api.row.search(
           table1._id!,
           {
diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts
index f660589951..5853f44bac 100644
--- a/packages/server/src/api/routes/tests/screen.spec.ts
+++ b/packages/server/src/api/routes/tests/screen.spec.ts
@@ -79,7 +79,7 @@ describe("/screens", () => {
     })
 
     async function checkScreens(roleId: string, screenIds: string[]) {
-      await config.setRole(roleId, async () => {
+      await config.loginAsRole(roleId, async () => {
         const res = await config.api.application.getDefinition(
           config.prodAppId!,
           {
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index a3f2fb7adc..fc5a4a7f9c 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -429,7 +429,7 @@ export default class TestConfiguration {
   // HEADERS
 
   // sets the role for the headers, for the period of a callback
-  async setRole(roleId: string, cb: () => Promise<unknown>) {
+  async loginAsRole(roleId: string, cb: () => Promise<unknown>) {
     const roleUser = await this.createUser({
       roles: {
         [this.prodAppId!]: roleId,
diff --git a/packages/server/src/tests/utilities/api/role.ts b/packages/server/src/tests/utilities/api/role.ts
index 05165cd38e..31bffc6f85 100644
--- a/packages/server/src/tests/utilities/api/role.ts
+++ b/packages/server/src/tests/utilities/api/role.ts
@@ -22,10 +22,6 @@ export class RoleAPI extends TestAPI {
   }
 
   save = async (body: SaveRoleRequest, expectations?: Expectations) => {
-    // the tests should always be creating the "new" version of roles
-    if (body.version === undefined) {
-      body.version = "name"
-    }
     return await this._post<SaveRoleResponse>(`/api/roles`, {
       body,
       expectations,

From 3b597f6405e23ed81e997ab89d457048dd542053 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 15:25:34 +0100
Subject: [PATCH 21/28] Fixing test case.

---
 packages/server/src/api/routes/tests/permissions.spec.ts | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts
index 90060eef81..7a9bb2f373 100644
--- a/packages/server/src/api/routes/tests/permissions.spec.ts
+++ b/packages/server/src/api/routes/tests/permissions.spec.ts
@@ -291,6 +291,8 @@ describe("/permission", () => {
   describe("multi-inheritance permissions", () => {
     let table1: Table, table2: Table, role1: Role, role2: Role
     beforeEach(async () => {
+      // create new app
+      await config.init()
       table1 = await config.createTable()
       table2 = await config.createTable()
       await config.api.row.save(table1._id!, {
@@ -301,7 +303,7 @@ describe("/permission", () => {
       })
       role1 = await config.api.roles.save(
         {
-          name: "role1",
+          name: "test_1",
           permissionId: PermissionLevel.WRITE,
           inherits: BUILTIN_ROLE_IDS.BASIC,
         },
@@ -309,7 +311,7 @@ describe("/permission", () => {
       )
       role2 = await config.api.roles.save(
         {
-          name: "role2",
+          name: "test_2",
           permissionId: PermissionLevel.WRITE,
           inherits: BUILTIN_ROLE_IDS.BASIC,
         },

From cfc5848d140d02f3f01de4d52a3a0a17e3c34190 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 16:10:32 +0100
Subject: [PATCH 22/28] Improving how traversal is performed for role
 inheritance.

---
 packages/backend-core/src/security/roles.ts | 109 +++++++++++---------
 1 file changed, 61 insertions(+), 48 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index fdea09944a..44abc64d00 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -10,6 +10,7 @@ import { getAppDB } from "../context"
 import { Screen, Role as RoleDoc, RoleUIMetadata } from "@budibase/types"
 import cloneDeep from "lodash/fp/cloneDeep"
 import { RoleColor, helpers } from "@budibase/shared-core"
+import { uniqBy } from "lodash"
 
 export const BUILTIN_ROLE_IDS = {
   ADMIN: "ADMIN",
@@ -38,6 +39,14 @@ export const RoleIDVersion = {
   NAME: "name",
 }
 
+function rolesInList(roleIds: string[], ids: string | string[]) {
+  if (Array.isArray(ids)) {
+    return ids.filter(id => roleIds.includes(id)).length === ids.length
+  } else {
+    return roleIds.includes(ids)
+  }
+}
+
 export class Role implements RoleDoc {
   _id: string
   _rev?: string
@@ -76,6 +85,54 @@ export class Role implements RoleDoc {
   }
 }
 
+export class RoleHierarchyTraversal {
+  allRoles: RoleDoc[]
+  opts?: { defaultPublic?: boolean }
+
+  constructor(allRoles: RoleDoc[], opts?: { defaultPublic?: boolean }) {
+    this.allRoles = allRoles
+    this.opts = opts
+  }
+
+  walk(role: RoleDoc): RoleDoc[] {
+    const opts = this.opts,
+      allRoles = this.allRoles
+    // this will be a full walked list of roles - which may contain duplicates
+    const roleList: RoleDoc[] = []
+    if (!role || !role._id) {
+      return roleList
+    }
+    roleList.push(role)
+    if (Array.isArray(role.inherits)) {
+      for (let roleId of role.inherits) {
+        const foundRole = findRole(roleId, allRoles, opts)
+        if (foundRole) {
+          return this.walk(foundRole)
+        }
+      }
+    } else {
+      const foundRoleIds: string[] = []
+      let currentRole: RoleDoc | undefined = role
+      while (
+        currentRole &&
+        currentRole.inherits &&
+        !rolesInList(foundRoleIds, currentRole.inherits)
+      ) {
+        if (Array.isArray(currentRole.inherits)) {
+          return this.walk(currentRole)
+        } else {
+          foundRoleIds.push(currentRole.inherits)
+          currentRole = findRole(currentRole.inherits, allRoles, opts)
+          if (role) {
+            roleList.push(role)
+          }
+        }
+      }
+    }
+    return uniqBy(roleList, role => role._id)
+  }
+}
+
 const BUILTIN_ROLES = {
   ADMIN: new Role(
     BUILTIN_IDS.ADMIN,
@@ -309,59 +366,15 @@ async function getAllUserRoles(
   if (userRoleId === BUILTIN_IDS.ADMIN) {
     return allRoles
   }
-  const rolesFound = (ids: string | string[]) => {
-    if (Array.isArray(ids)) {
-      return ids.filter(id => roleIds.includes(id)).length === ids.length
-    } else {
-      return roleIds.includes(ids)
-    }
-  }
-
-  const roleIds = [userRoleId]
-  const roles: RoleDoc[] = []
-  const iterateInherited = (role: RoleDoc | undefined) => {
-    if (!role || !role._id) {
-      return
-    }
-    roleIds.push(role._id)
-    roles.push(role)
-    if (Array.isArray(role.inherits)) {
-      role.inherits.forEach(roleId => {
-        const foundRole = findRole(roleId, allRoles, opts)
-        if (foundRole) {
-          iterateInherited(foundRole)
-        }
-      })
-    } else {
-      while (role && role.inherits && !rolesFound(role.inherits)) {
-        if (Array.isArray(role.inherits)) {
-          iterateInherited(role)
-          break
-        } else {
-          roleIds.push(role.inherits)
-          role = findRole(role.inherits, allRoles, opts)
-          if (role) {
-            roles.push(role)
-          }
-        }
-      }
-    }
-  }
 
   // get all the inherited roles
   const foundRole = findRole(userRoleId, allRoles, opts)
+  let roles: RoleDoc[] = []
   if (foundRole) {
-    iterateInherited(foundRole)
+    const traversal = new RoleHierarchyTraversal(allRoles, opts)
+    roles = traversal.walk(foundRole)
   }
-  const foundRoleIds: string[] = []
-  return roles.filter(role => {
-    if (role._id && !foundRoleIds.includes(role._id)) {
-      foundRoleIds.push(role._id)
-      return true
-    } else {
-      return false
-    }
-  })
+  return roles
 }
 
 export async function getUserRoleIdHierarchy(

From 3da3bccc01b29266241f507ee81de5c90763305a Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 16:27:47 +0100
Subject: [PATCH 23/28] Some fixes for traverser.

---
 packages/backend-core/src/security/roles.ts | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 44abc64d00..b3c18202d2 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -98,7 +98,7 @@ export class RoleHierarchyTraversal {
     const opts = this.opts,
       allRoles = this.allRoles
     // this will be a full walked list of roles - which may contain duplicates
-    const roleList: RoleDoc[] = []
+    let roleList: RoleDoc[] = []
     if (!role || !role._id) {
       return roleList
     }
@@ -107,7 +107,7 @@ export class RoleHierarchyTraversal {
       for (let roleId of role.inherits) {
         const foundRole = findRole(roleId, allRoles, opts)
         if (foundRole) {
-          return this.walk(foundRole)
+          roleList = roleList.concat(this.walk(foundRole))
         }
       }
     } else {
@@ -119,14 +119,18 @@ export class RoleHierarchyTraversal {
         !rolesInList(foundRoleIds, currentRole.inherits)
       ) {
         if (Array.isArray(currentRole.inherits)) {
-          return this.walk(currentRole)
+          return roleList.concat(this.walk(currentRole))
         } else {
           foundRoleIds.push(currentRole.inherits)
           currentRole = findRole(currentRole.inherits, allRoles, opts)
-          if (role) {
-            roleList.push(role)
+          if (currentRole) {
+            roleList.push(currentRole)
           }
         }
+        // loop now found - stop iterating
+        if (helpers.roles.checkForRoleInheritanceLoops(roleList)) {
+          break
+        }
       }
     }
     return uniqBy(roleList, role => role._id)
@@ -359,9 +363,6 @@ async function getAllUserRoles(
   opts?: { defaultPublic?: boolean }
 ): Promise<RoleDoc[]> {
   const allRoles = await getAllRoles()
-  if (helpers.roles.checkForRoleInheritanceLoops(allRoles)) {
-    throw new Error("Loop detected in roles - cannot list roles")
-  }
   // admins have access to all roles
   if (userRoleId === BUILTIN_IDS.ADMIN) {
     return allRoles

From 2ae1836b9ad75464a4f85d3eabb05a66a741ff7c Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 16:58:51 +0100
Subject: [PATCH 24/28] PR comments.

---
 packages/backend-core/src/security/roles.ts      |  8 +++-----
 packages/server/src/api/controllers/role.ts      |  6 +++---
 .../src/tests/utilities/TestConfiguration.ts     | 16 ++++------------
 .../shared-core/src/helpers/tests/roles.spec.ts  |  5 +++++
 4 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index b3c18202d2..366e74bec5 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -76,11 +76,9 @@ export class Role implements RoleDoc {
     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
+      inherits = inherits.map(prefixRoleIDNoBuiltin)
     }
+    this.inherits = inherits
     return this
   }
 }
@@ -264,7 +262,7 @@ export async function roleToNumber(id: string) {
             return findNumber(foundRole) + 1
           }
         })
-        .filter(number => !!number)
+        .filter(number => number)
         .sort()
         .pop()
       if (highestBuiltin != undefined) {
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index fef68a650c..89125f566e 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -78,10 +78,10 @@ export async function find(ctx: UserCtx<void, FindRoleResponse>) {
 
 export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
   const db = context.getAppDB()
-  let { _id, name, inherits, permissionId, version, uiMetadata } =
+  let { _id, _rev, name, inherits, permissionId, version, uiMetadata } =
     ctx.request.body
   let isCreate = false
-  if (!ctx.request.body._rev && !version) {
+  if (!_rev && !version) {
     version = roles.RoleIDVersion.NAME
   }
   const isNewVersion = version === roles.RoleIDVersion.NAME
@@ -132,7 +132,7 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     ctx.throw(400, "Role inheritance contains a loop, this is not supported")
   }
 
-  const foundRev = ctx.request.body._rev || dbRole?._rev
+  const foundRev = _rev || dbRole?._rev
   if (foundRev) {
     role._rev = foundRev
   }
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index fc5a4a7f9c..ca0a6e420a 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -432,7 +432,7 @@ export default class TestConfiguration {
   async loginAsRole(roleId: string, cb: () => Promise<unknown>) {
     const roleUser = await this.createUser({
       roles: {
-        [this.prodAppId!]: roleId,
+        [this.getProdAppId()]: roleId,
       },
       builder: { global: false },
       admin: { global: false },
@@ -443,17 +443,9 @@ export default class TestConfiguration {
       builder: false,
       prodApp: true,
     })
-    const temp = this.user
-    this.user = roleUser
-    await cb()
-    if (temp) {
-      this.user = temp
-      await this.login({
-        userId: temp._id!,
-        builder: true,
-        prodApp: false,
-      })
-    }
+    await this.withUser(roleUser, async () => {
+      await cb()
+    })
   }
 
   defaultHeaders(extras = {}, prodApp = false) {
diff --git a/packages/shared-core/src/helpers/tests/roles.spec.ts b/packages/shared-core/src/helpers/tests/roles.spec.ts
index 7b67e4e9e9..051a196a5b 100644
--- a/packages/shared-core/src/helpers/tests/roles.spec.ts
+++ b/packages/shared-core/src/helpers/tests/roles.spec.ts
@@ -64,5 +64,10 @@ describe("role utilities", () => {
       role1.inherits = "role_2"
       check(true)
     })
+
+    it("self reference contains loop", () => {
+      role("role1", "role1")
+      check(true)
+    })
   })
 })

From bd10a3d831b0e97fe5df2090d9ceed4e127ff76e Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 17:00:40 +0100
Subject: [PATCH 25/28] Missed comment.

---
 packages/backend-core/src/security/roles.ts | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 366e74bec5..56037f7403 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -273,12 +273,7 @@ export async function roleToNumber(id: string) {
     }
     return 0
   }
-  let highest = 0
-  for (let role of hierarchy) {
-    const roleNumber = findNumber(role)
-    highest = Math.max(roleNumber, highest)
-  }
-  return highest
+  return Math.max(...hierarchy.map(findNumber))
 }
 
 /**

From c74577c512fd3b6b063e54f7a52dd202dc92c412 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 17:15:41 +0100
Subject: [PATCH 26/28] Updating accessible utility API.

---
 .../server/src/api/routes/tests/role.spec.ts  | 50 +++++++++++--------
 .../src/tests/utilities/TestConfiguration.ts  | 23 ++++++++-
 .../server/src/tests/utilities/api/role.ts    |  6 +--
 3 files changed, 52 insertions(+), 27 deletions(-)

diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index 134078e6bf..a79f219311 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -228,29 +228,35 @@ describe("/roles", () => {
     })
 
     it("should be able to fetch accessible roles (with builder)", async () => {
-      const res = await config.api.roles.accessible(config.defaultHeaders(), {
-        status: 200,
+      await config.withHeaders(config.defaultHeaders(), async () => {
+        const res = await config.api.roles.accessible({
+          status: 200,
+        })
+        expect(res.length).toBe(5)
+        expect(typeof res[0]).toBe("string")
       })
-      expect(res.length).toBe(5)
-      expect(typeof res[0]).toBe("string")
     })
 
     it("should be able to fetch accessible roles (basic user)", async () => {
       const headers = await config.basicRoleHeaders()
-      const res = await config.api.roles.accessible(headers, {
-        status: 200,
+      await config.withHeaders(headers, async () => {
+        const res = await config.api.roles.accessible({
+          status: 200,
+        })
+        expect(res.length).toBe(2)
+        expect(res[0]).toBe("BASIC")
+        expect(res[1]).toBe("PUBLIC")
       })
-      expect(res.length).toBe(2)
-      expect(res[0]).toBe("BASIC")
-      expect(res[1]).toBe("PUBLIC")
     })
 
     it("should be able to fetch accessible roles (no user)", async () => {
-      const res = await config.api.roles.accessible(config.publicHeaders(), {
-        status: 200,
+      await config.withHeaders(config.publicHeaders(), async () => {
+        const res = await config.api.roles.accessible({
+          status: 200,
+        })
+        expect(res.length).toBe(1)
+        expect(res[0]).toBe("PUBLIC")
       })
-      expect(res.length).toBe(1)
-      expect(res[0]).toBe("PUBLIC")
     })
 
     it("should not fetch higher level accessible roles when a custom role header is provided", async () => {
@@ -261,13 +267,15 @@ describe("/roles", () => {
         permissionId: permissions.BuiltinPermissionID.READ_ONLY,
         version: "name",
       })
-      const res = await config.api.roles.accessible(
+      await config.withHeaders(
         { "x-budibase-role": customRoleName },
-        {
-          status: 200,
+        async () => {
+          const res = await config.api.roles.accessible({
+            status: 200,
+          })
+          expect(res).toEqual([customRoleName, "BASIC", "PUBLIC"])
         }
       )
-      expect(res).toEqual([customRoleName, "BASIC", "PUBLIC"])
     })
   })
 
@@ -297,10 +305,12 @@ describe("/roles", () => {
       const headers = await config.roleHeaders({
         roleId: role3,
       })
-      const res = await config.api.roles.accessible(headers, {
-        status: 200,
+      await config.withHeaders(headers, async () => {
+        const res = await config.api.roles.accessible({
+          status: 200,
+        })
+        expect(res).toEqual([role3, role1, "BASIC", "PUBLIC", role2, "POWER"])
       })
-      expect(res).toEqual([role3, role1, "BASIC", "PUBLIC", role2, "POWER"])
     })
   })
 })
diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts
index ca0a6e420a..713f8b31de 100644
--- a/packages/server/src/tests/utilities/TestConfiguration.ts
+++ b/packages/server/src/tests/utilities/TestConfiguration.ts
@@ -110,6 +110,7 @@ export default class TestConfiguration {
   tenantId?: string
   api: API
   csrfToken?: string
+  temporaryHeaders?: Record<string, string | string[]>
 
   constructor(openServer = true) {
     if (openServer) {
@@ -448,6 +449,18 @@ export default class TestConfiguration {
     })
   }
 
+  async withHeaders(
+    headers: Record<string, string | string[]>,
+    cb: () => Promise<unknown>
+  ) {
+    this.temporaryHeaders = headers
+    try {
+      await cb()
+    } finally {
+      this.temporaryHeaders = undefined
+    }
+  }
+
   defaultHeaders(extras = {}, prodApp = false) {
     const tenantId = this.getTenantId()
     const user = this.getUser()
@@ -471,7 +484,10 @@ export default class TestConfiguration {
     } else if (this.appId) {
       headers[constants.Header.APP_ID] = this.appId
     }
-    return headers
+    return {
+      ...headers,
+      ...this.temporaryHeaders,
+    }
   }
 
   publicHeaders({ prodApp = true } = {}) {
@@ -487,7 +503,10 @@ export default class TestConfiguration {
 
     headers[constants.Header.TENANT_ID] = this.getTenantId()
 
-    return headers
+    return {
+      ...headers,
+      ...this.temporaryHeaders,
+    }
   }
 
   async basicRoleHeaders() {
diff --git a/packages/server/src/tests/utilities/api/role.ts b/packages/server/src/tests/utilities/api/role.ts
index 31bffc6f85..5bd0647384 100644
--- a/packages/server/src/tests/utilities/api/role.ts
+++ b/packages/server/src/tests/utilities/api/role.ts
@@ -34,12 +34,8 @@ export class RoleAPI extends TestAPI {
     })
   }
 
-  accessible = async (
-    headers: Record<string, string | string[]>,
-    expectations?: Expectations
-  ) => {
+  accessible = async (expectations?: Expectations) => {
     return await this._get<AccessibleRolesResponse>(`/api/roles/accessible`, {
-      headers,
       expectations,
     })
   }

From 47de88f42fd08108c6d9eb08045c1710c8347151 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 17:47:55 +0100
Subject: [PATCH 27/28] Destroy test case.

---
 packages/backend-core/src/security/roles.ts   | 26 ++++++++++++-
 packages/server/src/api/controllers/role.ts   | 38 +++++++++++++------
 .../server/src/api/routes/tests/role.spec.ts  | 21 ++++++++++
 3 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index e549d2f3af..c14178cacb 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -290,11 +290,23 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string {
     : roleId1
 }
 
-function compareRoleIds(roleId1: string, roleId2: string) {
+export function compareRoleIds(roleId1: string, roleId2: string) {
   // make sure both role IDs are prefixed correctly
   return prefixRoleID(roleId1) === prefixRoleID(roleId2)
 }
 
+export function externalRole(role: RoleDoc): RoleDoc {
+  let _id: string | undefined
+  if (role._id) {
+    _id = getExternalRoleID(role._id)
+  }
+  return {
+    ...role,
+    _id,
+    inherits: getExternalRoleIDs(role.inherits, role.version),
+  }
+}
+
 /**
  * Given a list of roles, this will pick the role out, accounting for built ins.
  */
@@ -347,6 +359,18 @@ export async function getRole(
   return findRole(roleId, roleList, opts)
 }
 
+export async function saveRoles(roles: RoleDoc[]) {
+  const db = getAppDB()
+  await db.bulkDocs(
+    roles
+      .filter(role => role._id)
+      .map(role => ({
+        ...role,
+        _id: prefixRoleID(role._id!),
+      }))
+  )
+}
+
 /**
  * Simple function to get all the roles based on the top level user role ID.
  */
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 89125f566e..f26b4bae69 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -27,15 +27,27 @@ const UpdateRolesOptions = {
   REMOVED: "removed",
 }
 
-function externalRole(role: Role): Role {
-  let _id: string | undefined
-  if (role._id) {
-    _id = roles.getExternalRoleID(role._id)
+async function removeRoleFromOthers(roleId: string) {
+  const allOtherRoles = await roles.getAllRoles()
+  const updated: Role[] = []
+  for (let role of allOtherRoles) {
+    let changed = false
+    if (Array.isArray(role.inherits)) {
+      const newInherits = role.inherits.filter(
+        id => !roles.compareRoleIds(id, roleId)
+      )
+      changed = role.inherits.length !== newInherits.length
+      role.inherits = newInherits
+    } else if (role.inherits && roles.compareRoleIds(role.inherits, roleId)) {
+      role.inherits = roles.BUILTIN_ROLE_IDS.PUBLIC
+      changed = true
+    }
+    if (changed) {
+      updated.push(role)
+    }
   }
-  return {
-    ...role,
-    _id,
-    inherits: roles.getExternalRoleIDs(role.inherits, role.version),
+  if (updated.length) {
+    await roles.saveRoles(updated)
   }
 }
 
@@ -65,7 +77,7 @@ async function updateRolesOnUserTable(
 }
 
 export async function fetch(ctx: UserCtx<void, FetchRolesResponse>) {
-  ctx.body = (await roles.getAllRoles()).map(role => externalRole(role))
+  ctx.body = (await roles.getAllRoles()).map(role => roles.externalRole(role))
 }
 
 export async function find(ctx: UserCtx<void, FindRoleResponse>) {
@@ -73,7 +85,7 @@ export async function find(ctx: UserCtx<void, FindRoleResponse>) {
   if (!role) {
     ctx.throw(404, { message: "Role not found" })
   }
-  ctx.body = externalRole(role)
+  ctx.body = roles.externalRole(role)
 }
 
 export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
@@ -149,7 +161,7 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
     role.version
   )
   role._rev = result.rev
-  ctx.body = externalRole(role)
+  ctx.body = roles.externalRole(role)
 
   const devDb = context.getDevAppDB()
   const prodDb = context.getProdAppDB()
@@ -198,6 +210,10 @@ export async function destroy(ctx: UserCtx<void, DestroyRoleResponse>) {
     UpdateRolesOptions.REMOVED,
     role.version
   )
+
+  // clean up inherits
+  await removeRoleFromOthers(roleId)
+
   ctx.message = `Role ${ctx.params.roleId} deleted successfully`
   ctx.status = 200
 }
diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts
index a79f219311..5668295342 100644
--- a/packages/server/src/api/routes/tests/role.spec.ts
+++ b/packages/server/src/api/routes/tests/role.spec.ts
@@ -217,6 +217,27 @@ describe("/roles", () => {
         _id: dbCore.prefixRoleID(customRole._id!),
       })
     })
+
+    it("should disconnection roles when deleted", async () => {
+      const role1 = await config.api.roles.save({
+        name: "role1",
+        permissionId: BuiltinPermissionID.WRITE,
+        inherits: [BUILTIN_ROLE_IDS.BASIC],
+      })
+      const role2 = await config.api.roles.save({
+        name: "role2",
+        permissionId: BuiltinPermissionID.WRITE,
+        inherits: [BUILTIN_ROLE_IDS.BASIC, role1._id!],
+      })
+      const role3 = await config.api.roles.save({
+        name: "role3",
+        permissionId: BuiltinPermissionID.WRITE,
+        inherits: [BUILTIN_ROLE_IDS.BASIC, role2._id!],
+      })
+      await config.api.roles.destroy(role2, { status: 200 })
+      const found = await config.api.roles.find(role3._id!, { status: 200 })
+      expect(found.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC])
+    })
   })
 
   describe("accessible", () => {

From d0df75c9e421b6f9caa981f1234b1ded1affb768 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 17 Oct 2024 17:50:51 +0100
Subject: [PATCH 28/28] Adding back mock clearing.

---
 packages/server/src/api/routes/tests/screen.spec.ts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts
index 5853f44bac..5dfe3d2a44 100644
--- a/packages/server/src/api/routes/tests/screen.spec.ts
+++ b/packages/server/src/api/routes/tests/screen.spec.ts
@@ -110,6 +110,10 @@ describe("/screens", () => {
   })
 
   describe("save", () => {
+    beforeEach(() => {
+      jest.clearAllMocks()
+    })
+
     it("should be able to create a screen", async () => {
       const screen = basicScreen()
       const responseScreen = await config.api.screen.save(screen, {
@@ -127,6 +131,7 @@ describe("/screens", () => {
       screen._id = responseScreen._id
       screen._rev = responseScreen._rev
       screen.name = "edit"
+      jest.clearAllMocks()
 
       responseScreen = await config.api.screen.save(screen, { status: 200 })