-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Conversation
58fb4cc
to
1431744
Compare
Codecov Report
@@ Coverage Diff @@
## master #21765 +/- ##
==========================================
- Coverage 66.85% 66.68% -0.17%
==========================================
Files 1805 1805
Lines 69061 69071 +10
Branches 7369 7369
==========================================
- Hits 46169 46062 -107
- Misses 20995 21112 +117
Partials 1897 1897
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -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". | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@villebro I asked for some product/design feedback on what an alpha can see on the list page, too. |
01ae49c
to
8b685d0
Compare
hi @villebro !! I'm wondering if it might make sense for an Alpha user to see all Alerts & Reports in the list view, but with no actions in the actions column and a disabled "Active" toggle if they aren't the owner, like the bottom row here: In the future, maybe there could be a "View Only" mode to see report details even if you can't edit (though that is probably out of the scope of this PR) Open to feedback here though, what are your thoughts? |
3076df0
to
a54b285
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ReportScheduleStateMachine( | ||
session, self._execution_id, self._model, self._scheduled_dttm | ||
).run() | ||
with override_user(_get_user()): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"])
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def create_working_admin_report_schedule(self): | ||
with self.create_app().app_context(): | ||
|
||
admin_user = self.get_user("admin") |
There was a problem hiding this comment.
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):
....
....
There was a problem hiding this comment.
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.
@pytest.fixture() | ||
def create_working_alpha_report_schedule(self): | ||
with self.create_app().app_context(): | ||
|
||
alpha_user = self.get_user("alpha") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
I see where you're coming from, and I agree in the context of Alerts and Reports it could make sense to have elevated privileges for the Alpha role. However, in the current context, this would go against current RBAC conventions, as Alpha is only able to see owned Charts and Dashboards. Having different logic for what Alpha can see on Dashboards and Charts vs Alerts and Reports could be confusing. For this reason I'd almost propose starting a separate discussion about what Alpha should and should not be able to see, and then apply this consistently throughout all object types. Ping @dpgaspar , do you have any thoughts on this topic? |
@yousoph I stand corrected - the Alpha role does indeed have the |
b140db9
to
cb951b1
Compare
@yousoph I've changed the frontend so that non-admins will have a slightly different UX depending on whether or not they are owners (see screenshots in description) |
/testenv up FEATURE_ALERT_REPORTS=true |
@villebro Ephemeral environment spinning up at http://54.202.241.131:8080. Credentials are |
Thanks for the updates, this is looking great!! One thing I noticed for the Alpha user - when they try to save changes on a report that they don't own, they see a toast with this message: I think the buttons on the modal should be hidden (or disabled)... or if that's not doable we should at least update the toast message to clarify to the user why the save failed. |
I noticed the same (there's also an annoying pluralization on the model name, making it "Reportss" 😄 I checked that fixing these is slightly more work than I have time for now, but I'll try to find time to fix these in a follow up PR. |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Currently Gamma users have read and write permissions for Alerts & Reports and menu access for "Alerts & Reports". However, since they don't have access to the "Manage" menu, they can't see the menu item. This means that they can actually access the list view if the URL is provided to them. In addition, the list view shows all entries in the report schedule, although users are only able to edit entries they own.
This PR does the following:
all_datasource_access
permUPDATING.md
with instructions on how to give perms to Gamma usersAFTER
When an Alpha user enters the list view, non-owned objects will appear with the toggle disabled, no trashcan and the "Edit" + Pen replaced with "View" + Binoculars (note that I didn't change the modal by dsabling fields etc, as changing that is a slightly bigger effort):
![image](https://user-images.githubusercontent.com/33317356/195788432-9a1560fe-b7ad-4f4e-b2f7-7bdd1e66cb13.png)
When a regular Gamma user tries to access the Alerts & Reports list view with the direct URL, they get an "Access denied" error:
![image](https://user-images.githubusercontent.com/33317356/195040780-a5a5c65a-ded8-4c3e-bbc0-65253ce7d1eb.png)
When a Gamma user with the
![image](https://user-images.githubusercontent.com/33317356/195040724-e98542af-37ab-4fdd-9907-ae075eae2270.png)
read on ReportSchedule
perm accesses the list view, they can only see their own entries in the list view, despite there being four entries in the report schedule (see the Before screenshot):BEFORE
Previously the Gamma and Alpha user saw and were able to edit all users' entries, despite not being able to change them (here I'm trying to enable another user's alert as a Gamma user, which fails due to missing perms).
![image](https://user-images.githubusercontent.com/33317356/195040833-0b535e16-483a-4d18-85b3-18ca2e3ab5b5.png)
Previously Gamma users were able to access the list view if they knew the URL, despite not having menu access:
![image](https://user-images.githubusercontent.com/33317356/195040879-23e75e09-7aa8-4712-bd24-90cb22cef4ba.png)
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION