mikebridge commented on code in PR #40128:
URL: https://github.com/apache/superset/pull/40128#discussion_r3389952600
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -248,23 +249,109 @@ def import_dashboard( # noqa: C901
overwrite: bool = False,
ignore_permissions: bool = False,
) -> Dashboard:
+ """Import a dashboard from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ "owner" in the matrix above means the caller is in ``existing.owners``
+ OR is an admin (the ownership check is bypassed for admins). The
+ mutation path also requires ``security_manager.can_access_dashboard
+ (existing)`` to pass — a per-row RBAC check distinct from the
+ ``can_write`` model-level grant.
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dashboard back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission.
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dashboard",
)
- existing =
db.session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
+ # `user` is None for background / example-loader paths (no Flask request
+ # user). Combined with ``can_write=True`` (typically from
+ # ``ignore_permissions=True``), the ownership check below is intentionally
+ # skipped because the caller has already established trust at the command
+ # level. This matches pre-existing overwrite behaviour but now also applies
+ # to soft-deleted matches via ``needs_mutation``.
user = get_user()
- if existing:
- if overwrite and can_write and user:
+
+ if existing := find_existing_for_import(Dashboard, config["uuid"]):
+ is_soft_deleted = existing.deleted_at is not None
+ needs_mutation = overwrite or is_soft_deleted
Review Comment:
Done in 05d2ae7025 — the importer no longer fuses the two operations.
`import_dashboard` now branches explicitly on `existing.deleted_at is not None`
(RESTORE) vs `else` (OVERWRITE), each owning its own can_write/ownership gate
and per-operation error message; `needs_mutation` is gone. The same split was
applied to the charts and datasets importers so the three stay consistent.
Importer matrix tests still pass. Resolving.
##########
superset/dashboards/api.py:
##########
@@ -1213,6 +1235,64 @@ def bulk_delete(self, **kwargs: Any) -> Response:
except DashboardDeleteFailedError as ex:
return self.response_422(message=str(ex))
+ @expose("/<uuid>/restore", methods=("POST",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs:
f"{self.__class__.__name__}.restore",
+ log_to_statsd=False,
+ )
+ def restore(self, uuid: str) -> Response:
+ """Restore a soft-deleted dashboard.
+ ---
+ post:
+ summary: Restore a soft-deleted dashboard
+ parameters:
+ - in: path
+ schema:
+ type: string
+ format: uuid
+ name: uuid
+ responses:
+ 200:
+ description: Dashboard restored
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ RestoreDashboardCommand(uuid).run()
+ return self.response(200, message="OK")
+ except DashboardNotFoundError:
+ return self.response_404()
+ except DashboardForbiddenError:
+ return self.response_403()
+ except DashboardSlugConflictError as ex:
+ return self.response_422(message=str(ex))
+ except DashboardRestoreFailedError as ex:
+ logger.error(
+ "Error restoring model %s: %s",
+ self.__class__.__name__,
+ str(ex),
+ exc_info=True,
+ )
+ return self.response_422(message=str(ex))
Review Comment:
The error path is covered:
`tests/integration_tests/dashboards/soft_delete_tests.py::TestDashboardRestore::test_restore_failure_returns_422`
patches `RestoreDashboardCommand.run` to raise `DashboardRestoreFailedError`
and asserts the endpoint returns 422, which exercises this exact handler.
Resolving.
--
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]
