You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2022/10/15 07:03:42 UTC
[superset] branch master updated: fix(alerts): restrict list view and gamma perms (#21765)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 4c1777f20d fix(alerts): restrict list view and gamma perms (#21765)
4c1777f20d is described below
commit 4c1777f20d6ca3a91383ba7fc042f20c286a7795
Author: Ville Brofeldt <33...@users.noreply.github.com>
AuthorDate: Sat Oct 15 10:03:26 2022 +0300
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 ++++++++++++++++++++-
tests/integration_tests/reports/commands_tests.py | 2 +-
10 files changed, 221 insertions(+), 41 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index f41b160c81..4cbf33489a 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 CR [...]
+
### 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 5cf9483a52..d5b4d24150 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 cd9ba95608..c907d63f05 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) => (
- <Switch
- data-test="toggle-active"
- checked={original.active}
- onClick={(checked: boolean) => toggleActive(original, checked)}
- size="small"
- />
- ),
+ Cell: ({ row: { original } }: any) => {
+ const allowEdit =
+ original.owners.map((o: Owner) => o.id).includes(user.userId) ||
+ isUserAdmin(user);
+
+ return (
+ <Switch
+ disabled={!allowEdit}
+ data-test="toggle-active"
+ checked={original.active}
+ onClick={(checked: boolean) => 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 565d05c703..f84d6287e1 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 cd83255a90..41efb4821d 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 b4250af645..be5ee8053c 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 699d10fc9a..5fb87e0563 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 eaf827ef90..ea6040ced0 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 b39ea5308a..a304f08315 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 02e5ce8bb4..c82c1b5fdb 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