From 20f55e379581c497b91f9578954e965c8e3bc223 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 12:29:17 +0100 Subject: [PATCH 1/6] 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 eee2991b098bfc55d87aa032d02d372eb86b387d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 12:57:14 +0100 Subject: [PATCH 2/6] 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 3/6] 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 4/6] 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 5/6] 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 9d70b123e5bc2d8734a0f928b35944e2d3acf61f Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Wed, 9 Oct 2024 15:22:38 +0100 Subject: [PATCH 6/6] 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") }