Vitor-Avila commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4209911335
##########
superset/commands/report/update.py:
##########
@@ -65,6 +131,194 @@ def run(self) -> Model:
self.validate()
return ReportScheduleDAO.update(self._model, self._properties)
+ def _compare_fields(
+ self,
+ keys: frozenset[str],
+ current: dict[str, Any],
+ normalizers: dict[str, Callable[[Any], Any]],
+ ) -> set[str]:
+ """
+ Return submitted fields whose normalized values differ from the stored
values
+ """
+ changed: set[str] = set()
+ for field in keys & set(self._properties):
+ value = self._properties[field]
+ normalizer = normalizers.get(field)
+ if normalizer:
+ value = normalizer(value)
+ if value != current[field]:
+ changed.add(field)
+ return changed
+
+ def _changed_content_fields(self) -> set[str]:
+ """
+ Return the content fields whose payload value differs from the stored
+ one. Must run before other validators replace ids with model objects.
+ """
+ assert self._model is not None
+ model = self._model
+ current: dict[str, Any] = {
+ "chart": model.chart_id,
+ "dashboard": model.dashboard_id,
+ "extra": _normalize_json(model.extra_json),
+ "recipients": _normalize_recipients(model.recipients),
+ "report_format": model.report_format,
+ }
+ normalizers: dict[str, Callable[[Any], Any]] = {
+ "extra": _normalize_json,
+ "recipients": _normalize_recipients,
+ }
+ return self._compare_fields(CONTENT_FIELDS, current, normalizers)
+
+ def _changed_condition_fields(self) -> set[str]:
+ """Return alert-condition fields that differ from the stored values."""
+ assert self._model is not None
+ current = {
+ "database": self._model.database_id,
+ "sql": self._model.sql,
+ "validator_type": self._model.validator_type,
+ "validator_config_json":
_normalize_json(self._model.validator_config_json),
+ }
+ normalizers: dict[str, Callable[[Any], Any]] = {
+ "validator_config_json": _normalize_json,
+ }
+ return self._compare_fields(ALERT_CONDITION_FIELDS, current,
normalizers)
+
+ def _validate_executors(
+ self,
+ report_type: str,
+ changed_content_fields: set[str],
+ changed_condition_fields: set[str],
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Resolve the "Run As" fields (SIP-209) on update.
+
+ Fields absent from the payload keep their stored value. Admins may set
+ any active user or explicit executor type. Clearing content restores
the
+ legacy application-configured executor;
+ clearing the query executor inherits the content identity.
+ Non-admins may only point the fields at themselves and, when the
schedule
+ executes as another user,
+ may not change its content or recipients: doing so would let an editor
+ read data with someone else's credentials.
+ """
+ assert self._model is not None
+ has_run_as = bool({"run_as", "run_as_type"} & self._properties.keys())
+ has_query_as = bool(
+ {"run_alert_query_as", "run_alert_query_as_type"} &
self._properties.keys()
+ )
+ run_as_id = self._properties.pop("run_as", None)
+ query_as_id = self._properties.pop("run_alert_query_as", None)
+ if not is_feature_enabled("ALERT_REPORT_DYNAMIC_EXECUTOR"):
+ self._properties.pop("run_as_type", None)
+ self._properties.pop("run_alert_query_as_type", None)
+ return
+
+ run_as = (
+ self.validate_run_as(
+ "run_as", run_as_id, exceptions, default_to_current_user=False
+ )
+ if has_run_as
+ else self._model.run_as
+ )
+
+ query_as = None
+ if report_type == ReportScheduleType.REPORT:
+ if has_query_as and (
+ query_as_id is not None
+ or self._properties.get("run_alert_query_as_type") is not None
+ ):
+
exceptions.append(ReportScheduleRunAlertQueryAsNotAllowedError())
+ else:
+ self._properties["run_alert_query_as"] = None
+ self._properties["run_alert_query_as_type"] = None
+ elif has_query_as:
+ query_as = self.validate_run_as(
+ "run_alert_query_as",
+ query_as_id,
+ exceptions,
+ default_to_current_user=False,
+ )
+ else:
+ query_as = self._model.run_alert_query_as
+
+ if security_manager.is_admin():
+ return
+ current_user_id = get_user_id()
+ executes_as_other_user = any(
+ user is not None and user.id != current_user_id
+ for user in (run_as, query_as)
+ )
+ typed_executor = any(
+ self._properties.get(field, getattr(self._model, field))
+ not in (None, ExecutorType.FIXED_USER)
+ for field in ("run_as_type", "run_alert_query_as_type")
+ )
+ content_type = self._properties.get("run_as_type",
self._model.run_as_type)
+ unresolved_executor = (
+ content_type is None
+ or (content_type == ExecutorType.FIXED_USER and run_as is None)
+ or (
+ report_type == ReportScheduleType.ALERT
+ and query_as is None
+ and self._properties.get(
+ "run_alert_query_as_type",
self._model.run_alert_query_as_type
+ )
+ == ExecutorType.FIXED_USER
+ )
+ )
+ if (
+ executes_as_other_user or typed_executor or unresolved_executor
+ ) and changed_content_fields:
+ exceptions.append(ReportScheduleRunAsContentForbiddenError())
+
Review Comment:
Ok, ended up re-adding `report_format` to the `CONTENT_FIELDS`. It actually
allows some sort of content change in 2 scenarios:
* Changing an Alert from `ReportFormat.NONE` to anything else would now
include an attachment;
* Updating the `report_format` for a table chart with server-side pagination
enabled to CSV could expose additional data.
Added logic to cover this flow through the subscription modal as well for
feature parity.
--
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]
