From 00fe28e5576684930485221dde872b9fa1ff4a8d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 13:05:22 +0100 Subject: [PATCH 01/10] Simplifying retrieval of role hierarchy for accessible endpoint. --- packages/server/src/api/controllers/role.ts | 27 +++++---------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index d09fcf28f6..2d8556b581 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -226,35 +226,20 @@ export async function accessible(ctx: UserCtx) { if (!roleId) { roleId = roles.BUILTIN_ROLE_IDS.PUBLIC } + // If a custom role is provided in the header, filter out higher level roles + const roleHeader = ctx.header?.[Header.PREVIEW_ROLE] as string + const isBuilder = ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user) let roleIds: string[] = [] - if (ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user)) { + if (!roleHeader && isBuilder) { const appId = context.getAppId() if (appId) { roleIds = await roles.getAllRoleIds(appId) } + } else if (isBuilder && roleHeader) { + roleIds = await roles.getUserRoleIdHierarchy(roleHeader) } else { roleIds = await roles.getUserRoleIdHierarchy(roleId!) } - // 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 role = await roles.getRole(roleHeader) - const inherits = role?.inherits - const orderedRoles = roleIds.reverse() - let filteredRoles = [roleHeader] - for (let role of orderedRoles) { - filteredRoles = [role, ...filteredRoles] - if ( - (Array.isArray(inherits) && inherits.includes(role)) || - role === inherits - ) { - break - } - } - filteredRoles.pop() - roleIds = [roleHeader, ...filteredRoles] - } - ctx.body = roleIds.map(roleId => roles.getExternalRoleID(roleId)) } From f3d54f1b7d6e6d92956e2533efaa05cb7889570a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 13:54:54 +0100 Subject: [PATCH 02/10] Adding test cases. --- packages/backend-core/src/security/roles.ts | 16 +++++++++++++ packages/server/src/api/controllers/role.ts | 13 +++++++++++ .../server/src/api/routes/tests/role.spec.ts | 23 +++++++++++++++++++ 3 files changed, 52 insertions(+) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index c14178cacb..a301abea8b 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -213,6 +213,22 @@ export function getBuiltinRole(roleId: string): Role | undefined { return cloneDeep(role) } +export function validInherits( + allRoles: RoleDoc[], + inherits?: string | string[] +): boolean { + if (!inherits) { + return false + } + const find = (id: string) => allRoles.find(r => compareRoleIds(r._id!, id)) + if (Array.isArray(inherits)) { + const filtered = inherits.filter(roleId => find(roleId)) + return inherits.length !== 0 && filtered.length === inherits.length + } else { + return !!find(inherits) + } +} + /** * Works through the inheritance ranks to see how far up the builtin stack this ID is. */ diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 2d8556b581..fca0d287eb 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -18,10 +18,12 @@ import { UserCtx, UserMetadata, DocumentType, + PermissionLevel, } from "@budibase/types" import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core" import sdk from "../../sdk" import { builderSocket } from "../../websockets" +import { validInherits } from "@budibase/backend-core/src/security/roles" const UpdateRolesOptions = { CREATED: "created", @@ -125,6 +127,17 @@ export async function save(ctx: UserCtx) { ctx.throw(400, "Cannot change custom role name") } + // custom roles should always inherit basic - if they don't inherit anything else + if (!inherits && roles.validInherits(allRoles, dbRole?.inherits)) { + inherits = dbRole?.inherits + } else if (!roles.validInherits(allRoles, inherits)) { + inherits = [roles.BUILTIN_ROLE_IDS.BASIC] + } + // assume write permission level for newly created roles + if (isCreate && !permissionId) { + permissionId = PermissionLevel.WRITE + } + const role = new roles.Role(_id, name, permissionId, { displayName: uiMetadata?.displayName || name, description: uiMetadata?.description || "Custom role", diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 5668295342..2c9ce32937 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -38,6 +38,16 @@ describe("/roles", () => { _id: dbCore.prefixRoleID(res._id!), }) }) + + it("handle a role with no inherits", async () => { + const role = basicRole() + role.inherits = [] + + const res = await config.api.roles.save(role, { + status: 200, + }) + expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC]) + }) }) describe("update", () => { @@ -149,6 +159,19 @@ describe("/roles", () => { { status: 400, body: { message: LOOP_ERROR } } ) }) + + it("handle updating a role, without its inherits", async () => { + const res = await config.api.roles.save({ + ...basicRole(), + inherits: [BUILTIN_ROLE_IDS.ADMIN], + }) + // save again + const updatedRes = await config.api.roles.save({ + ...res, + inherits: undefined, + }) + expect(updatedRes.inherits).toEqual([BUILTIN_ROLE_IDS.ADMIN]) + }) }) describe("fetch", () => { From cf3ef6473471c44a07ec0b6d2a709c3f3b0648e6 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 15:03:13 +0100 Subject: [PATCH 03/10] Clean up role header implementation in currentapp.ts --- packages/server/src/middleware/currentapp.ts | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/server/src/middleware/currentapp.ts b/packages/server/src/middleware/currentapp.ts index d616377298..a8ef8bb251 100644 --- a/packages/server/src/middleware/currentapp.ts +++ b/packages/server/src/middleware/currentapp.ts @@ -56,22 +56,9 @@ export default async (ctx: UserCtx, next: any) => { ctx.request && (ctx.request.headers[constants.Header.PREVIEW_ROLE] as string) if (isBuilder && isDevApp && roleHeader) { - // Ensure the role is valid by ensuring a definition exists - try { - if (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 - } - } - } catch (error) { - // Swallow error and do nothing - } + roleId = roleHeader + // Delete admin and builder flags so that the specified role is honoured + ctx.user = users.removePortalUserPermissions(ctx.user) as ContextUser } } From ebc440a0b67dd5c8f6c38b10d3de6ec81f8af1f1 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 15:03:47 +0100 Subject: [PATCH 04/10] Another test case. --- .../server/src/api/routes/tests/role.spec.ts | 17 ++++++++++++++ .../server/src/tests/utilities/structures.ts | 23 +++++++++---------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 2c9ce32937..e1e5231bef 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -321,6 +321,23 @@ describe("/roles", () => { } ) }) + + it("should fetch preview role correctly even without basic specified", async () => { + const role = await config.api.roles.save(basicRole()) + // have to forcefully delete the inherits from DB - technically can't + // happen anymore - but good test case + await dbCore.getDB(config.appId!).put({ + ...role, + _id: dbCore.prefixRoleID(role._id!), + inherits: [], + }) + await config.withHeaders({ "x-budibase-role": role.name }, async () => { + const res = await config.api.roles.accessible({ + status: 200, + }) + expect(res).toEqual([role.name]) + }) + }) }) describe("accessible - multi-inheritance", () => { diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts index b38cc6484f..b63d6d1a78 100644 --- a/packages/server/src/tests/utilities/structures.ts +++ b/packages/server/src/tests/utilities/structures.ts @@ -8,31 +8,31 @@ import { } from "../../automations" import { AIOperationEnum, + AutoFieldSubType, Automation, AutomationActionStepId, + AutomationEventType, AutomationResults, AutomationStatus, AutomationStep, AutomationStepType, AutomationTrigger, AutomationTriggerStepId, + BBReferenceFieldSubType, + CreateViewRequest, Datasource, + FieldSchema, FieldType, + INTERNAL_TABLE_SOURCE_ID, + JsonFieldSubType, + LoopStepType, + Query, + Role, SourceName, Table, - INTERNAL_TABLE_SOURCE_ID, TableSourceType, - Query, Webhook, WebhookActionType, - AutomationEventType, - LoopStepType, - FieldSchema, - BBReferenceFieldSubType, - JsonFieldSubType, - AutoFieldSubType, - Role, - CreateViewRequest, } from "@budibase/types" import { LoopInput } from "../../definitions/automations" import { merge } from "lodash" @@ -439,7 +439,7 @@ export function updateRowAutomationWithFilters( appId: string, tableId: string ): Automation { - const automation: Automation = { + return { name: "updateRowWithFilters", type: "automation", appId, @@ -472,7 +472,6 @@ export function updateRowAutomationWithFilters( }, }, } - return automation } export function basicAutomationResults( From fe35ef74e72064930c5e348cf2dd290bfd8ca4dd Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 15:06:50 +0100 Subject: [PATCH 05/10] final test case. --- .../src/api/routes/tests/screen.spec.ts | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts index 5dfe3d2a44..894710ca27 100644 --- a/packages/server/src/api/routes/tests/screen.spec.ts +++ b/packages/server/src/api/routes/tests/screen.spec.ts @@ -86,7 +86,6 @@ describe("/screens", () => { status: 200, } ) - // basic and role1 screen expect(res.screens.length).toEqual(screenIds.length) expect(res.screens.map(s => s._id).sort()).toEqual(screenIds.sort()) }) @@ -107,6 +106,25 @@ describe("/screens", () => { screen2._id!, ]) }) + + it("should be able to fetch basic and screen 1 with role1 in role header", async () => { + await config.withHeaders( + { + "x-budibase-role": role1._id!, + }, + async () => { + const res = await config.api.application.getDefinition( + config.prodAppId!, + { + status: 200, + } + ) + const screenIds = [screen._id!, screen1._id!] + expect(res.screens.length).toEqual(screenIds.length) + expect(res.screens.map(s => s._id).sort()).toEqual(screenIds.sort()) + } + ) + }) }) describe("save", () => { From eb349b5fb309662da4fe560a6f57666ecb1b79f9 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 15:15:18 +0100 Subject: [PATCH 06/10] Linting. --- packages/server/src/api/controllers/role.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index fca0d287eb..728038e3a3 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -23,7 +23,6 @@ import { import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core" import sdk from "../../sdk" import { builderSocket } from "../../websockets" -import { validInherits } from "@budibase/backend-core/src/security/roles" const UpdateRolesOptions = { CREATED: "created", From 74870663e26b4316ec1d84036826e741cd3bbfbc Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 16:02:20 +0100 Subject: [PATCH 07/10] Addressing PR comments. --- packages/backend-core/src/security/roles.ts | 8 ++++---- packages/server/src/api/controllers/role.ts | 10 +++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index a301abea8b..7d9a11db48 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -220,7 +220,7 @@ export function validInherits( if (!inherits) { return false } - const find = (id: string) => allRoles.find(r => compareRoleIds(r._id!, id)) + const find = (id: string) => allRoles.find(r => roleIDsAreEqual(r._id!, id)) if (Array.isArray(inherits)) { const filtered = inherits.filter(roleId => find(roleId)) return inherits.length !== 0 && filtered.length === inherits.length @@ -306,7 +306,7 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string { : roleId1 } -export function compareRoleIds(roleId1: string, roleId2: string) { +export function roleIDsAreEqual(roleId1: string, roleId2: string) { // make sure both role IDs are prefixed correctly return prefixRoleID(roleId1) === prefixRoleID(roleId2) } @@ -339,7 +339,7 @@ export function findRole( roleId = prefixRoleID(roleId) } const dbRole = roles.find( - role => role._id && compareRoleIds(role._id, roleId) + role => role._id && roleIDsAreEqual(role._id, roleId) ) if (!dbRole && !isBuiltin(roleId) && opts?.defaultPublic) { return cloneDeep(BUILTIN_ROLES.PUBLIC) @@ -573,7 +573,7 @@ export class AccessController { } return ( - roleIds?.find(roleId => compareRoleIds(roleId, tryingRoleId)) !== + roleIds?.find(roleId => roleIDsAreEqual(roleId, tryingRoleId)) !== undefined ) } diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 728038e3a3..eb452b9997 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -23,6 +23,7 @@ import { import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core" import sdk from "../../sdk" import { builderSocket } from "../../websockets" +import { roleIDsAreEqual } from "@budibase/backend-core/src/security/roles" const UpdateRolesOptions = { CREATED: "created", @@ -36,11 +37,11 @@ async function removeRoleFromOthers(roleId: string) { let changed = false if (Array.isArray(role.inherits)) { const newInherits = role.inherits.filter( - id => !roles.compareRoleIds(id, roleId) + id => !roles.roleIDsAreEqual(id, roleId) ) changed = role.inherits.length !== newInherits.length role.inherits = newInherits - } else if (role.inherits && roles.compareRoleIds(role.inherits, roleId)) { + } else if (role.inherits && roles.roleIDsAreEqual(role.inherits, roleId)) { role.inherits = roles.BUILTIN_ROLE_IDS.PUBLIC changed = true } @@ -239,7 +240,10 @@ export async function accessible(ctx: UserCtx) { roleId = roles.BUILTIN_ROLE_IDS.PUBLIC } // If a custom role is provided in the header, filter out higher level roles - const roleHeader = ctx.header?.[Header.PREVIEW_ROLE] as string + const roleHeader = ctx.header[Header.PREVIEW_ROLE] + if (Array.isArray(roleHeader)) { + ctx.throw(400, `Too many roles specified in ${Header.PREVIEW_ROLE} header`) + } const isBuilder = ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user) let roleIds: string[] = [] if (!roleHeader && isBuilder) { From 99d867fe603cf37cbe3f901d6ce7b8f27f78fa8a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 16:04:18 +0100 Subject: [PATCH 08/10] Updating test case. --- packages/server/src/api/routes/tests/role.spec.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index e1e5231bef..07b88733af 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -165,11 +165,9 @@ describe("/roles", () => { ...basicRole(), inherits: [BUILTIN_ROLE_IDS.ADMIN], }) - // save again - const updatedRes = await config.api.roles.save({ - ...res, - inherits: undefined, - }) + // remove the roles so that it will default back to DB roles, then save again + delete res.inherits + const updatedRes = await config.api.roles.save(res) expect(updatedRes.inherits).toEqual([BUILTIN_ROLE_IDS.ADMIN]) }) }) From f91e0394028d67875e6907fe5703ff4c35a6c5b6 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 16:06:26 +0100 Subject: [PATCH 09/10] Adding test with invalid role IDs.# --- packages/server/src/api/routes/tests/role.spec.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 07b88733af..adb83ca793 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -39,6 +39,16 @@ describe("/roles", () => { }) }) + it("handle a role with invalid inherits", async () => { + const role = basicRole() + role.inherits = ["not_real", "some_other_not_real"] + + const res = await config.api.roles.save(role, { + status: 200, + }) + expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC]) + }) + it("handle a role with no inherits", async () => { const role = basicRole() role.inherits = [] From 0b2ac578c05e9dada12297cb2378b7ec9e9484c4 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 22 Oct 2024 16:27:53 +0100 Subject: [PATCH 10/10] linting. --- packages/server/src/api/controllers/role.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index eb452b9997..1047711983 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -23,7 +23,6 @@ import { import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core" import sdk from "../../sdk" import { builderSocket } from "../../websockets" -import { roleIDsAreEqual } from "@budibase/backend-core/src/security/roles" const UpdateRolesOptions = { CREATED: "created",