apache / apache/superset

[SIP-209] Improved Alerts & Reports

Open
#39,642 3 comments 2 reactions 1 assignee Claimed by @Vitor-Avila View on GitHub
alert-reports design:proposal sip
Dominant language
Python
Stars
74.8k
Forks
18.3k
Avg merge
2d 7h
Merged PRs (30d)
692

Description

## [SIP-209] Proposal for Improved Alerts & Reports

### Motivation
The Alerts & Reports feature has structural limitations that create real operational and security problems:

#### Unconstrained email recipients
Reports can be sent to any email address, including external domains outside the Organization. There is no way for admins to enforce that reports only reach internal addresses, or are limited to existing users.

#### Opaque execution model
The `ALERT_REPORT_EXECUTORS` config controls which Superset user account is impersonated when fetching the report data (for RBAC and database queries). This is a global, server-side setting that is invisible to admins and users working in the UI. When setting up a report, there is no way to know which user will actually be impersonated for the execution, making permission issues hard to diagnose and fix. This is specially important for Alerts & Reports powered by DB connections authenticated with OAuth2, since the impersonated account must have a valid DB OAuth2 token.

#### Restrictive `ALERTS_ATTACH_REPORTS` config
The `ALERTS_ATTACH_REPORTS` config allow users to entirely enable/disable attachments for alerts. Users need more flexibility to use a mixed approach (avoid attachments for a few configurations only).

### Proposed Changes

#### Per-report "Run As" field
Add a "Run As" field to each Alert/Report that specifies which user's RBAC permissions and OAuth2 tokens will be used at execution time. Alerts should have an additional "Run As" field for the alert SQL query, since the target audience might not have access to the DB table(s) used for validation.

For non-admins scheduling reports directly from a dashboard, the field would not be visible and automatically assigned to the logged-in user. API validation would also ensure that non-admins can only set up reports that impersonate their own account. **This ensures that a user can't schedule a report to get elevated access**.

The "Run As" fields in the UI (and API validations) will be gated behind a new FF `ALERT_REPORT_DYNAMIC_EXECUTOR`.

#### Global Alerts & Reports Configuration
Add an **Alerts & Reports configuration** button to the Alerts & Reports list page. Clicking on it opens a modal that exposes 5 configurations:

| Config | Status | Description |
|--------|--------|--------|
| Enable Attachments for Alerts | Migrate from `ALERTS_ATTACH_REPORTS` config. | Enable/disable attachments for Alerts. |
| Alert minimum interval | Migrate from `ALERT_MINIMUM_INTERVAL` config. | Specify the minimum allowed interval for an alert schedule. |
| Report minimum interval | Migrate from `REPORT_MINIMUM_INTERVAL` config. | Specify the minimum allowed interval for a report schedule. |
| Limit recipient to users | New config. | Limit the list of email recipients to existing users in the instance. |
| Allowed domains | New config. | List of allowed email domains to be used as recipients. |

These configurations would persist to a new DB table, with a proper DAO and `validate` method to fail when the new config conflicts with any existing alert/report, informing the user which one(s) should be adjusted before the desired configurations can be saved. The only exception here would be the "Enable Attachments for Alerts" config, changing this value will reflect on all existing Alerts immediately. The attachment configurations wouldn't be deleted (if disabled), just ignored. The field in the UI would also no longer be visible.

#### Allow the creation of Alerts without attachments
If the "Enable Attachments for Alerts" config is OFF, hide the attachments configuration for Alerts (current behavior with the `ALERTS_ATTACH_REPORTS` FF). If the config is on, still allow the creation of Alerts without an attachment.

### New or Changed Public Interfaces

#### REST API
* `GET /api/v1/report/configuration`: Read global report configs.
* `PUT /api/v1/report/configuration`: Update global report configs. Returns an error listing reports conflicting with the new config.
* `POST /api/v1/report/` and `PUT /api/v1/report/`: `run_as` and `run_alert_query_as` added to the add/edit/show columns. Corresponding `validate` methods validating new business logic.

#### Database
* `report_schedule` table: new `run_as_fk` and `run_alert_query_as_fk` columns (FK -> `ab_user.id`).
* New `report_config` table: simple key-value table to store the report configurations.

#### Frontend
* `AlertReportModal.tsx`: New "Run As" `AsyncSelect` fields gated behind `ALERT_REPORT_DYNAMIC_EXECUTOR` FF. Continue hiding the attachment configuration for Alerts if "Enable Attachments for Alerts" is OFF, make the configuration optional when it's ON.
* `AlertReportList/index.tsx`: New "Alerts & Reports configuration" button.
* New `ReportConfigurationModal.tsx`: form for the 5 global settings. Also displays reports conflicting with new proposed config.

#### Execution change
`AsyncExecuteReportScheduleCommand` will use `report_schedule.run_as` directly instead of calling `get_executor()` in case the `ALERT_REPORT_DYNAMIC_EXECUTOR` FF is enabled and the column has a value.

The execution will error out if:
1. The list of recipients doesn't comply with `allowed_email_domains` and `user_only` configurations.
2. The user that should be impersonated for execution is no longer available/active.

### New dependencies
None. All changes use existing patterns.

### Migration Plan and Compatibility

#### Alembic migration
1. Add `run_as_fk` and `run_alert_query_as_fk` columns set to `null`.
2. Create the new `report_config` table, with all configs set to `null`/`false`.
3. `ALERTS_ATTACH_REPORTS`, `ALERT_MINIMUM_INTERVAL`, `REPORT_MINIMUM_INTERVAL` would start being evaluated from this table, but fallback to existing app config if undefined.

#### App config changes
1. Add deprecation notice to the `ALERTS_ATTACH_REPORTS`, `ALERT_REPORTS_EXECUTORS`, `ALERT_MINIMUM_INTERVAL` and `REPORT_MINIMUM_INTERVAL` configs.
2. Add a new `ALERT_REPORT_DYNAMIC_EXECUTOR` FF set to `False` by default.

#### Backwards compatibility

By default, this SIP doesn't change anything:
* The migration just adds a new table and columns. Columns initiate as empty to avoid any change in behavior.
* `ALERT_REPORT_EXECUTORS` continue to work as is.
* `ALERT_REPORT_DYNAMIC_EXECUTOR` set to `False` by default.
* The attachment configuration for Alerts **would now be optional**, but this shouldn't cause any issues. It just adds more flexibility.

#### Rollout Strategy
Roll it out as a Beta/Testing feature, to get feedback and work on any improvements:
1. FF OFF, everything work as is. The only real change from day one is that Attachments are now optional when setting up Alerts.
2. Once the FF is enabled, the "Run As" fields are visible.
a. Existing Reports will still have the columns empty, and therefore work as is.
b. New report configs will require the field. For users setting up from the dashboard, their account will be used as well.
c. When editing existing Alerts/Reports, the fields would render defaulting to the `ALERT_REPORT_EXECUTORS` value. Saving would populate the column in the backend.

Once the feature is considered stable by the Community, start logging warnings on boot for instances using deprecated configs. In a future major version:

1. Implement a DB migration to populate any missing `report_schedule.run_as` and `report_schedule.run_alert_query_as` based on the `ALERT_REPORT_EXECUTORS` value.
2. Remove the deprecated configurations.
3. Set the new DB columns as required.

### Rejected Alternatives
- `ExecutorType` (Owner, Creator, Modifier, etc) enum in the “Run As" field: this mirrors the existing `ALERT_REPORTS_EXECUTORS` structure and would be more flexible, but it pushes the complexity back onto admins. A concrete user is easier to reason about and audit.
- Use configs for “Limit recipients to users” and “Allowed domains”: this would prevent UI modifications, and also the ability to validate and identify existing configurations that aren’t compatible with updated settings.
- Use existing `key_value` table for report configurations: while it would avoid creating a new table, the purpose of the existing table is different.
- Adopt the “Future stable rollout plan” from day one: while migrating away from `ALERT_REPORTS_EXECUTORS` directly would simplify the rollout plan, we have to consider that Alerts & Reports are crucial to several Organizations. Allowing a feedback loop for this new feature is important to ensure feature parity, consistency, and real improvements.
- Implement a User Picker on Recipients, so that Alerts & Reports are sent to accounts, not emails: while it’s an interesting approach to ensure reports are only sent to users and also allow a single report config to send distinct values based on the recipient accounts, it presents scalability issues. For example, a single report schedule could be set to every 5 minutes and include 20 users, requiring either 20 parallel tasks to get the data (intense on resources), or 20 sequential executions (suboptimal performance and likely not finished before the next execution). Could be explored in a V2 but out of scope now.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.