Use options instead of configuration for allowed location types

https://github.com/backstage/backstage/pull/13013

Signed-off-by: Stéphane MORI <stephane.mori@gmail.com>

.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

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

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(

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<CatalogProcessingOrchestrator> = {
@@ -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');
     });

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);