aminghadersohi commented on code in PR #44905:
URL: https://github.com/apache/superset/pull/44905#discussion_r4187079002
##########
superset/security/manager.py:
##########
@@ -4334,6 +4335,74 @@ def semantic_layer_before_update(
.values(perm=new_view_perm)
)
+ def semantic_layer_before_delete(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SemanticLayer",
+ ) -> None:
+ """
+ Remove child view permissions before the layer row is deleted.
+
+ Views the session has not loaded are deleted by the database
+ ``ON DELETE CASCADE`` (``passive_deletes=True``), so their ORM
+ ``after_delete`` hook never runs. Read their perms through the
+ connection while the rows still exist; views the ORM deletes itself
+ are already gone by now and clean up in ``semantic_view_after_delete``.
+ """
+ from superset.semantic_layers.models import ( # pylint:
disable=import-outside-toplevel
+ SemanticView,
+ )
+
+ sv_table = SemanticView.__table__ # pylint: disable=no-member
+ views: Sequence[Row[Any]] = connection.execute(
+ sv_table.select().where(sv_table.c.semantic_layer_uuid ==
target.uuid)
+ ).fetchall()
+ deleted_view_ids: set[int] = {view_row.id for view_row in views}
+ view_row: Row[Any]
+ for view_row in views:
+ if view_row.perm and not self._semantic_view_perm_owned_elsewhere(
+ connection, view_row.perm, deleted_view_ids
+ ):
+ self._delete_pvm_on_sqla_event(
+ mapper, connection, "datasource_access", view_row.perm
+ )
+
+ def _semantic_view_perm_owned_elsewhere(
+ self,
+ connection: Connection,
+ perm: str,
+ deleted_view_ids: set[int],
+ ) -> bool:
+ """
+ Whether a live resource other than the deleted views owns *perm*.
+
+ A deleted view's permission is removed only when no dataset and no
+ other semantic view still uses the same permission name; removing it
+ would otherwise revoke that resource's grants.
Review Comment:
Non-blocking: the guard is one-way. `dataset_after_delete`
(superset/security/manager.py L3958, outside this diff) still drops the PVM
unconditionally, so deleting the colliding dataset revokes grants on the
semantic view this PR now protects. Same check there would close it; fine as a
follow-up.
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -2404,3 +2418,113 @@ def
test_values_for_column_search_rejection_falls_back_unfiltered(
assert mock_implementation.get_values.call_args.args[1] is None
assert "rejected the value-search filter" in caplog.text
assert "category" in caplog.text
+
+
[email protected]("children_loaded", [False, True])
+def test_layer_delete_removes_child_view_permissions(
+ session: Any, children_loaded: bool
+) -> None:
+ """sc-123444: deleting a layer removes each child view's datasource_access
Review Comment:
Nit: internal tracker IDs carry no meaning in apache/superset; suggest
dropping it here and on L2447.
```suggestion
"""Deleting a layer removes each child view's datasource_access
```
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -2404,3 +2418,113 @@ def
test_values_for_column_search_rejection_falls_back_unfiltered(
assert mock_implementation.get_values.call_args.args[1] is None
assert "rejected the value-search filter" in caplog.text
assert "category" in caplog.text
+
+
[email protected]("children_loaded", [False, True])
+def test_layer_delete_removes_child_view_permissions(
+ session: Any, children_loaded: bool
+) -> None:
+ """sc-123444: deleting a layer removes each child view's datasource_access
+ PVM and its role grants, whether or not the views are loaded in the
session.
+
+ Unloaded views are removed by the database ``ON DELETE CASCADE``
+ (``passive_deletes=True``), so their ORM ``after_delete`` hook never runs.
+ Superset enables SQLite foreign keys on its metadata engines; enable them
+ here so the cascade behaves as it does in production.
+ """
+ from flask_appbuilder.security.sqla.models import PermissionView, Role
+ from sqlalchemy import text
+
+ from superset import security_manager
+
+ session.execute(text("PRAGMA foreign_keys=ON"))
+ SemanticLayer.metadata.create_all(session.get_bind())
+ layer: SemanticLayer = SemanticLayer(
+ uuid=uuid.uuid4(), name="Deleted Layer", type="test",
configuration="{}"
+ )
+ session.add(layer)
+ session.flush()
+ # Two children: while ``semantic_views`` is mapped as a scalar (SC-123445),
Review Comment:
Same here.
```suggestion
# Two children: while ``semantic_views`` is mapped as a scalar,
```
--
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]
