mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 13:47:19 +08:00
fix(backend): ensure members are removed from the corresponding litellm team when removed from an organization (#12996)
This commit is contained in:
@@ -16,10 +16,12 @@ from server.routes.org_models import (
|
||||
OrgMemberUpdate,
|
||||
RoleNotFoundError,
|
||||
)
|
||||
from storage.lite_llm_manager import LiteLlmManager
|
||||
from storage.org_member_store import OrgMemberStore
|
||||
from storage.role_store import RoleStore
|
||||
from storage.user_store import UserStore
|
||||
|
||||
from openhands.core.logger import openhands_logger as logger
|
||||
from openhands.utils.async_utils import call_sync_from_async
|
||||
|
||||
|
||||
@@ -176,7 +178,34 @@ class OrgMemberService:
|
||||
|
||||
return True, None
|
||||
|
||||
return await call_sync_from_async(_remove_member)
|
||||
success, error = await call_sync_from_async(_remove_member)
|
||||
|
||||
# If database removal succeeded, also remove from LiteLLM team
|
||||
if success:
|
||||
try:
|
||||
await LiteLlmManager.remove_user_from_team(
|
||||
str(target_user_id), str(org_id)
|
||||
)
|
||||
logger.info(
|
||||
'Successfully removed user from LiteLLM team',
|
||||
extra={
|
||||
'user_id': str(target_user_id),
|
||||
'org_id': str(org_id),
|
||||
},
|
||||
)
|
||||
except Exception as e:
|
||||
# Log but don't fail the operation - database removal already succeeded
|
||||
# LiteLLM state will be eventually consistent
|
||||
logger.warning(
|
||||
'Failed to remove user from LiteLLM team',
|
||||
extra={
|
||||
'user_id': str(target_user_id),
|
||||
'org_id': str(org_id),
|
||||
'error': str(e),
|
||||
},
|
||||
)
|
||||
|
||||
return success, error
|
||||
|
||||
@staticmethod
|
||||
async def update_org_member(
|
||||
|
||||
@@ -1159,6 +1159,149 @@ class TestOrgMemberServiceRemoveOrgMember:
|
||||
assert error is None
|
||||
mock_update_org.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_successful_removal_calls_litellm_remove_user_from_team(
|
||||
self,
|
||||
org_id,
|
||||
current_user_id,
|
||||
target_user_id,
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
owner_role,
|
||||
member_role,
|
||||
):
|
||||
"""Test that LiteLLM remove_user_from_team is called after successful database removal."""
|
||||
# Arrange
|
||||
with (
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.get_org_member'
|
||||
) as mock_get_member,
|
||||
patch(
|
||||
'server.services.org_member_service.RoleStore.get_role_by_id'
|
||||
) as mock_get_role,
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.remove_user_from_org'
|
||||
) as mock_remove,
|
||||
patch(
|
||||
'server.services.org_member_service.UserStore.get_user_by_id'
|
||||
) as mock_get_user,
|
||||
patch(
|
||||
'server.services.org_member_service.LiteLlmManager.remove_user_from_team',
|
||||
new_callable=AsyncMock,
|
||||
) as mock_litellm_remove,
|
||||
):
|
||||
mock_get_member.side_effect = [
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
]
|
||||
mock_get_role.side_effect = [owner_role, member_role]
|
||||
mock_remove.return_value = True
|
||||
mock_get_user.return_value = None
|
||||
|
||||
# Act
|
||||
success, error = await OrgMemberService.remove_org_member(
|
||||
org_id, target_user_id, current_user_id
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert success is True
|
||||
mock_litellm_remove.assert_called_once_with(
|
||||
str(target_user_id), str(org_id)
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_litellm_failure_does_not_fail_removal(
|
||||
self,
|
||||
org_id,
|
||||
current_user_id,
|
||||
target_user_id,
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
owner_role,
|
||||
member_role,
|
||||
):
|
||||
"""Test that LiteLLM failure doesn't fail the overall removal operation."""
|
||||
# Arrange
|
||||
with (
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.get_org_member'
|
||||
) as mock_get_member,
|
||||
patch(
|
||||
'server.services.org_member_service.RoleStore.get_role_by_id'
|
||||
) as mock_get_role,
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.remove_user_from_org'
|
||||
) as mock_remove,
|
||||
patch(
|
||||
'server.services.org_member_service.UserStore.get_user_by_id'
|
||||
) as mock_get_user,
|
||||
patch(
|
||||
'server.services.org_member_service.LiteLlmManager.remove_user_from_team',
|
||||
new_callable=AsyncMock,
|
||||
) as mock_litellm_remove,
|
||||
):
|
||||
mock_get_member.side_effect = [
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
]
|
||||
mock_get_role.side_effect = [owner_role, member_role]
|
||||
mock_remove.return_value = True
|
||||
mock_get_user.return_value = None
|
||||
mock_litellm_remove.side_effect = Exception('LiteLLM API error')
|
||||
|
||||
# Act
|
||||
success, error = await OrgMemberService.remove_org_member(
|
||||
org_id, target_user_id, current_user_id
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert success is True
|
||||
assert error is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_database_failure_skips_litellm_call(
|
||||
self,
|
||||
org_id,
|
||||
current_user_id,
|
||||
target_user_id,
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
owner_role,
|
||||
member_role,
|
||||
):
|
||||
"""Test that LiteLLM is not called when database removal fails."""
|
||||
# Arrange
|
||||
with (
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.get_org_member'
|
||||
) as mock_get_member,
|
||||
patch(
|
||||
'server.services.org_member_service.RoleStore.get_role_by_id'
|
||||
) as mock_get_role,
|
||||
patch(
|
||||
'server.services.org_member_service.OrgMemberStore.remove_user_from_org'
|
||||
) as mock_remove,
|
||||
patch(
|
||||
'server.services.org_member_service.LiteLlmManager.remove_user_from_team',
|
||||
new_callable=AsyncMock,
|
||||
) as mock_litellm_remove,
|
||||
):
|
||||
mock_get_member.side_effect = [
|
||||
requester_membership_owner,
|
||||
target_membership_user,
|
||||
]
|
||||
mock_get_role.side_effect = [owner_role, member_role]
|
||||
mock_remove.return_value = False
|
||||
|
||||
# Act
|
||||
success, error = await OrgMemberService.remove_org_member(
|
||||
org_id, target_user_id, current_user_id
|
||||
)
|
||||
|
||||
# Assert
|
||||
assert success is False
|
||||
mock_litellm_remove.assert_not_called()
|
||||
|
||||
|
||||
class TestOrgMemberServiceCanRemoveMember:
|
||||
"""Test cases for OrgMemberService._can_remove_member."""
|
||||
|
||||
Reference in New Issue
Block a user