sha174n commented on code in PR #45166:
URL: https://github.com/apache/superset/pull/45166#discussion_r4257887939
##########
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:
Added `test_trash_actions_require_datasource_grant_for_editor` in
fff9f2b642: an editor with dataset write but no grant gets 404 from
`purge-impact`, `purge` and `restore`, then 200 once granted; dropping this
line (or flipping `skip_base_filter`) fails it.
##########
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:
Both descriptions now mention the grant (spec regenerated) in fff9f2b642.
##########
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:
Dropped the re-lookup and updated the test to assert a single base-filtered
lookup that never reaches the editorship gate, in fff9f2b642.
##########
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:
Both docstrings no longer describe the ownership path, in fff9f2b642.
--
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]
