codeant-ai-for-open-source[bot] commented on code in PR #43151:
URL: https://github.com/apache/superset/pull/43151#discussion_r3781277649
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1573,18 +1573,47 @@ def
test_chart_data_with_incompatible_adhoc_column(self):
def test_chart_data_as_guest_user(self, is_guest_user, has_guest_access):
"""
Chart data API: Test response does not inlcude the SQL query for
embedded
- users.
+ users whose role lacks "can view query on Dashboard".
"""
g.user.rls = []
is_guest_user.return_value = True
has_guest_access.return_value = True
- rv = self.client.post(CHART_DATA_URI, json=self.query_context_payload)
+ with mock.patch(
+ "superset.charts.data.api.security_manager.can_access",
+ side_effect=lambda permission, view: (
+ (permission, view) != ("can_view_query", "Dashboard")
+ ),
+ ):
+ rv = self.client.post(CHART_DATA_URI,
json=self.query_context_payload)
data = json.loads(rv.data.decode("utf-8"))
result = data["result"]
excluded_key = "query"
assert all([excluded_key not in query for query in result]) # noqa:
C419
+
@mock.patch("superset.security.manager.SupersetSecurityManager.has_guest_access")
+
@mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user")
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ def test_chart_data_as_guest_user_allowed_to_view_query(
+ self, is_guest_user, has_guest_access
+ ):
+ """
Review Comment:
**Suggestion:** The newly added integration test is inside
`TestPostChartDataApi`, whose class is unconditionally marked with
`@pytest.mark.skip`, so this regression test never runs and cannot detect a
future break in guest query visibility. Move the test to an enabled test class
or remove the skip condition before relying on it for integration coverage.
[code quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Guest query visibility has no runtime integration coverage.
- ⚠️ Permission regressions can pass the integration suite undetected.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2b1dd40fe39d470e871ac22f34173660&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=2b1dd40fe39d470e871ac22f34173660&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/data/api_tests.py
**Line:** 1597:1600
**Comment:**
*Code Quality: The newly added integration test is inside
`TestPostChartDataApi`, whose class is unconditionally marked with
`@pytest.mark.skip`, so this regression test never runs and cannot detect a
future break in guest query visibility. Move the test to an enabled test class
or remove the skip condition before relying on it for integration coverage.
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%2F43151&comment_hash=855df3ce772e9fa248dcc54311345e8b9c9141a127df12b4d2698b90ec12e02c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43151&comment_hash=855df3ce772e9fa248dcc54311345e8b9c9141a127df12b4d2698b90ec12e02c&reaction=dislike'>👎</a>
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1573,18 +1573,47 @@ def
test_chart_data_with_incompatible_adhoc_column(self):
def test_chart_data_as_guest_user(self, is_guest_user, has_guest_access):
"""
Chart data API: Test response does not inlcude the SQL query for
embedded
- users.
+ users whose role lacks "can view query on Dashboard".
"""
g.user.rls = []
is_guest_user.return_value = True
has_guest_access.return_value = True
- rv = self.client.post(CHART_DATA_URI, json=self.query_context_payload)
+ with mock.patch(
+ "superset.charts.data.api.security_manager.can_access",
+ side_effect=lambda permission, view: (
+ (permission, view) != ("can_view_query", "Dashboard")
+ ),
+ ):
+ rv = self.client.post(CHART_DATA_URI,
json=self.query_context_payload)
data = json.loads(rv.data.decode("utf-8"))
result = data["result"]
excluded_key = "query"
assert all([excluded_key not in query for query in result]) # noqa:
C419
+
@mock.patch("superset.security.manager.SupersetSecurityManager.has_guest_access")
+
@mock.patch("superset.security.manager.SupersetSecurityManager.is_guest_user")
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ def test_chart_data_as_guest_user_allowed_to_view_query(
+ self, is_guest_user, has_guest_access
+ ):
+ """
+ Chart data API: Test response includes the SQL query for embedded users
+ whose role carries "can view query on Dashboard".
+ """
+ g.user.rls = []
+ is_guest_user.return_value = True
+ has_guest_access.return_value = True
+
+ with mock.patch(
+ "superset.charts.data.api.security_manager.can_access",
+ return_value=True,
+ ):
+ rv = self.client.post(CHART_DATA_URI,
json=self.query_context_payload)
+ data = json.loads(rv.data.decode("utf-8"))
Review Comment:
**Suggestion:** The test forces `security_manager.can_access` to return
`True`, so it only verifies that the response preserves an existing `query`
field and does not verify that a guest role with the actual
`can_view_query`/`Dashboard` FAB permission is resolved correctly. Configure
the guest role and permission in the test, or assert the real permission lookup
without replacing it with an unconditional mock. [code quality]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Role-to-permission wiring is not integration-tested.
- ⚠️ Incorrect FAB permission resolution can remain undetected.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e36be2bf3ac249a4adabe80b7b7da47b&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=e36be2bf3ac249a4adabe80b7b7da47b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/charts/data/api_tests.py
**Line:** 1608:1613
**Comment:**
*Code Quality: The test forces `security_manager.can_access` to return
`True`, so it only verifies that the response preserves an existing `query`
field and does not verify that a guest role with the actual
`can_view_query`/`Dashboard` FAB permission is resolved correctly. Configure
the guest role and permission in the test, or assert the real permission lookup
without replacing it with an unconditional mock.
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%2F43151&comment_hash=9bf74982db730f286eb1248068e606e37d9ce1bac4736ba87624276f06841aba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43151&comment_hash=9bf74982db730f286eb1248068e606e37d9ce1bac4736ba87624276f06841aba&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]
