mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
🐛 fix: Propagate User Identity to Subagent MCP Tool Calls (#12950)
* 🐛 fix: Propagate User Identity to Subagent MCP Tool Calls The `@librechat/agents` SDK's `SubagentExecutor` invokes the child workflow with a fresh configurable of `{ thread_id }` only — the parent's `user` / `user_id` are dropped on the way into the child graph. The child's `ToolNode` then dispatches `ON_TOOL_EXECUTE` to the parent's handler, which merges `{ ...configurable, ...toolConfigurable }`, but neither side carries user identity for subagents. Downstream MCP tools read `config.configurable.user?.id || user_id` and got `undefined`, so `MCPManager.getConnection` fell through to the "No connection found for server X" error path — it can't reach the user-connection lookup without a userId. Re-inject `user` (via `createSafeUser`) and `user_id` from `req.user` into the configurable returned by `loadToolsForExecution`. This is the single point all controllers (chat, Responses API, OpenAI-compat) flow through. For the parent agent it's a no-op (outer config already carries the same values); for subagents it fills the gap so MCP connection lookup, user-placeholder substitution, and tools that read configurable.user all work correctly. * 🐛 fix: Preserve `api-user` Fallback When Injecting Subagent Identity Codex review pointed out that the prior commit unconditionally wrote `user_id: req.user?.id` (and `user`) into `toolConfigurable`. The handler merges via `{ ...configurable, ...toolConfigurable }` — `toolConfigurable` wins — so when `req.user` is absent, this overwrote the outer config's `'api-user'` fallback (set by `responses.js` / `openai.js` for the unauthenticated API-key path) with `undefined`, breaking MCP connection lookup for that path. Only inject the keys when `req.user.id` is truthy. Omitting them lets the merge preserve whatever the outer configurable already had. Tests updated to assert key omission for `req.user` undefined / null / present without `id`. * 🩹 fix: Narrow `IUser.id` to required string `IUser` extends mongoose `Document`, which types `id?: any` (the optional virtual). At runtime `id` is always `_id.toString()` for a hydrated doc, so narrow the type to a required string. Closes two `@rollup/plugin-typescript` TS2322 warnings introduced by PR #12450 (OIDC Bearer Token Authentication for Remote Agent API) where `req.user = userResolution.user` and the `(req: Request, res: Response, next: NextFunction)` signature both failed against the project's local `Express.User` augmentation (`{ [key: string]: any; id: string; }`) because `IUser.id` was `any`/optional. Narrowing here fixes both at the source rather than casting at every assignment site. * 🩹 fix: Resolve TS Build Warnings Surfaced by `IUser.id` Narrowing Three rollup TS plugin warnings surfaced after narrowing `IUser.id` from `any` to `string`: - `utils/env.ts:95` — `safeUser[field] = user[field]` failed strict checking because indexed write through a union-typed key collapses the LHS to the intersection of all field write types (i.e., `undefined` when fields have mixed types). The previous `id?: any` on IUser had been masking this. Switch to `Object.assign(safeUser, { [field]: user[field] })` which widens the assignment. - `endpoints/google/initialize.ts:35` — `getUserKey({ userId: req.user?.id, ... })` failed because `req.user?.id` is now `string | undefined` (no longer `any`). Match the pattern already used in `endpoints/openAI/initialize.ts:49`: `req.user?.id ?? ''`. - `middleware/remoteAgentAuth.ts:465` — pre-existing, unrelated to the IUser change. The local (gitignored) `express.d.ts` augments `express.Request` but not `express-serve-static-core.Request`, so the explicit `(req: Request, ...)` annotation imported from `'express'` resolves to a Request whose `req.user` differs from the one `RequestHandler` expects internally. Type the closure as `RequestHandler` directly so TS infers params from the augmented type. * 🩹 fix: Cast `RemoteAgentAuth` Closure to `RequestHandler` My previous attempt removed the explicit `req: Request` annotation on the closure to side-step the outer `RequestHandler` mismatch. That shifted the error to every helper call site inside the closure (`getConfigOptions(req)`, `runApiKeyAuth(req, ...)` at 467/474/493/ 512/531), because the helpers annotate their params with `express.Request` (which has the local `Request.user` augmentation), while the unannotated closure inferred `req` as `express-serve-static-core.Request` (no augmentation). Reproduced locally by stubbing the gitignored `src/types/express.d.ts`. Right approach: keep the explicit `req: Request` annotation so the closure body matches the helpers' types, then cast at the return — `RequestHandler`'s internal `Request` resolves through `express-serve-static-core` and lacks the augmentation, so the cast is the boundary that bridges the two views of `req.user`. Verified against a build with the local express.d.ts stub: zero warnings on `remoteAgentAuth.ts`, `env.ts`, and `google/initialize.ts`.
This commit is contained in:
parent
f20419d0b7
commit
bff9bfea87
6 changed files with 138 additions and 3 deletions
|
|
@ -12,6 +12,7 @@ const {
|
|||
const {
|
||||
sendEvent,
|
||||
getToolkitKey,
|
||||
createSafeUser,
|
||||
getUserMCPAuthMap,
|
||||
loadToolDefinitions,
|
||||
GenerationJobManager,
|
||||
|
|
@ -1252,7 +1253,35 @@ async function loadToolsForExecution({
|
|||
}) {
|
||||
const appConfig = req.config;
|
||||
const allLoadedTools = [];
|
||||
/**
|
||||
* Re-inject user identity into the configurable returned to the
|
||||
* `ON_TOOL_EXECUTE` handler. The handler merges this onto whatever
|
||||
* configurable arrived with the dispatched event:
|
||||
* `{ ...configurable, ...toolConfigurable }`
|
||||
* For the parent agent the outer stream config (set in AgentClient /
|
||||
* Responses / OpenAI controllers) already carries `user` and `user_id`;
|
||||
* re-injecting the same values from `req.user` is a no-op there.
|
||||
*
|
||||
* For SUBAGENT tool calls the SDK's `SubagentExecutor` invokes the child
|
||||
* workflow with a fresh configurable of `{ thread_id }` only — the
|
||||
* parent's `user`, `user_id`, and `userMCPAuthMap` are dropped on the way
|
||||
* into the child graph. Without this re-injection, downstream MCP tools
|
||||
* read `config.configurable.user?.id || user_id === undefined` and
|
||||
* `MCPManager.getConnection` throws "No connection found for server X"
|
||||
* (it can't fall through to the user-connection path without a userId).
|
||||
*
|
||||
* Only set the keys when `req.user.id` is present. Because
|
||||
* `toolConfigurable` wins the merge, writing `user_id: undefined`
|
||||
* unconditionally would clobber the outer config's `'api-user'`
|
||||
* fallback (set by Responses/OpenAI controllers when `req.user` is
|
||||
* absent). Omitting the keys lets the merge preserve whatever the
|
||||
* outer configurable already had.
|
||||
*/
|
||||
const configurable = { userMCPAuthMap };
|
||||
if (req.user?.id) {
|
||||
configurable.user = createSafeUser(req.user);
|
||||
configurable.user_id = req.user.id;
|
||||
}
|
||||
|
||||
if (actionsEnabled === undefined) {
|
||||
const enabledCapabilities = await resolveAgentCapabilities(req, appConfig, agent?.id);
|
||||
|
|
|
|||
|
|
@ -363,6 +363,88 @@ describe('ToolService - Action Capability Gating', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('loadToolsForExecution — configurable user injection', () => {
|
||||
/**
|
||||
* Subagents invoked via `@librechat/agents`' SubagentExecutor receive
|
||||
* a fresh `configurable` of `{ thread_id }` only — the parent's
|
||||
* `user`/`user_id` are dropped on the way into the child workflow.
|
||||
* The handler in `packages/api/src/agents/handlers.ts` merges
|
||||
* `{ ...configurable, ...toolConfigurable }`, so the configurable
|
||||
* returned here MUST carry the user identity for MCP connection
|
||||
* lookup to succeed in subagent tool calls.
|
||||
*
|
||||
* Conversely, when `req.user` is absent (API-key path) we must NOT
|
||||
* write `user`/`user_id` keys, otherwise the merge would clobber the
|
||||
* outer config's `'api-user'` fallback set by Responses/OpenAI
|
||||
* controllers.
|
||||
*/
|
||||
it('returns configurable with user and user_id from req.user', async () => {
|
||||
const req = createMockReq([]);
|
||||
req.config = {};
|
||||
req.user = { id: 'user_abc', email: 'a@b.c', role: 'USER' };
|
||||
|
||||
const result = await loadToolsForExecution({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123' },
|
||||
toolNames: [],
|
||||
});
|
||||
|
||||
expect(result.configurable.user_id).toBe('user_abc');
|
||||
expect(result.configurable.user).toEqual(
|
||||
expect.objectContaining({ id: 'user_abc', email: 'a@b.c', role: 'USER' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('omits user keys when req.user is undefined so outer config fallback is preserved', async () => {
|
||||
const req = createMockReq([]);
|
||||
req.config = {};
|
||||
req.user = undefined;
|
||||
|
||||
const result = await loadToolsForExecution({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123' },
|
||||
toolNames: [],
|
||||
});
|
||||
|
||||
expect('user_id' in result.configurable).toBe(false);
|
||||
expect('user' in result.configurable).toBe(false);
|
||||
});
|
||||
|
||||
it('omits user keys when req.user is null so outer config fallback is preserved', async () => {
|
||||
const req = createMockReq([]);
|
||||
req.config = {};
|
||||
req.user = null;
|
||||
|
||||
const result = await loadToolsForExecution({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123' },
|
||||
toolNames: [],
|
||||
});
|
||||
|
||||
expect('user_id' in result.configurable).toBe(false);
|
||||
expect('user' in result.configurable).toBe(false);
|
||||
});
|
||||
|
||||
it('omits user keys when req.user is present but has no id', async () => {
|
||||
const req = createMockReq([]);
|
||||
req.config = {};
|
||||
req.user = { email: 'a@b.c' };
|
||||
|
||||
const result = await loadToolsForExecution({
|
||||
req,
|
||||
res: {},
|
||||
agent: { id: 'agent_123' },
|
||||
toolNames: [],
|
||||
});
|
||||
|
||||
expect('user_id' in result.configurable).toBe(false);
|
||||
expect('user' in result.configurable).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkCapability logic', () => {
|
||||
const createCheckCapability = (enabledCapabilities, logger = { warn: jest.fn() }) => {
|
||||
return (capability) => {
|
||||
|
|
|
|||
|
|
@ -32,7 +32,7 @@ export async function initializeGoogle({
|
|||
let userKey = null;
|
||||
if (expiresAt && isUserProvided) {
|
||||
checkUserKeyExpiry(expiresAt, EModelEndpoint.google);
|
||||
userKey = await db.getUserKey({ userId: req.user?.id, name: EModelEndpoint.google });
|
||||
userKey = await db.getUserKey({ userId: req.user?.id ?? '', name: EModelEndpoint.google });
|
||||
}
|
||||
|
||||
let serviceKey: Record<string, unknown> = {};
|
||||
|
|
|
|||
|
|
@ -462,7 +462,15 @@ export function createRemoteAgentAuth({
|
|||
updateUser,
|
||||
getAppConfig,
|
||||
}: RemoteAgentAuthDeps): RequestHandler {
|
||||
return async (req: Request, res: Response, next: NextFunction) => {
|
||||
/**
|
||||
* Annotated as `express.Request` (and helpers below take the same type)
|
||||
* so the local `Request.user` augmentation in `src/types/express.d.ts`
|
||||
* applies inside the closure. The closure is then cast to
|
||||
* `RequestHandler` at the return — `RequestHandler`'s internal
|
||||
* `Request` resolves through `express-serve-static-core` and lacks the
|
||||
* augmentation, so a direct return would mismatch on `user`.
|
||||
*/
|
||||
const handler = async (req: Request, res: Response, next: NextFunction) => {
|
||||
try {
|
||||
const initialConfigOptions = getConfigOptions(req);
|
||||
const config = await getAppConfig(initialConfigOptions);
|
||||
|
|
@ -559,4 +567,5 @@ export function createRemoteAgentAuth({
|
|||
return;
|
||||
}
|
||||
};
|
||||
return handler as RequestHandler;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -92,7 +92,14 @@ export function createSafeUser(
|
|||
const safeUser: Partial<SafeUser> & { federatedTokens?: IUser['federatedTokens'] } = {};
|
||||
for (const field of ALLOWED_USER_FIELDS) {
|
||||
if (field in user) {
|
||||
safeUser[field] = user[field];
|
||||
/**
|
||||
* Indexed write through a union-typed key would otherwise fail strict
|
||||
* checking — TS computes the LHS type as the *intersection* of all
|
||||
* field write types (which collapses to `undefined` when fields have
|
||||
* mixed types). `Object.assign` widens the assignment so each field
|
||||
* preserves its concrete type at runtime.
|
||||
*/
|
||||
Object.assign(safeUser, { [field]: user[field] });
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,14 @@ import { CursorPaginationParams } from '~/common';
|
|||
|
||||
export interface IUser extends Document {
|
||||
_id: Types.ObjectId;
|
||||
/**
|
||||
* Mongoose's `Document.id` virtual is typed `id?: any`. At runtime it's
|
||||
* always `_id.toString()` for a hydrated doc, so narrow to a required
|
||||
* string. This also lets `IUser` satisfy Express.User augmentations
|
||||
* (the OIDC remote-agent middleware assigns `req.user = IUser` where
|
||||
* the project's local `Express.User` requires `id: string`).
|
||||
*/
|
||||
id: string;
|
||||
name?: string;
|
||||
username?: string;
|
||||
email: string;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue