feat: unify NocoBase API key authentication
This commit is contained in:
@@ -13,8 +13,10 @@ class FakeNbClient:
|
||||
self.get_calls = []
|
||||
self.query_calls = []
|
||||
self.field_calls = []
|
||||
self.health_calls = []
|
||||
|
||||
def health(self):
|
||||
def health(self, resource, id_field):
|
||||
self.health_calls.append((resource, id_field))
|
||||
return {"ok": True, "status": "reachable"}
|
||||
|
||||
def list_records(self, resource, **kwargs):
|
||||
@@ -157,6 +159,14 @@ class FakeNbClient:
|
||||
|
||||
|
||||
class CrmServiceTests(unittest.TestCase):
|
||||
def test_crm_health_probes_the_configured_customer_resource(self) -> None:
|
||||
client = FakeNbClient()
|
||||
|
||||
result = CrmService(load_config(None), client).crm_health()
|
||||
|
||||
self.assertTrue(result["ok"])
|
||||
self.assertEqual(client.health_calls, [("customers", "id")])
|
||||
|
||||
def test_list_customers_applies_configured_fields_and_filters(self) -> None:
|
||||
client = FakeNbClient()
|
||||
service = CrmService(load_config(None), client)
|
||||
|
||||
@@ -117,7 +117,7 @@ class McpServiceConfigTests(unittest.TestCase):
|
||||
with self.assertRaisesRegex(RuntimeError, "Could not find nb"):
|
||||
manage_mcp_service.find_nb_path()
|
||||
|
||||
def test_generated_env_has_size_but_no_upload_jwt(self) -> None:
|
||||
def test_generated_env_has_size_but_no_nocobase_api_credentials(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
env_path = Path(tmp) / "mcp.env"
|
||||
|
||||
@@ -125,6 +125,8 @@ class McpServiceConfigTests(unittest.TestCase):
|
||||
|
||||
content = env_path.read_text(encoding="utf-8")
|
||||
self.assertIn("NOCOBASE_MCP_UPLOAD_MAX_BYTES=26214400\n", content)
|
||||
self.assertNotIn("NOCOBASE_MCP_API_TOKEN", content)
|
||||
self.assertNotIn("NOCOBASE_MCP_API_BASE_URL", content)
|
||||
self.assertNotIn("NOCOBASE_MCP_UPLOAD_JWT", content)
|
||||
self.assertNotIn("NOCOBASE_MCP_UPLOAD_API_BASE_URL", content)
|
||||
|
||||
@@ -133,8 +135,8 @@ class McpServiceConfigTests(unittest.TestCase):
|
||||
env_path = Path(tmp) / "mcp.env"
|
||||
env_path.write_text(
|
||||
"NOCOBASE_MCP_TOKEN=old-token\n"
|
||||
"NOCOBASE_MCP_UPLOAD_API_BASE_URL=https://nocobase.example/api\n"
|
||||
"NOCOBASE_MCP_UPLOAD_JWT=operator-jwt\n",
|
||||
"NOCOBASE_MCP_API_BASE_URL=https://nocobase.example/api\n"
|
||||
"NOCOBASE_MCP_API_TOKEN=operator-api-key\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
@@ -144,8 +146,8 @@ class McpServiceConfigTests(unittest.TestCase):
|
||||
self.assertIn("NOCOBASE_MCP_TOKEN=new-token\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_NB_BIN=/opt/node/bin/nb\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_UPLOAD_MAX_BYTES=26214400\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_UPLOAD_API_BASE_URL=https://nocobase.example/api\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_UPLOAD_JWT=operator-jwt\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_API_BASE_URL=https://nocobase.example/api\n", content)
|
||||
self.assertIn("NOCOBASE_MCP_API_TOKEN=operator-api-key\n", content)
|
||||
|
||||
def test_render_remote_config_prints_url_and_authorization_header(self) -> None:
|
||||
output = manage_mcp_service.render_remote_config(
|
||||
@@ -401,6 +403,31 @@ class McpServiceConfigTests(unittest.TestCase):
|
||||
self.assertIn("http://10.0.0.2:10087/mcp", output)
|
||||
self.assertIn("HTTP probe: reachable", output)
|
||||
|
||||
def test_service_status_reports_canonical_credentials_without_exposing_them(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as tmp:
|
||||
project_root = Path(tmp) / "project"
|
||||
project_root.mkdir()
|
||||
env_path = project_root / ".runtime" / "nocobase-crm-mcp.env"
|
||||
manage_mcp_service.write_token_env_file(env_path, "mcp-token")
|
||||
with env_path.open("a", encoding="utf-8") as env_file:
|
||||
env_file.write("NOCOBASE_MCP_API_BASE_URL=https://nb.example/api\n")
|
||||
env_file.write("NOCOBASE_MCP_API_TOKEN=never-print-this\n")
|
||||
|
||||
with (
|
||||
patch.object(manage_mcp_service, "detect_remote_hosts", return_value=[]),
|
||||
patch.object(manage_mcp_service, "service_state", return_value="inactive"),
|
||||
patch.object(
|
||||
manage_mcp_service,
|
||||
"probe_http_service",
|
||||
return_value={"reachable": False, "error": "stopped"},
|
||||
),
|
||||
):
|
||||
output = manage_mcp_service.service_status(project_root=project_root)
|
||||
|
||||
self.assertIn("NocoBase API credentials: configured (canonical)", output)
|
||||
self.assertNotIn("never-print-this", output)
|
||||
self.assertNotIn("https://nb.example/api", output)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
+60
-1
@@ -4,7 +4,7 @@ import subprocess
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
from nocobase_crm_mcp.nb_client import NbClient, NbCommandError
|
||||
from nocobase_crm_mcp.nb_client import NbAuthenticationError, NbClient, NbCommandError
|
||||
|
||||
|
||||
class NbClientTests(unittest.TestCase):
|
||||
@@ -59,6 +59,42 @@ class NbClientTests(unittest.TestCase):
|
||||
self.assertTrue(calls[0]["text"])
|
||||
self.assertFalse(calls[0]["check"])
|
||||
|
||||
def test_api_base_url_and_token_override_saved_env_auth_for_every_api_command(self) -> None:
|
||||
commands: list[list[str]] = []
|
||||
|
||||
def runner(cmd, **kwargs):
|
||||
commands.append(cmd)
|
||||
return subprocess.CompletedProcess(cmd, 0, stdout='{"data":[]}', stderr="")
|
||||
|
||||
client = NbClient(
|
||||
env_name="nb-mcp",
|
||||
api_base_url="https://nb.example/api/",
|
||||
api_token="long-lived-key",
|
||||
runner=runner,
|
||||
)
|
||||
client.list_records("customers")
|
||||
client.list_collection_fields("customers")
|
||||
|
||||
for command in commands:
|
||||
self.assertEqual(command[command.index("--api-base-url") + 1], "https://nb.example/api")
|
||||
self.assertEqual(command[command.index("--token") + 1], "long-lived-key")
|
||||
|
||||
def test_health_uses_token_aware_resource_probe(self) -> None:
|
||||
commands: list[list[str]] = []
|
||||
|
||||
def runner(cmd, **kwargs):
|
||||
commands.append(cmd)
|
||||
return subprocess.CompletedProcess(cmd, 0, stdout='{"data":[],"meta":{"count":0}}', stderr="")
|
||||
|
||||
health = NbClient(api_token="long-lived-key", runner=runner).health("customers", "id")
|
||||
|
||||
self.assertTrue(health["ok"])
|
||||
self.assertEqual(commands[0][:4], ["nb", "api", "resource", "list"])
|
||||
self.assertEqual(commands[0][commands[0].index("--resource") + 1], "customers")
|
||||
self.assertEqual(commands[0][commands[0].index("--fields") + 1], "id")
|
||||
self.assertEqual(commands[0][commands[0].index("--page-size") + 1], "1")
|
||||
self.assertEqual(commands[0][commands[0].index("--token") + 1], "long-lived-key")
|
||||
|
||||
def test_get_record_and_query_records_use_expected_commands(self) -> None:
|
||||
commands: list[list[str]] = []
|
||||
|
||||
@@ -229,6 +265,29 @@ class NbClientTests(unittest.TestCase):
|
||||
self.assertNotIn("super-secret-token", message)
|
||||
self.assertIn("<redacted>", message)
|
||||
|
||||
def test_authentication_failures_have_a_specific_error_type(self) -> None:
|
||||
cases = (
|
||||
"Request failed with status 401",
|
||||
'{"errors":[{"code":"EXPIRED_TOKEN"}]}',
|
||||
)
|
||||
|
||||
for stderr in cases:
|
||||
with self.subTest(stderr=stderr):
|
||||
def runner(cmd, **kwargs):
|
||||
return subprocess.CompletedProcess(cmd, 1, stdout="", stderr=stderr)
|
||||
|
||||
with self.assertRaises(NbAuthenticationError):
|
||||
NbClient(api_token="secret", runner=runner).list_records("customers")
|
||||
|
||||
def test_plain_forbidden_error_remains_an_acl_command_failure(self) -> None:
|
||||
def runner(cmd, **kwargs):
|
||||
return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="Request failed with status 403: No permissions")
|
||||
|
||||
with self.assertRaises(NbCommandError) as caught:
|
||||
NbClient(api_token="secret", runner=runner).list_records("customers")
|
||||
|
||||
self.assertNotIsInstance(caught.exception, NbAuthenticationError)
|
||||
|
||||
def test_invalid_json_output_raises_command_error(self) -> None:
|
||||
def runner(cmd, **kwargs):
|
||||
return subprocess.CompletedProcess(cmd, 0, stdout="HTTP 200", stderr="")
|
||||
|
||||
@@ -124,6 +124,26 @@ class NocoBaseHttpClientTests(unittest.TestCase):
|
||||
self.assertNotIn("super-secret-token", str(caught.exception))
|
||||
self.assertIn("<redacted>", str(caught.exception))
|
||||
|
||||
def test_http_error_distinguishes_invalid_key_from_acl_forbidden(self) -> None:
|
||||
responses = (
|
||||
(httpx.Response(401, json={"errors": [{"code": "INVALID_TOKEN"}]}), True),
|
||||
(httpx.Response(403, json={"errors": [{"code": "EXPIRED_TOKEN"}]}), True),
|
||||
(httpx.Response(403, json={"errors": [{"code": "NO_PERMISSIONS"}]}), False),
|
||||
)
|
||||
|
||||
for response, authentication_failed in responses:
|
||||
with self.subTest(status=response.status_code, authentication_failed=authentication_failed):
|
||||
client = NocoBaseHttpClient(
|
||||
"https://nb.example/api",
|
||||
"api-key",
|
||||
transport=httpx.MockTransport(lambda request, response=response: response),
|
||||
)
|
||||
|
||||
with self.assertRaises(NocoBaseHttpError) as caught:
|
||||
client.add_attachment_ids(7, [8])
|
||||
|
||||
self.assertEqual(caught.exception.authentication_failed, authentication_failed)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
@@ -187,16 +187,17 @@ class ProjectServiceTests(unittest.TestCase):
|
||||
|
||||
def test_association_errors_keep_project_id_and_map_to_stable_codes(self) -> None:
|
||||
cases = [
|
||||
(404, "attachment_not_found"),
|
||||
(401, "nb_upload_unauthorized"),
|
||||
(403, "nb_upload_unauthorized"),
|
||||
(500, "nb_upload_failed"),
|
||||
(NocoBaseHttpError(404, "attachment request failed"), "attachment_not_found"),
|
||||
(NocoBaseHttpError(401, "attachment request failed"), "nb_upload_unauthorized"),
|
||||
(NocoBaseHttpError(403, "permission denied"), "nb_upload_failed"),
|
||||
(NocoBaseHttpError(403, "expired", authentication_failed=True), "nb_upload_unauthorized"),
|
||||
(NocoBaseHttpError(500, "attachment request failed"), "nb_upload_failed"),
|
||||
]
|
||||
|
||||
for status_code, error_code in cases:
|
||||
with self.subTest(status_code=status_code):
|
||||
for upstream_error, error_code in cases:
|
||||
with self.subTest(status_code=upstream_error.status_code, error_code=error_code):
|
||||
self.setUp()
|
||||
self.http.add_error = NocoBaseHttpError(status_code, "attachment request failed")
|
||||
self.http.add_error = upstream_error
|
||||
|
||||
result = self.service.create_project(name="x", attachment_ids=[8])
|
||||
|
||||
|
||||
+66
-3
@@ -6,7 +6,7 @@ import httpx
|
||||
from starlette.responses import JSONResponse
|
||||
|
||||
from nocobase_crm_mcp import server as server_module
|
||||
from nocobase_crm_mcp.nb_client import NbCommandError
|
||||
from nocobase_crm_mcp.nb_client import NbAuthenticationError, NbCommandError
|
||||
from nocobase_crm_mcp.server import (
|
||||
DEFAULT_HTTP_HOST,
|
||||
DEFAULT_HTTP_PATH,
|
||||
@@ -15,6 +15,7 @@ from nocobase_crm_mcp.server import (
|
||||
FastMCP,
|
||||
create_http_app,
|
||||
create_server,
|
||||
_read_nocobase_environment,
|
||||
run_http_server,
|
||||
)
|
||||
|
||||
@@ -78,6 +79,11 @@ class ErrorCrmService(FakeCrmService):
|
||||
raise NbCommandError("nb command failed with exit code 1: denied")
|
||||
|
||||
|
||||
class AuthenticationErrorCrmService(FakeCrmService):
|
||||
def list_customers(self, **kwargs):
|
||||
raise NbAuthenticationError("Request failed with status 401")
|
||||
|
||||
|
||||
class MissingNbCrmService(FakeCrmService):
|
||||
def crm_health(self):
|
||||
raise FileNotFoundError(2, "No such file or directory", "nb")
|
||||
@@ -179,6 +185,18 @@ class ServerTests(unittest.TestCase):
|
||||
self.assertIn("denied", structured["error"]["message"])
|
||||
self.assertFalse(structured["error"]["retryable"])
|
||||
|
||||
def test_tools_return_specific_error_for_invalid_nocobase_api_key(self) -> None:
|
||||
if FastMCP is None:
|
||||
self.skipTest("MCP SDK is not installed")
|
||||
|
||||
server = create_server(crm_service=AuthenticationErrorCrmService(), readonly=True)
|
||||
|
||||
_content, structured = asyncio.run(server.call_tool("list_customers", {}))
|
||||
|
||||
self.assertEqual(structured["error"]["code"], "nb_auth_failed")
|
||||
self.assertIn("NOCOBASE_MCP_API_TOKEN", structured["error"]["hint"])
|
||||
self.assertFalse(structured["error"]["retryable"])
|
||||
|
||||
def test_tools_return_specific_error_when_nb_is_not_found(self) -> None:
|
||||
if FastMCP is None:
|
||||
self.skipTest("MCP SDK is not installed")
|
||||
@@ -230,10 +248,55 @@ class ServerTests(unittest.TestCase):
|
||||
self.assertEqual(response.status_code, 200)
|
||||
self.assertEqual(response.json(), {"uploaded": True})
|
||||
|
||||
def test_http_write_mode_requires_upload_api_base_url(self) -> None:
|
||||
def test_nocobase_environment_prefers_canonical_shared_credentials(self) -> None:
|
||||
with patch.dict(
|
||||
"os.environ",
|
||||
{
|
||||
"NOCOBASE_MCP_API_BASE_URL": "https://nb.example/api/",
|
||||
"NOCOBASE_MCP_API_TOKEN": "shared-key",
|
||||
"NOCOBASE_MCP_UPLOAD_MAX_BYTES": "123",
|
||||
},
|
||||
clear=True,
|
||||
):
|
||||
self.assertEqual(
|
||||
_read_nocobase_environment(),
|
||||
("https://nb.example/api", "shared-key", 123),
|
||||
)
|
||||
|
||||
def test_nocobase_environment_accepts_matching_legacy_credentials(self) -> None:
|
||||
with patch.dict(
|
||||
"os.environ",
|
||||
{
|
||||
"NOCOBASE_MCP_UPLOAD_API_BASE_URL": "https://legacy.example/api",
|
||||
"NOCOBASE_MCP_UPLOAD_JWT": "legacy-key",
|
||||
},
|
||||
clear=True,
|
||||
):
|
||||
self.assertEqual(
|
||||
_read_nocobase_environment(),
|
||||
("https://legacy.example/api", "legacy-key", 26214400),
|
||||
)
|
||||
|
||||
def test_nocobase_environment_rejects_incomplete_or_conflicting_credentials(self) -> None:
|
||||
cases = (
|
||||
{"NOCOBASE_MCP_API_BASE_URL": "https://nb.example/api"},
|
||||
{
|
||||
"NOCOBASE_MCP_API_BASE_URL": "https://nb.example/api",
|
||||
"NOCOBASE_MCP_API_TOKEN": "canonical",
|
||||
"NOCOBASE_MCP_UPLOAD_API_BASE_URL": "https://other.example/api",
|
||||
"NOCOBASE_MCP_UPLOAD_JWT": "legacy",
|
||||
},
|
||||
)
|
||||
|
||||
for values in cases:
|
||||
with self.subTest(values=set(values)), patch.dict("os.environ", values, clear=True):
|
||||
with self.assertRaises(RuntimeError):
|
||||
_read_nocobase_environment()
|
||||
|
||||
def test_http_write_mode_requires_shared_api_base_url(self) -> None:
|
||||
with patch.dict("os.environ", {"NOCOBASE_MCP_TOKEN": "secret"}, clear=True):
|
||||
with patch("uvicorn.run") as uvicorn_run:
|
||||
with self.assertRaisesRegex(RuntimeError, "NOCOBASE_MCP_UPLOAD_API_BASE_URL"):
|
||||
with self.assertRaisesRegex(RuntimeError, "NOCOBASE_MCP_API_BASE_URL"):
|
||||
run_http_server(
|
||||
config_path=None,
|
||||
host=DEFAULT_HTTP_HOST,
|
||||
|
||||
@@ -175,12 +175,17 @@ class UploadEndpointTests(unittest.IsolatedAsyncioTestCase):
|
||||
self.assertEqual(list(Path(temp_dir).iterdir()), [])
|
||||
|
||||
async def test_upload_maps_upstream_errors_and_removes_temp_file(self) -> None:
|
||||
cases = ((401, "nb_upload_unauthorized"), (403, "nb_upload_unauthorized"), (500, "nb_upload_failed"))
|
||||
cases = (
|
||||
(NocoBaseHttpError(401, "upstream failure"), "nb_upload_unauthorized"),
|
||||
(NocoBaseHttpError(403, "permission denied"), "nb_upload_failed"),
|
||||
(NocoBaseHttpError(403, "expired", authentication_failed=True), "nb_upload_unauthorized"),
|
||||
(NocoBaseHttpError(500, "upstream failure"), "nb_upload_failed"),
|
||||
)
|
||||
|
||||
for status_code, error_code in cases:
|
||||
with self.subTest(status_code=status_code), TemporaryDirectory() as temp_dir:
|
||||
for upstream_error, error_code in cases:
|
||||
with self.subTest(status_code=upstream_error.status_code, error_code=error_code), TemporaryDirectory() as temp_dir:
|
||||
http_client = FakeHttpClient()
|
||||
http_client.error = NocoBaseHttpError(status_code, "upstream failure")
|
||||
http_client.error = upstream_error
|
||||
async with await self._client_for(http_client, temp_dir) as client:
|
||||
response = await client.post(
|
||||
"/uploads",
|
||||
|
||||
Reference in New Issue
Block a user