From 4c1777f20d6ca3a91383ba7fc042f20c286a7795 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Sat, 15 Oct 2022 10:03:26 +0300 Subject: [PATCH] fix(alerts): restrict list view and gamma perms (#21765) --- UPDATING.md | 2 + .../src/views/CRUD/alert/AlertList.test.jsx | 4 +- .../src/views/CRUD/alert/AlertList.tsx | 37 +++-- superset/reports/api.py | 6 +- superset/reports/commands/execute.py | 37 ++--- superset/reports/dao.py | 2 + superset/reports/filters.py | 16 ++ superset/security/manager.py | 2 + tests/integration_tests/reports/api_tests.py | 154 +++++++++++++++++- .../reports/commands_tests.py | 2 +- 10 files changed, 221 insertions(+), 41 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index f41b160c81e1d..4cbf33489a435 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 5cf9483a526e0..d5b4d241502c0 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 cd9ba95608642..c907d63f05d18 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 565d05c7033e5..f84d6287e1e85 100644 --- a/superset/reports/api.py +++ b/superset/reports/api.py @@ -43,7 +43,7 @@ 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 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: 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 cd83255a907c7..41efb4821d507 100644 --- a/superset/reports/commands/execute.py +++ b/superset/reports/commands/execute.py @@ -68,7 +68,7 @@ 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 @@ 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 @@ def _get_url( **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 @@ def _get_screenshots(self) -> List[bytes]: 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 @@ def _get_embedded_data(self) -> pd.DataFrame: """ 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 @@ def __init__(self, task_id: str, model_id: int, scheduled_dttm: datetime): 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 b4250af6453f2..be5ee8053c48b 100644 --- a/superset/reports/dao.py +++ b/superset/reports/dao.py @@ -26,6 +26,7 @@ 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 @@ 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 699d10fc9afb9..5fb87e0563345 100644 --- a/superset/reports/filters.py +++ b/superset/reports/filters.py @@ -20,10 +20,26 @@ 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 eaf827ef90463..ea6040ced02a3 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 b39ea5308a8e9..a304f083159ab 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -23,9 +23,10 @@ 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.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 @@ def create_working_report_schedule(self): 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 @@ def test_get_list_report_schedule(self): 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 @@ def test_update_report_schedule(self): 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 @@ def test_update_report_schedule_state_working(self): 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 02e5ce8bb4dbf..c82c1b5fdb2d1 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