feat(dao): admin can remove self from object owners #15149

villebro · 2021-06-14T13:00:24Z

SUMMARY

Currently it is impossible for users to remove themselves from the list of users. This is to avoid the risk of a user mistakenly being "locked out" of a chart/dashboard/etc. This can be inconvenient if an admin edits an object, as they automatically become owners of the edited object.

This PR changes the logic as follows:

If an admin is editing an object, they can remove themselves from the list of owners. There is no risk of admins locking themselves out of an object, as they are able to edit all objects anyway.

The following is unchanged:

When a user (admins included) creates an object, they become the owner of the object.
When a non-admin edits an object and removes themselves from the list of owners, they are automatically re-added as owners. In other words, non-admin users have to be removed from the list of owners by another user (admin or regular).

BEFORE

Currently an admin is unable to remove themselves from the list of owners:
https://user-images.githubusercontent.com/33317356/121895878-5fd3c780-cd29-11eb-875b-273c323fc17e.mp4

AFTER

Now the admin can remove themselves from the owners:
https://user-images.githubusercontent.com/33317356/121895941-72e69780-cd29-11eb-988e-2ebea36a6c04.mp4

TESTING INSTRUCTIONS

Log in as admin
try to remove yourself from the list of owners
refresh the page, notice that "Admin" is again an owner

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

dpgaspar

Looks good, and I agree with the change.

Would be better to improve tests

superset/commands/utils.py

dpgaspar · 2021-06-14T17:40:53Z

superset/commands/utils.py

-    if user.id not in owner_ids:
+    if user.id not in owner_ids and "admin" not in [
+        role.name.lower() for role in user.roles
+    ]:


A bit out of scope, but there's an is_user_admin() on superset.views.base, seems misplaced and it probably would make more sense to place it on security manager. Also it uses g.user.
Another concern is that we may have a previous problem here regarding anonymous users (public access)

dpgaspar · 2021-06-14T17:48:45Z

tests/charts/api_tests.py

@@ -564,12 +564,13 @@ def test_update_chart_new_owner(self):
        gamma = self.get_user("gamma")
        admin = self.get_user("admin")
        chart_id = self.insert_chart("title", [gamma.id], 1).id
-        chart_data = {"slice_name": "title1_changed"}
+        chart_data = {"slice_name": "title1_changed", "owners": [admin.id]}


Would be nice to add a gamma user test also. Seems to be missing the same kind of test for reports and datasets

codecov · 2021-06-15T08:52:19Z

Codecov Report

Merging #15149 (45945ac) into master (b61c34f) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #15149      +/-   ##
==========================================
- Coverage   76.71%   76.50%   -0.22%     
==========================================
  Files         996      996              
  Lines       53080    53090      +10     
  Branches     6739     6739              
==========================================
- Hits        40721    40615     -106     
- Misses      12130    12246     +116     
  Partials      229      229

Flag	Coverage Δ
hive	`?`
mysql	`81.57% <100.00%> (+<0.01%)`	⬆️
postgres	`81.56% <100.00%> (-0.04%)`	⬇️
presto	`?`
python	`81.68% <100.00%> (-0.42%)`	⬇️
sqlite	`81.20% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/charts/commands/create.py	`92.15% <100.00%> (ø)`
superset/charts/commands/update.py	`88.40% <100.00%> (ø)`
superset/commands/base.py	`100.00% <100.00%> (ø)`
superset/commands/utils.py	`100.00% <100.00%> (ø)`
superset/dashboards/commands/create.py	`93.87% <100.00%> (ø)`
superset/dashboards/commands/update.py	`83.07% <100.00%> (ø)`
superset/datasets/commands/create.py	`100.00% <100.00%> (ø)`
superset/datasets/commands/update.py	`94.11% <100.00%> (-0.07%)`	⬇️
superset/reports/commands/create.py	`87.27% <100.00%> (ø)`
superset/reports/commands/update.py	`89.85% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b61c34f...45945ac. Read the comment docs.

willbarrett

This looks good! I particularly like the mixin approach

superset/commands/base.py

github-actions · 2021-08-13T07:49:24Z

@villebro Ephemeral environment spinning up at http://54.184.78.24:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2021-08-13T08:09:43Z

/testenv up

github-actions · 2021-08-13T08:14:36Z

@kgabryje Ephemeral environment spinning up at http://34.210.173.44:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions · 2021-08-13T09:43:09Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit d6f9c48)

…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py

superset-github-bot bot added the preset-io label Jun 14, 2021

pull-request-size bot added the size/L label Jun 14, 2021

villebro requested review from amitmiran137 and dpgaspar June 14, 2021 14:53

dpgaspar reviewed Jun 14, 2021

View reviewed changes

villebro force-pushed the villebro/command-mixin branch from 7a463f5 to 7ffe6bf Compare June 15, 2021 08:37

willbarrett approved these changes Jun 16, 2021

View reviewed changes

superset/commands/base.py Show resolved Hide resolved

villebro added 3 commits August 13, 2021 10:37

feat(dao): admin can remove self from object owners

de81b97

fix lint and add tests

4b6ef2f

clean up tests

45945ac

villebro force-pushed the villebro/command-mixin branch from 7ffe6bf to 45945ac Compare August 13, 2021 07:37

villebro merged commit d6f9c48 into apache:master Aug 13, 2021

rusackas deleted the villebro/command-mixin branch August 13, 2021 15:20

michael-s-molina mentioned this pull request Aug 13, 2021

[dashboard] edit dashboard will automatically become owner and not able to self remove ownership in Dashboard properties #14157

Closed

stevenuray pushed a commit to preset-io/superset that referenced this pull request Aug 13, 2021

feat(dao): admin can remove self from object owners (apache#15149)

f0fb37f

(cherry picked from commit d6f9c48)

villebro mentioned this pull request Aug 17, 2021

fix(explore): let admin overwrite slice #16290

Merged

9 tasks

villebro mentioned this pull request Aug 26, 2021

fix(datasets): add support for removing owners #16461

Merged

9 tasks

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

feat(dao): admin can remove self from object owners (apache#15149)

503e6ac

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

feat(dao): admin can remove self from object owners (apache#15149)

37b2b69

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dao): admin can remove self from object owners #15149

feat(dao): admin can remove self from object owners #15149

villebro commented Jun 14, 2021 •

edited

dpgaspar left a comment

dpgaspar Jun 14, 2021

dpgaspar Jun 14, 2021

codecov bot commented Jun 15, 2021 •

edited

willbarrett left a comment

github-actions bot commented Aug 13, 2021

kgabryje commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

feat(dao): admin can remove self from object owners #15149

feat(dao): admin can remove self from object owners #15149

Conversation

villebro commented Jun 14, 2021 • edited

SUMMARY

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

dpgaspar left a comment

Choose a reason for hiding this comment

dpgaspar Jun 14, 2021

Choose a reason for hiding this comment

dpgaspar Jun 14, 2021

Choose a reason for hiding this comment

codecov bot commented Jun 15, 2021 • edited

Codecov Report

willbarrett left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 13, 2021

kgabryje commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

villebro commented Jun 14, 2021 •

edited

codecov bot commented Jun 15, 2021 •

edited