feat(proxy): support object_permission in default_key_generate_params - #31776
Conversation
default_key_generate_params filled in a fixed whitelist of scalar fields plus a full-replace for models/metadata, but never touched object_permission, so admins had no way to set a default (e.g. mcp_tool_search_enabled, vector_stores) applied to every new key. Merge object_permission field-by-field instead of replacing it wholesale, so a caller-supplied field (e.g. mcp_servers) is preserved alongside defaulted fields the caller left unset.
|
|
Greptile SummaryThis PR extends
Confidence Score: 5/5Safe to merge — the change is a small, well-scoped addition that merges admin-configured defaults into data_json after all caller-scope validation, with no modifications to existing validation logic. The insertion point is correct: default object_permission values are applied after team-scope checks, so they cannot trigger false-positive 403s for non-admin callers. Both the default value and the caller value are guarded with isinstance(..., dict) before any dict operations are performed. A shallow copy is used for the wholesale-apply case, leaving the shared config object unmodified. The four new tests cover the key behavioral contracts. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds object_permission merging from default_key_generate_params after team-scope validation; logic is correctly ordered and type-guarded. |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds four mock-based tests covering the absent/partial/explicit/non-admin cases for the new default object_permission merging behavior. |
Reviews (2): Last reviewed commit: "fix(proxy): apply default object_permiss..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c4d61f01b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…elated to this PR's diff)
Injecting the default before validate_key_vector_stores_against_team / validate_key_search_tools_against_team ran meant a default containing a team-scoped field (e.g. vector_stores) looked like a caller-requested permission, turning ordinary non-admin personal key creation into a 403. Merge the default into data_json after those checks instead, and guard against a non-dict default value.
|
@greptile review |
…BerriAI#31776) * feat(proxy): support object_permission in default_key_generate_params default_key_generate_params filled in a fixed whitelist of scalar fields plus a full-replace for models/metadata, but never touched object_permission, so admins had no way to set a default (e.g. mcp_tool_search_enabled, vector_stores) applied to every new key. Merge object_permission field-by-field instead of replacing it wholesale, so a caller-supplied field (e.g. mcp_servers) is preserved alongside defaulted fields the caller left unset. * ci: retrigger proxy_pass_through_endpoint_tests (suspected flake, unrelated to this PR's diff) * fix(proxy): apply default object_permission after team-scope validation Injecting the default before validate_key_vector_stores_against_team / validate_key_search_tools_against_team ran meant a default containing a team-scoped field (e.g. vector_stores) looked like a caller-requested permission, turning ordinary non-admin personal key creation into a 403. Merge the default into data_json after those checks instead, and guard against a non-dict default value.
Relevant issues
Linear ticket
Pre-Submission checklist
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Live proxy against a real Postgres DB, config with
litellm_settings.default_key_generate_params.object_permission.vector_stores: ["default-proof-vs"]:Key generated (as proxy admin) with no
object_permissionin the request picks up the default:Key generated with a partial
object_permission(onlyagentsset) keeps the caller's explicit field and still gets the defaulted field merged in, instead of the default replacing the whole object:A non-admin (
alice,internal_user) creating her own personal key with noobject_permissionin the request is not rejected (the default is only merged in after the caller-scope validation, so it's never mistaken for a caller-requested team-scoped permission):Type
🆕 New Feature
Changes
default_key_generate_params(config.yamllitellm_settings.default_key_generate_params) filled a fixed whitelist of scalar fields plus a full-replace formodels/metadata, but never touchedobject_permissionat all, so there was no way for an admin to set a defaultobject_permissionvalue (e.g.vector_stores, and once #31486 lands,mcp_tool_search_enabled) applied to every new key._common_key_generation_helperinkey_management_endpoints.pynow merges the defaultobject_permissionintodata_jsonfield by field, aftervalidate_key_mcp_servers_against_team/validate_key_search_tools_against_team/validate_key_vector_stores_against_teamrun. Doing the merge after those checks (rather than earlier on the request object) matters: those checks reject team-scoped fields likevector_storeson a personal key from a non-admin caller, and an admin-configured default merged in before the checks would look exactly like a caller-requested permission, turning ordinary non-admin personal key creation into a 403. If the caller didn't setobject_permission, the default is applied wholesale; if the caller set a partialobject_permission, only the fields they left unset are filled from the default, so an explicit field (e.g.mcp_servers) is never clobbered.Caught by Greptile review (missing
isinstanceguard on a config-sourced value) and Codex review (the ordering issue described above) - both addressed in a follow-up commit with a regression test for the non-admin case.