sadpandajoe commented on code in PR #35662:
URL: https://github.com/apache/superset/pull/35662#discussion_r4205029948
##########
superset/jinja_context.py:
##########
@@ -1310,7 +1379,7 @@ def dataset_macro(
}
sqla_query = dataset.get_query_str_extended(query_obj, mutate=False)
sql = sqla_query.sql
- return f"(\n{sql}\n) AS dataset_{dataset_id}"
+ return f"(\n{sql}\n) AS {alias or f'dataset_{dataset.id}'}"
Review Comment:
The alias is spliced into the SQL verbatim. `{{ dataset(42, alias="order")
}}` renders `AS order`, which is a syntax error for a reserved word. An alias
with a space or hyphen fails the same way. If the value comes from something
like `url_param('alias', 'a')`, any text a viewer supplies lands in the query
as SQL, since `url_param` only escapes string-literal characters. Could the
alias be validated as a single identifier, or quoted with the dataset's
dialect, before it is interpolated?
##########
tests/unit_tests/jinja_context_test.py:
##########
@@ -1292,10 +1293,7 @@ def test_dataset_macro(mocker: MockerFixture) -> None:
)
DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806
DatasetDAO.find_by_id.return_value = dataset
- mocker.patch(
-
"superset.connectors.sqla.models.security_manager.get_guest_rls_filters",
- return_value=[],
- )
+ DatasetDAO.get_table_by_catalog_schema_and_name.return_value = dataset
Review Comment:
`get_table_by_catalog_schema_and_name` is mocked to return the same dataset
for every call, so nothing checks the qualifier semantics this PR adds. If
`schema=None` were treated like an omitted `schema`, or the qualifiers were
dropped, these tests would still pass. Could a test at the macro-to-DAO
boundary cover two datasets named `events` (one with `schema=None`, one with
`schema="staging"`), asserting ambiguity with no schema, the NULL-schema
dataset with `schema=None`, and the other with `schema="staging"`? A case where
the lookup raises `MultipleResultsFound` and asserts `DatasetInvalidError`
would also be useful, since that translation is untested.
##########
superset/jinja_context.py:
##########
@@ -1281,22 +1291,81 @@ def get_template_processor(
def dataset_macro(
- dataset_id: int,
+ dataset_id: int | str,
include_metrics: bool = False,
columns: list[str] | None = None,
+ schema: str | _Unset | None = _UNSET,
+ catalog: str | _Unset | None = _UNSET,
+ database_id: int | str | _Unset | None = _UNSET,
+ alias: str | None = None,
) -> str:
"""
- Given a dataset ID, return the SQL that represents it.
+ Given a dataset ID or name, return the SQL that represents it.
+
+ If ``dataset_id`` is an integer, it is treated as the unique dataset ID and
+ the optional ``schema``, ``catalog`` and ``database_id`` parameters are
+ ignored.
+
+ If ``dataset_id`` is a string, it is treated as a dataset name. The
optional
+ ``schema``, ``catalog`` and ``database_id`` parameters are used to narrow
+ down the search when provided. If multiple datasets match the provided
+ criteria, an error is raised because the dataset name is ambiguous.
The generated SQL includes all columns (including computed) by default.
Optionally
the user can also request metrics to be included, and columns to group by.
+
+ The ``alias`` parameter allows the user to specify an explicit alias for
the
+ returned subquery.
"""
# pylint: disable=import-outside-toplevel
+ from sqlalchemy.orm.exc import MultipleResultsFound
+
from superset.daos.dataset import DatasetDAO
- dataset = DatasetDAO.find_by_id(dataset_id)
+ filters: dict[str, Any] = {}
+
+ if database_id not in (_UNSET, None):
+ filters["database_id"] = database_id
+ if catalog is not _UNSET:
+ filters["catalog"] = catalog
+ if schema is not _UNSET:
+ filters["schema"] = schema
+
+ if isinstance(dataset_id, str):
Review Comment:
Any string argument now goes to the name lookup, but `dataset_id` strings
used to reach `find_by_id`, which accepts them. A template like `{{
dataset(url_param('dataset_id')) }}` with `?dataset_id=42` used to resolve
dataset 42. Now it searches for a dataset named `"42"`, so it raises
`DatasetNotFoundError`, or silently expands a different dataset if one happens
to be named `"42"`. The same applies to `database_id="1"`, which the DAO treats
as a database name. Should purely numeric strings still resolve by ID, or
should name lookup need an explicit selector?
##########
tests/integration_tests/test_jinja_context.py:
##########
@@ -226,3 +230,47 @@ def test_custom_template_processors_ignored(app_context:
AppContext) -> None:
template = "SELECT '$DATE()'"
tp = get_template_processor(database=maindb)
assert tp.process_template(template) == template
+
+
+def test_dataset_macro_access_filters(app_context: AppContext) -> None:
+ """Test that the dataset macro properly enforces datasource access security
+ by successfully resolving for a granted user and throwing an access
+ exception for a denied user."""
+ database = superset.utils.database.get_example_database()
+ table =
db.session.query(SqlaTable).filter_by(table_name="birth_names").one()
Review Comment:
This looks up `birth_names` with `.one()`, but nothing in this file loads
the `birth_names` fixture. Run on its own, the test would raise
`NoResultFound`, and it only passes in a full run if earlier files leave that
row behind. The denied-user assertion also assumes no earlier test granted
`datasource_access` on it to the `gamma2` role. Could the test load
`load_birth_names_dashboard_with_slices` and use a dedicated role for the
denied user?
--
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]
