sadpandajoe commented on code in PR #45034:
URL: https://github.com/apache/superset/pull/45034#discussion_r4227152589
##########
superset/security/manager.py:
##########
@@ -4484,21 +4493,62 @@ def _semantic_view_perm_owned_elsewhere(
SemanticView,
)
+ if self._retain_shared_datasource_perm_for_isolation(connection):
+ return True
+
+ self._lock_datasource_perms(connection, {perm})
+
+ # These owner probes remain unindexed by design. Retention deployments
Review Comment:
This probe runs once per purged dataset, and the comment relies on retention
deployments capping purges per run, but I can't find a cap: `_purge_model`
walks every eligible row (`_BATCH = 500` only pages the id scan). With a large
backlog of expired datasets, each unique permission now costs a scan of the
remaining `tables` and `semantic_views` rows, so purge time grows roughly
quadratically. Is there a bound I'm missing, or should `perm` be indexed / the
check batched per purge page?
##########
superset/security/manager.py:
##########
@@ -4429,6 +4425,12 @@ def semantic_layer_before_delete(
}
if not view_perms:
return
+ if self._retain_shared_datasource_perm_for_isolation(connection):
+ return
+
+ # The database can cascade unloaded views without firing their delete
+ # hooks. Serialize their permission cleanup with direct owner deletes.
+ self._lock_datasource_perms(connection, view_perms)
Review Comment:
Taking a `FOR UPDATE` lock on the view-menu row here, before the layer
delete cascades to its child views, reverses the order used by a direct view
delete (view row first, then this lock in `semantic_view_after_delete`). Two
concurrent requests — one deleting this layer, one deleting one of its views —
can each hold what the other needs, and the database will abort one of them
with a deadlock error. Is that interleaving acceptable, or should the layer
path take the permission lock after the cascade (or the view path take it
before deleting the row)?
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -5625,3 +5625,194 @@ def
test_reset_password_self_service_commits_cleared_flag(
mock_clear.assert_called_once_with(5)
# One commit for the session-invalidation stamp, one for the cleared flag.
assert mock_commit.call_count == 2
+
+
+def test_dataset_delete_uses_stored_permission_identity(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """The delete hook must retire the PVM actually stored on the dataset."""
+ sm: SupersetSecurityManager = SupersetSecurityManager(appbuilder)
+ target: MagicMock = MagicMock()
+ target.perm = "[stored](id:42)"
+ mapper: MagicMock = MagicMock()
+ connection: MagicMock = MagicMock()
+ mocker.patch.object(sm, "get_dataset_perm",
return_value="[derived](id:42)")
+ owned_elsewhere: MagicMock = mocker.patch.object(
+ sm, "_datasource_perm_owned_elsewhere", return_value=False
+ )
+ delete_pvm: MagicMock = mocker.patch.object(sm,
"_delete_pvm_on_sqla_event")
+
+ sm.dataset_after_delete(mapper, connection, target)
+
+ owned_elsewhere.assert_called_once_with(connection, "[stored](id:42)",
None)
+ delete_pvm.assert_called_once_with(
+ mapper, connection, "datasource_access", "[stored](id:42)"
+ )
+
+
+def test_dataset_delete_derives_missing_stored_permission(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """Legacy rows without a stored permission still clean up their grant."""
+ sm: SupersetSecurityManager = SupersetSecurityManager(appbuilder)
+ target: MagicMock = MagicMock()
+ target.perm = None
+ mapper: MagicMock = MagicMock()
+ connection: MagicMock = MagicMock()
+ mocker.patch.object(sm, "get_dataset_perm",
return_value="[derived](id:42)")
+ mocker.patch.object(sm, "_datasource_perm_owned_elsewhere",
return_value=False)
+ delete_pvm: MagicMock = mocker.patch.object(sm,
"_delete_pvm_on_sqla_event")
+
+ sm.dataset_after_delete(mapper, connection, target)
+
+ delete_pvm.assert_called_once_with(
+ mapper, connection, "datasource_access", "[derived](id:42)"
+ )
+
+
+def test_shared_permission_owner_probe_locks_permission_first(
+ app_context: None,
+) -> None:
+ """Competing final-owner deletes must serialize before checking owners."""
+ from sqlalchemy.dialects import postgresql
+
+ sm: SupersetSecurityManager = SupersetSecurityManager(appbuilder)
+ connection: MagicMock = MagicMock()
+ connection.execute.return_value.scalar_one_or_none.return_value = 1
+ connection.execute.return_value.first.return_value = None
+
+ assert (
+ sm._datasource_perm_owned_elsewhere( # pylint:
disable=protected-access
+ connection, "[shared](id:42)", None
+ )
+ is False
+ )
+
+ first_statement: Any = connection.execute.call_args_list[0].args[0]
+ sql: str = str(first_statement.compile(dialect=postgresql.dialect()))
+ assert "ab_view_menu" in sql
+ assert "FOR UPDATE" in sql
+
+
+def test_semantic_layer_delete_locks_child_permission_before_owner_probe(
+ app_context: None,
+) -> None:
+ """A cascading child delete must use the shared owner-deletion lock."""
+ from sqlalchemy.dialects import postgresql
+
+ sm: SupersetSecurityManager = SupersetSecurityManager(appbuilder)
+ connection: MagicMock = MagicMock()
+ connection.execute.side_effect = [
+ MagicMock(scalars=MagicMock(return_value=["[shared](id:42)"])),
+ MagicMock(all=MagicMock(return_value=[1])),
+ MagicMock(scalars=MagicMock(return_value=[])),
+ MagicMock(scalars=MagicMock(return_value=[])),
+ ]
+ target: MagicMock = MagicMock()
+ target.uuid = "layer-uuid"
+
+ sm.semantic_layer_before_delete(MagicMock(), connection, target)
+
+ lock_statement: Any = connection.execute.call_args_list[1].args[0]
+ sql: str = str(lock_statement.compile(dialect=postgresql.dialect()))
+ assert "ab_view_menu" in sql
+ assert "ORDER BY" in sql
+ assert "FOR UPDATE" in sql
+
+
[email protected]("isolation", ["REPEATABLE READ", "SERIALIZABLE",
None])
+def test_shared_permission_cleanup_retains_on_unsafe_mysql_isolation(
+ isolation: str | None, mocker: MockerFixture, app_context: None
+) -> None:
+ """An untrusted MySQL snapshot must not retire a shared grant."""
+ sm: SupersetSecurityManager = SupersetSecurityManager(appbuilder)
+ connection: MagicMock = MagicMock()
+ connection.dialect.name = "mysql"
Review Comment:
Every isolation-gate test pins `connection.dialect.name = "mysql"`, so the
`"mariadb"` entry in `_retain_shared_datasource_perm_for_isolation` is never
exercised. If MariaDB were dropped from that tuple, a `mariadb+pymysql`
metadata DB on `REPEATABLE READ` would run the owner probes on a stale snapshot
and could delete a shared PVM, with all of these tests still passing. Could you
add a case with `dialect.name = "mariadb"` and `REPEATABLE READ` asserting no
owner query and no PVM delete?
--
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]
