mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-14 00:16:20 +00:00
Keep the removed-connection note server-owned
Only removing a connection notes it on a kept tool. A config save through update_tool or update_tool_config carries the stored note and ignores the client's copy, so it can neither fake nor clear it; creating a tool, an MCP server save and an agent import start without one. The note is now written even for a connection with no catalog key, and a kept tool its owner gave credentials of its own since runs again.
This commit is contained in:
1 parent
d04f76e97a
commit
3cab8af753
6 files changed
+79
-10
No files matched your search
@@ -38,6 +38,7 @@ from docsgpt.agents.default_tools import (
|
||||
from docsgpt.api import api
|
||||
from docsgpt.api.pat.rules import allowed_ids
|
||||
from docsgpt.api.user.resource_access import AccessDenied, require
|
||||
from docsgpt.connectors.resolve import carry_removed_connection
|
||||
from docsgpt.core.model_utils import validate_model_id
|
||||
from docsgpt.core.url_validation import SSRFError, validate_url
|
||||
from docsgpt.security.safe_url import UnsafeUserUrlError, validate_user_base_url
|
||||
@@ -1138,7 +1139,8 @@ def _create_tool_from_spec(conn, user: str, tool: dict, secrets: dict, warnings:
|
||||
warnings.append(f"Tool type '{tool_type}' not available on this instance; skipped")
|
||||
return None
|
||||
config_requirements = inst.get_config_requirements() or {}
|
||||
config = dict(tool.get("config") or {})
|
||||
# An imported tool starts with no note that a connection was removed.
|
||||
config = carry_removed_connection(dict(tool.get("config") or {}), None)
|
||||
config.update(secrets or {})
|
||||
if tool_type == "api_tool":
|
||||
label = tool.get("display_name") or tool.get("name") or tool_type
|
||||
|
||||
@@ -19,6 +19,7 @@ from docsgpt.api.user.tools.routes import (
|
||||
transform_actions,
|
||||
)
|
||||
from docsgpt.cache import get_redis_instance
|
||||
from docsgpt.connectors.resolve import REMOVED_CONNECTION_KEY
|
||||
from docsgpt.core.url_validation import SSRFError, validate_url
|
||||
from docsgpt.security.encryption import decrypt_credentials, encrypt_credentials
|
||||
from docsgpt.storage.db.repositories.connector_sessions import (
|
||||
@@ -37,8 +38,10 @@ def _sanitize_mcp_transport(config):
|
||||
"""Normalise and validate the transport_type field.
|
||||
|
||||
Strips ``command`` / ``args`` keys that are only valid for local STDIO
|
||||
transports, and ``connection_id``, which only the tool executor sets
|
||||
(it picks whose MCP tokens the tool uses). Returns the cleaned
|
||||
transports, ``connection_id``, which only the tool executor sets
|
||||
(it picks whose MCP tokens the tool uses), and the note that a
|
||||
connection was removed, which only removing one writes (a save stores
|
||||
a fresh config, so it drops that note too). Returns the cleaned
|
||||
transport type string.
|
||||
"""
|
||||
transport_type = (config.get("transport_type") or "auto").lower()
|
||||
@@ -47,6 +50,7 @@ def _sanitize_mcp_transport(config):
|
||||
config.pop("command", None)
|
||||
config.pop("args", None)
|
||||
config.pop("connection_id", None)
|
||||
config.pop(REMOVED_CONNECTION_KEY, None)
|
||||
config["transport_type"] = transport_type
|
||||
return transport_type
|
||||
|
||||
|
||||
@@ -35,6 +35,7 @@ from docsgpt.api.user.resource_access import (
|
||||
)
|
||||
from docsgpt.api.user.team_sharing import visible_with_access
|
||||
from docsgpt.connectors.catalog import base_url, definition_for_tool
|
||||
from docsgpt.connectors.resolve import carry_removed_connection
|
||||
from docsgpt.connectors.service import account_tool_names
|
||||
from docsgpt.connectors.permissions import owner_credential_writes
|
||||
from docsgpt.core.settings import settings
|
||||
@@ -521,12 +522,16 @@ def _prepare_tool_config(tool_doc: dict, new_config: dict, config_requirements:
|
||||
Handles the three secret stores: ``config_requirements`` secrets
|
||||
(``encrypted_credentials``), api_tool header/query values, and the MCP
|
||||
origin-change rule (a new scheme, host or port drops stored credentials).
|
||||
A removed connection's note is carried over from the stored config; the
|
||||
client's copy is ignored.
|
||||
|
||||
Raises:
|
||||
CredentialsRequired: the MCP origin changed and no new secret arrived.
|
||||
"""
|
||||
owner_id = tool_doc["user_id"]
|
||||
existing_config = tool_doc.get("config") or {}
|
||||
# The note that its connection was removed is the server's to keep.
|
||||
new_config = carry_removed_connection(new_config, existing_config)
|
||||
if tool_doc.get("name") == "api_tool":
|
||||
return _seal_api_tool_secrets(new_config, existing_config, owner_id)
|
||||
moved = tool_doc.get("name") == "mcp_tool" and _mcp_origin_changed(new_config, existing_config)
|
||||
@@ -834,6 +839,9 @@ class CreateTool(Resource):
|
||||
missing_fields = check_required_fields(data, required_fields)
|
||||
if missing_fields:
|
||||
return missing_fields
|
||||
if isinstance(data.get("config"), dict):
|
||||
# Only removing a connection notes that it was removed.
|
||||
data["config"] = carry_removed_connection(data["config"], None)
|
||||
try:
|
||||
if data["name"] == "mcp_tool":
|
||||
server_url = (data.get("config", {}).get("server_url") or "").strip()
|
||||
|
||||
@@ -28,11 +28,33 @@ CONNECTION_NEEDS_RECONNECT = "connection_needs_reconnect"
|
||||
CONNECTION_REMOVED = "connection_removed"
|
||||
CONNECTOR_DISABLED = "connector_disabled"
|
||||
|
||||
# Tool config key ``remove_connection`` sets to the connector key when it
|
||||
# keeps a connection's tools, so they can say what they lost.
|
||||
# Tool config key ``remove_connection`` sets when it keeps a connection's
|
||||
# tools: the connector key, or True when the connection had none, so they can
|
||||
# say what they lost. Only the server writes it (see
|
||||
# :func:`carry_removed_connection`).
|
||||
REMOVED_CONNECTION_KEY = "removed_connection"
|
||||
|
||||
|
||||
def carry_removed_connection(new_config: dict, stored_config: Optional[dict]) -> dict:
|
||||
"""``new_config`` with the stored removed-connection note, never a client's.
|
||||
|
||||
A config save must neither fake the note nor clear it; every path that
|
||||
writes a tool config from a request passes it through here.
|
||||
|
||||
Args:
|
||||
new_config: The config about to be stored.
|
||||
stored_config: The tool's stored config (None or ``{}`` when creating it).
|
||||
|
||||
Returns:
|
||||
dict: A copy of ``new_config`` carrying the stored note, if any.
|
||||
"""
|
||||
out = {k: v for k, v in (new_config or {}).items() if k != REMOVED_CONNECTION_KEY}
|
||||
marker = (stored_config or {}).get(REMOVED_CONNECTION_KEY)
|
||||
if marker:
|
||||
out[REMOVED_CONNECTION_KEY] = marker
|
||||
return out
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class ResolvedConnection:
|
||||
"""The connection a call runs with, or why there is none.
|
||||
@@ -175,15 +197,18 @@ def connection_stop_reason(tool: dict, resolved: Optional[ResolvedConnection]) -
|
||||
|
||||
Returns:
|
||||
:data:`CONNECTION_REMOVED` when its connection was removed and the
|
||||
tool kept (``remove_connection`` notes it on the tool), or points at
|
||||
tool kept (``remove_connection`` notes it on the tool) with no
|
||||
credentials of its own, or points at
|
||||
one it may not use; :data:`CONNECTOR_DISABLED` when an admin turned
|
||||
the service off; :data:`CONNECTION_NEEDS_RECONNECT` when the account
|
||||
must sign in again; else None.
|
||||
"""
|
||||
if resolved is None:
|
||||
# Only a tool that had a connection lost it: a tool that never had
|
||||
# one (a tokenless ntfy, a legacy tool) runs on its own config.
|
||||
removed = (tool.get("config") or {}).get(REMOVED_CONNECTION_KEY)
|
||||
# one (a tokenless ntfy, a legacy tool) runs on its own config, and
|
||||
# so does a kept one its owner gave credentials of its own since.
|
||||
config = tool.get("config") or {}
|
||||
removed = config.get(REMOVED_CONNECTION_KEY) and not config.get("encrypted_credentials")
|
||||
return CONNECTION_REMOVED if removed and not tool.get("connection_id") else None
|
||||
if not resolved.enabled:
|
||||
return CONNECTOR_DISABLED
|
||||
|
||||
@@ -1460,9 +1460,9 @@ def remove_connection(conn, row: dict, *, sources: str = "keep", tools: str = "d
|
||||
text(
|
||||
"UPDATE user_tools SET status = false, "
|
||||
"config = COALESCE(config, '{}'::jsonb) || jsonb_build_object('removed_connection', "
|
||||
"CAST(:key AS text)) WHERE connection_id = CAST(:id AS uuid)"
|
||||
"CAST(:marker AS jsonb)) WHERE connection_id = CAST(:id AS uuid)"
|
||||
),
|
||||
{"id": connection_id, "key": catalog.connector_key_for_row(row) or ""},
|
||||
{"id": connection_id, "marker": json.dumps(catalog.connector_key_for_row(row) or True)},
|
||||
)
|
||||
conn.execute(
|
||||
text(
|
||||
|
||||
@@ -408,6 +408,36 @@ class TestToolWrites:
|
||||
# ---------------------------------------------------------------------------
|
||||
# 5. POST /api/update_tool_status
|
||||
# ---------------------------------------------------------------------------
|
||||
class TestRemovedConnectionNote:
|
||||
"""Only removing a connection notes it on a kept tool; config saves can't fake or clear it."""
|
||||
|
||||
@pytest.mark.parametrize("route", ["UpdateToolConfig", "UpdateTool"])
|
||||
def test_a_config_save_cannot_set_the_note(self, app, pg_conn, route):
|
||||
from docsgpt.api.user.tools import routes
|
||||
|
||||
tool = _tool(pg_conn, name="brave", config={})
|
||||
body = {"id": str(tool["id"]), "config": {"token": "x", "removed_connection": "brave"}}
|
||||
assert _call(app, pg_conn, getattr(routes, route), OWNER, json=body).status_code == 200
|
||||
assert "removed_connection" not in _row(pg_conn, tool["id"])["config"]
|
||||
|
||||
@pytest.mark.parametrize("route", ["UpdateToolConfig", "UpdateTool"])
|
||||
def test_a_config_save_keeps_the_stored_note(self, app, pg_conn, route):
|
||||
from docsgpt.api.user.tools import routes
|
||||
|
||||
tool = _tool(pg_conn, name="brave", config={"removed_connection": "brave"})
|
||||
for config in ({"token": "x"}, {"token": "y", "removed_connection": None}):
|
||||
body = {"id": str(tool["id"]), "config": config}
|
||||
assert _call(app, pg_conn, getattr(routes, route), OWNER, json=body).status_code == 200
|
||||
assert _row(pg_conn, tool["id"])["config"]["removed_connection"] == "brave"
|
||||
|
||||
def test_an_mcp_save_drops_the_note(self):
|
||||
from docsgpt.api.user.tools.mcp import _sanitize_mcp_transport
|
||||
|
||||
cfg = {"transport_type": "http", "removed_connection": "github"}
|
||||
_sanitize_mcp_transport(cfg)
|
||||
assert "removed_connection" not in cfg
|
||||
|
||||
|
||||
class TestUpdateToolStatusRoles:
|
||||
def test_owner_writes_status(self, app, pg_conn):
|
||||
from docsgpt.api.user.tools.routes import UpdateToolStatus
|
||||
|
||||
Reference in new issue
Block a user