aminghadersohi commented on code in PR #44683:
URL: https://github.com/apache/superset/pull/44683#discussion_r4113534555
##########
superset/commands/database/tables.py:
##########
@@ -177,3 +175,40 @@ def validate(self) -> None:
self._model = cast(Database, DatabaseDAO.find_by_id(self._db_id))
if not self._model:
raise DatabaseNotFoundError()
+
+ self._catalog_name = self._catalog_name or
self._model.get_default_catalog()
+ if not self._model.db_engine_spec.supports_schemas:
+ self._schema_name = None
+
+ if self._schema_name:
+ self._validate_schema(self._schema_name)
+
+ def _validate_schema(self, schema_name: str) -> None:
+ """
+ Accept only a schema that the schemas endpoint would list for this
user.
+
+ The schema has to exist in the database and be accessible to the user,
+ otherwise ``DatabaseSchemaNotFoundError`` is raised before any table or
+ view lookup is run.
+ """
+ try:
+ schemas = self._model.get_all_schema_names(
+ catalog=self._catalog_name,
+ cache=self._model.schema_cache_enabled,
+ cache_timeout=self._model.schema_cache_timeout or None,
+ force=self._force,
+ )
+ accessible = schema_name in schemas and bool(
+ security_manager.get_schemas_accessible_by_user(
+ self._model,
+ self._catalog_name,
+ {schema_name},
+ )
Review Comment:
No change here. The check deliberately matches `GET /schemas/`, which uses
the same `get_schemas_accessible_by_user` call, so both endpoints agree on
which schemas a user can see. It doesn't expose anything: `run()` still filters
every table and view through `get_datasources_accessible_by_user`, so a schema
that passes only through a dataset in another catalog returns an empty list, as
it did before this PR. The `datasource_access` branch not filtering by catalog
is shared security-manager behaviour and would need its own change.
##########
superset/commands/database/tables.py:
##########
@@ -177,3 +175,40 @@ def validate(self) -> None:
self._model = cast(Database, DatabaseDAO.find_by_id(self._db_id))
if not self._model:
raise DatabaseNotFoundError()
+
+ self._catalog_name = self._catalog_name or
self._model.get_default_catalog()
+ if not self._model.db_engine_spec.supports_schemas:
+ self._schema_name = None
+
+ if self._schema_name:
+ self._validate_schema(self._schema_name)
+
+ def _validate_schema(self, schema_name: str) -> None:
+ """
+ Accept only a schema that the schemas endpoint would list for this
user.
+
+ The schema has to exist in the database and be accessible to the user,
+ otherwise ``DatabaseSchemaNotFoundError`` is raised before any table or
+ view lookup is run.
+ """
+ try:
+ schemas = self._model.get_all_schema_names(
+ catalog=self._catalog_name,
+ cache=self._model.schema_cache_enabled,
+ cache_timeout=self._model.schema_cache_timeout or None,
+ force=self._force,
+ )
Review Comment:
Keeping it. The lookup is the same cached `get_all_schema_names` call the
schemas endpoint makes, with the same `schema_cache_enabled`/timeout settings.
Folding it into the table lookups wouldn't work, because an existing but empty
schema and a missing one both return no tables, and telling those apart is the
point of the change.
##########
superset/commands/database/tables.py:
##########
@@ -177,3 +175,40 @@ def validate(self) -> None:
self._model = cast(Database, DatabaseDAO.find_by_id(self._db_id))
if not self._model:
raise DatabaseNotFoundError()
+
+ self._catalog_name = self._catalog_name or
self._model.get_default_catalog()
+ if not self._model.db_engine_spec.supports_schemas:
+ self._schema_name = None
+
+ if self._schema_name:
+ self._validate_schema(self._schema_name)
+
+ def _validate_schema(self, schema_name: str) -> None:
+ """
+ Accept only a schema that the schemas endpoint would list for this
user.
+
+ The schema has to exist in the database and be accessible to the user,
+ otherwise ``DatabaseSchemaNotFoundError`` is raised before any table or
+ view lookup is run.
+ """
+ try:
+ schemas = self._model.get_all_schema_names(
+ catalog=self._catalog_name,
+ cache=self._model.schema_cache_enabled,
+ cache_timeout=self._model.schema_cache_timeout or None,
+ force=self._force,
+ )
+ accessible = schema_name in schemas and bool(
Review Comment:
Keeping exact matching. The client sends a name taken from the `/schemas/`
list, so it matches exactly. `get_schemas_accessible_by_user` and the
`schema_access` perms also compare case-sensitively, and case-insensitive
matching would be wrong on engines where two schemas can differ only by case
(e.g. quoted identifiers in Postgres). The lowercasing in the upload path is
for the `schemas_allowed_for_file_upload` config, not for engine identifiers.
##########
tests/unit_tests/commands/databases/tables_test.py:
##########
@@ -291,3 +309,130 @@ def test_tables_without_catalog(
cache=database_without_catalog.table_cache_enabled,
cache_timeout=database_without_catalog.table_cache_timeout,
)
+
+
+def test_tables_unknown_schema(
+ mocker: MockerFixture,
+ database_without_catalog: MagicMock,
+) -> None:
+ """
+ A schema that does not exist in the database is rejected before any
+ table or view lookup runs.
+ """
+ get_datasources_accessible_by_user = mocker.patch.object(
+ security_manager,
+ "get_datasources_accessible_by_user",
+ )
+
+ with pytest.raises(DatabaseSchemaNotFoundError):
+ TablesDatabaseCommand(1, None, "not_a_schema", False).run()
+
+ database_without_catalog.get_all_table_names_in_schema.assert_not_called()
+ database_without_catalog.get_all_view_names_in_schema.assert_not_called()
+
database_without_catalog.get_all_materialized_view_names_in_schema.assert_not_called()
Review Comment:
Wrapped in 11d2fc2448. For what it's worth, the repo doesn't enforce E501
here and pre-commit passed without the change.
--
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]
