codeant-ai-for-open-source[bot] commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3588407260
########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,290 @@ +# 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.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() Review Comment: **Suggestion:** Add an explicit type annotation to this mocked dashboard variable so the helper keeps full type-hint coverage. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This is a new Python variable that can be annotated, and it currently relies on inference only. That matches the type-hint rule for relevant variables. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c34bcf77f5d4ce8848075c2d40b7d8f&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=8c34bcf77f5d4ce8848075c2d40b7d8f&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py **Line:** 37:37 **Comment:** *Custom Rule: Add an explicit type annotation to this mocked dashboard variable so the helper keeps full type-hint coverage. 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=f2c242e779ee5448827857f2bd55d21b2cc523213137744358a3be92afdb4d55&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=f2c242e779ee5448827857f2bd55d21b2cc523213137744358a3be92afdb4d55&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,290 @@ +# 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.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.certified_by = certified_by + dashboard.certification_details = certification_details + return dashboard + + +class TestManageDashboardCertification: + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certification( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard() Review Comment: **Suggestion:** Annotate this local dashboard fixture variable with its expected type instead of relying on inference. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This local variable is introduced without any type annotation even though its type is known and can be annotated, so it fits the Python type-hint requirement. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ffa683589b614cd2b71d01e4c403e7a6&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=ffa683589b614cd2b71d01e4c403e7a6&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py **Line:** 53:53 **Comment:** *Custom Rule: Annotate this local dashboard fixture variable with its expected type instead of relying on inference. 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=d25131c80402e36fbde2945351bf01c9e5789dd2bfe45b581ef6ae93debbd1d1&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=d25131c80402e36fbde2945351bf01c9e5789dd2bfe45b581ef6ae93debbd1d1&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,290 @@ +# 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.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.certified_by = certified_by + dashboard.certification_details = certification_details + return dashboard + + +class TestManageDashboardCertification: + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certification( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard() + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "Data Platform Team", + "certification_details": "Verified against source-of-truth.", + } + }, + ) + + assert dash.certified_by == "Data Platform Team" + assert dash.certification_details == "Verified against source-of-truth." + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + # Response text is wrapped for LLM context (mirrors the read-path + # sanitization on DashboardInfo.certified_by), so check substring. + assert "Data Platform Team" in payload["certified_by"] + assert set(payload["changed_fields"]) == { + "certified_by", + "certification_details", + } + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certified_by_only( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(certification_details="Existing details") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42, "certified_by": "Analytics Guild"}}, + ) + + assert dash.certified_by == "Analytics Guild" + assert dash.certification_details == "Existing details" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == ["certified_by"] + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_clear_with_empty_string( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard( + certified_by="Old Team", certification_details="Old details" + ) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "", + "certification_details": "", + } + }, + ) + + assert dash.certified_by is None + assert dash.certification_details is None + payload = json.loads(result.content[0].text) + assert payload["certified_by"] is None + assert payload["certification_details"] is None + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_no_fields_is_noop(self, mock_get: Mock, mcp_server: object) -> None: + dash = _mock_dashboard(certified_by="Existing") + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42}}, + ) + + assert dash.certified_by == "Existing" + payload = json.loads(result.content[0].text) + assert payload["changed_fields"] == [] + assert any("No fields provided" in w for w in payload["warnings"]) + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 999999, "certified_by": "Team"}}, + ) + + payload = json.loads(result.content[0].text) + assert "not found" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_lookup_database_error_is_not_masked_as_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + """A real DB/infra failure during lookup must surface as a distinct + database error, not be collapsed into "not found".""" + from sqlalchemy.exc import SQLAlchemyError + + mock_get.side_effect = SQLAlchemyError("connection to server lost") + + with patch( + "superset.mcp_service.dashboard.tool.manage_dashboard_certification.logger" + ) as mock_logger: + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 999999, "certified_by": "Team"}}, + ) + + payload = json.loads(result.content[0].text) + assert "not found" not in (payload.get("error") or "").lower() + assert "database error" in (payload.get("error") or "").lower() + assert "connection to server lost" not in (payload.get("error") or "") + mock_logger.exception.assert_called_once() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_view_only_caller_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + """get_by_id_or_slug itself re-checks view access and raises + DashboardAccessDeniedError for dashboards the caller cannot see; + that must surface as permission_denied, not an unhandled error.""" + from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + ) + + mock_get.side_effect = DashboardAccessDeniedError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42, "certified_by": "Team"}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_non_owner_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.exceptions import SupersetSecurityException + + dash = _mock_dashboard() + mock_get.return_value = dash + + with patch( + "superset.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(Mock(message="forbidden")), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + {"request": {"identifier": 42, "certified_by": "Team"}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + assert dash.certified_by is None + + @pytest.mark.asyncio + async def test_xss_certified_by_is_sanitized(self, mcp_server: object) -> None: + from superset.mcp_service.dashboard.schemas import ( + ManageDashboardCertificationRequest, + ) + + request = ManageDashboardCertificationRequest( + identifier=1, certified_by="<script>alert(1)</script>Team" + ) Review Comment: **Suggestion:** Add an explicit type annotation to this request object variable to keep method-local variables fully typed. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This method-local variable is created without an explicit type annotation even though the concrete request type is obvious and can be annotated, so the type-hint rule applies. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a365c95b7dc47078bb701a92f852c07&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=3a365c95b7dc47078bb701a92f852c07&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py **Line:** 243:245 **Comment:** *Custom Rule: Add an explicit type annotation to this request object variable to keep method-local variables fully typed. 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=b7e38222f8604edc898bf8a3a3926a7e0e5ebdfee2a5903ac0cf9410f43fa0e7&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=b7e38222f8604edc898bf8a3a3926a7e0e5ebdfee2a5903ac0cf9410f43fa0e7&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py: ########## @@ -0,0 +1,290 @@ +# 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.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + certified_by: str | None = None, + certification_details: str | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.certified_by = certified_by + dashboard.certification_details = certification_details + return dashboard + + +class TestManageDashboardCertification: + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_set_certification( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard() + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_certification", + { + "request": { + "identifier": 42, + "certified_by": "Data Platform Team", + "certification_details": "Verified against source-of-truth.", + } + }, + ) + + assert dash.certified_by == "Data Platform Team" + assert dash.certification_details == "Verified against source-of-truth." + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) Review Comment: **Suggestion:** Add a concrete type annotation for the parsed response payload to satisfy variable-level typing requirements. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> The parsed JSON response is stored in a new local variable without an explicit type hint, and it is a variable that can be annotated under the rule. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fed08568c1ab4adb94002c7031190a30&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=fed08568c1ab4adb94002c7031190a30&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py **Line:** 71:71 **Comment:** *Custom Rule: Add a concrete type annotation for the parsed response payload to satisfy variable-level typing requirements. 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=b4930ad0c9d496ea0f5ea239dbbb286ef7e63067b33381611c3d09f2f5f31c5d&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=b4930ad0c9d496ea0f5ea239dbbb286ef7e63067b33381611c3d09f2f5f31c5d&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,469 @@ +# 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_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT: str = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST: str = "superset.commands.utils.populate_subject_list" + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] Review Comment: **Suggestion:** Replace this untyped lambda with a small named helper function that explicitly annotates its parameter and return type. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> The rule requires type hints on new or modified Python functions/callables where applicable. This lambda introduces an untyped callable in the new test code, so the suggestion correctly identifies a real type-hint omission. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6358d5c1d91f4596b975c50ad5a64519&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=6358d5c1d91f4596b975c50ad5a64519&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py **Line:** 83:83 **Comment:** *Custom Rule: Replace this untyped lambda with a small named helper function that explicitly annotates its parameter and return type. 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=b4dbc1aa76b78b2e40ef5aae7c22e393c9b570c57e542bcd578a860eea96faf1&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=b4dbc1aa76b78b2e40ef5aae7c22e393c9b570c57e542bcd578a860eea96faf1&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,469 @@ +# 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_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT: str = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST: str = "superset.commands.utils.populate_subject_list" + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] + mock_populate.return_value = [existing, new_owner] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + mock_populate.assert_called_once_with( + [100, 101], + default_to_user=False, + ensure_no_lockout=True, + field_name="editors", + ) + assert dash.editors == [existing, new_owner] + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + assert sorted(o["id"] for o in payload["owners"]) == [100, 101] + assert payload["added_owner_ids"] == [7] + assert payload["removed_owner_ids"] == [] + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_remove_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + keep = _mock_subject(100, 1, "admin") + remove = _mock_subject(101, 2, "carol") + dash = _mock_dashboard(editors=[keep, remove]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: keep}[uid] Review Comment: **Suggestion:** Use a named typed function for the side effect so the input and output types are explicitly declared. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This is another untyped lambda introduced in new Python test code. Under the type-hint rule, it is a real omission because the callable parameter and return contract are not annotated. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=652c0bc3347a4e8590d56a7f019916c2&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=652c0bc3347a4e8590d56a7f019916c2&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py **Line:** 122:122 **Comment:** *Custom Rule: Use a named typed function for the side effect so the input and output types are explicitly declared. 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=e7304ce59ae23077f9536f5a42bb00436a383d4f097f159adf8db0023af4a4ef&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=e7304ce59ae23077f9536f5a42bb00436a383d4f097f159adf8db0023af4a4ef&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,469 @@ +# 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_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT: str = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST: str = "superset.commands.utils.populate_subject_list" + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] + mock_populate.return_value = [existing, new_owner] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + mock_populate.assert_called_once_with( + [100, 101], + default_to_user=False, + ensure_no_lockout=True, + field_name="editors", + ) + assert dash.editors == [existing, new_owner] + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + assert sorted(o["id"] for o in payload["owners"]) == [100, 101] + assert payload["added_owner_ids"] == [7] + assert payload["removed_owner_ids"] == [] + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_remove_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + keep = _mock_subject(100, 1, "admin") + remove = _mock_subject(101, 2, "carol") + dash = _mock_dashboard(editors=[keep, remove]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: keep}[uid] + mock_populate.return_value = [keep] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [2]}}, + ) + + mock_populate.assert_called_once_with( + [100], default_to_user=False, ensure_no_lockout=True, field_name="editors" + ) + payload = json.loads(result.content[0].text) + assert payload["removed_owner_ids"] == [2] + assert [o["id"] for o in payload["owners"]] == [100] + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_all_owners_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + """Removing every owner must be rejected before any DB write.""" + original_editors = [_mock_subject(100, 1, "admin")] + dash = _mock_dashboard(editors=original_editors) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "at least one owner" in (payload.get("error") or "").lower() + # dashboard.editors must remain untouched + assert dash.editors is original_editors + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_unknown_owner_id_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [999]}}, + ) + + payload = json.loads(result.content[0].text) + assert "999" in (payload.get("error") or "") + assert "not currently owners" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 999999, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "not found" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_view_only_caller_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + """get_by_id_or_slug itself re-checks view access and raises + DashboardAccessDeniedError for dashboards the caller cannot see; + that must surface as permission_denied, not an unhandled error.""" + from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + ) + + mock_get.side_effect = DashboardAccessDeniedError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_non_owner_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.exceptions import SupersetSecurityException + + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + with patch( + "superset.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(Mock(message="forbidden")), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_unknown_user_id_in_add_rejected( + self, mock_get: Mock, mock_get_or_create: Mock, mcp_server: object + ) -> None: + existing = _mock_subject(100, 1, "admin") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: existing if uid == 1 else None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [99999]}}, + ) + + payload = json.loads(result.content[0].text) + assert "not exist" in (payload.get("error") or "").lower() + assert "find_users" in (payload.get("error") or "") + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_self_removal_auto_readded_warns( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + """When populate_subject_list re-adds a caller who tried to remove + themselves (ensure_no_lockout self-protection), the response + surfaces a warning instead of silently diverging from what was + requested.""" + admin = _mock_subject(100, 1, "admin") + carol = _mock_subject(101, 2, "carol") + dave = _mock_subject(102, 3, "dave") + dash = _mock_dashboard(editors=[admin, carol, dave]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {3: dave}[uid] + # Requested new subject ids == [102] (dave), but populate_subject_list + # simulates re-adding admin's subject per ensure_no_lockout. + mock_populate.return_value = [admin, dave] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [1, 2]}}, + ) + + mock_populate.assert_called_once_with( + [102], default_to_user=False, ensure_no_lockout=True, field_name="editors" + ) + payload = json.loads(result.content[0].text) + assert any("automatically re-added" in w for w in payload.get("warnings", [])) + + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_already_owner_is_noop_without_disclosure( + self, mock_session: Mock, mock_get: Mock, mcp_server: object + ) -> None: + """Adding an ID that is already an owner is a no-op: skip the DB + write and do not return the full owner list. Otherwise a caller + could enumerate owners via a disguised "change" request that + never actually changes anything.""" + existing = _mock_subject(100, 1, "admin") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [1]}}, + ) + + mock_session.commit.assert_not_called() + payload = json.loads(result.content[0].text) + assert payload["owners"] == [] + assert payload["added_owner_ids"] == [] + assert any("no effective change" in w.lower() for w in payload["warnings"]) + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_partial_change_still_discloses_full_owners( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + """A request that DOES change something (even if one of several + requested ops is redundant) still returns the full owner list β + only a fully no-op request is suppressed.""" + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] Review Comment: **Suggestion:** Introduce a named helper with explicit type annotations instead of this untyped lambda expression. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This is the same kind of untyped lambda as the first suggestion, in new code. The rule explicitly calls for type hints on functions/callables that can be annotated, so this is a verified violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=49a9e5d6a7cb47968957f1f03e165e69&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=49a9e5d6a7cb47968957f1f03e165e69&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py **Line:** 347:347 **Comment:** *Custom Rule: Introduce a named helper with explicit type annotations instead of this untyped lambda expression. 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=d7028d5ef2bf69f2304b861d6901615c1fe44253648ea6a3a56c498687f5565e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=d7028d5ef2bf69f2304b861d6901615c1fe44253648ea6a3a56c498687f5565e&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,469 @@ +# 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_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT: str = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST: str = "superset.commands.utils.populate_subject_list" + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] + mock_populate.return_value = [existing, new_owner] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + mock_populate.assert_called_once_with( + [100, 101], + default_to_user=False, + ensure_no_lockout=True, + field_name="editors", + ) + assert dash.editors == [existing, new_owner] + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + assert sorted(o["id"] for o in payload["owners"]) == [100, 101] + assert payload["added_owner_ids"] == [7] + assert payload["removed_owner_ids"] == [] + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_remove_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + keep = _mock_subject(100, 1, "admin") + remove = _mock_subject(101, 2, "carol") + dash = _mock_dashboard(editors=[keep, remove]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: keep}[uid] + mock_populate.return_value = [keep] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [2]}}, + ) + + mock_populate.assert_called_once_with( + [100], default_to_user=False, ensure_no_lockout=True, field_name="editors" + ) + payload = json.loads(result.content[0].text) + assert payload["removed_owner_ids"] == [2] + assert [o["id"] for o in payload["owners"]] == [100] + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_all_owners_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + """Removing every owner must be rejected before any DB write.""" + original_editors = [_mock_subject(100, 1, "admin")] + dash = _mock_dashboard(editors=original_editors) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "at least one owner" in (payload.get("error") or "").lower() + # dashboard.editors must remain untouched + assert dash.editors is original_editors + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_unknown_owner_id_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [999]}}, + ) + + payload = json.loads(result.content[0].text) + assert "999" in (payload.get("error") or "") + assert "not currently owners" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 999999, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "not found" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_view_only_caller_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + """get_by_id_or_slug itself re-checks view access and raises + DashboardAccessDeniedError for dashboards the caller cannot see; + that must surface as permission_denied, not an unhandled error.""" + from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + ) + + mock_get.side_effect = DashboardAccessDeniedError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_non_owner_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.exceptions import SupersetSecurityException + + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + with patch( + "superset.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(Mock(message="forbidden")), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_unknown_user_id_in_add_rejected( + self, mock_get: Mock, mock_get_or_create: Mock, mcp_server: object + ) -> None: + existing = _mock_subject(100, 1, "admin") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: existing if uid == 1 else None Review Comment: **Suggestion:** Refactor this lambda into a typed helper callable so the parameter and return contract are explicit. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> The code defines an untyped lambda in new test code. Since the custom rule flags Python code that omits type hints where they can be added, this is a valid violation. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=38bf40074e12462e9112a53613123c04&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=38bf40074e12462e9112a53613123c04&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py **Line:** 250:250 **Comment:** *Custom Rule: Refactor this lambda into a typed helper callable so the parameter and return contract are explicit. 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=d3d5db619635e6d178ab1847ffba980e5dfb0a7a926c1bb7c682c2248bf8fd2e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=d3d5db619635e6d178ab1847ffba980e5dfb0a7a926c1bb7c682c2248bf8fd2e&reaction=dislike'>π</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py: ########## @@ -0,0 +1,469 @@ +# 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_owners MCP tool. + +"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the +Subject-based model apache/superset#38831 introduced, replacing the legacy +``owners`` relationship). +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.subjects.types import SubjectType +from superset.utils import json + +DAO_GET: str = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" +GET_OR_CREATE_USER_SUBJECT: str = "superset.subjects.utils.get_or_create_user_subject" +POPULATE_SUBJECT_LIST: str = "superset.commands.utils.populate_subject_list" + + +def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock: + subject = Mock() + subject.id = id + subject.type = SubjectType.USER + subject.user_id = user_id + subject.role_id = None + subject.label = label + subject.active = True + return subject + + +def _mock_dashboard( + id: int = 42, + title: str = "Test Dashboard", + slug: str | None = "test-slug", + editors: list[Mock] | None = None, +) -> Mock: + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = slug + dashboard.editors = ( + editors if editors is not None else [_mock_subject(100, 1, "admin")] + ) + return dashboard + + +class TestManageDashboardOwners: + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_add_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + existing = _mock_subject(100, 1, "admin") + new_owner = _mock_subject(101, 7, "bob") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: existing, 7: new_owner}[uid] + mock_populate.return_value = [existing, new_owner] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + mock_populate.assert_called_once_with( + [100, 101], + default_to_user=False, + ensure_no_lockout=True, + field_name="editors", + ) + assert dash.editors == [existing, new_owner] + assert mock_session.commit.call_count >= 1 + payload = json.loads(result.content[0].text) + assert sorted(o["id"] for o in payload["owners"]) == [100, 101] + assert payload["added_owner_ids"] == [7] + assert payload["removed_owner_ids"] == [] + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_remove_owner( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + keep = _mock_subject(100, 1, "admin") + remove = _mock_subject(101, 2, "carol") + dash = _mock_dashboard(editors=[keep, remove]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {1: keep}[uid] + mock_populate.return_value = [keep] + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [2]}}, + ) + + mock_populate.assert_called_once_with( + [100], default_to_user=False, ensure_no_lockout=True, field_name="editors" + ) + payload = json.loads(result.content[0].text) + assert payload["removed_owner_ids"] == [2] + assert [o["id"] for o in payload["owners"]] == [100] + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_all_owners_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + """Removing every owner must be rejected before any DB write.""" + original_editors = [_mock_subject(100, 1, "admin")] + dash = _mock_dashboard(editors=original_editors) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "at least one owner" in (payload.get("error") or "").lower() + # dashboard.editors must remain untouched + assert dash.editors is original_editors + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_remove_unknown_owner_id_rejected( + self, mock_get: Mock, mcp_server: object + ) -> None: + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "remove_owner_ids": [999]}}, + ) + + payload = json.loads(result.content[0].text) + assert "999" in (payload.get("error") or "") + assert "not currently owners" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_dashboard_not_found( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.commands.dashboard.exceptions import DashboardNotFoundError + + mock_get.side_effect = DashboardNotFoundError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 999999, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert "not found" in (payload.get("error") or "").lower() + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_view_only_caller_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + """get_by_id_or_slug itself re-checks view access and raises + DashboardAccessDeniedError for dashboards the caller cannot see; + that must surface as permission_denied, not an unhandled error.""" + from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + ) + + mock_get.side_effect = DashboardAccessDeniedError() + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [1]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_non_owner_gets_permission_denied( + self, mock_get: Mock, mcp_server: object + ) -> None: + from superset.exceptions import SupersetSecurityException + + dash = _mock_dashboard(editors=[_mock_subject(100, 1, "admin")]) + mock_get.return_value = dash + + with patch( + "superset.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(Mock(message="forbidden")), + ): + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [7]}}, + ) + + payload = json.loads(result.content[0].text) + assert payload.get("permission_denied") is True + + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @pytest.mark.asyncio + async def test_unknown_user_id_in_add_rejected( + self, mock_get: Mock, mock_get_or_create: Mock, mcp_server: object + ) -> None: + existing = _mock_subject(100, 1, "admin") + dash = _mock_dashboard(editors=[existing]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: existing if uid == 1 else None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_owners", + {"request": {"identifier": 42, "add_owner_ids": [99999]}}, + ) + + payload = json.loads(result.content[0].text) + assert "not exist" in (payload.get("error") or "").lower() + assert "find_users" in (payload.get("error") or "") + + @patch(POPULATE_SUBJECT_LIST) + @patch(GET_OR_CREATE_USER_SUBJECT) + @patch(DAO_GET) + @patch("superset.extensions.db.session") + @pytest.mark.asyncio + async def test_self_removal_auto_readded_warns( + self, + mock_session: Mock, + mock_get: Mock, + mock_get_or_create: Mock, + mock_populate: Mock, + mcp_server: object, + ) -> None: + """When populate_subject_list re-adds a caller who tried to remove + themselves (ensure_no_lockout self-protection), the response + surfaces a warning instead of silently diverging from what was + requested.""" + admin = _mock_subject(100, 1, "admin") + carol = _mock_subject(101, 2, "carol") + dave = _mock_subject(102, 3, "dave") + dash = _mock_dashboard(editors=[admin, carol, dave]) + mock_get.return_value = dash + mock_get_or_create.side_effect = lambda uid: {3: dave}[uid] Review Comment: **Suggestion:** Replace this lambda with a typed function definition to satisfy type-hint requirements for callable behavior. [custom_rule] **Severity Level:** Minor π§Ή <details> <summary><b>Why it matters? β </b></summary> This lambda is untyped and appears in newly added Python code. That matches the custom type-hint ruleβs target, so the issue is real. </details> <details> <summary><b>Rule source π </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e1f4dda0484540718d7a9f0fba92e882&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=e1f4dda0484540718d7a9f0fba92e882&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:** tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_owners.py **Line:** 284:284 **Comment:** *Custom Rule: Replace this lambda with a typed function definition to satisfy type-hint requirements for callable behavior. 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=85000445163ac8f582daae0a4377dcc8a4c26f50b48c7d0069773ff3b359b7ff&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41606&comment_hash=85000445163ac8f582daae0a4377dcc8a4c26f50b48c7d0069773ff3b359b7ff&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]
