You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2026/04/09 05:28:13 UTC
[PR] fix: Drill to Detail for Embedded [superset]
Vitor-Avila opened a new pull request, #39214:
URL: https://github.com/apache/superset/pull/39214
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: Drill to Detail for Embedded [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3058046791
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
+ (
+ form_data.get("parent_slice_id") is None
+ and (
+ slc := self.session.query(Slice)
+ .filter(Slice.id == slice_id)
+ .one_or_none()
+ )
+ and slc in dashboard_.slices
+ and slc.datasource == datasource
+ )
+ or
+ # Multi-layer chart child access (has parent)
+ (
+ (
+ parent_id := form_data.get(
+ "parent_slice_id"
+ )
+ )
+ and (
+ parent_slc := self.session.query(Slice)
+ .filter(Slice.id == parent_id)
+ .one_or_none()
+ )
+ and parent_slc in dashboard_.slices
Review Comment:
The child slice is validated against the parent's config via _validate_child_in_parent_multilayer this ensures the slice_id is actually a child of the parent_slice_id in the deck_multi chart's deck_slices param. This is a false positive
--
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: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3055694998
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
+ (
+ form_data.get("parent_slice_id") is None
+ and (
+ slc := self.session.query(Slice)
+ .filter(Slice.id == slice_id)
+ .one_or_none()
+ )
+ and slc in dashboard_.slices
+ and slc.datasource == datasource
+ )
+ or
+ # Multi-layer chart child access (has parent)
+ (
+ (
+ parent_id := form_data.get(
+ "parent_slice_id"
+ )
+ )
+ and (
+ parent_slc := self.session.query(Slice)
+ .filter(Slice.id == parent_id)
+ .one_or_none()
+ )
+ and parent_slc in dashboard_.slices
Review Comment:
this is outside the scope of this PR. I'll tag @msyavuz here as he originally worked on this logic. Let me know if this is a concern, and if I can help with anything
--
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: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3055665776
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
Review Comment:
The only real changes to this block are:
1. I noticed we were validating `form_data.get("type") != "NATIVE_FILTER"` more than once, so I moved it to the top for single validation;
2. Re-named the method from `has_drill_by_access` to `has_drill_access`.
I can undo #1 if we think it's better
--
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: Drill to Detail for Embedded [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 #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3055662349
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
+ (
+ form_data.get("parent_slice_id") is None
+ and (
+ slc := self.session.query(Slice)
+ .filter(Slice.id == slice_id)
+ .one_or_none()
+ )
+ and slc in dashboard_.slices
+ and slc.datasource == datasource
+ )
+ or
+ # Multi-layer chart child access (has parent)
+ (
+ (
+ parent_id := form_data.get(
+ "parent_slice_id"
+ )
+ )
+ and (
+ parent_slc := self.session.query(Slice)
+ .filter(Slice.id == parent_id)
+ .one_or_none()
+ )
+ and parent_slc in dashboard_.slices
Review Comment:
**Suggestion:** The multi-layer child access path validates `parent_slice_id` and child membership in `deck_slices`, but it never verifies that the requested datasource actually belongs to that child chart. Because datasource authorization is granted when this branch is true, a forged request can pair a valid parent/child chart combination with an unrelated datasource and bypass datasource access checks. Add an explicit lookup of the child slice and ensure its datasource matches the requested datasource before granting access. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded guests query unauthorized datasets via ChartDataRestApi.data endpoint.
- ❌ Dashboard RBAC users bypass datasource checks for deck_multi children.
- ⚠️ Inconsistent child datasource validation in SupersetSecurityManager.raise_for_access.
```
</details>
```suggestion
and (
child_slc := self.session.query(Slice)
.filter(Slice.id == slice_id)
.one_or_none()
)
and parent_slc in dashboard_.slices
and child_slc.datasource == datasource
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a dashboard with roles or embedded access so that either DASHBOARD_RBAC or
EMBEDDED_SUPERSET is enabled and used (checked in
`SupersetSecurityManager.raise_for_access` at `superset/security/manager.py:162-179` and
`:269-271` via `is_feature_enabled("DASHBOARD_RBAC")` /
`is_feature_enabled("EMBEDDED_SUPERSET")`).
2. On that dashboard, add a `deck_multi` chart whose parent slice has a `deck_slices`
configuration including a child chart ID (validated by
`_validate_child_in_parent_multilayer` at `superset/security/manager.py:702-724`),
ensuring the child chart uses dataset A; also create another dataset B that the same
user/guest does NOT have `datasource_access` or schema access to.
3. As an embedded guest user (feature flag `EMBEDDED_SUPERSET`) or a RBAC viewer of that
dashboard, send a POST request to `ChartDataRestApi.data` at `/api/v1/chart/data`
(`superset/charts/data/api.py:280-99`) with a JSON body that `ChartDataQueryContextSchema`
accepts, where `datasource` points to dataset B (e.g. `{"id": <B_id>, "type": "table"}`)
and `form_data` contains `{"dashboardId": <dashboard_id>, "type": "DRILL_DETAIL",
"slice_id": <child_slice_id>, "parent_slice_id": <parent_slice_id>, ...}` so that
`slice_id` is the valid child from the parent's `deck_slices`.
4. The request flows through `ChartDataCommand.validate`
(`superset/commands/chart/data/get_data_command.py:39-40`) which calls
`QueryContext.raise_for_access` (`superset/common/query_context.py:18`), which delegates
to `QueryContextProcessor.raise_for_access` and then
`security_manager.raise_for_access(query_context=self._query_context)`
(`superset/common/query_context_processor.py:525-537);` inside
`SupersetSecurityManager.raise_for_access` (`superset/security/manager.py:145-180` and
`:2445-2647`), the user fails the normal schema/datasource/ownership checks, but passes
the multi-layer child branch at lines `2644-35` because `parent_slc` is in
`dashboard_.slices` and `_validate_child_in_parent_multilayer` returns True, and there is
currently no `child_slc.datasource == datasource` validation—so no
`SupersetSecurityException` is raised and the query executes against dataset B, returning
unauthorized data.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 2672:2672
**Comment:**
*Security: The multi-layer child access path validates `parent_slice_id` and child membership in `deck_slices`, but it never verifies that the requested datasource actually belongs to that child chart. Because datasource authorization is granted when this branch is true, a forged request can pair a valid parent/child chart combination with an unrelated datasource and bypass datasource access checks. Add an explicit lookup of the child slice and ensure its datasource matches the requested datasource before granting access.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39214&comment_hash=39346b352bab2423084ced45ef76a3fc5d8f78a97de742bc948469402ffb643a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39214&comment_hash=39346b352bab2423084ced45ef76a3fc5d8f78a97de742bc948469402ffb643a&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: Drill to Detail for Embedded [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39214:
URL: https://github.com/apache/superset/pull/39214#issuecomment-4212112154
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: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3059247340
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
+ (
+ form_data.get("parent_slice_id") is None
+ and (
+ slc := self.session.query(Slice)
+ .filter(Slice.id == slice_id)
+ .one_or_none()
+ )
+ and slc in dashboard_.slices
+ and slc.datasource == datasource
+ )
+ or
+ # Multi-layer chart child access (has parent)
+ (
+ (
+ parent_id := form_data.get(
+ "parent_slice_id"
+ )
+ )
+ and (
+ parent_slc := self.session.query(Slice)
+ .filter(Slice.id == parent_id)
+ .one_or_none()
+ )
+ and parent_slc in dashboard_.slices
Review Comment:
thanks for confirming 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] fix: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #39214:
URL: https://github.com/apache/superset/pull/39214
--
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: Drill to Detail for Embedded [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 #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3055653413
##########
tests/integration_tests/datasource_tests.py:
##########
@@ -543,6 +553,10 @@ def test_get_samples_embedded_user(
resp = self.client.post(uri, json={})
assert resp.status_code == 200
+ security_manager.del_permission_role(
+ security_manager.find_role("Gamma"), perm_view
+ )
Review Comment:
**Suggestion:** The temporary permission grant is only reverted at the end of the happy path. If the request/assertion raises before cleanup, `Gamma` keeps `can_samples`, leaking mutable auth state into later tests and causing order-dependent failures. Wrap the permission mutation in `try/finally` so cleanup always runs. [resource leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failing embedded samples test leaves Gamma with can_samples permission.
- ⚠️ Later authorization tests using Gamma become order-dependent and flaky.
- ⚠️ Test runs may mask real regressions in samples authorization.
```
</details>
```suggestion
gamma_role = security_manager.find_role("Gamma")
security_manager.add_permission_role(gamma_role, perm_view)
try:
mock_is_guest_user.return_value = True
mock_has_guest_access.return_value = True
mock_rls.return_value = []
tbl = self.get_table(name="birth_names")
dash = self.get_dash_by_slug("births")
uri = f"/datasource/samples?datasource_id={tbl.id}&datasource_type=table&dashboard_id={dash.id}" # noqa: E501
resp = self.client.post(uri, json={})
assert resp.status_code == 200
finally:
security_manager.del_permission_role(gamma_role, perm_view)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the integration test `TestDatasource.test_get_samples_embedded_user` defined in
`tests/integration_tests/datasource_tests.py:124-160` (hunk starting at diff line 530).
The test logs in as `GAMMA_USERNAME` at line 140 and then calls
`security_manager.find_permission_view_menu("can_samples", "Datasource")` and
`security_manager.add_permission_role(security_manager.find_role("Gamma"), perm_view)` at
lines 142-147, permanently granting `Gamma` the `can_samples` permission for the
`Datasource` view (these helpers come from
`superset.security.manager.SupersetSecurityManager` in `superset/security/manager.py`).
2. Observe that the test then configures guest mocks and issues a POST to
`/datasource/samples` at `tests/integration_tests/datasource_tests.py:153-154`, asserting
`assert resp.status_code == 200` at line 155. If any regression causes this endpoint to
return a non‑200 (e.g. a bug in the samples view logic or embedded-guest handling), this
assertion fails and pytest aborts the function immediately, so the cleanup block at lines
157-159 (`security_manager.del_permission_role(security_manager.find_role("Gamma"),
perm_view)`) is never executed.
3. Because `security_manager.add_permission_role` commits its changes using the shared
SQLAlchemy session (as evidenced by similar permission setup/teardown patterns with
explicit `db.session.commit()` and `security_manager.del_permission_role` in
`tests/integration_tests/databases/api_tests.py:2024-2033`), and `TestDatasource.tearDown`
in `tests/integration_tests/datasource_tests.py:95-101` only calls
`db.session.rollback()`, the added `can_samples` permission on the `Gamma` role persists
in the test database and in the `security_manager` role cache after the failing test
finishes.
4. In the same pytest run, execute other tests that rely on default `Gamma` permissions
(for example any test that logs in as `GAMMA_USERNAME` and exercises authorization around
`/datasource/samples`, such as `test_get_samples_embedded_user_without_dash_id` in
`tests/integration_tests/datasource_tests.py:167-179`). These later tests now see `Gamma`
with an unexpected `can_samples` permission, making their behavior order‑dependent and
potentially masking authorization regressions or causing confusing failures when the
module is run as a whole versus individual tests.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/datasource_tests.py
**Line:** 544:558
**Comment:**
*Resource Leak: The temporary permission grant is only reverted at the end of the happy path. If the request/assertion raises before cleanup, `Gamma` keeps `can_samples`, leaking mutable auth state into later tests and causing order-dependent failures. Wrap the permission mutation in `try/finally` so cleanup always runs.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39214&comment_hash=4af49f25befce30c6f5de809ae46ee8285c70a3391c4298176219d29fe0f8d66&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39214&comment_hash=4af49f25befce30c6f5de809ae46ee8285c70a3391c4298176219d29fe0f8d66&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: Drill to Detail for Embedded [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3059181145
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
Review Comment:
holly molly, maybe it's time to refactor this method ...
--
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: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3059613454
##########
superset/security/manager.py:
##########
@@ -2630,40 +2639,51 @@ def raise_for_access( # noqa: C901
)
)
or (
- # Chart.
form_data.get("type") != "NATIVE_FILTER"
- and (slice_id := form_data.get("slice_id"))
and (
- # Direct chart access (no parent)
(
- form_data.get("parent_slice_id") is None
+ # Chart.
+ (slice_id := form_data.get("slice_id"))
and (
- slc := self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
+ # Direct chart access (no parent)
Review Comment:
I'll try tackling that in a follow up. I believe the logic on its own is correct, but we could move each validation block to their own helper method (like drill validation is now)
--
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: Drill to Detail for Embedded [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39214:
URL: https://github.com/apache/superset/pull/39214#issuecomment-4211664512
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39214?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 `14.28571%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.24%. Comparing base ([`c496415`](https://app.codecov.io/gh/apache/superset/commit/c49641538d5e388ea684d050a4ba6a6422d1d02d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0bf26f1`](https://app.codecov.io/gh/apache/superset/commit/0bf26f17c43ab1cbf5710c2d2e21ec8e76ee41d2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39214?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/views/datasource/views.py](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&filepath=superset%2Fviews%2Fdatasource%2Fviews.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS92aWV3cy5weQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/views/datasource/utils.py](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&filepath=superset%2Fviews%2Fdatasource%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS91dGlscy5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39214?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (c496415) and HEAD (0bf26f1). Click for more details.
>
> <details><summary>HEAD has 87 uploads less than BASE</summary>
>
>| Flag | BASE (c496415) | HEAD (0bf26f1) |
>|------|------|------|
>|python|81|1|
>|presto|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39214 +/- ##
==========================================
- Coverage 64.38% 55.24% -9.14%
==========================================
Files 2550 2550
Lines 132141 132146 +5
Branches 30658 30659 +1
==========================================
- Hits 85074 73005 -12069
- Misses 45584 58489 +12905
+ Partials 1483 652 -831
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39214/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/39214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39214/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/39214/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/39214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.84% <14.28%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.84% <14.28%> (-20.59%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39214/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39214/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/39214?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: Drill to Detail for Embedded [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39214:
URL: https://github.com/apache/superset/pull/39214#issuecomment-4211651844
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] fix: Drill to Detail for Embedded [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39214:
URL: https://github.com/apache/superset/pull/39214#issuecomment-4217333233
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save 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] fix: Drill to Detail for Embedded [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39214:
URL: https://github.com/apache/superset/pull/39214#discussion_r3055667838
##########
tests/integration_tests/datasource_tests.py:
##########
@@ -543,6 +553,10 @@ def test_get_samples_embedded_user(
resp = self.client.post(uri, json={})
assert resp.status_code == 200
+ security_manager.del_permission_role(
+ security_manager.find_role("Gamma"), perm_view
+ )
Review Comment:
great catch! I'll implement this
--
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