feat(mcp): enforce per-user MCP tool-call entitlements in the auth module - #35146
Conversation
|
|
|
Companion docs PR: BerriAI/litellm-docs#710 (the permission-hierarchy page currently says "five distinct levels"; this adds the sixth) |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request adds per-user entitlement enforcement for MCP tool calls in the authentication module and updates the dashboard’s user access management behavior. One issue has been addressed, but an authorization gap remains when admins remove access granted indirectly through an access group or toolset. Stale server permissions can survive the removal, allowing the affected user to continue invoking tools on a server they should no longer be entitled to access. Open issues (1)
Fixed/addressed: 1 · PR risk: 7/10 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe PR adds per-user MCP entitlements across authorization, user-management APIs, cache invalidation, and the dashboard.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains from the previous review threads; the entitlement clear, self-update protection, cache invalidation, and cache-protocol fixes are present at the current head.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Adds cached internal-user entitlement resolution and applies user-level server and tool ceilings; the previously disputed lookup behavior remains intentional and no eligible defect was found. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Ensures admin-role shortcuts do not bypass an explicit internal-user MCP entitlement. |
| litellm/proxy/management_endpoints/internal_user_endpoints.py | Persists and clears user entitlements, protects them from self-update, and independently invalidates all authorization cache keys required by the prior review. |
| litellm/proxy/common_utils/user_api_key_cache.py | Co-locates the user-entitlement negative-cache sentinel with its cache-key builders, consistently centralizing the protocol requested previously. |
| ui/litellm-dashboard/src/app/(dashboard)/users/_components/user_edit_view.tsx | Adds user-facing MCP entitlement editing to the internal-user dashboard flow. |
| ui/litellm-dashboard/src/app/(dashboard)/users/_components/view_users/user_info_view.tsx | Displays persisted MCP entitlement information on the internal-user details page. |
Reviews (7): Last reviewed commit: "feat(mcp): enforce per-user MCP tool-cal..." | Re-trigger Greptile
c35d2eb to
64fc80c
Compare
|
@greptileai please review the current head Three of the four findings are fixed in that commit, each answered on its own thread:
The remaining P1 ("entitlement lookup fails open") I have pushed back on with the four sibling call sites that already behave identically for key auth, and filed as LIT-4960 for one deliberate decision across all five levels. Detail is on that thread. |
| // would delete its allowlist on every save and silently widen the human to every tool on it. | ||
| mcp_tool_permissions: Object.fromEntries( | ||
| Object.entries(asToolPermissions(formValues.mcp_tool_permissions)).filter( | ||
| ([serverId]) => !deselectedServerIds.has(serverId), |
There was a problem hiding this comment.
Medium: Removing indirect grants can preserve server access
When an admin removes an access group or toolset, its servers are absent from loadedDirectServers, so their old tool-permission entries survive this filter. The backend treats every server named in mcp_tool_permissions as independently entitled, allowing the user to continue invoking the removed server. Track which servers were supplied by the removed indirect grant and delete their permissions unless another selected source still grants them.
There was a problem hiding this comment.
Correct, and this is the converse of the Medium you raised last round, which is worth stating plainly because the two cannot both be fixed in this component.
The previous finding was that filtering by the selected servers DELETES the allowlist of an indirectly-granted server, widening the human to every tool on it. This one is that NOT deleting it leaves a stale entry that keeps a removed server reachable. Both are true, and they are the same underlying problem seen from two sides: this editor only ever renders the DIRECT server list, so it cannot know which servers a given access group or toolset supplies. Whichever way the filter is written, the case it cannot see is wrong.
Your suggested remedy needs that knowledge client-side ("track which servers were supplied by the removed indirect grant"), which means resolving group and toolset membership in the browser, and then deciding what to do when that resolution fails; a failed resolution either revokes a grant the admin never touched or leaves the stale entry anyway.
Pruning on the backend is not available either, because it contradicts a deliberate semantic: a server named only under mcp_tool_permissions is entitled on purpose, so granting one tool never requires naming its server twice. That holds for keys, teams and orgs as well as users (all_servers = direct_mcp_servers + access_group_servers + tool_perm_servers at four call sites in user_api_key_auth_mcp.py), and there is a test pinning it. A backend rule that dropped entries for servers not otherwise granted would delete exactly those intentional grants.
So of the two wrong behaviors, this PR takes the one that cannot escalate: a save never widens a grant, and an entry the editor cannot see is left alone rather than destroyed. A stale entry keeps a server reachable with the admin's own tool restriction still attached to it, which is a narrower outcome than the alternative of removing that restriction entirely.
The real fix is to make the editor able to see indirect grants, so the matrix renders servers reached through groups and toolsets and the admin can clear them like any other. That is a change to the shared MCPToolPermissions component affecting the key, team, organization and agent surfaces identically, so it belongs in its own PR; filed as LIT-4962 with both directions and this reasoning. Not resolving this thread, so the tradeoff stays visible to a reviewer.
There was a problem hiding this comment.
Thanks for the detail. I can't automatically confirm this is safe to dismiss, so I'm leaving the thread open for a maintainer to make the call.
There was a problem hiding this comment.
Reopening my own reply: I was wrong, and your remedy did not need what I said it needed.
I claimed the two directions could not both be fixed in this component, on the grounds that deciding correctly requires resolving group and toolset membership in the browser. That is only true for the PARTIAL case. There is a decision the save can make without any resolution at all, because "the admin retained no indirect grant source" is knowable from the form alone: if mcp_access_groups and mcp_toolsets are both empty, then nothing indirect supplies anything, so every entry whose server is not directly selected is stale and gets dropped. That is exactly the revocation you asked for, and it was available the whole time.
Worse, my filter did not even hold the line I claimed for it. I said "a save never widens", but retaining is the widening direction here: an entry under mcp_tool_permissions is itself a grant, so the save that removed the access group left the server reachable. Revocation through this editor was a no-op, which is the finding you filed.
The rule now is: keep an entry if the server is still directly selected, or if an access group or toolset the admin retained could still supply it; drop it otherwise. Both directions are pinned, and the revocation test fails against the previous filter:
× drops the tool allowlist of a server the admin just deselected
→ expected { 'srv-1': ['read'], …(1) } to deeply equal { 'srv-1': ['read'] }
× drops the allowlist of a server reached only through an access group the admin removed
→ expected { 'srv-via-group': ['read'] } to deeply equal {}
✓ keeps the allowlist of a server granted through a retained access group
✓ keeps the allowlist of a deselected server that a retained access group still supplies
✓ keeps the allowlist of a deselected server when a toolset is retained
One residue is left, and I would rather name it than claim it away this time. With two access groups retained and one removed, the save still cannot tell which of them supplied a given server, so the entry stays until an admin clears it. It errs toward keeping the admin's own restriction attached rather than dropping it, and it is visible rather than silent once the matrix renders the effective set, which is #35154 on the shared component. This page inherits that fix when it lands.
|
@greptileai please review the current head A review bot on the sibling team-surface PR (#35153) found a real hole in the same rule this PR introduced, so I have fixed it here rather than leaving it for a follow-up. A server can be granted both directly and through an access group or a toolset. Deselecting the direct grant while the group is retained matched "was in the loaded direct list, is no longer selected", so the allowlist was dropped, and because the group still supplies the server an absent allowlist reads as no restriction. That is the same widening this level exists to prevent, reintroduced on the overlapping-grant path. Dropping an allowlist is now gated on the human retaining no indirect grant source at all, which is the only case where a deselection provably removes reachability. Two tests cover it (a retained access group, and a retained toolset); removing the guard fails exactly those two and nothing else. Worth noting how this was caught: two independent implementations of the rule, mine and the team one, agreed with each other and both had the hole, because both reasoned from the direct server list alone. Agreement between implementations was weaker evidence than it looked. |
|
@greptileai please review the current head The P1 was right and it found a second, worse problem behind it. Your finding: an explicit empty The part your finding led me to, which is the more serious half: the same filter hid the field from the non-admin self-update guard. That guard tests Two tests, each mutation-checked: reverting the guard fails the non-admin test, and removing the clear path fails the admin test. 91 pass in the mapped file. Both lint delta gates OK. |
8236c7e to
0e1e7a4
Compare
0e1e7a4 to
6f9215d
Compare
…dule The MCP gateway resolved a caller's allowed servers and per-server tool allowlists from the key, the team, the end user and the agent, but never from the internal user row, so an admin had no way to bound what a person may call across every key they hold. Anything the key allowed went through The internal user now carries the same object_permission an admin already attaches to a key or a team, and the resolver applies it as a ceiling: the caller ends up with the intersection of what the key allows and what the user allows, so adding a user entitlement can only narrow, never widen. A level that names no server and no tool places no ceiling, which keeps every existing deployment on its current behavior /user/new and /user/update accept object_permission and reuse the same create-or-update helper the team endpoints use, so the row is written once and the three cached views of it (the user row, the object-permission link and the permission itself) are invalidated on write. Clearing it with an empty object now really unlinks the permission instead of being swallowed as an empty value A row that cannot be read at all places no ceiling, but a row that names a permission the database cannot return denies the call rather than falling through to the wider set, so a partial outage cannot hand out access the admin withheld The users page grows the MCP servers, access groups, toolsets and per-server tool pickers the key and team pages already have. A save keeps a tool allowlist whenever an access group or toolset the admin retained could still supply that server, since an allowlist is what narrows a grant and an absent one reads as no restriction; it drops the allowlist once nothing indirect survives to supply the server, so removing a grant really removes it
6f9215d to
5c8b017
Compare
3 participants
TLDR
Problem this solves:
/user/newand/user/updatesilently dropped theobject_permissionthey document/user/updateactually returned a raw prisma 400 for it/v2/user/infonever returned a user's entitlement, so no UI could show itHow it solves it:
object_permissionbecomes an MCP entitlement leveltools/listtime and attools/calltime/user/newand/user/updatenow persist it;/v2/user/inforeturns itRelevant issues
Linear ticket
Resolves LIT-4936
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy on a real Postgres, against a real MCP server (a three-tool issue tracker:
list_issuesread-only,create_issuewrite,delete_repodestructive). Two proxies on the same database so the only variable is the code: port 4938 runslitellm_internal_staging(440b1bcf65), port 4936 runs this branch. The before/after below was captured atc35d2ebb9eand re-verified unchanged at64fc80c235after the review fixes; every assertion in the API sections was re-run against that head.aliceis an internal user whose key grants the wholeissue_trackerserver; her own entitlement grants onlylist_issues. That is the shape the ticket asks for: the credential reaches a server, the human is entitled to a subset of its tools.Before, on litellm_internal_staging
The documented
object_permissionparameter is dropped by/user/newand
/user/updaterejects it outrightSeeding the entitlement straight into the database (what an admin would have set, if the API could store it)
the entitlement is still ignored at list time and at call time
After, on this branch
Same request the base branch 400'd on now persists the grants and links them
and
/v2/user/inforeturns them, which is what the dashboard readsThe same key that reached every tool a moment ago is now bound by the human's entitlement, at list time
and at tool-call time, which is the part the ticket asks for
while the tool she is entitled to is untouched
Nothing changes for a human with no entitlement
bobis an internal user with an identically scoped key and no entitlement of his own, so he places no ceiling and keeps every tool. This is the compatibility case: an existing deployment sees no behavior change until an admin grants someone an entitlementA grant and a revoke both take effect on the next call
An entitlement change makes three cache entries stale, so this is the one that would silently "work" while serving revoked access for a TTL. Granting and then revoking
delete_repowith no restart and no waiting, calling immediately after each writeThe dashboard, and what a checkbox there does on the wire
Internal Users -> alice -> Details now carries an MCP Permissions section, and Edit Settings opens the same server-plus-tool picker the key and team pages use, grouped by risk
The read view shows what the human is entitled to, per server
The whole flow, ending with
create_issuerevoked and the count dropping to one toolTicking
create_issueunder Create (Medium Risk) and saving, then asking the gateway what alice may now do, with no restartthen unticking it again and asking once more
A user cannot lift their own ceiling
Alice's own key is refused at the route layer, and her entitlement is intact afterwards. The field-level guard added here is the second gate, for callers that do reach
/user/updateType
🆕 New Feature
Changes
An MCP entitlement could already be attached to a virtual key, a team, an organization, an agent or a customer. The one principal that could not carry one was the human.
MCPRequestHandler.get_allowed_mcp_serversandget_allowed_tools_for_serverresolved key, team, end user, agent and org and never looked at the internal user's ownobject_permission, so there was no source of truth for "which people may perform which MCP tool calls" and nothing to read at tool-call time.The management side was worse than absent. Both
/user/newand/user/updatedocument anobject_permissionparameter, butgenerate_key_helper_fnonly ever forwardsobject_permission_id, so/user/newdropped the grants on the floor, and/user/updatepassed the nested object straight into the user-table write and came back with a prisma error as a 400./v2/user/inforeturned no entitlement at all, so even a correct write would have been invisible to the dashboard.The internal user now resolves as a ceiling. On the servers axis
_apply_user_server_ceilingintersects the resolved list with what the human is entitled to, and reports whether it narrowed anything so the org step still knows it may only cap a lower-level restriction rather than replace one. On the tools axis_apply_user_tool_ceilingintersects per server, and becomes the allowlist when no lower level restricts. Both run before the agent and org ceilings, and both are reached frompre_call_tool_check, so a client that hardcodes a tool name is refused with a 403 rather than merely not being told the tool exists.A human with no entitlement places no ceiling, so an existing deployment sees no change until an admin grants someone one. The two fault classes are deliberately not collapsed into that: a user row we cannot read leaves us unable to say whether the person is entitled at all, which is the state that existed before this level, so it places no ceiling; a row that names a permission we cannot read is a known entitlement with unknown contents, so it denies. The
user_id -> object_permission_idlink is cached with a sentinel for "no entitlement", exactly as the agent path does, so a human without one costs no DB read per MCP request and a warm request with one costs none either.The keyless gateway-admitted path is untouched. There a human's grants are already one source of a union across their own grants plus every team they are on, so re-applying them as a per-source ceiling would let one team's narrower scope silently bound another's; both ceilings skip when
keyless_sourceis set, and two tests pin that.Two smaller consequences worth calling out. An admin-role caller with no explicit key-level
mcp_serverslist normally short-circuits to the whole server registry; that shortcut now also checks whether the human places a ceiling, because an entitlement is the person's scope and a role is not a waiver of it. And/user/updateinvalidates the cached views of an entitlement after any change to it: the permission row under its own id, the user-to-permission link, and the cached user row. Leaving any behind means an admin revoking a tool keeps serving it until the management-object TTL expires. Two cases make the naive version of this wrong, and both are pinned by tests: a CLEAR writes no permission id at all, so keying the invalidation off the written value skips it entirely, and an upsert can mint a new row, which leaves the outgoing one cached under its id still holding the pre-update grants. So invalidation fires on the field being present and drops both the old and the new id.Two behaviors worth naming because a reviewer will reasonably ask. The user level fails CLOSED on an entitlement it cannot read, while the org level fails OPEN for key auth in the same condition (
_apply_primary_org_ceiling,_apply_agent_and_org_tool_ceilings). That asymmetry is deliberate: the org ceiling is one bound among several a key already carries, whereas a user entitlement exists only to bound, so dropping one we know exists is the exact silent widening the level is there to prevent. Both directions are covered by tests. And a toolset-scoped request (server.pymodel_copy(update={"object_permission": ...})) keeps itsuser_id, so it is now capped by the human's entitlement too, which is the intended reading of "the entitlement is the person's".A one-line fix rides along in
MCPServerPermissions.getMCPServerDisplayName: it interpolatedserverDetail.aliaswith no fallback, so any server without an alias rendered asnull (a23...f039). That is pre-existing and shared with the key, team, organization and agent surfaces, but it is the label of the widget this PR puts on a new page, so shipping it readingnullwas not an option. The related gap where the tool matrix renders nothing for a name-keyed grant is filed separately rather than fixed here.The tool-permission editor renders only the DIRECTLY selected servers, so it cannot show a server reached through an access group or a toolset, and a save has to decide what to do with an allowlist for a server it cannot show. An allowlist is the thing that narrows a grant and an absent one reads as no restriction, so dropping one is the direction that widens. Three rules follow, and each of the first two was a bug a review bot caught before this got the third. An entry is kept whenever an access group or toolset the admin retained could still supply its server. Once nothing indirect survives, an entry whose server is no longer selected is dropped, so removing a grant really removes it; a server named only under
mcp_tool_permissionsis entitled on purpose, so leaving that entry behind would keep a removed server reachable no matter what the admin unticked. And a key is resolved to its server through the server list before being compared, because the gateway accepts a server id, name or alias as amcp_tool_permissionskey and normalizes all three (expand_tool_permissions), so an entry written by the API or by config may use any of them; comparing keys against the selector's ids alone dropped a name- or alias-keyed entry for a server that was still granted, which is the same widening on the surface meant to fix it. The resolution is to servers PLURAL: names and aliases are not unique and the gateway unions such a key into every server answering to it, so the entry is kept while any one of them is still granted. Resolving to the first match would make the outcome depend on catalog order, and on the order where the deselected server comes first it would drop a restriction that was also covering a server still granted, which is a widening that reproduces on one deployment and not another. A key that resolves to no known server is kept, since a server we cannot identify is one we cannot confirm was deselected, which also covers a catalog that has not loaded.Five mutants, each failing in exactly one direction: first-match resolution fails the shared-name case in the deselected-first order only (which is why that test is a pair over both orders rather than a single case), the id-only comparison fails all six resolution cases, pruning an unresolvable key fails the two under-informed cases, retaining everything fails the four drop cases, and ignoring retained indirect grants fails the three keep cases.
The shared-name case here is the same underlying ambiguity as on #35154, arriving from the opposite side: there the risk is retaining a multi-match key, here it is dropping one. Both are symptoms of name-keyed entries being ambiguous by construction at the data layer, which is filed on its own as LIT-4982 rather than argued in either UI PR.
One residue is left rather than claimed away. With two access groups retained and one removed, the save cannot tell WHICH retained group supplies a given server, so the entry stays until an admin clears it. Rendering the effective set in the matrix is what removes the guessing, and since that is the shared component behind five surfaces it is fixed separately in #35154; the follow-up on this surface should delete this filter outright rather than adapt it, because once the editor shows the effective set there is nothing left to infer at save time
One pre-existing path can reach tool execution without the entitlement check: a legacy local-tool dispatch fallback that runs a handler directly when no server resolves. It affects the key, team, org and agent levels equally and this PR neither creates nor closes it; it is filed separately and is being fixed there. Two other paths were reported alongside it and have since been investigated and found not to be defects, so the earlier count of three in this description was wrong: the logging-object condition has no live caller that can trip it, and open-channel servers are already narrowable for every caller through the server's own
allowed_tools, which is enforced ahead of the entitlement check.object_permissionjoins the non-admin self-update protected fields. Sending an empty grant list means "no restriction", so a self-write is the escalation path here: it would lift a ceiling an admin placed on that person.QA runbook
Final Attestation