From d4db493519bfe45e662ad5c3c4bdfd5e3b4be6bc Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Wed, 18 Sep 2024 11:50:15 +0100 Subject: [PATCH 1/7] Set view permissions to explicit roles from the parent table --- .../server/src/api/controllers/permission.ts | 4 +-- packages/server/src/sdk/app/views/index.ts | 33 ++++++++++++++++--- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 66a3254348..b75af88067 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -20,7 +20,7 @@ import { import { removeFromArray } from "../../utilities" import sdk from "../../sdk" -const enum PermissionUpdateType { +export const enum PermissionUpdateType { REMOVE = "remove", ADD = "add", } @@ -37,7 +37,7 @@ async function getAllDBRoles(db: Database) { return body.rows.map(row => row.doc!) } -async function updatePermissionOnRole( +export async function updatePermissionOnRole( { roleId, resourceId, diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index d7e05abf2f..c580bfde50 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -1,5 +1,6 @@ import { FieldType, + PermissionLevel, RelationSchemaField, RenameColumn, Table, @@ -10,20 +11,22 @@ import { ViewV2ColumnEnriched, ViewV2Enriched, } from "@budibase/types" -import { HTTPError } from "@budibase/backend-core" +import { HTTPError, roles } from "@budibase/backend-core" import { features } from "@budibase/pro" import { helpers, PROTECTED_EXTERNAL_COLUMNS, PROTECTED_INTERNAL_COLUMNS, } from "@budibase/shared-core" - import * as utils from "../../../db/utils" import { isExternalTableID } from "../../../integrations/utils" - import * as internal from "./internal" import * as external from "./external" import sdk from "../../../sdk" +import { + updatePermissionOnRole, + PermissionUpdateType, +} from "src/api/controllers/permission" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { @@ -123,8 +126,30 @@ export async function create( viewRequest: Omit ): Promise { await guardViewSchema(tableId, viewRequest) + const view = await pickApi(tableId).create(tableId, viewRequest) - return pickApi(tableId).create(tableId, viewRequest) + // Set permissions to be the same as the table + const tablePerms = await sdk.permissions.getResourcePerms(tableId) + const readRole = tablePerms[PermissionLevel.READ]?.role + const writeRole = tablePerms[PermissionLevel.WRITE]?.role + await updatePermissionOnRole( + { + roleId: readRole || roles.BUILTIN_ROLE_IDS.BASIC, + resourceId: view.id, + level: PermissionLevel.READ, + }, + PermissionUpdateType.ADD + ) + await updatePermissionOnRole( + { + roleId: writeRole || roles.BUILTIN_ROLE_IDS.BASIC, + resourceId: view.id, + level: PermissionLevel.WRITE, + }, + PermissionUpdateType.ADD + ) + + return view } export async function update(tableId: string, view: ViewV2): Promise { From 55c7751dbbc6f96f66cc1b8d35d7561f797b528c Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Thu, 19 Sep 2024 15:12:03 +0100 Subject: [PATCH 2/7] Move permission updates into SDK --- .../server/src/api/controllers/permission.ts | 107 ++---------------- .../server/src/sdk/app/permissions/index.ts | 102 ++++++++++++++++- packages/server/src/sdk/app/views/index.ts | 5 +- 3 files changed, 111 insertions(+), 103 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index b75af88067..55b942686f 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -3,7 +3,6 @@ import { UserCtx, Database, Role, - PermissionLevel, GetResourcePermsResponse, ResourcePermissionInfo, GetDependantResourcesResponse, @@ -12,107 +11,15 @@ import { RemovePermissionRequest, RemovePermissionResponse, } from "@budibase/types" -import { getRoleParams } from "../../db/utils" import { CURRENTLY_SUPPORTED_LEVELS, getBasePermissions, } from "../../utilities/security" -import { removeFromArray } from "../../utilities" import sdk from "../../sdk" - -export const enum PermissionUpdateType { - REMOVE = "remove", - ADD = "add", -} +import { PermissionUpdateType } from "../../sdk/app/permissions" const SUPPORTED_LEVELS = CURRENTLY_SUPPORTED_LEVELS -// utility function to stop this repetition - permissions always stored under roles -async function getAllDBRoles(db: Database) { - const body = await db.allDocs( - getRoleParams(null, { - include_docs: true, - }) - ) - return body.rows.map(row => row.doc!) -} - -export async function updatePermissionOnRole( - { - roleId, - resourceId, - level, - }: { roleId: string; resourceId: string; level: PermissionLevel }, - updateType: PermissionUpdateType -) { - const db = context.getAppDB() - const remove = updateType === PermissionUpdateType.REMOVE - const isABuiltin = roles.isBuiltin(roleId) - const dbRoleId = roles.getDBRoleID(roleId) - const dbRoles = await getAllDBRoles(db) - const docUpdates: Role[] = [] - - // the permission is for a built in, make sure it exists - if (isABuiltin && !dbRoles.some(role => role._id === dbRoleId)) { - const builtin = roles.getBuiltinRoles()[roleId] - builtin._id = roles.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 - for (let role of dbRoles) { - let updated = false - const rolePermissions: Record = 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] as unknown as PermissionLevel] - : [] - } - // 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].indexOf(level) !== -1 - ) { - removeFromArray(rolePermissions[resourceId], level) - updated = true - } - // handle the adding, we're on the correct role, at it to this - if (!remove && role._id === dbRoleId) { - 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 - if (updated) { - role.permissions = rolePermissions - docUpdates.push(role) - } - } - - const response = await db.bulkDocs(docUpdates) - return response.map(resp => { - const version = docUpdates.find(role => role._id === resp.id)?.version - const _id = roles.getExternalRoleID(resp.id, version) - return { - _id, - rev: resp.rev, - error: resp.error, - reason: resp.reason, - } - }) -} - export function fetchBuiltin(ctx: UserCtx) { ctx.body = Object.values(permissions.getBuiltinPermissions()) } @@ -124,7 +31,7 @@ export function fetchLevels(ctx: UserCtx) { export async function fetch(ctx: UserCtx) { const db = context.getAppDB() - const dbRoles: Role[] = await getAllDBRoles(db) + const dbRoles: Role[] = await sdk.permissions.getAllDBRoles(db) let permissions: any = {} // create an object with structure role ID -> resource ID -> level for (let role of dbRoles) { @@ -186,12 +93,18 @@ export async function getDependantResources( export async function addPermission(ctx: UserCtx) { const params: AddPermissionRequest = ctx.params - ctx.body = await updatePermissionOnRole(params, PermissionUpdateType.ADD) + ctx.body = await sdk.permissions.updatePermissionOnRole( + params, + PermissionUpdateType.ADD + ) } export async function removePermission( ctx: UserCtx ) { const params: RemovePermissionRequest = ctx.params - ctx.body = await updatePermissionOnRole(params, PermissionUpdateType.REMOVE) + ctx.body = await sdk.permissions.updatePermissionOnRole( + params, + PermissionUpdateType.REMOVE + ) } diff --git a/packages/server/src/sdk/app/permissions/index.ts b/packages/server/src/sdk/app/permissions/index.ts index a6e81652ee..5f8882399b 100644 --- a/packages/server/src/sdk/app/permissions/index.ts +++ b/packages/server/src/sdk/app/permissions/index.ts @@ -1,22 +1,34 @@ -import { db, roles } from "@budibase/backend-core" +import { db, roles, context } from "@budibase/backend-core" import { PermissionLevel, PermissionSource, VirtualDocumentType, + Role, + Database, } from "@budibase/types" -import { extractViewInfoFromID, isViewID } from "../../../db/utils" +import { + extractViewInfoFromID, + isViewID, + getRoleParams, +} from "../../../db/utils" import { CURRENTLY_SUPPORTED_LEVELS, getBasePermissions, } from "../../../utilities/security" import sdk from "../../../sdk" import { isV2 } from "../views" +import { removeFromArray } from "../../../utilities" type ResourcePermissions = Record< string, { role: string; type: PermissionSource } > +export const enum PermissionUpdateType { + REMOVE = "remove", + ADD = "add", +} + export async function getInheritablePermissions( resourceId: string ): Promise { @@ -100,3 +112,89 @@ export async function getDependantResources( return } + +export async function updatePermissionOnRole( + { + roleId, + resourceId, + level, + }: { roleId: string; resourceId: string; level: PermissionLevel }, + updateType: PermissionUpdateType +) { + const db = context.getAppDB() + const remove = updateType === PermissionUpdateType.REMOVE + const isABuiltin = roles.isBuiltin(roleId) + const dbRoleId = roles.getDBRoleID(roleId) + const dbRoles = await getAllDBRoles(db) + const docUpdates: Role[] = [] + + // the permission is for a built in, make sure it exists + if (isABuiltin && !dbRoles.some(role => role._id === dbRoleId)) { + const builtin = roles.getBuiltinRoles()[roleId] + builtin._id = roles.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 + for (let role of dbRoles) { + let updated = false + const rolePermissions: Record = 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] as unknown as PermissionLevel] + : [] + } + // 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].indexOf(level) !== -1 + ) { + removeFromArray(rolePermissions[resourceId], level) + updated = true + } + // handle the adding, we're on the correct role, at it to this + if (!remove && role._id === dbRoleId) { + 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 + if (updated) { + role.permissions = rolePermissions + docUpdates.push(role) + } + } + + const response = await db.bulkDocs(docUpdates) + return response.map(resp => { + const version = docUpdates.find(role => role._id === resp.id)?.version + const _id = roles.getExternalRoleID(resp.id, version) + return { + _id, + rev: resp.rev, + error: resp.error, + reason: resp.reason, + } + }) +} + +// utility function to stop this repetition - permissions always stored under roles +export async function getAllDBRoles(db: Database) { + const body = await db.allDocs( + getRoleParams(null, { + include_docs: true, + }) + ) + return body.rows.map(row => row.doc!) +} diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index c580bfde50..47af484ebc 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -23,10 +23,7 @@ import { isExternalTableID } from "../../../integrations/utils" import * as internal from "./internal" import * as external from "./external" import sdk from "../../../sdk" -import { - updatePermissionOnRole, - PermissionUpdateType, -} from "src/api/controllers/permission" +import { updatePermissionOnRole, PermissionUpdateType } from "../permissions" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { From 418bbff2f57e573ce9f36796f5fb77014aaa4870 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Thu, 19 Sep 2024 15:15:19 +0100 Subject: [PATCH 3/7] Lint --- packages/server/src/api/controllers/permission.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 55b942686f..c7afb6a351 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -1,7 +1,6 @@ import { permissions, roles, context } from "@budibase/backend-core" import { UserCtx, - Database, Role, GetResourcePermsResponse, ResourcePermissionInfo, From 51e09ddf7b2c2d0274cd2b4a503a375993505f1d Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Mon, 23 Sep 2024 10:08:21 +0100 Subject: [PATCH 4/7] Update row action tests to revoke explicit view permissions when testing triggering against views --- packages/server/src/api/routes/tests/rowAction.spec.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index ef7d2afbba..efd28eb92f 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -826,11 +826,20 @@ describe("/rowsActions", () => { ) ).id + // Allow row action on view await config.api.rowAction.setViewPermission( tableId, viewId, rowAction.id ) + + // Delete explicit view permissions so they inherit table permissions + await config.api.permission.revoke({ + level: PermissionLevel.READ, + resourceId: viewId, + roleId: "inherited", + }) + return { permissionResource: tableId, triggerResouce: viewId } }, ], From b53bc5dfafde52441c691e0c4e7ce50bc4e4f149 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 27 Sep 2024 09:08:38 +0100 Subject: [PATCH 5/7] Update tests --- packages/server/src/api/routes/tests/rowAction.spec.ts | 2 +- packages/server/src/api/routes/tests/viewV2.spec.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index efd28eb92f..4fe248984a 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -835,9 +835,9 @@ describe("/rowsActions", () => { // Delete explicit view permissions so they inherit table permissions await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.PUBLIC, // Don't think this matters since we are revoking the permission level: PermissionLevel.READ, resourceId: viewId, - roleId: "inherited", }) return { permissionResource: tableId, triggerResouce: viewId } diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index c4a39ae8a9..e727f952e5 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -2248,6 +2248,11 @@ describe.each([ level: PermissionLevel.READ, resourceId: table._id!, }) + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.PUBLIC, // Don't think this matters since we are revoking the permission + level: PermissionLevel.READ, + resourceId: view.id, + }) await config.publish() const response = await config.api.viewV2.publicSearch(view.id) From 80f1de27de0c1a93cc9b07a5ce988cbf96441231 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 27 Sep 2024 09:19:42 +0100 Subject: [PATCH 6/7] Please just work tests --- .../server/src/api/routes/tests/permissions.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 0f059998ae..d2df14d7af 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -125,6 +125,12 @@ describe("/permission", () => { }) it("should be able to access the view data when the table is set to public and with no view permissions overrides", async () => { + // Make view inherit table permissions + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) // replicate changes before checking permissions await config.publish() @@ -138,6 +144,12 @@ describe("/permission", () => { resourceId: table._id, level: PermissionLevel.READ, }) + // Make view inherit table permissions + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) // replicate changes before checking permissions await config.publish() From 80cdcf99da47eb258222d5a3633af224333918d1 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Fri, 27 Sep 2024 10:35:23 +0100 Subject: [PATCH 7/7] Add additional comments to tests --- packages/server/src/api/routes/tests/permissions.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index d2df14d7af..625143e7fb 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -125,7 +125,7 @@ describe("/permission", () => { }) it("should be able to access the view data when the table is set to public and with no view permissions overrides", async () => { - // Make view inherit table permissions + // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, resourceId: view.id, @@ -144,7 +144,7 @@ describe("/permission", () => { resourceId: table._id, level: PermissionLevel.READ, }) - // Make view inherit table permissions + // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, resourceId: view.id,