aminghadersohi commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3583458277
########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,229 @@ +# 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. + +"""Tests for the manage_dashboard_certification MCP tool.""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) Review Comment: Fixed — `mock_auth` now has an explicit `Iterator[Mock]` return type annotation. See current `tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py` (same fixture pattern applied across all three dashboard tool test files). ########## superset/mcp_service/dashboard/schemas.py: ########## @@ -1028,6 +1028,291 @@ 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 = 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``." + ), + ) + add_role_ids: List[int] = Field( + default_factory=list, + description=( + "Role IDs to grant dashboard access to. Discover IDs with ``list_roles``." + ), + ) + remove_role_ids: List[int] = Field( + default_factory=list, + description=( + "Role IDs to revoke dashboard access from. Rejected if an ID is " + "not currently assigned to the dashboard." + ), + ) + + @model_validator(mode="after") + def _validate_operations(self) -> "ManageDashboardRolesRequest": + if not self.add_role_ids and not self.remove_role_ids: + raise ValueError( + "At least one of add_role_ids or remove_role_ids is required." + ) + overlap = sorted(set(self.add_role_ids) & set(self.remove_role_ids)) + if overlap: + raise ValueError( + "Role IDs cannot appear in both add_role_ids and " + f"remove_role_ids: {overlap}." + ) + return self + + +class ManageDashboardRolesResponse(BaseModel): + """Response schema for ``manage_dashboard_roles``.""" + + roles: List[SubjectInfo] = Field( + default_factory=list, + description=( + "Full list of ROLE-type viewer subjects (dashboard access " + "roles) after the operation. Any USER/GROUP-type viewers on " + "the dashboard are not included here." + ), + ) + dashboard_url: str | None = Field(None, description="URL to view the dashboard") + added_role_ids: List[int] = Field( + default_factory=list, + description="Role IDs actually added by this call.", + ) + removed_role_ids: List[int] = Field( + default_factory=list, + description="Role IDs actually removed by this call.", + ) + viewers_enabled: bool = Field( + default=False, + description=( + "Whether the ENABLE_VIEWERS feature flag is enabled on this " + "instance. When False, dashboard viewers are stored but have no " + "effect on access control — access still follows normal " + "Superset permissions/editorship." + ), + ) + warnings: List[str] = Field( + default_factory=list, description="Non-fatal advisory messages." + ) + 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 ManageDashboardCertificationRequest(BaseModel): + """Request schema for setting or clearing dashboard certification. + + ``certified_by`` and ``certification_details`` are independent optional + fields: omit (None) to leave a field unchanged, pass an empty string to + clear it, or pass a value to set it — mirrors the ``slug``/``css`` + clear-with-empty-string convention on ``update_dashboard``. + """ + + identifier: int | str = Field( + ..., + description=( + "Dashboard ID (integer), UUID, or slug. Same identifier shape " + "accepted by ``get_dashboard_info``." + ), + ) + certified_by: str | None = Field( + None, + max_length=500, + description=( + "Person or team certifying this dashboard. Pass an empty string " + "to clear. Omit (None) to leave unchanged." + ), + ) + certification_details: str | None = Field( + None, + max_length=5000, + description=( + "Details of the certification (why/how it was certified). Pass " + "an empty string to clear. Omit (None) to leave unchanged." + ), + ) + + @field_validator("certified_by") + @classmethod + def sanitize_certified_by(cls, v: str | None) -> str | None: + """Sanitize certified_by to prevent XSS; it renders as a UI badge.""" + if v is None or v == "": + return v + return sanitize_user_input(v, "certified_by", max_length=500, allow_empty=True) + + @field_validator("certification_details") + @classmethod + def sanitize_certification_details(cls, v: str | None) -> str | None: + """Sanitize certification_details to prevent XSS; it renders in the + same CertifiedBadge tooltip as certified_by.""" + if v is None or v == "": + return v + return sanitize_user_input( + v, "certification_details", max_length=5000, allow_empty=True + ) Review Comment: Fixed — `sanitize_certification_details` now raises `ValueError` when the input sanitizes down to an empty string (same guard applied to `sanitize_certified_by` right above it). A non-empty payload that sanitizes to nothing is rejected instead of silently clearing the field; only an explicit `""` still clears it. See current `superset/mcp_service/dashboard/schemas.py`, plus new regression tests in `test_manage_dashboard_certification.py`. -- 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]
