From fe85d897d59fb84466debf9d4744cf5d080c742b Mon Sep 17 00:00:00 2001 From: Kashish Mittal Date: Tue, 15 Apr 2025 17:08:43 -0400 Subject: [PATCH] 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; };