diff --git a/README.md b/README.md index 0c74bd8..2b132d6 100644 --- a/README.md +++ b/README.md @@ -92,12 +92,14 @@ They target only `t_projects` and the fixed field mapping in `projects` in `conf The same HTTP service exposes authenticated `POST /uploads`. Submit exactly one multipart field named `file`; the default single-file limit is 25 MiB (`NOCOBASE_MCP_UPLOAD_MAX_BYTES=26214400`). A successful upload returns an `upload_id`, which callers can pass to either project tool. The upload route and `/mcp` use the same MCP bearer token. -HTTP write mode also requires these server-side NocoBase credentials. Do not put them in `config.json`, pass them to MCP clients, or log them: +HTTP write mode also requires one server-side NocoBase API key. The same key is supplied explicitly to every `nb api` CRM query and every direct HTTP upload/association request, so the service does not depend on the saved `nb-mcp` basic login session. Do not put these values in `config.json`, pass them to MCP clients, or log them: -- `NOCOBASE_MCP_UPLOAD_API_BASE_URL` (for example `https://nocobase.example/api`) -- `NOCOBASE_MCP_UPLOAD_JWT` (a NocoBase user JWT) +- `NOCOBASE_MCP_API_BASE_URL` (for example `https://nocobase.example/api`) +- `NOCOBASE_MCP_API_TOKEN` (a scoped NocoBase API key) -`scripts/mcp_service.sh start` creates `.runtime/nocobase-crm-mcp.env` with mode `0600`, its parent directory with mode `0700`, and only the non-secret upload size setting. HTTP write mode fails at startup with the missing variable name when either credential is absent. +Use a dedicated NocoBase account and role with only the permissions listed below. Choose an appropriate API-key validity period and rotate the key deliberately; if NocoBase rejects the key, MCP tools return `nb_auth_failed` or `nb_upload_unauthorized` instead of attempting a password login or retry loop. + +`scripts/mcp_service.sh start` creates `.runtime/nocobase-crm-mcp.env` with mode `0600`, its parent directory with mode `0700`, and only the non-secret upload size setting. HTTP write mode fails at startup with the missing variable name when either credential is absent. For migration, the legacy pair `NOCOBASE_MCP_UPLOAD_API_BASE_URL` / `NOCOBASE_MCP_UPLOAD_JWT` remains accepted when both are present; conflicting canonical and legacy values are rejected. First-install flow: @@ -105,10 +107,10 @@ First-install flow: # Creates the private env file; the service is expected to fail until credentials are added. bash scripts/mcp_service.sh start -# Add server-side NocoBase credentials to the systemd EnvironmentFile. +# Add the server-side NocoBase API key to the systemd EnvironmentFile. cat >> .runtime/nocobase-crm-mcp.env <<'EOF' -NOCOBASE_MCP_UPLOAD_API_BASE_URL=https://nocobase.example/api -NOCOBASE_MCP_UPLOAD_JWT= +NOCOBASE_MCP_API_BASE_URL=https://nocobase.example/api +NOCOBASE_MCP_API_TOKEN= EOF # Start the service with the newly configured private env file. @@ -116,7 +118,7 @@ bash scripts/mcp_service.sh start curl -X POST -H "Authorization: Bearer " -F "file=@quote.pdf" http://:10087/uploads ``` -The NocoBase credentials belong in `.runtime/nocobase-crm-mcp.env`, not in shell-local variables or client configuration. Token rotation preserves these operator-managed env lines. An uploaded attachment that is not yet associated with a project is intentionally retained; this release does not auto-clean orphan attachments. It also provides no delete operation for projects or attachments. +The NocoBase credentials belong in `.runtime/nocobase-crm-mcp.env`, not in shell-local variables or client configuration. MCP bearer-token rotation preserves these operator-managed env lines. `scripts/mcp_service.sh status` reports whether canonical or legacy NocoBase credentials are configured without printing their values. An uploaded attachment that is not yet associated with a project is intentionally retained; this release does not auto-clean orphan attachments. It also provides no delete operation for projects or attachments. ## Permissions @@ -174,7 +176,7 @@ If `nb` is installed somewhere unusual, set its absolute path before starting: NOCOBASE_MCP_NB_BIN=/absolute/path/to/nb bash scripts/mcp_service.sh start ``` -The generated env file also includes the non-secret 25 MiB upload limit. The service fails at startup until the operator adds `NOCOBASE_MCP_UPLOAD_API_BASE_URL` and `NOCOBASE_MCP_UPLOAD_JWT` to that private file. Run `start` again after adding them; `new-token` preserves those operator-managed credential lines. +The generated env file also includes the non-secret 25 MiB upload limit. The service fails at startup until the operator adds `NOCOBASE_MCP_API_BASE_URL` and `NOCOBASE_MCP_API_TOKEN` to that private file. Run `start` again after adding them; `new-token` preserves those operator-managed credential lines. Show the saved URL/header/client config: diff --git a/nocobase_crm_mcp/crm.py b/nocobase_crm_mcp/crm.py index 6203f16..fe27485 100644 --- a/nocobase_crm_mcp/crm.py +++ b/nocobase_crm_mcp/crm.py @@ -38,7 +38,7 @@ class CrmService: self.client = client def crm_health(self) -> dict[str, Any]: - health = self.client.health() + health = self.client.health(self.config.customer.resource, self.config.customer.id_field) configured = { "env": self.config.env_name, "data_source": self.config.data_source, diff --git a/nocobase_crm_mcp/nb_client.py b/nocobase_crm_mcp/nb_client.py index 0a8fc05..7ce5e4e 100644 --- a/nocobase_crm_mcp/nb_client.py +++ b/nocobase_crm_mcp/nb_client.py @@ -2,6 +2,7 @@ from __future__ import annotations import json import os +import re import subprocess from collections.abc import Set as AbstractSet from pathlib import Path @@ -15,6 +16,10 @@ class NbCommandError(RuntimeError): """Raised when the NocoBase CLI command fails or returns invalid output.""" +class NbAuthenticationError(NbCommandError): + """Raised when NocoBase rejects the configured API credential.""" + + class NbClient: def __init__( self, @@ -22,6 +27,7 @@ class NbClient: env_name: str = "nb-mcp", data_source: str = "main", nb_bin: str = "nb", + api_base_url: str | None = None, api_token: str | None = None, role: str | None = None, cwd: str | Path | None = None, @@ -32,20 +38,20 @@ class NbClient: self.data_source = data_source expanded_nb_bin = Path(nb_bin).expanduser() self.nb_bin = str(expanded_nb_bin) if str(nb_bin).startswith("~") or expanded_nb_bin.is_absolute() else nb_bin + self.api_base_url = api_base_url.strip().rstrip("/") if api_base_url else None self.api_token = api_token self.role = role self.cwd = Path(cwd).resolve() if cwd else None self.timeout = timeout self.runner = runner or subprocess.run - def health(self) -> dict[str, Any]: - result = self._run_raw(["env", "status", self.env_name]) - output = result.stdout.strip() + def health(self, resource: str, id_field: str) -> dict[str, Any]: + self.list_records(resource, fields=[id_field], page=1, page_size=1) return { - "ok": result.returncode == 0 and "unreachable" not in output.lower(), + "ok": True, "env": self.env_name, - "status_output": output, - "stderr": self._redact(result.stderr.strip()), + "status_output": "authenticated", + "stderr": "", } def list_records( @@ -193,6 +199,8 @@ class NbClient: ] if self.data_source: command.extend(["--data-source", self.data_source]) + if self.api_base_url: + command.extend(["--api-base-url", self.api_base_url]) if self.role: command.extend(["--role", self.role]) if self.api_token: @@ -210,6 +218,8 @@ class NbClient: "--yes", "--json-output", ] + if self.api_base_url: + command.extend(["--api-base-url", self.api_base_url]) if self.role: command.extend(["--role", self.role]) if self.api_token: @@ -219,7 +229,8 @@ class NbClient: def _run_json(self, command: list[str]) -> dict[str, Any]: result = self._run_raw(command[1:], full_command=command) if result.returncode != 0: - raise NbCommandError(self._format_error(command, result)) + error_type = NbAuthenticationError if self._is_authentication_failure(result) else NbCommandError + raise error_type(self._format_error(command, result)) try: parsed = json.loads(result.stdout) except json.JSONDecodeError as error: @@ -269,6 +280,24 @@ class NbClient: f"stderr={self._redact(result.stderr.strip())}" ) + @staticmethod + def _is_authentication_failure(result: subprocess.CompletedProcess[str]) -> bool: + output = f"{result.stdout}\n{result.stderr}" + if re.search(r"\bstatus\s+401\b", output, flags=re.IGNORECASE): + return True + upper_output = output.upper() + return any( + code in upper_output + for code in ( + "EMPTY_TOKEN", + "INVALID_TOKEN", + "EXPIRED_TOKEN", + "BLOCKED_TOKEN", + "EXPIRED_SESSION", + "NOT_EXIST_USER", + ) + ) + def _redact_command(self, command: Sequence[str]) -> str: redacted: list[str] = [] skip_next = False diff --git a/nocobase_crm_mcp/nocobase_http.py b/nocobase_crm_mcp/nocobase_http.py index 020a2fc..4c8ff7b 100644 --- a/nocobase_crm_mcp/nocobase_http.py +++ b/nocobase_crm_mcp/nocobase_http.py @@ -8,8 +8,15 @@ import httpx class NocoBaseHttpError(RuntimeError): - def __init__(self, status_code: int, message: str) -> None: + def __init__( + self, + status_code: int, + message: str, + *, + authentication_failed: bool | None = None, + ) -> None: self.status_code = status_code + self.authentication_failed = status_code == 401 if authentication_failed is None else authentication_failed super().__init__(message) @@ -21,19 +28,19 @@ class NocoBaseHttpClient: def __init__( self, api_base_url: str, - jwt: str, + api_token: str, timeout: float = 30.0, transport: httpx.BaseTransport | None = None, ) -> None: self.api_base_url = api_base_url.strip().rstrip("/") - self.jwt = jwt.strip() + self.api_token = api_token.strip() if not self.api_base_url: raise ValueError("NocoBase API base URL must not be blank") - if not self.jwt: - raise ValueError("NocoBase JWT must not be blank") + if not self.api_token: + raise ValueError("NocoBase API token must not be blank") self._client = httpx.Client( base_url=self.api_base_url, - headers={"Authorization": f"Bearer {self.jwt}"}, + headers={"Authorization": f"Bearer {self.api_token}"}, timeout=timeout, transport=transport, ) @@ -79,10 +86,14 @@ class NocoBaseHttpClient: if response.is_success: return message = response.text.strip() or response.reason_phrase or "NocoBase request failed" - raise NocoBaseHttpError(response.status_code, self._redact(message)) + raise NocoBaseHttpError( + response.status_code, + self._redact(message), + authentication_failed=_is_authentication_failure(response), + ) def _redact(self, value: str) -> str: - return value.replace(self.jwt, "") + return value.replace(self.api_token, "") def _project_id_segment(project_id: Any) -> str: @@ -98,3 +109,20 @@ def validate_project_id(project_id: Any) -> str: if project_id in {".", ".."}: raise ValueError("project_id must not be a dot path segment") return str(project_id) + + +def _is_authentication_failure(response: httpx.Response) -> bool: + if response.status_code == 401: + return True + response_text = response.text.upper() + return any( + code in response_text + for code in ( + "EMPTY_TOKEN", + "INVALID_TOKEN", + "EXPIRED_TOKEN", + "BLOCKED_TOKEN", + "EXPIRED_SESSION", + "NOT_EXIST_USER", + ) + ) diff --git a/nocobase_crm_mcp/projects.py b/nocobase_crm_mcp/projects.py index f71c280..4ca3fc7 100644 --- a/nocobase_crm_mcp/projects.py +++ b/nocobase_crm_mcp/projects.py @@ -234,7 +234,7 @@ def _attachment_error(project_id: Any, error: Exception) -> dict[str, Any]: if isinstance(error, NocoBaseHttpError): if error.status_code == 404: code = "attachment_not_found" - elif error.status_code in {401, 403}: + elif error.authentication_failed: code = "nb_upload_unauthorized" else: code = "nb_upload_failed" diff --git a/nocobase_crm_mcp/server.py b/nocobase_crm_mcp/server.py index 64f2b46..52f51e2 100644 --- a/nocobase_crm_mcp/server.py +++ b/nocobase_crm_mcp/server.py @@ -28,7 +28,7 @@ except ModuleNotFoundError: from .config import load_config from .crm import CrmService -from .nb_client import NbClient, NbCommandError +from .nb_client import NbAuthenticationError, NbClient, NbCommandError from .nocobase_http import NocoBaseHttpClient from .projects import ProjectService from .uploads import UploadEndpoint @@ -42,6 +42,8 @@ ENABLE_CONFIG_TOOLS_ENV = "NOCOBASE_MCP_ENABLE_CONFIG_TOOLS" UPLOAD_API_BASE_URL_ENV = "NOCOBASE_MCP_UPLOAD_API_BASE_URL" UPLOAD_JWT_ENV = "NOCOBASE_MCP_UPLOAD_JWT" UPLOAD_MAX_BYTES_ENV = "NOCOBASE_MCP_UPLOAD_MAX_BYTES" +API_BASE_URL_ENV = "NOCOBASE_MCP_API_BASE_URL" +API_TOKEN_ENV = "NOCOBASE_MCP_API_TOKEN" DEFAULT_UPLOAD_MAX_BYTES = 25 * 1024 * 1024 @@ -109,6 +111,13 @@ def _make_service(config_path: str | Path | None = None) -> CrmService: def _safe_tool_result(operation: Callable[[], dict[str, Any]]) -> dict[str, Any]: try: return operation() + except NbAuthenticationError as error: + return _tool_error( + code="nb_auth_failed", + message=str(error), + hint=f"Rotate {API_TOKEN_ENV} in the private service environment file, then restart the service.", + retryable=False, + ) except NbCommandError as error: return _tool_error( code="nb_command_failed", @@ -402,13 +411,27 @@ def create_http_app(server: Any, token: str, upload_endpoint: UploadEndpoint | N return BearerTokenAuthMiddleware(app, token) -def _read_upload_environment() -> tuple[str, str, int]: - api_base_url = os.environ.get(UPLOAD_API_BASE_URL_ENV, "").strip() - if not api_base_url: - raise RuntimeError(f"{UPLOAD_API_BASE_URL_ENV} must be set for HTTP write mode") - jwt = os.environ.get(UPLOAD_JWT_ENV, "").strip() - if not jwt: - raise RuntimeError(f"{UPLOAD_JWT_ENV} must be set for HTTP write mode") +def _read_nocobase_environment() -> tuple[str, str, int]: + canonical_url = os.environ.get(API_BASE_URL_ENV, "").strip().rstrip("/") + canonical_token = os.environ.get(API_TOKEN_ENV, "").strip() + legacy_url = os.environ.get(UPLOAD_API_BASE_URL_ENV, "").strip().rstrip("/") + legacy_token = os.environ.get(UPLOAD_JWT_ENV, "").strip() + + canonical_any = bool(canonical_url or canonical_token) + legacy_any = bool(legacy_url or legacy_token) + if canonical_any and not (canonical_url and canonical_token): + raise RuntimeError(f"{API_BASE_URL_ENV} and {API_TOKEN_ENV} must be set together") + if legacy_any and not (legacy_url and legacy_token): + raise RuntimeError(f"{UPLOAD_API_BASE_URL_ENV} and {UPLOAD_JWT_ENV} must be set together") + if canonical_any and legacy_any and (canonical_url != legacy_url or canonical_token != legacy_token): + raise RuntimeError("Canonical and legacy NocoBase credential environment variables conflict") + if canonical_any: + api_base_url, api_token = canonical_url, canonical_token + elif legacy_any: + api_base_url, api_token = legacy_url, legacy_token + else: + raise RuntimeError(f"{API_BASE_URL_ENV} and {API_TOKEN_ENV} must be set for HTTP write mode") + raw_max_bytes = os.environ.get(UPLOAD_MAX_BYTES_ENV, str(DEFAULT_UPLOAD_MAX_BYTES)).strip() try: max_bytes = int(raw_max_bytes) @@ -416,7 +439,7 @@ def _read_upload_environment() -> tuple[str, str, int]: raise RuntimeError(f"{UPLOAD_MAX_BYTES_ENV} must be a positive integer") from error if max_bytes <= 0: raise RuntimeError(f"{UPLOAD_MAX_BYTES_ENV} must be a positive integer") - return api_base_url, jwt, max_bytes + return api_base_url, api_token, max_bytes def run_http_server( @@ -432,12 +455,18 @@ def run_http_server( import uvicorn token = _read_token_from_env(token_env) - api_base_url, jwt, max_bytes = _read_upload_environment() + api_base_url, api_token, max_bytes = _read_nocobase_environment() config = load_config(config_path) nb_bin = os.environ.get("NOCOBASE_MCP_NB_BIN", "").strip() or "nb" - nb_client = NbClient(env_name=config.env_name, data_source=config.data_source, nb_bin=nb_bin) + nb_client = NbClient( + env_name=config.env_name, + data_source=config.data_source, + nb_bin=nb_bin, + api_base_url=api_base_url, + api_token=api_token, + ) crm_service = CrmService(config, nb_client) - http_client = NocoBaseHttpClient(api_base_url, jwt) + http_client = NocoBaseHttpClient(api_base_url, api_token) project_service = ProjectService(config.project, nb_client, http_client) server = create_server( crm_service=crm_service, diff --git a/nocobase_crm_mcp/uploads.py b/nocobase_crm_mcp/uploads.py index 0a2d202..351a85f 100644 --- a/nocobase_crm_mcp/uploads.py +++ b/nocobase_crm_mcp/uploads.py @@ -87,7 +87,7 @@ class UploadEndpoint: {"upload_id": attachment_id, "filename": filename, "size": size, "mimetype": mimetype} ) except NocoBaseHttpError as error: - code = "nb_upload_unauthorized" if error.status_code in {401, 403} else "nb_upload_failed" + code = "nb_upload_unauthorized" if error.authentication_failed else "nb_upload_failed" return _error_response(code, "NocoBase attachment upload failed", 502) except Exception: return _error_response("nb_upload_failed", "NocoBase attachment upload failed", 502) diff --git a/scripts/manage_mcp_service.py b/scripts/manage_mcp_service.py index 4254336..54cdc31 100755 --- a/scripts/manage_mcp_service.py +++ b/scripts/manage_mcp_service.py @@ -29,6 +29,10 @@ DEFAULT_NB_BIN = "nb" DEFAULT_TOKEN_BYTES = 32 DEFAULT_UV_CACHE_DIR = "/tmp/uv-cache" DEFAULT_UPLOAD_MAX_BYTES = 25 * 1024 * 1024 +API_BASE_URL_ENV = "NOCOBASE_MCP_API_BASE_URL" +API_TOKEN_ENV = "NOCOBASE_MCP_API_TOKEN" +LEGACY_API_BASE_URL_ENV = "NOCOBASE_MCP_UPLOAD_API_BASE_URL" +LEGACY_API_TOKEN_ENV = "NOCOBASE_MCP_UPLOAD_JWT" MANAGED_ENV_NAMES = frozenset( { "NOCOBASE_MCP_TOKEN", @@ -438,11 +442,26 @@ def service_status(*, project_root: Path) -> str: f"Env file: {paths.env_path}", ] if paths.env_path.exists(): + values = read_service_env_file(paths.env_path) token, _host, port, path, _nb_env, _config_path, nb_bin = saved_service_config(project_root) hosts = detect_remote_hosts() host = hosts[0] if hosts else "" lines.append(f"MCP URL: http://{host}:{port}{path}") lines.append(f"nb binary: {nb_bin}") + canonical_configured = bool(values.get(API_BASE_URL_ENV) and values.get(API_TOKEN_ENV)) + legacy_configured = bool(values.get(LEGACY_API_BASE_URL_ENV) and values.get(LEGACY_API_TOKEN_ENV)) + if canonical_configured: + credential_state = "configured (canonical)" + elif legacy_configured: + credential_state = "configured (legacy)" + elif any( + values.get(name) + for name in (API_BASE_URL_ENV, API_TOKEN_ENV, LEGACY_API_BASE_URL_ENV, LEGACY_API_TOKEN_ENV) + ): + credential_state = "incomplete" + else: + credential_state = "not configured" + lines.append(f"NocoBase API credentials: {credential_state}") probe = probe_http_service("127.0.0.1", port, path, token=token) if probe.get("reachable"): lines.append(f"HTTP probe: reachable (status {probe.get('status')})") diff --git a/tests/test_crm_service.py b/tests/test_crm_service.py index f924950..96824bf 100644 --- a/tests/test_crm_service.py +++ b/tests/test_crm_service.py @@ -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) diff --git a/tests/test_mcp_service.py b/tests/test_mcp_service.py index 8568625..074255c 100644 --- a/tests/test_mcp_service.py +++ b/tests/test_mcp_service.py @@ -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() diff --git a/tests/test_nb_client.py b/tests/test_nb_client.py index d6dc138..e84eaf6 100644 --- a/tests/test_nb_client.py +++ b/tests/test_nb_client.py @@ -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("", 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="") diff --git a/tests/test_nocobase_http.py b/tests/test_nocobase_http.py index e7c6f59..09eedb9 100644 --- a/tests/test_nocobase_http.py +++ b/tests/test_nocobase_http.py @@ -124,6 +124,26 @@ class NocoBaseHttpClientTests(unittest.TestCase): self.assertNotIn("super-secret-token", str(caught.exception)) self.assertIn("", 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() diff --git a/tests/test_projects.py b/tests/test_projects.py index a99f075..5bda86e 100644 --- a/tests/test_projects.py +++ b/tests/test_projects.py @@ -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]) diff --git a/tests/test_server.py b/tests/test_server.py index cedbd47..0fc2cac 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -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, diff --git a/tests/test_uploads.py b/tests/test_uploads.py index 1e7a117..942a155 100644 --- a/tests/test_uploads.py +++ b/tests/test_uploads.py @@ -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",