Copilot commented on code in PR #40354: URL: https://github.com/apache/superset/pull/40354#discussion_r3326661877
########## superset/mcp_service/role/tool/update_role.py: ########## @@ -0,0 +1,105 @@ +# 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. + +import logging + +from fastmcp import Context +from flask_appbuilder.security.sqla.models import PermissionView +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import db, event_logger +from superset.mcp_service.role.schemas import UpdateRoleRequest, UpdateRoleResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="security", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update an existing role", + readOnlyHint=False, + destructiveHint=False, Review Comment: `update_role` can replace the full permission set (including removing existing permissions when `permission_ids=[]`), so advertising it as non-destructive can cause MCP clients to skip confirmation flows they use for destructive mutations. This should be marked destructive, matching the semantics of the tool. ########## superset/mcp_service/role/schemas.py: ########## @@ -0,0 +1,75 @@ +# 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. + +"""Pydantic schemas for role-related MCP tools.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + + +class CreateRoleRequest(BaseModel): + model_config = ConfigDict(populate_by_name=True) + + name: str = Field(..., description="Name for the new role. Must be unique.") Review Comment: `ab_role.name` is defined as `String(64)` in the metadata schema, but this request accepts any length. Overlong role names will pass MCP validation and then fail at the database/write layer instead of returning a clean validation error; constrain this field to the database limit. ########## superset/mcp_service/role/tool/update_role.py: ########## @@ -0,0 +1,105 @@ +# 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. + +import logging + +from fastmcp import Context +from flask_appbuilder.security.sqla.models import PermissionView +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import db, event_logger +from superset.mcp_service.role.schemas import UpdateRoleRequest, UpdateRoleResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="security", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update an existing role", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def update_role(request: UpdateRoleRequest, ctx: Context) -> UpdateRoleResponse: Review Comment: This new admin mutation path is not covered by unit tests. Similar MCP mutation tools (for example `create_virtual_dataset`) have schema, success, error, and permission-denied tests under `tests/unit_tests/mcp_service`, so the role tools should add coverage for duplicate names, missing roles, permission replacement (including an empty list), invalid permission IDs, and RBAC denial. ########## superset/mcp_service/role/schemas.py: ########## @@ -0,0 +1,75 @@ +# 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. + +"""Pydantic schemas for role-related MCP tools.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + + +class CreateRoleRequest(BaseModel): + model_config = ConfigDict(populate_by_name=True) + + name: str = Field(..., description="Name for the new role. Must be unique.") + permission_ids: list[int] = Field( + default_factory=list, + description=( + "Optional list of PermissionView IDs to assign to the role. " + "These correspond to FAB permission-view-menu pairs " + "(e.g., can_read on Chart). " + "Leave empty to create a role with no permissions." + ), + ) + + +class CreateRoleResponse(BaseModel): + id: int | None = Field(None, description="ID of the created role.") + name: str | None = Field(None, description="Name of the created role.") + error: str | None = Field( + None, + description="Error message if role creation failed, otherwise null.", + ) + + +class UpdateRoleRequest(BaseModel): + model_config = ConfigDict(populate_by_name=True) + + id: int = Field(..., description="ID of the role to update.") + name: str | None = Field( + None, + description=( + "New name for the role. Must be unique. Omit to keep the current name." + ), + ) Review Comment: `ab_role.name` is limited to 64 characters, but update requests can set an arbitrary-length name. A too-long rename will pass schema validation and fail during commit; add the same length constraint here so callers get a validation error before mutation. -- 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]
