diff --git a/.changeset/account-scoped-tool-policies.md b/.changeset/account-scoped-tool-policies.md new file mode 100644 index 0000000000..a258479d0a --- /dev/null +++ b/.changeset/account-scoped-tool-policies.md @@ -0,0 +1,5 @@ +--- +"@executor-js/react": patch +--- + +Tool policies set from an account section of the integration Tools tab now apply to that connection only, and each account header gets a menu to set a policy for the whole connection. Members no longer see policy controls they cannot use, and a refused policy write shows the server's reason. diff --git a/e2e/scenarios/policies-ui.test.ts b/e2e/scenarios/policies-ui.test.ts index ad45864429..00509252f5 100644 --- a/e2e/scenarios/policies-ui.test.ts +++ b/e2e/scenarios/policies-ui.test.ts @@ -4,18 +4,23 @@ // the category (group) row menu writes a subtree rule. The product promises // under test: // -// 1. Both menus surface the REAL stored pattern (connection-wildcarded -// `integration.*.*.tool`) before anything is written. +// 1. Both menus surface the REAL stored pattern (pinned to the account the +// row sits under, `integration...tool`) before +// anything is written. // 2. A leaf rule and a category rule coexist: the more specific leaf rule // keeps precedence over the later category rule, which covers the rest // of its group. -// 3. Rules are connection-agnostic: set from one account's section, they -// govern the other account's rows too, and the menu there shows the -// active rule with a Clear option. -// 4. The tool detail header's policy badge is the same authoring surface: +// 3. Rules are account-scoped: set from one account's section, they leave +// the other account's rows untouched. Two connections of one +// integration are different credentials (a bot token and a user token), +// so blocking a tool on one must not block it on the other. +// 4. The account header has its own menu that rules the whole connection +// (`integration...*`), recognizes its rule, and +// clears it. +// 5. The tool detail header's policy badge is the same authoring surface: // it writes the same stored pattern, recognizes its own rule afterward // (the Clear affordance), and Clear really removes the rule. -// 5. The rules materialize as manageable rows on /policies and persist +// 6. The rules materialize as manageable rows on /policies and persist // server-side with exactly the owner/pattern/action the UI promised. import { randomBytes } from "node:crypto"; @@ -83,11 +88,12 @@ scenario( const beta = ConnectionName.make(`beta${suffix}`); const accounts = [alpha, beta] as const; - // The UI hides owner/connection segments; a rule authored on a node is - // stored connection-wildcarded so it spans every account. - const leafPattern = `${integration}.*.*.records.create`; - const categoryPattern = `${integration}.*.*.records.*`; - const listLeafPattern = `${integration}.*.*.records.list`; + // The UI hides owner/connection segments in the row labels, but a rule + // authored under an account section is stored pinned to that account. + const leafPattern = `${integration}.org.${alpha}.records.create`; + const categoryPattern = `${integration}.org.${alpha}.records.*`; + const listLeafPattern = `${integration}.org.${alpha}.records.list`; + const betaAccountPattern = `${integration}.org.${beta}.*`; // Selfhost scenarios share one workspace — remove everything this one // made (policies, connections, the integration) even on failure. @@ -159,6 +165,14 @@ scenario( .getByRole("button") .filter({ hasText: leaf }) .getByLabel(label, { exact: true }); + // Wait until a leaf's indicator with this label is gone (after a clear). + const expectNoIndicator = async (connection: string, leaf: string, label: string) => { + await expect + .poll(() => leafIndicator(connection, leaf, label).count(), { + message: `${connection} ${leaf} still shows "${label}"`, + }) + .toBe(0); + }; const internalError = JSON.stringify({ _tag: "InternalError", traceId: "policy-write" }); await step("Open the integration's Tools tab", async () => { @@ -254,25 +268,58 @@ scenario( }, ); - await step("The same rules govern the second account's rows", async () => { + await step("The second account's rows are untouched", async () => { await closedGroup(beta, integration).click(); await closedGroup(beta, "records").click(); - await leafIndicator(beta, "create", `Blocked (matched ${leafPattern})`).waitFor(); - await leafIndicator( - beta, - "list", - `Require approval (matched ${categoryPattern})`, - ).waitFor(); + await sectionFor(beta).getByRole("button").filter({ hasText: "create" }).waitFor(); + expect( + await leafIndicator(beta, "create", `Blocked (matched ${leafPattern})`).count(), + "a rule set under one account does not block the same tool on another", + ).toBe(0); + expect( + await leafIndicator( + beta, + "list", + `Require approval (matched ${categoryPattern})`, + ).count(), + "a category rule set under one account does not reach another account", + ).toBe(0); }); await step("Reopening the menu offers to clear the active rule", async () => { - await policyMenuFor(beta, `${integration}.records.create`).click(); + await policyMenuFor(alpha, `${integration}.records.create`).click(); await page.getByRole("menuitem", { name: "Clear" }).waitFor(); await page.keyboard.press("Escape"); }); + await step("The account header blocks the whole second connection", async () => { + const headerMenu = sectionFor(beta).getByRole("button", { + name: `Set policy for ${integration} / ${beta}`, + exact: true, + }); + await headerMenu.click(); + // The header menu is headed by the whole-account pattern it will store. + await page.getByText(betaAccountPattern, { exact: true }).waitFor(); + await page.getByRole("menuitem", { name: "Block" }).click(); + await leafIndicator(beta, "create", `Blocked (matched ${betaAccountPattern})`).waitFor(); + await leafIndicator(beta, "list", `Blocked (matched ${betaAccountPattern})`).waitFor(); + // The first account is not affected by the second account's rule. + await leafIndicator(alpha, "create", `Blocked (matched ${leafPattern})`).waitFor(); + }); + + await step("The account header recognizes its rule and Clear removes it", async () => { + const headerMenu = sectionFor(beta).getByRole("button", { + name: `Set policy for ${integration} / ${beta}`, + exact: true, + }); + await headerMenu.click(); + await page.getByRole("menuitem", { name: "Clear" }).click(); + await sectionFor(beta).getByRole("button").filter({ hasText: "create" }).waitFor(); + await expectNoIndicator(beta, "create", `Blocked (matched ${betaAccountPattern})`); + }); + await step("Open the tool detail for records.list", async () => { - await sectionFor(beta).getByRole("button").filter({ hasText: "list" }).click(); + await sectionFor(alpha).getByRole("button").filter({ hasText: "list" }).click(); // The header badge reflects the inherited category rule. await page.getByRole("button", { name: `Matched policy: ${categoryPattern}` }).waitFor(); }); diff --git a/e2e/src/integration-creation-permissions.ts b/e2e/src/integration-creation-permissions.ts index c54309a569..cce298ac9f 100644 --- a/e2e/src/integration-creation-permissions.ts +++ b/e2e/src/integration-creation-permissions.ts @@ -108,6 +108,18 @@ export const integrationCreationPermissions = (admin: Identity, member: Identity expect(await action.isDisabled()).toBe(true); } }); + await step("Member sees tool policies without a way to change them", async () => { + // Policies on the Tools page are workspace rules the server refuses + // for members, so the row menus and the detail badge menu stay off. + await visit(page, "/tools"); + await page.getByRole("button").filter({ hasText: "executor" }).first().waitFor(); + expect( + await page.getByRole("button", { name: /^Set policy/ }).count(), + "members get no policy menus on tool rows", + ).toBe(0); + await visit(page, `/integrations/${slug}`); + await page.getByRole("button", { name: "Add connection", exact: true }).waitFor(); + }); await step("Member can still add a personal connection", async () => { await page.getByRole("button", { name: "Add connection", exact: true }).click(); const dialog = page.getByRole("dialog"); diff --git a/packages/react/src/api/error-reporting.test.ts b/packages/react/src/api/error-reporting.test.ts index 655f792ba7..c1c1d09e97 100644 --- a/packages/react/src/api/error-reporting.test.ts +++ b/packages/react/src/api/error-reporting.test.ts @@ -53,6 +53,20 @@ describe("frontend error reporting", () => { expect(messageFromExit(Exit.fail({ reason: "unknown" }), "Fallback")).toBe("Fallback"); }); + it("reads a message the error exposes as a prototype getter", () => { + // Schema-tagged API errors (e.g. OrgWriteDeniedError) declare no `message` + // field; the sentence lives on a class getter, which a struct decode misses. + class GetterError extends Data.TaggedError("GetterError")<{}> { + override get message(): string { + return "Requires a workspace admin."; + } + } + const exit = Exit.fail(new GetterError()); + + expect(messageFromExit(exit, "Fallback")).toBe("Requires a workspace admin."); + expect(messageFromUnknown(new GetterError(), "Fallback")).toBe("Requires a workspace admin."); + }); + it("reports failed exits with the provided context", () => { const exit = Exit.fail({ message: "Could not update integration" }); const { calls, report } = captureReports(); diff --git a/packages/react/src/api/error-reporting.tsx b/packages/react/src/api/error-reporting.tsx index 8a9eab0d03..2c19b8ce39 100644 --- a/packages/react/src/api/error-reporting.tsx +++ b/packages/react/src/api/error-reporting.tsx @@ -21,8 +21,17 @@ class FrontendHandledError extends Data.TaggedError("FrontendHandledError")<{ readonly context: FrontendErrorContext; }> {} -const ErrorMessage = Schema.Struct({ message: Schema.String }); -const decodeErrorMessage = Schema.decodeUnknownOption(ErrorMessage); +// Effect's tagged error classes (`Schema.TaggedErrorClass`) often declare no +// `message` field and expose it as a prototype getter instead, so a struct +// decode — which only sees own properties — would miss the very sentence the +// server wrote for the user. Read the property directly. +const decodeErrorMessage = (value: unknown): Option.Option<{ readonly message: string }> => { + if (typeof value !== "object" || value === null) return Option.none(); + const message: unknown = Reflect.get(value, "message"); + return typeof message === "string" && message.length > 0 + ? Option.some({ message }) + : Option.none(); +}; const TaggedValue = Schema.Struct({ _tag: Schema.String }); const decodeTaggedValue = Schema.decodeUnknownOption(TaggedValue); diff --git a/packages/react/src/components/tool-tree.tsx b/packages/react/src/components/tool-tree.tsx index f0fa20e6f2..d3b8c94908 100644 --- a/packages/react/src/components/tool-tree.tsx +++ b/packages/react/src/components/tool-tree.tsx @@ -3,7 +3,7 @@ import { ChevronRightIcon, MoreHorizontalIcon, SearchIcon, XIcon } from "lucide- import type { EffectivePolicy, Owner, ToolPolicyAction } from "@executor-js/sdk/shared"; import { ownerLabel, useOwnerDisplay } from "../api/owner-display"; import { trackEvent } from "../api/analytics"; -import { toPolicyPattern } from "../lib/policy-pattern"; +import { accountPolicyPattern, toPolicyPattern } from "../lib/policy-pattern"; import { Badge } from "./badge"; import { Button } from "./button"; import { Input } from "./input"; @@ -297,7 +297,9 @@ export function ToolTree(props: { * emit the tool's full dotted id; group rows emit `prefix.*`. */ onSetPolicy?: (pattern: string, action: ToolPolicyAction) => void; onClearPolicy?: (pattern: string) => void; - /** Maps the displayed row path into the persisted policy pattern. */ + /** Maps the displayed row path into the persisted policy pattern for the + * flat tree. Ignored in the account-grouped view, where every row writes a + * pattern pinned to its own account (`integration...`). */ patternForDisplay?: (displayPattern: string) => string; /** Sorted user-authored policies (most-precedent first). Used to * decide whether a node has its own exact-pattern user rule today @@ -429,26 +431,55 @@ export function ToolTree(props: { : (props.emptyLabel ?? "No tools available")} ) : groupByConnection ? ( - accountGroups.map((group) => ( -
-
- {ownerDisplay.showOwnerLabels ? ( - - {ownerLabel(group.owner)} - - ) : null} - - {group.integration && group.connection - ? `${group.integration} / ${group.connection}` - : group.connection || ownerDisplay.label(group.owner)} - - - {group.tools.length} - -
- -
- )) + accountGroups.map((group) => { + // Rows inside an account section author rules for THAT account + // only: two connections of one integration are different + // credentials, so a rule set under one must not govern the other. + const accountPattern = accountPolicyPattern(group.owner, group.connection); + const wholeAccountPattern = accountPattern(`${group.integration}.*`); + const wholeAccountRule = exactPatterns.get(wholeAccountPattern); + const accountLabel = + group.integration && group.connection + ? `${group.integration} / ${group.connection}` + : group.connection || ownerDisplay.label(group.owner); + return ( +
+
+ {ownerDisplay.showOwnerLabels ? ( + + {ownerLabel(group.owner)} + + ) : null} + + {accountLabel} + + + {group.tools.length} + + {onSetPolicy && group.integration && group.connection ? ( + + ) : null} +
+ +
+ ); + }) ) : ( )} diff --git a/packages/react/src/lib/integration-add.tsx b/packages/react/src/lib/integration-add.tsx index 1b1ddbac96..968bc0f3f9 100644 --- a/packages/react/src/lib/integration-add.tsx +++ b/packages/react/src/lib/integration-add.tsx @@ -11,20 +11,14 @@ import { Link } from "@tanstack/react-router"; import * as Exit from "effect/Exit"; import * as Option from "effect/Option"; import * as Predicate from "effect/Predicate"; -import * as Schema from "effect/Schema"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; import { integrationsOptimisticAtom } from "../api/atoms"; - -const ErrorMessage = Schema.Struct({ message: Schema.String }); -const decodeErrorMessage = Schema.decodeUnknownOption(ErrorMessage); +import { messageFromExit } from "../api/error-reporting"; /** The failed Exit's `message`, or `fallback` when the error carries none. */ -export const errorMessageFromExit = (exit: Exit.Exit, fallback: string): string => - Option.match(Option.flatMap(Exit.findErrorOption(exit), decodeErrorMessage), { - onNone: () => fallback, - onSome: ({ message }) => message, - }); +export const errorMessageFromExit: (exit: Exit.Exit, fallback: string) => string = + messageFromExit; export const isIntegrationAlreadyExistsExit = (exit: Exit.Exit): boolean => Option.match(Exit.findErrorOption(exit), { diff --git a/packages/react/src/lib/policy-pattern.test.ts b/packages/react/src/lib/policy-pattern.test.ts new file mode 100644 index 0000000000..0c3604eb0a --- /dev/null +++ b/packages/react/src/lib/policy-pattern.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { accountPolicyPattern, toPolicyPattern } from "./policy-pattern"; + +describe("policy pattern bridges", () => { + it("wildcards owner and connection for the connection-agnostic tree", () => { + expect(toPolicyPattern("slack.conversations.history")).toBe("slack.*.*.conversations.history"); + expect(toPolicyPattern("slack.conversations.*")).toBe("slack.*.*.conversations.*"); + expect(toPolicyPattern("slack.*")).toBe("slack.*"); + expect(toPolicyPattern("*")).toBe("*"); + }); + + it("pins owner and connection for a row inside an account section", () => { + const forBot = accountPolicyPattern("org", "bot"); + expect(forBot("slack.conversations.history")).toBe("slack.org.bot.conversations.history"); + expect(forBot("slack.conversations.*")).toBe("slack.org.bot.conversations.*"); + expect(forBot("slack.*")).toBe("slack.org.bot.*"); + expect(forBot("slack")).toBe("slack.org.bot.*"); + expect(forBot("*")).toBe("*"); + }); +}); diff --git a/packages/react/src/lib/policy-pattern.ts b/packages/react/src/lib/policy-pattern.ts index 7a274f4296..c2727f38b4 100644 --- a/packages/react/src/lib/policy-pattern.ts +++ b/packages/react/src/lib/policy-pattern.ts @@ -1,4 +1,4 @@ -import { matchPattern } from "@executor-js/sdk/shared"; +import { matchPattern, type Owner } from "@executor-js/sdk/shared"; // --------------------------------------------------------------------------- // Policy pattern bridge. @@ -29,3 +29,29 @@ export const toPolicyPattern = (displayPattern: string): string => { }; export { matchPattern }; + +// --------------------------------------------------------------------------- +// Account-scoped bridge. +// +// The account-grouped Tools tab shows the same tool once per connection. A +// rule authored from a row inside one account's section must govern THAT +// account only — a Slack bot connection and a Slack user connection are +// different credentials with different capabilities, and blocking a tool on +// one must not silently block it on the other. So instead of wildcarding the +// owner + connection segments, fill them in: `integration...`. +// `integration.*` becomes the whole-account subtree +// `integration...*`. Apply the returned mapper at both the site +// that BUILDS a pattern and the site that LOOKS UP the exact rule, exactly as +// with `toPolicyPattern`. +// --------------------------------------------------------------------------- + +export const accountPolicyPattern = + (owner: Owner, connection: string) => + (displayPattern: string): string => { + if (displayPattern === "*") return "*"; + const firstDot = displayPattern.indexOf("."); + if (firstDot === -1) return `${displayPattern}.${owner}.${connection}.*`; + const integration = displayPattern.slice(0, firstDot); + const rest = displayPattern.slice(firstDot + 1); + return `${integration}.${owner}.${connection}.${rest}`; + }; diff --git a/packages/react/src/pages/integration-detail.tsx b/packages/react/src/pages/integration-detail.tsx index f88412b3e3..a30d8da9d6 100644 --- a/packages/react/src/pages/integration-detail.tsx +++ b/packages/react/src/pages/integration-detail.tsx @@ -13,6 +13,7 @@ import { effectivePolicyFromSorted, type Connection, type Owner, + type ToolPolicyAction, } from "@executor-js/sdk/shared"; import { checkConnectionHealth, @@ -46,6 +47,7 @@ import { useExecutorDocumentTitle } from "../lib/document-title"; import { ErrorState } from "../components/error-state"; import { isAsyncResultLoading } from "../lib/async-result"; import { useConnectionsHealth } from "../lib/use-connection-health"; +import { accountPolicyPattern } from "../lib/policy-pattern"; import { integrationDetailInternalTabFromSearch, type IntegrationDetailInternalTab, @@ -142,6 +144,16 @@ export function IntegrationDetailPage(props: { // Integrations are workspace-owned; the server refuses catalog mutations // (update/remove) from non-admin members, so disable the controls for them. const canMutateIntegration = useCanCreateWorkspaceConnections(); + // Tool policies on this tab are workspace rules (`usePolicyActions("org")`), + // which the server refuses for non-admin members. Offer the menus only to + // those who can actually write them. + const canSetPolicy = canMutateIntegration; + const onSetPolicy = canSetPolicy + ? (pattern: string, action: ToolPolicyAction) => void policyActions.set(pattern, action) + : undefined; + const onClearPolicy = canSetPolicy + ? (pattern: string, policyId?: string) => void policyActions.clear(pattern, policyId) + : undefined; const canEdit = !isBuiltInIntegration && integrationData !== null; const canRefresh = integrationData?.canRefresh ?? false; const canRemove = integrationData?.canRemove ?? false; @@ -301,6 +313,7 @@ export function IntegrationDetailPage(props: { policy: effectivePolicyFromSorted(matchId, policyList, t.requiresApproval), owner: t.owner, connection: t.connection, + integration: t.integration, }; }); }, [tools, policyList]); @@ -620,8 +633,8 @@ export function IntegrationDetailPage(props: { tools={integrationTools} selectedToolId={selectedToolId} onSelect={setSelectedToolId} - onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern) => void policyActions.clear(pattern)} + onSetPolicy={onSetPolicy} + onClearPolicy={onClearPolicy} policies={sortedPolicies} groupByConnection={!isBuiltInIntegration} emptyLabel={hasToolSyncIssue ? emptyToolsTitle : undefined} @@ -636,9 +649,15 @@ export function IntegrationDetailPage(props: { toolName={selectedTool.name} staticTool={selection?.static} policy={selectedTool.policy} - onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern, policyId) => - void policyActions.clear(pattern, policyId) + onSetPolicy={onSetPolicy} + onClearPolicy={onClearPolicy} + // The header badge must write and look up the SAME + // account-pinned pattern the tree row under this + // account uses, or it cannot recognize its own rule. + patternForDisplay={ + selection && !selection.static + ? accountPolicyPattern(selection.owner, selection.connection) + : undefined } {...(!selection?.static && selectedBareName ? { diff --git a/packages/react/src/pages/tools.tsx b/packages/react/src/pages/tools.tsx index 60e6e0fa9a..cdb05d3e38 100644 --- a/packages/react/src/pages/tools.tsx +++ b/packages/react/src/pages/tools.tsx @@ -2,7 +2,11 @@ import { useMemo, useState } from "react"; import { Link } from "@tanstack/react-router"; import { useAtomRefresh, useAtomValue } from "@effect/atom-react"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; -import { ToolAddress, effectivePolicyFromSorted } from "@executor-js/sdk/shared"; +import { + ToolAddress, + effectivePolicyFromSorted, + type ToolPolicyAction, +} from "@executor-js/sdk/shared"; import { policiesOptimisticAtom, toolsAllAtom } from "../api/atoms"; import { usePolicyActions } from "../hooks/use-policy-actions"; @@ -13,6 +17,7 @@ import { Skeleton } from "../components/skeleton"; import { useExecutorDocumentTitle } from "../lib/document-title"; import { ErrorState } from "../components/error-state"; import { isAsyncResultLoading } from "../lib/async-result"; +import { useCanCreateWorkspaceConnections } from "../multiplayer/use-admin-nav"; // Dynamic tool policy patterns are derived from the connection-aware address. // Static tools (for example Executor's own tools) use their address directly. @@ -38,6 +43,15 @@ export function ToolsPage() { const refreshTools = useAtomRefresh(toolsAllAtom); const policies = useAtomValue(policiesOptimisticAtom); const policyActions = usePolicyActions("org"); + // Policies here are workspace rules, which the server refuses for non-admin + // members. Offer the menus only to those who can actually write them. + const canSetPolicy = useCanCreateWorkspaceConnections(); + const onSetPolicy = canSetPolicy + ? (pattern: string, action: ToolPolicyAction) => void policyActions.set(pattern, action) + : undefined; + const onClearPolicy = canSetPolicy + ? (pattern: string, policyId?: string) => void policyActions.clear(pattern, policyId) + : undefined; const [selectedToolId, setSelectedToolId] = useState(null); @@ -144,8 +158,8 @@ export function ToolsPage() { tools={summaries} selectedToolId={selectedToolId} onSelect={setSelectedToolId} - onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern) => void policyActions.clear(pattern)} + onSetPolicy={onSetPolicy} + onClearPolicy={onClearPolicy} policies={sortedPolicies} /> @@ -156,10 +170,8 @@ export function ToolsPage() { toolName={selectedTool.name} staticTool={selection?.static} policy={selectedTool.policy} - onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern, policyId) => - void policyActions.clear(pattern, policyId) - } + onSetPolicy={onSetPolicy} + onClearPolicy={onClearPolicy} /> ) : ( 0} />