codeant-ai-for-open-source[bot] commented on code in PR #41753:
URL: https://github.com/apache/superset/pull/41753#discussion_r3553398966
##########
superset/charts/filters.py:
##########
@@ -108,6 +111,12 @@ def apply(self, query: Query, value: Any) -> Query:
class ChartFilter(BaseFilter): # pylint: disable=too-few-public-methods
def apply(self, query: Query, value: Any) -> Query:
+ # Embedded guests are scoped to their token's dashboards first. A guest
+ # is never entitled to all charts, regardless of what its role grants,
+ # and an empty token scope denies all charts (a deny-all clause).
+ if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
Review Comment:
**Suggestion:** Replace the inline walrus assignment with a separate local
variable declaration and add an explicit type annotation for that variable
before the conditional check. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new walrus assignment introduces a local variable without any type
annotation, and this is exactly the kind of newly added Python code the
type-hints rule targets. The suggestion correctly identifies a real omission in
the added line.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5543dac1a4034987bb80f1adf4aacd88&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5543dac1a4034987bb80f1adf4aacd88&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/charts/filters.py
**Line:** 117:117
**Comment:**
*Custom Rule: Replace the inline walrus assignment with a separate
local variable declaration and add an explicit type annotation for that
variable before the conditional check.
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%2F41753&comment_hash=f5d16bb6c12f44ef6988b12a2e0134b0a586b301ad5decaee91a9ae1a6519d25&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=f5d16bb6c12f44ef6988b12a2e0134b0a586b301ad5decaee91a9ae1a6519d25&reaction=dislike'>👎</a>
##########
superset/utils/filters.py:
##########
@@ -41,3 +41,50 @@ def get_dataset_access_filters(
base_model.schema_perm.in_(schema_perms),
*args,
)
+
+
+def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
+ """SQLAlchemy condition scoping charts to the dashboards embedded via the
+ current guest token, or None when the user is not an embedded guest.
+
+ For an embedded guest the result is never None: a token with no dashboard
+ resources returns a deny-all clause so the chart filter fails closed
instead
+ of falling back to the ordinary role-based access path.
+
+ Mirrors how DashboardAccessFilter scopes dashboards.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import is_feature_enabled, security_manager
+ from superset.models.dashboard import Dashboard, is_uuid
+ from superset.models.embedded_dashboard import EmbeddedDashboard
+ from superset.security.guest_token import GuestTokenResourceType
+
+ if not is_feature_enabled("EMBEDDED_SUPERSET"):
+ return None
+ guest = security_manager.get_current_guest_user_if_guest()
+ if guest is None:
+ return None
+ # The user is an embedded guest from here: scope to the token's dashboards
+ # and never widen back to role-based access. No dashboards means deny all.
+ ids: list[Any] = [
+ r["id"]
+ for r in guest.resources
+ if r["type"] == GuestTokenResourceType.DASHBOARD.value
+ ]
+ if not ids:
+ return false()
+ # TODO (embedded): only use the uuid filter once uuids are rolled out
+ # A guest token may mix uuid and int dashboard ids during the uuid rollout.
+ # Route each id kind to its own column and OR them — a plain int sent to
the
+ # uuid-typed column would raise a bind/type error.
+ uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
Review Comment:
**Suggestion:** Add a concrete type hint for `uuid_ids` instead of leaving
this derived list unannotated. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
`uuid_ids` is a newly introduced derived list and is left unannotated even
though its type can be inferred and annotated. This is a real instance of
missing type hints in new Python code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eb7b92ce2def41f5b6417abe572c37f4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eb7b92ce2def41f5b6417abe572c37f4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/utils/filters.py
**Line:** 80:80
**Comment:**
*Custom Rule: Add a concrete type hint for `uuid_ids` instead of
leaving this derived list unannotated.
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%2F41753&comment_hash=12bd16cd335c78087e1819b10668dd65c0e6907cf7847de5c6667a56e5b40589&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=12bd16cd335c78087e1819b10668dd65c0e6907cf7847de5c6667a56e5b40589&reaction=dislike'>👎</a>
##########
superset/utils/filters.py:
##########
@@ -41,3 +41,50 @@ def get_dataset_access_filters(
base_model.schema_perm.in_(schema_perms),
*args,
)
+
+
+def guest_embedded_dashboard_filter() -> Optional[ColumnElement[bool]]:
+ """SQLAlchemy condition scoping charts to the dashboards embedded via the
+ current guest token, or None when the user is not an embedded guest.
+
+ For an embedded guest the result is never None: a token with no dashboard
+ resources returns a deny-all clause so the chart filter fails closed
instead
+ of falling back to the ordinary role-based access path.
+
+ Mirrors how DashboardAccessFilter scopes dashboards.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import is_feature_enabled, security_manager
+ from superset.models.dashboard import Dashboard, is_uuid
+ from superset.models.embedded_dashboard import EmbeddedDashboard
+ from superset.security.guest_token import GuestTokenResourceType
+
+ if not is_feature_enabled("EMBEDDED_SUPERSET"):
+ return None
+ guest = security_manager.get_current_guest_user_if_guest()
+ if guest is None:
+ return None
+ # The user is an embedded guest from here: scope to the token's dashboards
+ # and never widen back to role-based access. No dashboards means deny all.
+ ids: list[Any] = [
+ r["id"]
+ for r in guest.resources
+ if r["type"] == GuestTokenResourceType.DASHBOARD.value
+ ]
+ if not ids:
+ return false()
+ # TODO (embedded): only use the uuid filter once uuids are rolled out
+ # A guest token may mix uuid and int dashboard ids during the uuid rollout.
+ # Route each id kind to its own column and OR them — a plain int sent to
the
+ # uuid-typed column would raise a bind/type error.
+ uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
+ int_ids = [id_ for id_ in ids if not is_uuid(id_)]
Review Comment:
**Suggestion:** Add a concrete type hint for `int_ids` instead of leaving
this derived list unannotated. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
`int_ids` is also a new list variable without an explicit type annotation.
Since the type is evident, this is a valid type-hint omission under the rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e7422bd220f646848dca26e7aa0bd64f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e7422bd220f646848dca26e7aa0bd64f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/utils/filters.py
**Line:** 81:81
**Comment:**
*Custom Rule: Add a concrete type hint for `int_ids` instead of leaving
this derived list unannotated.
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%2F41753&comment_hash=63224dd0eb9082ede4025b5cacb53744c8ca6fb5bc5ad07557a3ff617f4181e7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=63224dd0eb9082ede4025b5cacb53744c8ca6fb5bc5ad07557a3ff617f4181e7&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_filters.py:
##########
@@ -0,0 +1,180 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for the ``ChartFilter`` embedded-guest scoping branch."""
+
+from typing import Any
+from unittest.mock import MagicMock
+
+import pytest
+from pytest_mock import MockerFixture
+from sqlalchemy import create_engine
+
+
+def test_chart_filter_scopes_guest_to_token_dashboards(mocker: MockerFixture)
-> None:
+ """A guest's chart list is scoped to the dashboards in its token."""
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=False
+ )
+ mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
+ return_value=Dashboard.id.in_([1, 2]),
+ )
+
+ captured: dict[str, Any] = {}
+ query: MagicMock = MagicMock()
+
+ def _capture_filter(clause: object) -> MagicMock:
+ captured["clause"] = clause
+ return query
+
+ query.filter.side_effect = _capture_filter
+
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ result = filt.apply(query, None)
+
+ assert result is query
+ query.filter.assert_called_once()
+ # Guest branch returns early: it must NOT take the datasource-access join
path.
+ query.join.assert_not_called()
+
+ compiled: str = str(
+ captured["clause"].compile(
+ create_engine("sqlite://"), compile_kwargs={"literal_binds": True}
+ )
+ )
+ assert "EXISTS" in compiled # scoped via the m2m dashboards relationship
+ assert "IN (1, 2)" in compiled
+
+
+def test_chart_filter_non_guest_skips_guest_branch(mocker: MockerFixture) ->
None:
+ """With no embedded guest, ChartFilter does not take the guest branch (it
+ proceeds to the normal viewer-access path instead of filtering on
dashboards)."""
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=False
+ )
+ guest_filter: MagicMock = mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
return_value=None
+ )
+ # Stop apply right after the guest check — reaching the viewer-access path
+ # proves the guest branch was skipped.
+ sentinel: RuntimeError = RuntimeError("reached viewer-access path")
+ mocker.patch.object(ChartFilter, "_apply_viewers", side_effect=sentinel)
+ query: MagicMock = MagicMock()
+
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+
+ with pytest.raises(RuntimeError, match="viewer-access path"):
+ filt.apply(query, None)
+
+ guest_filter.assert_called_once()
+ query.filter.assert_not_called() # guest branch (query.filter) was NOT
taken
+
+
+def test_chart_filter_admin_bypasses_all_scoping(mocker: MockerFixture) ->
None:
+ """A non-guest who can access all datasources gets the query back
untouched.
+
+ The guest scope check runs first but returns None for a non-guest, so the
+ all-datasources bypass still applies.
+ """
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ guest_filter: MagicMock = mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
return_value=None
+ )
+
+ query: MagicMock = MagicMock()
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+
+ assert filt.apply(query, None) is query
+ query.filter.assert_not_called()
+ guest_filter.assert_called_once()
+
+
+def test_chart_filter_guest_scoped_even_with_all_datasource_access(
+ mocker: MockerFixture,
+) -> None:
+ """A guest is scoped to its token's dashboards even when its role can
access
+ all datasources: the guest branch runs before the all-datasources
bypass."""
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ can_access = mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=True
+ )
+ mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
+ return_value=Dashboard.id.in_([1, 2]),
+ )
+ query: MagicMock = MagicMock()
+ query.filter.return_value = query
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+
+ assert filt.apply(query, None) is query
+ # Scoped via the guest branch — NOT returned untouched by the all-access
path.
+ query.filter.assert_called_once()
+ can_access.assert_not_called()
+
+
+def test_chart_filter_guest_no_resources_denied(mocker: MockerFixture) -> None:
+ """A guest whose token has no dashboards is denied all charts: the filter
+ returns a deny-all clause (not None), so apply scopes rather than falling
+ through to role-based access."""
+ from sqlalchemy import false
+
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.slice import Slice
+
+ mocker.patch.object(
+ security_manager, "can_access_all_datasources", return_value=False
+ )
+ mocker.patch(
+ "superset.charts.filters.guest_embedded_dashboard_filter",
+ return_value=false(),
+ )
+ viewers = mocker.patch.object(
+ ChartFilter, "_apply_viewers", side_effect=AssertionError("role path")
+ )
Review Comment:
**Suggestion:** Add a concrete type annotation for this local mock object
assignment so the new code consistently uses type hints. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a real omission of a type hint in newly added Python code: `viewers`
is assigned a mock object and could be annotated, but no explicit type is
present.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=43f5a60b09324f17a37de86931345f58&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=43f5a60b09324f17a37de86931345f58&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/charts/test_filters.py
**Line:** 170:172
**Comment:**
*Custom Rule: Add a concrete type annotation for this local mock object
assignment so the new code consistently uses type hints.
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%2F41753&comment_hash=a54973b0812502b8a0ca5baebc4c7f0a2b57d9a281c17e98d7a346a1f15a874a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=a54973b0812502b8a0ca5baebc4c7f0a2b57d9a281c17e98d7a346a1f15a874a&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/rls/tool/test_rls_tools.py:
##########
@@ -244,3 +244,31 @@ async def
test_list_rls_filters_subjects_only_select_columns(mock_list, mcp_serv
item = data["rls_filters"][0]
assert "subjects" in item
assert item["subjects"][0]["label"] == "Alpha"
+
+
[email protected]
Review Comment:
**Suggestion:** Add explicit type hints to this async test function
signature, including the fixture parameter type and a return type annotation.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The added async test function has no type hints for its fixture argument and
no return annotation, which matches the rule requiring type hints on new or
modified Python functions where applicable.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bba318b9dc3a482881559d36809ff4fb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bba318b9dc3a482881559d36809ff4fb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/rls/tool/test_rls_tools.py
**Line:** 249:249
**Comment:**
*Custom Rule: Add explicit type hints to this async test function
signature, including the fixture parameter type and a return type annotation.
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%2F41753&comment_hash=4931c025e547d194e52c92c6662a1110beaf750fa16a2d0c80b260496c90497b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=4931c025e547d194e52c92c6662a1110beaf750fa16a2d0c80b260496c90497b&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
