mikebridge commented on code in PR #40129:
URL: https://github.com/apache/superset/pull/40129#discussion_r3342757698
##########
superset/charts/api.py:
##########
@@ -122,6 +129,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
RouteMethod.IMPORT,
RouteMethod.RELATED,
"bulk_delete", # not using RouteMethod since locally defined
+ "restore",
Review Comment:
Confirmed and fixed in 6a4ebdfdc7. The substance was right: FAB's
`@protect()` decorator resolves `method_permission_name.get(method_name,
method_name)`, so without an entry for `restore` the gate was falling back to
`can_restore_Chart` — a permission no standard role carries. Admin-only
integration tests masked the gap because Admin bypasses FAB permission checks
entirely.
Fix mirrors the suggested diff and the existing `themes/api.py` pattern:
spread `MODEL_API_RW_METHOD_PERMISSION_MAP` into a new dict and add `"restore":
"write"`.
Regression test added (`test_restore_uses_can_write_permission`) that logs
in as Alpha, who carries `can_write_Chart` but not `can_restore_Chart`.
Soft-deletes an Alpha-owned chart, hits the restore endpoint, expects 200. If
anyone removes the mapping later, the test fails with a 403 from FAB before
reaching the command layer.
The same gap existed on the dashboards (#40128) and datasets (#40130)
sibling PRs and is fixed in companion commits there. Thanks for the catch.
--
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]
