diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index 99e5f0a4b2a..a7ee0170325 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -10,6 +10,7 @@ from copy import deepcopy from dataclasses import dataclass from functools import partial from itertools import chain +from types import MappingProxyType from typing import TYPE_CHECKING, Final, NamedTuple, Protocol, overload from fastapi import ( @@ -2708,6 +2709,75 @@ async def delete_group( raise handle_exception_on_proxy(e) +GROUP_PATCH_READ_ONLY_ATTRIBUTES: Final = frozenset({"id", "schemas", "meta"}) +_NO_FIELDS: Final[Mapping[str, object]] = MappingProxyType({}) + + +def _pathless_group_resource(op: SCIMPatchOperation) -> Mapping[str, object] | None: + """The partial Group resource a path-less op carries, or None when the op names a path. + + RFC 7644 Section 3.5.2 lets ``add`` and ``replace`` omit ``path`` and send the + attributes to apply as an object (what Okta Push Groups does on a rename); + ``remove`` always needs a path (Section 3.5.2.2). + """ + if op.path: + return None + resource: Final = _json_object_fields(op.value) + if op.op != "remove" and resource is not None: + return resource + detail: Final[_ScimErrorDetail] = { + "error": ( + "A remove operation requires a 'path' (RFC 7644 Section 3.5.2.2)" + if op.op == "remove" + else f"A {op.op} operation without a 'path' requires an object 'value' (RFC 7644 Section 3.5.2)" + ) + } + raise HTTPException(status_code=400, detail=detail) + + +def _group_patch_attribute_values(op: SCIMPatchOperation) -> tuple[tuple[str, object], ...]: + """The (attribute, value) pairs an operation applies, one per key of a path-less value.""" + resource: Final = _pathless_group_resource(op) + if resource is None: + return (((op.path or "").lower(), op.value),) + return tuple( + (key.lower(), value) + for key, value in resource.items() + if key and key.lower() not in GROUP_PATCH_READ_ONLY_ATTRIBUTES + ) + + +def _replaces_members(op: SCIMPatchOperation) -> bool: + if op.op != "replace": + return False + return any(attribute.startswith("members") for attribute, _ in _group_patch_attribute_values(op)) + + +def _patched_group_snapshot( + existing_snapshot: Mapping[str, object], + pathless_resources: Sequence[Mapping[str, object]], + mirrored_values: Sequence[tuple[str, object | None]], +) -> dict[str, object]: + """The ``scim_data`` snapshot after a PATCH: the path-less resources merged over the + existing snapshot in operation order (``members`` live in members_with_roles), then + each attribute in ``mirrored_values`` set to what the whole operation list left on + the team, so a later path op wins over an earlier path-less value; ``None`` drops it. + """ + pathless_items: Final = ( + (key, value) + for key, value in chain.from_iterable(resource.items() for resource in pathless_resources) + if key.lower() != "members" + ) + mirrored_keys: Final = frozenset(key.lower() for key, _ in mirrored_values) + kept: Final = ( + (key, value) + for key, value in chain(existing_snapshot.items(), pathless_items) + if key.lower() not in mirrored_keys + ) + refreshed: Final = ((key, value) for key, value in mirrored_values if value is not None) + return dict(chain(kept, refreshed)) + + async def _process_group_patch_operations( patch_ops: SCIMPatchOp, existing_team: LiteLLM_TeamTable, prisma_client: PrismaClient ) -> tuple[dict[str, object], set[str], set[str] | None]: @@ -2725,11 +2795,24 @@ async def _process_group_patch_operations( conditional on what the id turns out to be and leave members we should never have admitted - the phantom users this endpoint used to create for nested groups - impossible to clean up. + + A path-less op carries a partial Group resource: each attribute applies as if + sent with that path, and its attributes other than ``members`` (the roster + lives in members_with_roles) are merged in operation order into the + ``scim_data`` snapshot the PUT path writes, whose displayName and externalId + then mirror what the whole operation list left on the team. An empty metadata + key left behind by an earlier path-less op (stored whole under ``""``) is + dropped. """ update_data: Final[dict[str, object]] = {} + stored_metadata: Final[dict[str, object] | None] = existing_team.metadata + existing_metadata: Final = _json_object_fields(stored_metadata) or _NO_FIELDS + pathless_resources: Final = tuple( + resource for resource in map(_pathless_group_resource, patch_ops.Operations) if resource is not None + ) - # Create a fresh copy of existing metadata to avoid Prisma issues - metadata: Final = {**(existing_team.metadata or {}), SCIM_MANAGED_TEAM_METADATA_KEY: True} + kept_metadata_items: Final = ((key, value) for key, value in existing_metadata.items() if key) + metadata: Final = dict(chain(kept_metadata_items, ((SCIM_MANAGED_TEAM_METADATA_KEY, True),))) # Track member changes. members_with_roles is the source of truth for team # membership; the legacy `members` column is not populated by team creation @@ -2739,58 +2822,69 @@ async def _process_group_patch_operations( current_members: Final = set(await _get_team_member_user_ids_from_team(existing_team)) final_members = current_members.copy() - # Process each patch operation for op in patch_ops.Operations: - path = (op.path or "").lower() - value = op.value - op_type = op.op + for attribute, value in _group_patch_attribute_values(op): + op_type = op.op - if path == "displayname": - if op_type == "remove": - update_data["team_alias"] = None - else: - update_data["team_alias"] = str(value) - elif path == "externalid": - if op_type == "remove": - metadata.pop("externalId", None) - else: - metadata["externalId"] = str(value) - elif path.startswith("members"): - # Handle member operations - patched_members = ( - _parse_member_entries(value) - if value is not None - else tuple( - SCIMMember(value=member_id) for member_id in _extract_ids_from_path_filter(op.path, "members") + if attribute == "displayname": + if op_type == "remove": + update_data["team_alias"] = None + else: + update_data["team_alias"] = str(value) + elif attribute == "externalid": + if op_type == "remove": + metadata.pop("externalId", None) + else: + metadata["externalId"] = str(value) + elif attribute.startswith("members"): + patched_members = ( + _parse_member_entries(value) + if value is not None + else tuple( + SCIMMember(value=member_id) for member_id in _extract_ids_from_path_filter(op.path, "members") + ) ) + + if op_type == "remove": + final_members = final_members - await _member_ids_to_drop( + patched_members, frozenset(final_members), prisma_client + ) + else: + member_result = await _resolve_group_member_ids( + members=patched_members, + created_via="scim_group_patch", + prisma_client=prisma_client, + ) + if op_type == "replace": + final_members = set(member_result.all_member_ids) + elif op_type == "add": + final_members = final_members | set(member_result.all_member_ids) + elif op_type == "remove": + metadata.pop(attribute, None) + else: + metadata[attribute] = value + + if pathless_resources: + applied_attributes: Final = frozenset( + attribute for attribute, _ in chain.from_iterable(map(_group_patch_attribute_values, patch_ops.Operations)) + ) + mirrored_values: Final = tuple( + (snapshot_key, final_value) + for attribute, snapshot_key, final_value in ( + ("displayname", "displayName", update_data.get("team_alias")), + ("externalid", "externalId", metadata.get("externalId")), ) - - if op_type == "remove": - final_members = final_members - await _member_ids_to_drop( - patched_members, frozenset(final_members), prisma_client - ) - else: - member_result = await _resolve_group_member_ids( - members=patched_members, - created_via="scim_group_patch", - prisma_client=prisma_client, - ) - if op_type == "replace": - final_members = set(member_result.all_member_ids) - elif op_type == "add": - final_members = final_members | set(member_result.all_member_ids) - else: - # Handle other generic metadata - if op_type == "remove": - metadata.pop(path, None) - else: - metadata[path] = value + if attribute in applied_attributes + ) + metadata[SCIM_TEAM_DATA_METADATA_KEY] = _patched_group_snapshot( + existing_snapshot=_json_object_fields(existing_metadata.get(SCIM_TEAM_DATA_METADATA_KEY)) or _NO_FIELDS, + pathless_resources=pathless_resources, + mirrored_values=mirrored_values, + ) update_data["metadata"] = metadata - member_replace_present: Final = any( - op.op == "replace" and (op.path or "").lower().startswith("members") for op in patch_ops.Operations - ) + member_replace_present: Final = any(map(_replaces_members, patch_ops.Operations)) replace_target: Final = set(final_members) if member_replace_present else None return update_data, final_members, replace_target diff --git a/tests/e2e/ui/tests/integrationCritical/expected.json b/tests/e2e/ui/tests/integrationCritical/expected.json index c6ee6051cd4..81d3be247fd 100644 --- a/tests/e2e/ui/tests/integrationCritical/expected.json +++ b/tests/e2e/ui/tests/integrationCritical/expected.json @@ -9,5 +9,6 @@ "tests/e2e/ui/tests/integrationCritical/mcpUserEnvVars.spec.ts::clearing credentials for a server deleted underneath the modal reports the failure without losing the page", "tests/e2e/ui/tests/integrationCritical/costOptimizationModelGroups.spec.ts::cache leakage by model merges a deployment's resolved and requested model names into its model group", "tests/e2e/ui/tests/integrationCritical/logsDrawerCredentialCanary.spec.ts::the Logs drawer renders the stored request without the deployment api_key", - "tests/e2e/ui/tests/integrationCritical/toolPoliciesUserColumn.spec.ts::the Tool Policies page names the user behind the key that discovered a tool" + "tests/e2e/ui/tests/integrationCritical/toolPoliciesUserColumn.spec.ts::the Tool Policies page names the user behind the key that discovered a tool", + "tests/e2e/ui/tests/integrationCritical/teamMetadataEmptyKey.spec.ts::the team settings form skips a metadata row with an empty key and saving drops the key" ] diff --git a/tests/e2e/ui/tests/integrationCritical/teamMetadataEmptyKey.spec.ts b/tests/e2e/ui/tests/integrationCritical/teamMetadataEmptyKey.spec.ts new file mode 100644 index 00000000000..d24ba963f05 --- /dev/null +++ b/tests/e2e/ui/tests/integrationCritical/teamMetadataEmptyKey.spec.ts @@ -0,0 +1,90 @@ +import { + test, + expect, + APIRequestContext, + Page as PlaywrightPage, +} from "@playwright/test"; +import { randomUUID } from "node:crypto"; + +const master = process.env.LITELLM_MASTER_KEY ?? "sk-integration-master"; +const headers = { Authorization: `Bearer ${master}` }; + +async function createTeamCarryingAnEmptyMetadataKey( + request: APIRequestContext, +): Promise { + const created = await request.post("/team/new", { + headers, + data: { + team_alias: `int_empty_key_${randomUUID().replace(/-/g, "").slice(0, 12)}`, + }, + }); + expect(created.ok(), await created.text()).toBe(true); + const teamId = (await created.json()).team_id as string; + const seeded = await request.post("/team/update", { + headers, + data: { + team_id: teamId, + metadata: { "": { displayName: "stale" }, env: "staging" }, + }, + }); + expect(seeded.ok(), await seeded.text()).toBe(true); + return teamId; +} + +async function teamMetadata( + request: APIRequestContext, + teamId: string, +): Promise> { + const response = await request.get(`/team/info?team_id=${teamId}`, { + headers, + }); + expect(response.ok(), await response.text()).toBe(true); + const json = await response.json(); + return (json.team_info?.metadata ?? {}) as Record; +} + +async function loginAsAdmin(page: PlaywrightPage): Promise { + await page.goto("/ui/login"); + await page.getByPlaceholder("Enter your username").fill("admin"); + await page.getByPlaceholder("Enter your password").fill(master); + await page.getByRole("button", { name: "Login", exact: true }).click(); + await expect(page).toHaveURL( + (url) => url.pathname.startsWith("/ui") && !url.pathname.includes("login"), + ); +} + +test("the team settings form skips a metadata row with an empty key and saving drops the key", async ({ + page, + request, +}) => { + const teamId = await createTeamCarryingAnEmptyMetadataKey(request); + try { + expect(Object.keys(await teamMetadata(request, teamId))).toContain(""); + await loginAsAdmin(page); + await page.goto(`/ui/models-and-endpoints?team=${teamId}`); + await page.getByRole("tab", { name: "Settings" }).click(); + await page.getByRole("button", { name: /edit settings/i }).click(); + await expect(page.getByLabel(/Team Name/)).toBeVisible(); + const keys = page.getByPlaceholder("Key", { exact: true }); + await expect(keys).toHaveCount(1); + await expect(keys.first()).toHaveValue("env"); + await expect( + page.getByPlaceholder("Value", { exact: true }).first(), + ).toHaveValue("staging"); + await page.getByRole("button", { name: "Save Changes" }).click(); + await expect + .poll(async () => { + const metadata = await teamMetadata(request, teamId); + return { hasEmptyKey: "" in metadata, env: metadata.env }; + }) + .toEqual({ hasEmptyKey: false, env: "staging" }); + } finally { + const removed = await request.post("/team/delete", { + headers, + data: { team_ids: [teamId] }, + }); + expect(removed.ok() || removed.status() === 404, await removed.text()).toBe( + true, + ); + } +}); diff --git a/tests/integration/management/test_scim_group_pathless_patch.py b/tests/integration/management/test_scim_group_pathless_patch.py new file mode 100644 index 00000000000..5ed784d2bb8 --- /dev/null +++ b/tests/integration/management/test_scim_group_pathless_patch.py @@ -0,0 +1,549 @@ +import os +import signal +import threading +import uuid +from collections.abc import Callable, Mapping, Sequence +from concurrent.futures import ThreadPoolExecutor +from dataclasses import dataclass +from itertools import chain +from pathlib import Path +from types import MappingProxyType +from typing import Final + +import httpx +import psutil +import pytest +from integration._support.client import Gateway, Scenario, eventually, object_value, string_value +from integration._support.process import owned_proxy_process +from integration.authorization._guardrail_opt_out import upstream_hits +from pydantic import JsonValue + +PATCH_OP_SCHEMA: Final = "urn:ietf:params:scim:api:messages:2.0:PatchOp" +GROUP_SCHEMA: Final = "urn:ietf:params:scim:schemas:core:2.0:Group" +BURST_TEAMS: Final = 10 +BURST_REQUESTS_PER_TEAM: Final = 3 +KILL_AFTER_RESPONSES: Final = 5 + + +def patch_group( + candidate: Gateway, team: str, operations: Sequence[JsonValue], *, key: str | None = None +) -> httpx.Response: + return candidate.request( + "PATCH", + f"/scim/v2/Groups/{team}", + {"schemas": [PATCH_OP_SCHEMA], "Operations": list(operations)}, + key=key, + ) + + +def pathless(op: str, value: JsonValue) -> dict[str, JsonValue]: + return {"op": op, "value": value} + + +def pathed(op: str, path: str, value: JsonValue | None = None) -> dict[str, JsonValue]: + return {"op": op, "path": path, **({} if value is None else {"value": value})} + + +def team_info(candidate: Gateway, team: str) -> dict[str, JsonValue]: + return object_value(candidate.get("/team/info", {"team_id": team})["team_info"]) + + +def team_metadata(candidate: Gateway, team: str) -> dict[str, JsonValue]: + return object_value(team_info(candidate, team).get("metadata") or {}) + + +def team_alias(candidate: Gateway, team: str) -> JsonValue: + return team_info(candidate, team).get("team_alias") + + +def alias_and_metadata(candidate: Gateway, team: str) -> tuple[JsonValue, dict[str, JsonValue]]: + info: Final = team_info(candidate, team) + return info.get("team_alias"), object_value(info.get("metadata") or {}) + + +def member_ids(candidate: Gateway, team: str) -> frozenset[str]: + members: Final = team_info(candidate, team).get("members_with_roles") or [] + assert isinstance(members, list), members + return frozenset(string_value(object_value(member)["user_id"]) for member in members) + + +def group_member_ids(candidate: Gateway, team: str) -> frozenset[str]: + members: Final = candidate.get(f"/scim/v2/Groups/{team}").get("members") or [] + assert isinstance(members, list), members + return frozenset(string_value(object_value(member)["value"]) for member in members) + + +def scim_data(metadata: Mapping[str, JsonValue]) -> dict[str, JsonValue]: + return object_value(metadata["scim_data"]) + + +def scim_group(scenario: Scenario, members: Sequence[str]) -> str: + created: Final = scenario.gateway.request( + "POST", + "/scim/v2/Groups", + { + "schemas": [GROUP_SCHEMA], + "displayName": f"integration-{uuid.uuid4().hex}", + "members": [{"value": member} for member in members], + }, + ) + assert created.status_code == 201, created.text + team: Final = string_value(object_value(created.json())["id"]) + scenario.cleanups.callback(scenario.delete_team, team) + return team + + +def model_names(candidate: Gateway) -> frozenset[str]: + entries: Final = candidate.get("/model/info")["data"] + assert isinstance(entries, list), entries + return frozenset(string_value(object_value(entry)["model_name"]) for entry in entries) + + +def test_pathless_replace_renames_the_team_and_keeps_the_resource_under_scim_data(gateway: Gateway) -> None: + renamed: Final = f"okta-renamed-{uuid.uuid4().hex}" + external: Final = f"ext-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + response: Final = patch_group( + gateway, team, [pathless("replace", {"id": team, "displayName": renamed, "externalId": external})] + ) + assert response.status_code == 200, response.text + assert response.json()["displayName"] == renamed, response.text + assert team_alias(gateway, team) == renamed + metadata: Final = team_metadata(gateway, team) + assert set(metadata) == {"externalId", "scim_data", "scim_managed"}, metadata + assert metadata["externalId"] == external and metadata["scim_managed"] is True, metadata + assert scim_data(metadata) == {"id": team, "displayName": renamed, "externalId": external}, metadata + + +def test_pathless_replace_with_members_is_an_absolute_roster(gateway: Gateway) -> None: + renamed: Final = f"roster-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + first: Final = scenario.user(user_role="internal_user") + second: Final = scenario.user(user_role="internal_user") + team: Final = scim_group(scenario, [first]) + assert member_ids(gateway, team) == {first} + response: Final = patch_group( + gateway, team, [pathless("replace", {"displayName": renamed, "members": [{"value": second}]})] + ) + assert response.status_code == 200, response.text + assert member_ids(gateway, team) == {second} + assert group_member_ids(gateway, team) == {second} + assert team_alias(gateway, team) == renamed + metadata: Final = team_metadata(gateway, team) + assert "" not in metadata, metadata + assert "members" not in scim_data(metadata), metadata + + +def test_pathless_add_applies_the_attribute(gateway: Gateway) -> None: + external: Final = f"ext-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + alias: Final = team_alias(gateway, team) + response: Final = patch_group(gateway, team, [pathless("add", {"externalId": external})]) + assert response.status_code == 200, response.text + metadata: Final = team_metadata(gateway, team) + assert "" not in metadata, metadata + assert metadata["externalId"] == external, metadata + assert scim_data(metadata) == {"externalId": external}, metadata + assert team_alias(gateway, team) == alias + + +def test_pathed_operations_are_unchanged(gateway: Gateway) -> None: + renamed: Final = f"pathed-{uuid.uuid4().hex}" + external: Final = f"ext-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + first: Final = scenario.user(user_role="internal_user") + second: Final = scenario.user(user_role="internal_user") + team: Final = scim_group(scenario, [first]) + response: Final = patch_group( + gateway, + team, + [ + pathed("replace", "displayName", renamed), + pathed("replace", "externalId", external), + pathed("add", "members", [{"value": second}]), + pathed("remove", f'members[value eq "{first}"]'), + ], + ) + assert response.status_code == 200, response.text + assert response.json()["displayName"] == renamed, response.text + assert team_alias(gateway, team) == renamed + assert member_ids(gateway, team) == {second} + assert group_member_ids(gateway, team) == {second} + metadata: Final = team_metadata(gateway, team) + assert metadata["externalId"] == external, metadata + assert "" not in metadata, metadata + + +def test_pathless_patch_merges_into_the_put_snapshot(gateway: Gateway) -> None: + put_alias: Final = f"put-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + member: Final = scenario.user(user_role="internal_user") + team: Final = scim_group(scenario, [member]) + put: Final = gateway.request( + "PUT", + f"/scim/v2/Groups/{team}", + { + "schemas": [GROUP_SCHEMA], + "id": team, + "displayName": put_alias, + "externalId": "ext-v1", + "members": [{"value": member}], + }, + ) + assert put.status_code == 200, put.text + assert scim_data(team_metadata(gateway, team))["externalId"] == "ext-v1" + response: Final = patch_group(gateway, team, [pathless("add", {"externalId": "ext-v2"})]) + assert response.status_code == 200, response.text + metadata: Final = team_metadata(gateway, team) + snapshot: Final = scim_data(metadata) + assert "" not in metadata, metadata + assert metadata["externalId"] == "ext-v2", metadata + assert snapshot["displayName"] == put_alias and snapshot["externalId"] == "ext-v2", snapshot + assert team_alias(gateway, team) == put_alias + assert member_ids(gateway, team) == {member} + + +def test_group_patch_drops_the_empty_key_left_by_an_earlier_push(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + gateway.post("/team/update", {"team_id": team, "metadata": {"": {"displayName": "stale"}, "env": "staging"}}) + assert "" in team_metadata(gateway, team) + response: Final = patch_group(gateway, team, [pathed("replace", "externalId", "ext-after")]) + assert response.status_code == 200, response.text + metadata: Final = team_metadata(gateway, team) + assert "" not in metadata, metadata + assert metadata["env"] == "staging" and metadata["externalId"] == "ext-after", metadata + + +def test_later_path_op_wins_over_the_pathless_value(gateway: Gateway) -> None: + first: Final = f"first-{uuid.uuid4().hex}" + second: Final = f"second-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + response: Final = patch_group( + gateway, team, [pathless("replace", {"displayName": first}), pathed("replace", "displayName", second)] + ) + assert response.status_code == 200, response.text + assert response.json()["displayName"] == second, response.text + assert team_alias(gateway, team) == second + metadata: Final = team_metadata(gateway, team) + assert "" not in metadata, metadata + assert scim_data(metadata)["displayName"] == second, metadata + + +def test_read_only_attributes_do_not_become_metadata_keys(gateway: Gateway) -> None: + renamed: Final = f"readonly-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + response: Final = patch_group( + gateway, + team, + [ + pathless( + "replace", + { + "id": team, + "schemas": [GROUP_SCHEMA], + "meta": {"resourceType": "Group"}, + "displayName": renamed, + }, + ) + ], + ) + assert response.status_code == 200, response.text + metadata: Final = team_metadata(gateway, team) + assert set(metadata) == {"scim_data", "scim_managed"}, metadata + assert team_alias(gateway, team) == renamed + + +def test_pathless_rename_is_visible_from_the_peer_proxy(gateway: Gateway, peer: Gateway) -> None: + renamed: Final = f"peer-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + response: Final = patch_group(gateway, team, [pathless("replace", {"displayName": renamed})]) + assert response.status_code == 200, response.text + group: Final = eventually( + lambda: peer.get(f"/scim/v2/Groups/{team}"), lambda observed: observed.get("displayName") == renamed + ) + assert group["displayName"] == renamed, group + assert team_alias(peer, team) == renamed + assert "" not in team_metadata(peer, team) + + +def test_pathless_remove_is_rejected(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + before: Final = alias_and_metadata(gateway, team) + response: Final = patch_group(gateway, team, [pathless("remove", {"externalId": "ext-gone"})]) + assert response.status_code == 400, response.text + assert "RFC 7644 Section 3.5.2.2" in response.text, response.text + assert alias_and_metadata(gateway, team) == before + + +@pytest.mark.parametrize( + "operation", + [ + {"op": "replace", "value": "new-name"}, + {"op": "replace", "value": 7}, + {"op": "replace", "value": ["new-name"]}, + {"op": "replace", "value": ""}, + {"op": "replace", "value": "x" * 5120}, + {"op": "replace", "value": None}, + {"op": "replace"}, + {"op": "add", "value": "new-name"}, + ], + ids=["string", "int", "list", "empty-string", "5kb-string", "null", "missing", "add-string"], +) +def test_pathless_op_without_an_object_value_is_rejected(gateway: Gateway, operation: dict[str, JsonValue]) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + before: Final = alias_and_metadata(gateway, team) + response: Final = patch_group(gateway, team, [operation]) + assert response.status_code == 400, response.text + assert "RFC 7644 Section 3.5.2" in response.text, response.text + assert alias_and_metadata(gateway, team) == before + + +def test_pathless_empty_object_changes_nothing_but_marks_the_team(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + alias: Final = team_alias(gateway, team) + response: Final = patch_group(gateway, team, [pathless("replace", {})]) + assert response.status_code == 200, response.text + assert team_alias(gateway, team) == alias + metadata: Final = team_metadata(gateway, team) + assert metadata == {"scim_managed": True, "scim_data": {}}, metadata + + +def test_duplicate_pathless_ops_are_idempotent(gateway: Gateway) -> None: + external: Final = f"ext-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + response: Final = patch_group(gateway, team, [pathless("add", {"externalId": external})] * 2) + assert response.status_code == 200, response.text + metadata: Final = team_metadata(gateway, team) + assert metadata == {"scim_managed": True, "externalId": external, "scim_data": {"externalId": external}}, ( + metadata + ) + + +def test_unauthenticated_patch_changes_nothing(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + before: Final = alias_and_metadata(gateway, team) + response: Final = patch_group( + gateway, team, [pathless("replace", {"displayName": "intruder"})], key=f"sk-{uuid.uuid4().hex}" + ) + assert response.status_code == 401, response.text + assert alias_and_metadata(gateway, team) == before + + +def test_unknown_group_is_404(gateway: Gateway) -> None: + response: Final = patch_group( + gateway, f"missing-{uuid.uuid4().hex}", [pathless("replace", {"displayName": "ghost"})] + ) + assert response.status_code == 404, response.text + + +def test_malformed_patch_body_is_rejected(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + before: Final = alias_and_metadata(gateway, team) + response: Final = gateway.request( + "PATCH", f"/scim/v2/Groups/{team}", {"schemas": [PATCH_OP_SCHEMA], "Operations": "nope"} + ) + assert response.status_code in (400, 422), response.text + assert alias_and_metadata(gateway, team) == before + + +def test_pathless_and_pathed_scalar_coercion_agree(gateway: Gateway) -> None: + with gateway.scenario() as scenario: + via_pathless: Final = scenario.team() + via_path: Final = scenario.team() + first: Final = patch_group(gateway, via_pathless, [pathless("replace", {"displayName": 7})]) + second: Final = patch_group(gateway, via_path, [pathed("replace", "displayName", 7)]) + assert first.status_code == second.status_code == 200, (first.text, second.text) + assert team_alias(gateway, via_pathless) == team_alias(gateway, via_path), (first.text, second.text) + assert "" not in team_metadata(gateway, via_pathless) + + +def _patched_state( + candidate: Gateway, team: str, operations: Sequence[JsonValue] +) -> tuple[JsonValue, dict[str, JsonValue]]: + response: Final = patch_group(candidate, team, operations) + assert response.status_code == 200, response.text + return alias_and_metadata(candidate, team) + + +def test_repeated_pathless_patch_is_idempotent(gateway: Gateway) -> None: + renamed: Final = f"repeat-{uuid.uuid4().hex}" + with gateway.scenario() as scenario: + team: Final = scenario.team() + operations: Final = [pathless("replace", {"displayName": renamed, "externalId": "ext-repeat"})] + states: Final = tuple(_patched_state(gateway, team, operations) for _ in range(3)) + assert all(state == states[0] for state in states), states + assert states[0][0] == renamed, states + assert "" not in states[0][1], states + + +def test_concurrent_pathless_renames_converge(gateway: Gateway, peer: Gateway) -> None: + with gateway.scenario() as scenario: + team: Final = scenario.team() + aliases: Final = tuple(f"race-{index}-{uuid.uuid4().hex}" for index in range(10)) + candidates: Final = (gateway, peer) + with ThreadPoolExecutor(max_workers=len(aliases)) as pool: + responses: Final = tuple( + pool.map( + lambda indexed: patch_group( + candidates[indexed[0] % 2], team, [pathless("replace", {"displayName": indexed[1]})] + ), + enumerate(aliases), + ) + ) + assert all(response.status_code == 200 for response in responses), [ + response.text for response in responses if response.status_code != 200 + ] + alias: Final = team_alias(gateway, team) + assert alias in aliases, alias + metadata: Final = team_metadata(gateway, team) + assert "" not in metadata, metadata + assert scim_data(metadata)["displayName"] == alias, metadata + + +def test_team_key_keeps_serving_after_the_rename(gateway: Gateway, peer: Gateway) -> None: + with gateway.scenario() as scenario: + model: Final = scenario.model() + team: Final = scenario.team() + key: Final = scenario.key(team_id=team, models=[model]) + response: Final = patch_group( + gateway, team, [pathless("replace", {"displayName": f"serving-{uuid.uuid4().hex}"})] + ) + assert response.status_code == 200, response.text + eventually(lambda: model_names(peer), lambda names: model in names, seconds=60) + for candidate, marker in ((gateway, f"primary-{uuid.uuid4().hex}"), (peer, f"peer-{uuid.uuid4().hex}")): + completion: Final = candidate.chat(model, key=key, text=marker) + assert completion["choices"], completion + assert upstream_hits(gateway, marker) == 1, marker + + +@dataclass(frozen=True, slots=True) +class _Attempt: + team: str + alias: str + response: httpx.Response | None + + +class _KillSwitch: + def __init__(self, after: int, action: Callable[[], None]) -> None: + self._after: Final = after + self._action: Final = action + self._lock: Final = threading.Lock() + self._responses = 0 + + def tick(self) -> None: + with self._lock: + self._responses += 1 + if self._responses == self._after: + self._action() + + +def _burst_operations(alias: str, index: int) -> Sequence[JsonValue]: + shapes: Final = ( + [pathless("replace", {"displayName": alias, "externalId": f"ext-{index}"})], + [pathless("add", {"displayName": alias})], + [pathed("replace", "displayName", alias)], + ) + return shapes[index % len(shapes)] + + +def _attempt(candidate: Gateway, team: str, index: int, switch: _KillSwitch) -> _Attempt: + alias: Final = f"burst-{index}-{uuid.uuid4().hex}" + try: + return _Attempt(team, alias, patch_group(candidate, team, _burst_operations(alias, index))) + except httpx.TransportError: + return _Attempt(team, alias, None) + finally: + switch.tick() + + +def _team_attempts(candidate: Gateway, team: str, offset: int, switch: _KillSwitch) -> tuple[_Attempt, ...]: + return tuple(_attempt(candidate, team, offset + index, switch) for index in range(BURST_REQUESTS_PER_TEAM)) + + +def _burst(candidate: Gateway, teams: Sequence[str], disruption: Callable[[], None]) -> tuple[_Attempt, ...]: + switch: Final = _KillSwitch(KILL_AFTER_RESPONSES, disruption) + with ThreadPoolExecutor(max_workers=len(teams)) as pool: + per_team: Final = tuple( + pool.submit(_team_attempts, candidate, team, index * BURST_REQUESTS_PER_TEAM, switch) + for index, team in enumerate(teams) + ) + return tuple(chain.from_iterable(future.result() for future in per_team)) + + +def _burst_teams(scenario: Scenario) -> Mapping[str, str]: + origins: Final = tuple(f"origin-{uuid.uuid4().hex}" for _ in range(BURST_TEAMS)) + return MappingProxyType({scenario.team(team_alias=origin): origin for origin in origins}) + + +def _assert_team_reflected(candidate: Gateway, team: str, origin: str, sent: Sequence[_Attempt]) -> None: + aliases: Final = frozenset(attempt.alias for attempt in sent) + alias, metadata = alias_and_metadata(candidate, team) + assert "" not in metadata, metadata + if alias == origin: + assert all(attempt.response is None for attempt in sent), (team, sent) + assert "scim_managed" not in metadata, metadata + return + assert alias in aliases, (alias, aliases) + assert metadata["scim_managed"] is True, metadata + if sent[-1].response is not None: + assert alias == sent[-1].alias, (alias, sent[-1].alias) + snapshot: Final = metadata.get("scim_data") + if snapshot is not None: + assert object_value(snapshot).get("displayName") in aliases, snapshot + + +def _assert_burst_reflected(candidate: Gateway, teams: Mapping[str, str], attempts: Sequence[_Attempt]) -> None: + answered: Final = tuple(attempt for attempt in attempts if attempt.response is not None) + assert answered, "The whole burst failed to reach the proxy" + for attempt in answered: + assert attempt.response is not None and attempt.response.status_code == 200, attempt.response + assert attempt.response.json()["displayName"] == attempt.alias, attempt.response.text + for team, origin in teams.items(): + _assert_team_reflected(candidate, team, origin, tuple(attempt for attempt in attempts if attempt.team == team)) + + +def _patch_status(candidate: Gateway, team: str) -> int | None: + operations: Final = [pathless("replace", {"displayName": f"probe-{uuid.uuid4().hex}"})] + try: + return patch_group(candidate, team, operations).status_code + except httpx.TransportError: + return None + + +def _serving_workers(root: int) -> tuple[psutil.Process, ...]: + return tuple(child for child in psutil.Process(root).children() if child.children()) + + +@pytest.mark.timeout(240) +def test_worker_kill_mid_burst_keeps_serving_and_leaves_no_empty_key(gateway: Gateway, tmp_path: Path) -> None: + with owned_proxy_process(gateway, tmp_path, {}, workers=2) as owned, gateway.scenario() as scenario: + teams: Final = _burst_teams(scenario) + workers: Final = eventually( + lambda: _serving_workers(owned.process.pid), lambda children: len(children) >= 2, seconds=30 + ) + attempts: Final = _burst(owned.gateway, tuple(teams), lambda: os.kill(workers[0].pid, signal.SIGKILL)) + assert not workers[0].is_running() or workers[0].status() == psutil.STATUS_ZOMBIE, workers[0] + probe: Final = scenario.team() + eventually(lambda: _patch_status(owned.gateway, probe), lambda status_code: status_code == 200, seconds=60) + _assert_burst_reflected(owned.gateway, teams, attempts) + + +@pytest.mark.timeout(300) +def test_rolling_restart_mid_burst_drains_without_empty_keys(gateway: Gateway, tmp_path: Path) -> None: + with gateway.scenario() as scenario, owned_proxy_process(gateway, tmp_path, {}, workers=2) as replacement: + teams: Final = _burst_teams(scenario) + with owned_proxy_process(gateway, tmp_path, {}, workers=2) as retiring: + attempts: Final = _burst(retiring.gateway, tuple(teams), retiring.process.terminate) + _assert_burst_reflected(replacement.gateway, teams, attempts) diff --git a/tests/unit/proxy/management_endpoints/scim/test_scim_v2_endpoints.py b/tests/unit/proxy/management_endpoints/scim/test_scim_v2_endpoints.py index dbcf622bbb1..62d77a00f25 100644 --- a/tests/unit/proxy/management_endpoints/scim/test_scim_v2_endpoints.py +++ b/tests/unit/proxy/management_endpoints/scim/test_scim_v2_endpoints.py @@ -6167,3 +6167,203 @@ async def test_merge_placeholder_refuses_rows_that_are_not_a_lone_placeholder( assert reason in str(exc_info.value.message) team_member_add_mock.assert_not_awaited() prisma_client.db.litellm_usertable.delete.assert_not_awaited() + + +class _PatchedTeamRow: + def __init__(self, team: LiteLLM_TeamTable) -> None: + self.team = team + self.written: dict[str, object] = {} + + async def find_unique(self, *, where: dict[str, object]) -> LiteLLM_TeamTable: + return self.team + + async def update(self, *, where: dict[str, object], data: dict[str, object]) -> LiteLLM_TeamTable: + self.written = data + self.team = LiteLLM_TeamTable(**{**self.team.model_dump(), **data, "metadata": json.loads(str(data["metadata"]))}) + return self.team + + +@pytest.mark.asyncio +async def test_patch_group_pathless_replace_applies_attributes_and_drops_empty_key(mocker, monkeypatch): + """Okta Push Groups renames a group with a path-less ``replace`` whose value is a + partial Group resource. Each attribute must apply as if sent with its own path and + the resource must land in the ``scim_data`` snapshot, never whole under an empty + metadata key, and an empty key an earlier push left behind must be dropped so the + team saves from the Admin UI again.""" + from litellm.proxy import proxy_server + + group_id = "team-1" + existing_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="okta-push-group", + members=[], + members_with_roles=[Member(user_id="user1", role="user")], + metadata={ + "": {"id": group_id, "displayName": "okta-push-group-stale"}, + "scim_managed": True, + "scim_data": {"id": group_id, "displayName": "okta-push-group", "externalId": "ext-1"}, + }, + ) + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[ + SCIMPatchOperation( + op="replace", + value={"id": group_id, "displayName": "okta-push-group-renamed", "externalId": "ext-2"}, + ) + ], + ) + + team_rows = _PatchedTeamRow(existing_team) + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = team_rows + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock()) + mock_prisma_client.db.litellm_usertable.find_many = AsyncMock(return_value=()) + + monkeypatch.setattr(proxy_server, "prisma_client", mock_prisma_client) + mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership", AsyncMock()) + mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2._recompute_scim_member_roles", AsyncMock()) + + response = await patch_group(group_id=group_id, patch_ops=patch_ops) + + assert response.id == group_id + assert response.displayName == "okta-push-group-renamed" + written = team_rows.written + assert written["team_alias"] == "okta-push-group-renamed" + written_metadata = json.loads(written["metadata"]) + assert "" not in written_metadata + assert written_metadata["externalId"] == "ext-2" + assert written_metadata["scim_data"] == { + "id": group_id, + "displayName": "okta-push-group-renamed", + "externalId": "ext-2", + } + assert written_metadata["scim_managed"] is True + + +@pytest.mark.asyncio +async def test_process_group_patch_operations_pathless_replace_members_is_absolute(mocker, monkeypatch): + """A path-less ``replace`` carrying ``members`` declares the whole roster exactly like + ``replace`` with path ``members``, so it must be reported as the replace target, and the + read-only ``id`` it carries must never become a metadata key.""" + + async def mock_get_config(): + return {"litellm_settings": {"scim_upsert_user": True}} + + from litellm.proxy.proxy_server import proxy_config + + monkeypatch.setattr(proxy_config, "get_config", mock_get_config) + + existing_team = LiteLLM_TeamTable( + team_id="team-1", + team_alias="Team One", + members=[], + members_with_roles=[Member(user_id="old-user", role="user")], + ) + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="replace", value={"id": "team-1", "members": [{"value": "new-user"}]})], + ) + + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable.find_many = AsyncMock(return_value=(mocker.MagicMock(user_id="new-user"),)) + + update_data, final_members, replace_target = await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=mock_prisma_client, + ) + + assert final_members == {"new-user"} + assert replace_target == {"new-user"} + assert "id" not in update_data["metadata"] + assert "" not in update_data["metadata"] + assert update_data["metadata"]["scim_data"] == {"id": "team-1"} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("later_op", "expected_alias", "expected_external_id", "expected_snapshot"), + [ + ( + SCIMPatchOperation(op="replace", path="displayName", value="path-wins"), + "path-wins", + "ext-pathless", + {"id": "team-1", "displayName": "path-wins", "externalId": "ext-pathless"}, + ), + ( + SCIMPatchOperation(op="remove", path="displayName"), + None, + "ext-pathless", + {"id": "team-1", "externalId": "ext-pathless"}, + ), + ( + SCIMPatchOperation(op="replace", path="externalId", value="ext-path-wins"), + "pathless-name", + "ext-path-wins", + {"id": "team-1", "displayName": "pathless-name", "externalId": "ext-path-wins"}, + ), + ], +) +async def test_process_group_patch_operations_later_path_op_wins_over_pathless_snapshot( + mocker, later_op, expected_alias, expected_external_id, expected_snapshot +): + """Operations apply in order (RFC 7644 Section 3.5.2), so a path op after a path-less one + decides both the team's value and the ``scim_data`` snapshot; the snapshot must never keep + the path-less value the later op replaced or removed.""" + existing_team = LiteLLM_TeamTable( + team_id="team-1", + team_alias="Team One", + members=[], + members_with_roles=[], + metadata={"scim_managed": True, "scim_data": {"id": "team-1", "displayName": "Team One"}}, + ) + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[ + SCIMPatchOperation( + op="replace", + value={"id": "team-1", "displayName": "pathless-name", "externalId": "ext-pathless"}, + ), + later_op, + ], + ) + + update_data, _, _ = await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=mocker.MagicMock(), + ) + + assert update_data["team_alias"] == expected_alias + assert update_data["metadata"].get("externalId") == expected_external_id + assert update_data["metadata"]["scim_data"] == expected_snapshot + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("op", "value"), + [("remove", {"displayName": "okta-push-group"}), ("replace", "okta-push-group-renamed")], +) +async def test_process_group_patch_operations_rejects_pathless_op_it_cannot_apply(mocker, op, value): + """A path-less ``remove`` has no target and a path-less ``add``/``replace`` needs an + object value (RFC 7644 Section 3.5.2); neither may fall through to a metadata write + under an empty key.""" + existing_team = LiteLLM_TeamTable(team_id="team-1", team_alias="Team One", members=[], members_with_roles=[]) + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op=op, value=value)], + ) + + with pytest.raises(HTTPException) as exc: + await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=mocker.MagicMock(), + ) + + assert exc.value.status_code == 400 diff --git a/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.test.tsx b/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.test.tsx index 721653c6427..655d46ec8fe 100644 --- a/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.test.tsx +++ b/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.test.tsx @@ -52,6 +52,12 @@ describe("metadataObjectToPairs", () => { metadataObjectToPairs({ department: "research", logging: [{ callback_name: "langfuse" }] }, new Set(["logging"])), ).toEqual([{ key: "department", value: "research" }]); }); + + it("drops an empty key, which the form can never submit, so an API-written one does not block saving", () => { + expect(metadataObjectToPairs({ "": { displayName: "okta-push-group" }, scim_managed: true })).toEqual([ + { key: "scim_managed", value: "true" }, + ]); + }); }); describe("metadataPairsToObject", () => { diff --git a/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.tsx b/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.tsx index 1c67d48128f..6f20676c9e4 100644 --- a/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.tsx +++ b/ui/litellm-dashboard/src/components/common_components/MetadataKeyValueFields.tsx @@ -56,7 +56,7 @@ export function metadataObjectToPairs( excludedKeys: ReadonlySet = new Set(), ): MetadataPair[] { return Object.entries(metadata ?? {}) - .filter(([key]) => !excludedKeys.has(key)) + .filter(([key]) => key !== "" && !excludedKeys.has(key)) .map(([key, value]) => ({ key, value: formatMetadataValue(value) })); }