mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-14 23:21:35 +00:00
fix(keys): allow unchanged budget limits on self-service updates
This commit is contained in:
parent
8b323202ec
commit
0cd8ccb604
4 changed files with 115 additions and 18 deletions
|
|
@ -2241,6 +2241,12 @@ async def _validate_mcp_servers_for_key_update(
|
|||
return normalized_object_permission
|
||||
|
||||
|
||||
def _budget_limit_state(
|
||||
budget_limits: list[BudgetLimitEntry] | None,
|
||||
) -> tuple[tuple[str, float], ...]:
|
||||
return tuple((window.budget_duration, window.max_budget) for window in budget_limits or [])
|
||||
|
||||
|
||||
async def _validate_update_key_data(
|
||||
data: UpdateKeyRequest,
|
||||
existing_key_row: Any,
|
||||
|
|
@ -2313,20 +2319,21 @@ async def _validate_update_key_data(
|
|||
# - Anyone else (non-PROXY_ADMIN, not the owner, not a team member
|
||||
# on a team key): must pass _check_key_admin_access (PROXY_ADMIN
|
||||
# / key-owner / team-admin / org-admin of the key).
|
||||
# - max_budget / spend / budget_limits: always require the admin
|
||||
# check, even for the key owner or a team member (matches the
|
||||
# existing admin-only budget semantics). budget_limits uses
|
||||
# model_fields_set because an explicit null/[] clears the field
|
||||
# and must gate the same as setting or changing it.
|
||||
# - spend gates on presence alone (not a value diff): the DB spend
|
||||
# lags the live cross-pod counter, so letting an "unchanged" spend
|
||||
# through the non-admin path would let a key owner / team member
|
||||
# overwrite the live counter below real usage and silently weaken
|
||||
# enforcement.
|
||||
_existing_budget_limits = [
|
||||
BudgetLimitEntry.model_validate(window) for window in (getattr(existing_key_row, "budget_limits", None) or [])
|
||||
]
|
||||
_budget_limits_changed = "budget_limits" in data.model_fields_set and _budget_limit_state(
|
||||
data.budget_limits
|
||||
) != _budget_limit_state(_existing_budget_limits)
|
||||
_is_budget_change = (
|
||||
(data.max_budget is not None and data.max_budget != existing_key_row.max_budget)
|
||||
or data.spend is not None
|
||||
or "budget_limits" in data.model_fields_set
|
||||
or _budget_limits_changed
|
||||
)
|
||||
|
||||
_existing_metadata = getattr(existing_key_row, "metadata", None)
|
||||
|
|
|
|||
|
|
@ -10458,7 +10458,7 @@ class TestKeyOwnerPrivilegeEscalation:
|
|||
Policy:
|
||||
- created_by == caller → can edit any non-budget field without admin
|
||||
- created_by != caller (assigned user) → must pass admin check for any edit
|
||||
- budget changes (max_budget/spend) → always require admin
|
||||
- budget changes → always require admin
|
||||
- PROXY_ADMIN → unrestricted
|
||||
"""
|
||||
|
||||
|
|
@ -10472,6 +10472,7 @@ class TestKeyOwnerPrivilegeEscalation:
|
|||
row.team_id = None
|
||||
row.max_budget = None
|
||||
row.spend = 0.0
|
||||
row.budget_limits = None
|
||||
row.organization_id = None
|
||||
row.project_id = None
|
||||
return row
|
||||
|
|
@ -10631,6 +10632,13 @@ class TestKeyOwnerPrivilegeEscalation:
|
|||
"""Clearing budget_limits is a budget change and requires admin."""
|
||||
data = UpdateKeyRequest(key="sk-test", budget_limits=cleared_value)
|
||||
existing = self._make_existing_key(created_by="creator-123")
|
||||
existing.budget_limits = [
|
||||
{
|
||||
"budget_duration": "30d",
|
||||
"max_budget": 100.0,
|
||||
"reset_at": "2026-08-01T00:00:00Z",
|
||||
}
|
||||
]
|
||||
auth = self._make_auth(user_id="creator-123")
|
||||
|
||||
mock_check = AsyncMock(
|
||||
|
|
@ -10652,6 +10660,59 @@ class TestKeyOwnerPrivilegeEscalation:
|
|||
)
|
||||
mock_check.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("unchanged_value", [[], None])
|
||||
async def test_creator_can_send_unchanged_empty_budget_limits(
|
||||
self, unchanged_value
|
||||
):
|
||||
data = UpdateKeyRequest(key="sk-test", budget_limits=unchanged_value)
|
||||
existing = self._make_existing_key(created_by="creator-123")
|
||||
auth = self._make_auth(user_id="creator-123")
|
||||
|
||||
mock_check = AsyncMock()
|
||||
with patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints._check_key_admin_access",
|
||||
mock_check,
|
||||
):
|
||||
await _validate_update_key_data(
|
||||
data=data,
|
||||
existing_key_row=existing,
|
||||
user_api_key_dict=auth,
|
||||
llm_router=None,
|
||||
premium_user=False,
|
||||
prisma_client=AsyncMock(),
|
||||
user_api_key_cache=MagicMock(),
|
||||
)
|
||||
mock_check.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_creator_can_send_unchanged_existing_budget_limits(self):
|
||||
unchanged_window = {
|
||||
"budget_duration": "30d",
|
||||
"max_budget": 100.0,
|
||||
"reset_at": "2026-08-01T00:00:00Z",
|
||||
}
|
||||
data = UpdateKeyRequest(key="sk-test", budget_limits=[unchanged_window])
|
||||
existing = self._make_existing_key(created_by="creator-123")
|
||||
existing.budget_limits = [unchanged_window]
|
||||
auth = self._make_auth(user_id="creator-123")
|
||||
|
||||
mock_check = AsyncMock()
|
||||
with patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints._check_key_admin_access",
|
||||
mock_check,
|
||||
):
|
||||
await _validate_update_key_data(
|
||||
data=data,
|
||||
existing_key_row=existing,
|
||||
user_api_key_dict=auth,
|
||||
llm_router=None,
|
||||
premium_user=False,
|
||||
prisma_client=AsyncMock(),
|
||||
user_api_key_cache=MagicMock(),
|
||||
)
|
||||
mock_check.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_admin_can_clear_budget_limits(self):
|
||||
data = UpdateKeyRequest(key="sk-test", budget_limits=[])
|
||||
|
|
|
|||
|
|
@ -662,7 +662,31 @@ describe("KeyEditView", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("should resend existing budget windows on submit when they are left untouched", async () => {
|
||||
it("should omit budget_limits when no budget windows exist", async () => {
|
||||
const onSubmitMock = vi.fn().mockResolvedValue(undefined);
|
||||
renderWithProviders(
|
||||
<KeyEditView
|
||||
keyData={MOCK_KEY_DATA}
|
||||
onCancel={() => {}}
|
||||
onSubmit={onSubmitMock}
|
||||
accessToken={"test-token"}
|
||||
userID={"test-user"}
|
||||
userRole={"user"}
|
||||
premiumUser={false}
|
||||
/>,
|
||||
);
|
||||
|
||||
const submitButton = await screen.findByRole("button", { name: /save changes/i });
|
||||
await userEvent.click(submitButton);
|
||||
|
||||
await waitFor(() => {
|
||||
expect(onSubmitMock).toHaveBeenCalled();
|
||||
const callArgs = onSubmitMock.mock.calls[0][0];
|
||||
expect(callArgs.budget_limits).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
it("should omit existing budget windows when they are left untouched", async () => {
|
||||
const onSubmitMock = vi.fn().mockResolvedValue(undefined);
|
||||
const keyDataWithWindow = {
|
||||
...MOCK_KEY_DATA,
|
||||
|
|
@ -686,7 +710,7 @@ describe("KeyEditView", () => {
|
|||
await waitFor(() => {
|
||||
expect(onSubmitMock).toHaveBeenCalled();
|
||||
const callArgs = onSubmitMock.mock.calls[0][0];
|
||||
expect(callArgs.budget_limits).toEqual([{ budget_duration: "30d", max_budget: 100 }]);
|
||||
expect(callArgs.budget_limits).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -87,6 +87,13 @@ const getKeyTypeFromRoutes = (allowedRoutes: string[] | null | undefined): strin
|
|||
return "default";
|
||||
};
|
||||
|
||||
const areBudgetWindowsEqual = (first: readonly BudgetWindowEntry[], second: readonly BudgetWindowEntry[]): boolean =>
|
||||
first.length === second.length &&
|
||||
first.every(
|
||||
(window, index) =>
|
||||
window.budget_duration === second[index].budget_duration && window.max_budget === second[index].max_budget,
|
||||
);
|
||||
|
||||
export function KeyEditView({
|
||||
keyData,
|
||||
onCancel,
|
||||
|
|
@ -306,18 +313,16 @@ export function KeyEditView({
|
|||
values.duration = null;
|
||||
}
|
||||
|
||||
// Reconcile multi-window budget limits from the editor state, dropping
|
||||
// incomplete entries (no max_budget). Sending [] tells the backend to clear
|
||||
// all stored windows, so only send it when the user removed every window;
|
||||
// when entries remain but are still incomplete, omit the field so the saved
|
||||
// windows are left untouched (JSON.stringify drops the undefined key).
|
||||
const validWindows = budgetLimits.filter(
|
||||
(w) => w.budget_duration && w.max_budget !== null && w.max_budget !== undefined,
|
||||
);
|
||||
if (validWindows.length > 0) {
|
||||
values.budget_limits = validWindows;
|
||||
} else if (budgetLimits.length === 0) {
|
||||
values.budget_limits = [];
|
||||
const originalWindows = Array.isArray(keyData.budget_limits) ? keyData.budget_limits : [];
|
||||
if (!areBudgetWindowsEqual(budgetLimits, originalWindows)) {
|
||||
if (validWindows.length > 0) {
|
||||
values.budget_limits = validWindows;
|
||||
} else if (budgetLimits.length === 0) {
|
||||
values.budget_limits = [];
|
||||
}
|
||||
}
|
||||
|
||||
// Always send the current per-tag limit map so removing every row
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue