mikebridge commented on code in PR #45034:
URL: https://github.com/apache/superset/pull/45034#discussion_r4231640432
##########
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:
Good catch. In `c692272732` the layer callback locks its remaining child
view rows in ID order before taking the permission-row locks, which matches the
view-row-before-permission-row order of a direct view delete. The regression
test checks those two statements in order and fails against the previous
implementation. This removes the cycle you identified without dropping the
shared-owner serialization. The evidence is a SQL-order unit test, not a live
two-connection database test.
##########
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:
You're right: `_BATCH = 500` only pages the eligible IDs, and there is no
total cap on this branch. I've removed that claim in `c692272732`.
- With N deletions, T dataset rows and V semantic-view rows, the unindexed
probes can cost O(N(T+V)), so draining most of the dataset catalog can be
quadratic. `LIMIT 1` bounds the rows returned, not the rows scanned. I haven't
measured a representative large catalog.
- The per-run cap is tracked separately in #45029 (still open) rather than
duplicated here. It bounds how many purges, and so probes, run per invocation,
not each probe's scan cost.
##########
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:
Good catch. In `c692272732` the isolation tests run for both `mysql` and
`mariadb`, covering REPEATABLE READ, SERIALIZABLE, unknown and unreadable
isolation, safe READ COMMITTED, and dataset and layer deletion. The dataset
regression asserts both no owner query and no PVM delete. Removing MariaDB from
the gate makes all seven MariaDB cases fail; restoring it makes the suite pass.
--
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]
