codeant-ai-for-open-source[bot] commented on code in PR #41606:
URL: https://github.com/apache/superset/pull/41606#discussion_r3583973059
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,312 @@ class UpdateDashboardResponse(BaseModel):
)
+class ManageDashboardOwnersRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard owner management.
+
+ Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+ list with no safety guard, so an empty or partial list could silently
+ orphan a dashboard), this tool takes explicit add/remove operations and
+ rejects any change that would leave the dashboard with zero owners.
+
+ "Owners" here means USER-type entries in the dashboard's Subject-based
+ ``editors`` list (the ownership model apache/superset#38831 introduced,
+ replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+ editors already on the dashboard are left untouched by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
Review Comment:
**Suggestion:** `identifier` is declared as `int | str` without a
pre-validator that rejects booleans, so JSON `true`/`false` can be coerced to
`1`/`0` and mutate the wrong dashboard. Add the same `mode="before"`
bool-rejection guard already used by `DeleteDashboardRequest`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ manage_dashboard_owners may change owners on unintended dashboard.
- ⚠️ Owner governance workflows can silently drift from intent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe `DeleteDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py:136-151`, where `identifier: int
| str` has a
`@field_validator("identifier", mode="before")` named
`reject_bool_identifier` that raises
`ValueError` when `value` is a `bool`, specifically to avoid `true`/`false`
being treated
as IDs 1/0.
2. In the same file, inspect `ManageDashboardOwnersRequest` at lines
1065-1115:
`identifier: int | str = Field(...)` is declared on lines 1079-1085 without
any
corresponding `mode="before"` validator on `identifier`, unlike
`DeleteDashboardRequest`.
3. The `manage_dashboard_owners` FastMCP tool in
`superset/mcp_service/dashboard/tool/manage_dashboard_owners.py:270-307`
takes a
`ManageDashboardOwnersRequest` and passes `request.identifier` into
`_find_and_authorize_dashboard` at line 313, which then calls
`DashboardDAO.get_by_id_or_slug(identifier)` at lines 72-73.
4. When a client (typically the MCP LLM agent or another caller) sends JSON
like
`{"identifier": true, "add_owner_ids": [7]}`, Pydantic constructs
`ManageDashboardOwnersRequest`: because `identifier` is typed `int | str`
and `bool` is a
subclass of `int`, `true` is accepted as an integer and effectively treated
as ID `1`.
`_find_and_authorize_dashboard` then mutates owners on dashboard ID 1
instead of rejecting
the invalid boolean identifier, causing writes to the wrong dashboard.
Adding the same
bool-rejection validator pattern used in `DeleteDashboardRequest` would
prevent this.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1a8356f49591407dae77cb94e5fc5375&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=1a8356f49591407dae77cb94e5fc5375&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/schemas.py
**Line:** 1079:1085
**Comment:**
*Type Error: `identifier` is declared as `int | str` without a
pre-validator that rejects booleans, so JSON `true`/`false` can be coerced to
`1`/`0` and mutate the wrong dashboard. Add the same `mode="before"`
bool-rejection guard already used by `DeleteDashboardRequest`.
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%2F41606&comment_hash=2a9097a815b9299ba99edd88b9838f80167912bedf7fbd33afe3d7bf9b6ceb04&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=2a9097a815b9299ba99edd88b9838f80167912bedf7fbd33afe3d7bf9b6ceb04&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/manage_dashboard_owners.py:
##########
@@ -0,0 +1,390 @@
+# 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.
+
+"""
+Manage dashboard owners FastMCP tool
+
+Adds/removes dashboard owners via explicit operations, guarding against the
+"empty owners" footgun that the generic ``update_dashboard`` tool
+deliberately does not expose (a full-replacement ``owners``/``editors`` list
+has no "keep >=1 owner" guard of its own — ``populate_subject_list``'s
+``ensure_no_lockout`` only re-adds the CALLER, it does not prevent an admin
+from emptying the list outright).
+
+"Owners" are modeled as USER-type entries in the dashboard's Subject-based
+``editors`` list (apache/superset#38831 replaced the legacy ``owners``
+relationship with a unified Subject model covering User/Role/Group). Any
+ROLE- or GROUP-type editors already on the dashboard are preserved
+untouched by this tool.
+"""
+
+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.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+)
+from superset.exceptions import SupersetSecurityException
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ ManageDashboardOwnersRequest,
+ ManageDashboardOwnersResponse,
+)
+from superset.mcp_service.system.schemas import serialize_subject_object
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.subjects.exceptions import SubjectsNotFoundValidationError
+from superset.subjects.types import SubjectType
+
+logger: logging.Logger = logging.getLogger(__name__)
+
+
+def _find_and_authorize_dashboard(
+ identifier: int | str,
+) -> tuple[Any, ManageDashboardOwnersResponse | None]:
+ """Return (dashboard, None) on success or (None, error_response) on
failure.
+
+ Mirrors the helper in ``update_dashboard``: avoids ImportError before
+ Flask app initialisation by co-locating the imports it needs with the
+ call site rather than importing them at module load time.
+ """
+ from superset import security_manager
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ dashboard = DashboardDAO.get_by_id_or_slug(identifier)
+ except DashboardAccessDeniedError:
+ # get_by_id_or_slug re-checks view access and raises access-denied
+ # for dashboards the caller cannot see; surface it as the
+ # structured permission_denied response instead of an unhandled
+ # error.
+ return None, ManageDashboardOwnersResponse(
+ permission_denied=True,
+ error=(
+ "You do not have permission to access this dashboard. "
+ "Ask the user to grant access; do not retry."
+ ),
+ )
+ except (DashboardNotFoundError, SQLAlchemyError):
+ return None, ManageDashboardOwnersResponse(
+ error=f"Dashboard not found: {identifier!r}",
Review Comment:
**Suggestion:** This branch conflates operational database failures with
not-found errors, so transient DB issues are reported as “Dashboard not found”
instead of a database error. Separate `DashboardNotFoundError` and
`SQLAlchemyError` handling so callers can distinguish retryable infra failures
from missing resources. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ manage_dashboard_owners hides database errors as not-found.
- ⚠️ Operational diagnostics and retry logic become less accurate.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/dashboard/tool/manage_dashboard_owners.py`,
`_find_and_authorize_dashboard` (lines 60-107) calls
`DashboardDAO.get_by_id_or_slug(identifier)` at lines 72-73 inside a `try`
block.
2. The helper handles errors at lines 86-88 with `except
(DashboardNotFoundError,
SQLAlchemyError): return None,
ManageDashboardOwnersResponse(error=f"Dashboard not found:
{identifier!r}",)`, conflating genuine not-found cases with any
`SQLAlchemyError` raised
during lookup.
3. Contrast this with `_find_and_authorize_dashboard` in
`update_dashboard.py:74-91` and
dashboard lookup handling in `delete_dashboard.py:127-150`, where
`SQLAlchemyError` during
lookup is logged and surfaced as a database error (e.g., `"Dashboard lookup
failed due to
a database error."`) distinct from not-found responses.
4. To see the misclassification, invoke the `manage_dashboard_owners` tool
(function at
`manage_dashboard_owners.py:270-307`) with a valid `identifier` and valid
`add_owner_ids`/`remove_owner_ids`, then induce a database failure so that
`DashboardDAO.get_by_id_or_slug` raises `SQLAlchemyError`.
`_find_and_authorize_dashboard`
will respond with `ManageDashboardOwnersResponse(error="Dashboard not found:
<identifier>")` instead of a database error, preventing the caller from
distinguishing
transient infrastructure issues (where a retry is appropriate) from a truly
missing
dashboard.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=967786e7833449fe99e8126ebf2a1a20&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=967786e7833449fe99e8126ebf2a1a20&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/manage_dashboard_owners.py
**Line:** 86:88
**Comment:**
*Api Mismatch: This branch conflates operational database failures with
not-found errors, so transient DB issues are reported as “Dashboard not found”
instead of a database error. Separate `DashboardNotFoundError` and
`SQLAlchemyError` handling so callers can distinguish retryable infra failures
from missing resources.
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%2F41606&comment_hash=b1bb8064a48ff9442e552400120b4a5e9378b25da2ec44bae9ad8202445b5844&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=b1bb8064a48ff9442e552400120b4a5e9378b25da2ec44bae9ad8202445b5844&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,312 @@ class UpdateDashboardResponse(BaseModel):
)
+class ManageDashboardOwnersRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard owner management.
+
+ Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+ list with no safety guard, so an empty or partial list could silently
+ orphan a dashboard), this tool takes explicit add/remove operations and
+ rejects any change that would leave the dashboard with zero owners.
+
+ "Owners" here means USER-type entries in the dashboard's Subject-based
+ ``editors`` list (the ownership model apache/superset#38831 introduced,
+ replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+ editors already on the dashboard are left untouched by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ add_owner_ids: List[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to add as dashboard owners. Discover IDs with
``find_users``."
+ ),
+ )
+ remove_owner_ids: List[int] = Field(
+ default_factory=list,
+ description=(
+ "User IDs to remove from dashboard owners. Rejected if it would "
+ "leave the dashboard with zero owners, or if an ID is not "
+ "currently an owner."
+ ),
+ )
+
+ @model_validator(mode="after")
+ def _validate_operations(self) -> "ManageDashboardOwnersRequest":
+ if not self.add_owner_ids and not self.remove_owner_ids:
+ raise ValueError(
+ "At least one of add_owner_ids or remove_owner_ids is
required."
+ )
+ overlap: list[int] = sorted(
+ set(self.add_owner_ids) & set(self.remove_owner_ids)
+ )
+ if overlap:
+ raise ValueError(
+ "User IDs cannot appear in both add_owner_ids and "
+ f"remove_owner_ids: {overlap}."
+ )
+ return self
+
+
+class ManageDashboardOwnersResponse(BaseModel):
+ """Response schema for ``manage_dashboard_owners``."""
+
+ owners: List[SubjectInfo] = Field(
+ default_factory=list,
+ description=(
+ "Full list of USER-type editor subjects (dashboard owners) "
+ "after the operation. Any ROLE/GROUP-type editors on the "
+ "dashboard are not included here."
+ ),
+ )
+ dashboard_url: str | None = Field(None, description="URL to view the
dashboard")
+ added_owner_ids: List[int] = Field(
+ default_factory=list,
+ description="User IDs actually added as owners by this call.",
+ )
+ removed_owner_ids: List[int] = Field(
+ default_factory=list,
+ description="User IDs actually removed from owners by this call.",
+ )
+ warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Non-fatal advisory messages, e.g. that a non-admin caller was "
+ "automatically re-added as owner after trying to remove "
+ "themselves."
+ ),
+ )
+ error: str | None = Field(None, description="Error message, if operation
failed")
+ permission_denied: bool = Field(
+ default=False,
+ description=("True when the user lacks edit rights on the target
dashboard."),
+ )
+
+ @field_validator("error")
+ @classmethod
+ def sanitize_error_for_llm_context(cls, value: str | None) -> str | None:
+ """Wrap error text before it is exposed to LLM context."""
+ if value is None:
+ return value
+ return sanitize_for_llm_context(value, field_path=("error",))
+
+
+class ManageDashboardRolesRequest(BaseModel):
+ """Request schema for explicit add/remove dashboard RBAC role management.
+
+ Unlike ``update_dashboard``'s dropped ``roles`` field (a full-replacement
+ access-control list), this tool takes explicit add/remove operations.
+
+ "Roles" here means ROLE-type entries in the dashboard's Subject-based
+ ``viewers`` list (the access model apache/superset#38831 introduced,
+ replacing the legacy ``roles``/``DASHBOARD_RBAC`` relationship). Any
+ USER- or GROUP-type viewers already on the dashboard are left untouched
+ by this tool.
+ """
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
Review Comment:
**Suggestion:** `identifier` in the roles request has the same bool-coercion
problem (`bool` is a subclass of `int`), so `true` can resolve to dashboard ID
`1`. Add a pre-validation step to explicitly reject boolean identifiers. [type
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ manage_dashboard_roles may alter access on wrong dashboard.
- ⚠️ Role-based access control enforcement becomes unreliable.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Confirm the established pattern for boolean-rejection on dashboard
identifiers in
`DeleteDashboardRequest`
(`superset/mcp_service/dashboard/schemas.py:136-151`), where
`reject_bool_identifier` prevents `true`/`false` from being treated as IDs.
2. Inspect `ManageDashboardRolesRequest` in
`superset/mcp_service/dashboard/schemas.py:1161-1207`; its `identifier: int
| str =
Field(...)` on lines 1174-1180 has no `field_validator` guarding against
boolean values,
even though it shares the same ID-or-slug semantics.
3. The `manage_dashboard_roles` tool in
`superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:194-229` uses
`ManageDashboardRolesRequest` and passes `request.identifier` into
`_find_and_authorize_dashboard` at line 237, which calls
`DashboardDAO.get_by_id_or_slug(identifier)` at lines 70-72.
4. If a caller submits JSON `{"identifier": true, "add_role_ids": [5]}`,
Pydantic will
parse `identifier` as `int | str`, accept `true` as an `int` (1), and
`_find_and_authorize_dashboard` will operate on dashboard ID 1. The tool
will then adjust
access roles on the wrong dashboard without any validation error, whereas a
`mode="before"` validator that rejects `bool` (mirroring
`DeleteDashboardRequest`) would
block this.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f03a2ec6e84f4a38bd0b354a7a457dcc&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=f03a2ec6e84f4a38bd0b354a7a457dcc&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/schemas.py
**Line:** 1174:1180
**Comment:**
*Type Error: `identifier` in the roles request has the same
bool-coercion problem (`bool` is a subclass of `int`), so `true` can resolve to
dashboard ID `1`. Add a pre-validation step to explicitly reject boolean
identifiers.
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%2F41606&comment_hash=d4be31581c2a19ad4f48357fc4bfd12a1a4db2a502b6c69d83c1123569a02f72&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=d4be31581c2a19ad4f48357fc4bfd12a1a4db2a502b6c69d83c1123569a02f72&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/manage_dashboard_certification.py:
##########
@@ -0,0 +1,201 @@
+# 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.
+
+"""
+Manage dashboard certification FastMCP tool
+
+Sets or clears the ``certified_by`` / ``certification_details`` badge
+fields. Split out from the generic ``update_dashboard`` tool because
+certification is a distinct governance concern from layout/theme/metadata
+edits.
+"""
+
+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.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+)
+from superset.exceptions import SupersetSecurityException
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ ManageDashboardCertificationRequest,
+ ManageDashboardCertificationResponse,
+)
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+
+logger: logging.Logger = logging.getLogger(__name__)
+
+
+def _find_and_authorize_dashboard(
+ identifier: int | str,
+) -> tuple[Any, ManageDashboardCertificationResponse | None]:
+ """Return (dashboard, None) on success or (None, error_response) on
failure.
+
+ Mirrors the helper in ``update_dashboard``: avoids ImportError before
+ Flask app initialisation by co-locating the imports it needs with the
+ call site rather than importing them at module load time.
+ """
+ from superset import security_manager
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ dashboard = DashboardDAO.get_by_id_or_slug(identifier)
+ except DashboardAccessDeniedError:
+ # get_by_id_or_slug re-checks view access and raises access-denied
+ # for dashboards the caller cannot see; surface it as the
+ # structured permission_denied response instead of an unhandled
+ # error.
+ return None, ManageDashboardCertificationResponse(
+ permission_denied=True,
+ error=(
+ "You do not have permission to access this dashboard. "
+ "Ask the user to grant access; do not retry."
+ ),
+ )
+ except (DashboardNotFoundError, SQLAlchemyError):
+ return None, ManageDashboardCertificationResponse(
+ error=f"Dashboard not found: {identifier!r}",
+ )
Review Comment:
**Suggestion:** This handler maps `SQLAlchemyError` to “not found,” which is
incorrect and masks real backend failures. Split the exception handling so DB
errors return a database-failure message instead of a not-found error. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Database outages reported as misleading not-found certification errors.
- ⚠️ Callers may skip retries for transient database failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `_find_and_authorize_dashboard` in
`superset/mcp_service/dashboard/tool/manage_dashboard_certification.py:49-96`.
The helper
calls `DashboardDAO.get_by_id_or_slug(identifier)` at lines 61-63 inside a
`try` block.
2. Note the exception handling at lines 75-78: `except
(DashboardNotFoundError,
SQLAlchemyError): return None,
ManageDashboardCertificationResponse(error=f"Dashboard not
found: {identifier!r}",)`. Both a genuine not-found condition and any
`SQLAlchemyError`
(e.g., connection loss, timeout, migration problems) return the same
"Dashboard not found"
error message.
3. Compare this to `_find_and_authorize_dashboard` in
`superset/mcp_service/dashboard/tool/update_dashboard.py:74-91`, where
`DashboardNotFoundError` and `SQLAlchemyError` are handled separately:
not-found returns a
`DashboardError` with `error_type="DashboardNotFound"`, while
`SQLAlchemyError` logs the
exception and returns `error="Failed to look up dashboard due to a database
error.",
error_type="DatabaseError"`.
4. To reproduce the misclassification, run the
`manage_dashboard_certification` tool with
a valid identifier (e.g., an existing dashboard ID) and induce a database
failure so that
`DashboardDAO.get_by_id_or_slug` raises `SQLAlchemyError` (for example, by
simulating a
connectivity outage). `_find_and_authorize_dashboard` will catch the
`SQLAlchemyError` and
return a `ManageDashboardCertificationResponse` with `error="Dashboard not
found:
<identifier>"`, misleading the caller into believing the dashboard is
missing instead of
encountering a retryable infrastructure/database error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=12abeceba4f24265a1c8bd8e13407274&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=12abeceba4f24265a1c8bd8e13407274&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/manage_dashboard_certification.py
**Line:** 75:78
**Comment:**
*Api Mismatch: This handler maps `SQLAlchemyError` to “not found,”
which is incorrect and masks real backend failures. Split the exception
handling so DB errors return a database-failure message instead of a not-found
error.
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%2F41606&comment_hash=dae497b7f072b611af907d6a261308da6a8df9ab260d24d261a6eb740d127503&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=dae497b7f072b611af907d6a261308da6a8df9ab260d24d261a6eb740d127503&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:
##########
@@ -0,0 +1,324 @@
+# 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.
+
+"""
+Manage dashboard access roles FastMCP tool
+
+Adds/removes role-based dashboard access via explicit operations. Companion
+to ``manage_dashboard_owners`` — dashboard access roles are dropped from the
+generic ``update_dashboard`` tool because a full-replacement access-control
+list silently widens or narrows who can see a dashboard.
+
+"Roles" are modeled as ROLE-type entries in the dashboard's Subject-based
+``viewers`` list (apache/superset#38831 replaced the legacy
+``roles``/``DASHBOARD_RBAC`` relationship with a unified Subject model
+covering User/Role/Group, gated by the ``ENABLE_VIEWERS`` feature flag
+instead of ``DASHBOARD_RBAC``). Any USER- or GROUP-type viewers already on
+the dashboard are preserved untouched by this tool.
+"""
+
+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.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+)
+from superset.exceptions import SupersetSecurityException
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ ManageDashboardRolesRequest,
+ ManageDashboardRolesResponse,
+)
+from superset.mcp_service.system.schemas import serialize_subject_object
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.subjects.types import SubjectType
+
+logger: logging.Logger = logging.getLogger(__name__)
+
+
+def _find_and_authorize_dashboard(
+ identifier: int | str,
+) -> tuple[Any, ManageDashboardRolesResponse | None]:
+ """Return (dashboard, None) on success or (None, error_response) on
failure.
+
+ Mirrors the helper in ``update_dashboard``: avoids ImportError before
+ Flask app initialisation by co-locating the imports it needs with the
+ call site rather than importing them at module load time.
+ """
+ from superset import security_manager
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ dashboard = DashboardDAO.get_by_id_or_slug(identifier)
+ except DashboardAccessDeniedError:
+ # get_by_id_or_slug re-checks view access and raises access-denied
+ # for dashboards the caller cannot see; surface it as the
+ # structured permission_denied response instead of an unhandled
+ # error.
+ return None, ManageDashboardRolesResponse(
+ permission_denied=True,
+ error=(
+ "You do not have permission to access this dashboard. "
+ "Ask the user to grant access; do not retry."
+ ),
+ )
+ except (DashboardNotFoundError, SQLAlchemyError):
+ return None, ManageDashboardRolesResponse(
+ error=f"Dashboard not found: {identifier!r}",
Review Comment:
**Suggestion:** `SQLAlchemyError` is handled as a not-found condition, which
hides infrastructure/DB failures behind a false resource-missing message.
Return a distinct database-error response for `SQLAlchemyError` to preserve
correct caller behavior and retry logic. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ manage_dashboard_roles misreports database failures as not-found.
- ⚠️ RBAC automation cannot distinguish infra issues from deletions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Examine `_find_and_authorize_dashboard` in
`superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:58-105`; it
calls
`DashboardDAO.get_by_id_or_slug(identifier)` at lines 70-71 inside a `try`
block.
2. The error handling at lines 84-86 uses `except (DashboardNotFoundError,
SQLAlchemyError): return None,
ManageDashboardRolesResponse(error=f"Dashboard not found:
{identifier!r}",)`, so any `SQLAlchemyError` (e.g., connection issues, query
failures) is
indistinguishable from a legitimate not-found case to the caller.
3. Reference the established behavior in `update_dashboard.py:74-91`, where
`SQLAlchemyError` from the same DAO is logged with `logger.exception` and
surfaced as
`error="Failed to look up dashboard due to a database error.",
error_type="DatabaseError"`, and in `delete_dashboard.py:127-150`, where
lookup
`SQLAlchemyError` becomes `error="Dashboard lookup failed due to a database
error.",
error_type="LookupFailed"`.
4. To reproduce the buggy behavior, call the `manage_dashboard_roles` tool
(`manage_dashboard_roles.py:194-229`) with a valid dashboard identifier and
role changes,
then induce a database failure such that `DashboardDAO.get_by_id_or_slug`
raises
`SQLAlchemyError`. `_find_and_authorize_dashboard` will catch the
`SQLAlchemyError` and
return `ManageDashboardRolesResponse(error="Dashboard not found:
<identifier>")`, leading
the caller to wrongly treat a transient database issue as a permanently
missing dashboard
and potentially skipping appropriate retries or alerts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9d10e608cdcc417a9785b0b6afdec1f8&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=9d10e608cdcc417a9785b0b6afdec1f8&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/manage_dashboard_roles.py
**Line:** 84:86
**Comment:**
*Api Mismatch: `SQLAlchemyError` is handled as a not-found condition,
which hides infrastructure/DB failures behind a false resource-missing message.
Return a distinct database-error response for `SQLAlchemyError` to preserve
correct caller behavior and retry logic.
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%2F41606&comment_hash=48948d2f8d283cb10cf19705cc744eb9357f0df3c6359f0f8cdb648e5bd49f14&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=48948d2f8d283cb10cf19705cc744eb9357f0df3c6359f0f8cdb648e5bd49f14&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]
