codeant-ai-for-open-source[bot] commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3598879099
########## superset/mcp_service/dashboard/tool/governance_utils.py: ########## @@ -0,0 +1,107 @@ +# 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. + +""" +Shared helpers for the dashboard governance tools +(``manage_dashboard_owners`` / ``manage_dashboard_roles`` / +``manage_dashboard_certification``). + +``update_dashboard`` keeps its own variant of the lookup/authorization +helper because its not-found contract differs β it returns a +``DashboardError`` carrying an ``error_type`` rather than the tool's own +response schema; unifying that shape is left to a follow-up. +""" + +import logging +from typing import Any, TypeVar + +from sqlalchemy.exc import SQLAlchemyError + +from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + DashboardNotFoundError, +) +from superset.exceptions import SupersetSecurityException +from superset.mcp_service.dashboard.schemas import DashboardMutationErrorFields +from superset.mcp_service.utils.url_utils import get_superset_base_url + +logger: logging.Logger = logging.getLogger(__name__) + +ResponseT = TypeVar("ResponseT", bound=DashboardMutationErrorFields) + + +def find_and_authorize_dashboard( + identifier: int | str, + response_cls: type[ResponseT], +) -> tuple[Any, ResponseT | None]: + """Return (dashboard, None) on success or (None, error_response) on failure. + + ``response_cls`` is the calling tool's response schema; every failure + mode is reported through it so the caller has a single pre-condition + branch. 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, response_cls( + permission_denied=True, + error=( + "You do not have permission to access this dashboard. " + "Ask the user to grant access; do not retry." + ), + ) + except DashboardNotFoundError: + return None, response_cls( + error=f"Dashboard not found: {identifier!r}", + ) + except SQLAlchemyError: + logger.exception("Database error looking up dashboard %r", identifier) + return None, response_cls( + error="Failed to look up dashboard due to a database error.", + ) + + if dashboard is None: + return None, response_cls( + error=f"Dashboard not found: {identifier!r}", + ) + + try: + security_manager.raise_for_editorship(dashboard) + except SupersetSecurityException: + return None, response_cls( + permission_denied=True, + error=( + f"You don't have permission to edit dashboard " + f"'{dashboard.dashboard_title}' (ID: {dashboard.id})." + ), + ) Review Comment: **Suggestion:** The editorship check only handles authorization exceptions, but `raise_for_editorship` performs database access and can also raise `SQLAlchemyError`; that path currently escapes as an unhandled 500 instead of the toolβs structured database-error response. Add a `SQLAlchemyError` branch around this check and map it to a controlled error payload. [api mismatch] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β Dashboard MCP tools can crash on authorization database errors. β οΈ Inconsistent error reporting compared to lookup database failures. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Start the MCP FastMCP server and ensure dashboard tools are loaded via `superset/mcp_service/dashboard/tool/__init__.py:18-32`, which exposes `manage_dashboard_owners`, `manage_dashboard_roles`, and `manage_dashboard_certification`. 2. From an MCP client, call any of these tools (e.g. `manage_dashboard_owners` at `superset/mcp_service/dashboard/tool/manage_dashboard_owners.py:218-255`) with a valid `identifier` for a dashboard the caller can currently view. 3. Inside the tool, observe the call to `find_and_authorize_dashboard(request.identifier, ManageDashboardOwnersResponse)` at `superset/mcp_service/dashboard/tool/governance_utils.py:47-50`, which first runs `DashboardDAO.get_by_id_or_slug(identifier)` in a try/except and then proceeds to the editorship check. 4. Force a database-layer failure during the authorization check (for example, simulate a broken DB connection or session when `security_manager.raise_for_editorship(dashboard)` re-queries the resource at `superset/security/manager.py:4656-4715`); this raises `SQLAlchemyError` during `self.session.query(...).get(resource_id)`. 5. Because `find_and_authorize_dashboard` only catches `DashboardAccessDeniedError`, `DashboardNotFoundError`, and `SQLAlchemyError` around the lookup (`get_by_id_or_slug`) but not around `raise_for_editorship` (see `superset/mcp_service/dashboard/tool/governance_utils.py:62-84` versus lines `91-100`), the `SQLAlchemyError` from `raise_for_editorship` propagates out of the tool. 6. Observe that the MCP tool call fails with an unhandled server error (HTTP 500 / FastMCP exception), rather than returning a structured response_cls (e.g. `ManageDashboardOwnersResponse`) with a controlled database-error message or `permission_denied` flag. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=22912a93008b4494b7aaec80103613b8&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=22912a93008b4494b7aaec80103613b8&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/governance_utils.py **Line:** 91:100 **Comment:** *Api Mismatch: The editorship check only handles authorization exceptions, but `raise_for_editorship` performs database access and can also raise `SQLAlchemyError`; that path currently escapes as an unhandled 500 instead of the toolβs structured database-error response. Add a `SQLAlchemyError` branch around this check and map it to a controlled error payload. 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=e89d1ea7f4c30d5c4fb779b69fc61fa3af0a83268a3dc64cc60ead5c611d8cd1&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=e89d1ea7f4c30d5c4fb779b69fc61fa3af0a83268a3dc64cc60ead5c611d8cd1&reaction=dislike'>π</a> ########## superset/mcp_service/dashboard/tool/manage_dashboard_owners.py: ########## @@ -0,0 +1,354 @@ +# 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.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + ManageDashboardOwnersRequest, + ManageDashboardOwnersResponse, +) +from superset.mcp_service.dashboard.tool.governance_utils import ( + dashboard_url, + find_and_authorize_dashboard, +) +from superset.mcp_service.system.schemas import serialize_subject_object +from superset.subjects.exceptions import SubjectsNotFoundValidationError +from superset.subjects.types import SubjectType + +logger: logging.Logger = logging.getLogger(__name__) + + +def _owner_user_ids(dashboard: Any) -> list[int]: + """User IDs behind the dashboard's USER-type editor subjects.""" + return [ + subject.user_id + for subject in dashboard.editors + if subject.type == SubjectType.USER + ] + + +def _other_editors(dashboard: Any) -> list[Any]: + """Non-USER-type editors (ROLE/GROUP subjects), preserved untouched.""" + return [ + subject for subject in dashboard.editors if subject.type != SubjectType.USER + ] + + +def _compute_new_owner_ids( + current_owner_ids: list[int], request: ManageDashboardOwnersRequest +) -> tuple[list[int] | None, ManageDashboardOwnersResponse | None]: + """Apply add/remove operations and validate the result. + + Returns ``(new_owner_ids, None)`` on success or ``(None, error_response)`` + when a removal targets a non-owner or the result would be empty. + """ + unknown_removals = sorted(set(request.remove_owner_ids) - set(current_owner_ids)) + if unknown_removals: + return None, ManageDashboardOwnersResponse( + error=( + f"Cannot remove user IDs that are not currently owners: " + f"{unknown_removals}. Current owner IDs: " + f"{sorted(current_owner_ids)}." + ), + ) + + new_owner_ids = [ + owner_id + for owner_id in current_owner_ids + if owner_id not in request.remove_owner_ids + ] + for owner_id in request.add_owner_ids: + if owner_id not in new_owner_ids: + new_owner_ids.append(owner_id) + + if not new_owner_ids: + return None, ManageDashboardOwnersResponse( + error=( + "Cannot remove all owners; a dashboard must have at least " + "one owner. To transfer ownership, add the new owner in the " + "same call as removing the last existing one." + ), + ) + + return new_owner_ids, None + + +def _apply_owner_change( + dashboard: Any, new_owner_ids: list[int] +) -> tuple[list[Any] | None, ManageDashboardOwnersResponse | None]: + """Resolve the new owner user IDs to USER-type Subjects and persist. + + Mutates ``dashboard.editors`` in place on success β replacing the + USER-type entries while preserving any ROLE/GROUP-type editors. Returns + ``(resolved_owner_subjects, None)`` on success β captured before commit + so callers never need to dereference ``dashboard.editors`` post-commit + (SQLAlchemy expires ORM attributes on commit, and a failed + ``refresh()`` would otherwise leave a later ``dashboard.editors`` read + free to raise an unhandled ``SQLAlchemyError`` from a broken session) β + or ``(None, error_response)`` on failure. + """ + from superset.commands.utils import populate_subject_list + from superset.subjects.utils import get_or_create_user_subject + + try: + with event_logger.log_context(action="mcp.manage_dashboard_owners.apply"): + other_editors = _other_editors(dashboard) + + new_subject_ids: list[int] = [] + for user_id in new_owner_ids: + subject = get_or_create_user_subject(user_id) + if subject is None: + return None, ManageDashboardOwnersResponse( + error=( + f"User ID {user_id} does not exist. Use " + "find_users to resolve valid user IDs." + ), + ) + new_subject_ids.append(subject.id) + + try: + resolved_owner_subjects = populate_subject_list( + new_subject_ids, + default_to_user=False, + ensure_no_lockout=True, + field_name="editors", + ) + except SubjectsNotFoundValidationError: + return None, ManageDashboardOwnersResponse( + error=( + "One or more user IDs could not be resolved to " + "owners. Use find_users to resolve valid user IDs." + ), + ) + + dashboard.editors = other_editors + resolved_owner_subjects + db.session.commit() # pylint: disable=consider-using-transaction + try: + db.session.refresh(dashboard) + except SQLAlchemyError: + logger.warning( + "Dashboard %s owners updated but refresh failed; " + "continuing with current values", + dashboard.id, + exc_info=True, + ) + + except SQLAlchemyError as db_err: + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during error handling", + exc_info=True, + ) + logger.error("Dashboard owners update failed: %s", db_err, exc_info=True) + return None, ManageDashboardOwnersResponse( + error="Failed to update dashboard owners due to a database error.", + ) + + return resolved_owner_subjects, None + + +def _build_owner_warnings( + final_owner_ids: set[int], new_owner_ids: list[int] +) -> list[str]: + """Flag when the resolver re-added an ID that was not requested. + + Happens when a non-admin caller tries to remove themselves β + ``populate_subject_list``'s ``ensure_no_lockout`` self-protection + re-adds their USER subject. + """ + auto_added = final_owner_ids - set(new_owner_ids) + if not auto_added: + return [] + return [ + f"User ID(s) {sorted(auto_added)} were automatically re-added as " + "owner(s): non-admin callers cannot remove themselves from the " + "owners list." + ] + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + method_permission_name="write", + annotations=ToolAnnotations( + title="Manage dashboard owners", + readOnlyHint=False, + destructiveHint=True, + ), +) +def manage_dashboard_owners( + request: ManageDashboardOwnersRequest, ctx: Context +) -> ManageDashboardOwnersResponse: + """ + Add or remove dashboard owners with explicit, safe operations. + + Owners can edit the dashboard, manage its charts, and delete it. Unlike + ``update_dashboard``'s dropped ``owners`` field, this tool never accepts + a full-replacement list β only ``add_owner_ids``/``remove_owner_ids`` β + and rejects any change that would leave the dashboard with zero owners. + + Owners are the USER-type entries in the dashboard's Subject-based + ``editors`` list. Any ROLE- or GROUP-type editors already on the + dashboard are left untouched. + + A non-admin caller who removes themselves is automatically re-added + (mirrors the same self-protection ``update_dashboard``'s editorship + check relies on) unless an ``EXTRA_EDITORS_RESOLVER`` is configured on + the instance; the response's ``warnings`` reports when this happens. + + Privacy: the returned ``owners`` list is sanctioned only as confirmation + of the add/remove operation the caller explicitly requested on this + dashboard. Do not use it to answer "who owns X" for a dashboard the + caller did not ask to modify, and do not call this tool merely to look + up current owners β those remain off-limits per the server instructions. + A request that has no effective change (e.g. "adding" an ID that is + already an owner) returns an empty ``owners`` list rather than the full + current set, so this tool cannot be used as a disguised directory + lookup. + + Example:: + + manage_dashboard_owners(request={ + "identifier": 42, + "add_owner_ids": [7], + "remove_owner_ids": [3], + }) + """ + ctx.info( + f"Managing dashboard owners: identifier={request.identifier} " + f"add={request.add_owner_ids} remove={request.remove_owner_ids}" + ) + + dashboard, auth_error = find_and_authorize_dashboard( + request.identifier, ManageDashboardOwnersResponse + ) + if auth_error is not None: + return auth_error + + try: + current_owner_ids = _owner_user_ids(dashboard) + except SQLAlchemyError as db_err: + logger.error( + "Failed to load owners for dashboard %s: %s", + request.identifier, + db_err, + exc_info=True, + ) + return ManageDashboardOwnersResponse( + error="Failed to load dashboard owners due to a database error.", + ) + + new_owner_ids, compute_error = _compute_new_owner_ids(current_owner_ids, request) + if compute_error is not None: + return compute_error + assert new_owner_ids is not None # narrows for mypy; empty list errors above + + # No-op short-circuit: skip the DB write and, more importantly, the full + # owners list in the response. The owners list is only sanctioned as + # confirmation of an actual change (see docstring); returning it for a + # request that changes nothing would let a caller enumerate owners via + # a disguised no-op (e.g. "add" an ID that is already an owner). + # + # A self-removal-only request from a non-admin is deliberately NOT + # detected here even though ensure_no_lockout will revert it: whether + # the caller is actually re-added depends on resolution-time state + # (admin status, EXTRA_EDITORS_RESOLVER) that populate_subject_list + # owns, and predicting it here would duplicate that logic. That rare + # path is allowed to commit a redundant (state-preserving) write and is + # caught by the post-resolution no-op check further down. + if set(new_owner_ids) == set(current_owner_ids): + ctx.info(f"Dashboard {dashboard.id} owners unchanged; no-op request.") + return ManageDashboardOwnersResponse( + dashboard_url=dashboard_url(dashboard), + warnings=[ + "No effective change: requested owners already match the current state." + ], + ) + + resolved_owner_subjects, apply_error = _apply_owner_change(dashboard, new_owner_ids) + if apply_error is not None: + return apply_error + assert resolved_owner_subjects is not None # narrows for mypy + + final_owner_ids = { + subject.user_id + for subject in resolved_owner_subjects + if subject.type == SubjectType.USER + } Review Comment: **Suggestion:** The response is built by reading `resolved_owner_subjects` after `_apply_owner_change` has already committed; committed ORM instances are expired, so if the session is unhealthy (the same refresh-failure scenario this file comments about), those attribute reads can raise `SQLAlchemyError` and crash the tool after a successful write. Return primitive owner data captured before commit (as done in the roles tool), or wrap post-commit owner reads in SQLAlchemyError handling. [possible bug] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx β Owner management tool may fail after successful database commit. β οΈ Confusing client experience when changes apply but response crashes. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Use an MCP client to invoke the FastMCP tool `manage_dashboard_owners` defined at `superset/mcp_service/dashboard/tool/manage_dashboard_owners.py:218-255`, passing a valid dashboard `identifier` and non-empty `add_owner_ids` or `remove_owner_ids` so a write occurs. 2. The tool authorizes and loads the dashboard via `find_and_authorize_dashboard` at `governance_utils.py:47-102`, then computes `current_owner_ids` using `_owner_user_ids(dashboard)` at `manage_dashboard_owners.py:58-64` and derives `new_owner_ids` via `_compute_new_owner_ids` at `manage_dashboard_owners.py:74-110`. 3. After the no-op check at `manage_dashboard_owners.py:285-305`, the function calls `_apply_owner_change(dashboard, new_owner_ids)` at `manage_dashboard_owners.py:307`. 4. Inside `_apply_owner_change` (`manage_dashboard_owners.py:113-186`), `populate_subject_list` resolves USER-type subjects into `resolved_owner_subjects` (lines 146-152), `dashboard.editors` is mutated (line 161), and `db.session.commit()` runs at line 162. Immediately after, `db.session.refresh(dashboard)` is called and any `SQLAlchemyError` is caught and logged at lines 163-171, meaning commit may succeed but the session can be left unhealthy. 5. `_apply_owner_change` returns `resolved_owner_subjects` (the ORM Subject instances) to `manage_dashboard_owners` at line 186, after the commit. 6. Back in `manage_dashboard_owners`, the code computes `final_owner_ids = {subject.user_id for subject in resolved_owner_subjects if subject.type == SubjectType.USER}` at lines 312-316 and later builds the `owners` response list using `serialize_subject_object(subject)` within the list comprehension at lines 344-349. 7. If the SQLAlchemy session is unhealthy or the resolved Subject instances are expired on commit, accessing `subject.user_id`, `subject.type`, or serializing `subject` can trigger lazy reloads that raise `SQLAlchemyError`. These exceptions are not caught in `manage_dashboard_owners`, so the response path crashes even though the owners update at `db.session.commit()` has already succeeded. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5f35f38e91974939851f1e7478a6e5ce&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=5f35f38e91974939851f1e7478a6e5ce&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:** 307:316 **Comment:** *Possible Bug: The response is built by reading `resolved_owner_subjects` after `_apply_owner_change` has already committed; committed ORM instances are expired, so if the session is unhealthy (the same refresh-failure scenario this file comments about), those attribute reads can raise `SQLAlchemyError` and crash the tool after a successful write. Return primitive owner data captured before commit (as done in the roles tool), or wrap post-commit owner reads in SQLAlchemyError handling. 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=67118deb0e1060567e558b031c4ed515b19cda3d94860b9ae3877a0c5875457e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=67118deb0e1060567e558b031c4ed515b19cda3d94860b9ae3877a0c5875457e&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]
