diff --git a/UPDATING.md b/UPDATING.md index f41b160c8..4cbf33489 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -32,6 +32,8 @@ assists people when migrating to a new version. ### Breaking Changes +- [21765](https://github.com/apache/superset/pull/21765): For deployments that have enabled the "ALERT_REPORTS" feature flag, Gamma users will no longer have read and write access to Alerts & Reports by default. To give Gamma users the ability to schedule reports from the Dashboard and Explore view like before, create an additional role with "can read on ReportSchedule" and "can write on ReportSchedule" permissions. To further give Gamma users access to the "Alerts & Reports" menu and CRUD view, add "menu access on Manage" and "menu access on Alerts & Report" permissions to the role. + ### Potential Downtime ### Other diff --git a/superset-frontend/src/views/CRUD/alert/AlertList.test.jsx b/superset-frontend/src/views/CRUD/alert/AlertList.test.jsx index 5cf9483a5..d5b4d2415 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertList.test.jsx +++ b/superset-frontend/src/views/CRUD/alert/AlertList.test.jsx @@ -55,7 +55,7 @@ const mockalerts = [...new Array(3)].map((_, i) => ({ last_eval_dttm: Date.now(), last_state: 'ok', name: `alert ${i} `, - owners: [], + owners: [{ id: 1 }], recipients: [ { id: `${i}`, @@ -67,6 +67,8 @@ const mockalerts = [...new Array(3)].map((_, i) => ({ const mockUser = { userId: 1, + firstName: 'user 1', + lastName: 'lastname', }; fetchMock.get(alertsEndpoint, { diff --git a/superset-frontend/src/views/CRUD/alert/AlertList.tsx b/superset-frontend/src/views/CRUD/alert/AlertList.tsx index cd9ba9560..c907d63f0 100644 --- a/superset-frontend/src/views/CRUD/alert/AlertList.tsx +++ b/superset-frontend/src/views/CRUD/alert/AlertList.tsx @@ -44,6 +44,8 @@ import { useSingleViewResource, } from 'src/views/CRUD/hooks'; import { createErrorHandler, createFetchRelated } from 'src/views/CRUD/utils'; +import { isUserAdmin } from 'src/dashboard/util/permissionUtils'; +import Owner from 'src/types/Owner'; import AlertReportModal from './AlertReportModal'; import { AlertObject, AlertState } from './types'; @@ -316,14 +318,21 @@ function AlertList({ size: 'xl', }, { - Cell: ({ row: { original } }: any) => ( - toggleActive(original, checked)} - size="small" - /> - ), + Cell: ({ row: { original } }: any) => { + const allowEdit = + original.owners.map((o: Owner) => o.id).includes(user.userId) || + isUserAdmin(user); + + return ( + toggleActive(original, checked)} + size="small" + /> + ); + }, Header: t('Active'), accessor: 'active', id: 'active', @@ -337,6 +346,10 @@ function AlertList({ const handleGotoExecutionLog = () => history.push(`/${original.type.toLowerCase()}/${original.id}/log`); + const allowEdit = + original.owners.map((o: Owner) => o.id).includes(user.userId) || + isUserAdmin(user); + const actions = [ canEdit ? { @@ -349,14 +362,14 @@ function AlertList({ : null, canEdit ? { - label: 'edit-action', - tooltip: t('Edit'), + label: allowEdit ? 'edit-action' : 'preview-action', + tooltip: allowEdit ? t('Edit') : t('View'), placement: 'bottom', - icon: 'Edit', + icon: allowEdit ? 'Edit' : 'Binoculars', onClick: handleEdit, } : null, - canDelete + allowEdit && canDelete ? { label: 'delete-action', tooltip: t('Delete'), diff --git a/superset/reports/api.py b/superset/reports/api.py index 565d05c70..f84d6287e 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -43,7 +43,7 @@ from superset.reports.commands.exceptions import ( ReportScheduleUpdateFailedError, ) from superset.reports.commands.update import UpdateReportScheduleCommand -from superset.reports.filters import ReportScheduleAllTextFilter +from superset.reports.filters import ReportScheduleAllTextFilter, ReportScheduleFilter from superset.reports.models import ReportSchedule from superset.reports.schemas import ( get_delete_ids_schema, @@ -80,6 +80,10 @@ class ReportScheduleRestApi(BaseSupersetModelRestApi): resource_name = "report" allow_browser_login = True + base_filters = [ + ["id", ReportScheduleFilter, lambda: []], + ] + show_columns = [ "id", "active", diff --git a/superset/reports/commands/execute.py b/superset/reports/commands/execute.py index cd83255a9..41efb4821 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -68,7 +68,7 @@ from superset.reports.notifications import create_notification from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.exceptions import NotificationError from superset.utils.celery import session_scope -from superset.utils.core import HeaderDataType +from superset.utils.core import HeaderDataType, override_user from superset.utils.csv import get_chart_csv_data, get_chart_dataframe from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot from superset.utils.urls import get_url_path @@ -77,6 +77,13 @@ from superset.utils.webdriver import DashboardStandaloneMode logger = logging.getLogger(__name__) +def _get_user() -> User: + user = security_manager.find_user(username=app.config["THUMBNAIL_SELENIUM_USER"]) + if not user: + raise ReportScheduleSelleniumUserNotFoundError() + return user + + class BaseReportState: current_states: List[ReportState] = [] initial: bool = False @@ -193,22 +200,13 @@ class BaseReportState: **kwargs, ) - @staticmethod - def _get_user() -> User: - user = security_manager.find_user( - username=app.config["THUMBNAIL_SELENIUM_USER"] - ) - if not user: - raise ReportScheduleSelleniumUserNotFoundError() - return user - def _get_screenshots(self) -> List[bytes]: """ Get chart or dashboard screenshots :raises: ReportScheduleScreenshotFailedError """ url = self._get_url() - user = self._get_user() + user = _get_user() if self._report_schedule.chart: screenshot: Union[ChartScreenshot, DashboardScreenshot] = ChartScreenshot( url, @@ -239,7 +237,7 @@ class BaseReportState: def _get_csv_data(self) -> bytes: url = self._get_url(result_format=ChartDataResultFormat.CSV) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies( - self._get_user() + _get_user() ) if self._report_schedule.chart.query_context is None: @@ -265,7 +263,7 @@ class BaseReportState: """ url = self._get_url(result_format=ChartDataResultFormat.JSON) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies( - self._get_user() + _get_user() ) if self._report_schedule.chart.query_context is None: @@ -679,12 +677,13 @@ class AsyncExecuteReportScheduleCommand(BaseCommand): def run(self) -> None: with session_scope(nullpool=True) as session: try: - self.validate(session=session) - if not self._model: - raise ReportScheduleExecuteUnexpectedError() - ReportScheduleStateMachine( - session, self._execution_id, self._model, self._scheduled_dttm - ).run() + with override_user(_get_user()): + self.validate(session=session) + if not self._model: + raise ReportScheduleExecuteUnexpectedError() + ReportScheduleStateMachine( + session, self._execution_id, self._model, self._scheduled_dttm + ).run() except CommandException as ex: raise ex except Exception as ex: diff --git a/superset/reports/dao.py b/superset/reports/dao.py index b4250af64..be5ee8053 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -26,6 +26,7 @@ from sqlalchemy.orm import Session from superset.dao.base import BaseDAO from superset.dao.exceptions import DAOCreateFailedError, DAODeleteFailedError from superset.extensions import db +from superset.reports.filters import ReportScheduleFilter from superset.reports.models import ( ReportExecutionLog, ReportRecipients, @@ -43,6 +44,7 @@ REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER = "Notification sent with error" class ReportScheduleDAO(BaseDAO): model_cls = ReportSchedule + base_filter = ReportScheduleFilter @staticmethod def find_by_chart_id(chart_id: int) -> List[ReportSchedule]: diff --git a/superset/reports/filters.py b/superset/reports/filters.py index 699d10fc9..5fb87e056 100644 --- a/superset/reports/filters.py +++ b/superset/reports/filters.py @@ -20,10 +20,26 @@ from flask_babel import lazy_gettext as _ from sqlalchemy import or_ from sqlalchemy.orm.query import Query +from superset import db, security_manager from superset.reports.models import ReportSchedule from superset.views.base import BaseFilter +class ReportScheduleFilter(BaseFilter): # pylint: disable=too-few-public-methods + def apply(self, query: Query, value: Any) -> Query: + if security_manager.can_access_all_datasources(): + return query + owner_ids_query = ( + db.session.query(ReportSchedule.id) + .join(ReportSchedule.owners) + .filter( + security_manager.user_model.id + == security_manager.user_model.get_user_id() + ) + ) + return query.filter(ReportSchedule.id.in_(owner_ids_query)) + + class ReportScheduleAllTextFilter(BaseFilter): # pylint: disable=too-few-public-methods name = _("All Text") arg_name = "report_all_text" diff --git a/superset/security/manager.py b/superset/security/manager.py index eaf827ef9..ea6040ced 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -184,6 +184,8 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "Queries", "Import dashboards", "Upload a CSV", + "ReportSchedule", + "Alerts & Report", } ADMIN_ONLY_PERMISSIONS = { diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index b39ea5308..a304f0831 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -23,9 +23,10 @@ import pytz import pytest import prison +from parameterized import parameterized from sqlalchemy.sql import func -from superset import db +from superset import db, security_manager from superset.models.core import Database from superset.models.slice import Slice from superset.models.dashboard import Dashboard @@ -48,11 +49,95 @@ from tests.integration_tests.fixtures.birth_names_dashboard import ( from tests.integration_tests.reports.utils import insert_report_schedule REPORTS_COUNT = 10 +REPORTS_ROLE_NAME = "reports_role" +REPORTS_GAMMA_USER = "reports_gamma" class TestReportSchedulesApi(SupersetTestCase): @pytest.fixture() - def create_working_report_schedule(self): + def gamma_user_with_alerts_role(self): + with self.create_app().app_context(): + user = self.create_user( + REPORTS_GAMMA_USER, + "general", + "Gamma", + email=f"{REPORTS_GAMMA_USER}@superset.org", + ) + + security_manager.add_role(REPORTS_ROLE_NAME) + read_perm = security_manager.find_permission_view_menu( + "can_read", + "ReportSchedule", + ) + write_perm = security_manager.find_permission_view_menu( + "can_write", + "ReportSchedule", + ) + reports_role = security_manager.find_role(REPORTS_ROLE_NAME) + security_manager.add_permission_role(reports_role, read_perm) + security_manager.add_permission_role(reports_role, write_perm) + user.roles.append(reports_role) + + yield user + + # rollback changes (assuming cascade delete) + db.session.delete(reports_role) + db.session.delete(user) + db.session.commit() + + @pytest.fixture() + def create_working_admin_report_schedule(self): + with self.create_app().app_context(): + + admin_user = self.get_user("admin") + chart = db.session.query(Slice).first() + example_db = get_example_database() + + report_schedule = insert_report_schedule( + type=ReportScheduleType.ALERT, + name="name_admin_working", + crontab="* * * * *", + sql="SELECT value from table", + description="Report working", + chart=chart, + database=example_db, + owners=[admin_user], + last_state=ReportState.WORKING, + ) + + yield + + db.session.delete(report_schedule) + db.session.commit() + + @pytest.mark.usefixtures("gamma_user_with_alerts_role") + @pytest.fixture() + def create_working_gamma_report_schedule(self, gamma_user_with_alerts_role): + with self.create_app().app_context(): + + chart = db.session.query(Slice).first() + example_db = get_example_database() + + report_schedule = insert_report_schedule( + type=ReportScheduleType.ALERT, + name="name_gamma_working", + crontab="* * * * *", + sql="SELECT value from table", + description="Report working", + chart=chart, + database=example_db, + owners=[gamma_user_with_alerts_role], + last_state=ReportState.WORKING, + ) + + yield + + db.session.delete(report_schedule) + db.session.commit() + + @pytest.mark.usefixtures("gamma_user_with_alerts_role") + @pytest.fixture() + def create_working_shared_report_schedule(self, gamma_user_with_alerts_role): with self.create_app().app_context(): admin_user = self.get_user("admin") @@ -62,13 +147,13 @@ class TestReportSchedulesApi(SupersetTestCase): report_schedule = insert_report_schedule( type=ReportScheduleType.ALERT, - name="name_working", + name="name_shared_working", crontab="* * * * *", sql="SELECT value from table", description="Report working", chart=chart, database=example_db, - owners=[admin_user, alpha_user], + owners=[admin_user, alpha_user, gamma_user_with_alerts_role], last_state=ReportState.WORKING, ) @@ -305,6 +390,61 @@ class TestReportSchedulesApi(SupersetTestCase): data_keys = sorted(list(data["result"][1]["recipients"][0].keys())) assert expected_recipients_fields == data_keys + @parameterized.expand( + [ + ( + "admin", + { + "name_admin_working", + "name_gamma_working", + "name_shared_working", + }, + ), + ( + "alpha", + { + "name_admin_working", + "name_gamma_working", + "name_shared_working", + }, + ), + ( + REPORTS_GAMMA_USER, + { + "name_gamma_working", + "name_shared_working", + }, + ), + ], + ) + @pytest.mark.usefixtures( + "create_working_admin_report_schedule", + "create_working_gamma_report_schedule", + "create_working_shared_report_schedule", + "gamma_user_with_alerts_role", + ) + def test_get_list_report_schedule_perms(self, username, report_names): + """ + ReportSchedule Api: Test get list report schedules for different roles + """ + self.login(username=username) + uri = f"api/v1/report/" + rv = self.get_assert_metric(uri, "get_list") + + assert rv.status_code == 200 + data = json.loads(rv.data.decode("utf-8")) + assert {report["name"] for report in data["result"]} == report_names + + def test_get_list_report_schedule_gamma(self): + """ + ReportSchedule Api: Test get list report schedules for regular gamma user + """ + self.login(username="gamma") + uri = f"api/v1/report/" + rv = self.client.get(uri) + + assert rv.status_code == 403 + @pytest.mark.usefixtures("create_report_schedules") def test_get_list_report_schedule_sorting(self): """ @@ -1159,14 +1299,14 @@ class TestReportSchedulesApi(SupersetTestCase): assert updated_model.chart_id == report_schedule_data["chart"] assert updated_model.database_id == report_schedule_data["database"] - @pytest.mark.usefixtures("create_working_report_schedule") + @pytest.mark.usefixtures("create_working_shared_report_schedule") def test_update_report_schedule_state_working(self): """ ReportSchedule Api: Test update state in a working report """ report_schedule = ( db.session.query(ReportSchedule) - .filter(ReportSchedule.name == "name_working") + .filter(ReportSchedule.name == "name_shared_working") .one_or_none() ) @@ -1177,7 +1317,7 @@ class TestReportSchedulesApi(SupersetTestCase): assert rv.status_code == 200 report_schedule = ( db.session.query(ReportSchedule) - .filter(ReportSchedule.name == "name_working") + .filter(ReportSchedule.name == "name_shared_working") .one_or_none() ) assert report_schedule.last_state == ReportState.NOOP diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 02e5ce8bb..c82c1b5fd 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -17,7 +17,7 @@ import json from contextlib import contextmanager from datetime import datetime, timedelta, timezone -from typing import Any, Dict, List, Optional +from typing import List, Optional from unittest.mock import Mock, patch from uuid import uuid4