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/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 4531f40748..d8c9661c72 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -82,7 +82,10 @@ export async function save(ctx: UserCtx) { _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", () => { 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..f660589951 --- /dev/null +++ b/packages/server/src/api/routes/tests/screen.spec.ts @@ -0,0 +1,166 @@ +import { checkBuilderEndpoint } from "./utilities/TestFunctions" +import * as setup from "./utilities" +import { events, roles } from "@budibase/backend-core" +import { Screen, PermissionLevel, Role } 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("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 fetch basic and screen1 with role1", async () => { + await checkScreens(role1._id!, [screen._id!, screen1._id!]) + }) + + 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 basic, screen1 and screen2 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 config.api.screen.save(screen, { + status: 200, + }) + + 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 config.api.screen.save(screen, { status: 200 }) + screen._id = responseScreen._id + screen._rev = responseScreen._rev + screen.name = "edit" + + responseScreen = await config.api.screen.save(screen, { status: 200 }) + + 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 => { return await this._get(`/api/screens`, { expectations }) } + + save = async ( + screen: Screen, + expectations?: Expectations + ): Promise => { + return await this._post(`/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, + } + ) + } }