mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(ai-chat): hide other users' MCP servers from the chat unless shared (#11112)
* fix(ai-chat): hide other users' MCP servers from the chat unless shared An admin's database role lets the resource listing return every user's u/ MCP resource, so the chat's "+" menu and the assistant settings tab offered servers that carry someone else's credentials. Both lists, and the tool loader behind them, now keep a u/ server only when it belongs to the current user or its extra_perms name them or one of their groups. The Resources page is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC * fix(ai-chat): resolve the MCP viewer for the workspace being listed Username and groups are per workspace, and a session chat can operate on a workspace other than the one being browsed, so the filter now takes its identity from that workspace's whoami rather than from userStore. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC * fix(ai-chat): list the whole MCP catalog before filtering, one predicate The visibility filter runs after the server's LIMIT, so a 100-row page could drop the viewer's own servers behind foreign u/ rows. The three call sites now ask for 1000 and call the tested predicate directly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131DsyTiBR6Q54qPXbTX5sC --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
56dd940e34
commit
244ec13291
@@ -14,6 +14,11 @@ switch that decides whether this chat carries its tools.
|
||||
import DropdownV2 from '$lib/components/DropdownV2.svelte'
|
||||
import Toggle from '$lib/components/Toggle.svelte'
|
||||
import { isMcpEnabled, setMcpEnabled } from '$lib/components/mcp/enabledServers'
|
||||
import {
|
||||
isOwnOrSharedMcpPath,
|
||||
MCP_LIST_PER_PAGE,
|
||||
mcpViewer
|
||||
} from '$lib/components/mcp/ownServers'
|
||||
import { loadProviderIcon } from '$lib/components/mcp/providerIcon'
|
||||
import {
|
||||
cachedProviderKey,
|
||||
@@ -275,11 +280,15 @@ switch that decides whether this chat carries its tools.
|
||||
loading = true
|
||||
loadError = undefined
|
||||
try {
|
||||
const resources = await ResourceService.listResource({
|
||||
workspace: target,
|
||||
resourceType: 'mcp',
|
||||
perPage: 100
|
||||
})
|
||||
const [listed, viewer] = await Promise.all([
|
||||
ResourceService.listResource({
|
||||
workspace: target,
|
||||
resourceType: 'mcp',
|
||||
perPage: MCP_LIST_PER_PAGE
|
||||
}),
|
||||
mcpViewer(target)
|
||||
])
|
||||
const resources = listed.filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer))
|
||||
if (seq !== loadSeq) return
|
||||
servers = resources.map((r) => ({
|
||||
path: r.path,
|
||||
|
||||
@@ -4,7 +4,7 @@ const { getMcpToolsMock, callMcpToolMock, listResourceMock, session } = vi.hoist
|
||||
getMcpToolsMock: vi.fn(),
|
||||
callMcpToolMock: vi.fn(),
|
||||
listResourceMock: vi.fn(),
|
||||
session: { email: 'first@windmill.dev' }
|
||||
session: { email: 'first@windmill.dev', workspace_id: 'test-ws', username: 'hugo', pgroups: [] }
|
||||
}))
|
||||
|
||||
vi.mock('../shared', () => ({
|
||||
|
||||
@@ -2,6 +2,7 @@ import { z } from 'zod'
|
||||
import { ResourceService, type GetMcpToolsResponse } from '$lib/gen'
|
||||
import { createToolDef, type Tool } from '../shared'
|
||||
import { enabledMcpPaths } from '$lib/components/mcp/enabledServers'
|
||||
import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from '$lib/components/mcp/ownServers'
|
||||
|
||||
/**
|
||||
* Access to the MCP servers the user has connected (resources of type `mcp`)
|
||||
@@ -91,13 +92,18 @@ export async function loadMcpServers(workspace: string): Promise<McpServer[]> {
|
||||
const enabled = enabledMcpPaths(workspace)
|
||||
if (enabled.length === 0) return []
|
||||
try {
|
||||
const resources = await ResourceService.listResource({
|
||||
workspace,
|
||||
resourceType: 'mcp',
|
||||
perPage: 100
|
||||
})
|
||||
const [resources, viewer] = await Promise.all([
|
||||
ResourceService.listResource({
|
||||
workspace,
|
||||
resourceType: 'mcp',
|
||||
perPage: MCP_LIST_PER_PAGE
|
||||
}),
|
||||
mcpViewer(workspace)
|
||||
])
|
||||
return resources
|
||||
.filter((r) => enabled.includes(r.path))
|
||||
.filter(
|
||||
(r) => enabled.includes(r.path) && isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer)
|
||||
)
|
||||
.map((r) => ({ path: r.path, editedAt: r.edited_at }))
|
||||
} catch (e) {
|
||||
console.error('Failed to load MCP servers', e)
|
||||
|
||||
@@ -8,6 +8,7 @@ import type { Item } from '$lib/utils'
|
||||
import type { AIChatManager } from '../copilot/chat/AIChatManager.svelte'
|
||||
import { isMcpEnabled, setMcpEnabled } from './enabledServers'
|
||||
import { cachedProviderKey, rememberProviderKey } from './iconCache'
|
||||
import { isOwnOrSharedMcpPath, MCP_LIST_PER_PAGE, mcpViewer } from './ownServers'
|
||||
import { loadProviderIcon } from './providerIcon'
|
||||
import McpServerIcon from './McpServerIcon.svelte'
|
||||
|
||||
@@ -56,17 +57,22 @@ export class McpMenu {
|
||||
async #load(ws: string) {
|
||||
const seq = ++this.#seq
|
||||
try {
|
||||
const resources = await ResourceService.listResource({
|
||||
workspace: ws,
|
||||
resourceType: 'mcp',
|
||||
perPage: 100
|
||||
})
|
||||
const [resources, viewer] = await Promise.all([
|
||||
ResourceService.listResource({
|
||||
workspace: ws,
|
||||
resourceType: 'mcp',
|
||||
perPage: MCP_LIST_PER_PAGE
|
||||
}),
|
||||
mcpViewer(ws)
|
||||
])
|
||||
if (seq !== this.#seq) return
|
||||
this.#rows = resources.map((r) => ({
|
||||
path: r.path,
|
||||
editedAt: r.edited_at,
|
||||
enabled: isMcpEnabled(ws, r.path)
|
||||
}))
|
||||
this.#rows = resources
|
||||
.filter((r) => isOwnOrSharedMcpPath(r.path, r.extra_perms, viewer))
|
||||
.map((r) => ({
|
||||
path: r.path,
|
||||
editedAt: r.edited_at,
|
||||
enabled: isMcpEnabled(ws, r.path)
|
||||
}))
|
||||
this.#rowsWorkspace = ws
|
||||
void this.#loadIcons(ws, seq)
|
||||
} catch {
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const { session, roleMock } = vi.hoisted(() => ({
|
||||
session: { workspace_id: 'browsed', username: 'alice', pgroups: ['g/browsed-only'] },
|
||||
roleMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('$lib/stores', () => ({
|
||||
userStore: { subscribe: (run: (v: unknown) => void) => (run({ ...session }), () => {}) }
|
||||
}))
|
||||
vi.mock('$lib/user', () => ({ getWorkspaceRole: roleMock }))
|
||||
|
||||
import { isOwnOrSharedMcpPath, mcpViewer } from './ownServers'
|
||||
|
||||
const alice = { username: 'alice', pgroups: ['g/eng'] }
|
||||
|
||||
describe('isOwnOrSharedMcpPath', () => {
|
||||
it('keeps own, folder, and explicitly shared servers; drops the rest of u/', () => {
|
||||
expect(isOwnOrSharedMcpPath('u/alice/notion', {}, alice)).toBe(true)
|
||||
expect(isOwnOrSharedMcpPath('f/team/notion', {}, alice)).toBe(true)
|
||||
expect(isOwnOrSharedMcpPath('u/bob/notion', {}, alice)).toBe(false)
|
||||
expect(isOwnOrSharedMcpPath('u/alicex/notion', {}, alice)).toBe(false)
|
||||
expect(isOwnOrSharedMcpPath('u/bob/notion', { 'u/alice': false }, alice)).toBe(true)
|
||||
expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/eng': false }, alice)).toBe(true)
|
||||
expect(isOwnOrSharedMcpPath('u/bob/notion', { 'g/ops': true }, alice)).toBe(false)
|
||||
expect(isOwnOrSharedMcpPath('u/alice/notion', {}, { username: undefined, pgroups: [] })).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('mcpViewer', () => {
|
||||
beforeEach(() => roleMock.mockReset())
|
||||
|
||||
it('answers from the store only for the browsed workspace', async () => {
|
||||
expect(await mcpViewer('browsed')).toEqual({ username: 'alice', pgroups: ['g/browsed-only'] })
|
||||
expect(roleMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('looks the identity up for another workspace instead of reusing the browsed one', async () => {
|
||||
roleMock.mockResolvedValue({
|
||||
kind: 'resolved',
|
||||
user: { username: 'alice_other', pgroups: ['g/eng'] }
|
||||
})
|
||||
expect(await mcpViewer('other')).toEqual({ username: 'alice_other', pgroups: ['g/eng'] })
|
||||
expect(roleMock).toHaveBeenCalledWith('other')
|
||||
})
|
||||
|
||||
it('yields no identity when the lookup fails', async () => {
|
||||
roleMock.mockResolvedValue({ kind: 'lookup_failed' })
|
||||
expect(await mcpViewer('other')).toEqual({ username: undefined, pgroups: [] })
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,48 @@
|
||||
import { get } from 'svelte/store'
|
||||
import { userStore } from '$lib/stores'
|
||||
import { getWorkspaceRole } from '$lib/user'
|
||||
|
||||
/** The principals an `extra_perms` entry can name to grant this user access. */
|
||||
export type McpViewer = { username: string | undefined; pgroups: string[] }
|
||||
|
||||
/** Wide enough that a workspace's whole MCP catalog arrives at once: the filter
|
||||
* below runs after the server's LIMIT, so a short page could drop the viewer's
|
||||
* own rows while foreign `u/` rows fill it. */
|
||||
export const MCP_LIST_PER_PAGE = 1000
|
||||
|
||||
/**
|
||||
* Whether the chat lists this MCP server. A server under another user's `u/`
|
||||
* prefix carries that person's credentials, and an admin's database role lets
|
||||
* the resource listing return it. Acting through it would call the provider
|
||||
* as them, so the chat only offers it when its owner shared it explicitly.
|
||||
*/
|
||||
export function isOwnOrSharedMcpPath(
|
||||
path: string,
|
||||
extraPerms: Record<string, unknown> | undefined,
|
||||
viewer: McpViewer
|
||||
): boolean {
|
||||
if (!path.startsWith('u/')) return true
|
||||
if (viewer.username !== undefined && path.startsWith(`u/${viewer.username}/`)) return true
|
||||
if (!extraPerms) return false
|
||||
return (
|
||||
(viewer.username !== undefined && `u/${viewer.username}` in extraPerms) ||
|
||||
viewer.pgroups.some((g) => g in extraPerms)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* The viewer's identity in `workspace`. Username and groups are per workspace,
|
||||
* and a session chat can operate on a workspace other than the one being
|
||||
* browsed, so `userStore` only answers when it describes that same workspace.
|
||||
* A failed lookup yields no username: every `u/` server is then hidden rather
|
||||
* than judged against another workspace's identity.
|
||||
*/
|
||||
export async function mcpViewer(workspace: string): Promise<McpViewer> {
|
||||
const u = get(userStore)
|
||||
if (u?.workspace_id === workspace) return { username: u.username, pgroups: u.pgroups ?? [] }
|
||||
const role = await getWorkspaceRole(workspace)
|
||||
if (role.kind === 'resolved') {
|
||||
return { username: role.user.username, pgroups: role.user.pgroups ?? [] }
|
||||
}
|
||||
return { username: undefined, pgroups: [] }
|
||||
}
|
||||
Reference in New Issue
Block a user