From fdc6f0ad7dc27525ff29fae6eadedf6e39ce28a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20MORI?= Date: Fri, 19 Aug 2022 16:45:16 +0200 Subject: [PATCH] Use options instead of configuration for allowed location types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/backstage/backstage/pull/13013 Signed-off-by: Stéphane MORI --- .changeset/flat-plums-shout.md | 2 +- plugins/catalog-backend/config.d.ts | 17 ---- .../src/service/CatalogBuilder.ts | 18 +++- .../service/DefaultLocationService.test.ts | 97 +++---------------- .../src/service/DefaultLocationService.ts | 23 +++-- 5 files changed, 43 insertions(+), 114 deletions(-) diff --git a/.changeset/flat-plums-shout.md b/.changeset/flat-plums-shout.md index 578952c9c3..250d6a1b35 100644 --- a/.changeset/flat-plums-shout.md +++ b/.changeset/flat-plums-shout.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-catalog-backend': patch +'@backstage/plugin-catalog-backend': minor --- Allow unknown typed location from being registered via the location service by configuration settings diff --git a/plugins/catalog-backend/config.d.ts b/plugins/catalog-backend/config.d.ts index 473db9a35a..950ed2f63c 100644 --- a/plugins/catalog-backend/config.d.ts +++ b/plugins/catalog-backend/config.d.ts @@ -54,23 +54,6 @@ export interface Config { */ readonly?: boolean; - /** - * Allow unknown type when create location. - * - * Setting 'locationService.create.allowUnknownType=false' forbids to create - * location with unknown type through location service. - * This is the default value. - * - * Setting 'locationService.create.allowUnknownType=true' allows to create - * location with unknown type through location service. - * - */ - locationService?: { - create?: { - allowUnknownType: boolean; - }; - }; - /** * A set of static locations that the catalog shall always keep itself * up-to-date with. This is commonly used for large, permanent integrations diff --git a/plugins/catalog-backend/src/service/CatalogBuilder.ts b/plugins/catalog-backend/src/service/CatalogBuilder.ts index 57b5160a97..9015bf4297 100644 --- a/plugins/catalog-backend/src/service/CatalogBuilder.ts +++ b/plugins/catalog-backend/src/service/CatalogBuilder.ts @@ -147,7 +147,8 @@ export class CatalogBuilder { maxSeconds: 150, }); private locationAnalyzer: LocationAnalyzer | undefined = undefined; - private permissionRules: CatalogPermissionRule[]; + private readonly permissionRules: CatalogPermissionRule[]; + private allowedLocationType: string[]; /** * Creates a catalog builder. @@ -167,6 +168,7 @@ export class CatalogBuilder { this.processorsReplace = false; this.parser = undefined; this.permissionRules = Object.values(catalogPermissionRules); + this.allowedLocationType = ['url']; } /** @@ -363,6 +365,16 @@ export class CatalogBuilder { this.permissionRules.push(...permissionRules.flat()); } + /** + * Sets up the allowed location types from being registered via the location service. + * + * @param allowedLocationTypes + */ + setAllowedLocationTypes(allowedLocationTypes: string[]) { + this.allowedLocationType = allowedLocationTypes; + return this; + } + /** * Wires up and returns all of the component parts of the catalog */ @@ -465,7 +477,9 @@ export class CatalogBuilder { const locationAnalyzer = this.locationAnalyzer ?? new RepoLocationAnalyzer(logger, integrations); const locationService = new AuthorizedLocationService( - new DefaultLocationService(locationStore, orchestrator, config), + new DefaultLocationService(locationStore, orchestrator, { + allowedLocationTypes: this.allowedLocationType, + }), permissionEvaluator, ); const refreshService = new AuthorizedRefreshService( diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts index e936d2de93..d377ece202 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.test.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.test.ts @@ -18,7 +18,6 @@ import { DefaultLocationService } from './DefaultLocationService'; import { CatalogProcessingOrchestrator } from '../processing/types'; import { LocationStore } from './types'; import { InputError } from '@backstage/errors'; -import { ConfigReader } from '@backstage/config'; describe('DefaultLocationServiceTest', () => { const orchestrator: jest.Mocked = { @@ -31,17 +30,6 @@ describe('DefaultLocationServiceTest', () => { getLocation: jest.fn(), }; - const mockConfig = (allowUnknownType: boolean) => - new ConfigReader({ - catalog: { - locationService: { - create: { - allowUnknownType, - }, - }, - }, - }); - beforeEach(() => { jest.resetAllMocks(); }); @@ -92,11 +80,7 @@ describe('DefaultLocationServiceTest', () => { errors: [], }); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await locationService.createLocation( { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, true, @@ -161,11 +145,7 @@ describe('DefaultLocationServiceTest', () => { { id: '137', ...locationSpec }, ]); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); const result = await locationService.createLocation( { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, true, @@ -219,11 +199,7 @@ describe('DefaultLocationServiceTest', () => { errors: [], }); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await expect( locationService.createLocation( { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, @@ -253,11 +229,7 @@ describe('DefaultLocationServiceTest', () => { { id: '987', type: 'url', target: 'https://example.com' }, ]); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); const result = await locationService.createLocation( { type: 'url', target: 'https://backstage.io/catalog-info.yaml' }, true, @@ -276,11 +248,7 @@ describe('DefaultLocationServiceTest', () => { id: '123', }); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await expect( locationService.createLocation(locationSpec, false), ).resolves.toEqual({ @@ -308,11 +276,9 @@ describe('DefaultLocationServiceTest', () => { id: '123', }); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(true), - ); + const locationService = new DefaultLocationService(store, orchestrator, { + allowedLocationTypes: ['url', 'unknown'], + }); await expect( locationService.createLocation(locationSpec, false), ).resolves.toEqual({ @@ -330,28 +296,7 @@ describe('DefaultLocationServiceTest', () => { }); it('should not allow locations of unknown types by default', async () => { - const locationService = new DefaultLocationService( - store, - orchestrator, - new ConfigReader({}), - ); - await expect( - locationService.createLocation( - { - type: 'unknown', - target: 'https://backstage.io/catalog-info.yaml', - }, - false, - ), - ).rejects.toThrow(InputError); - }); - - it('should not allow locations of unknown types if configuration forbids it', async () => { - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await expect( locationService.createLocation( { @@ -371,11 +316,7 @@ describe('DefaultLocationServiceTest', () => { errors: [new Error('Error: Unable to read url')], }); - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await expect( locationService.createLocation( { @@ -390,11 +331,7 @@ describe('DefaultLocationServiceTest', () => { describe('listLocations', () => { it('should call locationStore.deleteLocation', async () => { - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await locationService.listLocations(); expect(store.listLocations).toBeCalled(); }); @@ -402,11 +339,7 @@ describe('DefaultLocationServiceTest', () => { describe('deleteLocation', () => { it('should call locationStore.deleteLocation', async () => { - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await locationService.deleteLocation('123'); expect(store.deleteLocation).toBeCalledWith('123'); }); @@ -414,11 +347,7 @@ describe('DefaultLocationServiceTest', () => { describe('getLocation', () => { it('should call locationStore.getLocation', async () => { - const locationService = new DefaultLocationService( - store, - orchestrator, - mockConfig(false), - ); + const locationService = new DefaultLocationService(store, orchestrator); await locationService.getLocation('123'); expect(store.getLocation).toBeCalledWith('123'); }); diff --git a/plugins/catalog-backend/src/service/DefaultLocationService.ts b/plugins/catalog-backend/src/service/DefaultLocationService.ts index e70d72a9ee..6603ebc08b 100644 --- a/plugins/catalog-backend/src/service/DefaultLocationService.ts +++ b/plugins/catalog-backend/src/service/DefaultLocationService.ts @@ -26,27 +26,30 @@ import { LocationInput, LocationService, LocationStore } from './types'; import { locationSpecToMetadataName } from '../util/conversion'; import { InputError } from '@backstage/errors'; import { DeferredEntity } from '@backstage/plugin-catalog-node'; -import { Config } from '@backstage/config'; + +export type DefaultLocationServiceOptions = { + allowedLocationTypes: string[]; +}; export class DefaultLocationService implements LocationService { constructor( private readonly store: LocationStore, private readonly orchestrator: CatalogProcessingOrchestrator, - private readonly config: Config, + private readonly options: DefaultLocationServiceOptions = { + allowedLocationTypes: ['url'], + }, ) {} async createLocation( input: LocationInput, dryRun: boolean, ): Promise<{ location: Location; entities: Entity[]; exists?: boolean }> { - const allowUnknownTypeConfigName = - 'catalog.locationService.create.allowUnknownType'; - const allowUnknownType = - this.config.has(allowUnknownTypeConfigName) && - this.config.getBoolean(allowUnknownTypeConfigName); - - if (!allowUnknownType && input.type !== 'url') { - throw new InputError(`Registered locations must be of type 'url'`); + if (!this.options.allowedLocationTypes.includes(input.type)) { + throw new InputError( + `Registered locations must be of an allowed type ${JSON.stringify( + this.options.allowedLocationTypes, + )}`, + ); } if (dryRun) { return this.dryRunCreateLocation(input);