Skip to content

Commit 6d6961b

Browse files
committed
šŸ› Bugfix: The agent_run process cannot invoke the MCP service with authentication.
[Specification Details] 1. When querying the mcp list, add the parameter is_need_auth to determine whether an auth_token should be returned. 2. Modify test cases.
1 parent d024053 commit 6d6961b

6 files changed

Lines changed: 208 additions & 9 deletions

File tree

ā€Žbackend/agents/create_agent_info.pyā€Ž

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ async def create_agent_run_info(
388388
version_no=version_no,
389389
)
390390

391-
remote_mcp_list = await get_remote_mcp_server_list(tenant_id=tenant_id)
391+
remote_mcp_list = await get_remote_mcp_server_list(tenant_id=tenant_id, is_need_auth=True)
392392
default_mcp_url = urljoin(LOCAL_MCP_SERVER, "sse")
393393
remote_mcp_list.append({
394394
"remote_mcp_server_name": "nexent",

ā€Žbackend/apps/remote_mcp_app.pyā€Ž

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ async def get_remote_proxies(
181181
remote_mcp_server_list = await get_remote_mcp_server_list(
182182
tenant_id=effective_tenant_id,
183183
user_id=user_id,
184+
is_need_auth=False
184185
)
185186
return JSONResponse(
186187
status_code=HTTPStatus.OK,

ā€Žbackend/services/remote_mcp_service.pyā€Ž

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ async def update_remote_mcp_server_list(
156156
)
157157

158158

159-
async def get_remote_mcp_server_list(tenant_id: str, user_id: str | None = None) -> list[dict]:
159+
async def get_remote_mcp_server_list(tenant_id: str, user_id: str | None = None, is_need_auth: bool = True) -> list[dict]:
160160
mcp_records = get_mcp_records_by_tenant(tenant_id=tenant_id)
161161
mcp_records_list = []
162162
can_edit_all = False
@@ -173,13 +173,16 @@ async def get_remote_mcp_server_list(tenant_id: str, user_id: str | None = None)
173173
permission = PERMISSION_EDIT if can_edit_all or str(
174174
created_by) == str(user_id) else PERMISSION_READ
175175

176-
mcp_records_list.append({
176+
record_dict = {
177177
"remote_mcp_server_name": record["mcp_name"],
178178
"remote_mcp_server": record["mcp_server"],
179179
"status": record["status"],
180180
"permission": permission,
181-
"mcp_id": record.get("mcp_id")
182-
})
181+
"mcp_id": record.get("mcp_id"),
182+
}
183+
if is_need_auth:
184+
record_dict["authorization_token"] = record.get("authorization_token")
185+
mcp_records_list.append(record_dict)
183186
return mcp_records_list
184187

185188

ā€Žtest/backend/agents/test_create_agent_info.pyā€Ž

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1689,7 +1689,7 @@ async def test_create_agent_run_info_success(self):
16891689
allow_memory_search=True,
16901690
version_no=1,
16911691
)
1692-
mock_get_mcp.assert_called_once_with(tenant_id="tenant_1")
1692+
mock_get_mcp.assert_called_once_with(tenant_id="tenant_1", is_need_auth=True)
16931693
mock_filter.assert_called_once_with("agent_config", {
16941694
"test_server": {
16951695
"remote_mcp_server_name": "test_server",
@@ -2117,6 +2117,55 @@ async def test_create_agent_run_info_no_published_version_fallback(self):
21172117
allow_memory_search=True,
21182118
version_no=0, # Fallback to draft version 0
21192119
)
2120+
# Verify that get_remote_mcp_server_list was called with is_need_auth=True
2121+
mock_get_mcp.assert_called_once_with(tenant_id="tenant_1", is_need_auth=True)
2122+
2123+
@pytest.mark.asyncio
2124+
async def test_create_agent_run_info_is_need_auth_true_includes_token(self):
2125+
"""Test that get_remote_mcp_server_list is called with is_need_auth=True and returns authorization_token"""
2126+
mock_agent_run_info.reset_mock()
2127+
with patch('backend.agents.create_agent_info.join_minio_file_description_to_query') as mock_join_query, \
2128+
patch('backend.agents.create_agent_info.create_model_config_list') as mock_create_models, \
2129+
patch('backend.agents.create_agent_info.get_remote_mcp_server_list', new_callable=AsyncMock) as mock_get_mcp, \
2130+
patch('backend.agents.create_agent_info.create_agent_config') as mock_create_agent, \
2131+
patch('backend.agents.create_agent_info.filter_mcp_servers_and_tools') as mock_filter, \
2132+
patch('backend.agents.create_agent_info.urljoin') as mock_urljoin, \
2133+
patch('backend.agents.create_agent_info.threading') as mock_threading, \
2134+
patch('backend.agents.create_agent_info.query_current_version_no') as mock_version_no:
2135+
2136+
mock_join_query.return_value = "processed_query"
2137+
mock_create_models.return_value = ["model_config"]
2138+
# Mock return value with authorization_token (when is_need_auth=True)
2139+
mock_get_mcp.return_value = [
2140+
{
2141+
"remote_mcp_server_name": "test_server",
2142+
"remote_mcp_server": "http://test.server",
2143+
"status": True,
2144+
"authorization_token": "secret_token_123",
2145+
"mcp_id": 1
2146+
}
2147+
]
2148+
mock_create_agent.return_value = "agent_config"
2149+
mock_urljoin.return_value = "http://nexent.mcp/sse"
2150+
mock_filter.return_value = ["http://test.server"]
2151+
mock_threading.Event.return_value = "stop_event"
2152+
mock_version_no.return_value = 1
2153+
2154+
await create_agent_run_info(
2155+
agent_id="agent_1",
2156+
minio_files=[],
2157+
query="test query",
2158+
history=[],
2159+
user_id="user_1",
2160+
tenant_id="tenant_1",
2161+
language="zh"
2162+
)
2163+
2164+
# Verify that get_remote_mcp_server_list was called with is_need_auth=True
2165+
mock_get_mcp.assert_called_once_with(tenant_id="tenant_1", is_need_auth=True)
2166+
2167+
# Verify that the returned data includes authorization_token (used in mcp_host construction)
2168+
assert mock_get_mcp.return_value[0]["authorization_token"] == "secret_token_123"
21202169

21212170

21222171
class TestJoinMinioFileDescriptionToQuery:

ā€Žtest/backend/app/test_remote_mcp_app.pyā€Ž

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ def test_get_remote_proxies_success(self, mock_get_list, mock_get_user_info):
412412
assert data["remote_mcp_server_list"][1]["permission"] == "READ_ONLY"
413413

414414
mock_get_user_info.assert_called_once()
415-
mock_get_list.assert_called_once_with(tenant_id="tenant456", user_id="user123")
415+
mock_get_list.assert_called_once_with(tenant_id="tenant456", user_id="user123", is_need_auth=False)
416416

417417
@patch('apps.remote_mcp_app.get_current_user_info')
418418
@patch('apps.remote_mcp_app.get_remote_mcp_server_list')
@@ -428,8 +428,8 @@ def test_get_remote_proxies_with_tenant_id_param(self, mock_get_list, mock_get_u
428428
)
429429

430430
assert response.status_code == HTTPStatus.OK
431-
# Verify that explicit tenant_id is used
432-
mock_get_list.assert_called_once_with(tenant_id="explicit_tenant789", user_id="user123")
431+
# Verify that explicit tenant_id is used and is_need_auth=False
432+
mock_get_list.assert_called_once_with(tenant_id="explicit_tenant789", user_id="user123", is_need_auth=False)
433433

434434
@patch('apps.remote_mcp_app.get_current_user_info')
435435
@patch('apps.remote_mcp_app.get_remote_mcp_server_list')
@@ -447,6 +447,51 @@ def test_get_remote_proxies_error(self, mock_get_list, mock_get_user_info):
447447
data = response.json()
448448
assert "Failed to get remote MCP proxy" in data["detail"]
449449

450+
@patch('apps.remote_mcp_app.get_current_user_info')
451+
@patch('apps.remote_mcp_app.get_remote_mcp_server_list')
452+
def test_get_remote_proxies_is_need_auth_false_excludes_token(self, mock_get_list, mock_get_user_info):
453+
"""Test that get_remote_mcp_server_list is called with is_need_auth=False and excludes authorization_token"""
454+
mock_get_user_info.return_value = ("user123", "tenant456", "en")
455+
# Mock return value without authorization_token (when is_need_auth=False)
456+
mock_server_list = [
457+
{
458+
"remote_mcp_server_name": "server1",
459+
"remote_mcp_server": "http://server1.com",
460+
"status": True,
461+
"permission": "EDIT",
462+
"mcp_id": 1
463+
},
464+
{
465+
"remote_mcp_server_name": "server2",
466+
"remote_mcp_server": "http://server2.com",
467+
"status": False,
468+
"permission": "READ_ONLY",
469+
"mcp_id": 2
470+
}
471+
]
472+
mock_get_list.return_value = mock_server_list
473+
474+
response = client.get(
475+
"/mcp/list",
476+
headers={"Authorization": "Bearer test_token"}
477+
)
478+
479+
assert response.status_code == HTTPStatus.OK
480+
data = response.json()
481+
assert "remote_mcp_server_list" in data
482+
assert len(data["remote_mcp_server_list"]) == 2
483+
484+
# Verify that authorization_token is not present in the response
485+
assert "authorization_token" not in data["remote_mcp_server_list"][0]
486+
assert "authorization_token" not in data["remote_mcp_server_list"][1]
487+
488+
# Verify that other fields are present
489+
assert data["remote_mcp_server_list"][0]["mcp_id"] == 1
490+
assert data["remote_mcp_server_list"][1]["mcp_id"] == 2
491+
492+
# Verify that get_remote_mcp_server_list was called with is_need_auth=False
493+
mock_get_list.assert_called_once_with(tenant_id="tenant456", user_id="user123", is_need_auth=False)
494+
450495

451496
class TestGetMCPRecord:
452497
"""Test endpoint for getting single MCP record by ID"""

ā€Žtest/backend/services/test_remote_mcp_service.pyā€Ž

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,107 @@ async def test_get_list_permission_admin_can_edit_all(self, mock_get, mock_get_u
483483
self.assertEqual(result[0]["permission"], "EDIT")
484484
self.assertEqual(result[1]["permission"], "EDIT")
485485

486+
@patch('backend.services.remote_mcp_service.get_mcp_records_by_tenant')
487+
async def test_get_list_with_is_need_auth_true(self, mock_get):
488+
"""Test getting server list with is_need_auth=True (default) includes authorization_token"""
489+
mock_get.return_value = [
490+
{
491+
"mcp_name": "n1",
492+
"mcp_server": "u1",
493+
"status": True,
494+
"authorization_token": "token123",
495+
"mcp_id": 1
496+
},
497+
{
498+
"mcp_name": "n2",
499+
"mcp_server": "u2",
500+
"status": False,
501+
"authorization_token": None,
502+
"mcp_id": 2
503+
}
504+
]
505+
506+
result = await get_remote_mcp_server_list('tid', is_need_auth=True)
507+
508+
self.assertEqual(len(result), 2)
509+
self.assertIn("authorization_token", result[0])
510+
self.assertEqual(result[0]["authorization_token"], "token123")
511+
self.assertIn("authorization_token", result[1])
512+
self.assertIsNone(result[1]["authorization_token"])
513+
514+
@patch('backend.services.remote_mcp_service.get_mcp_records_by_tenant')
515+
async def test_get_list_with_is_need_auth_false(self, mock_get):
516+
"""Test getting server list with is_need_auth=False excludes authorization_token"""
517+
mock_get.return_value = [
518+
{
519+
"mcp_name": "n1",
520+
"mcp_server": "u1",
521+
"status": True,
522+
"authorization_token": "token123",
523+
"mcp_id": 1
524+
},
525+
{
526+
"mcp_name": "n2",
527+
"mcp_server": "u2",
528+
"status": False,
529+
"authorization_token": "token456",
530+
"mcp_id": 2
531+
}
532+
]
533+
534+
result = await get_remote_mcp_server_list('tid', is_need_auth=False)
535+
536+
self.assertEqual(len(result), 2)
537+
self.assertNotIn("authorization_token", result[0])
538+
self.assertNotIn("authorization_token", result[1])
539+
# Verify other fields are still present
540+
self.assertEqual(result[0]["remote_mcp_server_name"], "n1")
541+
self.assertEqual(result[0]["mcp_id"], 1)
542+
self.assertEqual(result[1]["remote_mcp_server_name"], "n2")
543+
self.assertEqual(result[1]["mcp_id"], 2)
544+
545+
@patch('backend.services.remote_mcp_service.get_mcp_records_by_tenant')
546+
async def test_get_list_default_is_need_auth_true(self, mock_get):
547+
"""Test that default behavior (is_need_auth not specified) includes authorization_token"""
548+
mock_get.return_value = [
549+
{
550+
"mcp_name": "n1",
551+
"mcp_server": "u1",
552+
"status": True,
553+
"authorization_token": "token123",
554+
"mcp_id": 1
555+
}
556+
]
557+
558+
result = await get_remote_mcp_server_list('tid')
559+
560+
self.assertEqual(len(result), 1)
561+
self.assertIn("authorization_token", result[0])
562+
self.assertEqual(result[0]["authorization_token"], "token123")
563+
564+
@patch('backend.services.remote_mcp_service.get_user_tenant_by_user_id')
565+
@patch('backend.services.remote_mcp_service.get_mcp_records_by_tenant')
566+
async def test_get_list_with_user_id_and_is_need_auth_false(self, mock_get, mock_get_user_tenant):
567+
"""Test getting server list with user_id and is_need_auth=False"""
568+
mock_get_user_tenant.return_value = {"user_role": "USER"}
569+
mock_get.return_value = [
570+
{
571+
"mcp_name": "n1",
572+
"mcp_server": "u1",
573+
"status": True,
574+
"created_by": "user123",
575+
"authorization_token": "token123",
576+
"mcp_id": 1
577+
}
578+
]
579+
580+
result = await get_remote_mcp_server_list('tid', user_id="user123", is_need_auth=False)
581+
582+
self.assertEqual(len(result), 1)
583+
self.assertNotIn("authorization_token", result[0])
584+
self.assertEqual(result[0]["permission"], "EDIT")
585+
self.assertEqual(result[0]["mcp_id"], 1)
586+
486587

487588
class TestCheckMcpHealthAndUpdateDb(unittest.IsolatedAsyncioTestCase):
488589
"""Test check_mcp_health_and_update_db"""

0 commit comments

Comments
Ā (0)