From 12ad4f5e858f1f9f029d0f7238b14ff731ce83bb Mon Sep 17 00:00:00 2001 From: arc53-machine <232052973+arc53-machine@users.noreply.github.com> Date: Tue, 29 Sep 2026 11:12:07 +0100 Subject: [PATCH] Let a GitHub connection opt into write tools A GitHub connection's MCP tool can now point at GitHub's full endpoint (/mcp/) instead of the read-only one when its owner opts in, at setup (allow_writes) or later (PUT /api/connections//writes), which re-reads the actions and keeps the choices for those on both endpoints. Actions from the write endpoint are writes unless GitHub marks them read-only, so they default to asking first. Admins can forbid it per connector (allow_writes in Admin > Connectors, kept in app_metadata). Then the option is refused, a refresh goes back to read-only, and at run time the tool only ever calls the read-only endpoint and write calls are denied with a reason. --- docsgpt/agents/tool_executor.py | 29 ++ docsgpt/api/admin/connectors.py | 23 +- docsgpt/api/connector/connections.py | 55 ++- docsgpt/connectors/catalog.py | 11 +- docsgpt/connectors/mcp.py | 159 ++++++-- docsgpt/connectors/resolve.py | 11 + docsgpt/connectors/service.py | 83 +++- .../db/repositories/connector_policies.py | 10 + tests/connectors/test_github_writes.py | 363 ++++++++++++++++++ 9 files changed, 706 insertions(+), 38 deletions(-) create mode 100644 tests/connectors/test_github_writes.py diff --git a/docsgpt/agents/tool_executor.py b/docsgpt/agents/tool_executor.py index 95f8ba6b..51df4a3d 100644 --- a/docsgpt/agents/tool_executor.py +++ b/docsgpt/agents/tool_executor.py @@ -1033,6 +1033,29 @@ class ToolExecutor: or require_approval ) + # An admin forbade changes through this connector (GitHub): its tool + # already calls the read-only endpoint, so say why instead of failing. + if resolved is not None and not resolved.writes_allowed: + from docsgpt.connectors.permissions import ACCESS_WRITE, action_access + + if action_access(tool_data.get("name"), action_data) == ACCESS_WRITE: + return { + "call_id": call_id, + "name": llm_name, + "tool_name": tool_data.get("name", "unknown"), + "tool_id": tool_id, + "action_name": action_name, + "llm_name": llm_name, + "arguments": arguments, + "pause_type": "headless_denied", + "deny_reason": ( + f"An admin turned off changes through {resolved.connector_name or 'this service'}. " + "It can only look things up." + ), + "error_type": "tool_not_allowed", + "thought_signature": getattr(call, "thought_signature", None), + } + # A member running someone else's account (a shared tool in owner # mode) always confirms write actions, whatever the owner chose for # themselves. @@ -1793,6 +1816,12 @@ class ToolExecutor: f"{resolved.connector_name or 'This service'} needs to be connected", connection_id=resolved.connection_id, ) + if service.builtin_mcp_config(definition) is not None: + # Only the connector's own endpoints: GitHub's write one while + # the tool opted in and an admin allows it, else read-only. + tool_config["server_url"] = service.builtin_mcp_url( + definition, tool_config.get("server_url"), resolved.writes_allowed, + ) audit_delegation( resolved, invoker=self.user, diff --git a/docsgpt/api/admin/connectors.py b/docsgpt/api/admin/connectors.py index 814a89b0..fe360cfc 100644 --- a/docsgpt/api/admin/connectors.py +++ b/docsgpt/api/admin/connectors.py @@ -23,6 +23,7 @@ from docsgpt.storage.db.repositories.connector_policies import ( ALLOW_CUSTOM_MCP_KEY, CREDENTIAL_POLICIES, ConnectorPoliciesRepository, + allow_writes_key, ) from docsgpt.storage.db.session import db_readonly, db_session @@ -57,6 +58,7 @@ class AdminConnectorsResource(Resource): """Every connector with its policy, setup state and connection count.""" with db_readonly() as conn: policies = ConnectorPoliciesRepository(conn).all() + loaded = service.load_policies(conn) allow_custom = service.custom_mcp_allowed(conn) counts = _connection_counts(conn) connectors = [] @@ -88,6 +90,11 @@ class AdminConnectorsResource(Resource): "connection_count": counts.get(definition.key, 0), "docs_url": definition.docs_url, "mcp_url": definition.mcp_url, + # Whether members may let agents make changes (GitHub); + # None where the connector offers no such choice. + "allow_writes": ( + service.writes_allowed(loaded, definition.key) if definition.mcp_write_url else None + ), } ) return make_response( @@ -106,7 +113,7 @@ class AdminConnectorsResource(Resource): @admin_required def put(self): - """Update policies: {policies: {key: {enabled?, credential_mode?}}, allow_custom_mcp?}.""" + """Update policies: {policies: {key: {enabled?, credential_mode?, allow_writes?}}, allow_custom_mcp?}.""" body = request.get_json(silent=True) or {} updates = body.get("policies") or {} if not isinstance(updates, dict): @@ -119,6 +126,15 @@ class AdminConnectorsResource(Resource): return make_response(jsonify({"success": False, "message": "Unknown credential mode"}), 400) if "enabled" in change and not isinstance(change["enabled"], bool): return make_response(jsonify({"success": False, "message": "enabled must be true or false"}), 400) + if "allow_writes" in change: + if not catalog.get_definition(key).mcp_write_url: + return make_response( + jsonify({"success": False, "message": f"{key} has no write access to allow"}), 400, + ) + if not isinstance(change["allow_writes"], bool): + return make_response( + jsonify({"success": False, "message": "allow_writes must be true or false"}), 400, + ) allow_custom = body.get("allow_custom_mcp") if allow_custom is not None and not isinstance(allow_custom, bool): return make_response(jsonify({"success": False, "message": "allow_custom_mcp must be a boolean"}), 400) @@ -126,6 +142,11 @@ class AdminConnectorsResource(Resource): with db_session() as conn: repo = ConnectorPoliciesRepository(conn) for key, change in updates.items(): + if "allow_writes" in change: + value = "true" if change["allow_writes"] else "false" + AppMetadataRepository(conn).set(allow_writes_key(key), value) + if set(change) == {"allow_writes"}: + continue repo.upsert( key, enabled=change.get("enabled"), diff --git a/docsgpt/api/connector/connections.py b/docsgpt/api/connector/connections.py index 8c6cb5cf..352d8b2e 100644 --- a/docsgpt/api/connector/connections.py +++ b/docsgpt/api/connector/connections.py @@ -229,8 +229,9 @@ class ConnectionDisconnect(Resource): class ConnectionSetup(Resource): @api.doc( description=( - "Apply the connect wizard's choices: {create_tools, tool_permissions?, " - "sync?: {items, frequency, name?}}. Honours an Idempotency-Key header for the sync." + "Apply the connect wizard's choices: {create_tools, allow_writes?, tool_permissions?, " + "sync?: {items, frequency, name?}}. allow_writes points GitHub's tool at its write endpoint. " + "Honours an Idempotency-Key header for the sync." ) ) def post(self, connection_id: str): @@ -239,22 +240,31 @@ class ConnectionSetup(Resource): return _unauthorized() body = _json_body() create_tools = body.get("create_tools", True) + allow_writes = body.get("allow_writes", False) + if not isinstance(allow_writes, bool): + return _error("allow_writes must be true or false", 400) try: mcp_actions = None with db_readonly() as conn: row = _owned(conn, connection_id, user_id) discover = bool(row) and create_tools and service.needs_mcp_discovery(conn, row) + forbidden = bool(row) and allow_writes and not service.writes_allowed( + service.load_policies(conn), catalog.connector_key_for_row(row), + ) if row is None: return _not_found() if service.normalize_status(row) != service.STATUS_CONNECTED: return _error("Reconnect before setting up", 409, code="reconnect") + if forbidden: + return _error("Changes through this connector are turned off by an admin", 403, + code="writes_forbidden") if discover: # GitHub's tool is its MCP server: read its actions before # the write transaction, not while holding it open. from docsgpt.connectors.mcp import discover_builtin_actions try: - mcp_actions = discover_builtin_actions(user_id, row) + mcp_actions = discover_builtin_actions(user_id, row, writes=allow_writes) except service.ConnectionUnavailable: return _error("Reconnect before setting up", 409, code="reconnect") except Exception as err: @@ -269,7 +279,7 @@ class ConnectionSetup(Resource): if create_tools: tools = service.ensure_connection_tools( conn, user_id, row, permissions=body.get("tool_permissions") or None, - mcp_actions=mcp_actions, + mcp_actions=mcp_actions, mcp_writes=allow_writes, ) account_parameters = service.connection_parameters(row) tool_payload = [service.serialize_tool(tool, account_parameters) for tool in tools] @@ -587,6 +597,43 @@ class ConnectionRefreshTools(Resource): return make_response(jsonify({"success": True, **diff}), 200) +@connections_ns.route("/connections//writes") +class ConnectionWrites(Resource): + @api.doc( + description=( + "GitHub: let agents make changes through this connection, or only read: {allow}. " + "Re-reads the tool's actions from the matching endpoint. Owner only." + ) + ) + def put(self, connection_id: str): + from docsgpt.connectors.mcp import NoMcpTool, set_builtin_writes + + user_id = _user_id() + if not user_id: + return _unauthorized() + allow = _json_body().get("allow") + if not isinstance(allow, bool): + return _error("allow must be true or false", 400) + with db_readonly() as conn: + row = _owned(conn, connection_id, user_id) + if row is None: + return _not_found() + try: + result = set_builtin_writes(user_id, row, allow) + except service.WritesForbidden as err: + return _error(str(err), 403, code="writes_forbidden") + except NoMcpTool as err: + return _error(str(err), 409, code="no_tools") + except ValueError as err: + return _error(str(err), 400) + except service.ConnectionUnavailable: + return _error("Reconnect to continue", 409, code="reconnect") + except Exception as err: + current_app.logger.error(f"Error switching write access: {err}", exc_info=True) + return _error("The service's tools could not be reached. Try again.", 502, code="tools_unavailable") + return make_response(jsonify({"success": True, **result}), 200) + + @connections_ns.route("/connections/tools//credential-mode") class ToolCredentialMode(Resource): @api.doc( diff --git a/docsgpt/connectors/catalog.py b/docsgpt/connectors/catalog.py index e3a2d34d..ce3eefcc 100644 --- a/docsgpt/connectors/catalog.py +++ b/docsgpt/connectors/catalog.py @@ -89,6 +89,10 @@ class ConnectorDefinition: (created and enabled), ``ask`` or ``off``; ``sync`` likewise. mcp_url: MCP endpoint for presets, and for a built-in connector whose tool is its service's own MCP server (GitHub). + mcp_write_url: The same server's endpoint that also offers write + actions, which a connection opts into and an admin can forbid + (GitHub's full server beside its read-only one). ``mcp_url`` + stays the default. publisher: ``built_in``, ``preset`` or ``custom``. docs_url: Setup guide for admins. oauth_scopes: Scopes an MCP preset requests. @@ -115,6 +119,7 @@ class ConnectorDefinition: tool_templates: tuple[str, ...] = () setup: dict = field(default_factory=lambda: {"tools": "auto", "sync": "ask"}) mcp_url: Optional[str] = None + mcp_write_url: Optional[str] = None publisher: str = "built_in" docs_url: Optional[str] = None oauth_scopes: tuple[str, ...] = () @@ -163,6 +168,7 @@ class ConnectorDefinition: "tool_templates": list(self.tool_templates), "setup": dict(self.setup), "mcp_url": self.mcp_url, + "writes_opt_in": bool(self.mcp_write_url), "publisher": self.publisher, "docs_url": self.docs_url, "oauth_scopes": list(self.oauth_scopes), @@ -181,6 +187,7 @@ def base_url(url: Optional[str]) -> str: _DOCS = "https://docs.docsgpt.cloud/Guides/Connectors" GITHUB_MCP_URL = "https://api.githubcopilot.com/mcp/readonly" +GITHUB_MCP_WRITE_URL = "https://api.githubcopilot.com/mcp/" # Tool templates any connector's server can use (an MCP server, an OpenAPI # spec): a tool made from one belongs to its connection, not to a connector. @@ -239,9 +246,11 @@ _BUILT_IN: tuple[ConnectorDefinition, ...] = ( oauth_settings=("GITHUB_CLIENT_ID", "GITHUB_CLIENT_SECRET", "GITHUB_APP_SLUG"), sync_ingestor="github", setup_fields=(CredentialField("repo_url", "Repository", secret=False),), - # GitHub's own MCP server, read-only: agents look things up, never change them. + # GitHub's own MCP server, read-only unless the connection opts into + # changes (issues, comments, pull requests) and an admin allows them. tool_templates=("mcp_tool",), mcp_url=GITHUB_MCP_URL, + mcp_write_url=GITHUB_MCP_WRITE_URL, setup={"tools": "ask", "sync": "ask"}, docs_url=f"{_DOCS}#github", ), diff --git a/docsgpt/connectors/mcp.py b/docsgpt/connectors/mcp.py index 6087c2ba..344b3c20 100644 --- a/docsgpt/connectors/mcp.py +++ b/docsgpt/connectors/mcp.py @@ -1,15 +1,38 @@ -"""MCP helpers for connections: re-scanning a server's actions.""" +"""MCP helpers for connections: re-scanning a server's actions, switching GitHub's writes.""" from __future__ import annotations +from typing import Optional + from docsgpt.agents.tool_pins import carry_pins from docsgpt.connectors import catalog, service -from docsgpt.connectors.permissions import apply_default_permissions +from docsgpt.connectors.catalog import ConnectorDefinition +from docsgpt.connectors.permissions import ACCESS_READ, ACCESS_WRITE, apply_default_permissions from docsgpt.storage.db.repositories.connector_sessions import ConnectorSessionsRepository from docsgpt.storage.db.repositories.user_tools import UserToolsRepository from docsgpt.storage.db.session import db_readonly, db_session +class NoMcpTool(LookupError): + """The connection has no MCP tool yet (it was set up without tools).""" + + +def _builtin_definition(connection: dict) -> Optional[ConnectorDefinition]: + """The connection's built-in connector when its tool is that service's MCP server.""" + definition = catalog.get_definition(catalog.connector_key_for_row(connection)) + return definition if service.builtin_mcp_config(definition) is not None else None + + +def _write_endpoint_access(action: dict) -> dict: + """Stamp an action of a write endpoint: a read only when the server marks it read-only. + + GitHub marks its read tools, so a name is no evidence there: from the name + alone ``mark_all_notifications_read`` would pass for a read. + """ + annotations = action.get("annotations") if isinstance(action.get("annotations"), dict) else {} + return {**action, "access": ACCESS_READ if annotations.get("readOnlyHint") is True else ACCESS_WRITE} + + def _discover(user_id: str, connection: dict, tool: dict) -> list[dict]: from docsgpt.agents.tools.mcp_tool import MCPTool @@ -26,56 +49,138 @@ def _discover(user_id: str, connection: dict, tool: dict) -> list[dict]: return mcp_tool.get_actions_metadata() -def discover_builtin_actions(user_id: str, connection: dict) -> list[dict]: +def _classified(connection: dict, config: dict, actions: list[dict]) -> list[dict]: + """``actions`` as read from ``config``'s server, stamped strictly when it is a write endpoint.""" + definition = _builtin_definition(connection) + if definition is None or not definition.mcp_write_url or config.get("server_url") != definition.mcp_write_url: + return actions + return [_write_endpoint_access(action) for action in actions] + + +def discover_builtin_actions(user_id: str, connection: dict, writes: bool = False) -> list[dict]: """The actions of a built-in connector's MCP server (GitHub's), read with the connection. + Args: + user_id: The connection's owner. + connection: The connection row. + writes: Read the endpoint that also offers write actions; the caller + checked that an admin allows them. + Raises: ValueError: The connector has no MCP server of its own. service.ConnectionUnavailable: The connection needs reconnecting. Exception: The server could not be reached or listed. """ - definition = catalog.get_definition(catalog.connector_key_for_row(connection)) - config = service.builtin_mcp_config(definition) + config = service.builtin_mcp_config(_builtin_definition(connection), writes=writes) if config is None: raise ValueError("This connector has no MCP server") - return _discover(user_id, connection, {"config": config}) + return _classified(connection, config, _discover(user_id, connection, {"config": config})) -def refresh_mcp_tools(user_id: str, connection: dict) -> dict: - """Re-read the actions of every MCP tool on a connection. +def _rediscover(user_id: str, connection: dict, tool: dict, config: dict) -> tuple[set, set, dict]: + """Re-read one MCP tool's actions from ``config``'s server and store both. Actions that still exist keep the permissions the user chose and the fixed values of parameters they still have; new ones get the defaults from their annotations (reads always allowed, writes need approval); removed ones disappear. + Returns: + ``(added, removed, tool)``: action names, and the serialized tool. + """ + discovered = _classified(connection, config, _discover(user_id, connection, {**tool, "config": config})) + fresh = apply_default_permissions("mcp_tool", service._transform_actions(discovered)) + previous = {a.get("name"): a for a in (tool.get("actions") or []) if isinstance(a, dict)} + merged = [] + for action in fresh: + old = previous.get(action.get("name")) + if old is not None: + action = { + **carry_pins(old, action), + "active": old.get("active", True), + "require_approval": bool(old.get("require_approval")), + } + merged.append(action) + names = {a.get("name") for a in merged} + fields = {"actions": merged} + if config != (tool.get("config") or {}): + fields["config"] = config + with db_session() as conn: + repo = UserToolsRepository(conn) + repo.update(str(tool["id"]), user_id, fields) + serialized = service.serialize_tool(repo.get_any(str(tool["id"]), user_id)) + return names - set(previous), set(previous) - names, serialized + + +def refresh_mcp_tools(user_id: str, connection: dict) -> dict: + """Re-read the actions of every MCP tool on a connection. + + A built-in connector's tool is re-read from its own endpoint: the write + one while it is set up for writes and an admin allows them, the read-only + one otherwise, which it then keeps. + Returns: ``{"added": [...], "removed": [...], "tools": [...]}``. """ with db_readonly() as conn: tools = [t for t in ConnectorSessionsRepository(conn).list_tools(str(connection["id"])) if t.get("name") == "mcp_tool"] + policies = service.load_policies(conn) if tools else {} + definition = _builtin_definition(connection) added: set[str] = set() removed: set[str] = set() refreshed = [] for tool in tools: - fresh = apply_default_permissions("mcp_tool", service._transform_actions(_discover(user_id, connection, tool))) - previous = {a.get("name"): a for a in (tool.get("actions") or []) if isinstance(a, dict)} - merged = [] - for action in fresh: - old = previous.get(action.get("name")) - if old is not None: - action = { - **carry_pins(old, action), - "active": old.get("active", True), - "require_approval": bool(old.get("require_approval")), - } - merged.append(action) - names = {a.get("name") for a in merged} - added |= names - set(previous) - removed |= set(previous) - names - with db_session() as conn: - repo = UserToolsRepository(conn) - repo.update(str(tool["id"]), user_id, {"actions": merged}) - refreshed.append(service.serialize_tool(repo.get_any(str(tool["id"]), user_id))) + config = dict(tool.get("config") or {}) + if definition is not None: + config["server_url"] = service.builtin_mcp_url( + definition, config.get("server_url"), service.writes_allowed(policies, definition.key), + ) + tool_added, tool_removed, serialized = _rediscover(user_id, connection, tool, config) + added |= tool_added + removed |= tool_removed + refreshed.append(serialized) return {"added": sorted(added), "removed": sorted(removed), "tools": refreshed} + + +def set_builtin_writes(user_id: str, connection: dict, allow: bool) -> dict: + """Point a connection's built-in MCP tool (GitHub's) at its write or read-only endpoint. + + The tool's actions are re-read from the new endpoint, keeping the + choices made for actions that exist on both. + + Args: + user_id: The connection's owner. + connection: The connection row. + allow: Let agents make changes (the write endpoint) or only read. + + Returns: + ``{"added", "removed", "tools", "writes"}``. + + Raises: + ValueError: The connector offers no write access. + NoMcpTool: The connection has no such tool yet. + service.WritesForbidden: ``allow`` while an admin forbids writes. + service.ConnectionUnavailable: The connection needs reconnecting. + """ + definition = _builtin_definition(connection) + if definition is None or not definition.mcp_write_url: + raise ValueError("This connector has no write access to turn on") + with db_readonly() as conn: + tools = [t for t in ConnectorSessionsRepository(conn).list_tools(str(connection["id"])) + if t.get("name") == "mcp_tool"] + allowed = service.writes_allowed(service.load_policies(conn), definition.key) + if not tools: + raise NoMcpTool("Add this connection's tools first") + if allow and not allowed: + raise service.WritesForbidden(f"An admin turned off changes through {definition.name}") + added: set[str] = set() + removed: set[str] = set() + refreshed = [] + for tool in tools: + config = {**(tool.get("config") or {}), **service.builtin_mcp_config(definition, writes=allow)} + tool_added, tool_removed, serialized = _rediscover(user_id, connection, tool, config) + added |= tool_added + removed |= tool_removed + refreshed.append(serialized) + return {"added": sorted(added), "removed": sorted(removed), "tools": refreshed, "writes": allow} diff --git a/docsgpt/connectors/resolve.py b/docsgpt/connectors/resolve.py index fd9f47f9..4ed4d0fa 100644 --- a/docsgpt/connectors/resolve.py +++ b/docsgpt/connectors/resolve.py @@ -34,6 +34,8 @@ class ResolvedConnection: connector_key: Catalog key of the service. connector_name: Name shown to the user ("Notion", or a custom label). delegated: The row belongs to someone other than the invoker. + writes_allowed: Whether an admin lets agents make changes through a + connector that offers them as an opt-in (GitHub); True elsewhere. """ row: Optional[dict] @@ -41,6 +43,7 @@ class ResolvedConnection: connector_key: Optional[str] connector_name: Optional[str] delegated: bool = False + writes_allowed: bool = True @property def connection_id(self) -> Optional[str]: @@ -104,9 +107,17 @@ def resolve_connection(resource: dict, invoker_user_id: Optional[str]) -> Option connector_key=key, connector_name=_name_for(row or owned, key), delegated=bool(row and invoker_user_id and row.get("user_id") != invoker_user_id), + writes_allowed=_writes_allowed(policies, key), ) +def _writes_allowed(policies: dict, key: Optional[str]) -> bool: + definition = catalog.get_definition(key) if key else None + if definition is None or not definition.mcp_write_url: + return True + return service.writes_allowed(policies, key) + + def _member_connection(repo: ConnectorSessionsRepository, owned: Optional[dict], invoker: str) -> Optional[dict]: """The invoker's own connection to the service the owner's connection is for. diff --git a/docsgpt/connectors/service.py b/docsgpt/connectors/service.py index 94d02e19..5e2a45bf 100644 --- a/docsgpt/connectors/service.py +++ b/docsgpt/connectors/service.py @@ -304,6 +304,7 @@ def connection_detail(conn, row: dict) -> dict: detail = serialize_connection(row, {"sources": len(sources), "tools": len(tools)}) detail["sources"] = sources detail["tools"] = tools + detail["writes"] = builtin_writes(catalog.get_definition(detail["connector_key"]), tool_rows) return detail @@ -361,6 +362,7 @@ def catalog_for_user(conn, user_id: str, *, is_admin: bool, policies: Optional[d "status": status, "state": _card_state(definition, available, disabled, status), "credential_policy": policy.get("credential_mode") or "choose", + "writes_allowed": writes_allowed(policies, definition.key), } ) return entries @@ -1269,12 +1271,17 @@ def split_secrets(config: dict, config_requirements: dict) -> tuple[dict, dict]: return public, secrets -def builtin_mcp_config(definition: Optional[ConnectorDefinition]) -> Optional[dict]: +def builtin_mcp_config(definition: Optional[ConnectorDefinition], writes: bool = False) -> Optional[dict]: """Tool config of a built-in connector whose tool is its service's MCP server. The token is not part of it: the executor reads it from the connection at run time and sends it as a bearer token. + Args: + definition: The connector. + writes: Use the endpoint that also offers write actions, when the + connector has one (GitHub's full server). + Returns: The config, or None when the connector has no such tool. """ @@ -1285,7 +1292,40 @@ def builtin_mcp_config(definition: Optional[ConnectorDefinition]) -> Optional[di or "mcp_tool" not in definition.tool_templates ): return None - return {"server_url": definition.mcp_url, "auth_type": "bearer", "transport_type": "http", "timeout": 30} + url = definition.mcp_write_url if writes and definition.mcp_write_url else definition.mcp_url + return {"server_url": url, "auth_type": "bearer", "transport_type": "http", "timeout": 30} + + +def builtin_mcp_url(definition: Optional[ConnectorDefinition], stored_url: Optional[str], allowed: bool) -> str: + """The endpoint a built-in connector's MCP tool may call, whatever its config says. + + Only the connector's own endpoints are ever used: the write endpoint when + the tool was set up for writes and an admin allows them, the read-only + one otherwise (another path on the same host included). + + Args: + definition: The connector, with ``mcp_url`` set. + stored_url: The ``server_url`` in the tool's config. + allowed: Whether an admin allows writes through this connector. + """ + if allowed and definition.mcp_write_url and stored_url == definition.mcp_write_url: + return definition.mcp_write_url + return definition.mcp_url or "" + + +def builtin_writes(definition: Optional[ConnectorDefinition], tool_rows: Iterable[dict]) -> Optional[bool]: + """Whether a connection's built-in MCP tool is set up for writes. + + Returns: + None when the connector offers no writes or the connection has no such + tool yet; otherwise whether the tool uses the write endpoint. + """ + if definition is None or not definition.mcp_write_url or builtin_mcp_config(definition) is None: + return None + for tool in tool_rows: + if tool.get("name") == "mcp_tool": + return (_json(tool.get("config")) or {}).get("server_url") == definition.mcp_write_url + return None def needs_mcp_discovery(conn, connection: dict) -> bool: @@ -1303,6 +1343,7 @@ def ensure_connection_tools( connection: dict, permissions: Optional[dict] = None, mcp_actions: Optional[list] = None, + mcp_writes: bool = False, ) -> list[dict]: """Create the connector's tools once; later calls return the existing ones. @@ -1317,6 +1358,8 @@ def ensure_connection_tools( mcp_actions: The actions of a built-in connector's MCP server (GitHub's), discovered by the caller outside this transaction. Without them that tool is not created. + mcp_writes: Point that tool at the endpoint that also offers writes; + the caller checked that an admin allows them. """ repo = ConnectorSessionsRepository(conn) existing = repo.list_tools(str(connection["id"])) @@ -1325,7 +1368,7 @@ def ensure_connection_tools( if not definition or not definition.tool_templates: return existing have = {tool.get("name") for tool in existing} - mcp_config = builtin_mcp_config(definition) + mcp_config = builtin_mcp_config(definition, writes=mcp_writes) created = [] for template in definition.tool_templates: if template in have: @@ -1522,6 +1565,10 @@ class ConnectorDisabled(Exception): """An admin turned this connector (or custom MCP servers) off.""" +class WritesForbidden(Exception): + """An admin forbade write actions through this connector (GitHub's changes).""" + + def custom_mcp_allowed(conn) -> bool: """The instance-wide "Allow custom MCP servers" switch (on unless turned off).""" from docsgpt.storage.db.repositories.app_metadata import AppMetadataRepository @@ -1549,15 +1596,41 @@ def connector_is_enabled(policies: dict, connector_key: Optional[str]) -> bool: def load_policies(conn) -> dict[str, dict]: - """Every connector's policy, with custom MCP folded in from its switch.""" - from docsgpt.storage.db.repositories.connector_policies import ConnectorPoliciesRepository + """Every connector's policy, with custom MCP and write switches folded in. + + A connector that offers writes (GitHub) gets ``allow_writes``: False when + an admin forbade them. + """ + from docsgpt.storage.db.repositories.app_metadata import AppMetadataRepository + from docsgpt.storage.db.repositories.connector_policies import ConnectorPoliciesRepository, allow_writes_key policies = dict(ConnectorPoliciesRepository(conn).all()) if not custom_mcp_allowed(conn): policies["custom_mcp"] = {**policies.get("custom_mcp", {}), "enabled": False} + metadata = AppMetadataRepository(conn) + for definition in catalog.all_definitions(): + if definition.mcp_write_url and metadata.get(allow_writes_key(definition.key)) == "false": + policies[definition.key] = {**policies.get(definition.key, {}), "allow_writes": False} return policies +def writes_allowed(policies: dict, connector_key: Optional[str]) -> bool: + """Whether a connection to this connector may be set up for write actions. + + Args: + policies: ``connector_key`` to policy row, from :func:`load_policies`. + connector_key: The catalog key. + + Returns: + False for a connector that offers no writes, or whose writes an admin + forbade. + """ + definition = catalog.get_definition(connector_key) if connector_key else None + if definition is None or not definition.mcp_write_url: + return False + return (policies.get(connector_key) or {}).get("allow_writes") is not False + + def connector_enabled(conn, row: dict) -> bool: """Whether an admin left on the connector a connection row belongs to. diff --git a/docsgpt/storage/db/repositories/connector_policies.py b/docsgpt/storage/db/repositories/connector_policies.py index f557e95e..3ac628fc 100644 --- a/docsgpt/storage/db/repositories/connector_policies.py +++ b/docsgpt/storage/db/repositories/connector_policies.py @@ -12,6 +12,16 @@ CREDENTIAL_POLICIES = ("choose", "owner", "member") ALLOW_CUSTOM_MCP_KEY = "connectors.allow_custom_mcp" +def allow_writes_key(connector_key: str) -> str: + """``app_metadata`` key of the admin's "let agents make changes" switch for a connector. + + Only connectors whose tools opt into writes (GitHub) have one. It lives in + ``app_metadata`` like the custom MCP switch: ``"false"`` forbids writes, + anything else (or nothing) allows them. + """ + return f"connectors.{connector_key}.allow_writes" + + class ConnectorPoliciesRepository: """Whether a connector is enabled and which credential mode it forces.""" diff --git a/tests/connectors/test_github_writes.py b/tests/connectors/test_github_writes.py new file mode 100644 index 00000000..f7edee52 --- /dev/null +++ b/tests/connectors/test_github_writes.py @@ -0,0 +1,363 @@ +"""GitHub write access: opted into per connection, forbidden by an admin, enforced at run time.""" + +from __future__ import annotations + +import json +from contextlib import contextmanager +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from sqlalchemy import text + +import docsgpt.api.user # noqa: F401 (loads mcp_tool without the circular import) +import docsgpt.agents.tools.mcp_tool # noqa: F401,E402 (patched below) +from docsgpt.connectors import catalog, mcp, service +from docsgpt.security.encryption import encrypt_json + +READONLY_MCP = "https://api.githubcopilot.com/mcp/readonly" +WRITE_MCP = "https://api.githubcopilot.com/mcp/" + + +@pytest.fixture +def app(): + return Flask(__name__) + + +@contextmanager +def _db(conn): + @contextmanager + def _yield(): + yield conn + + with patch.multiple("docsgpt.api.connector.connections", db_session=_yield, db_readonly=_yield), \ + patch.multiple("docsgpt.api.admin.connectors", db_session=_yield, db_readonly=_yield), \ + patch.multiple("docsgpt.connectors.service", db_session=_yield, db_readonly=_yield), \ + patch.multiple("docsgpt.connectors.mcp", db_session=_yield, db_readonly=_yield), \ + patch.multiple("docsgpt.connectors.resolve", db_readonly=_yield): + yield + + +def _call(app, resource, method, path, user="alice", body=None, args=(), roles=None): + with app.test_request_context(path, method=method.upper(), json=body): + from flask import request + + request.decoded_token = {"sub": user, "roles": roles or ["user"]} if user else None + return getattr(resource(), method)(*args) + + +def _connection(conn, user="alice") -> str: + return str(conn.execute( + text( + "INSERT INTO connector_sessions (user_id, provider, connector_key, auth_kind, status, account_label, " + "encrypted_credentials) VALUES (:u, 'github', 'github', 'api_key', 'connected', 'octocat', :e) " + "RETURNING id" + ), + {"u": user, "e": encrypt_json({"credentials": {"access_token": "github_pat_alice"}}, user)}, + ).scalar()) + + +def _tool(conn, cid, server_url, actions) -> str: + return str(conn.execute( + text( + "INSERT INTO user_tools (user_id, name, config, actions, connection_id) VALUES ('alice', 'mcp_tool', " + "CAST(:c AS jsonb), CAST(:a AS jsonb), CAST(:cid AS uuid)) RETURNING id" + ), + {"c": json.dumps({"server_url": server_url, "auth_type": "bearer", "transport_type": "http", "timeout": 30}), + "a": json.dumps(actions), "cid": cid}, + ).scalar()) + + +def _row(conn, cid) -> dict: + return dict(conn.execute( + text("SELECT * FROM connector_sessions WHERE id = CAST(:c AS uuid)"), {"c": cid}, + ).one()._mapping) + + +def _stored(conn, cid) -> tuple[dict, dict]: + config, actions = conn.execute( + text("SELECT config, actions FROM user_tools WHERE connection_id = CAST(:c AS uuid)"), {"c": cid}, + ).one() + return config, {a["name"]: a for a in actions} + + +def _forbid(conn): + from docsgpt.storage.db.repositories.app_metadata import AppMetadataRepository + from docsgpt.storage.db.repositories.connector_policies import allow_writes_key + + AppMetadataRepository(conn).set(allow_writes_key("github"), "false") + + +def _action(name, read_only=None): + action = {"name": name, "description": name, + "parameters": {"type": "object", "properties": {"owner": {"type": "string"}}}} + if read_only is not None: + action["annotations"] = {"readOnlyHint": read_only} + return action + + +# What GitHub's servers list: the read-only endpoint only reads; the full one +# adds writes, annotated ``readOnlyHint: false`` (or not annotated at all). +READ_ACTIONS = [_action("get_issue", True), _action("search_code", True)] +WRITE_ACTIONS = READ_ACTIONS + [ + _action("create_issue", False), + _action("add_issue_comment", False), + _action("mark_all_notifications_read"), +] + + +def _fake_discovery(calls): + """Stands in for GitHub's MCP server: lists the actions of the endpoint asked for.""" + + def discover(user_id, connection, tool): + url = tool["config"]["server_url"] + calls.append(url) + return [dict(a) for a in (WRITE_ACTIONS if url == WRITE_MCP else READ_ACTIONS)] + + return discover + + +class TestCatalog: + def test_github_offers_writes_on_its_full_endpoint(self): + definition = catalog.get_definition("github") + assert definition.mcp_url == READONLY_MCP + assert definition.mcp_write_url == WRITE_MCP + assert definition.to_dict()["writes_opt_in"] is True + assert catalog.get_definition("telegram").to_dict()["writes_opt_in"] is False + + def test_members_see_whether_writes_are_allowed(self, pg_conn): + entries = {e["key"]: e for e in service.catalog_for_user(pg_conn, "alice", is_admin=False)} + assert entries["github"]["writes_allowed"] is True + assert entries["telegram"]["writes_allowed"] is False + _forbid(pg_conn) + entries = {e["key"]: e for e in service.catalog_for_user(pg_conn, "alice", is_admin=False)} + assert entries["github"]["writes_allowed"] is False + + def test_builtin_config_picks_the_endpoint(self): + definition = catalog.get_definition("github") + assert service.builtin_mcp_config(definition)["server_url"] == READONLY_MCP + assert service.builtin_mcp_config(definition, writes=True)["server_url"] == WRITE_MCP + + +class TestClassification: + def test_full_endpoint_actions_are_writes_unless_marked_read_only(self): + """A GitHub action is a read only when GitHub says so: a name like + ``mark_all_notifications_read`` must not pass for one.""" + connection = {"id": "c1", "auth_kind": "api_key", "status": "connected", "connector_key": "github"} + fake = MagicMock() + fake.get_actions_metadata.return_value = [dict(a) for a in WRITE_ACTIONS] + with patch.object(service, "access_credentials", return_value={"access_token": "t"}), \ + patch("docsgpt.agents.tools.mcp_tool.MCPTool", return_value=fake): + actions = mcp.discover_builtin_actions("alice", connection, writes=True) + access = {a["name"]: a["access"] for a in actions} + assert access == { + "get_issue": "read", "search_code": "read", + "create_issue": "write", "add_issue_comment": "write", "mark_all_notifications_read": "write", + } + + +class TestSetup: + def test_opting_in_uses_the_full_endpoint_and_writes_ask_first(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionDetail, ConnectionSetup + + cid = _connection(pg_conn) + calls = [] + with _db(pg_conn), patch.object(mcp, "_discover", side_effect=_fake_discovery(calls)): + resp = _call(app, ConnectionSetup, "post", f"/api/connections/{cid}/setup", + body={"create_tools": True, "allow_writes": True}, args=[cid]) + detail = _call(app, ConnectionDetail, "get", f"/api/connections/{cid}", args=[cid]).get_json() + assert resp.status_code == 200 + assert calls == [WRITE_MCP] + config, actions = _stored(pg_conn, cid) + assert config["server_url"] == WRITE_MCP + assert actions["create_issue"]["access"] == "write" and actions["create_issue"]["require_approval"] is True + assert actions["mark_all_notifications_read"]["access"] == "write" + assert actions["get_issue"]["access"] == "read" and not actions["get_issue"].get("require_approval") + assert detail["connection"]["writes"] is True + permissions = {a["name"]: a["permission"] for a in detail["connection"]["tools"][0]["actions"]} + assert permissions["create_issue"] == "ask" and permissions["get_issue"] == "always" + + def test_read_only_is_the_default(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionDetail, ConnectionSetup + + cid = _connection(pg_conn) + calls = [] + with _db(pg_conn), patch.object(mcp, "_discover", side_effect=_fake_discovery(calls)): + _call(app, ConnectionSetup, "post", f"/api/connections/{cid}/setup", + body={"create_tools": True}, args=[cid]) + detail = _call(app, ConnectionDetail, "get", f"/api/connections/{cid}", args=[cid]).get_json() + assert calls == [READONLY_MCP] + assert _stored(pg_conn, cid)[0]["server_url"] == READONLY_MCP + assert detail["connection"]["writes"] is False + + def test_forbidden_writes_are_refused(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionSetup + + cid = _connection(pg_conn) + _forbid(pg_conn) + with _db(pg_conn), patch.object(mcp, "_discover") as discover: + resp = _call(app, ConnectionSetup, "post", f"/api/connections/{cid}/setup", + body={"create_tools": True, "allow_writes": True}, args=[cid]) + assert resp.status_code == 403 + assert resp.get_json()["code"] == "writes_forbidden" + discover.assert_not_called() + assert pg_conn.execute(text("SELECT count(*) FROM user_tools")).scalar() == 0 + + +class TestSwitchWrites: + def _read_only_tool(self, conn, cid): + actions = mcp.apply_default_permissions("mcp_tool", service._transform_actions( + [dict(a) for a in READ_ACTIONS])) + # The user turned one read off; that choice survives switching. + actions = [{**a, "active": a["name"] != "search_code"} for a in actions] + return _tool(conn, cid, READONLY_MCP, actions) + + def test_turning_writes_on_and_off_keeps_choices(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionWrites + + cid = _connection(pg_conn) + self._read_only_tool(pg_conn, cid) + calls = [] + with _db(pg_conn), patch.object(mcp, "_discover", side_effect=_fake_discovery(calls)): + on = _call(app, ConnectionWrites, "put", f"/api/connections/{cid}/writes", + body={"allow": True}, args=[cid]) + config, actions = _stored(pg_conn, cid) + assert on.status_code == 200 + assert on.get_json()["writes"] is True + assert sorted(on.get_json()["added"]) == ["add_issue_comment", "create_issue", + "mark_all_notifications_read"] + assert config["server_url"] == WRITE_MCP + assert actions["search_code"]["active"] is False + assert actions["create_issue"]["require_approval"] is True + off = _call(app, ConnectionWrites, "put", f"/api/connections/{cid}/writes", + body={"allow": False}, args=[cid]) + assert calls == [WRITE_MCP, READONLY_MCP] + config, actions = _stored(pg_conn, cid) + assert off.get_json()["writes"] is False + assert config["server_url"] == READONLY_MCP + assert set(actions) == {"get_issue", "search_code"} + assert actions["search_code"]["active"] is False + + def test_forbidden_writes_cannot_be_turned_on_but_can_be_turned_off(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionWrites + + cid = _connection(pg_conn) + _tool(pg_conn, cid, WRITE_MCP, [_action("create_issue", False)]) + _forbid(pg_conn) + calls = [] + with _db(pg_conn), patch.object(mcp, "_discover", side_effect=_fake_discovery(calls)): + on = _call(app, ConnectionWrites, "put", "/x", body={"allow": True}, args=[cid]) + off = _call(app, ConnectionWrites, "put", "/x", body={"allow": False}, args=[cid]) + assert on.status_code == 403 and on.get_json()["code"] == "writes_forbidden" + assert off.status_code == 200 + assert _stored(pg_conn, cid)[0]["server_url"] == READONLY_MCP + + def test_owner_only_github_only_and_needs_the_tool(self, app, pg_conn): + from docsgpt.api.connector.connections import ConnectionWrites + + cid = _connection(pg_conn) + with _db(pg_conn): + bob = _call(app, ConnectionWrites, "put", "/x", user="bob", body={"allow": True}, args=[cid]) + no_tool = _call(app, ConnectionWrites, "put", "/x", body={"allow": True}, args=[cid]) + bad = _call(app, ConnectionWrites, "put", "/x", body={"allow": "yes"}, args=[cid]) + assert bob.status_code == 404 + assert no_tool.status_code == 409 and no_tool.get_json()["code"] == "no_tools" + assert bad.status_code == 400 + telegram = str(pg_conn.execute(text( + "INSERT INTO connector_sessions (user_id, provider, connector_key, auth_kind, status, account_label) " + "VALUES ('alice', 'telegram', 'telegram', 'api_key', 'connected', 'bot') RETURNING id" + )).scalar()) + with _db(pg_conn): + other = _call(app, ConnectionWrites, "put", "/x", body={"allow": True}, args=[telegram]) + assert other.status_code == 400 + + def test_refresh_after_an_admin_forbids_writes_goes_read_only(self, pg_conn): + cid = _connection(pg_conn) + _tool(pg_conn, cid, WRITE_MCP, [_action("create_issue", False)]) + _forbid(pg_conn) + calls = [] + with _db(pg_conn), patch.object(mcp, "_discover", side_effect=_fake_discovery(calls)): + result = mcp.refresh_mcp_tools("alice", _row(pg_conn, cid)) + assert calls == [READONLY_MCP] + assert result["removed"] == ["create_issue"] + assert _stored(pg_conn, cid)[0]["server_url"] == READONLY_MCP + + +class TestAdmin: + def test_admin_can_forbid_github_writes(self, app, pg_conn): + from docsgpt.api.admin.connectors import AdminConnectorsResource + + with _db(pg_conn): + before = _call(app, AdminConnectorsResource, "get", "/x", roles=["admin"]).get_json() + resp = _call(app, AdminConnectorsResource, "put", "/x", roles=["admin"], + body={"policies": {"github": {"allow_writes": False}}}) + rows = {c["key"]: c for c in before["connectors"]} + assert rows["github"]["allow_writes"] is True + assert rows["telegram"]["allow_writes"] is None + assert resp.status_code == 200 + assert {c["key"]: c for c in resp.get_json()["connectors"]}["github"]["allow_writes"] is False + assert service.writes_allowed(service.load_policies(pg_conn), "github") is False + + def test_rejects_writes_policy_where_it_means_nothing(self, app, pg_conn): + from docsgpt.api.admin.connectors import AdminConnectorsResource + + with _db(pg_conn): + other = _call(app, AdminConnectorsResource, "put", "/x", roles=["admin"], + body={"policies": {"telegram": {"allow_writes": False}}}) + not_bool = _call(app, AdminConnectorsResource, "put", "/x", roles=["admin"], + body={"policies": {"github": {"allow_writes": "no"}}}) + assert other.status_code == 400 + assert not_bool.status_code == 400 + + +def _tool_data(cid, server_url=WRITE_MCP): + return { + "id": "tool-gh", "user_id": "alice", "name": "mcp_tool", "connection_id": cid, + "config": {"server_url": server_url, "auth_type": "bearer"}, "credential_mode": "owner", + "actions": [ + {"name": "create_issue", "access": "write", "active": True, "require_approval": False}, + {"name": "get_issue", "access": "read", "active": True}, + ], + } + + +class TestRuntime: + def _loaded_url(self, pg_conn, tool): + from docsgpt.agents.tool_executor import ToolExecutor + + with _db(pg_conn), patch("docsgpt.agents.tool_executor.ToolManager") as manager: + ToolExecutor(user="alice")._get_or_load_tool(tool, "t1", "create_issue") + return manager.return_value.load_tool.call_args.kwargs["tool_config"]["server_url"] + + def test_write_endpoint_is_used_while_allowed(self, pg_conn): + assert self._loaded_url(pg_conn, _tool_data(_connection(pg_conn))) == WRITE_MCP + + def test_forbidden_writes_fall_back_to_the_read_only_endpoint(self, pg_conn): + cid = _connection(pg_conn) + _forbid(pg_conn) + assert self._loaded_url(pg_conn, _tool_data(cid)) == READONLY_MCP + + def test_any_other_path_on_githubs_host_is_read_only(self, pg_conn): + cid = _connection(pg_conn) + assert self._loaded_url(pg_conn, _tool_data(cid, "https://api.githubcopilot.com/mcp/x/all")) == READONLY_MCP + + def _pause(self, pg_conn, action_name): + from docsgpt.agents.tool_executor import ToolExecutor + + cid = _connection(pg_conn) + _forbid(pg_conn) + with _db(pg_conn), patch("docsgpt.agents.tool_executor.ToolActionParser") as parser: + parser.return_value.parse_args.return_value = ("t1", action_name, {}) + return ToolExecutor(user="alice").check_pause( + {"t1": _tool_data(cid)}, SimpleNamespace(id="c1", name=action_name, thought_signature=None), + "OpenAILLM", + ) + + def test_forbidden_write_is_denied_with_a_reason(self, pg_conn): + pause = self._pause(pg_conn, "create_issue") + assert pause["pause_type"] == "headless_denied" + assert "admin" in pause["deny_reason"] + + def test_reads_still_run_when_writes_are_forbidden(self, pg_conn): + assert self._pause(pg_conn, "get_issue") is None