Copilot commented on code in PR #40353: URL: https://github.com/apache/superset/pull/40353#discussion_r3285761464
########## superset/mcp_service/user/tool/create_user.py: ########## @@ -0,0 +1,114 @@ +# 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 superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.user.schemas import CreateUserRequest, CreateUserResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="User", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a new user account", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_user(request: CreateUserRequest, ctx: Context) -> CreateUserResponse: + """Create a new Superset user account. Admin access is required. + + Use this tool to provision a new user with a username, name, email, + password, and one or more roles. After creation, the user can log in + with the supplied credentials. + + Workflow: + 1. Call get_instance_info to discover available role names and IDs + 2. Call this tool with the desired credentials and role IDs + 3. The returned ``id`` uniquely identifies the new user + """ + await ctx.info( + "Creating user: username=%r, email=%r, role_ids=%s" + % (request.username, request.email, request.role_ids) Review Comment: `ctx.info()` includes the user's email address. MCP context logs are surfaced back to the client/LLM, so this leaks PII even though the response intentionally omits user-directory fields. Consider omitting the email entirely from these messages, or logging only a redacted/hashed form. ########## superset/mcp_service/user/tool/create_user.py: ########## @@ -0,0 +1,114 @@ +# 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 superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.user.schemas import CreateUserRequest, CreateUserResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="User", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a new user account", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_user(request: CreateUserRequest, ctx: Context) -> CreateUserResponse: + """Create a new Superset user account. Admin access is required. + + Use this tool to provision a new user with a username, name, email, + password, and one or more roles. After creation, the user can log in Review Comment: This new mutate tool adds multiple important branches (missing roles, `add_user` failure, and the success response omitting sensitive fields) but no unit tests were added alongside the existing MCP tool test suite. Please add tests to cover these behaviors to prevent regressions and accidental exposure of user-directory data. ########## superset/mcp_service/user/schemas.py: ########## @@ -0,0 +1,102 @@ +# 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 user-related MCP tools.""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, EmailStr, Field, field_validator + + +class CreateUserRequest(BaseModel): + """Request schema for create_user.""" + + model_config = ConfigDict(populate_by_name=True) + + username: str = Field( + ..., + min_length=1, + max_length=64, + description="Unique username for the new account.", + ) + first_name: str = Field( + ..., + min_length=1, + max_length=64, + description="First name of the user.", + ) + last_name: str = Field( + ..., + min_length=1, + max_length=64, + description="Last name of the user.", + ) + email: EmailStr = Field( + ..., + description="Email address for the user account.", + ) + password: str = Field( + ..., + min_length=1, + description="Plain-text password; will be hashed before storage.", + ) Review Comment: This request schema introduces a plaintext `password` field. MCP `LoggingMiddleware` only redacts sensitive keys at the top level of `params`, but tool calls are passed as `{"request": {...}}`, so `request.password` will still be logged in the audit/event payload unless sanitization is made recursive (or explicitly redacts nested `request` fields). Please ensure passwords are never written to logs/action logs. ########## superset/mcp_service/user/tool/create_user.py: ########## @@ -0,0 +1,114 @@ +# 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 superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.user.schemas import CreateUserRequest, CreateUserResponse + +logger = logging.getLogger(__name__) + + Review Comment: `logger = logging.getLogger(__name__)` is defined but never used in this module, which will fail linting (unused variable). Either remove it (and the `logging` import) or use it for server-side logging alongside the `ctx.*()` messages. -- 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]
