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

+             name=datasource_name)
+        }}
+    </h4>
+    <div id="app">


no need for id="app" afaict

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

caravel/templates/caravel/request_access.html

+{% block title %}{{ _("No Access!") }}{% endblock %}
+{% block body %}
+    {% include "caravel/flash_wrapper.html" %}
+    <h4>


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

@@ -1181,6 +1186,10 @@ def refresh_datasources(self):
            if datasource not in config.get('DRUID_DATA_SOURCE_BLACKLIST'):
                DruidDatasource.sync_to_db(datasource, self)

+    @property


oh this should now be in master now

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

caravel/models.py

+        return self.table
+
+    @property
+    def datasource_url(self):


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

+    list_columns = [
+        'username', 'user_roles', 'datasource_url',
+        'roles_with_datasource', 'created_on']
+    order_columns = [


nit: would fit on one line

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

caravel/views.py

+                models.DatasourceAccessRequest.druid_datasource_id ==
+                druid_datasource_id,
+            )
+        ).all()


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

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

caravel/views.py

+            )
+        ).all()
+
+        if len(duplicates) > 0:


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

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

caravel/views.py

+            .filter_by(id=request_access_id).first()
+        )
+        # check if you can approve
+        if (self.all_datasource_access() or


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

+                  "danger")
+            return redirect('/accessrequestsmodelview/list/')
+
+        db.session.delete(access_request)


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

+                  "danger")
+            return redirect('/accessrequestsmodelview/list/')
+
+        db.session.delete(access_request)


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

+                ).all()
+            )
+
+


nit: one too many black line

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

caravel/utils.py

+        'Security',
+        'UserDBModelView',
+        'SQL Lab <span class="label label-danger">alpha</span>',
+        'AccessRequestsModelView',


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

+    id = Column(Integer, primary_key=True)
+
+    table_id = Column(Integer, ForeignKey('tables.id'))
+    druid_datasource_id = Column(Integer, ForeignKey('datasources.id'))


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

+    """ORM model for the access requests for datasources and dbs."""
+    __tablename__ = 'access_request'
+    id = Column(Integer, primary_key=True)
+


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 label Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement permission request/approve flow. #1095

Implement permission request/approve flow. #1095

bkyryliuk commented Sep 12, 2016 •

edited

mistercrunch left a comment

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

mistercrunch Sep 16, 2016

bkyryliuk Sep 20, 2016

mistercrunch Sep 16, 2016

bkyryliuk Sep 20, 2016

bkyryliuk commented Sep 20, 2016

mistercrunch left a comment

mistercrunch Sep 21, 2016

bkyryliuk Sep 21, 2016

mistercrunch Sep 21, 2016

mistercrunch Sep 21, 2016

bkyryliuk Sep 21, 2016

mistercrunch Sep 21, 2016

bkyryliuk Sep 21, 2016

mistercrunch Sep 21, 2016

bkyryliuk Sep 21, 2016

bkyryliuk commented Sep 21, 2016

mistercrunch Sep 22, 2016 •

edited

bkyryliuk Sep 22, 2016

mistercrunch commented Sep 22, 2016

aleksarias commented Oct 23, 2020

aleksarias commented Oct 23, 2020 •

edited

aleksarias commented Oct 23, 2020

Implement permission request/approve flow. #1095

Implement permission request/approve flow. #1095

Conversation

bkyryliuk commented Sep 12, 2016 • edited

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Sep 20, 2016

mistercrunch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk commented Sep 21, 2016

mistercrunch Sep 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Sep 22, 2016

aleksarias commented Oct 23, 2020

aleksarias commented Oct 23, 2020 • edited

aleksarias commented Oct 23, 2020

bkyryliuk commented Sep 12, 2016 •

edited

mistercrunch Sep 22, 2016 •

edited

aleksarias commented Oct 23, 2020 •

edited