mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
fix: keep litellm_credential_name from LiteLLM Params JSON and gate stored credential attach to proxy admins (#39047)
* fix(ui): keep litellm_credential_name from LiteLLM Params JSON when no credential is selected Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(ui): drop null litellm_credential_name from AddModelPanel payload fixture Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): validate JSON litellm_credential_name against accessible credentials Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(proxy): enforce proxy-admin-only credential attachment on model create/update Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(proxy): raise ProxyException for unauthorized credential attach and gate /model/update Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * refactor(proxy): fold credential-change detection into can_user_attach_credential to satisfy complexity budget Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(proxy): decrypt stored credential name before unchanged-credential comparison Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(proxy): cover credential attach rejection on add_new_model and patch_model Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(proxy): annotate proxy-global patches with test-quality suppressions Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
parent
8ce02c0666
commit
aab9abdd1d
5 changed files with 208 additions and 6 deletions
|
|
@ -54,7 +54,10 @@ from litellm.proxy.common_utils.config_sync_pubsub import (
|
|||
coordination_redis_cache,
|
||||
publish_config_change,
|
||||
)
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import (
|
||||
decrypt_value_helper,
|
||||
encrypt_value_helper,
|
||||
)
|
||||
from litellm.proxy.common_utils.user_api_key_cache import UserApiKeyCache
|
||||
from litellm.proxy.management_endpoints.common_utils import _is_user_team_admin
|
||||
from litellm.proxy.management_endpoints.team_endpoints import (
|
||||
|
|
@ -701,6 +704,12 @@ async def patch_model(
|
|||
param="blocked",
|
||||
)
|
||||
|
||||
ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=patch_data.litellm_params,
|
||||
user_api_key_dict=user_api_key_dict,
|
||||
existing_litellm_params=db_model.litellm_params,
|
||||
)
|
||||
|
||||
_raise_on_strategy_router_write_violation(
|
||||
incoming_params=patch_data.litellm_params,
|
||||
existing_params=db_model.litellm_params,
|
||||
|
|
@ -1464,6 +1473,32 @@ class ModelManagementAuthChecks:
|
|||
)
|
||||
return True
|
||||
|
||||
@staticmethod
|
||||
def can_user_attach_credential(
|
||||
litellm_params: GenericLiteLLMParams | None,
|
||||
user_api_key_dict: UserAPIKeyAuth,
|
||||
existing_litellm_params: GenericLiteLLMParams | None = None,
|
||||
) -> Literal[True]:
|
||||
if litellm_params is None or litellm_params.litellm_credential_name is None:
|
||||
return True
|
||||
if existing_litellm_params is not None and existing_litellm_params.litellm_credential_name is not None:
|
||||
existing_credential_name: Final = decrypt_value_helper(
|
||||
value=existing_litellm_params.litellm_credential_name,
|
||||
key="litellm_credential_name",
|
||||
exception_type="debug",
|
||||
return_original_value=True,
|
||||
)
|
||||
if litellm_params.litellm_credential_name == existing_credential_name:
|
||||
return True
|
||||
if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN:
|
||||
return True
|
||||
raise ProxyException(
|
||||
message=f"Only a proxy admin can attach a stored credential (litellm_credential_name) to a model. Your role={user_api_key_dict.user_role}.",
|
||||
type=ProxyErrorTypes.auth_error.value,
|
||||
code=status.HTTP_403_FORBIDDEN,
|
||||
param="litellm_credential_name",
|
||||
)
|
||||
|
||||
@staticmethod
|
||||
async def allow_team_model_action(
|
||||
model_params: Deployment | updateDeployment,
|
||||
|
|
@ -1786,6 +1821,11 @@ async def add_new_model(
|
|||
premium_user=premium_user,
|
||||
)
|
||||
|
||||
ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=model_params.litellm_params,
|
||||
user_api_key_dict=user_api_key_dict,
|
||||
)
|
||||
|
||||
_raise_on_strategy_router_write_violation(
|
||||
incoming_params=model_params.litellm_params,
|
||||
existing_params=None,
|
||||
|
|
@ -1958,6 +1998,12 @@ async def update_model(
|
|||
premium_user=premium_user,
|
||||
)
|
||||
|
||||
ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=model_params.litellm_params,
|
||||
user_api_key_dict=user_api_key_dict,
|
||||
existing_litellm_params=deployment.litellm_params,
|
||||
)
|
||||
|
||||
_raise_on_strategy_router_write_violation(
|
||||
incoming_params=model_params.litellm_params,
|
||||
existing_params=deployment.litellm_params,
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ from litellm.proxy._types import (
|
|||
ReconcileOutcome,
|
||||
UserAPIKeyAuth,
|
||||
)
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper
|
||||
from litellm.proxy.management_endpoints.model_management_endpoints import (
|
||||
ModelManagementAuthChecks,
|
||||
_get_team_deployments,
|
||||
|
|
@ -263,6 +264,131 @@ class TestModelManagementAuthChecks:
|
|||
)
|
||||
assert "403" in str(exc_info.value)
|
||||
|
||||
def test_can_user_attach_credential_admin_success(self):
|
||||
result = ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
user_api_key_dict=self.admin_user,
|
||||
)
|
||||
assert result is True
|
||||
|
||||
def test_can_user_attach_credential_without_credential_allows_any_role(self):
|
||||
result = ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model"),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
)
|
||||
assert result is True
|
||||
|
||||
def test_can_user_attach_credential_team_admin_fails(self):
|
||||
with pytest.raises(Exception, match="Only a proxy admin can attach a stored credential") as exc_info:
|
||||
ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
)
|
||||
assert exc_info.value.code == "403"
|
||||
|
||||
def test_can_user_attach_credential_unchanged_existing_allows_any_role(self):
|
||||
result = ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
existing_litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
)
|
||||
assert result is True
|
||||
|
||||
def test_can_user_attach_credential_unchanged_encrypted_existing_allows_any_role(self, monkeypatch):
|
||||
monkeypatch.setenv("LITELLM_SALT_KEY", "sk-1234")
|
||||
encrypted_name = encrypt_value_helper(value="shared-credential")
|
||||
assert encrypted_name != "shared-credential"
|
||||
result = ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
existing_litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name=encrypted_name),
|
||||
)
|
||||
assert result is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_new_model_rejects_credential_attach_for_non_admin(self):
|
||||
from litellm.proxy._types import ProxyException
|
||||
from litellm.proxy.management_endpoints.model_management_endpoints import (
|
||||
add_new_model,
|
||||
)
|
||||
|
||||
mock_prisma = MagicMock()
|
||||
with (
|
||||
patch("litellm.proxy.proxy_server.prisma_client", mock_prisma), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch("litellm.proxy.proxy_server.store_model_in_db", True), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch("litellm.proxy.proxy_server.premium_user", True), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch( # test-quality-ok: prior auth check needs a live DB; only the credential check is under test
|
||||
"litellm.proxy.management_endpoints.model_management_endpoints.ModelManagementAuthChecks.can_user_make_model_call",
|
||||
new=AsyncMock(return_value=None),
|
||||
),
|
||||
):
|
||||
with pytest.raises(ProxyException) as exc_info:
|
||||
await add_new_model(
|
||||
model_params=Deployment(
|
||||
model_name="credential-model",
|
||||
litellm_params=LiteLLM_Params(
|
||||
model="openai/gpt-4o", litellm_credential_name="shared-credential"
|
||||
),
|
||||
model_info={"id": "credential-create-test"},
|
||||
),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
)
|
||||
assert exc_info.value.code == "403"
|
||||
mock_prisma.db.litellm_proxymodeltable.create.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_patch_model_rejects_credential_attach_for_non_admin(self):
|
||||
from litellm.proxy._types import ProxyException
|
||||
from litellm.proxy.management_endpoints.model_management_endpoints import (
|
||||
patch_model,
|
||||
)
|
||||
from litellm.types.router import updateLiteLLMParams
|
||||
|
||||
model_id = "credential-patch-test"
|
||||
db_model = Deployment(
|
||||
model_name="credential-model",
|
||||
litellm_params=LiteLLM_Params(model="openai/gpt-4o"),
|
||||
model_info={"id": model_id},
|
||||
)
|
||||
with (
|
||||
patch("litellm.proxy.proxy_server.prisma_client", MagicMock()), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch("litellm.proxy.proxy_server.llm_router", MagicMock()), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch("litellm.proxy.proxy_server.store_model_in_db", True), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch("litellm.proxy.proxy_server.premium_user", True), # test-quality-ok: endpoint reads proxy server globals with no injection seam
|
||||
patch( # test-quality-ok: stubs the DB row fetch; only the credential check is under test
|
||||
"litellm.proxy.management_endpoints.model_management_endpoints.get_db_model",
|
||||
new=AsyncMock(return_value=db_model),
|
||||
),
|
||||
patch( # test-quality-ok: prior auth check needs a live DB; only the credential check is under test
|
||||
"litellm.proxy.management_endpoints.model_management_endpoints.ModelManagementAuthChecks.can_user_make_model_call",
|
||||
new=AsyncMock(return_value=None),
|
||||
),
|
||||
patch( # test-quality-ok: asserts the DB write is never reached on rejection
|
||||
"litellm.proxy.management_endpoints.model_management_endpoints._update_team_model_in_db",
|
||||
new=AsyncMock(),
|
||||
) as mock_update,
|
||||
):
|
||||
with pytest.raises(ProxyException) as exc_info:
|
||||
await patch_model(
|
||||
model_id=model_id,
|
||||
patch_data=updateDeployment(
|
||||
litellm_params=updateLiteLLMParams(
|
||||
model="openai/gpt-4o", litellm_credential_name="shared-credential"
|
||||
)
|
||||
),
|
||||
user_api_key_dict=self.team_admin_user,
|
||||
)
|
||||
assert exc_info.value.code == "403"
|
||||
mock_update.assert_not_awaited()
|
||||
|
||||
def test_can_user_attach_credential_internal_user_fails(self):
|
||||
with pytest.raises(Exception, match="Only a proxy admin can attach a stored credential") as exc_info:
|
||||
ModelManagementAuthChecks.can_user_attach_credential(
|
||||
litellm_params=LiteLLM_Params(model="test_model", litellm_credential_name="shared-credential"),
|
||||
user_api_key_dict=self.normal_user,
|
||||
)
|
||||
assert exc_info.value.code == "403"
|
||||
|
||||
|
||||
class MockModelTable:
|
||||
def __init__(self, model_aliases: Dict[str, str], include: Optional[dict] = None):
|
||||
|
|
|
|||
|
|
@ -87,7 +87,6 @@ const alwaysMounted = {
|
|||
api_key: undefined,
|
||||
api_base: undefined,
|
||||
custom_llm_provider: "openai",
|
||||
litellm_credential_name: null,
|
||||
model: "gpt-4o",
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
import { describe, expect, it } from "vitest";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { prepareModelAddRequest } from "./handle_add_model_submit";
|
||||
|
||||
vi.mock("../networking", () => ({
|
||||
modelCreateCall: vi.fn(),
|
||||
}));
|
||||
|
||||
describe("prepareModelAddRequest", () => {
|
||||
it("returns deployment data for the most basic form", async () => {
|
||||
const formValues = {
|
||||
|
|
@ -73,4 +77,28 @@ describe("prepareModelAddRequest", () => {
|
|||
expect(deployment.litellmParamsObj.litellm_credential_name).toBe("selected-credential");
|
||||
expect(deployment.litellmParamsObj.timeout).toBe(5);
|
||||
});
|
||||
|
||||
it("keeps litellm_credential_name from LiteLLM Params JSON when no credential is selected", async () => {
|
||||
const formValues = {
|
||||
model_mappings: [
|
||||
{
|
||||
public_name: "Public Model",
|
||||
litellm_model: "litellm/public",
|
||||
},
|
||||
],
|
||||
model_name: "custom-model-name",
|
||||
litellm_extra_params: JSON.stringify({
|
||||
litellm_credential_name: "from-json",
|
||||
timeout: 5,
|
||||
}),
|
||||
litellm_credential_name: null,
|
||||
};
|
||||
|
||||
const deployments = await prepareModelAddRequest({ ...formValues }, "token", null);
|
||||
|
||||
expect(deployments).toHaveLength(1);
|
||||
const [deployment] = deployments!;
|
||||
expect(deployment.litellmParamsObj.litellm_credential_name).toBe("from-json");
|
||||
expect(deployment.litellmParamsObj.timeout).toBe(5);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -91,6 +91,9 @@ export const prepareModelAddRequest = async (formValues: Record<string, any>, ac
|
|||
if (value === "") {
|
||||
continue;
|
||||
}
|
||||
if (key === "litellm_credential_name" && value == null) {
|
||||
continue;
|
||||
}
|
||||
// Skip the custom_pricing and pricing_model fields as they're only used for UI control
|
||||
if (key === "custom_pricing" || key === "pricing_model" || key === "cache_control") {
|
||||
continue;
|
||||
|
|
@ -124,13 +127,13 @@ export const prepareModelAddRequest = async (formValues: Record<string, any>, ac
|
|||
if (value && value != undefined) {
|
||||
try {
|
||||
litellmExtraParams = JSON.parse(value);
|
||||
if ("litellm_credential_name" in litellmExtraParams) {
|
||||
delete litellmExtraParams.litellm_credential_name;
|
||||
}
|
||||
} catch (error) {
|
||||
toast.fromError("Failed to parse LiteLLM Extra Params: " + error);
|
||||
throw new Error("Failed to parse litellm_extra_params: " + error);
|
||||
}
|
||||
if ("litellm_credential_name" in litellmExtraParams && formValues.litellm_credential_name) {
|
||||
delete litellmExtraParams.litellm_credential_name;
|
||||
}
|
||||
for (const [key, value] of Object.entries(litellmExtraParams)) {
|
||||
litellmParamsObj[key] = value;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue