mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-05 13:29:44 +02:00
fix(agents): agent access owner-only editable, read-only for others (MUL-3963) (#4853)
* fix(agents): make agent access owner-only editable, read-only for others (MUL-3963) Interaction bug: a non-owner (incl. workspace admin) could open the AccessPicker and set an agent public — the backend silently ignored it and the UI bounced back to private. Access is owner-only, so non-owners must see a read-only state and the backend must reject real changes explicitly. Frontend: - AccessPicker renders a static, non-interactive read-only state when the viewer is not the owner: the current access value + a lock affordance + a tooltip "Only the agent owner can change who can run this agent." No clickable trigger is rendered, so a non-owner can never open a control the backend would reject (the GitHub/Notion pattern for permission settings you can see but not edit). The editable multi-select picker is unchanged for the owner. - agent-detail-inspector gates the picker on ownership specifically (currentUserId === agent.owner_id), NOT the general canEdit (which also admits admins, who may edit other fields but not access). - New locale key access.owner_only_readonly (en/zh-Hans/ja/ko). Backend: - UpdateAgent now returns an explicit 403 when a non-owner submits a REAL permission change (permissionInputChangesAgent compares requested mode + target set against the persisted state); a no-op resubmit (admin PATCH-as-PUT echoing unchanged permission) is still tolerated so admin edits of other fields keep working. Replaces the previous silent-drop that caused the bounce. Tests: - access-picker.test.tsx: non-owner gets a non-interactive read-only display with the owner-only tooltip; owner gets an interactive picker; owner can pick a member and stack workspace + member. - TestUpdateAgent_AccessChangeIsOwnerOnly: admin real change → 403; admin no-op resubmit → 200; admin editing other fields → 200; owner change → 200. Incidental: fixed a pre-existing base typecheck break in slash-command-suggestion.test.tsx (stray `signal` arg not in the suggestion items type) that otherwise fails the whole @multica/views typecheck. Refs MUL-3963. Co-authored-by: multica-agent <github@multica.ai> * fix(agents): compare legacy visibility, not expanded permission, for no-op detection (MUL-3963) PR #4853 review: permissionInputChangesAgent expanded a legacy-only visibility:"private" into a real private permission and compared it against the agent's actual permission. A member-only public_to agent derives legacy visibility "private", so an admin PATCH-as-PUT echoing visibility:"private" while editing another field was misread as a public_to→private downgrade and rejected with 403 — contradicting the "unchanged permission no-op is allowed" contract. Fix (per review): when a request carries ONLY legacy `visibility` (no permission_mode / invocation_targets), derive the agent's CURRENT legacy visibility from its real targets and compare the legacy string values. Equal = no-op (allowed); a real legacy change (e.g. "workspace") still returns 403. Requests that carry permission_mode / invocation_targets keep the precise mode+target comparison. Regression test TestUpdateAgent_LegacyVisibilityNoOpForMemberOnlyPublicTo: member-only public_to agent — admin submitting visibility:"private" + a non-permission field → 200 with targets unchanged; admin submitting visibility:"workspace" → 403. Go handler/composio suites green; migration 130 applied; go vet clean. Refs MUL-3963. Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: multica-agent <github@multica.ai>
This commit is contained in:
@@ -156,7 +156,13 @@ export function AgentDetailInspector({
|
||||
invocationTargets={agent.invocation_targets}
|
||||
visibility={agent.visibility}
|
||||
members={members}
|
||||
canEdit={canEdit}
|
||||
// Access is OWNER-ONLY (MUL-3963): a workspace admin can edit other
|
||||
// agent properties (canEdit) but NOT who may run the agent. Gate the
|
||||
// picker on ownership specifically so non-owners get the read-only
|
||||
// state instead of a control the backend would reject with 403.
|
||||
canEdit={
|
||||
currentUserId !== null && agent.owner_id === currentUserId
|
||||
}
|
||||
hasComposioAllowlist={
|
||||
(agent.composio_toolkit_allowlist ?? []).length > 0
|
||||
}
|
||||
|
||||
@@ -0,0 +1,119 @@
|
||||
// @vitest-environment jsdom
|
||||
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
|
||||
import type {
|
||||
AgentInvocationTarget,
|
||||
MemberWithUser,
|
||||
} from "@multica/core/types";
|
||||
import { I18nProvider } from "@multica/core/i18n/react";
|
||||
import enCommon from "../../../locales/en/common.json";
|
||||
import enAgents from "../../../locales/en/agents.json";
|
||||
import enIssues from "../../../locales/en/issues.json";
|
||||
|
||||
import { AccessPicker } from "./access-picker";
|
||||
|
||||
// ActorAvatar pulls workspace context (useWorkspaceId) that this unit test
|
||||
// doesn't provide; stub it — the picker logic under test doesn't depend on it.
|
||||
vi.mock("../../../common/actor-avatar", () => ({
|
||||
ActorAvatar: () => null,
|
||||
}));
|
||||
|
||||
const TEST_RESOURCES = {
|
||||
en: { common: enCommon, agents: enAgents, issues: enIssues },
|
||||
};
|
||||
|
||||
const MEMBERS = [
|
||||
{ user_id: "u1", name: "Alice", role: "member" },
|
||||
{ user_id: "u2", name: "Bob", role: "member" },
|
||||
] as unknown as MemberWithUser[];
|
||||
|
||||
function renderPicker(
|
||||
props: Partial<React.ComponentProps<typeof AccessPicker>> = {},
|
||||
) {
|
||||
const onChange = vi.fn();
|
||||
const utils = render(
|
||||
<I18nProvider locale="en" resources={TEST_RESOURCES}>
|
||||
<AccessPicker
|
||||
permissionMode="private"
|
||||
invocationTargets={[]}
|
||||
visibility="private"
|
||||
members={MEMBERS}
|
||||
canEdit
|
||||
onChange={onChange}
|
||||
{...props}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
return { ...utils, onChange };
|
||||
}
|
||||
|
||||
describe("AccessPicker owner-only editing (MUL-3963)", () => {
|
||||
beforeEach(() => cleanup());
|
||||
afterEach(() => cleanup());
|
||||
|
||||
it("renders a static, non-interactive read-only state for non-owners", () => {
|
||||
const targets: AgentInvocationTarget[] = [
|
||||
{ target_type: "workspace", target_id: "ws-1" },
|
||||
];
|
||||
renderPicker({
|
||||
canEdit: false,
|
||||
permissionMode: "public_to",
|
||||
invocationTargets: targets,
|
||||
visibility: "workspace",
|
||||
});
|
||||
|
||||
// No clickable trigger — a non-owner can never open the picker.
|
||||
expect(screen.queryByRole("button")).toBeNull();
|
||||
// The current access is still shown…
|
||||
expect(screen.getByTestId("access-readonly")).toBeInTheDocument();
|
||||
expect(screen.getByText("Workspace")).toBeInTheDocument();
|
||||
// …with the owner-only explanation surfaced as the accessible label.
|
||||
expect(
|
||||
screen.getByLabelText(
|
||||
"Only the agent owner can change who can run this agent.",
|
||||
),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders an interactive trigger for the owner", () => {
|
||||
renderPicker({ canEdit: true });
|
||||
expect(screen.getByRole("button")).toBeInTheDocument();
|
||||
// Private is the default summary.
|
||||
expect(screen.getAllByText("Only me").length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it("owner can pick a specific member, emitting a public_to member target", () => {
|
||||
const { onChange } = renderPicker({ canEdit: true });
|
||||
fireEvent.click(screen.getByRole("button"));
|
||||
// Checkbox order in the open popover: [0] workspace, [1] Alice, [2] Bob.
|
||||
const boxes = screen.getAllByRole("checkbox");
|
||||
fireEvent.click(boxes[1]);
|
||||
expect(onChange).toHaveBeenCalledWith({
|
||||
permission_mode: "public_to",
|
||||
invocation_targets: [{ target_type: "member", target_id: "u1" }],
|
||||
});
|
||||
});
|
||||
|
||||
it("owner can stack workspace + a member (mixed, multi-select)", () => {
|
||||
// Start from a member target; toggling the workspace checkbox must ADD a
|
||||
// workspace target rather than replacing the member one.
|
||||
const { onChange } = renderPicker({
|
||||
canEdit: true,
|
||||
permissionMode: "public_to",
|
||||
invocationTargets: [{ target_type: "member", target_id: "u1" }],
|
||||
visibility: "private",
|
||||
});
|
||||
fireEvent.click(screen.getByRole("button"));
|
||||
const boxes = screen.getAllByRole("checkbox");
|
||||
// [0] is the "Everyone in workspace" toggle.
|
||||
fireEvent.click(boxes[0]);
|
||||
expect(onChange).toHaveBeenCalledWith({
|
||||
permission_mode: "public_to",
|
||||
invocation_targets: [
|
||||
{ target_type: "workspace" },
|
||||
{ target_type: "member", target_id: "u1" },
|
||||
],
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -10,12 +10,16 @@ import type {
|
||||
MemberWithUser,
|
||||
} from "@multica/core/types";
|
||||
import { Checkbox } from "@multica/ui/components/ui/checkbox";
|
||||
import {
|
||||
Tooltip,
|
||||
TooltipTrigger,
|
||||
TooltipContent,
|
||||
} from "@multica/ui/components/ui/tooltip";
|
||||
import {
|
||||
PickerItem,
|
||||
PropertyPicker,
|
||||
} from "../../../issues/components/pickers";
|
||||
import { ActorAvatar } from "../../../common/actor-avatar";
|
||||
import { VisibilityBadge } from "../visibility-badge";
|
||||
import { useT } from "../../../i18n";
|
||||
import { CHIP_CLASS } from "./chip";
|
||||
|
||||
@@ -27,13 +31,20 @@ import { CHIP_CLASS } from "./chip";
|
||||
* Access is EITHER Private (only me) OR Public with a STACKABLE, MIXED
|
||||
* allow-list: the owner can combine "Everyone in workspace" + any number of
|
||||
* specific members + (future) teams on the same agent. `canInvokeAgent` on
|
||||
* the backend admits an actor when they match ANY target (OR), so the picker
|
||||
* emits the full union of every selected target and the whole set is replaced
|
||||
* on save. Team is a disabled placeholder in v1 but the structure is already
|
||||
* multi-select; any team targets that already exist are preserved on save.
|
||||
* the backend admits an actor matching ANY target (OR), so the picker emits
|
||||
* the full union of every selected target and the whole set is replaced on
|
||||
* save.
|
||||
*
|
||||
* Non-editors get the read-only `<VisibilityBadge>` so the display surface is
|
||||
* unchanged for viewers.
|
||||
* OWNER-ONLY (MUL-3963): access is the one agent property a workspace admin
|
||||
* may NOT change — only the agent owner decides who can run their agent, and
|
||||
* the backend rejects a non-owner permission change with 403. So `canEdit`
|
||||
* here must be passed as "is the viewer the agent owner", NOT the general
|
||||
* manage permission. When `canEdit` is false the control is a static,
|
||||
* non-interactive read-only display (current value + a lock affordance +
|
||||
* a tooltip explaining only the owner can change it), the same way GitHub /
|
||||
* Notion present a permission setting a viewer can see but not edit. There is
|
||||
* deliberately no clickable trigger in that state, so a non-owner can never
|
||||
* open a picker that the backend would only bounce back.
|
||||
*/
|
||||
|
||||
export type AccessChange = {
|
||||
@@ -60,7 +71,7 @@ function selectedTeamIds(targets: AgentInvocationTarget[]): string[] {
|
||||
export function AccessPicker({
|
||||
permissionMode,
|
||||
invocationTargets,
|
||||
visibility,
|
||||
visibility: _visibility,
|
||||
members,
|
||||
canEdit = true,
|
||||
hasComposioAllowlist = false,
|
||||
@@ -68,10 +79,17 @@ export function AccessPicker({
|
||||
}: {
|
||||
permissionMode: AgentPermissionMode;
|
||||
invocationTargets: AgentInvocationTarget[];
|
||||
/** Derived visibility, used only for the read-only badge path. */
|
||||
/**
|
||||
* Legacy derived visibility. No longer rendered directly (the read-only and
|
||||
* editable states both summarise permission_mode + targets), but kept in the
|
||||
* props so existing call sites compile unchanged.
|
||||
*/
|
||||
visibility: AgentVisibility;
|
||||
members: MemberWithUser[];
|
||||
/** When false, render a read-only `<VisibilityBadge>` and skip the popover. */
|
||||
/**
|
||||
* True ONLY when the viewer is the agent owner (MUL-3963 access is
|
||||
* owner-only). When false, render the static read-only state.
|
||||
*/
|
||||
canEdit?: boolean;
|
||||
/**
|
||||
* True when the agent already has a non-empty Composio toolkit allowlist.
|
||||
@@ -85,10 +103,8 @@ export function AccessPicker({
|
||||
const [open, setOpen] = useState(false);
|
||||
const [showComposioHint, setShowComposioHint] = useState(false);
|
||||
|
||||
if (!canEdit) {
|
||||
return <VisibilityBadge value={visibility} />;
|
||||
}
|
||||
|
||||
// Display summary of the current access, shared by the read-only and
|
||||
// editable states so they never drift.
|
||||
const isPrivate = permissionMode === "private";
|
||||
const workspaceOn = !isPrivate && hasWorkspaceTarget(invocationTargets);
|
||||
const memberIds = selectedMemberIds(invocationTargets);
|
||||
@@ -97,6 +113,47 @@ export function AccessPicker({
|
||||
const teamIds = selectedTeamIds(invocationTargets);
|
||||
const memberCount = memberIds.length;
|
||||
|
||||
const SummaryIcon = isPrivate
|
||||
? Lock
|
||||
: workspaceOn
|
||||
? Globe
|
||||
: memberCount > 0
|
||||
? Users
|
||||
: Globe;
|
||||
|
||||
const summaryLabel = isPrivate
|
||||
? t(($) => $.access.trigger_private)
|
||||
: workspaceOn
|
||||
? t(($) => $.access.trigger_workspace)
|
||||
: memberCount > 0
|
||||
? t(($) => $.access.trigger_members_count, { count: memberCount })
|
||||
: t(($) => $.access.trigger_members_empty);
|
||||
|
||||
// Read-only state for non-owners: current value + lock + owner-only tooltip.
|
||||
// No interactive trigger is rendered, so the control can never be clicked
|
||||
// into a change the backend would reject.
|
||||
if (!canEdit) {
|
||||
const readOnlyMsg = t(($) => $.access.owner_only_readonly);
|
||||
return (
|
||||
<Tooltip>
|
||||
<TooltipTrigger
|
||||
render={
|
||||
<span
|
||||
className="inline-flex items-center gap-1 text-xs text-muted-foreground"
|
||||
aria-label={readOnlyMsg}
|
||||
data-testid="access-readonly"
|
||||
>
|
||||
<SummaryIcon className="h-3 w-3 shrink-0" />
|
||||
<span className="truncate">{summaryLabel}</span>
|
||||
<Lock className="h-3 w-3 shrink-0 opacity-60" />
|
||||
</span>
|
||||
}
|
||||
/>
|
||||
<TooltipContent>{readOnlyMsg}</TooltipContent>
|
||||
</Tooltip>
|
||||
);
|
||||
}
|
||||
|
||||
// Build the union of every selected target and emit it. An empty union
|
||||
// collapses to Private (owner-only), which is the intuitive "nothing shared"
|
||||
// state rather than a public_to with no grants.
|
||||
@@ -145,22 +202,6 @@ export function AccessPicker({
|
||||
emit({ workspace: workspaceOn, members: Array.from(next), teams: teamIds });
|
||||
};
|
||||
|
||||
const TriggerIcon = isPrivate
|
||||
? Lock
|
||||
: workspaceOn
|
||||
? Globe
|
||||
: memberCount > 0
|
||||
? Users
|
||||
: Globe;
|
||||
|
||||
const triggerLabel = isPrivate
|
||||
? t(($) => $.access.trigger_private)
|
||||
: workspaceOn
|
||||
? t(($) => $.access.trigger_workspace)
|
||||
: memberCount > 0
|
||||
? t(($) => $.access.trigger_members_count, { count: memberCount })
|
||||
: t(($) => $.access.trigger_members_empty);
|
||||
|
||||
const tooltip = t(($) => $.access.tooltip);
|
||||
|
||||
return (
|
||||
@@ -178,8 +219,8 @@ export function AccessPicker({
|
||||
}
|
||||
trigger={
|
||||
<>
|
||||
<TriggerIcon className="h-3 w-3 shrink-0 text-muted-foreground" />
|
||||
<span className="truncate">{triggerLabel}</span>
|
||||
<SummaryIcon className="h-3 w-3 shrink-0 text-muted-foreground" />
|
||||
<span className="truncate">{summaryLabel}</span>
|
||||
</>
|
||||
}
|
||||
>
|
||||
|
||||
@@ -91,7 +91,6 @@ function items(qc: QueryClient, query = ""): SlashCommandItem[] {
|
||||
return config.items!({
|
||||
query,
|
||||
editor: {} as never,
|
||||
signal: new AbortController().signal,
|
||||
}) as SlashCommandItem[];
|
||||
}
|
||||
|
||||
|
||||
@@ -467,6 +467,7 @@
|
||||
"team_coming_soon": "Coming soon",
|
||||
"members_group": "People",
|
||||
"public_group": "Public access",
|
||||
"owner_only_readonly": "Only the agent owner can change who can run this agent.",
|
||||
"members_empty": "No workspace members to choose from",
|
||||
"composio_switch_hint": "Heads up — this agent has Composio apps enabled. Sharing it lets everyone with access use those apps through this agent."
|
||||
},
|
||||
|
||||
@@ -446,6 +446,7 @@
|
||||
"team_coming_soon": "近日公開",
|
||||
"members_group": "メンバー",
|
||||
"public_group": "公開アクセス",
|
||||
"owner_only_readonly": "このエージェントを実行できる相手は、オーナーのみが変更できます。",
|
||||
"members_empty": "選択できるワークスペースメンバーがいません",
|
||||
"composio_switch_hint": "ご注意 — このエージェントは Composio アプリが有効です。共有すると、アクセス権を持つ全員がこのエージェントを通じてこれらのアプリを利用できます。"
|
||||
},
|
||||
|
||||
@@ -454,6 +454,7 @@
|
||||
"team_coming_soon": "출시 예정",
|
||||
"members_group": "구성원",
|
||||
"public_group": "공개 액세스",
|
||||
"owner_only_readonly": "이 에이전트를 실행할 수 있는 대상은 소유자만 변경할 수 있습니다.",
|
||||
"members_empty": "선택할 워크스페이스 구성원이 없습니다",
|
||||
"composio_switch_hint": "참고 — 이 에이전트에는 Composio 앱이 활성화되어 있습니다. 공유하면 접근 권한이 있는 모든 사람이 이 에이전트를 통해 해당 앱을 사용할 수 있습니다."
|
||||
},
|
||||
|
||||
@@ -454,6 +454,7 @@
|
||||
"team_coming_soon": "即将推出",
|
||||
"members_group": "成员",
|
||||
"public_group": "公开访问",
|
||||
"owner_only_readonly": "只有该 Agent 的所有者可以更改谁能调用它。",
|
||||
"members_empty": "没有可选择的工作区成员",
|
||||
"composio_switch_hint": "提示 —— 该智能体已启用 Composio 应用。共享后,所有获得访问权限的人都可以通过该智能体使用这些应用。"
|
||||
},
|
||||
|
||||
@@ -1294,11 +1294,16 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) {
|
||||
targetRuntimeID = runtime.ID
|
||||
targetProvider = runtime.Provider
|
||||
}
|
||||
// Invocation permission (MUL-3963). Owner-only write: an admin who passes
|
||||
// permission fields is silently ignored (the invoke gate is owner/allow-
|
||||
// list based, so an admin-authored allow-list would just confuse the
|
||||
// owner). When the owner does pass them, permission_mode is authoritative;
|
||||
// otherwise legacy visibility is mapped. Targets are replaced wholesale.
|
||||
// Invocation permission (MUL-3963). OWNER-ONLY write: access is the one
|
||||
// agent property a workspace admin may NOT change (only the owner decides
|
||||
// who can run their agent — the overlay uses the owner's own Composio
|
||||
// connection, so admin-authored access would be confusing and unsafe).
|
||||
//
|
||||
// Non-owner behaviour: a *real* change is rejected with 403 so the contract
|
||||
// is explicit and matches the owner-only UI (the picker is read-only for
|
||||
// non-owners). A no-op resubmit — an admin editing OTHER fields via a
|
||||
// PATCH-as-PUT client that echoes the unchanged permission back — is
|
||||
// tolerated (dropped) so it doesn't break legitimate admin edits.
|
||||
_, hasPermissionMode := rawFields["permission_mode"]
|
||||
_, hasTargets := rawFields["invocation_targets"]
|
||||
permissionTouched := hasPermissionMode || hasTargets || req.Visibility != nil
|
||||
@@ -1307,7 +1312,16 @@ func (h *Handler) UpdateAgent(w http.ResponseWriter, r *http.Request) {
|
||||
if permissionTouched {
|
||||
isAgentOwner := uuidToString(existing.OwnerID) == requestUserID(r)
|
||||
if !isAgentOwner {
|
||||
slog.Debug("update agent: invocation permission write by non-owner silently dropped",
|
||||
changed, permErr := h.permissionInputChangesAgent(r.Context(), existing, req, hasPermissionMode, hasTargets)
|
||||
if permErr != nil {
|
||||
writeError(w, http.StatusInternalServerError, "failed to evaluate invocation permission change")
|
||||
return
|
||||
}
|
||||
if changed {
|
||||
writeError(w, http.StatusForbidden, "only the agent owner can change access (permission_mode / invocation_targets)")
|
||||
return
|
||||
}
|
||||
slog.Debug("update agent: non-owner permission fields matched current state; ignored",
|
||||
append(logger.RequestAttrs(r), "agent_id", id)...)
|
||||
} else {
|
||||
var targetsDTO []AgentInvocationTargetDTO
|
||||
|
||||
@@ -205,6 +205,73 @@ func (h *Handler) replaceInvocationTargets(ctx context.Context, agentID pgtype.U
|
||||
return nil
|
||||
}
|
||||
|
||||
// permissionInputChangesAgent reports whether the permission fields on an
|
||||
// update request would actually CHANGE the agent's current invocation
|
||||
// permission (mode or the set of targets). Used to let a non-owner's no-op
|
||||
// resubmit through (PATCH-as-PUT that echoes unchanged permission) while
|
||||
// rejecting a real change with 403. Invalid/absent permission input counts as
|
||||
// "no change" — a non-owner sending garbage should not get a 403 either, it is
|
||||
// simply ignored. Fails safe: on a DB error it returns changed=true so a
|
||||
// non-owner attempt is rejected rather than silently applied.
|
||||
func (h *Handler) permissionInputChangesAgent(ctx context.Context, existing db.Agent, req UpdateAgentRequest, hasPermissionMode, hasTargets bool) (bool, error) {
|
||||
// Legacy-only request: the caller sent ONLY `visibility`, no permission_mode
|
||||
// and no invocation_targets (an old client / PATCH-as-PUT echoing the field
|
||||
// back while editing something else). Compare on the DERIVED legacy
|
||||
// visibility, NOT by expanding "private" into a real private permission.
|
||||
// A member-only public_to agent derives to legacy "private", so an admin
|
||||
// resubmitting visibility:"private" is a NO-OP, not a public_to→private
|
||||
// downgrade. Only a real legacy change (e.g. "workspace") counts. (MUL-3963
|
||||
// review — this is the compatibility fix for PR #4853.)
|
||||
if !hasPermissionMode && !hasTargets {
|
||||
if req.Visibility == nil {
|
||||
return false, nil
|
||||
}
|
||||
current, err := h.Queries.ListAgentInvocationTargets(ctx, existing.ID)
|
||||
if err != nil {
|
||||
return true, err
|
||||
}
|
||||
submitted := "private"
|
||||
if *req.Visibility == "workspace" {
|
||||
submitted = "workspace"
|
||||
}
|
||||
return submitted != deriveLegacyVisibility(existing.PermissionMode, current), nil
|
||||
}
|
||||
|
||||
var targetsDTO []AgentInvocationTargetDTO
|
||||
if req.InvocationTargets != nil {
|
||||
targetsDTO = *req.InvocationTargets
|
||||
}
|
||||
perm, ok, err := parsePermissionInput(existing.WorkspaceID, req.PermissionMode, targetsDTO, hasPermissionMode, hasTargets, req.Visibility)
|
||||
if err != nil || !ok {
|
||||
// Unparseable or effectively no permission fields → treat as no change.
|
||||
return false, nil
|
||||
}
|
||||
if perm.mode != existing.PermissionMode {
|
||||
return true, nil
|
||||
}
|
||||
current, err := h.Queries.ListAgentInvocationTargets(ctx, existing.ID)
|
||||
if err != nil {
|
||||
return true, err
|
||||
}
|
||||
want := make(map[string]struct{}, len(perm.targets))
|
||||
for _, tgt := range perm.targets {
|
||||
want[tgt.targetType+":"+uuidToString(tgt.targetID)] = struct{}{}
|
||||
}
|
||||
have := make(map[string]struct{}, len(current))
|
||||
for _, row := range current {
|
||||
have[row.TargetType+":"+uuidToString(row.TargetID)] = struct{}{}
|
||||
}
|
||||
if len(want) != len(have) {
|
||||
return true, nil
|
||||
}
|
||||
for k := range want {
|
||||
if _, ok := have[k]; !ok {
|
||||
return true, nil
|
||||
}
|
||||
}
|
||||
return false, nil
|
||||
}
|
||||
|
||||
// enrichAgentResponseWithTargets loads an agent's invocation targets and
|
||||
// applies them to the response (InvocationTargets + derived Visibility). Used
|
||||
// by the single-agent detail / create / update responses.
|
||||
|
||||
@@ -614,3 +614,122 @@ func TestRevokeMember_InvocationTargetCleanupIsWorkspaceScoped(t *testing.T) {
|
||||
t.Errorf("workspace B target MUST survive removal from A (cross-workspace collateral), got %d", n)
|
||||
}
|
||||
}
|
||||
|
||||
// createPermissionTestAdmin inserts a fresh workspace member with the admin
|
||||
// role and returns its user id.
|
||||
func createPermissionTestAdmin(t *testing.T, email string) string {
|
||||
t.Helper()
|
||||
ctx := context.Background()
|
||||
var userID string
|
||||
if err := testPool.QueryRow(ctx, `INSERT INTO "user" (name, email) VALUES ($1, $2) RETURNING id`, email, email).Scan(&userID); err != nil {
|
||||
t.Fatalf("create admin user %s: %v", email, err)
|
||||
}
|
||||
t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM "user" WHERE id = $1`, userID) })
|
||||
if _, err := testPool.Exec(ctx, `INSERT INTO member (workspace_id, user_id, role) VALUES ($1, $2, 'admin')`, testWorkspaceID, userID); err != nil {
|
||||
t.Fatalf("add admin %s: %v", email, err)
|
||||
}
|
||||
return userID
|
||||
}
|
||||
|
||||
// TestUpdateAgent_AccessChangeIsOwnerOnly locks the interaction-bug fix
|
||||
// (separate PR): a workspace ADMIN who is NOT the agent owner may edit other
|
||||
// agent fields but must NOT change access — a real permission change returns an
|
||||
// explicit 403 (no more silent "bounce back"), while a no-op resubmit and
|
||||
// edits to other fields still succeed. The agent owner can change access.
|
||||
func TestUpdateAgent_AccessChangeIsOwnerOnly(t *testing.T) {
|
||||
if testHandler == nil || testPool == nil {
|
||||
t.Skip("database not available")
|
||||
}
|
||||
ctx := context.Background()
|
||||
|
||||
// Agent owned by testUserID, public_to workspace (createHandlerTestAgent).
|
||||
agentID := createHandlerTestAgent(t, "owner-only-access-agent", nil)
|
||||
adminID := createPermissionTestAdmin(t, "perm-access-admin@multica.test")
|
||||
|
||||
put := func(actorID string, body map[string]any) int {
|
||||
rec := httptest.NewRecorder()
|
||||
r := newRequestAs(actorID, "PUT", "/api/agents/"+agentID, body)
|
||||
r = withURLParam(r, "id", agentID)
|
||||
testHandler.UpdateAgent(rec, r)
|
||||
return rec.Code
|
||||
}
|
||||
|
||||
// Admin (non-owner) attempts a REAL access change → 403.
|
||||
rec := httptest.NewRecorder()
|
||||
r := newRequestAs(adminID, "PUT", "/api/agents/"+agentID, map[string]any{"permission_mode": "private"})
|
||||
r = withURLParam(r, "id", agentID)
|
||||
testHandler.UpdateAgent(rec, r)
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("admin access change: expected 403, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
// Access must be unchanged (still public_to workspace).
|
||||
if a, _ := testHandler.Queries.GetAgent(ctx, util.MustParseUUID(agentID)); a.PermissionMode != "public_to" {
|
||||
t.Errorf("access must be unchanged after rejected admin write, got %q", a.PermissionMode)
|
||||
}
|
||||
|
||||
// Admin no-op resubmit of the CURRENT permission (PATCH-as-PUT) → tolerated.
|
||||
if code := put(adminID, map[string]any{
|
||||
"permission_mode": "public_to",
|
||||
"invocation_targets": []map[string]any{{"target_type": "workspace"}},
|
||||
}); code != http.StatusOK {
|
||||
t.Errorf("admin no-op permission resubmit: expected 200, got %d", code)
|
||||
}
|
||||
|
||||
// Admin editing a NON-permission field still works.
|
||||
if code := put(adminID, map[string]any{"description": "renamed by admin"}); code != http.StatusOK {
|
||||
t.Errorf("admin editing other fields: expected 200, got %d", code)
|
||||
}
|
||||
|
||||
// The owner CAN change access.
|
||||
if code := put(testUserID, map[string]any{"permission_mode": "private"}); code != http.StatusOK {
|
||||
t.Errorf("owner access change: expected 200, got %d", code)
|
||||
}
|
||||
if n := invocationTargetCount(t, agentID); n != 0 {
|
||||
t.Errorf("owner set private: expected 0 targets, got %d", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestUpdateAgent_LegacyVisibilityNoOpForMemberOnlyPublicTo locks the PR #4853
|
||||
// compatibility fix: a member-only public_to agent DERIVES legacy visibility
|
||||
// "private", so an admin (non-owner) echoing visibility:"private" via an old
|
||||
// client / PATCH-as-PUT while editing another field must be treated as a NO-OP
|
||||
// (200, targets unchanged) — not misread as a public_to→private downgrade
|
||||
// (403). Submitting visibility:"workspace" is a real change and still 403.
|
||||
func TestUpdateAgent_LegacyVisibilityNoOpForMemberOnlyPublicTo(t *testing.T) {
|
||||
if testHandler == nil || testPool == nil {
|
||||
t.Skip("database not available")
|
||||
}
|
||||
ctx := context.Background()
|
||||
|
||||
memberX := createPermissionTestMember(t, "perm-legacyvis-x@multica.test")
|
||||
agentID := createPublicToAgentWithTargets(t, "legacy-vis-member-only-agent", []map[string]any{
|
||||
{"target_type": "member", "target_id": memberX},
|
||||
})
|
||||
adminID := createPermissionTestAdmin(t, "perm-legacyvis-admin@multica.test")
|
||||
|
||||
put := func(actorID string, body map[string]any) int {
|
||||
rec := httptest.NewRecorder()
|
||||
r := newRequestAs(actorID, "PUT", "/api/agents/"+agentID, body)
|
||||
r = withURLParam(r, "id", agentID)
|
||||
testHandler.UpdateAgent(rec, r)
|
||||
return rec.Code
|
||||
}
|
||||
|
||||
// Derived legacy visibility of a member-only public_to agent is "private".
|
||||
// Admin echoing that back while editing description → 200 no-op.
|
||||
if code := put(adminID, map[string]any{"visibility": "private", "description": "admin note"}); code != http.StatusOK {
|
||||
t.Fatalf("admin legacy visibility=private no-op: expected 200, got %d", code)
|
||||
}
|
||||
// Access must be untouched: still public_to with the one member target.
|
||||
if a, _ := testHandler.Queries.GetAgent(ctx, util.MustParseUUID(agentID)); a.PermissionMode != "public_to" {
|
||||
t.Errorf("permission_mode must stay public_to after legacy no-op, got %q", a.PermissionMode)
|
||||
}
|
||||
if n := invocationTargetCount(t, agentID); n != 1 {
|
||||
t.Errorf("member target must be intact after legacy no-op, got %d targets", n)
|
||||
}
|
||||
|
||||
// Admin submitting a REAL legacy change (workspace) is still rejected.
|
||||
if code := put(adminID, map[string]any{"visibility": "workspace"}); code != http.StatusForbidden {
|
||||
t.Errorf("admin legacy visibility=workspace (real change): expected 403, got %d", code)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user