diff --git a/litellm/proxy/management_endpoints/key_management_endpoints.py b/litellm/proxy/management_endpoints/key_management_endpoints.py index a3679b84bd6..a1c4fd842d2 100644 --- a/litellm/proxy/management_endpoints/key_management_endpoints.py +++ b/litellm/proxy/management_endpoints/key_management_endpoints.py @@ -535,6 +535,7 @@ def _check_allowed_routes_caller_permission( user_api_key_dict: UserAPIKeyAuth, *, allowed_routes_was_provided: bool = False, + allowed_routes_changed: bool | None = None, allow_safe_presets: bool = False, ) -> None: """ @@ -552,6 +553,8 @@ def _check_allowed_routes_caller_permission( carve-out below accepts any list of tokens in `_NON_ADMIN_SAFE_ALLOWED_ROUTES_PRESETS`. """ + if allowed_routes_changed is False: + return if not allowed_routes_was_provided and not allowed_routes: return if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN.value: @@ -576,6 +579,7 @@ def _check_allowed_routes_caller_permission( def _check_permissions_caller_permission( data: GenerateRequestBase, user_api_key_dict: UserAPIKeyAuth, + permissions_changed: bool | None = None, ) -> None: """ Require PROXY_ADMIN when `permissions` is present in the request body. @@ -584,6 +588,8 @@ def _check_permissions_caller_permission( omits the field (default flows through) is distinct from one that sends any explicit value. """ + if permissions_changed is False: + return permissions_in_request = "permissions" in data.model_fields_set if not permissions_in_request and not data.permissions: return @@ -2260,6 +2266,9 @@ async def _validate_update_key_data( allowed_routes=data.allowed_routes, user_api_key_dict=user_api_key_dict, allowed_routes_was_provided="allowed_routes" in data.model_fields_set, + allowed_routes_changed=( + "allowed_routes" in data.model_fields_set and data.allowed_routes != existing_key_row.allowed_routes + ), ) _check_passthrough_routes_caller_permission( data=data, @@ -2268,6 +2277,9 @@ async def _validate_update_key_data( _check_permissions_caller_permission( data=data, user_api_key_dict=user_api_key_dict, + permissions_changed=( + "permissions" in data.model_fields_set and data.permissions != existing_key_row.permissions + ), ) _validate_caller_can_change_key_ownership( 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 c9abdf09a5d..658372cafee 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 @@ -11436,46 +11436,6 @@ class TestAllowedRoutesCallerPermission: assert str(exc_info.value.code) == "403" assert "allowed_routes" in str(exc_info.value.message) - @pytest.mark.asyncio - async def test_non_admin_update_key_explicit_empty_allowed_routes_rejected(self): - """`update_key_fn` rejects a non-admin when `allowed_routes` is - present as `[]` in the request body. The value matches the model - default but `model_fields_set` distinguishes the two.""" - from litellm.proxy.management_endpoints.key_management_endpoints import ( - update_key_fn, - ) - - data = UpdateKeyRequest(key="sk-test", allowed_routes=[]) - assert "allowed_routes" in data.model_fields_set - user_api_key_dict = UserAPIKeyAuth( - user_id="internal-user-123", - user_role=LitellmUserRoles.INTERNAL_USER, - ) - mock_prisma_client = AsyncMock() - - with ( - patch("litellm.proxy.proxy_server.prisma_client", mock_prisma_client), - patch("litellm.proxy.proxy_server.user_api_key_cache", MagicMock()), - patch("litellm.proxy.proxy_server.user_custom_key_update", None), - patch("litellm.proxy.proxy_server.llm_router", None), - patch("litellm.proxy.proxy_server.premium_user", True), - patch("litellm.proxy.proxy_server.proxy_logging_obj", MagicMock()), - patch( - "litellm.proxy.management_endpoints.key_management_endpoints._get_and_validate_existing_key", - new_callable=AsyncMock, - return_value=MagicMock(), - ), - ): - with pytest.raises(ProxyException) as exc_info: - await update_key_fn( - request=MagicMock(), - data=data, - user_api_key_dict=user_api_key_dict, - litellm_changed_by=None, - ) - assert str(exc_info.value.code) == "403" - assert "allowed_routes" in str(exc_info.value.message) - @pytest.mark.asyncio async def test_non_admin_update_key_explicit_null_allowed_routes_rejected(self): """`update_key_fn` rejects a non-admin when `allowed_routes` is @@ -14332,6 +14292,8 @@ def _make_personal_key_row_for_alice(): max_budget=None, organization_id=None, project_id=None, + allowed_routes=[], + permissions={}, ) @@ -14370,33 +14332,79 @@ async def test_update_key_non_admin_permissions_non_empty_rejected(monkeypatch): assert "permissions" in str(exc.value.detail) +@pytest.mark.parametrize( + ("data", "existing_allowed_routes", "existing_permissions"), + [ + ( + UpdateKeyRequest( + key="sk-alice-personal", + tpm_limit=42, + allowed_routes=[], + ), + [], + {}, + ), + ( + UpdateKeyRequest( + key="sk-alice-personal", + tpm_limit=42, + allowed_routes=["llm_api_routes"], + ), + ["llm_api_routes"], + {}, + ), + ( + UpdateKeyRequest( + key="sk-alice-personal", + tpm_limit=42, + permissions={}, + ), + [], + {}, + ), + ( + UpdateKeyRequest( + key="sk-alice-personal", + tpm_limit=42, + permissions={"get_spend_routes": False}, + ), + [], + {"get_spend_routes": False}, + ), + ], +) @pytest.mark.asyncio -async def test_update_key_non_admin_permissions_explicit_empty_rejected(monkeypatch): - """`_validate_update_key_data` rejects a non-admin when `permissions` - is present as `{}` in the request body. The value matches the model - default but `model_fields_set` distinguishes the two.""" +async def test_update_key_non_admin_can_resubmit_unchanged_privileged_fields( + data, + existing_allowed_routes, + existing_permissions, + monkeypatch, +): mock_prisma_client = AsyncMock() mock_prisma_client.jsonify_object = lambda data: data monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma_client) - data = UpdateKeyRequest( - key="sk-alice-personal", - permissions={}, + existing_key_row = MagicMock( + token="hashed_alice_personal_key", + user_id="alice", + team_id=None, + created_by="alice", + max_budget=None, + organization_id=None, + project_id=None, + allowed_routes=existing_allowed_routes, + permissions=existing_permissions, ) - assert "permissions" in data.model_fields_set - with pytest.raises(HTTPException) as exc: - await _validate_update_key_data( - data=data, - existing_key_row=_make_personal_key_row_for_alice(), - user_api_key_dict=_make_alice_internal_user(), - llm_router=None, - premium_user=True, - prisma_client=mock_prisma_client, - user_api_key_cache=MagicMock(), - ) - assert exc.value.status_code == 403 - assert "permissions" in str(exc.value.detail) + await _validate_update_key_data( + data=data, + existing_key_row=existing_key_row, + user_api_key_dict=_make_alice_internal_user(), + llm_router=None, + premium_user=True, + prisma_client=mock_prisma_client, + user_api_key_cache=MagicMock(), + ) @pytest.mark.asyncio