EnxDev commented on PR #41133:
URL: https://github.com/apache/superset/pull/41133#issuecomment-4966665114
## EnxDev's Review Agent — apache/superset#41133 · HEAD 2adff6a
**request changes** — the newly-added `@permission_name("export")` is inert,
so the endpoint still enforces a *distinct* `can_export_xlsx` permission that
the frontend gate (`can_export`) never checks — the FE/BE mismatch I flagged
last round is not actually fixed. CI is now green (the earlier `test-mysql` +
`pre-commit` red is resolved).
_Supersedes my [prior review at HEAD
eac1480](https://github.com/apache/superset/pull/41133#issuecomment-4961525897)._
Feature PR (async dashboard→Excel). SQL/Jinja safety is delegated to
`ChartDataCommand` run under `override_user` (per-chart `raise_for_access`
applies), and the formula-injection / sheet-name guards are solid — so the
focus is the authz gate and the maintainer's open threads.
### 🔴 Functional
- **`superset/dashboards/api.py:853`** · _High_ —
`@permission_name("export")` does nothing here, so the endpoint is gated by a
brand-new `can_export_xlsx` the frontend never checks. `DashboardRestApi` sets
an explicit `method_permission_name` (line 291 =
`MODEL_API_RW_METHOD_PERMISSION_MAP` + `"restore"`); FAB then ignores
per-method `@permission_name`, and any method absent from that dict falls back
to `can_<method>`. `export_xlsx` isn't in it → required perm =
`can_export_xlsx`. Proof chain: `export_as_example` carries the identical
`@protect`/`@safe`/`@permission_name("export")` stack yet resolves to
`can_export_as_example`; and this PR's `test_info_security_dashboard` (an
exact-set assertion) adds `can_export_xlsx` and passes CI. The FE gates the
menu on `can_export` (`superset-frontend/src/dashboard/actions/hydrate.ts:347`
→ `dash_export_perm` → `userCanExport`). The two gates diverge:
`can_export_xlsx` is a fresh permission, not in Gamma's allowlist and
unassigned
to any role until a security sync runs — so a Gamma (or any not-yet-resynced)
user holding dashboard `can_export` sees "Export Data to Excel", clicks it, and
`@protect()` returns 403 → the FE maps any non-501 to the generic "Sorry,
something went wrong" toast. This also contradicts the PR's own "gated on
`can_export`" claim and the inert decorator's intent. Fix: add `"export_xlsx":
"export"` to `method_permission_name` (mirroring `"restore": "write"`), drop
the decorator, and revert the test line back to `can_export` — exactly what
@betodealmeida asked on `api_tests.py:738`. **regression test:** grant a role
dashboard `can_export` but not `can_export_xlsx`, POST `/export_xlsx/`, assert
202 (if reuse is intended) not 403; and assert `can_export_xlsx` is absent from
the dashboard `_info` permission set.
### 🟡 Should-fix
- **`superset/dashboards/api.py:859` (`mode="images"`)** — the endpoint
accepts `mode:"images"` regardless of the webdriver screenshot flags
(`ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS`,
`ENABLE_DASHBOARD_DOWNLOAD_WEBDRIVER_SCREENSHOT`) that gate it in the UI;
schema validation only checks `OneOf(["data","images"])`. Any export-permitted
user can POST it directly and drive N expensive headless renders the operator
disabled those flags to prevent — and with no webdriver every non-table chart
renders empty while they still get a "success" email. Validate `mode="images"`
against the same flags the menu checks. **regression test:** POST
`mode:"images"` with the screenshot flags off → 400/403, task not enqueued.
- **`superset/utils/excel_streaming.py`** — @betodealmeida's
"constant-memory streaming" thread is still open. `_write_chart_sheets`
materializes each chart's full result set (`command.run()` →
`result["queries"][i]["data"]` is an in-memory list), so peak memory is bounded
by the largest chart regardless of `constant_memory` mode — the docstring now
concedes this ("bounds only the writer's own footprint, not the caller's"). The
module still earns its place on multi-sheet + injection +
sheet-name-sanitization grounds, but the maintainer's "just reuse
`superset.utils.excel`" question is unresolved and the review carries a formal
CHANGES_REQUESTED.
### 🔵 Nits
- Most earlier inline asks *are* addressed at this HEAD — i18n `gettext`
wrappers, the `_humanize_ttl` helper, `boto3` in `pyproject.toml`, the
distributed-lock throttle, the truncation-notice row, and the `lstrip` before
the formula check. Good turnaround.
- `VERIFY_EXCEL_EXPORT.md` + `docker-compose-excel-verify.yml` read as
personal verification scaffolding (`amman-excel` project, `/etc/hosts` steps).
Dropping them also removes the need for the `check-yaml` exclusion added to
`.pre-commit-config.yaml` for the `!reset` tag.
- The already-in-progress branch returns `202` with an "already in progress"
message, but the FE treats any non-throw as success and shows "Your export is
being prepared…" — mildly misleading; no new export was enqueued.
### 🙌 Praise
- `superset/dashboards/api.py:948-964` — acquiring the in-flight lock before
enqueue and releasing it if `apply_async` throws (instead of leaking it until
the TTL) is a clean touch.
- `superset/utils/excel_streaming.py:_quote_if_formula` (lstrip before the
check) + sheet-name sanitize/de-dupe, and letting `SoftTimeLimitExceeded`
bypass the broad `except` in both the task and `screenshot.py` — the exact edge
cases prior review raised, handled and tested.
<!-- enxdev-review-agent:2adff6a -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 2adff6a._
--
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]
