Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@greptile can you please review this PR? |
Greptile SummaryThis PR adds team-scoped model overrides, a feature gated behind the Key changes:
Issues found:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Adds default_models to TeamBase/UpdateTeamRequest, models to LiteLLM_TeamMembership, and team_default_models/team_member_models to LiteLLM_VerificationTokenView. Type definitions look correct and consistent. |
| litellm/proxy/auth/auth_checks.py | Adds compute_effective_team_models and modifies can_team_access_model to check effective models (team defaults + member overrides) when the feature is enabled. When effective_models is empty, access is denied — but the original team_object.models is no longer consulted in override mode. |
| litellm/proxy/management_endpoints/common_utils.py | Adds models parameter to _upsert_budget_and_membership and adds _is_team_model_overrides_enabled env-based feature flag. Logic correctly propagates models to both create and update payloads. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds _validate_key_models_against_effective_team_models for key gen/update validation. Makes a direct DB query to fetch team membership, acceptable since it's in management endpoints not the critical request path. |
| litellm/proxy/management_endpoints/team_endpoints.py | Adds model override validation in _process_team_members and team_member_update, validating member models are a subset of team models. Correctly skips validation when team has all_proxy_models. |
| litellm/proxy/management_helpers/utils.py | Adds models parameter to add_new_member and includes it in the team membership creation data. Correctly handles the case where models alone (without a budget) should still create a membership record. |
| litellm/proxy/schema.prisma | Adds default_models String[] to LiteLLM_TeamTable and models String[] to LiteLLM_TeamMembership, both with @default([]). Schema changes are clean. |
| litellm/proxy/utils.py | Adds t.default_models AS team_default_models and tm.models AS team_member_models to the combined verification token SQL query. Uses existing LEFT JOINs so NULLs are handled by Pydantic defaults. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request with API Key] --> B{Feature Flag LITELLM_TEAM_MODEL_OVERRIDES?}
B -- Disabled --> C[Use team_object.models as before]
B -- Enabled --> D[Compute effective_models = team_default_models ∪ team_member_models]
D --> E{effective_models empty?}
E -- Yes --> F[DENY: No models available]
E -- No --> G[Use effective_models for access check]
C --> H[_can_object_call_model]
G --> H
H --> I{Model in allowed list?}
I -- Yes --> J[Allow Request]
I -- No --> K{Team has access_group_ids?}
K -- Yes --> L[Check access groups fallback]
K -- No --> M[DENY: Model not allowed]
L --> N{Model in access group?}
N -- Yes --> J
N -- No --> M
style F fill:#f66,stroke:#333,color:#fff
style M fill:#f66,stroke:#333,color:#fff
style J fill:#6f6,stroke:#333,color:#fff
Last reviewed commit: 7708d08
|
@greptile please review this PR? |
Additional Comments (1)
The new code at lines 149 and 225-232 uses |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…com/BerriAI/litellm into litellm_team_scoped_model_overrides
|
@greptile please review this PR? |
|
|
||
| from .auth_checks_organization import organization_role_based_access_check | ||
| from .auth_utils import get_model_from_request | ||
| from litellm.proxy.management_endpoints.common_utils import _is_team_model_overrides_enabled |
There was a problem hiding this comment.
Top-level import in auth module
Per CLAUDE.md, top-level imports are preferred. However, this creates a direct import from management_endpoints into the auth module. If common_utils ever imports from auth_checks (it currently doesn't, but key_management_endpoints.py already does a deferred from litellm.proxy.auth.auth_checks import compute_effective_team_models at line 921), this could lead to a circular import risk. Consider whether this should remain top-level or use a deferred import like the callers in key_management_endpoints.py do.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| models_to_check: List[str] = team_object.models if team_object else [] | ||
| if _is_team_model_overrides_enabled() and valid_token: | ||
| # Compute effective models: team defaults + per-user overrides | ||
| effective_models = compute_effective_team_models( | ||
| team_default_models=valid_token.team_default_models, | ||
| team_member_models=valid_token.team_member_models, | ||
| ) | ||
|
|
||
| # If effective_models is empty, and feature is enabled, deny access | ||
| if len(effective_models) == 0: | ||
| raise ProxyException( | ||
| message=f"Team not allowed to access model. No models available for user in this team. Model={model}.", | ||
| type=ProxyErrorTypes.team_model_access_denied, | ||
| param="model", | ||
| code=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
||
| models_to_check = effective_models |
There was a problem hiding this comment.
Override mode silently ignores team_object.models
When _is_team_model_overrides_enabled() is True and valid_token is present, the original team_object.models list is completely replaced by effective_models (the union of team_default_models and team_member_models). This means if a team admin sets models on the team but forgets to also populate default_models, the effective models will be empty and all requests will be denied — even though team_object.models has valid entries.
Consider falling back to team_object.models when the effective models are empty, or making default_models derive from models when unset, to avoid a confusing misconfiguration cliff.
| membership = await prisma_client.db.litellm_teammembership.find_unique( | ||
| where={"user_id_team_id": {"user_id": user_id, "team_id": team_id}} |
There was a problem hiding this comment.
Direct DB query bypasses caching layer
This find_unique call goes directly to Prisma instead of using a cached helper. While this is in a management endpoint (not the critical request path), there is already a get_team_member_object or similar pattern used elsewhere. If team membership data is cached, this raw query will miss the cache and could return stale-inconsistent results compared to the auth path which reads team_member_models from the SQL join in utils.py.
| if not data.models: | ||
| data.models = effective_models |
There was a problem hiding this comment.
Empty data.models silently overwritten with effective models
When data.models is falsy (empty list or None), this code silently assigns effective_models to data.models. This means if a user creates a key without specifying models, the key will be locked to whatever the current effective models are at creation time. If the team's default_models or member's models change later, previously created keys will still have the old snapshot. This may be intentional, but it's a subtle behavior worth documenting — keys don't dynamically inherit updated effective models.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
No description provided.