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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e6b6799
fix: remove alerts and reports perms for gamma
villebro Oct 10, 2022
9d928a8
add base filter for report schedule
villebro Oct 11, 2022
8678320
fix old tests
villebro Oct 11, 2022
ca2939f
add tests
villebro Oct 11, 2022
fbd2e2d
check g.user
villebro Oct 12, 2022
24409b0
address comment on UPDATING.md
villebro Oct 13, 2022
4bdcfee
make all alerts visible to alpha and add gamma tests
villebro Oct 13, 2022
cb951b1
disable forbidden actions
villebro Oct 14, 2022
c2761a3
fix test
villebro Oct 14, 2022
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
Prev Previous commit
Next Next commit
add tests
  • Loading branch information
@villebro
villebro committed Oct 14, 2022
commit ca2939fa0813a2a0788b7878f3f7fa674e080e5d
2 changes: 2 additions & 0 deletions UPDATING.md
View file
Open in desktop
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): Gamma users will no longer have read and write access to Alerts & Reports. To give Gamma users the ability to schedule alerts and reports like before, create an additional role with "can read on ReportSchedule", "can write on ReportSchedule", "menu access on Manage" and "menu access on Alerts & Report".

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
2 changes: 1 addition & 1 deletion superset/reports/filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from typing import Any

from flask_babel import lazy_gettext as _
from sqlalchemy import and_, or_
from sqlalchemy import or_
from sqlalchemy.orm.query import Query

from superset import db, security_manager
Expand Down
107 changes: 102 additions & 5 deletions tests/integration_tests/reports/api_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import pytest
import prison
from parameterized import parameterized
from sqlalchemy.sql import func

from superset import db
Expand Down Expand Up @@ -52,17 +53,67 @@

class TestReportSchedulesApi(SupersetTestCase):
@pytest.fixture()
def create_working_report_schedule(self):
def create_working_admin_report_schedule(self):
with self.create_app().app_context():

admin_user = self.get_user("admin")
Copy link
Member

Choose a reason for hiding this comment

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

one nit, in order to avoid extra indeed and context grammar, we should use app_context and login_admin fixtures

def create_working_report_schedule(app_context, login_admin):
    ....
    ....

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason I'm having trouble using these fixtures with the class-based tests. Let's leave this for a follow-up refactor PR when these tests are refactored into functional tests.

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.fixture()
def create_working_alpha_report_schedule(self):
with self.create_app().app_context():

alpha_user = self.get_user("alpha")
Copy link
Member

Choose a reason for hiding this comment

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

there is a fixture called login_as might help for this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

chart = db.session.query(Slice).first()
example_db = get_example_database()

report_schedule = insert_report_schedule(
type=ReportScheduleType.ALERT,
name="name_working",
name="name_alpha_working",
crontab="* * * * *",
sql="SELECT value from table",
description="Report working",
chart=chart,
database=example_db,
owners=[alpha_user],
last_state=ReportState.WORKING,
)

yield

db.session.delete(report_schedule)
db.session.commit()

@pytest.fixture()
def create_working_shared_report_schedule(self):
with self.create_app().app_context():

admin_user = self.get_user("admin")
alpha_user = self.get_user("alpha")
chart = db.session.query(Slice).first()
example_db = get_example_database()

report_schedule = insert_report_schedule(
type=ReportScheduleType.ALERT,
name="name_shared_working",
crontab="* * * * *",
sql="SELECT value from table",
description="Report working",
Expand Down Expand Up @@ -305,6 +356,52 @@ 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_alpha_working",
"name_shared_working",
},
),
(
"alpha",
{
"name_alpha_working",
"name_shared_working",
},
),
],
)
@pytest.mark.usefixtures(
"create_working_admin_report_schedule",
"create_working_alpha_report_schedule",
"create_working_shared_report_schedule",
)
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 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):
"""
Expand Down Expand Up @@ -1159,14 +1256,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()
)

Expand All @@ -1177,7 +1274,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
Expand Down