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,8 @@
   UserCtx,
   UserMetadata,
   DocumentType,
   PermissionLevel,
+  BuiltinPermissionID,
 } from "@budibase/types"
 import { RoleColor, sdk as sharedSdk, helpers } from "@budibase/shared-core"
 import sdk from "../../sdk"
@@ -134,7 +135,13 @@
   }
   // 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", () => {
@@ -316,7 +338,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 +378,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)