This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 0484f9c2091 fix(database): sync schema_access permissions on import
(#43950)
0484f9c2091 is described below
commit 0484f9c20918041c15eff75a186d4ee3696a13c5
Author: Joe Li <[email protected]>
AuthorDate: Wed Sep 9 11:50:20 2026 -0700
fix(database): sync schema_access permissions on import (#43950)
Co-authored-by: Claude Sonnet 5 <[email protected]>
---
superset/commands/database/importers/v1/utils.py | 31 +++-
superset/commands/database/utils.py | 27 +++-
.../databases/importers/v1/command_test.py | 52 +++++++
.../databases/commands/importers/v1/import_test.py | 162 +++++++++++++++++++++
tests/unit_tests/databases/commands/utils_test.py | 128 +++++++++++++++-
5 files changed, 391 insertions(+), 9 deletions(-)
diff --git a/superset/commands/database/importers/v1/utils.py
b/superset/commands/database/importers/v1/utils.py
index 5357500c197..b8637de3da7 100644
--- a/superset/commands/database/importers/v1/utils.py
+++ b/superset/commands/database/importers/v1/utils.py
@@ -32,7 +32,7 @@ from superset.commands.exceptions import ImportFailedError
from superset.constants import PASSWORD_MASK
from superset.databases.ssh_tunnel.models import SSHTunnel
from superset.databases.utils import make_url_safe
-from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
+from superset.db_engine_specs.exceptions import SupersetDBAPIError
from superset.exceptions import (
OAuth2RedirectError,
SupersetSecurityException,
@@ -96,6 +96,21 @@ def _refuse_stored_secret_reuse(existing: Database, config:
dict[str, Any]) -> N
)
+def _sync_permissions_best_effort(database: Database) -> None:
+ """
+ Sync catalog/schema permissions for ``database``, tolerating a transient
+ or OAuth2 failure rather than letting it fail the import.
+ """
+ try:
+ add_permissions(database)
+ except (SupersetDBAPIError, OAuth2RedirectError) as ex:
+ # ``add_permissions()`` calls ``get_all_catalog_names()`` outside of
+ # its own per-catalog error handling, so any DBAPI error mapped from
+ # that initial catalog discovery -- not just a connection failure --
+ # must be tolerated here too, or it fails the whole import.
+ logger.warning(ex.message)
+
+
def import_database( # noqa: C901
config: dict[str, Any],
overwrite: bool = False,
@@ -108,6 +123,15 @@ def import_database( # noqa: C901
existing =
db.session.query(Database).filter_by(uuid=config["uuid"]).first()
if existing:
if not overwrite or not can_write:
+ if can_write:
+ # Chart/dataset/saved-query/dashboard bundles that reference
+ # an already-imported database reach this branch; without
+ # this, a schema added to the live connection since the
+ # database was first imported would never get a first-time
+ # grant through this path either. ``add_permissions()`` does
+ # a live, uncached metadata scan, so this can be slow for
+ # cross-catalog-enabled engines -- see its own comment.
+ _sync_permissions_best_effort(existing)
return existing
config["id"] = existing.id
# Stored secrets must not be rebound to a different endpoint: without
@@ -184,9 +208,6 @@ def import_database( # noqa: C901
recursive=False,
)
- try:
- add_permissions(database)
- except (SupersetDBAPIConnectionError, OAuth2RedirectError) as ex:
- logger.warning(ex.message)
+ _sync_permissions_best_effort(database)
return database
diff --git a/superset/commands/database/utils.py
b/superset/commands/database/utils.py
index 0d40516419c..a4be6fce731 100644
--- a/superset/commands/database/utils.py
+++ b/superset/commands/database/utils.py
@@ -36,6 +36,7 @@ from superset.constants import PASSWORD_MASK
from superset.databases.ssh_tunnel.models import SSHTunnel
from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
+from superset.exceptions import OAuth2RedirectError
from superset.models.core import Database
from superset.security.manager import SupersetSecurityManager
from superset.utils import json
@@ -147,6 +148,29 @@ def ping(engine: Engine) -> bool:
return engine.dialect.do_ping(conn)
+def _get_all_schema_names_with_retry(
+ database: Database, catalog: str | None
+) -> set[str]:
+ """
+ Retry the live schema-listing call once before giving up on a catalog.
+
+ Some catalogs are visible but not listable (eg the ``rdsadmin`` catalog on
+ AWS RDS), but the exception caught by the caller doesn't distinguish that
+ from a one-off transient hiccup (eg schema metadata not yet visible right
+ after it was created). A single retry lets a schema that needs a
+ first-time grant survive a fluke without tolerating a persistently
+ unlistable catalog for any longer than before.
+ """
+ try:
+ return database.get_all_schema_names(catalog=catalog, cache=False)
+ except OAuth2RedirectError:
+ # Not transient: retrying would just kick off a second, redundant
+ # OAuth2 authorization redirect for the same request.
+ raise
+ except GenericDBException: # pylint: disable=broad-except
+ return database.get_all_schema_names(catalog=catalog, cache=False)
+
+
def add_permissions(database: Database) -> None:
"""
Add DAR for catalogs and schemas.
@@ -179,7 +203,8 @@ def add_permissions(database: Database) -> None:
for catalog in catalogs:
try:
- for schema in database.get_all_schema_names(catalog=catalog,
cache=False):
+ schemas = _get_all_schema_names_with_retry(database, catalog)
+ for schema in schemas:
security_manager.add_permission_view_menu(
"schema_access",
security_manager.get_schema_perm(
diff --git a/tests/unit_tests/commands/databases/importers/v1/command_test.py
b/tests/unit_tests/commands/databases/importers/v1/command_test.py
index bb4228af86f..66e547e39d4 100644
--- a/tests/unit_tests/commands/databases/importers/v1/command_test.py
+++ b/tests/unit_tests/commands/databases/importers/v1/command_test.py
@@ -133,3 +133,55 @@ def test_import_database_with_password_in_config(
uuid = configs["databases/examples.yaml"]["uuid"]
database = db.session.query(Database).filter_by(uuid=uuid).one()
assert database.password == "yaml_password" # noqa: S105
+
+
+def test_transient_schema_listing_failure_is_recovered_by_retry(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ A single transient failure while listing schemas for a connection must
+ not permanently discard the schema_access grant for a schema that
+ appeared on the live connection since the last successful sync. Before
+ this fix, ``add_permissions()`` gave up on a catalog after one failed
+ attempt at ``get_all_schema_names()`` -- even though the exception
+ caught (``GenericDBException``, a bare alias for the built-in
+ ``Exception``) does not distinguish a genuinely unlistable catalog from
+ a one-off connector hiccup -- so a schema that needed a first-time grant
+ during the failing attempt was never granted, silently, while the
+ import still reported success.
+ """
+ from superset import db, security_manager
+ from superset.commands.database.importers.v1 import ImportDatabasesCommand
+ from superset.db_engine_specs.sqlite import SqliteEngineSpec
+ from superset.models.core import Database
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ add_permission_view_menu = mocker.spy(security_manager,
"add_permission_view_menu")
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ configs = copy.deepcopy(databases_config)
+
+ # First import (existing=None): only "old_schema" exists on the live
+ # target connection.
+ mocker.patch.object(
+ SqliteEngineSpec, "get_schema_names", return_value={"old_schema"}
+ )
+ ImportDatabasesCommand._import(copy.deepcopy(configs))
+ add_permission_view_menu.reset_mock()
+
+ # A new schema now exists, but the first attempt to list schemas for
+ # this connection hits a transient failure (e.g. a driver hiccup right
+ # after the schema was created). Re-importing with overwrite=True --
+ # the only way to legitimately re-import an existing connection -- must
+ # still end up granting the new schema once the retry succeeds.
+ mocker.patch.object(
+ SqliteEngineSpec,
+ "get_schema_names",
+ side_effect=[Exception("transient driver error"), {"old_schema",
"new_schema"}],
+ )
+ ImportDatabasesCommand._import(copy.deepcopy(configs), overwrite=True)
+
+ add_permission_view_menu.assert_any_call("schema_access",
"[examples].[new_schema]")
diff --git a/tests/unit_tests/databases/commands/importers/v1/import_test.py
b/tests/unit_tests/databases/commands/importers/v1/import_test.py
index 27c2e4c6f69..94ca05d251a 100644
--- a/tests/unit_tests/databases/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/databases/commands/importers/v1/import_test.py
@@ -344,6 +344,129 @@ def
test_import_database_with_masked_encrypted_extra_existing_db(
assert encrypted["credentials_info"]["private_key"] != PASSWORD_MASK
+def test_import_database_existing_no_overwrite_backfills_permissions(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ ``import_database(config, overwrite=False)`` is called unconditionally by
+ the chart, dataset, and saved-query bundle importers
+ (``superset/commands/chart/importers/v1/__init__.py``,
+ ``superset/commands/dataset/importers/v1/__init__.py``, and
+ ``superset/commands/query/importers/v1/__init__.py``), and by the
+ dashboard importer
(``superset/commands/dashboard/importers/v1/__init__.py``)
+ whenever both ``overwrite``/``overwrite_all`` aren't simultaneously
+ ``True`` -- the common case -- whenever the bundle references a
+ database, including one that already exists in the target, eg because a
+ prior import already created it. Before this fix, the early return for
+ an existing, non-overwritten database skipped ``add_permissions()``
+ entirely, so a schema added to the live connection since that database
+ was first imported would never get a first-time grant through this path
+ either.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import database_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mock_add_permissions = mocker.patch(
+ "superset.commands.database.importers.v1.utils.add_permissions"
+ )
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config)
+ existing = import_database(config)
+ mock_add_permissions.reset_mock()
+
+ again = copy.deepcopy(database_config)
+ result = import_database(again, overwrite=False)
+
+ assert result.id == existing.id
+ mock_add_permissions.assert_called_once_with(existing)
+
+
+def test_import_database_existing_no_overwrite_no_permission_skips_backfill(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ The permission backfill added for the existing/no-overwrite branch must
+ stay behind the same ``can_write`` gate as every other permission-view
+ creation in ``import_database()`` -- a principal without database-write
+ access must not trigger it just by importing a
+ chart/dataset/saved-query/dashboard bundle that happens to reference an
+ existing database.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import database_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mock_add_permissions = mocker.patch(
+ "superset.commands.database.importers.v1.utils.add_permissions"
+ )
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config)
+ existing = import_database(config)
+ mock_add_permissions.reset_mock()
+
+ mocker.patch.object(security_manager, "can_access", return_value=False)
+
+ again = copy.deepcopy(database_config)
+ result = import_database(again, overwrite=False)
+
+ assert result.id == existing.id
+ mock_add_permissions.assert_not_called()
+
+
+def
test_import_database_existing_no_overwrite_backfill_oauth2_redirect_is_nonfatal(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ The permission backfill on the existing/no-overwrite branch must tolerate
+ an ``OAuth2RedirectError`` from ``add_permissions()`` the same way the
+ fresh-import path already does (see
+ ``test_import_database_oauth2_redirect_is_nonfatal``) -- logged, not
+ propagated, so the import still returns the existing database normally.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.exceptions import OAuth2RedirectError
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import database_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mock_add_permissions = mocker.patch(
+ "superset.commands.database.importers.v1.utils.add_permissions"
+ )
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config)
+ existing = import_database(config)
+ mock_add_permissions.reset_mock()
+ mock_add_permissions.side_effect = OAuth2RedirectError(
+ url="https://oauth.example.com/authorize";,
+ tab_id="abc-123",
+ redirect_uri="https://superset.example.com/callback";,
+ )
+
+ again = copy.deepcopy(database_config)
+ result = import_database(again, overwrite=False)
+
+ assert result.id == existing.id
+ mock_add_permissions.assert_called_once_with(existing)
+
+
def test_import_database_oauth2_redirect_is_nonfatal(
mocker: MockerFixture,
session: Session,
@@ -378,6 +501,45 @@ def test_import_database_oauth2_redirect_is_nonfatal(
mock_add_perms.assert_called_once_with(database)
+def test_import_database_dbapi_error_during_backfill_is_nonfatal(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ ``add_permissions()`` calls ``get_all_catalog_names()`` for catalog
+ discovery outside of its own per-catalog error handling, so a mapped
+ DBAPI error from that call -- not just a connection failure -- must be
+ tolerated the same way an ``OAuth2RedirectError`` already is, instead of
+ failing the whole import.
+ """
+ from superset import security_manager
+ from superset.commands.database.importers.v1.utils import import_database
+ from superset.db_engine_specs.exceptions import
SupersetDBAPIProgrammingError
+ from superset.models.core import Database
+ from tests.integration_tests.fixtures.importexport import database_config
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mock_add_permissions = mocker.patch(
+ "superset.commands.database.importers.v1.utils.add_permissions"
+ )
+
+ engine = db.session.get_bind()
+ Database.metadata.create_all(engine) # pylint: disable=no-member
+
+ config = copy.deepcopy(database_config)
+ existing = import_database(config)
+ mock_add_permissions.reset_mock()
+ mock_add_permissions.side_effect = SupersetDBAPIProgrammingError(
+ "permission denied for catalog discovery"
+ )
+
+ again = copy.deepcopy(database_config)
+ result = import_database(again, overwrite=False)
+
+ assert result.id == existing.id
+ mock_add_permissions.assert_called_once_with(existing)
+
+
def test_import_datasources_cli_encrypts_password(
mocker: MockerFixture,
session: Session,
diff --git a/tests/unit_tests/databases/commands/utils_test.py
b/tests/unit_tests/databases/commands/utils_test.py
index 266c4e5d864..d131f0a0b15 100644
--- a/tests/unit_tests/databases/commands/utils_test.py
+++ b/tests/unit_tests/databases/commands/utils_test.py
@@ -78,14 +78,20 @@ def test_add_permissions_get_default_catalog(mocker:
MockerFixture):
def test_add_permissions_handle_failures(mocker: MockerFixture) -> None:
"""
- Test adding permissions to a database when it's created in case
- the request to get all schemas for one fo the catalogs fail.
+ Test adding permissions to a database when it's created in case the
+ request to get all schemas for one of the catalogs keeps failing --
+ each failing catalog is retried once before being given up on.
"""
database = mocker.MagicMock()
database.database_name = "my_db"
database.db_engine_spec.supports_catalog = True
database.get_all_catalog_names.return_value = ["catalog1", "catalog2",
"catalog3"]
- database.get_all_schema_names.side_effect = [["schema1"], Exception,
["schema3"]]
+ database.get_all_schema_names.side_effect = [
+ ["schema1"],
+ Exception,
+ Exception,
+ ["schema3"],
+ ]
add_permission_view_menu = mocker.patch(
"superset.commands.database.importers.v1.utils.security_manager."
"add_permission_view_menu"
@@ -102,3 +108,119 @@ def test_add_permissions_handle_failures(mocker:
MockerFixture) -> None:
mocker.call("schema_access", "[my_db].[catalog3].[schema3]"),
]
)
+
+
+def test_add_permissions_retries_transient_failure(mocker: MockerFixture) ->
None:
+ """
+ A catalog whose schema listing fails only once (eg a transient driver
+ hiccup) must still get its schema permissions granted via the retry,
+ instead of being permanently skipped like a genuinely unlistable
+ catalog would be.
+ """
+ database = mocker.MagicMock()
+ database.database_name = "my_db"
+ database.db_engine_spec.supports_catalog = True
+ database.get_all_catalog_names.return_value = ["catalog1", "catalog2"]
+ database.get_all_schema_names.side_effect = [
+ ["schema1"],
+ Exception,
+ ["schema2"],
+ ]
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+
+ add_permissions(database)
+
+ add_permission_view_menu.assert_has_calls(
+ [
+ mocker.call("catalog_access", "[my_db].[catalog1]"),
+ mocker.call("catalog_access", "[my_db].[catalog2]"),
+ mocker.call("schema_access", "[my_db].[catalog1].[schema1]"),
+ mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+ ]
+ )
+
+
+def test_add_permissions_does_not_retry_oauth2_redirect(mocker: MockerFixture)
-> None:
+ """
+ An ``OAuth2RedirectError`` from the schema-listing call is a control-flow
+ signal, not a transient failure: retrying it would just kick off a
+ second, redundant OAuth2 authorization redirect for the same catalog.
+ """
+ from superset.exceptions import OAuth2RedirectError
+
+ database = mocker.MagicMock()
+ database.database_name = "my_db"
+ database.db_engine_spec.supports_catalog = True
+ database.get_all_catalog_names.return_value = ["catalog1", "catalog2"]
+ oauth2_error = OAuth2RedirectError(
+ url="https://oauth.example.com/authorize";,
+ tab_id="abc-123",
+ redirect_uri="https://superset.example.com/callback";,
+ )
+ database.get_all_schema_names.side_effect = [oauth2_error, ["schema2"]]
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+
+ add_permissions(database)
+
+ assert database.get_all_schema_names.call_count == 2
+ add_permission_view_menu.assert_has_calls(
+ [
+ mocker.call("catalog_access", "[my_db].[catalog1]"),
+ mocker.call("catalog_access", "[my_db].[catalog2]"),
+ mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+ ]
+ )
+ # [catalog1] must never be retried: the OAuth2 redirect aborts it after
+ # a single attempt, same as a genuinely unlistable catalog would be.
+ assert add_permission_view_menu.call_count == 3
+
+
+def test_add_permissions_tolerates_failure_creating_permission_view(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A failure while granting a schema's permission (as opposed to while
+ listing the catalog's schemas) must be tolerated the same way it was
+ before the schema-listing retry was introduced: the rest of that
+ catalog is abandoned, but the next catalog is still processed. The
+ retry is scoped to the schema-listing call only, so it must not narrow
+ this pre-existing tolerance.
+ """
+ database = mocker.MagicMock()
+ database.database_name = "my_db"
+ database.db_engine_spec.supports_catalog = True
+ database.get_all_catalog_names.return_value = ["catalog1", "catalog2"]
+ database.get_all_schema_names.side_effect = [
+ ["schema1a", "schema1b"],
+ ["schema2"],
+ ]
+ add_permission_view_menu = mocker.patch(
+ "superset.commands.database.importers.v1.utils.security_manager."
+ "add_permission_view_menu"
+ )
+ add_permission_view_menu.side_effect = [
+ None, # catalog_access [catalog1]
+ None, # catalog_access [catalog2]
+ Exception, # schema_access [catalog1].[schema1a] -- fails
+ None, # schema_access [catalog2].[schema2]
+ ]
+
+ add_permissions(database)
+
+ add_permission_view_menu.assert_has_calls(
+ [
+ mocker.call("catalog_access", "[my_db].[catalog1]"),
+ mocker.call("catalog_access", "[my_db].[catalog2]"),
+ mocker.call("schema_access", "[my_db].[catalog1].[schema1a]"),
+ mocker.call("schema_access", "[my_db].[catalog2].[schema2]"),
+ ]
+ )
+ # [catalog1].[schema1b] must never be attempted: the exception raised
+ # while granting [schema1a] aborts the rest of catalog1, same as before.
+ assert add_permission_view_menu.call_count == 4
