aminghadersohi commented on code in PR #41606:
URL: https://github.com/apache/superset/pull/41606#discussion_r3589207301
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,358 @@ 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."
+ ),
+ )
Review Comment:
Confirmed and fixed: added a `mode="before"` validator
(`reject_bool_owner_ids`) on `add_owner_ids`/`remove_owner_ids` that rejects
any boolean list element, mirroring the identifier guard.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,358 @@ 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."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @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 DashboardMutationErrorFields(BaseModel):
+ """Shared ``error``/``permission_denied`` fields for dashboard governance
+ mutation responses (owners/roles/certification), including the
+ validator that wraps ``error`` before it is exposed to LLM context.
+ """
+
+ 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 ManageDashboardOwnersResponse(DashboardMutationErrorFields):
+ """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."
+ ),
+ )
+
+ @field_validator("owners", mode="after")
+ @classmethod
+ def sanitize_owners_for_llm_context(
+ cls, value: List[SubjectInfo]
+ ) -> List[SubjectInfo]:
+ """Wrap owner labels before LLM exposure; owner display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: List[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("owners", "label")
+ )
+ if not clean_label:
+ continue
+ sanitized.append(subject.model_copy(update={"label": clean_label}))
+ return sanitized
+
+
+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."
+ ),
+ )
Review Comment:
Confirmed and fixed: added a `mode="before"` validator
(`reject_bool_role_ids`) on `add_role_ids`/`remove_role_ids` that rejects any
boolean list element.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,358 @@ 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."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @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 DashboardMutationErrorFields(BaseModel):
+ """Shared ``error``/``permission_denied`` fields for dashboard governance
+ mutation responses (owners/roles/certification), including the
+ validator that wraps ``error`` before it is exposed to LLM context.
+ """
+
+ 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 ManageDashboardOwnersResponse(DashboardMutationErrorFields):
+ """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."
+ ),
+ )
+
+ @field_validator("owners", mode="after")
+ @classmethod
+ def sanitize_owners_for_llm_context(
+ cls, value: List[SubjectInfo]
+ ) -> List[SubjectInfo]:
+ """Wrap owner labels before LLM exposure; owner display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: List[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("owners", "label")
+ )
+ if not clean_label:
+ continue
+ sanitized.append(subject.model_copy(update={"label": clean_label}))
+ return sanitized
+
+
+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."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and mutate the wrong dashboard; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+ @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: list[int] = 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(DashboardMutationErrorFields):
+ """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."
+ )
+
+ @field_validator("roles", mode="after")
+ @classmethod
+ def sanitize_roles_for_llm_context(
+ cls, value: List[SubjectInfo]
+ ) -> List[SubjectInfo]:
+ """Wrap role labels before LLM exposure; role display names are
+ user-controlled and render as plain text in this response, so an
+ unsanitized label could inject content into LLM context (CWE-79
+ analog for LLM-facing output). Entries that sanitize to an empty
+ label are dropped rather than surfaced with a blank identity."""
+ sanitized: List[SubjectInfo] = []
+ for subject in value:
+ if subject.label is None:
+ sanitized.append(subject)
+ continue
+ clean_label = sanitize_for_llm_context(
+ subject.label, field_path=("roles", "label")
+ )
+ if not clean_label:
+ continue
+ sanitized.append(subject.model_copy(update={"label": clean_label}))
+ return sanitized
+
+
+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``."
+ ),
+ )
Review Comment:
Confirmed and fixed: added the same `reject_bool_identifier` validator
(mode="before") to `ManageDashboardCertificationRequest` that owners/roles
requests already have.
--
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]
