Skip to content

Commit

Permalink
fix(alerts): restrict list view and gamma perms (#21765)
Browse files Browse the repository at this point in the history
  • Loading branch information
villebro committed Oct 15, 2022
1 parent 196c367 commit 4c1777f
Show file tree
Hide file tree
Showing 10 changed files with 221 additions and 41 deletions.
2 changes: 2 additions & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion superset-frontend/src/views/CRUD/alert/AlertList.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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}`,
Expand All @@ -67,6 +67,8 @@ const mockalerts = [...new Array(3)].map((_, i) => ({

const mockUser = {
userId: 1,
firstName: 'user 1',
lastName: 'lastname',
};

fetchMock.get(alertsEndpoint, {
Expand Down
37 changes: 25 additions & 12 deletions superset-frontend/src/views/CRUD/alert/AlertList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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',
Expand All @@ -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
? {
Expand All @@ -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'),
Expand Down
6 changes: 5 additions & 1 deletion superset/reports/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down
37 changes: 18 additions & 19 deletions superset/reports/commands/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
2 changes: 2 additions & 0 deletions superset/reports/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -43,6 +44,7 @@

class ReportScheduleDAO(BaseDAO):
model_cls = ReportSchedule
base_filter = ReportScheduleFilter

@staticmethod
def find_by_chart_id(chart_id: int) -> List[ReportSchedule]:
Expand Down
16 changes: 16 additions & 0 deletions superset/reports/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions superset/security/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"Queries",
"Import dashboards",
"Upload a CSV",
"ReportSchedule",
"Alerts & Report",
}

ADMIN_ONLY_PERMISSIONS = {
Expand Down

0 comments on commit 4c1777f

Please sign in to comment.