You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2026/08/21 16:10:49 UTC
[PR] fix(security): surface extra_editors in dashboard/chart lists [superset]
EnxDev opened a new pull request, #43392:
URL: https://github.com/apache/superset/pull/43392
### SUMMARY
`extra_editors` — editorship a deployment grants indirectly via `EXTRA_EDITORS_RESOLVER` — was attached only to single-object `GET` responses (`DashboardRestApi.get` / `ChartRestApi.get`), never to list responses. `DashboardCard`, `ChartCard`, and the Dashboard/Chart list pages also checked `editors` only. Combined, a resolver-granted editor saw the Edit affordance disabled on cards/list rows even though the server's `is_editor` already let them save once the dashboard/chart was open — a UI-affordance gap, not an authorization bug (confirmed via `raise_for_editorship`/`is_editor`, which already union `editors` with the resolver output).
- `superset/security/manager.py`: new `get_extra_editors_by_pk` batch-resolves
extra editors for a page of rows in one query, re-querying by the primary
keys FAB already selected (list responses only expose serialized rows to
`pre_get_list`, not model instances).
- `superset/dashboards/api.py` / `superset/charts/api.py`: a new
`pre_get_list` attaches `extra_editors` per row, gated on
`EXTRA_EDITORS_RESOLVER` being configured — zero extra cost when it isn't.
- `permissionUtils.ts`'s `isUserEditorOrAdmin` now takes an optional
`extraEditors` argument; `DashboardCard`, `ChartCard`, `DashboardList`, and
`ChartList` pass `extra_editors` alongside `editors`.
- `DatasetList` is untouched — datasets never got `EXTRA_EDITORS_RESOLVER`
support, so its `editors`-only check is already correct.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/user-attachments/assets/ec722c9d-7e11-43de-baba-aa04983de661
### TESTING INSTRUCTIONS
- `pytest tests/integration_tests/dashboards/api_tests.py -k extra_editors`
— 4 new tests: list responses omit/include `extra_editors` for
dashboards and charts depending on whether `EXTRA_EDITORS_RESOLVER` is
configured.
- `npm run test -- permissionUtils.test.ts` — new cases for
`isUserEditorOrAdmin`'s `extraEditors` union/precedence.
- Manually: configure `EXTRA_EDITORS_RESOLVER`, grant a non-editor user
editorship on a dashboard they can view but aren't listed as an editor
of, then confirm the dashboard list/card Edit affordance is enabled for
them.
### 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(security): surface extra_editors in dashboard/chart lists [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #43392:
URL: https://github.com/apache/superset/pull/43392
--
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(security): surface extra_editors in dashboard/chart lists [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43392:
URL: https://github.com/apache/superset/pull/43392#issuecomment-5394437420
**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(security): surface extra_editors in dashboard/chart lists [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 #43392:
URL: https://github.com/apache/superset/pull/43392#discussion_r3831852424
##########
superset/dashboards/api.py:
##########
@@ -431,6 +434,15 @@ def get_list(self, **kwargs: Any) -> Response:
"""
return super().get_list(**kwargs)
+ def pre_get_list(self, data: dict[str, Any]) -> None:
+ """Attach ``extra_editors`` to each row, matching the single-object GET."""
+ super().pre_get_list(data)
+ ids = data.get("ids", [])
+ extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids)
Review Comment:
**Suggestion:** The list endpoint now invokes `EXTRA_EDITORS_RESOLVER` once for every dashboard in the page through `get_extra_editors_by_pk`. A resolver that performs I/O or is computationally expensive therefore turns a single list request into one resolver execution per row, multiplying latency and potentially causing the entire listing to fail if any resolver invocation raises. Use a bounded or batch-aware resolution strategy, or isolate resolver failures so one problematic resource does not break the whole list response. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Configured dashboard lists incur resolver work per row.
- ❌ Resolver failure can make dashboard listings return an API error.
- ⚠️ Large dashboard pages amplify custom resolver latency.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 441:441
**Comment:**
*Performance: The list endpoint now invokes `EXTRA_EDITORS_RESOLVER` once for every dashboard in the page through `get_extra_editors_by_pk`. A resolver that performs I/O or is computationally expensive therefore turns a single list request into one resolver execution per row, multiplying latency and potentially causing the entire listing to fail if any resolver invocation raises. Use a bounded or batch-aware resolution strategy, or isolate resolver failures so one problematic resource does not break the whole list response.
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%2F43392&comment_hash=7f4746f181de19cf4cafff6ce649d4f39213c2b1c064ccbfff0d8abd26e8926d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43392&comment_hash=7f4746f181de19cf4cafff6ce649d4f39213c2b1c064ccbfff0d8abd26e8926d&reaction=dislike'>👎</a>
##########
superset/charts/api.py:
##########
@@ -410,6 +413,15 @@ def get(self, id_or_uuid: str) -> Response:
except ChartNotFoundError:
return self.response_404()
+ def pre_get_list(self, data: dict[str, Any]) -> None:
+ """Attach ``extra_editors`` to each row, matching the single-object GET."""
+ super().pre_get_list(data)
+ ids = data.get("ids", [])
+ extra_editors_by_id = get_extra_editors_by_pk(Slice, ids)
Review Comment:
**Suggestion:** The chart list path performs one `EXTRA_EDITORS_RESOLVER` call for each chart returned by the page through `get_extra_editors_by_pk`. This multiplies resolver latency with page size and propagates any resolver exception as a list-request failure, unlike the previous list behavior. Use a bounded or batch-aware resolution strategy, or handle per-chart resolver failures without aborting the entire response. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Configured chart lists incur resolver work per returned chart.
- ❌ Resolver failure can make chart listings return an API error.
- ⚠️ Large chart pages amplify custom resolver latency.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/charts/api.py
**Line:** 420:420
**Comment:**
*Performance: The chart list path performs one `EXTRA_EDITORS_RESOLVER` call for each chart returned by the page through `get_extra_editors_by_pk`. This multiplies resolver latency with page size and propagates any resolver exception as a list-request failure, unlike the previous list behavior. Use a bounded or batch-aware resolution strategy, or handle per-chart resolver failures without aborting the entire response.
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%2F43392&comment_hash=50d7e7d0ce2edb6eb833464c85009f83ceacb60ba83c05e7e933c26cc52a0271&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43392&comment_hash=50d7e7d0ce2edb6eb833464c85009f83ceacb60ba83c05e7e933c26cc52a0271&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(security): surface extra_editors in dashboard/chart lists [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43392:
URL: https://github.com/apache/superset/pull/43392#issuecomment-5372349671
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(security): surface extra_editors in dashboard/chart lists [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43392:
URL: https://github.com/apache/superset/pull/43392#issuecomment-5372393617
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43392?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 `20.83333%` with `19 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.33%. Comparing base ([`05842a6`](https://app.codecov.io/gh/apache/superset/commit/05842a6350f41bfcd57982b45b34bc8e29d81680?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5aa2b45`](https://app.codecov.io/gh/apache/superset/commit/5aa2b451f15ca6ab3845aa5ebbcdd41984c2f693?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/43392?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/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/43392?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==) | 12.50% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/charts/api.py](https://app.codecov.io/gh/apache/superset/pull/43392?src=pr&el=tree&filepath=superset%2Fcharts%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/dashboards/api.py](https://app.codecov.io/gh/apache/superset/pull/43392?src=pr&el=tree&filepath=superset%2Fdashboards%2Fapi.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43392?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 (05842a6) and HEAD (5aa2b45). Click for more details.
>
> <details><summary>HEAD has 59 uploads less than BASE</summary>
>
>| Flag | BASE (05842a6) | HEAD (5aa2b45) |
>|------|------|------|
>|python|52|1|
>|presto|9|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43392 +/- ##
==========================================
- Coverage 66.84% 57.33% -9.52%
==========================================
Files 2876 2876
Lines 164132 164252 +120
Branches 37875 37920 +45
==========================================
- Hits 109719 94173 -15546
- Misses 52238 69185 +16947
+ Partials 2175 894 -1281
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43392/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/43392/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/43392/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/43392/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/43392/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.02% <20.83%> (-0.05%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43392/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.02% <20.83%> (-19.29%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43392/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/43392/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 Harness](https://app.codecov.io/gh/apache/superset/pull/43392?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