From 45120c60ba364fdca13d81ae1b42cea194bb7573 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Wed, 21 Jun 2023 17:08:54 +0100
Subject: [PATCH 1/7] Getting basic regex in place to make sure role names can
 be IDs.

---
 .../backend/DataTable/modals/EditRoles.svelte | 23 +++++++++++++++----
 .../server/src/api/routes/utils/validators.ts |  2 +-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
index 600e331d3e..3f976b40f2 100644
--- a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
+++ b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
@@ -12,15 +12,14 @@
   let selectedRole = BASE_ROLE
   let errors = []
   let builtInRoles = ["Admin", "Power", "Basic", "Public"]
+  let validRegex = /^[a-zA-Z0-9_]*$/
   // Don't allow editing of public role
   $: editableRoles = $roles.filter(role => role._id !== "PUBLIC")
   $: selectedRoleId = selectedRole._id
   $: otherRoles = editableRoles.filter(role => role._id !== selectedRoleId)
   $: isCreating = selectedRoleId == null || selectedRoleId === ""
 
-  $: hasUniqueRoleName = !otherRoles
-    ?.map(role => role.name)
-    ?.includes(selectedRole.name)
+  $: roleNameError = getRoleNameError(selectedRole.name)
 
   $: valid =
     selectedRole.name &&
@@ -101,6 +100,20 @@
     }
   }
 
+  const getRoleNameError = name => {
+    const hasUniqueRoleName = !otherRoles
+      ?.map(role => role.name)
+      ?.includes(name)
+    const invalidRoleName = !validRegex.test(name)
+    if (!hasUniqueRoleName) {
+      return "Select a unique role name."
+    } else if (invalidRoleName) {
+      return "Please enter a role name consisting of only alphanumeric symbols and underscores"
+    } else {
+      return null
+    }
+  }
+
   onMount(fetchBasePermissions)
 </script>
 
@@ -108,7 +121,7 @@
   title="Edit Roles"
   confirmText={isCreating ? "Create" : "Save"}
   onConfirm={saveRole}
-  disabled={!valid || !hasUniqueRoleName}
+  disabled={!valid || roleNameError}
 >
   {#if errors.length}
     <ErrorsBox {errors} />
@@ -129,7 +142,7 @@
       label="Name"
       bind:value={selectedRole.name}
       disabled={shouldDisableRoleInput}
-      error={!hasUniqueRoleName ? "Select a unique role name." : null}
+      error={roleNameError}
     />
     <Select
       label="Inherits Role"
diff --git a/packages/server/src/api/routes/utils/validators.ts b/packages/server/src/api/routes/utils/validators.ts
index 4da680edd1..55766cd120 100644
--- a/packages/server/src/api/routes/utils/validators.ts
+++ b/packages/server/src/api/routes/utils/validators.ts
@@ -134,7 +134,7 @@ export function roleValidator() {
   return auth.joiValidator.body(Joi.object({
     _id: OPTIONAL_STRING,
     _rev: OPTIONAL_STRING,
-    name: Joi.string().required(),
+    name: Joi.string().regex(/^[a-zA-Z0-9_]*$/).required(),
     // this is the base permission ID (for now a built in)
     permissionId: Joi.string().valid(...Object.values(permissions.BuiltinPermissionID)).required(),
     permissions: Joi.object()

From baecab785d7d0a12abcd7f0268006f6713a8f532 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Thu, 22 Jun 2023 18:02:35 +0100
Subject: [PATCH 2/7] 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       |  8 +++-
 packages/backend-core/src/security/roles.ts   | 38 ++++++++++++++-----
 .../server/src/api/controllers/permission.ts  | 25 ++++++------
 packages/server/src/api/controllers/role.ts   | 35 ++++++++++++-----
 packages/types/src/documents/app/role.ts      |  1 +
 5 files changed, 74 insertions(+), 33 deletions(-)

diff --git a/packages/backend-core/src/docIds/ids.ts b/packages/backend-core/src/docIds/ids.ts
index 152977b3af..02cf2248e3 100644
--- a/packages/backend-core/src/docIds/ids.ts
+++ b/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) {
-  return `${DocumentType.ROLE}${SEPARATOR}${id || newid()}`
+export function generateRoleID(name: string) {
+  const prefix = `${DocumentType.ROLE}${SEPARATOR}`
+  if (name.startsWith(prefix)) {
+    return name
+  }
+  return `${prefix}${name}`
 }
 
 /**
diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index e8a3c76c0a..0c05e54c77 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/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) {
-  if (roleId?.startsWith(DocumentType.ROLE)) {
-    return roleId
+export function getDBRoleID(roleName: string) {
+  if (roleName?.startsWith(DocumentType.ROLE)) {
+    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
diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts
index bf2a905b51..20e64d2cfb 100644
--- a/packages/server/src/api/controllers/permission.ts
+++ b/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,
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 419643cdc7..883a29df64 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/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) {
-    _id = generateRoleID()
+  if (!_id || version === roles.RoleVersion.VERSION_2) {
+    _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
-  const role = await db.get(roleId)
+  let roleId = ctx.params.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
diff --git a/packages/types/src/documents/app/role.ts b/packages/types/src/documents/app/role.ts
index 2f558dfa45..2167041b90 100644
--- a/packages/types/src/documents/app/role.ts
+++ b/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
 }

From ced1dd95c73d5303355d40e79384f05aee34fd66 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 26 Jun 2023 16:35:16 +0100
Subject: [PATCH 3/7] Updating roles tests to make sure ID always valid.

---
 packages/server/src/tests/utilities/structures.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts
index 71f30f0074..31b052336e 100644
--- a/packages/server/src/tests/utilities/structures.ts
+++ b/packages/server/src/tests/utilities/structures.ts
@@ -263,7 +263,7 @@ export function basicLinkedRow(
 
 export function basicRole() {
   return {
-    name: "NewRole",
+    name: `NewRole_${utils.newid()}`,
     inherits: roles.BUILTIN_ROLE_IDS.BASIC,
     permissionId: permissions.BuiltinPermissionID.READ_ONLY,
   }

From 8882801315533dd3667c0e585ab8f80008995230 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 26 Jun 2023 17:01:50 +0100
Subject: [PATCH 4/7] Fixing lookup of custom role by ID in test case.

---
 packages/worker/src/api/routes/global/tests/roles.spec.ts | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/packages/worker/src/api/routes/global/tests/roles.spec.ts b/packages/worker/src/api/routes/global/tests/roles.spec.ts
index 477ccaf94c..3fae020aef 100644
--- a/packages/worker/src/api/routes/global/tests/roles.spec.ts
+++ b/packages/worker/src/api/routes/global/tests/roles.spec.ts
@@ -19,6 +19,7 @@ jest.mock("@budibase/backend-core", () => {
 
 let appId: string
 let appDb: Database
+const ROLE_NAME = "newRole"
 
 async function addAppMetadata() {
   await appDb.put({
@@ -34,7 +35,7 @@ describe("/api/global/roles", () => {
   const config = new TestConfiguration()
 
   const role = new roles.Role(
-    db.generateRoleID("newRole"),
+    db.generateRoleID(ROLE_NAME),
     roles.BUILTIN_ROLE_IDS.BASIC,
     permissions.BuiltinPermissionID.READ_ONLY
   )
@@ -66,7 +67,7 @@ describe("/api/global/roles", () => {
       const res = await config.api.roles.get()
       expect(res.body).toBeDefined()
       expect(res.body[appId].roles.length).toEqual(5)
-      expect(res.body[appId].roles.map((r: any) => r._id)).toContain(role._id)
+      expect(res.body[appId].roles.map((r: any) => r._id)).toContain(ROLE_NAME)
     })
   })
 

From 7e79c827a7f53af4021663614fb36c2e6e9efa4d Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Mon, 26 Jun 2023 19:10:04 +0100
Subject: [PATCH 5/7] Test fixes, as well as issue discovered by test around
 _id generation and event triggering.

---
 packages/server/src/api/controllers/role.ts       | 14 +++++++++++---
 packages/server/src/api/routes/tests/role.spec.js | 12 +++++++-----
 packages/server/src/tests/utilities/structures.ts |  1 +
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index 883a29df64..ff8dd3609c 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -55,11 +55,19 @@ export async function save(ctx: UserCtx) {
   const db = context.getAppDB()
   let { _id, name, inherits, permissionId, version } = ctx.request.body
   let isCreate = false
-  if (!_id || version === roles.RoleVersion.VERSION_2) {
+
+  if (_id && roles.isBuiltin(_id)) {
+    ctx.throw(400, "Cannot update builtin roles.")
+  }
+
+  // if not id found, then its creation
+  if (!_id) {
     _id = generateRoleID(name)
     isCreate = true
-  } else if (roles.isBuiltin(_id)) {
-    ctx.throw(400, "Cannot update builtin roles.")
+  }
+  // version 2 roles need updated to add back role_
+  else if (version === roles.RoleVersion.VERSION_2) {
+    _id = generateRoleID(name)
   }
 
   const role = new roles.Role(_id, name, permissionId).addInheritance(inherits)
diff --git a/packages/server/src/api/routes/tests/role.spec.js b/packages/server/src/api/routes/tests/role.spec.js
index f42ffe4846..5282597897 100644
--- a/packages/server/src/api/routes/tests/role.spec.js
+++ b/packages/server/src/api/routes/tests/role.spec.js
@@ -29,10 +29,11 @@ describe("/roles", () => {
 
   describe("create", () => {
     it("returns a success message when role is successfully created", async () => {
-      const res = await createRole()
+      const role = basicRole()
+      const res = await createRole(role)
 
       expect(res.res.statusMessage).toEqual(
-        "Role 'NewRole' created successfully."
+        `Role '${role.name}' created successfully.`
       )
       expect(res.body._id).toBeDefined()
       expect(res.body._rev).toBeDefined()
@@ -44,12 +45,13 @@ describe("/roles", () => {
 
   describe("update", () => {
     it("updates a role", async () => {
-      let res = await createRole()
+      const role = basicRole()
+      let res = await createRole(role)
       jest.clearAllMocks()
       res = await createRole(res.body)
 
       expect(res.res.statusMessage).toEqual(
-        "Role 'NewRole' created successfully."
+        `Role '${role.name}' created successfully.`
       )
       expect(res.body._id).toBeDefined()
       expect(res.body._rev).toBeDefined()
@@ -86,7 +88,7 @@ describe("/roles", () => {
       expect(powerUserRole.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
       expect(powerUserRole.permissionId).toEqual(BuiltinPermissionID.POWER)
 
-      const customRoleFetched = res.body.find(r => r._id === customRole._id)
+      const customRoleFetched = res.body.find(r => r._id === customRole.name)
       expect(customRoleFetched).toBeDefined()
       expect(customRoleFetched.inherits).toEqual(BUILTIN_ROLE_IDS.BASIC)
       expect(customRoleFetched.permissionId).toEqual(
diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts
index 31b052336e..fc8a2b3e98 100644
--- a/packages/server/src/tests/utilities/structures.ts
+++ b/packages/server/src/tests/utilities/structures.ts
@@ -266,6 +266,7 @@ export function basicRole() {
     name: `NewRole_${utils.newid()}`,
     inherits: roles.BUILTIN_ROLE_IDS.BASIC,
     permissionId: permissions.BuiltinPermissionID.READ_ONLY,
+    version: 2,
   }
 }
 

From 4d2aa2a67ba81f380e10388069b501e8c1f2e9fa Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 27 Jun 2023 12:45:00 +0100
Subject: [PATCH 6/7] PR comments.

---
 packages/backend-core/src/security/roles.ts       | 14 +++++++-------
 packages/server/src/api/controllers/role.ts       |  6 +++---
 packages/server/src/sdk/app/backups/exports.ts    | 10 +++++++---
 packages/server/src/tests/utilities/structures.ts |  2 +-
 packages/types/src/documents/app/role.ts          |  2 +-
 5 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index 0c05e54c77..b43017afb0 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -25,11 +25,11 @@ const EXTERNAL_BUILTIN_ROLE_IDS = [
   BUILTIN_IDS.PUBLIC,
 ]
 
-export const RoleVersion = {
+export const RoleIDVersion = {
   // original version, with a UUID based ID
-  VERSION_1: undefined,
+  UUID: undefined,
   // new version - with name based ID
-  VERSION_2: 2,
+  NAME: "name",
 }
 
 export class Role implements RoleDoc {
@@ -38,7 +38,7 @@ export class Role implements RoleDoc {
   name: string
   permissionId: string
   inherits?: string
-  version?: number
+  version?: string
   permissions = {}
 
   constructor(id: string, name: string, permissionId: string) {
@@ -46,7 +46,7 @@ export class Role implements RoleDoc {
     this.name = name
     this.permissionId = permissionId
     // version for managing the ID - removing the role_ when responding
-    this.version = RoleVersion.VERSION_2
+    this.version = RoleIDVersion.NAME
   }
 
   addInheritance(inherits: string) {
@@ -408,11 +408,11 @@ export function getDBRoleID(roleName: string) {
 /**
  * Remove the "role_" from builtin role IDs that have been written to the DB (for permissions).
  */
-export function getExternalRoleID(roleId: string, version?: number) {
+export function getExternalRoleID(roleId: string, version?: string) {
   // for built-in roles we want to remove the DB role ID element (role_)
   if (
     (roleId.startsWith(DocumentType.ROLE) && isBuiltin(roleId)) ||
-    version === RoleVersion.VERSION_2
+    version === RoleIDVersion.NAME
   ) {
     return roleId.split(`${DocumentType.ROLE}${SEPARATOR}`)[1]
   }
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index ff8dd3609c..f772405846 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -15,7 +15,7 @@ async function updateRolesOnUserTable(
   db: Database,
   roleId: string,
   updateOption: string,
-  roleVersion?: number
+  roleVersion: string | undefined
 ) {
   const table = await db.get(InternalTables.USER_METADATA)
   const schema = table.schema
@@ -26,7 +26,7 @@ async function updateRolesOnUserTable(
       updated = true
       const constraints = schema[prop].constraints
       const updatedRoleId =
-        roleVersion === roles.RoleVersion.VERSION_2
+        roleVersion === roles.RoleIDVersion.NAME
           ? roles.getExternalRoleID(roleId, roleVersion)
           : roleId
       const indexOf = constraints.inclusion.indexOf(updatedRoleId)
@@ -66,7 +66,7 @@ export async function save(ctx: UserCtx) {
     isCreate = true
   }
   // version 2 roles need updated to add back role_
-  else if (version === roles.RoleVersion.VERSION_2) {
+  else if (version === roles.RoleIDVersion.NAME) {
     _id = generateRoleID(name)
   }
 
diff --git a/packages/server/src/sdk/app/backups/exports.ts b/packages/server/src/sdk/app/backups/exports.ts
index 3be8c64159..306918cf80 100644
--- a/packages/server/src/sdk/app/backups/exports.ts
+++ b/packages/server/src/sdk/app/backups/exports.ts
@@ -22,12 +22,12 @@ import tar from "tar"
 
 const MemoryStream = require("memorystream")
 
-interface DBDumpOpts {
+export interface DBDumpOpts {
   filter?: any
   exportPath?: string
 }
 
-interface ExportOpts extends DBDumpOpts {
+export interface ExportOpts extends DBDumpOpts {
   tar?: boolean
   excludeRows?: boolean
   excludeLogs?: boolean
@@ -57,7 +57,10 @@ function tarFilesToTmp(tmpDir: string, files: string[]) {
  * a filter function or the name of the export.
  * @return {*} either a readable stream or a string
  */
-export async function exportDB(dbName: string, opts: DBDumpOpts = {}) {
+export async function exportDB(
+  dbName: string,
+  opts: DBDumpOpts = {}
+): Promise<DBDumpOpts> {
   const exportOpts = {
     filter: opts?.filter,
     batch_size: 1000,
@@ -178,6 +181,7 @@ export async function exportApp(appId: string, config?: ExportOpts) {
  * Streams a backup of the database state for an app
  * @param {string} appId The ID of the app which is to be backed up.
  * @param {boolean} excludeRows Flag to state whether the export should include data.
+ * @param {string} encryptPassword password for encrypting the export.
  * @returns {*} a readable stream of the backup which is written in real time
  */
 export async function streamExportApp({
diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts
index fc8a2b3e98..24eb1522a7 100644
--- a/packages/server/src/tests/utilities/structures.ts
+++ b/packages/server/src/tests/utilities/structures.ts
@@ -266,7 +266,7 @@ export function basicRole() {
     name: `NewRole_${utils.newid()}`,
     inherits: roles.BUILTIN_ROLE_IDS.BASIC,
     permissionId: permissions.BuiltinPermissionID.READ_ONLY,
-    version: 2,
+    version: "name",
   }
 }
 
diff --git a/packages/types/src/documents/app/role.ts b/packages/types/src/documents/app/role.ts
index 2167041b90..d126a67b16 100644
--- a/packages/types/src/documents/app/role.ts
+++ b/packages/types/src/documents/app/role.ts
@@ -4,5 +4,5 @@ export interface Role extends Document {
   permissionId: string
   inherits?: string
   permissions: { [key: string]: string[] }
-  version?: number
+  version?: string
 }

From fc537a0aabea601058ccfc2e99911f4367542063 Mon Sep 17 00:00:00 2001
From: mike12345567 <me@michaeldrury.co.uk>
Date: Tue, 27 Jun 2023 14:56:24 +0100
Subject: [PATCH 7/7] Further PR comments.

---
 packages/backend-core/src/docIds/ids.ts                   | 7 +++++++
 packages/backend-core/src/security/roles.ts               | 6 +++---
 .../components/backend/DataTable/modals/EditRoles.svelte  | 2 --
 packages/server/src/api/controllers/role.ts               | 8 ++++----
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/packages/backend-core/src/docIds/ids.ts b/packages/backend-core/src/docIds/ids.ts
index 02cf2248e3..e0ac85b3df 100644
--- a/packages/backend-core/src/docIds/ids.ts
+++ b/packages/backend-core/src/docIds/ids.ts
@@ -89,6 +89,13 @@ export function generateRoleID(name: string) {
   return `${prefix}${name}`
 }
 
+/**
+ * Utility function to be more verbose.
+ */
+export function prefixRoleID(name: string) {
+  return generateRoleID(name)
+}
+
 /**
  * Generates a new dev info document ID - this is scoped to a user.
  * @returns {string} The new dev info ID which info for dev (like api key) can be stored under.
diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts
index b43017afb0..cf5c6bc406 100644
--- a/packages/backend-core/src/security/roles.ts
+++ b/packages/backend-core/src/security/roles.ts
@@ -1,5 +1,5 @@
 import { BuiltinPermissionID, PermissionLevel } from "./permissions"
-import { generateRoleID, getRoleParams, DocumentType, SEPARATOR } from "../db"
+import { prefixRoleID, getRoleParams, DocumentType, SEPARATOR } from "../db"
 import { getAppDB } from "../context"
 import { doWithDB } from "../db"
 import { Screen, Role as RoleDoc } from "@budibase/types"
@@ -169,7 +169,7 @@ export async function getRole(
     )
   } else {
     // make sure has the prefix (if it has it then it won't be added)
-    roleId = generateRoleID(roleId)
+    roleId = prefixRoleID(roleId)
   }
   try {
     const db = getAppDB()
@@ -402,7 +402,7 @@ export function getDBRoleID(roleName: string) {
   if (roleName?.startsWith(DocumentType.ROLE)) {
     return roleName
   }
-  return generateRoleID(roleName)
+  return prefixRoleID(roleName)
 }
 
 /**
diff --git a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
index 3f976b40f2..b638d23168 100644
--- a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
+++ b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte
@@ -109,8 +109,6 @@
       return "Select a unique role name."
     } else if (invalidRoleName) {
       return "Please enter a role name consisting of only alphanumeric symbols and underscores"
-    } else {
-      return null
     }
   }
 
diff --git a/packages/server/src/api/controllers/role.ts b/packages/server/src/api/controllers/role.ts
index f772405846..57accfbbdc 100644
--- a/packages/server/src/api/controllers/role.ts
+++ b/packages/server/src/api/controllers/role.ts
@@ -29,10 +29,10 @@ async function updateRolesOnUserTable(
         roleVersion === roles.RoleIDVersion.NAME
           ? 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) {
+      const indexOfRoleId = constraints.inclusion.indexOf(updatedRoleId)
+      if (remove && indexOfRoleId !== -1) {
+        constraints.inclusion.splice(indexOfRoleId, 1)
+      } else if (!remove && indexOfRoleId === -1) {
         constraints.inclusion.push(updatedRoleId)
       }
       break