fix(scim): sync team roster and dedup teams for existing-user email upsert (#34183)

* fix(scim): sync team roster and dedup teams for existing-user email upsert

When POST /scim/v2/Users matched an already-existing user by email,
handle_existing_user_by_email raw-wrote the user's teams array but never
touched the team roster, so the user appeared in the group on their profile
yet was absent from the team directly (members_with_roles stayed empty). It
also did not dedup the teams built from repeated SCIM groups.

Route the existing-user team assignment through the same
_handle_team_membership_changes / team_member_add path the PUT update_user
handler uses, so members_with_roles, LiteLLM_TeamMembership, and the user's
teams array stay in sync, and dedup the teams derived from user.groups. The
user_id rewrite to the new userName is preserved and sequenced before the
roster sync so the roster never references a stale primary key.

* fix(scim): surface roster add failures on existing-user email upsert

Route the existing-email upsert's roster sync through patch_team_membership
with a new opt-in raise_on_error flag so a genuine team_member_add failure
propagates instead of being swallowed, and the deduped teams array is only
persisted after the roster sync succeeds. Without this, a failed add left the
endpoint reporting success while user.teams listed a team members_with_roles
never received.

The benign already-a-member case stays a no-op even under the strict path, and
the flag defaults to False so the PUT update_user, PATCH patch_user, and group
callers keep their existing best-effort behavior. SCIM POST is idempotent, so
surfacing the error lets the IdP retry and converge.

* fix(scim): surface roster removal failures symmetrically with adds

Make team_member_delete failures fail loud under the strict roster sync used by
the existing-email upsert, mirroring the add path, so a swallowed removal can no
longer let the user's teams array drop a team the roster still holds. The
idempotent case where the user is already absent from the team stays a no-op,
matching how an add treats the user already being in the team. Best-effort
behavior is preserved for the default raise_on_error=False callers.
This commit is contained in:
Yassin Kortam 2026-07-22 13:59:02 -07:00 • committed by GitHub
parent 38467631b6
commit 5abe5f82e1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 425 additions and 335 deletions

View file

@ -98,14 +98,27 @@ class UserProvisionerHelpers:
if not existing_user:
return None
# Update the user
new_teams = list(dict.fromkeys(new_user_request.teams or []))
if new_user_request.user_id != existing_user.user_id:
await UserRepository(prisma_client).table.update(
where={"user_id": existing_user.user_id},
data={"user_id": new_user_request.user_id},
)
await _handle_team_membership_changes(
user_id=new_user_request.user_id,
existing_teams=existing_user.teams or [],
new_teams=new_teams,
raise_on_error=True,
)
updated_user = await UserRepository(prisma_client).table.update(
where={"user_id": existing_user.user_id},
where={"user_id": new_user_request.user_id},
data={
"user_id": new_user_request.user_id,
"user_email": new_user_request.user_email,
"user_alias": new_user_request.user_alias,
"teams": new_user_request.teams,
"teams": new_teams,
"metadata": safe_dumps(new_user_request.metadata),
**({"user_role": new_user_request.user_role} if admin_group is not None else {}),
},
@ -440,7 +453,12 @@ async def _get_team_members_display(member_ids: List[str]) -> List[SCIMMember]:
return members
async def _handle_team_membership_changes(user_id: str, existing_teams: List[str], new_teams: List[str]) -> None:
async def _handle_team_membership_changes(
user_id: str,
existing_teams: List[str],
new_teams: List[str],
raise_on_error: bool = False,
) -> None:
"""Handle adding/removing user from teams based on changes."""
existing_teams_set = set(existing_teams)
new_teams_set = set(new_teams)
@ -453,6 +471,7 @@ async def _handle_team_membership_changes(user_id: str, existing_teams: List[str
user_id=user_id,
teams_ids_to_add_user_to=list(teams_to_add),
teams_ids_to_remove_user_from=list(teams_to_remove),
raise_on_error=raise_on_error,
)
@ -1497,16 +1516,29 @@ def _apply_patch_ops(
return update_data, final_team_set
def _is_user_not_in_team_error(exc: HTTPException) -> bool:
"""True when team_member_delete reports the user was already absent from the
team, which is the idempotent no-op case for a removal."""
detail = exc.detail
return isinstance(detail, dict) and detail.get("error") == "User not found in team"
async def patch_team_membership(
user_id: str,
teams_ids_to_add_user_to: List[str],
teams_ids_to_remove_user_from: List[str],
raise_on_error: bool = False,
) -> bool:
"""
Add or remove user from teams
Handles duplicate membership gracefully (idempotent operation).
If a user is already in a team, that's fine - we don't treat it as an error.
A user already being in a team (on add) or already absent from it (on
remove) is treated as a no-op, not an error.
When ``raise_on_error`` is True a genuine add or remove failure (anything
other than those idempotent no-ops) propagates instead of being swallowed,
so a caller can avoid persisting a teams array the roster never received.
"""
for _team_id in teams_ids_to_add_user_to:
try:
@ -1521,9 +1553,13 @@ async def patch_team_membership(
# Handle duplicate membership gracefully - this is idempotent
if e.type == ProxyErrorTypes.team_member_already_in_team:
verbose_proxy_logger.debug(f"User {user_id} is already in team {_team_id}, skipping add")
elif raise_on_error:
raise
else:
verbose_proxy_logger.exception(f"Error adding user to team {_team_id}: {e}")
except Exception as e:
if raise_on_error:
raise
verbose_proxy_logger.exception(f"Error adding user to team {_team_id}: {e}")
for _team_id in teams_ids_to_remove_user_from:
@ -1532,7 +1568,16 @@ async def patch_team_membership(
data=TeamMemberDeleteRequest(team_id=_team_id, user_id=user_id),
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
)
except HTTPException as e:
if _is_user_not_in_team_error(e):
verbose_proxy_logger.debug(f"User {user_id} is not in team {_team_id}, skipping remove")
elif raise_on_error:
raise
else:
verbose_proxy_logger.exception(f"Error removing user from team {_team_id}: {e}")
except Exception as e:
if raise_on_error:
raise
verbose_proxy_logger.exception(f"Error removing user from team {_team_id}: {e}")
return True