From 0eaf6b7d425c7895b2b9b713d7dc3ab94f5201a1 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Fri, 9 Aug 2024 15:59:04 +0100 Subject: [PATCH] Make it possible to get flag values from the license. --- packages/backend-core/src/features/index.ts | 40 +- .../src/features/tests/features.spec.ts | 347 +++++++++--------- packages/pro | 2 +- 3 files changed, 211 insertions(+), 178 deletions(-) diff --git a/packages/backend-core/src/features/index.ts b/packages/backend-core/src/features/index.ts index 3f0a6cf6b0..cce407bfb0 100644 --- a/packages/backend-core/src/features/index.ts +++ b/packages/backend-core/src/features/index.ts @@ -1,12 +1,12 @@ import env from "../environment" import * as context from "../context" import { PostHog, PostHogOptions } from "posthog-node" -import { IdentityType } from "@budibase/types" +import { IdentityType, UserCtx } from "@budibase/types" import tracer from "dd-trace" let posthog: PostHog | undefined export function init(opts?: PostHogOptions) { - if (env.POSTHOG_TOKEN) { + if (env.POSTHOG_TOKEN && env.POSTHOG_API_HOST) { posthog = new PostHog(env.POSTHOG_TOKEN, { host: env.POSTHOG_API_HOST, ...opts, @@ -101,17 +101,23 @@ export class FlagSet, T extends { [key: string]: V }> { return this.flags[name as keyof T] !== undefined } - async get(key: K): Promise[K]> { - const flags = await this.fetch() + async get( + key: K, + ctx?: UserCtx + ): Promise[K]> { + const flags = await this.fetch(ctx) return flags[key] } - async isEnabled>(key: K): Promise { - const flags = await this.fetch() + async isEnabled>( + key: K, + ctx?: UserCtx + ): Promise { + const flags = await this.fetch(ctx) return flags[key] } - async fetch(): Promise> { + async fetch(ctx?: UserCtx): Promise> { return await tracer.trace("features.fetch", async span => { const tags: Record = {} @@ -150,6 +156,26 @@ export class FlagSet, T extends { [key: string]: V }> { } } + const license = ctx?.user?.license + if (license) { + for (const feature of license.features) { + const flag = this.flags[feature] + if (!flag) { + continue + } + + if (flags[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-ignore + flags[feature] = true + tags[`flags.${feature}.source`] = "license" + } + } + const identity = context.getIdentity() if (posthog && identity?.type === IdentityType.USER) { const posthogFlags = await posthog.getAllFlagsAndPayloads(identity._id) diff --git a/packages/backend-core/src/features/tests/features.spec.ts b/packages/backend-core/src/features/tests/features.spec.ts index 95819831b2..9c928f5545 100644 --- a/packages/backend-core/src/features/tests/features.spec.ts +++ b/packages/backend-core/src/features/tests/features.spec.ts @@ -1,7 +1,7 @@ -import { IdentityContext, IdentityType } from "@budibase/types" +import { IdentityContext, IdentityType, UserCtx } from "@budibase/types" import { Flag, FlagSet, FlagValues, init } from "../" import { context } from "../.." -import { setEnv, withEnv } from "../../environment" +import environment, { withEnv } from "../../environment" import nodeFetch from "node-fetch" import nock from "nock" @@ -12,207 +12,214 @@ const schema = { } const flags = new FlagSet(schema) +interface TestCase { + it: string + identity?: Partial + environmentFlags?: string + posthogFlags?: PostHogFlags + licenseFlags?: Array + expected?: Partial> + errorMessage?: string | RegExp +} + +interface PostHogFlags { + featureFlags?: Record + featureFlagPayloads?: Record +} + +function mockPosthogFlags(flags: PostHogFlags) { + nock("https://us.i.posthog.com") + .post("/decide/?v=3", body => { + return body.token === "test" && body.distinct_id === "us_1234" + }) + .reply(200, flags) + .persist() +} + describe("feature flags", () => { - interface TestCase { - tenant: string - flags: string - expected: Partial> - } + beforeEach(() => { + nock.cleanAll() + }) it.each([ { - tenant: "tenant1", - flags: "tenant1:TEST_BOOLEAN", + it: "should should find a simple boolean flag in the environment", + environmentFlags: "default:TEST_BOOLEAN", expected: { TEST_BOOLEAN: true }, }, { - tenant: "tenant1", - flags: "tenant1:!TEST_BOOLEAN", + it: "should should find a simple netgative boolean flag in the environment", + environmentFlags: "default:!TEST_BOOLEAN", expected: { TEST_BOOLEAN: false }, }, { - tenant: "tenant1", - flags: "*:TEST_BOOLEAN", + it: "should should match stars in the environment", + environmentFlags: "*:TEST_BOOLEAN", expected: { TEST_BOOLEAN: true }, }, { - tenant: "tenant1", - flags: "tenant2:TEST_BOOLEAN", + it: "should not match a different tenant's flags", + environmentFlags: "otherTenant:TEST_BOOLEAN", expected: { TEST_BOOLEAN: false }, }, { - tenant: "tenant1", - flags: "", + it: "should return the defaults when no flags are set", + expected: flags.defaults(), + }, + { + it: "should fail when an environment flag is not recognised", + environmentFlags: "default:TEST_BOOLEAN,default:FOO", + errorMessage: "Feature: FOO is not an allowed option", + }, + { + it: "should be able to read boolean flags from PostHog", + posthogFlags: { + featureFlags: { TEST_BOOLEAN: true }, + }, + expected: { TEST_BOOLEAN: true }, + }, + { + it: "should be able to read string flags from PostHog", + posthogFlags: { + featureFlags: { TEST_STRING: true }, + featureFlagPayloads: { TEST_STRING: "test" }, + }, + expected: { TEST_STRING: "test" }, + }, + { + it: "should be able to read numeric flags from PostHog", + posthogFlags: { + featureFlags: { TEST_NUMBER: true }, + featureFlagPayloads: { TEST_NUMBER: "123" }, + }, + expected: { TEST_NUMBER: 123 }, + }, + { + it: "should not be able to override a negative environment flag from PostHog", + environmentFlags: "default:!TEST_BOOLEAN", + posthogFlags: { + featureFlags: { TEST_BOOLEAN: true }, + }, + expected: { TEST_BOOLEAN: false }, + }, + { + it: "should not be able to override a positive environment flag from PostHog", + environmentFlags: "default:TEST_BOOLEAN", + posthogFlags: { + featureFlags: { + TEST_BOOLEAN: false, + }, + }, + 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: { + featureFlags: { UNDEFINED: true }, + }, + expected: flags.defaults(), + }, + { + it: "should not error on unrecognised license flag", + licenseFlags: ["UNDEFINED"], expected: flags.defaults(), }, ])( - 'should find flags $expected for $tenant with string "$flags"', - ({ tenant, flags: envFlags, expected }) => - context.doInTenant(tenant, async () => - withEnv({ TENANT_FEATURE_FLAGS: envFlags }, async () => { - const values = await flags.fetch() - expect(values).toMatchObject(expected) + "$it", + async ({ + identity, + environmentFlags, + posthogFlags, + licenseFlags, + expected, + errorMessage, + }) => { + const env: Partial = { + TENANT_FEATURE_FLAGS: environmentFlags, + } - for (const [key, expectedValue] of Object.entries(expected)) { - const value = await flags.get(key as keyof typeof schema) - expect(value).toBe(expectedValue) + if (posthogFlags) { + mockPosthogFlags(posthogFlags) + env.POSTHOG_TOKEN = "test" + 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. + init({ fetch: nodeFetch }) + + const fullIdentity: IdentityContext = { + _id: "us_1234", + tenantId: "default", + type: IdentityType.USER, + email: "test@example.com", + firstName: "Test", + lastName: "User", + ...identity, + } + + await context.doInIdentityContext(fullIdentity, async () => { + if (errorMessage) { + await expect(flags.fetch(ctx as UserCtx)).rejects.toThrow( + errorMessage + ) + } else if (expected) { + const values = await flags.fetch(ctx as UserCtx) + 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 + ) + expect(value).toBe(expectedValue) + } + } else { + throw new Error("No expected value") } }) - ) + }) + } ) - interface FailedTestCase { - tenant: string - flags: string - expected: string | RegExp - } - - it.each([ - { - tenant: "tenant1", - flags: "tenant1:TEST_BOOLEAN,tenant1:FOO", - expected: "Feature: FOO is not an allowed option", - }, - ])( - "should fail with message \"$expected\" for $tenant with string '$flags'", - ({ tenant, flags: envFlags, expected }) => - context.doInTenant(tenant, () => - withEnv({ TENANT_FEATURE_FLAGS: envFlags }, () => - expect(flags.fetch()).rejects.toThrow(expected) - ) - ) - ) - - describe("posthog", () => { + it("should not error if PostHog is down", async () => { const identity: IdentityContext = { _id: "us_1234", - tenantId: "tenant1", + tenantId: "default", type: IdentityType.USER, email: "test@example.com", firstName: "Test", lastName: "User", } - let cleanup: () => void + nock("https://us.i.posthog.com") + .post("/decide/?v=3", body => { + return body.token === "test" && body.distinct_id === "us_1234" + }) + .reply(503) + .persist() - beforeAll(() => { - cleanup = setEnv({ POSTHOG_TOKEN: "test" }) - }) - - afterAll(() => { - cleanup() - }) - - beforeEach(() => { - nock.cleanAll() - - // We need to pass in node-fetch here otherwise nock won't get used - // because posthog-node uses axios under the hood. - init({ fetch: nodeFetch }) - }) - - function mockFlags(flags: { - featureFlags?: Record - featureFlagPayloads?: Record - }) { - nock("https://us.i.posthog.com") - .post("/decide/?v=3", body => { - return body.token === "test" && body.distinct_id === "us_1234" + await withEnv( + { POSTHOG_TOKEN: "test", POSTHOG_API_HOST: "https://us.i.posthog.com" }, + async () => { + await context.doInIdentityContext(identity, async () => { + await flags.fetch() }) - .reply(200, flags) - } - - it("should be able to read flags from posthog", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: true, - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(true) - }) - }) - - it("should be able to read flags from posthog with payloads", async () => { - mockFlags({ - featureFlags: { - TEST_STRING: true, - }, - featureFlagPayloads: { - TEST_STRING: "test payload", - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_STRING).toBe("test payload") - }) - }) - - it("should be able to read flags from posthog with numbers", async () => { - mockFlags({ - featureFlags: { - TEST_NUMBER: true, - }, - featureFlagPayloads: { - TEST_NUMBER: 123, - }, - }) - - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_NUMBER).toBe(123) - }) - }) - - it("should not fail when a flag is not known", async () => { - mockFlags({ - featureFlags: { - _SOME_RANDOM_FLAG: true, - }, - }) - - await context.doInIdentityContext(identity, async () => { - await expect(flags.fetch()).resolves.not.toThrow() - }) - }) - - it("should not override flags set in the environment", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: false, - }, - }) - - await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:TEST_BOOLEAN` }, - async () => { - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(true) - }) - } - ) - }) - - it("should not override flags set in the environment with a ! prefix", async () => { - mockFlags({ - featureFlags: { - TEST_BOOLEAN: true, - }, - }) - - await withEnv( - { TENANT_FEATURE_FLAGS: `${identity.tenantId}:!TEST_BOOLEAN` }, - async () => { - await context.doInIdentityContext(identity, async () => { - const values = await flags.fetch() - expect(values.TEST_BOOLEAN).toBe(false) - }) - } - ) - }) + } + ) }) }) diff --git a/packages/pro b/packages/pro index 7fe713e51a..b91a944c1f 160000 --- a/packages/pro +++ b/packages/pro @@ -1 +1 @@ -Subproject commit 7fe713e51afea77c8024579582a4e1a4ec1b55b3 +Subproject commit b91a944c1f02a3ee43ad22890ad53840eeced8fe