You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/04/24 20:27:44 UTC
[PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
eschutho opened a new pull request, #39638:
URL: https://github.com/apache/superset/pull/39638
### SUMMARY
PR #39576 removed `created_by_fk` as a filterable column on charts, dashboards, datasets, and databases. This broke the "find my own content" workflow — MCP clients can no longer ask the LLM to show them their own charts or dashboards.
This PR restores the capability while preserving the privacy goal. `created_by_fk` is re-added to the filter schemas, but `ModelListCore.run_tool` now validates that any `created_by_fk` filter value equals the current authenticated user's ID. Attempting to filter by another user's ID raises a `ValueError`.
**Before:** Filtering by `created_by_fk` was removed entirely, making self-lookup impossible.
**After:** `created_by_fk` filtering works for the current user's own ID only. Attempting to enumerate another user's content is rejected at the tool layer.
### TESTING INSTRUCTIONS
```bash
pytest \
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py \
tests/unit_tests/mcp_service/system/tool/test_get_current_user.py -q
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [x] Removes existing feature or API
Fixes the regression introduced by #39576.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4316265569
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39638?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `10.00000%` with `18 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.67%. Comparing base ([`edf4d03`](https://app.codecov.io/gh/apache/superset/commit/edf4d0321885a666404a5024395abf254e3dee6c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`dd04818`](https://app.codecov.io/gh/apache/superset/commit/dd04818297afe994d01e6bd7c8e9fd0dca01a305?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39638?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/mcp\_core.py](https://app.codecov.io/gh/apache/superset/pull/39638?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fmcp_core.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvbWNwX2NvcmUucHk=) | 10.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39638?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39638 +/- ##
==========================================
- Coverage 64.49% 63.67% -0.82%
==========================================
Files 2565 2565
Lines 133857 133877 +20
Branches 31084 31089 +5
==========================================
- Hits 86332 85249 -1083
- Misses 46030 47115 +1085
- Partials 1495 1513 +18
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.78% <10.00%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.55% <10.00%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.08% <10.00%> (-1.83%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.95% <10.00%> (-0.02%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39638/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39638?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4317419273
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 6b39e37a45e54f575b445d3568590162c066c605 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69ec1a09b23aca00087bf7db |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39638--superset-docs-preview.netlify.app](https://deploy-preview-39638--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39638--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3140148055
##########
superset/mcp_service/mcp_core.py:
##########
@@ -174,6 +174,38 @@ def _validate_order_column(self, order_column: str | None) -> None:
f"Allowed columns: {', '.join(self._sortable_columns)}"
)
+ @staticmethod
+ def _validate_created_by_fk_filters(filters: Any) -> None:
+ """Ensure created_by_fk filters only reference the current user's ID.
+
+ This allows self-lookup ("find my own charts/dashboards") while
+ preventing enumeration of other users' content.
+ """
+ if not filters:
+ return
+
+ from flask_login import current_user
+
+ filter_list = filters if isinstance(filters, list) else [filters]
+ for f in filter_list:
+ col = f.get("col") if isinstance(f, dict) else getattr(f, "col", None)
+ if col != "created_by_fk":
+ continue
+ value = f.get("value") if isinstance(f, dict) else getattr(f, "value", None)
+ if not current_user or not current_user.is_authenticated:
+ raise ValueError(
+ "created_by_fk filter requires an authenticated user"
+ )
+ try:
+ if int(value) != current_user.id:
+ raise ValueError(
+ "created_by_fk filter can only be used with your own user ID"
+ )
+ except (TypeError, ValueError) as exc:
+ raise ValueError(
+ "created_by_fk filter value must be a valid user ID"
+ ) from exc
Review Comment:
**Suggestion:** The `except (TypeError, ValueError)` block is too broad and catches the intentional `ValueError` raised for "other user's ID", so that case is incorrectly rewritten as "must be a valid user ID". This breaks the intended authorization error path and will fail callers/tests expecting the "own user ID" rejection. Narrow the `try/except` to only the `int(value)` conversion and keep the ownership mismatch error outside that `except`. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test for other-user filter rejection fails consistently.
- ⚠️ MCP list tools misreport authorization failure as invalid user ID.
- ⚠️ Error messaging no longer distinguishes permission vs format failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the PR's recommended tests, including
`tests/unit_tests/mcp_service/system/tool/test_mcp_core.py`, e.g. `pytest
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py -q`.
2. In `test_model_list_tool_rejects_created_by_fk_filter_for_other_user` at
`tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:206`, `current_user` is patched
(Mock with `is_authenticated=True` and `id=42`) and a `ModelListCore` instance is created.
3. The test calls `tool.run_tool(filters=[{"col": "created_by_fk", "opr": "eq", "value":
99}])` (same file around lines 225–231); this invokes `ModelListCore.run_tool` at
`superset/mcp_service/mcp_core.py:209`, which parses filters and then calls
`self._validate_created_by_fk_filters(filters)` at `mcp_core.py:231`.
4. Inside `_validate_created_by_fk_filters` at `superset/mcp_service/mcp_core.py:178–207`,
the filter with `col == "created_by_fk"` is processed: the `try` block at lines 199–203
calls `int(value)` and then raises `ValueError("created_by_fk filter can only be used with
your own user ID")` when `int(99) != current_user.id (42)`. Because the `except
(TypeError, ValueError)` at lines 204–207 wraps both the conversion and the comparison, it
also catches this intentional `ValueError` and re-raises `ValueError("created_by_fk filter
value must be a valid user ID")`. The test's `pytest.raises(ValueError, match="own user
ID")` (test file line 228) therefore fails, demonstrating that the broad `except` block is
incorrectly masking the authorization failure as a generic validation error.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmcp_core.py%0A%2A%2ALine%3A%2A%2A%20199%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20%60except%20%28TypeError%2C%20ValueError%29%60%20block%20is%20too%20broad%20and%20catches%20the%20intentional%20%60ValueError%60%20raised%20for%20%22other%20user%27s%20ID%22%2C%20so%20that%20case%20is%20incorrectly%20rewritten%20as%20%22must%20be%20a%20valid%20user%20ID%22.%20This%20breaks%20the%20intended%20authorization%20error%20path%20and%20will%20fail%20callers%2Ftests%20expecting%20the%20%22own%20user%20ID%22%20rejection.%20Narrow%20the%20%60try%2Fexcept%60%20to%20only%20the%20%60int%28value%29%60%20conversion%20and%20keep%20the%20ownership%20mismatch%20error%20outside%20that%20%60except%60.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20reso
lve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmcp_core.py%0A%2A%2ALine%3A%2A%2A%20199%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20%60except%20%28TypeError%2C%20ValueError%29%60%20block%20is%20too%20broad%20and%20catches%20the%20intentional%20%60ValueError%60%20raised%20for%20%22other%20user%27s%20ID%22%2C%20so%20that%20case%20is%20incorrectly%20rewritten%20as%20%22must%20be%20a%20valid%20
user%20ID%22.%20This%20breaks%20the%20intended%20authorization%20error%20path%20and%20will%20fail%20callers%2Ftests%20expecting%20the%20%22own%20user%20ID%22%20rejection.%20Narrow%20the%20%60try%2Fexcept%60%20to%20only%20the%20%60int%28value%29%60%20conversion%20and%20keep%20the%20ownership%20mismatch%20error%20outside%20that%20%60except%60.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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:** superset/mcp_service/mcp_core.py
**Line:** 199:207
**Comment:**
*Logic Error: The `except (TypeError, ValueError)` block is too broad and catches the intentional `ValueError` raised for "other user's ID", so that case is incorrectly rewritten as "must be a valid user ID". This breaks the intended authorization error path and will fail callers/tests expecting the "own user ID" rejection. Narrow the `try/except` to only the `int(value)` conversion and keep the ownership mismatch error outside that `except`.
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%2F39638&comment_hash=37996d96c1876e98f9c4a8a1d7e5c5480feb6fdf3cbaa9e592e9d29191bf1f0a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=37996d96c1876e98f9c4a8a1d7e5c5480feb6fdf3cbaa9e592e9d29191bf1f0a&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:
##########
@@ -177,6 +177,60 @@ def test_model_list_tool_allows_order_column_when_sortable_columns_not_declared(
tool.run_tool(order_column="name")
+def test_model_list_tool_allows_created_by_fk_filter_for_current_user():
+ """created_by_fk filter is accepted when the value matches the current user's ID."""
+ from unittest.mock import patch, Mock
+
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
Review Comment:
**Suggestion:** This patch target does not exist in `superset.mcp_service.mcp_core` because `current_user` is imported inside the validator method, not defined at module scope. `patch(...)` will raise `AttributeError` before the test reaches `run_tool`. Patch `flask_login.current_user` (or the lookup used by the method) instead so the test actually validates behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ `tests/unit_tests/mcp_service/system/tool/test_mcp_core.py` fails with AttributeError.
- ⚠️ created_by_fk privacy validation in ModelListCore remains untested.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/mcp_service/mcp_core.py` and locate `_validate_created_by_fk_filters` at
lines 177–207, where `current_user` is imported inside the method body via `from
flask_login import current_user` (line 187) and is never defined as a module-level
attribute.
2. Open `tests/unit_tests/mcp_service/system/tool/test_mcp_core.py` and find
`test_model_list_tool_allows_created_by_fk_filter_for_current_user` at lines 31–54 and
`test_model_list_tool_rejects_created_by_fk_filter_for_other_user` at lines 57–82; both
use `with patch("superset.mcp_service.mcp_core.current_user", current_user)` at lines
50–52 and 76–78 respectively.
3. Run the test file as recommended in the PR description, for example:
`pytest tests/unit_tests/mcp_service/system/tool/test_mcp_core.py -q`.
4. During collection/execution, `unittest.mock.patch` resolves the target
`"superset.mcp_service.mcp_core.current_user"` by importing the
`superset.mcp_service.mcp_core` module and looking for a `current_user` attribute; since
no such attribute exists (confirmed in step 1), `patch` raises `AttributeError: module
'superset.mcp_service.mcp_core' has no attribute 'current_user'` before
`ModelListCore.run_tool()` is ever called, so the tests never actually exercise the
created_by_fk filter validation.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20199%3A201%0A%2A%2AComment%3A%2A%2A%0A%09%2APossible%20Bug%3A%20This%20patch%20target%20does%20not%20exist%20in%20%60superset.mcp_service.mcp_core%60%20because%20%60current_user%60%20is%20imported%20inside%20the%20validator%20method%2C%20not%20defined%20at%20module%20scope.%20%60patch%28...%29%60%20will%20raise%20%60AttributeError%60%20before%20the%20test%20reaches%20%60run_tool%60.%20Patch%20%60flask_login.current_user%60%20%28or%20the%20lookup%20used%20by%20the%20method%29%20instead%20so%20the%20test%20actually%20validates%20behavior.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concis
e.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20199%3A201%0A%2A%2AComment%3A%2A%2A%0A%09%2APossible%20Bug%3A%20This%20patch%20target%20does%20not%20exist%20in%20%60superset.mcp_service.mcp_core%60%20because%20%60current_user%60%20is%20imported%20inside%20the%20validator%20method%2C%20not%20defined%20at%20module%20scope.%20%60patch%28...%29%60%20will%20raise%20%60AttributeError%60%20before%20the%20test%20reaches%20%60run_tool%60.%20Patch%20%60
flask_login.current_user%60%20%28or%20the%20lookup%20used%20by%20the%20method%29%20instead%20so%20the%20test%20actually%20validates%20behavior.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/system/tool/test_mcp_core.py
**Line:** 199:201
**Comment:**
*Possible Bug: This patch target does not exist in `superset.mcp_service.mcp_core` because `current_user` is imported inside the validator method, not defined at module scope. `patch(...)` will raise `AttributeError` before the test reaches `run_tool`. Patch `flask_login.current_user` (or the lookup used by the method) instead so the test actually validates 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%2F39638&comment_hash=f1c1efd3e8c6b05be1b88751b44038633941900a1cd1d1d879c81d82063c3312&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=f1c1efd3e8c6b05be1b88751b44038633941900a1cd1d1d879c81d82063c3312&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3140142509
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -58,10 +58,12 @@ class DatasetFilter(ColumnOperator):
"table_name",
"schema",
"database_name",
+ "created_by_fk",
] = Field(
...,
description="Column to filter on. Use get_schema(model_type='dataset') for "
- "available filter columns.",
+ "available filter columns. created_by_fk only accepts the current user's ID "
+ "(from get_instance_info's current_user.id).",
Review Comment:
**Suggestion:** Replace the newly added `created_by_fk` filter with a UUID-based creator identifier filter (and update its description accordingly) so the public API does not expose internal integer user IDs. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The added filter explicitly exposes `created_by_fk` and documents that it accepts the current user's integer ID from `current_user.id`. That matches the suggestion's concern about exposing an internal numeric identifier in the public API, so the violation is real.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2061%3A66%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Replace%20the%20newly%20added%20%60created_by_fk%60%20filter%20with%20a%20UUID-based%20creator%20identifier%20filter%20%28and%20update%20its%20description%20accordingly%29%20so%20the%20public%20API%20does%20not%20expose%20internal%20integer%20user%20IDs.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the
%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2061%3A66%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Replace%20the%20newly%20added%20%60created_by_fk%60%20filter%20with%20a%20UUID-based%20creator%20identifier%20filter%20%28and%20update%20its%20description%20accordingly%29%20so%20the%20public%20API%20does%20not%20expose%20internal%20integer%20user%20IDs.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20
fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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:** superset/mcp_service/dataset/schemas.py
**Line:** 61:66
**Comment:**
*Custom Rule: Replace the newly added `created_by_fk` filter with a UUID-based creator identifier filter (and update its description accordingly) so the public API does not expose internal integer user IDs.
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%2F39638&comment_hash=a87d2fbb5ac48c838302e6a351b2d5ac23c81b6589a2189d74aa55865cd7cbab&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=a87d2fbb5ac48c838302e6a351b2d5ac23c81b6589a2189d74aa55865cd7cbab&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:
##########
@@ -177,6 +177,60 @@ def test_model_list_tool_allows_order_column_when_sortable_columns_not_declared(
tool.run_tool(order_column="name")
+def test_model_list_tool_allows_created_by_fk_filter_for_current_user():
Review Comment:
**Suggestion:** Add an explicit return type hint to this newly added test function (for example, indicating it returns no value) to comply with the typing requirement for new Python functions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The newly added test function has no explicit return type annotation (`-> None`).
This matches the stated custom rule requiring explicit typing for new Python function definitions.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20180%3A180%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20an%20explicit%20return%20type%20hint%20to%20this%20newly%20added%20test%20function%20%28for%20example%2C%20indicating%20it%20returns%20no%20value%29%20to%20comply%20with%20the%20typing%20requirement%20for%20new%20Python%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20a
ll%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20180%3A180%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20an%20explicit%20return%20type%20hint%20to%20this%20newly%20added%20test%20function%20%28for%20example%2C%20indicating%20it%20returns%20no%20value%29%20to%20comply%20with%20the%20typing%20requirement%20for%20new%20Python%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user
%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/system/tool/test_mcp_core.py
**Line:** 180:180
**Comment:**
*Custom Rule: Add an explicit return type hint to this newly added test function (for example, indicating it returns no value) to comply with the typing requirement for new Python functions.
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%2F39638&comment_hash=89eaa29bdf9a5fe03ad6af288d4b8a45c175c3fd9a0d7cf83f9c2d8842b3daf4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=89eaa29bdf9a5fe03ad6af288d4b8a45c175c3fd9a0d7cf83f9c2d8842b3daf4&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:
##########
@@ -177,6 +177,60 @@ def test_model_list_tool_allows_order_column_when_sortable_columns_not_declared(
tool.run_tool(order_column="name")
+def test_model_list_tool_allows_created_by_fk_filter_for_current_user():
+ """created_by_fk filter is accepted when the value matches the current user's ID."""
+ from unittest.mock import patch, Mock
+
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
+ # Should not raise
+ tool.run_tool(filters=[{"col": "created_by_fk", "opr": "eq", "value": 42}])
+
+
+def test_model_list_tool_rejects_created_by_fk_filter_for_other_user():
Review Comment:
**Suggestion:** Add an explicit return type hint to this newly added test function (for example, indicating it returns no value) to comply with the typing requirement for new Python functions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The newly added test function is missing an explicit return type annotation.
That is a real violation of the custom typing rule described in the suggestion.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20206%3A206%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20an%20explicit%20return%20type%20hint%20to%20this%20newly%20added%20test%20function%20%28for%20example%2C%20indicating%20it%20returns%20no%20value%29%20to%20comply%20with%20the%20typing%20requirement%20for%20new%20Python%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20a
ll%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fsystem%2Ftool%2Ftest_mcp_core.py%0A%2A%2ALine%3A%2A%2A%20206%3A206%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20an%20explicit%20return%20type%20hint%20to%20this%20newly%20added%20test%20function%20%28for%20example%2C%20indicating%20it%20returns%20no%20value%29%20to%20comply%20with%20the%20typing%20requirement%20for%20new%20Python%20functions.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user
%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/system/tool/test_mcp_core.py
**Line:** 206:206
**Comment:**
*Custom Rule: Add an explicit return type hint to this newly added test function (for example, indicating it returns no value) to comply with the typing requirement for new Python functions.
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%2F39638&comment_hash=105e9eab71750fdd3c22ffaa8d8274394e72c821a89f43d8ec0fe8516bff214d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=105e9eab71750fdd3c22ffaa8d8274394e72c821a89f43d8ec0fe8516bff214d&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3140157284
##########
superset/mcp_service/database/schemas.py:
##########
@@ -59,10 +59,12 @@ class DatabaseFilter(ColumnOperator):
"database_name",
"expose_in_sqllab",
"allow_file_upload",
+ "created_by_fk",
Review Comment:
**Suggestion:** This re-introduces `created_by_fk` as a normal column filter, but without operator-level constraints it can be abused (`ne`, `nin`) to enumerate databases created by other users. Add enforcement that `created_by_fk` only accepts a direct equality match to the current user ID. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP database listing leaks other users' databases via filters.
- ⚠️ Undermines privacy constraints described in PR for MCP.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with MCP enabled and authenticate as user A; MCP model-list tooling is
wired through `ModelListCore` in `superset/mcp_service/system/tool/mcp_core.py` (verified
via tests in `tests/unit_tests/mcp_service/system/tool/test_mcp_core.py` that exercise
`ModelListCore.run_tool` with database filters).
2. Call the MCP list-databases tool (the Database model list core) with a filter payload
shaped like the tests (see filter schema in
`superset/mcp_service/database/schemas.py:50-75`), using:
- `col="created_by_fk"` (allowed by the `DatabaseFilter.col` Literal at line 62),
- `opr="ne"` (or `opr="nin"`, both supported by `ColumnOperatorEnum` used by
`DatabaseFilter.opr`),
- `value=<current_user.id>` for user A.
3. In `ModelListCore.run_tool` (in `superset/mcp_service/system/tool/mcp_core.py`), the
new privacy check added by this PR validates only that any `created_by_fk` filter `value`
equals `current_user.id`; because `value` is exactly user A's ID, this check passes and
the filter is accepted even though `opr` is `"ne"`/`"nin"`.
4. The filter is then converted into a SQLAlchemy condition for the Database model (via
generic ColumnOperator handling wired from `DatabaseFilter` into the query-building code
for database listing), producing `Database.created_by_fk != current_user.id` (or `NOT IN
{current_user.id}`), and the MCP tool response returns all databases created by users
other than A, allowing enumeration of other users' databases despite the intended
"self-only" restriction.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdatabase%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2062%3A62%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20This%20re-introduces%20%60created_by_fk%60%20as%20a%20normal%20column%20filter%2C%20but%20without%20operator-level%20constraints%20it%20can%20be%20abused%20%28%60ne%60%2C%20%60nin%60%29%20to%20enumerate%20databases%20created%20by%20other%20users.%20Add%20enforcement%20that%20%60created_by_fk%60%20only%20accepts%20a%20direct%20equality%20match%20to%20the%20current%20user%20ID.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%2
0user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdatabase%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2062%3A62%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20This%20re-introduces%20%60created_by_fk%60%20as%20a%20normal%20column%20filter%2C%20but%20without%20operator-level%20constraints%20it%20can%20be%20abused%20%28%60ne%60%2C%20%60nin%60%29%20to%20enumerate%20databases%20created%20by%20other%20users.%20Add%20enforcement%20that%20%60created_by_fk%60%20only%20accepts%20a%20direct%20equality%20match%20to%20the%20current%20user%20ID.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this
%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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:** superset/mcp_service/database/schemas.py
**Line:** 62:62
**Comment:**
*Security: This re-introduces `created_by_fk` as a normal column filter, but without operator-level constraints it can be abused (`ne`, `nin`) to enumerate databases created by other users. Add enforcement that `created_by_fk` only accepts a direct equality match to the current user ID.
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%2F39638&comment_hash=f76434946e9620f217395ceb509d31bb65acccc0986337c193c931442ceb7460&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=f76434946e9620f217395ceb509d31bb65acccc0986337c193c931442ceb7460&reaction=dislike'>👎</a>
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -58,10 +58,12 @@ class DatasetFilter(ColumnOperator):
"table_name",
"schema",
"database_name",
+ "created_by_fk",
Review Comment:
**Suggestion:** Adding `created_by_fk` as a generic filterable column introduces a data-leak path because callers can use inverse operators (for example `ne`) to list datasets not owned by the current user. Restrict `created_by_fk` to a strict equality check against the authenticated user ID and reject other operators/structures. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP list_datasets reveals datasets not owned by caller.
- ❌ Attackers enumerate other users' dataset metadata via filters.
- ⚠️ Same pattern affects chart and dashboard listing tools.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Authenticate to a Superset instance with MCP enabled and obtain your user ID via the
MCP help text in `superset/mcp_service/app.py:145-152` (step 1: `get_instance_info -> get
current_user.id`).
2. From an MCP client, call the `list_datasets` FastMCP tool defined in
`superset/mcp_service/dataset/tool/list_datasets.py:80-101`, passing a request like:
`{"filters": [{"col": "created_by_fk", "opr": "ne", "value": <current_user.id>}]}`,
where `<current_user.id>` is your own ID.
3. The request is parsed into `ListDatasetsRequest.filters` (schema in
`superset/mcp_service/dataset/schemas.py:218-229`) as a list of `DatasetFilter` objects
(`DatasetFilter` at `superset/mcp_service/dataset/schemas.py:49-75`), which explicitly
allows `col` `"created_by_fk"` and any `ColumnOperatorEnum` (including `"ne"`) as `opr`.
4. In `list_datasets`, a `ModelListCore` instance is created
(`superset/mcp_service/dataset/tool/list_datasets.py:159-171`) and `tool.run_tool` is
invoked with `filters=request.filters` (`lines 173-182`). Inside `ModelListCore.run_tool`
(`superset/mcp_service/mcp_core.py:209-231`), `filters` are parsed and
`_validate_created_by_fk_filters` is called (`mcp_core.py:178-207`): this function only
checks that `col == "created_by_fk"` and that `int(value) == current_user.id`, but it does
not inspect or restrict `opr`, so the `"ne"` operator is accepted as long as the value
equals your ID.
5. After validation, `ModelListCore.run_tool` calls `DatasetDAO.list` with
`column_operators=filters` (`superset/mcp_service/mcp_core.py:250-260`). `DatasetDAO`
inherits `BaseDAO`, which applies column operators via `BaseDAO.apply_column_operators`
(`superset/daos/base.py:477-549`): it constructs `ColumnOperatorEnum(opr)` and calls
`operator_enum.apply(column, value)`. For numeric columns like `created_by_fk`,
`TYPE_OPERATOR_MAP["number"]` (`superset/daos/base.py:123-134`) includes `ne`, so the
filter becomes `SqlaTable.created_by_fk != <current_user.id>`.
6. The resulting query returns all datasets whose `created_by_fk` is not your user ID; the
serialized response (`DatasetList` in `superset/mcp_service/dataset/schemas.py:184-215`)
is returned to the MCP client, allowing enumeration of other users' datasets via the
`list_datasets` tool despite the intended self-only restriction.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2061%3A61%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20Adding%20%60created_by_fk%60%20as%20a%20generic%20filterable%20column%20introduces%20a%20data-leak%20path%20because%20callers%20can%20use%20inverse%20operators%20%28for%20example%20%60ne%60%29%20to%20list%20datasets%20not%20owned%20by%20the%20current%20user.%20Restrict%20%60created_by_fk%60%20to%20a%20strict%20equality%20check%20against%20the%20authenticated%20user%20ID%20and%20reject%20other%20operators%2Fstructures.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20sa
me%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fdataset%2Fschemas.py%0A%2A%2ALine%3A%2A%2A%2061%3A61%0A%2A%2AComment%3A%2A%2A%0A%09%2ASecurity%3A%20Adding%20%60created_by_fk%60%20as%20a%20generic%20filterable%20column%20introduces%20a%20data-leak%20path%20because%20callers%20can%20use%20inverse%20operators%20%28for%20example%20%60ne%60%29%20to%20list%20datasets%20not%20owned%20by%20the%20current%20user.%20Restrict%20%60created_by_fk%60%20to%20a%20strict%20equality%20check%20against%20the%20authenticated%20user%20ID%20and%20reject%20other%20operators%2Fstructures.%0A%0AValidate%20the%20correctn
ess%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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:** superset/mcp_service/dataset/schemas.py
**Line:** 61:61
**Comment:**
*Security: Adding `created_by_fk` as a generic filterable column introduces a data-leak path because callers can use inverse operators (for example `ne`) to list datasets not owned by the current user. Restrict `created_by_fk` to a strict equality check against the authenticated user ID and reject other operators/structures.
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%2F39638&comment_hash=64c33c689714caffec454c95841e420eb6e998677e5dc0c820065995ad76560f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39638&comment_hash=64c33c689714caffec454c95841e420eb6e998677e5dc0c820065995ad76560f&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3140153763
##########
superset/mcp_service/mcp_core.py:
##########
@@ -174,6 +174,38 @@ def _validate_order_column(self, order_column: str | None) -> None:
f"Allowed columns: {', '.join(self._sortable_columns)}"
)
+ @staticmethod
+ def _validate_created_by_fk_filters(filters: Any) -> None:
+ """Ensure created_by_fk filters only reference the current user's ID.
+
+ This allows self-lookup ("find my own charts/dashboards") while
+ preventing enumeration of other users' content.
+ """
+ if not filters:
+ return
+
+ from flask_login import current_user
+
+ filter_list = filters if isinstance(filters, list) else [filters]
+ for f in filter_list:
+ col = f.get("col") if isinstance(f, dict) else getattr(f, "col", None)
+ if col != "created_by_fk":
+ continue
+ value = f.get("value") if isinstance(f, dict) else getattr(f, "value", None)
+ if not current_user or not current_user.is_authenticated:
+ raise ValueError(
+ "created_by_fk filter requires an authenticated user"
+ )
+ try:
+ if int(value) != current_user.id:
+ raise ValueError(
+ "created_by_fk filter can only be used with your own user ID"
+ )
+ except (TypeError, ValueError) as exc:
+ raise ValueError(
+ "created_by_fk filter value must be a valid user ID"
+ ) from exc
Review Comment:
**🟠 Architect Review — HIGH**
Unauthorized created_by_fk filter values are reported as "created_by_fk filter value must be a valid user ID" because the ValueError raised for a mismatched ID is thrown inside a try that catches ValueError, so the intended "own user ID" policy error is always replaced by the generic parse error message.
**Suggestion:** Separate parsing from authorization: first parse value to an int in a try/except that only wraps the int() call and raises "must be a valid user ID" on failure, then compare the parsed ID to current_user.id outside the try and raise the specific "own user ID" error when they differ so tests and clients see the intended policy message.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmcp_core.py%0A%2A%2ALine%3A%2A%2A%20200%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20Unauthorized%20created_by_fk%20filter%20values%20are%20reported%20as%20%22created_by_fk%20filter%20value%20must%20be%20a%20valid%20user%20ID%22%20because%20the%20ValueError%20raised%20for%20a%20mismatched%20ID%20is%20thrown%20inside%20a%20try%20that%20catches%20ValueError%2C%20so%20the%20intended%20%22own%
20user%20ID%22%20policy%20error%20is%20always%20replaced%20by%20the%20generic%20parse%20error%20message.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20MUST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%20change%20as%20small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surrounding%20code%20or%20expand%20scope%20beyond%20the%20finding.%0AOnce%20fix%20is%20implemented%2C%20also%20chec
k%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20an%20%2A%2AArchitect%20%2F%20Logical%20Review%2A%2A%20comment%20left%20during%20a%20code%20review.%20These%20reviews%20are%20first-class%2C%20important%20findings%20%E2%80%94%20not%20optional%20suggestions.%20Do%20NOT%20dismiss%20this%20as%20a%20%27big%20architectural%20change%27%20just%20because%20the%20title%20says%20architect%20review%3B%20most%20of%20these%20can%20be%20resolved%20with%20a%20small%2C%20localized%20fix%20once%20the%20intent%20is%20understood.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fmcp_core.py%0A%2A%2ALine%3A%2A%2A%20200%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AHIGH%3A%20Unauthoriz
ed%20created_by_fk%20filter%20values%20are%20reported%20as%20%22created_by_fk%20filter%20value%20must%20be%20a%20valid%20user%20ID%22%20because%20the%20ValueError%20raised%20for%20a%20mismatched%20ID%20is%20thrown%20inside%20a%20try%20that%20catches%20ValueError%2C%20so%20the%20intended%20%22own%20user%20ID%22%20policy%20error%20is%20always%20replaced%20by%20the%20generic%20parse%20error%20message.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AIf%20a%20suggested%20approach%20is%20provided%20above%2C%20use%20it%20as%20the%20authoritative%20instruction.%20If%20no%20explicit%20code%20suggestion%20is%20given%2C%20you%20MUST%20still%20draft%20and%20apply%20your%20own%20minimal%2C%20localized%20fix%20%E2%80%94%20do%20not%20punt%20back%20with%20%27no%20suggestion%20provided%2C%20review%20manually%27.%20Keep%20the%20change%20as%20
small%20as%20possible%3A%20add%20a%20guard%20clause%2C%20gate%20on%20a%20loading%20state%2C%20reorder%20an%20await%2C%20wrap%20in%20a%20conditional%2C%20etc.%20Do%20not%20refactor%20surrounding%20code%20or%20expand%20scope%20beyond%20the%20finding.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/mcp_service/mcp_core.py
**Line:** 200:207
**Comment:**
*HIGH: Unauthorized created_by_fk filter values are reported as "created_by_fk filter value must be a valid user ID" because the ValueError raised for a mismatched ID is thrown inside a try that catches ValueError, so the intended "own user ID" policy error is always replaced by the generic parse error message.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4316575666
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3140250345
##########
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:
##########
@@ -177,6 +178,56 @@ def test_model_list_tool_allows_order_column_when_sortable_columns_not_declared(
tool.run_tool(order_column="name")
+def test_model_list_tool_allows_created_by_fk_filter_for_current_user():
+ """created_by_fk filter is accepted when the value matches the current user's ID."""
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
+ # Should not raise
+ tool.run_tool(filters=[{"col": "created_by_fk", "opr": "eq", "value": 42}])
+
+
+def test_model_list_tool_rejects_created_by_fk_filter_for_other_user():
+ """created_by_fk filter is rejected when the value is another user's ID."""
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
+ with pytest.raises(ValueError, match="own user ID"):
+ tool.run_tool(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect Mock Target</b></div>
<div id="fix">
The patch targets a module attribute that doesn't exist for mocking current_user. Since current_user is imported locally from flask_login in _validate_created_by_fk_filters, the patch should target "flask_login.current_user" to properly mock it.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
"flask_login.current_user", current_user
````
</div>
</details>
</div>
<small><i>Code Review Run #ced8c1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/system/tool/test_mcp_core.py:
##########
@@ -177,6 +178,56 @@ def test_model_list_tool_allows_order_column_when_sortable_columns_not_declared(
tool.run_tool(order_column="name")
+def test_model_list_tool_allows_created_by_fk_filter_for_current_user():
+ """created_by_fk filter is accepted when the value matches the current user's ID."""
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
+ # Should not raise
+ tool.run_tool(filters=[{"col": "created_by_fk", "opr": "eq", "value": 42}])
+
+
+def test_model_list_tool_rejects_created_by_fk_filter_for_other_user():
+ """created_by_fk filter is rejected when the value is another user's ID."""
+ current_user = Mock()
+ current_user.is_authenticated = True
+ current_user.id = 42
+
+ tool = ModelListCore(
+ dao_class=DummyDAO,
+ output_schema=DummyOutputSchema,
+ item_serializer=dummy_serializer,
+ filter_type=None,
+ default_columns=["id", "name"],
+ search_columns=["name"],
+ list_field_name="items",
+ output_list_schema=DummyListSchema,
+ )
+
+ with patch(
+ "superset.mcp_service.mcp_core.current_user", current_user
+ ):
+ with pytest.raises(ValueError, match="own user ID"):
+ tool.run_tool(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect Mock Target</b></div>
<div id="fix">
Similar to the first test, this patch won't mock current_user correctly. Update to "flask_login.current_user".
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
"flask_login.current_user", current_user
````
</div>
</details>
</div>
<small><i>Code Review Run #ced8c1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): restore self-lookup via created_by_me flag [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4331376519
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): restore self-lookup via created_by_me flag [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #39638:
URL: https://github.com/apache/superset/pull/39638
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): restore self-lookup via created_by_me flag [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4332154920
**`superset/mcp_service/chart/schemas.py` — unannounced behavioral change in `ChartInfo` serializer**
The `when_used="json"` argument was dropped from the `@model_serializer` decorator:
```diff
-@model_serializer(mode="wrap", when_used="json")
+@model_serializer(mode="wrap")
```
Previously the custom serializer (which strips user-directory fields) only fired during JSON serialization — `.model_dump(mode="json")`. Without `when_used`, it now also fires during plain Python dict serialization — `.model_dump()`. This is actually a **correctness fix** (privacy fields were leaking in dict mode), and `test_privacy.py` validates both modes work correctly. But it's not called out in the PR summary, so any internal caller doing `chart_info.model_dump()` and expecting `created_by` / `changed_by` / `owners` to be present would silently get them stripped. Worth a one-liner in the PR description so reviewers don't miss it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): restore self-lookup via created_by_me flag [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4340294130
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): restore created_by_fk filter restricted to current user only [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39638:
URL: https://github.com/apache/superset/pull/39638#issuecomment-4316127879
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): restore self-lookup via created_by_me flag [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #39638:
URL: https://github.com/apache/superset/pull/39638#discussion_r3150776941
##########
superset/mcp_service/common/cache_schemas.py:
##########
@@ -83,6 +85,69 @@ class FormDataCacheControl(CacheControlMixin):
)
+class CreatedByMeMixin(BaseModel):
+ """Mixin that adds a created_by_me filter flag to list request schemas.
+
+ Provides a clean caller-facing alternative to exposing foreign key IDs.
+ The server translates the flag into the appropriate FK filter and injects
+ the current user's ID automatically.
+ """
+
+ created_by_me: Annotated[
+ bool,
+ Field(
+ default=False,
+ description=(
+ "When true, return only items created by the current user. "
+ "Can be combined with 'filters' but not with 'search'."
+ ),
+ ),
+ ]
+
+ @model_validator(mode="after")
+ def _validate_created_by_me_with_search(self) -> Any:
+ if getattr(self, "search", None) and self.created_by_me:
+ raise ValueError(
+ "'created_by_me' cannot be combined with 'search'. "
+ "Use 'created_by_me' alone or with 'filters'."
+ )
+ return self
+
+
+class OwnedByMeMixin(BaseModel):
+ """Mixin that adds an owned_by_me filter flag to list request schemas.
+
+ Provides a clean caller-facing alternative to exposing M2M owner IDs.
+ The server translates the flag into the appropriate owner filter and injects
+ the current user's ID automatically.
+
+ When combined with created_by_me, returns items where the current user is
+ either the creator OR an owner (union, not intersection).
Review Comment:
We can also make this an AND instead of an OR, or we could throw a schema error if both created by me and owned by me are passed. It seemed likely that the user is going to want both if they ask for both, which creates a new filter type. Most filters are OR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org