diff --git a/backend/app/api/tools.py b/backend/app/api/tools.py index 0471c443c..8afaf141e 100644 --- a/backend/app/api/tools.py +++ b/backend/app/api/tools.py @@ -131,12 +131,44 @@ async def _load_assigned_smithery_connection( def _resolve_target_tenant_id(current_user: User, tenant_id: str | None = None) -> uuid.UUID | None: + """Resolve a requested tenant and reject cross-tenant access by non-platform admins.""" if tenant_id: try: - return uuid.UUID(tenant_id) + target_tenant_id = uuid.UUID(tenant_id) except ValueError: raise HTTPException(status_code=400, detail="Invalid tenant_id format") - return current_user.tenant_id + else: + target_tenant_id = current_user.tenant_id + + if target_tenant_id != current_user.tenant_id and current_user.role != "platform_admin": + raise HTTPException(status_code=403, detail="No access to this tenant") + return target_tenant_id + + +def _require_tool_manager(current_user: User) -> None: + """Restrict tenant tool administration to organization and platform administrators.""" + if current_user.role not in ("platform_admin", "org_admin"): + raise HTTPException(status_code=403, detail="Tool management permission required") + + +async def _require_agent_tool_manager( + db: AsyncSession, + current_user: User, + agent_id: uuid.UUID, +): + """Load an agent only when the caller may manage its configuration.""" + agent = await _load_agent_for_tool_scope(db, agent_id) + if current_user.role == "platform_admin": + return agent + if not await can_manage_agent(db, current_user, agent): + raise HTTPException(status_code=403, detail="Agent manage permission required") + return agent + + +def _require_tool_record_access(current_user: User, tool: Tool) -> None: + """Ensure a tenant administrator cannot mutate another tenant's tool record.""" + if tool.tenant_id is not None: + _resolve_target_tenant_id(current_user, str(tool.tenant_id)) def _get_sensitive_keys(config_schema: dict | None = None) -> set[str]: @@ -199,6 +231,7 @@ async def list_tools( db: AsyncSession = Depends(get_db), ): """List platform tools scoped by tenant (builtin + tenant-specific).""" + _require_tool_manager(current_user) query = ( select(Tool) .where(Tool.source.in_(["builtin", "admin"])) @@ -229,7 +262,7 @@ async def list_tools( "enabled": t.enabled, "is_default": t.is_default, "source": t.source, - "config": company_config, + "config": mask_sensitive_fields(company_config, t.config_schema), "config_schema": t.config_schema or {}, "created_at": t.created_at.isoformat() if t.created_at else None, }) @@ -248,6 +281,7 @@ async def create_tool( own tenant but can be overridden via data.tenant_id. This allows platform admins to import MCP tools while viewing another company's settings page. """ + _require_tool_manager(current_user) # Resolve target tenant: explicit payload value takes priority so that # platform admins importing tools for another company work correctly. target_tenant_id = _resolve_target_tenant_id(current_user, data.tenant_id) @@ -295,12 +329,16 @@ async def update_tools_bulk( db: AsyncSession = Depends(get_db), ): """Bulk update the enabled status of multiple tools.""" + _require_tool_manager(current_user) tool_ids = [uuid.UUID(u.tool_id) for u in updates] result = await db.execute(select(Tool).where(Tool.id.in_(tool_ids))) tools_map = {str(t.id): t for t in result.scalars().all()} for update in updates: if update.tool_id in tools_map: + _require_tool_record_access(current_user, tools_map[update.tool_id]) + if tools_map[update.tool_id].source == "builtin" and current_user.role != "platform_admin": + raise HTTPException(status_code=403, detail="Platform admin permission required") tools_map[update.tool_id].enabled = update.enabled await db.commit() @@ -315,10 +353,12 @@ async def update_tool( db: AsyncSession = Depends(get_db), ): """Update a tool.""" + _require_tool_manager(current_user) result = await db.execute(select(Tool).where(Tool.id == tool_id)) tool = result.scalar_one_or_none() if not tool: raise HTTPException(status_code=404, detail="Tool not found") + _require_tool_record_access(current_user, tool) update_data = data.model_dump(exclude_unset=True) target_tenant_id = _resolve_target_tenant_id(current_user, update_data.pop("tenant_id", None)) @@ -332,6 +372,9 @@ async def update_tool( else: update_data["config"] = _encrypt_sensitive_fields(config_value, tool.config_schema) + if tool.source == "builtin" and update_data and current_user.role != "platform_admin": + raise HTTPException(status_code=403, detail="Platform admin permission required") + for field, value in update_data.items(): setattr(tool, field, value) await db.commit() @@ -345,10 +388,12 @@ async def delete_tool( db: AsyncSession = Depends(get_db), ): """Delete a tool (only non-builtin).""" + _require_tool_manager(current_user) result = await db.execute(select(Tool).where(Tool.id == tool_id)) tool = result.scalar_one_or_none() if not tool: raise HTTPException(status_code=404, detail="Tool not found") + _require_tool_record_access(current_user, tool) if tool.type == "builtin": raise HTTPException(status_code=400, detail="Cannot delete builtin tools") @@ -366,12 +411,11 @@ async def get_agent_tools( db: AsyncSession = Depends(get_db), ): """Get tools for a specific agent with their enabled status.""" - from app.services.agent_tools import _agent_has_feishu - has_feishu = await _agent_has_feishu(agent_id) - # Determine if this is a system agent (e.g. OKR Agent). # System agents can see all tools; regular agents cannot see okr_agent_only tools. - agent_obj = await _load_agent_for_tool_scope(db, agent_id) + agent_obj = await _require_agent_tool_manager(db, current_user, agent_id) + from app.services.agent_tools import _agent_has_feishu + has_feishu = await _agent_has_feishu(agent_id) is_system_agent = bool(agent_obj and agent_obj.is_system) # Agent-specific assignments @@ -456,7 +500,7 @@ async def update_agent_tools( db: AsyncSession = Depends(get_db), ): """Update tool assignments for an agent.""" - agent_obj = await _load_agent_for_tool_scope(db, agent_id) + agent_obj = await _require_agent_tool_manager(db, current_user, agent_id) assignments = await _load_agent_tool_assignments(db, agent_id) for u in updates: tool_id = uuid.UUID(u.tool_id) @@ -504,12 +548,7 @@ async def get_mcp_authorization_status( no_store_headers = {"Cache-Control": "no-store"} try: - agent = await _load_agent_for_tool_scope(db, agent_id) - if not await can_manage_agent(db, current_user, agent): - raise HTTPException( - status_code=403, - detail="Agent manage permission required", - ) + await _require_agent_tool_manager(db, current_user, agent_id) connection = await _load_assigned_smithery_connection( db, @@ -589,6 +628,7 @@ async def test_mcp_connection( - URL-embedded key (e.g. ?tavilyApiKey=xxx) — include in server_url. - Bearer token — pass via api_key field; sent as Authorization header. """ + _require_tool_manager(current_user) from app.services.mcp_client import MCPClient try: @@ -626,15 +666,8 @@ async def update_mcp_server( 2. URL query param (e.g. ?tavilyApiKey=xxx) — extracted from the URL and converted to Bearer by MCPClient automatically. """ - # Resolve target tenant - target_tenant_id: uuid.UUID | None = None - if data.tenant_id: - try: - target_tenant_id = uuid.UUID(data.tenant_id) - except ValueError: - raise HTTPException(status_code=400, detail="Invalid tenant_id format") - else: - target_tenant_id = current_user.tenant_id + _require_tool_manager(current_user) + target_tenant_id = _resolve_target_tenant_id(current_user, data.tenant_id) # Load all tools from this server under the target tenant result = await db.execute( @@ -674,6 +707,7 @@ async def list_agent_installed_tools( db: AsyncSession = Depends(get_db), ): """Admin endpoint: list user-installed tools scoped by tenant.""" + _require_tool_manager(current_user) from app.models.agent import Agent query = ( select(AgentTool, Tool, Agent) @@ -683,7 +717,8 @@ async def list_agent_installed_tools( .order_by(AgentTool.created_at.desc()) ) # Scope by tenant: only show tools installed by agents in this tenant - tid = tenant_id or (str(current_user.tenant_id) if current_user.tenant_id else None) + target_tenant_id = _resolve_target_tenant_id(current_user, tenant_id) + tid = str(target_tenant_id) if target_tenant_id else None if tid: from app.models.agent import Agent as Ag # Some local/prod databases still have agents.tenant_id as varchar from @@ -724,10 +759,12 @@ async def delete_agent_tool( db: AsyncSession = Depends(get_db), ): """Admin: remove an agent-tool assignment. Also deletes the tool record if no other agents use it.""" + _require_tool_manager(current_user) at_r = await db.execute(select(AgentTool).where(AgentTool.id == agent_tool_id)) at = at_r.scalar_one_or_none() if not at: raise HTTPException(status_code=404, detail="Agent tool assignment not found") + await _require_agent_tool_manager(db, current_user, at.agent_id) tool_id = at.tool_id await db.delete(at) await db.flush() @@ -760,11 +797,13 @@ async def get_agent_tool_config( Both configs are decrypted before returning. Global sensitive fields are masked so the frontend can show a key is configured without exposing it. """ + agent = await _require_agent_tool_manager(db, current_user, agent_id) tool_r = await db.execute(select(Tool).where(Tool.id == tool_id)) tool = tool_r.scalar_one_or_none() - if not tool: + if not tool or not _tool_record_visible_to_agent( + tool, agent.tenant_id, await _load_agent_tool_assignments(db, agent_id) + ): raise HTTPException(status_code=404, detail="Tool not found") - agent = await _load_agent_for_tool_scope(db, agent_id) at_r = await db.execute( select(AgentTool).where(AgentTool.agent_id == agent_id, AgentTool.tool_id == tool_id) ) @@ -781,7 +820,7 @@ async def get_agent_tool_config( # Merged: agent overrides take precedence over global defaults. # Use raw (non-masked) global as the base so the agent inherits actual values # at runtime, but the UI will show masked_global for display hints. - merged = {**raw_global, **(raw_agent or {})} + merged = {**masked_global, **(raw_agent or {})} return { "global_config": masked_global, "agent_config": raw_agent or {}, @@ -799,6 +838,7 @@ async def update_agent_tool_config( db: AsyncSession = Depends(get_db), ): """Save per-agent config override for a tool.""" + agent = await _require_agent_tool_manager(db, current_user, agent_id) # Check permission: only platform_admin and org_admin can modify allow_network if "allow_network" in data.config: if current_user.role not in ("platform_admin", "org_admin"): @@ -810,6 +850,10 @@ async def update_agent_tool_config( # Encrypt sensitive fields using the tool's config_schema for field type awareness tool_r2 = await db.execute(select(Tool).where(Tool.id == tool_id)) tool_for_schema = tool_r2.scalar_one_or_none() + if not tool_for_schema or not _tool_record_visible_to_agent( + tool_for_schema, agent.tenant_id, await _load_agent_tool_assignments(db, agent_id) + ): + raise HTTPException(status_code=404, detail="Tool not found") encrypted_config = _encrypt_sensitive_fields(data.config, tool_for_schema.config_schema if tool_for_schema else None) at_r = await db.execute( @@ -841,11 +885,10 @@ async def get_agent_tools_with_config( rather than Tool.config. We resolve those as part of the global config so the agent-level UI can show the inherited key hint. """ + # Determine if this is a system agent (e.g. OKR Agent). + agent_obj2 = await _require_agent_tool_manager(db, current_user, agent_id) from app.services.agent_tools import _agent_has_feishu has_feishu = await _agent_has_feishu(agent_id) - - # Determine if this is a system agent (e.g. OKR Agent). - agent_obj2 = await _load_agent_for_tool_scope(db, agent_id) is_system_agent2 = bool(agent_obj2 and agent_obj2.is_system) assignments = await _load_agent_tool_assignments(db, agent_id) @@ -982,10 +1025,9 @@ async def get_category_config( Sensitive fields in global_config are masked for display. Company-level values always take precedence at runtime. """ - from app.core.permissions import check_agent_access from app.models.channel_config import ChannelConfig - agent, _ = await check_agent_access(db, current_user, agent_id) + agent = await _require_agent_tool_manager(db, current_user, agent_id) # ── 1. Load company-level (global) config from Tool.config ────────────── # Find a tool in this category that actually has config data. @@ -1036,7 +1078,7 @@ async def get_category_config( # ── 3. Build effective config ─────────────────────────────────────────── # Priority: Agent config > Company config > Default # Agent can override company values by setting their own. - effective_config = {**raw_global, **raw_agent} + effective_config = {**masked_global, **raw_agent} return { "id": config_id, @@ -1060,10 +1102,10 @@ async def update_category_config( db: AsyncSession = Depends(get_db), ): """Update or create shared configuration for a tool category.""" - from app.core.permissions import check_agent_access, is_agent_creator + from app.core.permissions import is_agent_creator from app.models.channel_config import ChannelConfig - agent, _ = await check_agent_access(db, current_user, agent_id) + agent = await _require_agent_tool_manager(db, current_user, agent_id) if not is_agent_creator(current_user, agent): raise HTTPException(status_code=403, detail="Only creator can configure category") @@ -1117,10 +1159,10 @@ async def delete_category_config( db: AsyncSession = Depends(get_db), ): """Remove shared configuration for a tool category.""" - from app.core.permissions import check_agent_access, is_agent_creator + from app.core.permissions import is_agent_creator from app.models.channel_config import ChannelConfig - agent, _ = await check_agent_access(db, current_user, agent_id) + agent = await _require_agent_tool_manager(db, current_user, agent_id) if not is_agent_creator(current_user, agent): raise HTTPException(status_code=403, detail="Only creator can remove config") @@ -1141,6 +1183,7 @@ async def test_category_config( db: AsyncSession = Depends(get_db), ): """Test connectivity for a tool category.""" + await _require_agent_tool_manager(db, current_user, agent_id) if category == "atlassian": from app.api.atlassian import test_atlassian_channel return await test_atlassian_channel(agent_id, current_user, db) diff --git a/backend/tests/test_tool_tenant_scope.py b/backend/tests/test_tool_tenant_scope.py index 86fe77e52..c4f8f6881 100644 --- a/backend/tests/test_tool_tenant_scope.py +++ b/backend/tests/test_tool_tenant_scope.py @@ -1,7 +1,15 @@ import uuid from types import SimpleNamespace -from app.api.tools import _tool_record_visible_to_agent +import pytest +from fastapi import HTTPException + +from app.api.tools import ( + _require_tool_manager, + _require_tool_record_access, + _resolve_target_tenant_id, + _tool_record_visible_to_agent, +) def make_tool(**overrides): @@ -39,3 +47,39 @@ def test_agent_installed_tools_require_explicit_assignment(): assert _tool_record_visible_to_agent(installed_tool, tenant_id, {}) is False assert _tool_record_visible_to_agent(installed_tool, tenant_id, {str(tool_id): object()}) is True + +def make_user(tenant_id: uuid.UUID, role: str = "user"): + return SimpleNamespace(tenant_id=tenant_id, role=role) + + +def test_regular_users_cannot_access_tool_management(): + with pytest.raises(HTTPException, match="Tool management permission required") as error: + _require_tool_manager(make_user(uuid.uuid4())) + + assert error.value.status_code == 403 + + +def test_org_admin_cannot_select_another_tenant_for_tools(): + user = make_user(uuid.uuid4(), role="org_admin") + + with pytest.raises(HTTPException, match="No access to this tenant") as error: + _resolve_target_tenant_id(user, str(uuid.uuid4())) + + assert error.value.status_code == 403 + + +def test_platform_admin_can_select_another_tenant_for_tools(): + target_tenant_id = uuid.uuid4() + user = make_user(uuid.uuid4(), role="platform_admin") + + assert _resolve_target_tenant_id(user, str(target_tenant_id)) == target_tenant_id + + +def test_org_admin_cannot_mutate_a_foreign_tenant_tool(): + user = make_user(uuid.uuid4(), role="org_admin") + foreign_tool = make_tool(tenant_id=uuid.uuid4()) + + with pytest.raises(HTTPException, match="No access to this tenant") as error: + _require_tool_record_access(user, foreign_tool) + + assert error.value.status_code == 403