Merge pull request #13188 from Budibase/budi-7710-user-groups-do-not-fully-support-custom-roles-4

Type role controller.

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

@@ -84,16 +84,18 @@ export function getBuiltinRoles(): { [key: string]: RoleDoc } {
   return cloneDeep(BUILTIN_ROLES)
 }
 
-export const BUILTIN_ROLE_ID_ARRAY = Object.values(BUILTIN_ROLES).map(
+export function isBuiltin(role: string) {
-  role => role._id
+  return getBuiltinRole(role) !== undefined
-)
+}
 
-export const BUILTIN_ROLE_NAME_ARRAY = Object.values(BUILTIN_ROLES).map(
+export function getBuiltinRole(roleId: string): Role | undefined {
-  role => role.name
+  const role = Object.values(BUILTIN_ROLES).find(role =>
-)
+    roleId.includes(role._id)
-
+  )
-export function isBuiltin(role?: string) {
+  if (!role) {
-  return BUILTIN_ROLE_ID_ARRAY.some(builtin => role?.includes(builtin))
+    return undefined
+  }
+  return cloneDeep(role)
 }
 
 /**
@@ -123,7 +125,7 @@ export function builtinRoleToNumber(id?: string) {
 /**
  * Converts any role to a number, but has to be async to get the roles from db.
  */
-export async function roleToNumber(id?: string) {
+export async function roleToNumber(id: string) {
   if (isBuiltin(id)) {
     return builtinRoleToNumber(id)
   }
@@ -131,7 +133,7 @@ export async function roleToNumber(id?: string) {
     defaultPublic: true,
   })) as RoleDoc[]
   for (let role of hierarchy) {
-    if (isBuiltin(role?.inherits)) {
+    if (role?.inherits && isBuiltin(role.inherits)) {
       return builtinRoleToNumber(role.inherits) + 1
     }
   }
@@ -161,35 +163,28 @@ export function lowerBuiltinRoleID(roleId1?: string, roleId2?: string): string {
  * @returns The role object, which may contain an "inherits" property.
  */
 export async function getRole(
-  roleId?: string,
+  roleId: string,
   opts?: { defaultPublic?: boolean }
-): Promise<RoleDoc | undefined> {
+): Promise<RoleDoc> {
-  if (!roleId) {
-    return undefined
-  }
-  let role: any = {}
   // built in roles mostly come from the in-code implementation,
   // but can be extended by a doc stored about them (e.g. permissions)
-  if (isBuiltin(roleId)) {
+  let role: RoleDoc | undefined = getBuiltinRole(roleId)
-    role = cloneDeep(
+  if (!role) {
-      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 = prefixRoleID(roleId)
   }
   try {
     const db = getAppDB()
-    const dbRole = await db.get(getDBRoleID(roleId))
+    const dbRole = await db.get<RoleDoc>(getDBRoleID(roleId))
-    role = Object.assign(role, dbRole)
+    role = Object.assign(role || {}, dbRole)
     // finalise the ID
-    role._id = getExternalRoleID(role._id, role.version)
+    role._id = getExternalRoleID(role._id!, role.version)
   } catch (err) {
     if (!isBuiltin(roleId) && opts?.defaultPublic) {
       return cloneDeep(BUILTIN_ROLES.PUBLIC)
     }
     // only throw an error if there is no role at all
-    if (Object.keys(role).length === 0) {
+    if (!role || Object.keys(role).length === 0) {
       throw err
     }
   }
@@ -200,7 +195,7 @@ export async function getRole(
  * Simple function to get all the roles based on the top level user role ID.
  */
 async function getAllUserRoles(
-  userRoleId?: string,
+  userRoleId: string,
   opts?: { defaultPublic?: boolean }
 ): Promise<RoleDoc[]> {
   // admins have access to all roles
@@ -226,7 +221,7 @@ async function getAllUserRoles(
 }
 
 export async function getUserRoleIdHierarchy(
-  userRoleId?: string
+  userRoleId: string
 ): Promise<string[]> {
   const roles = await getUserRoleHierarchy(userRoleId)
   return roles.map(role => role._id!)
@@ -241,7 +236,7 @@ export async function getUserRoleIdHierarchy(
  * highest level of access and the last being the lowest level.
  */
 export async function getUserRoleHierarchy(
-  userRoleId?: string,
+  userRoleId: string,
   opts?: { defaultPublic?: boolean }
 ) {
   // special case, if they don't have a role then they are a public user
@@ -265,9 +260,9 @@ export function checkForRoleResourceArray(
   return rolePerms
 }
 
-export async function getAllRoleIds(appId?: string) {
+export async function getAllRoleIds(appId: string): Promise<string[]> {
   const roles = await getAllRoles(appId)
-  return roles.map(role => role._id)
+  return roles.map(role => role._id!)
 }
 
 /**

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

@@ -7,8 +7,14 @@ import {
 } from "@budibase/backend-core"
 import { getUserMetadataParams, InternalTables } from "../../db/utils"
 import {
+  AccessibleRolesResponse,
   Database,
+  DestroyRoleResponse,
+  FetchRolesResponse,
+  FindRoleResponse,
   Role,
+  SaveRoleRequest,
+  SaveRoleResponse,
   UserCtx,
   UserMetadata,
   UserRoles,
@@ -25,43 +31,36 @@ async function updateRolesOnUserTable(
   db: Database,
   roleId: string,
   updateOption: string,
-  roleVersion: string | undefined
+  roleVersion?: string
 ) {
   const table = await sdk.tables.getTable(InternalTables.USER_METADATA)
-  const schema = table.schema
+  const constraints = table.schema.roleId?.constraints
+  if (!constraints) {
+    return
+  }
+  const updatedRoleId =
+    roleVersion === roles.RoleIDVersion.NAME
+      ? roles.getExternalRoleID(roleId, roleVersion)
+      : roleId
+  const indexOfRoleId = constraints.inclusion!.indexOf(updatedRoleId)
   const remove = updateOption === UpdateRolesOptions.REMOVED
-  let updated = false
+  if (remove && indexOfRoleId !== -1) {
-  for (let prop of Object.keys(schema)) {
+    constraints.inclusion!.splice(indexOfRoleId, 1)
-    if (prop === "roleId") {
+  } else if (!remove && indexOfRoleId === -1) {
-      updated = true
+    constraints.inclusion!.push(updatedRoleId)
-      const constraints = schema[prop].constraints!
-      const updatedRoleId =
-        roleVersion === roles.RoleIDVersion.NAME
-          ? roles.getExternalRoleID(roleId, roleVersion)
-          : roleId
-      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
-    }
-  }
-  if (updated) {
-    await db.put(table)
   }
+  await db.put(table)
 }
 
-export async function fetch(ctx: UserCtx) {
+export async function fetch(ctx: UserCtx<void, FetchRolesResponse>) {
   ctx.body = await roles.getAllRoles()
 }
 
-export async function find(ctx: UserCtx) {
+export async function find(ctx: UserCtx<void, FindRoleResponse>) {
   ctx.body = await roles.getRole(ctx.params.roleId)
 }
 
-export async function save(ctx: UserCtx) {
+export async function save(ctx: UserCtx<SaveRoleRequest, SaveRoleResponse>) {
   const db = context.getAppDB()
   let { _id, name, inherits, permissionId, version } = ctx.request.body
   let isCreate = false
@@ -109,9 +108,9 @@ export async function save(ctx: UserCtx) {
   ctx.body = role
 }
 
-export async function destroy(ctx: UserCtx) {
+export async function destroy(ctx: UserCtx<void, DestroyRoleResponse>) {
   const db = context.getAppDB()
-  let roleId = ctx.params.roleId
+  let roleId = ctx.params.roleId as string
   if (roles.isBuiltin(roleId)) {
     ctx.throw(400, "Cannot delete builtin role.")
   } else {
@@ -144,14 +143,18 @@ export async function destroy(ctx: UserCtx) {
   ctx.status = 200
 }
 
-export async function accessible(ctx: UserCtx) {
+export async function accessible(ctx: UserCtx<void, AccessibleRolesResponse>) {
   let roleId = ctx.user?.roleId
   if (!roleId) {
     roleId = roles.BUILTIN_ROLE_IDS.PUBLIC
   }
   if (ctx.user && sharedSdk.users.isAdminOrBuilder(ctx.user)) {
     const appId = context.getAppId()
-    ctx.body = await roles.getAllRoleIds(appId)
+    if (!appId) {
+      ctx.body = []
+    } else {
+      ctx.body = await roles.getAllRoleIds(appId)
+    }
   } else {
     ctx.body = await roles.getUserRoleIdHierarchy(roleId!)
   }

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

@@ -63,7 +63,7 @@ export async function fetch(ctx: UserCtx) {
 export async function clientFetch(ctx: UserCtx) {
   const routing = await getRoutingStructure()
   let roleId = ctx.user?.role?._id
-  const roleIds = await roles.getUserRoleIdHierarchy(roleId)
+  const roleIds = roleId ? await roles.getUserRoleIdHierarchy(roleId) : []
   for (let topLevel of Object.values(routing.routes) as any) {
     for (let subpathKey of Object.keys(topLevel.subpaths)) {
       let found = false

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

@@ -251,10 +251,15 @@ describe("/applications", () => {
 
   describe("permissions", () => {
     it("should only return apps a user has access to", async () => {
-      const user = await config.createUser()
+      const user = await config.createUser({
+        builder: { global: false },
+        admin: { global: false },
+      })
 
-      const apps = await config.api.application.fetch()
+      await config.withUser(user, async () => {
-      expect(apps.length).toBeGreaterThan(0)
+        const apps = await config.api.application.fetch()
+        expect(apps).toHaveLength(0)
+      })
     })
   })
 })

packages/server/src/api/routes/tests/utilities/index.ts

@@ -1,5 +1,4 @@
-import TestConfig from "../../../../tests/utilities/TestConfiguration"
+import TestConfiguration from "../../../../tests/utilities/TestConfiguration"
-import env from "../../../../environment"
 import supertest from "supertest"
 
 export * as structures from "../../../../tests/utilities/structures"
@@ -47,10 +46,10 @@ export function delay(ms: number) {
 }
 
 let request: supertest.SuperTest<supertest.Test> | undefined | null,
-  config: TestConfig | null
+  config: TestConfiguration | null
 
 export function beforeAll() {
-  config = new TestConfig()
+  config = new TestConfiguration()
   request = config.getRequest()
 }
 

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

@@ -299,6 +299,16 @@ export default class TestConfiguration {
     }
   }
 
+  withUser(user: User, f: () => Promise<void>) {
+    const oldUser = this.user
+    this.user = user
+    try {
+      return f()
+    } finally {
+      this.user = oldUser
+    }
+  }
+
   // UTILS
 
   _req<Req extends Record<string, any> | void, Res>(

packages/types/src/api/web/index.ts

@@ -14,3 +14,4 @@ export * from "./cookies"
 export * from "./automation"
 export * from "./layout"
 export * from "./query"
+export * from "./role"

packages/types/src/api/web/role.ts

@@ -0,0 +1,22 @@
+import { Role } from "../../documents"
+
+export interface SaveRoleRequest {
+  _id?: string
+  _rev?: string
+  name: string
+  inherits: string
+  permissionId: string
+  version: string
+}
+
+export interface SaveRoleResponse extends Role {}
+
+export interface FindRoleResponse extends Role {}
+
+export type FetchRolesResponse = Role[]
+
+export interface DestroyRoleResponse {
+  message: string
+}
+
+export type AccessibleRolesResponse = string[]