feat(mcp): expose current user identity in get_instance_info and add created_by_fk filter#37967
feat(mcp): expose current user identity in get_instance_info and add created_by_fk filter#37967aminghadersohi wants to merge 3 commits intoapache:masterfrom
Conversation
…created_by_fk filter Add current_user field to get_instance_info response so the LLM chatbot knows who the authenticated user is. Add created_by_fk as a filter column to list_charts and list_dashboards so users can find their own assets. Also fixes parse_request decorator checking MCP_PARSE_REQUEST_ENABLED at decoration time instead of call time, and always accepts str in annotations.
Code Review Agent Run #59c89aActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # Simulate no user on g | ||
| del mock_g.user |
There was a problem hiding this comment.
Suggestion: The test that simulates the absence of g.user uses del mock_g.user on a MagicMock, but getattr(mock_g, "user", None) will still return a new Mock instead of None, so the production code will think a user exists and try to build a UserInfo from a Mock, causing validation errors and making the test not actually cover the "no user" case. Replace the deletion with explicitly setting mock_g.user = None so getattr(g, "user", None) correctly returns None and current_user remains None as asserted. [logic error]
Severity Level: Major ⚠️
- ❌ Unit test fails with AttributeError in local and CI runs.
- ⚠️ Intended "no current user" behavior remains unverified.| # Simulate no user on g | |
| del mock_g.user | |
| # Simulate no user on g by explicitly setting user to None | |
| mock_g.user = None |
Steps of Reproduction ✅
1. Run the specific unit test with the PR code: `pytest
tests/unit_tests/mcp_service/system/tool/test_get_current_user.py::TestGetInstanceInfoCurrentUserViaMCP::test_get_instance_info_no_user_returns_null`.
2. The test enters
`TestGetInstanceInfoCurrentUserViaMCP.test_get_instance_info_no_user_returns_null` at
`tests/unit_tests/mcp_service/system/tool/test_get_current_user.py:239`, where
`InstanceInfoCore.run_tool` is patched and `with patch("flask.g") as mock_g:` is executed
at line 246.
3. At line 248, `del mock_g.user` is executed on the `MagicMock` instance returned by
`patch("flask.g")`. Since `mock_g.user` has never been set on this `MagicMock`, its
`__delattr__` implementation raises `AttributeError: Mock object has no attribute 'user'`.
4. The test fails with an error before the MCP `Client` call at lines 250–251 can execute,
so the code path that should handle "no user on g" (the behavior this test claims to
verify) is never reached. The suggested change to `mock_g.user = None` avoids the
`AttributeError` and allows the test to run while still causing `getattr(g, "user",
None)`-style access in the production code to return `None`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/system/tool/test_get_current_user.py
**Line:** 247:248
**Comment:**
*Logic Error: The test that simulates the absence of `g.user` uses `del mock_g.user` on a MagicMock, but `getattr(mock_g, "user", None)` will still return a new Mock instead of `None`, so the production code will think a user exists and try to build a `UserInfo` from a Mock, causing validation errors and making the test not actually cover the "no user" case. Replace the deletion with explicitly setting `mock_g.user = None` so `getattr(g, "user", None)` correctly returns `None` and `current_user` remains `None` as asserted.
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.Address code review feedback: use mock_g.user = None instead of del mock_g.user for clearer intent in test_get_instance_info_no_user_returns_null.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #37967 +/- ##
===========================================
+ Coverage 0 66.42% +66.42%
===========================================
Files 0 668 +668
Lines 0 51328 +51328
Branches 0 5777 +5777
===========================================
+ Hits 0 34096 +34096
- Misses 0 15846 +15846
- Partials 0 1386 +1386
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:
|
The MCP client integration tests used a dotted string path to patch InstanceInfoCore.run_tool, but __init__.py re-exports get_instance_info as a function, which shadows the submodule name and breaks mock resolution on Python 3.10. Switch to patch.object(InstanceInfoCore, ...) which patches the method on the class and works across all versions.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Good catch — applied the fix in 35f2f3b. Using mock_g.user = None is clearer and avoids any MagicMock __delattr__ subtlety. Thanks!
(Responding on behalf of @aminghadersohi)
Code Review Agent Run #56dcd2Actionable 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 |
1 participant
SUMMARY
The MCP chatbot didn't know who the current user was. When asked "what's the chart I created most recently?", it queried all charts and assumed the user was whoever created the top result (e.g., confused one user with another).
This PR adds:
current_userfield toget_instance_inforesponse - exposes authenticated user identity (id, username, first_name, last_name, email) so the LLM knows who it's talking tocreated_by_fkfilter column onlist_chartsandlist_dashboards- lets the LLM filter assets by creatorparse_requestdecorator bug - was evaluatingMCP_PARSE_REQUEST_ENABLEDat decoration time instead of call time, causing string/dict parsing to silently fail when config was FalseArchitecture / Flow
Before (broken):
After (fixed):
Data Flow Diagram:
parse_requestdecorator fix:BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - backend/API only change
TESTING INSTRUCTIONS
Automated Tests (18 new tests):
QA Steps:
Verify current_user in get_instance_info:
get_instance_infovia MCP clientcurrent_userwithid,username,first_name,last_name,emailVerify created_by_fk filter on charts:
get_instance_info→current_user.idlist_chartswith filter:[{"col": "created_by_fk", "opr": "eq", "value": <your_id>}]Verify created_by_fk filter on dashboards:
list_dashboardsVerify chatbot behavior (end-to-end):
Verify parse_request fix:
MCP_PARSE_REQUEST_ENABLED = Falsein configget_schemawith a JSON string requestADDITIONAL INFORMATION
current_useris added to existingget_instance_inforesponselist_userstool (privacy) - users can only discover their own identitycreated_by_fkis an additional convenience filterparse_requestfix is unrelated but was discovered during testing (decorator was caching config at decoration time)