From 3cfe556b0f6e1d7f1816a3bf4ba26c49ce46a1e4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 10:54:42 +0200 Subject: [PATCH 01/30] Add rowaction permission to document --- packages/types/src/documents/app/rowAction.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/types/src/documents/app/rowAction.ts b/packages/types/src/documents/app/rowAction.ts index 84fa0e7f00..5863db9459 100644 --- a/packages/types/src/documents/app/rowAction.ts +++ b/packages/types/src/documents/app/rowAction.ts @@ -7,6 +7,10 @@ export interface TableRowActions extends Document { { name: string automationId: string + permissions: { + table: { runAllowed: boolean } + views: Record + } } > } From de3eae2d47938dcbf1e325069727abd81dc6d8c8 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 10:58:58 +0200 Subject: [PATCH 02/30] Fix types --- packages/server/src/sdk/app/rowActions.ts | 8 ++++++-- packages/types/src/documents/app/rowAction.ts | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index f557887b15..668b709120 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -41,7 +41,7 @@ export async function create(tableId: string, rowAction: { name: string }) { throw e } - doc = { _id: rowActionsId, actions: {} } + doc = { _id: rowActionsId, tableId, actions: {} } } ensureUniqueAndThrow(doc, action.name) @@ -75,6 +75,10 @@ export async function create(tableId: string, rowAction: { name: string }) { doc.actions[newRowActionId] = { name: action.name, automationId: automation._id!, + permissions: { + table: { runAllowed: true }, + views: {}, + }, } await db.put(doc) @@ -115,7 +119,7 @@ export async function update( ensureUniqueAndThrow(actionsDoc, action.name, rowActionId) actionsDoc.actions[rowActionId] = { - automationId: actionsDoc.actions[rowActionId].automationId, + ...actionsDoc.actions[rowActionId], ...action, } diff --git a/packages/types/src/documents/app/rowAction.ts b/packages/types/src/documents/app/rowAction.ts index 5863db9459..a5e43175ae 100644 --- a/packages/types/src/documents/app/rowAction.ts +++ b/packages/types/src/documents/app/rowAction.ts @@ -2,6 +2,7 @@ import { Document } from "../document" export interface TableRowActions extends Document { _id: string + tableId: string actions: Record< string, { From 5f3dcda73abf6a7f555e3ad48709d7e2385fb90c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 11:16:46 +0200 Subject: [PATCH 03/30] Refactor --- packages/server/src/sdk/app/rowActions.ts | 64 ++++++++++++------- packages/types/src/documents/app/rowAction.ts | 21 +++--- 2 files changed, 52 insertions(+), 33 deletions(-) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 668b709120..146c00998e 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -2,6 +2,7 @@ import { context, HTTPError, utils } from "@budibase/backend-core" import { AutomationTriggerStepId, + RowActionData, SEPARATOR, TableRowActions, VirtualDocumentType, @@ -101,26 +102,51 @@ export async function docExists(tableId: string) { return result } -export async function update( - tableId: string, - rowActionId: string, - rowAction: { name: string } -) { - const action = { name: rowAction.name.trim() } - const actionsDoc = await get(tableId) - - if (!actionsDoc.actions[rowActionId]) { +function getRowAction( + rowActions: TableRowActions, + rowActionId: string +): RowActionData { + if (!rowActions.actions[rowActionId]) { throw new HTTPError( - `Row action '${rowActionId}' not found in '${tableId}'`, + `Row action '${rowActionId}' not found in '${rowActions.tableId}'`, 400 ) } + return rowActions.actions[rowActionId] +} - ensureUniqueAndThrow(actionsDoc, action.name, rowActionId) +export async function update( + tableId: string, + rowActionId: string, + rowActionData: { name: string } +) { + rowActionData.name = rowActionData.name.trim() - actionsDoc.actions[rowActionId] = { - ...actionsDoc.actions[rowActionId], - ...action, + const actionsDoc = await get(tableId) + ensureUniqueAndThrow(actionsDoc, rowActionData.name, rowActionId) + + const rowAction = getRowAction(actionsDoc, rowActionId) + rowAction.name = rowActionData.name + + const db = context.getAppDB() + await db.put(actionsDoc) + + return { + id: rowActionId, + ...rowAction, + } +} + +export async function setViewPermission( + tableId: string, + rowActionId: string, + viewId: string +) { + const actionsDoc = await get(tableId) + + const rowAction = getRowAction(actionsDoc, rowActionId) + rowAction.permissions.views[viewId] = { + runAllowed: true, } const db = context.getAppDB() @@ -128,20 +154,14 @@ export async function update( return { id: rowActionId, - ...actionsDoc.actions[rowActionId], + ...rowAction, } } export async function remove(tableId: string, rowActionId: string) { const actionsDoc = await get(tableId) - const rowAction = actionsDoc.actions[rowActionId] - if (!rowAction) { - throw new HTTPError( - `Row action '${rowActionId}' not found in '${tableId}'`, - 400 - ) - } + const rowAction = getRowAction(actionsDoc, rowActionId) const { automationId } = rowAction const automation = await automations.get(automationId) diff --git a/packages/types/src/documents/app/rowAction.ts b/packages/types/src/documents/app/rowAction.ts index a5e43175ae..8014b1f8e5 100644 --- a/packages/types/src/documents/app/rowAction.ts +++ b/packages/types/src/documents/app/rowAction.ts @@ -3,15 +3,14 @@ import { Document } from "../document" export interface TableRowActions extends Document { _id: string tableId: string - actions: Record< - string, - { - name: string - automationId: string - permissions: { - table: { runAllowed: boolean } - views: Record - } - } - > + actions: Record +} + +export interface RowActionData { + name: string + automationId: string + permissions: { + table: { runAllowed: boolean } + views: Record + } } From 2aa71ab419f0efd7eb328f2a783d84533f6110aa Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 11:23:02 +0200 Subject: [PATCH 04/30] Set/unset --- .../src/api/controllers/rowAction/crud.ts | 16 ++++++++++++++++ packages/server/src/api/routes/rowAction.ts | 10 ++++++++++ packages/server/src/sdk/app/rowActions.ts | 19 +++++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/packages/server/src/api/controllers/rowAction/crud.ts b/packages/server/src/api/controllers/rowAction/crud.ts index bd5326d957..b94bc9d4b9 100644 --- a/packages/server/src/api/controllers/rowAction/crud.ts +++ b/packages/server/src/api/controllers/rowAction/crud.ts @@ -87,3 +87,19 @@ export async function remove(ctx: Ctx) { await sdk.rowActions.remove(table._id!, actionId) ctx.status = 204 } + +export async function setViewPermission(ctx: Ctx) { + const table = await getTable(ctx) + const { actionId, viewId } = ctx.params + + await sdk.rowActions.setViewPermission(table._id!, actionId, viewId) + ctx.status = 204 +} + +export async function unsetViewPermission(ctx: Ctx) { + const table = await getTable(ctx) + const { actionId, viewId } = ctx.params + + await sdk.rowActions.unsetViewPermission(table._id!, actionId, viewId) + ctx.status = 204 +} diff --git a/packages/server/src/api/routes/rowAction.ts b/packages/server/src/api/routes/rowAction.ts index c84d94c007..0fa0e07673 100644 --- a/packages/server/src/api/routes/rowAction.ts +++ b/packages/server/src/api/routes/rowAction.ts @@ -50,6 +50,16 @@ router authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), rowActionController.remove ) + .post( + "/api/tables/:tableId/actions/:actionId/permissions/:viewId", + authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + rowActionController.setViewPermission + ) + .delete( + "/api/tables/:tableId/actions/:actionId/permissions/:viewId", + authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + rowActionController.unsetViewPermission + ) // Other endpoints .post( diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 146c00998e..fdb93c129c 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -158,6 +158,25 @@ export async function setViewPermission( } } +export async function unsetViewPermission( + tableId: string, + rowActionId: string, + viewId: string +) { + const actionsDoc = await get(tableId) + + const rowAction = getRowAction(actionsDoc, rowActionId) + delete rowAction.permissions.views[viewId] + + const db = context.getAppDB() + await db.put(actionsDoc) + + return { + id: rowActionId, + ...rowAction, + } +} + export async function remove(tableId: string, rowActionId: string) { const actionsDoc = await get(tableId) From 6d3006e80e500d1d394d5bb2fd3c224c443cadcd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 11:37:07 +0200 Subject: [PATCH 05/30] Dry code --- packages/server/src/sdk/app/rowActions.ts | 103 +++++++++------------- 1 file changed, 44 insertions(+), 59 deletions(-) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index fdb93c129c..830df887a4 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -1,8 +1,6 @@ import { context, HTTPError, utils } from "@budibase/backend-core" - import { AutomationTriggerStepId, - RowActionData, SEPARATOR, TableRowActions, VirtualDocumentType, @@ -102,17 +100,31 @@ export async function docExists(tableId: string) { return result } -function getRowAction( - rowActions: TableRowActions, - rowActionId: string -): RowActionData { - if (!rowActions.actions[rowActionId]) { +async function updateDoc( + tableId: string, + rowActionId: string, + transformer: ( + tableRowActions: TableRowActions + ) => TableRowActions | Promise +) { + const actionsDoc = await get(tableId) + const rowAction = actionsDoc?.actions[rowActionId] + if (!rowAction) { throw new HTTPError( - `Row action '${rowActionId}' not found in '${rowActions.tableId}'`, + `Row action '${rowActionId}' not found in '${tableId}'`, 400 ) } - return rowActions.actions[rowActionId] + + const updated = await transformer(actionsDoc) + + const db = context.getAppDB() + await db.put(updated) + + return { + id: rowActionId, + ...updated.actions[rowActionId], + } } export async function update( @@ -120,21 +132,13 @@ export async function update( rowActionId: string, rowActionData: { name: string } ) { - rowActionData.name = rowActionData.name.trim() + const newName = rowActionData.name.trim() - const actionsDoc = await get(tableId) - ensureUniqueAndThrow(actionsDoc, rowActionData.name, rowActionId) - - const rowAction = getRowAction(actionsDoc, rowActionId) - rowAction.name = rowActionData.name - - const db = context.getAppDB() - await db.put(actionsDoc) - - return { - id: rowActionId, - ...rowAction, - } + return await updateDoc(tableId, rowActionId, actionsDoc => { + ensureUniqueAndThrow(actionsDoc, newName, rowActionId) + actionsDoc.actions[rowActionId].name = newName + return actionsDoc + }) } export async function setViewPermission( @@ -142,20 +146,12 @@ export async function setViewPermission( rowActionId: string, viewId: string ) { - const actionsDoc = await get(tableId) - - const rowAction = getRowAction(actionsDoc, rowActionId) - rowAction.permissions.views[viewId] = { - runAllowed: true, - } - - const db = context.getAppDB() - await db.put(actionsDoc) - - return { - id: rowActionId, - ...rowAction, - } + return await updateDoc(tableId, rowActionId, async actionsDoc => { + actionsDoc.actions[rowActionId].permissions.views[viewId] = { + runAllowed: true, + } + return actionsDoc + }) } export async function unsetViewPermission( @@ -163,32 +159,21 @@ export async function unsetViewPermission( rowActionId: string, viewId: string ) { - const actionsDoc = await get(tableId) - - const rowAction = getRowAction(actionsDoc, rowActionId) - delete rowAction.permissions.views[viewId] - - const db = context.getAppDB() - await db.put(actionsDoc) - - return { - id: rowActionId, - ...rowAction, - } + return await updateDoc(tableId, rowActionId, async actionsDoc => { + delete actionsDoc.actions[rowActionId].permissions.views[viewId] + return actionsDoc + }) } export async function remove(tableId: string, rowActionId: string) { - const actionsDoc = await get(tableId) + return await updateDoc(tableId, rowActionId, async actionsDoc => { + const { automationId } = actionsDoc.actions[rowActionId] + const automation = await automations.get(automationId) + await automations.remove(automation._id, automation._rev) - const rowAction = getRowAction(actionsDoc, rowActionId) - - const { automationId } = rowAction - const automation = await automations.get(automationId) - await automations.remove(automation._id, automation._rev) - delete actionsDoc.actions[rowActionId] - - const db = context.getAppDB() - await db.put(actionsDoc) + delete actionsDoc.actions[rowActionId] + return actionsDoc + }) } export async function run(tableId: any, rowActionId: any, rowId: string) { From ec2e5a0263d1563bb152ce3dd5528641acd25f4e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:11:09 +0200 Subject: [PATCH 06/30] Add test utils --- .../src/tests/utilities/api/rowAction.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index 80535e5853..d59f9aac5a 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -70,4 +70,42 @@ export class RowActionAPI extends TestAPI { } ) } + + setViewPermission = async ( + tableId: string, + viewId: string, + rowActionId: string, + expectations?: Expectations, + config?: { publicUser?: boolean } + ) => { + return await this._post( + `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, + { + expectations: { + ...expectations, + status: expectations?.status || 204, + }, + ...config, + } + ) + } + + unsetViewPermission = async ( + tableId: string, + viewId: string, + rowActionId: string, + expectations?: Expectations, + config?: { publicUser?: boolean } + ) => { + return await this._delete( + `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, + { + expectations: { + ...expectations, + status: expectations?.status || 204, + }, + ...config, + } + ) + } } From 42f849d9a1f84a641732c863ea69ddd1e7cd8a15 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:11:29 +0200 Subject: [PATCH 07/30] View permissions on rowActionResponse --- packages/types/src/api/web/app/rowAction.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/types/src/api/web/app/rowAction.ts b/packages/types/src/api/web/app/rowAction.ts index 2b2b6e1927..37cfaf0fbd 100644 --- a/packages/types/src/api/web/app/rowAction.ts +++ b/packages/types/src/api/web/app/rowAction.ts @@ -8,6 +8,7 @@ export interface RowActionResponse extends RowActionData { id: string tableId: string automationId: string + allowedViews: string[] | undefined } export interface RowActionsResponse { From d1c6edc437cf577bf7562d2a5ee87ed3b59e1d8b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:15:55 +0200 Subject: [PATCH 08/30] Return types --- .../src/api/controllers/rowAction/crud.ts | 49 ++++++++++++++++--- packages/server/src/sdk/app/rowActions.ts | 2 +- .../src/tests/utilities/api/rowAction.ts | 10 +--- packages/types/src/documents/app/rowAction.ts | 1 - 4 files changed, 46 insertions(+), 16 deletions(-) diff --git a/packages/server/src/api/controllers/rowAction/crud.ts b/packages/server/src/api/controllers/rowAction/crud.ts index b94bc9d4b9..9b090a9549 100644 --- a/packages/server/src/api/controllers/rowAction/crud.ts +++ b/packages/server/src/api/controllers/rowAction/crud.ts @@ -36,6 +36,7 @@ export async function find(ctx: Ctx) { tableId: table._id!, name: action.name, automationId: action.automationId, + allowedViews: flattenAllowedViews(action.permissions.views), }, }), {} @@ -58,6 +59,7 @@ export async function create( id: createdAction.id, name: createdAction.name, automationId: createdAction.automationId, + allowedViews: undefined, } ctx.status = 201 } @@ -77,6 +79,7 @@ export async function update( id: action.id, name: action.name, automationId: action.automationId, + allowedViews: undefined, } } @@ -88,18 +91,52 @@ export async function remove(ctx: Ctx) { ctx.status = 204 } -export async function setViewPermission(ctx: Ctx) { +export async function setViewPermission(ctx: Ctx) { const table = await getTable(ctx) const { actionId, viewId } = ctx.params - await sdk.rowActions.setViewPermission(table._id!, actionId, viewId) - ctx.status = 204 + const action = await sdk.rowActions.setViewPermission( + table._id!, + actionId, + viewId + ) + ctx.body = { + tableId: table._id!, + id: action.id, + name: action.name, + automationId: action.automationId, + allowedViews: flattenAllowedViews(action.permissions.views), + } } -export async function unsetViewPermission(ctx: Ctx) { +export async function unsetViewPermission(ctx: Ctx) { const table = await getTable(ctx) const { actionId, viewId } = ctx.params - await sdk.rowActions.unsetViewPermission(table._id!, actionId, viewId) - ctx.status = 204 + const action = await sdk.rowActions.unsetViewPermission( + table._id!, + actionId, + viewId + ) + + ctx.body = { + tableId: table._id!, + id: action.id, + name: action.name, + automationId: action.automationId, + allowedViews: flattenAllowedViews(action.permissions.views), + } +} + +function flattenAllowedViews( + permissions: Record +) { + const allowedPermissions = Object.entries(permissions || {}) + .filter(([_, p]) => p.runAllowed) + .map(([viewId]) => viewId) + if (!allowedPermissions.length) { + return undefined + } + + return allowedPermissions } diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 830df887a4..369327be86 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -40,7 +40,7 @@ export async function create(tableId: string, rowAction: { name: string }) { throw e } - doc = { _id: rowActionsId, tableId, actions: {} } + doc = { _id: rowActionsId, actions: {} } } ensureUniqueAndThrow(doc, action.name) diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index d59f9aac5a..b7420fb092 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -81,10 +81,7 @@ export class RowActionAPI extends TestAPI { return await this._post( `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, { - expectations: { - ...expectations, - status: expectations?.status || 204, - }, + expectations, ...config, } ) @@ -100,10 +97,7 @@ export class RowActionAPI extends TestAPI { return await this._delete( `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, { - expectations: { - ...expectations, - status: expectations?.status || 204, - }, + expectations, ...config, } ) diff --git a/packages/types/src/documents/app/rowAction.ts b/packages/types/src/documents/app/rowAction.ts index 8014b1f8e5..fc9a25c2e2 100644 --- a/packages/types/src/documents/app/rowAction.ts +++ b/packages/types/src/documents/app/rowAction.ts @@ -2,7 +2,6 @@ import { Document } from "../document" export interface TableRowActions extends Document { _id: string - tableId: string actions: Record } From 3ac1343b084c806bd7d04a2f317045917085cd46 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:33:56 +0200 Subject: [PATCH 09/30] set/unsetViewPermission tests --- .../src/api/routes/tests/rowAction.spec.ts | 117 ++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 5e043cb42c..ab73334a16 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -462,4 +462,121 @@ describe("/rowsActions", () => { } }) }) + + describe("setViewPermission", () => { + // unauthorisedTests() + + it("can set permission views", async () => { + for (const rowAction of createRowActionRequests(3)) { + await createRowAction(tableId, rowAction) + } + + const persisted = await config.api.rowAction.find(tableId) + + const [actionId1, actionId2] = _.sampleSize( + Object.keys(persisted.actions), + 2 + ) + + const viewId1 = generator.guid() + const viewId2 = generator.guid() + + await config.api.rowAction.setViewPermission( + tableId, + viewId1, + actionId1, + { + status: 200, + body: {}, + } + ) + const action1Result = await config.api.rowAction.setViewPermission( + tableId, + viewId2, + actionId1, + { + status: 200, + body: {}, + } + ) + const action2Result = await config.api.rowAction.setViewPermission( + tableId, + viewId1, + actionId2, + { + status: 200, + body: {}, + } + ) + + const expectedAction1 = expect.objectContaining({ + allowedViews: [viewId1, viewId2], + }) + const expectedAction2 = expect.objectContaining({ + allowedViews: [viewId1], + }) + + const expectedActions = expect.objectContaining({ + [actionId1]: expectedAction1, + [actionId2]: expectedAction2, + }) + expect(action1Result).toEqual(expectedAction1) + expect(action2Result).toEqual(expectedAction2) + expect((await config.api.rowAction.find(tableId)).actions).toEqual( + expectedActions + ) + }) + }) + + describe("unsetViewPermission", () => { + // unauthorisedTests() + + it("can unset permission views", async () => { + for (const rowAction of createRowActionRequests(3)) { + await createRowAction(tableId, rowAction) + } + + const persisted = await config.api.rowAction.find(tableId) + + const [actionId] = _.sampleSize(Object.keys(persisted.actions), 1) + + const viewId1 = generator.guid() + const viewId2 = generator.guid() + + await config.api.rowAction.setViewPermission(tableId, viewId1, actionId, { + status: 200, + body: {}, + }) + await config.api.rowAction.setViewPermission(tableId, viewId2, actionId, { + status: 200, + body: {}, + }) + + expect((await config.api.rowAction.find(tableId)).actions).toEqual( + expect.objectContaining({ + [actionId]: expect.objectContaining({ + allowedViews: [viewId1, viewId2], + }), + }) + ) + + const actionResult = await config.api.rowAction.unsetViewPermission( + tableId, + viewId1, + actionId, + { + status: 200, + body: {}, + } + ) + + const expectedAction = expect.objectContaining({ + allowedViews: [viewId2], + }) + expect(actionResult).toEqual(expectedAction) + expect( + (await config.api.rowAction.find(tableId)).actions[actionId] + ).toEqual(expectedAction) + }) + }) }) From 0ece6a4d2dafe65253c5f368f2109d0e4b8c9169 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:43:35 +0200 Subject: [PATCH 10/30] Fix unauthorised tests --- .../src/api/routes/tests/rowAction.spec.ts | 64 ++++++++++++++++--- 1 file changed, 54 insertions(+), 10 deletions(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index ab73334a16..e43220fc98 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -8,6 +8,7 @@ import { } from "@budibase/types" import * as setup from "./utilities" import { generator } from "@budibase/backend-core/tests" +import { Expectations } from "../../../tests/utilities/api/base" const expectAutomationId = () => expect.stringMatching(`^${DocumentType.AUTOMATION}_.+`) @@ -43,11 +44,14 @@ describe("/rowsActions", () => { .map(name => ({ name })) } - function unauthorisedTests() { + function unauthorisedTests( + apiDelegate: ( + expectations: Expectations, + testConfig?: { publicUser?: boolean } + ) => Promise + ) { it("returns unauthorised (401) for unauthenticated requests", async () => { - await createRowAction( - tableId, - createRowActionRequest(), + await apiDelegate( { status: 401, body: { @@ -77,7 +81,14 @@ describe("/rowsActions", () => { } describe("create", () => { - unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + createRowAction( + tableId, + createRowActionRequest(), + expectations, + testConfig + ) + ) it("creates new row actions for tables without existing actions", async () => { const rowAction = createRowActionRequest() @@ -247,7 +258,9 @@ describe("/rowsActions", () => { }) describe("find", () => { - unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.find(tableId, expectations, testConfig) + ) it("returns only the actions for the requested table", async () => { const rowActions: RowActionResponse[] = [] @@ -279,7 +292,15 @@ describe("/rowsActions", () => { }) describe("update", () => { - unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.update( + tableId, + generator.guid(), + createRowActionRequest(), + expectations, + testConfig + ) + ) it("can update existing actions", async () => { for (const rowAction of createRowActionRequests(3)) { @@ -398,7 +419,14 @@ describe("/rowsActions", () => { }) describe("delete", () => { - unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.delete( + tableId, + generator.guid(), + expectations, + testConfig + ) + ) it("can delete existing actions", async () => { const actions: RowActionResponse[] = [] @@ -464,7 +492,15 @@ describe("/rowsActions", () => { }) describe("setViewPermission", () => { - // unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.setViewPermission( + tableId, + generator.guid(), + generator.guid(), + expectations, + testConfig + ) + ) it("can set permission views", async () => { for (const rowAction of createRowActionRequests(3)) { @@ -529,7 +565,15 @@ describe("/rowsActions", () => { }) describe("unsetViewPermission", () => { - // unauthorisedTests() + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.unsetViewPermission( + tableId, + generator.guid(), + generator.guid(), + expectations, + testConfig + ) + ) it("can unset permission views", async () => { for (const rowAction of createRowActionRequests(3)) { From b461025639bb96867e681cf906122de0d474c88d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 12:57:10 +0200 Subject: [PATCH 11/30] Check views --- .../src/api/routes/tests/rowAction.spec.ts | 16 ++++++++++++---- packages/server/src/sdk/app/rowActions.ts | 9 +++++++++ .../server/src/tests/utilities/structures.ts | 12 ++++++++++++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index e43220fc98..06acc42f22 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -514,8 +514,12 @@ describe("/rowsActions", () => { 2 ) - const viewId1 = generator.guid() - const viewId2 = generator.guid() + const { id: viewId1 } = await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + const { id: viewId2 } = await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) await config.api.rowAction.setViewPermission( tableId, @@ -584,8 +588,12 @@ describe("/rowsActions", () => { const [actionId] = _.sampleSize(Object.keys(persisted.actions), 1) - const viewId1 = generator.guid() - const viewId2 = generator.guid() + const { id: viewId1 } = await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + const { id: viewId2 } = await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) await config.api.rowAction.setViewPermission(tableId, viewId1, actionId, { status: 200, diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 369327be86..768e450605 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -141,11 +141,19 @@ export async function update( }) } +async function guardView(tableId: string, viewId: string) { + const view = await sdk.views.get(viewId) + if (!view || view.tableId !== tableId) { + throw new HTTPError(`View '${viewId}' not found in '${tableId}'`, 400) + } +} + export async function setViewPermission( tableId: string, rowActionId: string, viewId: string ) { + await guardView(tableId, viewId) return await updateDoc(tableId, rowActionId, async actionsDoc => { actionsDoc.actions[rowActionId].permissions.views[viewId] = { runAllowed: true, @@ -159,6 +167,7 @@ export async function unsetViewPermission( rowActionId: string, viewId: string ) { + await guardView(tableId, viewId) return await updateDoc(tableId, rowActionId, async actionsDoc => { delete actionsDoc.actions[rowActionId].permissions.views[viewId] return actionsDoc diff --git a/packages/server/src/tests/utilities/structures.ts b/packages/server/src/tests/utilities/structures.ts index 8d64734ee3..2e501932b8 100644 --- a/packages/server/src/tests/utilities/structures.ts +++ b/packages/server/src/tests/utilities/structures.ts @@ -30,6 +30,7 @@ import { BBReferenceFieldSubType, JsonFieldSubType, AutoFieldSubType, + CreateViewRequest, } from "@budibase/types" import { LoopInput } from "../../definitions/automations" import { merge } from "lodash" @@ -145,6 +146,17 @@ export function view(tableId: string) { } } +function viewV2CreateRequest(tableId: string): CreateViewRequest { + return { + tableId, + name: generator.guid(), + } +} + +export const viewV2 = { + createRequest: viewV2CreateRequest, +} + export function automationStep( actionDefinition = BUILTIN_ACTION_DEFINITIONS.CREATE_ROW ): AutomationStep { From 98347b45ce2e1c2438ecf2ffe56fde5672a9db22 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 13:21:34 +0200 Subject: [PATCH 12/30] Extra tests --- .../src/api/routes/tests/rowAction.spec.ts | 147 ++++++++++-------- packages/server/src/sdk/app/rowActions.ts | 10 +- 2 files changed, 89 insertions(+), 68 deletions(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 06acc42f22..c9c2b2ced0 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -491,36 +491,54 @@ describe("/rowsActions", () => { }) }) - describe("setViewPermission", () => { - unauthorisedTests((expectations, testConfig) => - config.api.rowAction.setViewPermission( - tableId, - generator.guid(), - generator.guid(), - expectations, - testConfig + describe("set/unsetViewPermission", () => { + describe.each([ + ["setViewPermission", config.api.rowAction.setViewPermission], + ["unsetViewPermission", config.api.rowAction.unsetViewPermission], + ])("unauthorisedTests for %s", (__, delegateTest) => { + unauthorisedTests((expectations, testConfig) => + delegateTest( + tableId, + generator.guid(), + generator.guid(), + expectations, + testConfig + ) ) - ) + }) - it("can set permission views", async () => { + let tableIdForDescribe: string + let actionId1: string, actionId2: string + let viewId1: string, viewId2: string + beforeAll(async () => { + tableIdForDescribe = tableId for (const rowAction of createRowActionRequests(3)) { await createRowAction(tableId, rowAction) } - const persisted = await config.api.rowAction.find(tableId) - const [actionId1, actionId2] = _.sampleSize( - Object.keys(persisted.actions), - 2 - ) + const actions = _.sampleSize(Object.keys(persisted.actions), 2) + actionId1 = actions[0] + actionId2 = actions[1] - const { id: viewId1 } = await config.api.viewV2.create( - setup.structures.viewV2.createRequest(tableId) - ) - const { id: viewId2 } = await config.api.viewV2.create( - setup.structures.viewV2.createRequest(tableId) - ) + viewId1 = ( + await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + ).id + viewId2 = ( + await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + ).id + }) + beforeEach(() => { + // Hack to reuse tables for these given tests + tableId = tableIdForDescribe + }) + + it("can set permission views", async () => { await config.api.rowAction.setViewPermission( tableId, viewId1, @@ -566,56 +584,12 @@ describe("/rowsActions", () => { expectedActions ) }) - }) - - describe("unsetViewPermission", () => { - unauthorisedTests((expectations, testConfig) => - config.api.rowAction.unsetViewPermission( - tableId, - generator.guid(), - generator.guid(), - expectations, - testConfig - ) - ) it("can unset permission views", async () => { - for (const rowAction of createRowActionRequests(3)) { - await createRowAction(tableId, rowAction) - } - - const persisted = await config.api.rowAction.find(tableId) - - const [actionId] = _.sampleSize(Object.keys(persisted.actions), 1) - - const { id: viewId1 } = await config.api.viewV2.create( - setup.structures.viewV2.createRequest(tableId) - ) - const { id: viewId2 } = await config.api.viewV2.create( - setup.structures.viewV2.createRequest(tableId) - ) - - await config.api.rowAction.setViewPermission(tableId, viewId1, actionId, { - status: 200, - body: {}, - }) - await config.api.rowAction.setViewPermission(tableId, viewId2, actionId, { - status: 200, - body: {}, - }) - - expect((await config.api.rowAction.find(tableId)).actions).toEqual( - expect.objectContaining({ - [actionId]: expect.objectContaining({ - allowedViews: [viewId1, viewId2], - }), - }) - ) - const actionResult = await config.api.rowAction.unsetViewPermission( tableId, viewId1, - actionId, + actionId1, { status: 200, body: {}, @@ -627,8 +601,47 @@ describe("/rowsActions", () => { }) expect(actionResult).toEqual(expectedAction) expect( - (await config.api.rowAction.find(tableId)).actions[actionId] + (await config.api.rowAction.find(tableId)).actions[actionId1] ).toEqual(expectedAction) }) + + it.each([ + ["setViewPermission", config.api.rowAction.setViewPermission], + ["unsetViewPermission", config.api.rowAction.unsetViewPermission], + ])( + "cannot update permission views for unexisting views (%s)", + async (__, delegateTest) => { + const viewId = generator.guid() + + await delegateTest(tableId, viewId, actionId1, { + status: 400, + body: { + message: `View '${viewId}' not found in '${tableId}'`, + }, + }) + } + ) + + it.each([ + ["setViewPermission", config.api.rowAction.setViewPermission], + ["unsetViewPermission", config.api.rowAction.unsetViewPermission], + ])( + "cannot update permission views crossing table views (%s)", + async (__, delegateTest) => { + const anotherTable = await config.api.table.save( + setup.structures.basicTable() + ) + const { id: viewId } = await config.api.viewV2.create( + setup.structures.viewV2.createRequest(anotherTable._id!) + ) + + await delegateTest(tableId, viewId, actionId1, { + status: 400, + body: { + message: `View '${viewId}' not found in '${tableId}'`, + }, + }) + } + ) }) }) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 768e450605..c57d078145 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -142,7 +142,15 @@ export async function update( } async function guardView(tableId: string, viewId: string) { - const view = await sdk.views.get(viewId) + let view + try { + view = await sdk.views.get(viewId) + } catch (e: any) { + if (e?.message !== "Unable to extract table ID, is not a view ID") { + throw e + } + // View id is not valid + } if (!view || view.tableId !== tableId) { throw new HTTPError(`View '${viewId}' not found in '${tableId}'`, 400) } From 922b7460293b68661b6b9b47aee747422f9ab67a Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 13:42:20 +0200 Subject: [PATCH 13/30] Crud endpoints only for builder --- packages/server/src/api/routes/rowAction.ts | 16 +++++------ .../src/api/routes/tests/rowAction.spec.ts | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/rowAction.ts b/packages/server/src/api/routes/rowAction.ts index 0fa0e07673..34b4ee9fe3 100644 --- a/packages/server/src/api/routes/rowAction.ts +++ b/packages/server/src/api/routes/rowAction.ts @@ -2,9 +2,9 @@ import Router from "@koa/router" import Joi from "joi" import { middleware, permissions } from "@budibase/backend-core" import * as rowActionController from "../controllers/rowAction" -import { authorizedResource } from "../../middleware/authorized" +import authorized, { authorizedResource } from "../../middleware/authorized" -const { PermissionLevel, PermissionType } = permissions +const { PermissionLevel, PermissionType, BUILDER } = permissions function rowActionValidator() { return middleware.joiValidator.body( @@ -30,34 +30,34 @@ const router: Router = new Router() router .get( "/api/tables/:tableId/actions", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionController.find ) .post( "/api/tables/:tableId/actions", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionValidator(), rowActionController.create ) .put( "/api/tables/:tableId/actions/:actionId", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionValidator(), rowActionController.update ) .delete( "/api/tables/:tableId/actions/:actionId", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionController.remove ) .post( "/api/tables/:tableId/actions/:actionId/permissions/:viewId", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionController.setViewPermission ) .delete( "/api/tables/:tableId/actions/:actionId/permissions/:viewId", - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + authorized(BUILDER), rowActionController.unsetViewPermission ) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index c9c2b2ced0..fae8828c0a 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -4,11 +4,13 @@ import tk from "timekeeper" import { CreateRowActionRequest, DocumentType, + PermissionLevel, RowActionResponse, } from "@budibase/types" import * as setup from "./utilities" import { generator } from "@budibase/backend-core/tests" import { Expectations } from "../../../tests/utilities/api/base" +import { roles } from "@budibase/backend-core" const expectAutomationId = () => expect.stringMatching(`^${DocumentType.AUTOMATION}_.+`) @@ -69,6 +71,31 @@ describe("/rowsActions", () => { await config.withUser(user, async () => { await createRowAction(generator.guid(), createRowActionRequest(), { status: 403, + body: { + message: "Not Authorized", + }, + }) + }) + }) + + it("returns forbidden (403) for non-builder users even if they have table write permissions", async () => { + const user = await config.createUser({ + builder: {}, + }) + const tableId = generator.guid() + for (const role of Object.values(roles.BUILTIN_ROLE_IDS)) { + await config.api.permission.add({ + roleId: role, + resourceId: tableId, + level: PermissionLevel.EXECUTE, + }) + } + await config.withUser(user, async () => { + await createRowAction(tableId, createRowActionRequest(), { + status: 403, + body: { + message: "Not Authorized", + }, }) }) }) From 568f5719c662d266399d5ea750f8029f3a9649c9 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 14:37:29 +0200 Subject: [PATCH 14/30] Test utils --- .../src/tests/utilities/api/rowAction.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index b7420fb092..6bcf661076 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -2,6 +2,7 @@ import { CreateRowActionRequest, RowActionResponse, RowActionsResponse, + RowActionTriggerRequest, } from "@budibase/types" import { Expectations, TestAPI } from "./base" @@ -102,4 +103,21 @@ export class RowActionAPI extends TestAPI { } ) } + + trigger = async ( + tableId: string, + rowActionId: string, + body: RowActionTriggerRequest, + expectations?: Expectations, + config?: { publicUser?: boolean } + ) => { + return await this._post( + `/api/tables/${tableId}/actions/${rowActionId}/trigger`, + { + body, + expectations, + ...config, + } + ) + } } From 610621823ccf37412b03a9212752210e3fbd82a8 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 14:38:18 +0200 Subject: [PATCH 15/30] Publish --- packages/server/src/api/routes/tests/rowAction.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index fae8828c0a..0ed7af163f 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -90,6 +90,10 @@ describe("/rowsActions", () => { level: PermissionLevel.EXECUTE, }) } + + // replicate changes before checking permissions + await config.publish() + await config.withUser(user, async () => { await createRowAction(tableId, createRowActionRequest(), { status: 403, From e93934111f10c4172ec44b6aa489a8c0c56bd909 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 15:09:45 +0200 Subject: [PATCH 16/30] Allow running api prod tests --- packages/server/src/tests/utilities/api/base.ts | 4 +++- packages/server/src/tests/utilities/api/rowAction.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/server/src/tests/utilities/api/base.ts b/packages/server/src/tests/utilities/api/base.ts index 3a5f6529f8..b38f8eb56f 100644 --- a/packages/server/src/tests/utilities/api/base.ts +++ b/packages/server/src/tests/utilities/api/base.ts @@ -40,6 +40,7 @@ export interface RequestOpts { > expectations?: Expectations publicUser?: boolean + useProdApp?: boolean } export abstract class TestAPI { @@ -108,7 +109,8 @@ export abstract class TestAPI { const headersFn = publicUser ? this.config.publicHeaders.bind(this.config) - : this.config.defaultHeaders.bind(this.config) + : (extras = {}) => + this.config.defaultHeaders.bind(this.config)(extras, opts?.useProdApp) const app = getServer() let req = request(app)[method](url) diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index 6bcf661076..0f67205ca5 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -109,7 +109,7 @@ export class RowActionAPI extends TestAPI { rowActionId: string, body: RowActionTriggerRequest, expectations?: Expectations, - config?: { publicUser?: boolean } + config?: { publicUser?: boolean; useProdApp?: boolean } ) => { return await this._post( `/api/tables/${tableId}/actions/${rowActionId}/trigger`, From 6d1838d9076b1e0a655e753df2f3b0542f83b8bb Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 15:17:18 +0200 Subject: [PATCH 17/30] Create row action trigger test --- .../src/api/routes/tests/rowAction.spec.ts | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 0ed7af163f..f6e49ed094 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -5,12 +5,15 @@ import { CreateRowActionRequest, DocumentType, PermissionLevel, + Row, RowActionResponse, } from "@budibase/types" import * as setup from "./utilities" import { generator } from "@budibase/backend-core/tests" import { Expectations } from "../../../tests/utilities/api/base" import { roles } from "@budibase/backend-core" +import { automations } from "@budibase/pro" +import { trigger } from "src/api/controllers/automation" const expectAutomationId = () => expect.stringMatching(`^${DocumentType.AUTOMATION}_.+`) @@ -675,4 +678,60 @@ describe("/rowsActions", () => { } ) }) + + describe("trigger", () => { + let row: Row + let rowAction: RowActionResponse + + unauthorisedTests((expectations, testConfig) => + config.api.rowAction.trigger( + tableId, + rowAction.id, + { + rowId: row._id!, + }, + expectations, + { ...testConfig, useProdApp: true } + ) + ) + + beforeEach(async () => { + row = await config.api.row.save(tableId, {}) + rowAction = await createRowAction(tableId, createRowActionRequest()) + + await config.publish() + }) + + it("can trigger an automation given valid data", async () => { + await config.api.rowAction.trigger( + tableId, + rowAction.id, + { + rowId: row._id!, + }, + undefined, + { useProdApp: true } + ) + + const { data: automationLogs } = await config.doInContext( + config.getProdAppId(), + async () => + automations.logs.logSearch({ + startDate: await automations.logs.oldestLogDate(), + }) + ) + expect(automationLogs).toEqual([ + expect.objectContaining({ + automationId: rowAction.automationId, + trigger: expect.objectContaining({ + outputs: { + fields: {}, + row: await config.api.row.get(tableId, row._id!), + table: await config.api.table.get(tableId), + }, + }), + }), + ]) + }) + }) }) From 95d863b4a8b1fc26818c961d15449dadca4f195e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 16:12:39 +0200 Subject: [PATCH 18/30] Clean --- .../src/api/routes/tests/rowAction.spec.ts | 24 +++++++------------ .../src/tests/utilities/api/rowAction.ts | 2 +- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index f6e49ed094..074642f74c 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -683,6 +683,13 @@ describe("/rowsActions", () => { let row: Row let rowAction: RowActionResponse + beforeEach(async () => { + row = await config.api.row.save(tableId, {}) + rowAction = await createRowAction(tableId, createRowActionRequest()) + + await config.publish() + }) + unauthorisedTests((expectations, testConfig) => config.api.rowAction.trigger( tableId, @@ -695,23 +702,10 @@ describe("/rowsActions", () => { ) ) - beforeEach(async () => { - row = await config.api.row.save(tableId, {}) - rowAction = await createRowAction(tableId, createRowActionRequest()) - - await config.publish() - }) - it("can trigger an automation given valid data", async () => { - await config.api.rowAction.trigger( - tableId, - rowAction.id, - { + await config.api.rowAction.trigger(tableId, rowAction.id, { rowId: row._id!, - }, - undefined, - { useProdApp: true } - ) + }) const { data: automationLogs } = await config.doInContext( config.getProdAppId(), diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index 0f67205ca5..7ade1d835b 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -116,7 +116,7 @@ export class RowActionAPI extends TestAPI { { body, expectations, - ...config, + ...{ ...config, useProdApp: config?.useProdApp ?? true }, } ) } From 5ed66e9d4088340125048e571e798cee77987105 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 16:40:08 +0200 Subject: [PATCH 19/30] Lint --- packages/server/src/api/routes/tests/rowAction.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 074642f74c..a21163a978 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -13,7 +13,6 @@ import { generator } from "@budibase/backend-core/tests" import { Expectations } from "../../../tests/utilities/api/base" import { roles } from "@budibase/backend-core" import { automations } from "@budibase/pro" -import { trigger } from "src/api/controllers/automation" const expectAutomationId = () => expect.stringMatching(`^${DocumentType.AUTOMATION}_.+`) From dd14d0b646aca4953420082d1c9d52a47a84ecc7 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 17:06:40 +0200 Subject: [PATCH 20/30] Fix return types --- packages/backend-core/src/docIds/params.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/docIds/params.ts b/packages/backend-core/src/docIds/params.ts index d9baee3dc6..093724b55e 100644 --- a/packages/backend-core/src/docIds/params.ts +++ b/packages/backend-core/src/docIds/params.ts @@ -71,7 +71,7 @@ export function getQueryIndex(viewName: ViewName) { export const isTableId = (id: string) => { // this includes datasource plus tables return ( - id && + !!id && (id.startsWith(`${DocumentType.TABLE}${SEPARATOR}`) || id.startsWith(`${DocumentType.DATASOURCE_PLUS}${SEPARATOR}`)) ) From 6a43597dd3f1e138b467d039c71fe539322f6d4e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 17:07:02 +0200 Subject: [PATCH 21/30] Update paths --- packages/server/src/api/controllers/rowAction/run.ts | 4 ++-- packages/server/src/api/routes/rowAction.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/rowAction/run.ts b/packages/server/src/api/controllers/rowAction/run.ts index c4b6a276bf..f17b9594a7 100644 --- a/packages/server/src/api/controllers/rowAction/run.ts +++ b/packages/server/src/api/controllers/rowAction/run.ts @@ -2,9 +2,9 @@ import { RowActionTriggerRequest, Ctx } from "@budibase/types" import sdk from "../../../sdk" export async function run(ctx: Ctx) { - const { tableId, actionId } = ctx.params + const { sourceId, actionId } = ctx.params const { rowId } = ctx.request.body - await sdk.rowActions.run(tableId, actionId, rowId) + await sdk.rowActions.run(sourceId, actionId, rowId) ctx.status = 200 } diff --git a/packages/server/src/api/routes/rowAction.ts b/packages/server/src/api/routes/rowAction.ts index 34b4ee9fe3..846e568420 100644 --- a/packages/server/src/api/routes/rowAction.ts +++ b/packages/server/src/api/routes/rowAction.ts @@ -63,7 +63,7 @@ router // Other endpoints .post( - "/api/tables/:tableId/actions/:actionId/trigger", + "/api/tables/:sourceId/actions/:actionId/trigger", rowTriggerValidator(), authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), rowActionController.run From f193df41f1066f4358251f077219f379c5214d6f Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 17:10:14 +0200 Subject: [PATCH 22/30] Expose get row action --- packages/server/src/sdk/app/rowActions.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index c57d078145..e8fa3d41f0 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -87,7 +87,19 @@ export async function create(tableId: string, rowAction: { name: string }) { } } -export async function get(tableId: string) { +export async function get(tableId: string, rowActionId: string) { + const actionsDoc = await getAll(tableId) + const rowAction = actionsDoc?.actions[rowActionId] + if (!rowAction) { + throw new HTTPError( + `Row action '${rowActionId}' not found in '${tableId}'`, + 400 + ) + } + return rowAction +} + +async function getAll(tableId: string) { const db = context.getAppDB() const rowActionsId = generateRowActionsID(tableId) return await db.get(rowActionsId) @@ -107,7 +119,7 @@ async function updateDoc( tableRowActions: TableRowActions ) => TableRowActions | Promise ) { - const actionsDoc = await get(tableId) + const actionsDoc = await getAll(tableId) const rowAction = actionsDoc?.actions[rowActionId] if (!rowAction) { throw new HTTPError( @@ -199,7 +211,7 @@ export async function run(tableId: any, rowActionId: any, rowId: string) { throw new HTTPError("Table not found", 404) } - const { actions } = await get(tableId) + const { actions } = await getAll(tableId) const rowAction = actions[rowActionId] if (!rowAction) { From 868d193015d14d39f1a076319352278a3e1c733e Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 17:13:52 +0200 Subject: [PATCH 23/30] Add triggerRowActionAuthorised --- packages/server/src/api/routes/rowAction.ts | 7 ++-- .../src/api/routes/tests/rowAction.spec.ts | 42 +++++++++++++++++++ .../middleware/triggerRowActionAuthorised.ts | 42 +++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 packages/server/src/middleware/triggerRowActionAuthorised.ts diff --git a/packages/server/src/api/routes/rowAction.ts b/packages/server/src/api/routes/rowAction.ts index 846e568420..54154e3ee8 100644 --- a/packages/server/src/api/routes/rowAction.ts +++ b/packages/server/src/api/routes/rowAction.ts @@ -2,9 +2,10 @@ import Router from "@koa/router" import Joi from "joi" import { middleware, permissions } from "@budibase/backend-core" import * as rowActionController from "../controllers/rowAction" -import authorized, { authorizedResource } from "../../middleware/authorized" +import authorized from "../../middleware/authorized" +import { triggerRowActionAuthorised } from "../../middleware/triggerRowActionAuthorised" -const { PermissionLevel, PermissionType, BUILDER } = permissions +const { BUILDER } = permissions function rowActionValidator() { return middleware.joiValidator.body( @@ -65,7 +66,7 @@ router .post( "/api/tables/:sourceId/actions/:actionId/trigger", rowTriggerValidator(), - authorizedResource(PermissionType.TABLE, PermissionLevel.READ, "tableId"), + triggerRowActionAuthorised("sourceId", "actionId"), rowActionController.run ) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index a21163a978..0cf41cb36f 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -726,5 +726,47 @@ describe("/rowsActions", () => { }), ]) }) + + it("rejects triggering from a non-allowed view", async () => { + const viewId = ( + await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + ).id + + await config.api.rowAction.trigger( + viewId, + rowAction.id, + { + rowId: row._id!, + }, + { + status: 403, + body: { + message: `Row action '${rowAction.id}' is not enabled for view '${viewId}'"`, + }, + } + ) + + const { data: automationLogs } = await config.doInContext( + config.getProdAppId(), + async () => + automations.logs.logSearch({ + startDate: await automations.logs.oldestLogDate(), + }) + ) + expect(automationLogs).toEqual([ + expect.objectContaining({ + automationId: rowAction.automationId, + trigger: expect.objectContaining({ + outputs: { + fields: {}, + row: await config.api.row.get(tableId, row._id!), + table: await config.api.table.get(tableId), + }, + }), + }), + ]) + }) }) }) diff --git a/packages/server/src/middleware/triggerRowActionAuthorised.ts b/packages/server/src/middleware/triggerRowActionAuthorised.ts new file mode 100644 index 0000000000..4cc654c139 --- /dev/null +++ b/packages/server/src/middleware/triggerRowActionAuthorised.ts @@ -0,0 +1,42 @@ +import { Next } from "koa" +import { Ctx } from "@budibase/types" +import { paramSubResource } from "./resourceId" +import { docIds } from "@budibase/backend-core" +import * as utils from "../db/utils" +import sdk from "../sdk" + +export function triggerRowActionAuthorised( + sourcePath: string, + actionPath: string +) { + return async (ctx: Ctx, next: Next) => { + // Reusing the existing middleware to extract the value + paramSubResource(sourcePath, actionPath)(ctx, () => {}) + const { resourceId: sourceId, subResourceId: rowActionId } = ctx + + const isTableId = docIds.isTableId(sourceId) + const isViewId = utils.isViewID(sourceId) + if (!isTableId && !isViewId) { + ctx.throw(400, `'${sourceId}' is not a valid source id`) + } + + const tableId = isTableId + ? sourceId + : utils.extractViewInfoFromID(sourceId).tableId + const rowAction = await sdk.rowActions.get(tableId, rowActionId) + + if (isTableId && !rowAction.permissions.table.runAllowed) { + ctx.throw( + 403, + `Row action '${rowActionId}' is not enabled for table '${sourceId}'` + ) + } else if (isViewId && !rowAction.permissions.views[sourceId]?.runAllowed) { + ctx.throw( + 403, + `Row action '${rowActionId}' is not enabled for view '${sourceId}'` + ) + } + + return next() + } +} From 5cd3b9dc88aac5d3792947c52119d860e1c4f78a Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 17:26:59 +0200 Subject: [PATCH 24/30] Add tests --- .../src/api/controllers/rowAction/crud.ts | 2 +- .../src/api/routes/tests/rowAction.spec.ts | 68 +++++++++---------- packages/server/src/sdk/app/rowActions.ts | 2 +- 3 files changed, 36 insertions(+), 36 deletions(-) diff --git a/packages/server/src/api/controllers/rowAction/crud.ts b/packages/server/src/api/controllers/rowAction/crud.ts index 9b090a9549..c9b7756987 100644 --- a/packages/server/src/api/controllers/rowAction/crud.ts +++ b/packages/server/src/api/controllers/rowAction/crud.ts @@ -26,7 +26,7 @@ export async function find(ctx: Ctx) { return } - const { actions } = await sdk.rowActions.get(table._id!) + const { actions } = await sdk.rowActions.getAll(table._id!) const result: RowActionsResponse = { actions: Object.entries(actions).reduce>( (acc, [key, action]) => ({ diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index 0cf41cb36f..d61ea55e3b 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -687,32 +687,24 @@ describe("/rowsActions", () => { rowAction = await createRowAction(tableId, createRowActionRequest()) await config.publish() + tk.travel(Date.now() + 100) }) - unauthorisedTests((expectations, testConfig) => - config.api.rowAction.trigger( - tableId, - rowAction.id, - { - rowId: row._id!, - }, - expectations, - { ...testConfig, useProdApp: true } - ) - ) - - it("can trigger an automation given valid data", async () => { - await config.api.rowAction.trigger(tableId, rowAction.id, { - rowId: row._id!, - }) - + async function getAutomationLogs() { const { data: automationLogs } = await config.doInContext( config.getProdAppId(), async () => - automations.logs.logSearch({ - startDate: await automations.logs.oldestLogDate(), - }) + automations.logs.logSearch({ startDate: new Date().toISOString() }) ) + return automationLogs + } + + it("can trigger an automation given valid data", async () => { + await config.api.rowAction.trigger(tableId, rowAction.id, { + rowId: row._id!, + }) + + const automationLogs = await getAutomationLogs() expect(automationLogs).toEqual([ expect.objectContaining({ automationId: rowAction.automationId, @@ -743,28 +735,36 @@ describe("/rowsActions", () => { { status: 403, body: { - message: `Row action '${rowAction.id}' is not enabled for view '${viewId}'"`, + message: `Row action '${rowAction.id}' is not enabled for view '${viewId}'`, }, } ) - const { data: automationLogs } = await config.doInContext( - config.getProdAppId(), - async () => - automations.logs.logSearch({ - startDate: await automations.logs.oldestLogDate(), - }) + const automationLogs = await getAutomationLogs() + expect(automationLogs).toEqual([]) + }) + + it("triggers from an allowed view", async () => { + const viewId = ( + await config.api.viewV2.create( + setup.structures.viewV2.createRequest(tableId) + ) + ).id + + await config.api.rowAction.setViewPermission( + tableId, + viewId, + rowAction.id ) + + await config.api.rowAction.trigger(tableId, rowAction.id, { + rowId: row._id!, + }) + + const automationLogs = await getAutomationLogs() expect(automationLogs).toEqual([ expect.objectContaining({ automationId: rowAction.automationId, - trigger: expect.objectContaining({ - outputs: { - fields: {}, - row: await config.api.row.get(tableId, row._id!), - table: await config.api.table.get(tableId), - }, - }), }), ]) }) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index e8fa3d41f0..3d558f5345 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -99,7 +99,7 @@ export async function get(tableId: string, rowActionId: string) { return rowAction } -async function getAll(tableId: string) { +export async function getAll(tableId: string) { const db = context.getAppDB() const rowActionsId = generateRowActionsID(tableId) return await db.get(rowActionsId) From 6a8d55a00cf221a8b0cf411db01ebf32a3df3871 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 18:00:14 +0200 Subject: [PATCH 25/30] Add tests --- packages/server/src/api/controllers/rowAction/run.ts | 4 ++-- packages/server/src/api/routes/tests/rowAction.spec.ts | 4 +++- packages/server/src/middleware/triggerRowActionAuthorised.ts | 3 +++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/rowAction/run.ts b/packages/server/src/api/controllers/rowAction/run.ts index f17b9594a7..c4b6a276bf 100644 --- a/packages/server/src/api/controllers/rowAction/run.ts +++ b/packages/server/src/api/controllers/rowAction/run.ts @@ -2,9 +2,9 @@ import { RowActionTriggerRequest, Ctx } from "@budibase/types" import sdk from "../../../sdk" export async function run(ctx: Ctx) { - const { sourceId, actionId } = ctx.params + const { tableId, actionId } = ctx.params const { rowId } = ctx.request.body - await sdk.rowActions.run(sourceId, actionId, rowId) + await sdk.rowActions.run(tableId, actionId, rowId) ctx.status = 200 } diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index d61ea55e3b..aab26a2e2f 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -726,6 +726,7 @@ describe("/rowsActions", () => { ) ).id + await config.publish() await config.api.rowAction.trigger( viewId, rowAction.id, @@ -757,7 +758,8 @@ describe("/rowsActions", () => { rowAction.id ) - await config.api.rowAction.trigger(tableId, rowAction.id, { + await config.publish() + await config.api.rowAction.trigger(viewId, rowAction.id, { rowId: row._id!, }) diff --git a/packages/server/src/middleware/triggerRowActionAuthorised.ts b/packages/server/src/middleware/triggerRowActionAuthorised.ts index 4cc654c139..be5c6a97e1 100644 --- a/packages/server/src/middleware/triggerRowActionAuthorised.ts +++ b/packages/server/src/middleware/triggerRowActionAuthorised.ts @@ -23,6 +23,7 @@ export function triggerRowActionAuthorised( const tableId = isTableId ? sourceId : utils.extractViewInfoFromID(sourceId).tableId + const rowAction = await sdk.rowActions.get(tableId, rowActionId) if (isTableId && !rowAction.permissions.table.runAllowed) { @@ -37,6 +38,8 @@ export function triggerRowActionAuthorised( ) } + // Enrich tableId + ctx.params.tableId = tableId return next() } } From b54b2a7121f374e5e1f4163b1f82b74190ad7355 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 18:02:55 +0200 Subject: [PATCH 26/30] Lint --- packages/server/src/tests/utilities/api/base.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/server/src/tests/utilities/api/base.ts b/packages/server/src/tests/utilities/api/base.ts index b38f8eb56f..39ac5cefc0 100644 --- a/packages/server/src/tests/utilities/api/base.ts +++ b/packages/server/src/tests/utilities/api/base.ts @@ -108,7 +108,10 @@ export abstract class TestAPI { } const headersFn = publicUser - ? this.config.publicHeaders.bind(this.config) + ? (_extras = {}) => + this.config.publicHeaders.bind(this.config)({ + prodApp: opts?.useProdApp, + }) : (extras = {}) => this.config.defaultHeaders.bind(this.config)(extras, opts?.useProdApp) From 9f56b9916e26e1f0482aa6e8229cee2d3be772fd Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Mon, 26 Aug 2024 20:18:37 +0200 Subject: [PATCH 27/30] Fix --- packages/server/src/sdk/app/automations/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/sdk/app/automations/utils.ts b/packages/server/src/sdk/app/automations/utils.ts index 5d057697ca..2ce0012b6a 100644 --- a/packages/server/src/sdk/app/automations/utils.ts +++ b/packages/server/src/sdk/app/automations/utils.ts @@ -29,7 +29,7 @@ export async function getBuilderData( const rowActionNameCache: Record = {} async function getRowActionName(tableId: string, rowActionId: string) { if (!rowActionNameCache[tableId]) { - rowActionNameCache[tableId] = await sdk.rowActions.get(tableId) + rowActionNameCache[tableId] = await sdk.rowActions.getAll(tableId) } return rowActionNameCache[tableId].actions[rowActionId]?.name From eb58c696dde29f6b16a76c3bc36e18ce0cd85f07 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 29 Aug 2024 16:11:05 +0200 Subject: [PATCH 28/30] Clean --- .../src/api/routes/tests/rowAction.spec.ts | 48 ++++--------------- .../src/tests/utilities/api/rowAction.ts | 12 +++-- 2 files changed, 19 insertions(+), 41 deletions(-) diff --git a/packages/server/src/api/routes/tests/rowAction.spec.ts b/packages/server/src/api/routes/tests/rowAction.spec.ts index aab26a2e2f..40ba2c1811 100644 --- a/packages/server/src/api/routes/tests/rowAction.spec.ts +++ b/packages/server/src/api/routes/tests/rowAction.spec.ts @@ -150,7 +150,7 @@ describe("/rowsActions", () => { it("trims row action names", async () => { const name = " action name " - const res = await createRowAction(tableId, { name }, { status: 201 }) + const res = await createRowAction(tableId, { name }) expect(res).toEqual( expect.objectContaining({ @@ -218,9 +218,7 @@ describe("/rowsActions", () => { id: generator.guid(), valueToIgnore: generator.string(), } - const res = await createRowAction(tableId, dirtyRowAction, { - status: 201, - }) + const res = await createRowAction(tableId, dirtyRowAction) expect(res).toEqual({ name: rowAction.name, @@ -283,9 +281,9 @@ describe("/rowsActions", () => { const action2 = await createRowAction(tableId, createRowActionRequest()) for (const automationId of [action1.automationId, action2.automationId]) { - expect( - await config.api.automation.get(automationId, { status: 200 }) - ).toEqual(expect.objectContaining({ _id: automationId })) + expect(await config.api.automation.get(automationId)).toEqual( + expect.objectContaining({ _id: automationId }) + ) } }) }) @@ -374,13 +372,7 @@ describe("/rowsActions", () => { }) it("trims row action names", async () => { - const rowAction = await createRowAction( - tableId, - createRowActionRequest(), - { - status: 201, - } - ) + const rowAction = await createRowAction(tableId, createRowActionRequest()) const res = await config.api.rowAction.update(tableId, rowAction.id, { name: " action name ", @@ -572,32 +564,16 @@ describe("/rowsActions", () => { }) it("can set permission views", async () => { - await config.api.rowAction.setViewPermission( - tableId, - viewId1, - actionId1, - { - status: 200, - body: {}, - } - ) + await config.api.rowAction.setViewPermission(tableId, viewId1, actionId1) const action1Result = await config.api.rowAction.setViewPermission( tableId, viewId2, - actionId1, - { - status: 200, - body: {}, - } + actionId1 ) const action2Result = await config.api.rowAction.setViewPermission( tableId, viewId1, - actionId2, - { - status: 200, - body: {}, - } + actionId2 ) const expectedAction1 = expect.objectContaining({ @@ -622,11 +598,7 @@ describe("/rowsActions", () => { const actionResult = await config.api.rowAction.unsetViewPermission( tableId, viewId1, - actionId1, - { - status: 200, - body: {}, - } + actionId1 ) const expectedAction = expect.objectContaining({ diff --git a/packages/server/src/tests/utilities/api/rowAction.ts b/packages/server/src/tests/utilities/api/rowAction.ts index 7ade1d835b..1adb60d235 100644 --- a/packages/server/src/tests/utilities/api/rowAction.ts +++ b/packages/server/src/tests/utilities/api/rowAction.ts @@ -18,8 +18,8 @@ export class RowActionAPI extends TestAPI { { body: rowAction, expectations: { + status: 201, ...expectations, - status: expectations?.status || 201, }, ...config, } @@ -82,7 +82,10 @@ export class RowActionAPI extends TestAPI { return await this._post( `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, { - expectations, + expectations: { + status: 200, + ...expectations, + }, ...config, } ) @@ -98,7 +101,10 @@ export class RowActionAPI extends TestAPI { return await this._delete( `/api/tables/${tableId}/actions/${rowActionId}/permissions/${viewId}`, { - expectations, + expectations: { + status: 200, + ...expectations, + }, ...config, } ) From a7a5041b9140ed1a260784b1e91e8f6c348ffa3c Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Thu, 29 Aug 2024 16:14:18 +0200 Subject: [PATCH 29/30] Remove error message catching --- packages/server/src/sdk/app/rowActions.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/packages/server/src/sdk/app/rowActions.ts b/packages/server/src/sdk/app/rowActions.ts index 3d558f5345..aed2dff1a4 100644 --- a/packages/server/src/sdk/app/rowActions.ts +++ b/packages/server/src/sdk/app/rowActions.ts @@ -5,7 +5,7 @@ import { TableRowActions, VirtualDocumentType, } from "@budibase/types" -import { generateRowActionsID } from "../../db/utils" +import { generateRowActionsID, isViewID } from "../../db/utils" import automations from "./automations" import { definitions as TRIGGER_DEFINITIONS } from "../../automations/triggerInfo" import * as triggers from "../../automations/triggers" @@ -155,13 +155,8 @@ export async function update( async function guardView(tableId: string, viewId: string) { let view - try { + if (isViewID(viewId)) { view = await sdk.views.get(viewId) - } catch (e: any) { - if (e?.message !== "Unable to extract table ID, is not a view ID") { - throw e - } - // View id is not valid } if (!view || view.tableId !== tableId) { throw new HTTPError(`View '${viewId}' not found in '${tableId}'`, 400) From 66fdf033985a57b92170fc5f4b5239ced45e5595 Mon Sep 17 00:00:00 2001 From: Peter Clement Date: Tue, 3 Sep 2024 09:33:41 +0100 Subject: [PATCH 30/30] Refactor automation orchestrator to support branching (#14461) * refactor automation thread * fix infinite loop in branching * remove try catch * remove spacing * remove unecessary addition of branch outputs * pr comments * remove loopstep instance variable * add test to cover failure scenario * add tests for other automationf ailure conditions * update test name * use private keyword instead of underscore * parse int / string safely * fix refs * add condition support for branching and tests * create helper function for recursing all search filters * move helper func * fix import --- .../server/src/automations/automationUtils.ts | 10 + .../tests/scenarios/scenarios.spec.ts | 257 +++++++- packages/server/src/threads/automation.ts | 607 +++++++++--------- packages/shared-core/src/filters.ts | 19 + packages/shared-core/src/helpers/helpers.ts | 29 + .../api/routes/global/tests/realEmail.spec.ts | 33 +- 6 files changed, 599 insertions(+), 356 deletions(-) diff --git a/packages/server/src/automations/automationUtils.ts b/packages/server/src/automations/automationUtils.ts index bb94df9131..eacf81ef92 100644 --- a/packages/server/src/automations/automationUtils.ts +++ b/packages/server/src/automations/automationUtils.ts @@ -294,3 +294,13 @@ export function typecastForLooping(input: LoopStepInputs) { } return input.binding } + +export function ensureMaxIterationsAsNumber( + value: number | string | undefined +): number | undefined { + if (typeof value === "number") return value + if (typeof value === "string") { + return parseInt(value) + } + return undefined +} diff --git a/packages/server/src/automations/tests/scenarios/scenarios.spec.ts b/packages/server/src/automations/tests/scenarios/scenarios.spec.ts index c0c30e46e4..a0dab7f177 100644 --- a/packages/server/src/automations/tests/scenarios/scenarios.spec.ts +++ b/packages/server/src/automations/tests/scenarios/scenarios.spec.ts @@ -23,42 +23,148 @@ describe("Automation Scenarios", () => { afterAll(setup.afterAll) - // eslint-disable-next-line jest/no-commented-out-tests - // describe("Branching automations", () => { - // eslint-disable-next-line jest/no-commented-out-tests - // it("should run an automation with a trigger, loop, and create row step", async () => { - // const builder = createAutomationBuilder({ - // name: "Test Trigger with Loop and Create Row", - // }) + describe("Branching automations", () => { + it("should run a multiple nested branching automation", async () => { + const builder = createAutomationBuilder({ + name: "Test Trigger with Loop and Create Row", + }) - // builder - // .serverLog({ text: "Starting automation" }) - // .branch({ - // topLevelBranch1: { - // steps: stepBuilder => - // stepBuilder.serverLog({ text: "Branch 1" }).branch({ - // branch1: { - // steps: stepBuilder => - // stepBuilder.serverLog({ text: "Branch 1.1" }), - // condition: { notEmpty: { column: 10 } }, - // }, - // branch2: { - // steps: stepBuilder => - // stepBuilder.serverLog({ text: "Branch 1.2" }), - // condition: { fuzzy: { column: "sadsd" } }, - // }, - // }), - // condition: { equal: { column: 10 } }, - // }, - // topLevelBranch2: { - // steps: stepBuilder => stepBuilder.serverLog({ text: "Branch 2" }), - // condition: { equal: { column: 20 } }, - // }, - // }) - // .run() - // }) + const results = await builder + .appAction({ fields: {} }) + .serverLog({ text: "Starting automation" }) + .branch({ + topLevelBranch1: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Branch 1" }).branch({ + branch1: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Branch 1.1" }), + condition: { + equal: { "steps.1.success": true }, + }, + }, + branch2: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Branch 1.2" }), + condition: { + equal: { "steps.1.success": false }, + }, + }, + }), + condition: { + equal: { "steps.1.success": true }, + }, + }, + topLevelBranch2: { + steps: stepBuilder => stepBuilder.serverLog({ text: "Branch 2" }), + condition: { + equal: { "steps.1.success": false }, + }, + }, + }) + .run() - // }) + expect(results.steps[2].outputs.message).toContain("Branch 1.1") + }) + + it("should execute correct branch based on string equality", async () => { + const builder = createAutomationBuilder({ + name: "String Equality Branching", + }) + + const results = await builder + .appAction({ fields: { status: "active" } }) + .branch({ + activeBranch: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Active user" }), + condition: { + equal: { "trigger.fields.status": "active" }, + }, + }, + inactiveBranch: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Inactive user" }), + condition: { + equal: { "trigger.fields.status": "inactive" }, + }, + }, + }) + .run() + + expect(results.steps[0].outputs.message).toContain("Active user") + }) + + it("should handle multiple conditions with AND operator", async () => { + const builder = createAutomationBuilder({ + name: "Multiple AND Conditions Branching", + }) + + const results = await builder + .appAction({ fields: { status: "active", role: "admin" } }) + .branch({ + activeAdminBranch: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Active admin user" }), + condition: { + $and: { + conditions: [ + { equal: { "trigger.fields.status": "active" } }, + { equal: { "trigger.fields.role": "admin" } }, + ], + }, + }, + }, + otherBranch: { + steps: stepBuilder => stepBuilder.serverLog({ text: "Other user" }), + condition: { + notEqual: { "trigger.fields.status": "active" }, + }, + }, + }) + .run() + + expect(results.steps[0].outputs.message).toContain("Active admin user") + }) + + it("should handle multiple conditions with OR operator", async () => { + const builder = createAutomationBuilder({ + name: "Multiple OR Conditions Branching", + }) + + const results = await builder + .appAction({ fields: { status: "test", role: "user" } }) + .branch({ + specialBranch: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Special user" }), + condition: { + $or: { + conditions: [ + { equal: { "trigger.fields.status": "test" } }, + { equal: { "trigger.fields.role": "admin" } }, + ], + }, + }, + }, + regularBranch: { + steps: stepBuilder => + stepBuilder.serverLog({ text: "Regular user" }), + condition: { + $and: { + conditions: [ + { notEqual: { "trigger.fields.status": "active" } }, + { notEqual: { "trigger.fields.role": "admin" } }, + ], + }, + }, + }, + }) + .run() + + expect(results.steps[0].outputs.message).toContain("Special user") + }) + }) describe("Loop automations", () => { it("should run an automation with a trigger, loop, and create row step", async () => { @@ -108,6 +214,89 @@ describe("Automation Scenarios", () => { }) }) + it("should run an automation where a loop step is between two normal steps to ensure context correctness", async () => { + const builder = createAutomationBuilder({ + name: "Test Trigger with Loop and Create Row", + }) + + const results = await builder + .rowSaved( + { tableId: table._id! }, + { + row: { + name: "Trigger Row", + description: "This row triggers the automation", + }, + id: "1234", + revision: "1", + } + ) + .queryRows({ + tableId: table._id!, + }) + .loop({ + option: LoopStepType.ARRAY, + binding: [1, 2, 3], + }) + .serverLog({ text: "Message {{loop.currentItem}}" }) + .serverLog({ text: "{{steps.1.rows.0._id}}" }) + .run() + + results.steps[1].outputs.items.forEach( + (output: ServerLogStepOutputs, index: number) => { + expect(output).toMatchObject({ + success: true, + }) + expect(output.message).toContain(`Message ${index + 1}`) + } + ) + + expect(results.steps[2].outputs.message).toContain("ro_ta") + }) + + it("if an incorrect type is passed to the loop it should return an error", async () => { + const builder = createAutomationBuilder({ + name: "Test Loop error", + }) + + const results = await builder + .appAction({ fields: {} }) + .loop({ + option: LoopStepType.ARRAY, + binding: "1, 2, 3", + }) + .serverLog({ text: "Message {{loop.currentItem}}" }) + .run() + + expect(results.steps[0].outputs).toEqual({ + success: false, + status: "INCORRECT_TYPE", + }) + }) + + it("ensure the loop stops if the failure condition is reached", async () => { + const builder = createAutomationBuilder({ + name: "Test Loop error", + }) + + const results = await builder + .appAction({ fields: {} }) + .loop({ + option: LoopStepType.ARRAY, + binding: ["test", "test2", "test3"], + failure: "test2", + }) + .serverLog({ text: "Message {{loop.currentItem}}" }) + .run() + + expect(results.steps[0].outputs).toEqual( + expect.objectContaining({ + status: "FAILURE_CONDITION_MET", + success: false, + }) + ) + }) + it("should run an automation where a loop is successfully run twice", async () => { const builder = createAutomationBuilder({ name: "Test Trigger with Loop and Create Row", diff --git a/packages/server/src/threads/automation.ts b/packages/server/src/threads/automation.ts index c2470e78d4..eff8407104 100644 --- a/packages/server/src/threads/automation.ts +++ b/packages/server/src/threads/automation.ts @@ -8,7 +8,7 @@ import { import * as actions from "../automations/actions" import * as automationUtils from "../automations/automationUtils" import { replaceFakeBindings } from "../automations/loopUtils" - +import { dataFilters, helpers } from "@budibase/shared-core" import { default as AutomationEmitter } from "../events/AutomationEmitter" import { generateAutomationMetadataID, isProdAppID } from "../db/utils" import { definitions as triggerDefs } from "../automations/triggerInfo" @@ -23,12 +23,14 @@ import { AutomationStatus, AutomationStep, AutomationStepStatus, + BranchStep, LoopStep, + SearchFilters, } from "@budibase/types" import { AutomationContext, TriggerOutput } from "../definitions/automations" import { WorkerCallback } from "./definitions" import { context, logging } from "@budibase/backend-core" -import { processObject } from "@budibase/string-templates" +import { processObject, processStringSync } from "@budibase/string-templates" import { cloneDeep } from "lodash/fp" import { performance } from "perf_hooks" import * as sdkUtils from "../sdk/utils" @@ -64,36 +66,40 @@ function getLoopIterations(loopStep: LoopStep) { * inputs and handles any outputs. */ class Orchestrator { - _chainCount: number - _appId: string - _automation: Automation - _emitter: any - _context: AutomationContext - _job: Job - executionOutput: AutomationContext + private chainCount: number + private appId: string + private automation: Automation + private emitter: any + private context: AutomationContext + private job: Job + private loopStepOutputs: LoopStep[] + private stopped: boolean + private executionOutput: AutomationContext constructor(job: AutomationJob) { let automation = job.data.automation let triggerOutput = job.data.event const metadata = triggerOutput.metadata - this._chainCount = metadata ? metadata.automationChainCount! : 0 - this._appId = triggerOutput.appId as string - this._job = job + this.chainCount = metadata ? metadata.automationChainCount! : 0 + this.appId = triggerOutput.appId as string + this.job = job const triggerStepId = automation.definition.trigger.stepId triggerOutput = this.cleanupTriggerOutputs(triggerStepId, triggerOutput) // remove from context delete triggerOutput.appId delete triggerOutput.metadata // step zero is never used as the template string is zero indexed for customer facing - this._context = { steps: [{}], trigger: triggerOutput } - this._automation = automation + this.context = { steps: [{}], trigger: triggerOutput } + this.automation = automation // create an emitter which has the chain count for this automation run in it, so it can block // excessive chaining if required - this._emitter = new AutomationEmitter(this._chainCount + 1) + this.emitter = new AutomationEmitter(this.chainCount + 1) this.executionOutput = { trigger: {}, steps: [] } // setup the execution output const triggerId = automation.definition.trigger.id this.updateExecutionOutput(triggerId, triggerStepId, null, triggerOutput) + this.loopStepOutputs = [] + this.stopped = false } cleanupTriggerOutputs(stepId: string, triggerOutput: TriggerOutput) { @@ -112,7 +118,7 @@ class Orchestrator { } async getMetadata(): Promise { - const metadataId = generateAutomationMetadataID(this._automation._id!) + const metadataId = generateAutomationMetadataID(this.automation._id!) const db = context.getAppDB() let metadata: AutomationMetadata try { @@ -127,15 +133,15 @@ class Orchestrator { } async stopCron(reason: string) { - if (!this._job.opts.repeat) { + if (!this.job.opts.repeat) { return } logging.logWarn( - `CRON disabled reason=${reason} - ${this._appId}/${this._automation._id}` + `CRON disabled reason=${reason} - ${this.appId}/${this.automation._id}` ) - const automation = this._automation + const automation = this.automation const trigger = automation.definition.trigger - await disableCronById(this._job.id) + await disableCronById(this.job.id) this.updateExecutionOutput( trigger.id, trigger.stepId, @@ -149,7 +155,7 @@ class Orchestrator { } async checkIfShouldStop(metadata: AutomationMetadata): Promise { - if (!metadata.errorCount || !this._job.opts.repeat) { + if (!metadata.errorCount || !this.job.opts.repeat) { return false } if (metadata.errorCount >= MAX_AUTOMATION_RECURRING_ERRORS) { @@ -161,7 +167,7 @@ class Orchestrator { async updateMetadata(metadata: AutomationMetadata) { const output = this.executionOutput, - automation = this._automation + automation = this.automation if (!output || !isRecurring(automation)) { return } @@ -216,7 +222,7 @@ class Orchestrator { output: any, result: { success: boolean; status: string } ) { - if (!currentLoopStepIndex) { + if (currentLoopStepIndex === undefined) { throw new Error("No loop step number provided.") } this.executionOutput.steps.splice(currentLoopStepIndex, 0, { @@ -229,7 +235,7 @@ class Orchestrator { }, inputs: step.inputs, }) - this._context.steps.splice(currentLoopStepIndex, 0, { + this.context.steps.splice(currentLoopStepIndex, 0, { ...output, success: result.success, status: result.status, @@ -242,25 +248,15 @@ class Orchestrator { { resource: "automation" }, async span => { span?.addTags({ - appId: this._appId, - automationId: this._automation._id, + appId: this.appId, + automationId: this.automation._id, }) + this.context.env = await sdkUtils.getEnvironmentVariables() - // this will retrieve from context created at start of thread - this._context.env = await sdkUtils.getEnvironmentVariables() - let automation = this._automation - let stopped = false - let loopStep: LoopStep | undefined - - let stepCount = 0 - let currentLoopStepIndex: number = 0 - let loopSteps: LoopStep[] | undefined = [] let metadata - let timeoutFlag = false - let wasLoopStep = false - let timeout = this._job.data.event.timeout + // check if this is a recurring automation, - if (isProdAppID(this._appId) && isRecurring(automation)) { + if (isProdAppID(this.appId) && isRecurring(this.automation)) { span?.addTags({ recurring: true }) metadata = await this.getMetadata() const shouldStop = await this.checkIfShouldStop(metadata) @@ -270,272 +266,22 @@ class Orchestrator { } } const start = performance.now() - for (const step of automation.definition.steps) { - const stepSpan = tracer.startSpan("Orchestrator.execute.step", { - childOf: span, - }) - stepSpan.addTags({ - resource: "automation", - step: { - stepId: step.stepId, - id: step.id, - name: step.name, - type: step.type, - title: step.stepTitle, - internal: step.internal, - deprecated: step.deprecated, - }, - }) - let input, - iterations = 1, - iterationCount = 0 - - try { - if (timeoutFlag) { - span?.addTags({ timedOut: true }) - break - } - - if (timeout) { - setTimeout(() => { - timeoutFlag = true - }, timeout || env.AUTOMATION_THREAD_TIMEOUT) - } - - stepCount++ - if (step.stepId === AutomationActionStepId.LOOP) { - loopStep = step - currentLoopStepIndex = stepCount - continue - } - - if (loopStep) { - input = await processObject(loopStep.inputs, this._context) - iterations = getLoopIterations(loopStep) - stepSpan?.addTags({ step: { iterations } }) - } - - for (let stepIndex = 0; stepIndex < iterations; stepIndex++) { - let originalStepInput = cloneDeep(step.inputs) - if (loopStep && input?.binding) { - let tempOutput = { - items: loopSteps, - iterations: iterationCount, - } - try { - loopStep.inputs.binding = automationUtils.typecastForLooping( - loopStep.inputs - ) - } catch (err) { - this.updateContextAndOutput( - currentLoopStepIndex, - step, - tempOutput, - { - status: AutomationErrors.INCORRECT_TYPE, - success: false, - } - ) - loopSteps = undefined - loopStep = undefined - break - } - let item: any[] = [] - if ( - typeof loopStep.inputs.binding === "string" && - loopStep.inputs.option === "String" - ) { - item = automationUtils.stringSplit(loopStep.inputs.binding) - } else if (Array.isArray(loopStep.inputs.binding)) { - item = loopStep.inputs.binding - } - this._context.steps[currentLoopStepIndex] = { - currentItem: item[stepIndex], - } - - originalStepInput = replaceFakeBindings( - originalStepInput, - currentLoopStepIndex - ) - - if ( - stepIndex === env.AUTOMATION_MAX_ITERATIONS || - (loopStep.inputs.iterations && - stepIndex === loopStep.inputs.iterations) - ) { - this.updateContextAndOutput( - currentLoopStepIndex, - step, - tempOutput, - { - status: AutomationErrors.MAX_ITERATIONS, - success: true, - } - ) - loopSteps = undefined - loopStep = undefined - break - } - - let isFailure = false - const currentItem = - this._context.steps[currentLoopStepIndex]?.currentItem - if (currentItem && typeof currentItem === "object") { - isFailure = Object.keys(currentItem).some(value => { - return currentItem[value] === loopStep?.inputs.failure - }) - } else { - isFailure = - currentItem && currentItem === loopStep.inputs.failure - } - - if (isFailure) { - this.updateContextAndOutput( - currentLoopStepIndex, - step, - tempOutput, - { - status: AutomationErrors.FAILURE_CONDITION, - success: false, - } - ) - loopSteps = undefined - loopStep = undefined - break - } - } - - // execution stopped, record state for that - if (stopped) { - this.updateExecutionOutput( - step.id, - step.stepId, - {}, - STOPPED_STATUS - ) - continue - } - - let stepFn = await this.getStepFunctionality( - step.stepId as AutomationActionStepId - ) - let inputs = await processObject(originalStepInput, this._context) - inputs = automationUtils.cleanInputValues( - inputs, - step.schema.inputs - ) - try { - // appId is always passed - const outputs = await stepFn({ - inputs: inputs, - appId: this._appId, - emitter: this._emitter, - context: this._context, - }) - - this._context.steps[stepCount] = outputs - // if filter causes us to stop execution don't break the loop, set a var - // so that we can finish iterating through the steps and record that it stopped - if ( - step.stepId === AutomationActionStepId.FILTER && - !outputs.result - ) { - stopped = true - this.updateExecutionOutput( - step.id, - step.stepId, - step.inputs, - { - ...outputs, - ...STOPPED_STATUS, - } - ) - continue - } - if (loopStep && loopSteps) { - loopSteps.push(outputs) - } else { - this.updateExecutionOutput( - step.id, - step.stepId, - step.inputs, - outputs - ) - } - } catch (err) { - console.error(`Automation error - ${step.stepId} - ${err}`) - return err - } - - if (loopStep) { - iterationCount++ - if (stepIndex === iterations - 1) { - loopStep = undefined - this._context.steps.splice(currentLoopStepIndex, 1) - break - } - } - } - } finally { - stepSpan?.finish() - } - - if (loopStep && iterations === 0) { - loopStep = undefined - this.executionOutput.steps.splice(currentLoopStepIndex + 1, 0, { - id: step.id, - stepId: step.stepId, - outputs: { - status: AutomationStepStatus.NO_ITERATIONS, - success: true, - }, - inputs: {}, - }) - - this._context.steps.splice(currentLoopStepIndex, 1) - iterations = 1 - } - - // Delete the step after the loop step as it's irrelevant, since information is included - // in the loop step - if (wasLoopStep && !loopStep) { - this._context.steps.splice(currentLoopStepIndex + 1, 1) - wasLoopStep = false - } - if (loopSteps && loopSteps.length) { - let tempOutput = { - success: true, - items: loopSteps, - iterations: iterationCount, - } - this.executionOutput.steps.splice(currentLoopStepIndex + 1, 0, { - id: step.id, - stepId: step.stepId, - outputs: tempOutput, - inputs: step.inputs, - }) - this._context.steps[currentLoopStepIndex] = tempOutput - - wasLoopStep = true - loopSteps = [] - } - } + await this.executeSteps(this.automation.definition.steps) const end = performance.now() const executionTime = end - start console.info( - `Automation ID: ${automation._id} Execution time: ${executionTime} milliseconds`, + `Automation ID: ${this.automation._id} Execution time: ${executionTime} milliseconds`, { _logKey: "automation", executionTime, } ) - // store the logs for the automation run try { - await storeLog(this._automation, this.executionOutput) + await storeLog(this.automation, this.executionOutput) } catch (e: any) { if (e.status === 413 && e.request?.data) { // if content is too large we shouldn't log it @@ -544,13 +290,288 @@ class Orchestrator { } logging.logAlert("Error writing automation log", e) } - if (isProdAppID(this._appId) && isRecurring(automation) && metadata) { + if ( + isProdAppID(this.appId) && + isRecurring(this.automation) && + metadata + ) { await this.updateMetadata(metadata) } return this.executionOutput } ) } + + private async executeSteps(steps: AutomationStep[]): Promise { + return tracer.trace( + "Orchestrator.executeSteps", + { resource: "automation" }, + async span => { + let stepIndex = 0 + const timeout = + this.job.data.event.timeout || env.AUTOMATION_THREAD_TIMEOUT + + try { + await helpers.withTimeout( + timeout, + (async () => { + while (stepIndex < steps.length) { + const step = steps[stepIndex] + if (step.stepId === AutomationActionStepId.BRANCH) { + await this.executeBranchStep(step) + stepIndex++ + } else if (step.stepId === AutomationActionStepId.LOOP) { + stepIndex = await this.executeLoopStep(step, steps, stepIndex) + } else { + await this.executeStep(step) + stepIndex++ + } + } + })() + ) + } catch (error: any) { + if (error.errno === "ETIME") { + span?.addTags({ timedOut: true }) + console.warn(`Automation execution timed out after ${timeout}ms`) + } + } + } + ) + } + + private async executeLoopStep( + loopStep: LoopStep, + steps: AutomationStep[], + currentIndex: number + ): Promise { + await processObject(loopStep.inputs, this.context) + const iterations = getLoopIterations(loopStep) + let stepToLoopIndex = currentIndex + 1 + let iterationCount = 0 + let shouldCleanup = true + + for (let loopStepIndex = 0; loopStepIndex < iterations; loopStepIndex++) { + try { + loopStep.inputs.binding = automationUtils.typecastForLooping( + loopStep.inputs + ) + } catch (err) { + this.updateContextAndOutput( + stepToLoopIndex, + steps[stepToLoopIndex], + {}, + { + status: AutomationErrors.INCORRECT_TYPE, + success: false, + } + ) + shouldCleanup = false + break + } + const maxIterations = automationUtils.ensureMaxIterationsAsNumber( + loopStep.inputs.iterations + ) + + if ( + loopStepIndex === env.AUTOMATION_MAX_ITERATIONS || + (loopStep.inputs.iterations && loopStepIndex === maxIterations) + ) { + this.updateContextAndOutput( + stepToLoopIndex, + steps[stepToLoopIndex], + { + items: this.loopStepOutputs, + iterations: loopStepIndex, + }, + { + status: AutomationErrors.MAX_ITERATIONS, + success: true, + } + ) + shouldCleanup = false + break + } + + let isFailure = false + const currentItem = this.getCurrentLoopItem(loopStep, loopStepIndex) + if (currentItem && typeof currentItem === "object") { + isFailure = Object.keys(currentItem).some(value => { + return currentItem[value] === loopStep?.inputs.failure + }) + } else { + isFailure = currentItem && currentItem === loopStep.inputs.failure + } + + if (isFailure) { + this.updateContextAndOutput( + loopStepIndex, + steps[stepToLoopIndex], + { + items: this.loopStepOutputs, + iterations: loopStepIndex, + }, + { + status: AutomationErrors.FAILURE_CONDITION, + success: false, + } + ) + shouldCleanup = false + break + } + + this.context.steps[currentIndex + 1] = { + currentItem: this.getCurrentLoopItem(loopStep, loopStepIndex), + } + + stepToLoopIndex = currentIndex + 1 + + await this.executeStep(steps[stepToLoopIndex], stepToLoopIndex) + iterationCount++ + } + + if (shouldCleanup) { + let tempOutput = + iterations === 0 + ? { + status: AutomationStepStatus.NO_ITERATIONS, + success: true, + } + : { + success: true, + items: this.loopStepOutputs, + iterations: iterationCount, + } + + // Loop Step clean up + this.executionOutput.steps.splice(currentIndex + 1, 0, { + id: steps[stepToLoopIndex].id, + stepId: steps[stepToLoopIndex].stepId, + outputs: tempOutput, + inputs: steps[stepToLoopIndex].inputs, + }) + this.context.steps[currentIndex + 1] = tempOutput + this.loopStepOutputs = [] + } + + return stepToLoopIndex + 1 + } + private async executeBranchStep(branchStep: BranchStep): Promise { + const { branches, children } = branchStep.inputs + + for (const branch of branches) { + const condition = await this.evaluateBranchCondition(branch.condition) + if (condition) { + const branchSteps = children?.[branch.name] || [] + await this.executeSteps(branchSteps) + break + } + } + } + + private async evaluateBranchCondition( + conditions: SearchFilters + ): Promise { + const toFilter: Record = {} + + const processedConditions = dataFilters.recurseSearchFilters( + conditions, + filter => { + Object.entries(filter).forEach(([_, value]) => { + Object.entries(value).forEach(([field, _]) => { + const fromContext = processStringSync( + `{{ literal ${field} }}`, + this.context + ) + toFilter[field] = fromContext + }) + }) + return filter + } + ) + + const result = dataFilters.runQuery([toFilter], processedConditions) + return result.length > 0 + } + private async executeStep( + step: AutomationStep, + loopIteration?: number + ): Promise { + return tracer.trace( + "Orchestrator.execute.step", + { resource: "automation" }, + async span => { + span?.addTags({ + resource: "automation", + step: { + stepId: step.stepId, + id: step.id, + name: step.name, + type: step.type, + title: step.stepTitle, + internal: step.internal, + deprecated: step.deprecated, + }, + }) + + if (this.stopped) { + this.updateExecutionOutput(step.id, step.stepId, {}, STOPPED_STATUS) + return + } + + let originalStepInput = cloneDeep(step.inputs) + if (loopIteration !== undefined) { + originalStepInput = replaceFakeBindings( + originalStepInput, + loopIteration + ) + } + const stepFn = await this.getStepFunctionality(step.stepId) + let inputs = await processObject(originalStepInput, this.context) + inputs = automationUtils.cleanInputValues(inputs, step.schema.inputs) + + const outputs = await stepFn({ + inputs: inputs, + appId: this.appId, + emitter: this.emitter, + context: this.context, + }) + this.handleStepOutput(step, outputs, loopIteration) + } + ) + } + + private getCurrentLoopItem(loopStep: LoopStep, index: number): any { + if (!loopStep) return null + if ( + typeof loopStep.inputs.binding === "string" && + loopStep.inputs.option === "String" + ) { + return automationUtils.stringSplit(loopStep.inputs.binding)[index] + } else if (Array.isArray(loopStep.inputs.binding)) { + return loopStep.inputs.binding[index] + } + return null + } + + private handleStepOutput( + step: AutomationStep, + outputs: any, + loopIteration: number | undefined + ): void { + if (step.stepId === AutomationActionStepId.FILTER && !outputs.result) { + this.stopped = true + this.updateExecutionOutput(step.id, step.stepId, step.inputs, { + ...outputs, + ...STOPPED_STATUS, + }) + } else if (loopIteration !== undefined) { + this.loopStepOutputs = this.loopStepOutputs || [] + this.loopStepOutputs.push(outputs) + } else { + this.updateExecutionOutput(step.id, step.stepId, step.inputs, outputs) + } + this.context.steps[this.context.steps.length] = outputs + } } export function execute(job: Job, callback: WorkerCallback) { diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index fa0b5c92ed..70fb24b373 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -127,6 +127,25 @@ export function recurseLogicalOperators( return filters } +export function recurseSearchFilters( + filters: SearchFilters, + processFn: (filter: SearchFilters) => SearchFilters +): SearchFilters { + // Process the current level + filters = processFn(filters) + + // Recurse through logical operators + for (const logical of Object.values(LogicalOperator)) { + if (filters[logical]) { + filters[logical]!.conditions = filters[logical]!.conditions.map( + condition => recurseSearchFilters(condition, processFn) + ) + } + } + + return filters +} + /** * Removes any fields that contain empty strings that would cause inconsistent * behaviour with how backend tables are filtered (no value means no filter). diff --git a/packages/shared-core/src/helpers/helpers.ts b/packages/shared-core/src/helpers/helpers.ts index 16891de35b..8dbdb7bbfd 100644 --- a/packages/shared-core/src/helpers/helpers.ts +++ b/packages/shared-core/src/helpers/helpers.ts @@ -83,3 +83,32 @@ export const getUserLabel = (user: User) => { return email } } + +export function cancelableTimeout( + timeout: number +): [Promise, () => void] { + let timeoutId: NodeJS.Timeout + return [ + new Promise((resolve, reject) => { + timeoutId = setTimeout(() => { + reject({ + status: 301, + errno: "ETIME", + }) + }, timeout) + }), + () => { + clearTimeout(timeoutId) + }, + ] +} + +export async function withTimeout( + timeout: number, + promise: Promise +): Promise { + const [timeoutPromise, cancel] = cancelableTimeout(timeout) + const result = (await Promise.race([promise, timeoutPromise])) as T + cancel() + return result +} diff --git a/packages/worker/src/api/routes/global/tests/realEmail.spec.ts b/packages/worker/src/api/routes/global/tests/realEmail.spec.ts index a18d8ee247..99dfb7f824 100644 --- a/packages/worker/src/api/routes/global/tests/realEmail.spec.ts +++ b/packages/worker/src/api/routes/global/tests/realEmail.spec.ts @@ -2,6 +2,8 @@ jest.unmock("node-fetch") import { TestConfiguration } from "../../../../tests" import { EmailTemplatePurpose } from "../../../../constants" import { objectStore } from "@budibase/backend-core" +import { helpers } from "@budibase/shared-core" + import tk from "timekeeper" import { EmailAttachment } from "@budibase/types" @@ -12,33 +14,6 @@ const nodemailer = require("nodemailer") // for the real email tests give them a long time to try complete/fail jest.setTimeout(30000) -function cancelableTimeout(timeout: number): [Promise, () => void] { - let timeoutId: NodeJS.Timeout - return [ - new Promise((resolve, reject) => { - timeoutId = setTimeout(() => { - reject({ - status: 301, - errno: "ETIME", - }) - }, timeout) - }), - () => { - clearTimeout(timeoutId) - }, - ] -} - -async function withTimeout( - timeout: number, - promise: Promise -): Promise { - const [timeoutPromise, cancel] = cancelableTimeout(timeout) - const result = (await Promise.race([promise, timeoutPromise])) as T - cancel() - return result -} - describe("/api/global/email", () => { const config = new TestConfiguration() @@ -57,8 +32,8 @@ describe("/api/global/email", () => { ) { let response, text try { - await withTimeout(20000, config.saveEtherealSmtpConfig()) - await withTimeout(20000, config.saveSettingsConfig()) + await helpers.withTimeout(20000, config.saveEtherealSmtpConfig()) + await helpers.withTimeout(20000, config.saveSettingsConfig()) let res if (attachments) { res = await config.api.emails