Fixing issues with builtin roles living in the database as well as in code (easier to change in the future this way) - discovered by basic test case.

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

@@ -3,8 +3,15 @@ const {
   PermissionLevels,
   higherPermission,
 } = require("../../utilities/security/permissions")
+const {
+  isBuiltin,
+  getDBRoleID,
+  getExternalRoleID,
+  BUILTIN_ROLES,
+} = require("../../utilities/security/roles")
 const { getRoleParams } = require("../../db/utils")
 const CouchDB = require("../../db")
+const { cloneDeep } = require("lodash/fp")
 
 const PermissionUpdateType = {
   REMOVE: "remove",
@@ -18,6 +25,8 @@ async function updatePermissionOnRole(
 ) {
   const db = new CouchDB(appId)
   const remove = updateType === PermissionUpdateType.REMOVE
+  const isABuiltin = isBuiltin(roleId)
+  const dbRoleId = getDBRoleID(roleId)
   const body = await db.allDocs(
     getRoleParams(null, {
       include_docs: true,
@@ -26,7 +35,12 @@ async function updatePermissionOnRole(
   const dbRoles = body.rows.map(row => row.doc)
   const docUpdates = []
 
-  // TODO NEED TO HANDLE BUILTINS HERE - THE dbRoles doesn't contain them
+  // the permission is for a built in, make sure it exists
+  if (isABuiltin && !dbRoles.some(role => role._id === dbRoleId)) {
+    const builtin = cloneDeep(BUILTIN_ROLES[roleId])
+    builtin._id = getDBRoleID(builtin._id)
+    dbRoles.push(builtin)
+  }
 
   // now try to find any roles which need updated, e.g. removing the
   // resource from another role and then adding to the new role
@@ -34,18 +48,18 @@ async function updatePermissionOnRole(
     let updated = false
     const rolePermissions = role.permissions ? role.permissions : {}
     // handle the removal/updating the role which has this permission first
-    // the updating (role._id !== roleId) is required because a resource/level can
+    // the updating (role._id !== dbRoleId) is required because a resource/level can
     // only be permitted in a single role (this reduces hierarchy confusion and simplifies
     // the general UI for this, rather than needing to show everywhere it is used)
     if (
-      (role._id !== roleId || remove) &&
+      (role._id !== dbRoleId || remove) &&
       rolePermissions[resourceId] === level
     ) {
       delete rolePermissions[resourceId]
       updated = true
     }
     // handle the adding, we're on the correct role, at it to this
-    if (!remove && role._id === roleId) {
+    if (!remove && role._id === dbRoleId) {
       rolePermissions[resourceId] = level
       updated = true
     }
@@ -56,7 +70,11 @@ async function updatePermissionOnRole(
     }
   }
 
-  return db.bulkDocs(docUpdates)
+  const response = await db.bulkDocs(docUpdates)
+  return response.map(resp => ({
+    ...resp,
+    _id: getExternalRoleID(resp._id),
+  }))
 }
 
 exports.fetchBuiltin = function(ctx) {
@@ -81,8 +99,9 @@ exports.getResourcePerms = async function(ctx) {
   for (let role of roles) {
     // update the various roleIds in the resource permissions
     if (role.permissions && role.permissions[resourceId]) {
-      resourcePerms[role._id] = higherPermission(
-        resourcePerms[role._id],
+      const roleId = getExternalRoleID(role._id)
+      resourcePerms[roleId] = higherPermission(
+        resourcePerms[roleId],
         role.permissions[resourceId]
       )
     }

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

@@ -1,8 +1,11 @@
 const CouchDB = require("../../db")
 const {
   BUILTIN_ROLES,
+  BUILTIN_ROLE_IDS,
   Role,
   getRole,
+  isBuiltin,
+  getExternalRoleID,
 } = require("../../utilities/security/roles")
 const {
   generateRoleID,
@@ -16,6 +19,14 @@ const UpdateRolesOptions = {
   REMOVED: "removed",
 }
 
+// exclude internal roles like builder
+const EXTERNAL_BUILTIN_ROLE_IDS = [
+  BUILTIN_ROLE_IDS.ADMIN,
+  BUILTIN_ROLE_IDS.POWER,
+  BUILTIN_ROLE_IDS.BASIC,
+  BUILTIN_ROLE_IDS.PUBLIC,
+]
+
 async function updateRolesOnUserTable(db, roleId, updateOption) {
   const table = await db.get(ViewNames.USERS)
   const schema = table.schema
@@ -46,16 +57,22 @@ exports.fetch = async function(ctx) {
       include_docs: true,
     })
   )
-  const customRoles = body.rows.map(row => row.doc)
+  const roles = body.rows.map(row => row.doc)
 
-  // exclude internal roles like builder
-  const staticRoles = [
-    BUILTIN_ROLES.ADMIN,
-    BUILTIN_ROLES.POWER,
-    BUILTIN_ROLES.BASIC,
-    BUILTIN_ROLES.PUBLIC,
-  ]
-  ctx.body = [...staticRoles, ...customRoles]
+  // need to combine builtin with any DB record of them (for sake of permissions)
+  for (let builtinRoleId of EXTERNAL_BUILTIN_ROLE_IDS) {
+    const builtinRole = BUILTIN_ROLES[builtinRoleId]
+    const dbBuiltin = roles.filter(
+      dbRole => getExternalRoleID(dbRole._id) === builtinRoleId
+    )[0]
+    if (dbBuiltin == null) {
+      roles.push(builtinRole)
+    } else {
+      dbBuiltin._id = getExternalRoleID(dbBuiltin._id)
+      roles.push(Object.assign(builtinRole, dbBuiltin))
+    }
+  }
+  ctx.body = roles
 }
 
 exports.find = async function(ctx) {
@@ -67,6 +84,8 @@ exports.save = async function(ctx) {
   let { _id, name, inherits, permissionId } = ctx.request.body
   if (!_id) {
     _id = generateRoleID()
+  } else if (isBuiltin(_id)) {
+    ctx.throw(400, "Cannot update builtin roles.")
   }
   const role = new Role(_id, name)
     .addPermission(permissionId)
@@ -84,6 +103,9 @@ exports.save = async function(ctx) {
 exports.destroy = async function(ctx) {
   const db = new CouchDB(ctx.user.appId)
   const roleId = ctx.params.roleId
+  if (isBuiltin(roleId)) {
+    ctx.throw(400, "Cannot delete builtin role.")
+  }
   // first check no users actively attached to role
   const users = (
     await db.allDocs(
@@ -94,7 +116,7 @@ exports.destroy = async function(ctx) {
   ).rows.map(row => row.doc)
   const usersWithRole = users.filter(user => user.roleId === roleId)
   if (usersWithRole.length !== 0) {
-    ctx.throw("Cannot delete role when it is in use.")
+    ctx.throw(400, "Cannot delete role when it is in use.")
   }
 
   await db.remove(roleId, ctx.params.rev)

packages/server/src/api/routes/tests/couchTestUtils.js

@@ -88,6 +88,21 @@ exports.createRole = async (request, appId) => {
   return res.body
 }
 
+exports.addPermission = async (
+  request,
+  appId,
+  role,
+  resource,
+  level = "read"
+) => {
+  const res = await request
+    .post(`/api/permission/${role}/${resource}/${level}`)
+    .set(exports.defaultHeaders(appId))
+    .expect("Content-Type", /json/)
+    .expect(200)
+  return res.body
+}
+
 exports.createLinkedTable = async (request, appId) => {
   // get the ID to link to
   const table = await exports.createTable(request, appId)

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

@@ -3,16 +3,18 @@ const {
   createTable,
   supertest,
   defaultHeaders,
+  addPermission,
 } = require("./couchTestUtils")
 const { BUILTIN_ROLE_IDS } = require("../../../utilities/security/roles")
 
-const STD_ROLE_ID = BUILTIN_ROLE_IDS.BASIC
+const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC
 
 describe("/permission", () => {
   let server
   let request
   let appId
   let table
+  let perms
 
   beforeAll(async () => {
     ;({ request, server } = await supertest())
@@ -26,6 +28,7 @@ describe("/permission", () => {
     let app = await createApplication(request)
     appId = app.instance._id
     table = await createTable(request, appId)
+    perms = await addPermission(request, appId, STD_ROLE_ID, table._id)
   })
 
   describe("levels", () => {
@@ -42,10 +45,15 @@ describe("/permission", () => {
     })
   })
 
-  describe("add", () => {
+  describe("test", () => {
     it("should be able to add permission to a role for the table", async () => {
+      expect(perms.length).toEqual(1)
+      expect(perms[0].id).toEqual(`${STD_ROLE_ID}`)
+    })
+
+    it("should get the resource permissions", async () => {
       const res = await request
-        .post(`/api/permission/${STD_ROLE_ID}/${table._id}/read`)
+        .get(`/api/permission/${table._id}`)
         .set(defaultHeaders(appId))
         .expect("Content-Type", /json/)
         .expect(200)

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

@@ -1,7 +1,5 @@
 const {
   createApplication,
-  createTable,
-  createView,
   supertest,
   defaultHeaders,
 } = require("./couchTestUtils")
@@ -20,8 +18,6 @@ describe("/roles", () => {
   let server
   let request
   let appId
-  let table
-  let view
 
   beforeAll(async () => {
     ;({ request, server } = await supertest())
@@ -34,8 +30,6 @@ describe("/roles", () => {
   beforeEach(async () => {
     let app = await createApplication(request)
     appId = app.instance._id
-    table = await createTable(request, appId)
-    view = await createView(request, appId, table._id)
   })
 
   describe("create", () => {

packages/server/src/db/utils.js

@@ -170,8 +170,8 @@ exports.getAppParams = (appId = null, otherProps = {}) => {
  * Generates a new role ID.
  * @returns {string} The new role ID which the role doc can be stored under.
  */
-exports.generateRoleID = () => {
-  return `${DocumentTypes.ROLE}${SEPARATOR}${newid()}`
+exports.generateRoleID = id => {
+  return `${DocumentTypes.ROLE}${SEPARATOR}${id || newid()}`
 }
 
 /**

packages/server/src/middleware/authorized.js

@@ -64,12 +64,12 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => {
     ctx.throw(403, "Not Authorized")
   }
 
-  if (
-    hasResource(ctx) &&
-    doesHaveResourcePermission(permissions, permLevel, ctx)
-  ) {
-    return next()
-  }
+  // if (
+  //   hasResource(ctx) &&
+  //   doesHaveResourcePermission(permissions, permLevel, ctx)
+  // ) {
+  //   return next()
+  // }
 
   if (!doesHaveBasePermission(permType, permLevel, basePermissions)) {
     ctx.throw(403, "User does not have permission")

packages/server/src/middleware/resourceId.js

@@ -21,13 +21,17 @@ class ResourceIdGetter {
       main = this.main,
       sub = this.sub
     return (ctx, next) => {
-      if (main != null && ctx.request[parameter][main]) {
-        ctx.resourceId = ctx.request[parameter][main]
+      const request = ctx.request[parameter] || ctx[parameter]
+      if (request == null) {
+        return next()
       }
-      if (sub != null && ctx.request[parameter][sub]) {
-        ctx.subResourceId = ctx.request[parameter][sub]
+      if (main != null && request[main]) {
+        ctx.resourceId = request[main]
       }
-      next()
+      if (sub != null && request[sub]) {
+        ctx.subResourceId = request[sub]
+      }
+      return next()
     }
   }
 }

packages/server/src/utilities/security/roles.js

@@ -1,10 +1,7 @@
 const CouchDB = require("../../db")
 const { cloneDeep } = require("lodash/fp")
-const {
-  BUILTIN_PERMISSION_IDS,
-  higherPermission,
-  doesHaveBasePermission,
-} = require("./permissions")
+const { BUILTIN_PERMISSION_IDS, higherPermission } = require("./permissions")
+const { generateRoleID, DocumentTypes, SEPARATOR } = require("../../db/utils")
 
 const BUILTIN_IDS = {
   ADMIN: "ADMIN",
@@ -48,15 +45,15 @@ exports.BUILTIN_ROLES = {
 }
 
 exports.BUILTIN_ROLE_ID_ARRAY = Object.values(exports.BUILTIN_ROLES).map(
-  level => level._id
+  role => role._id
 )
 
 exports.BUILTIN_ROLE_NAME_ARRAY = Object.values(exports.BUILTIN_ROLES).map(
-  level => level.name
+  role => role.name
 )
 
 function isBuiltin(role) {
-  return exports.BUILTIN_ROLE_ID_ARRAY.indexOf(role) !== -1
+  return exports.BUILTIN_ROLE_ID_ARRAY.some(builtin => role.includes(builtin))
 }
 
 /**
@@ -82,6 +79,8 @@ exports.getRole = async (appId, roleId) => {
     const db = new CouchDB(appId)
     const dbRole = await db.get(roleId)
     role = Object.assign(role, dbRole)
+    // finalise the ID
+    role._id = exports.getExternalRoleID(role._id)
   } catch (err) {
     // only throw an error if there is no role at all
     if (Object.keys(role).length === 0) {
@@ -202,6 +201,27 @@ class AccessController {
   }
 }
 
+/**
+ * Adds the "role_" for builtin role IDs which are to be written to the DB (for permissions).
+ */
+exports.getDBRoleID = roleId => {
+  if (roleId.startsWith(DocumentTypes.ROLE)) {
+    return roleId
+  }
+  return generateRoleID(roleId)
+}
+
+/**
+ * Remove the "role_" from builtin role IDs that have been written to the DB (for permissions).
+ */
+exports.getExternalRoleID = roleId => {
+  // for built in roles we want to remove the DB role ID element (role_)
+  if (roleId.startsWith(DocumentTypes.ROLE) && isBuiltin(roleId)) {
+    return roleId.split(`${DocumentTypes.ROLE}${SEPARATOR}`)[1]
+  }
+  return roleId
+}
+
 exports.AccessController = AccessController
 exports.BUILTIN_ROLE_IDS = BUILTIN_IDS
 exports.isBuiltin = isBuiltin