From 5f582dd1dc9c47dbeea8dda0d856a007cb2d2e67 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 5 Feb 2021 15:58:25 +0000 Subject: [PATCH 01/12] Initial work towards rbac. --- .../backend/DataTable/modals/EditRoles.svelte | 16 ++++---- .../server/src/api/controllers/hosting.js | 1 + .../server/src/api/controllers/permission.js | 25 ++++++++++-- packages/server/src/api/routes/permission.js | 40 ++++++++++++++++++- .../server/src/utilities/builder/hosting.js | 1 + .../src/utilities/security/permissions.js | 1 + .../server/src/utilities/security/roles.js | 15 +++++-- 7 files changed, 83 insertions(+), 16 deletions(-) diff --git a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte index d8f094b3cb..9b82c2047e 100644 --- a/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte +++ b/packages/builder/src/components/backend/DataTable/modals/EditRoles.svelte @@ -6,7 +6,7 @@ import ErrorsBox from "components/common/ErrorsBox.svelte" import { backendUiStore } from "builderStore" - let permissions = [] + let basePermissions = [] let selectedRole = {} let errors = [] let builtInRoles = ["Admin", "Power", "Basic", "Public"] @@ -16,9 +16,9 @@ ) $: isCreating = selectedRoleId == null || selectedRoleId === "" - const fetchPermissions = async () => { - const permissionsResponse = await api.get("/api/permissions") - permissions = await permissionsResponse.json() + const fetchBasePermissions = async () => { + const permissionsResponse = await api.get("/api/permission/builtin") + basePermissions = await permissionsResponse.json() } // Changes the selected role @@ -81,7 +81,7 @@ } } - onMount(fetchPermissions) + onMount(fetchBasePermissions) - {#each permissions as permission} - + {#each basePermissions as basePerm} + {/each} {/if} diff --git a/packages/server/src/api/controllers/hosting.js b/packages/server/src/api/controllers/hosting.js index 1d1884eb52..280d24d378 100644 --- a/packages/server/src/api/controllers/hosting.js +++ b/packages/server/src/api/controllers/hosting.js @@ -14,6 +14,7 @@ exports.fetchInfo = async ctx => { } exports.save = async ctx => { + console.trace("DID A SAVE!") const db = new CouchDB(BUILDER_CONFIG_DB) const { type } = ctx.request.body if (type === HostingTypes.CLOUD && ctx.request.body._rev) { diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index a2715a5363..9ff788b5cc 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -1,6 +1,25 @@ -const { BUILTIN_PERMISSIONS } = require("../../utilities/security/permissions") +const { + BUILTIN_PERMISSIONS, + PermissionLevels, +} = require("../../utilities/security/permissions") -exports.fetch = async function(ctx) { - // TODO: need to build out custom permissions +function updatePermissionOnRole(roleId, permissions, remove = false) { + +} + +exports.fetchBuiltin = function(ctx) { ctx.body = Object.values(BUILTIN_PERMISSIONS) } + +exports.fetchLevels = function(ctx) { + ctx.body = Object.values(PermissionLevels) +} + +exports.addPermission = async function(ctx) { + const permissions = ctx.body.permissions, appId = ctx.appId + updatePermissionOnRole +} + +exports.removePermission = async function(ctx) { + const permissions = ctx.body.permissions, appId = ctx.appId +} diff --git a/packages/server/src/api/routes/permission.js b/packages/server/src/api/routes/permission.js index 9dcec253b3..3dbce73599 100644 --- a/packages/server/src/api/routes/permission.js +++ b/packages/server/src/api/routes/permission.js @@ -1,10 +1,46 @@ const Router = require("@koa/router") const controller = require("../controllers/permission") const authorized = require("../../middleware/authorized") -const { BUILDER } = require("../../utilities/security/permissions") +const { + BUILDER, + PermissionLevels, +} = require("../../utilities/security/permissions") +const Joi = require("joi") +const joiValidator = require("../../middleware/joi-validator") const router = Router() -router.get("/api/permissions", authorized(BUILDER), controller.fetch) +function generateAddValidator() { + const permLevelArray = Object.values(PermissionLevels) + // prettier-ignore + return joiValidator.body(Joi.object({ + permissions: Joi.object() + .pattern(/.*/, [Joi.string().valid(...permLevelArray)]) + .required() + }).unknown(true)) +} + +function generateRemoveValidator() { + // prettier-ignore + return joiValidator.body(Joi.object({ + permissions: Joi.array().items(Joi.string()) + }).unknown(true)) +} + +router + .get("/api/permission/builtin", authorized(BUILDER), controller.fetchBuiltin) + .get("/api/permission/levels", authorized(BUILDER), controller.fetchLevels) + .patch( + "/api/permission/:roleId/add", + authorized(BUILDER), + generateAddValidator(), + controller.addPermission + ) + .patch( + "/api/permission/:roleId/remove", + authorized(BUILDER), + generateRemoveValidator(), + controller.removePermission + ) module.exports = router diff --git a/packages/server/src/utilities/builder/hosting.js b/packages/server/src/utilities/builder/hosting.js index 3c02410afd..24ca76dc3e 100644 --- a/packages/server/src/utilities/builder/hosting.js +++ b/packages/server/src/utilities/builder/hosting.js @@ -23,6 +23,7 @@ exports.HostingTypes = { } exports.getHostingInfo = async () => { + console.trace("DID A GET!") const db = new CouchDB(BUILDER_CONFIG_DB) let doc try { diff --git a/packages/server/src/utilities/security/permissions.js b/packages/server/src/utilities/security/permissions.js index c00f3ce6c7..12010dcc40 100644 --- a/packages/server/src/utilities/security/permissions.js +++ b/packages/server/src/utilities/security/permissions.js @@ -7,6 +7,7 @@ const PermissionLevels = { ADMIN: "admin", } +// these are the global types, that govern the underlying default behaviour const PermissionTypes = { TABLE: "table", USER: "user", diff --git a/packages/server/src/utilities/security/roles.js b/packages/server/src/utilities/security/roles.js index 6b6ec39b24..2379e090fd 100644 --- a/packages/server/src/utilities/security/roles.js +++ b/packages/server/src/utilities/security/roles.js @@ -66,14 +66,23 @@ exports.getRole = async (appId, roleId) => { if (!roleId) { return null } - let role + let role = {} + // 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)) { role = cloneDeep( Object.values(exports.BUILTIN_ROLES).find(role => role._id === roleId) ) - } else { + } + try { const db = new CouchDB(appId) - role = await db.get(roleId) + const dbRole = await db.get(roleId) + role = Object.assign(role, dbRole) + } catch (err) { + // only throw an error if there is no role at all + if (Object.keys(role).length === 0) { + throw err + } } return role } From 39a16b93f88fbc4dcac44e7aa06be2652710ca6f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 5 Feb 2021 18:46:15 +0000 Subject: [PATCH 02/12] Further work, need to have a larger think about the API of this. --- .../server/src/api/controllers/permission.js | 38 +++++++++++++++++-- packages/server/src/api/routes/permission.js | 10 ++--- packages/server/src/api/routes/role.js | 10 ++++- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index 9ff788b5cc..c7ffcda672 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -2,9 +2,34 @@ const { BUILTIN_PERMISSIONS, PermissionLevels, } = require("../../utilities/security/permissions") +const { getRoleParams } = require("../../db/utils") +const CouchDB = require("../../db") -function updatePermissionOnRole(roleId, permissions, remove = false) { +async function updatePermissionOnRole( + appId, + roleId, + permissions, + remove = false +) { + const db = new CouchDB(appId) + const body = await db.allDocs( + getRoleParams(null, { + include_docs: true, + }) + ) + const dbRoles = body.rows.map(row => row.doc) + const docUpdates = [] + // now try to find any roles which need updated, e.g. removing the + // resource from another role and then adding to the new role + for (let role of dbRoles) { + if (role.permissions) { + // TODO + } + } + + // TODO: NEED TO WORK THIS PART OUT + return await db.bulkDocs(docUpdates) } exports.fetchBuiltin = function(ctx) { @@ -16,10 +41,15 @@ exports.fetchLevels = function(ctx) { } exports.addPermission = async function(ctx) { - const permissions = ctx.body.permissions, appId = ctx.appId - updatePermissionOnRole + const appId = ctx.appId, + roleId = ctx.params.roleId, + resourceId = ctx.params.resourceId + ctx.body = await updatePermissionOnRole(appId, roleId, resourceId) } exports.removePermission = async function(ctx) { - const permissions = ctx.body.permissions, appId = ctx.appId + const appId = ctx.appId, + roleId = ctx.params.roleId, + resourceId = ctx.params.resourceId + ctx.body = await updatePermissionOnRole(appId, roleId, resourceId, true) } diff --git a/packages/server/src/api/routes/permission.js b/packages/server/src/api/routes/permission.js index 3dbce73599..aa312d6537 100644 --- a/packages/server/src/api/routes/permission.js +++ b/packages/server/src/api/routes/permission.js @@ -30,16 +30,14 @@ function generateRemoveValidator() { router .get("/api/permission/builtin", authorized(BUILDER), controller.fetchBuiltin) .get("/api/permission/levels", authorized(BUILDER), controller.fetchLevels) - .patch( - "/api/permission/:roleId/add", + .post( + "/api/permission/:roleId/:resourceId", authorized(BUILDER), - generateAddValidator(), controller.addPermission ) - .patch( - "/api/permission/:roleId/remove", + .delete( + "/api/permission/:roleId/:resourceId", authorized(BUILDER), - generateRemoveValidator(), controller.removePermission ) diff --git a/packages/server/src/api/routes/role.js b/packages/server/src/api/routes/role.js index 98ac333e17..760acaa7e7 100644 --- a/packages/server/src/api/routes/role.js +++ b/packages/server/src/api/routes/role.js @@ -1,7 +1,10 @@ const Router = require("@koa/router") const controller = require("../controllers/role") const authorized = require("../../middleware/authorized") -const { BUILDER } = require("../../utilities/security/permissions") +const { + BUILDER, + PermissionLevels, +} = require("../../utilities/security/permissions") const Joi = require("joi") const joiValidator = require("../../middleware/joi-validator") const { @@ -11,12 +14,17 @@ const { const router = Router() function generateValidator() { + const permLevelArray = Object.values(PermissionLevels) // prettier-ignore return joiValidator.body(Joi.object({ _id: Joi.string().optional(), _rev: Joi.string().optional(), name: Joi.string().required(), + // this is the base permission ID (for now a built in) permissionId: Joi.string().valid(...Object.values(BUILTIN_PERMISSION_IDS)).required(), + permissions: Joi.object() + .pattern(/.*/, [Joi.string().valid(...permLevelArray)]) + .optional(), inherits: Joi.string().optional(), }).unknown(true)) } From a55d836c337be06e49822eb99f414b9a28d31b5c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 8 Feb 2021 17:22:07 +0000 Subject: [PATCH 03/12] Adding in resource IDs everywhere they should be accessible. --- .../server/src/api/controllers/permission.js | 82 +++++++++++++++---- packages/server/src/api/controllers/row.js | 4 +- packages/server/src/api/routes/automation.js | 17 +++- packages/server/src/api/routes/permission.js | 32 ++++---- packages/server/src/api/routes/query.js | 9 ++ packages/server/src/api/routes/row.js | 13 ++- packages/server/src/api/routes/table.js | 11 ++- .../server/src/middleware/joi-validator.js | 4 + packages/server/src/middleware/resourceId.js | 55 +++++++++++++ .../src/utilities/security/permissions.js | 26 +++++- 10 files changed, 212 insertions(+), 41 deletions(-) create mode 100644 packages/server/src/middleware/resourceId.js diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index c7ffcda672..cca1e0f696 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -1,17 +1,23 @@ const { BUILTIN_PERMISSIONS, PermissionLevels, + higherPermission, } = require("../../utilities/security/permissions") const { getRoleParams } = require("../../db/utils") const CouchDB = require("../../db") +const PermissionUpdateType = { + REMOVE: "remove", + ADD: "add", +} + async function updatePermissionOnRole( appId, - roleId, - permissions, - remove = false + { roleId, resourceId, level }, + updateType ) { const db = new CouchDB(appId) + const remove = updateType === PermissionUpdateType.REMOVE const body = await db.allDocs( getRoleParams(null, { include_docs: true, @@ -23,13 +29,32 @@ async function updatePermissionOnRole( // now try to find any roles which need updated, e.g. removing the // resource from another role and then adding to the new role for (let role of dbRoles) { - if (role.permissions) { - // TODO + 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 + // 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) && + 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) { + rolePermissions[resourceId] = level + updated = true + } + // handle the update, add it to bulk docs to perform at end + if (updated) { + role.permissions = rolePermissions + docUpdates.push(role) } } - // TODO: NEED TO WORK THIS PART OUT - return await db.bulkDocs(docUpdates) + return db.bulkDocs(docUpdates) } exports.fetchBuiltin = function(ctx) { @@ -37,19 +62,44 @@ exports.fetchBuiltin = function(ctx) { } exports.fetchLevels = function(ctx) { - ctx.body = Object.values(PermissionLevels) + // for now only provide the read/write perms externally + ctx.body = [PermissionLevels.WRITE, PermissionLevels.READ] +} + +exports.getResourcePerms = async function(ctx) { + const resourceId = ctx.params.resourceId + const db = new CouchDB(ctx.appId) + const body = await db.allDocs( + getRoleParams(null, { + include_docs: true, + }) + ) + const roles = body.rows.map(row => row.doc) + const resourcePerms = {} + 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], + role.permissions[resourceId] + ) + } + } + ctx.body = resourcePerms } exports.addPermission = async function(ctx) { - const appId = ctx.appId, - roleId = ctx.params.roleId, - resourceId = ctx.params.resourceId - ctx.body = await updatePermissionOnRole(appId, roleId, resourceId) + ctx.body = await updatePermissionOnRole( + ctx.appId, + ctx.params, + PermissionUpdateType.ADD + ) } exports.removePermission = async function(ctx) { - const appId = ctx.appId, - roleId = ctx.params.roleId, - resourceId = ctx.params.resourceId - ctx.body = await updatePermissionOnRole(appId, roleId, resourceId, true) + ctx.body = await updatePermissionOnRole( + ctx.appId, + ctx.params, + PermissionUpdateType.REMOVE + ) } diff --git a/packages/server/src/api/controllers/row.js b/packages/server/src/api/controllers/row.js index 904dd08ed1..857d1dd2ad 100644 --- a/packages/server/src/api/controllers/row.js +++ b/packages/server/src/api/controllers/row.js @@ -54,7 +54,7 @@ async function findRow(db, appId, tableId, rowId) { exports.patch = async function(ctx) { const appId = ctx.user.appId const db = new CouchDB(appId) - let row = await db.get(ctx.params.id) + let row = await db.get(ctx.params.rowId) const table = await db.get(row.tableId) const patchfields = ctx.request.body @@ -123,7 +123,7 @@ exports.save = async function(ctx) { // if the row obj had an _id then it will have been retrieved const existingRow = ctx.preExisting if (existingRow) { - ctx.params.id = row._id + ctx.params.rowId = row._id await exports.patch(ctx) return } diff --git a/packages/server/src/api/routes/automation.js b/packages/server/src/api/routes/automation.js index 8644c75787..3088bc521f 100644 --- a/packages/server/src/api/routes/automation.js +++ b/packages/server/src/api/routes/automation.js @@ -8,6 +8,7 @@ const { PermissionTypes, } = require("../../utilities/security/permissions") const Joi = require("joi") +const { bodyResource, paramResource } = require("../../middleware/resourceId") const router = Router() @@ -64,9 +65,15 @@ router controller.getDefinitionList ) .get("/api/automations", authorized(BUILDER), controller.fetch) - .get("/api/automations/:id", authorized(BUILDER), controller.find) + .get( + "/api/automations/:id", + paramResource("id"), + authorized(BUILDER), + controller.find + ) .put( "/api/automations", + bodyResource("_id"), authorized(BUILDER), generateValidator(true), controller.update @@ -79,9 +86,15 @@ router ) .post( "/api/automations/:id/trigger", + paramResource("id"), authorized(PermissionTypes.AUTOMATION, PermissionLevels.EXECUTE), controller.trigger ) - .delete("/api/automations/:id/:rev", authorized(BUILDER), controller.destroy) + .delete( + "/api/automations/:id/:rev", + paramResource("id"), + authorized(BUILDER), + controller.destroy + ) module.exports = router diff --git a/packages/server/src/api/routes/permission.js b/packages/server/src/api/routes/permission.js index aa312d6537..1d7c042f99 100644 --- a/packages/server/src/api/routes/permission.js +++ b/packages/server/src/api/routes/permission.js @@ -10,34 +10,36 @@ const joiValidator = require("../../middleware/joi-validator") const router = Router() -function generateAddValidator() { +function generateValidator() { const permLevelArray = Object.values(PermissionLevels) // prettier-ignore - return joiValidator.body(Joi.object({ - permissions: Joi.object() - .pattern(/.*/, [Joi.string().valid(...permLevelArray)]) - .required() - }).unknown(true)) -} - -function generateRemoveValidator() { - // prettier-ignore - return joiValidator.body(Joi.object({ - permissions: Joi.array().items(Joi.string()) + return joiValidator.params(Joi.object({ + level: Joi.string().valid(...permLevelArray).required(), + resourceId: Joi.string(), + roleId: Joi.string(), }).unknown(true)) } router .get("/api/permission/builtin", authorized(BUILDER), controller.fetchBuiltin) .get("/api/permission/levels", authorized(BUILDER), controller.fetchLevels) - .post( - "/api/permission/:roleId/:resourceId", + .get( + "/api/permission/:resourceId", authorized(BUILDER), + controller.getResourcePerms + ) + // adding a specific role/level for the resource overrides the underlying access control + .post( + "/api/permission/:roleId/:resourceId/:level", + authorized(BUILDER), + generateValidator(), controller.addPermission ) + // deleting the level defaults it back the underlying access control for the resource .delete( - "/api/permission/:roleId/:resourceId", + "/api/permission/:roleId/:resourceId/:level", authorized(BUILDER), + generateValidator(), controller.removePermission ) diff --git a/packages/server/src/api/routes/query.js b/packages/server/src/api/routes/query.js index 8a84138af5..ad71a53452 100644 --- a/packages/server/src/api/routes/query.js +++ b/packages/server/src/api/routes/query.js @@ -8,6 +8,11 @@ const { PermissionTypes, } = require("../../utilities/security/permissions") const joiValidator = require("../../middleware/joi-validator") +const { + bodyResource, + bodySubResource, + paramResource, +} = require("../../middleware/resourceId") const router = Router() @@ -50,23 +55,27 @@ router .get("/api/queries", authorized(BUILDER), queryController.fetch) .post( "/api/queries", + bodySubResource("datasourceId", "_id"), authorized(BUILDER), generateQueryValidation(), queryController.save ) .post( "/api/queries/preview", + bodyResource("datasourceId"), authorized(BUILDER), generateQueryPreviewValidation(), queryController.preview ) .post( "/api/queries/:queryId", + paramResource("queryId"), authorized(PermissionTypes.QUERY, PermissionLevels.WRITE), queryController.execute ) .delete( "/api/queries/:queryId/:revId", + paramResource("queryId"), authorized(BUILDER), queryController.destroy ) diff --git a/packages/server/src/api/routes/row.js b/packages/server/src/api/routes/row.js index 63964e3066..494ea61608 100644 --- a/packages/server/src/api/routes/row.js +++ b/packages/server/src/api/routes/row.js @@ -2,6 +2,10 @@ const Router = require("@koa/router") const rowController = require("../controllers/row") const authorized = require("../../middleware/authorized") const usage = require("../../middleware/usageQuota") +const { + paramResource, + paramSubResource, +} = require("../../middleware/resourceId") const { PermissionLevels, PermissionTypes, @@ -12,37 +16,44 @@ const router = Router() router .get( "/api/:tableId/:rowId/enrich", + paramSubResource("tableId", "rowId"), authorized(PermissionTypes.TABLE, PermissionLevels.READ), rowController.fetchEnrichedRow ) .get( "/api/:tableId/rows", + paramResource("tableId"), authorized(PermissionTypes.TABLE, PermissionLevels.READ), rowController.fetchTableRows ) .get( "/api/:tableId/rows/:rowId", + paramSubResource("tableId", "rowId"), authorized(PermissionTypes.TABLE, PermissionLevels.READ), rowController.find ) .post( "/api/:tableId/rows", + paramResource("tableId"), authorized(PermissionTypes.TABLE, PermissionLevels.WRITE), usage, rowController.save ) .patch( - "/api/:tableId/rows/:id", + "/api/:tableId/rows/:rowId", + paramSubResource("tableId", "rowId"), authorized(PermissionTypes.TABLE, PermissionLevels.WRITE), rowController.patch ) .post( "/api/:tableId/rows/validate", + paramResource("tableId"), authorized(PermissionTypes.TABLE, PermissionLevels.WRITE), rowController.validate ) .delete( "/api/:tableId/rows/:rowId/:revId", + paramSubResource("tableId", "rowId"), authorized(PermissionTypes.TABLE, PermissionLevels.WRITE), usage, rowController.destroy diff --git a/packages/server/src/api/routes/table.js b/packages/server/src/api/routes/table.js index ef0eb7caec..da5c753b83 100644 --- a/packages/server/src/api/routes/table.js +++ b/packages/server/src/api/routes/table.js @@ -1,6 +1,7 @@ const Router = require("@koa/router") const tableController = require("../controllers/table") const authorized = require("../../middleware/authorized") +const { paramResource, bodyResource } = require("../../middleware/resourceId") const { BUILDER, PermissionLevels, @@ -13,10 +14,17 @@ router .get("/api/tables", authorized(BUILDER), tableController.fetch) .get( "/api/tables/:id", + paramResource("id"), authorized(PermissionTypes.TABLE, PermissionLevels.READ), tableController.find ) - .post("/api/tables", authorized(BUILDER), tableController.save) + .post( + "/api/tables", + // allows control over updating a table + bodyResource("_id"), + authorized(BUILDER), + tableController.save + ) .post( "/api/tables/csv/validate", authorized(BUILDER), @@ -24,6 +32,7 @@ router ) .delete( "/api/tables/:tableId/:revId", + paramResource("tableId"), authorized(BUILDER), tableController.destroy ) diff --git a/packages/server/src/middleware/joi-validator.js b/packages/server/src/middleware/joi-validator.js index 7ded06fe81..1686b0e727 100644 --- a/packages/server/src/middleware/joi-validator.js +++ b/packages/server/src/middleware/joi-validator.js @@ -22,3 +22,7 @@ function validate(schema, property) { module.exports.body = schema => { return validate(schema, "body") } + +module.exports.params = schema => { + return validate(schema, "params") +} diff --git a/packages/server/src/middleware/resourceId.js b/packages/server/src/middleware/resourceId.js new file mode 100644 index 0000000000..e21c7d0c5b --- /dev/null +++ b/packages/server/src/middleware/resourceId.js @@ -0,0 +1,55 @@ +class ResourceIdGetter { + constructor(ctxProperty) { + this.parameter = ctxProperty + this.main = null + this.sub = null + return this + } + + mainResource(field) { + this.main = field + return this + } + + subResource(field) { + this.sub = field + return this + } + + build() { + const parameter = this.parameter, + main = this.main, + sub = this.sub + return (ctx, next) => { + if (main != null && ctx.request[parameter][main]) { + ctx.resourceId = ctx.request[parameter][main] + } + if (sub != null && ctx.request[parameter][sub]) { + ctx.subResourceId = ctx.request[parameter][sub] + } + next() + } + } +} + +module.exports.paramResource = main => { + return new ResourceIdGetter("params").mainResource(main).build() +} + +module.exports.paramSubResource = (main, sub) => { + return new ResourceIdGetter("params") + .mainResource(main) + .subResource(sub) + .build() +} + +module.exports.bodyResource = main => { + return new ResourceIdGetter("body").mainResource(main).build() +} + +module.exports.bodySubResource = (main, sub) => { + return new ResourceIdGetter("body") + .mainResource(main) + .subResource(sub) + .build() +} diff --git a/packages/server/src/utilities/security/permissions.js b/packages/server/src/utilities/security/permissions.js index 12010dcc40..8e3dc4e831 100644 --- a/packages/server/src/utilities/security/permissions.js +++ b/packages/server/src/utilities/security/permissions.js @@ -30,12 +30,11 @@ function Permission(type, level) { */ function getAllowedLevels(userPermLevel) { switch (userPermLevel) { - case PermissionLevels.READ: - return [PermissionLevels.READ] - case PermissionLevels.WRITE: - return [PermissionLevels.READ, PermissionLevels.WRITE] case PermissionLevels.EXECUTE: return [PermissionLevels.EXECUTE] + case PermissionLevels.READ: + return [PermissionLevels.EXECUTE, PermissionLevels.READ] + case PermissionLevels.WRITE: case PermissionLevels.ADMIN: return [ PermissionLevels.READ, @@ -116,6 +115,25 @@ exports.doesHavePermission = (permType, permLevel, permissionIds) => { return false } +exports.higherPermission = (perm1, perm2) => { + function toNum(perm) { + switch (perm) { + // not everything has execute privileges + case PermissionLevels.EXECUTE: + return 0 + case PermissionLevels.READ: + return 1 + case PermissionLevels.WRITE: + return 2 + case PermissionLevels.ADMIN: + return 3 + default: + return -1 + } + } + return toNum(perm1) > toNum(perm2) ? perm1 : perm2 +} + // utility as a lot of things need simply the builder permission exports.BUILDER = PermissionTypes.BUILDER exports.PermissionTypes = PermissionTypes From 7a65a59c781153e7c1744b85d455c1e1f85e646c Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 8 Feb 2021 17:52:22 +0000 Subject: [PATCH 04/12] Making use of the resourceId in the middleware package. --- packages/server/src/middleware/authorized.js | 23 +++++++++++--- .../src/utilities/security/permissions.js | 30 ++++++++++++++++++- .../server/src/utilities/security/roles.js | 30 ++++++++++++++----- 3 files changed, 71 insertions(+), 12 deletions(-) diff --git a/packages/server/src/middleware/authorized.js b/packages/server/src/middleware/authorized.js index ad2c4344fa..b838ef4e29 100644 --- a/packages/server/src/middleware/authorized.js +++ b/packages/server/src/middleware/authorized.js @@ -1,10 +1,11 @@ const { BUILTIN_ROLE_IDS, - getUserPermissionIds, + getUserPermissions, } = require("../utilities/security/roles") const { PermissionTypes, - doesHavePermission, + doesHaveResourcePermission, + doesHaveBasePermission, } = require("../utilities/security/permissions") const env = require("../environment") const { isAPIKeyValid } = require("../utilities/security/apikey") @@ -14,6 +15,10 @@ const ADMIN_ROLES = [BUILTIN_ROLE_IDS.ADMIN, BUILTIN_ROLE_IDS.BUILDER] const LOCAL_PASS = new RegExp(["webhooks/trigger", "webhooks/schema"].join("|")) +function hasResource(ctx) { + return ctx.resourceId != null +} + module.exports = (permType, permLevel = null) => async (ctx, next) => { // webhooks can pass locally if (!env.CLOUD && LOCAL_PASS.test(ctx.request.url)) { @@ -47,7 +52,10 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => { } const role = ctx.user.role - const permissions = await getUserPermissionIds(ctx.appId, role._id) + const { basePermissions, permissions } = await getUserPermissions( + ctx.appId, + role._id + ) if (ADMIN_ROLES.indexOf(role._id) !== -1) { return next() } @@ -56,7 +64,14 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => { ctx.throw(403, "Not Authorized") } - if (!doesHavePermission(permType, permLevel, permissions)) { + 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/utilities/security/permissions.js b/packages/server/src/utilities/security/permissions.js index 8e3dc4e831..c0bc26cb8f 100644 --- a/packages/server/src/utilities/security/permissions.js +++ b/packages/server/src/utilities/security/permissions.js @@ -97,7 +97,35 @@ exports.BUILTIN_PERMISSIONS = { }, } -exports.doesHavePermission = (permType, permLevel, permissionIds) => { +exports.doesHaveResourcePermission = ( + permissions, + permLevel, + { resourceId, subResourceId } +) => { + // set foundSub to not subResourceId, incase there is no subResource + let foundMain = false, + foundSub = !subResourceId + for (let [resource, level] of Object.entries(permissions)) { + const levels = getAllowedLevels(level) + if (resource === resourceId && levels.indexOf(permLevel) !== -1) { + foundMain = true + } + if ( + subResourceId && + resource === subResourceId && + levels.indexOf(permLevel) !== -1 + ) { + foundSub = true + } + // this will escape if foundMain only when no sub resource + if (foundMain && foundSub) { + break + } + } + return foundMain && foundSub +} + +exports.doesHaveBasePermission = (permType, permLevel, permissionIds) => { const builtins = Object.values(exports.BUILTIN_PERMISSIONS) let permissions = flatten( builtins diff --git a/packages/server/src/utilities/security/roles.js b/packages/server/src/utilities/security/roles.js index 2379e090fd..c5cf28783d 100644 --- a/packages/server/src/utilities/security/roles.js +++ b/packages/server/src/utilities/security/roles.js @@ -1,6 +1,10 @@ const CouchDB = require("../../db") const { cloneDeep } = require("lodash/fp") -const { BUILTIN_PERMISSION_IDS } = require("./permissions") +const { + BUILTIN_PERMISSION_IDS, + higherPermission, + doesHaveBasePermission, +} = require("./permissions") const BUILTIN_IDS = { ADMIN: "ADMIN", @@ -127,14 +131,26 @@ exports.getUserRoleHierarchy = async (appId, userRoleId) => { * Get all of the user permissions which could be found across the role hierarchy * @param appId The ID of the application from which roles should be obtained. * @param userRoleId The user's role ID, this can be found in their access token. - * @returns {Promise} A list of permission IDs these should all be unique. + * @returns {Promise<{basePermissions: string[], permissions: Object}>} the base + * permission IDs as well as any custom resource permissions. */ -exports.getUserPermissionIds = async (appId, userRoleId) => { - return [ - ...new Set( - (await getAllUserRoles(appId, userRoleId)).map(role => role.permissionId) - ), +exports.getUserPermissions = async (appId, userRoleId) => { + const rolesHierarchy = await getAllUserRoles(appId, userRoleId) + const basePermissions = [ + ...new Set(rolesHierarchy.map(role => role.permissionId)), ] + const permissions = {} + for (let role of rolesHierarchy) { + if (role.permissions) { + for (let [resource, level] of Object.entries(role.permissions)) { + permissions[resource] = higherPermission(permissions[resource], level) + } + } + } + return { + basePermissions, + permissions, + } } class AccessController { From c0aaaf00469ef92137dbc842efb6e4497e13d41b Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 8 Feb 2021 18:30:30 +0000 Subject: [PATCH 05/12] Some more work and start of a test case towards resource permissions. --- .../server/src/api/controllers/permission.js | 2 + .../src/api/routes/tests/couchTestUtils.js | 18 +++++++ .../src/api/routes/tests/permissions.spec.js | 54 +++++++++++++++++++ .../server/src/utilities/builder/hosting.js | 1 - 4 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 packages/server/src/api/routes/tests/permissions.spec.js diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index cca1e0f696..7c2827e551 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -26,6 +26,8 @@ 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 + // now try to find any roles which need updated, e.g. removing the // resource from another role and then adding to the new role for (let role of dbRoles) { diff --git a/packages/server/src/api/routes/tests/couchTestUtils.js b/packages/server/src/api/routes/tests/couchTestUtils.js index 59a9b6a7c2..898a40bb4f 100644 --- a/packages/server/src/api/routes/tests/couchTestUtils.js +++ b/packages/server/src/api/routes/tests/couchTestUtils.js @@ -4,6 +4,9 @@ const { BUILTIN_ROLE_IDS } = require("../../../utilities/security/roles") const packageJson = require("../../../../package") const jwt = require("jsonwebtoken") const env = require("../../../environment") +const { + BUILTIN_PERMISSION_IDS, +} = require("../../../utilities/security/permissions") const TEST_CLIENT_ID = "test-client-id" @@ -70,6 +73,21 @@ exports.createTable = async (request, appId, table, removeId = true) => { return res.body } +exports.createRole = async (request, appId) => { + const roleBody = { + name: "NewRole", + inherits: BUILTIN_ROLE_IDS.BASIC, + permissionId: BUILTIN_PERMISSION_IDS.READ_ONLY, + } + const res = await request + .post(`/api/roles`) + .send(roleBody) + .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 new file mode 100644 index 0000000000..8e9effd408 --- /dev/null +++ b/packages/server/src/api/routes/tests/permissions.spec.js @@ -0,0 +1,54 @@ +const { + createApplication, + createTable, + supertest, + defaultHeaders, +} = require("./couchTestUtils") +const { BUILTIN_ROLE_IDS } = require("../../../utilities/security/roles") + +const STD_ROLE_ID = BUILTIN_ROLE_IDS.BASIC + +describe("/permission", () => { + let server + let request + let appId + let table + + beforeAll(async () => { + ;({ request, server } = await supertest()) + }) + + afterAll(() => { + server.close() + }) + + beforeEach(async () => { + let app = await createApplication(request) + appId = app.instance._id + table = await createTable(request, appId) + }) + + describe("levels", () => { + it("should be able to get levels", async () => { + const res = await request + .get(`/api/permission/levels`) + .set(defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body).toBeDefined() + expect(res.body.length).toEqual(2) + expect(res.body).toContain("read") + expect(res.body).toContain("write") + }) + }) + + describe("add", () => { + it("should be able to add permission to a role for the table", async () => { + const res = await request + .post(`/api/permission/${STD_ROLE_ID}/${table._id}/read`) + .set(defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + }) + }) +}) diff --git a/packages/server/src/utilities/builder/hosting.js b/packages/server/src/utilities/builder/hosting.js index 24ca76dc3e..3c02410afd 100644 --- a/packages/server/src/utilities/builder/hosting.js +++ b/packages/server/src/utilities/builder/hosting.js @@ -23,7 +23,6 @@ exports.HostingTypes = { } exports.getHostingInfo = async () => { - console.trace("DID A GET!") const db = new CouchDB(BUILDER_CONFIG_DB) let doc try { From bed26d9b1b507fa5a59c60d25f0a7b04dca5b756 Mon Sep 17 00:00:00 2001 From: Martin McKeaveney Date: Tue, 9 Feb 2021 09:28:37 +0000 Subject: [PATCH 06/12] Update docker-compose.yaml --- hosting/docker-compose.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hosting/docker-compose.yaml b/hosting/docker-compose.yaml index bab7f61df1..e8408d9a7d 100644 --- a/hosting/docker-compose.yaml +++ b/hosting/docker-compose.yaml @@ -84,7 +84,7 @@ services: #- "4369:4369" #- "9100:9100" volumes: - - couchdb_data:/couchdb + - couchdb_data:/opt/couchdb/data couch-init: image: curlimages/curl From 62b2f78421d492707c70183d00d96cbca9aaad35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Keviin=20=C3=85berg=20Kultalahti?= Date: Tue, 9 Feb 2021 13:40:27 +0100 Subject: [PATCH 07/12] fixes multiselect in modals bug by bumping bbui --- packages/builder/package.json | 2 +- packages/builder/yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/builder/package.json b/packages/builder/package.json index bd76d10ee1..09fd4dac48 100644 --- a/packages/builder/package.json +++ b/packages/builder/package.json @@ -63,7 +63,7 @@ } }, "dependencies": { - "@budibase/bbui": "^1.58.2", + "@budibase/bbui": "^1.58.3", "@budibase/client": "^0.7.6", "@budibase/colorpicker": "1.0.1", "@budibase/string-templates": "^0.7.6", diff --git a/packages/builder/yarn.lock b/packages/builder/yarn.lock index 9946326ed4..648ebf1155 100644 --- a/packages/builder/yarn.lock +++ b/packages/builder/yarn.lock @@ -842,10 +842,10 @@ lodash "^4.17.19" to-fast-properties "^2.0.0" -"@budibase/bbui@^1.58.2": - version "1.58.2" - resolved "https://registry.yarnpkg.com/@budibase/bbui/-/bbui-1.58.2.tgz#1b9a5b1bf20597c1ea85ebba69acfec01ef6edce" - integrity sha512-Gn4yCNpoVhtVhRDuWEYdaBK/oAfccTvehywgbyH4sHKaY7aQ7v0679nsJsOHBjNPleKy5YN3ZLhneh5k3F1O2Q== +"@budibase/bbui@^1.58.3": + version "1.58.3" + resolved "https://registry.yarnpkg.com/@budibase/bbui/-/bbui-1.58.3.tgz#86ad6aa68eec7426e1ccdf1f7e7fc957cb57d3a3" + integrity sha512-PpbxfBhVpmP0EO1nPBhrz486EHCIgtJlXELC/ElzjG+FCQZSCvDSM7mq/97FOW35iYdTiQTlwFgOtvOgT1P8IQ== dependencies: markdown-it "^12.0.2" quill "^1.3.7" From cc02397b0149be9cf60c589ae11520ef7127eca4 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 13:01:45 +0000 Subject: [PATCH 08/12] 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 From ec5f9788c5dd5c3459b0e317eff52c78ee0bdbb2 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 13:14:23 +0000 Subject: [PATCH 09/12] Fixing minor bug with permission add. --- packages/server/src/api/controllers/permission.js | 9 +++++---- packages/server/src/api/index.js | 1 + packages/server/src/api/routes/tests/permissions.spec.js | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index e395057651..372e804958 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -71,10 +71,11 @@ async function updatePermissionOnRole( } const response = await db.bulkDocs(docUpdates) - return response.map(resp => ({ - ...resp, - _id: getExternalRoleID(resp._id), - })) + return response.map(resp => { + resp._id = getExternalRoleID(resp.id) + delete resp.id + return resp + }) } exports.fetchBuiltin = function(ctx) { diff --git a/packages/server/src/api/index.js b/packages/server/src/api/index.js index 5b9b51363b..7b063cb522 100644 --- a/packages/server/src/api/index.js +++ b/packages/server/src/api/index.js @@ -47,6 +47,7 @@ router.use(async (ctx, next) => { message: err.message, status: ctx.status, } + console.trace(err) } }) diff --git a/packages/server/src/api/routes/tests/permissions.spec.js b/packages/server/src/api/routes/tests/permissions.spec.js index d4ca9aa8a2..fc5ce23f96 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.js +++ b/packages/server/src/api/routes/tests/permissions.spec.js @@ -48,7 +48,7 @@ describe("/permission", () => { 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}`) + expect(perms[0]._id).toEqual(`${STD_ROLE_ID}`) }) it("should get the resource permissions", async () => { From 9fb4c14835a1d857cb7891e706939b99ceb84702 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 16:01:02 +0000 Subject: [PATCH 10/12] Some more fixes after testing permissions a bit further. --- .../server/src/api/controllers/permission.js | 39 ++++++++++++--- packages/server/src/api/routes/permission.js | 1 + .../src/api/routes/tests/couchTestUtils.js | 16 +++++++ .../src/api/routes/tests/permissions.spec.js | 48 ++++++++++++++++++- packages/server/src/middleware/authorized.js | 12 ++--- 5 files changed, 103 insertions(+), 13 deletions(-) diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index 372e804958..1e6bd1869c 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -18,6 +18,16 @@ const PermissionUpdateType = { ADD: "add", } +// utility function to stop this repetition - permissions always stored under roles +async function getAllDBRoles(db) { + const body = await db.allDocs( + getRoleParams(null, { + include_docs: true, + }) + ) + return body.rows.map(row => row.doc) +} + async function updatePermissionOnRole( appId, { roleId, resourceId, level }, @@ -27,12 +37,7 @@ async function updatePermissionOnRole( const remove = updateType === PermissionUpdateType.REMOVE const isABuiltin = isBuiltin(roleId) const dbRoleId = getDBRoleID(roleId) - const body = await db.allDocs( - getRoleParams(null, { - include_docs: true, - }) - ) - const dbRoles = body.rows.map(row => row.doc) + const dbRoles = await getAllDBRoles(db) const docUpdates = [] // the permission is for a built in, make sure it exists @@ -87,6 +92,28 @@ exports.fetchLevels = function(ctx) { ctx.body = [PermissionLevels.WRITE, PermissionLevels.READ] } +exports.fetch = async function(ctx) { + const db = new CouchDB(ctx.appId) + const roles = await getAllDBRoles(db) + let permissions = {} + // create an object with structure role ID -> resource ID -> level + for (let role of roles) { + if (role.permissions) { + const roleId = getExternalRoleID(role._id) + if (permissions[roleId] == null) { + permissions[roleId] = {} + } + for (let [resource, level] of Object.entries(role.permissions)) { + permissions[roleId][resource] = higherPermission( + permissions[roleId][resource], + level + ) + } + } + } + ctx.body = permissions +} + exports.getResourcePerms = async function(ctx) { const resourceId = ctx.params.resourceId const db = new CouchDB(ctx.appId) diff --git a/packages/server/src/api/routes/permission.js b/packages/server/src/api/routes/permission.js index 1d7c042f99..9cf5ae287e 100644 --- a/packages/server/src/api/routes/permission.js +++ b/packages/server/src/api/routes/permission.js @@ -23,6 +23,7 @@ function generateValidator() { router .get("/api/permission/builtin", authorized(BUILDER), controller.fetchBuiltin) .get("/api/permission/levels", authorized(BUILDER), controller.fetchLevels) + .get("/api/permission", authorized(BUILDER), controller.fetch) .get( "/api/permission/:resourceId", authorized(BUILDER), diff --git a/packages/server/src/api/routes/tests/couchTestUtils.js b/packages/server/src/api/routes/tests/couchTestUtils.js index 3d99e16873..0bc7f90998 100644 --- a/packages/server/src/api/routes/tests/couchTestUtils.js +++ b/packages/server/src/api/routes/tests/couchTestUtils.js @@ -73,6 +73,22 @@ exports.createTable = async (request, appId, table, removeId = true) => { return res.body } +exports.createRow = async (request, appId, tableId, row = null) => { + row = row || { + name: "Test Contact", + description: "original description", + status: "new", + tableId: tableId, + } + const res = await request + .post(`/api/${tableId}/rows`) + .send(row) + .set(exports.defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + return res.body +} + exports.createRole = async (request, appId) => { const roleBody = { name: "NewRole", diff --git a/packages/server/src/api/routes/tests/permissions.spec.js b/packages/server/src/api/routes/tests/permissions.spec.js index fc5ce23f96..e1322b7b13 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.js +++ b/packages/server/src/api/routes/tests/permissions.spec.js @@ -1,12 +1,14 @@ const { createApplication, createTable, + createRow, supertest, defaultHeaders, addPermission, } = require("./couchTestUtils") const { BUILTIN_ROLE_IDS } = require("../../../utilities/security/roles") +const HIGHER_ROLE_ID = BUILTIN_ROLE_IDS.BASIC const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { @@ -15,6 +17,7 @@ describe("/permission", () => { let appId let table let perms + let row beforeAll(async () => { ;({ request, server } = await supertest()) @@ -29,8 +32,17 @@ describe("/permission", () => { appId = app.instance._id table = await createTable(request, appId) perms = await addPermission(request, appId, STD_ROLE_ID, table._id) + row = await createRow(request, appId, table._id) }) + async function getTablePermissions() { + return request + .get(`/api/permission/${table._id}`) + .set(defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + } + describe("levels", () => { it("should be able to get levels", async () => { const res = await request @@ -45,7 +57,7 @@ describe("/permission", () => { }) }) - describe("test", () => { + describe("add", () => { 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}`) @@ -57,6 +69,40 @@ describe("/permission", () => { .set(defaultHeaders(appId)) .expect("Content-Type", /json/) .expect(200) + expect(res.body[STD_ROLE_ID]).toEqual("read") + }) + + it("should get resource permissions with multiple roles", async () => { + perms = await addPermission(request, appId, HIGHER_ROLE_ID, table._id, "write") + const res = await getTablePermissions() + expect(res.body[HIGHER_ROLE_ID]).toEqual("write") + expect(res.body[STD_ROLE_ID]).toEqual("read") + const allRes = await request + .get(`/api/permission`) + .set(defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + expect(allRes.body[HIGHER_ROLE_ID][table._id]).toEqual("write") + expect(allRes.body[STD_ROLE_ID][table._id]).toEqual("read") + }) + }) + + describe("remove", () => { + it("should be able to remove the permission", async () => { + const res = await request + .delete(`/api/permission/${STD_ROLE_ID}/${table._id}/read`) + .set(defaultHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body[0]._id).toEqual(STD_ROLE_ID) + const permsRes = await getTablePermissions() + expect(permsRes.body[STD_ROLE_ID]).toBeUndefined() + }) + }) + + describe("check public user allowed", () => { + it("should be able to read the row", async () => { + // TODO }) }) }) diff --git a/packages/server/src/middleware/authorized.js b/packages/server/src/middleware/authorized.js index 82ca7f98f2..b838ef4e29 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") From 821de55363e771c536a6a85fbaa9efb30801b7df Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 17:24:36 +0000 Subject: [PATCH 11/12] Adding basic permissions test which proves a public user can read from a table, but cannot write. --- .../src/api/routes/tests/couchTestUtils.js | 19 ++++++++++++++-- .../src/api/routes/tests/permissions.spec.js | 19 +++++++++++++++- .../server/src/api/routes/tests/row.spec.js | 8 ++----- packages/server/src/middleware/authorized.js | 22 +++++++++++-------- .../server/src/utilities/security/roles.js | 2 +- 5 files changed, 51 insertions(+), 19 deletions(-) diff --git a/packages/server/src/api/routes/tests/couchTestUtils.js b/packages/server/src/api/routes/tests/couchTestUtils.js index 0bc7f90998..d89dce4087 100644 --- a/packages/server/src/api/routes/tests/couchTestUtils.js +++ b/packages/server/src/api/routes/tests/couchTestUtils.js @@ -40,6 +40,17 @@ exports.defaultHeaders = appId => { return headers } +exports.publicHeaders = appId => { + const headers = { + Accept: "application/json", + } + if (appId) { + headers["x-budibase-app-id"] = appId + } + + return headers +} + exports.BASE_TABLE = { name: "TestTable", type: "table", @@ -73,13 +84,17 @@ exports.createTable = async (request, appId, table, removeId = true) => { return res.body } -exports.createRow = async (request, appId, tableId, row = null) => { - row = row || { +exports.makeBasicRow = tableId => { + return { name: "Test Contact", description: "original description", status: "new", tableId: tableId, } +} + +exports.createRow = async (request, appId, tableId, row = null) => { + row = row || exports.makeBasicRow(tableId) const res = await request .post(`/api/${tableId}/rows`) .send(row) diff --git a/packages/server/src/api/routes/tests/permissions.spec.js b/packages/server/src/api/routes/tests/permissions.spec.js index e1322b7b13..8353eb271d 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.js +++ b/packages/server/src/api/routes/tests/permissions.spec.js @@ -5,6 +5,8 @@ const { supertest, defaultHeaders, addPermission, + publicHeaders, + makeBasicRow, } = require("./couchTestUtils") const { BUILTIN_ROLE_IDS } = require("../../../utilities/security/roles") @@ -102,7 +104,22 @@ describe("/permission", () => { describe("check public user allowed", () => { it("should be able to read the row", async () => { - // TODO + const res = await request + .get(`/api/${table._id}/rows`) + .set(publicHeaders(appId)) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body[0]._id).toEqual(row._id) + }) + + it("shouldn't allow writing from a public user", async () => { + const res = await request + .post(`/api/${table._id}/rows`) + .send(makeBasicRow(table._id)) + .set(publicHeaders(appId)) + .expect("Content-Type", /json/) + .expect(403) + expect(res.status).toEqual(403) }) }) }) diff --git a/packages/server/src/api/routes/tests/row.spec.js b/packages/server/src/api/routes/tests/row.spec.js index 47977ed207..c411016b71 100644 --- a/packages/server/src/api/routes/tests/row.spec.js +++ b/packages/server/src/api/routes/tests/row.spec.js @@ -5,6 +5,7 @@ const { defaultHeaders, createLinkedTable, createAttachmentTable, + makeBasicRow, } = require("./couchTestUtils"); const { enrichRows } = require("../../../utilities") const env = require("../../../environment") @@ -30,12 +31,7 @@ describe("/rows", () => { app = await createApplication(request) appId = app.instance._id table = await createTable(request, appId) - row = { - name: "Test Contact", - description: "original description", - status: "new", - tableId: table._id - } + row = makeBasicRow(table._id) }) const createRow = async r => diff --git a/packages/server/src/middleware/authorized.js b/packages/server/src/middleware/authorized.js index b838ef4e29..7eac602f78 100644 --- a/packages/server/src/middleware/authorized.js +++ b/packages/server/src/middleware/authorized.js @@ -43,12 +43,8 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => { // don't expose builder endpoints in the cloud if (env.CLOUD && permType === PermissionTypes.BUILDER) return - if (!ctx.auth.authenticated) { - ctx.throw(403, "Session not authenticated") - } - if (!ctx.user) { - ctx.throw(403, "User not found") + ctx.throw(403, "No user info found") } const role = ctx.user.role @@ -56,11 +52,15 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => { ctx.appId, role._id ) - if (ADMIN_ROLES.indexOf(role._id) !== -1) { - return next() - } + const isAdmin = ADMIN_ROLES.indexOf(role._id) !== -1 + const isAuthed = ctx.auth.authenticated - if (permType === PermissionTypes.BUILDER) { + // this may need to change in the future, right now only admins + // can have access to builder features, this is hard coded into + // our rules + if (isAdmin && isAuthed) { + return next() + } else if (permType === PermissionTypes.BUILDER) { ctx.throw(403, "Not Authorized") } @@ -71,6 +71,10 @@ module.exports = (permType, permLevel = null) => async (ctx, next) => { return next() } + if (!isAuthed) { + ctx.throw(403, "Session not authenticated") + } + if (!doesHaveBasePermission(permType, permLevel, basePermissions)) { ctx.throw(403, "User does not have permission") } diff --git a/packages/server/src/utilities/security/roles.js b/packages/server/src/utilities/security/roles.js index afc7fd217f..447c049e1d 100644 --- a/packages/server/src/utilities/security/roles.js +++ b/packages/server/src/utilities/security/roles.js @@ -77,7 +77,7 @@ exports.getRole = async (appId, roleId) => { } try { const db = new CouchDB(appId) - const dbRole = await db.get(roleId) + const dbRole = await db.get(exports.getDBRoleID(roleId)) role = Object.assign(role, dbRole) // finalise the ID role._id = exports.getExternalRoleID(role._id) From 182d7200516ae307953410398af02723640f2a0f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 9 Feb 2021 17:43:22 +0000 Subject: [PATCH 12/12] Fixing a minor issue with self hosting deployment/app creation. --- .../server/src/utilities/builder/hosting.js | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/packages/server/src/utilities/builder/hosting.js b/packages/server/src/utilities/builder/hosting.js index 3c02410afd..c265c26dd0 100644 --- a/packages/server/src/utilities/builder/hosting.js +++ b/packages/server/src/utilities/builder/hosting.js @@ -94,17 +94,22 @@ exports.getDeployedApps = async () => { } const workerUrl = !env.CLOUD ? await exports.getWorkerUrl() : env.WORKER_URL const hostingKey = !env.CLOUD ? hostingInfo.selfHostKey : env.HOSTING_KEY - const response = await fetch(`${workerUrl}/api/apps`, { - method: "GET", - headers: { - "x-budibase-auth": hostingKey, - }, - }) - const json = await response.json() - for (let value of Object.values(json)) { - if (value.url) { - value.url = value.url.toLowerCase() + try { + const response = await fetch(`${workerUrl}/api/apps`, { + method: "GET", + headers: { + "x-budibase-auth": hostingKey, + }, + }) + const json = await response.json() + for (let value of Object.values(json)) { + if (value.url) { + value.url = value.url.toLowerCase() + } } + return json + } catch (err) { + // error, cannot determine deployed apps, don't stop app creation - sort this later + return {} } - return json }