sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4199833846
##########
superset/commands/report/update.py:
##########
@@ -88,6 +284,14 @@ def validate(self) -> None: # noqa: C901
exceptions: list[ValidationError] = []
+ # Snapshot content changes before validators replace ids with objects.
+ # Only needed for the "Run As" authorization rules (SIP-209).
+ changed_content_fields = (
Review Comment:
The content snapshot is taken here, before the `creation_method` block below
replaces `recipients` with the requesting user's email. A non-admin editor who
PUTs the stored recipients unchanged on a chart/dashboard subscription that
runs as someone else compares as "no change", then gets their own address
swapped in. The schedule keeps the other user's `run_as`, so that user's
rendered content is delivered to the editor with no
`ReportScheduleRunAsContentForbiddenError`. Should the snapshot be taken after
the recipient rewrite, or should `recipients` be counted as changed whenever
the rewrite alters them?
##########
superset/commands/report/update_config.py:
##########
@@ -0,0 +1,145 @@
+# 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.
+import logging
+from functools import partial
+from typing import Any
+
+from croniter import CroniterBadDateError
+
+from superset.commands.base import BaseCommand
+from superset.commands.report.exceptions import (
+ ReportConfigConflictError,
+ ReportConfigUpdateFailedError,
+)
+from superset.daos.report import ReportConfigDAO, ReportScheduleDAO
+from superset.reports.models import (
+ ReportConfigKey,
+ ReportSchedule,
+ ReportScheduleType,
+)
+from superset.reports.utils import cron_meets_minimum_interval,
get_email_addresses
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class UpdateReportConfigCommand(BaseCommand):
+ """
+ Update the global Alerts & Reports configuration.
+
+ Recipient restrictions and minimum intervals are validated against every
+ existing schedule: when any of them would become invalid under the new
+ configuration the command fails and lists the impacted schedules, so admins
+ can fix them before tightening the policy. Enabling/disabling attachments
+ for alerts applies immediately to all alerts and is never a conflict.
+ """
+
+ def __init__(self, data: dict[str, Any]):
+ self._properties = data.copy()
+ self._impacted: list[dict[str, Any]] = []
+
+ @transaction(on_error=partial(on_error,
reraise=ReportConfigUpdateFailedError))
+ def run(self) -> dict[str, Any]:
+ self.validate()
+ ReportConfigDAO.upsert(self._properties)
Review Comment:
Saving the configuration never checks attachment readiness. Create an alert
with `report_format="PNG"` and no chart/dashboard while global attachments are
off (`requires_attachment()` allows it), then turn attachments on here: the
save succeeds, but the alert has no asset, so its next delivery fails in
`_get_url()`. Should enabling attachments list such alerts as conflicts like
the recipient and frequency checks do?
##########
docs/admin_docs/configuration/alerts-reports.mdx:
##########
@@ -418,6 +418,108 @@ 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.
+- When a non-admin edits an alert/report configured to run as someone else,
they can
+ change its metadata, type, and condition (database, SQL, and validator), but
not its
+ selected chart/dashboard, filters, attachment format, or recipients. To
change those,
+ they must first set **Run as** to their own account via the **Execute using
my permissions**
+ button (or ask an admin). For alerts, this action selects the current user
for both content
+ and SQL condition execution. Legacy schedules without an explicit ``run_as``
user also
+ require this switch before a non-admin changes the delivered asset or
recipients.
+- Untouched legacy schedules continue using `ALERT_REPORTS_EXECUTORS`.
**Application
+ default** preserves this behavior in the Run as selector, including when an
admin
+ clears an explicit executor.
+- A selected user that is deleted remains recorded as a **Specific user**
selection.
+ Resolving a deleted or inactive user fails rather than switching to a legacy
executor.
+ Alert SQL templates, SQL mutation hooks, authorization and database access
all use
+ the query executor's identity.
+
+It's possible to set a custom "Run As" tooltip value via the
`ALERT_REPORTS_RUN_AS_TOOLTIP`
+config in `superset_config.py`. This is useful if you want to provide further
context on what
+the "Application default" means. **It does not change the feature behavior**.
+`ALERT_REPORTS_EXECUTORS` is deprecated in favor of this field and will be
removed once the
+feature graduates from the flag.
+
+### Alerts & Reports configuration
+
+Admins can manage the global Alerts & Reports settings at runtime from the
**Configuration**
+button on the Alerts & Reports list page (also available through
+`GET`/`PUT /api/v1/report/configuration/`). Values are stored in the
`report_config` table;
+a setting that was never saved falls back to the corresponding
`superset_config.py` value or
+feature flag.
+
+| Setting | Fallback | Description |
+| --- | --- | --- |
+| Enable attachments for alerts | `ALERTS_ATTACH_REPORTS` FF | Whether alerts
deliver their attachment. Applies to subsequent attempts; saving a change
cancels pending attempts. |
Review Comment:
This row says saving the setting "cancels pending attempts", but
`UpdateReportConfigCommand` only validates and upserts, and the retry section
further down says setting changes don't cancel queued retries (they read the
latest value when they start). An admin disabling attachments would expect a
pending retry to be dropped when it still runs. Could this sentence be removed?
##########
superset/commands/report/update.py:
##########
@@ -42,17 +47,61 @@
from superset.exceptions import SupersetSecurityException
from superset.reports.models import (
ReportCreationMethod,
+ ReportDataFormat,
ReportRecipientType,
ReportSchedule,
ReportScheduleType,
ReportState,
)
+from superset.tasks.exceptions import ExecutorNotFoundError
+from superset.tasks.types import ExecutorType
+from superset.tasks.utils import get_executor
from superset.utils import json
-from superset.utils.core import get_user_email
+from superset.utils.core import get_user, get_user_email, get_user_id
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
+# Payload fields that change the delivered asset, its rendering, or its
recipients.
+# When a schedule executes as another user, only admins may change these.
+CONTENT_FIELDS: frozenset[str] = frozenset(
Review Comment:
`CONTENT_FIELDS` doesn't include `sql`, `database` or the validator fields,
so a non-admin editor can rewrite an alert's condition while
`run_alert_query_as` still points at another user. Save-time
`validate_alert_query` authorizes as the saving user and skips the check when
templated SQL doesn't parse before rendering, while execution renders and runs
the query as the stored executor. For example, an editor submits `{% if
current_username() != 'editor' %}SELECT secret FROM restricted.t LIMIT 1{% else
%}SELECT 1{% endif %}`, runs the alert, and reads the result from
`last_value_row_json` or the log `value`. Should changes to the alert query
count as content for the other-identity guard?
##########
superset/commands/report/update.py:
##########
@@ -42,17 +47,61 @@
from superset.exceptions import SupersetSecurityException
from superset.reports.models import (
ReportCreationMethod,
+ ReportDataFormat,
ReportRecipientType,
ReportSchedule,
ReportScheduleType,
ReportState,
)
+from superset.tasks.exceptions import ExecutorNotFoundError
+from superset.tasks.types import ExecutorType
+from superset.tasks.utils import get_executor
from superset.utils import json
-from superset.utils.core import get_user_email
+from superset.utils.core import get_user, get_user_email, get_user_id
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
+# Payload fields that change the delivered asset, its rendering, or its
recipients.
+# When a schedule executes as another user, only admins may change these.
+CONTENT_FIELDS: frozenset[str] = frozenset(
+ {
+ "chart",
+ "dashboard",
+ "extra",
+ "recipients",
+ "report_format",
+ }
+)
+
+
+def _normalize_json(value: Any) -> str:
+ """Canonical JSON string for a dict/JSON-string value (for comparisons)."""
+ if isinstance(value, str):
+ try:
+ value = json.loads(value)
+ except json.JSONDecodeError:
+ return value
+ if value is None:
+ value = {}
+ if not isinstance(value, (dict, list)):
+ return str(value)
+ return json.dumps(value, sort_keys=True)
+
+
+def _normalize_recipients(recipients: Any) -> list[tuple[str, str]]:
+ """Canonical, order-independent representation of a recipients list."""
+ normalized: list[tuple[str, str]] = []
+ for recipient in recipients or []:
+ if isinstance(recipient, dict):
+ recipient_type = recipient.get("type")
+ config = recipient.get("recipient_config_json")
+ else:
+ recipient_type = recipient.type
+ config = recipient.recipient_config_json
+ normalized.append((str(recipient_type), _normalize_json(config)))
Review Comment:
A legacy recipient stored as `{"target": "[email protected]"}` (e.g. from
chart/dashboard subscriptions) is compared with the modal's resubmitted
`{"target": ..., "ccTarget": "", "bccTarget": ""}`, so the JSON differs and
`recipients` is flagged as changed. With the flag on, a non-admin renaming a
schedule that runs as another user or the application default then gets
`ReportScheduleRunAsContentForbiddenError` without touching recipients. Should
empty `ccTarget`/`bccTarget` be dropped before comparing?
##########
superset/reports/schemas.py:
##########
@@ -125,6 +127,46 @@
"Creation method is used to inform the frontend whether the report/alert
was "
"created in the dashboard, chart, or alerts and reports UI."
)
+run_as_description = (
+ "ID of the user whose credentials (RBAC permissions, database OAuth2
tokens) "
+ "are used when rendering the content (screenshot, PDF, CSV, XLSX or text).
"
+ "Admins can set any active user; non-admins can only set themselves. Only"
+ "honored when the ALERT_REPORT_DYNAMIC_EXECUTOR feature flag is enabled. "
+ "Admins can set this and run_as_type to null to use
ALERT_REPORTS_EXECUTORS."
+)
+run_alert_query_as_description = (
+ "ID of the user whose credentials are used when running the alert
condition "
+ "SQL query, useful when the audience of the alert does not have access to
the "
+ "database. Null inherits the content executor at execution time."
+)
+
+
+_RUN_AS_FIELD_KEYS = (
+ "run_as",
+ "run_alert_query_as",
+ "run_as_type",
+ "run_alert_query_as_type",
+)
+_REPORT_EXECUTOR_TYPES = [
+ ExecutorType.FIXED_USER,
+]
+
+
+class RunAsFieldStripMixin:
+ """Drop the "Run As" fields from the raw payload when the dynamic executor
+ feature (``ALERT_REPORT_DYNAMIC_EXECUTOR``) is disabled, so callers on the
+ legacy path never store an executor by accident."""
+
+ @pre_load
+ def strip_run_as_fields_if_disabled(
+ self,
+ data: dict[str, Any],
+ **kwargs: Any,
+ ) -> dict[str, Any]:
+ if not is_feature_enabled("ALERT_REPORT_DYNAMIC_EXECUTOR"):
Review Comment:
Nothing exercises this `pre_load` strip: the schema tests deserialize single
fields, and the command tests bypass the schemas. If the hook were removed, a
legacy client posting `run_as_type: "creator"` with
`ALERT_REPORT_DYNAMIC_EXECUTOR` off would start getting validation errors (or
storing an executor) and no test would fail. Could a test `load()` a POST/PUT
payload with the flag off and assert the four executor keys are absent, and
with it on that invalid values error?
##########
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)
Review Comment:
The new tests stub `validate_report_frequency` in the create/update
fixtures, and the DAO and cron helper tests cover their pieces separately, so
nothing checks that this lookup uses the saved minimum. If it fell back to the
app config, a saved report minimum of 600 with an app minimum of 0 would still
accept `* * * * *` and every current test would pass. Could a test call
`validate_report_frequency()` against the real `ReportConfigDAO` and assert
`ReportScheduleFrequencyNotAllowed`?
--
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]
