Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: share modify/access permissions for queries and dashboard #1113

Merged
merged 29 commits into from Oct 28, 2016

Conversation

@whummer
Copy link
Contributor

whummer commented Jun 13, 2016

Hi @arikfr,

This PR allows users to share edit permissions for a given query or dashboard. This feature request has previously been discussed here.

A new button "Share Edit Permissions" has been added to the "..." details popup when viewing the source of a query (or dashboard).
image

To manage the access permissions, a modal dialog is used (similar to the dialog used for managing group members). To give an existing user "modify" access, the user can simply be selected from the search/dropdown list. To revoke access, use the "Remove" button and changes will be effected immediately. Note that the same modal code (view and controller) is used for both, query permissions and dashboard permissions, to avoid code duplication.
image

At the core of the approach is a new table and model entity Change, which keeps track of the historical changes made to model entities (i.e., Query, and Dashboard). Whenever a change is made, a new record is added to the changes table. The UI is aware of the current "version" of the query/dashboard it is working with, and the approach implements "optimistic locking" to avoid concurrent changes to override each other. If a conflict is detected, a warning message is displayed to the user (see below). This can be easily tested by using two browser sessions with two different users, attempting to modify the same query.
image

The access permissions are handled via a new API endpoint /api/access/, which allows to 1) list, 2) grant, and 3) revoke access permissions, as well as to 4) attempt an access and receive an access "granted" or "denied" decision.

A set of tests has been added to cover the basic functionality of managing access permissions.

@whummer whummer force-pushed the whummer:feat/share-access-permissions branch Jun 13, 2016
@rohanpd rohanpd mentioned this pull request Jun 14, 2016
@whummer whummer force-pushed the whummer:feat/share-access-permissions branch Jun 14, 2016
@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Jun 20, 2016

Thanks! That's a lot of changes... I am hoping to start going through it this week, but it will take some time.

@arikfr arikfr added this to the v0.12.0 milestone Jun 20, 2016
@whummer whummer force-pushed the whummer:feat/share-access-permissions branch Aug 12, 2016
@whummer

This comment has been minimized.

Copy link
Contributor Author

whummer commented Aug 12, 2016

Hi @arikfr, we rebased this branch to current master. Any chance we can get this released for the next milestone? Cheers

@whummer whummer force-pushed the whummer:feat/share-access-permissions branch Aug 29, 2016
@arikfr arikfr changed the title Share modify/access permissions for queries and dashboard Add: share modify/access permissions for queries and dashboard Aug 29, 2016
@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/access.py Outdated
.where(AccessPermission.object_type == object_type)\
.where(AccessPermission.access_type == access_type)\
.where(AccessPermission.grantee == grantee)\
.where(AccessPermission.grantor == self.current_user)

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

Does this mean that only the user who gave the permission can revoke it?

I think we can limit the look up here to (object_id, object_type, grantee). This means that we need to verify that the current user is the object owner or admin (we have a helper function for this).

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

Also this should be encapsulated in the model, so this becomes:

permission = AccessPermission.get_by_object_and_grantee(object_type, object_id, grantee)
permission.delete_instance()

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done, this has been moved to the model, and we're now verifying that the current user is the object owner or admin (using the helper function).

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/access.py Outdated

class AccessAttemptResource(BaseResource):

def post(self, object_type, object_id, access_type):

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

Why is this a POST request and not GET?

This comment has been minimized.

Copy link
@whummer

whummer Sep 8, 2016

Author Contributor

Intuitively, GET to me is more about getting the current state from an API, whereas POST is more where you actively try to run an action against the API (in this case posting a request to try and obtain an access permission). That's why we chose POST here. But I'm happy to change this to GET instead.

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 8, 2016

Member

It's not really trying to obtain* access, but really checking the access status. No resources get changed and it's a "safe" action, and hence semantically it doesn't feel right (to me) to use POST here.

* I mean obtain by creating something new.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done. Changed to GET

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/api.py Outdated
@@ -69,6 +70,11 @@ def json_representation(data, code, headers=None):
api.add_org_resource(QueryRefreshResource, '/api/queries/<query_id>/refresh', endpoint='query_refresh')
api.add_org_resource(QueryResource, '/api/queries/<query_id>', endpoint='query')

api.add_org_resource(AccessListResource, '/api/access/<object_type>/<object_id>', endpoint='list_access')
api.add_org_resource(AccessGrantResource, '/api/access/<object_type>/<object_id>', endpoint='grant_access')
api.add_org_resource(AccessRevokeResource, '/api/access/<object_type>/<object_id>', endpoint='revoke_access')

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

How about we name this resource as access_permission?

Also you can merge the last 3 classes into one (assuming you change the access check to GET). So it becomes AccessPermissionListResource and AccessPermissionResource.

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

After looking at the usage, I think we should change this to be: /api/<object_type>/<object_id>/acl, and object_type should be queries and dashboards to be consistent with the rest of the API. In the handler itself, we can map it to Query and Dashboard (or the object class).

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done. Adapted the API layout accordingly.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/models.py Outdated

id = peewee.PrimaryKeyField()
object_type = peewee.CharField()
object_id = peewee.IntegerField()

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

We should add an index on object_id, object_type.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/models.py Outdated
object = GFKField('object_type', 'object_id')
access_type = peewee.CharField()
grantor = peewee.ForeignKeyField(User, related_name='grantor')
grantee = peewee.ForeignKeyField(User, related_name='grantee')

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

How about we make grantee a GFK field as well? So we can later on add the option to grant to permission to an ApiKey object or a Group object.

It's NTH so we can skip this for now, as it should be an easy migration.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/models.py Outdated

id = peewee.PrimaryKeyField()
object_id = peewee.CharField()
object_type = peewee.CharField()

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

Same here re. index for object.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/access.py Outdated

class AccessGrantResource(BaseResource):

def post(self, object_type, object_id):

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

You need to make sure here that the user is admin or owner of the granted object.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/access.py Outdated
perm.access_type = access_type
perm.grantor = self.current_user
perm.grantee = grantee
perm.save()

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

The logic for granting permission should be encapsulated in a model method of AccessPermission.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done. Moved to AccessPermission model.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/access.py Outdated
if access:
return {'result': 'access_granted'}
abort(403)
return False

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

abort throws an exception, so this line is not needed.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

good catch, thanks. removed.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/dashboards.py Outdated
@@ -8,6 +11,19 @@
from redash.handlers.base import BaseResource, get_object_or_404


def _save_change(user, dashboard_id, old_dashboard, new_dashboard, change_type):

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

I think we can generalize this and have a single method for all change-tracking objects. Also it should be at the model level.

Ideally, it will be a hook that is being called on the model's #save method, but at that point we don't know who the user who made the change :-\ Maybe we can add another save method named tracked_save that will also take as a parameter the user who made the change.

One benefit of doing this at the model level, is that we can leverage dirty_fields property of the model (that will return the fields that were changed).

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Very good point, our initial implementation was indeed a bit messy and not generic at all.

This has been refactored and completely generalized. The _save_change function has been removed from dashboards.py and queries.py, and instead we now handle this functionality in the Model directly (see Change.save_change). This way things are much cleaner and more re-usable.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/dashboards.py Outdated
for dashboard in dashboards:
last_change = models.Change.get_latest(object_id=dashboard['id'], object_type=models.Dashboard.__name__)
if last_change:
dashboard['latest_version'] = last_change.id

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

The current version of a dashboard should be the tracked object's property. It will save us from doing this lookup, but also will be used for the locking.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Done. This change also impacted other parts of the code (e.g., the UI code where we previously tracked a propery latest_version which is now simply the version property of the model instance).

We've re-run all tests and did additional manual testing to ensure that the original functionality is still preserved.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/dashboards.py Outdated
return response

@require_permission('edit_dashboard')
def post(self, dashboard_slug):
dashboard_properties = request.get_json(force=True)
# TODO: either convert all requests to use slugs or ids
dashboard = models.Dashboard.get_by_id_and_org(dashboard_slug, self.current_org)

# check access permissions
if self.current_user.id != dashboard.user.id:

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

We should use is_admin_or_owner here (this is the helper I mentioned in another comment).

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

Good point. Done.

@arikfr
arikfr reviewed Sep 6, 2016
View changes
redash/handlers/dashboards.py Outdated
last_change = models.Change.get_latest(object_id=dashboard.id, object_type=models.Dashboard.__name__)
if last_change and 'latest_version' in dashboard_properties:
if last_change.id > dashboard_properties['latest_version']:
abort(409) # HTTP 'Conflict' status code

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 6, 2016

Member

This is very optimistic locking :-) As it hopes that no one else will save anything in the time that passes from this check until line 110. While usually it's OK, it's still a risk.

The optimistic locking I was referring to is what Charles suggested in this comment:

class BaseVersionedModel(BaseModel):
    version = IntegerField(default=0)

    def save_optimistic(self):
        if not self.version:
            return self.save()  # Since this is an `INSERT`, just call regular save method.

        # Update any data that has changed and bump the version counter.
        field_data = dict(self._data)
        current_version = field_data.pop('version', 0)
        field_data = self._prune_fields(field_data, self.dirty_fields)
        if not field_data:
            raise ValueError('No changes have been made.')

        ModelClass = type(self)
        field_data['version'] = ModelClass.version + 1  # Atomic increment

        query = ModelClass.update(**field_data).where(
            (ModelClass.version == current_version) &
            (ModelClass.id == self.id))

        nrows = query.execute()
        if nrows == 0:
            # It looks like another process has updated the version number.
            raise ConflictDetectedException()  # Raise exception? Return False?
        else:
            self.version += 1  # Update in-memory version number.
            return nrows

Note the use of update + where. It might be needed some changes, to use our callbacks for pre/post save.

This comment has been minimized.

Copy link
@whummer

whummer Sep 9, 2016

Author Contributor

This part of the handler has also been updated to work with the new version property in the model. Conceptually, it should provide the same functionality as the snippet that Charles had suggested.

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Sep 6, 2016

Finally reviewed the code. I mostly reviewed the server code... I will go over the frontend code in more detail as we progress.

Sorry for the lots of comments, it just that this is a critical piece of the system...

@whummer

This comment has been minimized.

Copy link
Contributor Author

whummer commented Sep 6, 2016

Awesome, thanks for the detailed feedback. We'll try our best to address your comments in the next couple of days... Cheers

@whummer whummer force-pushed the whummer:feat/share-access-permissions branch 2 times, most recently Sep 9, 2016
@whummer

This comment has been minimized.

Copy link
Contributor Author

whummer commented Sep 9, 2016

Hey @arikfr, we've carefully considered the feedback and refactored the implementation based on your suggestions.

The major changes are around adding the version field to the model classes (Query and Dashboard for now), as well as moving most of the core functionality from the handlers to the model classes, which indeed should make the code much more readable and reusable.

The PR is now ready for a second round of review. Looking forward to receiving more feedback. Cheers.

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Sep 9, 2016

@whummer super. I will review the UI code now and then give the server code a second pass. Thank you for the rapid follow up.

migrations/0026_add_access_control_tables.py Outdated
try:
with db.database.transaction():
migrate(
migrator.add_column(tbl, 'version', field)

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

You can keep it as is, but in the future I think it's simpler to write/easier to read (and understand) the "straightforward" version of the code:

migrate(
    migrator.add_column('queries', 'version', Query.version),
    migrator.add_column('dashboards', 'version', Dashboard.version)
)

It's even the same number of loc :-)

}

if ($scope.foundUsers === undefined) {
User.query(function(users) {

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

In the case of queries, we need to filter here for users who have full access to the query data source. Ideally it's something we will let the backend do, to avoid replicating logic in the UI.

We can postpone implementing this to the end, as it doesn't change the interface behavior.

rd_ui/app/scripts/controllers/controllers.js Outdated
$scope.addGrantee = function(user) {
$scope.newGrantees.selected = undefined;
var body = {'access_type': 'modify', 'user_id': user.id};
$http.post($scope.api_access, body).success(function() {

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

(indentation)

rd_ui/app/scripts/controllers/controllers.js Outdated

// List users that are granted permissions
var loadGrantees = function() {
$http.get($scope.api_access).success(function(result) {

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

(Indentation)

rd_ui/app/scripts/controllers/dashboard.js Outdated
$scope.showSharePermissionsModal = function() {
// Create scope for share permissions dialog and pass api path to it
var scope = $scope.$new();
$scope.api_access = 'api/dashboards/' + $scope.dashboard.id + '/acl';

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

Except for objects we get from the backend, we use camelCasing in the frontend code (api_access -> apiAccess). Also I think aclUrl will better convey the meaning of this variable.

redash/models.py Outdated
revoke_permission(object_type=object_type, object_id=object_id, access_type=access_type)

def delete_instance(self, *args, **kwargs):
super(AccessPermission, self).delete_instance(*args, **kwargs)

This comment has been minimized.

Copy link
@arikfr
redash/models.py Outdated
'object_type': self.object_type,
'access_type': self.access_type,
'grantor': self.grantor.id,
'grantee': self.grantee.id

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

Use self.grantor_id and self.grantee_id or it will query the database for the user models here...

redash/models.py Outdated
query = query.order_by(Change.created_at.desc())
for change in query:
return change
except Exception, e:

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

Why would we have an exception here?

redash/models.py Outdated
if change_type:
query = query.where(Change.change_type == change_type)
query = query.order_by(Change.created_at.desc())
for change in query:

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

It will query the database for all the changes, create objects for them and then use only the first...

Just do: return query.limit(1).first() (first() by itself doesn't limit the result set).

self.save(*args, **kwargs)
# save Change record
new_change = Change.save_change(user=changing_user, old_object=old_object, new_object=self)
return new_change

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

Instead of duplicating the tracked_save logic, let's have a mixin for this like we have for timestamps.

Also I'm not happy with the added complexity of the way the actual change is tracked. We currently have no use for the change itself, how about we don't implement this as part of this pull request?

This comment has been minimized.

Copy link
@arikfr

arikfr Sep 18, 2016

Member

Also I just noticed that you didn't implement the actual optimistic locking here...

Copy link
Member

arikfr left a comment

Did another pass at the code , this time along with the frontend code.

I guess some of the comments might seem about semantics only (like the name changes), but as the person who will need to maintain in the future, it's important...

Also we need to add more tests to cover the change tracking/locking functionality. If you want, you're welcome to add me to your repo as a collaborator and I will add those (and maybe do some of the smaller changes).

I think that in the future, before doing such large change, I will just add you as a collaborator on getredash/redash...

Thanks.

@arikfr
arikfr approved these changes Oct 26, 2016
@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Oct 26, 2016

OK, I'm done (!). The UI for managing permissions is behind a feature flag and turned off by default. There are few "open" issues that we will address before we "graduate" this feature:

  • Multiple users can add widgets to the dashboard without getting the conflict error. Need to decide if it's an issue.
  • We need to decide how to handle the case when someone gives you edit permissions to a query you don't have access to its data source. We should probably not allow this in the first place.
  • We need to send an email / notification to the grantee once he's granted access.

Anything I forgot?

I'm not merging this yet, as I want to merge a few other things first, but I will merge it tomorrow.

@whummer

This comment has been minimized.

Copy link
Contributor Author

whummer commented Oct 27, 2016

Wow @arikfr , awesome work. Thanks for putting so much effort into finishing off this PR. Super excited to see this merged tomorrow.

@arikfr arikfr merged commit 2f09043 into getredash:master Oct 28, 2016
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@adamlwgriffiths

This comment has been minimized.

Copy link
Contributor

adamlwgriffiths commented Nov 3, 2016

We've noticed the 'the dashboard has been changed by another user' message come up when there should be no reason. Not sure what has triggered it, but attempting a second save 'fixed it'.

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Nov 10, 2016

@adamlwgriffiths does it come up every time? if not, any ideas on how to reproduce?

@adamlwgriffiths

This comment has been minimized.

Copy link
Contributor

adamlwgriffiths commented Nov 10, 2016

I haven't been able to pin it down, so these may not be correct as I may have performed other actions prior / in-between.

The first time I saved a dashboard (created prior to this patch) the message appeared.
Opening a dashboard, then editing a widget / query in another tab, would cause this message when I then went to re-order widgets using the 'edit dashboard' dialog.
I've also received it when editing queries, can't put a finger on when. I've also noticed that a 'query saved' dialog will appear below the 'changed by another user' dialog.

Most of these were with a single user on the system, so any conflict is either from multiple tabs or bugs. It's still a frustrating dialog and really drags down the UX.

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Nov 13, 2016

I think I know what happened in your case:

  1. Add widget to the dashboard.
  2. Try to rearrange them.
  3. You get the conflict message.

That's obviously not the intended behavior and will be fixed before wrapping up v0.12.

In normal situations you won't see this message often (or at all). I think one improvement we can introduce is to give you the option to override the changes anyway.

arikfr added a commit that referenced this pull request Nov 15, 2016
arikfr added a commit that referenced this pull request Nov 28, 2016
@ssoltane

This comment has been minimized.

Copy link

ssoltane commented Jan 18, 2017

Hi,
I updated my redash version from v0.11 to v0.12. But i can't see the feature.
Is there a flag that i need to activate ?

Thanks

@adamlwgriffiths

This comment has been minimized.

Copy link
Contributor

adamlwgriffiths commented Jan 18, 2017

Which feature specifically? The permissions menu item?

@ssoltane

This comment has been minimized.

Copy link

ssoltane commented Jan 18, 2017

I can't see the "Share edit permission" for queries or dashboards when i click on "..."

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Jan 19, 2017

@ssoltane It's part of 0.12. As mentioned in the comments, the UI is disabled with a feature flag. You need to add the environment variable FEATURE_SHOW_PERMISSIONS_CONTROL and set it to true.

If you used our AMIs or bootstrap script, then just add this to the /opt/redash/.env file.

@ssoltane

This comment has been minimized.

Copy link

ssoltane commented Jan 19, 2017

Super thks for the information :).

@adamlwgriffiths

This comment has been minimized.

Copy link
Contributor

adamlwgriffiths commented Feb 7, 2017

@arikfr I'm not sure if this is still relevant (I'm using angular 1.2).

I'm able to reliablyreplicate the 'query changed' dialog by changing the data source from the drop down and hitting save.

@arikfr

This comment has been minimized.

Copy link
Member

arikfr commented Feb 8, 2017

@adamlwgriffiths I don't see it happen in v1.0.0.

@galgoogle

This comment has been minimized.

Copy link

galgoogle commented Oct 23, 2017

@arikfr , I've added the FEATURE_SHOW_PERMISSIONS_CONTROL, but still can't see the edit permissions. I'm using version 2.0.0.
I'm looking for a way users will be able to run a query but won't be able to modify it.

@satyadeepk

This comment has been minimized.

Copy link

satyadeepk commented Jan 3, 2018

This is the right environment variable to be added to /opt/redash/.env

export REDASH_FEATURE_SHOW_PERMISSIONS_CONTROL="true"

It works with this.
image

@galgoogle

This comment has been minimized.

Copy link

galgoogle commented Jan 8, 2018

@satyadeepk I've added it, and I see now "Manage Permissions". But when using it, it doesn't affect anything.

shinsuke-nara pushed a commit to KiiCorp/redash that referenced this pull request Mar 1, 2019
…issions

Add: share modify/access permissions for queries and dashboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.