Implement permission request/approve flow. #1095

bkyryliuk · 2016-09-12T16:30:03Z

This PR implements the basic request permissions flow: #1049
For the 1st iteration it is pretty simple flow:

permission requests exist on the user - datasource (table or druid) level
user is redirected to the request form if he/she doesn't have permissions
admins, alphas or datasource owners have permissions to approve the requests. only admins can grant roles to the users
the access request view is visible to admins only
there are 2 ways to grant the permission to the user, grant the user role that has access to the datasource, extend the role the user already has to provide access to the datasource
disallows multiple requests to the same datasource
extensive testing, it was quite tricky to make it working

Potential future improvements:

request access to the databases, not only to the datasources
include free form message in the request form
offer to create a role if there are 0 roles granting access to the given datasource
email notifications
batch approval
automatic clean up of the request if approving one of the requests resolves all others
store some metadata - dashboard / slide ids
better support for the dashboards - show the slices you have access to and allow to request access to others

Reviewers:

Screenshots

mistercrunch

Looks neat!

mistercrunch · 2016-09-16T16:49:27Z

caravel/templates/caravel/request_access.html

no need for id="app" afaict

mistercrunch · 2016-09-16T16:50:01Z

caravel/templates/caravel/request_access.html

wrap the whole thing in a <div class="container"> to get a nice left and right margin

mistercrunch · 2016-09-16T17:11:02Z

caravel/models.py

oh this should now be in master now

mistercrunch · 2016-09-16T23:28:32Z

caravel/models.py

Rename to datasource_link
I'd except a link to be a <a> and a url to be http://...

mistercrunch · 2016-09-16T23:31:40Z

caravel/views.py

nit: would fit on one line

mistercrunch · 2016-09-16T23:33:31Z

caravel/views.py

NIT: it works here, but .all() could be aligned with .filter(

mistercrunch · 2016-09-16T23:34:10Z

caravel/views.py

Empty lists evals as False, so you can simply if duplicates:

mistercrunch · 2016-09-16T23:37:51Z

caravel/views.py

this endpoint and model related perms should be added to the admin role only (check the cil's init function), then we don't need anything else than @has_access.

Done and covered in the unit tests

mistercrunch · 2016-09-16T23:40:47Z

caravel/views.py

Not needed but there could be a need to cleanup here. If I asked access to 12 tables and you grant a single role that gives access to all 8 tables, you could clean up the other 7 requests, but let's leave this out for v1.

Yep, will do it in separate PR

bkyryliuk · 2016-09-20T18:50:51Z

@mistercrunch - please take another look

mistercrunch

Sorry for the "close to the finish line" changes in requirements, tell me what you think.

mistercrunch · 2016-09-21T03:44:50Z

caravel/views.py

I should have caught this before, but it's critical that we keep an audit trail of who granted access to what/who/when. One way to do this is to not delete the request, and flag it as resolved instead (add a state field to the model, a fk to the userid who granted access, timestamp when it was granted, and maybe the resolution that was applied).

Another option is to store the operation in another model.

@log_this - tackles it now

mistercrunch · 2016-09-21T03:46:14Z

tests/base_tests.py

nit: one too many black line

mistercrunch · 2016-09-21T03:48:45Z

caravel/utils.py

We probably need a new role for people who can grant rights to others that are not admin. It doesn't have to be part of this PR, your call.

agree, separate PR

mistercrunch · 2016-09-21T03:51:31Z

caravel/models.py

we probably want to follow the new pattern of datasource_id and datasource_type that was introduced in the Yahoo PR

mistercrunch · 2016-09-21T03:54:56Z

caravel/models.py

related to my other comment about auditing who granted what to who, I'd add:

state as ('requested', 'granted' and maybe eventually 'denied')
processed_by_userid Integer
processed_on Datetime

I've refactored the code in a way that @log_this will have all needed information

bkyryliuk · 2016-09-21T23:02:09Z

@mistercrunch - please take one more look on the PR, couple new features

cleans up all the user requests, not just 1 once the access to the datasource is granted1.
logs the requests and approves
follows new rules of the datasource access

mistercrunch · 2016-09-22T15:22:18Z

caravel/views.py

+            flash(USER_MISSING_ERR, "alert")
+            return json_error_response(USER_MISSING_ERR)
+
+        requests = (session.query(models.DatasourceAccessRequest).filter(


optional: indent is inconsistent with other queries, looks like maybe sublime auto-indented that.

Sometimes when I have long strings that get referenced a lot in a small section of code, I create a shortcut as in DAR = models.DatasourceAccessRequest.

Done, thanks

mistercrunch · 2016-09-22T15:33:04Z

LGTM. This is an awesome feature, great work!

It's lowering friction around obtaining access, granting access, and addressing the lack of auditing around access, all this is super impactful.

Follow up, related items:

maintain a system role for people who can grant rights
document the flows in the security section of the docs
a quick way to see the columns in the table (tooltip? link to endpoint that list the columns for a table?)
it's not clear yet, but something around managing how granters could only grant permissions to a subset of dataset (this may be over-thinking it at this point)

aleksarias · 2020-10-23T00:26:16Z

Hello, I don't see any information on how to activate this feature. I don't see it in the latest release of superset, 0.37.2. Does anyone know if it can be activated in that version?

aleksarias · 2020-10-23T00:33:51Z

Found the setting the full example config file: https://github.com/apache/incubator-superset/blob/master/superset/config.py

# Set this to false if you don't want users to be able to request/grant
# datasource access requests from/to other users.
ENABLE_ACCESS_REQUEST = False

aleksarias · 2020-10-23T15:39:54Z

After enabling this feature, I'm getting "NOT FOUND".
https://mydomain.com/superset/dashboard/46/superset/request_access/?dashboard_id=46&

Is there something else that needs to be activated in order to get this feature to work?

bkyryliuk changed the title ~~Work in progress. Implement permission request/approve flow.~~ Implement permission request/approve flow. Sep 15, 2016

mistercrunch reviewed Sep 16, 2016

View reviewed changes

mistercrunch requested changes Sep 21, 2016

View reviewed changes

Bogdan Kyryliuk added 2 commits September 21, 2016 10:22

Implement permission request/approve flow

40d590f

Address the comments.

9122d05

Refactor the code to support multiple datasources.

61bb048

mistercrunch reviewed Sep 22, 2016

View reviewed changes

Bogdan Kyryliuk and others added 2 commits September 22, 2016 09:29

Reformat the queries.

9550555

Merge branch 'master' into request_access

2fe3877

bkyryliuk merged commit cbc70d3 into apache:master Sep 22, 2016

dpgaspar mentioned this pull request Sep 5, 2019

Bump FAB to 2.1.13 #8176

Merged

12 tasks

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021

fix(plugin-chart-table): check if control values are set (apache#1095)

a927143

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021

fix(plugin-chart-table): check if control values are set (apache#1095)

a7ec000

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021

fix(plugin-chart-table): check if control values are set (apache#1095)

5df9a17

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021

fix(plugin-chart-table): check if control values are set (apache#1095)

33198ab

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 19, 2024

mistercrunch added the 🚢 0.11.0 First shipped in 0.11.0 label Feb 19, 2024

Implement permission request/approve flow. #1095

Implement permission request/approve flow. #1095

Uh oh!

Conversation

bkyryliuk commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mistercrunch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkyryliuk commented Sep 20, 2016

Uh oh!

mistercrunch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkyryliuk commented Sep 21, 2016

Uh oh!

mistercrunch Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mistercrunch commented Sep 22, 2016

Uh oh!

aleksarias commented Oct 23, 2020

Uh oh!

aleksarias commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aleksarias commented Oct 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bkyryliuk commented Sep 12, 2016 •

edited

Loading

mistercrunch Sep 22, 2016 •

edited

Loading

aleksarias commented Oct 23, 2020 •

edited

Loading