aminghadersohi commented on code in PR #41606: URL: https://github.com/apache/superset/pull/41606#discussion_r3589210793
########## 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: Fixed: added an explicit type annotation. ########## 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: Fixed: added an explicit type annotation. ########## 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: Fixed: added an explicit type annotation. ########## 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: Fixed: added an explicit type annotation. ########## 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: Fixed: extracted a typed `_get_or_create_side_effect()` helper and replaced all the untyped inline lambdas in this file with it (also cuts the repeated mapping-lookup boilerplate across the 6 call sites). ########## 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: Fixed: extracted a typed `_get_or_create_side_effect()` helper and replaced all the untyped inline lambdas in this file with it (also cuts the repeated mapping-lookup boilerplate across the 6 call sites). ########## 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: Fixed: extracted a typed `_get_or_create_side_effect()` helper and replaced all the untyped inline lambdas in this file with it (also cuts the repeated mapping-lookup boilerplate across the 6 call sites). -- 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]
