From 9c92288f7f94932698ff09e31de2012ee6f5d459 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 16 Oct 2024 15:18:37 +0100 Subject: [PATCH 1/3] 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 { +): Promise { 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) { } export async function find(ctx: UserCtx) { - 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) { @@ -202,7 +207,8 @@ export async function accessible(ctx: UserCtx) { // 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 Date: Wed, 16 Oct 2024 15:46:03 +0100 Subject: [PATCH 2/3] 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) { 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 Date: Wed, 16 Oct 2024 17:26:48 +0100 Subject: [PATCH 3/3] 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) { - ctx.body = await roles.getAllRoles() + ctx.body = (await roles.getAllRoles()).map(role => externalRole(role)) } export async function find(ctx: UserCtx) { @@ -61,7 +73,7 @@ export async function find(ctx: UserCtx) { 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) { 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) { 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) { 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) { } } 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 {