From ff4a50c7689fc4fb61ea7b1b62de1bcf126d5d6d Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Sat, 1 Aug 2026 14:39:47 -0700 Subject: [PATCH] test(proxy): separate the member_add permission gate from the provisioning gate Adding a team member by a user_id with no user row is now proxy-admin-only, so the /team/member_add authz matrix, which targeted a never-seeded user_id, started 403ing every non-proxy-admin caller. Seed the member as a real user row so the matrix reads _validate_team_member_add_permissions alone; leaving it unseeded and relaxing the expectations to 403 would have left all 18 rows green with that gate deleted outright. Cover the new gate at the HTTP boundary, where only the helper was pinned before: a team admin and an org admin both clear the permission check on the same team and are still refused an unprovisioned user_id, with no user row left behind. Pin the escape hatch that refusal names too, so closing the email-invite path for non-proxy-admins cannot pass silently. Promote the user seeder the member-info pins had kept private to conftest, and reclaim invited users by their scratch-prefixed email, since an invite allocates the user_id server-side. --- tests/proxy_behavior/management/conftest.py | 37 +++++- .../management/test_team_member_add.py | 116 +++++++++++++++++- .../test_team_member_info_validation.py | 31 +---- 3 files changed, 156 insertions(+), 28 deletions(-) diff --git a/tests/proxy_behavior/management/conftest.py b/tests/proxy_behavior/management/conftest.py index 5b33ee6fd89..4c5b2ee9949 100644 --- a/tests/proxy_behavior/management/conftest.py +++ b/tests/proxy_behavior/management/conftest.py @@ -194,6 +194,33 @@ async def create_scratch_team( return team_id +async def create_scratch_user( + prisma, + scratch_prefix: str, + *, + suffix: str, + user_email: Optional[str] = None, +) -> str: + """Raw-seed a scratch-tagged user row with no key; returns its user_id. + + The passive counterpart to create_scratch_actor: a user that requests are + made *about* rather than *by*, so it needs no verification token. The + user_id matches Scratch.tag(suffix), so the teardown reclaims it. + + A member named by user_id must already exist for a non-proxy-admin caller + — `_validate_member_user_id_provisioning` 403s a user_id with no user row + — so an authz matrix targeting a member has to seed one, or every + non-proxy-admin row 403s on provisioning and the permission gate under + test goes unexercised. + """ + user_id = f"{scratch_prefix}-{suffix}" + data: Dict[str, Any] = {"user_id": user_id, "user_role": "internal_user"} + if user_email is not None: + data["user_email"] = user_email + await prisma.db.litellm_usertable.create(data=data) + return user_id + + async def create_scratch_org( prisma, scratch_prefix: str, @@ -331,8 +358,16 @@ async def scratch(prisma): await prisma.db.litellm_teamtable.delete_many( where={"team_id": {"startswith": handle.prefix}} ) + # Inviting a member by user_email allocates the user_id server-side + # (a uuid), so a scratch-prefixed email is the only handle on that + # row — sweep both, matching the token sweep above. await prisma.db.litellm_usertable.delete_many( - where={"user_id": {"startswith": handle.prefix}} + where={ + "OR": [ + {"user_id": {"startswith": handle.prefix}}, + {"user_email": {"startswith": handle.prefix}}, + ] + } ) # F1+F3 seed scratch orgs via create_scratch_org; the world seeder is # the only other writer of LiteLLM_OrganizationTable and uses the diff --git a/tests/proxy_behavior/management/test_team_member_add.py b/tests/proxy_behavior/management/test_team_member_add.py index a0dc4a7ecaf..3309aa11cb0 100644 --- a/tests/proxy_behavior/management/test_team_member_add.py +++ b/tests/proxy_behavior/management/test_team_member_add.py @@ -2,7 +2,7 @@ import litellm import pytest from .actors import Actor -from .conftest import create_scratch_team +from .conftest import create_scratch_team, create_scratch_user pytestmark = pytest.mark.asyncio(loop_scope="session") @@ -12,6 +12,12 @@ pytestmark = pytest.mark.asyncio(loop_scope="session") # or an org admin of the team's org may add members; everyone else is 403. # Unlike /team/update there is no route gate in front, so the team-admin # branch is reachable (TEAM_ADMIN, an internal_user, is allowed on its team). +# +# The member being added is seeded as a real user row so this matrix reads the +# permission gate alone. Naming a user_id with no user row would 403 every +# non-proxy-admin on the downstream provisioning gate instead, which would +# leave the matrix green even with _validate_team_member_add_permissions +# deleted. That gate gets its own matrix below. _MATRIX = [ ("alpha/proxy_admin", Actor.PROXY_ADMIN, "alpha", 200), ("alpha/org_admin", Actor.ORG_ADMIN, "alpha", 200), @@ -68,7 +74,9 @@ async def test_team_member_add_authz_matrix( ): await _seed_target(prisma, world, shape, scratch.prefix) caller = world.keys[actor] - new_member_id = scratch.tag("newmember") + new_member_id = await create_scratch_user( + prisma, scratch.prefix, suffix="newmember" + ) resp = await proxy_client.post( "/team/member_add", @@ -92,6 +100,110 @@ async def test_team_member_add_authz_matrix( assert new_member_id not in _member_ids(row), "denied but member added" +# Naming a user_id with no user row creates that user as a side effect, so +# _validate_member_user_id_provisioning restricts it to PROXY_ADMIN — creating +# users outright is proxy-admin-only, and member_add must not be a way around +# that. Every actor here clears the permission gate on the alpha team (all +# three are 200 in the matrix above), so the only thing separating them is the +# provisioning gate. +_UNPROVISIONED = [ + ("proxy_admin", Actor.PROXY_ADMIN, 200), + ("team_admin", Actor.TEAM_ADMIN, 403), + ("org_admin", Actor.ORG_ADMIN, 403), +] + + +@pytest.mark.parametrize( + "actor,expected_status", + [(a, s) for (_id, a, s) in _UNPROVISIONED], + ids=[s[0] for s in _UNPROVISIONED], +) +async def test_team_member_add_unprovisioned_user_id_is_proxy_admin_only( + actor: Actor, + expected_status: int, + proxy_client, + prisma, + scratch, + world, +): + await _seed_target(prisma, world, "alpha", scratch.prefix) + caller = world.keys[actor] + # Deliberately NOT seeded — create_scratch_user is what the matrix above + # calls, and its absence here is the whole scenario. + unprovisioned_id = scratch.tag("unprovisioned") + assert ( + await prisma.db.litellm_usertable.find_unique( + where={"user_id": unprovisioned_id} + ) + is None + ), "setup: user must not exist yet" + + resp = await proxy_client.post( + "/team/member_add", + headers={"Authorization": f"Bearer {caller.cleartext}"}, + json={ + "team_id": scratch.prefix, + "member": {"user_id": unprovisioned_id, "role": "user"}, + }, + ) + assert ( + resp.status_code == expected_status + ), f"{actor.value}: {resp.status_code} {resp.text}" + + row = await prisma.db.litellm_teamtable.find_unique( + where={"team_id": scratch.prefix} + ) + assert row is not None + created = await prisma.db.litellm_usertable.find_unique( + where={"user_id": unprovisioned_id} + ) + if expected_status == 200: + assert unprovisioned_id in _member_ids(row) + assert created is not None, "proxy admin add did not provision the user" + else: + assert unprovisioned_id not in _member_ids(row), "denied but member added" + # The point of the gate: a denied caller must not leave a user row behind. + assert created is None, "denied but user row was created" + + +async def test_team_member_add_email_invite_open_to_team_admin( + proxy_client, + prisma, + scratch, + world, +): + """The escape hatch the provisioning 403 names must actually work. + + That message tells a non-proxy-admin to add the member by user_email + instead. Inviting an unknown email allocates the user_id server-side, so + it stays open to team admins; if it ever closed, the 403 would be sending + callers down a dead end. + """ + await _seed_target(prisma, world, "alpha", scratch.prefix) + caller = world.keys[Actor.TEAM_ADMIN] + email = f"{scratch.prefix}-invitee@example.com" + + resp = await proxy_client.post( + "/team/member_add", + headers={"Authorization": f"Bearer {caller.cleartext}"}, + json={ + "team_id": scratch.prefix, + "member": {"user_email": email, "role": "user"}, + }, + ) + assert resp.status_code == 200, resp.text + + invited = await prisma.db.litellm_usertable.find_first( + where={"user_email": email} + ) + assert invited is not None, "invite did not create the user" + row = await prisma.db.litellm_teamtable.find_unique( + where={"team_id": scratch.prefix} + ) + assert row is not None + assert invited.user_id in _member_ids(row) + + # Available-team self-join: a non-admin caller may add ITSELF to a team listed # in litellm.default_internal_user_params["available_teams"], but the bypass # must not escalate to role=admin or inject another user. diff --git a/tests/proxy_behavior/management/test_team_member_info_validation.py b/tests/proxy_behavior/management/test_team_member_info_validation.py index 5139865d2b4..b22e3579400 100644 --- a/tests/proxy_behavior/management/test_team_member_info_validation.py +++ b/tests/proxy_behavior/management/test_team_member_info_validation.py @@ -11,33 +11,14 @@ shape: a payload that silently lands the membership against the WRONG user_id is invisible from response-body alone). """ -from typing import Any, Dict, Optional - import pytest from .actors import Actor -from .conftest import create_scratch_team +from .conftest import create_scratch_team, create_scratch_user pytestmark = pytest.mark.asyncio(loop_scope="session") -async def _seed_scratch_user( - prisma, - scratch_prefix: str, - *, - suffix: str, - user_email: Optional[str] = None, -) -> str: - """Raw-seed a scratch-prefixed user row; returns user_id. Scratch teardown - reclaims by user_id prefix.""" - user_id = f"{scratch_prefix}-{suffix}" - data: Dict[str, Any] = {"user_id": user_id, "user_role": "internal_user"} - if user_email is not None: - data["user_email"] = user_email - await prisma.db.litellm_usertable.create(data=data) - return user_id - - # --------------------------------------------------------------------------- # Both None → 400 ("Either user_id or user_email must be provided") # --------------------------------------------------------------------------- @@ -68,10 +49,10 @@ async def test_member_add_email_id_mismatch_rejected( proxy_client, prisma, scratch, world ): email = f"{scratch.prefix}-mismatch@example.com" - real_user_id = await _seed_scratch_user( + real_user_id = await create_scratch_user( prisma, scratch.prefix, suffix="real", user_email=email ) - other_user_id = await _seed_scratch_user(prisma, scratch.prefix, suffix="other") + other_user_id = await create_scratch_user(prisma, scratch.prefix, suffix="other") assert real_user_id != other_user_id # sanity team_id = await create_scratch_team(prisma, team_id=scratch.tag("team")) seeder = world.keys[Actor.PROXY_ADMIN].cleartext @@ -102,7 +83,7 @@ async def test_member_add_email_only_resolves_user_id( proxy_client, prisma, scratch, world ): email = f"{scratch.prefix}-resolve@example.com" - user_id = await _seed_scratch_user( + user_id = await create_scratch_user( prisma, scratch.prefix, suffix="lookup", user_email=email ) team_id = await create_scratch_team(prisma, team_id=scratch.tag("team")) @@ -171,8 +152,8 @@ async def test_member_add_duplicate_email_rejected( proxy_client, prisma, scratch, world ): email = f"{scratch.prefix}-dup@example.com" - await _seed_scratch_user(prisma, scratch.prefix, suffix="dup1", user_email=email) - await _seed_scratch_user(prisma, scratch.prefix, suffix="dup2", user_email=email) + await create_scratch_user(prisma, scratch.prefix, suffix="dup1", user_email=email) + await create_scratch_user(prisma, scratch.prefix, suffix="dup2", user_email=email) team_id = await create_scratch_team(prisma, team_id=scratch.tag("team")) seeder = world.keys[Actor.PROXY_ADMIN].cleartext resp = await proxy_client.post(