From 152ae1e3c55e76833403b8d0f70425092b152a3a Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Wed, 12 Mar 2025 18:35:41 -0400 Subject: [PATCH 01/16] feat: initial implementation of scaffolder granular permissions Signed-off-by: Kashish Mittal Co-authored-by: Frank Kong --- .../scaffolder-backend/src/service/alpha.ts | 27 +++- .../src/service/permissions.ts | 35 +++++ .../src/service/router.test.ts | 124 ++++++++++++++++++ .../scaffolder-backend/src/service/router.ts | 124 ++++++++++++++---- .../src/util/checkPermissions.ts | 66 ++++++++++ plugins/scaffolder-common/src/permissions.ts | 9 ++ .../ScaffolderPageContextMenu.tsx | 7 +- .../ListTasksPage/ListTasksPage.tsx | 24 ++-- .../components/OngoingTask/ContextMenu.tsx | 3 + .../components/OngoingTask/OngoingTask.tsx | 5 +- .../src/components/Router/Router.tsx | 19 +-- 11 files changed, 384 insertions(+), 59 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/alpha.ts b/plugins/scaffolder-backend/src/service/alpha.ts index 7d22c9d262..ac14f0b1ba 100644 --- a/plugins/scaffolder-backend/src/service/alpha.ts +++ b/plugins/scaffolder-backend/src/service/alpha.ts @@ -17,9 +17,14 @@ import { RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, RESOURCE_TYPE_SCAFFOLDER_ACTION, + RESOURCE_TYPE_SCAFFOLDER_TASK, } from '@backstage/plugin-scaffolder-common/alpha'; import { createConditionExports } from '@backstage/plugin-permission-node'; -import { scaffolderTemplateRules, scaffolderActionRules } from './rules'; +import { + scaffolderTemplateRules, + scaffolderActionRules, + scaffolderTaskRules, +} from './rules'; const templateConditionExports = createConditionExports({ pluginId: 'scaffolder', @@ -33,6 +38,12 @@ const actionsConditionExports = createConditionExports({ rules: scaffolderActionRules, }); +const taskConditionExports = createConditionExports({ + pluginId: 'scaffolder', + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, + rules: scaffolderTaskRules, +}); + /** * `createScaffolderTemplateConditionalDecision` can be used when authoring policies to * create conditional decisions. It requires a permission of type @@ -90,3 +101,17 @@ export const createScaffolderActionConditionalDecision = * @alpha */ export const scaffolderActionConditions = actionsConditionExports.conditions; + +/** + * @alpha + */ +export const createScaffolderTaskConditionalDecision = + taskConditionExports.createConditionalDecision; + +/** + * These conditions are used when creating conditional decisions for scaffolder + * tasks that are returned by authorization policies. + * + * @alpha + */ +export const scaffolderTaskConditions = taskConditionExports.conditions; diff --git a/plugins/scaffolder-backend/src/service/permissions.ts b/plugins/scaffolder-backend/src/service/permissions.ts index 9b12e115b9..4843fcb51e 100644 --- a/plugins/scaffolder-backend/src/service/permissions.ts +++ b/plugins/scaffolder-backend/src/service/permissions.ts @@ -21,9 +21,24 @@ import { } from '@backstage/plugin-scaffolder-common'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION, + RESOURCE_TYPE_SCAFFOLDER_TASK, RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, } from '@backstage/plugin-scaffolder-common/alpha'; import { PermissionRuleParams } from '@backstage/plugin-permission-common'; +import { + SerializedTask, + TaskFilter, + TaskFilters, +} from '@backstage/plugin-scaffolder-node'; + +/** + * + * @public + */ +export type ScaffolderPermissionRuleInput = + | TemplatePermissionRuleInput + | ActionPermissionRuleInput + | TaskPermissionRuleInput; /** * @public @@ -59,3 +74,23 @@ export function isActionPermissionRuleInput( ): permissionRule is ActionPermissionRuleInput { return permissionRule.resourceType === RESOURCE_TYPE_SCAFFOLDER_ACTION; } + +/** + * @public + */ +export type TaskPermissionRuleInput< + TParams extends PermissionRuleParams = PermissionRuleParams, +> = PermissionRule< + SerializedTask, + { + property: TaskFilter['property']; + values: any; + }, + typeof RESOURCE_TYPE_SCAFFOLDER_TASK, + TParams +>; +export function isTaskPermissionRuleInput( + permissionRule: ScaffolderPermissionRuleInput, +): permissionRule is TaskPermissionRuleInput { + return permissionRule.resourceType === RESOURCE_TYPE_SCAFFOLDER_TASK; +} \ No newline at end of file diff --git a/plugins/scaffolder-backend/src/service/router.test.ts b/plugins/scaffolder-backend/src/service/router.test.ts index 679ac44640..661939eaf2 100644 --- a/plugins/scaffolder-backend/src/service/router.test.ts +++ b/plugins/scaffolder-backend/src/service/router.test.ts @@ -945,6 +945,36 @@ describe('scaffolder router', () => { order: [{ order: 'desc', field: 'created_at' }], }); }); + + it('disallows users from seeing tasks they do not own', async () => { + const { router, taskBroker, permissions } = await createTestRouter(); + jest + .spyOn(permissions, 'authorizeConditional') + .mockImplementationOnce(async () => [ + { + conditions: { + resourceType: 'scaffolder-task', + rule: 'IS_TASK_OWNER', + params: { createdBy: ['user'] }, + }, + pluginId: 'scaffolder', + resourceType: 'scaffolder-task', + result: AuthorizeResult.CONDITIONAL, + }, + ]); + const response = await request(router).get( + `/v2/tasks?createdBy=not-user`, + ); + expect(taskBroker.list).toHaveBeenCalledWith({ + filters: { createdBy: ['not-user'], status: undefined }, + order: undefined, + pagination: { limit: undefined, offset: undefined }, + permissionFilters: { key: 'created_by', values: ['user'] }, + }); + expect(response.status).toBe(200); + expect(response.body.totalTasks).toBe(0); + expect(response.body.tasks).toEqual([]); + }); }); describe('GET /v2/tasks/:taskId', () => { @@ -966,6 +996,37 @@ describe('scaffolder router', () => { expect(response.body.status).toBe('completed'); expect(response.body.secrets).toBeUndefined(); }); + it('disallows users from seeing tasks they do not own', async () => { + const { router, permissions, taskBroker } = await createTestRouter(); + jest + .spyOn(permissions, 'authorizeConditional') + .mockImplementationOnce(async () => [ + { + conditions: { + resourceType: 'scaffolder-task', + rule: 'IS_TASK_OWNER', + params: { createdBy: ['user'] }, + }, + pluginId: 'scaffolder', + resourceType: 'scaffolder-task', + result: AuthorizeResult.CONDITIONAL, + }, + ]); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: 'not-user', + }); + + const response = await request(router).get(`/v2/tasks/a-random-id`); + expect(taskBroker.get).toHaveBeenCalledWith('a-random-id'); + expect(response.error).not.toBeFalsy(); + }); }); describe('GET /v2/tasks/:taskId/eventstream', () => { @@ -1206,6 +1267,40 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ }); expect(subscriber!.closed).toBe(true); }); + it('disallows users from seeing events for tasks they do not own', async () => { + const { permissions, router, taskBroker } = await createTestRouter(); + + jest + .spyOn(permissions, 'authorizeConditional') + .mockImplementationOnce(async () => [ + { + conditions: { + resourceType: 'scaffolder-task', + rule: 'IS_TASK_OWNER', + params: { createdBy: ['user'] }, + }, + pluginId: 'scaffolder', + resourceType: 'scaffolder-task', + result: AuthorizeResult.CONDITIONAL, + }, + ]); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: 'not-user', + }); + + const response = await request(router).get( + `/v2/tasks/a-random-id/events`, + ); + expect(taskBroker.get).toHaveBeenCalledWith('a-random-id'); + expect(response.error).not.toBeFalsy(); + }); }); describe('POST /v2/dry-run', () => { @@ -1233,6 +1328,35 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ expect.anything(), ); }); + it('disallows users from seeing tasks they do not own', async () => { + const { permissions, router, taskBroker } = await createTestRouter(); + jest + .spyOn(permissions, 'authorizeConditional') + .mockImplementationOnce(async () => [ + { + conditions: { + resourceType: 'scaffolder-task', + rule: 'IS_TASK_OWNER', + params: { createdBy: ['user'] }, + }, + pluginId: 'scaffolder', + resourceType: 'scaffolder-task', + result: AuthorizeResult.CONDITIONAL, + }, + ]); + const response = await request(router).get( + `/v2/tasks?createdBy=not-user`, + ); + expect(taskBroker.list).toHaveBeenCalledWith({ + filters: { createdBy: ['not-user'], status: undefined }, + order: undefined, + pagination: { limit: undefined, offset: undefined }, + permissionFilters: { key: 'created_by', values: ['user'] }, + }); + expect(response.status).toBe(200); + expect(response.body.totalTasks).toBe(0); + expect(response.body.tasks).toEqual([]); + }); }); describe('GET /v2/autocomplete/:provider/:resource', () => { diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 0ea6293f36..b97626b5f3 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -42,6 +42,9 @@ import { EventsService } from '@backstage/plugin-events-node'; import { createConditionAuthorizer, createPermissionIntegrationRouter, + PermissionRule, + createConditionTransformer, + ConditionTransformer, } from '@backstage/plugin-permission-node'; import { TaskSpec, @@ -51,7 +54,9 @@ import { import { RESOURCE_TYPE_SCAFFOLDER_ACTION, RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, + RESOURCE_TYPE_SCAFFOLDER_TASK, scaffolderActionPermissions, + scaffolderTaskPermissions, scaffolderPermissions, scaffolderTemplatePermissions, taskCancelPermission, @@ -89,7 +94,11 @@ import { import { createDryRunner } from '../scaffolder/dryrun'; import { StorageTaskBroker } from '../scaffolder/tasks/StorageTaskBroker'; import { InternalTaskSecrets } from '../scaffolder/tasks/types'; -import { checkPermission } from '../util/checkPermissions'; +import { + checkPermission, + checkTaskPermission, + getAuthorizeConditions, +} from '../util/checkPermissions'; import { findTemplate, getEntityBaseUrl, @@ -97,7 +106,7 @@ import { parseNumberParam, parseStringsParam, } from './helpers'; -import { scaffolderActionRules, scaffolderTemplateRules } from './rules'; + import { convertFiltersToRecord, convertGlobalsToRecord, @@ -107,6 +116,9 @@ import { } from '../util/templating'; import { createDefaultFilters } from '../lib/templating/filters/createDefaultFilters'; import { + ScaffolderPermissionRuleInput, + TaskPermissionRuleInput, + isTaskPermissionRuleInput, ActionPermissionRuleInput, isActionPermissionRuleInput, isTemplatePermissionRuleInput, @@ -114,6 +126,12 @@ import { } from './permissions'; import { CatalogService } from '@backstage/plugin-catalog-node'; +import { + scaffolderActionRules, + scaffolderTemplateRules, + scaffolderTaskRules, +} from './rules'; + /** * RouterOptions */ @@ -139,11 +157,11 @@ export interface RouterOptions { | CreatedTemplateGlobal[]; additionalWorkspaceProviders?: Record; permissions?: PermissionsService; - permissionRules?: Array< - TemplatePermissionRuleInput | ActionPermissionRuleInput - >; - auth: AuthService; - httpAuth: HttpAuthService; + permissionRules?: Array; + auth?: AuthService; + httpAuth?: HttpAuthService; + identity?: IdentityApi; + discovery?: DiscoveryService; events?: EventsService; auditor?: AuditorService; autocompleteHandlers?: Record; @@ -312,15 +330,24 @@ export async function createRouter( const actionRules: ActionPermissionRuleInput[] = Object.values( scaffolderActionRules, ); + const taskRules: TaskPermissionRuleInput[] = + Object.values(scaffolderTaskRules); if (permissionRules) { templateRules.push( ...permissionRules.filter(isTemplatePermissionRuleInput), ); actionRules.push(...permissionRules.filter(isActionPermissionRuleInput)); + taskRules.push(...permissionRules.filter(isTaskPermissionRuleInput)); } - const isAuthorized = createConditionAuthorizer(Object.values(templateRules)); + const isTemplateAuthorized = createConditionAuthorizer( + Object.values(templateRules), + ); + const isTaskAuthorized = createConditionAuthorizer(Object.values(taskRules)); + + const taskTransformConditions: ConditionTransformer = + createConditionTransformer(Object.values(taskRules)); const permissionIntegrationRouter = createPermissionIntegrationRouter({ resources: [ @@ -334,6 +361,18 @@ export async function createRouter( permissions: scaffolderActionPermissions, rules: actionRules, }, + { + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, + permissions: scaffolderTaskPermissions, + rules: taskRules, + getResources: async resourceRefs => { + return Promise.all( + resourceRefs.map(async taskId => { + return await taskBroker.get(taskId); + }), + ); + }, + }, ], permissions: scaffolderPermissions, }); @@ -532,11 +571,6 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); - await checkPermission({ - credentials, - permissions: [taskReadPermission], - permissionService: permissions, - }); if (!taskBroker.list) { throw new Error( @@ -564,6 +598,13 @@ export async function createRouter( const limit = parseNumberParam(req.query.limit, 'limit'); const offset = parseNumberParam(req.query.offset, 'offset'); + const taskPermissionFilters = await getAuthorizeConditions({ + credentials: credentials, + permission: taskReadPermission, + permissionService: permissions, + transformConditions: taskTransformConditions, + }); + const tasks = await taskBroker.list({ filters: { createdBy, @@ -574,6 +615,7 @@ export async function createRouter( limit: limit ? limit[0] : undefined, offset: offset ? offset[0] : undefined, }, + permissionFilters: taskPermissionFilters, }); await auditorEvent?.success(); @@ -598,13 +640,17 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); - await checkPermission({ - credentials, - permissions: [taskReadPermission], - permissionService: permissions, - }); const task = await taskBroker.get(taskId); + + await checkTaskPermission({ + credentials, + permission: taskReadPermission, + permissionService: permissions, + task: task, + isTaskAuthorized, + }); + if (!task) { throw new NotFoundError(`Task with id ${taskId} does not exist`); } @@ -634,11 +680,13 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); - // Requires both read and cancel permissions - await checkPermission({ + const task = await taskBroker.get(taskId); + await checkTaskPermission({ credentials, - permissions: [taskCancelPermission, taskReadPermission], + permission: taskCancelPermission, permissionService: permissions, + task: task, + isTaskAuthorized, }); await taskBroker.cancel?.(taskId); @@ -666,13 +714,23 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); + const task = await taskBroker.get(taskId); + // Requires both read and cancel permissions await checkPermission({ credentials, - permissions: [taskCreatePermission, taskReadPermission], + permissions: [taskCreatePermission], permissionService: permissions, }); + await checkTaskPermission({ + credentials, + permission: taskReadPermission, + permissionService: permissions, + task: task, + isTaskAuthorized, + }); + await auditorEvent?.success(); const { token } = await auth.getPluginRequestToken({ @@ -711,10 +769,14 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); - await checkPermission({ + const task = await taskBroker.get(taskId); + + await checkTaskPermission({ credentials, - permissions: [taskReadPermission], + permission: taskReadPermission, permissionService: permissions, + task: task, + isTaskAuthorized, }); const after = @@ -783,10 +845,14 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); - await checkPermission({ + const task = await taskBroker.get(taskId); + + await checkTaskPermission({ credentials, - permissions: [taskReadPermission], + permission: taskReadPermission, permissionService: permissions, + task: task, + isTaskAuthorized, }); const after = Number(req.query.after) || undefined; @@ -1023,18 +1089,18 @@ export async function createRouter( // Authorize parameters if (Array.isArray(template.spec.parameters)) { template.spec.parameters = template.spec.parameters.filter(step => - isAuthorized(parameterDecision, step), + isTemplateAuthorized(parameterDecision, step), ); } else if ( template.spec.parameters && - !isAuthorized(parameterDecision, template.spec.parameters) + !isTemplateAuthorized(parameterDecision, template.spec.parameters) ) { template.spec.parameters = undefined; } // Authorize steps template.spec.steps = template.spec.steps.filter(step => - isAuthorized(stepDecision, step), + isTemplateAuthorized(stepDecision, step), ); return template; diff --git a/plugins/scaffolder-backend/src/util/checkPermissions.ts b/plugins/scaffolder-backend/src/util/checkPermissions.ts index b6c0395fbb..950f506e55 100644 --- a/plugins/scaffolder-backend/src/util/checkPermissions.ts +++ b/plugins/scaffolder-backend/src/util/checkPermissions.ts @@ -21,7 +21,13 @@ import { NotAllowedError } from '@backstage/errors'; import { AuthorizeResult, BasicPermission, + PermissionCriteria, + PolicyDecision, + ResourcePermission, } from '@backstage/plugin-permission-common'; +import { ConditionTransformer } from '@backstage/plugin-permission-node'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; +import { TaskFilters } from '@backstage/plugin-scaffolder-node'; export type checkPermissionOptions = { credentials: BackstageCredentials; @@ -29,6 +35,24 @@ export type checkPermissionOptions = { permissionService?: PermissionsService; }; +export type checkTaskPermissionOptions = { + credentials: BackstageCredentials; + permission: ResourcePermission; + permissionService?: PermissionsService; + task: SerializedTask; + isTaskAuthorized: ( + decision: PolicyDecision, + resource: SerializedTask | undefined, + ) => boolean; +}; + +export type authorizeConditionsOptions = { + credentials: BackstageCredentials; + permission: ResourcePermission; + permissionService?: PermissionsService; + transformConditions: ConditionTransformer; +}; + /** * Does a basic check on permissions. Throws 403 error if any permission responds with AuthorizeResult.DENY * @public @@ -51,3 +75,45 @@ export async function checkPermission(options: checkPermissionOptions) { } } } + +/** + * Does a conditional permission check for scaffolder task reading and cancellation. + * Throws 403 error if permission responds with AuthorizeResult.DENY, or does not resolve to true during the conditional rule check + * @public + */ +export async function checkTaskPermission(options: checkTaskPermissionOptions) { + const { permission, permissionService, credentials, task, isTaskAuthorized } = + options; + if (permissionService) { + const [taskDecision] = await permissionService.authorizeConditional( + [{ permission: permission }], + { credentials }, + ); + if ( + taskDecision.result === AuthorizeResult.DENY || + !isTaskAuthorized(taskDecision, task) + ) { + throw new NotAllowedError(); + } + } +} + +/** Fetches and transforms authorization conditions into filters, or returns `undefined` if the decision is not conditional. + * @public + */ +export const getAuthorizeConditions = async ( + options: authorizeConditionsOptions, +): Promise | undefined> => { + const { permission, permissionService, credentials, transformConditions } = + options; + if (permissionService) { + const [taskDecision] = await permissionService.authorizeConditional( + [{ permission: permission }], + { credentials }, + ); + if (taskDecision.result === AuthorizeResult.CONDITIONAL) { + return transformConditions(taskDecision.conditions); + } + } + return undefined; +}; diff --git a/plugins/scaffolder-common/src/permissions.ts b/plugins/scaffolder-common/src/permissions.ts index e758291e75..36c72072e9 100644 --- a/plugins/scaffolder-common/src/permissions.ts +++ b/plugins/scaffolder-common/src/permissions.ts @@ -30,6 +30,13 @@ export const RESOURCE_TYPE_SCAFFOLDER_TEMPLATE = 'scaffolder-template'; */ export const RESOURCE_TYPE_SCAFFOLDER_ACTION = 'scaffolder-action'; +/** + * Permission resource type which corresponds to scaffolder tasks + * + * @alpha + */ +export const RESOURCE_TYPE_SCAFFOLDER_TASK = 'scaffolder-task'; + /** * This permission is used to authorize actions that involve executing * an action from a template. @@ -89,6 +96,7 @@ export const taskReadPermission = createPermission({ attributes: { action: 'read', }, + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, }); /** @@ -111,6 +119,7 @@ export const taskCreatePermission = createPermission({ export const taskCancelPermission = createPermission({ name: 'scaffolder.task.cancel', attributes: {}, + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, }); /** diff --git a/plugins/scaffolder-react/src/next/components/ScaffolderPageContextMenu/ScaffolderPageContextMenu.tsx b/plugins/scaffolder-react/src/next/components/ScaffolderPageContextMenu/ScaffolderPageContextMenu.tsx index 9546792983..2df4ff3b71 100644 --- a/plugins/scaffolder-react/src/next/components/ScaffolderPageContextMenu/ScaffolderPageContextMenu.tsx +++ b/plugins/scaffolder-react/src/next/components/ScaffolderPageContextMenu/ScaffolderPageContextMenu.tsx @@ -30,7 +30,6 @@ import Functions from '@material-ui/icons/Functions'; import MoreVert from '@material-ui/icons/MoreVert'; import { SyntheticEvent, useState } from 'react'; import { usePermission } from '@backstage/plugin-permission-react'; -import { taskReadPermission } from '@backstage/plugin-scaffolder-common/alpha'; import { templateManagementPermission } from '@backstage/plugin-scaffolder-common/alpha'; import { scaffolderReactTranslationRef } from '../../../translation'; @@ -69,10 +68,6 @@ export function ScaffolderPageContextMenu( const classes = useStyles(); const [anchorEl, setAnchorEl] = useState(); - const { allowed: canReadTasks } = usePermission({ - permission: taskReadPermission, - }); - const { allowed: canManageTemplates } = usePermission({ permission: templateManagementPermission, }); @@ -164,7 +159,7 @@ export function ScaffolderPageContextMenu( /> )} - {onTasksClicked && canReadTasks && ( + {onTasksClicked && ( diff --git a/plugins/scaffolder/src/components/ListTasksPage/ListTasksPage.tsx b/plugins/scaffolder/src/components/ListTasksPage/ListTasksPage.tsx index 6d48cec125..d6b5b7116e 100644 --- a/plugins/scaffolder/src/components/ListTasksPage/ListTasksPage.tsx +++ b/plugins/scaffolder/src/components/ListTasksPage/ListTasksPage.tsx @@ -91,14 +91,22 @@ const ListTaskPageContent = (props: MyTaskPageProps) => { if (error) { return ( - <> - - - + + + setOwnerFilter(id)} + /> + + + + + + ); } diff --git a/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx b/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx index e1a5f37ddc..ee27053a99 100644 --- a/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx @@ -46,6 +46,7 @@ type ContextMenuProps = { onStartOver?: () => void; onToggleLogs?: (state: boolean) => void; onToggleButtonBar?: (state: boolean) => void; + taskId?: string; isCancelButtonDisabled: boolean; onCancel: () => void; }; @@ -67,6 +68,7 @@ export const ContextMenu = (props: ContextMenuProps) => { onStartOver, onToggleLogs, onToggleButtonBar, + taskId } = props; const { getPageTheme } = useTheme(); const pageTheme = getPageTheme({ themeId: 'website' }); @@ -76,6 +78,7 @@ export const ContextMenu = (props: ContextMenuProps) => { const { allowed: canReadTask } = usePermission({ permission: taskReadPermission, + resourceRef: taskId }); const { allowed: canCreateTask } = usePermission({ diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx index c9a119e4a1..c01aa79441 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx @@ -46,7 +46,7 @@ import { TaskSteps, } from '@backstage/plugin-scaffolder-react/alpha'; import { useAsync } from '@react-hookz/web'; -import { usePermission } from '@backstage/plugin-permission-react'; +import { usePermission} from '@backstage/plugin-permission-react'; import { taskCancelPermission, taskCreatePermission, @@ -142,10 +142,12 @@ function OngoingTaskContent(props: { // Used dummy string value for `resourceRef` since `allowed` field will always return `false` if `resourceRef` is `undefined` const { allowed: canCancelTask } = usePermission({ permission: taskCancelPermission, + resourceRef: taskId, }); const { allowed: canReadTask } = usePermission({ permission: taskReadPermission, + resourceRef: taskId, }); const { allowed: canCreateTask } = usePermission({ @@ -269,6 +271,7 @@ function OngoingTaskContent(props: { onRetry={triggerRetry} onToggleLogs={setLogVisibleState} onToggleButtonBar={setButtonBarVisibleState} + taskId={taskId} onCancel={triggerCancel} isCancelButtonDisabled={isCancelButtonDisabled} /> diff --git a/plugins/scaffolder/src/components/Router/Router.tsx b/plugins/scaffolder/src/components/Router/Router.tsx index 91567c1ddd..d8dee73849 100644 --- a/plugins/scaffolder/src/components/Router/Router.tsx +++ b/plugins/scaffolder/src/components/Router/Router.tsx @@ -60,10 +60,7 @@ import { CustomFieldsPage, } from '../../alpha/components/TemplateEditorPage'; import { RequirePermission } from '@backstage/plugin-permission-react'; -import { - taskReadPermission, - templateManagementPermission, -} from '@backstage/plugin-scaffolder-common/alpha'; +import { templateManagementPermission } from '@backstage/plugin-scaffolder-common/alpha'; import { useApp } from '@backstage/core-plugin-api'; import { FormField, OpaqueFormField } from '@internal/scaffolder'; import { useAsync, useMountEffect } from '@react-hookz/web'; @@ -182,11 +179,9 @@ export const InternalRouter = ( - - + } /> - - - } + element={} /> Date: Wed, 12 Mar 2025 19:28:29 -0400 Subject: [PATCH 02/16] added files related to db queries, api-reports and changeset Signed-off-by: Kashish Mittal --- .changeset/weak-bags-behave.md | 11 ++ .../scaffolder-backend/report-alpha.api.md | 34 ++++ plugins/scaffolder-backend/report.api.md | 3 + .../tasks/DatabaseTaskStore.test.ts | 69 ++++++++ .../src/scaffolder/tasks/DatabaseTaskStore.ts | 95 ++++++++++- .../src/service/permissions.ts | 5 +- .../src/service/router.test.ts | 1 + .../scaffolder-backend/src/service/router.ts | 11 +- .../src/service/rules.test.ts | 159 ++++++++++++++++++ .../scaffolder-backend/src/service/rules.ts | 64 +++++++ plugins/scaffolder-common/report-alpha.api.md | 13 +- plugins/scaffolder-node/package.json | 1 + plugins/scaffolder-node/report.api.md | 21 +++ plugins/scaffolder-node/src/tasks/index.ts | 2 + plugins/scaffolder-node/src/tasks/types.ts | 21 +++ .../components/OngoingTask/OngoingTask.tsx | 1 - yarn.lock | 1 + 17 files changed, 493 insertions(+), 19 deletions(-) create mode 100644 .changeset/weak-bags-behave.md diff --git a/.changeset/weak-bags-behave.md b/.changeset/weak-bags-behave.md new file mode 100644 index 0000000000..2c7bc1d37c --- /dev/null +++ b/.changeset/weak-bags-behave.md @@ -0,0 +1,11 @@ +--- +'@backstage/plugin-scaffolder-backend': minor +'@backstage/plugin-scaffolder-common': minor +'@backstage/plugin-scaffolder-react': minor +'@backstage/plugin-scaffolder-node': minor +'@backstage/plugin-scaffolder': minor +--- + +BREAKING : Added two new scaffolder rules for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on creators (`hasCreatedBy`) and granting template owners visibility into all runs of their templates (`hasTemplateEntityRefs`). + +BREAKING: Removed requirement to have both `scaffolder.task.read` and `scaffolder.task.cancel` permissions to cancel tasks. diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index 958d68fc25..cd9ca2eb00 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -10,6 +10,8 @@ import { PermissionCondition } from '@backstage/plugin-permission-common'; import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { ResourcePermission } from '@backstage/plugin-permission-common'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; +import { TaskFilter } from '@backstage/plugin-scaffolder-node'; import { TemplateEntityStepV1beta3 } from '@backstage/plugin-scaffolder-common'; import { TemplateParametersV1beta3 } from '@backstage/plugin-scaffolder-common'; @@ -19,6 +21,12 @@ export const createScaffolderActionConditionalDecision: ( conditions: PermissionCriteria>, ) => ConditionalPolicyDecision; +// @alpha (undocumented) +export const createScaffolderTaskConditionalDecision: ( + permission: ResourcePermission<'scaffolder-task'>, + conditions: PermissionCriteria>, +) => ConditionalPolicyDecision; + // @alpha export const createScaffolderTemplateConditionalDecision: ( permission: ResourcePermission<'scaffolder-template'>, @@ -76,6 +84,32 @@ export const scaffolderActionConditions: Conditions<{ >; }>; +// @alpha +export const scaffolderTaskConditions: Conditions<{ + hasCreatedBy: PermissionRule< + SerializedTask, + { + property: TaskFilter['property']; + values: any; + }, + 'scaffolder-task', + { + createdBy: string[]; + } + >; + hasTemplateEntityRefs: PermissionRule< + SerializedTask, + { + property: TaskFilter['property']; + values: any; + }, + 'scaffolder-task', + { + templateEntityRefs: string[]; + } + >; +}>; + // @alpha export const scaffolderTemplateConditions: Conditions<{ hasTag: PermissionRule< diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index 1f2a17a384..9da363e872 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -17,6 +17,7 @@ import { JsonObject } from '@backstage/types'; import { JsonValue } from '@backstage/types'; import { Knex } from 'knex'; import { LoggerService } from '@backstage/backend-plugin-api'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionEvaluator } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { PermissionRuleParams } from '@backstage/plugin-permission-common'; @@ -28,6 +29,7 @@ import { SerializedTaskEvent } from '@backstage/plugin-scaffolder-node'; import { TaskBroker } from '@backstage/plugin-scaffolder-node'; import { TaskCompletionState } from '@backstage/plugin-scaffolder-node'; import { TaskContext } from '@backstage/plugin-scaffolder-node'; +import { TaskFilters } from '@backstage/plugin-scaffolder-node'; import { TaskRecovery } from '@backstage/plugin-scaffolder-common'; import { TaskSecrets } from '@backstage/plugin-scaffolder-node'; import { TaskSpec } from '@backstage/plugin-scaffolder-common'; @@ -351,6 +353,7 @@ export class DatabaseTaskStore implements TaskStore { order: 'asc' | 'desc'; field: string; }[]; + permissionFilters?: PermissionCriteria; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts index eb0418b5ed..2d9b4db124 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts @@ -22,6 +22,8 @@ import { ConflictError } from '@backstage/errors'; import { createMockDirectory } from '@backstage/backend-test-utils'; import fs from 'fs-extra'; import { EventsService } from '@backstage/plugin-events-node'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { TaskFilters } from '@backstage/plugin-scaffolder-node'; const createStore = async (events?: EventsService) => { const manager = DatabaseManager.fromConfig( @@ -231,6 +233,73 @@ describe('DatabaseTaskStore', () => { expect(tasks[0].id).toBeDefined(); }); + it('should filter tasks based on permissionFilters', async () => { + const { store } = await createStore(); + + await store.createTask({ + spec: { + templateInfo: { entityRef: 'template:default/three' }, + } as TaskSpec, + createdBy: 'user:default/one', + }); + + await store.createTask({ + spec: { + templateInfo: { entityRef: 'template:default/four' }, + } as TaskSpec, + createdBy: 'user:default/two', + }); + + await store.createTask({ + spec: { + templateInfo: { entityRef: 'template:default/one' }, + } as TaskSpec, + createdBy: 'user:default/three', + }); + + await store.createTask({ + spec: { + templateInfo: { entityRef: 'template:default/two' }, + } as TaskSpec, + createdBy: 'user:default/three', + }); + + await store.createTask({ + spec: { + templateInfo: { entityRef: 'template:default/three' }, + } as TaskSpec, + createdBy: 'user:default/three', + }); + + const permissionFilters: PermissionCriteria = { + anyOf: [ + { + property: 'createdBy', + values: ['user:default/one', 'user:default/two'], + }, + { + property: 'templateEntityRefs', + values: ['template:default/one', 'template:default/two'], + }, + ], + }; + + const { tasks, totalTasks } = await store.list({ + permissionFilters: permissionFilters, + }); + + expect(totalTasks).toBe(4); + + expect(tasks).not.toEqual( + expect.arrayContaining([ + expect.objectContaining({ + createdBy: 'user:default/three', + spec: { templateInfo: { entityRef: 'template:default/three' } }, + }), + ]), + ); + }); + it('should sent an event to start cancelling the task', async () => { const { store } = await createStore(eventsService); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index f81a79a5a0..b9b5fd0d7c 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -35,6 +35,7 @@ import { SerializedTask, SerializedTaskEvent, TaskEventType, + TaskFilter, TaskSecrets, TaskStatus, } from '@backstage/plugin-scaffolder-node'; @@ -48,6 +49,14 @@ import { } from '@backstage/plugin-scaffolder-node/alpha'; import { flattenParams } from '../../service/helpers'; import { EventsService } from '@backstage/plugin-events-node'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; +import { + isAndCriteria, + isNotCriteria, + isOrCriteria, +} from '@backstage/plugin-permission-node'; +import { TaskFilters } from '@backstage/plugin-scaffolder-node'; +import { compact } from 'lodash'; const migrationsDir = resolvePackagePath( '@backstage/plugin-scaffolder-backend', @@ -195,6 +204,63 @@ export class DatabaseTaskStore implements TaskStore { } } + private isTaskFilter(filter: any): filter is TaskFilter { + return filter.hasOwnProperty('property'); + } + + private parseFilter( + filter: PermissionCriteria, + query: Knex.QueryBuilder, + db: Knex, + negate: boolean = false, + ): Knex.QueryBuilder { + // handle not criteria + if (isNotCriteria(filter)) { + return this.parseFilter(filter.not, query, db, !negate); + } + + if (this.isTaskFilter(filter)) { + const values: string[] = compact(filter.values) ?? []; + + if (filter.property === 'createdBy') { + query.whereIn('created_by', [...new Set(values)]); + } + + if (filter.property === 'templateEntityRefs' && values.length > 0) { + const dbClient = this.db.client.config.client; + const placeholders = values.map(() => '?').join(', '); + if (dbClient === 'pg') { + query.whereRaw( + `spec::jsonb->'templateInfo'->>'entityRef' IN (${placeholders})`, + values, + ); + } else if (dbClient === 'better-sqlite3') { + query.whereRaw( + `json_extract(spec, '$.templateInfo.entityRef') IN (${placeholders})`, + values, + ); + } + } + return query; + } + + return query[negate ? 'andWhereNot' : 'andWhere'](subQuery => { + if (isOrCriteria(filter)) { + for (const subFilter of filter.anyOf ?? []) { + subQuery.orWhere(subQueryInner => + this.parseFilter(subFilter, subQueryInner, db, false), + ); + } + } else if (isAndCriteria(filter)) { + for (const subFilter of filter.allOf ?? []) { + subQuery.andWhere(subQueryInner => + this.parseFilter(subFilter, subQueryInner, db, false), + ); + } + } + }); + } + async list(options: { createdBy?: string; status?: TaskStatus; @@ -207,16 +273,31 @@ export class DatabaseTaskStore implements TaskStore { offset?: number; }; order?: { order: 'asc' | 'desc'; field: string }[]; + permissionFilters?: PermissionCriteria; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number }> { - const { createdBy, status, pagination, order, filters } = options ?? {}; + const { createdBy, status, pagination, order, filters, permissionFilters } = + options ?? {}; const queryBuilder = this.db('tasks'); - if (createdBy || filters?.createdBy) { - const arr: string[] = flattenParams( - createdBy, - filters?.createdBy, - ); - queryBuilder.whereIn('created_by', [...new Set(arr)]); + const createdByValues = flattenParams( + createdBy, + filters?.createdBy, + ); + + const combinedPermissionFilters: + | PermissionCriteria + | undefined = + createdByValues.length > 0 + ? { + allOf: [ + { property: 'createdBy', values: createdByValues }, + ...(permissionFilters ? [permissionFilters] : []), + ], + } + : permissionFilters; + + if (combinedPermissionFilters) { + this.parseFilter(combinedPermissionFilters, queryBuilder, this.db); } if (status || filters?.status) { diff --git a/plugins/scaffolder-backend/src/service/permissions.ts b/plugins/scaffolder-backend/src/service/permissions.ts index 4843fcb51e..cd10cabde0 100644 --- a/plugins/scaffolder-backend/src/service/permissions.ts +++ b/plugins/scaffolder-backend/src/service/permissions.ts @@ -28,7 +28,6 @@ import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { SerializedTask, TaskFilter, - TaskFilters, } from '@backstage/plugin-scaffolder-node'; /** @@ -52,7 +51,7 @@ export type TemplatePermissionRuleInput< TParams >; export function isTemplatePermissionRuleInput( - permissionRule: TemplatePermissionRuleInput | ActionPermissionRuleInput, + permissionRule: ScaffolderPermissionRuleInput, ): permissionRule is TemplatePermissionRuleInput { return permissionRule.resourceType === RESOURCE_TYPE_SCAFFOLDER_TEMPLATE; } @@ -70,7 +69,7 @@ export type ActionPermissionRuleInput< TParams >; export function isActionPermissionRuleInput( - permissionRule: TemplatePermissionRuleInput | ActionPermissionRuleInput, + permissionRule: ScaffolderPermissionRuleInput, ): permissionRule is ActionPermissionRuleInput { return permissionRule.resourceType === RESOURCE_TYPE_SCAFFOLDER_ACTION; } diff --git a/plugins/scaffolder-backend/src/service/router.test.ts b/plugins/scaffolder-backend/src/service/router.test.ts index 661939eaf2..baad34457a 100644 --- a/plugins/scaffolder-backend/src/service/router.test.ts +++ b/plugins/scaffolder-backend/src/service/router.test.ts @@ -1420,3 +1420,4 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ }); }); }); + diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index b97626b5f3..72a22333b9 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -42,7 +42,6 @@ import { EventsService } from '@backstage/plugin-events-node'; import { createConditionAuthorizer, createPermissionIntegrationRouter, - PermissionRule, createConditionTransformer, ConditionTransformer, } from '@backstage/plugin-permission-node'; @@ -132,6 +131,10 @@ import { scaffolderTaskRules, } from './rules'; +import { + TaskFilters, +} from '@backstage/plugin-scaffolder-node'; + /** * RouterOptions */ @@ -158,10 +161,8 @@ export interface RouterOptions { additionalWorkspaceProviders?: Record; permissions?: PermissionsService; permissionRules?: Array; - auth?: AuthService; - httpAuth?: HttpAuthService; - identity?: IdentityApi; - discovery?: DiscoveryService; + auth: AuthService; + httpAuth: HttpAuthService; events?: EventsService; auditor?: AuditorService; autocompleteHandlers?: Record; diff --git a/plugins/scaffolder-backend/src/service/rules.test.ts b/plugins/scaffolder-backend/src/service/rules.test.ts index 8e24cacb32..f50a015a2e 100644 --- a/plugins/scaffolder-backend/src/service/rules.test.ts +++ b/plugins/scaffolder-backend/src/service/rules.test.ts @@ -22,10 +22,14 @@ import { hasProperty, hasStringProperty, hasTag, + hasCreatedBy, + hasTemplateEntityRefs, } from './rules'; import { createConditionAuthorizer } from '@backstage/plugin-permission-node'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-common/alpha'; import { AuthorizeResult } from '@backstage/plugin-permission-common'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; +import { TaskSpec } from '@backstage/plugin-scaffolder-common'; describe('hasTag', () => { describe('apply', () => { @@ -523,3 +527,158 @@ describe('hasStringProperty', () => { ); }); }); + +describe('hasCreatedBy', () => { + describe('apply', () => { + const task: SerializedTask = { + id: 'a-random-id', + spec: {} as TaskSpec, + status: 'completed', + createdAt: '', + createdBy: 'user:default/user-1', + }; + it('returns false when createdBy is an empty array', () => { + expect( + hasCreatedBy.apply(task, { + createdBy: [], + }), + ).toEqual(false); + }); + it('returns false when createdBy is not matched (single user in createdBy)', () => { + expect( + hasCreatedBy.apply(task, { + createdBy: ['not-matched'], + }), + ).toEqual(false); + }); + it('returns true when createdBy matches (single user in createdBy)', () => { + expect( + hasCreatedBy.apply(task, { + createdBy: ['user:default/user-1'], + }), + ).toEqual(true); + }); + it('returns false when createdBy is not matched (multiple users in createdBy)', () => { + expect( + hasCreatedBy.apply(task, { + createdBy: [ + 'user:default/user-2', + 'user:default/user-3', + 'user:default/user-4', + ], + }), + ).toEqual(false); + }); + it('returns true when createdBy matches (multiple users in createdBy)', () => { + expect( + hasCreatedBy.apply(task, { + createdBy: [ + 'user:default/user-1', + 'user:default/user-2', + 'user:default/user-3', + ], + }), + ).toEqual(true); + }); + }); + describe('toQuery', () => { + it('returns the correct query filter with values (single user in createdBy)', () => { + expect( + hasCreatedBy.toQuery({ + createdBy: ['user:default/user-1'], + }), + ).toEqual({ property: 'createdBy', values: ['user:default/user-1'] }); + }); + }); + it('returns the correct query filter with values (multiple users in createdBy)', () => { + expect( + hasCreatedBy.toQuery({ + createdBy: ['user:default/user-1', 'user:default/user-2'], + }), + ).toEqual({ + property: 'createdBy', + values: ['user:default/user-1', 'user:default/user-2'], + }); + }); +}); + +describe('hasTemplateEntityRefs', () => { + describe('apply', () => { + const task: SerializedTask = { + id: 'a-random-id', + spec: { + templateInfo: { entityRef: 'template:default/test-1' }, + } as TaskSpec, + status: 'completed', + createdAt: '', + }; + it('returns false when templateEntityRefs is an empty array', () => { + expect( + hasTemplateEntityRefs.apply(task, { + templateEntityRefs: [], + }), + ).toEqual(false); + }); + it('returns false when templateEntityRef is not matched (single entityRef in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.apply(task, { + templateEntityRefs: ['template:default/not-matched'], + }), + ).toEqual(false); + }); + it('returns true when templateEntityRef matches (single entityRef in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.apply(task, { + templateEntityRefs: ['template:default/test-1'], + }), + ).toEqual(true); + }); + it('returns false when templateEntityRefs is not matched (multiple entitRefs in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.apply(task, { + templateEntityRefs: [ + 'template:default/test-2', + 'template:default/test-3', + 'template:default/test-4', + ], + }), + ).toEqual(false); + }); + it('returns true when templateEntityRefs matches (multiple entityRefs in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.apply(task, { + templateEntityRefs: [ + 'template:default/test-2', + 'template:default/test-1', + 'template:default/test-3', + ], + }), + ).toEqual(true); + }); + }); + describe('toQuery', () => { + it('returns the correct query filter with values (single entityRef in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.toQuery({ + templateEntityRefs: ['template:default/test-1'], + }), + ).toEqual({ + property: 'templateEntityRefs', + values: ['template:default/test-1'], + }); + }); + }); + it('returns the correct query filter with values (multiple entityRefs in templateEntityRefs)', () => { + expect( + hasTemplateEntityRefs.toQuery({ + templateEntityRefs: [ + 'template:default/test-1', + 'template:default/test-2', + ], + }), + ).toEqual({ + property: 'templateEntityRefs', + values: ['template:default/test-1', 'template:default/test-2'], + }); + }); +}); diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index b197810757..316e8e65dc 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -15,9 +15,11 @@ */ import { makeCreatePermissionRule } from '@backstage/plugin-permission-node'; + import { RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, RESOURCE_TYPE_SCAFFOLDER_ACTION, + RESOURCE_TYPE_SCAFFOLDER_TASK, } from '@backstage/plugin-scaffolder-common/alpha'; import { @@ -25,6 +27,8 @@ import { TemplateParametersV1beta3, } from '@backstage/plugin-scaffolder-common'; +import { SerializedTask, TaskFilter } from '@backstage/plugin-scaffolder-node'; + import { z } from 'zod'; import { JsonObject, JsonPrimitive } from '@backstage/types'; import { get } from 'lodash'; @@ -129,6 +133,65 @@ function buildHasProperty>({ }); } +export const createTaskPermissionRule = makeCreatePermissionRule< + SerializedTask, + { + property: TaskFilter['property']; + values: any; + }, + typeof RESOURCE_TYPE_SCAFFOLDER_TASK +>(); + +export const hasCreatedBy = createTaskPermissionRule({ + name: 'HAS_CREATED_BY', + description: 'Allows tasks created by certain users to be accessible', + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, + paramsSchema: z.object({ + createdBy: z + .array(z.string()) + .describe( + 'List of creater entity refs; only tasks created by these users will be viewable', + ), + }), + apply: (resource, { createdBy }) => { + if (!resource.createdBy) { + return false; + } + return createdBy.includes(resource.createdBy); + }, + toQuery: ({ createdBy }) => { + return { + property: 'createdBy' as TaskFilter['property'], + values: createdBy, + }; + }, +}); + +export const hasTemplateEntityRefs = createTaskPermissionRule({ + name: 'HAS_TEMPLATE_ENTITY_REFS', + description: 'Match tasks with the given template entity refs', + resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, + paramsSchema: z.object({ + templateEntityRefs: z + .array(z.string()) + .describe( + 'List of template entity refs; only tasks related to these templates will be viewable', + ), + }), + apply: (resource, { templateEntityRefs }) => { + if (!resource.spec.templateInfo) { + return false; + } + return templateEntityRefs.includes(resource.spec.templateInfo.entityRef); + }, + toQuery: ({ templateEntityRefs }) => { + return { + property: 'templateEntityRefs' as TaskFilter['property'], + values: templateEntityRefs, + }; + }, +}); + export const scaffolderTemplateRules = { hasTag }; export const scaffolderActionRules = { hasActionId, @@ -136,3 +199,4 @@ export const scaffolderActionRules = { hasNumberProperty, hasStringProperty, }; +export const scaffolderTaskRules = { hasCreatedBy, hasTemplateEntityRefs }; diff --git a/plugins/scaffolder-common/report-alpha.api.md b/plugins/scaffolder-common/report-alpha.api.md index 97a3de63c5..0bed459cc8 100644 --- a/plugins/scaffolder-common/report-alpha.api.md +++ b/plugins/scaffolder-common/report-alpha.api.md @@ -12,6 +12,9 @@ export const actionExecutePermission: ResourcePermission<'scaffolder-action'>; // @alpha export const RESOURCE_TYPE_SCAFFOLDER_ACTION = 'scaffolder-action'; +// @alpha +export const RESOURCE_TYPE_SCAFFOLDER_TASK = 'scaffolder-task'; + // @alpha export const RESOURCE_TYPE_SCAFFOLDER_TEMPLATE = 'scaffolder-template'; @@ -23,22 +26,26 @@ export const scaffolderPermissions: ( | BasicPermission | ResourcePermission<'scaffolder-action'> | ResourcePermission<'scaffolder-template'> + | ResourcePermission<'scaffolder-task'> )[]; // @alpha -export const scaffolderTaskPermissions: BasicPermission[]; +export const scaffolderTaskPermissions: ( + | BasicPermission + | ResourcePermission<'scaffolder-task'> +)[]; // @alpha export const scaffolderTemplatePermissions: ResourcePermission<'scaffolder-template'>[]; // @alpha -export const taskCancelPermission: BasicPermission; +export const taskCancelPermission: ResourcePermission<'scaffolder-task'>; // @alpha export const taskCreatePermission: BasicPermission; // @alpha -export const taskReadPermission: BasicPermission; +export const taskReadPermission: ResourcePermission<'scaffolder-task'>; // @alpha export const templateManagementPermission: BasicPermission; diff --git a/plugins/scaffolder-node/package.json b/plugins/scaffolder-node/package.json index cd03ead977..9efecdfbdc 100644 --- a/plugins/scaffolder-node/package.json +++ b/plugins/scaffolder-node/package.json @@ -58,6 +58,7 @@ "@backstage/catalog-model": "workspace:^", "@backstage/errors": "workspace:^", "@backstage/integration": "workspace:^", + "@backstage/plugin-permission-common": "workspace:^", "@backstage/plugin-scaffolder-common": "workspace:^", "@backstage/types": "workspace:^", "@isomorphic-git/pgp-plugin": "^0.0.7", diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index a23a9137b1..4674ee8563 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -9,6 +9,7 @@ import { JsonObject } from '@backstage/types'; import { JsonValue } from '@backstage/types'; import { LoggerService } from '@backstage/backend-plugin-api'; import { Observable } from '@backstage/types'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { Schema } from 'jsonschema'; import { ScmIntegrationRegistry } from '@backstage/integration'; import { ScmIntegrations } from '@backstage/integration'; @@ -373,6 +374,7 @@ export interface TaskBroker { order: 'asc' | 'desc'; field: string; }[]; + permissionFilters?: PermissionCriteria; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number; @@ -464,6 +466,25 @@ export interface TaskContext { // @public export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered'; +// @public +export type TaskFilter = { + property: 'createdBy' | 'templateEntityRefs'; + values: Array | undefined; +}; + +// @public +export type TaskFilters = + | { + anyOf: TaskFilter[]; + } + | { + allOf: TaskFilter[]; + } + | { + not: TaskFilter; + } + | TaskFilter; + // @public export type TaskSecrets = Record & { backstageToken?: string; diff --git a/plugins/scaffolder-node/src/tasks/index.ts b/plugins/scaffolder-node/src/tasks/index.ts index 930de95237..7e93e84cc0 100644 --- a/plugins/scaffolder-node/src/tasks/index.ts +++ b/plugins/scaffolder-node/src/tasks/index.ts @@ -18,6 +18,8 @@ export type { TaskSecrets, SerializedTask, SerializedTaskEvent, + TaskFilter, + TaskFilters, TaskBroker, TaskBrokerDispatchOptions, TaskBrokerDispatchResult, diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index d8cfd24530..8101080dae 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -15,6 +15,7 @@ */ import { BackstageCredentials } from '@backstage/backend-plugin-api'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { TaskSpec } from '@backstage/plugin-scaffolder-common'; import { JsonObject, JsonValue, Observable } from '@backstage/types'; @@ -104,6 +105,25 @@ export type TaskBrokerDispatchOptions = { createdBy?: string; }; +/** + * TaskFilter + * @public + */ +export type TaskFilter = { + property: 'createdBy' | 'templateEntityRefs'; + values: Array | undefined; +}; + +/** + * TaskFilters + * @public + */ +export type TaskFilters = + | { anyOf: TaskFilter[] } + | { allOf: TaskFilter[] } + | { not: TaskFilter } + | TaskFilter; + /** * Task * @@ -194,6 +214,7 @@ export interface TaskBroker { offset?: number; }; order?: { order: 'asc' | 'desc'; field: string }[]; + permissionFilters?: PermissionCriteria; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number }>; /** diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx index c01aa79441..b3c82dca19 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx @@ -139,7 +139,6 @@ function OngoingTaskContent(props: { const [logsVisible, setLogVisibleState] = useState(false); const [buttonBarVisible, setButtonBarVisibleState] = useState(true); - // Used dummy string value for `resourceRef` since `allowed` field will always return `false` if `resourceRef` is `undefined` const { allowed: canCancelTask } = usePermission({ permission: taskCancelPermission, resourceRef: taskId, diff --git a/yarn.lock b/yarn.lock index c3e37b0ac0..ccceb14d2c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7615,6 +7615,7 @@ __metadata: "@backstage/config": "workspace:^" "@backstage/errors": "workspace:^" "@backstage/integration": "workspace:^" + "@backstage/plugin-permission-common": "workspace:^" "@backstage/plugin-scaffolder-common": "workspace:^" "@backstage/types": "workspace:^" "@isomorphic-git/pgp-plugin": "npm:^0.0.7" From bc3161292c1da4041a6bef61f299684497c0ad45 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Thu, 13 Mar 2025 10:33:50 -0400 Subject: [PATCH 03/16] modify docs related to scaffolder task permissions Signed-off-by: Kashish Mittal --- ...authorizing-scaffolder-template-details.md | 121 ++++++++++++++++-- 1 file changed, 108 insertions(+), 13 deletions(-) diff --git a/docs/features/software-templates/authorizing-scaffolder-template-details.md b/docs/features/software-templates/authorizing-scaffolder-template-details.md index d4798b2540..c5b0f0b1f4 100644 --- a/docs/features/software-templates/authorizing-scaffolder-template-details.md +++ b/docs/features/software-templates/authorizing-scaffolder-template-details.md @@ -176,7 +176,9 @@ class ExamplePermissionPolicy implements PermissionPolicy { ### Authorizing scaffolder tasks -The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. +The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. You can also restrict access to tasks to only their owners or admin users and allow template owners to view all runs of their templates. + +The following is a simple example of how to do this: ```ts title="packages/src/backend/plugins/permissions.ts" /* highlight-add-start */ @@ -185,34 +187,117 @@ import { taskCreatePermission, taskReadPermission, } from '@backstage/plugin-scaffolder-common/alpha'; +import { + AuthorizeResult, + isPermission, + PolicyDecision, +} from '@backstage/plugin-permission-common'; +import { + PermissionPolicy, + PolicyQuery, + PolicyQueryUser, +} from '@backstage/plugin-permission-node'; +import { + createScaffolderTaskConditionalDecision, + scaffolderTaskConditions, +} from '@backstage/plugin-scaffolder-backend/alpha'; +import { CatalogClient } from '@backstage/catalog-client'; /* highlight-add-end */ class ExamplePermissionPolicy implements PermissionPolicy { + private readonly catalogClient: CatalogClient; + + constructor(catalogClient: CatalogClient) { + this.catalogClient = catalogClient; + } + + // Fetches all templates owned by the user from the catalog API. + async getUserOwnedTemplates(userRef: string): Promise { + if (!userRef) return []; + + const response = await this.catalogClient.getEntities({ + filter: { + kind: ['Template'], + 'relations.ownedBy': [userRef], + }, + }); + + return response.items.map( + item => + `${item.kind.toLocaleLowerCase()}:${item.metadata.namespace}/${ + item.metadata.name + }`, + ); + } + async handle( request: PolicyQuery, user?: PolicyQueryUser, ): Promise { /* highlight-add-start */ - if (isPermission(request.permission, taskCreatePermission)) { - if (user?.info.userEntityRef === 'user:default/spiderman') { + if (isPermission(request.permission, taskReadPermission)) { + // Allow admin1 to read any task + if (user?.info.userEntityRef === 'user:default/admin1') { return { result: AuthorizeResult.ALLOW, }; } + + // Retrieve templates that the user owns + const userOwnedTemplates = await this.getUserOwnedTemplates( + user?.info.userEntityRef || '', + ); + + // Allow users to read tasks they created or task runs of templates they own + return createScaffolderTaskConditionalDecision(request.permission, { + anyOf: [ + scaffolderTaskConditions.hasCreatedBy({ + createdBy: user?.info.userEntityRef + ? [user?.info.userEntityRef] + : [], + }), + scaffolderTaskConditions.hasTemplateEntityRefs({ + templateEntityRefs: userOwnedTemplates, + }), + ], + }); + } + + if (isPermission(request.permission, taskCreatePermission)) { + const userArray = ['user:default/spiderman', 'user:default/admin1']; + const allowed = userArray.some( + allowedUser => user?.info.userEntityRef === allowedUser, + ); + if (allowed) { + return { + result: AuthorizeResult.ALLOW, + }; + } + return { + result: AuthorizeResult.DENY, + }; } if (isPermission(request.permission, taskCancelPermission)) { + // Allow spiderman to cancel only his tasks if (user?.info.userEntityRef === 'user:default/spiderman') { + return createScaffolderTaskConditionalDecision( + request.permission, + scaffolderTaskConditions.hasCreatedBy({ + createdBy: user?.info.userEntityRef + ? [user?.info.userEntityRef] + : [], + }), + ); + } + // Allow admin1 to cancel any task + if (user?.info.userEntityRef === 'user:default/admin1') { return { result: AuthorizeResult.ALLOW, }; } - } - if (isPermission(request.permission, taskReadPermission)) { - if (user?.info.userEntityRef === 'user:default/spiderman') { - return { - result: AuthorizeResult.ALLOW, - }; - } + return { + result: AuthorizeResult.DENY, + }; } /* highlight-add-end */ @@ -225,11 +310,21 @@ class ExamplePermissionPolicy implements PermissionPolicy { In the provided example permission policy, we only grant the `spiderman` user permissions to perform/access the following actions/resources: -- Read all scaffolder tasks and their associated events/logs. -- Cancel any ongoing scaffolder tasks. +- Read scaffolder tasks and their associated events/logs for tasks created by `spiderman`. +- Read scaffolder task runs of templates owned by `spiderman` +- Cancel ongoing scaffolder tasks created by `spiderman`. - Trigger software templates, which effectively creates new scaffolder tasks. -Any other user would be denied access to these actions/resources. +On the other hand, the `admin1` user is granted: + +- Read access to all scaffolder tasks and their associated events/logs. +- The ability to cancel any ongoing scaffolder task. +- The ability to trigger software templates. + +All other users are granted permissions to perform/access the following actions/resources: + +- Read scaffolder tasks and their associated events/logs for tasks created by the user. +- Read scaffolder task runs of the templates owned by the user Although the rules exported by the scaffolder are simple, combining them can help you achieve more complex use cases. From 9bdd6dedf332418017c415c6dc73d7412dc05f53 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Thu, 13 Mar 2025 11:55:05 -0400 Subject: [PATCH 04/16] update tests for Ongoing Tasks Page Signed-off-by: Kashish Mittal --- .changeset/weak-bags-behave.md | 2 - .../scaffolder-backend/src/service/router.ts | 11 +++--- .../src/util/checkPermissions.ts | 30 +++++++++----- .../OngoingTask/OngoingTask.test.tsx | 39 +++++++++++++++++-- 4 files changed, 62 insertions(+), 20 deletions(-) diff --git a/.changeset/weak-bags-behave.md b/.changeset/weak-bags-behave.md index 2c7bc1d37c..0e103cb147 100644 --- a/.changeset/weak-bags-behave.md +++ b/.changeset/weak-bags-behave.md @@ -7,5 +7,3 @@ --- BREAKING : Added two new scaffolder rules for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on creators (`hasCreatedBy`) and granting template owners visibility into all runs of their templates (`hasTemplateEntityRefs`). - -BREAKING: Removed requirement to have both `scaffolder.task.read` and `scaffolder.task.cancel` permissions to cancel tasks. diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 72a22333b9..291fcbdae1 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -646,7 +646,7 @@ export async function createRouter( await checkTaskPermission({ credentials, - permission: taskReadPermission, + permissions: [taskReadPermission], permissionService: permissions, task: task, isTaskAuthorized, @@ -682,9 +682,10 @@ export async function createRouter( try { const credentials = await httpAuth.credentials(req); const task = await taskBroker.get(taskId); + // Requires both read and cancel permissions await checkTaskPermission({ credentials, - permission: taskCancelPermission, + permissions: [taskCancelPermission, taskReadPermission], permissionService: permissions, task: task, isTaskAuthorized, @@ -726,7 +727,7 @@ export async function createRouter( await checkTaskPermission({ credentials, - permission: taskReadPermission, + permissions: [taskReadPermission], permissionService: permissions, task: task, isTaskAuthorized, @@ -774,7 +775,7 @@ export async function createRouter( await checkTaskPermission({ credentials, - permission: taskReadPermission, + permissions: [taskReadPermission], permissionService: permissions, task: task, isTaskAuthorized, @@ -850,7 +851,7 @@ export async function createRouter( await checkTaskPermission({ credentials, - permission: taskReadPermission, + permissions: [taskReadPermission], permissionService: permissions, task: task, isTaskAuthorized, diff --git a/plugins/scaffolder-backend/src/util/checkPermissions.ts b/plugins/scaffolder-backend/src/util/checkPermissions.ts index 950f506e55..08a4ae1b13 100644 --- a/plugins/scaffolder-backend/src/util/checkPermissions.ts +++ b/plugins/scaffolder-backend/src/util/checkPermissions.ts @@ -37,7 +37,7 @@ export type checkPermissionOptions = { export type checkTaskPermissionOptions = { credentials: BackstageCredentials; - permission: ResourcePermission; + permissions: ResourcePermission[]; permissionService?: PermissionsService; task: SerializedTask; isTaskAuthorized: ( @@ -82,18 +82,28 @@ export async function checkPermission(options: checkPermissionOptions) { * @public */ export async function checkTaskPermission(options: checkTaskPermissionOptions) { - const { permission, permissionService, credentials, task, isTaskAuthorized } = - options; + const { + permissions, + permissionService, + credentials, + task, + isTaskAuthorized, + } = options; if (permissionService) { - const [taskDecision] = await permissionService.authorizeConditional( - [{ permission: permission }], + const permissionRequest = permissions.map(permission => ({ + permission, + })); + const authorizationResponses = await permissionService.authorizeConditional( + permissionRequest, { credentials }, ); - if ( - taskDecision.result === AuthorizeResult.DENY || - !isTaskAuthorized(taskDecision, task) - ) { - throw new NotAllowedError(); + for (const response of authorizationResponses) { + if ( + response.result === AuthorizeResult.DENY || + !isTaskAuthorized(response, task) + ) { + throw new NotAllowedError(); + } } } } diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx index a576b1a9bc..2ef7ddc4d5 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx @@ -161,20 +161,53 @@ describe('OngoingTask', () => { await expect(rendered.findByText('Hide Logs')).resolves.toBeInTheDocument(); }); - it('should have cancel and start over buttons be disabled without the proper permissions', async () => { + it('should render not found error page when user does not have permission to read the task', async () => { const permissionApi = mockApis.permission({ authorize: AuthorizeResult.DENY, }); + + await expect(render(permissionApi)).rejects.toThrow( + 'Reached NotFound Page', + ); + }); + + it('should have cancel button be disabled when user has read permission but lacks cancel permission', async () => { + const permissionApi = mockApis.permission({ + authorize: request => { + if (request.permission.name === 'scaffolder.task.cancel') { + return AuthorizeResult.DENY; + } + return AuthorizeResult.ALLOW; + }, + }); const rendered = await render(permissionApi); const { getByTestId } = rendered; expect(getByTestId('cancel-button')).toHaveClass('Mui-disabled'); - expect(getByTestId('start-over-button')).toHaveClass('Mui-disabled'); await act(async () => { fireEvent.click(getByTestId('menu-button')); }); expect(getByTestId('cancel-task')).toHaveClass('Mui-disabled'); - expect(getByTestId('start-over-task')).toHaveClass('Mui-disabled'); + }); + + it('should have start over button be disabled when user has read permission but lacks create permission', async () => { + const permissionApi = mockApis.permission({ + authorize: request => { + if (request.permission.name === 'scaffolder.task.create') { + return AuthorizeResult.DENY; + } + return AuthorizeResult.ALLOW; + }, + }); + const rendered = await render(permissionApi); + + const { getByTestId } = rendered; + expect(getByTestId('start-over-button')).toHaveClass('Mui-disabled'); + + await act(async () => { + fireEvent.click(getByTestId('menu-button')); + }); + expect(getByTestId('start-over-button')).toHaveClass('Mui-disabled'); }); }); From c359d72cc2527d844ce276c180eef23be98dfe01 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Mon, 17 Mar 2025 11:03:16 -0400 Subject: [PATCH 05/16] update docs based on review comments Signed-off-by: Kashish Mittal --- ...authorizing-scaffolder-template-details.md | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/docs/features/software-templates/authorizing-scaffolder-template-details.md b/docs/features/software-templates/authorizing-scaffolder-template-details.md index c5b0f0b1f4..fd246463d0 100644 --- a/docs/features/software-templates/authorizing-scaffolder-template-details.md +++ b/docs/features/software-templates/authorizing-scaffolder-template-details.md @@ -73,7 +73,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { isPermission(request.permission, templateParameterReadPermission) || isPermission(request.permission, templateStepReadPermission) ) { - if (user?.info.userEntityRef === 'user:default/spiderman') + if (user?.info.userEntityRef === 'user:default/bob') return createScaffolderTemplateConditionalDecision(request.permission, { not: scaffolderTemplateConditions.hasTag({ tag: 'secret' }), }); @@ -87,7 +87,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { } ``` -In this example, the user `spiderman` is not authorized to read parameters or steps marked with the `secret` tag. +In this example, the user `bob` is not authorized to read parameters or steps marked with the `secret` tag. By combining this feature with restricting the ingestion of templates in the Catalog as recommended in our threat model, you can create a solid system to restrict certain actions. @@ -113,7 +113,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { ): Promise { /* highlight-add-start */ if (isPermission(request.permission, actionExecutePermission)) { - if (user?.info.userEntityRef === 'user:default/spiderman') { + if (user?.info.userEntityRef === 'user:default/alice') { return createScaffolderActionConditionalDecision(request.permission, { not: scaffolderActionConditions.hasActionId({ actionId: 'debug:log', @@ -130,10 +130,10 @@ class ExamplePermissionPolicy implements PermissionPolicy { } ``` -With this permission policy, the user `spiderman` won't be able to execute the `debug:log` action. +With this permission policy, the user `alice` won't be able to execute the `debug:log` action. You can also restrict the input provided to the action by combining multiple rules. -In the example below, `spiderman` won't be able to execute `debug:log` when passing `{ "message": "not-this!" }` as action input: +In the example below, `alice` won't be able to execute `debug:log` when passing `{ "message": "not-this!" }` as action input: ```ts title="packages/backend/src/plugins/permission.ts" /* highlight-add-start */ @@ -151,7 +151,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { ): Promise { /* highlight-add-start */ if (isPermission(request.permission, actionExecutePermission)) { - if (user?.info.userEntityRef === 'user:default/spiderman') { + if (user?.info.userEntityRef === 'user:default/alice') { return createScaffolderActionConditionalDecision(request.permission, { not: { allOf: [ @@ -176,7 +176,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { ### Authorizing scaffolder tasks -The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. You can also restrict access to tasks to only their owners or admin users and allow template owners to view all runs of their templates. +The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. You can configure policies to restrict access to tasks to only their creators, while also granting specific users the permission to view all tasks. Additionally, template owners can be granted permissions to view all runs of their templates. The following is a simple example of how to do this: @@ -236,8 +236,8 @@ class ExamplePermissionPolicy implements PermissionPolicy { ): Promise { /* highlight-add-start */ if (isPermission(request.permission, taskReadPermission)) { - // Allow admin1 to read any task - if (user?.info.userEntityRef === 'user:default/admin1') { + // Allow alice to read any task + if (user?.info.userEntityRef === 'user:default/alice') { return { result: AuthorizeResult.ALLOW, }; @@ -264,7 +264,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { } if (isPermission(request.permission, taskCreatePermission)) { - const userArray = ['user:default/spiderman', 'user:default/admin1']; + const userArray = ['user:default/bob', 'user:default/alice']; const allowed = userArray.some( allowedUser => user?.info.userEntityRef === allowedUser, ); @@ -278,8 +278,8 @@ class ExamplePermissionPolicy implements PermissionPolicy { }; } if (isPermission(request.permission, taskCancelPermission)) { - // Allow spiderman to cancel only his tasks - if (user?.info.userEntityRef === 'user:default/spiderman') { + // Allow bob to cancel only his tasks + if (user?.info.userEntityRef === 'user:default/bob') { return createScaffolderTaskConditionalDecision( request.permission, scaffolderTaskConditions.hasCreatedBy({ @@ -289,8 +289,8 @@ class ExamplePermissionPolicy implements PermissionPolicy { }), ); } - // Allow admin1 to cancel any task - if (user?.info.userEntityRef === 'user:default/admin1') { + // Allow alice to cancel any task + if (user?.info.userEntityRef === 'user:default/alice') { return { result: AuthorizeResult.ALLOW, }; @@ -308,14 +308,14 @@ class ExamplePermissionPolicy implements PermissionPolicy { } ``` -In the provided example permission policy, we only grant the `spiderman` user permissions to perform/access the following actions/resources: +In the provided example permission policy, we only grant the user `bob` permissions to perform/access the following actions/resources: -- Read scaffolder tasks and their associated events/logs for tasks created by `spiderman`. -- Read scaffolder task runs of templates owned by `spiderman` -- Cancel ongoing scaffolder tasks created by `spiderman`. +- Read scaffolder tasks and their associated events/logs for tasks created by `bob`. +- Read scaffolder task runs of templates owned by `bob` +- Cancel ongoing scaffolder tasks created by `bob`. - Trigger software templates, which effectively creates new scaffolder tasks. -On the other hand, the `admin1` user is granted: +On the other hand, the user `alice` is granted: - Read access to all scaffolder tasks and their associated events/logs. - The ability to cancel any ongoing scaffolder task. From 5f01d02238abd5b6371eb7ae2d82072d4e989837 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Wed, 2 Apr 2025 11:48:48 -0400 Subject: [PATCH 06/16] fix failing test Signed-off-by: Kashish Mittal --- plugins/scaffolder-backend/src/service/router.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/scaffolder-backend/src/service/router.test.ts b/plugins/scaffolder-backend/src/service/router.test.ts index baad34457a..661939eaf2 100644 --- a/plugins/scaffolder-backend/src/service/router.test.ts +++ b/plugins/scaffolder-backend/src/service/router.test.ts @@ -1420,4 +1420,3 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ }); }); }); - From fe85d897d59fb84466debf9d4744cf5d080c742b Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Tue, 15 Apr 2025 17:08:43 -0400 Subject: [PATCH 07/16] Remove hasTemplateEntityRefs and only keep hasCreatedBy Signed-off-by: Kashish Mittal --- .changeset/weak-bags-behave.md | 5 +- ...authorizing-scaffolder-template-details.md | 53 ++---------- .../scaffolder-backend/report-alpha.api.md | 11 --- plugins/scaffolder-backend/report.api.md | 1 + .../tasks/DatabaseTaskStore.test.ts | 55 +++++-------- .../src/scaffolder/tasks/DatabaseTaskStore.ts | 24 ++---- .../src/scaffolder/tasks/types.ts | 3 + .../src/service/rules.test.ts | 82 ------------------- .../scaffolder-backend/src/service/rules.ts | 27 +----- plugins/scaffolder-node/report.api.md | 2 +- plugins/scaffolder-node/src/tasks/types.ts | 2 +- 11 files changed, 46 insertions(+), 219 deletions(-) diff --git a/.changeset/weak-bags-behave.md b/.changeset/weak-bags-behave.md index 0e103cb147..c8024f8ff4 100644 --- a/.changeset/weak-bags-behave.md +++ b/.changeset/weak-bags-behave.md @@ -6,4 +6,7 @@ '@backstage/plugin-scaffolder': minor --- -BREAKING : Added two new scaffolder rules for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on creators (`hasCreatedBy`) and granting template owners visibility into all runs of their templates (`hasTemplateEntityRefs`). +BREAKING : + +- Converted `scaffolder.task.read` and `scaffolder.task.cancel` into Resource Permissions. +- Added a new scaffolder rule `hasCreatedBy` for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on task creators. diff --git a/docs/features/software-templates/authorizing-scaffolder-template-details.md b/docs/features/software-templates/authorizing-scaffolder-template-details.md index fd246463d0..200035e434 100644 --- a/docs/features/software-templates/authorizing-scaffolder-template-details.md +++ b/docs/features/software-templates/authorizing-scaffolder-template-details.md @@ -176,7 +176,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { ### Authorizing scaffolder tasks -The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. You can configure policies to restrict access to tasks to only their creators, while also granting specific users the permission to view all tasks. Additionally, template owners can be granted permissions to view all runs of their templates. +The scaffolder plugin also exposes permissions that can restrict access to tasks, task logs, task creation, and task cancellation. This can be useful if you want to control who has access to these areas of the scaffolder. You can configure policies to restrict access to tasks to only their creators, while also granting specific users the permission to view all tasks. The following is a simple example of how to do this: @@ -201,35 +201,9 @@ import { createScaffolderTaskConditionalDecision, scaffolderTaskConditions, } from '@backstage/plugin-scaffolder-backend/alpha'; -import { CatalogClient } from '@backstage/catalog-client'; /* highlight-add-end */ class ExamplePermissionPolicy implements PermissionPolicy { - private readonly catalogClient: CatalogClient; - - constructor(catalogClient: CatalogClient) { - this.catalogClient = catalogClient; - } - - // Fetches all templates owned by the user from the catalog API. - async getUserOwnedTemplates(userRef: string): Promise { - if (!userRef) return []; - - const response = await this.catalogClient.getEntities({ - filter: { - kind: ['Template'], - 'relations.ownedBy': [userRef], - }, - }); - - return response.items.map( - item => - `${item.kind.toLocaleLowerCase()}:${item.metadata.namespace}/${ - item.metadata.name - }`, - ); - } - async handle( request: PolicyQuery, user?: PolicyQueryUser, @@ -243,24 +217,13 @@ class ExamplePermissionPolicy implements PermissionPolicy { }; } - // Retrieve templates that the user owns - const userOwnedTemplates = await this.getUserOwnedTemplates( - user?.info.userEntityRef || '', + // Allow users to read tasks they created + return createScaffolderTaskConditionalDecision( + request.permission, + scaffolderTaskConditions.hasCreatedBy({ + createdBy: user?.info.userEntityRef ? [user?.info.userEntityRef] : [], + }), ); - - // Allow users to read tasks they created or task runs of templates they own - return createScaffolderTaskConditionalDecision(request.permission, { - anyOf: [ - scaffolderTaskConditions.hasCreatedBy({ - createdBy: user?.info.userEntityRef - ? [user?.info.userEntityRef] - : [], - }), - scaffolderTaskConditions.hasTemplateEntityRefs({ - templateEntityRefs: userOwnedTemplates, - }), - ], - }); } if (isPermission(request.permission, taskCreatePermission)) { @@ -311,7 +274,6 @@ class ExamplePermissionPolicy implements PermissionPolicy { In the provided example permission policy, we only grant the user `bob` permissions to perform/access the following actions/resources: - Read scaffolder tasks and their associated events/logs for tasks created by `bob`. -- Read scaffolder task runs of templates owned by `bob` - Cancel ongoing scaffolder tasks created by `bob`. - Trigger software templates, which effectively creates new scaffolder tasks. @@ -324,7 +286,6 @@ On the other hand, the user `alice` is granted: All other users are granted permissions to perform/access the following actions/resources: - Read scaffolder tasks and their associated events/logs for tasks created by the user. -- Read scaffolder task runs of the templates owned by the user Although the rules exported by the scaffolder are simple, combining them can help you achieve more complex use cases. diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index cd9ca2eb00..bf2dd186a9 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -97,17 +97,6 @@ export const scaffolderTaskConditions: Conditions<{ createdBy: string[]; } >; - hasTemplateEntityRefs: PermissionRule< - SerializedTask, - { - property: TaskFilter['property']; - values: any; - }, - 'scaffolder-task', - { - templateEntityRefs: string[]; - } - >; }>; // @alpha diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index 9da363e872..e22a7d1370 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -501,6 +501,7 @@ export interface TaskStore { limit?: number; offset?: number; }; + permissionFilters?: PermissionCriteria; order?: { order: 'asc' | 'desc'; field: string; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts index 2d9b4db124..e86610e159 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts @@ -237,66 +237,51 @@ describe('DatabaseTaskStore', () => { const { store } = await createStore(); await store.createTask({ - spec: { - templateInfo: { entityRef: 'template:default/three' }, - } as TaskSpec, + spec: {} as TaskSpec, createdBy: 'user:default/one', }); await store.createTask({ - spec: { - templateInfo: { entityRef: 'template:default/four' }, - } as TaskSpec, + spec: {} as TaskSpec, createdBy: 'user:default/two', }); await store.createTask({ - spec: { - templateInfo: { entityRef: 'template:default/one' }, - } as TaskSpec, + spec: {} as TaskSpec, createdBy: 'user:default/three', }); await store.createTask({ - spec: { - templateInfo: { entityRef: 'template:default/two' }, - } as TaskSpec, - createdBy: 'user:default/three', + spec: {} as TaskSpec, + createdBy: 'user:default/one', }); await store.createTask({ - spec: { - templateInfo: { entityRef: 'template:default/three' }, - } as TaskSpec, - createdBy: 'user:default/three', + spec: {} as TaskSpec, + createdBy: 'user:default/four', }); const permissionFilters: PermissionCriteria = { - anyOf: [ - { - property: 'createdBy', - values: ['user:default/one', 'user:default/two'], - }, - { - property: 'templateEntityRefs', - values: ['template:default/one', 'template:default/two'], - }, - ], + not: { + property: 'createdBy', + values: ['user:default/three', 'user:default/four'], + }, }; const { tasks, totalTasks } = await store.list({ permissionFilters: permissionFilters, }); - expect(totalTasks).toBe(4); + console.log(JSON.stringify); - expect(tasks).not.toEqual( - expect.arrayContaining([ - expect.objectContaining({ - createdBy: 'user:default/three', - spec: { templateInfo: { entityRef: 'template:default/three' } }, - }), - ]), + expect(totalTasks).toBe(3); + + const createdByList = tasks.map(task => task.createdBy); + expect(createdByList).toEqual( + expect.arrayContaining(['user:default/one', 'user:default/two']), + ); + expect(createdByList).not.toEqual( + expect.arrayContaining(['user:default/three', 'user:default/four']), ); }); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index b9b5fd0d7c..795a4b7ff6 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -216,6 +216,7 @@ export class DatabaseTaskStore implements TaskStore { ): Knex.QueryBuilder { // handle not criteria if (isNotCriteria(filter)) { + console.log(`reached here: ${JSON.stringify(filter)}`); return this.parseFilter(filter.not, query, db, !negate); } @@ -223,24 +224,13 @@ export class DatabaseTaskStore implements TaskStore { const values: string[] = compact(filter.values) ?? []; if (filter.property === 'createdBy') { - query.whereIn('created_by', [...new Set(values)]); - } - - if (filter.property === 'templateEntityRefs' && values.length > 0) { - const dbClient = this.db.client.config.client; - const placeholders = values.map(() => '?').join(', '); - if (dbClient === 'pg') { - query.whereRaw( - `spec::jsonb->'templateInfo'->>'entityRef' IN (${placeholders})`, - values, - ); - } else if (dbClient === 'better-sqlite3') { - query.whereRaw( - `json_extract(spec, '$.templateInfo.entityRef') IN (${placeholders})`, - values, - ); + if (negate) { + query.whereNotIn('created_by', [...new Set(values)]); + } else { + query.whereIn('created_by', [...new Set(values)]); } } + return query; } @@ -300,6 +290,8 @@ export class DatabaseTaskStore implements TaskStore { this.parseFilter(combinedPermissionFilters, queryBuilder, this.db); } + console.log(`Parse FIlters: ${queryBuilder}`); + if (status || filters?.status) { const arr: TaskStatus[] = flattenParams( status, diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts index a3d9e3b741..62ffb171ff 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts @@ -23,7 +23,9 @@ import { SerializedTaskEvent, SerializedTask, TaskStatus, + TaskFilters, } from '@backstage/plugin-scaffolder-node'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; /** * TaskStoreEmitOptions @@ -131,6 +133,7 @@ export interface TaskStore { limit?: number; offset?: number; }; + permissionFilters?: PermissionCriteria; order?: { order: 'asc' | 'desc'; field: string }[]; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number }>; diff --git a/plugins/scaffolder-backend/src/service/rules.test.ts b/plugins/scaffolder-backend/src/service/rules.test.ts index f50a015a2e..68b2930e32 100644 --- a/plugins/scaffolder-backend/src/service/rules.test.ts +++ b/plugins/scaffolder-backend/src/service/rules.test.ts @@ -23,7 +23,6 @@ import { hasStringProperty, hasTag, hasCreatedBy, - hasTemplateEntityRefs, } from './rules'; import { createConditionAuthorizer } from '@backstage/plugin-permission-node'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-common/alpha'; @@ -601,84 +600,3 @@ describe('hasCreatedBy', () => { }); }); }); - -describe('hasTemplateEntityRefs', () => { - describe('apply', () => { - const task: SerializedTask = { - id: 'a-random-id', - spec: { - templateInfo: { entityRef: 'template:default/test-1' }, - } as TaskSpec, - status: 'completed', - createdAt: '', - }; - it('returns false when templateEntityRefs is an empty array', () => { - expect( - hasTemplateEntityRefs.apply(task, { - templateEntityRefs: [], - }), - ).toEqual(false); - }); - it('returns false when templateEntityRef is not matched (single entityRef in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.apply(task, { - templateEntityRefs: ['template:default/not-matched'], - }), - ).toEqual(false); - }); - it('returns true when templateEntityRef matches (single entityRef in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.apply(task, { - templateEntityRefs: ['template:default/test-1'], - }), - ).toEqual(true); - }); - it('returns false when templateEntityRefs is not matched (multiple entitRefs in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.apply(task, { - templateEntityRefs: [ - 'template:default/test-2', - 'template:default/test-3', - 'template:default/test-4', - ], - }), - ).toEqual(false); - }); - it('returns true when templateEntityRefs matches (multiple entityRefs in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.apply(task, { - templateEntityRefs: [ - 'template:default/test-2', - 'template:default/test-1', - 'template:default/test-3', - ], - }), - ).toEqual(true); - }); - }); - describe('toQuery', () => { - it('returns the correct query filter with values (single entityRef in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.toQuery({ - templateEntityRefs: ['template:default/test-1'], - }), - ).toEqual({ - property: 'templateEntityRefs', - values: ['template:default/test-1'], - }); - }); - }); - it('returns the correct query filter with values (multiple entityRefs in templateEntityRefs)', () => { - expect( - hasTemplateEntityRefs.toQuery({ - templateEntityRefs: [ - 'template:default/test-1', - 'template:default/test-2', - ], - }), - ).toEqual({ - property: 'templateEntityRefs', - values: ['template:default/test-1', 'template:default/test-2'], - }); - }); -}); diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index 316e8e65dc..d4f7afae89 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -167,31 +167,6 @@ export const hasCreatedBy = createTaskPermissionRule({ }, }); -export const hasTemplateEntityRefs = createTaskPermissionRule({ - name: 'HAS_TEMPLATE_ENTITY_REFS', - description: 'Match tasks with the given template entity refs', - resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, - paramsSchema: z.object({ - templateEntityRefs: z - .array(z.string()) - .describe( - 'List of template entity refs; only tasks related to these templates will be viewable', - ), - }), - apply: (resource, { templateEntityRefs }) => { - if (!resource.spec.templateInfo) { - return false; - } - return templateEntityRefs.includes(resource.spec.templateInfo.entityRef); - }, - toQuery: ({ templateEntityRefs }) => { - return { - property: 'templateEntityRefs' as TaskFilter['property'], - values: templateEntityRefs, - }; - }, -}); - export const scaffolderTemplateRules = { hasTag }; export const scaffolderActionRules = { hasActionId, @@ -199,4 +174,4 @@ export const scaffolderActionRules = { hasNumberProperty, hasStringProperty, }; -export const scaffolderTaskRules = { hasCreatedBy, hasTemplateEntityRefs }; +export const scaffolderTaskRules = { hasCreatedBy }; diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index 4674ee8563..09829d2723 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -468,7 +468,7 @@ export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered'; // @public export type TaskFilter = { - property: 'createdBy' | 'templateEntityRefs'; + property: 'createdBy'; values: Array | undefined; }; diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index 8101080dae..53169e3432 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -110,7 +110,7 @@ export type TaskBrokerDispatchOptions = { * @public */ export type TaskFilter = { - property: 'createdBy' | 'templateEntityRefs'; + property: 'createdBy'; values: Array | undefined; }; From 63a8d8e1710552caeb9e408baa53b48ad892ba81 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Fri, 25 Apr 2025 11:52:24 -0400 Subject: [PATCH 08/16] add a getTasks method to get multiple tasks through 1 DB query Signed-off-by: Kashish Mittal --- plugins/scaffolder-backend/report.api.md | 4 ++ .../src/scaffolder/tasks/DatabaseTaskStore.ts | 47 ++++++++++++++----- .../src/scaffolder/tasks/StorageTaskBroker.ts | 10 ++++ .../src/scaffolder/tasks/types.ts | 2 + .../scaffolder-backend/src/service/router.ts | 8 +--- plugins/scaffolder-node/report.api.md | 2 + plugins/scaffolder-node/src/tasks/types.ts | 2 + 7 files changed, 55 insertions(+), 20 deletions(-) diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index e22a7d1370..cc7d83c7e1 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -329,6 +329,8 @@ export class DatabaseTaskStore implements TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) + getTasks(taskIds: string[]): Promise; + // (undocumented) getTaskState({ taskId }: { taskId: string }): Promise< | { state: JsonObject; @@ -483,6 +485,8 @@ export interface TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) + getTasks(taskIds: string[]): Promise; + // (undocumented) getTaskState?({ taskId }: { taskId: string }): Promise< | { state: JsonObject; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index 795a4b7ff6..fc866996b5 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -344,24 +344,45 @@ export class DatabaseTaskStore implements TaskStore { throw new NotFoundError(`No task with id '${taskId}' found`); } try { - const spec = JSON.parse(result.spec); - const secrets = result.secrets ? JSON.parse(result.secrets) : undefined; - const state = this.getState(result); - return { - id: result.id, - spec, - status: result.status, - lastHeartbeatAt: parseSqlDateToIsoString(result.last_heartbeat_at), - createdAt: parseSqlDateToIsoString(result.created_at), - createdBy: result.created_by ?? undefined, - secrets, - state, - }; + return this.parseTaskRow(result); } catch (error) { throw new Error(`Failed to parse spec of task '${taskId}', ${error}`); } } + async getTasks(taskIds: string[]): Promise { + const results = await this.db('tasks') + .whereIn('id', taskIds) + .select(); + + return results.map(result => { + try { + return this.parseTaskRow(result); + } catch (error) { + throw new Error( + `Failed to parse spec of task '${result.id}', ${error}`, + ); + } + }); + } + + private parseTaskRow(result: RawDbTaskRow): SerializedTask { + const spec = JSON.parse(result.spec); + const secrets = result.secrets ? JSON.parse(result.secrets) : undefined; + const state = this.getState(result); + + return { + id: result.id, + spec, + status: result.status, + lastHeartbeatAt: parseSqlDateToIsoString(result.last_heartbeat_at), + createdAt: parseSqlDateToIsoString(result.created_at), + createdBy: result.created_by ?? undefined, + secrets, + state, + }; + } + async createTask( options: TaskStoreCreateTaskOptions, ): Promise { diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts index 86f9f7fcf7..4d3c9414aa 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts @@ -29,6 +29,7 @@ import { TaskBrokerDispatchOptions, TaskCompletionState, TaskContext, + TaskFilters, TaskSecrets, TaskStatus, } from '@backstage/plugin-scaffolder-node'; @@ -43,6 +44,7 @@ import ObservableImpl from 'zen-observable'; import { DefaultWorkspaceService, WorkspaceService } from './WorkspaceService'; import { readDuration } from './helper'; import { InternalTaskSecrets, TaskStore } from './types'; +import { PermissionCriteria } from '@backstage/plugin-permission-common'; type TaskState = { checkpoints: { @@ -291,6 +293,7 @@ export class StorageTaskBroker implements TaskBroker { offset?: number; }; order?: { order: 'asc' | 'desc'; field: string }[]; + permissionFilters?: PermissionCriteria; }): Promise<{ tasks: SerializedTask[]; totalTasks?: number }> { if (!this.storage.list) { throw new Error( @@ -400,6 +403,13 @@ export class StorageTaskBroker implements TaskBroker { return this.storage.getTask(taskId); } + /** + * {@inheritdoc TaskBroker.getTasks} + */ + getTasks(taskIds: string[]): Promise { + return this.storage.getTasks(taskIds); + } + /** * {@inheritdoc TaskBroker.event$} */ diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts index 62ffb171ff..f9d10ddb0f 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts @@ -110,6 +110,8 @@ export interface TaskStore { getTask(taskId: string): Promise; + getTasks(taskIds: string[]): Promise; + claimTask(): Promise; completeTask(options: { diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 291fcbdae1..ba476ae25f 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -366,13 +366,7 @@ export async function createRouter( resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, permissions: scaffolderTaskPermissions, rules: taskRules, - getResources: async resourceRefs => { - return Promise.all( - resourceRefs.map(async taskId => { - return await taskBroker.get(taskId); - }), - ); - }, + getResources: resourceRefs => taskBroker.getTasks(resourceRefs), }, ], permissions: scaffolderPermissions, diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index 09829d2723..ba8263cf62 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -361,6 +361,8 @@ export interface TaskBroker { // (undocumented) get(taskId: string): Promise; // (undocumented) + getTasks(taskIds: string[]): Promise; + // (undocumented) list?(options?: { filters?: { createdBy?: string | string[]; diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index 53169e3432..c15832645d 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -204,6 +204,8 @@ export interface TaskBroker { get(taskId: string): Promise; + getTasks(taskIds: string[]): Promise; + list?(options?: { filters?: { createdBy?: string | string[]; From 66f50e6a421dad2e9e833cb6d3dfab823e8a1fa6 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Fri, 25 Apr 2025 12:43:25 -0400 Subject: [PATCH 09/16] remove console.log statements Signed-off-by: Kashish Mittal --- .../src/scaffolder/tasks/DatabaseTaskStore.test.ts | 2 -- .../src/scaffolder/tasks/DatabaseTaskStore.ts | 3 --- 2 files changed, 5 deletions(-) diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts index e86610e159..28fdd5cb4e 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts @@ -272,8 +272,6 @@ describe('DatabaseTaskStore', () => { permissionFilters: permissionFilters, }); - console.log(JSON.stringify); - expect(totalTasks).toBe(3); const createdByList = tasks.map(task => task.createdBy); diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index fc866996b5..6a375b974b 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -216,7 +216,6 @@ export class DatabaseTaskStore implements TaskStore { ): Knex.QueryBuilder { // handle not criteria if (isNotCriteria(filter)) { - console.log(`reached here: ${JSON.stringify(filter)}`); return this.parseFilter(filter.not, query, db, !negate); } @@ -290,8 +289,6 @@ export class DatabaseTaskStore implements TaskStore { this.parseFilter(combinedPermissionFilters, queryBuilder, this.db); } - console.log(`Parse FIlters: ${queryBuilder}`); - if (status || filters?.status) { const arr: TaskStatus[] = flattenParams( status, From d2b8e6e461414a49ddda09d1f9f100fa168455e0 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Mon, 28 Apr 2025 11:27:59 -0400 Subject: [PATCH 10/16] address review comments Signed-off-by: Kashish Mittal --- .../scaffolder-backend/report-alpha.api.md | 3 +- plugins/scaffolder-backend/report.api.md | 4 --- .../tasks/DatabaseTaskStore.test.ts | 2 +- .../src/scaffolder/tasks/DatabaseTaskStore.ts | 32 ++++--------------- .../src/scaffolder/tasks/StorageTaskBroker.ts | 7 ---- .../src/scaffolder/tasks/types.ts | 2 -- .../src/service/permissions.ts | 5 ++- .../scaffolder-backend/src/service/router.ts | 2 +- .../src/service/rules.test.ts | 4 +-- .../scaffolder-backend/src/service/rules.ts | 6 ++-- plugins/scaffolder-node/report.api.md | 6 ++-- plugins/scaffolder-node/src/tasks/types.ts | 6 ++-- .../OngoingTask/OngoingTask.test.tsx | 10 ------ .../components/OngoingTask/OngoingTask.tsx | 2 +- 14 files changed, 21 insertions(+), 70 deletions(-) diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index bf2dd186a9..6c64bfa848 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -11,7 +11,6 @@ import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { SerializedTask } from '@backstage/plugin-scaffolder-node'; -import { TaskFilter } from '@backstage/plugin-scaffolder-node'; import { TemplateEntityStepV1beta3 } from '@backstage/plugin-scaffolder-common'; import { TemplateParametersV1beta3 } from '@backstage/plugin-scaffolder-common'; @@ -89,7 +88,7 @@ export const scaffolderTaskConditions: Conditions<{ hasCreatedBy: PermissionRule< SerializedTask, { - property: TaskFilter['property']; + key: string; values: any; }, 'scaffolder-task', diff --git a/plugins/scaffolder-backend/report.api.md b/plugins/scaffolder-backend/report.api.md index cc7d83c7e1..e22a7d1370 100644 --- a/plugins/scaffolder-backend/report.api.md +++ b/plugins/scaffolder-backend/report.api.md @@ -329,8 +329,6 @@ export class DatabaseTaskStore implements TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) getTaskState({ taskId }: { taskId: string }): Promise< | { state: JsonObject; @@ -485,8 +483,6 @@ export interface TaskStore { // (undocumented) getTask(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) getTaskState?({ taskId }: { taskId: string }): Promise< | { state: JsonObject; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts index 28fdd5cb4e..9c1ed35b27 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts @@ -263,7 +263,7 @@ describe('DatabaseTaskStore', () => { const permissionFilters: PermissionCriteria = { not: { - property: 'createdBy', + key: 'created_by', values: ['user:default/three', 'user:default/four'], }, }; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts index 6a375b974b..f07d827f45 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts @@ -205,7 +205,7 @@ export class DatabaseTaskStore implements TaskStore { } private isTaskFilter(filter: any): filter is TaskFilter { - return filter.hasOwnProperty('property'); + return filter.hasOwnProperty('key'); } private parseFilter( @@ -214,20 +214,16 @@ export class DatabaseTaskStore implements TaskStore { db: Knex, negate: boolean = false, ): Knex.QueryBuilder { - // handle not criteria if (isNotCriteria(filter)) { return this.parseFilter(filter.not, query, db, !negate); } if (this.isTaskFilter(filter)) { const values: string[] = compact(filter.values) ?? []; - - if (filter.property === 'createdBy') { - if (negate) { - query.whereNotIn('created_by', [...new Set(values)]); - } else { - query.whereIn('created_by', [...new Set(values)]); - } + if (negate) { + query.whereNotIn(filter.key, values); + } else { + query.whereIn(filter.key, values); } return query; @@ -279,7 +275,7 @@ export class DatabaseTaskStore implements TaskStore { createdByValues.length > 0 ? { allOf: [ - { property: 'createdBy', values: createdByValues }, + { key: 'created_by', values: createdByValues }, ...(permissionFilters ? [permissionFilters] : []), ], } @@ -347,22 +343,6 @@ export class DatabaseTaskStore implements TaskStore { } } - async getTasks(taskIds: string[]): Promise { - const results = await this.db('tasks') - .whereIn('id', taskIds) - .select(); - - return results.map(result => { - try { - return this.parseTaskRow(result); - } catch (error) { - throw new Error( - `Failed to parse spec of task '${result.id}', ${error}`, - ); - } - }); - } - private parseTaskRow(result: RawDbTaskRow): SerializedTask { const spec = JSON.parse(result.spec); const secrets = result.secrets ? JSON.parse(result.secrets) : undefined; diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts index 4d3c9414aa..9e154a6dfc 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts @@ -403,13 +403,6 @@ export class StorageTaskBroker implements TaskBroker { return this.storage.getTask(taskId); } - /** - * {@inheritdoc TaskBroker.getTasks} - */ - getTasks(taskIds: string[]): Promise { - return this.storage.getTasks(taskIds); - } - /** * {@inheritdoc TaskBroker.event$} */ diff --git a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts index f9d10ddb0f..62ffb171ff 100644 --- a/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts +++ b/plugins/scaffolder-backend/src/scaffolder/tasks/types.ts @@ -110,8 +110,6 @@ export interface TaskStore { getTask(taskId: string): Promise; - getTasks(taskIds: string[]): Promise; - claimTask(): Promise; completeTask(options: { diff --git a/plugins/scaffolder-backend/src/service/permissions.ts b/plugins/scaffolder-backend/src/service/permissions.ts index cd10cabde0..e31e627b03 100644 --- a/plugins/scaffolder-backend/src/service/permissions.ts +++ b/plugins/scaffolder-backend/src/service/permissions.ts @@ -27,7 +27,6 @@ import { import { PermissionRuleParams } from '@backstage/plugin-permission-common'; import { SerializedTask, - TaskFilter, } from '@backstage/plugin-scaffolder-node'; /** @@ -82,8 +81,8 @@ export type TaskPermissionRuleInput< > = PermissionRule< SerializedTask, { - property: TaskFilter['property']; - values: any; + key: string; + values?: string[]; }, typeof RESOURCE_TYPE_SCAFFOLDER_TASK, TParams diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index ba476ae25f..21e4c8b688 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -136,6 +136,7 @@ import { } from '@backstage/plugin-scaffolder-node'; /** + * RouterOptions */ export interface RouterOptions { @@ -366,7 +367,6 @@ export async function createRouter( resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, permissions: scaffolderTaskPermissions, rules: taskRules, - getResources: resourceRefs => taskBroker.getTasks(resourceRefs), }, ], permissions: scaffolderPermissions, diff --git a/plugins/scaffolder-backend/src/service/rules.test.ts b/plugins/scaffolder-backend/src/service/rules.test.ts index 68b2930e32..92d3718496 100644 --- a/plugins/scaffolder-backend/src/service/rules.test.ts +++ b/plugins/scaffolder-backend/src/service/rules.test.ts @@ -586,7 +586,7 @@ describe('hasCreatedBy', () => { hasCreatedBy.toQuery({ createdBy: ['user:default/user-1'], }), - ).toEqual({ property: 'createdBy', values: ['user:default/user-1'] }); + ).toEqual({ key: 'created_by', values: ['user:default/user-1'] }); }); }); it('returns the correct query filter with values (multiple users in createdBy)', () => { @@ -595,7 +595,7 @@ describe('hasCreatedBy', () => { createdBy: ['user:default/user-1', 'user:default/user-2'], }), ).toEqual({ - property: 'createdBy', + key: 'created_by', values: ['user:default/user-1', 'user:default/user-2'], }); }); diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index d4f7afae89..69fb69bb73 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -27,7 +27,7 @@ import { TemplateParametersV1beta3, } from '@backstage/plugin-scaffolder-common'; -import { SerializedTask, TaskFilter } from '@backstage/plugin-scaffolder-node'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; import { z } from 'zod'; import { JsonObject, JsonPrimitive } from '@backstage/types'; @@ -136,7 +136,7 @@ function buildHasProperty>({ export const createTaskPermissionRule = makeCreatePermissionRule< SerializedTask, { - property: TaskFilter['property']; + key: string; values: any; }, typeof RESOURCE_TYPE_SCAFFOLDER_TASK @@ -161,7 +161,7 @@ export const hasCreatedBy = createTaskPermissionRule({ }, toQuery: ({ createdBy }) => { return { - property: 'createdBy' as TaskFilter['property'], + key: 'created_by', values: createdBy, }; }, diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index ba8263cf62..8fe081ad13 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -361,8 +361,6 @@ export interface TaskBroker { // (undocumented) get(taskId: string): Promise; // (undocumented) - getTasks(taskIds: string[]): Promise; - // (undocumented) list?(options?: { filters?: { createdBy?: string | string[]; @@ -470,8 +468,8 @@ export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered'; // @public export type TaskFilter = { - property: 'createdBy'; - values: Array | undefined; + key: string; + values?: Array | undefined; }; // @public diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index c15832645d..96c081c4c8 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -110,8 +110,8 @@ export type TaskBrokerDispatchOptions = { * @public */ export type TaskFilter = { - property: 'createdBy'; - values: Array | undefined; + key: string; + values?: Array | undefined; }; /** @@ -204,8 +204,6 @@ export interface TaskBroker { get(taskId: string): Promise; - getTasks(taskIds: string[]): Promise; - list?(options?: { filters?: { createdBy?: string | string[]; diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx index 2ef7ddc4d5..5e44d5cf43 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx @@ -161,16 +161,6 @@ describe('OngoingTask', () => { await expect(rendered.findByText('Hide Logs')).resolves.toBeInTheDocument(); }); - it('should render not found error page when user does not have permission to read the task', async () => { - const permissionApi = mockApis.permission({ - authorize: AuthorizeResult.DENY, - }); - - await expect(render(permissionApi)).rejects.toThrow( - 'Reached NotFound Page', - ); - }); - it('should have cancel button be disabled when user has read permission but lacks cancel permission', async () => { const permissionApi = mockApis.permission({ authorize: request => { diff --git a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx index b3c82dca19..bc0d5fe1aa 100644 --- a/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx @@ -46,7 +46,7 @@ import { TaskSteps, } from '@backstage/plugin-scaffolder-react/alpha'; import { useAsync } from '@react-hookz/web'; -import { usePermission} from '@backstage/plugin-permission-react'; +import { usePermission } from '@backstage/plugin-permission-react'; import { taskCancelPermission, taskCreatePermission, From e161cf1043738733ddd4dbd93442d0ef2c3b6e07 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Fri, 9 May 2025 15:22:19 -0400 Subject: [PATCH 11/16] enforce values as string[] instead of any Signed-off-by: Kashish Mittal --- plugins/scaffolder-backend/src/service/router.ts | 1 - plugins/scaffolder-node/report.api.md | 2 +- plugins/scaffolder-node/src/tasks/types.ts | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 21e4c8b688..47bdb416de 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -136,7 +136,6 @@ import { } from '@backstage/plugin-scaffolder-node'; /** - * RouterOptions */ export interface RouterOptions { diff --git a/plugins/scaffolder-node/report.api.md b/plugins/scaffolder-node/report.api.md index 8fe081ad13..4a126a63d3 100644 --- a/plugins/scaffolder-node/report.api.md +++ b/plugins/scaffolder-node/report.api.md @@ -469,7 +469,7 @@ export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered'; // @public export type TaskFilter = { key: string; - values?: Array | undefined; + values?: string[]; }; // @public diff --git a/plugins/scaffolder-node/src/tasks/types.ts b/plugins/scaffolder-node/src/tasks/types.ts index 96c081c4c8..2d2c3737c5 100644 --- a/plugins/scaffolder-node/src/tasks/types.ts +++ b/plugins/scaffolder-node/src/tasks/types.ts @@ -111,7 +111,7 @@ export type TaskBrokerDispatchOptions = { */ export type TaskFilter = { key: string; - values?: Array | undefined; + values?: string[]; }; /** From ea4bdfa48e751e91d37050a5db592ced23857033 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Fri, 16 May 2025 10:31:05 -0400 Subject: [PATCH 12/16] rename hasCreatedBy to isTaskOwner Signed-off-by: Kashish Mittal --- .changeset/weak-bags-behave.md | 2 +- .../authorizing-scaffolder-template-details.md | 4 ++-- plugins/scaffolder-backend/report-alpha.api.md | 2 +- .../src/service/rules.test.ts | 18 +++++++++--------- .../scaffolder-backend/src/service/rules.ts | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/.changeset/weak-bags-behave.md b/.changeset/weak-bags-behave.md index c8024f8ff4..25a0147917 100644 --- a/.changeset/weak-bags-behave.md +++ b/.changeset/weak-bags-behave.md @@ -9,4 +9,4 @@ BREAKING : - Converted `scaffolder.task.read` and `scaffolder.task.cancel` into Resource Permissions. -- Added a new scaffolder rule `hasCreatedBy` for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on task creators. +- Added a new scaffolder rule `isTaskOwner` for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on task creators. diff --git a/docs/features/software-templates/authorizing-scaffolder-template-details.md b/docs/features/software-templates/authorizing-scaffolder-template-details.md index 200035e434..7ff2cd9377 100644 --- a/docs/features/software-templates/authorizing-scaffolder-template-details.md +++ b/docs/features/software-templates/authorizing-scaffolder-template-details.md @@ -220,7 +220,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { // Allow users to read tasks they created return createScaffolderTaskConditionalDecision( request.permission, - scaffolderTaskConditions.hasCreatedBy({ + scaffolderTaskConditions.isTaskOwner({ createdBy: user?.info.userEntityRef ? [user?.info.userEntityRef] : [], }), ); @@ -245,7 +245,7 @@ class ExamplePermissionPolicy implements PermissionPolicy { if (user?.info.userEntityRef === 'user:default/bob') { return createScaffolderTaskConditionalDecision( request.permission, - scaffolderTaskConditions.hasCreatedBy({ + scaffolderTaskConditions.isTaskOwner({ createdBy: user?.info.userEntityRef ? [user?.info.userEntityRef] : [], diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index 6c64bfa848..47b87602f6 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -85,7 +85,7 @@ export const scaffolderActionConditions: Conditions<{ // @alpha export const scaffolderTaskConditions: Conditions<{ - hasCreatedBy: PermissionRule< + isTaskOwner: PermissionRule< SerializedTask, { key: string; diff --git a/plugins/scaffolder-backend/src/service/rules.test.ts b/plugins/scaffolder-backend/src/service/rules.test.ts index 92d3718496..85e393247a 100644 --- a/plugins/scaffolder-backend/src/service/rules.test.ts +++ b/plugins/scaffolder-backend/src/service/rules.test.ts @@ -22,7 +22,7 @@ import { hasProperty, hasStringProperty, hasTag, - hasCreatedBy, + isTaskOwner, } from './rules'; import { createConditionAuthorizer } from '@backstage/plugin-permission-node'; import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-common/alpha'; @@ -527,7 +527,7 @@ describe('hasStringProperty', () => { }); }); -describe('hasCreatedBy', () => { +describe('isTaskOwner', () => { describe('apply', () => { const task: SerializedTask = { id: 'a-random-id', @@ -538,28 +538,28 @@ describe('hasCreatedBy', () => { }; it('returns false when createdBy is an empty array', () => { expect( - hasCreatedBy.apply(task, { + isTaskOwner.apply(task, { createdBy: [], }), ).toEqual(false); }); it('returns false when createdBy is not matched (single user in createdBy)', () => { expect( - hasCreatedBy.apply(task, { + isTaskOwner.apply(task, { createdBy: ['not-matched'], }), ).toEqual(false); }); it('returns true when createdBy matches (single user in createdBy)', () => { expect( - hasCreatedBy.apply(task, { + isTaskOwner.apply(task, { createdBy: ['user:default/user-1'], }), ).toEqual(true); }); it('returns false when createdBy is not matched (multiple users in createdBy)', () => { expect( - hasCreatedBy.apply(task, { + isTaskOwner.apply(task, { createdBy: [ 'user:default/user-2', 'user:default/user-3', @@ -570,7 +570,7 @@ describe('hasCreatedBy', () => { }); it('returns true when createdBy matches (multiple users in createdBy)', () => { expect( - hasCreatedBy.apply(task, { + isTaskOwner.apply(task, { createdBy: [ 'user:default/user-1', 'user:default/user-2', @@ -583,7 +583,7 @@ describe('hasCreatedBy', () => { describe('toQuery', () => { it('returns the correct query filter with values (single user in createdBy)', () => { expect( - hasCreatedBy.toQuery({ + isTaskOwner.toQuery({ createdBy: ['user:default/user-1'], }), ).toEqual({ key: 'created_by', values: ['user:default/user-1'] }); @@ -591,7 +591,7 @@ describe('hasCreatedBy', () => { }); it('returns the correct query filter with values (multiple users in createdBy)', () => { expect( - hasCreatedBy.toQuery({ + isTaskOwner.toQuery({ createdBy: ['user:default/user-1', 'user:default/user-2'], }), ).toEqual({ diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index 69fb69bb73..05575cbbe3 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -142,8 +142,8 @@ export const createTaskPermissionRule = makeCreatePermissionRule< typeof RESOURCE_TYPE_SCAFFOLDER_TASK >(); -export const hasCreatedBy = createTaskPermissionRule({ - name: 'HAS_CREATED_BY', +export const isTaskOwner = createTaskPermissionRule({ + name: 'IS_TASK_OWNER', description: 'Allows tasks created by certain users to be accessible', resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK, paramsSchema: z.object({ @@ -174,4 +174,4 @@ export const scaffolderActionRules = { hasNumberProperty, hasStringProperty, }; -export const scaffolderTaskRules = { hasCreatedBy }; +export const scaffolderTaskRules = { isTaskOwner }; From 5612be37b7c3cc62e674b4b0c4bd17ed298dc113 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Wed, 4 Jun 2025 16:51:30 -0400 Subject: [PATCH 13/16] fix prettier issues Signed-off-by: Kashish Mittal --- plugins/scaffolder-backend/src/service/permissions.ts | 6 ++---- plugins/scaffolder-backend/src/service/router.ts | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/permissions.ts b/plugins/scaffolder-backend/src/service/permissions.ts index e31e627b03..b73cce7a74 100644 --- a/plugins/scaffolder-backend/src/service/permissions.ts +++ b/plugins/scaffolder-backend/src/service/permissions.ts @@ -25,9 +25,7 @@ import { RESOURCE_TYPE_SCAFFOLDER_TEMPLATE, } from '@backstage/plugin-scaffolder-common/alpha'; import { PermissionRuleParams } from '@backstage/plugin-permission-common'; -import { - SerializedTask, -} from '@backstage/plugin-scaffolder-node'; +import { SerializedTask } from '@backstage/plugin-scaffolder-node'; /** * @@ -91,4 +89,4 @@ export function isTaskPermissionRuleInput( permissionRule: ScaffolderPermissionRuleInput, ): permissionRule is TaskPermissionRuleInput { return permissionRule.resourceType === RESOURCE_TYPE_SCAFFOLDER_TASK; -} \ No newline at end of file +} diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 47bdb416de..15aed8a4fb 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -131,9 +131,7 @@ import { scaffolderTaskRules, } from './rules'; -import { - TaskFilters, -} from '@backstage/plugin-scaffolder-node'; +import { TaskFilters } from '@backstage/plugin-scaffolder-node'; /** * RouterOptions From cd8bf7e1442c6093fa98886c652d9ba9167d0fe9 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Wed, 18 Jun 2025 16:34:12 -0400 Subject: [PATCH 14/16] fix failing tests Signed-off-by: Kashish Mittal --- .../src/service/router.test.ts | 40 +++++++++++++++++++ .../components/OngoingTask/ContextMenu.tsx | 4 +- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/plugins/scaffolder-backend/src/service/router.test.ts b/plugins/scaffolder-backend/src/service/router.test.ts index 661939eaf2..1516ebb79c 100644 --- a/plugins/scaffolder-backend/src/service/router.test.ts +++ b/plugins/scaffolder-backend/src/service/router.test.ts @@ -1032,6 +1032,16 @@ describe('scaffolder router', () => { describe('GET /v2/tasks/:taskId/eventstream', () => { it('should return log messages', async () => { const { router, taskBroker } = await createTestRouter(); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: '', + }); let subscriber: ZenObservable.SubscriptionObserver<{ events: SerializedTaskEvent[]; }>; @@ -1117,6 +1127,16 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ it('should return log messages with after query', async () => { const { router, taskBroker } = await createTestRouter(); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: '', + }); let subscriber: ZenObservable.SubscriptionObserver<{ events: SerializedTaskEvent[]; }>; @@ -1181,6 +1201,16 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ describe('GET /v2/tasks/:taskId/events', () => { it('should return log messages', async () => { const { router, taskBroker } = await createTestRouter(); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: '', + }); let subscriber: ZenObservable.SubscriptionObserver<{ events: SerializedTaskEvent[]; }>; @@ -1241,6 +1271,16 @@ data: {"id":1,"taskId":"a-random-id","type":"completion","createdAt":"","body":{ it('should return log messages with after query', async () => { const { router, taskBroker } = await createTestRouter(); + (taskBroker.get as jest.Mocked['get']).mockResolvedValue({ + id: 'a-random-id', + spec: {} as any, + status: 'completed', + createdAt: '', + secrets: { + __initiatorCredentials: JSON.stringify(credentials), + }, + createdBy: '', + }); let subscriber: ZenObservable.SubscriptionObserver<{ events: SerializedTaskEvent[]; }>; diff --git a/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx b/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx index ee27053a99..2df1fc4ddb 100644 --- a/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx +++ b/plugins/scaffolder/src/components/OngoingTask/ContextMenu.tsx @@ -68,7 +68,7 @@ export const ContextMenu = (props: ContextMenuProps) => { onStartOver, onToggleLogs, onToggleButtonBar, - taskId + taskId, } = props; const { getPageTheme } = useTheme(); const pageTheme = getPageTheme({ themeId: 'website' }); @@ -78,7 +78,7 @@ export const ContextMenu = (props: ContextMenuProps) => { const { allowed: canReadTask } = usePermission({ permission: taskReadPermission, - resourceRef: taskId + resourceRef: taskId, }); const { allowed: canCreateTask } = usePermission({ From a99cb92f6e5b416102d1f07be4e6dc0a296ec5b2 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Tue, 24 Jun 2025 17:23:57 -0400 Subject: [PATCH 15/16] update changeset Signed-off-by: Kashish Mittal --- .changeset/weak-bags-behave.md | 7 ++++--- plugins/scaffolder-backend/src/service/router.ts | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.changeset/weak-bags-behave.md b/.changeset/weak-bags-behave.md index 25a0147917..57bf1903f1 100644 --- a/.changeset/weak-bags-behave.md +++ b/.changeset/weak-bags-behave.md @@ -6,7 +6,8 @@ '@backstage/plugin-scaffolder': minor --- -BREAKING : +BREAKING `/alpha`: Converted `scaffolder.task.read` and `scaffolder.task.cancel` into Resource Permissions. -- Converted `scaffolder.task.read` and `scaffolder.task.cancel` into Resource Permissions. -- Added a new scaffolder rule `isTaskOwner` for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on task creators. +BREAKING `/alpha`: Added a new scaffolder rule `isTaskOwner` for `scaffolder.task.read` and `scaffolder.task.cancel` to allow for conditional permission policies such as restricting access to tasks and task events based on task creators. + +BREAKING `/alpha`: Retrying a task now requires both `scaffolder.task.read` and `scaffolder.task.create` permissions, replacing the previous requirement of `scaffolder.task.read` and `scaffolder.task.cancel`. diff --git a/plugins/scaffolder-backend/src/service/router.ts b/plugins/scaffolder-backend/src/service/router.ts index 15aed8a4fb..6e476d9a19 100644 --- a/plugins/scaffolder-backend/src/service/router.ts +++ b/plugins/scaffolder-backend/src/service/router.ts @@ -709,7 +709,7 @@ export async function createRouter( const credentials = await httpAuth.credentials(req); const task = await taskBroker.get(taskId); - // Requires both read and cancel permissions + // Requires both read and create permissions await checkPermission({ credentials, permissions: [taskCreatePermission], From f66e65a789aa642575ca4ad1d51c935695153ec3 Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Tue, 24 Jun 2025 18:25:24 -0400 Subject: [PATCH 16/16] address review comments Signed-off-by: Kashish Mittal --- plugins/scaffolder-backend/report-alpha.api.md | 6 ++---- plugins/scaffolder-backend/src/service/rules.ts | 7 ++----- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/plugins/scaffolder-backend/report-alpha.api.md b/plugins/scaffolder-backend/report-alpha.api.md index 47b87602f6..966d71acaa 100644 --- a/plugins/scaffolder-backend/report-alpha.api.md +++ b/plugins/scaffolder-backend/report-alpha.api.md @@ -11,6 +11,7 @@ import { PermissionCriteria } from '@backstage/plugin-permission-common'; import { PermissionRule } from '@backstage/plugin-permission-node'; import { ResourcePermission } from '@backstage/plugin-permission-common'; import { SerializedTask } from '@backstage/plugin-scaffolder-node'; +import { TaskFilter } from '@backstage/plugin-scaffolder-node'; import { TemplateEntityStepV1beta3 } from '@backstage/plugin-scaffolder-common'; import { TemplateParametersV1beta3 } from '@backstage/plugin-scaffolder-common'; @@ -87,10 +88,7 @@ export const scaffolderActionConditions: Conditions<{ export const scaffolderTaskConditions: Conditions<{ isTaskOwner: PermissionRule< SerializedTask, - { - key: string; - values: any; - }, + TaskFilter, 'scaffolder-task', { createdBy: string[]; diff --git a/plugins/scaffolder-backend/src/service/rules.ts b/plugins/scaffolder-backend/src/service/rules.ts index 05575cbbe3..5753668089 100644 --- a/plugins/scaffolder-backend/src/service/rules.ts +++ b/plugins/scaffolder-backend/src/service/rules.ts @@ -27,7 +27,7 @@ import { TemplateParametersV1beta3, } from '@backstage/plugin-scaffolder-common'; -import { SerializedTask } from '@backstage/plugin-scaffolder-node'; +import { SerializedTask, TaskFilter } from '@backstage/plugin-scaffolder-node'; import { z } from 'zod'; import { JsonObject, JsonPrimitive } from '@backstage/types'; @@ -135,10 +135,7 @@ function buildHasProperty>({ export const createTaskPermissionRule = makeCreatePermissionRule< SerializedTask, - { - key: string; - values: any; - }, + TaskFilter, typeof RESOURCE_TYPE_SCAFFOLDER_TASK >();