From c8ef404560057603b576e38f1abbc68b7b4506ab Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 13:01:45 +0000 Subject: [PATCH] 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. --- .../server/src/api/controllers/permission.js | 33 +++++++++++---- packages/server/src/api/controllers/role.js | 42 ++++++++++++++----- .../src/api/routes/tests/couchTestUtils.js | 15 +++++++ .../src/api/routes/tests/permissions.spec.js | 14 +++++-- .../server/src/api/routes/tests/role.spec.js | 6 --- packages/server/src/db/utils.js | 4 +- packages/server/src/middleware/authorized.js | 12 +++--- packages/server/src/middleware/resourceId.js | 14 ++++--- .../server/src/utilities/security/roles.js | 36 ++++++++++++---- 9 files changed, 129 insertions(+), 47 deletions(-) diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index 7c2827e551..e395057651 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/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] ) } diff --git a/packages/server/src/api/controllers/role.js b/packages/server/src/api/controllers/role.js index f20ca26dd7..59afcc06de 100644 --- a/packages/server/src/api/controllers/role.js +++ b/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) diff --git a/packages/server/src/api/routes/tests/couchTestUtils.js b/packages/server/src/api/routes/tests/couchTestUtils.js index 898a40bb4f..3d99e16873 100644 --- a/packages/server/src/api/routes/tests/couchTestUtils.js +++ b/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) diff --git a/packages/server/src/api/routes/tests/permissions.spec.js b/packages/server/src/api/routes/tests/permissions.spec.js index 8e9effd408..d4ca9aa8a2 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.js +++ b/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) diff --git a/packages/server/src/api/routes/tests/role.spec.js b/packages/server/src/api/routes/tests/role.spec.js index ed9a0ff694..b86b285dbc 100644 --- a/packages/server/src/api/routes/tests/role.spec.js +++ b/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", () => { diff --git a/packages/server/src/db/utils.js b/packages/server/src/db/utils.js index 4045e63313..5e7e74d711 100644 --- a/packages/server/src/db/utils.js +++ b/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()}` } /** diff --git a/packages/server/src/middleware/authorized.js b/packages/server/src/middleware/authorized.js index b838ef4e29..82ca7f98f2 100644 --- a/packages/server/src/middleware/authorized.js +++ b/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") diff --git a/packages/server/src/middleware/resourceId.js b/packages/server/src/middleware/resourceId.js index e21c7d0c5b..4351901dad 100644 --- a/packages/server/src/middleware/resourceId.js +++ b/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() } } } diff --git a/packages/server/src/utilities/security/roles.js b/packages/server/src/utilities/security/roles.js index c5cf28783d..afc7fd217f 100644 --- a/packages/server/src/utilities/security/roles.js +++ b/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