fix(proxy): align team member add with existing user provisioning rules - #35435
Conversation
Adding a team member by a user_id with no user row created that row as a side effect for any caller permitted to add members, while creating users directly is restricted to proxy admins. Restrict that path to proxy admins too; adding an existing user, and inviting a new one by user_email (where the user_id is allocated server-side), are unchanged. Also record the membership change, and any user row it creates, in the audit log, matching /team/update, /user/new and /key/*.
Greptile SummaryThe PR aligns team-member additions with existing user-provisioning permissions and adds audit records for membership and user creation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/team_endpoints.py | Adds bulk user-ID resolution, provisioning authorization, pre-existing-user classification, and audit logging to the team-member-add flow. |
| tests/proxy_unit_tests/test_proxy_server.py | Updates the existing team-admin test to expect rejection when a non-proxy administrator supplies an unused user ID. |
| tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py | Adds unit coverage for the new provisioning rules, bulk resolution, audit payload shape, and newly created-user classification. |
Reviews (2): Last reviewed commit: "refactor(proxy): resolve team member loo..." | Re-trigger Greptile
PR overviewThis PR updates the proxy team-management endpoint so adding team members follows the existing user provisioning rules. One security issue remains open: when audit logging is enabled, a team or organization administrator can submit a very large member list and trigger unbounded concurrent audit inserts, potentially exhausting the shared database connection pool. One prior issue has been addressed, but member-count limits or bounded batching are still needed. Open issues (1)
Fixed/addressed: 1 · PR risk: 4/10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… rejection message Resolve the requested member user_ids with a single find_many instead of one lookup per member, so a large member list no longer turns into that many round-trips before the permission check runs. Write the member-add audit entries concurrently rather than one after another, and list at most a few ids in the rejection message instead of echoing the whole request back. Update the team-admin member-add case that covered adding a user_id with no user row, which the endpoint now leaves to proxy admins.
QA verdict: PASS, with one product gap to decide onRan this branch against a live proxy on Proxy config used for the QA session model_list:
- model_name: anthropic-haiku-4-5
litellm_params:
model: anthropic/claude-haiku-4-5
api_key: os.environ/ANTHROPIC_API_KEY
general_settings:
master_key: sk-1234
litellm_settings:
store_audit_logs: trueDATABASE_URL="postgresql://litellm:litellm@127.0.0.1:5433/litellm" LITELLM_MASTER_KEY=sk-1234 \
uv run --no-sync python litellm/proxy/proxy_cli.py --config /home/ubuntu/qa35435/config.yaml --port 4000The regression this PR closes, before and after, same request and same callerBefore, on curl -sX POST "$BASE/team/member_add" -H "Authorization: Bearer $TEAM_ADMIN_KEY" \
-H 'Content-Type: application/json' \
-d '{"team_id":"3f87004a-...","member":{"user_id":"injected-b1","role":"user"}}'
# HTTP 200
curl -s "$BASE/user/info?user_id=injected-b1" -H "Authorization: Bearer sk-1234"
# HTTP 200, user_role: internal_userAfter, on this branch (port 4000) # HTTP 403
{"detail":{"error":"Only proxy admins can add a user_id that does not exist yet: injected-a1. Add the member by user_email to invite a new user, or ask a proxy admin to create the user first."}}
curl -s "$BASE/user/info?user_id=injected-a1" -H "Authorization: Bearer sk-1234"
# HTTP 404The team's Happy paths that stay open (all 200)Team admin adds an existing user by Sad pathsTeam admin unknown id 403; org admin unknown id 403; list containing one unknown id is rejected as a whole with 403 and the known member from that same list is not partially added; a new One expectation in the PR description reads slightly differently in practice: a member with neither Edge cases, bulk behaviour and the batched lookupThirteen unknown ids in one request produce a message capped at ten with "and 3 more"; a case-differing email matches the existing row instead of creating a duplicate; twenty brand-new email invites in one call succeed and create twenty rows; thirty existing users added in one call succeed The fan-out that the review bots flagged is genuinely fixed by I could not test the ambiguous-email case where one address matches two user rows, because Audit logging, with
|
…ers only Both fields select from a server-side search over existing accounts, so a typed-in address or id never becomes a value. Say so up front rather than letting the form look like it accepts a new user and fail on submit. Applies to the organization member modal too, which shares this component.
|
@veria-ai re review |
| after_value=_members_audit_value(after_members), | ||
| ) | ||
|
|
||
| await asyncio.gather(*created_user_entries, membership_entry) |
There was a problem hiding this comment.
Low: Unbounded audit-log fan-out
A team or organization admin can submit an arbitrarily large list of new email members, causing this call to schedule one audit database insert per member concurrently. When audit logging is enabled, this can exhaust the shared connection pool; cap the request's member count or write these entries in bounded batches.
TLDR
Problem this solves:
/team/member_addcreated user rows for non-proxy-admins/team/member_addwrote no audit log at allHow it solves it:
user_iduser_idis allocated server-sideRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Run against a live proxy on
localhost:4001. The caller is a team admin whose proxy-level role isinternal_user;sk-1234is the master key. Setup for both runs: create a team, create theinternal_user, make them a team admin of that teamBefore, at
f2cfa86713(base of this branch). The team admin picks an arbitraryuser_idandthe proxy creates a fully-roled user for it
After, at
e8e2e07ef6(this branch). Same request, same callerFlows that stay open, also at
e8e2e07ef6Audit log, at
e8e2e07ef6(needslitellm_settings.store_audit_logs: trueand a license). Ateam admin invites a new user by email; before this change
/auditreturned nothing for either themembership change or the created user
Type
🐛 Bug Fix
Changes
_validate_member_user_id_provisioningrejects auser_idwith no user row when the caller is nota proxy admin, so choosing the identifier for a new account lines up with the restriction that
already applies to creating one directly. Team and org admins keep both of the paths they actually
use day to day: adding someone who already exists, and inviting someone new by
user_email, wherethe
user_idis generated server-side rather than supplied by the caller_create_team_member_add_audit_logsrecords the membership change on the team and a creation entryfor any user row the request adds, which brings this endpoint in line with
/team/update,/user/newand/key/*. The set of ids that already existed is captured before the members areadded, because the list-payload branch of
_update_team_members_listback-fills the caller's ownMemberobjects with ids of users the same request just created; reading it afterwards wouldclassify a new user as pre-existing and skip its entry
Worth flagging for review: this is a behavior change for anyone pre-provisioning by a caller-chosen
user_idas a team or org admin rather than byuser_email, who now gets a 403. The internalcallers were checked and are unaffected, since
map_user_to_teams,create_team_member_add_taskand
_add_user_to_teameither pass a proxy-admin identity or run after the user row existsFinal Attestation