sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4202748466
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -418,6 +418,111 @@ ALERT_REPORTS_EXECUTORS = [FixedExecutor("admin")]
Please refer to `ExecutorType` in the codebase for other executor types.
+#### Per-schedule "Run As" executor
+
+`ALERT_REPORTS_EXECUTORS` is a global, server-side setting that is invisible
to the people
+configuring alerts and reports, which makes permission problems hard to
diagnose and is
+especially painful for databases authenticated with OAuth2, where the
impersonated account
+must hold a valid token.
[SIP-209](https://github.com/apache/superset/issues/39642)
+introduces an explicit, per-schedule alternative behind the
`ALERT_REPORT_DYNAMIC_EXECUTOR`
+[feature flag](/admin-docs/configuration/configuring-superset#feature-flags):
+
+```python
+FEATURE_FLAGS = {
+ "ALERT_REPORTS": True,
+ "ALERT_REPORT_DYNAMIC_EXECUTOR": True,
+}
+```
+
+When the flag is enabled:
+
+- Every alert/report gets a **Run as** field recording which user's RBAC
permissions and
+ database credentials are used to render the content. Alerts get an additional
+ **Run alert query as** field for the alert condition SQL, so the audience of
an alert
+ does not need direct database access.
+- Admins can select **Application default** or **Specific user**:
+ * **Application default:** resolves through `ALERT_REPORTS_EXECUTORS` value.
+ * **Specific user:** User picker option.
+ Non-admins can only select themselves. They see no user search and the API
rejects
+ selecting another user. Reports scheduled from a dashboard or chart always
execute
+ as the subscribing user.
Review Comment:
"Reports scheduled from a dashboard or chart always execute as the
subscribing user" isn't true for existing subscriptions: the migration leaves
the executor fields null and execution falls back to `ALERT_REPORTS_EXECUTORS`.
An operator on `FixedExecutor("admin")` who enables the flag would assume
subscriptions now use the subscriber's permissions and OAuth tokens, but
untouched ones still run as admin. Could this say it applies to newly created
subscriptions and note how to move existing ones?
##########
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:
With the flag on, a migrated chart/dashboard subscription has `run_as_type =
None`, so `unresolved_executor` is true and any content change by a non-admin
is rejected here. The chart's "Edit email report" modal
(`features/reports/ReportModal`) is not touched by this PR and sends no
`run_as` or takeover action, so its owner changing the format PNG to CSV gets a
422 with no way to resolve it from that UI. Is the subscription editor meant to
stay unusable for legacy subscriptions, or should it send `run_as` / should the
server default a null type for the subscription's own creator?
##########
superset/reports/utils.py:
##########
@@ -0,0 +1,190 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Helpers shared by the Alerts & Reports commands, DAO and execution engine.
+
+Recipient and interval helpers are shared by save-time and execution-time
+validation.
+"""
+
+from __future__ import annotations
+
+import re
+from collections.abc import Iterable
+from typing import Any, TYPE_CHECKING
+
+from croniter import croniter
+
+from superset import is_feature_enabled
+from superset.commands.report.exceptions import
ReportScheduleExecutorNotFoundError
+from superset.reports.models import ReportRecipientType
+from superset.tasks.types import ExecutorType
+from superset.utils import json
+
+if TYPE_CHECKING:
+ from flask_appbuilder.security.sqla.models import User
+
+ from superset.reports.models import ReportSchedule
+
+# Recipient fields inside ``recipient_config_json`` holding e-mail addresses.
+EMAIL_RECIPIENT_FIELDS = ("target", "ccTarget", "bccTarget")
+_EMAIL_SEPARATOR_RE = re.compile(r"[,;]")
+
+# Simple hostname validation for the allowed e-mail domains configuration.
+EMAIL_DOMAIN_REGEX = re.compile(
+
r"^(?!\*\..*\.\*$)(?:\*\.)?(?=.{1,253}$)([a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+"
+ r"(?:[a-zA-Z]{2,63}|\*)$"
+)
+
+
+def get_dynamic_executor(
+ report_schedule: ReportSchedule,
+ *,
+ alert_query: bool = False,
+) -> User | None:
+ """
+ Return the user explicitly configured to execute ``report_schedule``.
+
+ When ``alert_query`` is set, a typed query executor is preferred; an unset
+ query type inherits the content selection. An unset content type returns
+ ``None`` so callers use ``ALERT_REPORTS_EXECUTORS``. The type marker is
+ authoritative even if a stale user relationship is present.
+
+ The returned user may be inactive; callers decide how to surface that.
+ """
+ if not is_feature_enabled("ALERT_REPORT_DYNAMIC_EXECUTOR"):
+ return None
+ field = "run_as"
+ if alert_query and report_schedule.run_alert_query_as_type is not None:
+ field = "run_alert_query_as"
+ executor_type = getattr(report_schedule, f"{field}_type")
+ if executor_type is None:
+ return None
+ if executor_type != ExecutorType.FIXED_USER:
+ raise ReportScheduleExecutorNotFoundError(str(executor_type))
+ user = getattr(report_schedule, field)
+ if user is None:
+ raise ReportScheduleExecutorNotFoundError("deleted user")
+ return user
+
+
+def split_email_addresses(value: str | None) -> list[str]:
Review Comment:
`split_email_addresses` only splits on `,` and `;`, but delivery uses
`recipients_string_to_list`, which also splits on whitespace. A stored
recipient like `[email protected] [email protected]` is read here as one
address with domain `example.com`, so it passes an `example.com` allowed-domain
policy (both in the save-time conflict scan and at send time) while the email
goes to both. Can this use the same separator set as delivery (`,`, `;`,
whitespace)?
##########
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:
A non-admin editor who only renames or reschedules a report that runs as
someone else can still get a 422 and be forced to take over execution. The
modal's save always writes `extra.dashboard.nativeFilters = []`
(AlertReportModal.tsx:1033), so a stored `extra` like
`{"dashboard":{"anchor":""}}` (e.g. created through the API) no longer matches
after `_normalize_json`, and `extra` counts as a content change. The same
happens for a non-email recipient such as `{"target":"C12345678"}`: the modal
adds empty `ccTarget`/`bccTarget`, and the normalization at line 117 only
strips those for Email. Should absent/empty filter lists and empty cc/bcc be
normalized the same way for every recipient type?
--
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]
