Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions modules/analytics/middlewares/analytics.requireFeatureFlag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/**
* Module dependencies
*/
import FeatureFlagsService from '../services/analytics.featureFlags.service.js';

import responses from '../../../lib/helpers/responses.js';

/**
* Returns Express middleware that gates access based on a PostHog feature flag.
*
* The middleware evaluates the flag for the authenticated user, passing
* organisation context when available. Behaviour by scenario:
*
* - Flag enabled -> next()
* - Flag disabled -> 403
* - Analytics not configured (no PostHog key) -> next() (fail-open so
* projects not using PostHog are never blocked)
*
* @param {string} flagName - PostHog feature flag key
* @returns {Function} Express middleware function
*/
function requireFeatureFlag(flagName) {

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per .github/copilot-instructions.md, every new/modified function should have a JSDoc header. requireFeatureFlagMiddleware is a newly introduced named function but currently has no JSDoc block (only the factory function does). Consider adding a short JSDoc for the middleware function as well (params + async return).

Suggested change
function requireFeatureFlag(flagName) {
function requireFeatureFlag(flagName) {
/**
* Express middleware that enforces the given feature flag for the current request.
*
* @param {import('express').Request} req - Express request object
* @param {import('express').Response} res - Express response object
* @param {import('express').NextFunction} next - Express next middleware callback
* @returns {Promise<void>} Resolves when the middleware has completed processing
*/

Copilot uses AI. Check for mistakes.
return async function requireFeatureFlagMiddleware(req, res, next) {
const distinctId = req.user?._id ? String(req.user._id) : undefined;

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

distinctId is derived only from req.user._id, but elsewhere in the codebase req.user.id is also used. This middleware will incorrectly 401 for authenticated requests where the user object only has id. Consider resolving distinctId from req.user._id || req.user.id (and keep the String() cast), and add a unit test for the id-only case.

Suggested change
const distinctId = req.user?._id ? String(req.user._id) : undefined;
const userId = req.user?._id ?? req.user?.id;
const distinctId = userId ? String(userId) : undefined;

Copilot uses AI. Check for mistakes.
if (!distinctId) {
return responses.error(res, 401, 'Unauthorized', 'Authentication required to evaluate feature flag')();
}

try {
const options = {};
if (req.organization?._id) {
options.groups = { company: String(req.organization._id) };
Comment on lines +31 to +32

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Organization context is read only from req.organization._id. In several places the org identifier may be available as req.organization.id as well, so this can omit group context unexpectedly. Consider using req.organization._id || req.organization.id when building options.groups (and extend the unit tests to cover the id-only org case).

Suggested change
if (req.organization?._id) {
options.groups = { company: String(req.organization._id) };
const organizationId = req.organization?._id || req.organization?.id;
if (organizationId) {
options.groups = { company: String(organizationId) };

Copilot uses AI. Check for mistakes.
}

const enabled = await FeatureFlagsService.isEnabled(flagName, distinctId, options);

// isEnabled returns false both when the flag is off AND when analytics
// is not configured. Distinguish by checking getVariant: undefined
// means not configured (fail-open), while false/string means configured.
if (!enabled) {
const variant = await FeatureFlagsService.getVariant(flagName, distinctId, options);
// undefined -> analytics not configured -> fail-open
if (variant === undefined) return next();

Comment on lines +35 to +44

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The middleware currently performs up to two feature flag evaluations when a flag is disabled (isEnabled then getVariant) in order to detect the “not configured” case. This adds avoidable latency/load in the common “flag off” path. Consider changing FeatureFlagsService.isEnabled to preserve the underlying undefined (not configured) signal (or adding a single call that returns both { enabled, configured }), so the middleware can decide fail-open vs 403 without a second PostHog call.

Suggested change
const enabled = await FeatureFlagsService.isEnabled(flagName, distinctId, options);
// isEnabled returns false both when the flag is off AND when analytics
// is not configured. Distinguish by checking getVariant: undefined
// means not configured (fail-open), while false/string means configured.
if (!enabled) {
const variant = await FeatureFlagsService.getVariant(flagName, distinctId, options);
// undefined -> analytics not configured -> fail-open
if (variant === undefined) return next();
// Use a single feature flag evaluation to determine both whether
// analytics is configured and whether the flag is enabled.
const variant = await FeatureFlagsService.getVariant(flagName, distinctId, options);
// undefined -> analytics not configured -> fail-open
if (variant === undefined) {
return next();
}
// Analytics is configured; treat falsy variants (e.g. false/null)
// as "flag disabled" and deny access. Truthy variants are considered
// enabled and allowed through.
if (!variant) {

Copilot uses AI. Check for mistakes.
return responses.error(res, 403, 'Forbidden', 'Feature not available on your current plan')({
type: 'FEATURE_FLAG_DISABLED',
flag: flagName,
Comment on lines +46 to +47

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responses.error does not merge arbitrary fields from the provided object into the JSON payload; it only uses it to derive status, errorCode (from err.code), and description. Passing { type: 'FEATURE_FLAG_DISABLED', flag: ... } therefore won’t surface either value to clients (and errorCode will stay SERVER_ERROR). If you want a stable client-visible code, pass { code: 'FEATURE_FLAG_DISABLED' } (or an Error with .code) instead; if you need flag in the response body, responses.error would need to be extended to include extra fields.

Suggested change
type: 'FEATURE_FLAG_DISABLED',
flag: flagName,
code: 'FEATURE_FLAG_DISABLED',

Copilot uses AI. Check for mistakes.
});
}

return next();
} catch (err) {
return next(err);
}
};
}

export default requireFeatureFlag;
60 changes: 60 additions & 0 deletions modules/analytics/services/analytics.featureFlags.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/**
* Module dependencies
*/
import AnalyticsService from './analytics.service.js';

Comment on lines +4 to +5

Copilot AI Mar 25, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR title/description mention "local eval", and modules/analytics/config/analytics.development.config.js defines a posthog.personalApiKey, but the current AnalyticsService init only reads { apiKey, host } and does not pass personalApiKey / local-eval options to the PostHog client. As written, this wrapper will rely on whatever evaluation mode AnalyticsService currently uses. Either update the analytics client init to wire through personalApiKey (and local eval config), or adjust the PR description to avoid claiming local evaluation.

Copilot uses AI. Check for mistakes.
/**
* Check whether a feature flag is enabled for a given user.
* Builds the PostHog options object from the provided context so callers
* don't need to know the PostHog SDK shape.
*
* Returns `false` when analytics is not configured (safe default) so that
* downstream projects without PostHog are never blocked.
*
* @param {string} flag - Feature flag key
* @param {string} distinctId - User identifier
* @param {Object} [options] - Evaluation context
* @param {Object} [options.personProperties] - Properties for local evaluation
* @param {Object} [options.groups] - Group identifiers (e.g. { company: orgId })
* @param {Object} [options.groupProperties] - Properties per group type
* @returns {Promise<boolean>} true when the flag is enabled, false otherwise
*/
const isEnabled = async (flag, distinctId, options = {}) => {
const { personProperties, groups, groupProperties } = options;
const phOptions = {};
if (personProperties) phOptions.personProperties = personProperties;
if (groups) phOptions.groups = groups;
if (groupProperties) phOptions.groupProperties = groupProperties;

const result = await AnalyticsService.isFeatureEnabled(flag, distinctId, phOptions);
// Normalise undefined (not configured) to false for a safe default
return result === true;
};

/**
* Get the variant value of a feature flag for a given user.
* Returns the variant key string, a boolean, or `undefined` when analytics
* is not configured.
*
* @param {string} flag - Feature flag key
* @param {string} distinctId - User identifier
* @param {Object} [options] - Evaluation context
* @param {Object} [options.personProperties] - Properties for local evaluation
* @param {Object} [options.groups] - Group identifiers (e.g. { company: orgId })
* @param {Object} [options.groupProperties] - Properties per group type
* @returns {Promise<string|boolean|undefined>} Variant value
*/
const getVariant = async (flag, distinctId, options = {}) => {
const { personProperties, groups, groupProperties } = options;
const phOptions = {};
if (personProperties) phOptions.personProperties = personProperties;
if (groups) phOptions.groups = groups;
if (groupProperties) phOptions.groupProperties = groupProperties;

return AnalyticsService.getFeatureFlag(flag, distinctId, phOptions);
};

export default {
isEnabled,
getVariant,
};
126 changes: 126 additions & 0 deletions modules/analytics/tests/analytics.featureFlags.service.unit.tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/**
* Module dependencies.
*/
import { jest, beforeEach, afterEach, describe, test, expect } from '@jest/globals';

/**
* Unit tests for analytics feature flags service
*/
describe('Analytics feature flags service unit tests:', () => {
let FeatureFlagsService;
let mockAnalyticsService;

beforeEach(async () => {
jest.resetModules();

mockAnalyticsService = {
isFeatureEnabled: jest.fn(),
getFeatureFlag: jest.fn(),
};

jest.unstable_mockModule('../services/analytics.service.js', () => ({
default: mockAnalyticsService,
}));

const mod = await import('../services/analytics.featureFlags.service.js');
FeatureFlagsService = mod.default;
});

afterEach(() => {
jest.restoreAllMocks();
});

describe('isEnabled', () => {
test('should return true when flag is enabled', async () => {
mockAnalyticsService.isFeatureEnabled.mockResolvedValue(true);

const result = await FeatureFlagsService.isEnabled('my-flag', 'user-1');

expect(result).toBe(true);
expect(mockAnalyticsService.isFeatureEnabled).toHaveBeenCalledWith('my-flag', 'user-1', {});
});

test('should return false when flag is disabled', async () => {
mockAnalyticsService.isFeatureEnabled.mockResolvedValue(false);

const result = await FeatureFlagsService.isEnabled('my-flag', 'user-1');

expect(result).toBe(false);
});

test('should return false when analytics is not configured (undefined)', async () => {
mockAnalyticsService.isFeatureEnabled.mockResolvedValue(undefined);

const result = await FeatureFlagsService.isEnabled('my-flag', 'user-1');

expect(result).toBe(false);
});

test('should pass personProperties and groups to analytics service', async () => {
mockAnalyticsService.isFeatureEnabled.mockResolvedValue(true);

await FeatureFlagsService.isEnabled('my-flag', 'user-1', {
personProperties: { plan: 'pro' },
groups: { company: 'org-1' },
groupProperties: { company: { plan: 'enterprise' } },
});

expect(mockAnalyticsService.isFeatureEnabled).toHaveBeenCalledWith('my-flag', 'user-1', {
personProperties: { plan: 'pro' },
groups: { company: 'org-1' },
groupProperties: { company: { plan: 'enterprise' } },
});
});

test('should omit undefined option keys from PostHog options', async () => {
mockAnalyticsService.isFeatureEnabled.mockResolvedValue(false);

await FeatureFlagsService.isEnabled('my-flag', 'user-1', {});

expect(mockAnalyticsService.isFeatureEnabled).toHaveBeenCalledWith('my-flag', 'user-1', {});
});
});

describe('getVariant', () => {
test('should return variant string when flag has multivariate value', async () => {
mockAnalyticsService.getFeatureFlag.mockResolvedValue('variant-a');

const result = await FeatureFlagsService.getVariant('my-flag', 'user-1');

expect(result).toBe('variant-a');
expect(mockAnalyticsService.getFeatureFlag).toHaveBeenCalledWith('my-flag', 'user-1', {});
});

test('should return boolean when flag is a simple toggle', async () => {
mockAnalyticsService.getFeatureFlag.mockResolvedValue(false);

const result = await FeatureFlagsService.getVariant('my-flag', 'user-1');

expect(result).toBe(false);
});

test('should return undefined when analytics is not configured', async () => {
mockAnalyticsService.getFeatureFlag.mockResolvedValue(undefined);

const result = await FeatureFlagsService.getVariant('my-flag', 'user-1');

expect(result).toBeUndefined();
});

test('should pass personProperties, groups, and groupProperties', async () => {
mockAnalyticsService.getFeatureFlag.mockResolvedValue('variant-b');

await FeatureFlagsService.getVariant('my-flag', 'user-1', {
personProperties: { email: 'a@b.com' },
groups: { company: 'org-1' },
groupProperties: { company: { name: 'Acme' } },
});

expect(mockAnalyticsService.getFeatureFlag).toHaveBeenCalledWith('my-flag', 'user-1', {
personProperties: { email: 'a@b.com' },
groups: { company: 'org-1' },
groupProperties: { company: { name: 'Acme' } },
});
});
});
});
123 changes: 123 additions & 0 deletions modules/analytics/tests/analytics.requireFeatureFlag.unit.tests.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* Module dependencies.
*/
import { jest, beforeEach, afterEach, describe, test, expect } from '@jest/globals';

/**
* Unit tests for requireFeatureFlag middleware
*/
describe('requireFeatureFlag middleware unit tests:', () => {
let requireFeatureFlag;
let mockFeatureFlagsService;
let req;
let res;
let next;

beforeEach(async () => {
jest.resetModules();

mockFeatureFlagsService = {
isEnabled: jest.fn(),
getVariant: jest.fn(),
};

jest.unstable_mockModule('../services/analytics.featureFlags.service.js', () => ({
default: mockFeatureFlagsService,
}));

const mod = await import('../middlewares/analytics.requireFeatureFlag.js');
requireFeatureFlag = mod.default;

req = {
user: { _id: 'user-123' },
organization: { _id: 'org-456' },
};

res = {
status: jest.fn().mockReturnThis(),
json: jest.fn().mockReturnThis(),
};

next = jest.fn();
});

afterEach(() => {
jest.restoreAllMocks();
});

test('should call next when flag is enabled', async () => {
mockFeatureFlagsService.isEnabled.mockResolvedValue(true);

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(next).toHaveBeenCalledWith();
expect(mockFeatureFlagsService.isEnabled).toHaveBeenCalledWith(
'beta-feature',
'user-123',
{ groups: { company: 'org-456' } },
);
});

test('should return 403 when flag is disabled', async () => {
mockFeatureFlagsService.isEnabled.mockResolvedValue(false);
mockFeatureFlagsService.getVariant.mockResolvedValue(false);

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(next).not.toHaveBeenCalled();
expect(res.status).toHaveBeenCalledWith(403);
expect(res.json).toHaveBeenCalledWith(
expect.objectContaining({
type: 'error',
message: 'Forbidden',
}),
);
});

test('should call next when analytics is not configured (fail-open)', async () => {
mockFeatureFlagsService.isEnabled.mockResolvedValue(false);
mockFeatureFlagsService.getVariant.mockResolvedValue(undefined);

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(next).toHaveBeenCalledWith();
});

test('should return 401 when user is not authenticated', async () => {
req.user = undefined;

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(next).not.toHaveBeenCalled();
expect(res.status).toHaveBeenCalledWith(401);
});

test('should not include groups when organization is absent', async () => {
req.organization = undefined;
mockFeatureFlagsService.isEnabled.mockResolvedValue(true);

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(mockFeatureFlagsService.isEnabled).toHaveBeenCalledWith(
'beta-feature',
'user-123',
{},
);
expect(next).toHaveBeenCalledWith();
});

test('should call next(err) on unexpected errors', async () => {
const error = new Error('PostHog timeout');
mockFeatureFlagsService.isEnabled.mockRejectedValue(error);

const middleware = requireFeatureFlag('beta-feature');
await middleware(req, res, next);

expect(next).toHaveBeenCalledWith(error);
});
});
Loading