From 649a33d3c8f800233b45934467cd1db020fcab2e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 23 Sep 2021 19:04:53 +0100 Subject: [PATCH 1/5] Getting rid of the concept of permissions hierarchy, roles still have a hierarchy and base permissions still follow the old system, but resources can be given a stack of separate permissions which don't override each other. --- packages/auth/src/security/permissions.js | 7 +- packages/auth/src/security/roles.js | 15 +++- .../server/src/api/controllers/permission.js | 68 ++++++++----------- .../server/src/api/routes/tests/role.spec.js | 2 +- packages/server/src/utilities/index.js | 8 +++ 5 files changed, 50 insertions(+), 50 deletions(-) diff --git a/packages/auth/src/security/permissions.js b/packages/auth/src/security/permissions.js index 03fa5fa562..d0308d783e 100644 --- a/packages/auth/src/security/permissions.js +++ b/packages/auth/src/security/permissions.js @@ -139,8 +139,7 @@ exports.doesHaveResourcePermission = ( // set foundSub to not subResourceId, incase there is no subResource let foundMain = false, foundSub = false - for (let [resource, level] of Object.entries(permissions)) { - const levels = getAllowedLevels(level) + for (let [resource, levels] of Object.entries(permissions)) { if (resource === resourceId && levels.indexOf(permLevel) !== -1) { foundMain = true } @@ -177,10 +176,6 @@ exports.doesHaveBasePermission = (permType, permLevel, permissionIds) => { return false } -exports.higherPermission = (perm1, perm2) => { - return levelToNumber(perm1) > levelToNumber(perm2) ? perm1 : perm2 -} - exports.isPermissionLevelHigherThanRead = level => { return levelToNumber(level) > 1 } diff --git a/packages/auth/src/security/roles.js b/packages/auth/src/security/roles.js index baa8fc40dc..71fbc10132 100644 --- a/packages/auth/src/security/roles.js +++ b/packages/auth/src/security/roles.js @@ -1,6 +1,6 @@ const { getDB } = require("../db") const { cloneDeep } = require("lodash/fp") -const { BUILTIN_PERMISSION_IDS, higherPermission } = require("./permissions") +const { BUILTIN_PERMISSION_IDS } = require("./permissions") const { generateRoleID, getRoleParams, @@ -193,8 +193,17 @@ exports.getUserPermissions = async (appId, userRoleId) => { 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) + for (let [resource, levels] of Object.entries(role.permissions)) { + if (!permissions[resource]) { + permissions[resource] = [] + } + const permsSet = new Set(permissions[resource]) + if (Array.isArray(levels)) { + levels.forEach(level => permsSet.add(level)) + } else { + permsSet.add(levels) + } + permissions[resource] = [...permsSet] } } } diff --git a/packages/server/src/api/controllers/permission.js b/packages/server/src/api/controllers/permission.js index e269f8c41d..6c02663649 100644 --- a/packages/server/src/api/controllers/permission.js +++ b/packages/server/src/api/controllers/permission.js @@ -1,9 +1,4 @@ -const { - getBuiltinPermissions, - PermissionLevels, - isPermissionLevelHigherThanRead, - higherPermission, -} = require("@budibase/auth/permissions") +const { getBuiltinPermissions } = require("@budibase/auth/permissions") const { isBuiltin, getDBRoleID, @@ -16,6 +11,7 @@ const { CURRENTLY_SUPPORTED_LEVELS, getBasePermissions, } = require("../../utilities/security") +const { removeFromArray } = require("../../utilities") const PermissionUpdateType = { REMOVE: "remove", @@ -24,22 +20,6 @@ const PermissionUpdateType = { const SUPPORTED_LEVELS = CURRENTLY_SUPPORTED_LEVELS -// quick function to perform a bit of weird logic, make sure fetch calls -// always say a write role also has read permission -function fetchLevelPerms(permissions, level, roleId) { - if (!permissions) { - permissions = {} - } - permissions[level] = roleId - if ( - isPermissionLevelHigherThanRead(level) && - !permissions[PermissionLevels.READ] - ) { - permissions[PermissionLevels.READ] = roleId - } - return permissions -} - // utility function to stop this repetition - permissions always stored under roles async function getAllDBRoles(db) { const body = await db.allDocs( @@ -74,23 +54,31 @@ async function updatePermissionOnRole( for (let role of dbRoles) { let updated = false const rolePermissions = role.permissions ? role.permissions : {} + // make sure its an array, also handle migrating + if ( + !rolePermissions[resourceId] || + !Array.isArray(rolePermissions[resourceId]) + ) { + rolePermissions[resourceId] = + typeof rolePermissions[resourceId] === "string" + ? [rolePermissions[resourceId]] + : [] + } // handle the removal/updating the role which has this permission first // 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 !== dbRoleId || remove) && - rolePermissions[resourceId] === level + rolePermissions[resourceId].indexOf(level) !== -1 ) { - delete rolePermissions[resourceId] + removeFromArray(rolePermissions[resourceId], level) updated = true } // handle the adding, we're on the correct role, at it to this if (!remove && role._id === dbRoleId) { - rolePermissions[resourceId] = higherPermission( - rolePermissions[resourceId], - level - ) + const set = new Set(rolePermissions[resourceId]) + rolePermissions[resourceId] = [...set.add(level)] updated = true } // handle the update, add it to bulk docs to perform at end @@ -127,12 +115,11 @@ exports.fetch = async function (ctx) { continue } const roleId = getExternalRoleID(role._id) - for (let [resource, level] of Object.entries(role.permissions)) { - permissions[resource] = fetchLevelPerms( - permissions[resource], - level, - roleId - ) + for (let [resource, levelArr] of Object.entries(role.permissions)) { + const levels = Array.isArray(levelArr) ? [levelArr] : levelArr + const perms = {} + levels.forEach(level => (perms[level] = roleId)) + permissions[resource] = perms } } // apply the base permissions @@ -157,12 +144,13 @@ exports.getResourcePerms = async function (ctx) { for (let level of SUPPORTED_LEVELS) { // update the various roleIds in the resource permissions for (let role of roles) { - if (role.permissions && role.permissions[resourceId] === level) { - permissions = fetchLevelPerms( - permissions, - level, - getExternalRoleID(role._id) - ) + const rolePerms = role.permissions + if ( + rolePerms && + (rolePerms[resourceId] === level || + rolePerms[resourceId].indexOf(level) !== -1) + ) { + permissions[level] = getExternalRoleID(role._id) } } } diff --git a/packages/server/src/api/routes/tests/role.spec.js b/packages/server/src/api/routes/tests/role.spec.js index ad42ef180a..d74a84b2b2 100644 --- a/packages/server/src/api/routes/tests/role.spec.js +++ b/packages/server/src/api/routes/tests/role.spec.js @@ -72,7 +72,7 @@ describe("/roles", () => { .expect(200) expect(res.body.length).toBeGreaterThan(0) const power = res.body.find(role => role._id === BUILTIN_ROLE_IDS.POWER) - expect(power.permissions[table._id]).toEqual("read") + expect(power.permissions[table._id]).toEqual(["read"]) }) }) diff --git a/packages/server/src/utilities/index.js b/packages/server/src/utilities/index.js index a81f9ddcf5..e24d04c581 100644 --- a/packages/server/src/utilities/index.js +++ b/packages/server/src/utilities/index.js @@ -10,6 +10,14 @@ exports.wait = ms => new Promise(resolve => setTimeout(resolve, ms)) exports.isDev = env.isDev +exports.removeFromArray = (array, element) => { + const index = array.indexOf(element) + if (index !== -1) { + array.splice(index, 1) + } + return array +} + /** * Makes sure that a URL has the correct number of slashes, while maintaining the * http(s):// double slashes. From 1f474ca10675089750f8ae73f5674bff84756b18 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 24 Sep 2021 18:10:30 +0100 Subject: [PATCH 2/5] Minimal fix for issue involving JSON views and newlines with postgres, this fix couldn't be more over-arching as it risked breaking new lines across the board. Have included a script for setting up the test scenario as well. This fixes issue #2612. --- .../integrations/pg-json/docker-compose.yml | 28 +++++++++++++++++++ .../scripts/integrations/pg-json/init.sql | 22 +++++++++++++++ .../scripts/integrations/pg-json/reset.sh | 3 ++ packages/server/src/integrations/postgres.ts | 12 ++++++++ packages/server/src/utilities/index.js | 11 ++++++++ 5 files changed, 76 insertions(+) create mode 100644 packages/server/scripts/integrations/pg-json/docker-compose.yml create mode 100644 packages/server/scripts/integrations/pg-json/init.sql create mode 100755 packages/server/scripts/integrations/pg-json/reset.sh diff --git a/packages/server/scripts/integrations/pg-json/docker-compose.yml b/packages/server/scripts/integrations/pg-json/docker-compose.yml new file mode 100644 index 0000000000..6bc307a86d --- /dev/null +++ b/packages/server/scripts/integrations/pg-json/docker-compose.yml @@ -0,0 +1,28 @@ +version: "3.8" +services: + db: + container_name: postgres-json + image: postgres + restart: always + environment: + POSTGRES_USER: root + POSTGRES_PASSWORD: root + POSTGRES_DB: main + ports: + - "5432:5432" + volumes: + #- pg_data:/var/lib/postgresql/data/ + - ./init.sql:/docker-entrypoint-initdb.d/init.sql + + pgadmin: + container_name: pgadmin-json + image: dpage/pgadmin4 + restart: always + environment: + PGADMIN_DEFAULT_EMAIL: root@root.com + PGADMIN_DEFAULT_PASSWORD: root + ports: + - "5050:80" + +#volumes: +# pg_data: diff --git a/packages/server/scripts/integrations/pg-json/init.sql b/packages/server/scripts/integrations/pg-json/init.sql new file mode 100644 index 0000000000..06a5b4901d --- /dev/null +++ b/packages/server/scripts/integrations/pg-json/init.sql @@ -0,0 +1,22 @@ +SELECT 'CREATE DATABASE main' +WHERE NOT EXISTS (SELECT FROM pg_database WHERE datname = 'main')\gexec +CREATE TABLE jsonTable ( + id character varying(32), + data jsonb, + text text +); + +INSERT INTO jsonTable (id, data) VALUES ('1', '{"id": 1, "age": 1, "name": "Mike", "newline": "this is text with a\n newline in it"}'); + +CREATE VIEW jsonView AS SELECT + x.id, + x.age, + x.name, + x.newline +FROM + jsonTable c, + LATERAL jsonb_to_record(c.data) x (id character varying(32), + age BIGINT, + name TEXT, + newline TEXT + ); diff --git a/packages/server/scripts/integrations/pg-json/reset.sh b/packages/server/scripts/integrations/pg-json/reset.sh new file mode 100755 index 0000000000..32778bd11f --- /dev/null +++ b/packages/server/scripts/integrations/pg-json/reset.sh @@ -0,0 +1,3 @@ +#!/bin/bash +docker-compose down +docker volume prune -f diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts index 23a8685648..15a500b67e 100644 --- a/packages/server/src/integrations/postgres.ts +++ b/packages/server/src/integrations/postgres.ts @@ -13,6 +13,9 @@ module PostgresModule { const Sql = require("./base/sql") const { FieldTypes } = require("../constants") const { buildExternalTableId, convertType, copyExistingPropsOver } = require("./utils") + const { escapeDangerousCharacters } = require("../utilities") + + const JSON_REGEX = /'{.*}'::json/s interface PostgresConfig { host: string @@ -94,6 +97,15 @@ module PostgresModule { } async function internalQuery(client: any, query: SqlQuery) { + // need to handle a specific issue with json data types in postgres, + // new lines inside the JSON data will break it + const matches = query.sql.match(JSON_REGEX) + if (matches && matches.length > 0) { + for (let match of matches) { + const escaped = escapeDangerousCharacters(match) + query.sql = query.sql.replace(match, escaped) + } + } try { return await client.query(query.sql, query.bindings || []) } catch (err) { diff --git a/packages/server/src/utilities/index.js b/packages/server/src/utilities/index.js index a81f9ddcf5..aac3610515 100644 --- a/packages/server/src/utilities/index.js +++ b/packages/server/src/utilities/index.js @@ -106,3 +106,14 @@ exports.deleteEntityMetadata = async (appId, type, entityId) => { await db.remove(id, rev) } } + +exports.escapeDangerousCharacters = string => { + return string + .replace(/[\\]/g, "\\\\") + .replace(/[\/]/g, "\\/") + .replace(/[\b]/g, "\\b") + .replace(/[\f]/g, "\\f") + .replace(/[\n]/g, "\\n") + .replace(/[\r]/g, "\\r") + .replace(/[\t]/g, "\\t") +} From 02a3b7425e8f2de760f77c53062afcc38b5031d3 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 27 Sep 2021 12:17:59 +0100 Subject: [PATCH 3/5] Removing useless statement. --- packages/server/src/utilities/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/utilities/index.js b/packages/server/src/utilities/index.js index aac3610515..3aa43976e1 100644 --- a/packages/server/src/utilities/index.js +++ b/packages/server/src/utilities/index.js @@ -110,7 +110,6 @@ exports.deleteEntityMetadata = async (appId, type, entityId) => { exports.escapeDangerousCharacters = string => { return string .replace(/[\\]/g, "\\\\") - .replace(/[\/]/g, "\\/") .replace(/[\b]/g, "\\b") .replace(/[\f]/g, "\\f") .replace(/[\n]/g, "\\n") From ba2f6ee1202e7f97c15ea225294add65b4c4e971 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Mon, 27 Sep 2021 13:17:31 +0100 Subject: [PATCH 4/5] Fixing issue discovered by test case. --- packages/server/src/integrations/postgres.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts index 15a500b67e..e06e3936c8 100644 --- a/packages/server/src/integrations/postgres.ts +++ b/packages/server/src/integrations/postgres.ts @@ -99,11 +99,13 @@ module PostgresModule { async function internalQuery(client: any, query: SqlQuery) { // need to handle a specific issue with json data types in postgres, // new lines inside the JSON data will break it - const matches = query.sql.match(JSON_REGEX) - if (matches && matches.length > 0) { - for (let match of matches) { - const escaped = escapeDangerousCharacters(match) - query.sql = query.sql.replace(match, escaped) + if (query && query.sql) { + const matches = query.sql.match(JSON_REGEX) + if (matches && matches.length > 0) { + for (let match of matches) { + const escaped = escapeDangerousCharacters(match) + query.sql = query.sql.replace(match, escaped) + } } } try { From 4f4e9a9d2b35ed78ce51c2ccfd71937f409f1431 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Mon, 27 Sep 2021 14:25:00 +0000 Subject: [PATCH 5/5] v0.9.140-alpha.12 --- lerna.json | 2 +- packages/auth/package.json | 2 +- packages/bbui/package.json | 2 +- packages/builder/package.json | 8 ++++---- packages/cli/package.json | 2 +- packages/client/package.json | 6 +++--- packages/server/package.json | 8 ++++---- packages/string-templates/package.json | 2 +- packages/worker/package.json | 6 +++--- 9 files changed, 19 insertions(+), 19 deletions(-) diff --git a/lerna.json b/lerna.json index e4dd4f4661..f3ea43dcc1 100644 --- a/lerna.json +++ b/lerna.json @@ -1,5 +1,5 @@ { - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "npmClient": "yarn", "packages": [ "packages/*" diff --git a/packages/auth/package.json b/packages/auth/package.json index 3be63010df..d42cb7b2c3 100644 --- a/packages/auth/package.json +++ b/packages/auth/package.json @@ -1,6 +1,6 @@ { "name": "@budibase/auth", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "description": "Authentication middlewares for budibase builder and apps", "main": "src/index.js", "author": "Budibase", diff --git a/packages/bbui/package.json b/packages/bbui/package.json index 23f75a9565..e9aba805a9 100644 --- a/packages/bbui/package.json +++ b/packages/bbui/package.json @@ -1,7 +1,7 @@ { "name": "@budibase/bbui", "description": "A UI solution used in the different Budibase projects.", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "license": "AGPL-3.0", "svelte": "src/index.js", "module": "dist/bbui.es.js", diff --git a/packages/builder/package.json b/packages/builder/package.json index 4c0c6f1248..67ea2da45a 100644 --- a/packages/builder/package.json +++ b/packages/builder/package.json @@ -1,6 +1,6 @@ { "name": "@budibase/builder", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "license": "AGPL-3.0", "private": true, "scripts": { @@ -65,10 +65,10 @@ } }, "dependencies": { - "@budibase/bbui": "^0.9.140-alpha.11", - "@budibase/client": "^0.9.140-alpha.11", + "@budibase/bbui": "^0.9.140-alpha.12", + "@budibase/client": "^0.9.140-alpha.12", "@budibase/colorpicker": "1.1.2", - "@budibase/string-templates": "^0.9.140-alpha.11", + "@budibase/string-templates": "^0.9.140-alpha.12", "@sentry/browser": "5.19.1", "@spectrum-css/page": "^3.0.1", "@spectrum-css/vars": "^3.0.1", diff --git a/packages/cli/package.json b/packages/cli/package.json index b71457a71a..f489f4635e 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@budibase/cli", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "description": "Budibase CLI, for developers, self hosting and migrations.", "main": "src/index.js", "bin": { diff --git a/packages/client/package.json b/packages/client/package.json index 8d387eb674..44ac4d6f2d 100644 --- a/packages/client/package.json +++ b/packages/client/package.json @@ -1,6 +1,6 @@ { "name": "@budibase/client", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "license": "MPL-2.0", "module": "dist/budibase-client.js", "main": "dist/budibase-client.js", @@ -19,9 +19,9 @@ "dev:builder": "rollup -cw" }, "dependencies": { - "@budibase/bbui": "^0.9.140-alpha.11", + "@budibase/bbui": "^0.9.140-alpha.12", "@budibase/standard-components": "^0.9.139", - "@budibase/string-templates": "^0.9.140-alpha.11", + "@budibase/string-templates": "^0.9.140-alpha.12", "regexparam": "^1.3.0", "shortid": "^2.2.15", "svelte-spa-router": "^3.0.5" diff --git a/packages/server/package.json b/packages/server/package.json index 1e2dc602ae..3b13e4bd28 100644 --- a/packages/server/package.json +++ b/packages/server/package.json @@ -1,7 +1,7 @@ { "name": "@budibase/server", "email": "hi@budibase.com", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "description": "Budibase Web Server", "main": "src/index.js", "repository": { @@ -62,9 +62,9 @@ "author": "Budibase", "license": "AGPL-3.0-or-later", "dependencies": { - "@budibase/auth": "^0.9.140-alpha.11", - "@budibase/client": "^0.9.140-alpha.11", - "@budibase/string-templates": "^0.9.140-alpha.11", + "@budibase/auth": "^0.9.140-alpha.12", + "@budibase/client": "^0.9.140-alpha.12", + "@budibase/string-templates": "^0.9.140-alpha.12", "@elastic/elasticsearch": "7.10.0", "@koa/router": "8.0.0", "@sendgrid/mail": "7.1.1", diff --git a/packages/string-templates/package.json b/packages/string-templates/package.json index 9ea3486427..1e73c0a708 100644 --- a/packages/string-templates/package.json +++ b/packages/string-templates/package.json @@ -1,6 +1,6 @@ { "name": "@budibase/string-templates", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "description": "Handlebars wrapper for Budibase templating.", "main": "src/index.cjs", "module": "dist/bundle.mjs", diff --git a/packages/worker/package.json b/packages/worker/package.json index be9ffabce5..94f3d0b577 100644 --- a/packages/worker/package.json +++ b/packages/worker/package.json @@ -1,7 +1,7 @@ { "name": "@budibase/worker", "email": "hi@budibase.com", - "version": "0.9.140-alpha.11", + "version": "0.9.140-alpha.12", "description": "Budibase background service", "main": "src/index.js", "repository": { @@ -25,8 +25,8 @@ "author": "Budibase", "license": "AGPL-3.0-or-later", "dependencies": { - "@budibase/auth": "^0.9.140-alpha.11", - "@budibase/string-templates": "^0.9.140-alpha.11", + "@budibase/auth": "^0.9.140-alpha.12", + "@budibase/string-templates": "^0.9.140-alpha.12", "@koa/router": "^8.0.0", "@techpass/passport-openidconnect": "^0.3.0", "aws-sdk": "^2.811.0",