From 3405e6d6b7ac795398b03d720267a0804c2d46f1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 16:12:01 +0100 Subject: [PATCH 01/20] Make new tables require ADMIN permissions to read and write. --- .../server/src/api/routes/tests/table.spec.ts | 9 ++++++- packages/server/src/utilities/security.ts | 26 ++++++++++++------- packages/types/src/documents/document.ts | 24 +++++++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index d6c1651ef0..f82476d412 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,4 @@ -import { context, docIds, events } from "@budibase/backend-core" +import { context, docIds, events, roles } from "@budibase/backend-core" import { PROTECTED_EXTERNAL_COLUMNS, PROTECTED_INTERNAL_COLUMNS, @@ -189,6 +189,13 @@ describe.each([ ) } ) + + it("should create tables with ADMIN read and write permissions", async () => { + const table = await config.api.table.save(tableForDatasource(datasource)) + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions.read.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + expect(permissions.write.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + }) }) describe("update", () => { diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index 01a3468c9c..40dba465f1 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -1,5 +1,6 @@ import { permissions, roles } from "@budibase/backend-core" import { DocumentType, VirtualDocumentType } from "../db/utils" +import { getDocumentType, getVirtualDocumentType } from "@budibase/types" export const CURRENTLY_SUPPORTED_LEVELS: string[] = [ permissions.PermissionLevel.WRITE, @@ -8,14 +9,16 @@ export const CURRENTLY_SUPPORTED_LEVELS: string[] = [ ] export function getPermissionType(resourceId: string) { - const docType = Object.values(DocumentType).filter(docType => - resourceId.startsWith(docType) - )[0] - switch (docType as DocumentType | VirtualDocumentType) { - case DocumentType.TABLE: - case DocumentType.ROW: + const virtualDocType = getVirtualDocumentType(resourceId) + switch (virtualDocType) { case VirtualDocumentType.VIEW: return permissions.PermissionType.TABLE + } + + const docType = getDocumentType(resourceId) + switch (docType) { + case DocumentType.TABLE: + case DocumentType.ROW: case DocumentType.AUTOMATION: return permissions.PermissionType.AUTOMATION case DocumentType.WEBHOOK: @@ -39,15 +42,18 @@ export function getBasePermissions(resourceId: string) { if (!role.permissionId) { continue } + const perms = permissions.getBuiltinPermissionByID(role.permissionId) if (!perms) { continue } + const typedPermission = perms.permissions.find(perm => perm.type === type) - if ( - typedPermission && - CURRENTLY_SUPPORTED_LEVELS.indexOf(typedPermission.level) !== -1 - ) { + if (!typedPermission) { + continue + } + + if (CURRENTLY_SUPPORTED_LEVELS.includes(typedPermission.level)) { const level = typedPermission.level basePermissions[level] = roles.lowerBuiltinRoleID( basePermissions[level], diff --git a/packages/types/src/documents/document.ts b/packages/types/src/documents/document.ts index f5facfae9d..2f0da1081a 100644 --- a/packages/types/src/documents/document.ts +++ b/packages/types/src/documents/document.ts @@ -42,6 +42,17 @@ export enum DocumentType { ROW_ACTIONS = "ra", } +// Because DocumentTypes can overlap, we need to make sure that we search +// longest first to ensure we get the correct type. +const sortedDocumentTypes = Object.values(DocumentType).sort( + (a, b) => b.length - a.length // descending +) +export function getDocumentType(id: string): DocumentType | undefined { + return sortedDocumentTypes.find(docType => + id.startsWith(`${docType}${SEPARATOR}`) + ) +} + // these are the core documents that make up the data, design // and automation sections of an app. This excludes any internal // rows as we shouldn't import data. @@ -72,6 +83,19 @@ export enum VirtualDocumentType { ROW_ACTION = "row_action", } +// Because VirtualDocumentTypes can overlap, we need to make sure that we search +// longest first to ensure we get the correct type. +const sortedVirtualDocumentTypes = Object.values(VirtualDocumentType).sort( + (a, b) => b.length - a.length // descending +) +export function getVirtualDocumentType( + id: string +): VirtualDocumentType | undefined { + return sortedVirtualDocumentTypes.find(docType => + id.startsWith(`${docType}${SEPARATOR}`) + ) +} + export interface Document { _id?: string _rev?: string From 4cde2f26ad7a9c74ce041122d652cef5eebef9e0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:06:54 +0100 Subject: [PATCH 02/20] wip, tests broken --- .../server/src/api/controllers/permission.ts | 7 ++- .../src/api/routes/tests/permissions.spec.ts | 61 +++++++++++-------- .../server/src/api/routes/tests/table.spec.ts | 17 +++++- .../src/tests/utilities/api/permission.ts | 10 +++ packages/server/src/utilities/security.ts | 4 +- packages/types/src/api/web/app/permission.ts | 4 ++ 6 files changed, 73 insertions(+), 30 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index c7afb6a351..668501b3f3 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -9,6 +9,7 @@ import { AddPermissionRequest, RemovePermissionRequest, RemovePermissionResponse, + FetchResourcePermissionInfoResponse, } from "@budibase/types" import { CURRENTLY_SUPPORTED_LEVELS, @@ -28,7 +29,9 @@ export function fetchLevels(ctx: UserCtx) { ctx.body = SUPPORTED_LEVELS } -export async function fetch(ctx: UserCtx) { +export async function fetch( + ctx: UserCtx +) { const db = context.getAppDB() const dbRoles: Role[] = await sdk.permissions.getAllDBRoles(db) let permissions: any = {} @@ -49,7 +52,7 @@ export async function fetch(ctx: UserCtx) { } } // apply the base permissions - const finalPermissions: Record> = {} + const finalPermissions: FetchResourcePermissionInfoResponse = {} for (let [resource, permission] of Object.entries(permissions)) { const basePerms = getBasePermissions(resource) finalPermissions[resource] = Object.assign(basePerms, permission) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index b148d6fde1..f8974f44aa 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,7 +12,7 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let table: Table & { _id: string } + let table: Table let perms: Document[] let row: Row let view: ViewV2 @@ -26,15 +26,33 @@ describe("/permission", () => { beforeEach(async () => { mocks.licenses.useCloudFree() - table = (await config.createTable()) as typeof table + table = await config.createTable() + + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.READ, + }) + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.WRITE, + }) + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.EXECUTE, + }) + row = await config.createRow() view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), }) + perms = await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) }) @@ -74,27 +92,22 @@ describe("/permission", () => { }) }) - it("should get resource permissions with multiple roles", async () => { + it.only("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.WRITE, }) - const res = await config.api.permission.get(table._id) - expect(res).toEqual({ - permissions: { - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - }, + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions).toEqual({ + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, + execute: { permissionType: "BASE", role: "BASIC" }, }) - const allRes = await request - .get(`/api/permission`) - .set(config.defaultHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) - expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + const all = await config.api.permission.fetch() + expect(all[table._id!]["read"]).toEqual(STD_ROLE_ID) + expect(all[table._id!]["write"]).toEqual(HIGHER_ROLE_ID) }) }) @@ -102,11 +115,11 @@ describe("/permission", () => { it("should be able to remove the permission", async () => { const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id) + const permsRes = await config.api.permission.get(table._id!) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) }) @@ -142,7 +155,7 @@ describe("/permission", () => { it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) @@ -167,7 +180,7 @@ describe("/permission", () => { }) await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) // replicate changes before checking permissions @@ -179,8 +192,8 @@ describe("/permission", () => { it("shouldn't allow writing from a public user", async () => { const res = await request - .post(`/api/${table._id}/rows`) - .send(basicRow(table._id)) + .post(`/api/${table._id!}/rows`) + .send(basicRow(table._id!)) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(401) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index f82476d412..60e8142522 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -21,6 +21,7 @@ import { ViewCalculation, ViewV2Enriched, RowExportFormat, + PermissionSource, } from "@budibase/types" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" @@ -193,8 +194,20 @@ describe.each([ it("should create tables with ADMIN read and write permissions", async () => { const table = await config.api.table.save(tableForDatasource(datasource)) const { permissions } = await config.api.permission.get(table._id!) - expect(permissions.read.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) - expect(permissions.write.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + expect(permissions).toEqual({ + read: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + write: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + execute: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + }) }) }) diff --git a/packages/server/src/tests/utilities/api/permission.ts b/packages/server/src/tests/utilities/api/permission.ts index 986796d9a1..b4e641a1be 100644 --- a/packages/server/src/tests/utilities/api/permission.ts +++ b/packages/server/src/tests/utilities/api/permission.ts @@ -1,6 +1,7 @@ import { AddPermissionRequest, AddPermissionResponse, + FetchResourcePermissionInfoResponse, GetResourcePermsResponse, RemovePermissionRequest, RemovePermissionResponse, @@ -26,6 +27,15 @@ export class PermissionAPI extends TestAPI { ) } + fetch = async ( + expectations?: Expectations + ): Promise => { + return await this._get( + `/api/permission`, + { expectations } + ) + } + revoke = async ( request: RemovePermissionRequest, expectations?: Expectations diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index 40dba465f1..a3353f6184 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -35,9 +35,9 @@ export function getPermissionType(resourceId: string) { /** * works out the basic permissions based on builtin roles for a resource, using its ID */ -export function getBasePermissions(resourceId: string) { +export function getBasePermissions(resourceId: string): Record { const type = getPermissionType(resourceId) - const basePermissions: { [key: string]: string } = {} + const basePermissions: Record = {} for (let [roleId, role] of Object.entries(roles.getBuiltinRoles())) { if (!role.permissionId) { continue diff --git a/packages/types/src/api/web/app/permission.ts b/packages/types/src/api/web/app/permission.ts index bead2a4279..a5c4df5733 100644 --- a/packages/types/src/api/web/app/permission.ts +++ b/packages/types/src/api/web/app/permission.ts @@ -1,5 +1,9 @@ import { PermissionLevel } from "../../../sdk" +export interface FetchResourcePermissionInfoResponse { + [key: string]: Record +} + export interface ResourcePermissionInfo { role: string permissionType: string From 9f84262940f34178bc57a44e7b11231f0528e066 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:12:37 +0200 Subject: [PATCH 03/20] Clean --- packages/server/src/api/controllers/permission.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 668501b3f3..148cea401a 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -33,7 +33,7 @@ export async function fetch( ctx: UserCtx ) { const db = context.getAppDB() - const dbRoles: Role[] = await sdk.permissions.getAllDBRoles(db) + const dbRoles = await sdk.permissions.getAllDBRoles(db) let permissions: any = {} // create an object with structure role ID -> resource ID -> level for (let role of dbRoles) { From aa0a1737c8021508ecf6316001788a72c3d094b4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:16:17 +0200 Subject: [PATCH 04/20] Fix tests --- packages/server/src/api/controllers/permission.ts | 1 - packages/server/src/api/routes/tests/permissions.spec.ts | 3 +-- packages/server/src/utilities/security.ts | 1 + 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 148cea401a..cbe260367c 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, - Role, GetResourcePermsResponse, ResourcePermissionInfo, GetDependantResourcesResponse, diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index f8974f44aa..8601ed2df5 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -92,7 +92,7 @@ describe("/permission", () => { }) }) - it.only("should get resource permissions with multiple roles", async () => { + it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, resourceId: table._id!, @@ -102,7 +102,6 @@ describe("/permission", () => { expect(permissions).toEqual({ read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - execute: { permissionType: "BASE", role: "BASIC" }, }) const all = await config.api.permission.fetch() diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index a3353f6184..4f93c33ee4 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -19,6 +19,7 @@ export function getPermissionType(resourceId: string) { switch (docType) { case DocumentType.TABLE: case DocumentType.ROW: + return permissions.PermissionType.TABLE case DocumentType.AUTOMATION: return permissions.PermissionType.AUTOMATION case DocumentType.WEBHOOK: From 1e0902a8313273d522a1c2ffa8bc6e2cd5947f05 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:40:15 +0200 Subject: [PATCH 05/20] Cleanup tess --- .../src/api/routes/tests/permissions.spec.ts | 277 +++++++++--------- 1 file changed, 137 insertions(+), 140 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 8601ed2df5..b938bff279 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,10 +12,7 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let table: Table let perms: Document[] - let row: Row - let view: ViewV2 afterAll(setup.afterAll) @@ -25,36 +22,6 @@ describe("/permission", () => { beforeEach(async () => { mocks.licenses.useCloudFree() - - table = await config.createTable() - - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.READ, - }) - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.WRITE, - }) - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.EXECUTE, - }) - - row = await config.createRow() - view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - }) - - perms = await config.api.permission.add({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, - }) }) describe("levels", () => { @@ -72,131 +39,161 @@ describe("/permission", () => { }) }) - 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}`) - }) + describe("table permissions", () => { + let table: Table & { _id: string } + let row: Row + let view: ViewV2 - it("should get the resource permissions", async () => { - const res = await request - .get(`/api/permission/${table._id}`) - .set(config.defaultHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(res.body).toEqual({ - permissions: { - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, - }, + beforeEach(async () => { + mocks.licenses.useCloudFree() + + table = (await config.createTable()) as typeof table + row = await config.createRow() + view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), }) - }) - - it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ - roleId: HIGHER_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.WRITE, - }) - const { permissions } = await config.api.permission.get(table._id!) - expect(permissions).toEqual({ - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - }) - - const all = await config.api.permission.fetch() - expect(all[table._id!]["read"]).toEqual(STD_ROLE_ID) - expect(all[table._id!]["write"]).toEqual(HIGHER_ROLE_ID) - }) - }) - - describe("remove", () => { - it("should be able to remove the permission", async () => { - const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id!, + resourceId: table._id, level: PermissionLevel.READ, }) - expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id!) - expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() - }) - }) - - describe("check public user allowed", () => { - it("should be able to read the row", async () => { - // replicate changes before checking permissions - await config.publish() - - const res = await request - .get(`/api/${table._id}/rows`) - .set(config.publicHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(res.body[0]._id).toEqual(row._id) }) - 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. Needed for backwards compatibility with existing views. - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, + 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}`) }) - // replicate changes before checking permissions - await config.publish() + it("should get the resource permissions", async () => { + const res = await request + .get(`/api/permission/${table._id}`) + .set(config.defaultHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body).toEqual({ + permissions: { + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, + }, + }) + }) - const res = await config.api.viewV2.publicSearch(view.id) - expect(res.rows[0]._id).toEqual(row._id) + it("should get resource permissions with multiple roles", async () => { + perms = await config.api.permission.add({ + roleId: HIGHER_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.WRITE, + }) + const res = await config.api.permission.get(table._id) + expect(res).toEqual({ + permissions: { + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, + }, + }) + + const allRes = await request + .get(`/api/permission`) + .set(config.defaultHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) + expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + }) }) - it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, + describe("remove", () => { + it("should be able to remove the permission", async () => { + const res = await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + expect(res[0]._id).toEqual(STD_ROLE_ID) + const permsRes = await config.api.permission.get(table._id) + expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) - - // Make view inherit table permissions. Needed for backwards compatibility with existing views. - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, - }) - - // replicate changes before checking permissions - await config.publish() - - await config.api.viewV2.publicSearch(view.id, undefined, { status: 401 }) }) - it("should use the view permissions", async () => { - await config.api.permission.add({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, - }) - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, - }) - // replicate changes before checking permissions - await config.publish() + describe("check public user allowed", () => { + it("should be able to read the row", async () => { + // replicate changes before checking permissions + await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) - expect(res.rows[0]._id).toEqual(row._id) - }) + const res = await request + .get(`/api/${table._id}/rows`) + .set(config.publicHeaders()) + .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(basicRow(table._id!)) - .set(config.publicHeaders()) - .expect("Content-Type", /json/) - .expect(401) - expect(res.status).toEqual(401) + 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. Needed for backwards compatibility with existing views. + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + + // replicate changes before checking permissions + await config.publish() + + const res = await config.api.viewV2.publicSearch(view.id) + expect(res.rows[0]._id).toEqual(row._id) + }) + + it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + + // Make view inherit table permissions. Needed for backwards compatibility with existing views. + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + + // replicate changes before checking permissions + await config.publish() + + await config.api.viewV2.publicSearch(view.id, undefined, { + status: 401, + }) + }) + + it("should use the view permissions", async () => { + await config.api.permission.add({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + // replicate changes before checking permissions + await config.publish() + + const res = await config.api.viewV2.publicSearch(view.id) + expect(res.rows[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(basicRow(table._id)) + .set(config.publicHeaders()) + .expect("Content-Type", /json/) + .expect(401) + expect(res.status).toEqual(401) + }) }) }) From 38685b50df2a68fb19ac8c0b2e85a3e0530b571d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:50:05 +0200 Subject: [PATCH 06/20] Add extra test --- .../src/api/routes/tests/permissions.spec.ts | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index b938bff279..3b45d042c7 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -1,5 +1,5 @@ import { roles } from "@budibase/backend-core" -import { Document, PermissionLevel, Row, Table, ViewV2 } from "@budibase/types" +import { Document, PermissionLevel, Row, ViewV2 } from "@budibase/types" import * as setup from "./utilities" import { generator, mocks } from "@budibase/backend-core/tests" @@ -12,7 +12,6 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let perms: Document[] afterAll(setup.afterAll) @@ -40,26 +39,43 @@ describe("/permission", () => { }) describe("table permissions", () => { - let table: Table & { _id: string } + let tableId: string let row: Row let view: ViewV2 + let perms: Document[] beforeEach(async () => { mocks.licenses.useCloudFree() - table = (await config.createTable()) as typeof table + const table = await config.createTable() + tableId = table._id! row = await config.createRow() view = await config.api.viewV2.create({ - tableId: table._id!, + tableId, name: generator.guid(), }) perms = await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) }) + it("tables should be defaulted to admin", async () => { + const table = await config.createTable() + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions).toEqual({ + read: { + permissionType: "BASE", + role: "BASIC", + }, + write: { + permissionType: "BASE", + role: "BASIC", + }, + }) + }) + describe("add", () => { it("should be able to add permission to a role for the table", async () => { expect(perms.length).toEqual(1) @@ -68,7 +84,7 @@ describe("/permission", () => { it("should get the resource permissions", async () => { const res = await request - .get(`/api/permission/${table._id}`) + .get(`/api/permission/${tableId}`) .set(config.defaultHeaders()) .expect("Content-Type", /json/) .expect(200) @@ -83,10 +99,10 @@ describe("/permission", () => { it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.WRITE, }) - const res = await config.api.permission.get(table._id) + const res = await config.api.permission.get(tableId) expect(res).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, @@ -99,8 +115,8 @@ describe("/permission", () => { .set(config.defaultHeaders()) .expect("Content-Type", /json/) .expect(200) - expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) - expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + expect(allRes.body[tableId]["read"]).toEqual(STD_ROLE_ID) + expect(allRes.body[tableId]["write"]).toEqual(HIGHER_ROLE_ID) }) }) @@ -108,11 +124,11 @@ describe("/permission", () => { it("should be able to remove the permission", async () => { const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id) + const permsRes = await config.api.permission.get(tableId) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) }) @@ -123,7 +139,7 @@ describe("/permission", () => { await config.publish() const res = await request - .get(`/api/${table._id}/rows`) + .get(`/api/${tableId}/rows`) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(200) @@ -148,7 +164,7 @@ describe("/permission", () => { it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) @@ -175,7 +191,7 @@ describe("/permission", () => { }) await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) // replicate changes before checking permissions @@ -187,8 +203,8 @@ describe("/permission", () => { it("shouldn't allow writing from a public user", async () => { const res = await request - .post(`/api/${table._id}/rows`) - .send(basicRow(table._id)) + .post(`/api/${tableId}/rows`) + .send(basicRow(tableId)) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(401) From 56459b27367780fbb739ab68b0be499e67f09b5d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 15:47:35 +0200 Subject: [PATCH 07/20] Type --- packages/backend-core/src/security/permissions.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index 98704f16c6..4ed2cd3954 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -65,7 +65,13 @@ export enum BuiltinPermissionID { POWER = "power", } -export const BUILTIN_PERMISSIONS = { +export const BUILTIN_PERMISSIONS: { + [key in keyof typeof BuiltinPermissionID]: { + _id: (typeof BuiltinPermissionID)[key] + name: string + permissions: Permission[] + } +} = { PUBLIC: { _id: BuiltinPermissionID.PUBLIC, name: "Public", From ebd762cdb69a21d3edc5adee018f42bb5d1e730d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:19:18 +0200 Subject: [PATCH 08/20] Fixes --- packages/backend-core/src/security/roles.ts | 4 ++-- packages/server/src/api/controllers/permission.ts | 4 ++-- packages/server/src/api/routes/tests/permissions.spec.ts | 8 +++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 65339832cf..9ae3788aab 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -42,7 +42,7 @@ export class Role implements RoleDoc { _id: string _rev?: string name: string - permissionId: string + permissionId: BuiltinPermissionID inherits?: string version?: string permissions: Record = {} @@ -51,7 +51,7 @@ export class Role implements RoleDoc { constructor( id: string, name: string, - permissionId: string, + permissionId: BuiltinPermissionID, uiMetadata?: RoleUIMetadata ) { this._id = id diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index cbe260367c..a58d94ce80 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -33,7 +33,7 @@ export async function fetch( ) { const db = context.getAppDB() const dbRoles = await sdk.permissions.getAllDBRoles(db) - let permissions: any = {} + let permissions: Record> = {} // create an object with structure role ID -> resource ID -> level for (let role of dbRoles) { if (!role.permissions) { @@ -45,7 +45,7 @@ export async function fetch( } for (let [resource, levelArr] of Object.entries(role.permissions)) { const levels: string[] = Array.isArray(levelArr) ? levelArr : [levelArr] - const perms: Record = {} + const perms: Record = permissions[resource] || {} levels.forEach(level => (perms[level] = roleId!)) permissions[resource] = perms } diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 3b45d042c7..83452b4f1f 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -39,6 +39,8 @@ describe("/permission", () => { }) describe("table permissions", () => { + const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN + let tableId: string let row: Row let view: ViewV2 @@ -67,11 +69,11 @@ describe("/permission", () => { expect(permissions).toEqual({ read: { permissionType: "BASE", - role: "BASIC", + role: DEFAULT_TABLE_ROLE_ID, }, write: { permissionType: "BASE", - role: "BASIC", + role: DEFAULT_TABLE_ROLE_ID, }, }) }) @@ -91,7 +93,7 @@ describe("/permission", () => { expect(res.body).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, + write: { permissionType: "BASE", role: DEFAULT_TABLE_ROLE_ID }, }, }) }) From a0d3bf99931cad51c269d099b8b25c32d9b1ea2b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:25:08 +0200 Subject: [PATCH 09/20] Remove redundant ok button --- .../backend/DataTable/modals/ManageAccessModal.svelte | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte b/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte index 48b584690e..9f5cd9fd95 100644 --- a/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte +++ b/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte @@ -94,7 +94,7 @@ loadDependantInfo() - + Manage Access {#if requiresPlanToModify} From 37eb66e0d52875dc978cb5367d7773d2bc1b1dbc Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:32:35 +0200 Subject: [PATCH 10/20] Cleanup tests --- .../src/api/routes/tests/permissions.spec.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 83452b4f1f..c8b7597c9d 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -43,19 +43,12 @@ describe("/permission", () => { let tableId: string let row: Row - let view: ViewV2 let perms: Document[] beforeEach(async () => { - mocks.licenses.useCloudFree() - const table = await config.createTable() tableId = table._id! row = await config.createRow() - view = await config.api.viewV2.create({ - tableId, - name: generator.guid(), - }) perms = await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -136,6 +129,15 @@ describe("/permission", () => { }) describe("check public user allowed", () => { + let view: ViewV2 + + beforeEach(async () => { + view = await config.api.viewV2.create({ + tableId, + name: generator.guid(), + }) + }) + it("should be able to read the row", async () => { // replicate changes before checking permissions await config.publish() From 577ab5b6ceaa68c6a58f37b7fcc5fb601b86679b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:32:56 +0200 Subject: [PATCH 11/20] Fix tests --- packages/backend-core/src/security/permissions.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index 4ed2cd3954..aaf83f78df 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -84,7 +84,8 @@ export const BUILTIN_PERMISSIONS: { name: "Read only", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.READ), - new Permission(PermissionType.TABLE, PermissionLevel.READ), + // TODO: don't add "breaking changes" + // new Permission(PermissionType.TABLE, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), ], }, @@ -93,7 +94,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read/Write", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.WRITE), - new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.LEGACY_VIEW, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), @@ -103,7 +104,7 @@ export const BUILTIN_PERMISSIONS: { _id: BuiltinPermissionID.POWER, name: "Power", permissions: [ - new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.USER, PermissionLevel.READ), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.WEBHOOK, PermissionLevel.READ), From ff402c54e0ad79dde56eb568ee87573197bb56ff Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:48:39 +0200 Subject: [PATCH 12/20] Add view tests --- .../src/api/routes/tests/permissions.spec.ts | 81 ++++++++++++++++--- packages/server/src/sdk/app/views/index.ts | 25 +----- 2 files changed, 69 insertions(+), 37 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index c8b7597c9d..5dfece3126 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -1,5 +1,5 @@ import { roles } from "@budibase/backend-core" -import { Document, PermissionLevel, Row, ViewV2 } from "@budibase/types" +import { Document, PermissionLevel, Row } from "@budibase/types" import * as setup from "./utilities" import { generator, mocks } from "@budibase/backend-core/tests" @@ -9,6 +9,8 @@ const { BUILTIN_ROLE_IDS } = roles const HIGHER_ROLE_ID = BUILTIN_ROLE_IDS.BASIC const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC +const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN + describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() @@ -39,16 +41,12 @@ describe("/permission", () => { }) describe("table permissions", () => { - const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN - let tableId: string - let row: Row let perms: Document[] beforeEach(async () => { const table = await config.createTable() tableId = table._id! - row = await config.createRow() perms = await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -129,13 +127,16 @@ describe("/permission", () => { }) describe("check public user allowed", () => { - let view: ViewV2 + let viewId: string + let row: Row beforeEach(async () => { - view = await config.api.viewV2.create({ + const view = await config.api.viewV2.create({ tableId, name: generator.guid(), }) + viewId = view.id + row = await config.createRow() }) it("should be able to read the row", async () => { @@ -154,14 +155,14 @@ describe("/permission", () => { // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) // replicate changes before checking permissions await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) + const res = await config.api.viewV2.publicSearch(viewId) expect(res.rows[0]._id).toEqual(row._id) }) @@ -175,14 +176,14 @@ describe("/permission", () => { // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) // replicate changes before checking permissions await config.publish() - await config.api.viewV2.publicSearch(view.id, undefined, { + await config.api.viewV2.publicSearch(viewId, undefined, { status: 401, }) }) @@ -190,7 +191,7 @@ describe("/permission", () => { it("should use the view permissions", async () => { await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) await config.api.permission.revoke({ @@ -201,7 +202,7 @@ describe("/permission", () => { // replicate changes before checking permissions await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) + const res = await config.api.viewV2.publicSearch(viewId) expect(res.rows[0]._id).toEqual(row._id) }) @@ -217,6 +218,60 @@ describe("/permission", () => { }) }) + describe("view permissions", () => { + let tableId: string + let viewId: string + + beforeEach(async () => { + const table = await config.createTable() + tableId = table._id! + + const view = await config.api.viewV2.create({ + tableId, + name: generator.guid(), + }) + viewId = view.id + }) + + it("default permissions inherits the table default value", async () => { + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + write: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) + + it("default permissions inherits explicit table permissions", async () => { + await config.api.permission.add({ + roleId: STD_ROLE_ID, + resourceId: tableId, + level: PermissionLevel.READ, + }) + + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: STD_ROLE_ID, + inheritablePermission: STD_ROLE_ID, + }, + write: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) + }) + describe("fetch builtins", () => { it("should be able to fetch builtin definitions", async () => { const res = await request diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 44f6beedb1..36d6dd6f85 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -3,7 +3,6 @@ import { canGroupBy, FieldType, isNumeric, - PermissionLevel, RelationSchemaField, RenameColumn, Table, @@ -13,7 +12,7 @@ import { ViewV2ColumnEnriched, ViewV2Enriched, } from "@budibase/types" -import { context, docIds, HTTPError, roles } from "@budibase/backend-core" +import { context, docIds, HTTPError } from "@budibase/backend-core" import { helpers, PROTECTED_EXTERNAL_COLUMNS, @@ -26,7 +25,6 @@ import { isExternalTableID } from "../../../integrations/utils" import * as internal from "./internal" import * as external from "./external" import sdk from "../../../sdk" -import { PermissionUpdateType, updatePermissionOnRole } from "../permissions" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { @@ -245,27 +243,6 @@ export async function create( const view = await 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 } From 389c4a94673ec21a6bb37badeaeb81261e1413a9 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:49:45 +0200 Subject: [PATCH 13/20] Add more tests --- .../src/api/routes/tests/permissions.spec.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 5dfece3126..44b57cf7cb 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -270,6 +270,28 @@ describe("/permission", () => { }, }) }) + + it("can sets permissions inherits explicit view permissions", async () => { + await config.api.permission.add({ + roleId: HIGHER_ROLE_ID, + resourceId: viewId, + level: PermissionLevel.WRITE, + }) + + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + write: { + permissionType: "EXPLICIT", + role: HIGHER_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) }) describe("fetch builtins", () => { From b3efea95bfc300347228377490f97f15481f2455 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 12:31:47 +0200 Subject: [PATCH 14/20] Undo base permissions --- packages/backend-core/src/security/permissions.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index aaf83f78df..4ed2cd3954 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -84,8 +84,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read only", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.READ), - // TODO: don't add "breaking changes" - // new Permission(PermissionType.TABLE, PermissionLevel.READ), + new Permission(PermissionType.TABLE, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), ], }, @@ -94,7 +93,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read/Write", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.WRITE), - // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.LEGACY_VIEW, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), @@ -104,7 +103,7 @@ export const BUILTIN_PERMISSIONS: { _id: BuiltinPermissionID.POWER, name: "Power", permissions: [ - // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.USER, PermissionLevel.READ), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.WEBHOOK, PermissionLevel.READ), From d01462221fb8e8da9e83bc3b5bf171b30a7bff44 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:15:02 +0200 Subject: [PATCH 15/20] Set default permissions --- .../server/src/api/controllers/permission.ts | 9 ++++----- .../src/api/routes/tests/permissions.spec.ts | 20 +++++++------------ .../server/src/sdk/app/permissions/index.ts | 20 +++++++++++++++++++ packages/server/src/sdk/app/tables/create.ts | 8 ++++++++ packages/types/src/api/web/app/permission.ts | 4 ++-- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index a58d94ce80..ead48d3db8 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -94,18 +94,17 @@ export async function getDependantResources( export async function addPermission(ctx: UserCtx) { const params: AddPermissionRequest = ctx.params - ctx.body = await sdk.permissions.updatePermissionOnRole( - params, - PermissionUpdateType.ADD - ) + await sdk.permissions.updatePermissionOnRole(params, PermissionUpdateType.ADD) + ctx.status = 200 } export async function removePermission( ctx: UserCtx ) { const params: RemovePermissionRequest = ctx.params - ctx.body = await sdk.permissions.updatePermissionOnRole( + await sdk.permissions.updatePermissionOnRole( params, PermissionUpdateType.REMOVE ) + ctx.status = 200 } diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 44b57cf7cb..180f91fb42 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -42,12 +42,11 @@ describe("/permission", () => { describe("table permissions", () => { let tableId: string - let perms: Document[] beforeEach(async () => { const table = await config.createTable() tableId = table._id! - perms = await config.api.permission.add({ + await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, level: PermissionLevel.READ, @@ -59,11 +58,11 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(table._id!) expect(permissions).toEqual({ read: { - permissionType: "BASE", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, }, write: { - permissionType: "BASE", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, }, }) @@ -71,11 +70,6 @@ describe("/permission", () => { 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}`) - }) - - it("should get the resource permissions", async () => { const res = await request .get(`/api/permission/${tableId}`) .set(config.defaultHeaders()) @@ -84,13 +78,13 @@ describe("/permission", () => { expect(res.body).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: DEFAULT_TABLE_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID }, }, }) }) it("should get resource permissions with multiple roles", async () => { - perms = await config.api.permission.add({ + await config.api.permission.add({ roleId: HIGHER_ROLE_ID, resourceId: tableId, level: PermissionLevel.WRITE, @@ -115,12 +109,12 @@ describe("/permission", () => { describe("remove", () => { it("should be able to remove the permission", async () => { - const res = await config.api.permission.revoke({ + await config.api.permission.revoke({ roleId: STD_ROLE_ID, resourceId: tableId, level: PermissionLevel.READ, }) - expect(res[0]._id).toEqual(STD_ROLE_ID) + const permsRes = await config.api.permission.get(tableId) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) diff --git a/packages/server/src/sdk/app/permissions/index.ts b/packages/server/src/sdk/app/permissions/index.ts index 2c3c0af95b..97af9ccc83 100644 --- a/packages/server/src/sdk/app/permissions/index.ts +++ b/packages/server/src/sdk/app/permissions/index.ts @@ -185,6 +185,26 @@ export async function updatePermissionOnRole( }) } +export async function setPermissions( + resourceId: string, + { + writeRole, + readRole, + }: { + writeRole: string + readRole: string + } +) { + await updatePermissionOnRole( + { roleId: writeRole, resourceId, level: PermissionLevel.WRITE }, + PermissionUpdateType.ADD + ) + await updatePermissionOnRole( + { roleId: readRole, resourceId, level: PermissionLevel.READ }, + PermissionUpdateType.ADD + ) +} + // utility function to stop this repetition - permissions always stored under roles export async function getAllDBRoles(db: Database) { const body = await db.allDocs( diff --git a/packages/server/src/sdk/app/tables/create.ts b/packages/server/src/sdk/app/tables/create.ts index ed6d6baeb0..0b15cdb15a 100644 --- a/packages/server/src/sdk/app/tables/create.ts +++ b/packages/server/src/sdk/app/tables/create.ts @@ -3,6 +3,8 @@ import { Row, Table } from "@budibase/types" import * as external from "./external" import * as internal from "./internal" import { isExternal } from "./utils" +import { setPermissions } from "../permissions" +import { roles } from "@budibase/backend-core" export async function create( table: Omit, @@ -15,5 +17,11 @@ export async function create( } else { createdTable = await internal.create(table, rows, userId) } + + await setPermissions(createdTable._id!, { + writeRole: roles.BUILTIN_ROLE_IDS.ADMIN, + readRole: roles.BUILTIN_ROLE_IDS.ADMIN, + }) + return createdTable } diff --git a/packages/types/src/api/web/app/permission.ts b/packages/types/src/api/web/app/permission.ts index a5c4df5733..b40310f21c 100644 --- a/packages/types/src/api/web/app/permission.ts +++ b/packages/types/src/api/web/app/permission.ts @@ -25,7 +25,7 @@ export interface AddedPermission { reason?: string } -export type AddPermissionResponse = AddedPermission[] +export interface AddPermissionResponse {} export interface AddPermissionRequest { roleId: string @@ -34,4 +34,4 @@ export interface AddPermissionRequest { } export interface RemovePermissionRequest extends AddPermissionRequest {} -export interface RemovePermissionResponse extends AddPermissionResponse {} +export interface RemovePermissionResponse {} From c84cda40b33c2c082b7dee171fc7c4df7bc3309a Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:36:37 +0200 Subject: [PATCH 16/20] Set default permissions to view --- .../src/api/routes/tests/permissions.spec.ts | 16 ++++++++-------- packages/server/src/sdk/app/views/index.ts | 8 ++++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 180f91fb42..a479adb4cf 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -227,23 +227,23 @@ describe("/permission", () => { viewId = view.id }) - it("default permissions inherits the table default value", async () => { + it("default permissions inherits and persists the table default value", async () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, write: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, }) }) - it("default permissions inherits explicit table permissions", async () => { + it("does not update view permissions once persisted, even if table permissions change", async () => { await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -253,12 +253,12 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", - role: STD_ROLE_ID, + permissionType: "EXPLICIT", + role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: STD_ROLE_ID, }, write: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, @@ -275,7 +275,7 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 36d6dd6f85..629454fecc 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -3,6 +3,7 @@ import { canGroupBy, FieldType, isNumeric, + PermissionLevel, RelationSchemaField, RenameColumn, Table, @@ -243,6 +244,13 @@ export async function create( const view = await pickApi(tableId).create(tableId, viewRequest) + // Set permissions to be the same as the table + const tablePerms = await sdk.permissions.getResourcePerms(tableId) + await sdk.permissions.setPermissions(view.id, { + writeRole: tablePerms[PermissionLevel.WRITE].role, + readRole: tablePerms[PermissionLevel.READ].role, + }) + return view } From 85ef2f1d2c6e748cf4a7e3d689b292938ecdef7d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:38:01 +0200 Subject: [PATCH 17/20] Fix build issue --- packages/backend-core/src/security/roles.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 9ae3788aab..65339832cf 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -42,7 +42,7 @@ export class Role implements RoleDoc { _id: string _rev?: string name: string - permissionId: BuiltinPermissionID + permissionId: string inherits?: string version?: string permissions: Record = {} @@ -51,7 +51,7 @@ export class Role implements RoleDoc { constructor( id: string, name: string, - permissionId: BuiltinPermissionID, + permissionId: string, uiMetadata?: RoleUIMetadata ) { this._id = id From 92372d1a4bbe0a164e91686a8bf1191f1724a449 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:48:20 +0200 Subject: [PATCH 18/20] Fix test --- packages/server/src/api/routes/tests/table.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 60e8142522..362ea24f8a 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -203,10 +203,6 @@ describe.each([ permissionType: PermissionSource.EXPLICIT, role: roles.BUILTIN_ROLE_IDS.ADMIN, }, - execute: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, }) }) }) From a8476a1c8bc3b1fb8e27c6b9b3a53f6dc7ae3b73 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 16:30:48 +0200 Subject: [PATCH 19/20] Fix tests --- packages/server/src/migrations/tests/index.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/migrations/tests/index.spec.ts b/packages/server/src/migrations/tests/index.spec.ts index d06cd37b69..6b3f3314ba 100644 --- a/packages/server/src/migrations/tests/index.spec.ts +++ b/packages/server/src/migrations/tests/index.spec.ts @@ -71,7 +71,7 @@ describe("migrations", () => { expect(events.datasource.created).toHaveBeenCalledTimes(2) expect(events.layout.created).toHaveBeenCalledTimes(1) expect(events.query.created).toHaveBeenCalledTimes(2) - expect(events.role.created).toHaveBeenCalledTimes(2) + expect(events.role.created).toHaveBeenCalledTimes(3) // created roles + admin (created on table creation) expect(events.table.created).toHaveBeenCalledTimes(3) expect(events.view.created).toHaveBeenCalledTimes(2) expect(events.view.calculationCreated).toHaveBeenCalledTimes(1) @@ -82,7 +82,7 @@ describe("migrations", () => { // to make sure caching is working as expected expect( events.processors.analyticsProcessor.processEvent - ).toHaveBeenCalledTimes(23) + ).toHaveBeenCalledTimes(24) // Addtion of of the events above }) }) }) From 6a0f80f28e71891a8ea4c3458b46a66db19839de Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 17:13:46 +0200 Subject: [PATCH 20/20] Remove duplicated test --- .../server/src/api/routes/tests/table.spec.ts | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 362ea24f8a..d6c1651ef0 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,4 @@ -import { context, docIds, events, roles } from "@budibase/backend-core" +import { context, docIds, events } from "@budibase/backend-core" import { PROTECTED_EXTERNAL_COLUMNS, PROTECTED_INTERNAL_COLUMNS, @@ -21,7 +21,6 @@ import { ViewCalculation, ViewV2Enriched, RowExportFormat, - PermissionSource, } from "@budibase/types" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" @@ -190,21 +189,6 @@ describe.each([ ) } ) - - it("should create tables with ADMIN read and write permissions", async () => { - const table = await config.api.table.save(tableForDatasource(datasource)) - const { permissions } = await config.api.permission.get(table._id!) - expect(permissions).toEqual({ - read: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, - write: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, - }) - }) }) describe("update", () => {