EnxDev commented on code in PR #45166:
URL: https://github.com/apache/superset/pull/45166#discussion_r4256967448
##########
superset/datasets/api.py:
##########
@@ -155,6 +155,7 @@
forbidden=DatasetForbiddenError,
delete_failed=DatasetDeleteFailedError,
impact_collector=collect_dataset_impact,
+ skip_base_filter=RestoreDatasetCommand.skip_base_filter,
Review Comment:
Nothing pins this line. The dataset purge tests run as admin or as Alpha,
who holds `all_datasource_access`, and `test_restore_purge_hoist.py` only
checks the chart and dashboard bindings, so dropping it leaves every test green.
Could we add a case where an editor with dataset write but no grant on the
dataset gets a 404 from `purge-impact` and `purge`? Same for restore, which is
only covered by the mocked `skip_base_filter=False` kwarg in `restore_test.py`.
##########
superset/security/manager.py:
##########
@@ -5088,10 +5088,7 @@ def raise_for_access( # noqa: C901
# A SQL Lab query's own author should be able to explore/chart
# the exact query they already ran without an additional
# dataset-level ``datasource_access`` grant on every table it
- # happens to touch. This mirrors the ownership bypass already
- # granted to dataset owners via ``is_editor`` further below in
- # this same method; the query path had no equivalent authorship
- # bypass.
+ # happens to touch.
Review Comment:
Two test docstrings still describe the bypass this sentence used to point
at: `tests/unit_tests/explore/utils_test.py` around line 467 and
`tests/integration_tests/explore/form_data/commands_tests.py` around line 479.
The first one says the data "becomes explorable to them the moment it's
saved as a dataset" through ownership, which is the opposite of the new rule.
Worth fixing them in the same pass.
##########
superset/mcp_service/dataset/tool/restore_dataset.py:
##########
@@ -75,12 +73,11 @@ async def _deny_restore(
) -> RestoreDatasetResponse | None:
"""Return an error response unless the caller may restore ``dataset``.
- ``_find_dataset_for_restore`` bypasses the RBAC base filter, so without
- this gate iterating identifiers would disclose the existence and exact
- name of datasets the caller cannot see (the web API answers 404 for
- those). A dataset outside the caller's RBAC scope reads as not found; a
- visible one the caller cannot edit gets a permission error naming only
- its id.
+ Runs before any response embeds the dataset's name, so iterating
+ identifiers cannot disclose the existence and exact name of datasets the
+ caller cannot see (the web API answers 404 for those). A dataset outside
+ the caller's RBAC scope reads as not found; a visible one the caller
Review Comment:
With the first lookup keeping the base filter, the re-lookup further down in
this function runs the same filtered query, so its `NotFound` branch can't be
reached any more.
`test_restore_dataset_inaccessible_dataset_reads_as_not_found` still passes,
but only because the first lookup returns `None` now. It never gets to
`_deny_restore`, and its docstring still describes an unfiltered lookup. Mind
dropping the re-lookup and updating that test?
##########
superset/datasets/api.py:
##########
@@ -1494,9 +1495,7 @@ def restore(self, uuid: str) -> Response:
@safe
@statsd_metrics
@event_logger.log_this_with_context(
- action=lambda self, *args, **kwargs: (
- f"{self.__class__.__name__}.purge_impact"
- ),
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.purge_impact",
log_to_statsd=False,
)
def purge_impact(self, uuid: str) -> Response:
Review Comment:
Nit, take it or leave it. The descriptions on this route and on `purge`
below still say "Limited to owners and admins (same audience as restore)".
An editor without a grant on the dataset gets a 404 from both after this
change, so the OpenAPI text could mention the grant.
--
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]
