feat: Dashboard Level Access : dashboard lists #10594

Ofeknielsen · 2020-08-13T11:49:25Z

Summary

As the first phase of SIP-51 "Proposal for Dashboard Level Access", added dashboard permission enforcement for getting dashboards list (via superset API or view).
The feature is activated by turn the flag "DASHBOARD_LEVEL_ACCESS" on.
When request to get dashboard list is handled, the "dashboard filter" filter the dashboards by the user dashboard access permissions, indeed answer the question does the user have "all dashboards permissions" or permissions for specific dashboards.
As default behavior, Gamma users have all dashboards access permission
On create/edit/copy dashboards - dashboards view menu and permissionView are created/updated.
On delete, the dashboards view menu and permissionView are deleted
On superset init phase (while the flag is on) - permissions created for all the exists dashboards.

TEST PLAN

Added new github action: Feature Flag - Dashboard Level Access
Using "pytest skip-if" on tests which tests previous logic (when the flag is off)

ADDITIONAL INFORMATION

With the feature, we have tried to increase the quality of the code by meeting the standard programming principles:

Don't Repeat Yourself principle

Use constants instead of reusing "hard coded"
Delete filters from superset.view.dashboard
DashboardSecurityManager instead writing duplicate code in security-manager ("can access all")
Dashboard utils inside tests.dashboard

Separation Of Concerns and Single Responsibility principles

Added dashbaord_level_access_initializer.py - instead writing the logic inside the cli file.
DashboardSecurityMixin instead writing the code in dashboard model or security_manager
DashboardSecurityManager instead writing the code in security_manager
Refactor set_perm by extracting code to set_permissions_views
Writing update_view_menu and find_view_menu_by_pattern as general code in security_manager instead in the callee files.

Tests - dashboard test refactor

Decouple tests from specific data -
Decouple tests to other tests

Python best practises

use of "__" in method/variables (__set_permission_by_connection in security_manager)

resolve #10408

request-info · 2020-08-13T11:49:28Z

We would appreciate it if you could provide us with more info about this issue/pr! Please do not leave the title or description empty.

amitmiran137 · 2020-08-13T12:02:19Z

#10408

codecov-commenter · 2020-08-13T12:21:37Z

Codecov Report

Merging #10594 into master will increase coverage by 0.04%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master   #10594      +/-   ##
==========================================
+ Coverage   61.64%   61.69%   +0.04%     
==========================================
  Files         815      816       +1     
  Lines       38498    38646     +148     
  Branches     3620     3620              
==========================================
+ Hits        23733    23843     +110     
- Misses      14579    14617      +38     
  Partials      186      186

Flag	Coverage Δ
#javascript	`62.05% <ø> (ø)`
#python	`61.48% <77.77%> (+0.07%)`	⬆️

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

Impacted Files	Coverage Δ
superset/config.py	`90.11% <ø> (ø)`
superset/dashbaord_level_access_initializer.py	`0.00% <0.00%> (ø)`
superset/cli.py	`39.67% <20.00%> (-0.33%)`	⬇️
superset/dashboards/security.py	`98.21% <98.21%> (ø)`
superset/constants.py	`100.00% <100.00%> (ø)`
superset/dashboards/commands/bulk_delete.py	`91.66% <100.00%> (+0.75%)`	⬆️
superset/dashboards/commands/create.py	`92.85% <100.00%> (+0.54%)`	⬆️
superset/dashboards/commands/delete.py	`91.66% <100.00%> (+0.75%)`	⬆️
superset/dashboards/commands/update.py	`89.28% <100.00%> (+0.60%)`	⬆️
superset/dashboards/filters.py	`97.91% <100.00%> (+1.36%)`	⬆️
... and 11 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 7f1e4e4...e09a206. Read the comment docs.

feat delete dashboard permission on delete dashboard

# Conflicts: # superset/models/dashboard.py

test are failing bc of the after insert that prevents fetching the dashboard with FK relationships

Ofeknielsen · 2020-08-25T09:24:09Z

superset/models/dashboard.py

@@ -496,3 +548,13 @@ def event_after_dashboard_changed(  # pylint: disable=unused-argument
 if is_feature_enabled("THUMBNAILS_SQLA_LISTENERS"):
    sqla.event.listen(Dashboard, "after_insert", event_after_dashboard_changed)
    sqla.event.listen(Dashboard, "after_update", event_after_dashboard_changed)
+
+
+sqla.event.listen(Dashboard, "after_insert", SecuredMixin.after_insert)


some more details
We implemented it by subscribing to 'after_insert' event and insert the new objects by using the passed connection

aspedrosa · 2020-08-25T10:31:42Z

Resolve #10413, resolve #10564

feat delete dashboard permission on delete dashboard

…g and end of relevant tests

…beginning and end of relevant tests" This reverts commit bf1b80c

…l_access/dashboard_by_id_endpoints * upstream/master: (29 commits) fix(presto): default unknown types to string type (apache#10753) feat(row-level-security): add base filter type and filter grouping (apache#10946) docs: add gallery screenshot & link in README (apache#10988) docs: add a "Gallery" page (apache#10968) build: add PR lint action (apache#10990) adding filters back that caused issues (apache#10989) chore: selectors refactor in SQLLab test suite (Cypress) (apache#10944) ESLint: Remove ts-ignore comments (apache#10933) style: fix checkbox color (apache#10970) fix: changed disabled rules in datasets module (apache#10979) fix: update the time filter for 'Last Year' option in explore (apache#10829) fix: use nullpool even for user lookup in the celery (apache#10938) Allow empty observations in alerting (apache#10939) fix: re-enabling several globally disabled lint rules (apache#10957) fix: setting specific exceptions common/query_context.py (apache#10942) Pylint disabled rule `pointless-string-statement` is not raising warining anymore - removing (apache#10975) fix: pylint disabled rules in dashboard/api.py (apache#10976) fix: removed disabled lint rule `too-many-locals` in connectors/base/models.py (apache#10958) ESLint: Re-enable rule no-access-state-in-setstate (apache#10870) fix: simply is_adhoc_metric (apache#10964) ...

…d of this PR

…boards_permissions * upstream/master: (46 commits) fix: surface connection error messages on the client (apache#11077) fix(jest): using UTC mock date (apache#11079) removing unused component (apache#11072) changing to the correct hex color (apache#11073) style: remove unecessary padding (apache#11071) fix: database list checkboxes (apache#11068) feat: adding all icons from the design system to the codebase (apache#11033) fix: sql lab autocomplete width (apache#11063) clickable labels have outlines, storybook shows them (apache#11034) fixed routes for customer in docs (apache#11052) Revert "style: fix checkbox color (apache#10970)" (apache#11051) feat: add "created by" to dashboard CRUD view (apache#11030) Changed `tags.py` and `helpers.py` in `models` module: removed disabled pylint rule `unused_import`, changed unused arguments to private and removed disabled rule `unused-argument. Removed redundant rules.` (apache#11037) chore: updated lint rules in models module (apache#11036) Removed disable global pytlint rule `standarderror-builtin` which isn't appearing for Python3 (apache#11038) Enabled argument-differ for bulk_delete (apache#11039) Enabled no-self-use pylint rule in security. Formatter (apache#11041) Changed variable name from capitals to lowercase and changed lint rule (apache#11044) Revert "ESLint: Re-enable rule default-props-match-prop-types (apache#10868)" (apache#11050) feat(saved_queries): add custom api filter for all string & text fields (apache#11031) ... # Conflicts: # superset/config.py # tests/dashboards/api_tests.py

…missions' into feat/add_all_dashboards_permissions * ofekNIelsen/feat/add_all_dashboards_permissions: # Conflicts: # tests/dashboards/api_tests.py # tests/dashboards/dashboard_test_utils.py

amitmiran137 · 2020-10-29T08:53:03Z

superset/config.py

@@ -308,6 +308,7 @@ def _try_json_readsha(  # pylint: disable=unused-argument
    "TAGGING_SYSTEM": False,
    "SQLLAB_BACKEND_PERSISTENCE": False,
    "LISTVIEWS_DEFAULT_CARD_VIEW": False,
+    "DASHBOARD_LEVEL_ACCESS": False,
 }



move DASHBOARD_LEVEL_MODE=[""] as a config

Add all dashboards permissions views

bd220fa

request-info bot added the need:more-info Requires more information from author label Aug 13, 2020

pull-request-size bot added the size/M label Aug 13, 2020

Fix format

9ee4320

Ofeknielsen closed this Aug 13, 2020

Ofeknielsen reopened this Aug 13, 2020

amitmiran137 mentioned this pull request Aug 13, 2020

[SIP-51] Dashboard Level Access #10408

Closed

Fix format

a3059cf

Add dashboard permission view on new dashboard

3558487

pull-request-size bot added size/L and removed size/M labels Aug 16, 2020

ofekisrael and others added 6 commits August 19, 2020 20:01

feat update dashboard permissions on update dashboard title

ab84aba

feat delete dashboard permission on delete dashboard

Merge branch 'master' into feat/add_all_dashboards_permissions

c79ff7b

# Conflicts: # superset/models/dashboard.py

Fix tests after merge from upstream/master

55ad339

fix: comment after_insert

fa3d6aa

test are failing bc of the after insert that prevents fetching the dashboard with FK relationships

fix: add apache license

a887b38

fix: readd after_insert

376beda

amitmiran137 reviewed Aug 24, 2020

View reviewed changes

fix: pylint

94866d4

ofekisrael added 7 commits August 26, 2020 13:35

Add all dashboards permissions views

88fbc6f

Fix format

0954592

Fix format

29f51f7

Add dashboard permission view on new dashboard

3851de1

feat update dashboard permissions on update dashboard title

e2ad850

feat delete dashboard permission on delete dashboard

Fix tests after merge from upstream/master

e604c0b

Add all dashboard access permission enforcement

6aa96ed

amitNielsen added 18 commits September 18, 2020 06:39

fix: undo draft

a933f61

fix: pre-commit

a02c699

fix: pylint

50d685e

fix: pylint additions

1ea8344

fix: pre-commit apache#532

eb6e7e2

fix: fixesss linting and precommit

c2ec36c

fix: fix typing

45f927f

fix: bed merge of feature_flags

7351254

chore: remove unused import

ea85926

chore: remove experiment tox env

31ab992

chore: remove unused test config

21b9aed

refactor: remove skipIf and set feature flag in the tests in beginnin…

bf1b80c

…g and end of relevant tests

Revert "refactor: remove skipIf and set feature flag in the tests in …

dabf9c0

…beginning and end of relevant tests" This reverts commit bf1b80c

test: add github action for the feature

c1fa652

fix: name the github action

9410afa

fix: make action work the right SUPERSET_CONFIG module path

51344c2

fix: bad yaml syntax

298fd9a

fix: missed changes

8b40662

Ofeknielsen changed the title ~~[WIP] - Dashboard access permissions~~ Dashboard access permissions Sep 21, 2020

Ofeknielsen changed the title ~~Dashboard access permissions~~ Feat: Dashboard Level Access : dashboard lists Sep 21, 2020

amitmiran137 approved these changes Sep 22, 2020

View reviewed changes

Ofeknielsen changed the title ~~Feat: Dashboard Level Access : dashboard lists~~ feat: Dashboard Level Access : dashboard lists Sep 22, 2020

amitNielsen added 5 commits September 23, 2020 22:43

refactor: remove all unrelated change to manager.py to remove overhea…

21a3ed2

…d of this PR

chore: undo unrelevant change, moved to a separated PR

0190c02

Merge remote-tracking branch 'ofekNIelsen/feat/add_all_dashboards_per…

e09a206

…missions' into feat/add_all_dashboards_permissions * ofekNIelsen/feat/add_all_dashboards_permissions: # Conflicts: # tests/dashboards/api_tests.py # tests/dashboards/dashboard_test_utils.py

amitmiran137 reviewed Oct 29, 2020

View reviewed changes

Ofeknielsen closed this Jan 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Dashboard Level Access : dashboard lists #10594

feat: Dashboard Level Access : dashboard lists #10594

Ofeknielsen commented Aug 13, 2020 •

edited

Loading

request-info bot commented Aug 13, 2020

amitmiran137 commented Aug 13, 2020

codecov-commenter commented Aug 13, 2020 •

edited

Loading

This comment was marked as resolved.

Ofeknielsen Aug 25, 2020 •

edited

Loading

aspedrosa commented Aug 25, 2020 •

edited

Loading

amitmiran137 Oct 29, 2020

feat: Dashboard Level Access : dashboard lists #10594

feat: Dashboard Level Access : dashboard lists #10594

Conversation

Ofeknielsen commented Aug 13, 2020 • edited Loading

Summary

TEST PLAN

ADDITIONAL INFORMATION

Don't Repeat Yourself principle

Separation Of Concerns and Single Responsibility principles

Tests - dashboard test refactor

Python best practises

request-info bot commented Aug 13, 2020

amitmiran137 commented Aug 13, 2020

codecov-commenter commented Aug 13, 2020 • edited Loading

Codecov Report

This comment was marked as resolved.

Ofeknielsen Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

aspedrosa commented Aug 25, 2020 • edited Loading

amitmiran137 Oct 29, 2020

Choose a reason for hiding this comment

Ofeknielsen commented Aug 13, 2020 •

edited

Loading

codecov-commenter commented Aug 13, 2020 •

edited

Loading

Ofeknielsen Aug 25, 2020 •

edited

Loading

aspedrosa commented Aug 25, 2020 •

edited

Loading