mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 05:37:20 +08:00
fix(backend): propagate LLM settings to all org members when admin saves settings (#13326)
This commit is contained in:
@@ -11,9 +11,10 @@ from pydantic import SecretStr
|
||||
from server.auth.token_manager import TokenManager
|
||||
from server.constants import LITE_LLM_API_URL
|
||||
from server.logger import logger
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy import select, update
|
||||
from sqlalchemy.orm import joinedload
|
||||
from storage.database import a_session_maker
|
||||
from storage.encrypt_utils import encrypt_value
|
||||
from storage.lite_llm_manager import LiteLlmManager, get_openhands_cloud_key_alias
|
||||
from storage.org import Org
|
||||
from storage.org_member import OrgMember
|
||||
@@ -186,6 +187,30 @@ class SaasSettingsStore(SettingsStore):
|
||||
if hasattr(model, key):
|
||||
setattr(model, key, value)
|
||||
|
||||
# Propagate LLM settings to all org members
|
||||
# This ensures all members see the same LLM configuration when an admin saves
|
||||
# Note: Concurrent saves by multiple admins will result in last-write-wins.
|
||||
# Consider adding optimistic locking if this becomes a problem.
|
||||
member_update_values: dict = {}
|
||||
if item.llm_model is not None:
|
||||
member_update_values['llm_model'] = item.llm_model
|
||||
if item.llm_base_url is not None:
|
||||
member_update_values['llm_base_url'] = item.llm_base_url
|
||||
if item.max_iterations is not None:
|
||||
member_update_values['max_iterations'] = item.max_iterations
|
||||
if item.llm_api_key is not None:
|
||||
member_update_values['_llm_api_key'] = encrypt_value(
|
||||
item.llm_api_key.get_secret_value()
|
||||
)
|
||||
|
||||
if member_update_values:
|
||||
stmt = (
|
||||
update(OrgMember)
|
||||
.where(OrgMember.org_id == org_id)
|
||||
.values(**member_update_values)
|
||||
)
|
||||
await session.execute(stmt)
|
||||
|
||||
await session.commit()
|
||||
|
||||
@classmethod
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import uuid
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
@@ -233,3 +234,165 @@ async def test_ensure_api_key_generates_new_key_when_verification_fails(
|
||||
|
||||
assert item.llm_api_key is not None
|
||||
assert item.llm_api_key.get_secret_value() == new_key
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def org_with_multiple_members_fixture(session_maker):
|
||||
"""Set up an organization with multiple members for testing LLM settings propagation.
|
||||
|
||||
Uses sync session to avoid UUID conversion issues with async SQLite.
|
||||
"""
|
||||
from storage.encrypt_utils import decrypt_value
|
||||
from storage.org import Org
|
||||
from storage.org_member import OrgMember
|
||||
from storage.role import Role
|
||||
from storage.user import User
|
||||
|
||||
# Use realistic UUIDs that work well with SQLite
|
||||
org_id = uuid.UUID('5594c7b6-f959-4b81-92e9-b09c206f5081')
|
||||
admin_user_id = uuid.UUID('5594c7b6-f959-4b81-92e9-b09c206f5082')
|
||||
member1_user_id = uuid.UUID('5594c7b6-f959-4b81-92e9-b09c206f5083')
|
||||
member2_user_id = uuid.UUID('5594c7b6-f959-4b81-92e9-b09c206f5084')
|
||||
|
||||
with session_maker() as session:
|
||||
# Create role
|
||||
role = Role(id=10, name='member', rank=3)
|
||||
session.add(role)
|
||||
|
||||
# Create org
|
||||
org = Org(
|
||||
id=org_id,
|
||||
name='test-org',
|
||||
org_version=1,
|
||||
enable_default_condenser=True,
|
||||
enable_proactive_conversation_starters=True,
|
||||
)
|
||||
session.add(org)
|
||||
|
||||
# Create users
|
||||
admin_user = User(
|
||||
id=admin_user_id, current_org_id=org_id, user_consents_to_analytics=True
|
||||
)
|
||||
session.add(admin_user)
|
||||
|
||||
member1_user = User(
|
||||
id=member1_user_id, current_org_id=org_id, user_consents_to_analytics=True
|
||||
)
|
||||
session.add(member1_user)
|
||||
|
||||
member2_user = User(
|
||||
id=member2_user_id, current_org_id=org_id, user_consents_to_analytics=True
|
||||
)
|
||||
session.add(member2_user)
|
||||
|
||||
# Create org members with DIFFERENT initial LLM settings
|
||||
admin_member = OrgMember(
|
||||
org_id=org_id,
|
||||
user_id=admin_user_id,
|
||||
role_id=10,
|
||||
llm_api_key='admin-initial-key',
|
||||
llm_model='old-model-v1',
|
||||
llm_base_url='http://old-url-1.com',
|
||||
max_iterations=10,
|
||||
status='active',
|
||||
)
|
||||
session.add(admin_member)
|
||||
|
||||
member1 = OrgMember(
|
||||
org_id=org_id,
|
||||
user_id=member1_user_id,
|
||||
role_id=10,
|
||||
llm_api_key='member1-initial-key',
|
||||
llm_model='old-model-v2',
|
||||
llm_base_url='http://old-url-2.com',
|
||||
max_iterations=20,
|
||||
status='active',
|
||||
)
|
||||
session.add(member1)
|
||||
|
||||
member2 = OrgMember(
|
||||
org_id=org_id,
|
||||
user_id=member2_user_id,
|
||||
role_id=10,
|
||||
llm_api_key='member2-initial-key',
|
||||
llm_model='old-model-v3',
|
||||
llm_base_url='http://old-url-3.com',
|
||||
max_iterations=30,
|
||||
status='active',
|
||||
)
|
||||
session.add(member2)
|
||||
|
||||
session.commit()
|
||||
|
||||
return {
|
||||
'org_id': org_id,
|
||||
'admin_user_id': admin_user_id,
|
||||
'member1_user_id': member1_user_id,
|
||||
'member2_user_id': member2_user_id,
|
||||
'decrypt_value': decrypt_value,
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_store_propagates_llm_settings_to_all_org_members(
|
||||
session_maker, async_session_maker, mock_config, org_with_multiple_members_fixture
|
||||
):
|
||||
"""When admin saves LLM settings, all org members should receive the updated settings.
|
||||
|
||||
This test verifies using a real database that:
|
||||
1. The bulk UPDATE targets the correct organization (WHERE clause is correct)
|
||||
2. All LLM fields are correctly set (llm_model, llm_base_url, max_iterations, llm_api_key)
|
||||
3. The llm_api_key is properly encrypted
|
||||
4. All members in the org receive the same updated values
|
||||
"""
|
||||
from sqlalchemy import select
|
||||
from storage.org_member import OrgMember
|
||||
|
||||
# Arrange
|
||||
fixture = org_with_multiple_members_fixture
|
||||
org_id = fixture['org_id']
|
||||
admin_user_id = str(fixture['admin_user_id'])
|
||||
decrypt_value = fixture['decrypt_value']
|
||||
|
||||
store = SaasSettingsStore(admin_user_id, mock_config)
|
||||
|
||||
new_settings = DataSettings(
|
||||
llm_model='new-shared-model/gpt-4',
|
||||
llm_base_url='http://new-shared-url.com',
|
||||
max_iterations=100,
|
||||
llm_api_key=SecretStr('new-shared-api-key'),
|
||||
)
|
||||
|
||||
# Act - call store() with async session
|
||||
with patch('storage.saas_settings_store.a_session_maker', async_session_maker):
|
||||
await store.store(new_settings)
|
||||
|
||||
# Assert - verify ALL org members have the updated LLM settings using sync session
|
||||
with session_maker() as session:
|
||||
result = session.execute(select(OrgMember).where(OrgMember.org_id == org_id))
|
||||
members = result.scalars().all()
|
||||
|
||||
# Verify we have all 3 members
|
||||
assert len(members) == 3, f'Expected 3 org members, got {len(members)}'
|
||||
|
||||
for member in members:
|
||||
# Verify LLM model is updated
|
||||
assert (
|
||||
member.llm_model == 'new-shared-model/gpt-4'
|
||||
), f'Expected llm_model to be updated for member {member.user_id}'
|
||||
|
||||
# Verify LLM base URL is updated
|
||||
assert (
|
||||
member.llm_base_url == 'http://new-shared-url.com'
|
||||
), f'Expected llm_base_url to be updated for member {member.user_id}'
|
||||
|
||||
# Verify max_iterations is updated
|
||||
assert (
|
||||
member.max_iterations == 100
|
||||
), f'Expected max_iterations to be 100 for member {member.user_id}'
|
||||
|
||||
# Verify the API key is encrypted and decrypts to the correct value
|
||||
decrypted_key = decrypt_value(member._llm_api_key)
|
||||
assert (
|
||||
decrypted_key == 'new-shared-api-key'
|
||||
), f'Expected llm_api_key to decrypt to new-shared-api-key for member {member.user_id}'
|
||||
|
||||
Reference in New Issue
Block a user