fix(mcp): resolve team.access_group_ids → MCP servers - #28997
Conversation
A virtual key whose team has an MCP-granting access group attached via /v1/access_group now sees that server through /v1/mcp/server (and can call tools on it) instead of getting an empty list. The runtime already resolves the key's unified access_group_ids; this adds the symmetric resolution on the team side, mirroring the model-side pattern in can_team_access_model — the group being on the team is itself the gate, so no assigned_team_ids re-check is needed. Resolves #27657
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR fixes a bug where virtual keys in teams with
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the MCP team permission lookup, correctly mirrors the established model-side pattern, and all modified tests preserve their original behavioral assertions. The fix is a well-contained addition to _get_allowed_mcp_servers_for_team: it fetches the team via the cache-first get_team_object helper (consistent with the existing key-level access group path) and unions in servers from team.access_group_ids using the same _get_mcp_server_ids_from_access_groups helper used elsewhere. The existing try/except guard ensures any lookup failure degrades gracefully to an empty list rather than an unhandled exception. Four new regression tests directly reproduce the reported bug, and the modified JWT tests preserve their intersection/inheritance assertions while correctly updating mock targets to match the new code path. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Core fix: _get_allowed_mcp_servers_for_team now fetches team via get_team_object (cache-first) and unions legacy object_permission servers with unified access_group_ids servers. Logic is correct and mirrors the existing model-side pattern. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Existing tests updated to mock at get_team_object level; four new regression tests added covering the team access_group_ids bug path, including null guard, union with object_permission, and end-to-end helper flow. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_jwt_mcp_enforcement.py | test_e2e_jwt_team_mcp_key_intersection updated to patch auth_checks.get_team_object instead of _get_team_object_permission; intersection assertion is preserved and still exercises the same boundary condition. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_jwt_mcp_simple.py | Simple JWT tests updated to mock get_team_object with a full LiteLLM_TeamTable (including object_permission); behavioral assertions unchanged, coverage is maintained. |
Reviews (2): Last reviewed commit: "chore(mcp): address greptile review on t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| if team_obj is None: | ||
| return [] | ||
|
|
||
| team_access_group_servers = await _get_mcp_server_ids_from_access_groups( |
There was a problem hiding this comment.
Medium: Missing access-group assignment check
A team admin can write access_group_ids on their team through /team/update; this now grants every MCP server in those groups without confirming the access group’s assigned_team_ids includes user_api_key_auth.team_id. Filter these groups through the access-group assignment metadata before adding access_mcp_server_ids, or restrict team-level access_group_ids changes to proxy-admin-managed assignment paths.
There was a problem hiding this comment.
pre-existing. model-side already reads team.access_group_ids without an assigned_team_ids check (auth_checks.py:3146-3162). root cause is /team/update permitting the write.
PR overviewThe PR still has an open authorization gap around team-level MCP server resolution. A team admin can set access group IDs on their team and inherit MCP server access from those groups without verifying that the groups are actually assigned to that team, which could allow unauthorized access to MCP servers. No issues have been addressed yet, so the current security posture still depends on closing this access-control bypass. Open issues (1)
Fixed/addressed: 0 · PR risk: 7/10 |
Forward already-imported prisma_client / user_api_key_cache / proxy_logging_obj to _get_mcp_server_ids_from_access_groups so it skips its lazy re-import path. Update test docstring + assertions to reflect that the resolver is invoked with [] (and short-circuits without DB access) rather than skipped entirely.
|
@greptileai re review |
ea01533
into
litellm_internal_staging
* fix(mcp): resolve team.access_group_ids → MCP servers A virtual key whose team has an MCP-granting access group attached via /v1/access_group now sees that server through /v1/mcp/server (and can call tools on it) instead of getting an empty list. The runtime already resolves the key's unified access_group_ids; this adds the symmetric resolution on the team side, mirroring the model-side pattern in can_team_access_model — the group being on the team is itself the gate, so no assigned_team_ids re-check is needed. Resolves BerriAI#27657 * chore(mcp): address greptile review on team access-group resolver Forward already-imported prisma_client / user_api_key_cache / proxy_logging_obj to _get_mcp_server_ids_from_access_groups so it skips its lazy re-import path. Update test docstring + assertions to reflect that the resolver is invoked with [] (and short-circuits without DB access) rather than skipped entirely.
2 participants
Summary
A virtual key whose team has an MCP-granting access group attached via
/v1/access_groupnow sees that server through/v1/mcp/serverand can call tools on it. Previously the runtime only resolved the key's ownaccess_group_ids—team.access_group_idswas ignored, so virtual keys in such teams got HTTP 200[]from discovery and 403 from tool calls. The fix mirrors the existing model-side pattern incan_team_access_model: the group being attached to the team is itself the gate, noassigned_team_idsre-check needed.Fix is in
_get_allowed_mcp_servers_for_team(user_api_key_auth_mcp.py) — fetches the full team object viaget_team_objectand unions in MCP servers fromteam.access_group_idsusing the existing_get_mcp_server_ids_from_access_groupshelper.Screenshots
before
after
Test plan
test_team_access_group_ids_resolve_to_mcp_servers— direct repro of the bugtest_team_access_group_ids_union_with_object_permission— legacyobject_permission+ unifiedaccess_group_idsuniontest_team_access_group_ids_empty_returns_no_extras— null guard, no resolver call when emptytest_get_allowed_mcp_servers_includes_team_access_group_extras_end_to_end— top-level helper pathtest_user_api_key_auth_mcp,test_jwt_mcp_simple,test_jwt_mcp_enforcement) for the new internal call site — 134 tests pass across MCP auth + JWT MCP suitesaccess_mcp_server_ids, attached to a team, generated a virtual key in that team —GET /v1/mcp/serverreturns[]before, returns the server afterResolves #27657
Resolves LIT-3181