diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index c14178cacb..7d9a11db48 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 => roleIDsAreEqual(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. */ @@ -290,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) } @@ -323,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) @@ -557,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 d09fcf28f6..1047711983 100644 --- a/packages/server/src/api/controllers/role.ts +++ b/packages/server/src/api/controllers/role.ts @@ -18,6 +18,7 @@ import { UserCtx, UserMetadata, DocumentType, + PermissionLevel, } from "@budibase/types" import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core" import sdk from "../../sdk" @@ -35,11 +36,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 } @@ -125,6 +126,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", @@ -226,35 +238,23 @@ 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] + 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 (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)) } diff --git a/packages/server/src/api/routes/tests/role.spec.ts b/packages/server/src/api/routes/tests/role.spec.ts index 5668295342..adb83ca793 100644 --- a/packages/server/src/api/routes/tests/role.spec.ts +++ b/packages/server/src/api/routes/tests/role.spec.ts @@ -38,6 +38,26 @@ describe("/roles", () => { _id: dbCore.prefixRoleID(res._id!), }) }) + + 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 = [] + + const res = await config.api.roles.save(role, { + status: 200, + }) + expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC]) + }) }) describe("update", () => { @@ -149,6 +169,17 @@ 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], + }) + // 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]) + }) }) describe("fetch", () => { @@ -298,6 +329,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/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", () => { 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 } } 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(