From 1f9c33c53a6bb12a8b50ab7db5d862f6ea33226d Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 16 Oct 2024 21:29:33 +0100 Subject: [PATCH 01/15] Test cases based on frontend. --- .../server/src/api/routes/tests/role.spec.ts | 28 +++++++++++++++++++ packages/types/src/api/web/role.ts | 2 ++ 2 files changed, 30 insertions(+) diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 6ed5dfd30f..134078e6bf 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", () => { 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 } From 97b1c52f70c318323f74ad481326196770f30f04 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 16 Oct 2024 22:08:07 +0100 Subject: [PATCH 02/15] Protect against old roles with differences when it comes to loops. --- packages/shared-core/src/helpers/roles.ts | 16 ++++++++++++---- .../shared-core/src/helpers/tests/roles.spec.ts | 7 +++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/packages/shared-core/src/helpers/roles.ts b/packages/shared-core/src/helpers/roles.ts index 24b65501ce..8dee928edd 100644 --- a/packages/shared-core/src/helpers/roles.ts +++ b/packages/shared-core/src/helpers/roles.ts @@ -1,4 +1,11 @@ -import { Role, SEPARATOR, DocumentType } 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..7b67e4e9e9 100644 --- a/packages/shared-core/src/helpers/tests/roles.spec.ts +++ b/packages/shared-core/src/helpers/tests/roles.spec.ts @@ -57,5 +57,12 @@ 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) + }) }) }) From cf3f5b97c6f7c7312d5e6c46ae120fd8892ee823 Mon Sep 17 00:00:00 2001 From: saewoohan <112225610+saewoohan@users.noreply.github.com> Date: Thu, 17 Oct 2024 23:14:47 +0900 Subject: [PATCH 03/15] fix: updated url to point to settings --- packages/builder/src/pages/builder/portal/apps/index.svelte | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 => { From b2e718504c90dda132f1a5ac714bfbaeb7a551b4 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 15:17:36 +0100 Subject: [PATCH 04/15] Adri PR comments. --- packages/server/src/api/controllers/role.ts | 6 ++++-- packages/server/src/api/routes/tests/permissions.spec.ts | 4 ++-- packages/server/src/api/routes/tests/screen.spec.ts | 2 +- packages/server/src/tests/utilities/TestConfiguration.ts | 2 +- packages/server/src/tests/utilities/api/role.ts | 4 ---- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 7b9f2d7658..fef68a650c 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -72,9 +72,8 @@ 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 = externalRole(role) } export async function save(ctx: UserCtx) { @@ -82,6 +81,9 @@ export async function save(ctx: UserCtx) { let { _id, name, inherits, permissionId, version, uiMetadata } = ctx.request.body let isCreate = false + if (!ctx.request.body._rev && !version) { + version = roles.RoleIDVersion.NAME + } const isNewVersion = version === roles.RoleIDVersion.NAME if (_id && roles.isBuiltin(_id)) { diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 46ce656459..90060eef81 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -328,7 +328,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 +347,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/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts index f660589951..5853f44bac 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!, { diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index a3f2fb7adc..fc5a4a7f9c 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -429,7 +429,7 @@ 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, diff --git a/packages/server/src/tests/utilities/api/role.ts b/packages/server/src/tests/utilities/api/role.ts index 05165cd38e..31bffc6f85 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, From 3b597f6405e23ed81e997ab89d457048dd542053 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 15:25:34 +0100 Subject: [PATCH 05/15] Fixing test case. --- packages/server/src/api/routes/tests/permissions.spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 90060eef81..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, }, From cfc5848d140d02f3f01de4d52a3a0a17e3c34190 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 16:10:32 +0100 Subject: [PATCH 06/15] Improving how traversal is performed for role inheritance. --- packages/backend-core/src/security/roles.ts | 109 +++++++++++--------- 1 file changed, 61 insertions(+), 48 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index fdea09944a..44abc64d00 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -10,6 +10,7 @@ import { getAppDB } from "../context" import { Screen, Role as RoleDoc, RoleUIMetadata } 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", @@ -38,6 +39,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 @@ -76,6 +85,54 @@ export class Role implements RoleDoc { } } +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 + const 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) { + return 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 this.walk(currentRole) + } else { + foundRoleIds.push(currentRole.inherits) + currentRole = findRole(currentRole.inherits, allRoles, opts) + if (role) { + roleList.push(role) + } + } + } + } + return uniqBy(roleList, role => role._id) + } +} + const BUILTIN_ROLES = { ADMIN: new Role( BUILTIN_IDS.ADMIN, @@ -309,59 +366,15 @@ async function getAllUserRoles( 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( From 3da3bccc01b29266241f507ee81de5c90763305a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 16:27:47 +0100 Subject: [PATCH 07/15] Some fixes for traverser. --- packages/backend-core/src/security/roles.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 44abc64d00..b3c18202d2 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -98,7 +98,7 @@ export class RoleHierarchyTraversal { const opts = this.opts, allRoles = this.allRoles // this will be a full walked list of roles - which may contain duplicates - const roleList: RoleDoc[] = [] + let roleList: RoleDoc[] = [] if (!role || !role._id) { return roleList } @@ -107,7 +107,7 @@ export class RoleHierarchyTraversal { for (let roleId of role.inherits) { const foundRole = findRole(roleId, allRoles, opts) if (foundRole) { - return this.walk(foundRole) + roleList = roleList.concat(this.walk(foundRole)) } } } else { @@ -119,14 +119,18 @@ export class RoleHierarchyTraversal { !rolesInList(foundRoleIds, currentRole.inherits) ) { if (Array.isArray(currentRole.inherits)) { - return this.walk(currentRole) + return roleList.concat(this.walk(currentRole)) } else { foundRoleIds.push(currentRole.inherits) currentRole = findRole(currentRole.inherits, allRoles, opts) - if (role) { - roleList.push(role) + if (currentRole) { + roleList.push(currentRole) } } + // loop now found - stop iterating + if (helpers.roles.checkForRoleInheritanceLoops(roleList)) { + break + } } } return uniqBy(roleList, role => role._id) @@ -359,9 +363,6 @@ 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 From 2ae1836b9ad75464a4f85d3eabb05a66a741ff7c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 16:58:51 +0100 Subject: [PATCH 08/15] PR comments. --- packages/backend-core/src/security/roles.ts | 8 +++----- packages/server/src/api/controllers/role.ts | 6 +++--- .../src/tests/utilities/TestConfiguration.ts | 16 ++++------------ .../shared-core/src/helpers/tests/roles.spec.ts | 5 +++++ 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index b3c18202d2..366e74bec5 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -76,11 +76,9 @@ 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 } } @@ -264,7 +262,7 @@ export async function roleToNumber(id: string) { return findNumber(foundRole) + 1 } }) - .filter(number => !!number) + .filter(number => number) .sort() .pop() if (highestBuiltin != undefined) { diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index fef68a650c..89125f566e 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -78,10 +78,10 @@ export async function find(ctx: UserCtx) { 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 (!ctx.request.body._rev && !version) { + if (!_rev && !version) { version = roles.RoleIDVersion.NAME } const isNewVersion = version === roles.RoleIDVersion.NAME @@ -132,7 +132,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 } diff --git a/packages/server/src/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index fc5a4a7f9c..ca0a6e420a 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.ts +++ b/packages/server/src/tests/utilities/TestConfiguration.ts @@ -432,7 +432,7 @@ export default class TestConfiguration { 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,17 +443,9 @@ 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() + }) } defaultHeaders(extras = {}, prodApp = false) { diff --git a/packages/shared-core/src/helpers/tests/roles.spec.ts b/packages/shared-core/src/helpers/tests/roles.spec.ts index 7b67e4e9e9..051a196a5b 100644 --- a/packages/shared-core/src/helpers/tests/roles.spec.ts +++ b/packages/shared-core/src/helpers/tests/roles.spec.ts @@ -64,5 +64,10 @@ describe("role utilities", () => { role1.inherits = "role_2" check(true) }) + + it("self reference contains loop", () => { + role("role1", "role1") + check(true) + }) }) }) From bd10a3d831b0e97fe5df2090d9ceed4e127ff76e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 17:00:40 +0100 Subject: [PATCH 09/15] Missed comment. --- packages/backend-core/src/security/roles.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 366e74bec5..56037f7403 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -273,12 +273,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)) } /** From c74577c512fd3b6b063e54f7a52dd202dc92c412 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 17:15:41 +0100 Subject: [PATCH 10/15] Updating accessible utility API. --- .../server/src/api/routes/tests/role.spec.ts | 50 +++++++++++-------- .../src/tests/utilities/TestConfiguration.ts | 23 ++++++++- .../server/src/tests/utilities/api/role.ts | 6 +-- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 134078e6bf..a79f219311 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -228,29 +228,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 () => { @@ -261,13 +267,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"]) }) }) @@ -297,10 +305,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/tests/utilities/TestConfiguration.ts b/packages/server/src/tests/utilities/TestConfiguration.ts index ca0a6e420a..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) { @@ -448,6 +449,18 @@ export default class TestConfiguration { }) } + async withHeaders( + headers: Record, + cb: () => Promise + ) { + this.temporaryHeaders = headers + try { + await cb() + } finally { + this.temporaryHeaders = undefined + } + } + defaultHeaders(extras = {}, prodApp = false) { const tenantId = this.getTenantId() const user = this.getUser() @@ -471,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 } = {}) { @@ -487,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 31bffc6f85..5bd0647384 100644 --- a/packages/server/src/tests/utilities/api/role.ts +++ b/packages/server/src/tests/utilities/api/role.ts @@ -34,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, }) } From 47de88f42fd08108c6d9eb08045c1710c8347151 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 17:47:55 +0100 Subject: [PATCH 11/15] Destroy test case. --- packages/backend-core/src/security/roles.ts | 26 ++++++++++++- packages/server/src/api/controllers/role.ts | 38 +++++++++++++------ .../server/src/api/routes/tests/role.spec.ts | 21 ++++++++++ 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index e549d2f3af..c14178cacb 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -290,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. */ @@ -347,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. */ diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts index 89125f566e..f26b4bae69 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -27,15 +27,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) } } @@ -65,7 +77,7 @@ 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) { @@ -73,7 +85,7 @@ export async function find(ctx: UserCtx) { if (!role) { ctx.throw(404, { message: "Role not found" }) } - ctx.body = externalRole(role) + ctx.body = roles.externalRole(role) } export async function save(ctx: UserCtx) { @@ -149,7 +161,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 +210,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 } diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index a79f219311..5668295342 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -217,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", () => { From d0df75c9e421b6f9caa981f1234b1ded1affb768 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 17 Oct 2024 17:50:51 +0100 Subject: [PATCH 12/15] Adding back mock clearing. --- packages/server/src/api/routes/tests/screen.spec.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/server/src/api/routes/tests/screen.spec.ts b/packages/server/src/api/routes/tests/screen.spec.ts index 5853f44bac..5dfe3d2a44 100644 --- a/packages/server/src/api/routes/tests/screen.spec.ts +++ b/packages/server/src/api/routes/tests/screen.spec.ts @@ -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 }) From 1109e27b04f18a1a9e0b7da00b8fadb10dfaad25 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 18 Oct 2024 09:56:35 +0100 Subject: [PATCH 13/15] Display numbers using the locale format in tables --- .../frontend-core/src/components/grid/cells/TextCell.svelte | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/frontend-core/src/components/grid/cells/TextCell.svelte b/packages/frontend-core/src/components/grid/cells/TextCell.svelte index 0cf0ab2004..1f6415598f 100644 --- a/packages/frontend-core/src/components/grid/cells/TextCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/TextCell.svelte @@ -52,7 +52,11 @@ {:else}
- {value ?? ""} + {#if type === "number"} + {(value ?? "").toLocaleString()} + {:else} + {value ?? ""} + {/if}
{/if} From 1509d7d650928bba8ff92e4207afbb9f1894259a Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 18 Oct 2024 10:44:36 +0100 Subject: [PATCH 14/15] Use single number formatter for more performant locale string conversions --- .../frontend-core/src/components/grid/cells/TextCell.svelte | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/frontend-core/src/components/grid/cells/TextCell.svelte b/packages/frontend-core/src/components/grid/cells/TextCell.svelte index 1f6415598f..b6b6756068 100644 --- a/packages/frontend-core/src/components/grid/cells/TextCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/TextCell.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 b6b6756068..9275bca3c6 100644 --- a/packages/frontend-core/src/components/grid/cells/TextCell.svelte +++ b/packages/frontend-core/src/components/grid/cells/TextCell.svelte @@ -1,7 +1,3 @@ - -