Vitor-Avila commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4203206653
##########
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,
Review Comment:
Done!
##########
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:
Had a lot of thought about this one. I've decided to remove the
`report_format` from the `CONTENT_FIELDS` definition. It doesn't really change
the content someone is getting, just the format in which the content is
delivered. It might change on the rare occasion of a server-side paginated
chart, but I think it's a well-scoped scenario.
The asset subscription modal only provides a way to update the format, it
doesn't allow to edit any of the other `CONTENT_FIELDS`, so this flow also gets
unblocked with this 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]
