codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3593612819
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -3262,6 +3264,173 @@ def
test_get_all_related_viewers_with_extra_filters(self):
response = json.loads(rv.data.decode("utf-8"))
assert response["count"] > 0
+ def test_export_xlsx_501_when_bucket_unset(self):
+ """Dashboard API: export_xlsx returns 501 when the S3 bucket is
unset."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-501", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 501
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_missing_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 404 for an unknown dashboard."""
+ self.login(ADMIN_USERNAME)
+ rv = self.client.post("api/v1/dashboard/99999999/export_xlsx/")
+ assert rv.status_code == 404
+ mock_task.apply_async.assert_not_called()
+
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_400_for_empty_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 400 for a dashboard with no
charts."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard("xlsx-empty", None, [admin.id])
+ self.login(ADMIN_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 400
+ mock_task.apply_async.assert_not_called()
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.AcquireDistributedLock")
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_202_enqueues_task(self, mock_task, mock_acquire):
+ """Dashboard API: export_xlsx enqueues the task and returns 202 +
job_id."""
+ self.login(ADMIN_USERNAME)
+ dashboard =
db.session.query(Dashboard).filter_by(slug="world_health").first()
+ rv = self.client.post(
+ f"api/v1/dashboard/{dashboard.id}/export_xlsx/",
+ json={"active_data_mask": {}},
+ )
+ assert rv.status_code == 202
+ body = json.loads(rv.data.decode("utf-8"))
+ job_id = body["job_id"]
+ assert job_id
+ # The in-flight lock is acquired before the task is enqueued.
+ mock_acquire.return_value.run.assert_called_once()
+ mock_task.apply_async.assert_called_once()
+ _, kwargs = mock_task.apply_async.call_args
+ assert kwargs["task_id"] == job_id
+ assert kwargs["kwargs"]["dashboard_id"] == dashboard.id
+
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.AcquireDistributedLock")
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_202_when_export_already_in_progress(
+ self, mock_task, mock_acquire
+ ):
+ """Dashboard API: export_xlsx does not enqueue a second concurrent
export."""
+ # An in-flight lock is already held for this user+dashboard.
+ mock_acquire.return_value.run.side_effect =
LockAlreadyHeldException("held")
+ self.login(ADMIN_USERNAME)
+ dashboard =
db.session.query(Dashboard).filter_by(slug="world_health").first()
+ rv = self.client.post(
+ f"api/v1/dashboard/{dashboard.id}/export_xlsx/",
+ json={"active_data_mask": {}},
+ )
+ assert rv.status_code == 202
+ assert "already in progress" in rv.data.decode("utf-8")
+ mock_task.apply_async.assert_not_called()
+
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ def test_export_xlsx_404_for_inaccessible_dashboard(self, mock_task):
+ """Dashboard API: export_xlsx returns 404 for a dashboard the user
can't see."""
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "xlsx-private", None, [admin.id], published=False
+ )
+ self.login(GAMMA_USERNAME)
+ try:
+ rv =
self.client.post(f"api/v1/dashboard/{dashboard.id}/export_xlsx/")
+ assert rv.status_code == 404
+ mock_task.apply_async.assert_not_called()
+ finally:
+ db.session.delete(dashboard)
+ db.session.commit()
+
+ @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
+ @with_config({"EXCEL_EXPORT_S3_BUCKET": "exports"})
+ @patch("superset.dashboards.api.AcquireDistributedLock")
+ @patch("superset.dashboards.api.export_dashboard_excel")
+ @patch("superset.dashboards.api.security_manager.raise_for_access")
+ def test_export_xlsx_admitted_with_can_export_only(
+ self, mock_raise, mock_task, mock_acquire
+ ):
Review Comment:
**Suggestion:** This test patches out `security_manager.raise_for_access`,
which bypasses a real authorization check in the endpoint and can let the test
pass even if access-control behavior regresses. Remove this patch (or assert it
is called with the expected dashboard) so the test validates the true
integration of permission mapping plus dashboard access logic. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Excel export auth regressions may go undetected by tests.
⚠️ Test passes even if raise_for_access call removed.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the dashboard Excel export endpoint implementation at
`superset/dashboards/api.py:1588-1619` and
`superset/dashboards/api.py:1600-1675`. The
`DashboardRestApi.export_xlsx` method (decorated with
`@expose("/<pk>/export_xlsx/",
methods=("POST",))` and `@protect()`) calls
`security_manager.raise_for_access(dashboard=dashboard)` inside a `try/except
SupersetSecurityException` block to enforce a 403 when the user lacks access.
2. Inspect the integration test
`test_export_xlsx_admitted_with_can_export_only` in
`tests/integration_tests/dashboards/api_tests.py:164-193` (corresponding to
diff lines
3363-3393). The test is decorated with
`@patch("superset.dashboards.api.security_manager.raise_for_access")` and
receives
`mock_raise` but never asserts on it; `raise_for_access` is replaced with a
MagicMock
during the test run.
3. Run the test suite (e.g. `pytest
tests/integration_tests/dashboards/api_tests.py::TestDashboardApi::test_export_xlsx_admitted_with_can_export_only`).
During this test, the real authorization logic inside
`security_manager.raise_for_access`
(implemented in `superset/security/manager.py` and exercised by
`tests/unit_tests/subjects/test_raise_for_access.py`) is completely bypassed
because the
patched MagicMock never raises `SupersetSecurityException`, regardless of
actual dashboard
access rules.
4. To see the false-positive behavior concretely, deliberately break the
endpoint’s access
check by removing or commenting out the
`security_manager.raise_for_access(dashboard=dashboard)` call in
`superset/dashboards/api.py:72-75` (inside `export_xlsx`) and rerun
`test_export_xlsx_admitted_with_can_export_only`. The test still passes
(asserting only
status 202 and `export_dashboard_excel.apply_async` being called) even
though the endpoint
no longer performs the dashboard access check, proving that the current
patching strategy
lets authorization regressions slip through.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5f7405b38ae547c4bd36a85bdca521a7&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=5f7405b38ae547c4bd36a85bdca521a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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:** tests/integration_tests/dashboards/api_tests.py
**Line:** 3367:3370
**Comment:**
*Incomplete Implementation: This test patches out
`security_manager.raise_for_access`, which bypasses a real authorization check
in the endpoint and can let the test pass even if access-control behavior
regresses. Remove this patch (or assert it is called with the expected
dashboard) so the test validates the true integration of permission mapping
plus dashboard access logic.
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%2F41133&comment_hash=cf19d27cc6071ac22e53ee19f9a98f89a8d1a28d1b36953f419304bd578b25fd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=cf19d27cc6071ac22e53ee19f9a98f89a8d1a28d1b36953f419304bd578b25fd&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]
