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] 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)