codeant-ai-for-open-source[bot] commented on code in PR #41472: URL: https://github.com/apache/superset/pull/41472#discussion_r3556001103
########## superset/mcp_service/dashboard/tool/delete_dashboard.py: ########## @@ -0,0 +1,217 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP tool: delete_dashboard +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import is_feature_enabled +from superset.commands.dashboard.exceptions import ( + DashboardDeleteFailedReportsExistError, + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeleteDashboardRequest, + DeleteDashboardResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_dashboard_by_identifier(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID, UUID string, or slug. Returns None.""" + from superset.daos.dashboard import DashboardDAO + + if isinstance(identifier, int) or ( + isinstance(identifier, str) and identifier.isdigit() + ): + return DashboardDAO.find_by_id(int(identifier)) + # Try UUID, then fall back to slug. + dashboard = DashboardDAO.find_by_id(identifier, id_column="uuid") + if dashboard: + return dashboard + try: + return DashboardDAO.get_by_id_or_slug(identifier) + except DashboardNotFoundError: + return None Review Comment: **Suggestion:** The lookup helper assumes `DashboardDAO.get_by_id_or_slug()` only raises not-found, but for UUID identifiers it can also raise access-denied. Because that exception is not caught here (or at the call site), the tool can fail with an unhandled error instead of returning a structured `DeleteDashboardResponse`. Catch access-denied from this DAO call and map it to a normal tool response (for example not-found or permission-denied, depending on the intended disclosure model). [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ❌ delete_dashboard MCP tool can crash on access-denied. ⚠️ Agents see generic tool failure, not permission response. ⚠️ Behavior inconsistent with duplicate_dashboard access-denied handling. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Invoke the MCP tool function `delete_dashboard` in `superset/mcp_service/dashboard/tool/delete_dashboard.py:59-81` with a `DeleteDashboardRequest` whose `identifier` is a slug or UUID of a dashboard that exists but the current user cannot access (no view permission). 2. Inside `delete_dashboard`, observe it calls `_find_dashboard_by_identifier(request.identifier)` at `superset/mcp_service/dashboard/tool/delete_dashboard.py:84-86`, which for non-numeric identifiers executes the UUID/slug branch at lines 18-25. 3. In `_find_dashboard_by_identifier` (`delete_dashboard.py:10-25`), `DashboardDAO.find_by_id(identifier, id_column="uuid")` at lines 19-20 returns `None`, so the code enters the `try` block at line 61 and calls `DashboardDAO.get_by_id_or_slug(identifier)` at `superset/superset/daos/dashboard.py:17-41`. 4. In `DashboardDAO.get_by_id_or_slug` (`daos/dashboard.py:17-41`), when the user lacks access, `dashboard.raise_for_access()` at lines 36-37 raises `SupersetSecurityException`, which is caught and re-raised as `DashboardAccessDeniedError` at lines 38-39. 5. The helper `_find_dashboard_by_identifier` only catches `DashboardNotFoundError` (lines 62-64) and does not catch `DashboardAccessDeniedError`, so that exception propagates out of `_find_dashboard_by_identifier` and bypasses the outer `except SQLAlchemyError` in `delete_dashboard` (lines 84-93), causing the MCP tool call to fail with an unhandled `DashboardAccessDeniedError` instead of returning a structured `DeleteDashboardResponse`. This contrasts with `duplicate_dashboard` in `superset/mcp_service/dashboard/tool/duplicate_dashboard.py:11-35`, which explicitly catches `DashboardAccessDeniedError` and returns a normal response. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0601e360066f43fbade293f75af4eba3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0601e360066f43fbade293f75af4eba3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/delete_dashboard.py **Line:** 61:64 **Comment:** *Api Mismatch: The lookup helper assumes `DashboardDAO.get_by_id_or_slug()` only raises not-found, but for UUID identifiers it can also raise access-denied. Because that exception is not caught here (or at the call site), the tool can fail with an unhandled error instead of returning a structured `DeleteDashboardResponse`. Catch access-denied from this DAO call and map it to a normal tool response (for example not-found or permission-denied, depending on the intended disclosure model). Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=2536a4f3a23ad2cd253efd519a1b02bede207a928a0c1b758b1a1ce8e12adde8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=2536a4f3a23ad2cd253efd519a1b02bede207a928a0c1b758b1a1ce8e12adde8&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
