From f51ec23cabda2e852f55709b638e6373871410cc Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 19 May 2026 16:07:22 -0500 Subject: [PATCH] Address Greptile authorization feedback --- .../agent-adapter-validation-routes.test.ts | 6 + .../agent-instructions-routes.test.ts | 6 + .../agent-permissions-routes.test.ts | 10 ++ .../src/__tests__/agent-skills-routes.test.ts | 6 + .../agent-test-environment-routes.test.ts | 6 + .../__tests__/authorization-service.test.ts | 25 ++++ ...ue-agent-mutation-ownership-routes.test.ts | 21 ++- ...access-authorization-host-services.test.ts | 16 +++ server/src/routes/agents.ts | 127 ++++-------------- server/src/routes/issues.ts | 98 +++----------- server/src/services/authorization.ts | 59 ++++---- server/src/services/plugin-host-services.ts | 31 ++++- 12 files changed, 196 insertions(+), 215 deletions(-) diff --git a/server/src/__tests__/agent-adapter-validation-routes.test.ts b/server/src/__tests__/agent-adapter-validation-routes.test.ts index b33fd21d017..198efd9cc2b 100644 --- a/server/src/__tests__/agent-adapter-validation-routes.test.ts +++ b/server/src/__tests__/agent-adapter-validation-routes.test.ts @@ -10,6 +10,7 @@ const mockAgentService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), ensureMembership: vi.fn(), setPrincipalPermission: vi.fn(), @@ -192,6 +193,11 @@ describe("agent routes adapter validation", () => { mockCompanySkillService.listRuntimeSkillEntries.mockResolvedValue([]); mockCompanySkillService.resolveRequestedSkillKeys.mockResolvedValue([]); mockAccessService.canUser.mockResolvedValue(true); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant", + }); mockAccessService.hasPermission.mockResolvedValue(true); mockAccessService.ensureMembership.mockResolvedValue(undefined); mockAccessService.setPrincipalPermission.mockResolvedValue(undefined); diff --git a/server/src/__tests__/agent-instructions-routes.test.ts b/server/src/__tests__/agent-instructions-routes.test.ts index c29fca24374..f35ef610605 100644 --- a/server/src/__tests__/agent-instructions-routes.test.ts +++ b/server/src/__tests__/agent-instructions-routes.test.ts @@ -21,6 +21,7 @@ const mockAgentInstructionsService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), })); @@ -175,6 +176,11 @@ describe("agent instructions bundle routes", () => { vi.clearAllMocks(); mockSyncInstructionsBundleConfigFromFilePath.mockImplementation((_agent, config) => config); mockFindServerAdapter.mockImplementation((_type: string) => ({ type: _type })); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant", + }); mockAgentService.getById.mockResolvedValue(makeAgent()); mockAgentService.update.mockImplementation(async (_id: string, patch: Record) => ({ ...makeAgent(), diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 2609322d50d..28a49a69e21 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -51,6 +51,7 @@ const mockAgentService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), getMembership: vi.fn(), ensureMembership: vi.fn(), @@ -302,6 +303,7 @@ describe.sequential("agent permission routes", () => { mockAgentService.getChainOfCommand.mockReset(); mockAgentService.resolveByReference.mockReset(); mockAccessService.canUser.mockReset(); + mockAccessService.decide.mockReset(); mockAccessService.hasPermission.mockReset(); mockAccessService.getMembership.mockReset(); mockAccessService.ensureMembership.mockReset(); @@ -342,6 +344,14 @@ describe.sequential("agent permission routes", () => { mockAgentService.update.mockResolvedValue(baseAgent); mockAgentService.updatePermissions.mockResolvedValue(baseAgent); mockAccessService.canUser.mockResolvedValue(true); + mockAccessService.decide.mockImplementation(async (input: { action?: string }) => { + const allowed = Boolean(await mockAccessService.canUser()); + return { + allowed, + reason: allowed ? "allow_explicit_grant" : "deny_missing_grant", + explanation: allowed ? "Allowed by test grant" : `Missing test grant for ${input.action ?? "action"}`, + }; + }); mockAccessService.hasPermission.mockResolvedValue(false); mockAccessService.getMembership.mockResolvedValue({ id: "membership-1", diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index 84537a9b107..1001f69409f 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -11,6 +11,7 @@ const mockAgentService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), getMembership: vi.fn(), listPrincipalGrants: vi.fn(), @@ -315,6 +316,11 @@ describe.sequential("agent skill routes", () => { ); mockLogActivity.mockResolvedValue(undefined); mockAccessService.canUser.mockResolvedValue(true); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant", + }); mockAccessService.hasPermission.mockResolvedValue(true); mockAccessService.getMembership.mockResolvedValue(null); mockAccessService.listPrincipalGrants.mockResolvedValue([]); diff --git a/server/src/__tests__/agent-test-environment-routes.test.ts b/server/src/__tests__/agent-test-environment-routes.test.ts index 3063fb70fa2..40730c051b1 100644 --- a/server/src/__tests__/agent-test-environment-routes.test.ts +++ b/server/src/__tests__/agent-test-environment-routes.test.ts @@ -10,6 +10,7 @@ const mockAgentService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), getMembership: vi.fn(async () => null), listPrincipalGrants: vi.fn(async () => []), @@ -120,6 +121,11 @@ describe("agent test-environment route", () => { beforeEach(async () => { vi.resetModules(); vi.clearAllMocks(); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant", + }); mockEnvironmentService.getById.mockResolvedValue({ id: "11111111-1111-4111-8111-111111111111", companyId: "company-1", diff --git a/server/src/__tests__/authorization-service.test.ts b/server/src/__tests__/authorization-service.test.ts index ab6a868275d..f19d669cf2f 100644 --- a/server/src/__tests__/authorization-service.test.ts +++ b/server/src/__tests__/authorization-service.test.ts @@ -330,6 +330,31 @@ describeEmbeddedPostgres("authorization service", () => { }); }); + it("denies legacy board assignment context for viewers", async () => { + const company = await createCompany(db, "BoardViewerAssignment"); + const userId = `user-${randomUUID()}`; + const targetAgent = await createAgent(db, company.id, { role: "engineer" }); + await db.insert(companyMemberships).values({ + companyId: company.id, + principalType: "user", + principalId: userId, + status: "active", + membershipRole: "viewer", + }); + + const decision = await authorizationService(db).decide({ + actor: { type: "board", userId, companyIds: [company.id], source: "session" }, + action: "tasks:assign", + resource: { type: "issue", companyId: company.id, assigneeAgentId: targetAgent.id }, + scope: { assigneeAgentId: targetAgent.id }, + }); + + expect(decision).toMatchObject({ + allowed: false, + reason: "deny_missing_grant", + }); + }); + it("denies simple-mode assignment to a target agent from another company", async () => { const sourceCompany = await createCompany(db, "AssignmentSource"); const targetCompany = await createCompany(db, "AssignmentTarget"); diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index f8f3777781f..7503bdf6379 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -30,6 +30,7 @@ const mockIssueService = vi.hoisted(() => ({ const mockAccessService = vi.hoisted(() => ({ canUser: vi.fn(), + decide: vi.fn(), hasPermission: vi.fn(), })); @@ -274,8 +275,14 @@ describe("agent issue mutation checkout ownership", () => { vi.doUnmock("../middleware/index.js"); registerRouteMocks(); vi.clearAllMocks(); - delete (mockAccessService as any).decide; mockAccessService.canUser.mockReset(); + mockAccessService.decide.mockReset(); + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "tasks:assign", + action: input.action, + reason: input.action === "tasks:assign" ? "allow_explicit_grant" : "deny_missing_grant", + explanation: input.action === "tasks:assign" ? "Allowed by test assignment default." : "Missing permission.", + })); mockAccessService.hasPermission.mockReset(); mockAgentService.getById.mockReset(); mockAgentService.list.mockReset(); @@ -683,12 +690,12 @@ describe("agent issue mutation checkout ownership", () => { }); it("allows agents with the active-checkout management grant to mutate active checkouts", async () => { - mockAccessService.hasPermission.mockImplementation(async ( - _companyId: string, - _principalType: string, - principalId: string, - permissionKey: string, - ) => principalId === peerAgentId && permissionKey === "tasks:manage_active_checkouts"); + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: input.action === "tasks:manage_active_checkouts", + action: input.action, + reason: input.action === "tasks:manage_active_checkouts" ? "allow_explicit_grant" : "deny_missing_grant", + explanation: input.action === "tasks:manage_active_checkouts" ? "Allowed by checkout management grant." : "Missing permission.", + })); const res = await request(await createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Managed update" }); diff --git a/server/src/__tests__/plugin-access-authorization-host-services.test.ts b/server/src/__tests__/plugin-access-authorization-host-services.test.ts index 0e1648965d1..c16434f947e 100644 --- a/server/src/__tests__/plugin-access-authorization-host-services.test.ts +++ b/server/src/__tests__/plugin-access-authorization-host-services.test.ts @@ -239,6 +239,22 @@ describeEmbeddedPostgres("plugin access and authorization host services", () => reason: "deny_policy_restricted", }); expect(explanation).toMatchObject(preview); + + const injectedBoardPreview = await services.authorization.previewAssignment({ + companyId: company.id, + actor: { + type: "board", + userId: "operator", + companyIds: [company.id], + source: "local_implicit", + isInstanceAdmin: true, + } as any, + target: { assigneeAgentId: targetAgent!.id }, + }); + expect(injectedBoardPreview).toMatchObject({ + allowed: false, + reason: "deny_policy_restricted", + }); services.dispose(); }); }); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index ce683cb9bde..07c2dd6de33 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -552,63 +552,32 @@ export function agentRoutes( async function assertCanCreateAgentsForCompany(req: Request, companyId: string) { assertCompanyAccess(req, companyId); - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: req.actor, - action: "agents:create", - resource: { type: "company", companyId }, - }); - if (!decision.allowed) { - throw forbidden(decision.explanation); - } - if (req.actor.type !== "agent") return null; - const actorAgent = req.actor.agentId ? await svc.getById(req.actor.agentId) : null; - if (!actorAgent || actorAgent.companyId !== companyId) { - throw forbidden("Agent key cannot access another company"); - } - return actorAgent; + const decision = await access.decide({ + actor: req.actor, + action: "agents:create", + resource: { type: "company", companyId }, + }); + if (!decision.allowed) { + throw forbidden(decision.explanation); } - - if (req.actor.type === "board") { - if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return null; - const allowed = await access.canUser(companyId, req.actor.userId, "agents:create"); - if (!allowed) { - throw forbidden("Missing permission: agents:create"); - } - return null; - } - if (!req.actor.agentId) throw forbidden("Agent authentication required"); - const actorAgent = await svc.getById(req.actor.agentId); + if (req.actor.type !== "agent") return null; + const actorAgent = req.actor.agentId ? await svc.getById(req.actor.agentId) : null; if (!actorAgent || actorAgent.companyId !== companyId) { throw forbidden("Agent key cannot access another company"); } - const allowedByGrant = await access.hasPermission(companyId, "agent", actorAgent.id, "agents:create"); - if (!allowedByGrant && !canCreateAgents(actorAgent)) { - throw forbidden("Missing permission: can create agents"); - } return actorAgent; } async function assertBoardCanManageAgentsForCompany(req: Request, companyId: string) { assertBoard(req); assertCompanyAccess(req, companyId); - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: req.actor, - action: "agents:create", - resource: { type: "company", companyId }, - }); - if (decision.allowed) return; - throw forbidden(decision.explanation); - } - - if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return; - const allowed = await access.canUser(companyId, req.actor.userId, "agents:create"); - if (!allowed) { - throw forbidden("Missing permission: agents:create"); - } + const decision = await access.decide({ + actor: req.actor, + action: "agents:create", + resource: { type: "company", companyId }, + }); + if (decision.allowed) return; + throw forbidden(decision.explanation); } async function assertCanReadConfigurations(req: Request, companyId: string) { @@ -630,25 +599,12 @@ export function agentRoutes( async function actorCanReadConfigurationsForCompany(req: Request, companyId: string) { assertCompanyAccess(req, companyId); - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: req.actor, - action: "agent_config:read", - resource: { type: "company", companyId }, - }); - return decision.allowed; - } - - if (req.actor.type === "board") { - if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return true; - return access.canUser(companyId, req.actor.userId, "agents:create"); - } - if (!req.actor.agentId) return false; - const actorAgent = await svc.getById(req.actor.agentId); - if (!actorAgent || actorAgent.companyId !== companyId) return false; - const allowedByGrant = await access.hasPermission(companyId, "agent", actorAgent.id, "agents:create"); - return allowedByGrant || canCreateAgents(actorAgent); + const decision = await access.decide({ + actor: req.actor, + action: "agent_config:read", + resource: { type: "company", companyId }, + }); + return decision.allowed; } async function buildSkippedWakeupResponse( @@ -720,38 +676,13 @@ export function agentRoutes( async function assertCanUpdateAgent(req: Request, targetAgent: { id: string; companyId: string }) { assertCompanyAccess(req, targetAgent.companyId); - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: req.actor, - action: "agent_config:update", - resource: { type: "agent", companyId: targetAgent.companyId, agentId: targetAgent.id }, - }); - if (decision.allowed) return; - throw forbidden(decision.explanation); - } - - if (req.actor.type === "board") { - await assertBoardCanManageAgentsForCompany(req, targetAgent.companyId); - return; - } - if (!req.actor.agentId) throw forbidden("Agent authentication required"); - - const actorAgent = await svc.getById(req.actor.agentId); - if (!actorAgent || actorAgent.companyId !== targetAgent.companyId) { - throw forbidden("Agent key cannot access another company"); - } - - if (actorAgent.id === targetAgent.id) return; - if (actorAgent.role === "ceo") return; - const allowedByGrant = await access.hasPermission( - targetAgent.companyId, - "agent", - actorAgent.id, - "agents:create", - ); - if (allowedByGrant || canCreateAgents(actorAgent)) return; - throw forbidden("Only CEO or agent creators can modify other agents"); + const decision = await access.decide({ + actor: req.actor, + action: "agent_config:update", + resource: { type: "agent", companyId: targetAgent.companyId, agentId: targetAgent.id }, + }); + if (decision.allowed) return; + throw forbidden(decision.explanation); } async function assertCanReadAgent(req: Request, targetAgent: { companyId: string }) { diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index ebcb20c9e76..3e2549b373e 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -1246,12 +1246,6 @@ export function issueRoutes( return (req.actor.companyIds ?? []).includes(companyId); } - function canCreateAgentsLegacy(agent: { permissions: Record | null | undefined; role: string }) { - if (agent.role === "ceo") return true; - if (!agent.permissions || typeof agent.permissions !== "object") return false; - return Boolean((agent.permissions as Record).canCreateAgents); - } - type TaskAssignmentAuthorizationScope = { issueId?: string | null; projectId?: string | null; @@ -1278,41 +1272,22 @@ export function issueRoutes( assignmentScope?: TaskAssignmentAuthorizationScope, ) { assertCompanyAccess(req, companyId); - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: req.actor, - action: "tasks:assign", - resource: { - type: "issue", - companyId, - issueId: assignmentScope?.issueId ?? null, - projectId: assignmentScope?.projectId ?? null, - parentIssueId: assignmentScope?.parentIssueId ?? null, - assigneeAgentId: assignmentScope?.assigneeAgentId ?? null, - assigneeUserId: assignmentScope?.assigneeUserId ?? null, - }, - scope: assignmentScope ?? null, - }); - if (decision.allowed) return; - throw forbidden(decision.explanation); - } - - if (req.actor.type === "board") { - if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return; - const allowed = await access.canUser(companyId, req.actor.userId, "tasks:assign"); - if (!allowed) throw forbidden("Missing permission: tasks:assign"); - return; - } - if (req.actor.type === "agent") { - if (!req.actor.agentId) throw forbidden("Agent authentication required"); - const allowedByGrant = await access.hasPermission(companyId, "agent", req.actor.agentId, "tasks:assign"); - if (allowedByGrant) return; - const actorAgent = await agentsSvc.getById(req.actor.agentId); - if (actorAgent && actorAgent.companyId === companyId && canCreateAgentsLegacy(actorAgent)) return; - throw forbidden("Missing permission: tasks:assign"); - } - throw unauthorized(); + const decision = await access.decide({ + actor: req.actor, + action: "tasks:assign", + resource: { + type: "issue", + companyId, + issueId: assignmentScope?.issueId ?? null, + projectId: assignmentScope?.projectId ?? null, + parentIssueId: assignmentScope?.parentIssueId ?? null, + assigneeAgentId: assignmentScope?.assigneeAgentId ?? null, + assigneeUserId: assignmentScope?.assigneeUserId ?? null, + }, + scope: assignmentScope ?? null, + }); + if (decision.allowed) return; + throw forbidden(decision.explanation); } function requireAgentRunId(req: Request, res: Response) { @@ -1328,41 +1303,12 @@ export function issueRoutes( companyId: string, assigneeAgentId: string, ) { - const decide = (access as typeof access & { decide?: typeof access.decide }).decide; - if (typeof decide === "function") { - const decision = await decide({ - actor: { type: "agent", agentId: actorAgentId, companyId }, - action: "tasks:manage_active_checkouts", - resource: { type: "issue", companyId, assigneeAgentId }, - }); - return decision.allowed; - } - - const allowedByGrant = await access.hasPermission( - companyId, - "agent", - actorAgentId, - "tasks:manage_active_checkouts", - ); - if (allowedByGrant) return true; - - const companyAgents = await agentsSvc.list(companyId); - const agentsById = new Map(companyAgents.map((agent) => [agent.id, agent])); - const actorAgent = agentsById.get(actorAgentId); - if (!actorAgent) return false; - if (canCreateAgentsLegacy(actorAgent)) return true; - - // Reporting-chain managers may intervene in an agent's active checkout - // without taking the task over. Peers must own the checkout/run first. - let cursor: string | null = assigneeAgentId; - for (let depth = 0; cursor && depth < 50; depth += 1) { - const assignee = agentsById.get(cursor); - if (!assignee) return false; - if (assignee.reportsTo === actorAgentId) return true; - cursor = assignee.reportsTo; - } - - return false; + const decision = await access.decide({ + actor: { type: "agent", agentId: actorAgentId, companyId }, + action: "tasks:manage_active_checkouts", + resource: { type: "issue", companyId, assigneeAgentId }, + }); + return decision.allowed; } async function assertAgentIssueMutationAllowed( diff --git a/server/src/services/authorization.ts b/server/src/services/authorization.ts index e3760146ea5..f0948ffc06e 100644 --- a/server/src/services/authorization.ts +++ b/server/src/services/authorization.ts @@ -152,6 +152,8 @@ type AssignmentPolicyEffect = | { kind: "requires_approval"; explanation: string } | { kind: "unknown"; explanation: string }; +type AgentHierarchyRow = { id: string; reportsTo: string | null }; + function evaluateAuthorizationPolicyForAssignment( policy: Record | null | undefined, label: string, @@ -222,15 +224,13 @@ function evaluateAuthorizationPolicyForAssignment( return { kind: "none" }; } -async function isAgentInSubtree(db: Db, companyId: string, rootAgentId: string, targetAgentId: string) { +function agentIsInSubtree( + agentsById: Map, + rootAgentId: string, + targetAgentId: string, +) { if (rootAgentId === targetAgentId) return true; - const rows = await db - .select({ id: agents.id, reportsTo: agents.reportsTo }) - .from(agents) - .where(eq(agents.companyId, companyId)); - const agentsById = new Map(rows.map((agent) => [agent.id, agent])); - let cursor: string | null = targetAgentId; for (let depth = 0; cursor && depth < 50; depth += 1) { const current = agentsById.get(cursor); @@ -241,6 +241,22 @@ async function isAgentInSubtree(db: Db, companyId: string, rootAgentId: string, return false; } +async function loadCompanyAgentHierarchy(db: Db, companyId: string) { + const rows = await db + .select({ id: agents.id, reportsTo: agents.reportsTo }) + .from(agents) + .where(eq(agents.companyId, companyId)); + return new Map(rows.map((agent) => [agent.id, agent])); +} + +async function isAgentInSubtree(db: Db, companyId: string, rootAgentId: string, targetAgentId: string) { + return agentIsInSubtree( + await loadCompanyAgentHierarchy(db, companyId), + rootAgentId, + targetAgentId, + ); +} + async function scopeAllows( db: Db, companyId: string, @@ -302,9 +318,10 @@ async function scopeAllows( if (subtreeRootAgentIds.length > 0) { constrained = true; if (!targetAssigneeAgentId) return false; + const agentsById = await loadCompanyAgentHierarchy(db, companyId); let matchesSubtree = false; for (const rootAgentId of subtreeRootAgentIds) { - if (await isAgentInSubtree(db, companyId, rootAgentId, targetAssigneeAgentId)) { + if (agentIsInSubtree(agentsById, rootAgentId, targetAssigneeAgentId)) { matchesSubtree = true; break; } @@ -530,20 +547,7 @@ export function authorizationService(db: Db) { } async function isManagerOf(companyId: string, managerAgentId: string, assigneeAgentId: string) { - const rows = await db - .select({ id: agents.id, reportsTo: agents.reportsTo }) - .from(agents) - .where(eq(agents.companyId, companyId)); - const agentsById = new Map(rows.map((agent) => [agent.id, agent])); - - let cursor: string | null = assigneeAgentId; - for (let depth = 0; cursor && depth < 50; depth += 1) { - const assignee = agentsById.get(cursor); - if (!assignee) return false; - if (assignee.reportsTo === managerAgentId) return true; - cursor = assignee.reportsTo; - } - return false; + return isAgentInSubtree(db, companyId, managerAgentId, assigneeAgentId); } async function decide(input: { @@ -611,6 +615,7 @@ export function authorizationService(db: Db) { } if (input.actor.type === "board") { + let taskAssignmentPolicyEffect: AssignmentPolicyEffect | null = null; if (input.actor.source === "local_implicit") { return allow({ action: input.action, @@ -641,6 +646,7 @@ export function authorizationService(db: Db) { }); } const policyEffect = await assignmentPolicyEffect(input.resource); + taskAssignmentPolicyEffect = policyEffect; const policyDeny = await denyForAssignmentPolicyIfNeeded(policyEffect); if (policyDeny) return policyDeny; const membership = await getActiveMembership(companyId, "user", input.actor.userId); @@ -651,13 +657,6 @@ export function authorizationService(db: Db) { explanation: "Allowed by simple mode company-wide task assignment default.", }); } - if (policyEffect.kind === "none" && !input.actor.memberships && input.actor.companyIds?.includes(companyId)) { - return allow({ - action: input.action, - reason: "allow_simple_company_member", - explanation: "Allowed by legacy company membership context.", - }); - } } if (!permissionKey) { return deny({ @@ -669,7 +668,7 @@ export function authorizationService(db: Db) { if (input.action === "tasks:assign") { const grantDecision = await decideWithTaskAssignmentGrants("user", input.actor.userId); if (grantDecision.allowed) return grantDecision; - const policyEffect = await assignmentPolicyEffect(input.resource); + const policyEffect = taskAssignmentPolicyEffect ?? await assignmentPolicyEffect(input.resource); if (policyEffect.kind === "restricted") return denyRestrictedAssignmentPolicy(policyEffect); return grantDecision; } diff --git a/server/src/services/plugin-host-services.ts b/server/src/services/plugin-host-services.ts index 22ccac212e6..b3f8eaaea64 100644 --- a/server/src/services/plugin-host-services.ts +++ b/server/src/services/plugin-host-services.ts @@ -73,7 +73,7 @@ import { isIP } from "node:net"; import { logger } from "../middleware/logger.js"; import { getTelemetryClient } from "../telemetry.js"; import { accessService } from "./access.js"; -import { authorizationService } from "./authorization.js"; +import { authorizationService, type AuthorizationActor } from "./authorization.js"; import { sanitizeRecord } from "../redaction.js"; // --------------------------------------------------------------------------- @@ -955,6 +955,29 @@ export function buildHostServices( }; }; + const pluginAssignmentActor = (actor: { + type: "agent" | "board"; + agentId?: string | null; + companyId?: string | null; + userId?: string | null; + companyIds?: string[]; + }): AuthorizationActor => { + if (actor.type === "agent") { + return { + type: "agent", + agentId: actor.agentId ?? null, + companyId: actor.companyId ?? null, + source: "agent_key", + }; + } + return { + type: "board", + userId: actor.userId ?? null, + companyIds: Array.isArray(actor.companyIds) ? actor.companyIds : [], + source: "session", + }; + }; + const policyPathForResource = (resourceType: "company" | "agent" | "project" | "issue") => { switch (resourceType) { case "agent": @@ -2201,7 +2224,7 @@ export function buildHostServices( async updateMember(params) { const companyId = ensureCompanyId(params.companyId); await ensurePluginAvailableForCompany(companyId); - const updated = await access.updateMember(companyId, params.memberId, params.patch as any); + const updated = await access.updateMember(companyId, params.memberId, params.patch); if (!updated) throw new Error("Member not found"); await logPluginActivity({ companyId, @@ -2439,7 +2462,7 @@ export function buildHostServices( const companyId = ensureCompanyId(params.companyId); await ensurePluginAvailableForCompany(companyId); return authorization.decide({ - actor: params.actor as any, + actor: pluginAssignmentActor(params.actor), action: "tasks:assign", resource: { type: "issue", companyId, ...params.target }, scope: { @@ -2455,7 +2478,7 @@ export function buildHostServices( const companyId = ensureCompanyId(params.companyId); await ensurePluginAvailableForCompany(companyId); return authorization.decide({ - actor: params.actor as any, + actor: pluginAssignmentActor(params.actor), action: "tasks:assign", resource: { type: "issue", companyId, ...params.target }, scope: {