You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/10/11 08:39:24 UTC
[GitHub] [superset] villebro opened a new pull request, #21765: fix(alerts): restrict list view and gamma perms
villebro opened a new pull request, #21765:
URL: https://github.com/apache/superset/pull/21765
### SUMMARY
Currently Gamma users have read and write permissions for Alerts & Reports and access the "Alerts & Reports" menu. However, since they don't have access to the "Manage" menu, they can't see the menu. 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:
- Removes "can read on ReportSchedule", "can write on ReportSchedule" and "Alerts & Report" permissions from Gamma users
- Adds a new base filter to only show owned entries for non-admin users
- Updates existing tests where attempting to change non-owned entries resulted in a 403 (these are now 404)
- Adds tests to assert that admin and alpha users see the correct entries in the list view (admin sees all, alpha only owned entries)
- Adds test to assert that gamma user gets a 403 on the list view.
- Adds an entry to `UPDATING.md` with instructions on
### AFTER
Now an Alpha user can only see their own entries in the list view, despite there being four entries in the report schedule:
When a Gamma user tries to access the Alerts & Reports list view with the direct URL, they get an "Access denied" error:
### BEFORE
Previously the same user saw all users' entries, despite not being able to change them (here I'm trying to enable another user's alert as an Alpha user, which fails due to missing perms).
Previously Gamma users were able to access the list view if they knew the URL, despite not having menu access:
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1277053011
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:
There could be a case where an Alpha user is a recipient of a report but not the owner, and not seeing it in the list view could be confusing or lead to duplicates being created unintentionally.
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1278607253
@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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1279679663
> 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:
>
> An error occurred while fetching reports: "Forbidden"
>
>
>
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro merged PR #21765:
URL: https://github.com/apache/superset/pull/21765
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994455757
##########
superset/reports/commands/execute.py:
##########
@@ -675,12 +673,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()):
Review Comment:
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 ๐ )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1277598937
> > 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:  There could be a case where an Alpha user is a recipient of a report but not the owner, and not seeing it in the list view could be confusing or lead to duplicates being created unintentionally.
> > 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?
>
> 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.
@yousoph I stand corrected - the Alpha role does indeed have the `can_access_all_datasources`, which gives the Alpha user full visibility into all dashboards, datasets and charts. I'm going to change this logic so that instead of checking `is_admin`, I'm going to check for `can_access_all_datasources`, and if that's the case, it will show all Alerts & Reports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r992867099
##########
UPDATING.md:
##########
@@ -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".
+
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994329253
##########
superset/reports/commands/execute.py:
##########
@@ -675,12 +673,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()):
Review Comment:
i think it for switching current user to the `user = security_manager.find_user(username=app.config["THUMBNAIL_SELENIUM_USER"])`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1278725965
@villebro Ephemeral environment spinning up at http://54.202.241.131:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dpgaspar commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994311029
##########
superset/reports/commands/execute.py:
##########
@@ -675,12 +673,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()):
Review Comment:
since this method always runs on a celery async context I see no issue here, but what are we trying to solve?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1277380495
> 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:  There could be a case where an Alpha user is a recipient of a report but not the owner, and not seeing it in the list view could be confusing or lead to duplicates being created unintentionally.
>
> 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?
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1274439250
# [Codecov](https://codecov.io/gh/apache/superset/pull/21765?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21765](https://codecov.io/gh/apache/superset/pull/21765?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (58fb4cc) into [master](https://codecov.io/gh/apache/superset/commit/d7ee443a134f88aa7fff8d2d038b50c94fb039e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d7ee443) will **decrease** coverage by `11.37%`.
> The diff coverage is `63.63%`.
> :exclamation: Current head 58fb4cc differs from pull request most recent head 1431744. Consider uploading reports for the commit 1431744 to get more accurate results
```diff
@@ Coverage Diff @@
## master #21765 +/- ##
===========================================
- Coverage 66.85% 55.47% -11.38%
===========================================
Files 1800 1800
Lines 68967 68977 +10
Branches 7339 7339
===========================================
- Hits 46107 38268 -7839
- Misses 20968 28817 +7849
Partials 1892 1892
```
| Flag | Coverage ฮ | |
|---|---|---|
| hive | `52.93% <63.63%> (+<0.01%)` | :arrow_up: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.83% <63.63%> (+<0.01%)` | :arrow_up: |
| python | `57.93% <63.63%> (-23.52%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.05% <63.63%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21765?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage ฮ | |
|---|---|---|
| [superset/security/manager.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `64.71% <รธ> (-31.04%)` | :arrow_down: |
| [superset/reports/filters.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9maWx0ZXJzLnB5) | `61.90% <42.85%> (-30.96%)` | :arrow_down: |
| [superset/reports/api.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9hcGkucHk=) | `58.95% <100.00%> (-31.28%)` | :arrow_down: |
| [superset/reports/dao.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9kYW8ucHk=) | `39.68% <100.00%> (-45.00%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| ... and [287 more](https://codecov.io/gh/apache/superset/pull/21765/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: Weโre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
yousoph commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1279493232
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:
An error occurred while fetching reports: "Forbidden"
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994319333
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -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")
+ 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")
Review Comment:
there is a fixture called `login_as` might help for this case.
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -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")
Review Comment:
one nit, in order to avoid extra indeed and context grammar, we should use `app_context` and `login_admin` fixtures
```python
def create_working_report_schedule(app_context, login_admin):
....
....
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1275416263
@villebro I asked for some product/design feedback on what an alpha can see on the list page, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1279679788
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21765:
URL: https://github.com/apache/superset/pull/21765#issuecomment-1278645302
/testenv up FEATURE_ALERT_REPORTS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r993341352
##########
UPDATING.md:
##########
@@ -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".
+
Review Comment:
Good point - I'll make the entry more explicit, specifically scoping this to deployments that have enabled the "ALERT_REPORTS" feature flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] dpgaspar commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994340813
##########
superset/reports/commands/execute.py:
##########
@@ -675,12 +673,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()):
Review Comment:
@zhaoyongjie yes it switches the user, just wondering why we need `g.user` set on async tasks, to set `changed_by` fields?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21765: fix(alerts): restrict list view and gamma perms
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21765:
URL: https://github.com/apache/superset/pull/21765#discussion_r994630958
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -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")
Review Comment:
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.
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -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")
+ 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")
Review Comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org