From 9a15277fa1474dde4f435f189acc992000e3643c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 09:11:54 +0200 Subject: [PATCH 01/11] Split authorized middleware to handle resource id fetch --- packages/server/src/api/routes/row.ts | 6 ++++-- packages/server/src/middleware/authorized.ts | 14 +++++++++++++- packages/server/src/middleware/resourceId.ts | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/row.ts b/packages/server/src/api/routes/row.ts index a4ac8aa3ee..60bc4e0735 100644 --- a/packages/server/src/api/routes/row.ts +++ b/packages/server/src/api/routes/row.ts @@ -1,10 +1,11 @@ import Router from "@koa/router" import * as rowController from "../controllers/row" -import authorized from "../../middleware/authorized" +import authorized, { authorizedResource } from "../../middleware/authorized" import { paramResource, paramSubResource } from "../../middleware/resourceId" import { permissions } from "@budibase/backend-core" import { internalSearchValidator } from "./utils/validators" import trimViewRowInfo from "../../middleware/trimViewRowInfo" +import { extractViewInfoFromID } from "../../db/utils" const { PermissionType, PermissionLevel } = permissions const router: Router = new Router() @@ -269,7 +270,8 @@ router router.post( "/api/v2/views/:viewId/search", - authorized(PermissionType.TABLE, PermissionLevel.READ), + paramResource("viewId", val => extractViewInfoFromID(val).tableId), + authorizedResource(PermissionType.TABLE, PermissionLevel.READ), rowController.views.searchView ) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 915344f747..930fb0f0ea 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -74,7 +74,8 @@ const checkAuthorizedResource = async ( } } -export default ( +const authorized = + ( permType: PermissionType, permLevel?: PermissionLevel, opts = { schema: false } @@ -143,3 +144,14 @@ export default ( // csrf protection return csrf(ctx, next) } + +export default ( + permType: PermissionType, + permLevel?: PermissionLevel, + opts = { schema: false } +) => authorized(permType, permLevel, opts) + +export const authorizedResource = ( + permType: PermissionType, + permLevel?: PermissionLevel +) => authorized(permType, permLevel) diff --git a/packages/server/src/middleware/resourceId.ts b/packages/server/src/middleware/resourceId.ts index 0917941061..1ad0b2a0c1 100644 --- a/packages/server/src/middleware/resourceId.ts +++ b/packages/server/src/middleware/resourceId.ts @@ -43,6 +43,7 @@ export class ResourceIdGetter { } } +/** @deprecated we should use the authorizedResource middleware instead */ export function paramResource(main: string) { return new ResourceIdGetter("params").mainResource(main).build() } From b3802070645278b13a37793e53cc027d4ed801c1 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 09:36:44 +0200 Subject: [PATCH 02/11] Merge resource and authorized, allowing transformers --- packages/server/src/api/routes/row.ts | 8 ++++++-- packages/server/src/middleware/authorized.ts | 21 +++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/routes/row.ts b/packages/server/src/api/routes/row.ts index 60bc4e0735..9f4beb8173 100644 --- a/packages/server/src/api/routes/row.ts +++ b/packages/server/src/api/routes/row.ts @@ -270,8 +270,12 @@ router router.post( "/api/v2/views/:viewId/search", - paramResource("viewId", val => extractViewInfoFromID(val).tableId), - authorizedResource(PermissionType.TABLE, PermissionLevel.READ), + authorizedResource( + PermissionType.TABLE, + PermissionLevel.READ, + "viewId", + val => extractViewInfoFromID(val).tableId + ), rowController.views.searchView ) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 930fb0f0ea..a754bcc1f0 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -8,6 +8,7 @@ import { import { PermissionLevel, PermissionType, Role, UserCtx } from "@budibase/types" import builderMiddleware from "./builder" import { isWebhookEndpoint } from "./utils" +import { paramResource } from "./resourceId" function hasResource(ctx: any) { return ctx.resourceId != null @@ -78,7 +79,8 @@ const authorized = ( permType: PermissionType, permLevel?: PermissionLevel, - opts = { schema: false } + opts = { schema: false }, + resourceId?: { path: string; transformer?: (val: string) => string } ) => async (ctx: any, next: any) => { // webhooks don't need authentication, each webhook unique @@ -99,6 +101,15 @@ const authorized = ? PermissionLevel.WRITE : PermissionLevel.READ const appId = context.getAppId() + + if (resourceId?.path) { + // Reusing the existing middleware to extract the value + paramResource(resourceId.path)(ctx, () => {}) + if (resourceId.transformer) { + ctx.resourceId = resourceId.transformer(ctx.resourceId) + } + } + if (appId && hasResource(ctx)) { resourceRoles = await roles.getRequiredResourceRole(permLevel!, ctx) if (opts && opts.schema) { @@ -153,5 +164,9 @@ export default ( export const authorizedResource = ( permType: PermissionType, - permLevel?: PermissionLevel -) => authorized(permType, permLevel) + permLevel: PermissionLevel, + path: string, + transformer?: (val: string) => string +) => { + return authorized(permType, permLevel, undefined, { path, transformer }) +} From 972cc9916babfc6427624a0e042142f6985aecf6 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 09:39:38 +0200 Subject: [PATCH 03/11] Add inheritance tests --- .../src/api/routes/tests/permissions.spec.ts | 26 +++++++++++++++++++ .../server/src/tests/utilities/api/viewV2.ts | 8 ++++-- 2 files changed, 32 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 118d35f8fd..3437f65a46 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,6 +12,7 @@ import { PermissionLevel, Row, Table, + ViewV2, } from "@budibase/types" import * as setup from "./utilities" @@ -27,6 +28,7 @@ describe("/permission", () => { let table: Table & { _id: string } let perms: Document[] let row: Row + let view: ViewV2 afterAll(setup.afterAll) @@ -39,6 +41,7 @@ describe("/permission", () => { table = (await config.createTable()) as typeof table row = await config.createRow() + view = await config.api.viewV2.create({ tableId: table._id }) perms = await config.api.permission.set({ roleId: STD_ROLE_ID, resourceId: table._id, @@ -162,6 +165,29 @@ describe("/permission", () => { 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 () => { + // replicate changes before checking permissions + await config.publish() + + const res = await config.api.viewV2.search(view.id, undefined, { + usePublicUser: true, + }) + expect(res.body.rows[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 () => { + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + + await config.api.viewV2.search(view.id, undefined, { + expectStatus: 403, + usePublicUser: true, + }) + }) + it("shouldn't allow writing from a public user", async () => { const res = await request .post(`/api/${table._id}/rows`) diff --git a/packages/server/src/tests/utilities/api/viewV2.ts b/packages/server/src/tests/utilities/api/viewV2.ts index 1520154641..bba65e187f 100644 --- a/packages/server/src/tests/utilities/api/viewV2.ts +++ b/packages/server/src/tests/utilities/api/viewV2.ts @@ -77,12 +77,16 @@ export class ViewV2API extends TestAPI { search = async ( viewId: string, params?: SearchViewRowRequest, - { expectStatus } = { expectStatus: 200 } + { expectStatus = 200, usePublicUser = false } = {} ) => { return this.request .post(`/api/v2/views/${viewId}/search`) .send(params) - .set(this.config.defaultHeaders()) + .set( + usePublicUser + ? this.config.publicHeaders() + : this.config.defaultHeaders() + ) .expect("Content-Type", /json/) .expect(expectStatus) } From bfa2b491f349030c42c436fcb55a99c23fffa7ef Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 10:22:08 +0200 Subject: [PATCH 04/11] Allow view permission type --- packages/server/src/api/routes/row.ts | 9 ++--- packages/server/src/middleware/authorized.ts | 38 +++++++++++++++----- packages/types/src/sdk/permissions.ts | 1 + 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/packages/server/src/api/routes/row.ts b/packages/server/src/api/routes/row.ts index 9f4beb8173..c29cb65eac 100644 --- a/packages/server/src/api/routes/row.ts +++ b/packages/server/src/api/routes/row.ts @@ -5,7 +5,7 @@ import { paramResource, paramSubResource } from "../../middleware/resourceId" import { permissions } from "@budibase/backend-core" import { internalSearchValidator } from "./utils/validators" import trimViewRowInfo from "../../middleware/trimViewRowInfo" -import { extractViewInfoFromID } from "../../db/utils" + const { PermissionType, PermissionLevel } = permissions const router: Router = new Router() @@ -270,12 +270,7 @@ router router.post( "/api/v2/views/:viewId/search", - authorizedResource( - PermissionType.TABLE, - PermissionLevel.READ, - "viewId", - val => extractViewInfoFromID(val).tableId - ), + authorizedResource(PermissionType.VIEW, PermissionLevel.READ, "viewId"), rowController.views.searchView ) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index a754bcc1f0..3d4c44a108 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -6,9 +6,11 @@ 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" function hasResource(ctx: any) { return ctx.resourceId != null @@ -75,12 +77,31 @@ const checkAuthorizedResource = async ( } } +const resourceIdTranformers: Partial< + Record Promise> +> = { + [PermissionType.VIEW]: async ctx => { + const { resourceId } = ctx + if (!isViewID(resourceId)) { + ctx.throw(400, `"${resourceId}" is not a valid viewId`) + } + + 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, permLevel?: PermissionLevel, opts = { schema: false }, - resourceId?: { path: string; transformer?: (val: string) => string } + resourcePath?: string ) => async (ctx: any, next: any) => { // webhooks don't need authentication, each webhook unique @@ -102,15 +123,15 @@ const authorized = : PermissionLevel.READ const appId = context.getAppId() - if (resourceId?.path) { + if (resourcePath) { // Reusing the existing middleware to extract the value - paramResource(resourceId.path)(ctx, () => {}) - if (resourceId.transformer) { - ctx.resourceId = resourceId.transformer(ctx.resourceId) - } + paramResource(resourcePath)(ctx, () => {}) } if (appId && hasResource(ctx)) { + if (resourceIdTranformers[permType]) { + await resourceIdTranformers[permType]!(ctx) + } resourceRoles = await roles.getRequiredResourceRole(permLevel!, ctx) if (opts && opts.schema) { otherLevelRoles = await roles.getRequiredResourceRole(otherLevel, ctx) @@ -165,8 +186,7 @@ export default ( export const authorizedResource = ( permType: PermissionType, permLevel: PermissionLevel, - path: string, - transformer?: (val: string) => string + resourcePath: string ) => { - return authorized(permType, permLevel, undefined, { path, transformer }) + return authorized(permType, permLevel, undefined, resourcePath) } diff --git a/packages/types/src/sdk/permissions.ts b/packages/types/src/sdk/permissions.ts index 9fe1970e44..a33d4985ee 100644 --- a/packages/types/src/sdk/permissions.ts +++ b/packages/types/src/sdk/permissions.ts @@ -15,4 +15,5 @@ export enum PermissionType { BUILDER = "builder", GLOBAL_BUILDER = "globalBuilder", QUERY = "query", + VIEW = "view", } From 9a7a3b9c7209878ebdce0e4a3b374f0f10aa7d20 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 10:23:21 +0200 Subject: [PATCH 05/11] Rename test --- packages/server/src/api/routes/tests/permissions.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 3437f65a46..5261259b77 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -175,7 +175,7 @@ describe("/permission", () => { expect(res.body.rows[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 () => { + 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, From 84a6f239a93c6b96b5a1b409a7ff603077575be9 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:05:31 +0200 Subject: [PATCH 06/11] Migrate tests to ts --- ...{authorized.spec.js => authorized.spec.ts} | 91 +++++++++++-------- 1 file changed, 52 insertions(+), 39 deletions(-) rename packages/server/src/middleware/tests/{authorized.spec.js => authorized.spec.ts} (65%) diff --git a/packages/server/src/middleware/tests/authorized.spec.js b/packages/server/src/middleware/tests/authorized.spec.ts similarity index 65% rename from packages/server/src/middleware/tests/authorized.spec.js rename to packages/server/src/middleware/tests/authorized.spec.ts index 3adc4d99a1..900928f0bc 100644 --- a/packages/server/src/middleware/tests/authorized.spec.js +++ b/packages/server/src/middleware/tests/authorized.spec.ts @@ -1,34 +1,40 @@ jest.mock("../../environment", () => ({ - prod: false, - isTest: () => true, - isProd: () => this.prod, - _set: function(key, value) { - this.prod = value === "production" - } - }) -) -const authorizedMiddleware = require("../authorized").default -const env = require("../../environment") -const { PermissionType, PermissionLevel } = require("@budibase/types") + prod: false, + isTest: () => true, + // @ts-ignore + isProd: () => this.prod, + _set: function (key: string, value: string) { + this.prod = value === "production" + }, +})) +import authorizedMiddleware from "../authorized" +import env from "../../environment" +import { PermissionType, PermissionLevel } from "@budibase/types" const APP_ID = "" class TestConfiguration { - constructor(role) { - this.middleware = authorizedMiddleware(role) + middleware: (ctx: any, next: any) => Promise + next: () => void + throw: () => void + headers: Record + ctx: any + + constructor() { + this.middleware = authorizedMiddleware(PermissionType.APP) this.next = jest.fn() this.throw = jest.fn() this.headers = {} this.ctx = { headers: {}, request: { - url: "" + url: "", }, appId: APP_ID, auth: {}, next: this.next, throw: this.throw, - get: (name) => this.headers[name], + get: (name: string) => this.headers[name], } } @@ -36,32 +42,33 @@ class TestConfiguration { return this.middleware(this.ctx, this.next) } - setUser(user) { + setUser(user: any) { this.ctx.user = user } - setMiddlewareRequiredPermission(...perms) { + setMiddlewareRequiredPermission(...perms: any[]) { + // @ts-ignore this.middleware = authorizedMiddleware(...perms) } - setResourceId(id) { + setResourceId(id: string) { this.ctx.resourceId = id } - setAuthenticated(isAuthed) { + setAuthenticated(isAuthed: boolean) { this.ctx.isAuthenticated = isAuthed } - setRequestUrl(url) { + setRequestUrl(url: string) { this.ctx.request.url = url } - setEnvironment(isProd) { + setEnvironment(isProd: boolean) { env._set("NODE_ENV", isProd ? "production" : "jest") } - setRequestHeaders(headers) { - this.ctx.headers = headers + setRequestHeaders(headers: Record) { + this.ctx.headers = headers } afterEach() { @@ -69,10 +76,9 @@ class TestConfiguration { } } - describe("Authorization middleware", () => { const next = jest.fn() - let config + let config: TestConfiguration afterEach(() => { config.afterEach() @@ -83,8 +89,6 @@ describe("Authorization middleware", () => { }) describe("non-webhook call", () => { - let config - beforeEach(() => { config = new TestConfiguration() config.setEnvironment(true) @@ -102,21 +106,21 @@ describe("Authorization middleware", () => { _id: "user", role: { _id: "ADMIN", - } + }, }) await config.executeMiddleware() expect(config.next).toHaveBeenCalled() }) - + it("throws if the user does not have builder permissions", async () => { config.setEnvironment(false) config.setMiddlewareRequiredPermission(PermissionType.BUILDER) config.setUser({ role: { - _id: "" - } + _id: "", + }, }) await config.executeMiddleware() @@ -127,8 +131,8 @@ describe("Authorization middleware", () => { config.setResourceId(PermissionType.QUERY) config.setUser({ role: { - _id: "" - } + _id: "", + }, }) config.setMiddlewareRequiredPermission(PermissionType.QUERY) @@ -139,25 +143,34 @@ describe("Authorization middleware", () => { it("throws if the user session is not authenticated", async () => { config.setUser({ role: { - _id: "" + _id: "", }, }) config.setAuthenticated(false) await config.executeMiddleware() - expect(config.throw).toHaveBeenCalledWith(403, "Session not authenticated") + expect(config.throw).toHaveBeenCalledWith( + 403, + "Session not authenticated" + ) }) it("throws if the user does not have base permissions to perform the operation", async () => { config.setUser({ role: { - _id: "" + _id: "", }, }) - config.setMiddlewareRequiredPermission(PermissionType.ADMIN, PermissionLevel.BASIC) - + config.setMiddlewareRequiredPermission( + PermissionType.APP, + PermissionLevel.READ + ) + await config.executeMiddleware() - expect(config.throw).toHaveBeenCalledWith(403, "User does not have permission") + expect(config.throw).toHaveBeenCalledWith( + 403, + "User does not have permission" + ) }) }) }) From 8359185a22e704146eb7a48fad182241045adfa2 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:23:30 +0200 Subject: [PATCH 07/11] Add unhappy paths tests --- packages/server/src/middleware/authorized.ts | 18 ++++--- .../src/middleware/tests/authorized.spec.ts | 49 +++++++++++++++++-- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 3d4c44a108..52ced1b9cc 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -82,8 +82,14 @@ const resourceIdTranformers: Partial< > = { [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 viewId`) + ctx.throw(400, `"${resourceId}" is not a valid view id`) + return } if (await features.isViewPermissionEnabled()) { @@ -121,17 +127,17 @@ const authorized = permLevel === PermissionLevel.READ ? PermissionLevel.WRITE : PermissionLevel.READ - const appId = context.getAppId() if (resourcePath) { // Reusing the existing middleware to extract the value paramResource(resourcePath)(ctx, () => {}) } - if (appId && hasResource(ctx)) { - if (resourceIdTranformers[permType]) { - await resourceIdTranformers[permType]!(ctx) - } + if (resourceIdTranformers[permType]) { + await resourceIdTranformers[permType]!(ctx) + } + + if (hasResource(ctx)) { resourceRoles = await roles.getRequiredResourceRole(permLevel!, ctx) if (opts && opts.schema) { otherLevelRoles = await roles.getRequiredResourceRole(otherLevel, ctx) diff --git a/packages/server/src/middleware/tests/authorized.spec.ts b/packages/server/src/middleware/tests/authorized.spec.ts index 900928f0bc..4d6f281294 100644 --- a/packages/server/src/middleware/tests/authorized.spec.ts +++ b/packages/server/src/middleware/tests/authorized.spec.ts @@ -3,13 +3,16 @@ jest.mock("../../environment", () => ({ isTest: () => true, // @ts-ignore isProd: () => this.prod, - _set: function (key: string, value: string) { + _set: function (_key: string, value: string) { this.prod = value === "production" }, })) + +import { PermissionType, PermissionLevel } from "@budibase/types" + import authorizedMiddleware from "../authorized" import env from "../../environment" -import { PermissionType, PermissionLevel } from "@budibase/types" +import { generateTableID, generateViewID } from "../../db/utils" const APP_ID = "" @@ -51,7 +54,7 @@ class TestConfiguration { this.middleware = authorizedMiddleware(...perms) } - setResourceId(id: string) { + setResourceId(id?: string) { this.ctx.resourceId = id } @@ -85,6 +88,7 @@ describe("Authorization middleware", () => { }) beforeEach(() => { + jest.clearAllMocks() config = new TestConfiguration() }) @@ -172,5 +176,44 @@ describe("Authorization middleware", () => { "User does not have permission" ) }) + + describe("view type", () => { + const tableId = generateTableID() + const viewId = generateViewID(tableId) + + beforeEach(() => { + config.setMiddlewareRequiredPermission( + PermissionType.VIEW, + PermissionLevel.READ + ) + config.setResourceId(viewId) + + config.setUser({ + role: { + _id: "", + }, + }) + }) + + 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` + ) + }) + }) }) }) From cfeb6993cc82a973ac39efb1212ca07eaa85ae5e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:46:36 +0200 Subject: [PATCH 08/11] Test authorised view use cases --- packages/server/src/middleware/authorized.ts | 11 +++- .../src/middleware/tests/authorized.spec.ts | 57 ++++++++++++++++++- 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 52ced1b9cc..35d373efbf 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -138,9 +138,16 @@ const authorized = } if (hasResource(ctx)) { - resourceRoles = await roles.getRequiredResourceRole(permLevel!, ctx) + const { resourceId, subResourceId } = ctx + resourceRoles = await roles.getRequiredResourceRole(permLevel!, { + resourceId, + subResourceId, + }) if (opts && opts.schema) { - otherLevelRoles = await roles.getRequiredResourceRole(otherLevel, ctx) + otherLevelRoles = await roles.getRequiredResourceRole(otherLevel, { + resourceId, + subResourceId, + }) } } diff --git a/packages/server/src/middleware/tests/authorized.spec.ts b/packages/server/src/middleware/tests/authorized.spec.ts index 4d6f281294..d47430e63f 100644 --- a/packages/server/src/middleware/tests/authorized.spec.ts +++ b/packages/server/src/middleware/tests/authorized.spec.ts @@ -1,3 +1,10 @@ +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, @@ -13,9 +20,14 @@ import { PermissionType, PermissionLevel } 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 { initProMocks } from "../../tests/utilities/mocks/pro" const APP_ID = "" +initProMocks() + class TestConfiguration { middleware: (ctx: any, next: any) => Promise next: () => void @@ -80,7 +92,6 @@ class TestConfiguration { } describe("Authorization middleware", () => { - const next = jest.fn() let config: TestConfiguration afterEach(() => { @@ -89,6 +100,7 @@ describe("Authorization middleware", () => { beforeEach(() => { jest.clearAllMocks() + mocks.licenses.useCloudFree() config = new TestConfiguration() }) @@ -181,6 +193,11 @@ describe("Authorization middleware", () => { const tableId = generateTableID() const viewId = generateViewID(tableId) + const mockedGetRequiredResourceRole = + roles.getRequiredResourceRole as jest.MockedFunction< + typeof roles.getRequiredResourceRole + > + beforeEach(() => { config.setMiddlewareRequiredPermission( PermissionType.VIEW, @@ -188,13 +205,49 @@ describe("Authorization middleware", () => { ) config.setResourceId(viewId) + mockedGetRequiredResourceRole.mockResolvedValue(["PUBLIC"]) + config.setUser({ + _id: "user", role: { - _id: "", + _id: "PUBLIC", }, }) }) + it("will ignore view permissions if flag is off", 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() From 3e70369832dffc9c4a925267b44b5d5ac5e05f54 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:47:42 +0200 Subject: [PATCH 09/11] Use --- .../server/src/sdk/app/permissions/tests/permissions.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/server/src/sdk/app/permissions/tests/permissions.spec.ts b/packages/server/src/sdk/app/permissions/tests/permissions.spec.ts index 4c2768dde4..4c233e68fa 100644 --- a/packages/server/src/sdk/app/permissions/tests/permissions.spec.ts +++ b/packages/server/src/sdk/app/permissions/tests/permissions.spec.ts @@ -1,12 +1,13 @@ -import TestConfiguration from "../../../../tests/utilities/TestConfiguration" import { PermissionLevel } from "@budibase/types" import { mocks, structures } from "@budibase/backend-core/tests" import { resourceActionAllowed } from ".." import { generateViewID } from "../../../../db/utils" +import { initProMocks } from "../../../../tests/utilities/mocks/pro" + +initProMocks() describe("permissions sdk", () => { beforeEach(() => { - new TestConfiguration() mocks.licenses.useCloudFree() }) From 12be5a3d839cb03a463456c030cd0de98f927346 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:47:51 +0200 Subject: [PATCH 10/11] Setuo init pro mocks --- packages/server/src/tests/utilities/mocks/pro.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 packages/server/src/tests/utilities/mocks/pro.ts diff --git a/packages/server/src/tests/utilities/mocks/pro.ts b/packages/server/src/tests/utilities/mocks/pro.ts new file mode 100644 index 0000000000..a365ff40c4 --- /dev/null +++ b/packages/server/src/tests/utilities/mocks/pro.ts @@ -0,0 +1,10 @@ +// init the licensing mock +import { mocks } from "@budibase/backend-core/tests" +import * as pro from "@budibase/pro" + +export const initProMocks = () => { + mocks.licenses.init(pro) + + // use unlimited license by default + mocks.licenses.useUnlimited() +} From c0581408e9d4e592be3191b1d704234a228bccdd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 24 Aug 2023 11:48:38 +0200 Subject: [PATCH 11/11] Add extra tests --- .../src/api/routes/tests/permissions.spec.ts | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 5261259b77..3fa21cb677 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -15,6 +15,7 @@ import { ViewV2, } from "@budibase/types" import * as setup from "./utilities" +import { mocks } from "@budibase/backend-core/tests" const { basicRow } = setup.structures const { BUILTIN_ROLE_IDS } = roles @@ -37,6 +38,7 @@ describe("/permission", () => { }) beforeEach(async () => { + mocks.licenses.useCloudFree() mockedSdk.resourceActionAllowed.mockResolvedValue({ allowed: true }) table = (await config.createTable()) as typeof table @@ -181,6 +183,8 @@ describe("/permission", () => { resourceId: table._id, level: PermissionLevel.READ, }) + // replicate changes before checking permissions + await config.publish() await config.api.viewV2.search(view.id, undefined, { expectStatus: 403, @@ -188,6 +192,47 @@ describe("/permission", () => { }) }) + it("should ignore the view permissions if the flag is not on", async () => { + await config.api.permission.set({ + 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() + + await config.api.viewV2.search(view.id, undefined, { + expectStatus: 403, + usePublicUser: true, + }) + }) + + it("should use the view permissions if the flag is on", async () => { + mocks.licenses.useViewPermissions() + await config.api.permission.set({ + 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.search(view.id, undefined, { + usePublicUser: true, + }) + expect(res.body.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`)