diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index 95ccb7bbe0b..23dfe9e0c07 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -1547,10 +1547,17 @@ async def _check_project_key_limits( data: GenerateKeyRequest | UpdateKeyRequest, prisma_client: PrismaClient, user_api_key_cache: UserApiKeyCache, + key_team_id: str | None = None, ) -> None: """ - Validate that key's models and budget respect its project's limits. + Validate that the key belongs to the project's team, and that its models + and budget respect the project's limits. + - The project's owning team must be the key's team. A project is created + under exactly one team and its budget and models are that team's, so a + key on another team recorded under it charges a tenant that never granted + anything — and issuing one needs no proxy-admin rights, only the project + id (#41089). A project with no team belongs to nobody and is left alone. - Key models must be a subset of project models, except the all-team-models / all-proxy-models sentinels, which inherit a parent scope and are narrowed by the project at request time - Key max_budget must be <= project max_budget @@ -1567,6 +1574,17 @@ async def _check_project_key_limits( detail={"error": f"Project not found, project_id={project_id}"}, ) + # Validate the project's team owns the key + if project_obj.team_id is not None and project_obj.team_id != key_team_id: + raise HTTPException( + status_code=403, + detail={ + "error": f"Project {project_id} belongs to team {project_obj.team_id}, " + f"but the key belongs to {key_team_id if key_team_id is not None else 'no team'}. " + "A project can only be attached to keys of the team that owns it." + }, + ) + # Validate key models are a subset of project models if data.models and len(project_obj.models) > 0: for m in data.models: @@ -1922,6 +1940,7 @@ async def generate_key_fn( data=data, prisma_client=prisma_client, user_api_key_cache=user_api_key_cache, + key_team_id=data.team_id, ) return await _common_key_generation_helper( @@ -2864,12 +2883,18 @@ async def _validate_update_key_data( _project_id_to_check: Final = ( data.project_id if "project_id" in data.model_fields_set else existing_key_row.project_id ) - if _project_id_to_check is not None and (data.models is not None or data.max_budget is not None): + # Also when the project itself is being set or changed: that is exactly when + # the team that owns it has to be checked, and a request that moves only the + # project carries neither models nor max_budget (#41089). + if _project_id_to_check is not None and ( + "project_id" in data.model_fields_set or data.models is not None or data.max_budget is not None + ): await _check_project_key_limits( project_id=_project_id_to_check, data=data, prisma_client=checked_prisma_client, user_api_key_cache=user_api_key_cache, + key_team_id=(data.team_id if "team_id" in data.model_fields_set else existing_key_row.team_id), ) # When the caller asks to change the key's organization_id, require that diff --git a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py index 2ac52da57df..37e7bc95285 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py @@ -17849,11 +17849,13 @@ async def test_regenerate_key_repoints_live_membership_not_the_key_row_it_read( ) == ["attached-model"] -async def _cache_with_project(project_id: str, project_models: list[str]) -> UserApiKeyCache: +async def _cache_with_project( + project_id: str, project_models: list[str], team_id: str | None = "team-lit-5823" +) -> UserApiKeyCache: user_api_key_cache = UserApiKeyCache() await user_api_key_cache.async_set_cache( key=_project_cache_key(project_id), - value=LiteLLM_ProjectTableCachedObj(project_id=project_id, team_id="team-lit-5823", models=project_models), + value=LiteLLM_ProjectTableCachedObj(project_id=project_id, team_id=team_id, models=project_models), model_type=LiteLLM_ProjectTableCachedObj, ) return user_api_key_cache @@ -17871,6 +17873,7 @@ async def test_check_project_key_limits_accepts_inherited_model_sentinels(reques data=request_cls(key="sk-lit-5823", models=[sentinel]), prisma_client=MagicMock(), user_api_key_cache=user_api_key_cache, + key_team_id="team-lit-5823", ) @@ -17889,6 +17892,7 @@ async def test_check_project_key_limits_still_rejects_real_model_outside_project data=request_cls(key="sk-lit-5823", models=key_models), prisma_client=MagicMock(), user_api_key_cache=user_api_key_cache, + key_team_id="team-lit-5823", ) assert exc_info.value.status_code == 400 @@ -18257,7 +18261,10 @@ async def test_project_detachment_preserves_omission_and_other_key_fields(): @pytest.mark.asyncio async def test_project_detachment_uses_effective_project_for_validation(project_id: str | None): existing: Final = LiteLLM_VerificationToken(token="project-detach-token", project_id="project-orbit") - cache: Final = await _cache_with_project("project-orbit", ["model-orbit"]) + # An unowned project: this cell is about WHICH project the validation uses, + # so the ownership gate (#41089) must not be what it measures. Giving the + # key a team instead would pull the whole team lookup into a MagicMock db. + cache: Final = await _cache_with_project("project-orbit", ["model-orbit"], team_id=None) data: Final = UpdateKeyRequest(key=existing.token, project_id=project_id, models=["model-other"]) if project_id is None: await _validate_update_key_data( diff --git a/tests/test_litellm/proxy/management_endpoints/test_key_project_team_ownership.py b/tests/test_litellm/proxy/management_endpoints/test_key_project_team_ownership.py new file mode 100644 index 00000000000..082c9a0e501 --- /dev/null +++ b/tests/test_litellm/proxy/management_endpoints/test_key_project_team_ownership.py @@ -0,0 +1,115 @@ +"""A project may only be attached to keys of the team that owns it (#41089). + +A project is created under exactly one team, and its budget and models are that +team's. Nothing checked that the key's team matched, so an admin of team-a — a +member of no other team — could issue a key on their own team pointing at a +team-b project, and team-b would see the spend under a project they never +granted anything on. Only the project id was needed. + +The negative controls are the point: a project with no owning team is left +alone, and a key on the owning team still passes, or the check would be a wall +rather than a boundary. +""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import HTTPException + +from litellm.models.project import LiteLLM_ProjectTable +from litellm.proxy._types import GenerateKeyRequest, UpdateKeyRequest + + +def _project(team_id, models=None, project_id="proj-1"): + return LiteLLM_ProjectTable( + project_id=project_id, + team_id=team_id, + models=models or [], + ) + + +async def _check(project, data, key_team_id): + """Drive _check_project_key_limits with the project the store would return.""" + from litellm.proxy.management_endpoints import key_management_endpoints as kme + + original = kme.get_project_object + kme.get_project_object = AsyncMock(return_value=project) + try: + await kme._check_project_key_limits( + project_id=project.project_id, + data=data, + prisma_client=MagicMock(), + user_api_key_cache=MagicMock(), + key_team_id=key_team_id, + ) + finally: + kme.get_project_object = original + + +@pytest.mark.asyncio +async def test_a_key_may_not_point_at_another_teams_project(): + with pytest.raises(HTTPException) as exc: + await _check( + _project(team_id="team-b"), + GenerateKeyRequest(team_id="team-a"), + key_team_id="team-a", + ) + assert exc.value.status_code == 403 + detail = str(exc.value.detail) + assert "team-b" in detail and "team-a" in detail + + +@pytest.mark.asyncio +async def test_a_key_with_no_team_may_not_point_at_a_teams_project(): + # The issue's step 5: no team at all still charges a team's project. + with pytest.raises(HTTPException) as exc: + await _check( + _project(team_id="team-b"), + GenerateKeyRequest(), + key_team_id=None, + ) + assert exc.value.status_code == 403 + assert "no team" in str(exc.value.detail) + + +@pytest.mark.asyncio +async def test_a_key_on_the_owning_team_is_accepted(): + await _check( + _project(team_id="team-b"), + GenerateKeyRequest(team_id="team-b"), + key_team_id="team-b", + ) + + +@pytest.mark.asyncio +async def test_a_project_with_no_team_is_left_alone(): + # Nobody owns it, so there is no boundary to cross — rejecting here would + # break every project created outside a team. + await _check(_project(team_id=None), GenerateKeyRequest(team_id="team-a"), key_team_id="team-a") + await _check(_project(team_id=None), GenerateKeyRequest(), key_team_id=None) + + +@pytest.mark.asyncio +async def test_the_ownership_check_runs_before_the_model_check(): + # A foreign project must be refused as foreign, not as "model not allowed": + # the 400 would read as a configuration problem and hide the tenancy one. + with pytest.raises(HTTPException) as exc: + await _check( + _project(team_id="team-b", models=["gpt-4o-mini"]), + GenerateKeyRequest(team_id="team-a", models=["gpt-4o"]), + key_team_id="team-a", + ) + assert exc.value.status_code == 403 + + +@pytest.mark.asyncio +async def test_an_update_carries_the_keys_existing_team(): + # /key/update sends no team_id when only the project changes, so the check + # has to use the key's stored team rather than treating it as absent. + with pytest.raises(HTTPException) as exc: + await _check( + _project(team_id="team-b"), + UpdateKeyRequest(key="sk-x", project_id="proj-1"), + key_team_id="team-a", + ) + assert exc.value.status_code == 403