codeant-ai-for-open-source[bot] commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3598877215
########## superset/mcp_service/dashboard/tool/manage_dashboard_roles.py: ########## @@ -0,0 +1,315 @@ +# 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.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + ManageDashboardRolesRequest, + ManageDashboardRolesResponse, +) +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.types import SubjectType + +logger: logging.Logger = logging.getLogger(__name__) + + +def _viewer_role_ids(dashboard: Any) -> list[int]: + """Role IDs behind the dashboard's ROLE-type viewer subjects.""" + return [ + subject.role_id + for subject in dashboard.viewers + if subject.type == SubjectType.ROLE + ] + + +def _other_viewers(dashboard: Any) -> list[Any]: + """Non-ROLE-type viewers (USER/GROUP subjects), preserved untouched.""" + return [ + subject for subject in dashboard.viewers if subject.type != SubjectType.ROLE + ] + + +def _compute_new_role_ids( + dashboard: Any, request: ManageDashboardRolesRequest, viewers_enabled: bool +) -> tuple[list[int] | None, list[int] | None, ManageDashboardRolesResponse | None]: + """Load current viewer roles and apply add/remove operations. + + Returns ``(current_role_ids, new_role_ids, None)`` on success or + ``(None, None, error_response)`` when the initial lazy-load fails or a + removal targets an unassigned role. The pre-call ``current_role_ids`` + is returned so the caller can report true added/removed deltas. + """ + try: + current_role_ids = _viewer_role_ids(dashboard) + except SQLAlchemyError as db_err: + logger.error( + "Failed to load roles for dashboard %s: %s", + request.identifier, + db_err, + exc_info=True, + ) + return ( + None, + None, + ManageDashboardRolesResponse( + viewers_enabled=viewers_enabled, + error="Failed to load dashboard roles due to a database error.", + ), + ) + + unknown_removals = sorted(set(request.remove_role_ids) - set(current_role_ids)) + if unknown_removals: + return ( + None, + None, + ManageDashboardRolesResponse( + viewers_enabled=viewers_enabled, + error=( + f"Cannot remove role IDs that are not currently assigned: " + f"{unknown_removals}. Current role IDs: " + f"{sorted(current_role_ids)}." + ), + ), + ) + + new_role_ids = [ + role_id + for role_id in current_role_ids + if role_id not in request.remove_role_ids + ] + for role_id in request.add_role_ids: + if role_id not in new_role_ids: + new_role_ids.append(role_id) + + return current_role_ids, new_role_ids, None + + +def _resolve_role_subjects(new_role_ids: list[int]) -> tuple[list[Any], list[int]]: + """Resolve role IDs to ROLE-type Subjects, one at a time. + + Mirrors the owners tool's ``get_or_create_user_subject`` loop: a role + that exists but has no synced Subject row yet is synced on demand + instead of being misreported as nonexistent. Returns + ``(resolved_subjects, missing_role_ids)`` where the latter contains + only IDs with no matching role at all. + """ + from superset.subjects.utils import get_or_create_role_subject + + resolved_role_subjects: list[Any] = [] + missing_role_ids: list[int] = [] + for role_id in new_role_ids: + subject = get_or_create_role_subject(role_id) + if subject is None: + missing_role_ids.append(role_id) + else: + resolved_role_subjects.append(subject) + return resolved_role_subjects, missing_role_ids + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + method_permission_name="write", + annotations=ToolAnnotations( + title="Manage dashboard access roles", + readOnlyHint=False, + destructiveHint=True, + ), +) +def manage_dashboard_roles( + request: ManageDashboardRolesRequest, ctx: Context +) -> ManageDashboardRolesResponse: + """ + Add or remove dashboard access roles with explicit operations. + + Dashboard access roles restrict who can view a dashboard to members of + the listed roles, on top of normal Superset permissions. An empty roles + list means "no role restriction" — the dashboard is visible per standard + permissions instead. This only takes effect when the ``ENABLE_VIEWERS`` + feature flag is enabled; the response's ``viewers_enabled`` field + reports whether it is, and ``warnings`` notes when a change was applied + but has no live effect. + + Roles are the ROLE-type entries in the dashboard's Subject-based + ``viewers`` list. Any USER- or GROUP-type viewers already on the + dashboard are left untouched. + + Unlike ``update_dashboard``'s dropped ``roles`` field, this tool never + accepts a full-replacement list — only + ``add_role_ids``/``remove_role_ids``. + + Privacy: the returned ``roles`` 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 can access X" for a dashboard + the caller did not ask to modify, and do not call this tool merely to + look up current roles — those remain off-limits per the server + instructions. A request that has no effective change (e.g. "adding" a + role that is already assigned) returns an empty ``roles`` list rather + than the full current set, so this tool cannot be used as a disguised + directory lookup. + + Example:: + + manage_dashboard_roles(request={ + "identifier": 42, + "add_role_ids": [5], + }) + """ + from superset import is_feature_enabled + + ctx.info( + f"Managing dashboard roles: identifier={request.identifier} " + f"add={request.add_role_ids} remove={request.remove_role_ids}" + ) + + dashboard, auth_error = find_and_authorize_dashboard( + request.identifier, ManageDashboardRolesResponse + ) + if auth_error is not None: + return auth_error + + viewers_enabled = is_feature_enabled("ENABLE_VIEWERS") + warnings: list[str] = [] + if not viewers_enabled: + warnings.append( + "The ENABLE_VIEWERS feature flag is disabled on this instance; " + "dashboard viewers will be stored but have no effect on access " + "control until it is enabled." + ) + + current_role_ids, new_role_ids, compute_error = _compute_new_role_ids( + dashboard, request, viewers_enabled + ) + if compute_error is not None: + return compute_error + assert current_role_ids is not None # narrows for mypy + assert new_role_ids is not None # narrows for mypy + + # No-op short-circuit: skip the DB write and, more importantly, the + # full roles list in the response — mirrors manage_dashboard_owners. + # The roles 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 access roles via a disguised no-op + # (e.g. "add" a role that is already assigned). + if set(new_role_ids) == set(current_role_ids): + ctx.info(f"Dashboard {dashboard.id} roles unchanged; no-op request.") + return ManageDashboardRolesResponse( + dashboard_url=dashboard_url(dashboard), + viewers_enabled=viewers_enabled, + warnings=warnings + + ["No effective change: requested roles already match the current state."], + ) + + try: + with event_logger.log_context(action="mcp.manage_dashboard_roles.apply"): + other_viewers = _other_viewers(dashboard) + + resolved_role_subjects, missing_role_ids = _resolve_role_subjects( + new_role_ids + ) + if missing_role_ids: + return ManageDashboardRolesResponse( + viewers_enabled=viewers_enabled, + error=( + f"One or more role IDs do not exist: " + f"{sorted(missing_role_ids)}. Use list_roles to " + "resolve valid role IDs." + ), + ) Review Comment: **Suggestion:** The early return on `missing_role_ids` exits without rolling back the SQLAlchemy session, but `_resolve_role_subjects` may already have created/flushed Subject rows for earlier valid role IDs. That makes a failed request non-atomic and can leak unintended DB side effects into later work on the same session. Roll back before returning this error (or validate all role IDs up front before any create/flush calls). [missing cleanup] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Failed role updates still create subjects for valid roles. - ⚠️ MCP tool's DB changes not fully transactionally atomic. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the FastMCP tool function `manage_dashboard_roles` defined in `superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:159-197` with a request whose `identifier` points to an existing dashboard and `add_role_ids` contains both a valid role ID and a nonexistent role ID (e.g., `[existing_role_id_without_subject, 999999]`), leaving `remove_role_ids` empty. 2. Inside `manage_dashboard_roles`, `_compute_new_role_ids` at `superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:220-224` calculates `new_role_ids` including both role IDs, then execution enters the `try` block with `event_logger.log_context(action="mcp.manage_dashboard_roles.apply")` at lines 243-246. 3. At `superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:247-249`, `_resolve_role_subjects(new_role_ids)` is called; this helper uses `get_or_create_role_subject` in `superset/subjects/utils.py:156-176`, which for each valid role ID without a Subject row calls `sync_role_subject(role)` and `db.session.flush()` (line 175), creating and flushing new Subject rows into the current SQLAlchemy transaction before returning. 4. Back in `manage_dashboard_roles`, the `missing_role_ids` check at `superset/mcp_service/dashboard/tool/manage_dashboard_roles.py:250-258` detects the nonexistent role ID and immediately returns a `ManageDashboardRolesResponse` with an `error`, but this path does not call `db.session.rollback()`; any Subject rows created and flushed for earlier valid role IDs remain in the session, and if the surrounding request later commits, those DB mutations persist even though the overall role update was reported as a failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=70c16b9ed70446e09910f80a3ebadec8&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=70c16b9ed70446e09910f80a3ebadec8&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:** 247:258 **Comment:** *Missing Cleanup: The early return on `missing_role_ids` exits without rolling back the SQLAlchemy session, but `_resolve_role_subjects` may already have created/flushed Subject rows for earlier valid role IDs. That makes a failed request non-atomic and can leak unintended DB side effects into later work on the same session. Roll back before returning this error (or validate all role IDs up front before any create/flush calls). 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=73a8a4483ab743ec7aec47c4e47565fc31cdb2f11831bad643591c0902698f28&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=73a8a4483ab743ec7aec47c4e47565fc31cdb2f11831bad643591c0902698f28&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]
