codeant-ai-for-open-source[bot] commented on code in PR #39997:
URL: https://github.com/apache/superset/pull/39997#discussion_r3214991258
##########
superset/commands/chart/update.py:
##########
@@ -112,18 +112,22 @@ def validate(self) -> None: # noqa: C901
if not self._model:
raise ChartNotFoundError()
- # Check and update ownership; when only updating query context we
ignore
- # ownership so the update can be performed by report workers
+ # Ownership is always required; report workers updating query_context
+ # are granted access via the report schedule's own permissions, so they
+ # will pass this check. Only the owner-list update is skipped for
+ # query-context-only updates because there is no owners payload to set.
+ try:
+ security_manager.raise_for_ownership(self._model)
+ except SupersetSecurityException as ex:
+ raise ChartForbiddenError() from ex
Review Comment:
**Suggestion:** Moving ownership enforcement to always run breaks report
executions for charts whose report owner is not also a chart owner. Report
tasks execute as `ALERT_REPORTS_EXECUTORS` (default `OWNER`, i.e. report
owner), and when query context is missing they trigger a query-context-only
chart update; this now raises 403 and prevents query context from being saved,
causing CSV/TEXT report flows to fail. Keep the API ownership fix for end
users, but add an internal/report-worker path that can update query context
without requiring chart ownership (or ensure execution user is a chart
owner/admin before this call). [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ CSV report execution fails for some scheduled charts.
- ❌ TEXT report generation fails when query_context missing.
- ⚠️ Requires aligning report and chart ownership manually.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a chart `Slice` and a `ReportSchedule` pointing to this chart,
with different
owners: chart owners set via `Slice.owners` (see
`superset/models/slice.py`), and report
owners via `ReportSchedule.owners` in `superset/reports/models.py:153-157`.
Ensure at
least one report owner user `U` is not in the chart's owners list.
2. Trigger execution of the report for CSV or TEXT output so that
`ExecuteReportCommand._get_csv_data()` or `_get_embedded_data()` runs in
`superset/commands/report/execute.py:102-146,148-175`. These methods call
`get_executor(...)` with `executors=app.config["ALERT_REPORTS_EXECUTORS"]`,
which by
default is `[ExecutorType.OWNER]` as configured in
`superset/config.py:1951-1959`, causing
the task to execute as report owner `U`.
3. Ensure the chart has no saved query context so that
`self._report_schedule.chart.query_context is None` evaluates True in
`_get_csv_data` /
`_get_embedded_data`
(`superset/commands/report/execute.py:112-114,162-164`); this
triggers `self._update_query_context()`, which calls
`self._get_screenshots()` and, in
turn, `ChartScreenshot.get_screenshot(user=user)`
(`superset/utils/screenshots.py:210-214`) to load the chart Explore URL as
user `U` and
cause the frontend to issue a query-context-only chart update (`PUT
/api/v1/chart/<id>`
with only `query_context` and `query_context_generation`).
4. That PUT request is handled by `ChartRestApi.put` in
`superset/charts/api.py:380-77`,
which constructs and runs `UpdateChartCommand(pk, item)`. In
`UpdateChartCommand.validate`
(`superset/commands/chart/update.py:28-55`), the new unconditional ownership
check calls
`security_manager.raise_for_ownership(self._model)` inside the
`try`/`except` block at
lines 46-53. Because user `U` is not a chart owner or admin,
`raise_for_ownership` raises
`SupersetSecurityException`, which is translated into `ChartForbiddenError`
and then an
HTTP 403 response from `ChartRestApi.put`. The query-context-only update
fails,
`chart.query_context` remains `None`, and subsequent CSV/TEXT report
executions continue
to hit the `_update_query_context` path without ever persisting
query_context, resulting
in failed or looping CSV/TEXT report flows for charts where report owners
differ from
chart owners.
```
</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%2Fcommands%2Fchart%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20119%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Moving%20ownership%20enforcement%20to%20always%20run%20breaks%20report%20executions%20for%20charts%20whose%20report%20owner%20is%20not%20also%20a%20chart%20owner.%20Report%20tasks%20execute%20as%20%60ALERT_REPORTS_EXECUTORS%60%20%28default%20%60OWNER%60%2C%20i.e.%20report%20owner%29%2C%20and%20when%20query%20context%20is%20missing%20they%20trigger%20a%20query-context-only%20chart%20update%3B%20this%20now%20raises%20403%20and%20prevents%20query%20context%20from%20being%20saved%2C%20causing%20CSV%2FTEXT%20report%20flows%20to%20fail.%20Keep%20the%20API%20ownership%20fix%20for%20end%20users%2C%20but%20add%20an%20internal%2Freport-worker%20path%20that%20can%20update%20query%20context%20without%20requiring%20
chart%20ownership%20%28or%20ensure%20execution%20user%20is%20a%20chart%20owner%2Fadmin%20before%20this%20call%29.%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%2Fcommands%2Fchart%2Fupdate.py%0A%2A%2ALine%3A%2A%2A%20119%3A122%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Moving%20ownership%20enforcement%20to%20always
%20run%20breaks%20report%20executions%20for%20charts%20whose%20report%20owner%20is%20not%20also%20a%20chart%20owner.%20Report%20tasks%20execute%20as%20%60ALERT_REPORTS_EXECUTORS%60%20%28default%20%60OWNER%60%2C%20i.e.%20report%20owner%29%2C%20and%20when%20query%20context%20is%20missing%20they%20trigger%20a%20query-context-only%20chart%20update%3B%20this%20now%20raises%20403%20and%20prevents%20query%20context%20from%20being%20saved%2C%20causing%20CSV%2FTEXT%20report%20flows%20to%20fail.%20Keep%20the%20API%20ownership%20fix%20for%20end%20users%2C%20but%20add%20an%20internal%2Freport-worker%20path%20that%20can%20update%20query%20context%20without%20requiring%20chart%20ownership%20%28or%20ensure%20execution%20user%20is%20a%20chart%20owner%2Fadmin%20before%20this%20call%29.%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%20f
ix%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/commands/chart/update.py
**Line:** 119:122
**Comment:**
*Logic Error: Moving ownership enforcement to always run breaks report
executions for charts whose report owner is not also a chart owner. Report
tasks execute as `ALERT_REPORTS_EXECUTORS` (default `OWNER`, i.e. report
owner), and when query context is missing they trigger a query-context-only
chart update; this now raises 403 and prevents query context from being saved,
causing CSV/TEXT report flows to fail. Keep the API ownership fix for end
users, but add an internal/report-worker path that can update query context
without requiring chart ownership (or ensure execution user is a chart
owner/admin before this call).
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%2F39997&comment_hash=e862859ae6903fa7221a76b7cbfeebebbb6269cea8a06ea28f3b68296b8a0661&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39997&comment_hash=e862859ae6903fa7221a76b7cbfeebebbb6269cea8a06ea28f3b68296b8a0661&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]
