From 65c9feb9849dfbeb108ba561b5755a818f13a617 Mon Sep 17 00:00:00 2001 From: David Leek Date: Mon, 1 Jul 2024 15:57:19 +0200 Subject: [PATCH 01/10] chore: delete project api tokens when last mapped project is removed --- .../features/project/createProjectService.ts | 7 +++++++ src/lib/features/project/project-service.ts | 21 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/lib/features/project/createProjectService.ts b/src/lib/features/project/createProjectService.ts index 6744b34698f9..d884dfb3b6f7 100644 --- a/src/lib/features/project/createProjectService.ts +++ b/src/lib/features/project/createProjectService.ts @@ -45,6 +45,8 @@ import { ProjectOwnersReadModel } from './project-owners-read-model'; import { FakeProjectOwnersReadModel } from './fake-project-owners-read-model'; import { FakeProjectFlagCreatorsReadModel } from './fake-project-flag-creators-read-model'; import { ProjectFlagCreatorsReadModel } from './project-flag-creators-read-model'; +import FakeApiTokenStore from '../../../test/fixtures/fake-api-token-store'; +import { ApiTokenStore } from '../../db/api-token-store'; export const createProjectService = ( db: Db, @@ -109,6 +111,8 @@ export const createProjectService = ( eventService, ); + const apiTokenStore = new ApiTokenStore(db, eventBus, getLogger); + const privateProjectChecker = createPrivateProjectChecker(db, config); return new ProjectService( @@ -123,6 +127,7 @@ export const createProjectService = ( projectStatsStore, projectOwnersReadModel, projectFlagCreatorsReadModel, + apiTokenStore, }, config, accessService, @@ -153,6 +158,7 @@ export const createFakeProjectService = ( const { featureToggleService } = createFakeFeatureToggleService(config); const favoriteFeaturesStore = new FakeFavoriteFeaturesStore(); const favoriteProjectsStore = new FakeFavoriteProjectsStore(); + const apiTokenStore = new FakeApiTokenStore(); const eventService = new EventService( { eventStore, @@ -188,6 +194,7 @@ export const createFakeProjectService = ( featureTypeStore, accountStore, projectStatsStore, + apiTokenStore, }, config, accessService, diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 2549d9c0f1a8..3b88efb34eb6 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -53,6 +53,7 @@ import { type ProjectCreated, type IProjectOwnersReadModel, ADMIN, + type IApiTokenStore, } from '../../types'; import type { IProjectAccessModel, @@ -140,6 +141,8 @@ export default class ProjectService { private accountStore: IAccountStore; + private apiTokenStore: IApiTokenStore; + private favoritesService: FavoritesService; private eventService: EventService; @@ -162,6 +165,7 @@ export default class ProjectService { featureTypeStore, accountStore, projectStatsStore, + apiTokenStore, }: Pick< IUnleashStores, | 'projectStore' @@ -174,6 +178,7 @@ export default class ProjectService { | 'accountStore' | 'projectStatsStore' | 'featureTypeStore' + | 'apiTokenStore' >, config: IUnleashConfig, accessService: AccessService, @@ -192,6 +197,7 @@ export default class ProjectService { this.eventStore = eventStore; this.featureToggleStore = featureToggleStore; this.featureTypeStore = featureTypeStore; + this.apiTokenStore = apiTokenStore; this.featureToggleService = featureToggleService; this.favoritesService = favoriteService; this.privateProjectChecker = privateProjectChecker; @@ -537,8 +543,23 @@ export default class ProjectService { auditUser, ); + const allTokens = await this.apiTokenStore.getAll(); + const projectTokens = allTokens.filter( + (token) => + (token.projects && + token.projects.length === 1 && + token.projects[0] === id) || + token.project === id, + ); + await this.projectStore.delete(id); + await Promise.all( + projectTokens.map((token) => + this.apiTokenStore.delete(token.secret), + ), + ); + await this.eventService.storeEvent( new ProjectDeletedEvent({ project: id, From 05b48f9e3cb5bfdb8d41988a14579fa0af77872d Mon Sep 17 00:00:00 2001 From: David Leek Date: Wed, 3 Jul 2024 10:29:38 +0200 Subject: [PATCH 02/10] chore(test): cover project-deletion api-token cleanup with tests --- .../project/project-service.e2e.test.ts | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index 3dcd277d92ee..c79892b56c78 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -9,7 +9,7 @@ import { RoleName } from '../../types/model'; import { randomId } from '../../util/random-id'; import EnvironmentService from '../project-environments/environment-service'; import IncompatibleProjectError from '../../error/incompatible-project-error'; -import { EventService } from '../../services'; +import { ApiTokenService, EventService } from '../../services'; import { FeatureEnvironmentEvent } from '../../types/events'; import { addDays, subDays } from 'date-fns'; import { @@ -28,7 +28,8 @@ import { } from '../../types'; import type { User } from '../../server-impl'; import { BadDataError, InvalidOperationError } from '../../error'; -import { extractAuditInfoFromUser } from '../../util'; +import { DEFAULT_ENV, extractAuditInfoFromUser } from '../../util'; +import { ApiTokenType } from '../../types/models/api-token'; let stores: IUnleashStores; let db: ITestDb; @@ -40,6 +41,7 @@ let environmentService: EnvironmentService; let featureToggleService: FeatureToggleService; let user: User; // many methods in this test use User instead of IUser let auditUser: IAuditUser; +let apiTokenService: ApiTokenService; let opsUser: IUser; let group: IGroup; @@ -89,6 +91,7 @@ beforeAll(async () => { environmentService = new EnvironmentService(stores, config, eventService); projectService = createProjectService(db.rawDatabase, config); + apiTokenService = new ApiTokenService(stores, config, eventService); }); beforeEach(async () => { await stores.accessStore.addUserToRole(opsUser.id, 1, ''); @@ -2488,6 +2491,67 @@ test('deleting a project with archived flags should result in any remaining arch expect(flags.find((t) => t.name === flagName)).toBeUndefined(); }); +test('should also delete api tokens that were only bound to deleted project', async () => { + const project2 = 'some'; + const tokenName = 'test'; + + await projectService.createProject( + { + id: project2, + name: 'Test Project 2', + }, + user, + auditUser, + ); + + const token = await apiTokenService.createApiToken({ + type: ApiTokenType.CLIENT, + tokenName, + environment: DEFAULT_ENV, + project: project2, + }); + + await projectService.deleteProject(project2, user, auditUser); + const deletedToken = await apiTokenService.getToken(token.secret); + expect(deletedToken).toBeUndefined(); +}); + +test('should not delete project bound api tokens still bound to project', async () => { + const project2 = 'token-deleted-project'; + const project3 = 'token-not-deleted-project'; + const tokenName = 'test'; + + await projectService.createProject( + { + id: project2, + name: 'Test Project 2', + }, + user, + auditUser, + ); + + await projectService.createProject( + { + id: project3, + name: 'Test Project 3', + }, + user, + auditUser, + ); + + const token = await apiTokenService.createApiToken({ + type: ApiTokenType.CLIENT, + tokenName, + environment: DEFAULT_ENV, + projects: [project2, project3], + }); + + await projectService.deleteProject(project2, user, auditUser); + const fetchedToken = await apiTokenService.getToken(token.secret); + expect(fetchedToken).not.toBeUndefined(); + expect(fetchedToken.project).toBe(project3); +}); + test('deleting a project with no archived flags should not result in an error', async () => { const project = { id: 'project-with-nothing', From 69548ae8e797f0322e68f567903cf6bd52e45d55 Mon Sep 17 00:00:00 2001 From: David Leek Date: Wed, 3 Jul 2024 10:32:53 +0200 Subject: [PATCH 03/10] chore(test): project api tokens deleted when multiple (and last connected) projects deleted --- .../project/project-service.e2e.test.ts | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index c79892b56c78..f63b44887398 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -2552,6 +2552,42 @@ test('should not delete project bound api tokens still bound to project', async expect(fetchedToken.project).toBe(project3); }); +test('should delete project bound api tokens when all projects they belong to are deleted', async () => { + const project2 = 'token-deleted-project-1'; + const project3 = 'token-deleted-project-2'; + const tokenName = 'test'; + + await projectService.createProject( + { + id: project2, + name: 'Test Project 2', + }, + user, + auditUser, + ); + + await projectService.createProject( + { + id: project3, + name: 'Test Project 3', + }, + user, + auditUser, + ); + + const token = await apiTokenService.createApiToken({ + type: ApiTokenType.CLIENT, + tokenName, + environment: DEFAULT_ENV, + projects: [project2, project3], + }); + + await projectService.deleteProject(project2, user, auditUser); + await projectService.deleteProject(project3, user, auditUser); + const fetchedToken = await apiTokenService.getToken(token.secret); + expect(fetchedToken).toBeUndefined(); +}); + test('deleting a project with no archived flags should not result in an error', async () => { const project = { id: 'project-with-nothing', From 4b521935454b944e426cb26ac614bee2c43b0645 Mon Sep 17 00:00:00 2001 From: David Leek Date: Sun, 7 Jul 2024 23:52:42 +0200 Subject: [PATCH 04/10] feat: add cleanApiTokenWhenOrphaned flag --- src/lib/types/experimental.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 4d2097d30a8c..17c6bb000896 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -63,7 +63,8 @@ export type IFlagKey = | 'flagCreator' | 'anonymizeProjectOwners' | 'resourceLimits' - | 'extendedMetrics'; + | 'extendedMetrics' + | 'cleanApiTokenWhenOrphaned'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -304,6 +305,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_EXTENDED_METRICS, false, ), + cleanApiTokenWhenOrphaned: parseEnvVarBoolean( + process.env.UNLEASH_EXPERIMENTAL_CLEAN_API_TOKEN_WHEN_ORPHANED, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = { From 35ea6e2f6ba3b8ec9ead0e8cb85ceab77b011c5b Mon Sep 17 00:00:00 2001 From: David Leek Date: Sun, 7 Jul 2024 23:53:28 +0200 Subject: [PATCH 05/10] feat: if flag enabled - delete orphaned tokens when deleting projects. else just delete projects --- src/lib/features/project/project-service.ts | 32 ++++++++++++--------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 68b2bb92c915..46ba74576bfc 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -564,22 +564,26 @@ export default class ProjectService { auditUser, ); - const allTokens = await this.apiTokenStore.getAll(); - const projectTokens = allTokens.filter( - (token) => - (token.projects && - token.projects.length === 1 && - token.projects[0] === id) || - token.project === id, - ); + if (this.flagResolver.isEnabled('cleanApiTokenWhenOrphaned')) { + const allTokens = await this.apiTokenStore.getAll(); + const projectTokens = allTokens.filter( + (token) => + (token.projects && + token.projects.length === 1 && + token.projects[0] === id) || + token.project === id, + ); - await this.projectStore.delete(id); + await this.projectStore.delete(id); - await Promise.all( - projectTokens.map((token) => - this.apiTokenStore.delete(token.secret), - ), - ); + await Promise.all( + projectTokens.map((token) => + this.apiTokenStore.delete(token.secret), + ), + ); + } else { + await this.projectStore.delete(id); + } await this.eventService.storeEvent( new ProjectDeletedEvent({ From 52bdea582e15cbaa193bfb0a43e48d691f21fb8b Mon Sep 17 00:00:00 2001 From: David Leek Date: Mon, 8 Jul 2024 11:24:03 +0200 Subject: [PATCH 06/10] fix: rename variables project2 and 3 to 1 and 2 --- .../project/project-service.e2e.test.ts | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index f63b44887398..49158e5b389b 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -2492,13 +2492,13 @@ test('deleting a project with archived flags should result in any remaining arch }); test('should also delete api tokens that were only bound to deleted project', async () => { - const project2 = 'some'; + const project = 'some'; const tokenName = 'test'; await projectService.createProject( { - id: project2, - name: 'Test Project 2', + id: project, + name: 'Test Project 1', }, user, auditUser, @@ -2508,23 +2508,23 @@ test('should also delete api tokens that were only bound to deleted project', as type: ApiTokenType.CLIENT, tokenName, environment: DEFAULT_ENV, - project: project2, + project: project, }); - await projectService.deleteProject(project2, user, auditUser); + await projectService.deleteProject(project, user, auditUser); const deletedToken = await apiTokenService.getToken(token.secret); expect(deletedToken).toBeUndefined(); }); test('should not delete project bound api tokens still bound to project', async () => { - const project2 = 'token-deleted-project'; - const project3 = 'token-not-deleted-project'; + const project1 = 'token-deleted-project'; + const project2 = 'token-not-deleted-project'; const tokenName = 'test'; await projectService.createProject( { - id: project2, - name: 'Test Project 2', + id: project1, + name: 'Test Project 1', }, user, auditUser, @@ -2532,8 +2532,8 @@ test('should not delete project bound api tokens still bound to project', async await projectService.createProject( { - id: project3, - name: 'Test Project 3', + id: project2, + name: 'Test Project 2', }, user, auditUser, @@ -2543,24 +2543,24 @@ test('should not delete project bound api tokens still bound to project', async type: ApiTokenType.CLIENT, tokenName, environment: DEFAULT_ENV, - projects: [project2, project3], + projects: [project1, project2], }); - await projectService.deleteProject(project2, user, auditUser); + await projectService.deleteProject(project1, user, auditUser); const fetchedToken = await apiTokenService.getToken(token.secret); expect(fetchedToken).not.toBeUndefined(); - expect(fetchedToken.project).toBe(project3); + expect(fetchedToken.project).toBe(project2); }); test('should delete project bound api tokens when all projects they belong to are deleted', async () => { - const project2 = 'token-deleted-project-1'; - const project3 = 'token-deleted-project-2'; + const project1 = 'token-deleted-project-1'; + const project2 = 'token-deleted-project-2'; const tokenName = 'test'; await projectService.createProject( { - id: project2, - name: 'Test Project 2', + id: project1, + name: 'Test Project 1', }, user, auditUser, @@ -2568,8 +2568,8 @@ test('should delete project bound api tokens when all projects they belong to ar await projectService.createProject( { - id: project3, - name: 'Test Project 3', + id: project2, + name: 'Test Project 2', }, user, auditUser, @@ -2579,11 +2579,11 @@ test('should delete project bound api tokens when all projects they belong to ar type: ApiTokenType.CLIENT, tokenName, environment: DEFAULT_ENV, - projects: [project2, project3], + projects: [project1, project2], }); + await projectService.deleteProject(project1, user, auditUser); await projectService.deleteProject(project2, user, auditUser); - await projectService.deleteProject(project3, user, auditUser); const fetchedToken = await apiTokenService.getToken(token.secret); expect(fetchedToken).toBeUndefined(); }); From 1e98d5854af2139fa5ea73813d7fee65a4ae576b Mon Sep 17 00:00:00 2001 From: David Leek Date: Mon, 8 Jul 2024 13:25:29 +0200 Subject: [PATCH 07/10] chore: add flagResolver to constructor arguments --- src/lib/features/project/createProjectService.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/lib/features/project/createProjectService.ts b/src/lib/features/project/createProjectService.ts index d884dfb3b6f7..599df4a9db43 100644 --- a/src/lib/features/project/createProjectService.ts +++ b/src/lib/features/project/createProjectService.ts @@ -111,7 +111,12 @@ export const createProjectService = ( eventService, ); - const apiTokenStore = new ApiTokenStore(db, eventBus, getLogger); + const apiTokenStore = new ApiTokenStore( + db, + eventBus, + getLogger, + flagResolver, + ); const privateProjectChecker = createPrivateProjectChecker(db, config); From 79f25305a68e793cf49b452627f554a37a0839af Mon Sep 17 00:00:00 2001 From: David Leek Date: Mon, 8 Jul 2024 21:23:31 +0200 Subject: [PATCH 08/10] test: snapshot for new feature flag --- src/lib/__snapshots__/create-config.test.ts.snap | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 481faa66c6f9..0064124bf73d 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -82,6 +82,7 @@ exports[`should create default config 1`] = ` "automatedActions": false, "caseInsensitiveInOperators": false, "celebrateUnleash": false, + "cleanApiTokenWhenOrphaned": false, "collectTrafficDataUsage": false, "commandBarUI": false, "demo": false, From 4112fe34ebf1d36f81a47f7651fc2628c0d2cfa4 Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Mon, 8 Jul 2024 21:36:13 +0200 Subject: [PATCH 09/10] Update src/lib/features/project/project-service.e2e.test.ts Co-authored-by: Thomas Heartman --- src/lib/features/project/project-service.e2e.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index 49158e5b389b..42fde0deb2cf 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -2552,7 +2552,7 @@ test('should not delete project bound api tokens still bound to project', async expect(fetchedToken.project).toBe(project2); }); -test('should delete project bound api tokens when all projects they belong to are deleted', async () => { +test('should delete project-bound api tokens when all projects they belong to are deleted', async () => { const project1 = 'token-deleted-project-1'; const project2 = 'token-deleted-project-2'; const tokenName = 'test'; From 51a6ba69e968f664201481043ee1dda6819324d7 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 9 Jul 2024 13:27:43 +0200 Subject: [PATCH 10/10] Update src/lib/features/project/project-service.e2e.test.ts --- src/lib/features/project/project-service.e2e.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index 42fde0deb2cf..f243d54adae4 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -2516,7 +2516,7 @@ test('should also delete api tokens that were only bound to deleted project', as expect(deletedToken).toBeUndefined(); }); -test('should not delete project bound api tokens still bound to project', async () => { +test('should not delete project-bound api tokens still bound to project', async () => { const project1 = 'token-deleted-project'; const project2 = 'token-not-deleted-project'; const tokenName = 'test';