Handling the removal of the role_ prefix where applicable so that new role IDs present in the exact same way as built in roles.

packages/backend-core/src/docIds/ids.ts

@@ -81,8 +81,12 @@ export function generateAppUserID(prodAppId: string, userId: string) {
  * Generates a new role ID.
  * @returns {string} The new role ID which the role doc can be stored under.
  */
-export function generateRoleID(id?: any) {
+export function generateRoleID(name: string) {
-  return `${DocumentType.ROLE}${SEPARATOR}${id || newid()}`
+  const prefix = `${DocumentType.ROLE}${SEPARATOR}`
+  if (name.startsWith(prefix)) {
+    return name
+  }
+  return `${prefix}${name}`
 }
 
 /**

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

@@ -25,18 +25,28 @@ const EXTERNAL_BUILTIN_ROLE_IDS = [
   BUILTIN_IDS.PUBLIC,
 ]
 
+export const RoleVersion = {
+  // original version, with a UUID based ID
+  VERSION_1: undefined,
+  // new version - with name based ID
+  VERSION_2: 2,
+}
+
 export class Role implements RoleDoc {
   _id: string
   _rev?: string
   name: string
   permissionId: string
   inherits?: string
+  version?: number
   permissions = {}
 
   constructor(id: string, name: string, permissionId: string) {
     this._id = id
     this.name = name
     this.permissionId = permissionId
+    // version for managing the ID - removing the role_ when responding
+    this.version = RoleVersion.VERSION_2
   }
 
   addInheritance(inherits: string) {
@@ -157,13 +167,16 @@ export async function getRole(
     role = cloneDeep(
       Object.values(BUILTIN_ROLES).find(role => role._id === roleId)
     )
+  } else {
+    // make sure has the prefix (if it has it then it won't be added)
+    roleId = generateRoleID(roleId)
   }
   try {
     const db = getAppDB()
     const dbRole = await db.get(getDBRoleID(roleId))
     role = Object.assign(role, dbRole)
     // finalise the ID
-    role._id = getExternalRoleID(role._id)
+    role._id = getExternalRoleID(role._id, role.version)
   } catch (err) {
     if (!isBuiltin(roleId) && opts?.defaultPublic) {
       return cloneDeep(BUILTIN_ROLES.PUBLIC)
@@ -261,6 +274,9 @@ export async function getAllRoles(appId?: string) {
         })
       )
       roles = body.rows.map((row: any) => row.doc)
+      roles.forEach(
+        role => (role._id = getExternalRoleID(role._id!, role.version))
+      )
     }
     const builtinRoles = getBuiltinRoles()
 
@@ -268,14 +284,15 @@ export async function getAllRoles(appId?: string) {
     for (let builtinRoleId of EXTERNAL_BUILTIN_ROLE_IDS) {
       const builtinRole = builtinRoles[builtinRoleId]
       const dbBuiltin = roles.filter(
-        dbRole => getExternalRoleID(dbRole._id) === builtinRoleId
+        dbRole =>
+          getExternalRoleID(dbRole._id!, dbRole.version) === builtinRoleId
       )[0]
       if (dbBuiltin == null) {
         roles.push(builtinRole || builtinRoles.BASIC)
       } else {
         // remove role and all back after combining with the builtin
         roles = roles.filter(role => role._id !== dbBuiltin._id)
-        dbBuiltin._id = getExternalRoleID(dbBuiltin._id)
+        dbBuiltin._id = getExternalRoleID(dbBuiltin._id!, dbBuiltin.version)
         roles.push(Object.assign(builtinRole, dbBuiltin))
       }
     }
@@ -381,19 +398,22 @@ export class AccessController {
 /**
  * Adds the "role_" for builtin role IDs which are to be written to the DB (for permissions).
  */
-export function getDBRoleID(roleId?: string) {
+export function getDBRoleID(roleName: string) {
-  if (roleId?.startsWith(DocumentType.ROLE)) {
+  if (roleName?.startsWith(DocumentType.ROLE)) {
-    return roleId
+    return roleName
   }
-  return generateRoleID(roleId)
+  return generateRoleID(roleName)
 }
 
 /**
  * Remove the "role_" from builtin role IDs that have been written to the DB (for permissions).
  */
-export function getExternalRoleID(roleId?: string) {
+export function getExternalRoleID(roleId: string, version?: number) {
   // for built-in roles we want to remove the DB role ID element (role_)
-  if (roleId?.startsWith(DocumentType.ROLE) && isBuiltin(roleId)) {
+  if (
+    (roleId.startsWith(DocumentType.ROLE) && isBuiltin(roleId)) ||
+    version === RoleVersion.VERSION_2
+  ) {
     return roleId.split(`${DocumentType.ROLE}${SEPARATOR}`)[1]
   }
   return roleId

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

@@ -5,7 +5,7 @@ import {
   getBasePermissions,
 } from "../../utilities/security"
 import { removeFromArray } from "../../utilities"
-import { BBContext, Database, Role } from "@budibase/types"
+import { UserCtx, Database, Role } from "@budibase/types"
 
 const PermissionUpdateType = {
   REMOVE: "remove",
@@ -38,12 +38,12 @@ async function updatePermissionOnRole(
   const isABuiltin = roles.isBuiltin(roleId)
   const dbRoleId = roles.getDBRoleID(roleId)
   const dbRoles = await getAllDBRoles(db)
-  const docUpdates = []
+  const docUpdates: Role[] = []
 
   // the permission is for a built in, make sure it exists
   if (isABuiltin && !dbRoles.some(role => role._id === dbRoleId)) {
     const builtin = roles.getBuiltinRoles()[roleId]
-    builtin._id = roles.getDBRoleID(builtin._id)
+    builtin._id = roles.getDBRoleID(builtin._id!)
     dbRoles.push(builtin)
   }
 
@@ -88,22 +88,23 @@ async function updatePermissionOnRole(
 
   const response = await db.bulkDocs(docUpdates)
   return response.map((resp: any) => {
-    resp._id = roles.getExternalRoleID(resp.id)
+    const version = docUpdates.find(role => role._id === resp.id)?.version
+    resp._id = roles.getExternalRoleID(resp.id, version)
     delete resp.id
     return resp
   })
 }
 
-export function fetchBuiltin(ctx: BBContext) {
+export function fetchBuiltin(ctx: UserCtx) {
   ctx.body = Object.values(permissions.getBuiltinPermissions())
 }
 
-export function fetchLevels(ctx: BBContext) {
+export function fetchLevels(ctx: UserCtx) {
   // for now only provide the read/write perms externally
   ctx.body = SUPPORTED_LEVELS
 }
 
-export async function fetch(ctx: BBContext) {
+export async function fetch(ctx: UserCtx) {
   const db = context.getAppDB()
   const dbRoles: Role[] = await getAllDBRoles(db)
   let permissions: any = {}
@@ -112,7 +113,7 @@ export async function fetch(ctx: BBContext) {
     if (!role.permissions) {
       continue
     }
-    const roleId = roles.getExternalRoleID(role._id)
+    const roleId = roles.getExternalRoleID(role._id!, role.version)
     if (!roleId) {
       ctx.throw(400, "Unable to retrieve role")
     }
@@ -132,7 +133,7 @@ export async function fetch(ctx: BBContext) {
   ctx.body = finalPermissions
 }
 
-export async function getResourcePerms(ctx: BBContext) {
+export async function getResourcePerms(ctx: UserCtx) {
   const resourceId = ctx.params.resourceId
   const db = context.getAppDB()
   const body = await db.allDocs(
@@ -154,14 +155,14 @@ export async function getResourcePerms(ctx: BBContext) {
         rolePerms[resourceId] &&
         rolePerms[resourceId].indexOf(level) !== -1
       ) {
-        permissions[level] = roles.getExternalRoleID(role._id)!
+        permissions[level] = roles.getExternalRoleID(role._id, role.version)!
       }
     }
   }
   ctx.body = Object.assign(getBasePermissions(resourceId), permissions)
 }
 
-export async function addPermission(ctx: BBContext) {
+export async function addPermission(ctx: UserCtx) {
   ctx.body = await updatePermissionOnRole(
     ctx.appId,
     ctx.params,
@@ -169,7 +170,7 @@ export async function addPermission(ctx: BBContext) {
   )
 }
 
-export async function removePermission(ctx: BBContext) {
+export async function removePermission(ctx: UserCtx) {
   ctx.body = await updatePermissionOnRole(
     ctx.appId,
     ctx.params,

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

@@ -14,7 +14,8 @@ const UpdateRolesOptions = {
 async function updateRolesOnUserTable(
   db: Database,
   roleId: string,
-  updateOption: string
+  updateOption: string,
+  roleVersion?: number
 ) {
   const table = await db.get(InternalTables.USER_METADATA)
   const schema = table.schema
@@ -24,11 +25,15 @@ async function updateRolesOnUserTable(
     if (prop === "roleId") {
       updated = true
       const constraints = schema[prop].constraints
-      const indexOf = constraints.inclusion.indexOf(roleId)
+      const updatedRoleId =
+        roleVersion === roles.RoleVersion.VERSION_2
+          ? roles.getExternalRoleID(roleId, roleVersion)
+          : roleId
+      const indexOf = constraints.inclusion.indexOf(updatedRoleId)
       if (remove && indexOf !== -1) {
         constraints.inclusion.splice(indexOf, 1)
       } else if (!remove && indexOf === -1) {
-        constraints.inclusion.push(roleId)
+        constraints.inclusion.push(updatedRoleId)
       }
       break
     }
@@ -48,14 +53,15 @@ export async function find(ctx: UserCtx) {
 
 export async function save(ctx: UserCtx) {
   const db = context.getAppDB()
-  let { _id, name, inherits, permissionId } = ctx.request.body
+  let { _id, name, inherits, permissionId, version } = ctx.request.body
   let isCreate = false
-  if (!_id) {
+  if (!_id || version === roles.RoleVersion.VERSION_2) {
-    _id = generateRoleID()
+    _id = generateRoleID(name)
     isCreate = true
   } else if (roles.isBuiltin(_id)) {
     ctx.throw(400, "Cannot update builtin roles.")
   }
+
   const role = new roles.Role(_id, name, permissionId).addInheritance(inherits)
   if (ctx.request.body._rev) {
     role._rev = ctx.request.body._rev
@@ -66,7 +72,12 @@ export async function save(ctx: UserCtx) {
   } else {
     await events.role.updated(role)
   }
-  await updateRolesOnUserTable(db, _id, UpdateRolesOptions.CREATED)
+  await updateRolesOnUserTable(
+    db,
+    _id,
+    UpdateRolesOptions.CREATED,
+    role.version
+  )
   role._rev = result.rev
   ctx.body = role
   ctx.message = `Role '${role.name}' created successfully.`
@@ -74,11 +85,14 @@ export async function save(ctx: UserCtx) {
 
 export async function destroy(ctx: UserCtx) {
   const db = context.getAppDB()
-  const roleId = ctx.params.roleId
+  let roleId = ctx.params.roleId
-  const role = await db.get(roleId)
   if (roles.isBuiltin(roleId)) {
     ctx.throw(400, "Cannot delete builtin role.")
+  } else {
+    // make sure has the prefix (if it has it then it won't be added)
+    roleId = generateRoleID(roleId)
   }
+  const role = await db.get(roleId)
   // first check no users actively attached to role
   const users = (
     await db.allDocs(
@@ -97,7 +111,8 @@ export async function destroy(ctx: UserCtx) {
   await updateRolesOnUserTable(
     db,
     ctx.params.roleId,
-    UpdateRolesOptions.REMOVED
+    UpdateRolesOptions.REMOVED,
+    role.version
   )
   ctx.message = `Role ${ctx.params.roleId} deleted successfully`
   ctx.status = 200

packages/types/src/documents/app/role.ts

@@ -4,4 +4,5 @@ export interface Role extends Document {
   permissionId: string
   inherits?: string
   permissions: { [key: string]: string[] }
+  version?: number
 }