feat(dashboard_rbac): provide data access based on dashboard access #13992

amitmiran137 · 2021-04-07T16:05:17Z

SUMMARY

This is the second milestone in dashboard_rbac support where having a dashboard role entitles the user data access to all of the datasets within a dashboard

following @suddjian recommendation on an existing PR, I decided to take a simpler solution

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

dashboard_data_access.mov

TEST PLAN

enable DASHBOARD_RBAC ff
create a separated user with Gamma role
on dashboard properties give Gamma role access to the dashboard
sign in as the gamma user
check that he can access the dashboard and all charts all loaded inside

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

etr2460 · 2021-04-07T16:23:33Z

Is this the default behavior we want to support? I wonder if there's value in creating alternate SecurityManagerPlugins for different use cases, and then if you wanted dashboard_rbac for your deployment, you'd use the specific SecurityManager for that use case.

Mainly asking as I haven't been following this work too closely, and this looks like a fairly specific change (for example it doesn't apply to Druid datasources and assumes that a BaseDatasource is always a SqlaTable)

amitmiran137 · 2021-04-07T16:56:02Z

Is this the default behavior we want to support? I wonder if there's value in creating alternate SecurityManagerPlugins for different use cases, and then if you wanted dashboard_rbac for your deployment, you'd use the specific SecurityManager for that use case.

Mainly asking as I haven't been following this work too closely, and this looks like a fairly specific change (for example it doesn't apply to Druid datasources and assumes that a BaseDatasource is always a SqlaTable)

the entire feature is under the DASHBOARD_RBAC ff
and about the druid datasources, I'll change it to use the BaseDatasource

etr2460 · 2021-04-07T17:05:05Z

it's under the feature flag, but this check is added to the security manager without the feature flag right? should we be checking the flag here too?

amitmiran137 · 2021-04-07T17:16:01Z

it's under the feature flag, but this check is added to the security manager without the feature flag right? should we be checking the flag here too?

thank you for the input, I added the the FF restriction and the more generic support for table

suddjian

Nice, much cleaner implementation! Happy to approve once the new method has some tests.

superset/security/manager.py

codecov · 2021-04-07T21:49:25Z

Codecov Report

Merging #13992 (33d64d5) into master (b394733) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 33d64d5 differs from pull request most recent head 573ed3e. Consider uploading reports for the commit 573ed3e to get more accurate results

@@            Coverage Diff             @@
##           master   #13992      +/-   ##
==========================================
- Coverage   79.76%   79.64%   -0.12%     
==========================================
  Files         942      942              
  Lines       47675    47689      +14     
  Branches     5984     5984              
==========================================
- Hits        38029    37984      -45     
- Misses       9525     9584      +59     
  Partials      121      121

Flag	Coverage Δ
cypress	`56.31% <ø> (-0.01%)`	⬇️
hive	`?`
javascript	`69.67% <ø> (ø)`
mysql	`80.73% <100.00%> (+0.01%)`	⬆️
postgres	`80.76% <100.00%> (?)`
presto	`80.48% <100.00%> (+0.01%)`	⬆️
python	`81.06% <100.00%> (-0.22%)`	⬇️
sqlite	`80.36% <100.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...src/dashboard/components/PropertiesModal/index.jsx	`85.61% <ø> (ø)`
superset/views/dashboard/mixin.py	`95.00% <ø> (ø)`
superset/security/manager.py	`91.43% <100.00%> (+0.34%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`74.42% <0.00%> (-16.42%)`	⬇️
...dashboard/components/SliceHeaderControls/index.jsx	`77.89% <0.00%> (-1.06%)`	⬇️
superset/connectors/sqla/models.py	`89.69% <0.00%> (-0.25%)`	⬇️
superset/utils/core.py	`88.70% <0.00%> (-0.13%)`	⬇️
superset/views/base_api.py	`98.28% <0.00%> (+0.42%)`	⬆️
superset/db_engine_specs/postgres.py	`96.80% <0.00%> (+1.06%)`	⬆️
... and 1 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 b394733...573ed3e. Read the comment docs.

villebro

First pass comments

superset/security/manager.py

Co-authored-by: Ville Brofeldt <[email protected]>

superset/security/manager.py

tests/utils_tests.py

villebro

A few minor comments. Should we update this string to state that granting a role access to a dashboard will bypass datasource level checks?

superset/superset/views/dashboard/mixin.py

Lines 68 to 69 in 2c96c5b

    
           "These roles are always applied in addition to restrictions on dataset " 
        
           "level access. "

Or should we rather add a new config flag in which we can specify that dashboard RBAC either overrides or is supplemental to dataset access control?

superset/security/manager.py

amitmiran137 · 2021-04-09T14:51:07Z

IMO we should just change the string the explains instead of an additional feature flag.

That way both use cases could live on different dashboards
One dashboard can use roles while the other can use dataset permissions

suddjian

Thanks for all the adjustments, looks good!

* master: (53 commits) test: Adds tests to the UndoRedoKeyListeners component (#13919) chore: Adds dataMask reducer to reducerIndex (#13951) test: Tests audit for the Dashboard FilterBar (#13916) fix: unable to apply logging format (#14074) refactor: Bootstrap to AntD - Slider (#13989) chore(spa refactor): refactoring dashboard to use api's instead of bootstrapdata (#13306) fix(listview): update listview feature flag (#13906) Add docs for configuring Docker Compose setup (#13961) feat: invalid password error message (Postgres) (#14038) fix: flacky test in test_update_dataset_item_w_override_columns (#14082) feat: Implement Celery SoftTimeLimit handling (#13740) feat: only send alert error emails to owners of the alert (#13862) feat: add descriptions to report emails (#13827) Make chart exclude itself from cross filtering (#14046) fix: fix bug when remove chart not removing it's related cross filter data (#14081) feat(native-filters): Add default first value to select filter (#13726) feat: Make async query JWT cookie domain configurable (#14007) fix: add exception to catch session not having JWT (#14036) Use consistent chart value (#14031) fix: Use superset generic db to catch external_metadata queries (#13974) ...

tejpochiraju · 2021-05-08T05:36:09Z

Just here to say thanks! I was just starting to implement a rather complex role + user API integration to automate access from our IoT platform. This solves half the problem 👍🏾

…pache#13992) * feat: provide data access based onb dashboard access * chore: adjust code after CR comments * fix: add brackets * fix: type * chore: add tests * fix: pre-commit * fix: pre-commit and lint * fix: fix test * fix: pre-commit * fix: fix local pylint warnings * revert: birth_names pylint change bc it affects tests * Update superset/security/manager.py Co-authored-by: Ville Brofeldt <[email protected]> * Update superset/security/manager.py * Update tests/utils_tests.py * fix: after CR * fix: after CR from ville * chore: update roles description Co-authored-by: Ville Brofeldt <[email protected]>

MM-Lehmann · 2022-03-04T14:56:42Z

The fallback (to dataset perms) doesn't seem to work on v1.4.1
When the FF is active, no access is granted to any dashboard unless roles are set in dashboards. It doesn't look like the intended behaviour...

nikhil-kuyya-talentas · 2022-03-11T12:15:54Z

Thank you for pull request on role based controls. I am trying to extend to our(my company) requirement to chart level.
Can you help me if possible, how these can_access_based_on_dashboard method query is working little bit.
[Update]: got the understanding,
[MySolution]: I have created the custom can_access_based_on_chart and passed extra slice_id parameter.

nikhil-kuyya-talentas · 2022-03-15T12:18:41Z

should we check the can_access_based_on_dashboard if there is not slice_id in query_context and viz.
given that i don't have access to data menu items.

These case when i come to explore view by typing the url. <host_name>/superset/explore/table/1/
Select the explore options for the data and query. now in these it works and fetch the data for me.

feat: provide data access based onb dashboard access

0cd982f

pull-request-size bot added the size/S label Apr 7, 2021

amitmiran137 mentioned this pull request Apr 7, 2021

feat(dashboard_rbac): dashboard extra jwt #13773

Closed

6 tasks

amitmiran137 requested a review from suddjian April 7, 2021 16:54

chore: adjust code after CR comments

1746490

pull-request-size bot added size/M and removed size/S labels Apr 7, 2021

fix: add brackets

46d7c4d

amitmiran137 requested a review from villebro April 7, 2021 17:30

suddjian requested changes Apr 7, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

etr2460 reviewed Apr 7, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

etr2460 reviewed Apr 7, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

fix: type

39788a8

pull-request-size bot added size/S and removed size/M labels Apr 7, 2021

chore: add tests

f1f5a44

pull-request-size bot added size/M and removed size/S labels Apr 7, 2021

amitmiran137 requested a review from suddjian April 7, 2021 20:50

fix: pre-commit

e27347a

amitmiran137 closed this Apr 8, 2021

amitmiran137 reopened this Apr 8, 2021

amitmiran137 added 2 commits April 8, 2021 11:28

fix: pre-commit and lint

dfa3e76

fix: fix test

3b88b9b

villebro reviewed Apr 8, 2021

View reviewed changes

superset/security/manager.py Show resolved Hide resolved

superset/security/manager.py Outdated Show resolved Hide resolved

superset/security/manager.py Outdated Show resolved Hide resolved

amitmiran137 added 2 commits April 8, 2021 15:18

fix: pre-commit

2409504

fix: fix local pylint warnings

3982948

junlincc added dashboard:security:access Related to the security access of the Dashboard release:note labels Apr 8, 2021

amitmiran137 and others added 2 commits April 9, 2021 08:49

revert: birth_names pylint change bc it affects tests

e22d4f6

Update superset/security/manager.py

3ff2d96

Co-authored-by: Ville Brofeldt <[email protected]>

amitmiran137 commented Apr 9, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

Update superset/security/manager.py

3cd3c68

amitmiran137 commented Apr 9, 2021

View reviewed changes

tests/utils_tests.py Outdated Show resolved Hide resolved

amitmiran137 and others added 2 commits April 9, 2021 08:53

Update tests/utils_tests.py

06398cb

fix: after CR

6c65241

villebro reviewed Apr 9, 2021

View reviewed changes

superset/security/manager.py Outdated Show resolved Hide resolved

superset/security/manager.py Outdated Show resolved Hide resolved

fix: after CR from ville

b763172

suddjian approved these changes Apr 12, 2021

View reviewed changes

amitmiran137 added 2 commits April 13, 2021 10:22

chore: update roles description

573ed3e

amitmiran137 merged commit 8c5b6b1 into master Apr 13, 2021

nytai mentioned this pull request Jul 12, 2021

Implicit Datasource Access for Dashboard RBAC #15632

Closed

rusackas deleted the feat/dashboard_dataset_access branch January 30, 2023 19:21

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard_rbac): provide data access based on dashboard access #13992

feat(dashboard_rbac): provide data access based on dashboard access #13992

amitmiran137 commented Apr 7, 2021

etr2460 commented Apr 7, 2021

amitmiran137 commented Apr 7, 2021

etr2460 commented Apr 7, 2021

amitmiran137 commented Apr 7, 2021

suddjian left a comment

codecov bot commented Apr 7, 2021 •

edited

Loading

villebro left a comment

villebro left a comment

amitmiran137 commented Apr 9, 2021 •

edited

Loading

suddjian left a comment

tejpochiraju commented May 8, 2021

MM-Lehmann commented Mar 4, 2022

nikhil-kuyya-talentas commented Mar 11, 2022 •

edited

Loading

nikhil-kuyya-talentas commented Mar 15, 2022

	"These roles are always applied in addition to restrictions on dataset "
	"level access. "

feat(dashboard_rbac): provide data access based on dashboard access #13992

feat(dashboard_rbac): provide data access based on dashboard access #13992

Conversation

amitmiran137 commented Apr 7, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

etr2460 commented Apr 7, 2021

amitmiran137 commented Apr 7, 2021

etr2460 commented Apr 7, 2021

amitmiran137 commented Apr 7, 2021

suddjian left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

amitmiran137 commented Apr 9, 2021 • edited Loading

suddjian left a comment

Choose a reason for hiding this comment

tejpochiraju commented May 8, 2021

MM-Lehmann commented Mar 4, 2022

nikhil-kuyya-talentas commented Mar 11, 2022 • edited Loading

nikhil-kuyya-talentas commented Mar 15, 2022

codecov bot commented Apr 7, 2021 •

edited

Loading

amitmiran137 commented Apr 9, 2021 •

edited

Loading

nikhil-kuyya-talentas commented Mar 11, 2022 •

edited

Loading