From a6f6942288f674a902de77220d030e8cb4d81f6a Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 3 Oct 2024 16:10:07 +0100 Subject: [PATCH 01/57] Fixing an issue with corrupt relationship records referencing rows which don't exist, this is a temporary measure as these relationships should be cleaned up correctly but for now ignore any which reference rows which no longer exist. --- .../backend-core/src/db/couch/DatabaseImpl.ts | 12 +++++++---- .../src/db/linkedRows/LinkController.ts | 20 ++++++++++--------- packages/types/src/sdk/db.ts | 2 +- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/packages/backend-core/src/db/couch/DatabaseImpl.ts b/packages/backend-core/src/db/couch/DatabaseImpl.ts index 274c1b9e93..472e0f44ad 100644 --- a/packages/backend-core/src/db/couch/DatabaseImpl.ts +++ b/packages/backend-core/src/db/couch/DatabaseImpl.ts @@ -213,17 +213,21 @@ export class DatabaseImpl implements Database { async getMultiple( ids: string[], - opts?: { allowMissing?: boolean } + opts?: { allowMissing?: boolean; excludeDocs?: boolean } ): Promise { // get unique ids = [...new Set(ids)] + const includeDocs = !opts?.excludeDocs const response = await this.allDocs({ keys: ids, - include_docs: true, + include_docs: includeDocs, }) const rowUnavailable = (row: RowResponse) => { // row is deleted - key lookup can return this - if (row.doc == null || ("deleted" in row.value && row.value.deleted)) { + if ( + (includeDocs && row.doc == null) || + (row.value && "deleted" in row.value && row.value.deleted) + ) { return true } return row.error === "not_found" @@ -237,7 +241,7 @@ export class DatabaseImpl implements Database { const missingIds = missing.map(row => row.key).join(", ") throw new Error(`Unable to get documents: ${missingIds}`) } - return rows.map(row => row.doc!) + return rows.map(row => (includeDocs ? row.doc! : row.value)) } async remove(idOrDoc: string | Document, rev?: string) { diff --git a/packages/server/src/db/linkedRows/LinkController.ts b/packages/server/src/db/linkedRows/LinkController.ts index 85a160713b..3c0601c9ee 100644 --- a/packages/server/src/db/linkedRows/LinkController.ts +++ b/packages/server/src/db/linkedRows/LinkController.ts @@ -211,19 +211,21 @@ class LinkController { linkedSchema?.type === FieldType.LINK && linkedSchema?.relationshipType === RelationshipType.ONE_TO_MANY ) { - let links = ( - await getLinkDocuments({ - tableId: field.tableId, - rowId: linkId, - }) - ).filter( - link => - link.id !== row._id && link.fieldName === linkedSchema.name + let links = await getLinkDocuments({ + tableId: field.tableId, + rowId: linkId, + fieldName: linkedSchema.name, + }) + + // check all the related rows exist + const foundRecords = await this._db.getMultiple( + links.map(l => l.id), + { allowMissing: true, excludeDocs: true } ) // The 1 side of 1:N is already related to something else // You must remove the existing relationship - if (links.length > 0) { + if (foundRecords.length > 0) { throw new Error( `1:N Relationship Error: Record already linked to another.` ) diff --git a/packages/types/src/sdk/db.ts b/packages/types/src/sdk/db.ts index a081f4f1a2..49baaa5bb1 100644 --- a/packages/types/src/sdk/db.ts +++ b/packages/types/src/sdk/db.ts @@ -133,7 +133,7 @@ export interface Database { exists(docId: string): Promise getMultiple( ids: string[], - opts?: { allowMissing?: boolean } + opts?: { allowMissing?: boolean; excludeDocs?: boolean } ): Promise remove(idOrDoc: Document): Promise remove(idOrDoc: string, rev?: string): Promise From 86846eff3f79192385f420b200b1a58b59a0042f Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 3 Oct 2024 16:22:32 +0100 Subject: [PATCH 02/57] Small fix. --- packages/server/src/db/linkedRows/LinkController.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/server/src/db/linkedRows/LinkController.ts b/packages/server/src/db/linkedRows/LinkController.ts index 3c0601c9ee..944542e73f 100644 --- a/packages/server/src/db/linkedRows/LinkController.ts +++ b/packages/server/src/db/linkedRows/LinkController.ts @@ -214,8 +214,10 @@ class LinkController { let links = await getLinkDocuments({ tableId: field.tableId, rowId: linkId, - fieldName: linkedSchema.name, - }) + }).filter( + link => + link.id !== row._id && link.fieldName === linkedSchema.name + ) // check all the related rows exist const foundRecords = await this._db.getMultiple( From 6fb844753b4ffcc7b3af857c723a0f193b2bc1f5 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 3 Oct 2024 16:24:54 +0100 Subject: [PATCH 03/57] Another small fix. --- packages/server/src/db/linkedRows/LinkController.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/server/src/db/linkedRows/LinkController.ts b/packages/server/src/db/linkedRows/LinkController.ts index 944542e73f..1cd4240d4b 100644 --- a/packages/server/src/db/linkedRows/LinkController.ts +++ b/packages/server/src/db/linkedRows/LinkController.ts @@ -211,10 +211,12 @@ class LinkController { linkedSchema?.type === FieldType.LINK && linkedSchema?.relationshipType === RelationshipType.ONE_TO_MANY ) { - let links = await getLinkDocuments({ - tableId: field.tableId, - rowId: linkId, - }).filter( + let links = ( + await getLinkDocuments({ + tableId: field.tableId, + rowId: linkId, + }) + ).filter( link => link.id !== row._id && link.fieldName === linkedSchema.name ) From 586dd8fea74726963177af3118806fe4eafd6288 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Thu, 3 Oct 2024 18:10:58 +0100 Subject: [PATCH 04/57] Fixing an issue with newlines between coalesce statements in Postgres - we were escaping newlines even if they were valid when given a list of JSON operations to perform. --- .../routes/tests/queries/generic-sql.spec.ts | 29 +++++++++++++++++++ packages/server/src/integrations/postgres.ts | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts index e6da4693eb..0979f8bed3 100644 --- a/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts +++ b/packages/server/src/api/routes/tests/queries/generic-sql.spec.ts @@ -28,6 +28,7 @@ describe.each( const config = setup.getConfig() const isOracle = dbName === DatabaseName.ORACLE const isMsSQL = dbName === DatabaseName.SQL_SERVER + const isPostgres = dbName === DatabaseName.POSTGRES let rawDatasource: Datasource let datasource: Datasource @@ -47,6 +48,9 @@ describe.each( transformer: "return data", readable: true, } + if (query.fields?.sql && typeof query.fields.sql !== "string") { + throw new Error("Unable to create with knex structure in 'sql' field") + } return await config.api.query.save( { ...defaultQuery, ...query }, expectations @@ -207,6 +211,31 @@ describe.each( expect(prodQuery.parameters).toBeUndefined() expect(prodQuery.schema).toBeDefined() }) + + isPostgres && + it("should be able to handle a JSON aggregate with newlines", async () => { + const jsonStatement = `COALESCE(json_build_object('name', name),'{"name":{}}'::json)` + const query = await createQuery({ + fields: { + sql: client("test_table") + .select([ + "*", + client.raw( + `${jsonStatement} as json,\n${jsonStatement} as json2` + ), + ]) + .toString(), + }, + }) + const res = await config.api.query.execute( + query._id!, + {}, + { + status: 200, + } + ) + expect(res).toBeDefined() + }) }) }) diff --git a/packages/server/src/integrations/postgres.ts b/packages/server/src/integrations/postgres.ts index 3652864991..28400f616f 100644 --- a/packages/server/src/integrations/postgres.ts +++ b/packages/server/src/integrations/postgres.ts @@ -41,7 +41,7 @@ if (types) { types.setTypeParser(1184, (val: any) => val) // timestampz } -const JSON_REGEX = /'{.*}'::json/s +const JSON_REGEX = /'{\s*.*?\s*}'::json/gs const Sql = sql.Sql interface PostgresConfig { From 11804f6ddd5318c80a80ac06a11368f89ad4d284 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Mon, 7 Oct 2024 18:18:04 +0100 Subject: [PATCH 05/57] Create a feature flag helper for tests. --- .../backend-core/src/features/features.ts | 300 ++++++++++++++++++ packages/backend-core/src/features/index.ts | 283 +---------------- .../backend-core/src/features/tests/utils.ts | 64 ++++ .../src/api/routes/tests/application.spec.ts | 11 +- .../server/src/api/routes/tests/row.spec.ts | 30 +- .../src/api/routes/tests/search.spec.ts | 18 +- .../src/api/routes/tests/templates.spec.ts | 73 +++-- .../src/api/routes/tests/viewV2.spec.ts | 24 +- .../tests/20240604153647_initial_sqs.spec.ts | 5 +- .../sdk/app/rows/search/tests/search.spec.ts | 15 +- .../tests/outputProcessing.spec.ts | 8 +- .../api/routes/global/tests/auditLogs.spec.ts | 10 +- 12 files changed, 458 insertions(+), 383 deletions(-) create mode 100644 packages/backend-core/src/features/features.ts create mode 100644 packages/backend-core/src/features/tests/utils.ts diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts new file mode 100644 index 0000000000..90a395d52a --- /dev/null +++ b/packages/backend-core/src/features/features.ts @@ -0,0 +1,300 @@ +import env from "../environment" +import * as context from "../context" +import { PostHog, PostHogOptions } from "posthog-node" +import { FeatureFlag, IdentityType, UserCtx } from "@budibase/types" +import tracer from "dd-trace" +import { Duration } from "../utils" + +let posthog: PostHog | undefined +export function init(opts?: PostHogOptions) { + if ( + env.POSTHOG_TOKEN && + env.POSTHOG_API_HOST && + !env.SELF_HOSTED && + env.POSTHOG_FEATURE_FLAGS_ENABLED + ) { + console.log("initializing posthog client...") + posthog = new PostHog(env.POSTHOG_TOKEN, { + host: env.POSTHOG_API_HOST, + personalApiKey: env.POSTHOG_PERSONAL_TOKEN, + featureFlagsPollingInterval: Duration.fromMinutes(3).toMs(), + ...opts, + }) + } else { + console.log("posthog disabled") + } +} + +export function shutdown() { + posthog?.shutdown() +} + +export abstract class Flag { + static boolean(defaultValue: boolean): Flag { + return new BooleanFlag(defaultValue) + } + + static string(defaultValue: string): Flag { + return new StringFlag(defaultValue) + } + + static number(defaultValue: number): Flag { + return new NumberFlag(defaultValue) + } + + protected constructor(public defaultValue: T) {} + + abstract parse(value: any): T +} + +type UnwrapFlag = F extends Flag ? U : never + +export type FlagValues = { + [K in keyof T]: UnwrapFlag +} + +type KeysOfType = { + [K in keyof T]: T[K] extends Flag ? K : never +}[keyof T] + +class BooleanFlag extends Flag { + parse(value: any) { + if (typeof value === "string") { + return ["true", "t", "1"].includes(value.toLowerCase()) + } + + if (typeof value === "boolean") { + return value + } + + throw new Error(`could not parse value "${value}" as boolean`) + } +} + +class StringFlag extends Flag { + parse(value: any) { + if (typeof value === "string") { + return value + } + throw new Error(`could not parse value "${value}" as string`) + } +} + +class NumberFlag extends Flag { + parse(value: any) { + if (typeof value === "number") { + return value + } + + if (typeof value === "string") { + const parsed = parseFloat(value) + if (!isNaN(parsed)) { + return parsed + } + } + + throw new Error(`could not parse value "${value}" as number`) + } +} + +export interface EnvFlagEntry { + tenantId: string + key: string + value: boolean +} + +export function parseEnvFlags(flags: string): EnvFlagEntry[] { + const split = flags.split(",").map(x => x.split(":")) + const result: EnvFlagEntry[] = [] + for (const [tenantId, ...features] of split) { + for (let feature of features) { + let value = true + if (feature.startsWith("!")) { + feature = feature.slice(1) + value = false + } + result.push({ tenantId, key: feature, value }) + } + } + return result +} + +export class FlagSet, T extends { [key: string]: V }> { + // This is used to safely cache flags sets in the current request context. + // Because multiple sets could theoretically exist, we don't want the cache of + // one to leak into another. + private readonly setId: string + + constructor(private readonly flagSchema: T) { + this.setId = crypto.randomUUID() + } + + defaults(): FlagValues { + return Object.keys(this.flagSchema).reduce((acc, key) => { + const typedKey = key as keyof T + acc[typedKey] = this.flagSchema[key].defaultValue + return acc + }, {} as FlagValues) + } + + isFlagName(name: string | number | symbol): name is keyof T { + return this.flagSchema[name as keyof T] !== undefined + } + + async get( + key: K, + ctx?: UserCtx + ): Promise[K]> { + const flags = await this.fetch(ctx) + return flags[key] + } + + async isEnabled>( + key: K, + ctx?: UserCtx + ): Promise { + const flags = await this.fetch(ctx) + return flags[key] + } + + async fetch(ctx?: UserCtx): Promise> { + return await tracer.trace("features.fetch", async span => { + const cachedFlags = context.getFeatureFlags>(this.setId) + if (cachedFlags) { + span?.addTags({ fromCache: true }) + return cachedFlags + } + + const tags: Record = {} + const flagValues = this.defaults() + const currentTenantId = context.getTenantId() + const specificallySetFalse = new Set() + + for (const { tenantId, key, value } of parseEnvFlags( + env.TENANT_FEATURE_FLAGS || "" + )) { + if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { + continue + } + + tags[`readFromEnvironmentVars`] = true + + if (value === false) { + specificallySetFalse.add(key) + } + + // ignore unknown flags + if (!this.isFlagName(key)) { + continue + } + + if (typeof flagValues[key] !== "boolean") { + throw new Error(`Feature: ${key} is not a boolean`) + } + + // @ts-expect-error - TS does not like you writing into a generic type, + // but we know that it's okay in this case because it's just an object. + flagValues[key as keyof FlagValues] = value + tags[`flags.${key}.source`] = "environment" + } + + const license = ctx?.user?.license + if (license) { + tags[`readFromLicense`] = true + + for (const feature of license.features) { + if (!this.isFlagName(feature)) { + continue + } + + if ( + flagValues[feature] === true || + specificallySetFalse.has(feature) + ) { + // If the flag is already set to through environment variables, we + // don't want to override it back to false here. + continue + } + + // @ts-expect-error - TS does not like you writing into a generic type, + // but we know that it's okay in this case because it's just an object. + flagValues[feature] = true + tags[`flags.${feature}.source`] = "license" + } + } + + const identity = context.getIdentity() + tags[`identity.type`] = identity?.type + tags[`identity.tenantId`] = identity?.tenantId + tags[`identity._id`] = identity?._id + + if (posthog && identity?.type === IdentityType.USER) { + tags[`readFromPostHog`] = true + + const personProperties: Record = {} + if (identity.tenantId) { + personProperties.tenantId = identity.tenantId + } + + const posthogFlags = await posthog.getAllFlagsAndPayloads( + identity._id, + { + personProperties, + } + ) + + for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { + if (!this.isFlagName(name)) { + // We don't want an unexpected PostHog flag to break the app, so we + // just log it and continue. + console.warn(`Unexpected posthog flag "${name}": ${value}`) + continue + } + + if (flagValues[name] === true || specificallySetFalse.has(name)) { + // If the flag is already set to through environment variables, we + // don't want to override it back to false here. + continue + } + + const payload = posthogFlags.featureFlagPayloads?.[name] + const flag = this.flagSchema[name] + try { + // @ts-expect-error - TS does not like you writing into a generic + // type, but we know that it's okay in this case because it's just + // an object. + flagValues[name] = flag.parse(payload || value) + tags[`flags.${name}.source`] = "posthog" + } catch (err) { + // We don't want an invalid PostHog flag to break the app, so we just + // log it and continue. + console.warn(`Error parsing posthog flag "${name}": ${value}`, err) + } + } + } + + context.setFeatureFlags(this.setId, flagValues) + for (const [key, value] of Object.entries(flagValues)) { + tags[`flags.${key}.value`] = value + } + span?.addTags(tags) + + return flagValues + }) + } +} + +// This is the primary source of truth for feature flags. If you want to add a +// new flag, add it here and use the `fetch` and `get` functions to access it. +// All of the machinery in this file is to make sure that flags have their +// default values set correctly and their types flow through the system. +export const flags = new FlagSet({ + DEFAULT_VALUES: Flag.boolean(env.isDev()), + AUTOMATION_BRANCHING: Flag.boolean(env.isDev()), + SQS: Flag.boolean(env.isDev()), + [FeatureFlag.AI_CUSTOM_CONFIGS]: Flag.boolean(env.isDev()), + [FeatureFlag.ENRICHED_RELATIONSHIPS]: Flag.boolean(env.isDev()), +}) + +type UnwrapPromise = T extends Promise ? U : T +export type FeatureFlags = UnwrapPromise> diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 2b915e5689..f77a62fd4d 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,281 +1,2 @@ -import env from "../environment" -import * as context from "../context" -import { PostHog, PostHogOptions } from "posthog-node" -import { FeatureFlag, IdentityType, UserCtx } from "@budibase/types" -import tracer from "dd-trace" -import { Duration } from "../utils" - -let posthog: PostHog | undefined -export function init(opts?: PostHogOptions) { - if ( - env.POSTHOG_TOKEN && - env.POSTHOG_API_HOST && - !env.SELF_HOSTED && - env.POSTHOG_FEATURE_FLAGS_ENABLED - ) { - console.log("initializing posthog client...") - posthog = new PostHog(env.POSTHOG_TOKEN, { - host: env.POSTHOG_API_HOST, - personalApiKey: env.POSTHOG_PERSONAL_TOKEN, - featureFlagsPollingInterval: Duration.fromMinutes(3).toMs(), - ...opts, - }) - } else { - console.log("posthog disabled") - } -} - -export function shutdown() { - posthog?.shutdown() -} - -export abstract class Flag { - static boolean(defaultValue: boolean): Flag { - return new BooleanFlag(defaultValue) - } - - static string(defaultValue: string): Flag { - return new StringFlag(defaultValue) - } - - static number(defaultValue: number): Flag { - return new NumberFlag(defaultValue) - } - - protected constructor(public defaultValue: T) {} - - abstract parse(value: any): T -} - -type UnwrapFlag = F extends Flag ? U : never - -export type FlagValues = { - [K in keyof T]: UnwrapFlag -} - -type KeysOfType = { - [K in keyof T]: T[K] extends Flag ? K : never -}[keyof T] - -class BooleanFlag extends Flag { - parse(value: any) { - if (typeof value === "string") { - return ["true", "t", "1"].includes(value.toLowerCase()) - } - - if (typeof value === "boolean") { - return value - } - - throw new Error(`could not parse value "${value}" as boolean`) - } -} - -class StringFlag extends Flag { - parse(value: any) { - if (typeof value === "string") { - return value - } - throw new Error(`could not parse value "${value}" as string`) - } -} - -class NumberFlag extends Flag { - parse(value: any) { - if (typeof value === "number") { - return value - } - - if (typeof value === "string") { - const parsed = parseFloat(value) - if (!isNaN(parsed)) { - return parsed - } - } - - throw new Error(`could not parse value "${value}" as number`) - } -} - -export class FlagSet, T extends { [key: string]: V }> { - // This is used to safely cache flags sets in the current request context. - // Because multiple sets could theoretically exist, we don't want the cache of - // one to leak into another. - private readonly setId: string - - constructor(private readonly flagSchema: T) { - this.setId = crypto.randomUUID() - } - - defaults(): FlagValues { - return Object.keys(this.flagSchema).reduce((acc, key) => { - const typedKey = key as keyof T - acc[typedKey] = this.flagSchema[key].defaultValue - return acc - }, {} as FlagValues) - } - - isFlagName(name: string | number | symbol): name is keyof T { - return this.flagSchema[name as keyof T] !== undefined - } - - async get( - key: K, - ctx?: UserCtx - ): Promise[K]> { - const flags = await this.fetch(ctx) - return flags[key] - } - - async isEnabled>( - key: K, - ctx?: UserCtx - ): Promise { - const flags = await this.fetch(ctx) - return flags[key] - } - - async fetch(ctx?: UserCtx): Promise> { - return await tracer.trace("features.fetch", async span => { - const cachedFlags = context.getFeatureFlags>(this.setId) - if (cachedFlags) { - span?.addTags({ fromCache: true }) - return cachedFlags - } - - const tags: Record = {} - const flagValues = this.defaults() - const currentTenantId = context.getTenantId() - const specificallySetFalse = new Set() - - const split = (env.TENANT_FEATURE_FLAGS || "") - .split(",") - .map(x => x.split(":")) - for (const [tenantId, ...features] of split) { - if (!tenantId || (tenantId !== "*" && tenantId !== currentTenantId)) { - continue - } - - tags[`readFromEnvironmentVars`] = true - - for (let feature of features) { - let value = true - if (feature.startsWith("!")) { - feature = feature.slice(1) - value = false - specificallySetFalse.add(feature) - } - - // ignore unknown flags - if (!this.isFlagName(feature)) { - continue - } - - if (typeof flagValues[feature] !== "boolean") { - throw new Error(`Feature: ${feature} is not a boolean`) - } - - // @ts-expect-error - TS does not like you writing into a generic type, - // but we know that it's okay in this case because it's just an object. - flagValues[feature as keyof FlagValues] = value - tags[`flags.${feature}.source`] = "environment" - } - } - - const license = ctx?.user?.license - if (license) { - tags[`readFromLicense`] = true - - for (const feature of license.features) { - if (!this.isFlagName(feature)) { - continue - } - - if ( - flagValues[feature] === true || - specificallySetFalse.has(feature) - ) { - // If the flag is already set to through environment variables, we - // don't want to override it back to false here. - continue - } - - // @ts-expect-error - TS does not like you writing into a generic type, - // but we know that it's okay in this case because it's just an object. - flagValues[feature] = true - tags[`flags.${feature}.source`] = "license" - } - } - - const identity = context.getIdentity() - tags[`identity.type`] = identity?.type - tags[`identity.tenantId`] = identity?.tenantId - tags[`identity._id`] = identity?._id - - if (posthog && identity?.type === IdentityType.USER) { - tags[`readFromPostHog`] = true - - const personProperties: Record = {} - if (identity.tenantId) { - personProperties.tenantId = identity.tenantId - } - - const posthogFlags = await posthog.getAllFlagsAndPayloads( - identity._id, - { - personProperties, - } - ) - - for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { - if (!this.isFlagName(name)) { - // We don't want an unexpected PostHog flag to break the app, so we - // just log it and continue. - console.warn(`Unexpected posthog flag "${name}": ${value}`) - continue - } - - if (flagValues[name] === true || specificallySetFalse.has(name)) { - // If the flag is already set to through environment variables, we - // don't want to override it back to false here. - continue - } - - const payload = posthogFlags.featureFlagPayloads?.[name] - const flag = this.flagSchema[name] - try { - // @ts-expect-error - TS does not like you writing into a generic - // type, but we know that it's okay in this case because it's just - // an object. - flagValues[name] = flag.parse(payload || value) - tags[`flags.${name}.source`] = "posthog" - } catch (err) { - // We don't want an invalid PostHog flag to break the app, so we just - // log it and continue. - console.warn(`Error parsing posthog flag "${name}": ${value}`, err) - } - } - } - - context.setFeatureFlags(this.setId, flagValues) - for (const [key, value] of Object.entries(flagValues)) { - tags[`flags.${key}.value`] = value - } - span?.addTags(tags) - - return flagValues - }) - } -} - -// This is the primary source of truth for feature flags. If you want to add a -// new flag, add it here and use the `fetch` and `get` functions to access it. -// All of the machinery in this file is to make sure that flags have their -// default values set correctly and their types flow through the system. -export const flags = new FlagSet({ - DEFAULT_VALUES: Flag.boolean(env.isDev()), - AUTOMATION_BRANCHING: Flag.boolean(env.isDev()), - SQS: Flag.boolean(env.isDev()), - [FeatureFlag.AI_CUSTOM_CONFIGS]: Flag.boolean(env.isDev()), - [FeatureFlag.ENRICHED_RELATIONSHIPS]: Flag.boolean(env.isDev()), -}) +export * from "./features" +export * as testutils from "./tests/utils" diff --git a/packages/backend-core/src/features/tests/utils.ts b/packages/backend-core/src/features/tests/utils.ts new file mode 100644 index 0000000000..cc633c083d --- /dev/null +++ b/packages/backend-core/src/features/tests/utils.ts @@ -0,0 +1,64 @@ +import { FeatureFlags, parseEnvFlags } from ".." +import { setEnv } from "../../environment" + +function getCurrentFlags(): Record> { + const result: Record> = {} + for (const { tenantId, key, value } of parseEnvFlags( + process.env.TENANT_FEATURE_FLAGS || "" + )) { + const tenantFlags = result[tenantId] || {} + // Don't allow overwriting specifically false flags, to match the beheaviour + // of FlagSet. + if (tenantFlags[key] === false) { + continue + } + tenantFlags[key] = value + result[tenantId] = tenantFlags + } + return result +} + +function buildFlagString( + flags: Record> +): string { + const parts: string[] = [] + for (const [tenantId, tenantFlags] of Object.entries(flags)) { + for (const [key, value] of Object.entries(tenantFlags)) { + if (value === false) { + parts.push(`${tenantId}:!${key}`) + } else { + parts.push(`${tenantId}:${key}`) + } + } + } + return parts.join(",") +} + +export function setFeatureFlags( + tenantId: string, + flags: Partial +): () => void { + const current = getCurrentFlags() + for (const [key, value] of Object.entries(flags)) { + const tenantFlags = current[tenantId] || {} + tenantFlags[key] = value + current[tenantId] = tenantFlags + } + const flagString = buildFlagString(current) + return setEnv({ TENANT_FEATURE_FLAGS: flagString }) +} + +export function withFeatureFlags( + tenantId: string, + flags: Partial, + f: () => T +) { + const cleanup = setFeatureFlags(tenantId, flags) + const result = f() + if (result instanceof Promise) { + return result.finally(cleanup) + } else { + cleanup() + return result + } +} diff --git a/packages/server/src/api/routes/tests/application.spec.ts b/packages/server/src/api/routes/tests/application.spec.ts index 40ed828dce..6e41c8b4ec 100644 --- a/packages/server/src/api/routes/tests/application.spec.ts +++ b/packages/server/src/api/routes/tests/application.spec.ts @@ -19,6 +19,7 @@ import { utils, context, withEnv as withCoreEnv, + features, } from "@budibase/backend-core" import env from "../../../environment" import { type App } from "@budibase/types" @@ -358,9 +359,13 @@ describe("/applications", () => { .delete(`/api/global/roles/${prodAppId}`) .reply(200, {}) - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, async () => { - await config.api.application.delete(app.appId) - }) + await features.testutils.withFeatureFlags( + "*", + { SQS: true }, + async () => { + await config.api.application.delete(app.appId) + } + ) }) }) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 5222069460..3fc140e8e5 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -15,6 +15,7 @@ import { tenancy, withEnv as withCoreEnv, setEnv as setCoreEnv, + features, } from "@budibase/backend-core" import { quotas } from "@budibase/pro" import { @@ -98,12 +99,12 @@ describe.each([ let envCleanup: (() => void) | undefined beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, () => config.init()) - if (isLucene) { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }) - } else if (isSqs) { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }) - } + await features.testutils.withFeatureFlags("*", { SQS: true }, () => + config.init() + ) + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { const rawDatasource = await dsProvider @@ -2517,15 +2518,9 @@ describe.each([ let flagCleanup: (() => void) | undefined beforeAll(async () => { - const env = { - TENANT_FEATURE_FLAGS: `*:${FeatureFlag.ENRICHED_RELATIONSHIPS}`, - } - if (isSqs) { - env.TENANT_FEATURE_FLAGS = `${env.TENANT_FEATURE_FLAGS},*:SQS` - } else { - env.TENANT_FEATURE_FLAGS = `${env.TENANT_FEATURE_FLAGS},*:!SQS` - } - flagCleanup = setCoreEnv(env) + flagCleanup = features.testutils.setFeatureFlags("*", { + ENRICHED_RELATIONSHIPS: true, + }) const aux2Table = await config.api.table.save(saveTableRequest()) const aux2Data = await config.api.row.save(aux2Table._id!, {}) @@ -2752,9 +2747,10 @@ describe.each([ it.each(testScenarios)( "does not enrich relationships when not enabled (via %s)", async (__, retrieveDelegate) => { - await withCoreEnv( + await features.testutils.withFeatureFlags( + "*", { - TENANT_FEATURE_FLAGS: `*:!${FeatureFlag.ENRICHED_RELATIONSHIPS}`, + ENRICHED_RELATIONSHIPS: false, }, async () => { const otherRows = _.sampleSize(auxData, 5) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 110899e292..51965b5574 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -7,9 +7,9 @@ import { import { context, db as dbCore, + features, MAX_VALID_DATE, MIN_VALID_DATE, - setEnv as setCoreEnv, SQLITE_DESIGN_DOC_ID, utils, withEnv as withCoreEnv, @@ -94,16 +94,12 @@ describe.each([ } beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, () => config.init()) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + await features.testutils.withFeatureFlags("*", { SQS: true }, () => + config.init() + ) + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (config.app?.appId) { config.app = await config.api.application.update(config.app?.appId, { diff --git a/packages/server/src/api/routes/tests/templates.spec.ts b/packages/server/src/api/routes/tests/templates.spec.ts index 6f4d468a68..a4f9318720 100644 --- a/packages/server/src/api/routes/tests/templates.spec.ts +++ b/packages/server/src/api/routes/tests/templates.spec.ts @@ -2,7 +2,11 @@ import * as setup from "./utilities" import path from "path" import nock from "nock" import { generator } from "@budibase/backend-core/tests" -import { withEnv as withCoreEnv, env as coreEnv } from "@budibase/backend-core" +import { + withEnv as withCoreEnv, + env as coreEnv, + features, +} from "@budibase/backend-core" interface App { background: string @@ -85,41 +89,44 @@ describe("/templates", () => { it.each(["sqs", "lucene"])( `should be able to create an app from a template (%s)`, async source => { - const env: Partial = { - TENANT_FEATURE_FLAGS: source === "sqs" ? "*:SQS" : "", - } + await features.testutils.withFeatureFlags( + "*", + { SQS: source === "sqs" }, + async () => { + const name = generator.guid().replaceAll("-", "") + const url = `/${name}` - await withCoreEnv(env, async () => { - const name = generator.guid().replaceAll("-", "") - const url = `/${name}` - - const app = await config.api.application.create({ - name, - url, - useTemplate: "true", - templateName: "Agency Client Portal", - templateKey: "app/agency-client-portal", - }) - expect(app.name).toBe(name) - expect(app.url).toBe(url) - - await config.withApp(app, async () => { - const tables = await config.api.table.fetch() - expect(tables).toHaveLength(2) - - tables.sort((a, b) => a.name.localeCompare(b.name)) - const [agencyProjects, users] = tables - expect(agencyProjects.name).toBe("Agency Projects") - expect(users.name).toBe("Users") - - const { rows } = await config.api.row.search(agencyProjects._id!, { - tableId: agencyProjects._id!, - query: {}, + const app = await config.api.application.create({ + name, + url, + useTemplate: "true", + templateName: "Agency Client Portal", + templateKey: "app/agency-client-portal", }) + expect(app.name).toBe(name) + expect(app.url).toBe(url) - expect(rows).toHaveLength(3) - }) - }) + await config.withApp(app, async () => { + const tables = await config.api.table.fetch() + expect(tables).toHaveLength(2) + + tables.sort((a, b) => a.name.localeCompare(b.name)) + const [agencyProjects, users] = tables + expect(agencyProjects.name).toBe("Agency Projects") + expect(users.name).toBe("Users") + + const { rows } = await config.api.row.search( + agencyProjects._id!, + { + tableId: agencyProjects._id!, + query: {}, + } + ) + + expect(rows).toHaveLength(3) + }) + } + ) } ) }) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 04e51fc212..ca4805864b 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -37,6 +37,7 @@ import { withEnv as withCoreEnv, setEnv as setCoreEnv, env, + features, } from "@budibase/backend-core" describe.each([ @@ -102,18 +103,13 @@ describe.each([ } beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: isSqs ? "*:SQS" : "" }, () => + await features.testutils.withFeatureFlags("*", { SQS: isSqs }, () => config.init() ) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { datasource = await config.createDatasource({ @@ -2490,12 +2486,8 @@ describe.each([ describe("foreign relationship columns", () => { let envCleanup: () => void beforeAll(() => { - const flags = [`*:${FeatureFlag.ENRICHED_RELATIONSHIPS}`] - if (env.TENANT_FEATURE_FLAGS) { - flags.push(...env.TENANT_FEATURE_FLAGS.split(",")) - } - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: flags.join(","), + envCleanup = features.testutils.setFeatureFlags("*", { + ENRICHED_RELATIONSHIPS: true, }) }) diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index b4f708edb5..759665f1c2 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -2,6 +2,7 @@ import * as setup from "../../../api/routes/tests/utilities" import { basicTable } from "../../../tests/utilities/structures" import { db as dbCore, + features, SQLITE_DESIGN_DOC_ID, withEnv as withCoreEnv, } from "@budibase/backend-core" @@ -71,11 +72,11 @@ function oldLinkDocument(): Omit { } async function sqsDisabled(cb: () => Promise) { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }, cb) + await features.testutils.withFeatureFlags("*", { SQS: false }, cb) } async function sqsEnabled(cb: () => Promise) { - await withCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }, cb) + await features.testutils.withFeatureFlags("*", { SQS: true }, cb) } describe("SQS migration", () => { diff --git a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts index e7fd095865..fce1e14094 100644 --- a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts +++ b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts @@ -13,6 +13,7 @@ import { generator } from "@budibase/backend-core/tests" import { withEnv as withCoreEnv, setEnv as setCoreEnv, + features, } from "@budibase/backend-core" import { DatabaseName, @@ -41,19 +42,13 @@ describe.each([ let table: Table beforeAll(async () => { - await withCoreEnv({ TENANT_FEATURE_FLAGS: isSqs ? "*:SQS" : "" }, () => + await features.testutils.withFeatureFlags("*", { SQS: isSqs }, () => config.init() ) - if (isLucene) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:!SQS", - }) - } else if (isSqs) { - envCleanup = setCoreEnv({ - TENANT_FEATURE_FLAGS: "*:SQS", - }) - } + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: isSqs, + }) if (dsProvider) { datasource = await config.createDatasource({ diff --git a/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts b/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts index b6c2cdb11c..8cbe585d90 100644 --- a/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts +++ b/packages/server/src/utilities/rowProcessor/tests/outputProcessing.spec.ts @@ -8,7 +8,7 @@ import { } from "@budibase/types" import { outputProcessing } from ".." import { generator, structures } from "@budibase/backend-core/tests" -import { setEnv as setCoreEnv } from "@budibase/backend-core" +import { features } from "@budibase/backend-core" import * as bbReferenceProcessor from "../bbReferenceProcessor" import TestConfiguration from "../../../tests/utilities/TestConfiguration" @@ -21,7 +21,7 @@ jest.mock("../bbReferenceProcessor", (): typeof bbReferenceProcessor => ({ describe("rowProcessor - outputProcessing", () => { const config = new TestConfiguration() - let cleanupEnv: () => void = () => {} + let cleanupFlags: () => void = () => {} beforeAll(async () => { await config.init() @@ -33,11 +33,11 @@ describe("rowProcessor - outputProcessing", () => { beforeEach(() => { jest.resetAllMocks() - cleanupEnv = setCoreEnv({ TENANT_FEATURE_FLAGS: "*SQS" }) + cleanupFlags = features.testutils.setFeatureFlags("*", { SQS: true }) }) afterEach(() => { - cleanupEnv() + cleanupFlags() }) const processOutputBBReferenceMock = diff --git a/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts b/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts index b09c27762d..b540836583 100644 --- a/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts +++ b/packages/worker/src/api/routes/global/tests/auditLogs.spec.ts @@ -1,5 +1,5 @@ import { mocks, structures } from "@budibase/backend-core/tests" -import { context, events, setEnv as setCoreEnv } from "@budibase/backend-core" +import { context, events, features } from "@budibase/backend-core" import { Event, IdentityType } from "@budibase/types" import { TestConfiguration } from "../../../../tests" @@ -17,11 +17,9 @@ describe.each(["lucene", "sql"])("/api/global/auditlogs (%s)", method => { let envCleanup: (() => void) | undefined beforeAll(async () => { - if (method === "lucene") { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:!SQS" }) - } else if (method === "sql") { - envCleanup = setCoreEnv({ TENANT_FEATURE_FLAGS: "*:SQS" }) - } + envCleanup = features.testutils.setFeatureFlags("*", { + SQS: method === "sql", + }) await config.beforeAll() }) From d0c57c82ad978ac0f1d5d15aa41d8442e6287939 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:15:53 +0100 Subject: [PATCH 06/57] Fix lint. --- .../server/src/api/routes/tests/application.spec.ts | 8 +------- packages/server/src/api/routes/tests/row.spec.ts | 3 --- packages/server/src/api/routes/tests/templates.spec.ts | 6 +----- packages/server/src/api/routes/tests/viewV2.spec.ts | 10 +--------- .../tests/20240604153647_initial_sqs.spec.ts | 1 - .../src/sdk/app/rows/search/tests/search.spec.ts | 6 +----- 6 files changed, 4 insertions(+), 30 deletions(-) diff --git a/packages/server/src/api/routes/tests/application.spec.ts b/packages/server/src/api/routes/tests/application.spec.ts index 6e41c8b4ec..fe8250bde5 100644 --- a/packages/server/src/api/routes/tests/application.spec.ts +++ b/packages/server/src/api/routes/tests/application.spec.ts @@ -14,13 +14,7 @@ jest.mock("../../../utilities/redis", () => ({ import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" import { AppStatus } from "../../../db/utils" -import { - events, - utils, - context, - withEnv as withCoreEnv, - features, -} from "@budibase/backend-core" +import { events, utils, context, features } from "@budibase/backend-core" import env from "../../../environment" import { type App } from "@budibase/types" import tk from "timekeeper" diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 3fc140e8e5..207420bf9f 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -13,8 +13,6 @@ import { context, InternalTable, tenancy, - withEnv as withCoreEnv, - setEnv as setCoreEnv, features, } from "@budibase/backend-core" import { quotas } from "@budibase/pro" @@ -41,7 +39,6 @@ import { TableSchema, JsonFieldSubType, RowExportFormat, - FeatureFlag, RelationSchemaField, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" diff --git a/packages/server/src/api/routes/tests/templates.spec.ts b/packages/server/src/api/routes/tests/templates.spec.ts index a4f9318720..d5483c54b4 100644 --- a/packages/server/src/api/routes/tests/templates.spec.ts +++ b/packages/server/src/api/routes/tests/templates.spec.ts @@ -2,11 +2,7 @@ import * as setup from "./utilities" import path from "path" import nock from "nock" import { generator } from "@budibase/backend-core/tests" -import { - withEnv as withCoreEnv, - env as coreEnv, - features, -} from "@budibase/backend-core" +import { features } from "@budibase/backend-core" interface App { background: string diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index fe926170b9..dce5a660b5 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -22,7 +22,6 @@ import { RelationshipType, TableSchema, RenameColumn, - FeatureFlag, BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, @@ -31,14 +30,7 @@ import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" import merge from "lodash/merge" import { quotas } from "@budibase/pro" -import { - db, - roles, - withEnv as withCoreEnv, - setEnv as setCoreEnv, - env, - features, -} from "@budibase/backend-core" +import { db, roles, features } from "@budibase/backend-core" describe.each([ ["lucene", undefined], diff --git a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts index 759665f1c2..fe44b7b901 100644 --- a/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts +++ b/packages/server/src/appMigrations/migrations/tests/20240604153647_initial_sqs.spec.ts @@ -4,7 +4,6 @@ import { db as dbCore, features, SQLITE_DESIGN_DOC_ID, - withEnv as withCoreEnv, } from "@budibase/backend-core" import { LinkDocument, diff --git a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts index fce1e14094..4d8a6b6d69 100644 --- a/packages/server/src/sdk/app/rows/search/tests/search.spec.ts +++ b/packages/server/src/sdk/app/rows/search/tests/search.spec.ts @@ -10,11 +10,7 @@ import { import TestConfiguration from "../../../../../tests/utilities/TestConfiguration" import { search } from "../../../../../sdk/app/rows/search" import { generator } from "@budibase/backend-core/tests" -import { - withEnv as withCoreEnv, - setEnv as setCoreEnv, - features, -} from "@budibase/backend-core" +import { features } from "@budibase/backend-core" import { DatabaseName, getDatasource, From 9b3ab049a4d46200ed357dbfc7d4282c2686334e Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:50:16 +0100 Subject: [PATCH 07/57] wip --- .../src/api/routes/tests/viewV2.spec.ts | 30 +++++++++++++++++++ packages/server/src/sdk/app/views/index.ts | 3 +- packages/types/src/documents/app/row.ts | 20 +++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 5dabae6a03..5b25b48656 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -26,6 +26,8 @@ import { BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, + canGroupBy, + ViewV2Type, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -721,6 +723,34 @@ describe.each([ }, }) }) + + it.only("cannot use complex types as group-by fields", async () => { + const complexTypes = Object.values(FieldType).filter( + type => !canGroupBy(type) + ) + for (const type of complexTypes) { + const field = { name: "field", type } as FieldSchema + const table = await config.api.table.save( + saveTableRequest({ schema: { field } }) + ) + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + field: { visible: true }, + }, + }, + { + status: 400, + body: { + message: "", + }, + } + ) + } + }) }) describe("update", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 5445851d88..76b2f6e347 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -1,6 +1,7 @@ import { CalculationType, FieldType, + isNumeric, PermissionLevel, RelationSchemaField, RenameColumn, @@ -103,7 +104,7 @@ async function guardCalculationViewSchema( ) } - if (!isCount && !helpers.schema.isNumeric(targetSchema)) { + if (!isCount && !isNumeric(targetSchema.type)) { throw new HTTPError( `Calculation field "${name}" references field "${schema.field}" which is not a numeric field`, 400 diff --git a/packages/types/src/documents/app/row.ts b/packages/types/src/documents/app/row.ts index 42440b2988..673d481e3c 100644 --- a/packages/types/src/documents/app/row.ts +++ b/packages/types/src/documents/app/row.ts @@ -127,6 +127,26 @@ export const JsonTypes = [ FieldType.ARRAY, ] +export const NumericTypes = [FieldType.NUMBER, FieldType.BIGINT] + +export function isNumeric(type: FieldType) { + return NumericTypes.includes(type) +} + +export const GroupByTypes = [ + FieldType.STRING, + FieldType.LONGFORM, + FieldType.OPTIONS, + FieldType.NUMBER, + FieldType.BOOLEAN, + FieldType.DATETIME, + FieldType.BIGINT, +] + +export function canGroupBy(type: FieldType) { + return GroupByTypes.includes(type) +} + export interface RowAttachment { size: number name: string From 5c3adbed270a91fade0798024c18e0f9bbc19dd8 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 15:34:34 +0100 Subject: [PATCH 08/57] Validate you can't group by complex fields. --- .../src/api/routes/tests/viewV2.spec.ts | 54 +++++++++---------- packages/server/src/sdk/app/views/index.ts | 8 +++ 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index fa8b005ae9..c7c040717a 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -25,8 +25,8 @@ import { BBReferenceFieldSubType, NumericCalculationFieldMetadata, ViewV2Schema, - canGroupBy, ViewV2Type, + JsonTypes, } from "@budibase/types" import { generator, mocks } from "@budibase/backend-core/tests" import { DatabaseName, getDatasource } from "../../../integrations/tests/utils" @@ -738,33 +738,33 @@ describe.each([ }) }) - it.only("cannot use complex types as group-by fields", async () => { - const complexTypes = Object.values(FieldType).filter( - type => !canGroupBy(type) - ) - for (const type of complexTypes) { - const field = { name: "field", type } as FieldSchema - const table = await config.api.table.save( - saveTableRequest({ schema: { field } }) - ) - await config.api.viewV2.create( - { - tableId: table._id!, - name: generator.guid(), - type: ViewV2Type.CALCULATION, - schema: { - field: { visible: true }, + // We don't allow the creation of tables with most JsonTypes when using + // external datasources. + isInternal && + it("cannot use complex types as group-by fields", async () => { + for (const type of JsonTypes) { + const field = { name: "field", type } as FieldSchema + const table = await config.api.table.save( + saveTableRequest({ schema: { field } }) + ) + await config.api.viewV2.create( + { + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + field: { visible: true }, + }, }, - }, - { - status: 400, - body: { - message: "", - }, - } - ) - } - }) + { + status: 400, + body: { + message: `Grouping by fields of type "${type}" is not supported`, + }, + } + ) + } + }) }) describe("update", () => { diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 1d0e633332..44f6beedb1 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -1,5 +1,6 @@ import { CalculationType, + canGroupBy, FieldType, isNumeric, PermissionLevel, @@ -121,6 +122,13 @@ async function guardCalculationViewSchema( 400 ) } + + if (!canGroupBy(targetSchema.type)) { + throw new HTTPError( + `Grouping by fields of type "${targetSchema.type}" is not supported`, + 400 + ) + } } } From 5dc85230d9ee8f373bc11cc484592257c8f95206 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 17:04:45 +0100 Subject: [PATCH 09/57] Add a test to make sure 'shadowing' a field works as expected in view calculations. --- .../src/api/routes/tests/viewV2.spec.ts | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index c7c040717a..58176cb597 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -738,6 +738,41 @@ describe.each([ }) }) + !isLucene && + it("does not get confused when a calculation field shadows a basic one", async () => { + const table = await config.api.table.save( + saveTableRequest({ + schema: { + age: { + name: "age", + type: FieldType.NUMBER, + }, + }, + }) + ) + + await config.api.row.bulkImport(table._id!, { + rows: [{ age: 1 }, { age: 2 }, { age: 3 }], + }) + + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + age: { + visible: true, + calculationType: CalculationType.SUM, + field: "age", + }, + }, + }) + + const { rows } = await config.api.row.search(view.id) + expect(rows).toHaveLength(1) + expect(rows[0].age).toEqual(6) + }) + // We don't allow the creation of tables with most JsonTypes when using // external datasources. isInternal && From 0182d4a09f8b4657dae2731779fc595fdeea5567 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 17:55:42 +0100 Subject: [PATCH 10/57] Make sure calculation views cannot be used to write or modify rows. --- .../src/api/controllers/row/external.ts | 6 ++ .../src/api/controllers/row/internal.ts | 7 +++ .../src/api/routes/tests/viewV2.spec.ts | 58 +++++++++++++++++++ packages/server/src/sdk/app/rows/external.ts | 5 ++ packages/server/src/sdk/app/rows/internal.ts | 7 ++- 5 files changed, 82 insertions(+), 1 deletion(-) diff --git a/packages/server/src/api/controllers/row/external.ts b/packages/server/src/api/controllers/row/external.ts index 18a9be5087..4fcd612ec3 100644 --- a/packages/server/src/api/controllers/row/external.ts +++ b/packages/server/src/api/controllers/row/external.ts @@ -27,6 +27,7 @@ import { } from "../../../utilities/rowProcessor" import { cloneDeep } from "lodash" import { generateIdForRow } from "./utils" +import { helpers } from "@budibase/shared-core" export async function handleRequest( operation: T, @@ -42,6 +43,11 @@ export async function handleRequest( export async function patch(ctx: UserCtx) { const source = await utils.getSource(ctx) + + if (sdk.views.isView(source) && helpers.views.isCalculationView(source)) { + ctx.throw(400, "Cannot update rows through a calculation view") + } + const table = await utils.getTableFromSource(source) const { _id, ...rowData } = ctx.request.body diff --git a/packages/server/src/api/controllers/row/internal.ts b/packages/server/src/api/controllers/row/internal.ts index bb9de6ce52..53b24d517c 100644 --- a/packages/server/src/api/controllers/row/internal.ts +++ b/packages/server/src/api/controllers/row/internal.ts @@ -22,13 +22,20 @@ import sdk from "../../../sdk" import { getLinkedTableIDs } from "../../../db/linkedRows/linkUtils" import { flatten } from "lodash" import { findRow } from "../../../sdk/app/rows/internal" +import { helpers } from "@budibase/shared-core" export async function patch(ctx: UserCtx) { const { tableId } = utils.getSourceId(ctx) const source = await utils.getSource(ctx) + + if (sdk.views.isView(source) && helpers.views.isCalculationView(source)) { + ctx.throw(400, "Cannot update rows through a calculation view") + } + const table = sdk.views.isView(source) ? await sdk.views.getTable(source.id) : source + const inputs = ctx.request.body const isUserTable = tableId === InternalTables.USER_METADATA let oldRow diff --git a/packages/server/src/api/routes/tests/viewV2.spec.ts b/packages/server/src/api/routes/tests/viewV2.spec.ts index 58176cb597..195481ae10 100644 --- a/packages/server/src/api/routes/tests/viewV2.spec.ts +++ b/packages/server/src/api/routes/tests/viewV2.spec.ts @@ -1978,6 +1978,30 @@ describe.each([ expect(newRow.one).toBeUndefined() expect(newRow.two).toEqual("bar") }) + + it("should not be possible to create a row in a calculation view", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + id: { visible: true }, + one: { visible: true }, + }, + }) + + await config.api.row.save( + view.id, + { one: "foo" }, + { + status: 400, + body: { + message: "Cannot insert rows through a calculation view", + status: 400, + }, + } + ) + }) }) describe("patch", () => { @@ -2042,6 +2066,40 @@ describe.each([ expect(row.one).toEqual("foo") expect(row.two).toEqual("newBar") }) + + it("should not be possible to modify a row in a calculation view", async () => { + const view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), + type: ViewV2Type.CALCULATION, + schema: { + id: { visible: true }, + one: { visible: true }, + }, + }) + + const newRow = await config.api.row.save(table._id!, { + one: "foo", + two: "bar", + }) + + await config.api.row.patch( + view.id, + { + tableId: table._id!, + _id: newRow._id!, + _rev: newRow._rev!, + one: "newFoo", + two: "newBar", + }, + { + status: 400, + body: { + message: "Cannot update rows through a calculation view", + }, + } + ) + }) }) describe("destroy", () => { diff --git a/packages/server/src/sdk/app/rows/external.ts b/packages/server/src/sdk/app/rows/external.ts index 060ef3738a..f7cc38bf0a 100644 --- a/packages/server/src/sdk/app/rows/external.ts +++ b/packages/server/src/sdk/app/rows/external.ts @@ -15,6 +15,7 @@ import { } from "../../../utilities/rowProcessor" import cloneDeep from "lodash/fp/cloneDeep" import { tryExtractingTableAndViewId } from "./utils" +import { helpers } from "@budibase/shared-core" export async function getRow( sourceId: string | Table | ViewV2, @@ -54,6 +55,10 @@ export async function save( source = await sdk.tables.getTable(tableId) } + if (sdk.views.isView(source) && helpers.views.isCalculationView(source)) { + throw new HTTPError("Cannot insert rows through a calculation view", 400) + } + const row = await inputProcessing(userId, cloneDeep(source), inputs) const validateResult = await sdk.rows.utils.validate({ diff --git a/packages/server/src/sdk/app/rows/internal.ts b/packages/server/src/sdk/app/rows/internal.ts index 9306609132..bd03b77f94 100644 --- a/packages/server/src/sdk/app/rows/internal.ts +++ b/packages/server/src/sdk/app/rows/internal.ts @@ -1,4 +1,4 @@ -import { context, db } from "@budibase/backend-core" +import { context, db, HTTPError } from "@budibase/backend-core" import { Row, Table, ViewV2 } from "@budibase/types" import sdk from "../../../sdk" import { finaliseRow } from "../../../api/controllers/row/staticFormula" @@ -10,6 +10,7 @@ import * as linkRows from "../../../db/linkedRows" import { InternalTables } from "../../../db/utils" import { getFullUser } from "../../../utilities/users" import { getSource, tryExtractingTableAndViewId } from "./utils" +import { helpers } from "@budibase/shared-core" export async function save( tableOrViewId: string, @@ -29,6 +30,10 @@ export async function save( table = source } + if (sdk.views.isView(source) && helpers.views.isCalculationView(source)) { + throw new HTTPError("Cannot insert rows through a calculation view", 400) + } + if (!inputs._rev && !inputs._id) { inputs._id = db.generateRowID(inputs.tableId) } From 0c532721877395c9f81be8bf26319c8c059c3597 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Wed, 9 Oct 2024 08:22:54 +0100 Subject: [PATCH 11/57] Allow empty queries when searching tables --- packages/frontend-core/src/api/tables.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/frontend-core/src/api/tables.js b/packages/frontend-core/src/api/tables.js index 34d2371e1a..823e1d1675 100644 --- a/packages/frontend-core/src/api/tables.js +++ b/packages/frontend-core/src/api/tables.js @@ -40,7 +40,7 @@ export const buildTableEndpoints = API => ({ sortType, paginate, }) => { - if (!tableId || !query) { + if (!tableId) { return { rows: [], } From bf62153b8b28b4688e4ee777c312561d881a9cfb Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:16:32 +0200 Subject: [PATCH 12/57] Add test with conditions --- .../src/api/routes/global/tests/users.spec.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/worker/src/api/routes/global/tests/users.spec.ts b/packages/worker/src/api/routes/global/tests/users.spec.ts index a654c42359..c8e71f7eb4 100644 --- a/packages/worker/src/api/routes/global/tests/users.spec.ts +++ b/packages/worker/src/api/routes/global/tests/users.spec.ts @@ -741,6 +741,25 @@ describe("/api/global/users", () => { it("should throw an error if public query performed", async () => { await config.api.users.searchUsers({}, { status: 403, noHeaders: true }) }) + + it("should be able to search using logical conditions", async () => { + const user = await config.createUser() + const response = await config.api.users.searchUsers({ + query: { + $and: { + conditions: [ + { + $and: { + conditions: [{ string: { email: user.email } }], + }, + }, + ], + }, + }, + }) + expect(response.body.data.length).toBe(1) + expect(response.body.data[0].email).toBe(user.email) + }) }) describe("DELETE /api/global/users/:userId", () => { From 0e24df2ddf125c1c9d11d20273712007163a2797 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:32:12 +0200 Subject: [PATCH 13/57] Improve types --- packages/shared-core/src/filters.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/shared-core/src/filters.ts b/packages/shared-core/src/filters.ts index b10375acb0..40c70a8a23 100644 --- a/packages/shared-core/src/filters.ts +++ b/packages/shared-core/src/filters.ts @@ -639,19 +639,19 @@ export function fixupFilterArrays(filters: SearchFilters) { return filters } -export function search( - docs: Record[], - query: RowSearchParams -): SearchResponse> { +export function search>( + docs: T[], + query: Omit +): SearchResponse { let result = runQuery(docs, query.query) if (query.sort) { result = sort(result, query.sort, query.sortOrder || SortOrder.ASCENDING) } - let totalRows = result.length + const totalRows = result.length if (query.limit) { result = limit(result, query.limit.toString()) } - const response: SearchResponse> = { rows: result } + const response: SearchResponse = { rows: result } if (query.countRows) { response.totalRows = totalRows } From 8b0c84b2ea67aa65a19f85b539d3e518bacd1aaf Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:32:39 +0200 Subject: [PATCH 14/57] In memory filters --- packages/backend-core/src/users/users.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/users/users.ts b/packages/backend-core/src/users/users.ts index d8546afa8b..96fb79c0d0 100644 --- a/packages/backend-core/src/users/users.ts +++ b/packages/backend-core/src/users/users.ts @@ -24,6 +24,7 @@ import * as context from "../context" import { getGlobalDB } from "../context" import { isCreator } from "./utils" import { UserDB } from "./db" +import { dataFilters } from "@budibase/shared-core" type GetOpts = { cleanup?: boolean } @@ -263,9 +264,12 @@ export async function paginatedUsers({ cleanup: true, }) } else { - // no search, query allDocs - const response = await db.allDocs(getGlobalUserParams(null, opts)) - userList = response.rows.map((row: any) => row.doc) + const response = await db.allDocs(getGlobalUserParams(null, opts)) + userList = response.rows.map(row => row.doc!) + + if (query) { + userList = dataFilters.search(userList, { query }).rows + } } return pagination(userList, pageSize, { paginate: true, From 3cf96c589cd102958529d88c2b806f238aa3cd4d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:33:15 +0200 Subject: [PATCH 15/57] Implement logical conditions --- packages/shared-core/src/utils.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/shared-core/src/utils.ts b/packages/shared-core/src/utils.ts index 14b3c84425..4847e911e9 100644 --- a/packages/shared-core/src/utils.ts +++ b/packages/shared-core/src/utils.ts @@ -5,6 +5,7 @@ import { SearchFilters, BasicOperator, ArrayOperator, + isLogicalSearchOperator, } from "@budibase/types" import * as Constants from "./constants" import { removeKeyNumbering } from "./filters" @@ -97,10 +98,20 @@ export function isSupportedUserSearch(query: SearchFilters) { { op: BasicOperator.EQUAL, key: "_id" }, { op: ArrayOperator.ONE_OF, key: "_id" }, ] - for (let [key, operation] of Object.entries(query)) { + for (const [key, operation] of Object.entries(query)) { if (typeof operation !== "object") { return false } + + if (isLogicalSearchOperator(key)) { + for (const condition of query[key]!.conditions) { + if (!isSupportedUserSearch(condition)) { + return false + } + } + return true + } + const fields = Object.keys(operation || {}) // this filter doesn't contain options - ignore if (fields.length === 0) { From d811b9527f82ce55f1bae7c5c0a4a3bbfdeee84b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:44:04 +0200 Subject: [PATCH 16/57] Fix limit issues --- packages/backend-core/src/users/users.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/backend-core/src/users/users.ts b/packages/backend-core/src/users/users.ts index 96fb79c0d0..f4838597b6 100644 --- a/packages/backend-core/src/users/users.ts +++ b/packages/backend-core/src/users/users.ts @@ -263,13 +263,17 @@ export async function paginatedUsers({ userList = await bulkGetGlobalUsersById(query?.oneOf?._id, { cleanup: true, }) + } else if (query) { + // TODO: this should use SQS search, but the logic is built in the 'server' package. Using the in-memory filtering to get this working meanwhile + const response = await db.allDocs( + getGlobalUserParams(null, { ...opts, limit: undefined }) + ) + userList = response.rows.map(row => row.doc!) + userList = dataFilters.search(userList, { query, limit: opts.limit }).rows } else { + // no search, query allDocs const response = await db.allDocs(getGlobalUserParams(null, opts)) userList = response.rows.map(row => row.doc!) - - if (query) { - userList = dataFilters.search(userList, { query }).rows - } } return pagination(userList, pageSize, { paginate: true, From c2a5f673aeb4df00ce3f31cf14cb7594fc1bc926 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 10:51:51 +0200 Subject: [PATCH 17/57] Lint --- packages/server/src/api/routes/tests/search.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/server/src/api/routes/tests/search.spec.ts b/packages/server/src/api/routes/tests/search.spec.ts index 51965b5574..1ccc9bfdc9 100644 --- a/packages/server/src/api/routes/tests/search.spec.ts +++ b/packages/server/src/api/routes/tests/search.spec.ts @@ -187,7 +187,6 @@ describe.each([ if (isInMemory) { return dataFilters.search(_.cloneDeep(rows), { ...this.query, - tableId: tableOrViewId, }) } else { return config.api.row.search(tableOrViewId, this.query) From 6916269cbb87850a46d4020719f15d5c5e320e31 Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Wed, 9 Oct 2024 09:59:05 +0100 Subject: [PATCH 18/57] Fix duplicate API call spam due to incorrect comparison of view filters --- .../src/components/grid/stores/datasources/viewV2.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/frontend-core/src/components/grid/stores/datasources/viewV2.js b/packages/frontend-core/src/components/grid/stores/datasources/viewV2.js index 8e4321be96..84d5eb153e 100644 --- a/packages/frontend-core/src/components/grid/stores/datasources/viewV2.js +++ b/packages/frontend-core/src/components/grid/stores/datasources/viewV2.js @@ -191,7 +191,7 @@ export const initialise = context => { if ($view?.id !== $datasource.id) { return } - if (JSON.stringify($filter) !== JSON.stringify($view.query)) { + if (JSON.stringify($filter) !== JSON.stringify($view.queryUI)) { await datasource.actions.saveDefinition({ ...$view, query: $filter, From abd7b3b84e5e1fda05e4987e34cc66072d33e347 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Wed, 9 Oct 2024 09:12:59 +0000 Subject: [PATCH 19/57] Bump version to 2.32.13 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index 55721b6b2d..faed02052e 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "2.32.12", + "version": "2.32.13", "npmClient": "yarn", "packages": [ "packages/*", From 9d06c705ac47af317c3acb41d4b36a0236b9ee82 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 12:47:43 +0200 Subject: [PATCH 20/57] Remove ContextUser usages in favor of just ids --- .../src/api/controllers/table/internal.ts | 4 ++-- .../api/controllers/table/tests/utils.spec.ts | 2 +- .../server/src/api/controllers/table/utils.ts | 20 +++++++++---------- .../src/sdk/app/tables/internal/index.ts | 5 ++--- 4 files changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/server/src/api/controllers/table/internal.ts b/packages/server/src/api/controllers/table/internal.ts index 4286d51d3e..256c5b1f0a 100644 --- a/packages/server/src/api/controllers/table/internal.ts +++ b/packages/server/src/api/controllers/table/internal.ts @@ -33,7 +33,7 @@ export async function save( try { const { table } = await sdk.tables.internal.save(tableToSave, { - user: ctx.user, + userId: ctx.user._id, rowsToImport: rows, tableId: ctx.request.body._id, renaming, @@ -72,7 +72,7 @@ export async function bulkImport( await handleDataImport(table, { importRows: rows, identifierFields, - user: ctx.user, + userId: ctx.user._id, }) return table } diff --git a/packages/server/src/api/controllers/table/tests/utils.spec.ts b/packages/server/src/api/controllers/table/tests/utils.spec.ts index dad0146696..68b00d3268 100644 --- a/packages/server/src/api/controllers/table/tests/utils.spec.ts +++ b/packages/server/src/api/controllers/table/tests/utils.spec.ts @@ -41,7 +41,7 @@ describe("utils", () => { const data = [{ name: "Alice" }, { name: "Bob" }, { name: "Claire" }] - const result = await importToRows(data, table, config.user) + const result = await importToRows(data, table, config.user?._id) expect(result).toEqual([ expect.objectContaining({ autoId: 1, diff --git a/packages/server/src/api/controllers/table/utils.ts b/packages/server/src/api/controllers/table/utils.ts index d568e5f33e..106d0e23a6 100644 --- a/packages/server/src/api/controllers/table/utils.ts +++ b/packages/server/src/api/controllers/table/utils.ts @@ -18,7 +18,6 @@ import { quotas } from "@budibase/pro" import { events, context, features } from "@budibase/backend-core" import { AutoFieldSubType, - ContextUser, Datasource, Row, SourceName, @@ -122,7 +121,7 @@ export function makeSureTableUpToDate(table: Table, tableToSave: Table) { export async function importToRows( data: Row[], table: Table, - user?: ContextUser, + userId?: string, opts?: { keepCouchId: boolean } ) { const originalTable = table @@ -136,7 +135,7 @@ export async function importToRows( // We use a reference to table here and update it after input processing, // so that we can auto increment auto IDs in imported data properly - const processed = await inputProcessing(user?._id, table, row, { + const processed = await inputProcessing(userId, table, row, { noAutoRelationships: true, }) row = processed @@ -167,11 +166,10 @@ export async function importToRows( export async function handleDataImport( table: Table, - opts?: { identifierFields?: string[]; user?: ContextUser; importRows?: Row[] } + opts?: { identifierFields?: string[]; userId?: string; importRows?: Row[] } ) { const schema = table.schema const identifierFields = opts?.identifierFields || [] - const user = opts?.user const importRows = opts?.importRows if (!importRows || !isRows(importRows) || !isSchema(schema)) { @@ -181,7 +179,7 @@ export async function handleDataImport( const db = context.getAppDB() const data = parse(importRows, table) - const finalData = await importToRows(data, table, user, { + const finalData = await importToRows(data, table, opts?.userId, { keepCouchId: identifierFields.includes("_id"), }) @@ -282,22 +280,22 @@ export function checkStaticTables(table: Table) { class TableSaveFunctions { db: Database - user?: ContextUser + userId?: string oldTable?: Table importRows?: Row[] rows: Row[] constructor({ - user, + userId, oldTable, importRows, }: { - user?: ContextUser + userId?: string oldTable?: Table importRows?: Row[] }) { this.db = context.getAppDB() - this.user = user + this.userId = userId this.oldTable = oldTable this.importRows = importRows // any rows that need updated @@ -329,7 +327,7 @@ class TableSaveFunctions { table = await handleSearchIndexes(table) table = await handleDataImport(table, { importRows: this.importRows, - user: this.user, + userId: this.userId, }) if (await features.flags.isEnabled("SQS")) { await sdk.tables.sqs.addTable(table) diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index c0beed0db8..7c72834ee0 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -5,7 +5,6 @@ import { ViewStatisticsSchema, ViewV2, Row, - ContextUser, } from "@budibase/types" import { hasTypeChanged, @@ -27,7 +26,7 @@ import { quotas } from "@budibase/pro" export async function save( table: Table, opts?: { - user?: ContextUser + userId?: string tableId?: string rowsToImport?: Row[] renaming?: RenameColumn @@ -63,7 +62,7 @@ export async function save( // saving a table is a complex operation, involving many different steps, this // has been broken out into a utility to make it more obvious/easier to manipulate const tableSaveFunctions = new TableSaveFunctions({ - user: opts?.user, + userId: opts?.userId, oldTable, importRows: opts?.rowsToImport, }) From 15d124bfafbc8ad5de5d31f1bc73721d3d8fe0e8 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 12:58:10 +0200 Subject: [PATCH 21/57] Move internal creation to sdk --- .../server/src/api/controllers/table/index.ts | 10 +++-- packages/server/src/sdk/app/tables/create.ts | 18 +++++++++ packages/server/src/sdk/app/tables/index.ts | 2 + .../src/sdk/app/tables/internal/index.ts | 39 ++++++++++++++++++- 4 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 packages/server/src/sdk/app/tables/create.ts diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index 7bec1581b4..a1969f44fc 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -106,14 +106,18 @@ export async function save(ctx: UserCtx) { const isImport = table.rows const renaming = ctx.request.body._rename + const isCreate = !table._id + checkDefaultFields(table) - const api = pickApi({ table }) - let savedTable = await api.save(ctx, renaming) - if (!table._id) { + let savedTable: Table + if (isCreate) { + savedTable = await sdk.tables.create(table) savedTable = await sdk.tables.enrichViewSchemas(savedTable) await events.table.created(savedTable) } else { + const api = pickApi({ table }) + savedTable = await api.save(ctx, renaming) await events.table.updated(savedTable) } if (renaming) { diff --git a/packages/server/src/sdk/app/tables/create.ts b/packages/server/src/sdk/app/tables/create.ts new file mode 100644 index 0000000000..585623d0a4 --- /dev/null +++ b/packages/server/src/sdk/app/tables/create.ts @@ -0,0 +1,18 @@ +import { Row, Table } from "@budibase/types" + +// import * as external from "./external" +import * as internal from "./internal" +import { isExternal } from "./utils" + +export async function create( + table: Omit, + rows?: Row[], + userId?: string +): Promise { + if (isExternal({ table })) { + // const datasourceId = table.sourceId! + throw "not implemented" + // return await external.create(table, rows, userId) + } + return await internal.create(table, rows, userId) +} diff --git a/packages/server/src/sdk/app/tables/index.ts b/packages/server/src/sdk/app/tables/index.ts index fcf7051e7c..326b2e1456 100644 --- a/packages/server/src/sdk/app/tables/index.ts +++ b/packages/server/src/sdk/app/tables/index.ts @@ -1,5 +1,6 @@ import { populateExternalTableSchemas } from "./validation" import * as getters from "./getters" +import * as create from "./create" import * as updates from "./update" import * as utils from "./utils" import { migrate } from "./migration" @@ -7,6 +8,7 @@ import * as sqs from "./internal/sqs" export default { populateExternalTableSchemas, + ...create, ...updates, ...getters, ...utils, diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index 7c72834ee0..b2081fee96 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -5,6 +5,7 @@ import { ViewStatisticsSchema, ViewV2, Row, + TableSourceType, } from "@budibase/types" import { hasTypeChanged, @@ -15,14 +16,48 @@ import { EventType, updateLinks } from "../../../../db/linkedRows" import { cloneDeep } from "lodash/fp" import isEqual from "lodash/isEqual" import { runStaticFormulaChecks } from "../../../../api/controllers/table/bulkFormula" -import { context } from "@budibase/backend-core" +import { context, HTTPError } from "@budibase/backend-core" import { findDuplicateInternalColumns } from "@budibase/shared-core" import { getTable } from "../getters" import { checkAutoColumns } from "./utils" import * as viewsSdk from "../../views" -import { getRowParams } from "../../../../db/utils" +import { generateTableID, getRowParams } from "../../../../db/utils" import { quotas } from "@budibase/pro" +export async function create(table: Table, rows?: Row[], userId?: string) { + const tableId = generateTableID() + + let tableToSave: Table = { + _id: tableId, + ...table, + // Ensure these fields are populated, even if not sent in the request + type: table.type || "table", + sourceType: TableSourceType.INTERNAL, + } + + const isImport = !!rows + + if (!tableToSave.views) { + tableToSave.views = {} + } + + try { + const { table } = await save(tableToSave, { + userId, + rowsToImport: rows, + isImport, + }) + + return table + } catch (err: any) { + if (err instanceof Error) { + throw new HTTPError(err.message, 400) + } else { + throw new HTTPError(err.message || err, err.status || 500) + } + } +} + export async function save( table: Table, opts?: { From 4efe335b65f6354d1f2b6a2a62c4ad39a0babfe4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 13:20:02 +0200 Subject: [PATCH 22/57] Create external table in sdk --- packages/server/src/sdk/app/tables/create.ts | 11 +++--- .../src/sdk/app/tables/external/index.ts | 36 +++++++++++++++++-- .../src/sdk/app/tables/internal/index.ts | 6 +++- 3 files changed, 45 insertions(+), 8 deletions(-) diff --git a/packages/server/src/sdk/app/tables/create.ts b/packages/server/src/sdk/app/tables/create.ts index 585623d0a4..ed6d6baeb0 100644 --- a/packages/server/src/sdk/app/tables/create.ts +++ b/packages/server/src/sdk/app/tables/create.ts @@ -1,6 +1,6 @@ import { Row, Table } from "@budibase/types" -// import * as external from "./external" +import * as external from "./external" import * as internal from "./internal" import { isExternal } from "./utils" @@ -9,10 +9,11 @@ export async function create( rows?: Row[], userId?: string ): Promise
{ + let createdTable: Table if (isExternal({ table })) { - // const datasourceId = table.sourceId! - throw "not implemented" - // return await external.create(table, rows, userId) + createdTable = await external.create(table) + } else { + createdTable = await internal.create(table, rows, userId) } - return await internal.create(table, rows, userId) + return createdTable } diff --git a/packages/server/src/sdk/app/tables/external/index.ts b/packages/server/src/sdk/app/tables/external/index.ts index e374e70c87..941d193b94 100644 --- a/packages/server/src/sdk/app/tables/external/index.ts +++ b/packages/server/src/sdk/app/tables/external/index.ts @@ -8,8 +8,11 @@ import { ViewV2, AutoFieldSubType, } from "@budibase/types" -import { context } from "@budibase/backend-core" -import { buildExternalTableId } from "../../../../integrations/utils" +import { context, HTTPError } from "@budibase/backend-core" +import { + breakExternalTableId, + buildExternalTableId, +} from "../../../../integrations/utils" import { foreignKeyStructure, hasTypeChanged, @@ -86,6 +89,35 @@ function validate(table: Table, oldTable?: Table) { } } +function getDatasourceId(table: Table) { + if (!table) { + throw new Error("No table supplied") + } + if (table.sourceId) { + return table.sourceId + } + if (!table._id) { + throw new Error("No table ID supplied") + } + return breakExternalTableId(table._id).datasourceId +} + +export async function create(table: Omit) { + const datasourceId = getDatasourceId(table) + + const tableToCreate = { ...table, created: true } + try { + const result = await save(datasourceId!, tableToCreate) + return result.table + } catch (err: any) { + if (err instanceof Error) { + throw new HTTPError(err.message, 400) + } else { + throw new HTTPError(err?.message || err, err.status || 500) + } + } +} + export async function save( datasourceId: string, update: Table, diff --git a/packages/server/src/sdk/app/tables/internal/index.ts b/packages/server/src/sdk/app/tables/internal/index.ts index b2081fee96..fbcbed03dc 100644 --- a/packages/server/src/sdk/app/tables/internal/index.ts +++ b/packages/server/src/sdk/app/tables/internal/index.ts @@ -24,7 +24,11 @@ import * as viewsSdk from "../../views" import { generateTableID, getRowParams } from "../../../../db/utils" import { quotas } from "@budibase/pro" -export async function create(table: Table, rows?: Row[], userId?: string) { +export async function create( + table: Omit, + rows?: Row[], + userId?: string +) { const tableId = generateTableID() let tableToSave: Table = { From 80ae7cbe0ba1b4f892e5a7cc8521947a09e36d94 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 13:21:22 +0200 Subject: [PATCH 23/57] Rename --- packages/server/src/api/controllers/table/external.ts | 2 +- packages/server/src/api/controllers/table/index.ts | 2 +- packages/server/src/api/controllers/table/internal.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/table/external.ts b/packages/server/src/api/controllers/table/external.ts index 5b15d3d9c7..6f09bf4a61 100644 --- a/packages/server/src/api/controllers/table/external.ts +++ b/packages/server/src/api/controllers/table/external.ts @@ -31,7 +31,7 @@ function getDatasourceId(table: Table) { return breakExternalTableId(table._id).datasourceId } -export async function save( +export async function updateTable( ctx: UserCtx, renaming?: RenameColumn ) { diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index a1969f44fc..0e44548d99 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -117,7 +117,7 @@ export async function save(ctx: UserCtx) { await events.table.created(savedTable) } else { const api = pickApi({ table }) - savedTable = await api.save(ctx, renaming) + savedTable = await api.updateTable(ctx, renaming) await events.table.updated(savedTable) } if (renaming) { diff --git a/packages/server/src/api/controllers/table/internal.ts b/packages/server/src/api/controllers/table/internal.ts index 256c5b1f0a..983f578bbc 100644 --- a/packages/server/src/api/controllers/table/internal.ts +++ b/packages/server/src/api/controllers/table/internal.ts @@ -12,7 +12,7 @@ import { } from "@budibase/types" import sdk from "../../../sdk" -export async function save( +export async function updateTable( ctx: UserCtx, renaming?: RenameColumn ) { From 20f55e379581c497b91f9578954e965c8e3bc223 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 12:29:17 +0100 Subject: [PATCH 24/57] Still fetch flags when the user is not logged in. --- .../backend-core/src/features/features.ts | 38 +++++++------ .../src/features/tests/features.spec.ts | 55 ++++++++++++++++++- 2 files changed, 75 insertions(+), 18 deletions(-) diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts index 90a395d52a..305c53ff3a 100644 --- a/packages/backend-core/src/features/features.ts +++ b/packages/backend-core/src/features/features.ts @@ -1,7 +1,8 @@ import env from "../environment" +import * as crypto from "crypto" import * as context from "../context" import { PostHog, PostHogOptions } from "posthog-node" -import { FeatureFlag, IdentityType, UserCtx } from "@budibase/types" +import { FeatureFlag, UserCtx } from "@budibase/types" import tracer from "dd-trace" import { Duration } from "../utils" @@ -224,24 +225,29 @@ export class FlagSet, T extends { [key: string]: V }> { } const identity = context.getIdentity() - tags[`identity.type`] = identity?.type - tags[`identity.tenantId`] = identity?.tenantId - tags[`identity._id`] = identity?._id - if (posthog && identity?.type === IdentityType.USER) { + let userId = identity?._id + if (!userId && ctx?.ip) { + userId = crypto.createHash("sha512").update(ctx.ip).digest("hex") + } + + let tenantId = identity?.tenantId + if (!tenantId) { + tenantId = currentTenantId + } + + tags[`identity.type`] = identity?.type + tags[`identity._id`] = identity?._id + tags[`tenantId`] = tenantId + tags[`userId`] = userId + + if (posthog && userId) { tags[`readFromPostHog`] = true - const personProperties: Record = {} - if (identity.tenantId) { - personProperties.tenantId = identity.tenantId - } - - const posthogFlags = await posthog.getAllFlagsAndPayloads( - identity._id, - { - personProperties, - } - ) + const personProperties: Record = { tenantId } + const posthogFlags = await posthog.getAllFlagsAndPayloads(userId, { + personProperties, + }) for (const [name, value] of Object.entries(posthogFlags.featureFlags)) { if (!this.isFlagName(name)) { diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 01c9bfa3c6..bf01ad67f3 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -4,6 +4,7 @@ import * as context from "../../context" import environment, { withEnv } from "../../environment" import nodeFetch from "node-fetch" import nock from "nock" +import * as crypto from "crypto" const schema = { TEST_BOOLEAN: Flag.boolean(false), @@ -27,10 +28,14 @@ interface PostHogFlags { featureFlagPayloads?: Record } -function mockPosthogFlags(flags: PostHogFlags) { +function mockPosthogFlags( + flags: PostHogFlags, + opts?: { token?: string; distinct_id?: string } +) { + const { token = "test", distinct_id = "us_1234" } = opts || {} nock("https://us.i.posthog.com") .post("/decide/?v=3", body => { - return body.token === "test" && body.distinct_id === "us_1234" + return body.token === token && body.distinct_id === distinct_id }) .reply(200, flags) .persist() @@ -214,6 +219,14 @@ describe("feature flags", () => { lastName: "User", } + // We need to pass in node-fetch here otherwise nock won't get used + // because posthog-node uses axios under the hood. + init({ + fetch: (url, opts) => { + return nodeFetch(url, opts) + }, + }) + nock("https://us.i.posthog.com") .post("/decide/?v=3", body => { return body.token === "test" && body.distinct_id === "us_1234" @@ -230,4 +243,42 @@ describe("feature flags", () => { } ) }) + + it("should still get flags when user is logged out", async () => { + const env: Partial = { + SELF_HOSTED: false, + POSTHOG_FEATURE_FLAGS_ENABLED: "true", + POSTHOG_API_HOST: "https://us.i.posthog.com", + POSTHOG_TOKEN: "test", + } + + const ctx = { ip: "127.0.0.1" } as UserCtx + const hashedIp = crypto.createHash("sha512").update(ctx.ip).digest("hex") + + await withEnv(env, async () => { + mockPosthogFlags( + { + featureFlags: { TEST_BOOLEAN: true }, + }, + { + distinct_id: hashedIp, + } + ) + + // We need to pass in node-fetch here otherwise nock won't get used + // because posthog-node uses axios under the hood. + init({ + fetch: (url, opts) => { + return nodeFetch(url, opts) + }, + }) + + await context.doInTenant("default", async () => { + const result = await flags.fetch(ctx) + expect(result.TEST_BOOLEAN).toBe(true) + }) + + shutdown() + }) + }) }) From f05bf25e21e0246b9baa726d2ae71e7b5a09fa15 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 13:31:11 +0200 Subject: [PATCH 25/57] Add failing test --- packages/server/src/api/routes/tests/row.spec.ts | 12 +++++++++++- packages/server/src/tests/utilities/api/row.ts | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 207420bf9f..0995bd3824 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1846,7 +1846,7 @@ describe.each([ }) describe("exportRows", () => { - beforeAll(async () => { + beforeEach(async () => { table = await config.api.table.save(defaultTable()) }) @@ -1883,6 +1883,16 @@ describe.each([ }) }) + it("should allow exporting without filtering", async () => { + const existing = await config.api.row.save(table._id!, {}) + const res = await config.api.row.exportRows(table._id!) + const results = JSON.parse(res) + expect(results.length).toEqual(1) + const row = results[0] + + expect(row._id).toEqual(existing._id) + }) + it("should allow exporting only certain columns", async () => { const existing = await config.api.row.save(table._id!, {}) const res = await config.api.row.exportRows(table._id!, { diff --git a/packages/server/src/tests/utilities/api/row.ts b/packages/server/src/tests/utilities/api/row.ts index 6bec59fdf7..52317e142a 100644 --- a/packages/server/src/tests/utilities/api/row.ts +++ b/packages/server/src/tests/utilities/api/row.ts @@ -105,7 +105,7 @@ export class RowAPI extends TestAPI { exportRows = async ( tableId: string, - body: ExportRowsRequest, + body?: ExportRowsRequest, format: RowExportFormat = RowExportFormat.JSON, expectations?: Expectations ) => { From 865b7a97e0f6c7f96d39a84f22255dd8e565c6bf Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 13:31:34 +0200 Subject: [PATCH 26/57] Fix --- packages/server/src/sdk/app/rows/search/internal/internal.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/sdk/app/rows/search/internal/internal.ts b/packages/server/src/sdk/app/rows/search/internal/internal.ts index 6617fc376c..0ca10e82d4 100644 --- a/packages/server/src/sdk/app/rows/search/internal/internal.ts +++ b/packages/server/src/sdk/app/rows/search/internal/internal.ts @@ -62,10 +62,10 @@ export async function exportRows( ).rows.map(row => row.doc!) result = await outputProcessing(table, response) - } else if (query) { + } else { let searchResponse = await sdk.rows.search({ tableId, - query, + query: query || {}, sort, sortOrder, }) From 3b01f404dd500033ca71a23dae04557ac87b2283 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 13:49:24 +0200 Subject: [PATCH 27/57] Fix imports --- packages/server/src/api/controllers/table/index.ts | 6 +++--- packages/server/src/api/controllers/table/internal.ts | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/server/src/api/controllers/table/index.ts b/packages/server/src/api/controllers/table/index.ts index 0e44548d99..404f82e57c 100644 --- a/packages/server/src/api/controllers/table/index.ts +++ b/packages/server/src/api/controllers/table/index.ts @@ -102,8 +102,8 @@ export async function find(ctx: UserCtx) { export async function save(ctx: UserCtx) { const appId = ctx.appId - const table = ctx.request.body - const isImport = table.rows + const { rows, ...table } = ctx.request.body + const isImport = rows const renaming = ctx.request.body._rename const isCreate = !table._id @@ -112,7 +112,7 @@ export async function save(ctx: UserCtx) { let savedTable: Table if (isCreate) { - savedTable = await sdk.tables.create(table) + savedTable = await sdk.tables.create(table, rows, ctx.user._id) savedTable = await sdk.tables.enrichViewSchemas(savedTable) await events.table.created(savedTable) } else { diff --git a/packages/server/src/api/controllers/table/internal.ts b/packages/server/src/api/controllers/table/internal.ts index 983f578bbc..40ce5e279d 100644 --- a/packages/server/src/api/controllers/table/internal.ts +++ b/packages/server/src/api/controllers/table/internal.ts @@ -25,8 +25,6 @@ export async function updateTable( sourceType: rest.sourceType || TableSourceType.INTERNAL, } - const isImport = !!rows - if (!tableToSave.views) { tableToSave.views = {} } @@ -37,7 +35,6 @@ export async function updateTable( rowsToImport: rows, tableId: ctx.request.body._id, renaming, - isImport, }) return table From 4414da710ca22cd212df5d0040c30013715b1766 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Wed, 9 Oct 2024 11:53:39 +0000 Subject: [PATCH 28/57] Bump version to 2.32.14 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index faed02052e..9b765f17ae 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "2.32.13", + "version": "2.32.14", "npmClient": "yarn", "packages": [ "packages/*", From eee2991b098bfc55d87aa032d02d372eb86b387d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 12:57:14 +0100 Subject: [PATCH 29/57] Rejib IP fetching. --- .../backend-core/src/context/mainContext.ts | 9 ++++ packages/backend-core/src/context/types.ts | 1 + .../backend-core/src/features/features.ts | 47 ++++--------------- packages/backend-core/src/middleware/index.ts | 1 + packages/backend-core/src/middleware/ip.ts | 12 +++++ packages/server/src/koa.ts | 1 + packages/worker/src/index.ts | 1 + 7 files changed, 33 insertions(+), 39 deletions(-) create mode 100644 packages/backend-core/src/middleware/ip.ts diff --git a/packages/backend-core/src/context/mainContext.ts b/packages/backend-core/src/context/mainContext.ts index 25b273e51c..64ba240fa5 100644 --- a/packages/backend-core/src/context/mainContext.ts +++ b/packages/backend-core/src/context/mainContext.ts @@ -253,6 +253,11 @@ export function getAppId(): string | undefined { } } +export function getIP(): string | undefined { + const context = Context.get() + return context?.ip +} + export const getProdAppId = () => { const appId = getAppId() if (!appId) { @@ -281,6 +286,10 @@ export function doInScimContext(task: any) { return newContext(updates, task) } +export function doInIPContext(ip: string, task: any) { + return newContext({ ip }, task) +} + export async function ensureSnippetContext(enabled = !env.isTest()) { const ctx = getCurrentContext() diff --git a/packages/backend-core/src/context/types.ts b/packages/backend-core/src/context/types.ts index ee84b49459..5549a47ff7 100644 --- a/packages/backend-core/src/context/types.ts +++ b/packages/backend-core/src/context/types.ts @@ -9,6 +9,7 @@ export type ContextMap = { identity?: IdentityContext environmentVariables?: Record isScim?: boolean + ip?: string automationId?: string isMigrating?: boolean vm?: VM diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts index 305c53ff3a..5f2ba50f67 100644 --- a/packages/backend-core/src/features/features.ts +++ b/packages/backend-core/src/features/features.ts @@ -2,7 +2,7 @@ import env from "../environment" import * as crypto from "crypto" import * as context from "../context" import { PostHog, PostHogOptions } from "posthog-node" -import { FeatureFlag, UserCtx } from "@budibase/types" +import { FeatureFlag } from "@budibase/types" import tracer from "dd-trace" import { Duration } from "../utils" @@ -142,23 +142,17 @@ export class FlagSet, T extends { [key: string]: V }> { return this.flagSchema[name as keyof T] !== undefined } - async get( - key: K, - ctx?: UserCtx - ): Promise[K]> { - const flags = await this.fetch(ctx) + async get(key: K): Promise[K]> { + const flags = await this.fetch() return flags[key] } - async isEnabled>( - key: K, - ctx?: UserCtx - ): Promise { - const flags = await this.fetch(ctx) + async isEnabled>(key: K): Promise { + const flags = await this.fetch() return flags[key] } - async fetch(ctx?: UserCtx): Promise> { + async fetch(): Promise> { return await tracer.trace("features.fetch", async span => { const cachedFlags = context.getFeatureFlags>(this.setId) if (cachedFlags) { @@ -199,36 +193,11 @@ export class FlagSet, T extends { [key: string]: V }> { tags[`flags.${key}.source`] = "environment" } - const license = ctx?.user?.license - if (license) { - tags[`readFromLicense`] = true - - for (const feature of license.features) { - if (!this.isFlagName(feature)) { - continue - } - - if ( - flagValues[feature] === true || - specificallySetFalse.has(feature) - ) { - // If the flag is already set to through environment variables, we - // don't want to override it back to false here. - continue - } - - // @ts-expect-error - TS does not like you writing into a generic type, - // but we know that it's okay in this case because it's just an object. - flagValues[feature] = true - tags[`flags.${feature}.source`] = "license" - } - } - const identity = context.getIdentity() let userId = identity?._id - if (!userId && ctx?.ip) { - userId = crypto.createHash("sha512").update(ctx.ip).digest("hex") + if (!userId) { + userId = context.getIP() } let tenantId = identity?.tenantId diff --git a/packages/backend-core/src/middleware/index.ts b/packages/backend-core/src/middleware/index.ts index e1eb7f1d26..20c2125b13 100644 --- a/packages/backend-core/src/middleware/index.ts +++ b/packages/backend-core/src/middleware/index.ts @@ -20,3 +20,4 @@ export { default as correlation } from "../logging/correlation/middleware" export { default as errorHandling } from "./errorHandling" export { default as querystringToBody } from "./querystringToBody" export * as joiValidator from "./joi-validator" +export { default as ip } from "./ip" diff --git a/packages/backend-core/src/middleware/ip.ts b/packages/backend-core/src/middleware/ip.ts new file mode 100644 index 0000000000..69a0a2da38 --- /dev/null +++ b/packages/backend-core/src/middleware/ip.ts @@ -0,0 +1,12 @@ +import { Ctx } from "@budibase/types" +import { doInIPContext } from "../context" + +export default async (ctx: Ctx, next: any) => { + if (ctx.ip) { + doInIPContext(ctx.ip, () => { + return next() + }) + } else { + return next() + } +} diff --git a/packages/server/src/koa.ts b/packages/server/src/koa.ts index 9f90c04b50..dd8063795d 100644 --- a/packages/server/src/koa.ts +++ b/packages/server/src/koa.ts @@ -35,6 +35,7 @@ export default function createKoaApp() { app.use(middleware.correlation) app.use(middleware.pino) + app.use(middleware.ip) app.use(userAgent) const server = http.createServer(app.callback()) diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts index 79e6f4493d..9c3c053baf 100644 --- a/packages/worker/src/index.ts +++ b/packages/worker/src/index.ts @@ -54,6 +54,7 @@ app.use(koaBody({ multipart: true })) app.use(koaSession(app)) app.use(middleware.correlation) app.use(middleware.pino) +app.use(middleware.ip) app.use(userAgent) // authentication From adad73a9e2210ed0986d70eab5b5a55f377216d5 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 13:10:52 +0100 Subject: [PATCH 30/57] Fix tests. --- .../backend-core/src/features/features.ts | 5 ++- .../src/features/tests/features.spec.ts | 43 +++++-------------- packages/backend-core/src/middleware/ip.ts | 2 +- 3 files changed, 15 insertions(+), 35 deletions(-) diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts index 5f2ba50f67..efe6495cb5 100644 --- a/packages/backend-core/src/features/features.ts +++ b/packages/backend-core/src/features/features.ts @@ -197,7 +197,10 @@ export class FlagSet, T extends { [key: string]: V }> { let userId = identity?._id if (!userId) { - userId = context.getIP() + const ip = context.getIP() + if (ip) { + userId = crypto.createHash("sha512").update(ip).digest("hex") + } } let tenantId = identity?.tenantId diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index bf01ad67f3..664490dcda 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -18,7 +18,6 @@ interface TestCase { identity?: Partial environmentFlags?: string posthogFlags?: PostHogFlags - licenseFlags?: Array expected?: Partial> errorMessage?: string | RegExp } @@ -117,17 +116,6 @@ describe("feature flags", () => { }, expected: { TEST_BOOLEAN: true }, }, - { - it: "should be able to set boolean flags through the license", - licenseFlags: ["TEST_BOOLEAN"], - expected: { TEST_BOOLEAN: true }, - }, - { - it: "should not be able to override a negative environment flag from license", - environmentFlags: "default:!TEST_BOOLEAN", - licenseFlags: ["TEST_BOOLEAN"], - expected: { TEST_BOOLEAN: false }, - }, { it: "should not error on unrecognised PostHog flag", posthogFlags: { @@ -135,18 +123,12 @@ describe("feature flags", () => { }, expected: flags.defaults(), }, - { - it: "should not error on unrecognised license flag", - licenseFlags: ["UNDEFINED"], - expected: flags.defaults(), - }, ])( "$it", async ({ identity, environmentFlags, posthogFlags, - licenseFlags, expected, errorMessage, }) => { @@ -162,8 +144,6 @@ describe("feature flags", () => { env.POSTHOG_API_HOST = "https://us.i.posthog.com" } - const ctx = { user: { license: { features: licenseFlags || [] } } } - await withEnv(env, async () => { // We need to pass in node-fetch here otherwise nock won't get used // because posthog-node uses axios under the hood. @@ -185,18 +165,13 @@ describe("feature flags", () => { await context.doInIdentityContext(fullIdentity, async () => { if (errorMessage) { - await expect(flags.fetch(ctx as UserCtx)).rejects.toThrow( - errorMessage - ) + await expect(flags.fetch()).rejects.toThrow(errorMessage) } else if (expected) { - const values = await flags.fetch(ctx as UserCtx) + const values = await flags.fetch() expect(values).toMatchObject(expected) for (const [key, expectedValue] of Object.entries(expected)) { - const value = await flags.get( - key as keyof typeof schema, - ctx as UserCtx - ) + const value = await flags.get(key as keyof typeof schema) expect(value).toBe(expectedValue) } } else { @@ -252,8 +227,8 @@ describe("feature flags", () => { POSTHOG_TOKEN: "test", } - const ctx = { ip: "127.0.0.1" } as UserCtx - const hashedIp = crypto.createHash("sha512").update(ctx.ip).digest("hex") + const ip = "127.0.0.1" + const hashedIp = crypto.createHash("sha512").update(ip).digest("hex") await withEnv(env, async () => { mockPosthogFlags( @@ -273,9 +248,11 @@ describe("feature flags", () => { }, }) - await context.doInTenant("default", async () => { - const result = await flags.fetch(ctx) - expect(result.TEST_BOOLEAN).toBe(true) + await context.doInIPContext(ip, async () => { + await context.doInTenant("default", async () => { + const result = await flags.fetch() + expect(result.TEST_BOOLEAN).toBe(true) + }) }) shutdown() diff --git a/packages/backend-core/src/middleware/ip.ts b/packages/backend-core/src/middleware/ip.ts index 69a0a2da38..940f644ad6 100644 --- a/packages/backend-core/src/middleware/ip.ts +++ b/packages/backend-core/src/middleware/ip.ts @@ -3,7 +3,7 @@ import { doInIPContext } from "../context" export default async (ctx: Ctx, next: any) => { if (ctx.ip) { - doInIPContext(ctx.ip, () => { + return await doInIPContext(ctx.ip, () => { return next() }) } else { From a4090243ecc0895a3bdb152d74187c0e67e70e43 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 13:17:55 +0100 Subject: [PATCH 31/57] Fix lint. --- packages/backend-core/src/features/tests/features.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 664490dcda..9af8a8f4bb 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -1,4 +1,4 @@ -import { IdentityContext, IdentityType, UserCtx } from "@budibase/types" +import { IdentityContext, IdentityType } from "@budibase/types" import { Flag, FlagSet, FlagValues, init, shutdown } from "../" import * as context from "../../context" import environment, { withEnv } from "../../environment" From 26f2deb234596e3d273f4550023f5238774410dd Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 13:34:43 +0100 Subject: [PATCH 32/57] Set proxy setting on Koa application. --- packages/backend-core/src/features/features.ts | 2 ++ packages/server/src/koa.ts | 1 + packages/worker/src/index.ts | 1 + 3 files changed, 4 insertions(+) diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts index efe6495cb5..cb095e792c 100644 --- a/packages/backend-core/src/features/features.ts +++ b/packages/backend-core/src/features/features.ts @@ -198,6 +198,8 @@ export class FlagSet, T extends { [key: string]: V }> { let userId = identity?._id if (!userId) { const ip = context.getIP() + // TODO; REMOVE THIS + tags["userIP"] = ip if (ip) { userId = crypto.createHash("sha512").update(ip).digest("hex") } diff --git a/packages/server/src/koa.ts b/packages/server/src/koa.ts index dd8063795d..acae433cc3 100644 --- a/packages/server/src/koa.ts +++ b/packages/server/src/koa.ts @@ -12,6 +12,7 @@ import { userAgent } from "koa-useragent" export default function createKoaApp() { const app = new Koa() + app.proxy = true let mbNumber = parseInt(env.HTTP_MB_LIMIT || "10") if (!mbNumber || isNaN(mbNumber)) { diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts index 9c3c053baf..fb6a97a844 100644 --- a/packages/worker/src/index.ts +++ b/packages/worker/src/index.ts @@ -46,6 +46,7 @@ bootstrap() const app: Application = new Application() app.keys = ["secret", "key"] +app.proxy = true // set up top level koa middleware app.use(handleScimBody) From d62d5b7043af74f015c654c78c014f849e96428e Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 15:09:38 +0100 Subject: [PATCH 33/57] Fixing an issue with removing relationships from the many side of a table in SQL, this was not correctly updating the other table. --- packages/backend-core/src/sql/sql.ts | 25 +++++++++++++------ .../api/controllers/row/ExternalRequest.ts | 22 ++++++++-------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index ed8dc929d6..05b38db309 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -325,14 +325,18 @@ class InternalBuilder { return input } - private parseBody(body: any) { - for (let [key, value] of Object.entries(body)) { - const { column } = this.splitter.run(key) - const schema = this.table.schema[column] - if (!schema) { - continue + private parseBody(body: Record) { + try { + for (let [key, value] of Object.entries(body)) { + const { column } = this.splitter.run(key) + const schema = this.table.schema[column] + if (!schema) { + continue + } + body[key] = this.parse(value, schema) } - body[key] = this.parse(value, schema) + } catch (err) { + console.log(err) } return body } @@ -1259,6 +1263,10 @@ class InternalBuilder { create(opts: QueryOptions): Knex.QueryBuilder { const { body } = this.query + if (!body) { + throw new Error("Cannot create without row body") + } + let query = this.qualifiedKnex({ alias: false }) const parsedBody = this.parseBody(body) @@ -1417,6 +1425,9 @@ class InternalBuilder { update(opts: QueryOptions): Knex.QueryBuilder { const { body, filters } = this.query + if (!body) { + throw new Error("Cannot update without row body") + } let query = this.qualifiedKnex() const parsedBody = this.parseBody(body) query = this.addFilters(query, filters) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index ed8836626c..62ceedb56b 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -269,18 +269,13 @@ export class ExternalRequest { } } - private async removeManyToManyRelationships( - rowId: string, - table: Table, - colName: string - ) { + private async removeManyToManyRelationships(rowId: string, table: Table) { const tableId = table._id! const filters = this.prepareFilters(rowId, {}, table) // safety check, if there are no filters on deletion bad things happen if (Object.keys(filters).length !== 0) { return getDatasourceAndQuery({ endpoint: getEndpoint(tableId, Operation.DELETE), - body: { [colName]: null }, filters, meta: { table, @@ -291,13 +286,18 @@ export class ExternalRequest { } } - private async removeOneToManyRelationships(rowId: string, table: Table) { + private async removeOneToManyRelationships( + rowId: string, + table: Table, + colName: string + ) { const tableId = table._id! const filters = this.prepareFilters(rowId, {}, table) // safety check, if there are no filters on deletion bad things happen if (Object.keys(filters).length !== 0) { return getDatasourceAndQuery({ endpoint: getEndpoint(tableId, Operation.UPDATE), + body: { [colName]: null }, filters, meta: { table, @@ -595,8 +595,8 @@ export class ExternalRequest { for (let row of rows) { const rowId = generateIdForRow(row, table) const promise: Promise = isMany - ? this.removeManyToManyRelationships(rowId, table, colName) - : this.removeOneToManyRelationships(rowId, table) + ? this.removeManyToManyRelationships(rowId, table) + : this.removeOneToManyRelationships(rowId, table, colName) if (promise) { promises.push(promise) } @@ -619,12 +619,12 @@ export class ExternalRequest { rows.map(row => { const rowId = generateIdForRow(row, table) return isMany - ? this.removeManyToManyRelationships( + ? this.removeManyToManyRelationships(rowId, table) + : this.removeOneToManyRelationships( rowId, table, relationshipColumn.fieldName ) - : this.removeOneToManyRelationships(rowId, table) }) ) } From 9d70b123e5bc2d8734a0f928b35944e2d3acf61f Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 15:22:38 +0100 Subject: [PATCH 34/57] Remove IP tag. --- packages/backend-core/src/features/features.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/backend-core/src/features/features.ts b/packages/backend-core/src/features/features.ts index cb095e792c..efe6495cb5 100644 --- a/packages/backend-core/src/features/features.ts +++ b/packages/backend-core/src/features/features.ts @@ -198,8 +198,6 @@ export class FlagSet, T extends { [key: string]: V }> { let userId = identity?._id if (!userId) { const ip = context.getIP() - // TODO; REMOVE THIS - tags["userIP"] = ip if (ip) { userId = crypto.createHash("sha512").update(ip).digest("hex") } From 3405e6d6b7ac795398b03d720267a0804c2d46f1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 4 Oct 2024 16:12:01 +0100 Subject: [PATCH 35/57] Make new tables require ADMIN permissions to read and write. --- .../server/src/api/routes/tests/table.spec.ts | 9 ++++++- packages/server/src/utilities/security.ts | 26 ++++++++++++------- packages/types/src/documents/document.ts | 24 +++++++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index d6c1651ef0..f82476d412 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,4 @@ -import { context, docIds, events } from "@budibase/backend-core" +import { context, docIds, events, roles } from "@budibase/backend-core" import { PROTECTED_EXTERNAL_COLUMNS, PROTECTED_INTERNAL_COLUMNS, @@ -189,6 +189,13 @@ describe.each([ ) } ) + + it("should create tables with ADMIN read and write permissions", async () => { + const table = await config.api.table.save(tableForDatasource(datasource)) + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions.read.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + expect(permissions.write.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + }) }) describe("update", () => { diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index 01a3468c9c..40dba465f1 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -1,5 +1,6 @@ import { permissions, roles } from "@budibase/backend-core" import { DocumentType, VirtualDocumentType } from "../db/utils" +import { getDocumentType, getVirtualDocumentType } from "@budibase/types" export const CURRENTLY_SUPPORTED_LEVELS: string[] = [ permissions.PermissionLevel.WRITE, @@ -8,14 +9,16 @@ export const CURRENTLY_SUPPORTED_LEVELS: string[] = [ ] export function getPermissionType(resourceId: string) { - const docType = Object.values(DocumentType).filter(docType => - resourceId.startsWith(docType) - )[0] - switch (docType as DocumentType | VirtualDocumentType) { - case DocumentType.TABLE: - case DocumentType.ROW: + const virtualDocType = getVirtualDocumentType(resourceId) + switch (virtualDocType) { case VirtualDocumentType.VIEW: return permissions.PermissionType.TABLE + } + + const docType = getDocumentType(resourceId) + switch (docType) { + case DocumentType.TABLE: + case DocumentType.ROW: case DocumentType.AUTOMATION: return permissions.PermissionType.AUTOMATION case DocumentType.WEBHOOK: @@ -39,15 +42,18 @@ export function getBasePermissions(resourceId: string) { if (!role.permissionId) { continue } + const perms = permissions.getBuiltinPermissionByID(role.permissionId) if (!perms) { continue } + const typedPermission = perms.permissions.find(perm => perm.type === type) - if ( - typedPermission && - CURRENTLY_SUPPORTED_LEVELS.indexOf(typedPermission.level) !== -1 - ) { + if (!typedPermission) { + continue + } + + if (CURRENTLY_SUPPORTED_LEVELS.includes(typedPermission.level)) { const level = typedPermission.level basePermissions[level] = roles.lowerBuiltinRoleID( basePermissions[level], diff --git a/packages/types/src/documents/document.ts b/packages/types/src/documents/document.ts index f5facfae9d..2f0da1081a 100644 --- a/packages/types/src/documents/document.ts +++ b/packages/types/src/documents/document.ts @@ -42,6 +42,17 @@ export enum DocumentType { ROW_ACTIONS = "ra", } +// Because DocumentTypes can overlap, we need to make sure that we search +// longest first to ensure we get the correct type. +const sortedDocumentTypes = Object.values(DocumentType).sort( + (a, b) => b.length - a.length // descending +) +export function getDocumentType(id: string): DocumentType | undefined { + return sortedDocumentTypes.find(docType => + id.startsWith(`${docType}${SEPARATOR}`) + ) +} + // these are the core documents that make up the data, design // and automation sections of an app. This excludes any internal // rows as we shouldn't import data. @@ -72,6 +83,19 @@ export enum VirtualDocumentType { ROW_ACTION = "row_action", } +// Because VirtualDocumentTypes can overlap, we need to make sure that we search +// longest first to ensure we get the correct type. +const sortedVirtualDocumentTypes = Object.values(VirtualDocumentType).sort( + (a, b) => b.length - a.length // descending +) +export function getVirtualDocumentType( + id: string +): VirtualDocumentType | undefined { + return sortedVirtualDocumentTypes.find(docType => + id.startsWith(`${docType}${SEPARATOR}`) + ) +} + export interface Document { _id?: string _rev?: string From 4cde2f26ad7a9c74ce041122d652cef5eebef9e0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 8 Oct 2024 11:06:54 +0100 Subject: [PATCH 36/57] wip, tests broken --- .../server/src/api/controllers/permission.ts | 7 ++- .../src/api/routes/tests/permissions.spec.ts | 61 +++++++++++-------- .../server/src/api/routes/tests/table.spec.ts | 17 +++++- .../src/tests/utilities/api/permission.ts | 10 +++ packages/server/src/utilities/security.ts | 4 +- packages/types/src/api/web/app/permission.ts | 4 ++ 6 files changed, 73 insertions(+), 30 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index c7afb6a351..668501b3f3 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -9,6 +9,7 @@ import { AddPermissionRequest, RemovePermissionRequest, RemovePermissionResponse, + FetchResourcePermissionInfoResponse, } from "@budibase/types" import { CURRENTLY_SUPPORTED_LEVELS, @@ -28,7 +29,9 @@ export function fetchLevels(ctx: UserCtx) { ctx.body = SUPPORTED_LEVELS } -export async function fetch(ctx: UserCtx) { +export async function fetch( + ctx: UserCtx +) { const db = context.getAppDB() const dbRoles: Role[] = await sdk.permissions.getAllDBRoles(db) let permissions: any = {} @@ -49,7 +52,7 @@ export async function fetch(ctx: UserCtx) { } } // apply the base permissions - const finalPermissions: Record> = {} + const finalPermissions: FetchResourcePermissionInfoResponse = {} for (let [resource, permission] of Object.entries(permissions)) { const basePerms = getBasePermissions(resource) finalPermissions[resource] = Object.assign(basePerms, permission) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index b148d6fde1..f8974f44aa 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,7 +12,7 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let table: Table & { _id: string } + let table: Table let perms: Document[] let row: Row let view: ViewV2 @@ -26,15 +26,33 @@ describe("/permission", () => { beforeEach(async () => { mocks.licenses.useCloudFree() - table = (await config.createTable()) as typeof table + table = await config.createTable() + + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.READ, + }) + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.WRITE, + }) + await config.api.permission.revoke({ + roleId: roles.BUILTIN_ROLE_IDS.ADMIN, + resourceId: table._id!, + level: PermissionLevel.EXECUTE, + }) + row = await config.createRow() view = await config.api.viewV2.create({ tableId: table._id!, name: generator.guid(), }) + perms = await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) }) @@ -74,27 +92,22 @@ describe("/permission", () => { }) }) - it("should get resource permissions with multiple roles", async () => { + it.only("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.WRITE, }) - const res = await config.api.permission.get(table._id) - expect(res).toEqual({ - permissions: { - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - }, + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions).toEqual({ + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, + execute: { permissionType: "BASE", role: "BASIC" }, }) - const allRes = await request - .get(`/api/permission`) - .set(config.defaultHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) - expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + const all = await config.api.permission.fetch() + expect(all[table._id!]["read"]).toEqual(STD_ROLE_ID) + expect(all[table._id!]["write"]).toEqual(HIGHER_ROLE_ID) }) }) @@ -102,11 +115,11 @@ describe("/permission", () => { it("should be able to remove the permission", async () => { const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id) + const permsRes = await config.api.permission.get(table._id!) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) }) @@ -142,7 +155,7 @@ describe("/permission", () => { it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) @@ -167,7 +180,7 @@ describe("/permission", () => { }) await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: table._id!, level: PermissionLevel.READ, }) // replicate changes before checking permissions @@ -179,8 +192,8 @@ describe("/permission", () => { it("shouldn't allow writing from a public user", async () => { const res = await request - .post(`/api/${table._id}/rows`) - .send(basicRow(table._id)) + .post(`/api/${table._id!}/rows`) + .send(basicRow(table._id!)) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(401) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index f82476d412..60e8142522 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -21,6 +21,7 @@ import { ViewCalculation, ViewV2Enriched, RowExportFormat, + PermissionSource, } from "@budibase/types" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" @@ -193,8 +194,20 @@ describe.each([ it("should create tables with ADMIN read and write permissions", async () => { const table = await config.api.table.save(tableForDatasource(datasource)) const { permissions } = await config.api.permission.get(table._id!) - expect(permissions.read.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) - expect(permissions.write.role).toEqual(roles.BUILTIN_ROLE_IDS.ADMIN) + expect(permissions).toEqual({ + read: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + write: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + execute: { + permissionType: PermissionSource.EXPLICIT, + role: roles.BUILTIN_ROLE_IDS.ADMIN, + }, + }) }) }) diff --git a/packages/server/src/tests/utilities/api/permission.ts b/packages/server/src/tests/utilities/api/permission.ts index 986796d9a1..b4e641a1be 100644 --- a/packages/server/src/tests/utilities/api/permission.ts +++ b/packages/server/src/tests/utilities/api/permission.ts @@ -1,6 +1,7 @@ import { AddPermissionRequest, AddPermissionResponse, + FetchResourcePermissionInfoResponse, GetResourcePermsResponse, RemovePermissionRequest, RemovePermissionResponse, @@ -26,6 +27,15 @@ export class PermissionAPI extends TestAPI { ) } + fetch = async ( + expectations?: Expectations + ): Promise => { + return await this._get( + `/api/permission`, + { expectations } + ) + } + revoke = async ( request: RemovePermissionRequest, expectations?: Expectations diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index 40dba465f1..a3353f6184 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -35,9 +35,9 @@ export function getPermissionType(resourceId: string) { /** * works out the basic permissions based on builtin roles for a resource, using its ID */ -export function getBasePermissions(resourceId: string) { +export function getBasePermissions(resourceId: string): Record { const type = getPermissionType(resourceId) - const basePermissions: { [key: string]: string } = {} + const basePermissions: Record = {} for (let [roleId, role] of Object.entries(roles.getBuiltinRoles())) { if (!role.permissionId) { continue diff --git a/packages/types/src/api/web/app/permission.ts b/packages/types/src/api/web/app/permission.ts index bead2a4279..a5c4df5733 100644 --- a/packages/types/src/api/web/app/permission.ts +++ b/packages/types/src/api/web/app/permission.ts @@ -1,5 +1,9 @@ import { PermissionLevel } from "../../../sdk" +export interface FetchResourcePermissionInfoResponse { + [key: string]: Record +} + export interface ResourcePermissionInfo { role: string permissionType: string From 9f84262940f34178bc57a44e7b11231f0528e066 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:12:37 +0200 Subject: [PATCH 37/57] Clean --- packages/server/src/api/controllers/permission.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 668501b3f3..148cea401a 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -33,7 +33,7 @@ export async function fetch( ctx: UserCtx ) { const db = context.getAppDB() - const dbRoles: Role[] = await sdk.permissions.getAllDBRoles(db) + const dbRoles = await sdk.permissions.getAllDBRoles(db) let permissions: any = {} // create an object with structure role ID -> resource ID -> level for (let role of dbRoles) { From aa0a1737c8021508ecf6316001788a72c3d094b4 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:16:17 +0200 Subject: [PATCH 38/57] Fix tests --- packages/server/src/api/controllers/permission.ts | 1 - packages/server/src/api/routes/tests/permissions.spec.ts | 3 +-- packages/server/src/utilities/security.ts | 1 + 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index 148cea401a..cbe260367c 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -1,7 +1,6 @@ import { permissions, roles, context } from "@budibase/backend-core" import { UserCtx, - Role, GetResourcePermsResponse, ResourcePermissionInfo, GetDependantResourcesResponse, diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index f8974f44aa..8601ed2df5 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -92,7 +92,7 @@ describe("/permission", () => { }) }) - it.only("should get resource permissions with multiple roles", async () => { + it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, resourceId: table._id!, @@ -102,7 +102,6 @@ describe("/permission", () => { expect(permissions).toEqual({ read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - execute: { permissionType: "BASE", role: "BASIC" }, }) const all = await config.api.permission.fetch() diff --git a/packages/server/src/utilities/security.ts b/packages/server/src/utilities/security.ts index a3353f6184..4f93c33ee4 100644 --- a/packages/server/src/utilities/security.ts +++ b/packages/server/src/utilities/security.ts @@ -19,6 +19,7 @@ export function getPermissionType(resourceId: string) { switch (docType) { case DocumentType.TABLE: case DocumentType.ROW: + return permissions.PermissionType.TABLE case DocumentType.AUTOMATION: return permissions.PermissionType.AUTOMATION case DocumentType.WEBHOOK: From 1e0902a8313273d522a1c2ffa8bc6e2cd5947f05 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:40:15 +0200 Subject: [PATCH 39/57] Cleanup tess --- .../src/api/routes/tests/permissions.spec.ts | 277 +++++++++--------- 1 file changed, 137 insertions(+), 140 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 8601ed2df5..b938bff279 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -12,10 +12,7 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let table: Table let perms: Document[] - let row: Row - let view: ViewV2 afterAll(setup.afterAll) @@ -25,36 +22,6 @@ describe("/permission", () => { beforeEach(async () => { mocks.licenses.useCloudFree() - - table = await config.createTable() - - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.READ, - }) - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.WRITE, - }) - await config.api.permission.revoke({ - roleId: roles.BUILTIN_ROLE_IDS.ADMIN, - resourceId: table._id!, - level: PermissionLevel.EXECUTE, - }) - - row = await config.createRow() - view = await config.api.viewV2.create({ - tableId: table._id!, - name: generator.guid(), - }) - - perms = await config.api.permission.add({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, - }) }) describe("levels", () => { @@ -72,131 +39,161 @@ describe("/permission", () => { }) }) - describe("add", () => { - it("should be able to add permission to a role for the table", async () => { - expect(perms.length).toEqual(1) - expect(perms[0]._id).toEqual(`${STD_ROLE_ID}`) - }) + describe("table permissions", () => { + let table: Table & { _id: string } + let row: Row + let view: ViewV2 - it("should get the resource permissions", async () => { - const res = await request - .get(`/api/permission/${table._id}`) - .set(config.defaultHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(res.body).toEqual({ - permissions: { - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, - }, + beforeEach(async () => { + mocks.licenses.useCloudFree() + + table = (await config.createTable()) as typeof table + row = await config.createRow() + view = await config.api.viewV2.create({ + tableId: table._id!, + name: generator.guid(), }) - }) - - it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ - roleId: HIGHER_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.WRITE, - }) - const { permissions } = await config.api.permission.get(table._id!) - expect(permissions).toEqual({ - read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, - }) - - const all = await config.api.permission.fetch() - expect(all[table._id!]["read"]).toEqual(STD_ROLE_ID) - expect(all[table._id!]["write"]).toEqual(HIGHER_ROLE_ID) - }) - }) - - describe("remove", () => { - it("should be able to remove the permission", async () => { - const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id!, + resourceId: table._id, level: PermissionLevel.READ, }) - expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id!) - expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() - }) - }) - - describe("check public user allowed", () => { - it("should be able to read the row", async () => { - // replicate changes before checking permissions - await config.publish() - - const res = await request - .get(`/api/${table._id}/rows`) - .set(config.publicHeaders()) - .expect("Content-Type", /json/) - .expect(200) - expect(res.body[0]._id).toEqual(row._id) }) - it("should be able to access the view data when the table is set to public and with no view permissions overrides", async () => { - // Make view inherit table permissions. Needed for backwards compatibility with existing views. - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, + describe("add", () => { + it("should be able to add permission to a role for the table", async () => { + expect(perms.length).toEqual(1) + expect(perms[0]._id).toEqual(`${STD_ROLE_ID}`) }) - // replicate changes before checking permissions - await config.publish() + it("should get the resource permissions", async () => { + const res = await request + .get(`/api/permission/${table._id}`) + .set(config.defaultHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body).toEqual({ + permissions: { + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, + }, + }) + }) - const res = await config.api.viewV2.publicSearch(view.id) - expect(res.rows[0]._id).toEqual(row._id) + it("should get resource permissions with multiple roles", async () => { + perms = await config.api.permission.add({ + roleId: HIGHER_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.WRITE, + }) + const res = await config.api.permission.get(table._id) + expect(res).toEqual({ + permissions: { + read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: HIGHER_ROLE_ID }, + }, + }) + + const allRes = await request + .get(`/api/permission`) + .set(config.defaultHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) + expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + }) }) - it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, + describe("remove", () => { + it("should be able to remove the permission", async () => { + const res = await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + expect(res[0]._id).toEqual(STD_ROLE_ID) + const permsRes = await config.api.permission.get(table._id) + expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) - - // Make view inherit table permissions. Needed for backwards compatibility with existing views. - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, - }) - - // replicate changes before checking permissions - await config.publish() - - await config.api.viewV2.publicSearch(view.id, undefined, { status: 401 }) }) - it("should use the view permissions", async () => { - await config.api.permission.add({ - roleId: STD_ROLE_ID, - resourceId: view.id, - level: PermissionLevel.READ, - }) - await config.api.permission.revoke({ - roleId: STD_ROLE_ID, - resourceId: table._id!, - level: PermissionLevel.READ, - }) - // replicate changes before checking permissions - await config.publish() + describe("check public user allowed", () => { + it("should be able to read the row", async () => { + // replicate changes before checking permissions + await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) - expect(res.rows[0]._id).toEqual(row._id) - }) + const res = await request + .get(`/api/${table._id}/rows`) + .set(config.publicHeaders()) + .expect("Content-Type", /json/) + .expect(200) + expect(res.body[0]._id).toEqual(row._id) + }) - it("shouldn't allow writing from a public user", async () => { - const res = await request - .post(`/api/${table._id!}/rows`) - .send(basicRow(table._id!)) - .set(config.publicHeaders()) - .expect("Content-Type", /json/) - .expect(401) - expect(res.status).toEqual(401) + it("should be able to access the view data when the table is set to public and with no view permissions overrides", async () => { + // Make view inherit table permissions. Needed for backwards compatibility with existing views. + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + + // replicate changes before checking permissions + await config.publish() + + const res = await config.api.viewV2.publicSearch(view.id) + expect(res.rows[0]._id).toEqual(row._id) + }) + + it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + + // Make view inherit table permissions. Needed for backwards compatibility with existing views. + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + + // replicate changes before checking permissions + await config.publish() + + await config.api.viewV2.publicSearch(view.id, undefined, { + status: 401, + }) + }) + + it("should use the view permissions", async () => { + await config.api.permission.add({ + roleId: STD_ROLE_ID, + resourceId: view.id, + level: PermissionLevel.READ, + }) + await config.api.permission.revoke({ + roleId: STD_ROLE_ID, + resourceId: table._id, + level: PermissionLevel.READ, + }) + // replicate changes before checking permissions + await config.publish() + + const res = await config.api.viewV2.publicSearch(view.id) + expect(res.rows[0]._id).toEqual(row._id) + }) + + it("shouldn't allow writing from a public user", async () => { + const res = await request + .post(`/api/${table._id}/rows`) + .send(basicRow(table._id)) + .set(config.publicHeaders()) + .expect("Content-Type", /json/) + .expect(401) + expect(res.status).toEqual(401) + }) }) }) From 38685b50df2a68fb19ac8c0b2e85a3e0530b571d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 13:50:05 +0200 Subject: [PATCH 40/57] Add extra test --- .../src/api/routes/tests/permissions.spec.ts | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index b938bff279..3b45d042c7 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -1,5 +1,5 @@ import { roles } from "@budibase/backend-core" -import { Document, PermissionLevel, Row, Table, ViewV2 } from "@budibase/types" +import { Document, PermissionLevel, Row, ViewV2 } from "@budibase/types" import * as setup from "./utilities" import { generator, mocks } from "@budibase/backend-core/tests" @@ -12,7 +12,6 @@ const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() - let perms: Document[] afterAll(setup.afterAll) @@ -40,26 +39,43 @@ describe("/permission", () => { }) describe("table permissions", () => { - let table: Table & { _id: string } + let tableId: string let row: Row let view: ViewV2 + let perms: Document[] beforeEach(async () => { mocks.licenses.useCloudFree() - table = (await config.createTable()) as typeof table + const table = await config.createTable() + tableId = table._id! row = await config.createRow() view = await config.api.viewV2.create({ - tableId: table._id!, + tableId, name: generator.guid(), }) perms = await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) }) + it("tables should be defaulted to admin", async () => { + const table = await config.createTable() + const { permissions } = await config.api.permission.get(table._id!) + expect(permissions).toEqual({ + read: { + permissionType: "BASE", + role: "BASIC", + }, + write: { + permissionType: "BASE", + role: "BASIC", + }, + }) + }) + describe("add", () => { it("should be able to add permission to a role for the table", async () => { expect(perms.length).toEqual(1) @@ -68,7 +84,7 @@ describe("/permission", () => { it("should get the resource permissions", async () => { const res = await request - .get(`/api/permission/${table._id}`) + .get(`/api/permission/${tableId}`) .set(config.defaultHeaders()) .expect("Content-Type", /json/) .expect(200) @@ -83,10 +99,10 @@ describe("/permission", () => { it("should get resource permissions with multiple roles", async () => { perms = await config.api.permission.add({ roleId: HIGHER_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.WRITE, }) - const res = await config.api.permission.get(table._id) + const res = await config.api.permission.get(tableId) expect(res).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, @@ -99,8 +115,8 @@ describe("/permission", () => { .set(config.defaultHeaders()) .expect("Content-Type", /json/) .expect(200) - expect(allRes.body[table._id]["read"]).toEqual(STD_ROLE_ID) - expect(allRes.body[table._id]["write"]).toEqual(HIGHER_ROLE_ID) + expect(allRes.body[tableId]["read"]).toEqual(STD_ROLE_ID) + expect(allRes.body[tableId]["write"]).toEqual(HIGHER_ROLE_ID) }) }) @@ -108,11 +124,11 @@ describe("/permission", () => { it("should be able to remove the permission", async () => { const res = await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) expect(res[0]._id).toEqual(STD_ROLE_ID) - const permsRes = await config.api.permission.get(table._id) + const permsRes = await config.api.permission.get(tableId) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) }) @@ -123,7 +139,7 @@ describe("/permission", () => { await config.publish() const res = await request - .get(`/api/${table._id}/rows`) + .get(`/api/${tableId}/rows`) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(200) @@ -148,7 +164,7 @@ describe("/permission", () => { it("should not be able to access the view data when the table is not public and there are no view permissions overrides", async () => { await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) @@ -175,7 +191,7 @@ describe("/permission", () => { }) await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: table._id, + resourceId: tableId, level: PermissionLevel.READ, }) // replicate changes before checking permissions @@ -187,8 +203,8 @@ describe("/permission", () => { it("shouldn't allow writing from a public user", async () => { const res = await request - .post(`/api/${table._id}/rows`) - .send(basicRow(table._id)) + .post(`/api/${tableId}/rows`) + .send(basicRow(tableId)) .set(config.publicHeaders()) .expect("Content-Type", /json/) .expect(401) From 56459b27367780fbb739ab68b0be499e67f09b5d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 15:47:35 +0200 Subject: [PATCH 41/57] Type --- packages/backend-core/src/security/permissions.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index 98704f16c6..4ed2cd3954 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -65,7 +65,13 @@ export enum BuiltinPermissionID { POWER = "power", } -export const BUILTIN_PERMISSIONS = { +export const BUILTIN_PERMISSIONS: { + [key in keyof typeof BuiltinPermissionID]: { + _id: (typeof BuiltinPermissionID)[key] + name: string + permissions: Permission[] + } +} = { PUBLIC: { _id: BuiltinPermissionID.PUBLIC, name: "Public", From 923f677bc60f8164d8d41ac5599aa76e5488b082 Mon Sep 17 00:00:00 2001 From: Budibase Staging Release Bot <> Date: Wed, 9 Oct 2024 14:57:15 +0000 Subject: [PATCH 42/57] Bump version to 2.32.15 --- lerna.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lerna.json b/lerna.json index 9b765f17ae..4d36affd04 100644 --- a/lerna.json +++ b/lerna.json @@ -1,6 +1,6 @@ { "$schema": "node_modules/lerna/schemas/lerna-schema.json", - "version": "2.32.14", + "version": "2.32.15", "npmClient": "yarn", "packages": [ "packages/*", From ebd762cdb69a21d3edc5adee018f42bb5d1e730d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:19:18 +0200 Subject: [PATCH 43/57] Fixes --- packages/backend-core/src/security/roles.ts | 4 ++-- packages/server/src/api/controllers/permission.ts | 4 ++-- packages/server/src/api/routes/tests/permissions.spec.ts | 8 +++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 65339832cf..9ae3788aab 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -42,7 +42,7 @@ export class Role implements RoleDoc { _id: string _rev?: string name: string - permissionId: string + permissionId: BuiltinPermissionID inherits?: string version?: string permissions: Record = {} @@ -51,7 +51,7 @@ export class Role implements RoleDoc { constructor( id: string, name: string, - permissionId: string, + permissionId: BuiltinPermissionID, uiMetadata?: RoleUIMetadata ) { this._id = id diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index cbe260367c..a58d94ce80 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -33,7 +33,7 @@ export async function fetch( ) { const db = context.getAppDB() const dbRoles = await sdk.permissions.getAllDBRoles(db) - let permissions: any = {} + let permissions: Record> = {} // create an object with structure role ID -> resource ID -> level for (let role of dbRoles) { if (!role.permissions) { @@ -45,7 +45,7 @@ export async function fetch( } for (let [resource, levelArr] of Object.entries(role.permissions)) { const levels: string[] = Array.isArray(levelArr) ? levelArr : [levelArr] - const perms: Record = {} + const perms: Record = permissions[resource] || {} levels.forEach(level => (perms[level] = roleId!)) permissions[resource] = perms } diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 3b45d042c7..83452b4f1f 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -39,6 +39,8 @@ describe("/permission", () => { }) describe("table permissions", () => { + const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN + let tableId: string let row: Row let view: ViewV2 @@ -67,11 +69,11 @@ describe("/permission", () => { expect(permissions).toEqual({ read: { permissionType: "BASE", - role: "BASIC", + role: DEFAULT_TABLE_ROLE_ID, }, write: { permissionType: "BASE", - role: "BASIC", + role: DEFAULT_TABLE_ROLE_ID, }, }) }) @@ -91,7 +93,7 @@ describe("/permission", () => { expect(res.body).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: HIGHER_ROLE_ID }, + write: { permissionType: "BASE", role: DEFAULT_TABLE_ROLE_ID }, }, }) }) From a0d3bf99931cad51c269d099b8b25c32d9b1ea2b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:25:08 +0200 Subject: [PATCH 44/57] Remove redundant ok button --- .../backend/DataTable/modals/ManageAccessModal.svelte | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte b/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte index 48b584690e..9f5cd9fd95 100644 --- a/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte +++ b/packages/builder/src/components/backend/DataTable/modals/ManageAccessModal.svelte @@ -94,7 +94,7 @@ loadDependantInfo() - + Manage Access {#if requiresPlanToModify} From 37eb66e0d52875dc978cb5367d7773d2bc1b1dbc Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:32:35 +0200 Subject: [PATCH 45/57] Cleanup tests --- .../src/api/routes/tests/permissions.spec.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 83452b4f1f..c8b7597c9d 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -43,19 +43,12 @@ describe("/permission", () => { let tableId: string let row: Row - let view: ViewV2 let perms: Document[] beforeEach(async () => { - mocks.licenses.useCloudFree() - const table = await config.createTable() tableId = table._id! row = await config.createRow() - view = await config.api.viewV2.create({ - tableId, - name: generator.guid(), - }) perms = await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -136,6 +129,15 @@ describe("/permission", () => { }) describe("check public user allowed", () => { + let view: ViewV2 + + beforeEach(async () => { + view = await config.api.viewV2.create({ + tableId, + name: generator.guid(), + }) + }) + it("should be able to read the row", async () => { // replicate changes before checking permissions await config.publish() From 577ab5b6ceaa68c6a58f37b7fcc5fb601b86679b Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:32:56 +0200 Subject: [PATCH 46/57] Fix tests --- packages/backend-core/src/security/permissions.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index 4ed2cd3954..aaf83f78df 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -84,7 +84,8 @@ export const BUILTIN_PERMISSIONS: { name: "Read only", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.READ), - new Permission(PermissionType.TABLE, PermissionLevel.READ), + // TODO: don't add "breaking changes" + // new Permission(PermissionType.TABLE, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), ], }, @@ -93,7 +94,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read/Write", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.WRITE), - new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.LEGACY_VIEW, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), @@ -103,7 +104,7 @@ export const BUILTIN_PERMISSIONS: { _id: BuiltinPermissionID.POWER, name: "Power", permissions: [ - new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.USER, PermissionLevel.READ), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.WEBHOOK, PermissionLevel.READ), From ff402c54e0ad79dde56eb568ee87573197bb56ff Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:48:39 +0200 Subject: [PATCH 47/57] Add view tests --- .../src/api/routes/tests/permissions.spec.ts | 81 ++++++++++++++++--- packages/server/src/sdk/app/views/index.ts | 25 +----- 2 files changed, 69 insertions(+), 37 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index c8b7597c9d..5dfece3126 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -1,5 +1,5 @@ import { roles } from "@budibase/backend-core" -import { Document, PermissionLevel, Row, ViewV2 } from "@budibase/types" +import { Document, PermissionLevel, Row } from "@budibase/types" import * as setup from "./utilities" import { generator, mocks } from "@budibase/backend-core/tests" @@ -9,6 +9,8 @@ const { BUILTIN_ROLE_IDS } = roles const HIGHER_ROLE_ID = BUILTIN_ROLE_IDS.BASIC const STD_ROLE_ID = BUILTIN_ROLE_IDS.PUBLIC +const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN + describe("/permission", () => { let request = setup.getRequest() let config = setup.getConfig() @@ -39,16 +41,12 @@ describe("/permission", () => { }) describe("table permissions", () => { - const DEFAULT_TABLE_ROLE_ID = BUILTIN_ROLE_IDS.ADMIN - let tableId: string - let row: Row let perms: Document[] beforeEach(async () => { const table = await config.createTable() tableId = table._id! - row = await config.createRow() perms = await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -129,13 +127,16 @@ describe("/permission", () => { }) describe("check public user allowed", () => { - let view: ViewV2 + let viewId: string + let row: Row beforeEach(async () => { - view = await config.api.viewV2.create({ + const view = await config.api.viewV2.create({ tableId, name: generator.guid(), }) + viewId = view.id + row = await config.createRow() }) it("should be able to read the row", async () => { @@ -154,14 +155,14 @@ describe("/permission", () => { // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) // replicate changes before checking permissions await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) + const res = await config.api.viewV2.publicSearch(viewId) expect(res.rows[0]._id).toEqual(row._id) }) @@ -175,14 +176,14 @@ describe("/permission", () => { // Make view inherit table permissions. Needed for backwards compatibility with existing views. await config.api.permission.revoke({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) // replicate changes before checking permissions await config.publish() - await config.api.viewV2.publicSearch(view.id, undefined, { + await config.api.viewV2.publicSearch(viewId, undefined, { status: 401, }) }) @@ -190,7 +191,7 @@ describe("/permission", () => { it("should use the view permissions", async () => { await config.api.permission.add({ roleId: STD_ROLE_ID, - resourceId: view.id, + resourceId: viewId, level: PermissionLevel.READ, }) await config.api.permission.revoke({ @@ -201,7 +202,7 @@ describe("/permission", () => { // replicate changes before checking permissions await config.publish() - const res = await config.api.viewV2.publicSearch(view.id) + const res = await config.api.viewV2.publicSearch(viewId) expect(res.rows[0]._id).toEqual(row._id) }) @@ -217,6 +218,60 @@ describe("/permission", () => { }) }) + describe("view permissions", () => { + let tableId: string + let viewId: string + + beforeEach(async () => { + const table = await config.createTable() + tableId = table._id! + + const view = await config.api.viewV2.create({ + tableId, + name: generator.guid(), + }) + viewId = view.id + }) + + it("default permissions inherits the table default value", async () => { + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + write: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) + + it("default permissions inherits explicit table permissions", async () => { + await config.api.permission.add({ + roleId: STD_ROLE_ID, + resourceId: tableId, + level: PermissionLevel.READ, + }) + + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: STD_ROLE_ID, + inheritablePermission: STD_ROLE_ID, + }, + write: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) + }) + describe("fetch builtins", () => { it("should be able to fetch builtin definitions", async () => { const res = await request diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 44f6beedb1..36d6dd6f85 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -3,7 +3,6 @@ import { canGroupBy, FieldType, isNumeric, - PermissionLevel, RelationSchemaField, RenameColumn, Table, @@ -13,7 +12,7 @@ import { ViewV2ColumnEnriched, ViewV2Enriched, } from "@budibase/types" -import { context, docIds, HTTPError, roles } from "@budibase/backend-core" +import { context, docIds, HTTPError } from "@budibase/backend-core" import { helpers, PROTECTED_EXTERNAL_COLUMNS, @@ -26,7 +25,6 @@ import { isExternalTableID } from "../../../integrations/utils" import * as internal from "./internal" import * as external from "./external" import sdk from "../../../sdk" -import { PermissionUpdateType, updatePermissionOnRole } from "../permissions" function pickApi(tableId: any) { if (isExternalTableID(tableId)) { @@ -245,27 +243,6 @@ export async function create( const view = await pickApi(tableId).create(tableId, viewRequest) - // Set permissions to be the same as the table - const tablePerms = await sdk.permissions.getResourcePerms(tableId) - const readRole = tablePerms[PermissionLevel.READ]?.role - const writeRole = tablePerms[PermissionLevel.WRITE]?.role - await updatePermissionOnRole( - { - roleId: readRole || roles.BUILTIN_ROLE_IDS.BASIC, - resourceId: view.id, - level: PermissionLevel.READ, - }, - PermissionUpdateType.ADD - ) - await updatePermissionOnRole( - { - roleId: writeRole || roles.BUILTIN_ROLE_IDS.BASIC, - resourceId: view.id, - level: PermissionLevel.WRITE, - }, - PermissionUpdateType.ADD - ) - return view } From 389c4a94673ec21a6bb37badeaeb81261e1413a9 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Tue, 8 Oct 2024 17:49:45 +0200 Subject: [PATCH 48/57] Add more tests --- .../src/api/routes/tests/permissions.spec.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 5dfece3126..44b57cf7cb 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -270,6 +270,28 @@ describe("/permission", () => { }, }) }) + + it("can sets permissions inherits explicit view permissions", async () => { + await config.api.permission.add({ + roleId: HIGHER_ROLE_ID, + resourceId: viewId, + level: PermissionLevel.WRITE, + }) + + const { permissions } = await config.api.permission.get(viewId) + expect(permissions).toEqual({ + read: { + permissionType: "INHERITED", + role: DEFAULT_TABLE_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + write: { + permissionType: "EXPLICIT", + role: HIGHER_ROLE_ID, + inheritablePermission: DEFAULT_TABLE_ROLE_ID, + }, + }) + }) }) describe("fetch builtins", () => { From b3efea95bfc300347228377490f97f15481f2455 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 12:31:47 +0200 Subject: [PATCH 49/57] Undo base permissions --- packages/backend-core/src/security/permissions.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/backend-core/src/security/permissions.ts b/packages/backend-core/src/security/permissions.ts index aaf83f78df..4ed2cd3954 100644 --- a/packages/backend-core/src/security/permissions.ts +++ b/packages/backend-core/src/security/permissions.ts @@ -84,8 +84,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read only", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.READ), - // TODO: don't add "breaking changes" - // new Permission(PermissionType.TABLE, PermissionLevel.READ), + new Permission(PermissionType.TABLE, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), ], }, @@ -94,7 +93,7 @@ export const BUILTIN_PERMISSIONS: { name: "Read/Write", permissions: [ new Permission(PermissionType.QUERY, PermissionLevel.WRITE), - // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.LEGACY_VIEW, PermissionLevel.READ), new Permission(PermissionType.APP, PermissionLevel.READ), @@ -104,7 +103,7 @@ export const BUILTIN_PERMISSIONS: { _id: BuiltinPermissionID.POWER, name: "Power", permissions: [ - // new Permission(PermissionType.TABLE, PermissionLevel.WRITE), + new Permission(PermissionType.TABLE, PermissionLevel.WRITE), new Permission(PermissionType.USER, PermissionLevel.READ), new Permission(PermissionType.AUTOMATION, PermissionLevel.EXECUTE), new Permission(PermissionType.WEBHOOK, PermissionLevel.READ), From d01462221fb8e8da9e83bc3b5bf171b30a7bff44 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:15:02 +0200 Subject: [PATCH 50/57] Set default permissions --- .../server/src/api/controllers/permission.ts | 9 ++++----- .../src/api/routes/tests/permissions.spec.ts | 20 +++++++------------ .../server/src/sdk/app/permissions/index.ts | 20 +++++++++++++++++++ packages/server/src/sdk/app/tables/create.ts | 8 ++++++++ packages/types/src/api/web/app/permission.ts | 4 ++-- 5 files changed, 41 insertions(+), 20 deletions(-) diff --git a/packages/server/src/api/controllers/permission.ts b/packages/server/src/api/controllers/permission.ts index a58d94ce80..ead48d3db8 100644 --- a/packages/server/src/api/controllers/permission.ts +++ b/packages/server/src/api/controllers/permission.ts @@ -94,18 +94,17 @@ export async function getDependantResources( export async function addPermission(ctx: UserCtx) { const params: AddPermissionRequest = ctx.params - ctx.body = await sdk.permissions.updatePermissionOnRole( - params, - PermissionUpdateType.ADD - ) + await sdk.permissions.updatePermissionOnRole(params, PermissionUpdateType.ADD) + ctx.status = 200 } export async function removePermission( ctx: UserCtx ) { const params: RemovePermissionRequest = ctx.params - ctx.body = await sdk.permissions.updatePermissionOnRole( + await sdk.permissions.updatePermissionOnRole( params, PermissionUpdateType.REMOVE ) + ctx.status = 200 } diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 44b57cf7cb..180f91fb42 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -42,12 +42,11 @@ describe("/permission", () => { describe("table permissions", () => { let tableId: string - let perms: Document[] beforeEach(async () => { const table = await config.createTable() tableId = table._id! - perms = await config.api.permission.add({ + await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, level: PermissionLevel.READ, @@ -59,11 +58,11 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(table._id!) expect(permissions).toEqual({ read: { - permissionType: "BASE", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, }, write: { - permissionType: "BASE", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, }, }) @@ -71,11 +70,6 @@ describe("/permission", () => { describe("add", () => { it("should be able to add permission to a role for the table", async () => { - expect(perms.length).toEqual(1) - expect(perms[0]._id).toEqual(`${STD_ROLE_ID}`) - }) - - it("should get the resource permissions", async () => { const res = await request .get(`/api/permission/${tableId}`) .set(config.defaultHeaders()) @@ -84,13 +78,13 @@ describe("/permission", () => { expect(res.body).toEqual({ permissions: { read: { permissionType: "EXPLICIT", role: STD_ROLE_ID }, - write: { permissionType: "BASE", role: DEFAULT_TABLE_ROLE_ID }, + write: { permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID }, }, }) }) it("should get resource permissions with multiple roles", async () => { - perms = await config.api.permission.add({ + await config.api.permission.add({ roleId: HIGHER_ROLE_ID, resourceId: tableId, level: PermissionLevel.WRITE, @@ -115,12 +109,12 @@ describe("/permission", () => { describe("remove", () => { it("should be able to remove the permission", async () => { - const res = await config.api.permission.revoke({ + await config.api.permission.revoke({ roleId: STD_ROLE_ID, resourceId: tableId, level: PermissionLevel.READ, }) - expect(res[0]._id).toEqual(STD_ROLE_ID) + const permsRes = await config.api.permission.get(tableId) expect(permsRes.permissions[STD_ROLE_ID]).toBeUndefined() }) diff --git a/packages/server/src/sdk/app/permissions/index.ts b/packages/server/src/sdk/app/permissions/index.ts index 2c3c0af95b..97af9ccc83 100644 --- a/packages/server/src/sdk/app/permissions/index.ts +++ b/packages/server/src/sdk/app/permissions/index.ts @@ -185,6 +185,26 @@ export async function updatePermissionOnRole( }) } +export async function setPermissions( + resourceId: string, + { + writeRole, + readRole, + }: { + writeRole: string + readRole: string + } +) { + await updatePermissionOnRole( + { roleId: writeRole, resourceId, level: PermissionLevel.WRITE }, + PermissionUpdateType.ADD + ) + await updatePermissionOnRole( + { roleId: readRole, resourceId, level: PermissionLevel.READ }, + PermissionUpdateType.ADD + ) +} + // utility function to stop this repetition - permissions always stored under roles export async function getAllDBRoles(db: Database) { const body = await db.allDocs( diff --git a/packages/server/src/sdk/app/tables/create.ts b/packages/server/src/sdk/app/tables/create.ts index ed6d6baeb0..0b15cdb15a 100644 --- a/packages/server/src/sdk/app/tables/create.ts +++ b/packages/server/src/sdk/app/tables/create.ts @@ -3,6 +3,8 @@ import { Row, Table } from "@budibase/types" import * as external from "./external" import * as internal from "./internal" import { isExternal } from "./utils" +import { setPermissions } from "../permissions" +import { roles } from "@budibase/backend-core" export async function create( table: Omit, @@ -15,5 +17,11 @@ export async function create( } else { createdTable = await internal.create(table, rows, userId) } + + await setPermissions(createdTable._id!, { + writeRole: roles.BUILTIN_ROLE_IDS.ADMIN, + readRole: roles.BUILTIN_ROLE_IDS.ADMIN, + }) + return createdTable } diff --git a/packages/types/src/api/web/app/permission.ts b/packages/types/src/api/web/app/permission.ts index a5c4df5733..b40310f21c 100644 --- a/packages/types/src/api/web/app/permission.ts +++ b/packages/types/src/api/web/app/permission.ts @@ -25,7 +25,7 @@ export interface AddedPermission { reason?: string } -export type AddPermissionResponse = AddedPermission[] +export interface AddPermissionResponse {} export interface AddPermissionRequest { roleId: string @@ -34,4 +34,4 @@ export interface AddPermissionRequest { } export interface RemovePermissionRequest extends AddPermissionRequest {} -export interface RemovePermissionResponse extends AddPermissionResponse {} +export interface RemovePermissionResponse {} From c84cda40b33c2c082b7dee171fc7c4df7bc3309a Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:36:37 +0200 Subject: [PATCH 51/57] Set default permissions to view --- .../src/api/routes/tests/permissions.spec.ts | 16 ++++++++-------- packages/server/src/sdk/app/views/index.ts | 8 ++++++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/server/src/api/routes/tests/permissions.spec.ts b/packages/server/src/api/routes/tests/permissions.spec.ts index 180f91fb42..a479adb4cf 100644 --- a/packages/server/src/api/routes/tests/permissions.spec.ts +++ b/packages/server/src/api/routes/tests/permissions.spec.ts @@ -227,23 +227,23 @@ describe("/permission", () => { viewId = view.id }) - it("default permissions inherits the table default value", async () => { + it("default permissions inherits and persists the table default value", async () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, write: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, }) }) - it("default permissions inherits explicit table permissions", async () => { + it("does not update view permissions once persisted, even if table permissions change", async () => { await config.api.permission.add({ roleId: STD_ROLE_ID, resourceId: tableId, @@ -253,12 +253,12 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", - role: STD_ROLE_ID, + permissionType: "EXPLICIT", + role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: STD_ROLE_ID, }, write: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, @@ -275,7 +275,7 @@ describe("/permission", () => { const { permissions } = await config.api.permission.get(viewId) expect(permissions).toEqual({ read: { - permissionType: "INHERITED", + permissionType: "EXPLICIT", role: DEFAULT_TABLE_ROLE_ID, inheritablePermission: DEFAULT_TABLE_ROLE_ID, }, diff --git a/packages/server/src/sdk/app/views/index.ts b/packages/server/src/sdk/app/views/index.ts index 36d6dd6f85..629454fecc 100644 --- a/packages/server/src/sdk/app/views/index.ts +++ b/packages/server/src/sdk/app/views/index.ts @@ -3,6 +3,7 @@ import { canGroupBy, FieldType, isNumeric, + PermissionLevel, RelationSchemaField, RenameColumn, Table, @@ -243,6 +244,13 @@ export async function create( const view = await pickApi(tableId).create(tableId, viewRequest) + // Set permissions to be the same as the table + const tablePerms = await sdk.permissions.getResourcePerms(tableId) + await sdk.permissions.setPermissions(view.id, { + writeRole: tablePerms[PermissionLevel.WRITE].role, + readRole: tablePerms[PermissionLevel.READ].role, + }) + return view } From 85ef2f1d2c6e748cf4a7e3d689b292938ecdef7d Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:38:01 +0200 Subject: [PATCH 52/57] Fix build issue --- packages/backend-core/src/security/roles.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/security/roles.ts b/packages/backend-core/src/security/roles.ts index 9ae3788aab..65339832cf 100644 --- a/packages/backend-core/src/security/roles.ts +++ b/packages/backend-core/src/security/roles.ts @@ -42,7 +42,7 @@ export class Role implements RoleDoc { _id: string _rev?: string name: string - permissionId: BuiltinPermissionID + permissionId: string inherits?: string version?: string permissions: Record = {} @@ -51,7 +51,7 @@ export class Role implements RoleDoc { constructor( id: string, name: string, - permissionId: BuiltinPermissionID, + permissionId: string, uiMetadata?: RoleUIMetadata ) { this._id = id From 92372d1a4bbe0a164e91686a8bf1191f1724a449 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 14:48:20 +0200 Subject: [PATCH 53/57] Fix test --- packages/server/src/api/routes/tests/table.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 60e8142522..362ea24f8a 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -203,10 +203,6 @@ describe.each([ permissionType: PermissionSource.EXPLICIT, role: roles.BUILTIN_ROLE_IDS.ADMIN, }, - execute: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, }) }) }) From a8476a1c8bc3b1fb8e27c6b9b3a53f6dc7ae3b73 Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 16:30:48 +0200 Subject: [PATCH 54/57] Fix tests --- packages/server/src/migrations/tests/index.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/migrations/tests/index.spec.ts b/packages/server/src/migrations/tests/index.spec.ts index d06cd37b69..6b3f3314ba 100644 --- a/packages/server/src/migrations/tests/index.spec.ts +++ b/packages/server/src/migrations/tests/index.spec.ts @@ -71,7 +71,7 @@ describe("migrations", () => { expect(events.datasource.created).toHaveBeenCalledTimes(2) expect(events.layout.created).toHaveBeenCalledTimes(1) expect(events.query.created).toHaveBeenCalledTimes(2) - expect(events.role.created).toHaveBeenCalledTimes(2) + expect(events.role.created).toHaveBeenCalledTimes(3) // created roles + admin (created on table creation) expect(events.table.created).toHaveBeenCalledTimes(3) expect(events.view.created).toHaveBeenCalledTimes(2) expect(events.view.calculationCreated).toHaveBeenCalledTimes(1) @@ -82,7 +82,7 @@ describe("migrations", () => { // to make sure caching is working as expected expect( events.processors.analyticsProcessor.processEvent - ).toHaveBeenCalledTimes(23) + ).toHaveBeenCalledTimes(24) // Addtion of of the events above }) }) }) From 6a0f80f28e71891a8ea4c3458b46a66db19839de Mon Sep 17 00:00:00 2001 From: Adria Navarro Date: Wed, 9 Oct 2024 17:13:46 +0200 Subject: [PATCH 55/57] Remove duplicated test --- .../server/src/api/routes/tests/table.spec.ts | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/packages/server/src/api/routes/tests/table.spec.ts b/packages/server/src/api/routes/tests/table.spec.ts index 362ea24f8a..d6c1651ef0 100644 --- a/packages/server/src/api/routes/tests/table.spec.ts +++ b/packages/server/src/api/routes/tests/table.spec.ts @@ -1,4 +1,4 @@ -import { context, docIds, events, roles } from "@budibase/backend-core" +import { context, docIds, events } from "@budibase/backend-core" import { PROTECTED_EXTERNAL_COLUMNS, PROTECTED_INTERNAL_COLUMNS, @@ -21,7 +21,6 @@ import { ViewCalculation, ViewV2Enriched, RowExportFormat, - PermissionSource, } from "@budibase/types" import { checkBuilderEndpoint } from "./utilities/TestFunctions" import * as setup from "./utilities" @@ -190,21 +189,6 @@ describe.each([ ) } ) - - it("should create tables with ADMIN read and write permissions", async () => { - const table = await config.api.table.save(tableForDatasource(datasource)) - const { permissions } = await config.api.permission.get(table._id!) - expect(permissions).toEqual({ - read: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, - write: { - permissionType: PermissionSource.EXPLICIT, - role: roles.BUILTIN_ROLE_IDS.ADMIN, - }, - }) - }) }) describe("update", () => { From 70ab14319d8061fee391d0472e5f9a325d902698 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 16:51:11 +0100 Subject: [PATCH 56/57] Adding test case for removing from many side of relationships in SQL. --- .../api/controllers/row/ExternalRequest.ts | 22 ++++++++++++---- .../server/src/api/routes/tests/row.spec.ts | 26 +++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 62ceedb56b..3510b5ed75 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -4,6 +4,7 @@ import { AutoFieldSubType, AutoReason, Datasource, + DatasourcePlusQueryResponse, FieldSchema, FieldType, FilterType, @@ -557,8 +558,9 @@ export class ExternalRequest { return matchesPrimaryLink } - const matchesSecondayLink = row[linkSecondary] === body?.[linkSecondary] - return matchesPrimaryLink && matchesSecondayLink + const matchesSecondaryLink = + row[linkSecondary] === body?.[linkSecondary] + return matchesPrimaryLink && matchesSecondaryLink } const existingRelationship = rows.find((row: { [key: string]: any }) => @@ -743,9 +745,19 @@ export class ExternalRequest { // aliasing can be disabled fully if desired const aliasing = new sdk.rows.AliasTables(Object.keys(this.tables)) - let response = env.SQL_ALIASING_DISABLE - ? await getDatasourceAndQuery(json) - : await aliasing.queryWithAliasing(json, makeExternalQuery) + let response: DatasourcePlusQueryResponse + // there's a chance after input processing nothing needs updated, so pass over the call + // we might still need to perform other operations like updating the foreign keys on other rows + if ( + this.operation === Operation.UPDATE && + Object.keys(row || {}).length === 0 + ) { + response = [config.row!] + } else { + response = env.SQL_ALIASING_DISABLE + ? await getDatasourceAndQuery(json) + : await aliasing.queryWithAliasing(json, makeExternalQuery) + } // if it's a counting operation there will be no more processing, just return the number if (this.operation === Operation.COUNT) { diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 0995bd3824..6f56916ddc 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1114,6 +1114,32 @@ describe.each([ expect(getResp.user2[0]._id).toEqual(user2._id) }) + it("should be able to remove a relationship from many side", async () => { + const row = await config.api.row.save(otherTable._id!, { + name: "test", + description: "test", + }) + const row2 = await config.api.row.save(otherTable._id!, { + name: "test", + description: "test", + }) + const { _id } = await config.api.row.save(table._id!, { + relationship: [{ _id: row._id }, { _id: row2._id }], + }) + const relatedRow = await config.api.row.get(table._id!, _id!, { + status: 200, + }) + expect(relatedRow.relationship.length).toEqual(2) + await config.api.row.save(table._id!, { + ...relatedRow, + relationship: [{ _id: row._id }], + }) + const afterRelatedRow = await config.api.row.get(table._id!, _id!, { + status: 200, + }) + expect(afterRelatedRow.relationship.length).toEqual(1) + }) + it("should be able to update relationships when both columns are same name", async () => { let row = await config.api.row.save(table._id!, { name: "test", From 00048a2d3e797e710ef640d15b77289035026631 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Wed, 9 Oct 2024 17:04:27 +0100 Subject: [PATCH 57/57] Addressing PR comments. --- packages/backend-core/src/sql/sql.ts | 16 ++++++---------- .../src/api/controllers/row/ExternalRequest.ts | 6 ++++-- packages/server/src/api/routes/tests/row.spec.ts | 1 + 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/packages/backend-core/src/sql/sql.ts b/packages/backend-core/src/sql/sql.ts index 05b38db309..f122ad1c41 100644 --- a/packages/backend-core/src/sql/sql.ts +++ b/packages/backend-core/src/sql/sql.ts @@ -326,17 +326,13 @@ class InternalBuilder { } private parseBody(body: Record) { - try { - for (let [key, value] of Object.entries(body)) { - const { column } = this.splitter.run(key) - const schema = this.table.schema[column] - if (!schema) { - continue - } - body[key] = this.parse(value, schema) + for (let [key, value] of Object.entries(body)) { + const { column } = this.splitter.run(key) + const schema = this.table.schema[column] + if (!schema) { + continue } - } catch (err) { - console.log(err) + body[key] = this.parse(value, schema) } return body } diff --git a/packages/server/src/api/controllers/row/ExternalRequest.ts b/packages/server/src/api/controllers/row/ExternalRequest.ts index 3510b5ed75..56522acb33 100644 --- a/packages/server/src/api/controllers/row/ExternalRequest.ts +++ b/packages/server/src/api/controllers/row/ExternalRequest.ts @@ -671,6 +671,7 @@ export class ExternalRequest { config.includeSqlRelationships === IncludeRelationship.INCLUDE // clean up row on ingress using schema + const unprocessedRow = config.row const processed = this.inputProcessing(row, table) row = processed.row let manyRelationships = processed.manyRelationships @@ -750,9 +751,10 @@ export class ExternalRequest { // we might still need to perform other operations like updating the foreign keys on other rows if ( this.operation === Operation.UPDATE && - Object.keys(row || {}).length === 0 + Object.keys(row || {}).length === 0 && + unprocessedRow ) { - response = [config.row!] + response = [unprocessedRow] } else { response = env.SQL_ALIASING_DISABLE ? await getDatasourceAndQuery(json) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 6f56916ddc..b86ec38d08 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -1138,6 +1138,7 @@ describe.each([ status: 200, }) expect(afterRelatedRow.relationship.length).toEqual(1) + expect(afterRelatedRow.relationship[0]._id).toEqual(row._id) }) it("should be able to update relationships when both columns are same name", async () => {