sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4308746493
##########
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:
Authorization renders the SQL with the submitted `template_params` as plain
template variables, but execution doesn't: `get_sqla_query` overwrites
`extra_cache_keys`, `removed_filters` and `applied_filters` after merging the
params, and `BaseTemplateProcessor.__init__` consumes them as constructor
arguments instead of exposing them in the context. So a dataset editor with
access to only `public.orders` can submit
`template_params='{"extra_cache_keys":[]}'` with `SELECT * FROM {{
'public.orders' if extra_cache_keys is defined else 'private.secrets' }}`: the
check sees `public.orders`, while chart-data renders `private.secrets` and
accepts editorship. Should these reserved names be rejected in
`template_params` (or dropped before rendering here) so the authorized SQL
matches what executes?
##########
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:
This equality check treats `{"x": 1}` and `{"x": true}` as the same params,
but Jinja doesn't (`x is sameas true`). A dataset editor with access to only
`public.orders` can create a virtual dataset like `SELECT * FROM {{
'private.secrets' if x is sameas true else 'public.orders' }}` with
`template_params='{"x":1}'` (authorized as `public.orders`), then PUT only
`template_params='{"x":true}'`. `_template_params_differ` returns False, so the
SQL check is skipped, and chart-data now renders `private.secrets` and accepts
editorship. The same dict-equality skip is in `importers/v1/utils.py`
(`requested_params == stored_params`) and `views/datasource/views.py`
(`requested_template_params != _stored_template_params(...)`). Could the
change-gate compare something type-sensitive (e.g. the canonical JSON text, or
treat any non-identical raw text as a change), shared by all three paths?
--
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]
