mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-13 08:15:43 +00:00
Validate tool action updates and keep fixed values owner-only
update_tool_actions now checks the submitted actions against the tool's stored ones: no new actions or parameters, fixed values must fit their parameter's type. A team editor can still switch actions on and off but gets 403 for changing a fixed value.
This commit is contained in:
1 parent
34038ae1c2
commit
f6bdd853d5
2 files changed
+125
-16
No files matched your search
@@ -1,5 +1,7 @@
|
||||
"""Tool management routes."""
|
||||
|
||||
import copy
|
||||
|
||||
from flask import current_app, jsonify, make_response, request
|
||||
from flask_restx import fields, Namespace, Resource
|
||||
|
||||
@@ -13,6 +15,7 @@ from docsgpt.agents.default_tools import (
|
||||
is_synthesized_tool_id,
|
||||
WORKFLOW_ONLY_BUILTINS,
|
||||
)
|
||||
from docsgpt.agents.tool_pins import merge_submitted_actions, PinChangeRefused
|
||||
from docsgpt.agents.tools.spec_parser import parse_spec
|
||||
from docsgpt.agents.tools.tool_manager import ToolManager
|
||||
from docsgpt.api import api
|
||||
@@ -185,6 +188,24 @@ def transform_actions(actions_metadata):
|
||||
return transformed
|
||||
|
||||
|
||||
def _stored_actions(tool_doc: dict) -> list:
|
||||
"""A tool's stored actions, or its class's own for a row stored without any.
|
||||
|
||||
Args:
|
||||
tool_doc: The ``user_tools`` row.
|
||||
|
||||
Returns:
|
||||
The action list submitted actions are validated against.
|
||||
"""
|
||||
actions = tool_doc.get("actions") or []
|
||||
if actions or tool_doc.get("name") in ("mcp_tool", "api_tool"):
|
||||
return actions
|
||||
tool_instance = tool_manager.tools.get(tool_doc.get("name"))
|
||||
if tool_instance is None:
|
||||
return actions
|
||||
return transform_actions(copy.deepcopy(tool_instance.get_actions_metadata()))
|
||||
|
||||
|
||||
tools_ns = Namespace("tools", description="Tool management operations", path="/api")
|
||||
|
||||
# Tools the Connectors page adds through "Add custom connector" rather than
|
||||
@@ -814,18 +835,29 @@ class UpdateToolActions(Resource):
|
||||
with db_session() as conn:
|
||||
repo = UserToolsRepository(conn)
|
||||
tool_doc = repo.get_any(data["id"], user)
|
||||
if tool_doc:
|
||||
repo.update(str(tool_doc["id"]), user, {"actions": data["actions"]})
|
||||
else:
|
||||
owner = user
|
||||
if tool_doc is None:
|
||||
# Team editor write path (secrets stay owner-only — actions
|
||||
# carry no credentials, so editing them is safe).
|
||||
owner = effective_write_owner(conn, "tool", data["id"], user)
|
||||
if not owner:
|
||||
tool_doc = repo.get_by_id(data["id"]) if owner else None
|
||||
if not tool_doc or tool_doc.get("user_id") != owner:
|
||||
return make_response(
|
||||
jsonify({"success": False, "message": "Tool not found"}),
|
||||
404,
|
||||
)
|
||||
repo.update(data["id"], owner, {"actions": data["actions"]})
|
||||
# The stored actions are the schema: nothing can be added,
|
||||
# and a fixed value is the owner's to set (an editor pinning
|
||||
# a Telegram chat would redirect the owner's bot).
|
||||
try:
|
||||
actions = merge_submitted_actions(
|
||||
_stored_actions(tool_doc), data["actions"], may_change_pins=owner == user,
|
||||
)
|
||||
except PinChangeRefused as err:
|
||||
return make_response(jsonify({"success": False, "message": str(err)}), 403)
|
||||
except ValueError as err:
|
||||
return make_response(jsonify({"success": False, "message": str(err)}), 400)
|
||||
repo.update(str(tool_doc["id"]), owner, {"actions": actions})
|
||||
except Exception as err:
|
||||
current_app.logger.error(
|
||||
f"Error updating tool actions: {err}", exc_info=True
|
||||
|
||||
@@ -1306,26 +1306,103 @@ class TestUpdateToolActionsHappy:
|
||||
response = UpdateToolActions().post()
|
||||
assert response.status_code == 404
|
||||
|
||||
def test_updates_actions(self, app, pg_conn):
|
||||
from docsgpt.api.user.tools.routes import UpdateToolActions
|
||||
@staticmethod
|
||||
def _seed_actions_tool(pg_conn, user):
|
||||
from docsgpt.storage.db.repositories.user_tools import UserToolsRepository
|
||||
|
||||
user = "u-actions"
|
||||
tool = _seed_tool(pg_conn, user=user)
|
||||
return UserToolsRepository(pg_conn).create(
|
||||
user,
|
||||
"telegram",
|
||||
config={},
|
||||
display_name="Telegram",
|
||||
description="",
|
||||
actions=[
|
||||
{
|
||||
"name": "telegram_send_message",
|
||||
"description": "Send a message",
|
||||
"active": True,
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"text": {"type": "string", "filled_by_llm": True, "value": ""},
|
||||
"chat_id": {"type": "string", "filled_by_llm": True, "value": ""},
|
||||
},
|
||||
},
|
||||
}
|
||||
],
|
||||
status=True,
|
||||
)
|
||||
|
||||
def _post(self, app, pg_conn, user, tool_id, actions):
|
||||
from docsgpt.api.user.tools.routes import UpdateToolActions
|
||||
|
||||
with _patch_tools_db(pg_conn), app.test_request_context(
|
||||
"/api/update_tool_actions",
|
||||
method="POST",
|
||||
json={
|
||||
"id": str(tool["id"]),
|
||||
"actions": [
|
||||
{"name": "action_1", "active": True, "parameters": {}}
|
||||
],
|
||||
},
|
||||
json={"id": tool_id, "actions": actions},
|
||||
):
|
||||
from flask import request
|
||||
request.decoded_token = {"sub": user}
|
||||
response = UpdateToolActions().post()
|
||||
return UpdateToolActions().post()
|
||||
|
||||
@staticmethod
|
||||
def _stored(pg_conn, tool):
|
||||
from docsgpt.storage.db.repositories.user_tools import UserToolsRepository
|
||||
|
||||
return UserToolsRepository(pg_conn).get_by_id(str(tool["id"]))["actions"][0]
|
||||
|
||||
def test_updates_actions(self, app, pg_conn):
|
||||
user = "u-actions"
|
||||
tool = self._seed_actions_tool(pg_conn, user)
|
||||
response = self._post(
|
||||
app, pg_conn, user, str(tool["id"]), [{"name": "telegram_send_message", "active": False}],
|
||||
)
|
||||
assert response.status_code == 200
|
||||
assert self._stored(pg_conn, tool)["active"] is False
|
||||
|
||||
def test_owner_pins_a_parameter(self, app, pg_conn):
|
||||
user = "u-actions-pin"
|
||||
tool = self._seed_actions_tool(pg_conn, user)
|
||||
response = self._post(app, pg_conn, user, str(tool["id"]), [{
|
||||
"name": "telegram_send_message",
|
||||
"parameters": {"properties": {"chat_id": {"filled_by_llm": False, "value": "123"}}},
|
||||
}])
|
||||
assert response.status_code == 200
|
||||
chat_id = self._stored(pg_conn, tool)["parameters"]["properties"]["chat_id"]
|
||||
assert chat_id["filled_by_llm"] is False and chat_id["value"] == "123"
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"actions",
|
||||
[
|
||||
[{"name": "made_up_action", "active": True}],
|
||||
[{"name": "telegram_send_message", "parameters": {"properties": {"token": {"value": "x"}}}}],
|
||||
[{"active": True}],
|
||||
"not a list",
|
||||
],
|
||||
)
|
||||
def test_rejects_actions_that_do_not_match_the_schema(self, app, pg_conn, actions):
|
||||
user = "u-actions-bad"
|
||||
tool = self._seed_actions_tool(pg_conn, user)
|
||||
response = self._post(app, pg_conn, user, str(tool["id"]), actions)
|
||||
assert response.status_code == 400
|
||||
assert self._stored(pg_conn, tool)["active"] is True
|
||||
|
||||
def test_team_editor_can_change_actions_but_not_pins(self, app, pg_conn):
|
||||
owner, editor = "u-owner", "u-editor"
|
||||
tool = self._seed_actions_tool(pg_conn, owner)
|
||||
with patch("docsgpt.api.user.tools.routes.effective_write_owner", return_value=owner):
|
||||
toggled = self._post(
|
||||
app, pg_conn, editor, str(tool["id"]), [{"name": "telegram_send_message", "active": False}],
|
||||
)
|
||||
pinned = self._post(app, pg_conn, editor, str(tool["id"]), [{
|
||||
"name": "telegram_send_message",
|
||||
"parameters": {"properties": {"chat_id": {"filled_by_llm": False, "value": "666"}}},
|
||||
}])
|
||||
assert toggled.status_code == 200
|
||||
assert pinned.status_code == 403
|
||||
stored = self._stored(pg_conn, tool)
|
||||
assert stored["active"] is False
|
||||
assert stored["parameters"]["properties"]["chat_id"]["filled_by_llm"] is True
|
||||
|
||||
|
||||
class TestUpdateToolStatusHappy:
|
||||
|
||||
Reference in new issue
Block a user