Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(alerts): restrict list view and gamma perms #21765

Merged
merged 9 commits into from
Oct 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro I wonder if someone were to read this entry without seeing your explanation in the PR, if they would think that they need to grant access to Gamma users to keep the same functionality that we have now. This is tricky, because to most people it won't look like a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I'll make the entry more explicit, specifically scoping this to deployments that have enabled the "ALERT_REPORTS" feature flag.

### 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()):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this method always runs on a celery async context I see no issue here, but what are we trying to solve?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it for switching current user to the user = security_manager.find_user(username=app.config["THUMBNAIL_SELENIUM_USER"])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhaoyongjie yes it switches the user, just wondering why we need g.user set on async tasks, to set changed_by fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new BaseFilter checks sm.is_admin(), which in turn checks the roles of g.user, which otherwise will be unset. So without this change executing Alerts and Reports fails (the integration tests thankfully picked this up 👍 )

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
Loading