sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4202272488
##########
superset/commands/report/alert.py:
##########
@@ -214,37 +216,41 @@ def _execute_query(self) -> pd.DataFrame:
:raises AlertQueryError: SQL query is not valid
:raises AlertQueryTimeout: The SQL query received a celery soft timeout
"""
- sql_template = jinja_context.get_template_processor(
- database=self._report_schedule.database
- )
-
try:
- rendered_sql =
sql_template.process_template(self._report_schedule.sql)
- self._validate_rendered_sql(rendered_sql)
- limited_rendered_sql =
self._report_schedule.database.apply_limit_to_sql(
- rendered_sql, ALERT_SQL_LIMIT
- )
-
- if app.config["MUTATE_ALERT_QUERY"]:
- limited_rendered_sql = (
- self._report_schedule.database.mutate_sql_based_on_config(
- limited_rendered_sql
- )
+ user = get_dynamic_executor(self._report_schedule,
alert_query=True)
+ if user is not None:
+ username = user.username
+ else:
+ executor, username = get_executor( # pylint:
disable=unused-variable
+ executors=app.config["ALERT_REPORTS_EXECUTORS"],
+ model=self._report_schedule,
)
-
- executor, username = get_executor( # pylint:
disable=unused-variable
- executors=app.config["ALERT_REPORTS_EXECUTORS"],
- model=self._report_schedule,
- )
- user = security_manager.find_user(username)
+ user = security_manager.find_user(username)
# A deleted/disabled executor user makes find_user return None.
Raise
# the dedicated error so the handler below re-surfaces it instead
of
# masking it as an opaque AlertQueryError (or letting the missing
user
# surface as a NoneType error from the downstream auth flow).
- if user is None:
+ if user is None or not user.is_active:
raise ReportScheduleExecutorNotFoundError(username)
with override_user(user):
Review Comment:
`override_user` doesn't restore `g.user` when the body raises, so a failing
alert query leaves the separate query executor as the current user for the rest
of the run. With `ALERT_REPORTS_EXECUTORS=[MODIFIER]`, content running as Alice
and the query as Bob, a query error is persisted with `changed_by_fk` set to
Bob, and the next execution resolves its content executor to Bob. Should the
override be wrapped in `try/finally` (or the error-state write done after
restoring the user) so a query-only identity can't become the content executor?
##########
superset/commands/report/update.py:
##########
@@ -65,6 +114,153 @@ def run(self) -> Model:
self.validate()
return ReportScheduleDAO.update(self._model, self._properties)
+ 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,
+ }
+ changed: set[str] = set()
+ for field in CONTENT_FIELDS & 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 _validate_executors(
+ self,
+ report_type: str,
+ changed_content_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"):
Review Comment:
With the flag off this returns early and `changed_content_fields` is also
skipped, but the stored `run_as`/`run_as_type` are left in place. A non-admin
editor can then change the dashboard, recipients or format of a schedule that
was set to run as an admin while the flag was on. When the flag is turned back
on, `get_dynamic_executor` resolves to that stored admin again and the editor's
content is rendered with the admin's RLS and OAuth credentials. Should the
other-identity content check also apply to a stored executor while the flag is
off, or should those stored values be cleared when a non-admin edits content?
##########
superset/commands/report/base.py:
##########
@@ -345,45 +393,118 @@ def validate_report_frequency(
report_type: str,
) -> None:
"""
- Validates if the report scheduled frequency doesn't exceed a limit
- configured in `config.py`.
+ Validates if the report scheduled frequency doesn't exceed the minimum
+ interval in effect.
:param cron_schedule: The cron schedule configured.
:param report_type: The report type (Alert/Report).
"""
config_key = (
- "ALERT_MINIMUM_INTERVAL"
+ ReportConfigKey.ALERT_MINIMUM_INTERVAL
if report_type == ReportScheduleType.ALERT
- else "REPORT_MINIMUM_INTERVAL"
+ else ReportConfigKey.REPORT_MINIMUM_INTERVAL
)
- minimum_interval = app.config.get(config_key, 0)
- if callable(minimum_interval):
- minimum_interval = minimum_interval()
+ minimum_interval = ReportConfigDAO.get_effective_value(config_key)
+ if minimum_interval is None:
+ return
if not isinstance(minimum_interval, int):
logger.error(
"Invalid value for %s: %s", config_key, minimum_interval,
exc_info=True
)
return
- # Since configuration is in minutes, we only need to validate
- # in case `minimum_interval` is <= 120 (2min)
- if minimum_interval < 120:
- return
-
- iterations = 60 if minimum_interval <= 3660 else 24
try:
- schedule = croniter(cron_schedule)
- current_exec = next(schedule)
-
- for _i in range(iterations):
- next_exec = next(schedule)
- diff, current_exec = next_exec - current_exec, next_exec
- if int(diff) < minimum_interval:
- raise ReportScheduleFrequencyNotAllowed(
- report_type=report_type,
minimum_interval=minimum_interval
- )
+ if not cron_meets_minimum_interval(cron_schedule,
minimum_interval):
+ raise ReportScheduleFrequencyNotAllowed(
+ report_type=report_type, minimum_interval=minimum_interval
+ )
except CroniterBadDateError as ex:
raise ReportScheduleCrontabNotValidError(
cron_schedule=cron_schedule
) from ex
+
+ def validate_report_format(
+ self,
+ report_type: str,
+ report_format: str | None,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Reports always deliver content: the "no attachment" format is only
+ allowed on alerts.
+ """
+ if (
+ report_type == ReportScheduleType.REPORT
+ and report_format == ReportDataFormat.NONE
+ ):
+ exceptions.append(ReportScheduleFormatRequiredError())
+
+ def validate_recipients_policy(self, exceptions: list[ValidationError]) ->
None:
+ """
+ Validate the e-mail recipients in the payload against the global
+ recipient policy (allowed domains and/or existing users only).
+ """
+ recipients = self._properties.get("recipients")
+ if not recipients:
+ return
+ disallowed = ReportConfigDAO.find_disallowed_addresses(
+ get_email_addresses(recipients)
+ )
+ if disallowed:
+
exceptions.append(ReportScheduleRecipientNotAllowedError(disallowed))
+
+ def validate_run_as(
+ self,
+ field_name: str,
+ user_id: int | None,
+ exceptions: list[ValidationError],
+ *,
+ default_to_current_user: bool,
+ ) -> User | None:
+ """Validate a specific user or an admin-selected application default.
+
+ A blank query executor inherits the content executor. A blank content
+ executor selected by an admin uses the legacy application
configuration.
+ """
+ is_admin = security_manager.is_admin()
+ current_user = get_user()
+ type_field = f"{field_name}_type"
+ executor_type = self._properties.pop(type_field, None)
+ if executor_type is not None and executor_type !=
ExecutorType.FIXED_USER:
+ if not is_admin:
+
exceptions.append(ReportScheduleRunAsForbiddenError(field_name))
+ return None
+ exceptions.append(
+ ValidationError(
+ _("Choose Application default or a specific user"),
+ field_name=field_name,
+ )
+ )
+ return None
+
+ if user_id is None and executor_type != ExecutorType.FIXED_USER:
+ if field_name == "run_alert_query_as":
Review Comment:
Clearing the query executor is allowed for a non-admin without any identity
check. On an alert whose `run_as` is another user and whose
`run_alert_query_as` is the editor, `PUT {"run_alert_query_as": null}` clears
both query fields, and the query then inherits the other user's identity. The
guard in `_validate_executors` only fires when a content field also changes,
and the alert's SQL is not one of them, so the unchanged query now runs with
that user's database access and the editor can read the result from the alert's
last value or logs. Should clearing `run_alert_query_as` be treated like a
content change (or rejected for a non-admin) whenever the inherited `run_as`
isn't the requester?
--
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]
