diff --git a/packages/server/src/api/routes/row.ts b/packages/server/src/api/routes/row.ts index a4ac8aa3ee..c29cb65eac 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" + const { PermissionType, PermissionLevel } = permissions const router: Router = new Router() @@ -269,7 +270,7 @@ router router.post( "/api/v2/views/:viewId/search", - authorized(PermissionType.TABLE, PermissionLevel.READ), + authorizedResource(PermissionType.VIEW, PermissionLevel.READ, "viewId"), rowController.views.searchView ) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 118d35f8fd..3fa21cb677 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,8 +12,10 @@ import { PermissionLevel, Row, Table, + 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 @@ -27,6 +29,7 @@ describe("/permission", () => { let table: Table & { _id: string } let perms: Document[] let row: Row + let view: ViewV2 afterAll(setup.afterAll) @@ -35,10 +38,12 @@ describe("/permission", () => { }) beforeEach(async () => { + mocks.licenses.useCloudFree() mockedSdk.resourceActionAllowed.mockResolvedValue({ allowed: true }) 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 +167,72 @@ 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 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, + }) + // replicate changes before checking permissions + await config.publish() + + await config.api.viewV2.search(view.id, undefined, { + expectStatus: 403, + usePublicUser: true, + }) + }) + + 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`) diff --git a/packages/server/src/middleware/authorized.ts b/packages/server/src/middleware/authorized.ts index 915344f747..35d373efbf 100644 --- a/packages/server/src/middleware/authorized.ts +++ b/packages/server/src/middleware/authorized.ts @@ -6,8 +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 @@ -74,10 +77,37 @@ const checkAuthorizedResource = async ( } } -export default ( +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, permLevel?: PermissionLevel, - opts = { schema: false } + opts = { schema: false }, + resourcePath?: string ) => async (ctx: any, next: any) => { // webhooks don't need authentication, each webhook unique @@ -97,11 +127,27 @@ export default ( permLevel === PermissionLevel.READ ? PermissionLevel.WRITE : PermissionLevel.READ - const appId = context.getAppId() - if (appId && hasResource(ctx)) { - resourceRoles = await roles.getRequiredResourceRole(permLevel!, ctx) + + if (resourcePath) { + // Reusing the existing middleware to extract the value + paramResource(resourcePath)(ctx, () => {}) + } + + if (resourceIdTranformers[permType]) { + await resourceIdTranformers[permType]!(ctx) + } + + if (hasResource(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, + }) } } @@ -143,3 +189,17 @@ 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, + resourcePath: string +) => { + return authorized(permType, permLevel, undefined, resourcePath) +} 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() } diff --git a/packages/server/src/middleware/tests/authorized.spec.js b/packages/server/src/middleware/tests/authorized.spec.js deleted file mode 100644 index 3adc4d99a1..0000000000 --- a/packages/server/src/middleware/tests/authorized.spec.js +++ /dev/null @@ -1,163 +0,0 @@ -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") - -const APP_ID = "" - -class TestConfiguration { - constructor(role) { - this.middleware = authorizedMiddleware(role) - this.next = jest.fn() - this.throw = jest.fn() - this.headers = {} - this.ctx = { - headers: {}, - request: { - url: "" - }, - appId: APP_ID, - auth: {}, - next: this.next, - throw: this.throw, - get: (name) => this.headers[name], - } - } - - executeMiddleware() { - return this.middleware(this.ctx, this.next) - } - - setUser(user) { - this.ctx.user = user - } - - setMiddlewareRequiredPermission(...perms) { - this.middleware = authorizedMiddleware(...perms) - } - - setResourceId(id) { - this.ctx.resourceId = id - } - - setAuthenticated(isAuthed) { - this.ctx.isAuthenticated = isAuthed - } - - setRequestUrl(url) { - this.ctx.request.url = url - } - - setEnvironment(isProd) { - env._set("NODE_ENV", isProd ? "production" : "jest") - } - - setRequestHeaders(headers) { - this.ctx.headers = headers - } - - afterEach() { - jest.clearAllMocks() - } -} - - -describe("Authorization middleware", () => { - const next = jest.fn() - let config - - afterEach(() => { - config.afterEach() - }) - - beforeEach(() => { - config = new TestConfiguration() - }) - - describe("non-webhook call", () => { - let config - - beforeEach(() => { - config = new TestConfiguration() - config.setEnvironment(true) - config.setAuthenticated(true) - }) - - it("throws when no user data is present in context", async () => { - await config.executeMiddleware() - - expect(config.throw).toHaveBeenCalledWith(403, "No user info found") - }) - - it("passes on to next() middleware if user is an admin", async () => { - config.setUser({ - _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: "" - } - }) - await config.executeMiddleware() - - expect(config.throw).toHaveBeenCalledWith(403, "Not Authorized") - }) - - it("passes on to next() middleware if the user has resource permission", async () => { - config.setResourceId(PermissionType.QUERY) - config.setUser({ - role: { - _id: "" - } - }) - config.setMiddlewareRequiredPermission(PermissionType.QUERY) - - await config.executeMiddleware() - expect(config.next).toHaveBeenCalled() - }) - - it("throws if the user session is not authenticated", async () => { - config.setUser({ - role: { - _id: "" - }, - }) - config.setAuthenticated(false) - - await config.executeMiddleware() - 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: "" - }, - }) - config.setMiddlewareRequiredPermission(PermissionType.ADMIN, PermissionLevel.BASIC) - - await config.executeMiddleware() - expect(config.throw).toHaveBeenCalledWith(403, "User does not have permission") - }) - }) -}) diff --git a/packages/server/src/middleware/tests/authorized.spec.ts b/packages/server/src/middleware/tests/authorized.spec.ts new file mode 100644 index 0000000000..d47430e63f --- /dev/null +++ b/packages/server/src/middleware/tests/authorized.spec.ts @@ -0,0 +1,272 @@ +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" + }, +})) + +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 + 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: "", + }, + appId: APP_ID, + auth: {}, + next: this.next, + throw: this.throw, + get: (name: string) => this.headers[name], + } + } + + executeMiddleware() { + return this.middleware(this.ctx, this.next) + } + + setUser(user: any) { + this.ctx.user = user + } + + setMiddlewareRequiredPermission(...perms: any[]) { + // @ts-ignore + this.middleware = authorizedMiddleware(...perms) + } + + setResourceId(id?: string) { + this.ctx.resourceId = id + } + + setAuthenticated(isAuthed: boolean) { + this.ctx.isAuthenticated = isAuthed + } + + setRequestUrl(url: string) { + this.ctx.request.url = url + } + + setEnvironment(isProd: boolean) { + env._set("NODE_ENV", isProd ? "production" : "jest") + } + + setRequestHeaders(headers: Record) { + this.ctx.headers = headers + } + + afterEach() { + jest.clearAllMocks() + } +} + +describe("Authorization middleware", () => { + let config: TestConfiguration + + afterEach(() => { + config.afterEach() + }) + + beforeEach(() => { + jest.clearAllMocks() + mocks.licenses.useCloudFree() + config = new TestConfiguration() + }) + + describe("non-webhook call", () => { + beforeEach(() => { + config = new TestConfiguration() + config.setEnvironment(true) + config.setAuthenticated(true) + }) + + it("throws when no user data is present in context", async () => { + await config.executeMiddleware() + + expect(config.throw).toHaveBeenCalledWith(403, "No user info found") + }) + + it("passes on to next() middleware if user is an admin", async () => { + config.setUser({ + _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: "", + }, + }) + await config.executeMiddleware() + + expect(config.throw).toHaveBeenCalledWith(403, "Not Authorized") + }) + + it("passes on to next() middleware if the user has resource permission", async () => { + config.setResourceId(PermissionType.QUERY) + config.setUser({ + role: { + _id: "", + }, + }) + config.setMiddlewareRequiredPermission(PermissionType.QUERY) + + await config.executeMiddleware() + expect(config.next).toHaveBeenCalled() + }) + + it("throws if the user session is not authenticated", async () => { + config.setUser({ + role: { + _id: "", + }, + }) + config.setAuthenticated(false) + + await config.executeMiddleware() + 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: "", + }, + }) + config.setMiddlewareRequiredPermission( + PermissionType.APP, + PermissionLevel.READ + ) + + await config.executeMiddleware() + expect(config.throw).toHaveBeenCalledWith( + 403, + "User does not have permission" + ) + }) + + describe("view type", () => { + const tableId = generateTableID() + const viewId = generateViewID(tableId) + + const mockedGetRequiredResourceRole = + roles.getRequiredResourceRole as jest.MockedFunction< + typeof roles.getRequiredResourceRole + > + + beforeEach(() => { + config.setMiddlewareRequiredPermission( + PermissionType.VIEW, + PermissionLevel.READ + ) + config.setResourceId(viewId) + + mockedGetRequiredResourceRole.mockResolvedValue(["PUBLIC"]) + + config.setUser({ + _id: "user", + role: { + _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() + 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` + ) + }) + }) + }) +}) 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() }) 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) } 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() +} 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", }