diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 3f26368efe..c14178cacb 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -17,6 +17,7 @@ import { } 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", @@ -37,6 +38,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 @@ -66,15 +75,65 @@ 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 } } +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 + let 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) { + roleList = roleList.concat(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 roleList.concat(this.walk(currentRole)) + } else { + foundRoleIds.push(currentRole.inherits) + currentRole = findRole(currentRole.inherits, allRoles, opts) + if (currentRole) { + roleList.push(currentRole) + } + } + // loop now found - stop iterating + if (helpers.roles.checkForRoleInheritanceLoops(roleList)) { + break + } + } + } + return uniqBy(roleList, role => role._id) + } +} + const BUILTIN_ROLES = { ADMIN: new Role( BUILTIN_IDS.ADMIN, @@ -202,7 +261,7 @@ export async function roleToNumber(id: string) { return findNumber(foundRole) + 1 } }) - .filter(number => !!number) + .filter(number => number) .sort() .pop() if (highestBuiltin != undefined) { @@ -213,12 +272,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)) } /** @@ -236,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. */ @@ -293,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. */ @@ -301,66 +379,19 @@ async function getAllUserRoles( opts?: { defaultPublic?: boolean } ): Promise { 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 } - 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( diff --git a/packages/builder/src/pages/builder/portal/apps/index.svelte b/packages/builder/src/pages/builder/portal/apps/index.svelte index ddd37dc4a3..c3fcfd65ff 100644 --- a/packages/builder/src/pages/builder/portal/apps/index.svelte +++ b/packages/builder/src/pages/builder/portal/apps/index.svelte @@ -76,9 +76,7 @@ const params = new URLSearchParams({ open: "error", }) - $goto( - `/builder/app/${appId}/settings/automation-history?${params.toString()}` - ) + $goto(`/builder/app/${appId}/settings/automations?${params.toString()}`) } const errorCount = errors => { diff --git a/packages/frontend-core/src/components/grid/cells/NumberCell.svelte b/packages/frontend-core/src/components/grid/cells/NumberCell.svelte index 2acec7d11e..c8ae96ef21 100644 --- a/packages/frontend-core/src/components/grid/cells/NumberCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/NumberCell.svelte @@ -1,3 +1,7 @@ + + - + diff --git a/packages/frontend-core/src/components/grid/cells/TextCell.svelte b/packages/frontend-core/src/components/grid/cells/TextCell.svelte index 0cf0ab2004..9275bca3c6 100644 --- a/packages/frontend-core/src/components/grid/cells/TextCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/TextCell.svelte @@ -7,11 +7,13 @@ export let type = "text" export let readonly = false export let api + export let format = null let input let active = false $: editable = focused && !readonly + $: displayValue = format?.(value) ?? value ?? "" const handleChange = e => { onChange(e.target.value) @@ -52,7 +54,7 @@ {:else}
- {value ?? ""} + {displayValue}
{/if} diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 34455d2c52..d09fcf28f6 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -28,15 +28,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) } } @@ -66,23 +78,25 @@ async function updateRolesOnUserTable( } export async function fetch(ctx: UserCtx) { - 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) { const role = await roles.getRole(ctx.params.roleId) if (!role) { ctx.throw(404, { message: "Role not found" }) - } else { - ctx.body = externalRole(role) } + ctx.body = roles.externalRole(role) } export async function save(ctx: UserCtx) { 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 (!_rev && !version) { + version = roles.RoleIDVersion.NAME + } const isNewVersion = version === roles.RoleIDVersion.NAME if (_id && roles.isBuiltin(_id)) { @@ -131,7 +145,7 @@ export async function save(ctx: UserCtx) { 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 } @@ -148,7 +162,7 @@ export async function save(ctx: UserCtx) { 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 +212,10 @@ export async function destroy(ctx: UserCtx) { UpdateRolesOptions.REMOVED, role.version ) + + // clean up inherits + await removeRoleFromOthers(roleId) + ctx.message = `Role ${ctx.params.roleId} deleted successfully` ctx.status = 200 builderSocket?.emitRoleDeletion(ctx, role) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 46ce656459..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, }, @@ -328,7 +330,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 +349,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/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 6ed5dfd30f..5668295342 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", () => { @@ -189,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", () => { @@ -200,29 +249,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 () => { @@ -233,13 +288,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"]) }) }) @@ -269,10 +326,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/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts index f660589951..5dfe3d2a44 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!, { @@ -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 }) diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index a3f2fb7adc..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 constructor(openServer = true) { if (openServer) { @@ -429,10 +430,10 @@ export default class TestConfiguration { // HEADERS // sets the role for the headers, for the period of a callback - async setRole(roleId: string, cb: () => Promise) { + async loginAsRole(roleId: string, cb: () => Promise) { const roleUser = await this.createUser({ roles: { - [this.prodAppId!]: roleId, + [this.getProdAppId()]: roleId, }, builder: { global: false }, admin: { global: false }, @@ -443,16 +444,20 @@ 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() + }) + } + + async withHeaders( + headers: Record, + cb: () => Promise + ) { + this.temporaryHeaders = headers + try { + await cb() + } finally { + this.temporaryHeaders = undefined } } @@ -479,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 } = {}) { @@ -495,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 05165cd38e..5bd0647384 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(`/api/roles`, { body, expectations, @@ -38,12 +34,8 @@ export class RoleAPI extends TestAPI { }) } - accessible = async ( - headers: Record, - expectations?: Expectations - ) => { + accessible = async (expectations?: Expectations) => { return await this._get(`/api/roles/accessible`, { - headers, expectations, }) } diff --git a/packages/shared-core/src/helpers/roles.ts b/packages/shared-core/src/helpers/roles.ts index 27f28cff82..8dee928edd 100644 --- a/packages/shared-core/src/helpers/roles.ts +++ b/packages/shared-core/src/helpers/roles.ts @@ -1,4 +1,11 @@ -import { Role } 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() 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..051a196a5b 100644 --- a/packages/shared-core/src/helpers/tests/roles.spec.ts +++ b/packages/shared-core/src/helpers/tests/roles.spec.ts @@ -57,5 +57,17 @@ 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) + }) + + it("self reference contains loop", () => { + role("role1", "role1") + check(true) + }) }) }) 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 version?: string uiMetadata?: RoleUIMetadata }