fix(core): Check the delete permission when the agent page moves a skill (no-changelog) (#41222)

packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts

@@ -478,19 +478,21 @@ describe('AgentSkillRefsService', () => {
 				{ projectId: null, agentId: 'agent-1' },
 				'agent-1',
 				ctx,
-				undefined,
+				{},
 			);
 		});
 
 		it('checks the name that the same save gives the skill', async () => {
-			await service.setAvailability(agent, 'skill_a', 'project', ctx, 'Condense');
+			await service.setAvailability(agent, 'skill_a', 'project', ctx, {
+				nameAfterMove: 'Condense',
+			});
 
 			expect(skillService.changeTarget).toHaveBeenCalledWith(
 				'skill_a',
 				{ projectId: agent.projectId, agentId: null },
 				'agent-1',
 				ctx,
-				'Condense',
+				{ nameAfterMove: 'Condense' },
 			);
 		});
 	});

packages/cli/src/modules/agents/__tests__/agent-skills.service.test.ts

@@ -364,6 +364,7 @@ describe('AgentSkillsService', () => {
 				skills: { skill_a: skill },
 			});
 			skillRefs.availabilityOf.mockResolvedValue('project');
+			skillService.assertCanMove.mockResolvedValue({ projectId, agentId: null });
 
 			await service.updateSkill(
 				agentId,
@@ -377,7 +378,40 @@ describe('AgentSkillsService', () => {
 			);
 
 			expect(skillService.assertCanEdit).toHaveBeenCalledWith(user, ['skill_a']);
-			expect(skillRefs.setAvailability).toHaveBeenCalledWith(agent, 'skill_a', 'agent', {});
+			expect(skillService.assertCanMove).toHaveBeenCalledWith(user, 'skill_a');
+			expect(skillRefs.setAvailability).toHaveBeenCalledWith(
+				agent,
+				'skill_a',
+				'agent',
+				{},
+				{ checkedSource: { projectId, agentId: null } },
+			);
+			expect(skillService.save).not.toHaveBeenCalled();
+		});
+
+		it('refuses to change where the skill can be added without the delete permission', async () => {
+			givenAgent({
+				schema: { ...baseConfig, skills: [{ type: 'skill', id: 'skill_a' }] },
+				skills: { skill_a: skill },
+			});
+			skillRefs.availabilityOf.mockResolvedValue('project');
+			skillService.assertCanMove.mockRejectedValue(
+				new ForbiddenError('You are not allowed to move this skill out of its scope.'),
+			);
+
+			await expect(
+				service.updateSkill(
+					agentId,
+					projectId,
+					'skill_a',
+					{},
+					context,
+					undefined,
+					undefined,
+					'agent',
+				),
+			).rejects.toThrow('You are not allowed to move this skill out of its scope.');
+			expect(skillRefs.setAvailability).not.toHaveBeenCalled();
 			expect(skillService.save).not.toHaveBeenCalled();
 		});
 

packages/cli/src/modules/agents/__tests__/skill.service.test.ts

@@ -659,7 +659,7 @@ describe('SkillService', () => {
 				{ projectId: null, agentId: null },
 				undefined,
 				{},
-				'Fresh name',
+				{ nameAfterMove: 'Fresh name' },
 			);
 
 			expect(skills.findLatestSummaries).not.toHaveBeenCalled();
@@ -679,6 +679,43 @@ describe('SkillService', () => {
 			expect(skills.updateTarget).not.toHaveBeenCalled();
 		});
 
+		it('refuses the move when the skill left the scope that the permission check saw', async () => {
+			skills.findUsingAgentIds.mockResolvedValue([]);
+			// Another move put the skill in a project after `assertCanMove` checked the instance.
+			skills.findByIds.mockResolvedValue([mock<Skill>({ projectId: 'p2', agentId: null })]);
+
+			await expect(
+				service.changeTarget(
+					'skill_a',
+					{ projectId: null, agentId: 'agent-1' },
+					'agent-1',
+					{},
+					{
+						checkedSource: { projectId: null, agentId: null },
+					},
+				),
+			).rejects.toThrow('The skill was moved somewhere else.');
+			expect(skills.findByIds).toHaveBeenCalledWith(['skill_a'], TX_CTX);
+			expect(skills.updateTarget).not.toHaveBeenCalled();
+		});
+
+		it('moves the skill when it is still in the scope that the permission check saw', async () => {
+			skills.findUsingAgentIds.mockResolvedValue([]);
+			skills.findByIds.mockResolvedValue([mock<Skill>({ projectId: null, agentId: null })]);
+
+			await service.changeTarget(
+				'skill_a',
+				{ projectId: 'p1', agentId: null },
+				undefined,
+				{},
+				{
+					checkedSource: { projectId: null, agentId: null },
+				},
+			);
+
+			expect(skills.updateTarget).toHaveBeenCalled();
+		});
+
 		it('moves an unused skill when no agent is kept', async () => {
 			skills.findUsingAgentIds.mockResolvedValue([]);
 

packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts

@@ -513,6 +513,7 @@ describe('SkillsApiService', () => {
 				new Map([['project-1', { name: 'Team', type: 'team' as const }]]),
 			);
 			skillService.save.mockResolvedValue(saved);
+			skillService.assertCanMove.mockResolvedValue({ projectId: null, agentId: null });
 
 			await service.update(
 				user,
@@ -525,7 +526,8 @@ describe('SkillsApiService', () => {
 				{ projectId: 'project-1', agentId: null },
 				undefined,
 				TX_CTX,
-				undefined,
+				// The move is refused under its lock if the skill left the scope checked here.
+				{ nameAfterMove: undefined, checkedSource: { projectId: null, agentId: null } },
 			);
 			expect(skillService.save).toHaveBeenCalledWith(
 				'skill_a',

packages/cli/src/modules/agents/agent-skill-refs.service.ts

@@ -13,7 +13,12 @@ import type { Agent } from './entities/agent.entity';
 import { withoutLegacySkillRefs, type AgentSkillRefs } from './json-config/agent-document';
 import { AgentRepository } from './repositories/agent.repository';
 import { SkillRepository } from './repositories/skill.repository';
-import { SkillService, toAgentSkill, type SkillRef } from './skills/skill.service';
+import {
+	SkillService,
+	toAgentSkill,
+	type SkillMoveOptions,
+	type SkillRef,
+} from './skills/skill.service';
 import { SkillsHubRollout } from './skills/skills-hub-rollout';
 import { getAgentSkillHash } from './utils/agent-config-hash';
 import { getMissingSkillIds } from './utils/agent-missing-skill-ids';
@@ -447,15 +452,14 @@ export class AgentSkillRefsService {
 		skillId: string,
 		availableTo: AgentSkillAvailability,
 		ctx: OperationContext,
-		/** The name the skill has after this request, when the same request renames it. */
-		nameAfterMove?: string,
+		options: SkillMoveOptions = {},
 	): Promise<void> {
 		await this.skillService.changeTarget(
 			skillId,
 			targetFor(agent, availableTo),
 			agent.id,
 			ctx,
-			nameAfterMove,
+			options,
 		);
 	}
 

packages/cli/src/modules/agents/agent-skills.service.ts

@@ -27,6 +27,7 @@ import { composeJsonConfig } from './json-config/agent-config-composition';
 import type { AgentSkillRefs } from './json-config/agent-document';
 import { AgentRepository } from './repositories/agent.repository';
 import { findIntroducedNameClashes, findNameClashGroups } from './skills/skill-names';
+import type { SkillTarget } from './repositories/skill.repository';
 import { SkillService, withTrimmedName } from './skills/skill.service';
 import { getAgentConfigHash, getAgentSkillHash } from './utils/agent-config-hash';
 import { markAgentDraftDirty, saveAgentDraftFenced } from './utils/agent-draft.utils';
@@ -242,14 +243,24 @@ export class AgentSkillsService {
 		this.validateSkill(updated);
 
 		const nextAvailability = await this.availabilityChange(skillId, availableTo);
+		let checkedSource: SkillTarget | undefined;
 		if (nextAvailability) {
 			await this.skillService.assertCanEdit(context.user, [skillId]);
+			checkedSource = await this.skillService.assertCanMove(context.user, skillId);
 			await this.assertCanCreateSkills(context.user, entity.projectId, nextAvailability);
 		}
 
 		if (isEqual(existing, updated)) {
 			if (nextAvailability) {
-				await this.agentSkillRefs.setAvailability(entity, skillId, nextAvailability, {});
+				await this.agentSkillRefs.setAvailability(
+					entity,
+					skillId,
+					nextAvailability,
+					{},
+					{
+						checkedSource,
+					},
+				);
 			}
 			return {
 				id: skillId,
@@ -281,13 +292,10 @@ export class AgentSkillsService {
 		const saved = await this.transactionRunner.run({}, async (ctx) => {
 			if (nextAvailability) {
 				// The move checks the name the skill has after the save, not the old one.
-				await this.agentSkillRefs.setAvailability(
-					entity,
-					skillId,
-					nextAvailability,
-					ctx,
-					updated.name,
-				);
+				await this.agentSkillRefs.setAvailability(entity, skillId, nextAvailability, ctx, {
+					nameAfterMove: updated.name,
+					checkedSource,
+				});
 			}
 			if (pinned) {
 				await saveAgentDraftFenced(this.agentRepository, entity, ctx);

packages/cli/src/modules/agents/skills/skill.service.ts

@@ -98,6 +98,13 @@ function toRecord(rows: Map<string, ResolvedSkillRow>): Record<string, AgentSkil
  * attach checks, edit permission and the delete guard. The runtime loads skills by id
  * with no permission check.
  */
+export type SkillMoveOptions = {
+	/** The name the skill has after this request, when the same request renames it. */
+	nameAfterMove?: string;
+	/** The scope `assertCanMove` checked. The move is refused if the skill left it since. */
+	checkedSource?: SkillTarget;
+};
+
 @Service()
 export class SkillService {
 	constructor(
@@ -507,11 +514,24 @@ export class SkillService {
 		target: SkillTarget,
 		keepAgentId: string | undefined,
 		ctx: OperationContext = {},
-		/** The name the skill has after this request, when the same request renames it. */
-		nameAfterMove?: string,
+		{ nameAfterMove, checkedSource }: SkillMoveOptions = {},
 	): Promise<void> {
 		await this.txRunner.run(ctx, async (txCtx) => {
 			await this.skillRepository.lockForEdit([skillId], txCtx);
+			// `assertCanMove` ran before the lock. If another move changed the scope since,
+			// the permission was checked on the wrong scope.
+			if (checkedSource) {
+				const [locked] = await this.skillRepository.findByIds([skillId], txCtx);
+				if (!locked) throw new NotFoundError('Skill not found');
+				if (
+					locked.projectId !== checkedSource.projectId ||
+					locked.agentId !== checkedSource.agentId
+				) {
+					throw new ConflictError(
+						'The skill was moved somewhere else. Reload to see where it is now.',
+					);
+				}
+			}
 			const others = (await this.skillRepository.findUsingAgentIds(skillId, txCtx)).filter(
 				(id) => id !== keepAgentId,
 			);
@@ -597,6 +617,20 @@ export class SkillService {
 		return await this.canAccess(user, skill, 'update');
 	}
 
+	/**
+	 * A move takes the skill out of its scope, as a delete does, so it needs the delete
+	 * permission there. A custom role can grant update without delete.
+	 */
+	async assertCanMove(user: User, skillId: string): Promise<SkillTarget> {
+		const [skill] = await this.skillRepository.findByIds([skillId]);
+		if (!skill) throw new NotFoundError('Skill not found');
+		if (!(await this.canAccess(user, skill, 'delete'))) {
+			throw new ForbiddenError('You are not allowed to move this skill out of its scope.');
+		}
+		// Pass this to `changeTarget` as `checkedSource`, so the move fails if the scope changed.
+		return { projectId: skill.projectId, agentId: skill.agentId };
+	}
+
 	async assertCanEdit(user: User, skillIds: string[], ctx: OperationContext = {}): Promise<void> {
 		const skills = await this.skillRepository.findByIds(skillIds, ctx);
 		const names = await this.latestNames(

packages/cli/src/modules/agents/skills/skills-api.service.ts

@@ -99,11 +99,7 @@ export class SkillsApiService {
 			target !== undefined &&
 			(target.projectId !== skill.projectId || target.agentId !== skill.agentId);
 
-		// A move takes the skill out of its scope, as a delete does, so it needs both rights
-		// there. A custom role can grant update without delete.
-		if (moves && !(await this.skillService.canAccess(user, skill, 'delete'))) {
-			throw new ForbiddenError('You are not allowed to move this skill out of its scope.');
-		}
+		const checkedSource = moves ? await this.skillService.assertCanMove(user, skillId) : undefined;
 
 		if (!moves) {
 			const { followingAgentIds: _following, ...saved } = await this.skillService.save(
@@ -118,7 +114,10 @@ export class SkillsApiService {
 		// first and checks the final name in the new scope only: a name that only the old
 		// scope holds does not block it. The save then checks a rename in the new scope too.
 		const { followingAgentIds, ...saved } = await this.txRunner.run({}, async (ctx) => {
-			await this.skillService.changeTarget(skillId, target, undefined, ctx, update.name);
+			await this.skillService.changeTarget(skillId, target, undefined, ctx, {
+				nameAfterMove: update.name,
+				checkedSource,
+			});
 			return await this.skillService.save(skillId, update, user.id, ctx);
 		});
 		// `save` leaves the refresh to the caller when it joins a transaction.

packages/cli/test/integration/agents/agent-skills-hub.test.ts

@@ -37,7 +37,8 @@ import {
 import { getAgentSkillHash } from '@/modules/agents/utils/agent-config-hash';
 import { Telemetry } from '@/telemetry';
 
-import { createMember, createOwner } from '../shared/db/users';
+import { createCustomRoleWithScopeSlugs } from '../shared/db/roles';
+import { createMember, createOwner, createUser } from '../shared/db/users';
 
 const config: AgentJsonConfig = { name: 'Agent', model: '', instructions: 'Help.' };
 
@@ -396,6 +397,65 @@ describe('Agent skills in the skills hub', () => {
 		expect(Object.values(skills)).toEqual([body('Private')]);
 	});
 
+	it('refuses to move a skill out of a scope where the user cannot delete it', async () => {
+		// A custom role that may create and edit instance skills but delete none.
+		const role = await createCustomRoleWithScopeSlugs(
+			['skill:create', 'skill:read', 'skill:update'],
+			{
+				roleType: 'global',
+			},
+		);
+		const noDelete = await createUser({ role });
+		await linkUserToProject(noDelete, project, 'project:editor');
+		const instanceSkill = await createSkill(body('Company glossary'), { projectId: null });
+		const agent = await createAgent([instanceSkill]);
+
+		await expect(
+			agentSkillsService.updateSkill(
+				agent.id,
+				project.id,
+				instanceSkill,
+				{},
+				{ user: noDelete, modifiedBy: 'user' },
+				undefined,
+				undefined,
+				'agent',
+			),
+		).rejects.toThrow('You are not allowed to move this skill out of its scope.');
+		expect((await Container.get(SkillRepository).findByIds([instanceSkill]))[0]).toMatchObject({
+			projectId: null,
+			agentId: null,
+		});
+	});
+
+	it('refuses to move a project skill for a project role without delete', async () => {
+		const role = await createCustomRoleWithScopeSlugs(
+			['projectSkill:create', 'projectSkill:read', 'projectSkill:update'],
+			{ roleType: 'project' },
+		);
+		const noDelete = await createMember();
+		await linkUserToProject(noDelete, project, role.slug as never);
+		const projectSkill = await createSkill(body('Tone of voice'));
+		const agent = await createAgent([projectSkill]);
+
+		await expect(
+			agentSkillsService.updateSkill(
+				agent.id,
+				project.id,
+				projectSkill,
+				{},
+				{ user: noDelete, modifiedBy: 'user' },
+				undefined,
+				undefined,
+				'agent',
+			),
+		).rejects.toThrow('You are not allowed to move this skill out of its scope.');
+		expect((await Container.get(SkillRepository).findByIds([projectSkill]))[0]).toMatchObject({
+			projectId: project.id,
+			agentId: null,
+		});
+	});
+
 	it('trims the name of a skill the agent page creates, and refuses a blank one', async () => {
 		const agent = await createAgent([]);
 		const context = { user: editor, modifiedBy: 'user' as const };