From fc0ee3f462bf1eac4d257bef3bece8a74bf8c6a4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Sep 2023 13:23:21 +0200 Subject: [PATCH 1/5] Types --- packages/backend-core/src/db/db.ts | 6 +++++- packages/backend-core/src/security/roles.ts | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/db/db.ts b/packages/backend-core/src/db/db.ts index f13eb9a965..9aae64b892 100644 --- a/packages/backend-core/src/db/db.ts +++ b/packages/backend-core/src/db/db.ts @@ -11,7 +11,11 @@ export function getDB(dbName?: string, opts?: any): Database { // we have to use a callback for this so that we can close // the DB when we're done, without this manual requests would // need to close the database when done with it to avoid memory leaks -export async function doWithDB(dbName: string, cb: any, opts = {}) { +export async function doWithDB( + dbName: string, + cb: (db: Database) => Promise, + opts = {} +) { const db = getDB(dbName, opts) // need this to be async so that we can correctly close DB after all // async operations have been completed diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 081193b433..f17db380f1 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -253,7 +253,7 @@ export function checkForRoleResourceArray( * Given an app ID this will retrieve all of the roles that are currently within that app. * @return {Promise} An array of the role objects that were found. */ -export async function getAllRoles(appId?: string) { +export async function getAllRoles(appId?: string): Promise { if (appId) { return doWithDB(appId, internal) } else { From f6329e6a22959ef36a5d74732c1370fafd62db77 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Sep 2023 14:12:37 +0200 Subject: [PATCH 2/5] Add tests --- .../server/src/api/routes/tests/row.spec.ts | 84 ++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index f28b96b9d8..7c2e425980 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1,11 +1,12 @@ import tk from "timekeeper" import { outputProcessing } from "../../../utilities/rowProcessor" import * as setup from "./utilities" -import { context, tenancy } from "@budibase/backend-core" +import { context, roles, tenancy } from "@budibase/backend-core" import { quotas } from "@budibase/pro" import { FieldType, MonthlyQuotaName, + PermissionLevel, QuotaUsageType, Row, SortOrder, @@ -16,6 +17,7 @@ import { import { expectAnyInternalColsAttributes, generator, + mocks, structures, } from "@budibase/backend-core/tests" @@ -37,6 +39,7 @@ describe("/rows", () => { }) beforeEach(async () => { + mocks.licenses.useCloudFree() table = await config.createTable() row = basicRow(table._id!) }) @@ -1314,6 +1317,85 @@ describe("/rows", () => { bookmark: expect.any(String), }) }) + + describe("permissions", () => { + let viewId: string + let tableId: string + + beforeAll(async () => { + const table = await config.createTable(userTable()) + const rows = [] + for (let i = 0; i < 10; i++) { + rows.push(await config.createRow({ tableId: table._id })) + } + + const createViewResponse = await config.api.viewV2.create() + + tableId = table._id! + viewId = createViewResponse.id + }) + + beforeEach(() => { + mocks.licenses.useViewPermissions() + }) + + it("does not allow public users to fetch by default", async () => { + await config.publish() + await config.api.viewV2.search(viewId, undefined, { + expectStatus: 403, + usePublicUser: true, + }) + }) + + it("allow public users to fetch when permissions are explicit", async () => { + await config.api.permission.set({ + roleId: roles.BUILTIN_ROLE_IDS.PUBLIC, + level: PermissionLevel.READ, + resourceId: viewId, + }) + await config.publish() + + const response = await config.api.viewV2.search(viewId, undefined, { + usePublicUser: true, + }) + + expect(response.body.rows).toHaveLength(10) + }) + + it("allow public users to fetch when permissions are inherited", async () => { + await config.api.permission.set({ + roleId: roles.BUILTIN_ROLE_IDS.PUBLIC, + level: PermissionLevel.READ, + resourceId: tableId, + }) + await config.publish() + + const response = await config.api.viewV2.search(viewId, undefined, { + usePublicUser: true, + }) + + expect(response.body.rows).toHaveLength(10) + }) + + it("respects inherited permissions, not allowing not public views from public tables", async () => { + await config.api.permission.set({ + roleId: roles.BUILTIN_ROLE_IDS.PUBLIC, + level: PermissionLevel.READ, + resourceId: tableId, + }) + await config.api.permission.set({ + roleId: roles.BUILTIN_ROLE_IDS.POWER, + level: PermissionLevel.READ, + resourceId: viewId, + }) + await config.publish() + + await config.api.viewV2.search(viewId, undefined, { + usePublicUser: true, + expectStatus: 403, + }) + }) + }) }) }) }) From ad8fb0165703ba15ca051339266a3ec249822229 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Sep 2023 14:13:10 +0200 Subject: [PATCH 3/5] Use permissions sdk on authorized middleware --- packages/server/src/middleware/authorized.ts | 63 +++++++------------- 1 file changed, 22 insertions(+), 41 deletions(-) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 35d373efbf..b2ffeacaf8 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -6,11 +6,10 @@ import { users, } from "@budibase/backend-core" import { PermissionLevel, PermissionType, Role, UserCtx } from "@budibase/types" -import { features } from "@budibase/pro" import builderMiddleware from "./builder" import { isWebhookEndpoint } from "./utils" import { paramResource } from "./resourceId" -import { extractViewInfoFromID, isViewID } from "../db/utils" +import sdk from "../sdk" function hasResource(ctx: any) { return ctx.resourceId != null @@ -77,31 +76,6 @@ const checkAuthorizedResource = async ( } } -const resourceIdTranformers: Partial< - Record Promise> -> = { - [PermissionType.VIEW]: async ctx => { - const { resourceId } = ctx - if (!resourceId) { - ctx.throw(400, `Cannot obtain the view id`) - return - } - - if (!isViewID(resourceId)) { - ctx.throw(400, `"${resourceId}" is not a valid view id`) - return - } - - if (await features.isViewPermissionEnabled()) { - ctx.subResourceId = ctx.resourceId - ctx.resourceId = extractViewInfoFromID(resourceId).tableId - } else { - ctx.resourceId = extractViewInfoFromID(resourceId).tableId - delete ctx.subResourceId - } - }, -} - const authorized = ( permType: PermissionType, @@ -121,8 +95,8 @@ const authorized = } // get the resource roles - let resourceRoles: any = [] - let otherLevelRoles: any = [] + let resourceRoles: string[] = [] + let otherLevelRoles: string[] = [] const otherLevel = permLevel === PermissionLevel.READ ? PermissionLevel.WRITE @@ -133,21 +107,28 @@ const authorized = paramResource(resourcePath)(ctx, () => {}) } - if (resourceIdTranformers[permType]) { - await resourceIdTranformers[permType]!(ctx) - } - if (hasResource(ctx)) { const { resourceId, subResourceId } = ctx - resourceRoles = await roles.getRequiredResourceRole(permLevel!, { - resourceId, - subResourceId, - }) + + const permissions = await sdk.permissions.getResourcePerms(resourceId) + const subPermissions = + !!subResourceId && + (await sdk.permissions.getResourcePerms(subResourceId)) + + function getPermLevel(permLevel: string) { + let result: string[] = [] + if (permissions[permLevel]) { + result.push(permissions[permLevel].role) + } + if (subPermissions && subPermissions[permLevel]) { + result.push(subPermissions[permLevel].role) + } + return result + } + + resourceRoles = getPermLevel(permLevel!) if (opts && opts.schema) { - otherLevelRoles = await roles.getRequiredResourceRole(otherLevel, { - resourceId, - subResourceId, - }) + otherLevelRoles = getPermLevel(otherLevel!) } } From d16110ca7336d75d55d048a2dcc9878d7b152863 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Sep 2023 15:49:58 +0200 Subject: [PATCH 4/5] Fix tests --- .../src/middleware/tests/authorized.spec.ts | 100 +++++------------- 1 file changed, 26 insertions(+), 74 deletions(-) diff --git a/packages/server/src/middleware/tests/authorized.spec.ts b/packages/server/src/middleware/tests/authorized.spec.ts index d47430e63f..0741f50e4d 100644 --- a/packages/server/src/middleware/tests/authorized.spec.ts +++ b/packages/server/src/middleware/tests/authorized.spec.ts @@ -1,28 +1,20 @@ -jest.mock("@budibase/backend-core", () => ({ - ...jest.requireActual("@budibase/backend-core"), - roles: { - ...jest.requireActual("@budibase/backend-core").roles, - getRequiredResourceRole: jest.fn().mockResolvedValue([]), - }, -})) -jest.mock("../../environment", () => ({ - prod: false, - isTest: () => true, - // @ts-ignore - isProd: () => this.prod, - _set: function (_key: string, value: string) { - this.prod = value === "production" - }, +jest.mock("../../sdk/app/permissions", () => ({ + ...jest.requireActual("../../sdk/app/permissions"), + getResourcePerms: jest.fn().mockResolvedValue([]), })) -import { PermissionType, PermissionLevel } from "@budibase/types" +import { + PermissionType, + PermissionLevel, + PermissionSource, +} from "@budibase/types" import authorizedMiddleware from "../authorized" import env from "../../environment" import { generateTableID, generateViewID } from "../../db/utils" -import { roles } from "@budibase/backend-core" -import { mocks } from "@budibase/backend-core/tests" +import { generator, mocks } from "@budibase/backend-core/tests" import { initProMocks } from "../../tests/utilities/mocks/pro" +import { getResourcePerms } from "../../sdk/app/permissions" const APP_ID = "" @@ -189,23 +181,26 @@ describe("Authorization middleware", () => { ) }) - describe("view type", () => { - const tableId = generateTableID() - const viewId = generateViewID(tableId) - - const mockedGetRequiredResourceRole = - roles.getRequiredResourceRole as jest.MockedFunction< - typeof roles.getRequiredResourceRole - > + describe("with resource", () => { + let resourceId: string + const mockedGetResourcePerms = getResourcePerms as jest.MockedFunction< + typeof getResourcePerms + > beforeEach(() => { config.setMiddlewareRequiredPermission( PermissionType.VIEW, PermissionLevel.READ ) - config.setResourceId(viewId) + resourceId = generator.guid() + config.setResourceId(resourceId) - mockedGetRequiredResourceRole.mockResolvedValue(["PUBLIC"]) + mockedGetResourcePerms.mockResolvedValue({ + [PermissionLevel.READ]: { + role: "PUBLIC", + type: PermissionSource.BASE, + }, + }) config.setUser({ _id: "user", @@ -215,57 +210,14 @@ describe("Authorization middleware", () => { }) }) - it("will ignore view permissions if flag is off", async () => { + it("will fetch resource permissions when resource is set", async () => { await config.executeMiddleware() expect(config.throw).not.toBeCalled() expect(config.next).toHaveBeenCalled() - expect(mockedGetRequiredResourceRole).toBeCalledTimes(1) - expect(mockedGetRequiredResourceRole).toBeCalledWith( - PermissionLevel.READ, - expect.objectContaining({ - resourceId: tableId, - subResourceId: undefined, - }) - ) - }) - - it("will use view permissions if flag is on", async () => { - mocks.licenses.useViewPermissions() - await config.executeMiddleware() - - expect(config.throw).not.toBeCalled() - expect(config.next).toHaveBeenCalled() - - expect(mockedGetRequiredResourceRole).toBeCalledTimes(1) - expect(mockedGetRequiredResourceRole).toBeCalledWith( - PermissionLevel.READ, - expect.objectContaining({ - resourceId: tableId, - subResourceId: viewId, - }) - ) - }) - - it("throw an exception if the resource id is not provided", async () => { - config.setResourceId(undefined) - await config.executeMiddleware() - expect(config.throw).toHaveBeenNthCalledWith( - 1, - 400, - "Cannot obtain the view id" - ) - }) - - it("throw an exception if the resource id is not a valid view id", async () => { - config.setResourceId(tableId) - await config.executeMiddleware() - expect(config.throw).toHaveBeenNthCalledWith( - 1, - 400, - `"${tableId}" is not a valid view id` - ) + expect(mockedGetResourcePerms).toBeCalledTimes(1) + expect(mockedGetResourcePerms).toBeCalledWith(resourceId) }) }) }) From c5e061c9bfe248f940a963606adc9cbed31542db Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 4 Sep 2023 16:22:50 +0200 Subject: [PATCH 5/5] Fix types --- packages/server/src/sdk/app/backups/exports.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/sdk/app/backups/exports.ts b/packages/server/src/sdk/app/backups/exports.ts index 306918cf80..307cdf4015 100644 --- a/packages/server/src/sdk/app/backups/exports.ts +++ b/packages/server/src/sdk/app/backups/exports.ts @@ -60,7 +60,7 @@ function tarFilesToTmp(tmpDir: string, files: string[]) { export async function exportDB( dbName: string, opts: DBDumpOpts = {} -): Promise { +): Promise { const exportOpts = { filter: opts?.filter, batch_size: 1000,