feat(mcp): add list and get tools for row level security and plugins#40306
Closed
aminghadersohi wants to merge 5 commits into
Closed
feat(mcp): add list and get tools for row level security and plugins#40306aminghadersohi wants to merge 5 commits into
aminghadersohi wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40306 +/- ##
==========================================
- Coverage 64.14% 63.83% -0.32%
==========================================
Files 2592 2601 +9
Lines 138846 140231 +1385
Branches 32201 32496 +295
==========================================
+ Hits 89069 89520 +451
- Misses 48245 49146 +901
- Partials 1532 1565 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aminghadersohi
force-pushed
the
mcp-rls-plugins-99978
branch
3 times, most recently
from
215873f to
f472e23
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…er roles to be returned RLS filter `roles` (which roles a filter applies to) are core RLS data, not user-directory metadata. Including 'roles' in USER_DIRECTORY_FIELDS caused filter_user_directory_columns() to strip it from any requested select_columns list, making it impossible to retrieve via list_rls_filters. No dashboard/chart/dataset schema defines a 'roles' field, so removing it from the block set has no privacy impact on other tools. Fixes test_list_rls_filters_returns_tables_and_roles.
…in RLS list tool 'roles' on a dashboard/chart exposes who has access to the resource and should be stripped by the USER_DIRECTORY_FIELDS privacy filter. 'roles' in an RLS filter is which roles the filter applies to — it is core filter data, not user-directory metadata. The RLS list tool now derives its column selection directly from ALL_RLS_COLUMNS (bypassing ModelListCore's USER_DIRECTORY_FIELDS filtering) so that RLS roles are selectable while dashboard roles remain hidden. Fixes three failing unit tests: - test_list_dashboards_omits_requested_user_directory_fields - test_get_allowed_fields_always_denies_user_directory_fields - test_filter_sensitive_data_strips_user_directory_fields_even_if_allowed Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
aminghadersohi
force-pushed
the
mcp-rls-plugins-99978
branch
from
f472e23 to
6e3a325
Compare
dosubot
Bot
added
api
Contributor
Author
|
Closing in favor of a correctly-named branch (amin/mcp-rls-plugins). |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #213174
Actionable Suggestions - 1
-
superset/mcp_service/rls/schemas.py - 1
- Missing pyright ignore comment · Line 64-64
Additional Suggestions - 4
-
tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py - 3
-
Incomplete test assertions · Line 78-92Test `test_list_plugins_basic` creates a plugin with `bundle_url` but doesn't assert this field. Per BITO.md rule [6262], tests should verify all business logic fields, not just subset. Adding this assertion ensures the field is correctly serialized in list responses.
Code suggestion
--- tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py (lines 78-92) --- 78: +@patch("superset.mcp_service.plugin.dao.DynamicPluginDAO.list") 79: +@pytest.mark.asyncio 80: +async def test_list_plugins_basic(mock_list, mcp_server): 81: + plugin = create_mock_plugin() 82: + mock_list.return_value = ([plugin], 1) 83: + 84: + async with Client(mcp_server) as client: 85: + result = await client.call_tool("list_plugins", {}) 86: + data = json.loads(result.content[0].text) 87: + assert "plugins" in data 88: + assert len(data["plugins"]) == 1 89: + assert data["plugins"][0]["id"] == 1 90: + assert data["plugins"][0]["name"] == "My Plugin" 91: + assert data["plugins"][0]["key"] == "my_plugin" 92: + assert data["plugins"][0]["bundle_url"] == "https://example.com/plugin.js" -
Incomplete test assertions · Line 94-108Test `test_list_plugins_with_request` only checks count fields but doesn't verify the actual plugin data is present. Per BITO.md rule [6262], tests should verify complete behavior including data content, not just metadata like counts.
Code suggestion
--- tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py (lines 94-108) --- 94: +@patch("superset.mcp_service.plugin.dao.DynamicPluginDAO.list") 95: +@pytest.mark.asyncio 96: +async def test_list_plugins_with_request(mock_list, mcp_server): 97: + plugin = create_mock_plugin() 98: + mock_list.return_value = ([plugin], 1) 99: + 100: + async with Client(mcp_server) as client: 101: + request = ListPluginsRequest(page=1, page_size=10) 102: + result = await client.call_tool( 103: + "list_plugins", {"request": request.model_dump()} 104: + ) 105: + data = json.loads(result.content[0].text) 106: + assert data["count"] == 1 107: + assert data["total_count"] == 1 108: + assert len(data["plugins"]) == 1 -
Incomplete test assertions · Line 110-123Test `test_list_plugins_with_search` only checks plugin name but doesn't verify other fields like id or key. Per BITO.md rule [6262], comprehensive assertions ensure search filtering doesn't corrupt other fields.
Code suggestion
--- tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py (lines 110-123) --- 110: +@patch("superset.mcp_service.plugin.dao.DynamicPluginDAO.list") 111: +@pytest.mark.asyncio 112: +async def test_list_plugins_with_search(mock_list, mcp_server): 113: + plugin = create_mock_plugin(name="Custom Chart") 114: + mock_list.return_value = ([plugin], 1) 115: + 116: + async with Client(mcp_server) as client: 117: + request = ListPluginsRequest(page=1, page_size=10, search="custom") 118: + result = await client.call_tool( 119: + "list_plugins", {"request": request.model_dump()} 120: + ) 121: + data = json.loads(result.content[0].text) 122: + assert data["plugins"][0]["name"] == "Custom Chart" 123: + assert data["plugins"][0]["id"] == 1
-
-
superset/mcp_service/privacy.py - 1
-
Docstring omits filtered field · Line 143-143The docstring no longer mentions 'roles' but `USER_DIRECTORY_FIELDS` at line 43 still includes `"roles"` in the frozenset, and the function filters it out. This creates a documentation/implementation mismatch that will mislead developers about the function's actual behavior.
Code suggestion
--- superset/mcp_service/privacy.py (lines 140-146) --- 142: def filter_user_directory_fields(data: dict[str, Any]) -> dict[str, Any]: 143: - """Remove fields that expose users, owners, or access metadata.""" 143: + """Remove fields that expose users, roles, owners, or access metadata.""" 144: return { 145: key: value for key, value in data.items() if key not in USER_DIRECTORY_FIELDS 146: }
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/mcp_service/plugin/tool/get_plugin_info.py - 1
- Blind exception catch too broad · Line 92-92
-
superset/mcp_service/rls/tool/get_rls_filter_info.py - 1
- Avoid catching blind Exception · Line 92-92
Review Details
-
Files reviewed - 19 · Commit Range:
10ea6da..6e3a325- superset/mcp_service/app.py
- superset/mcp_service/plugin/__init__.py
- superset/mcp_service/plugin/dao.py
- superset/mcp_service/plugin/schemas.py
- superset/mcp_service/plugin/tool/__init__.py
- superset/mcp_service/plugin/tool/get_plugin_info.py
- superset/mcp_service/plugin/tool/list_plugins.py
- superset/mcp_service/privacy.py
- superset/mcp_service/rls/__init__.py
- superset/mcp_service/rls/schemas.py
- superset/mcp_service/rls/tool/__init__.py
- superset/mcp_service/rls/tool/get_rls_filter_info.py
- superset/mcp_service/rls/tool/list_rls_filters.py
- tests/unit_tests/mcp_service/plugin/__init__.py
- tests/unit_tests/mcp_service/plugin/tool/__init__.py
- tests/unit_tests/mcp_service/plugin/tool/test_plugin_tools.py
- tests/unit_tests/mcp_service/rls/__init__.py
- tests/unit_tests/mcp_service/rls/tool/__init__.py
- tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| class RlsColumnFilter(ColumnOperator): | ||
| """Filter object for RLS filter listing.""" | ||
|
|
||
| col: Literal["name", "filter_type"] = Field( |
Contributor
There was a problem hiding this comment.
Missing pyright ignore comment
The col field overrides ColumnOperator.col with a Literal type but lacks the pyright ignore comment present in all other filter classes (DashboardFilter, DatasetFilter, DatabaseFilter, ChartFilter). This causes pyright type-checking warnings.
Code suggestion
Check the AI-generated fix before applying
--- superset/mcp_service/rls/schemas.py
+++ superset/mcp_service/rls/schemas.py
@@ -64,6 +64,6 @@
"""
- col: Literal["name", "filter_type"] = Field(
+ col: Literal["name", "filter_type"] = Field( # pyright: ignore[reportIncompatibleVariableOverride]
...,
description="Column to filter on.",
)
Code Review Run #213174
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Adds 4 new MCP tools across two new domains as part of story #99978 (Add list and info MCP tools for all accessible menu features):
Row Level Security (
superset/mcp_service/rls/)list_rls_filters— List RLS filters with filtering (byname,filter_type), search, pagination, and column selection. Admin access required.get_rls_filter_info— Get full RLS filter details by ID, including tables, roles, and WHERE clause. Admin access required.Dynamic Plugins (
superset/mcp_service/plugin/)list_plugins— List dynamic plugins with filtering (byname,key), search, pagination, and column selection. Admin access required.get_plugin_info— Get full plugin details by ID, including key and bundle URL. Admin access required.Both domains follow the established pattern:
ModelListCore/ModelGetInfoCore, Pydantic schemas with column filtering via serialization context,@tooldecorator with RBAC viaclass_permission_name, andevent_loggerinstrumentation.A minimal
DynamicPluginDAOis co-located insuperset/mcp_service/plugin/dao.pysince no top-level DAO exists forDynamicPlugin.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only MCP tool additions.
TESTING INSTRUCTIONS
Run the new unit tests:
ADDITIONAL INFORMATION