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