mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix(site/src/pages/AgentsPage): hide owner chat actions from shared chat viewers (#29691)
> 🤖 This PR was written by Coder Agents on behalf of Jake Howell. Viewers of a shared chat saw the full actions menu next to the chat title and in the sidebar, and Pin/Unpin, Rename, Archive, and Archive & delete workspace all write to the chat record, so a viewer was changing the owner's sidebar and title. The menu's owner actions are now gated on chat ownership in both `ChatTopBar` and the sidebar `ChatTreeNode`. Viewers keep the "Show subagents" toggle, which is the touch path for expanding a row, and the menu trigger is suppressed when nothing remains. New Storybook stories capture the viewer states in the top bar and sidebar. Closes CODAGT-1095 (https://linear.app/codercom/issue/CODAGT-1095/prevent-shared-chat-viewers-from-modifying-owner-chat-settings) <details> <summary>Decision log</summary> - **Backend needs no change.** `patchChat` already requires `chat:update`, and the only sharing role (`read`) grants `ActionRead`, so viewers already get 404 from the API (`coderd/exp_chats_acl_test.go` covers the rename case; pin and archive go through the same handler and gate). The reporter succeeded because admin roles legitimately grant `chat:update` deployment-wide. - **Ownership, not an authorization check, decides visibility.** An authz check would still show the menu to admins viewing a shared chat, which is the reported surprise. The chat list only ever returns owned or shared-with-me chats, so there is no admin "manage other users' chats" surface in this UI to lose. The rule lives in one place, `canManageChat`, mirroring the existing `currentUser.id !== chat.owner_id` convention in `AgentChatPageView`. - **`AgentsPageLayout.stories.tsx` fixture.** Its `buildChat` gave every chat a foreign `owner_id` from before sharing existed, a state the list API cannot produce. Dropped so those sidebar rows model owned chats now that ownership matters for the row menu. </details>
This commit is contained in:
@@ -94,9 +94,6 @@ const todayTimestamp = new Date().toISOString();
|
||||
const buildChat = (overrides: Partial<Chat> = {}): Chat => ({
|
||||
...MockChat,
|
||||
id: "chat-default",
|
||||
owner_id: "owner-1",
|
||||
owner_username: "owner",
|
||||
owner_name: undefined,
|
||||
last_model_config_id: defaultModels[0].id,
|
||||
created_at: oneWeekAgo,
|
||||
updated_at: oneWeekAgo,
|
||||
|
||||
@@ -0,0 +1,44 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { MockChat } from "#/testHelpers/chatEntities";
|
||||
import { MockUserOwner } from "#/testHelpers/entities";
|
||||
import { canManageChat, chatHasMenuActions } from "./ChatActionsMenuItems";
|
||||
|
||||
const sharedByAnotherUser = {
|
||||
...MockChat,
|
||||
owner_id: "sharing-user",
|
||||
shared: true,
|
||||
};
|
||||
|
||||
describe("canManageChat", () => {
|
||||
it("is true only for the chat owner", () => {
|
||||
expect(canManageChat(MockChat, MockUserOwner.id)).toBe(true);
|
||||
expect(canManageChat(sharedByAnotherUser, MockUserOwner.id)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("chatHasMenuActions", () => {
|
||||
it("hides the menu from viewers unless the subagents toggle is available", () => {
|
||||
expect(chatHasMenuActions(sharedByAnotherUser, { canManage: false })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(
|
||||
chatHasMenuActions(sharedByAnotherUser, {
|
||||
canManage: false,
|
||||
hasSubagentsToggle: true,
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("hides the menu from owners only for archived child chats", () => {
|
||||
expect(chatHasMenuActions(MockChat, { canManage: true })).toBe(true);
|
||||
expect(
|
||||
chatHasMenuActions({ ...MockChat, archived: true }, { canManage: true }),
|
||||
).toBe(true);
|
||||
expect(
|
||||
chatHasMenuActions(
|
||||
{ ...MockChat, archived: true, parent_chat_id: "parent-chat" },
|
||||
{ canManage: true },
|
||||
),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -45,19 +45,46 @@ type SeparatorComponent =
|
||||
| typeof DropdownMenuSeparator
|
||||
| typeof ContextMenuSeparator;
|
||||
|
||||
/**
|
||||
* Pin, rename, and archive write to the chat record itself, so from a shared
|
||||
* chat they would change the owner's sidebar and title. Sharing only grants
|
||||
* read access, and admins who would pass the server's update check should
|
||||
* not manage another user's chat from a shared view either, so ownership
|
||||
* rather than authorization decides who sees those actions.
|
||||
*/
|
||||
export const canManageChat = (
|
||||
chat: TypesGen.Chat,
|
||||
currentUserId: string,
|
||||
): boolean => chat.owner_id === currentUserId;
|
||||
|
||||
type ChatMenuActionsOptions = {
|
||||
readonly canManage: boolean;
|
||||
/** Whether the menu offers the subagents toggle, the only viewer action. */
|
||||
readonly hasSubagentsToggle?: boolean;
|
||||
};
|
||||
|
||||
/**
|
||||
* Archive state is root-only on the backend and cascades to children, so
|
||||
* child chats expose no archive or unarchive actions. An archived child chat
|
||||
* therefore has no menu actions at all; call sites use this to hide the menu
|
||||
* trigger instead of rendering an empty menu.
|
||||
* therefore has no menu actions at all, and a non-owner only has the
|
||||
* subagents toggle; call sites use this to hide the menu trigger instead of
|
||||
* rendering an empty menu.
|
||||
*/
|
||||
export const chatHasMenuActions = (chat: TypesGen.Chat): boolean => {
|
||||
export const chatHasMenuActions = (
|
||||
chat: TypesGen.Chat,
|
||||
{ canManage, hasSubagentsToggle = false }: ChatMenuActionsOptions,
|
||||
): boolean => {
|
||||
if (!canManage) {
|
||||
return hasSubagentsToggle;
|
||||
}
|
||||
const isArchivedChild = chat.archived && getParentChatID(chat) !== undefined;
|
||||
return !isArchivedChild;
|
||||
};
|
||||
|
||||
type ChatActionsMenuItemsProps = {
|
||||
readonly chat: TypesGen.Chat;
|
||||
/** See {@link canManageChat}. When false, only the subagents toggle renders. */
|
||||
readonly canManage: boolean;
|
||||
readonly hasWorkspace: boolean;
|
||||
readonly isArchiving?: boolean;
|
||||
readonly isArchiveBlocked?: boolean;
|
||||
@@ -77,6 +104,7 @@ type ChatActionsMenuItemsProps = {
|
||||
|
||||
export const ChatActionsMenuItems: FC<ChatActionsMenuItemsProps> = ({
|
||||
chat,
|
||||
canManage,
|
||||
hasWorkspace,
|
||||
isArchiving = false,
|
||||
isArchiveBlocked = false,
|
||||
@@ -113,6 +141,10 @@ export const ChatActionsMenuItems: FC<ChatActionsMenuItemsProps> = ({
|
||||
</Item>
|
||||
) : null;
|
||||
|
||||
if (!canManage) {
|
||||
return subagentToggle;
|
||||
}
|
||||
|
||||
return (
|
||||
<>
|
||||
{showPinAction && (
|
||||
|
||||
@@ -72,6 +72,7 @@ const defaultProps = {
|
||||
const meta: Meta<typeof ChatTopBar> = {
|
||||
title: "pages/AgentsPage/ChatTopBar",
|
||||
component: ChatTopBar,
|
||||
decorators: [withAuthProvider],
|
||||
beforeEach: () => {
|
||||
requestArchiveAgent.mockClear();
|
||||
requestArchiveAndDeleteWorkspace.mockClear();
|
||||
@@ -85,6 +86,7 @@ const meta: Meta<typeof ChatTopBar> = {
|
||||
},
|
||||
parameters: {
|
||||
layout: "fullscreen",
|
||||
user: MockUserOwner,
|
||||
reactRouter: reactRouterParameters({
|
||||
location: { path: "/agents/chat-1" },
|
||||
routing: [
|
||||
@@ -112,6 +114,19 @@ export const SharedChat: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
/** Viewers of another user's chat get no owner actions menu. */
|
||||
export const SharedChatViewer: Story = {
|
||||
args: {
|
||||
chat: {
|
||||
...MockChat,
|
||||
owner_id: "sharing-user",
|
||||
owner_username: "sharing-user",
|
||||
owner_name: "Sharing User",
|
||||
shared: true,
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
export const WithPanelOpen: Story = {
|
||||
args: {
|
||||
panel: {
|
||||
@@ -570,7 +585,7 @@ export const PreservesArchivedFilterOnMobileBack: Story = {
|
||||
};
|
||||
|
||||
export const ShareChatButton: Story = {
|
||||
decorators: [withAuthProvider, withDashboardProvider],
|
||||
decorators: [withDashboardProvider],
|
||||
args: {
|
||||
chat: {
|
||||
...MockChat,
|
||||
@@ -578,7 +593,6 @@ export const ShareChatButton: Story = {
|
||||
},
|
||||
},
|
||||
parameters: {
|
||||
user: MockUserOwner,
|
||||
queries: [
|
||||
{
|
||||
key: getAuthorizationKey({
|
||||
|
||||
@@ -24,10 +24,12 @@ import {
|
||||
DropdownMenuTrigger,
|
||||
} from "#/components/DropdownMenu/DropdownMenu";
|
||||
import { Popover, PopoverTrigger } from "#/components/Popover/Popover";
|
||||
import { useAuthenticated } from "#/hooks/useAuthenticated";
|
||||
import type { AgentsPageOutletContext } from "../AgentsPageLayout";
|
||||
import { parsePullRequestUrl } from "../utils/pullRequest";
|
||||
import {
|
||||
ChatActionsMenuItems,
|
||||
canManageChat,
|
||||
chatFamilyAllowsArchive,
|
||||
chatHasMenuActions,
|
||||
} from "./ChatActionsMenuItems";
|
||||
@@ -95,6 +97,7 @@ export const ChatTopBar: FC<ChatTopBarProps> = ({
|
||||
panel,
|
||||
}) => {
|
||||
const { isEmbedded } = useEmbedContext();
|
||||
const { user: currentUser } = useAuthenticated();
|
||||
const location = useLocation();
|
||||
const parentChatID = getParentChatID(chat);
|
||||
const parentChatQuery = useQuery({
|
||||
@@ -137,6 +140,7 @@ export const ChatTopBar: FC<ChatTopBarProps> = ({
|
||||
const chatTitle = chat?.title;
|
||||
const isArchived = chat?.archived ?? false;
|
||||
const isSharedChat = chat?.shared;
|
||||
const canManage = chat !== undefined && canManageChat(chat, currentUser.id);
|
||||
const hasWorkspace = Boolean(chat?.workspace_id);
|
||||
const isArchivingThisChat = Boolean(
|
||||
isArchiving &&
|
||||
@@ -151,6 +155,14 @@ export const ChatTopBar: FC<ChatTopBarProps> = ({
|
||||
)
|
||||
: false;
|
||||
const showPinAction = Boolean(requestPinAgent && requestUnpinAgent);
|
||||
// Suppressed when there is no chat to act on (loading and not-found views)
|
||||
// and when the chat has no menu actions (archived child chats and chats
|
||||
// shared by another user).
|
||||
const showActionsMenu =
|
||||
!isEmbedded &&
|
||||
chat !== undefined &&
|
||||
Boolean(chatTitle) &&
|
||||
chatHasMenuActions(chat, { canManage });
|
||||
const diffStatus = chat?.diff_status;
|
||||
|
||||
const prUrl = diffStatus?.url;
|
||||
@@ -230,10 +242,8 @@ export const ChatTopBar: FC<ChatTopBarProps> = ({
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
{/* Actions menu sits inline with the title so it tracks the title's right edge.
|
||||
Suppressed when there is no chat to act on (loading and not-found views)
|
||||
and when the chat has no menu actions (archived child chats). */}
|
||||
{!isEmbedded && chat && chatTitle && chatHasMenuActions(chat) && (
|
||||
{/* Actions menu sits inline with the title so it tracks the title's right edge. */}
|
||||
{chat && showActionsMenu && (
|
||||
<DropdownMenu>
|
||||
<DropdownMenuTrigger asChild>
|
||||
<Button
|
||||
@@ -251,6 +261,7 @@ export const ChatTopBar: FC<ChatTopBarProps> = ({
|
||||
>
|
||||
<ChatActionsMenuItems
|
||||
chat={chat}
|
||||
canManage={canManage}
|
||||
hasWorkspace={hasWorkspace}
|
||||
isArchiving={isArchivingThisChat}
|
||||
isArchiveBlocked={isArchiveBlocked}
|
||||
|
||||
@@ -243,6 +243,80 @@ export const SharedUnreadChat: Story = {
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* The active row normally swaps its timestamp for the actions trigger, but
|
||||
* another user's shared chat has no owner actions, so the timestamp stays.
|
||||
*/
|
||||
export const ActiveSharedChatViewerHasNoActions: Story = {
|
||||
args: {
|
||||
chats: [
|
||||
buildChat({
|
||||
id: "shared-active",
|
||||
title: "Shared active chat",
|
||||
owner_id: "sharing-user",
|
||||
owner_name: "Sharing User",
|
||||
owner_username: "sharing-user",
|
||||
shared: true,
|
||||
}),
|
||||
],
|
||||
},
|
||||
parameters: {
|
||||
reactRouter: reactRouterParameters({
|
||||
location: {
|
||||
path: "/agents/shared-active",
|
||||
pathParams: { agentId: "shared-active" },
|
||||
},
|
||||
routing: agentsRouting,
|
||||
}),
|
||||
},
|
||||
};
|
||||
|
||||
/** Viewers keep the subagents toggle, the touch path for expanding a row. */
|
||||
export const SharedChatViewerMenuOnlyTogglesSubagents: Story = {
|
||||
args: {
|
||||
chats: [
|
||||
buildChat({
|
||||
id: "shared-parent",
|
||||
title: "Shared parent chat",
|
||||
owner_id: "sharing-user",
|
||||
owner_name: "Sharing User",
|
||||
owner_username: "sharing-user",
|
||||
shared: true,
|
||||
children: [
|
||||
buildChat({
|
||||
id: "shared-child",
|
||||
title: "Shared child chat",
|
||||
owner_id: "sharing-user",
|
||||
owner_name: "Sharing User",
|
||||
owner_username: "sharing-user",
|
||||
parent_chat_id: "shared-parent",
|
||||
root_chat_id: "shared-parent",
|
||||
}),
|
||||
],
|
||||
}),
|
||||
],
|
||||
},
|
||||
parameters: {
|
||||
reactRouter: reactRouterParameters({
|
||||
location: {
|
||||
path: "/agents/shared-parent",
|
||||
pathParams: { agentId: "shared-parent" },
|
||||
},
|
||||
routing: agentsRouting,
|
||||
}),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
await userEvent.click(
|
||||
within(canvasElement).getByRole("button", {
|
||||
name: "Open actions for Shared parent chat",
|
||||
}),
|
||||
);
|
||||
await within(document.body).findByRole("menuitem", {
|
||||
name: "Show subagents (1)",
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
export const ChatStreamingOverridesTurnSummary: Story = {
|
||||
args: {
|
||||
chats: [
|
||||
|
||||
@@ -299,6 +299,7 @@ export const ChatsPanel: FC<ChatsPanelProps> = ({
|
||||
isLoadingModelConfigs,
|
||||
chatErrorReasons,
|
||||
activeChatId,
|
||||
currentUserId,
|
||||
isArchiving,
|
||||
archivingChatId,
|
||||
toggleExpanded,
|
||||
|
||||
@@ -12,6 +12,7 @@ export type ChatTreeContextValue = {
|
||||
readonly isLoadingModelConfigs: boolean;
|
||||
readonly chatErrorReasons: Record<string, string>;
|
||||
readonly activeChatId: string | undefined;
|
||||
readonly currentUserId: string;
|
||||
readonly isArchiving: boolean;
|
||||
readonly archivingChatId: string | null;
|
||||
readonly toggleExpanded: (chatID: string) => void;
|
||||
|
||||
@@ -27,6 +27,7 @@ import { Spinner } from "#/components/Spinner/Spinner";
|
||||
import { shortRelativeTime } from "#/utils/time";
|
||||
import {
|
||||
ChatActionsMenuItems,
|
||||
canManageChat,
|
||||
chatFamilyAllowsArchive,
|
||||
chatHasMenuActions,
|
||||
} from "../../ChatActionsMenuItems";
|
||||
@@ -57,6 +58,7 @@ export const ChatTreeNode: FC<ChatTreeNodeProps> = ({ chat, depth = 0 }) => {
|
||||
isLoadingModelConfigs,
|
||||
chatErrorReasons,
|
||||
activeChatId,
|
||||
currentUserId,
|
||||
isArchiving,
|
||||
archivingChatId,
|
||||
toggleExpanded,
|
||||
@@ -147,7 +149,11 @@ export const ChatTreeNode: FC<ChatTreeNodeProps> = ({ chat, depth = 0 }) => {
|
||||
const isArchivingThisChat = isArchiving && archivingChatId === chat.id;
|
||||
const isExpanded = normalizedSearch ? true : (expandedById[chatID] ?? false);
|
||||
|
||||
const hasMenuActions = chatHasMenuActions(chat);
|
||||
const canManage = canManageChat(chat, currentUserId);
|
||||
const hasMenuActions = chatHasMenuActions(chat, {
|
||||
canManage,
|
||||
hasSubagentsToggle: hasChildren,
|
||||
});
|
||||
|
||||
const hoverLayout =
|
||||
"[@media(hover:hover)]:hover:-mx-2 [@media(hover:hover)]:hover:pl-3 [@media(hover:hover)]:hover:pr-3.5 [@media(hover:hover)]:hover:rounded-none";
|
||||
@@ -155,6 +161,7 @@ export const ChatTreeNode: FC<ChatTreeNodeProps> = ({ chat, depth = 0 }) => {
|
||||
"has-[[aria-current=page]]:-mx-2 has-[[aria-current=page]]:pl-[11px] has-[[aria-current=page]]:pr-3.5 has-[[aria-current=page]]:rounded-none has-[[aria-current=page]]:border-l has-[[aria-current=page]]:border-content-primary [@media(hover:hover)]:has-[[aria-current=page]]:hover:pl-[11px]";
|
||||
const sharedMenuItemProps = {
|
||||
chat,
|
||||
canManage,
|
||||
hasWorkspace: Boolean(workspaceId),
|
||||
isArchiving,
|
||||
isArchiveBlocked: !chatFamilyAllowsArchive(chat.status, chat.children),
|
||||
|
||||
Reference in New Issue
Block a user