From f222a2eab999408abdff5c6146b0ae5826e4ad10 Mon Sep 17 00:00:00 2001 From: Hellgren Heikki Date: Tue, 23 Sep 2025 21:57:10 +0300 Subject: [PATCH 1/2] fix: distributed actions visibility in scaffolder depending on the plugin startup order, some distributed actions might not be available when the scaffolder backend starts. this was mentioned in the comment that is now removed. while this might change in the future, this would patch the current way the distributed actions are fetched. related #31213 and #31207 closes #31187 Signed-off-by: Hellgren Heikki --- .changeset/loud-windows-give.md | 8 + app-config.yaml | 4 +- .../actions/TemplateActionRegistry.test.ts | 278 ++++++++++++++++++ .../actions/TemplateActionRegistry.ts | 79 ++++- .../src/scaffolder/actions/index.ts | 5 +- .../dryrun/DecoratedActionsRegistry.ts | 33 ++- .../src/scaffolder/dryrun/createDryRunner.ts | 2 +- .../tasks/NunjucksWorkflowRunner.test.ts | 14 +- .../tasks/NunjucksWorkflowRunner.ts | 6 +- .../src/scaffolder/tasks/TaskWorker.ts | 2 +- .../scaffolder-backend/src/service/router.ts | 91 ++---- 11 files changed, 438 insertions(+), 84 deletions(-) create mode 100644 .changeset/loud-windows-give.md create mode 100644 plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts diff --git a/.changeset/loud-windows-give.md b/.changeset/loud-windows-give.md new file mode 100644 index 0000000000..7e9ea6881d --- /dev/null +++ b/.changeset/loud-windows-give.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-scaffolder-backend': patch +--- + +Fixed distributed actions not being visible in the scaffolder template actions. + +Depending on the plugin startup order, some of the distributed actions were not being registered correctly, +causing them to be invisible in the scaffolder template actions list. diff --git a/app-config.yaml b/app-config.yaml index eacf9a96aa..b45255d26c 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -65,7 +65,9 @@ backend: - host: example.com - host: '*.mozilla.org' # workingDirectory: /tmp # Use this to configure a working directory for the scaffolder, defaults to the OS temp-dir - + actions: + pluginSources: + - catalog # See README.md in the proxy-backend plugin for information on the configuration format proxy: endpoints: diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts new file mode 100644 index 0000000000..f3a3b15ec9 --- /dev/null +++ b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts @@ -0,0 +1,278 @@ +/* + * Copyright 2021 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ConflictError, NotFoundError } from '@backstage/errors'; +import { createTemplateAction } from '@backstage/plugin-scaffolder-node'; +import { DefaultTemplateActionRegistry } from './TemplateActionRegistry'; +import { mockCredentials, mockServices } from '@backstage/backend-test-utils'; +import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha'; + +describe('DefaultTemplateActionRegistry', () => { + let registry: DefaultTemplateActionRegistry; + let mockActionsService: ReturnType; + let mockAuthService: ReturnType; + let mockLogger: ReturnType; + + beforeEach(() => { + mockActionsService = actionsRegistryServiceMock(); + mockAuthService = mockServices.auth(); + mockLogger = mockServices.logger.mock(); + + registry = new DefaultTemplateActionRegistry( + mockActionsService, + mockAuthService, + mockLogger, + ); + }); + + describe('register', () => { + it('should register a template action successfully', () => { + const action = createTemplateAction({ + id: 'test-action', + description: 'Test action', + handler: jest.fn(), + }); + + expect(() => registry.register(action)).not.toThrow(); + }); + + it('should throw ConflictError when registering action with duplicate ID', () => { + const action1 = createTemplateAction({ + id: 'duplicate-action', + description: 'First action', + handler: jest.fn(), + }); + + const action2 = createTemplateAction({ + id: 'duplicate-action', + description: 'Second action', + handler: jest.fn(), + }); + + registry.register(action1); + + expect(() => registry.register(action2)).toThrow(ConflictError); + expect(() => registry.register(action2)).toThrow( + "Template action with ID 'duplicate-action' has already been registered", + ); + }); + }); + + describe('get', () => { + it('should return a registered action', async () => { + const action = createTemplateAction({ + id: 'test-action', + description: 'Test action', + handler: jest.fn(), + }); + + registry.register(action); + const result = await registry.get('test-action'); + + expect(result).toBe(action); + }); + + it('should return action from ActionsService', async () => { + mockActionsService.register({ + name: 'service-action', + title: 'Service Action', + description: 'Service action', + schema: { + input: z => z.object({}), + output: z => z.object({}), + }, + attributes: { + readOnly: true, + destructive: false, + }, + action: async () => ({ output: {} }), + }); + + const result = await registry.get('test:service-action'); + + expect(result.id).toBe('test:service-action'); + expect(result.description).toBe('Service action'); + expect(result.supportsDryRun).toBe(true); + }); + + it('should throw NotFoundError when action is not found', async () => { + await expect(registry.get('non-existent-action')).rejects.toThrow( + NotFoundError, + ); + await expect(registry.get('non-existent-action')).rejects.toThrow( + "Template action with ID 'non-existent-action' is not registered", + ); + }); + + it('should use own service credentials when getting action', async () => { + const spy = jest.spyOn(mockActionsService, 'list'); + + await expect(registry.get('non-existent')).rejects.toThrow(); + + expect(spy).toHaveBeenCalledWith({ + credentials: await mockAuthService.getOwnServiceCredentials(), + }); + }); + }); + + describe('list', () => { + it('should return empty map when no actions are registered', async () => { + const result = await registry.list(); + + expect(result).toBeInstanceOf(Map); + expect(result.size).toBe(0); + }); + + it('should return all registered actions', async () => { + const action1 = createTemplateAction({ + id: 'action-1', + description: 'First action', + handler: jest.fn(), + }); + + const action2 = createTemplateAction({ + id: 'action-2', + description: 'Second action', + handler: jest.fn(), + }); + + registry.register(action1); + registry.register(action2); + + const result = await registry.list(); + + expect(result.size).toBe(2); + expect(result.get('action-1')).toBe(action1); + expect(result.get('action-2')).toBe(action2); + }); + + it('should include actions from ActionsService', async () => { + mockActionsService.register({ + name: 'service-action', + title: 'Service Action', + description: 'Service action', + schema: { + input: z => z.object({}), + output: z => z.object({}), + }, + attributes: { + readOnly: true, + destructive: false, + }, + action: async () => ({ output: {} }), + }); + + const result = await registry.list(); + + expect(result.size).toBe(1); + const action = result.get('test:service-action'); + expect(action).toBeDefined(); + expect(action!.id).toBe('test:service-action'); + expect(action!.description).toBe('Service action'); + expect(action!.supportsDryRun).toBe(true); + }); + + it('should prioritize locally registered actions over service actions', async () => { + const localAction = createTemplateAction({ + id: 'test:same-id', + description: 'Local action', + handler: jest.fn(), + }); + + mockActionsService.register({ + name: 'same-id', + title: 'Same ID', + description: 'Service action', + schema: { + input: z => z.object({}), + output: z => z.object({}), + }, + action: async () => ({ output: {} }), + }); + + registry.register(localAction); + const result = await registry.list(); + + expect(result.get('test:same-id')).toBe(localAction); + expect(mockLogger.warn).toHaveBeenCalledWith( + "Template action with ID 'test:same-id' has already been registered, skipping action provided by actions service", + ); + }); + + it('should use provided credentials when listing actions', async () => { + const credentials = mockCredentials.user(); + const spy = jest.spyOn(mockActionsService, 'list'); + + await registry.list({ credentials }); + + expect(spy).toHaveBeenCalledWith({ credentials }); + }); + + it('should use own service credentials when no credentials provided', async () => { + const spy = jest.spyOn(mockActionsService, 'list'); + + await registry.list(); + + expect(spy).toHaveBeenCalledWith({ + credentials: await mockAuthService.getOwnServiceCredentials(), + }); + }); + + it('should set supportsDryRun to false for destructive actions', async () => { + mockActionsService.register({ + name: 'destructive-action', + title: 'Destructive Action', + description: 'Destructive action', + schema: { + input: z => z.object({}), + output: z => z.object({}), + }, + attributes: { + readOnly: false, + destructive: true, + }, + action: async () => ({ output: {} }), + }); + + const result = await registry.list(); + const action = result.get('test:destructive-action'); + + expect(action!.supportsDryRun).toBe(false); + }); + + it('should set supportsDryRun to false for non-readonly actions', async () => { + mockActionsService.register({ + name: 'non-readonly-action', + title: 'Non-readonly Action', + description: 'Non-readonly action', + schema: { + input: z => z.object({}), + output: z => z.object({}), + }, + attributes: { + readOnly: false, + destructive: false, + }, + action: async () => ({ output: {} }), + }); + + const result = await registry.list(); + const action = result.get('test:non-readonly-action'); + + expect(action!.supportsDryRun).toBe(false); + }); + }); +}); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts index d6d7685a21..7bcf120646 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts @@ -16,13 +16,39 @@ import { ConflictError, NotFoundError } from '@backstage/errors'; import { TemplateAction } from '@backstage/plugin-scaffolder-node'; +import { ActionsService } from '@backstage/backend-plugin-api/alpha'; +import { + AuthService, + BackstageCredentials, + LoggerService, +} from '@backstage/backend-plugin-api'; +import { isPlainObject } from 'lodash'; +import { Schema } from 'jsonschema'; +import { JsonObject } from '@backstage/types'; + +/** + * @internal + */ +export interface TemplateActionRegistry { + register(action: TemplateAction): void; + get(actionId: string): Promise>; + list(options?: { + credentials?: BackstageCredentials; + }): Promise>>; +} /** * Registry of all registered template actions. */ -export class TemplateActionRegistry { +export class DefaultTemplateActionRegistry implements TemplateActionRegistry { private readonly actions = new Map(); + constructor( + private readonly actionsRegistry: ActionsService, + private readonly auth: AuthService, + private readonly logger: LoggerService, + ) {} + register(action: TemplateAction) { if (this.actions.has(action.id)) { throw new ConflictError( @@ -33,8 +59,8 @@ export class TemplateActionRegistry { this.actions.set(action.id, action); } - get(actionId: string): TemplateAction { - const action = this.actions.get(actionId); + async get(actionId: string): Promise> { + const action = (await this.list()).get(actionId); if (!action) { throw new NotFoundError( `Template action with ID '${actionId}' is not registered. See https://backstage.io/docs/features/software-templates/builtin-actions/ on how to add a new action module.`, @@ -43,7 +69,50 @@ export class TemplateActionRegistry { return action; } - list(): TemplateAction[] { - return [...this.actions.values()]; + async list(options?: { + credentials?: BackstageCredentials; + }): Promise>> { + const ret = new Map(this.actions); + + const { actions } = await this.actionsRegistry.list({ + credentials: + options?.credentials ?? (await this.auth.getOwnServiceCredentials()), + }); + + for (const action of actions) { + if (ret.has(action.id)) { + this.logger.warn( + `Template action with ID '${action.id}' has already been registered, skipping action provided by actions service`, + ); + continue; + } + + ret.set(action.id, { + id: action.id, + description: action.description, + examples: [], + supportsDryRun: + action.attributes?.readOnly === true && + action.attributes?.destructive === false, + handler: async ctx => { + const { output } = await this.actionsRegistry.invoke({ + id: action.id, + input: ctx.input, + credentials: await ctx.getInitiatorCredentials(), + }); + + if (isPlainObject(output)) { + for (const [key, value] of Object.entries(output as JsonObject)) { + ctx.output(key as keyof typeof output, value); + } + } + }, + schema: { + input: action.schema.input as Schema, + output: action.schema.output as Schema, + }, + }); + } + return ret; } } diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/index.ts b/plugins/scaffolder-backend/src/scaffolder/actions/index.ts index ad128b19b7..f6bd39397c 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/index.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/index.ts @@ -16,4 +16,7 @@ export * from './builtin'; -export { TemplateActionRegistry } from './TemplateActionRegistry'; +export { + type TemplateActionRegistry, + DefaultTemplateActionRegistry, +} from './TemplateActionRegistry'; diff --git a/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts b/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts index 7109a6621e..b3f4b5e2cf 100644 --- a/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts +++ b/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts @@ -16,24 +16,43 @@ import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { TemplateActionRegistry } from '../actions'; +import { BackstageCredentials } from '@backstage/backend-plugin-api'; /** @internal */ -export class DecoratedActionsRegistry extends TemplateActionRegistry { +export class DecoratedActionsRegistry implements TemplateActionRegistry { + private readonly innerActions: Map = new Map(); + constructor( private readonly innerRegistry: TemplateActionRegistry, extraActions: Array, ) { - super(); for (const action of extraActions) { - this.register(action); + this.innerActions.set(action.id, action); } } - get(actionId: string): TemplateAction { + async get(actionId: string): Promise { try { - return super.get(actionId); - } catch { - return this.innerRegistry.get(actionId); + return await this.innerRegistry.get(actionId); + } catch (e) { + if (!this.innerActions.has(actionId)) { + throw e; + } + return this.innerActions.get(actionId)!; } } + + async list(options?: { + credentials?: BackstageCredentials; + }): Promise>> { + const inner = await this.innerRegistry.list(options); + return new Map>([ + ...inner, + ...this.innerActions, + ]); + } + + register(action: TemplateAction): void { + this.innerRegistry.register(action); + } } diff --git a/plugins/scaffolder-backend/src/scaffolder/dryrun/createDryRunner.ts b/plugins/scaffolder-backend/src/scaffolder/dryrun/createDryRunner.ts index d384443252..43c5aea40f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/dryrun/createDryRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/dryrun/createDryRunner.ts @@ -41,9 +41,9 @@ import fs from 'fs-extra'; import path from 'path'; import { fileURLToPath } from 'url'; import { v4 as uuid } from 'uuid'; -import { TemplateActionRegistry } from '../actions'; import { NunjucksWorkflowRunner } from '../tasks/NunjucksWorkflowRunner'; import { DecoratedActionsRegistry } from './DecoratedActionsRegistry'; +import { TemplateActionRegistry } from '../actions'; interface DryRunInput { spec: TaskSpec; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 8019bd4134..886ddda36f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -15,15 +15,18 @@ */ import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner'; -import { TemplateActionRegistry } from '../actions'; +import { + DefaultTemplateActionRegistry, + TemplateActionRegistry, +} from '../actions'; import { ScmIntegrations } from '@backstage/integration'; import { JsonObject } from '@backstage/types'; import { ConfigReader } from '@backstage/config'; import { TaskSpec } from '@backstage/plugin-scaffolder-common'; import { createTemplateAction, - TaskSecrets, TaskContext, + TaskSecrets, } from '@backstage/plugin-scaffolder-node'; import { UserEntity } from '@backstage/catalog-model'; import { @@ -36,6 +39,7 @@ import { mockCredentials, mockServices, } from '@backstage/backend-test-utils'; +import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha'; describe('NunjucksWorkflowRunner', () => { let actionRegistry: TemplateActionRegistry; @@ -104,7 +108,11 @@ describe('NunjucksWorkflowRunner', () => { // This one is ESM-only stripAnsi = await import('strip-ansi').then(m => m.default); - actionRegistry = new TemplateActionRegistry(); + actionRegistry = new DefaultTemplateActionRegistry( + actionsRegistryServiceMock(), + mockServices.auth(), + mockServices.logger.mock(), + ); fakeActionHandler = jest.fn(); fakeTaskLog = jest.fn(); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index bcae93fb4d..d13fa58a94 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -33,7 +33,7 @@ import { SecureTemplater, SecureTemplateRenderer, } from '../../lib/templating/SecureTemplater'; -import { TemplateActionRegistry } from '../actions/TemplateActionRegistry'; +import { TemplateActionRegistry } from '../actions/TemplateActionRegistry.ts'; import { generateExampleOutput, isTruthy } from './helper'; import { TaskTrackType, WorkflowResponse, WorkflowRunner } from './types'; @@ -61,8 +61,8 @@ import { createCounterMetric, createHistogramMetric } from '../../util/metrics'; import { BackstageLoggerTransport, WinstonLogger } from './logger'; import { convertFiltersToRecord } from '../../util/templating'; import { - CheckpointState, CheckpointContext, + CheckpointState, } from '@backstage/plugin-scaffolder-node/alpha'; type NunjucksWorkflowRunnerOptions = { @@ -245,7 +245,7 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { return; } const action: TemplateAction = - this.options.actionRegistry.get(step.action); + await this.options.actionRegistry.get(step.action); const { taskLogger } = createStepLogger({ task, step, diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts index adfdf0b553..7b85dd1270 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts @@ -25,7 +25,7 @@ import { TemplateGlobal, } from '@backstage/plugin-scaffolder-node'; import PQueue from 'p-queue'; -import { TemplateActionRegistry } from '../actions/TemplateActionRegistry'; +import { TemplateActionRegistry } from '../actions/TemplateActionRegistry.ts'; import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner'; import { WorkflowRunner } from './types'; import { setTimeout } from 'timers/promises'; diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 2a0792346d..bcdcedb70d 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -26,7 +26,7 @@ import { resolveSafeChildPath, SchedulerService, } from '@backstage/backend-plugin-api'; -import { Schema } from 'jsonschema'; +import { validate } from 'jsonschema'; import { CompoundEntityRef, Entity, @@ -41,10 +41,10 @@ import { ScmIntegrations } from '@backstage/integration'; import { EventsService } from '@backstage/plugin-events-node'; import { - createConditionAuthorizer, - createPermissionIntegrationRouter, - createConditionTransformer, ConditionTransformer, + createConditionAuthorizer, + createConditionTransformer, + createPermissionIntegrationRouter, } from '@backstage/plugin-permission-node'; import { TaskSpec, @@ -53,11 +53,11 @@ import { } from '@backstage/plugin-scaffolder-common'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION, - RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, RESOURCE_TYPE_SCAFFOLDER_TASK, + RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, scaffolderActionPermissions, - scaffolderTaskPermissions, scaffolderPermissions, + scaffolderTaskPermissions, scaffolderTemplatePermissions, taskCancelPermission, taskCreatePermission, @@ -67,6 +67,7 @@ import { } from '@backstage/plugin-scaffolder-common/alpha'; import { TaskBroker, + TaskFilters, TaskStatus, TemplateAction, TemplateFilter, @@ -80,15 +81,14 @@ import { } from '@backstage/plugin-scaffolder-node/alpha'; import { HumanDuration, JsonObject } from '@backstage/types'; import express from 'express'; -import { validate } from 'jsonschema'; import { Duration } from 'luxon'; import { pathToFileURL } from 'url'; import { v4 as uuid } from 'uuid'; import { z } from 'zod'; import { DatabaseTaskStore, + DefaultTemplateActionRegistry, TaskWorker, - TemplateActionRegistry, } from '../scaffolder'; import { createDryRunner } from '../scaffolder/dryrun'; import { StorageTaskBroker } from '../scaffolder/tasks/StorageTaskBroker'; @@ -115,25 +115,22 @@ import { } from '../util/templating'; import { createDefaultFilters } from '../lib/templating/filters/createDefaultFilters'; import { - ScaffolderPermissionRuleInput, - TaskPermissionRuleInput, - isTaskPermissionRuleInput, ActionPermissionRuleInput, isActionPermissionRuleInput, + isTaskPermissionRuleInput, isTemplatePermissionRuleInput, + ScaffolderPermissionRuleInput, + TaskPermissionRuleInput, TemplatePermissionRuleInput, } from './permissions'; import { CatalogService } from '@backstage/plugin-catalog-node'; import { scaffolderActionRules, - scaffolderTemplateRules, scaffolderTaskRules, + scaffolderTemplateRules, } from './rules'; - -import { TaskFilters } from '@backstage/plugin-scaffolder-node'; import { ActionsService } from '@backstage/backend-plugin-api/alpha'; -import { isPlainObject } from 'lodash'; /** * RouterOptions @@ -272,7 +269,11 @@ export async function createRouter( taskBroker = options.taskBroker; } - const actionRegistry = new TemplateActionRegistry(); + const actionRegistry = new DefaultTemplateActionRegistry( + actionsRegistry, + auth, + logger, + ); const templateExtensions = { additionalTemplateFilters: convertFiltersToRecord( @@ -306,48 +307,10 @@ export async function createRouter( workers.push(worker); } - // TODO(blam): it's a little unfortunate that you have to restart the scaffolder - // backend in order to pick these up. We should really just make `ActionsRegistry.get()` async - // and then we can move this logic into the there instead. - // But we can't make those changes until next major. - // Alternatively, we could look at setting up a periodic task that refreshes the actions registry, but - // not feeling that it's worth the complexity. - const { actions: distributedActions } = await actionsRegistry.list({ - credentials: await auth.getOwnServiceCredentials(), - }); - for (const action of actions) { actionRegistry.register(action); } - for (const action of distributedActions) { - actionRegistry.register({ - id: action.id, - description: action.description, - examples: [], - supportsDryRun: - action.attributes?.readOnly === true && - action.attributes?.destructive === false, - handler: async ctx => { - const { output } = await actionsRegistry.invoke({ - id: action.id, - input: ctx.input, - credentials: await ctx.getInitiatorCredentials(), - }); - - if (isPlainObject(output)) { - for (const [key, value] of Object.entries(output as JsonObject)) { - ctx.output(key as keyof typeof output, value); - } - } - }, - schema: { - input: action.schema.input as Schema, - output: action.schema.output as Schema, - }, - }); - } - const launchWorkers = () => workers.forEach(worker => worker.start()); const shutdownWorkers = async () => { @@ -479,16 +442,20 @@ export async function createRouter( eventId: 'action-fetch', request: req, }); + const credentials = await httpAuth.credentials(req); try { - const actionsList = actionRegistry.list().map(action => { - return { - id: action.id, - description: action.description, - examples: action.examples, - schema: action.schema, - }; - }); + const list = await actionRegistry.list({ credentials }); + const actionsList = Array.from(list.values()) + .map(action => { + return { + id: action.id, + description: action.description, + examples: action.examples, + schema: action.schema, + }; + }) + .sort((a, b) => a.id.localeCompare(b.id)); await auditorEvent?.success(); From a39f28c17a73d13e92fb3272a07d972fddfb93a7 Mon Sep 17 00:00:00 2001 From: Hellgren Heikki Date: Wed, 24 Sep 2025 09:48:22 +0300 Subject: [PATCH 2/2] fix(scaffolder): use initiator credentials for getting action Signed-off-by: Hellgren Heikki --- .../actions/TemplateActionRegistry.test.ts | 78 ++++++++----------- .../actions/TemplateActionRegistry.ts | 25 +++--- .../dryrun/DecoratedActionsRegistry.ts | 11 ++- .../tasks/NunjucksWorkflowRunner.test.ts | 1 - .../tasks/NunjucksWorkflowRunner.ts | 6 +- .../src/scaffolder/tasks/TaskWorker.ts | 2 +- .../scaffolder-backend/src/service/router.ts | 1 - 7 files changed, 60 insertions(+), 64 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts index f3a3b15ec9..c5d437d57b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts @@ -23,17 +23,14 @@ import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha' describe('DefaultTemplateActionRegistry', () => { let registry: DefaultTemplateActionRegistry; let mockActionsService: ReturnType; - let mockAuthService: ReturnType; let mockLogger: ReturnType; beforeEach(() => { mockActionsService = actionsRegistryServiceMock(); - mockAuthService = mockServices.auth(); mockLogger = mockServices.logger.mock(); registry = new DefaultTemplateActionRegistry( mockActionsService, - mockAuthService, mockLogger, ); }); @@ -80,7 +77,9 @@ describe('DefaultTemplateActionRegistry', () => { }); registry.register(action); - const result = await registry.get('test-action'); + const result = await registry.get('test-action', { + credentials: mockCredentials.user(), + }); expect(result).toBe(action); }); @@ -101,7 +100,9 @@ describe('DefaultTemplateActionRegistry', () => { action: async () => ({ output: {} }), }); - const result = await registry.get('test:service-action'); + const result = await registry.get('test:service-action', { + credentials: mockCredentials.user(), + }); expect(result.id).toBe('test:service-action'); expect(result.description).toBe('Service action'); @@ -109,28 +110,26 @@ describe('DefaultTemplateActionRegistry', () => { }); it('should throw NotFoundError when action is not found', async () => { - await expect(registry.get('non-existent-action')).rejects.toThrow( - NotFoundError, - ); - await expect(registry.get('non-existent-action')).rejects.toThrow( + await expect( + registry.get('non-existent-action', { + credentials: mockCredentials.user(), + }), + ).rejects.toThrow(NotFoundError); + await expect( + registry.get('non-existent-action', { + credentials: mockCredentials.user(), + }), + ).rejects.toThrow( "Template action with ID 'non-existent-action' is not registered", ); }); - - it('should use own service credentials when getting action', async () => { - const spy = jest.spyOn(mockActionsService, 'list'); - - await expect(registry.get('non-existent')).rejects.toThrow(); - - expect(spy).toHaveBeenCalledWith({ - credentials: await mockAuthService.getOwnServiceCredentials(), - }); - }); }); describe('list', () => { it('should return empty map when no actions are registered', async () => { - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); expect(result).toBeInstanceOf(Map); expect(result.size).toBe(0); @@ -152,7 +151,9 @@ describe('DefaultTemplateActionRegistry', () => { registry.register(action1); registry.register(action2); - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); expect(result.size).toBe(2); expect(result.get('action-1')).toBe(action1); @@ -175,7 +176,9 @@ describe('DefaultTemplateActionRegistry', () => { action: async () => ({ output: {} }), }); - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); expect(result.size).toBe(1); const action = result.get('test:service-action'); @@ -204,7 +207,9 @@ describe('DefaultTemplateActionRegistry', () => { }); registry.register(localAction); - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); expect(result.get('test:same-id')).toBe(localAction); expect(mockLogger.warn).toHaveBeenCalledWith( @@ -212,25 +217,6 @@ describe('DefaultTemplateActionRegistry', () => { ); }); - it('should use provided credentials when listing actions', async () => { - const credentials = mockCredentials.user(); - const spy = jest.spyOn(mockActionsService, 'list'); - - await registry.list({ credentials }); - - expect(spy).toHaveBeenCalledWith({ credentials }); - }); - - it('should use own service credentials when no credentials provided', async () => { - const spy = jest.spyOn(mockActionsService, 'list'); - - await registry.list(); - - expect(spy).toHaveBeenCalledWith({ - credentials: await mockAuthService.getOwnServiceCredentials(), - }); - }); - it('should set supportsDryRun to false for destructive actions', async () => { mockActionsService.register({ name: 'destructive-action', @@ -247,7 +233,9 @@ describe('DefaultTemplateActionRegistry', () => { action: async () => ({ output: {} }), }); - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); const action = result.get('test:destructive-action'); expect(action!.supportsDryRun).toBe(false); @@ -269,7 +257,9 @@ describe('DefaultTemplateActionRegistry', () => { action: async () => ({ output: {} }), }); - const result = await registry.list(); + const result = await registry.list({ + credentials: mockCredentials.user(), + }); const action = result.get('test:non-readonly-action'); expect(action!.supportsDryRun).toBe(false); diff --git a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts index 7bcf120646..dab1447eee 100644 --- a/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts +++ b/plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts @@ -18,7 +18,6 @@ import { ConflictError, NotFoundError } from '@backstage/errors'; import { TemplateAction } from '@backstage/plugin-scaffolder-node'; import { ActionsService } from '@backstage/backend-plugin-api/alpha'; import { - AuthService, BackstageCredentials, LoggerService, } from '@backstage/backend-plugin-api'; @@ -31,9 +30,12 @@ import { JsonObject } from '@backstage/types'; */ export interface TemplateActionRegistry { register(action: TemplateAction): void; - get(actionId: string): Promise>; - list(options?: { - credentials?: BackstageCredentials; + get( + actionId: string, + options: { credentials: BackstageCredentials }, + ): Promise>; + list(options: { + credentials: BackstageCredentials; }): Promise>>; } @@ -45,7 +47,6 @@ export class DefaultTemplateActionRegistry implements TemplateActionRegistry { constructor( private readonly actionsRegistry: ActionsService, - private readonly auth: AuthService, private readonly logger: LoggerService, ) {} @@ -59,8 +60,11 @@ export class DefaultTemplateActionRegistry implements TemplateActionRegistry { this.actions.set(action.id, action); } - async get(actionId: string): Promise> { - const action = (await this.list()).get(actionId); + async get( + actionId: string, + options: { credentials: BackstageCredentials }, + ): Promise> { + const action = (await this.list(options)).get(actionId); if (!action) { throw new NotFoundError( `Template action with ID '${actionId}' is not registered. See https://backstage.io/docs/features/software-templates/builtin-actions/ on how to add a new action module.`, @@ -69,14 +73,13 @@ export class DefaultTemplateActionRegistry implements TemplateActionRegistry { return action; } - async list(options?: { - credentials?: BackstageCredentials; + async list(options: { + credentials: BackstageCredentials; }): Promise>> { const ret = new Map(this.actions); const { actions } = await this.actionsRegistry.list({ - credentials: - options?.credentials ?? (await this.auth.getOwnServiceCredentials()), + credentials: options.credentials, }); for (const action of actions) { diff --git a/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts b/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts index b3f4b5e2cf..edbe28a0f5 100644 --- a/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts +++ b/plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts @@ -31,9 +31,12 @@ export class DecoratedActionsRegistry implements TemplateActionRegistry { } } - async get(actionId: string): Promise { + async get( + actionId: string, + options: { credentials: BackstageCredentials }, + ): Promise { try { - return await this.innerRegistry.get(actionId); + return await this.innerRegistry.get(actionId, options); } catch (e) { if (!this.innerActions.has(actionId)) { throw e; @@ -42,8 +45,8 @@ export class DecoratedActionsRegistry implements TemplateActionRegistry { } } - async list(options?: { - credentials?: BackstageCredentials; + async list(options: { + credentials: BackstageCredentials; }): Promise>> { const inner = await this.innerRegistry.list(options); return new Map>([ diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts index 886ddda36f..2c661fab56 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts @@ -110,7 +110,6 @@ describe('NunjucksWorkflowRunner', () => { actionRegistry = new DefaultTemplateActionRegistry( actionsRegistryServiceMock(), - mockServices.auth(), mockServices.logger.mock(), ); fakeActionHandler = jest.fn(); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts index d13fa58a94..4d070daa52 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts @@ -33,7 +33,7 @@ import { SecureTemplater, SecureTemplateRenderer, } from '../../lib/templating/SecureTemplater'; -import { TemplateActionRegistry } from '../actions/TemplateActionRegistry.ts'; +import { TemplateActionRegistry } from '../actions/TemplateActionRegistry'; import { generateExampleOutput, isTruthy } from './helper'; import { TaskTrackType, WorkflowResponse, WorkflowRunner } from './types'; @@ -245,7 +245,9 @@ export class NunjucksWorkflowRunner implements WorkflowRunner { return; } const action: TemplateAction = - await this.options.actionRegistry.get(step.action); + await this.options.actionRegistry.get(step.action, { + credentials: await task.getInitiatorCredentials(), + }); const { taskLogger } = createStepLogger({ task, step, diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts index 7b85dd1270..adfdf0b553 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/TaskWorker.ts @@ -25,7 +25,7 @@ import { TemplateGlobal, } from '@backstage/plugin-scaffolder-node'; import PQueue from 'p-queue'; -import { TemplateActionRegistry } from '../actions/TemplateActionRegistry.ts'; +import { TemplateActionRegistry } from '../actions/TemplateActionRegistry'; import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner'; import { WorkflowRunner } from './types'; import { setTimeout } from 'timers/promises'; diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index bcdcedb70d..3894ffb876 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -271,7 +271,6 @@ export async function createRouter( const actionRegistry = new DefaultTemplateActionRegistry( actionsRegistry, - auth, logger, );