From 225d8a91f19e3cbedc57d34095188b79e7adac15 Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Mon, 8 Jul 2024 09:52:10 +0200 Subject: [PATCH] API tokens scoped to deleted projects shouldn't give wildcard access (#7499) If you have SDK tokens scoped to projects that are deleted, you should not get access to any flags with those. --------- Co-authored-by: David Leek --- .../__snapshots__/create-config.test.ts.snap | 1 + src/lib/db/api-token-store.ts | 106 ++++++++---- src/lib/db/index.ts | 7 +- .../api-tokens/createApiTokenService.ts | 7 +- .../createInstanceStatsService.ts | 7 +- src/lib/types/experimental.ts | 5 + .../e2e/api/admin/api-token.auth.e2e.test.ts | 46 +++--- src/test/e2e/api/admin/api-token.e2e.test.ts | 4 +- .../feature.token.deleted.project.e2e.test.ts | 153 ++++++++++++++++++ .../e2e/services/edge-service.e2e.test.ts | 4 +- 10 files changed, 276 insertions(+), 64 deletions(-) create mode 100644 src/test/e2e/api/client/feature.token.deleted.project.e2e.test.ts diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 312b572a1a..5d7dd96361 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -76,6 +76,7 @@ exports[`should create default config 1`] = ` "flagResolver": FlagResolver { "experiments": { "adminTokenKillSwitch": false, + "allowOrphanedWildcardTokens": false, "anonymiseEventLog": false, "anonymizeProjectOwners": false, "automatedActions": false, diff --git a/src/lib/db/api-token-store.ts b/src/lib/db/api-token-store.ts index 57838bc5e0..9eb7333d16 100644 --- a/src/lib/db/api-token-store.ts +++ b/src/lib/db/api-token-store.ts @@ -5,7 +5,7 @@ import type { Logger, LogProvider } from '../logger'; import NotFoundError from '../error/notfound-error'; import type { IApiTokenStore } from '../types/stores/api-token-store'; import { - type ApiTokenType, + ApiTokenType, type IApiToken, type IApiTokenCreate, isAllProjects, @@ -13,6 +13,7 @@ import { import { ALL_PROJECTS } from '../util/constants'; import type { Db } from './db'; import { inTransaction } from './transaction'; +import type { IFlagResolver } from '../types'; const TABLE = 'api_tokens'; const API_LINK_TABLE = 'api_token_project'; @@ -35,33 +36,44 @@ interface ITokenRow extends ITokenInsert { project: string; } -const tokenRowReducer = (acc, tokenRow) => { - const { project, ...token } = tokenRow; - if (!acc[tokenRow.secret]) { - acc[tokenRow.secret] = { - secret: token.secret, - tokenName: token.token_name ? token.token_name : token.username, - type: token.type.toLowerCase(), - project: ALL, - projects: [ALL], - environment: token.environment ? token.environment : ALL, - expiresAt: token.expires_at, - createdAt: token.created_at, - alias: token.alias, - seenAt: token.seen_at, - username: token.token_name ? token.token_name : token.username, - }; - } - const currentToken = acc[tokenRow.secret]; - if (tokenRow.project) { - if (isAllProjects(currentToken.projects)) { - currentToken.projects = []; +const createTokenRowReducer = + (allowOrphanedWildcardTokens: boolean) => (acc, tokenRow) => { + const { project, ...token } = tokenRow; + if (!acc[tokenRow.secret]) { + if ( + !allowOrphanedWildcardTokens && + !tokenRow.project && + !tokenRow.secret.startsWith('*:') && + (tokenRow.type === ApiTokenType.CLIENT || + tokenRow.type === ApiTokenType.FRONTEND) + ) { + return acc; + } + + acc[tokenRow.secret] = { + secret: token.secret, + tokenName: token.token_name ? token.token_name : token.username, + type: token.type.toLowerCase(), + project: ALL, + projects: [ALL], + environment: token.environment ? token.environment : ALL, + expiresAt: token.expires_at, + createdAt: token.created_at, + alias: token.alias, + seenAt: token.seen_at, + username: token.token_name ? token.token_name : token.username, + }; } - currentToken.projects.push(tokenRow.project); - currentToken.project = currentToken.projects.join(','); - } - return acc; -}; + const currentToken = acc[tokenRow.secret]; + if (tokenRow.project) { + if (isAllProjects(currentToken.projects)) { + currentToken.projects = []; + } + currentToken.projects.push(tokenRow.project); + currentToken.project = currentToken.projects.join(','); + } + return acc; + }; const toRow = (newToken: IApiTokenCreate) => ({ username: newToken.tokenName ?? newToken.username, @@ -74,8 +86,14 @@ const toRow = (newToken: IApiTokenCreate) => ({ alias: newToken.alias || null, }); -const toTokens = (rows: any[]): IApiToken[] => { - const tokens = rows.reduce(tokenRowReducer, {}); +const toTokens = ( + rows: any[], + allowOrphanedWildcardTokens: boolean, +): IApiToken[] => { + const tokens = rows.reduce( + createTokenRowReducer(allowOrphanedWildcardTokens), + {}, + ); return Object.values(tokens); }; @@ -86,7 +104,14 @@ export class ApiTokenStore implements IApiTokenStore { private db: Db; - constructor(db: Db, eventBus: EventEmitter, getLogger: LogProvider) { + private readonly flagResolver: IFlagResolver; + + constructor( + db: Db, + eventBus: EventEmitter, + getLogger: LogProvider, + flagResolver: IFlagResolver, + ) { this.db = db; this.logger = getLogger('api-tokens.js'); this.timer = (action: string) => @@ -94,6 +119,7 @@ export class ApiTokenStore implements IApiTokenStore { store: 'api-tokens', action, }); + this.flagResolver = flagResolver; } async count(): Promise { @@ -120,7 +146,10 @@ export class ApiTokenStore implements IApiTokenStore { const stopTimer = this.timer('getAll'); const rows = await this.makeTokenProjectQuery(); stopTimer(); - return toTokens(rows); + const allowOrphanedWildcardTokens = this.flagResolver.isEnabled( + 'allowOrphanedWildcardTokens', + ); + return toTokens(rows, allowOrphanedWildcardTokens); } async getAllActive(): Promise { @@ -129,7 +158,10 @@ export class ApiTokenStore implements IApiTokenStore { .where('expires_at', 'IS', null) .orWhere('expires_at', '>', 'now()'); stopTimer(); - return toTokens(rows); + const allowOrphanedWildcardTokens = this.flagResolver.isEnabled( + 'allowOrphanedWildcardTokens', + ); + return toTokens(rows, allowOrphanedWildcardTokens); } private makeTokenProjectQuery() { @@ -200,7 +232,10 @@ export class ApiTokenStore implements IApiTokenStore { key, ); stopTimer(); - return toTokens(row)[0]; + const allowOrphanedWildcardTokens = this.flagResolver.isEnabled( + 'allowOrphanedWildcardTokens', + ); + return toTokens(row, allowOrphanedWildcardTokens)[0]; } async delete(secret: string): Promise { @@ -217,7 +252,10 @@ export class ApiTokenStore implements IApiTokenStore { .where({ secret }) .returning('*'); if (rows.length > 0) { - return toTokens(rows)[0]; + const allowOrphanedWildcardTokens = this.flagResolver.isEnabled( + 'allowOrphanedWildcardTokens', + ); + return toTokens(rows, allowOrphanedWildcardTokens)[0]; } throw new NotFoundError('Could not find api-token.'); } diff --git a/src/lib/db/index.ts b/src/lib/db/index.ts index d7e97cf6db..6f0e8bb096 100644 --- a/src/lib/db/index.ts +++ b/src/lib/db/index.ts @@ -97,7 +97,12 @@ export const createStores = ( tagTypeStore: new TagTypeStore(db, eventBus, getLogger), addonStore: new AddonStore(db, eventBus, getLogger), accessStore: new AccessStore(db, eventBus, getLogger), - apiTokenStore: new ApiTokenStore(db, eventBus, getLogger), + apiTokenStore: new ApiTokenStore( + db, + eventBus, + getLogger, + config.flagResolver, + ), resetTokenStore: new ResetTokenStore(db, eventBus, getLogger), sessionStore: new SessionStore(db, eventBus, getLogger), userFeedbackStore: new UserFeedbackStore(db, eventBus, getLogger), diff --git a/src/lib/features/api-tokens/createApiTokenService.ts b/src/lib/features/api-tokens/createApiTokenService.ts index 5a834509a6..18b236c628 100644 --- a/src/lib/features/api-tokens/createApiTokenService.ts +++ b/src/lib/features/api-tokens/createApiTokenService.ts @@ -15,7 +15,12 @@ export const createApiTokenService = ( config: IUnleashConfig, ): ApiTokenService => { const { eventBus, getLogger } = config; - const apiTokenStore = new ApiTokenStore(db, eventBus, getLogger); + const apiTokenStore = new ApiTokenStore( + db, + eventBus, + getLogger, + config.flagResolver, + ); const environmentStore = new EnvironmentStore(db, eventBus, getLogger); const eventService = createEventsService(db, config); diff --git a/src/lib/features/instance-stats/createInstanceStatsService.ts b/src/lib/features/instance-stats/createInstanceStatsService.ts index 0d133e781e..ed73b81b6f 100644 --- a/src/lib/features/instance-stats/createInstanceStatsService.ts +++ b/src/lib/features/instance-stats/createInstanceStatsService.ts @@ -78,7 +78,12 @@ export const createInstanceStatsService = (db: Db, config: IUnleashConfig) => { getLogger, ); const eventStore = new EventStore(db, getLogger); - const apiTokenStore = new ApiTokenStore(db, eventBus, getLogger); + const apiTokenStore = new ApiTokenStore( + db, + eventBus, + getLogger, + flagResolver, + ); const clientMetricsStoreV2 = new ClientMetricsStoreV2( db, getLogger, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 4d2097d30a..75e6f76b58 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -63,6 +63,7 @@ export type IFlagKey = | 'flagCreator' | 'anonymizeProjectOwners' | 'resourceLimits' + | 'allowOrphanedWildcardTokens' | 'extendedMetrics'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -300,6 +301,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_RESOURCE_LIMITS, false, ), + allowOrphanedWildcardTokens: parseEnvVarBoolean( + process.env.UNLEASH_ORPHANED_TOKENS_KILL_SWITCH, + false, + ), extendedMetrics: parseEnvVarBoolean( process.env.UNLEASH_EXPERIMENTAL_EXTENDED_METRICS, false, diff --git a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts index 67404d10da..692137bce3 100644 --- a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts @@ -79,7 +79,7 @@ test('editor users should only get client or frontend tokens', async () => { projects: [], tokenName: '', username: 'test', - secret: '1234', + secret: '*:environment.1234', type: ApiTokenType.CLIENT, }); @@ -88,7 +88,7 @@ test('editor users should only get client or frontend tokens', async () => { projects: [], tokenName: '', username: 'frontend', - secret: '12345', + secret: '*:environment.12345', type: ApiTokenType.FRONTEND, }); @@ -97,7 +97,7 @@ test('editor users should only get client or frontend tokens', async () => { projects: [], tokenName: '', username: 'test', - secret: 'sdfsdf2d', + secret: '*:*.sdfsdf2d', type: ApiTokenType.ADMIN, }); @@ -141,7 +141,7 @@ test('viewer users should not be allowed to fetch tokens', async () => { projects: [], tokenName: '', username: 'test', - secret: '1234', + secret: '*:environment.1234', type: ApiTokenType.CLIENT, }); @@ -150,7 +150,7 @@ test('viewer users should not be allowed to fetch tokens', async () => { projects: [], tokenName: '', username: 'test', - secret: 'sdfsdf2d', + secret: '*:*.sdfsdf2d', type: ApiTokenType.ADMIN, }); @@ -553,7 +553,7 @@ describe('Fine grained API token permissions', () => { tokenName: '', username: 'client', - secret: 'client_secret', + secret: '*:environment.client_secret', type: ApiTokenType.CLIENT, }); @@ -562,7 +562,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'admin', - secret: 'sdfsdf2admin_secret', + secret: '*:*.sdfsdf2admin_secret', type: ApiTokenType.ADMIN, }); await stores.apiTokenStore.insert({ @@ -570,7 +570,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontender', - secret: 'sdfsdf2dfrontend_Secret', + secret: '*:environment:sdfsdf2dfrontend_Secret', type: ApiTokenType.FRONTEND, }); await request @@ -637,7 +637,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'client', - secret: 'client_secret_1234', + secret: '*:environment.client_secret_1234', type: ApiTokenType.CLIENT, }); @@ -646,7 +646,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'admin', - secret: 'admin_secret_1234', + secret: '*:*.admin_secret_1234', type: ApiTokenType.ADMIN, }); await stores.apiTokenStore.insert({ @@ -654,7 +654,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontender', - secret: 'frontend_secret_1234', + secret: '*:environment.frontend_secret_1234', type: ApiTokenType.FRONTEND, }); await request @@ -699,7 +699,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'client', - secret: 'client_secret_4321', + secret: '*:environment.client_secret_4321', type: ApiTokenType.CLIENT, }); @@ -708,7 +708,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'admin', - secret: 'admin_secret_4321', + secret: '*:*.admin_secret_4321', type: ApiTokenType.ADMIN, }); await stores.apiTokenStore.insert({ @@ -716,7 +716,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontender', - secret: 'frontend_secret_4321', + secret: '*:environment.frontend_secret_4321', type: ApiTokenType.FRONTEND, }); await request @@ -760,7 +760,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'client', - secret: 'client_secret_4321', + secret: '*:environment.client_secret_4321', type: ApiTokenType.CLIENT, }); await stores.apiTokenStore.insert({ @@ -768,7 +768,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'admin', - secret: 'admin_secret_4321', + secret: '*:*.admin_secret_4321', type: ApiTokenType.ADMIN, }); await stores.apiTokenStore.insert({ @@ -776,7 +776,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontender', - secret: 'frontend_secret_4321', + secret: '*:environment.frontend_secret_4321', type: ApiTokenType.FRONTEND, }); await request @@ -848,7 +848,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'cilent', - secret: 'update_client_token', + secret: '*:environment.update_client_token', type: ApiTokenType.CLIENT, }); await request @@ -910,7 +910,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontend', - secret: 'update_frontend_token', + secret: '*:environment.update_frontend_token', type: ApiTokenType.FRONTEND, }); await request @@ -973,7 +973,7 @@ describe('Fine grained API token permissions', () => { tokenName: '', username: 'admin', - secret: 'update_admin_token', + secret: '*:*.update_admin_token', type: ApiTokenType.ADMIN, }); await request @@ -1038,7 +1038,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'cilent', - secret: 'delete_client_token', + secret: '*:environment.delete_client_token', type: ApiTokenType.CLIENT, }); await request @@ -1100,7 +1100,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'frontend', - secret: 'delete_frontend_token', + secret: '*:environment.delete_frontend_token', type: ApiTokenType.FRONTEND, }); await request @@ -1161,7 +1161,7 @@ describe('Fine grained API token permissions', () => { projects: [], tokenName: '', username: 'admin', - secret: 'delete_admin_token', + secret: '*:*:delete_admin_token', type: ApiTokenType.ADMIN, }); await request diff --git a/src/test/e2e/api/admin/api-token.e2e.test.ts b/src/test/e2e/api/admin/api-token.e2e.test.ts index 93d120d13e..744b942717 100644 --- a/src/test/e2e/api/admin/api-token.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.e2e.test.ts @@ -122,7 +122,7 @@ test('creates new admin token with expiry', async () => { }); test('update client token with expiry', async () => { - const tokenSecret = 'random-secret-update'; + const tokenSecret = '*:environment.random-secret-update'; await db.stores.apiTokenStore.insert({ username: 'test', @@ -187,7 +187,7 @@ test('creates a lot of client tokens', async () => { }); test('removes api token', async () => { - const tokenSecret = 'random-secret'; + const tokenSecret = '*:environment.random-secret'; await db.stores.apiTokenStore.insert({ environment: 'development', diff --git a/src/test/e2e/api/client/feature.token.deleted.project.e2e.test.ts b/src/test/e2e/api/client/feature.token.deleted.project.e2e.test.ts new file mode 100644 index 0000000000..7dbe545671 --- /dev/null +++ b/src/test/e2e/api/client/feature.token.deleted.project.e2e.test.ts @@ -0,0 +1,153 @@ +import { type IUnleashTest, setupAppWithAuth } from '../../helpers/test-helper'; +import dbInit, { type ITestDb } from '../../helpers/database-init'; +import getLogger from '../../../fixtures/no-logger'; +import type { ApiTokenService } from '../../../../lib/services/api-token-service'; +import { ApiTokenType } from '../../../../lib/types/models/api-token'; +import { DEFAULT_ENV } from '../../../../lib/util/constants'; +import { TEST_AUDIT_USER } from '../../../../lib/types'; +import { User } from '../../../../lib/server-impl'; + +let app: IUnleashTest; +let db: ITestDb; + +let apiTokenService: ApiTokenService; + +const environment = 'testing'; +const project = 'default'; +const project2 = 'some'; +const deletionProject = 'deletion'; +const deletionTokenName = 'delete'; +const feature1 = 'f1.token.access'; +const feature2 = 'f2.token.access'; +const feature3 = 'f3.p2.token.access'; + +beforeAll(async () => { + db = await dbInit('feature_api_api_access_client_deletion', getLogger); + app = await setupAppWithAuth(db.stores, {}, db.rawDatabase); + apiTokenService = app.services.apiTokenService; + + const { featureToggleServiceV2, environmentService } = app.services; + const { environmentStore, projectStore } = db.stores; + + await environmentStore.create({ + name: environment, + type: 'test', + }); + + await projectStore.create({ + id: project2, + name: 'Test Project 2', + description: '', + mode: 'open' as const, + }); + + await projectStore.create({ + id: deletionProject, + name: 'Deletion Project', + description: '', + mode: 'open' as const, + }); + + await environmentService.addEnvironmentToProject( + environment, + project, + TEST_AUDIT_USER, + ); + await environmentService.addEnvironmentToProject( + environment, + project2, + TEST_AUDIT_USER, + ); + + await featureToggleServiceV2.createFeatureToggle( + project, + { + name: feature1, + description: 'the #1 feature', + }, + TEST_AUDIT_USER, + ); + + await featureToggleServiceV2.createStrategy( + { + name: 'default', + constraints: [], + parameters: {}, + }, + { projectId: project, featureName: feature1, environment: DEFAULT_ENV }, + TEST_AUDIT_USER, + ); + await featureToggleServiceV2.createStrategy( + { + name: 'default', + constraints: [], + parameters: {}, + }, + { projectId: project, featureName: feature1, environment }, + TEST_AUDIT_USER, + ); + + // create feature 2 + await featureToggleServiceV2.createFeatureToggle( + project, + { + name: feature2, + }, + TEST_AUDIT_USER, + ); + await featureToggleServiceV2.createStrategy( + { + name: 'default', + constraints: [], + parameters: {}, + }, + { projectId: project, featureName: feature2, environment }, + TEST_AUDIT_USER, + ); + + // create feature 3 + await featureToggleServiceV2.createFeatureToggle( + project2, + { + name: feature3, + }, + TEST_AUDIT_USER, + ); + await featureToggleServiceV2.createStrategy( + { + name: 'default', + constraints: [], + parameters: {}, + }, + { projectId: project2, featureName: feature3, environment }, + TEST_AUDIT_USER, + ); +}); + +afterAll(async () => { + await app.destroy(); + await db.destroy(); +}); + +test('doesnt return feature flags if project deleted', async () => { + const token = await apiTokenService.createApiToken({ + type: ApiTokenType.CLIENT, + tokenName: deletionTokenName, + environment, + project: deletionProject, + }); + + await app.services.projectService.deleteProject( + deletionProject, + new User(TEST_AUDIT_USER), + TEST_AUDIT_USER, + ); + + await app.services.apiTokenService.fetchActiveTokens(); + + await app.request + .get('/api/client/features') + .set('Authorization', token.secret) + .expect('Content-Type', /json/) + .expect(401); +}); diff --git a/src/test/e2e/services/edge-service.e2e.test.ts b/src/test/e2e/services/edge-service.e2e.test.ts index 30f2c0c9d5..4801140d91 100644 --- a/src/test/e2e/services/edge-service.e2e.test.ts +++ b/src/test/e2e/services/edge-service.e2e.test.ts @@ -69,7 +69,7 @@ test('should only return valid tokens', async () => { const expiredToken = await stores.apiTokenStore.insert({ tokenName: 'expired', - secret: 'expired-secret', + secret: '*:environment.expired-secret', type: ApiTokenType.CLIENT, expiresAt: yesterday, projects: ['*'], @@ -78,7 +78,7 @@ test('should only return valid tokens', async () => { const activeToken = await stores.apiTokenStore.insert({ tokenName: 'default-valid', - secret: 'valid-secret', + secret: '*:environment.valid-secret', type: ApiTokenType.CLIENT, expiresAt: tomorrow, projects: ['*'],