mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-08 03:08:45 +00:00
fix(chatgpt): keep membership reads fail-closed and clear the CI-only gates
Four required checks were red on the merge commit. None of them came from a conflict hunk: each is a place where the merged tree disagrees with the code around it, and two only misbehave inside CI's environment. - `litellm/proxy/auth/auth_checks.py`: this PR gave `get_team_membership` a `raise_on_error` flag and defaulted it to False, so every caller -- all seven of them pre-existing -- started treating a failed membership read as "no membership row". Upstream main has no such flag and lets the exception surface, so the default quietly inverted fail-closed authorization into a grant: a Redis or database outage handed a team member the team's model scope and budget instead of rejecting the request. Default it to True, which covers the member budget check, the member model-scope check, JWT team resolution, and the compact summary gate, and name the one caller that genuinely wants the other behavior: `_team_member_granted_models` outside strict mode only attributes grants, so an unreadable member scope still degrades to "no member-level scope". - `tests/test_litellm/proxy/test_live_route_registration.py`: the websocket test runs the proxy lifespan, and the boot check now refuses an unset or weak master key before the app serves a single request, so the request never reached a Live route and all nine parametrizations failed where CI exports no key. Set one in the test rather than inherit whatever the environment has. A real key, not the weak-key opt-out, keeps the assertion honest: with a key in place the legacy sideband dependency rejects `Bearer test`, so awaiting `_auth` still proves the Live routes authenticate first. - `litellm/proxy/realtime_endpoints/call_supervision.py`: observer cleanup gathered a tuple whose length depended on a branch, which no `gather` overload can bind. That was the one `reportCallIssue` this PR added over the basedpyright budget ceiling. Two explicit branches gather the same tasks with the same cancellation and drain semantics. Correction to the merge commit message: it says the config-over-database login throttle test was dropped in favour of upstream's; both tests were actually kept, and both pass. Validation on this tree: 4546 passed, 1 skipped for the whole `proxy-auth` shard (auth, hooks, policy engine, client, realtime call redis) under `TZ=UTC`, which is the suite CI runs; the six membership fail-closed tests that were red now pass; `test_live_route_registration.py` passes 30 with the environment master key set, unset, and set to a rejected weak key; 546 passed across `tests/test_litellm/proxy/realtime_endpoints` and `tests/unit/realtime_api` after the gather change; `basedpyright` reports no `reportCallIssue` in `call_supervision.py`, and every remaining `reportCallIssue` in a file this PR touches sits on a line `git blame` traces to upstream main, so the rule returns to its base count of 113; `ruff check` and `ruff format --check` clean on the changed files. Local-only note, unrelated to this PR: `tests/test_litellm/proxy/hooks/test_batch_rate_limiter.py::test_cumulative_batch_tokens_over_tpd_returns_429_with_remaining_daily_window` fails under `TZ=Europe/Brussels` and passes under `TZ=UTC`. Upstream added it in `438d46cb50` and CI runs in UTC, so it is not affected by anything here.
This commit is contained in:
parent
f8994fde2d
commit
fc305aa2a1
3 changed files with 21 additions and 5 deletions
|
|
@ -2338,12 +2338,18 @@ async def get_team_membership(
|
|||
user_api_key_cache: UserApiKeyCache,
|
||||
parent_otel_span: Span | None = None,
|
||||
proxy_logging_obj: ProxyLogging | None = None,
|
||||
raise_on_error: bool = False,
|
||||
raise_on_error: bool = True,
|
||||
) -> Optional["LiteLLM_TeamMembership"]:
|
||||
"""
|
||||
Returns team membership object if user is member of team.
|
||||
|
||||
Do a isolated check for team membership vs. doing a combined key + team + user + team-membership check, as key might come in frequently for different users/teams. Larger call will slowdown query time. This way we get to cache the constant (key/team/user info) and only update based on the changing value (team membership).
|
||||
|
||||
``raise_on_error`` defaults to True because the callers that apply member-level limits -- the budget and
|
||||
model-scope checks in ``common_checks``, the JWT team resolution, and the compact summary gate -- cannot
|
||||
tell an absent row apart from a failed read, so swallowing an outage there hands the member whatever the
|
||||
team allows. A caller that only attributes grants, and can proceed with the lists it already holds,
|
||||
passes False and degrades to "no member-level scope".
|
||||
"""
|
||||
if user_id is None or team_id is None:
|
||||
return None
|
||||
|
|
@ -4514,6 +4520,9 @@ async def _team_member_granted_models(
|
|||
prisma_client=prisma_client,
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
proxy_logging_obj=proxy_logging_obj,
|
||||
# Spelled out because it is the one caller that wants the opposite of the default: outside
|
||||
# strict mode this walk only attributes grants, so an unreadable member scope degrades to
|
||||
# "no member-level scope" instead of failing the request.
|
||||
raise_on_error=strict_grant_lookup,
|
||||
)
|
||||
return () if team_membership is None else _member_allowed_models(team_membership)
|
||||
|
|
|
|||
|
|
@ -186,10 +186,12 @@ class CallSupervisor:
|
|||
reader.cancel()
|
||||
if lease_failed is not None:
|
||||
lease_failed.cancel()
|
||||
await asyncio.gather(
|
||||
*((reader, stopped, lease_failed) if lease_failed is not None else (reader, stopped)),
|
||||
return_exceptions=True,
|
||||
)
|
||||
# Branching rather than a conditional star-unpacked tuple: the overload solver cannot
|
||||
# bind one result type across a tuple whose length depends on the branch.
|
||||
if lease_failed is not None:
|
||||
await asyncio.gather(reader, stopped, lease_failed, return_exceptions=True)
|
||||
else:
|
||||
await asyncio.gather(reader, stopped, return_exceptions=True)
|
||||
with suppress(Exception):
|
||||
await self._upstream.close()
|
||||
if not self._usage_complete():
|
||||
|
|
|
|||
|
|
@ -50,6 +50,11 @@ def test_public_live_websockets_reach_live_auth_before_legacy_sideband(monkeypat
|
|||
authenticate = AsyncMock(side_effect=HTTPException(403, "Live authentication rejected"))
|
||||
monkeypatch.setattr(live, "_auth", authenticate)
|
||||
monkeypatch.setattr(proxy_server, "general_settings", {})
|
||||
# TestClient runs the proxy lifespan, and the boot check refuses a weak or unset master key
|
||||
# before the app serves anything. Set a safe key here instead of relying on the ambient one, so
|
||||
# the request really reaches the routes: with a key in place the legacy sideband dependency
|
||||
# would reject the "Bearer test" header, so awaiting _auth still proves which auth ran first.
|
||||
monkeypatch.setenv("LITELLM_MASTER_KEY", "sk-live-route-registration-test-master-key")
|
||||
# A previous proxy test may leave the module scheduler bound to a closed loop.
|
||||
monkeypatch.setattr(proxy_server, "scheduler", None)
|
||||
with TestClient(proxy_server.app) as client:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue