diff --git a/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts index b6a414bab86..de45ffeb71a 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-skill-refs.service.test.ts +++ b/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' }, ); }); }); diff --git a/packages/cli/src/modules/agents/__tests__/agent-skills.service.test.ts b/packages/cli/src/modules/agents/__tests__/agent-skills.service.test.ts index 2d8f213fc9f..cad6059b556 100644 --- a/packages/cli/src/modules/agents/__tests__/agent-skills.service.test.ts +++ b/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(); }); diff --git a/packages/cli/src/modules/agents/__tests__/skill.service.test.ts b/packages/cli/src/modules/agents/__tests__/skill.service.test.ts index 7bd46e3d09a..44577414ede 100644 --- a/packages/cli/src/modules/agents/__tests__/skill.service.test.ts +++ b/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({ 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({ 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([]); diff --git a/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts b/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts index f81724c9bbd..be78e9699fc 100644 --- a/packages/cli/src/modules/agents/__tests__/skills-api.service.test.ts +++ b/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', diff --git a/packages/cli/src/modules/agents/agent-skill-refs.service.ts b/packages/cli/src/modules/agents/agent-skill-refs.service.ts index 0705ffd6de9..1825973756d 100644 --- a/packages/cli/src/modules/agents/agent-skill-refs.service.ts +++ b/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 { await this.skillService.changeTarget( skillId, targetFor(agent, availableTo), agent.id, ctx, - nameAfterMove, + options, ); } diff --git a/packages/cli/src/modules/agents/agent-skills.service.ts b/packages/cli/src/modules/agents/agent-skills.service.ts index dd7ad11ff3f..b27df0170a3 100644 --- a/packages/cli/src/modules/agents/agent-skills.service.ts +++ b/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); diff --git a/packages/cli/src/modules/agents/skills/skill.service.ts b/packages/cli/src/modules/agents/skills/skill.service.ts index 70f38bd6e60..3c1257ccd64 100644 --- a/packages/cli/src/modules/agents/skills/skill.service.ts +++ b/packages/cli/src/modules/agents/skills/skill.service.ts @@ -98,6 +98,13 @@ function toRecord(rows: Map): Record { 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 { + 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 { const skills = await this.skillRepository.findByIds(skillIds, ctx); const names = await this.latestNames( diff --git a/packages/cli/src/modules/agents/skills/skills-api.service.ts b/packages/cli/src/modules/agents/skills/skills-api.service.ts index 784f2f9db3c..6522ab12fc5 100644 --- a/packages/cli/src/modules/agents/skills/skills-api.service.ts +++ b/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. diff --git a/packages/cli/test/integration/agents/agent-skills-hub.test.ts b/packages/cli/test/integration/agents/agent-skills-hub.test.ts index 502ec7c0b33..7dce2405cb6 100644 --- a/packages/cli/test/integration/agents/agent-skills-hub.test.ts +++ b/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 };