Merge pull request #14853 from Budibase/fix/role-validation

Role validation - allow permissionId to be optional

packages/backend-core/src/security/permissions.ts

@@ -1,4 +1,8 @@
-import { PermissionLevel, PermissionType } from "@budibase/types"
+import {
+ PermissionLevel,
+ PermissionType,
+ BuiltinPermissionID,
+} from "@budibase/types"
 import flatten from "lodash/flatten"
 import cloneDeep from "lodash/fp/cloneDeep"
 
@@ -57,14 +61,6 @@ export function getAllowedLevels(userPermLevel: PermissionLevel): string[] {
  }
 }
 
-export enum BuiltinPermissionID {
- PUBLIC = "public",
- READ_ONLY = "read_only",
- WRITE = "write",
- ADMIN = "admin",
- POWER = "power",
-}
-
 export const BUILTIN_PERMISSIONS: {
  [key in keyof typeof BuiltinPermissionID]: {
  _id: (typeof BuiltinPermissionID)[key]

packages/backend-core/src/security/roles.ts

@@ -1,5 +1,4 @@
 import semver from "semver"
-import { BuiltinPermissionID, PermissionLevel } from "./permissions"
 import {
  prefixRoleID,
  getRoleParams,
@@ -14,6 +13,8 @@ import {
  RoleUIMetadata,
  Database,
  App,
+ BuiltinPermissionID,
+ PermissionLevel,
 } from "@budibase/types"
 import cloneDeep from "lodash/fp/cloneDeep"
 import { RoleColor, helpers } from "@budibase/shared-core"
@@ -50,7 +51,7 @@ export class Role implements RoleDoc {
  _id: string
  _rev?: string
  name: string
- permissionId: string
+ permissionId: BuiltinPermissionID
  inherits?: string | string[]
  version?: string
  permissions: Record<string, PermissionLevel[]> = {}
@@ -59,7 +60,7 @@ export class Role implements RoleDoc {
  constructor(
  id: string,
  name: string,
- permissionId: string,
+ permissionId: BuiltinPermissionID,
  uiMetadata?: RoleUIMetadata
  ) {
  this._id = id

packages/backend-core/src/security/tests/permissions.spec.ts

@@ -1,6 +1,7 @@
 import cloneDeep from "lodash/cloneDeep"
 import * as permissions from "../permissions"
 import { BUILTIN_ROLE_IDS } from "../roles"
+import { BuiltinPermissionID } from "@budibase/types"
 
 describe("levelToNumber", () => {
  it("should return 0 for EXECUTE", () => {
@@ -77,7 +78,7 @@ describe("doesHaveBasePermission", () => {
  const rolesHierarchy = [
  {
  roleId: BUILTIN_ROLE_IDS.ADMIN,
- permissionId: permissions.BuiltinPermissionID.ADMIN,
+ permissionId: BuiltinPermissionID.ADMIN,
  },
  ]
  expect(
@@ -91,7 +92,7 @@ describe("doesHaveBasePermission", () => {
  const rolesHierarchy = [
  {
  roleId: BUILTIN_ROLE_IDS.PUBLIC,
- permissionId: permissions.BuiltinPermissionID.PUBLIC,
+ permissionId: BuiltinPermissionID.PUBLIC,
  },
  ]
  expect(
@@ -129,7 +130,7 @@ describe("getBuiltinPermissions", () => {
 describe("getBuiltinPermissionByID", () => {
  it("returns correct permission object for valid ID", () => {
  const expectedPermission = {
- _id: permissions.BuiltinPermissionID.PUBLIC,
+ _id: BuiltinPermissionID.PUBLIC,
  name: "Public",
  permissions: [
  new permissions.Permission(

packages/server/src/api/controllers/role.ts

@@ -18,7 +18,7 @@ import {
  UserCtx,
  UserMetadata,
  DocumentType,
- PermissionLevel,
+ BuiltinPermissionID,
 } from "@budibase/types"
 import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core"
 import sdk from "../../sdk"
@@ -134,7 +134,13 @@ export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
  }
  // assume write permission level for newly created roles
  if (isCreate && !permissionId) {
- permissionId = PermissionLevel.WRITE
+ permissionId = BuiltinPermissionID.WRITE
+ } else if (!permissionId && dbRole?.permissionId) {
+ permissionId = dbRole.permissionId
+ }
+
+ if (!permissionId) {
+ ctx.throw(400, "Role requires permissionId to be specified.")
  }
 
  const role = new roles.Role(_id, name, permissionId, {

packages/server/src/api/routes/tests/application.spec.ts

@@ -16,7 +16,7 @@ import * as setup from "./utilities"
 import { AppStatus } from "../../../db/utils"
 import { events, utils, context, features } from "@budibase/backend-core"
 import env from "../../../environment"
-import { type App } from "@budibase/types"
+import { type App, BuiltinPermissionID } from "@budibase/types"
 import tk from "timekeeper"
 import * as uuid from "uuid"
 import { structures } from "@budibase/backend-core/tests"
@@ -80,7 +80,7 @@ describe("/applications", () => {
  const role = await config.api.roles.save({
  name: "Test",
  inherits: "PUBLIC",
- permissionId: "read_only",
+ permissionId: BuiltinPermissionID.READ_ONLY,
  version: "name",
  })
 
@@ -112,7 +112,7 @@ describe("/applications", () => {
  const role = await config.api.roles.save({
  name: roleName,
  inherits: "PUBLIC",
- permissionId: "read_only",
+ permissionId: BuiltinPermissionID.READ_ONLY,
  version: "name",
  })
 

packages/server/src/api/routes/tests/permissions.spec.ts

@@ -1,5 +1,12 @@
 import { roles } from "@budibase/backend-core"
-import { Document, PermissionLevel, Role, Row, Table } from "@budibase/types"
+import {
+ BuiltinPermissionID,
+ Document,
+ PermissionLevel,
+ Role,
+ Row,
+ Table,
+} from "@budibase/types"
 import * as setup from "./utilities"
 import { generator, mocks } from "@budibase/backend-core/tests"
 
@@ -304,15 +311,15 @@ describe("/permission", () => {
  role1 = await config.api.roles.save(
  {
  name: "test_1",
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  inherits: BUILTIN_ROLE_IDS.BASIC,
  },
  { status: 200 }
  )
  role2 = await config.api.roles.save(
  {
  name: "test_2",
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  inherits: BUILTIN_ROLE_IDS.BASIC,
  },
  { status: 200 }
@@ -345,7 +352,7 @@ describe("/permission", () => {
  it("should be able to fetch two tables, with different roles, using multi-inheritance", async () => {
  const role3 = await config.api.roles.save({
  name: "role3",
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  inherits: [role1._id!, role2._id!],
  })
 

packages/server/src/api/routes/tests/role.spec.ts

@@ -1,15 +1,9 @@
-import {
- roles,
- events,
- permissions,
- db as dbCore,
-} from "@budibase/backend-core"
+import { roles, events, db as dbCore } from "@budibase/backend-core"
 import * as setup from "./utilities"
-import { PermissionLevel } from "@budibase/types"
+import { PermissionLevel, BuiltinPermissionID } from "@budibase/types"
 
 const { basicRole } = setup.structures
 const { BUILTIN_ROLE_IDS } = roles
-const { BuiltinPermissionID } = permissions
 
 const LOOP_ERROR = "Role inheritance contains a loop, this is not supported"
 
@@ -58,6 +52,19 @@ describe("/roles", () => {
  })
  expect(res.inherits).toEqual([BUILTIN_ROLE_IDS.BASIC])
  })
+
+ it("save role without permissionId", async () => {
+ const res = await config.api.roles.save(
+ {
+ ...basicRole(),
+ permissionId: undefined,
+ },
+ {
+ status: 200,
+ }
+ )
+ expect(res.permissionId).toEqual(PermissionLevel.WRITE)
+ })
  })
 
  describe("update", () => {
@@ -149,15 +156,15 @@ describe("/roles", () => {
  _id: id1,
  name: id1,
  permissions: {},
- permissionId: "write",
+ permissionId: BuiltinPermissionID.WRITE,
  version: "name",
  inherits: ["POWER"],
  })
  await config.api.roles.save({
  _id: id2,
  permissions: {},
  name: id2,
- permissionId: "write",
+ permissionId: BuiltinPermissionID.WRITE,
  version: "name",
  inherits: [id1],
  })
@@ -176,10 +183,25 @@ describe("/roles", () => {
  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)
+ const updatedRes = await config.api.roles.save({
+ ...res,
+ inherits: undefined,
+ })
  expect(updatedRes.inherits).toEqual([BUILTIN_ROLE_IDS.ADMIN])
  })
+
+ it("handle updating a role, without its permissionId", async () => {
+ const res = await config.api.roles.save({
+ ...basicRole(),
+ permissionId: BuiltinPermissionID.READ_ONLY,
+ })
+ // permission ID can be removed during update
+ const updatedRes = await config.api.roles.save({
+ ...res,
+ permissionId: undefined,
+ })
+ expect(updatedRes.permissionId).toEqual(BuiltinPermissionID.READ_ONLY)
+ })
  })
 
  describe("fetch", () => {
@@ -210,9 +232,7 @@ describe("/roles", () => {
  const customRoleFetched = res.find(r => r._id === customRole.name)
  expect(customRoleFetched).toBeDefined()
  expect(customRoleFetched!.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
- expect(customRoleFetched!.permissionId).toEqual(
- BuiltinPermissionID.READ_ONLY
- )
+ expect(customRoleFetched!.permissionId).toEqual(BuiltinPermissionID.WRITE)
  })
 
  it("should be able to get the role with a permission added", async () => {
@@ -316,7 +336,7 @@ describe("/roles", () => {
  await config.api.roles.save({
  name: customRoleName,
  inherits: roles.BUILTIN_ROLE_IDS.BASIC,
- permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+ permissionId: BuiltinPermissionID.READ_ONLY,
  version: "name",
  })
  await config.withHeaders(
@@ -356,19 +376,19 @@ describe("/roles", () => {
  const { _id: roleId1 } = await config.api.roles.save({
  name: role1,
  inherits: roles.BUILTIN_ROLE_IDS.BASIC,
- permissionId: permissions.BuiltinPermissionID.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  version: "name",
  })
  const { _id: roleId2 } = await config.api.roles.save({
  name: role2,
  inherits: roles.BUILTIN_ROLE_IDS.POWER,
- permissionId: permissions.BuiltinPermissionID.POWER,
+ permissionId: BuiltinPermissionID.POWER,
  version: "name",
  })
  await config.api.roles.save({
  name: role3,
  inherits: [roleId1!, roleId2!],
- permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+ permissionId: BuiltinPermissionID.READ_ONLY,
  version: "name",
  })
  const headers = await config.roleHeaders({

packages/server/src/api/routes/tests/screen.spec.ts

@@ -1,7 +1,7 @@
 import { checkBuilderEndpoint } from "./utilities/TestFunctions"
 import * as setup from "./utilities"
 import { events, roles } from "@budibase/backend-core"
-import { Screen, PermissionLevel, Role } from "@budibase/types"
+import { Screen, Role, BuiltinPermissionID } from "@budibase/types"
 
 const { basicScreen } = setup.structures
 
@@ -40,17 +40,17 @@ describe("/screens", () => {
  role1 = await config.api.roles.save({
  name: "role1",
  inherits: roles.BUILTIN_ROLE_IDS.BASIC,
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  })
  role2 = await config.api.roles.save({
  name: "role2",
  inherits: roles.BUILTIN_ROLE_IDS.BASIC,
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  })
  multiRole = await config.api.roles.save({
  name: "multiRole",
  inherits: [role1._id!, role2._id!],
- permissionId: PermissionLevel.WRITE,
+ permissionId: BuiltinPermissionID.WRITE,
  })
  screen1 = await config.api.screen.save(
  {

packages/server/src/api/routes/utils/validators.ts

@@ -8,6 +8,7 @@ import {
  SearchFilters,
  Table,
  WebhookActionType,
+ BuiltinPermissionID,
 } from "@budibase/types"
 import Joi, { CustomValidator } from "joi"
 import { ValidSnippetNameRegex, helpers } from "@budibase/shared-core"
@@ -214,8 +215,8 @@ export function roleValidator() {
  }).optional(),
  // this is the base permission ID (for now a built in)
  permissionId: Joi.string()
- .valid(...Object.values(permissions.BuiltinPermissionID))
- .required(),
+ .valid(...Object.values(BuiltinPermissionID))
+ .optional(),
  permissions: Joi.object()
  .pattern(
  /.*/,

packages/server/src/tests/utilities/structures.ts

@@ -1,4 +1,4 @@
-import { permissions, roles, utils } from "@budibase/backend-core"
+import { roles, utils } from "@budibase/backend-core"
 import { createHomeScreen } from "../../constants/screens"
 import { EMPTY_LAYOUT } from "../../constants/layouts"
 import { cloneDeep } from "lodash/fp"
@@ -33,6 +33,7 @@ import {
  TableSourceType,
  Webhook,
  WebhookActionType,
+ BuiltinPermissionID,
 } from "@budibase/types"
 import { LoopInput } from "../../definitions/automations"
 import { merge } from "lodash"
@@ -515,7 +516,7 @@ export function basicRole(): Role {
  return {
  name: `NewRole_${utils.newid()}`,
  inherits: roles.BUILTIN_ROLE_IDS.BASIC,
- permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+ permissionId: BuiltinPermissionID.WRITE,
  permissions: {},
  version: "name",
  }

packages/shared-core/src/helpers/tests/roles.spec.ts

@@ -1,5 +1,5 @@
 import { checkForRoleInheritanceLoops } from "../roles"
-import { Role } from "@budibase/types"
+import { BuiltinPermissionID, Role } from "@budibase/types"
 
 /**
  * This unit test exists as this utility will be used in the frontend and backend, confirmation
@@ -19,7 +19,7 @@ function role(id: string, inherits: string | string[]): TestRole {
  _id: id,
  inherits: inherits,
  name: "ROLE",
- permissionId: "PERMISSION",
+ permissionId: BuiltinPermissionID.WRITE,
  permissions: {}, // not needed for this test
  }
  allRoles.push(role)