feat: update assistants page to only show qualified actions #5951
Conversation
|
Someone is attempting to deploy a commit to the Danswer Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR attempts to restrict tool visibility in the assistant editor to only show tools that are public, owned by the user, or accessible to admins. The implementation adds permission filtering to the MemoizedToolList component and passes user context from AssistantEditor.
Critical Issues:
- The
is_publicproperty does not exist in the backendToolSnapshotmodel (backend/onyx/server/features/tool/models.py:8-20), causing the filter logic on line 60 to always fail - All references to
tool.is_publicwill beundefined, breaking the intended permission model entirely - MCP server tools are not filtered at all - they bypass permission checks completely since
MCPServerSectionrenders them directly without filtering - The
MemoizedMCPServerToolscomponent is missing the requiredisPublicprop, causing a type error
Root Cause:
The frontend assumes tools have an is_public field, but the backend never returns this property. Either the backend needs to add this field to ToolSnapshot, or the frontend needs a different approach to determine tool visibility.
Confidence Score: 0/5
- This PR has critical bugs that completely break the intended functionality and will likely cause runtime errors
- The core feature relies on a non-existent
is_publicproperty from the backend API, causing the permission filtering to fail entirely. Additionally, MCP tools are not filtered at all, and there's a missing required prop that will cause TypeScript errors. The feature cannot work as intended without backend changes. - Pay close attention to
web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx- requires coordination with backend to addis_publicfield toToolSnapshotmodel
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx | 0/5 | Added permission filtering logic for tools, but references non-existent is_public property causing complete feature breakage. MCP tools lack permission filtering entirely. |
| web/src/app/admin/assistants/AssistantEditor.tsx | 3/5 | Passes user prop to MemoizedToolList component to enable permission checks - minor change with no direct issues. |
Sequence Diagram
sequenceDiagram
participant User
participant AssistantEditor
participant MemoizedToolList
participant MemoizedToolCheckbox
participant Backend API
User->>AssistantEditor: Load assistant editor page
AssistantEditor->>Backend API: Fetch tools
Backend API-->>AssistantEditor: Return ToolSnapshot[] (no is_public field)
AssistantEditor->>MemoizedToolList: Pass customTools + user
MemoizedToolList->>MemoizedToolList: Filter tools by is_public/role/owner
Note over MemoizedToolList: BUG: tool.is_public is undefined<br/>Filter always excludes non-admin/non-owner tools
MemoizedToolList->>MemoizedToolCheckbox: Render filtered tools
MemoizedToolCheckbox->>MemoizedToolCheckbox: Check isDisabled = !isOwner && !isAdmin && !isPublic
Note over MemoizedToolCheckbox: isPublic is always false/undefined<br/>Non-owner/non-admin tools incorrectly disabled
MemoizedToolCheckbox-->>User: Display tools (with wrong permissions)
Additional Comments (1)
-
web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx, line 85-108 (link)logic: MCP server tools are not filtered by permissions (unlike custom tools on lines 58-63), and this component doesn't appear to be used anywhere. The
MCPServerSectioncomponent inFormSections.tsx:160-218renders MCP tools directly without permission filtering. MCP tools should also respect the same permission model.
2 files reviewed, 4 comments
| {tools | ||
| .filter( | ||
| (tool) => | ||
| tool.is_public === true || |
There was a problem hiding this comment.
logic: The is_public property does not exist on ToolSnapshot type. Backend model at backend/onyx/server/features/tool/models.py:8-20 shows ToolSnapshot only has: id, name, description, definition, display_name, in_code_tool_id, custom_headers, passthrough_auth, mcp_server_id, user_id, oauth_config_id, oauth_config_name. This filter will always evaluate to false for tool.is_public === true, breaking the permission logic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx
Line: 60:60
Comment:
**logic:** The `is_public` property does not exist on `ToolSnapshot` type. Backend model at `backend/onyx/server/features/tool/models.py:8-20` shows `ToolSnapshot` only has: id, name, description, definition, display_name, in_code_tool_id, custom_headers, passthrough_auth, mcp_server_id, user_id, oauth_config_id, oauth_config_name. This filter will always evaluate to false for `tool.is_public === true`, breaking the permission logic.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| isOwner={tool.user_id === user?.id} | ||
| isAdmin={user?.role === UserRole.ADMIN} | ||
| isPublic={tool.is_public === true} |
There was a problem hiding this comment.
logic: Same issue: tool.is_public doesn't exist in the backend API response, so this will always be undefined.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx
Line: 77:77
Comment:
**logic:** Same issue: `tool.is_public` doesn't exist in the backend API response, so this will always be `undefined`.
How can I resolve this? If you propose a fix, please make it concise.| description={tool.description} | ||
| isOwner={tool.user_id === user?.id} | ||
| isAdmin={user?.role === UserRole.ADMIN} | ||
| /> |
There was a problem hiding this comment.
syntax: Missing required isPublic prop for MemoizedToolCheckbox (defined on line 25). This will cause a TypeScript error and runtime issues.
| /> | |
| isPublic={tool.is_public === true} |
However, note that tool.is_public doesn't exist in the backend (see other comments), so this entire approach needs reconsidering.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/components/admin/assistants/MemoizedToolCheckboxes.tsx
Line: 104:104
Comment:
**syntax:** Missing required `isPublic` prop for `MemoizedToolCheckbox` (defined on line 25). This will cause a TypeScript error and runtime issues.
```suggestion
isPublic={tool.is_public === true}
```
However, note that `tool.is_public` doesn't exist in the backend (see other comments), so this entire approach needs reconsidering.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
trial2onyx
changed the title
1 participant
Description
Now we will only show an action in the assistant create page if 1, action is public, or 2, the user is admin or 3, the user is the action's owner
attachment 1 is from a normal users. attachment 2 is from a curator and attachment 3 is from admin.
How Has This Been Tested?
[Describe the tests you ran to verify your changes]
Additional Options
Summary by cubic
Limit the Assistants page to show only actions a user can use: public actions, the user’s own, or all actions for admins. This enforces permissions and reduces noise in the create flow.