fix(mcp): serialize dashboard role permissions#41404
Conversation
dosubot
Bot
added
authentication:RBAC
Code Review Agent Run #378ac8Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41404 +/- ##
==========================================
- Coverage 64.44% 64.42% -0.02%
==========================================
Files 2655 2655
Lines 145491 145524 +33
Branches 33585 33589 +4
==========================================
- Hits 93762 93759 -3
- Misses 50028 50062 +34
- Partials 1701 1703 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #7d4856Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| assert "slug" in data["columns_loaded"] | ||
|
|
||
|
|
||
| def test_dashboard_role_serializer_serializes_permission_view_names() -> None: |
There was a problem hiding this comment.
Suggestion: Add a short docstring to this new test function describing the serializer behavior it validates. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python test function and it has no docstring. The custom rule requires new functions to be documented inline, so the suggestion identifies a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 140:140
**Comment:**
*Custom Rule: Add a short docstring to this new test function describing the serializer behavior it validates.
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| } | ||
|
|
||
|
|
||
| def test_dashboard_role_serializer_skips_bad_permission_items() -> None: |
There was a problem hiding this comment.
Suggestion: Add a docstring to this new test function so its edge-case intent is explicitly documented. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python test function and there is no docstring immediately under it. That violates the rule requiring new functions to include docstrings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 157:157
**Comment:**
*Custom Rule: Add a docstring to this new test function so its edge-case intent is explicitly documented.
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|
|
||
|
|
||
| def test_dashboard_role_serializer_skips_bad_permission_items() -> None: | ||
| class DetachedPermission: |
There was a problem hiding this comment.
Suggestion: Add a brief class docstring explaining the purpose of this helper class used in the test. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly introduced helper class does not have a docstring. The custom rule explicitly requires new classes to be documented inline.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 158:158
**Comment:**
*Custom Rule: Add a brief class docstring explaining the purpose of this helper class used in the test.
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|
|
||
|
|
||
| def test_dashboard_role_serializer_handles_detached_permissions() -> None: | ||
| class DetachedRole: |
There was a problem hiding this comment.
Suggestion: Add a class docstring to describe why this detached-role test double exists. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added helper class inside a test function and it lacks a docstring. That matches the custom rule for new classes needing inline documentation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 184:184
**Comment:**
*Custom Rule: Add a class docstring to describe why this detached-role test double exists.
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|
|
||
|
|
||
| def test_dashboard_role_serializer_stops_on_detached_permission_iterator() -> None: | ||
| class DetachedPermissionIterator: |
There was a problem hiding this comment.
Suggestion: Add a short docstring to this iterator helper class to clarify the failure scenario it simulates. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new helper class also lacks a docstring. Since the rule requires newly added classes to be documented, the suggestion is valid.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py
**Line:** 208:208
**Comment:**
*Custom Rule: Add a short docstring to this iterator helper class to clarify the failure scenario it simulates.
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
Code Review Agent Run #214208Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Fix MCP dashboard role permission serialization to handle Flask-AppBuilder
PermissionViewobjects safely.Previously, dashboard role serialization assumed permissions were simple objects with a direct
nameattribute. FAB role permissions areoften represented as
PermissionViewobjects, where the effective permission name is composed frompermission.nameandview_menu.namesuch as
can_read on Dashboard. This could cause MCP dashboard serialization to fail when role permission data was accessed.This change makes role permission serialization defensive by:
PermissionViewobjectsRelated to #41001
AS-IS
name.PermissionViewobjects were not serialized into the expected permission string format.rolesremain stripped from dashboard output.TO-BE
can_read on Dashboard.rolesdoes not expose role data in dashboard list responses.TESTING INSTRUCTIONS
Verified locally with Docker using the current branch build.
docker compose -f docker-compose-light.yml up --build -d superset-light 2. Started the MCP server from the built image on port 5008. 3. Called the actual MCP HTTP endpoint with FastMCP client and requested dashboard fields including roles: await client.call_tool( "list_dashboards", { "request": { "page": 1, "page_size": 3, "select_columns": ["id", "dashboard_title", "roles"], } }, )Result:
Observed response summary:
{ "count": 3, "columns_requested": ["id", "dashboard_title"], "columns_loaded": ["id", "dashboard_title"], "columns_available_has_roles": false, "first_dashboard_keys": ["dashboard_title", "id"] }Also verified the tool-search proxy path:
Static validation:
ADDITIONAL INFORMATION
This PR does not re-expose dashboard owners or roles in MCP dashboard list responses. Those fields are intentionally stripped by the current MCP privacy policy.