geido commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4313532038
##########
superset/commands/dataset/update.py:
##########
@@ -69,6 +73,23 @@
)
+def _template_params_differ(requested: str | None, stored: str | None) -> bool:
+ """
+ Could replacing ``stored`` params with ``requested`` change what a
+ templated SQL renders?
+
+ Compared the way execution reads them, so re-serialized JSON with the same
+ values is no change. Params execution would read differently than any
+ access check can (see ``parse_template_params``) always count as one.
+ """
+ if requested == stored:
+ return False
+ try:
+ return parse_template_params(requested) !=
parse_template_params(stored)
Review Comment:
Fixed in 93ad47b. The change-gate now compares `template_params` the way
execution reads them, through a shared `template_params_equivalent` helper
(canonical JSON of the parsed values). It is type-sensitive — `1`, `true` and
`1.0` are distinct — while staying insensitive to key order and whitespace, so
a params-only edit that changes a value's JSON type re-runs the SQL access
check. Used on all three paths you mentioned: update
(`_template_params_differ`), the v1 importer (the unchanged-binding comparison)
and `/datasource/save/` (`_repoints_sql`).
Tests: `test_template_params_equivalent_is_type_sensitive`, plus per-path
`test_update_dataset_template_params_type_change_is_rechecked`,
`test_import_dataset_template_params_type_change_runs_binding_check`, and
`test_save_template_params_type_change_is_rechecked`.
##########
superset/commands/dataset/create.py:
##########
@@ -219,27 +235,35 @@ def validate(self) -> None: # noqa: C901
exceptions.append(TableNotFoundValidationError(table))
if sql:
- try:
- security_manager.raise_for_access(
- database=database,
- sql=sql,
- catalog=catalog,
- schema=schema,
- )
- except SupersetSecurityException as ex:
-
exceptions.append(DatasetDataAccessIsNotAllowed(ex.error.message))
- except SupersetParseError as ex:
- exceptions.append(
- ValidationError(
- f"Invalid SQL: {ex.error.message}",
- field_name="sql",
+ # Unreadable params already failed validation above; skip the check
+ # rather than report an access decision made against other params.
+ if template_params_valid:
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=sql,
+ catalog=catalog,
+ schema=schema,
+ template_params=template_params,
Review Comment:
Fixed in 93ad47b. The access check now renders with the submitted
`template_params` minus the names `BaseTemplateProcessor` takes as constructor
arguments rather than exposing as Jinja variables — `extra_cache_keys`,
`removed_filters`, `applied_filters`, `database`, `query`, `table` (collected
in `RESERVED_TEMPLATE_PARAM_NAMES`, next to the processor). Dropping them
(`template_params_for_access_check`) keeps the authorized render aligned with
execution, which doesn't expose these as context variables. Applied on create,
update, import, duplicate and `/datasource/save/`.
Tests: `test_template_params_for_access_check_drops_reserved_names`, plus
per-path assertions that the reserved names don't reach the check
(`test_create_dataset_auth_check_drops_reserved_params` and the
update/import/save/duplicate equivalents).
--
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]
