Merge pull request #14511 from Budibase/BUDI-8429/row-action-view-security

Row action view security

packages/server/src/api/routes/tests/rowAction.spec.ts

@@ -5,11 +5,10 @@ import {
   CreateRowActionRequest,
   DocumentType,
   PermissionLevel,
-  Row,
   RowActionResponse,
 } from "@budibase/types"
 import * as setup from "./utilities"
-import { generator } from "@budibase/backend-core/tests"
+import { generator, mocks } from "@budibase/backend-core/tests"
 import { Expectations } from "../../../tests/utilities/api/base"
 import { roles } from "@budibase/backend-core"
 import { automations } from "@budibase/pro"
@@ -651,13 +650,27 @@ describe("/rowsActions", () => {
   })
 
   describe("trigger", () => {
-    let row: Row
+    let viewId: string
+    let rowId: string
     let rowAction: RowActionResponse
 
     beforeEach(async () => {
-      row = await config.api.row.save(tableId, {})
+      const row = await config.api.row.save(tableId, {})
+      rowId = row._id!
       rowAction = await createRowAction(tableId, createRowActionRequest())
 
+      viewId = (
+        await config.api.viewV2.create(
+          setup.structures.viewV2.createRequest(tableId)
+        )
+      ).id
+
+      await config.api.rowAction.setViewPermission(
+        tableId,
+        viewId,
+        rowAction.id
+      )
+
       await config.publish()
       tk.travel(Date.now() + 100)
     })
@@ -673,9 +686,7 @@ describe("/rowsActions", () => {
 
     it("can trigger an automation given valid data", async () => {
       expect(await getAutomationLogs()).toBeEmpty()
-      await config.api.rowAction.trigger(tableId, rowAction.id, {
+      await config.api.rowAction.trigger(viewId, rowAction.id, { rowId })
-        rowId: row._id!,
-      })
 
       const automationLogs = await getAutomationLogs()
       expect(automationLogs).toEqual([
@@ -687,8 +698,11 @@ describe("/rowsActions", () => {
             inputs: null,
             outputs: {
               fields: {},
-              row: await config.api.row.get(tableId, row._id!),
+              row: await config.api.row.get(tableId, rowId),
-              table: await config.api.table.get(tableId),
+              table: {
+                ...(await config.api.table.get(tableId)),
+                views: expect.anything(),
+              },
               automation: expect.objectContaining({
                 _id: rowAction.automationId,
               }),
@@ -709,9 +723,7 @@ describe("/rowsActions", () => {
       await config.api.rowAction.trigger(
         viewId,
         rowAction.id,
-        {
+        { rowId },
-          rowId: row._id!,
-        },
         {
           status: 403,
           body: {
@@ -738,10 +750,9 @@ describe("/rowsActions", () => {
       )
 
       await config.publish()
+
       expect(await getAutomationLogs()).toBeEmpty()
-      await config.api.rowAction.trigger(viewId, rowAction.id, {
+      await config.api.rowAction.trigger(viewId, rowAction.id, { rowId })
-        rowId: row._id!,
-      })
 
       const automationLogs = await getAutomationLogs()
       expect(automationLogs).toEqual([
@@ -750,5 +761,170 @@ describe("/rowsActions", () => {
         }),
       ])
     })
+
+    describe("role permission checks", () => {
+      beforeAll(() => {
+        mocks.licenses.useViewPermissions()
+      })
+
+      afterAll(() => {
+        mocks.licenses.useCloudFree()
+      })
+
+      function createUser(role: string) {
+        return config.createUser({
+          admin: { global: false },
+          builder: {},
+          roles: { [config.getProdAppId()]: role },
+        })
+      }
+
+      const allowedRoleConfig = (() => {
+        function getRolesLowerThan(role: string) {
+          const result = Object.values(roles.BUILTIN_ROLE_IDS).filter(
+            r => r !== role && roles.lowerBuiltinRoleID(r, role) === r
+          )
+          return result
+        }
+        return Object.values(roles.BUILTIN_ROLE_IDS).flatMap(r =>
+          [r, ...getRolesLowerThan(r)].map(p => [r, p])
+        )
+      })()
+
+      const disallowedRoleConfig = (() => {
+        function getRolesHigherThan(role: string) {
+          const result = Object.values(roles.BUILTIN_ROLE_IDS).filter(
+            r => r !== role && roles.lowerBuiltinRoleID(r, role) === role
+          )
+          return result
+        }
+        return Object.values(roles.BUILTIN_ROLE_IDS).flatMap(r =>
+          getRolesHigherThan(r).map(p => [r, p])
+        )
+      })()
+
+      describe.each([
+        [
+          "view (with implicit views)",
+          async () => {
+            const viewId = (
+              await config.api.viewV2.create(
+                setup.structures.viewV2.createRequest(tableId)
+              )
+            ).id
+
+            await config.api.rowAction.setViewPermission(
+              tableId,
+              viewId,
+              rowAction.id
+            )
+            return { permissionResource: viewId, triggerResouce: viewId }
+          },
+        ],
+        [
+          "view (without implicit views)",
+          async () => {
+            const viewId = (
+              await config.api.viewV2.create(
+                setup.structures.viewV2.createRequest(tableId)
+              )
+            ).id
+
+            await config.api.rowAction.setViewPermission(
+              tableId,
+              viewId,
+              rowAction.id
+            )
+            return { permissionResource: tableId, triggerResouce: viewId }
+          },
+        ],
+      ])("checks for %s", (_, getResources) => {
+        it.each(allowedRoleConfig)(
+          "allows triggering if the user has read permission (user %s, table %s)",
+          async (userRole, resourcePermission) => {
+            const { permissionResource, triggerResouce } = await getResources()
+
+            await config.api.permission.add({
+              level: PermissionLevel.READ,
+              resourceId: permissionResource,
+              roleId: resourcePermission,
+            })
+
+            const normalUser = await createUser(userRole)
+
+            await config.withUser(normalUser, async () => {
+              await config.publish()
+              await config.api.rowAction.trigger(
+                triggerResouce,
+                rowAction.id,
+                { rowId },
+                { status: 200 }
+              )
+            })
+          }
+        )
+
+        it.each(disallowedRoleConfig)(
+          "rejects if the user does not have table read permission (user %s, table %s)",
+          async (userRole, resourcePermission) => {
+            const { permissionResource, triggerResouce } = await getResources()
+            await config.api.permission.add({
+              level: PermissionLevel.READ,
+              resourceId: permissionResource,
+              roleId: resourcePermission,
+            })
+
+            const normalUser = await createUser(userRole)
+
+            await config.withUser(normalUser, async () => {
+              await config.publish()
+              await config.api.rowAction.trigger(
+                triggerResouce,
+                rowAction.id,
+                { rowId },
+                {
+                  status: 403,
+                  body: { message: "User does not have permission" },
+                }
+              )
+
+              const automationLogs = await getAutomationLogs()
+              expect(automationLogs).toBeEmpty()
+            })
+          }
+        )
+      })
+
+      it.each(allowedRoleConfig)(
+        "does not allow running row actions for tables by default even",
+        async (userRole, resourcePermission) => {
+          await config.api.permission.add({
+            level: PermissionLevel.READ,
+            resourceId: tableId,
+            roleId: resourcePermission,
+          })
+
+          const normalUser = await createUser(userRole)
+
+          await config.withUser(normalUser, async () => {
+            await config.publish()
+            await config.api.rowAction.trigger(
+              tableId,
+              rowAction.id,
+              { rowId },
+              {
+                status: 403,
+                body: {
+                  message: `Row action '${rowAction.id}' is not enabled for table '${tableId}'`,
+                },
+              }
+            )
+
+            const automationLogs = await getAutomationLogs()
+            expect(automationLogs).toBeEmpty()
+          })
+        }
+      )
+    })
   })
 })

packages/server/src/middleware/authorized.ts

@@ -88,7 +88,7 @@ const authorized =
     opts = { schema: false },
     resourcePath?: string
   ) =>
-  async (ctx: any, next: any) => {
+  async (ctx: UserCtx, next: any) => {
     // webhooks don't need authentication, each webhook unique
     // also internal requests (between services) don't need authorized
     if (isWebhookEndpoint(ctx) || ctx.internal) {

packages/server/src/middleware/triggerRowActionAuthorised.ts

@@ -1,40 +1,52 @@
 import { Next } from "koa"
-import { Ctx } from "@budibase/types"
+import { PermissionLevel, PermissionType, UserCtx } from "@budibase/types"
-import { paramSubResource } from "./resourceId"
 import { docIds } from "@budibase/backend-core"
 import * as utils from "../db/utils"
 import sdk from "../sdk"
+import { authorizedResource } from "./authorized"
 
 export function triggerRowActionAuthorised(
   sourcePath: string,
   actionPath: string
 ) {
-  return async (ctx: Ctx, next: Next) => {
+  return async (ctx: UserCtx, next: Next) => {
-    // Reusing the existing middleware to extract the value
+    async function getResourceIds() {
-    paramSubResource(sourcePath, actionPath)(ctx, () => {})
+      const sourceId: string = ctx.params[sourcePath]
-    const { resourceId: sourceId, subResourceId: rowActionId } = ctx
+      const rowActionId: string = ctx.params[actionPath]
 
-    const isTableId = docIds.isTableId(sourceId)
+      const isTableId = docIds.isTableId(sourceId)
-    const isViewId = utils.isViewID(sourceId)
+      const isViewId = utils.isViewID(sourceId)
-    if (!isTableId && !isViewId) {
+      if (!isTableId && !isViewId) {
-      ctx.throw(400, `'${sourceId}' is not a valid source id`)
+        ctx.throw(400, `'${sourceId}' is not a valid source id`)
+      }
+
+      const tableId = isTableId
+        ? sourceId
+        : utils.extractViewInfoFromID(sourceId).tableId
+      const viewId = isTableId ? undefined : sourceId
+      return { tableId, viewId, rowActionId }
     }
 
-    const tableId = isTableId
+    const { tableId, viewId, rowActionId } = await getResourceIds()
-      ? sourceId
-      : utils.extractViewInfoFromID(sourceId).tableId
 
+    // Check if the user has permissions to the table/view
+    await authorizedResource(
+      !viewId ? PermissionType.TABLE : PermissionType.VIEW,
+      PermissionLevel.READ,
+      sourcePath
+    )(ctx, () => {})
+
+    // Check is the row action can run for the given view/table
     const rowAction = await sdk.rowActions.get(tableId, rowActionId)
-
+    if (!viewId && !rowAction.permissions.table.runAllowed) {
-    if (isTableId && !rowAction.permissions.table.runAllowed) {
       ctx.throw(
         403,
-        `Row action '${rowActionId}' is not enabled for table '${sourceId}'`
+        `Row action '${rowActionId}' is not enabled for table '${tableId}'`
       )
-    } else if (isViewId && !rowAction.permissions.views[sourceId]?.runAllowed) {
+    } else if (viewId && !rowAction.permissions.views[viewId]?.runAllowed) {
       ctx.throw(
         403,
-        `Row action '${rowActionId}' is not enabled for view '${sourceId}'`
+        `Row action '${rowActionId}' is not enabled for view '${viewId}'`
       )
     }
 

packages/server/src/sdk/app/rowActions.ts

@@ -75,7 +75,7 @@ export async function create(tableId: string, rowAction: { name: string }) {
     name: action.name,
     automationId: automation._id!,
     permissions: {
-      table: { runAllowed: true },
+      table: { runAllowed: false },
       views: {},
     },
   }