Permissions support for Workbooks, Datasources, Projects

[DepthDeluxe]

Addresses #41
@t8y8 previously commented in the conversation.

My approach here was to minimize the duplication of code as Projects, Datasources, and Workbooks all have the same remote access model. The two bigger classes, PermissionsItem and EndpointWithPermissions are more like mixins which add the functionality.

Other classes added are GranteeCapabilityItem which stores the actual permissions for a single group and Permission which stores all the constants.

I tried to closely follow the way the data and naming conventions are represented in the API. One thing to note: group permissions and user permissions are stored in the same list. The previous partial implementation suggested that we keep user and group permissions separate. I wanted to keep them together to match the API. In addition, the permissions collection is a list so there is a chance for users of this client library might have two separate GranteeCapabilityItems for the same entity.

I also added a couple of test cases to validate reading. I tested reading permissions and updating permissions for both users and groups on a Tableau Server 10.1 deployment.

[DepthDeluxe]

Unit tests have been failing due to some other external thing. Last time I checked trunk is actually broken at the following unit test

====================================================================== ERROR: test_populate_preview_image (test.test_workbook.WorkbookTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/travis/build/tableau/server-client-python/test/test_workbook.py", line 313, in test_populate_preview_image self.assertEqual(response, single_workbook.preview_image) AttributeError: 'WorkbookItem' object has no attribute 'preview_image' 

[t8y8]

@DepthDeluxe Great! I will take a look as soon as I can. Sadly hackathon has passed so this slips back into my 'after hours' time, but I'll do my best.

A first comment that I have on a quick pass: I'm a known detractor of trying to subclass Endpoint, since I don't know what that might grow to in the future (TaggableEndpoint, EndpointWithPermissions, Etc). Is it possible to use composition, much like was one for tagging (31df99b)?

For the test, the CI isn't failing on master, I'll try and take a look at that.

Update: This is going with a simple dict approach for passing in the permissions map, which is one I mostly favored. I'll keep reviewing :)

[DepthDeluxe]

@t8y8, I see your point. I believe this shouldn't be too bad to rework.

Regarding your update, are you ok with the way permissions are organized? I could do something like put the PermissionsItem inside another dictionary to ensure duplication doesn't happen.

[DepthDeluxe]

@t8y8, I have addressed the inheritance issue by replacing with composition. There is _PermissionsEndpoint which other Endpoints can instantiate and pass calls to. In addition, I managed to fix the unit test problem. For some reason whenever I checkout the development branch and commit, my local git client removes the workbook.preview_image endpoint. Now all the checks pass.

Let me know if there is anything else you want changed.

class _PermissionsEndpoint(Endpoint): ''' Adds permission model to another endpoint Tableau permissions model is identical between objects but they are nested under the parent object endpoint (i.e. permissions for workbooks are under /workbooks/:id/permission). This class is meant to be instantated inside a parent endpoint which has these supported endpoints ''' 

[t8y8]

Thanks for updating the PR!

I'll try to be more responsive to future PRs :)

I left several comments, some should be simple, others are questions or thoughts on alternatives to provoke discussion.

I think the endpoint design is good, just some cleanup and questions.

I don't like nesting the model as it currently stands, but I acknowledge that it's basically following the XML format. I will need to play with this and some old thoughts on the matter to propose any alternatives -- so in the mean time I commented on the code as it stands.

[t8y8]

I see your note about why this must be a lambda but I don't fully grok it yet. _ResourceTagger doesn't do this -- why is this endpoint different?

[DepthDeluxe]

@t8y8, you need a lambda because you can't assign the parent baseurl on instantiation because it currently isn't known. It is only known once you sign in and the library determines what API to use.

This problem was solved separately in _ResourceTagger by taking in a baseurl every single time you call any member function. I didn't like this model because since the object is owned by a parent endpoint, its baseurl should be part of the object. I guess an alternative here could be requiring the parent to provide itself to this object's constructor, but I felt that produced unnecessary coupling.

[DepthDeluxe]

for reference, below is a snippet from _ResourceTagger

 # Remove and add tags to match the resource item's tag set def update_tags(self, baseurl, resource_item): if resource_item.tags != resource_item._initial_tags: add_set = resource_item.tags - resource_item._initial_tags remove_set = resource_item._initial_tags - resource_item.tags for tag in remove_set: self._delete_tag(baseurl, resource_item.id, tag) if add_set: resource_item.tags = self._add_tags(baseurl, resource_item.id, add_set) resource_item._initial_tags = copy.copy(resource_item.tags) logger.info('Updated tags to {0}'.format(resource_item.tags)) 

As you can see, baseurl is a parameter to the member function

[t8y8]

Got it, thank you! We may want to refactor _ResourceTagger to do the same #notetoself

[t8y8]

key -> capability_name
value -> mode
Or something similar, just key and value are a little too generic for me

[t8y8]

I appreciate the descriptive names, but I think the verbosity actually hurts a bit here. Probably because our code/xml has the longest most obtuse names possible :(

grantee_capability can just be 'capability', we know we're in the context of a grantee in the loop.

grantee_capabilities_element can probably safely be grantee_cap_element

[t8y8]

This is the item that needs the most discussion, because getting the model API right is what most users will 'feel'.

I'm not sure yet about this approach, I need to write some sample scripts against it.

That said, some off-my-head comments are included to spur discussion. Feel free to take a new idea and run with it or disagree.

[t8y8]

I don't like this being represented in the model at all, where is it needed and can we get away with making this 'dumb' to the PermissionsModel and just call the object a 'resource' or something?

[t8y8]

Similarly, I don't like this being represented in the model at all, where is it needed and can we get away with making this 'dumb' to the PermissionsModel and just call the object a 'grantee' or something?

[DepthDeluxe]

Removing the difference between User permission and Group permission could be risky because users may not know what do do with the grantee_id field.

We could either subclass GranteeCapabilityItem (after renaming as mentioned above) to UserGranteeCapabilityItem and GroupGranteeCapabilityItem. Don't really like that since this is just kinda a data bag of an object. Instead, we could have two fields user_id and group_id, either of which are populated depending on what type the permission is for.

What do you think?

[t8y8]

There are pros and cons to splitting these out into their own namespaces -- discoverability and some safety, but it is enforced server idea already so this might just be extra code to maintain.

Still... I think this is probably fine.

[DepthDeluxe]

@t8y8, the other option here is to implement permission classes for each type and have a factory method that instantiates the proper ones. So instead the API could be something like

workbook_capability.add_comment = True # for the "Allow" permission workbook_capability.change_permissions = False # for the "Deny" permission workbook_capability.delete = None # for the "empty" permission datasource_capability.export_xml = False ... 

[t8y8]

I really like this design, optionally taking in kwarg pairs if you want a functional approach.

[mcw0933]

Ping - we have an interest in this PR, wondering if it's still underway? What's remaining to do?

[t8y8]

This is where we had debate on an old approach -- I'd like to call this a Rule or PermissionsRule despite that not being the XML name. It's what the UI/Docs and humans tend to call "the combo of resource and grantee"

What do you think?

[DepthDeluxe]

Seems reasonable, GranteeCapabilityItem doesn't really mean anything to me

[t8y8]

I'm considering a different way to parse this... but for now can you pull the list of permission_type options into a constant.

You also don't need the 'break' statements at the end of the try

[DepthDeluxe]

Sure, it looks like it might be possible to pull the first element within the parent but that could be risky across different versions. I can definitely pull into a constant.

[t8y8]

Raise an exception and log the issue please

[t8y8]

@DepthDeluxe I played with some things a bit, and here is the high level API I like for setting permissions. It matches the conceptual model of the UI, and I think feels pretty clean. It's quite different from the underlying XML though:

PermissionsRule(grantee : Union[UserItem, GroupItem], capabilities : Dict[Capability, Mode]) server.projects.update_permission(permissions : List[PermissionsRule]) cool_guy_permissions = {Perms.Caps.Read: Perms.Mode.Allow, Perms.Caps.Write: Perms.Mode.Allow} tyler_permissions = PermissionsRule(tyler_user_item, cool_guy_permissions) all_user_permissions = PermissionsRule(all_users_group, {Perms.Caps.Read: Perms.Mode.Deny}) permissions = [tyler_permissions, all_user_permissions]

I need to think through delete a little more, but what do you think?

EDIT: I thought through one final problem, which is how to inject the correct item attribute to get the project or workbook or datasource tag. What if the permissions endpoints take the item they’re updating and a permissionsrule item:

project_item = get_project_by_name(‘abc’) server.project.update_permissions(project_item, [permission_one, permission_two]) 

The request factory/serializer would then be the only thing that needs to know how to combine them together. PermissionRules can be reused across types, which might be handy if you want to loop ove a bunch of workbooks and data sources, or something like that.

That combining code is also reusable, just take the ‘type’ of a content item and its ID and add it into the xml.

[t8y8]

@DepthDeluxe Just wanted to ping you for your thoughts and see if you were still interested in working on this? We'd really love to get this ready to merge and fill the gap in the library :)

[DepthDeluxe]

@t8y8 thanks for the ping, haven't been able to work on this as much as I'd like lately...

I like your suggestion for the API, the thing I'm worried about is API call magnification if there are a lot of specific permissions set for a workbook (i.e. REST call to retrieve UserItem and GroupItem for every single permission set on the server). I'm concerned here because I'm writing an audit tool that looks for a specific group permission on every workbook on our server, ideally, I wouldn't like to pull other items that I'm not looking at... What about making that attribute load lazily if not already available? We could allow you to search for the ID and then load more information as needed.

In regard to your edit, I believe that's the way it's done already, at least based on my understanding of what you are saying.

+ def update(self, item, permission_item): + url = '{0}/{1}/permissions'.format(self.owner_baseurl(), item.id) + update_req = RequestFactory.Permission.add_req(permission_item) + response = self.put_request(url, update_req) + permissions = PermissionsItem.from_response(response.content, + self.parent_srv.namespace) + + logger.info('Updated permissions for item {0}'.format(item.id)) + + return permissions 

For deletion, we would want the ability to easily delete a single permission along with easily deleting permissions for a UserItem and GroupItem in a single call.

At least for me the REST API is quirky in that you need to delete an Allow permission before adding a Deny permission. Could this be addressed through the API somehow? Might be hard if we are also updating a lot of other permissions. Could just run delete and then update to replace with what is specified. Not sure if that is a good idea.

[t8y8]

@DepthDeluxe I don't quite follow what you mean by call magnification, can you provide an example?

We could lazy load attributes, we basically do that for anything that requires a populate_x call, just depends on what you mean exactly.

I think a PermissionsRule is just your ItemWithPermissions except we simplify the Capabilities a bit, and we just hold a single grantee object instead. Oh, is this what you mean by magnification, where the grantee object only holds the ID and you want more attributes from it?

[DepthDeluxe]

@t8y8, yes I'm concerned about making a lot of requests. This wouldn't be the case if we kept the mapping based on IDs like below.

permissions: { "$USER_ID": { "Read": "Allow", "Modify": "Deny" }, "$GROUP_ID": { "Modify": "Allow" } } 

where $USER_ID and $GROUP_ID are the ids of the target. This way we can ensure that there aren't multiple PermissionsRules for the same ID. The only thing I don't like about this is mixing the two IDs together, if we just have the IDs how do we know if we are referring to a user or a group?

Also I'm leaving out an important thing here: my primary use-case is an automated scanning process that will check the All Users group permission and set them to a predefined value if it isn't what its supposed to be. Our current discussion is more around how to set permissions but reading them is just as important.

I guess then the approach would be

1. Get the All Users group id
2. Get the permissions for each workbook
3. Check if All Users id is present in the collection
   a. If it is, then check to see if it is the right value. If it isn't set properly, then set the permission to proper value
   b. If it isn't set the permissions

Perhaps the structure should be more like this:

group_permissions: { "$USER_ID": { "Read": "Allow", "Modify": "Deny" }, } user_permissions: { "$GROUP_ID": { "Modify": "Allow" } } 

This way we only get what we are looking for. Thus the class would be more like

PermissionsItem(group_permissions: Dict[GroupId, Dict[Capability, Mode]], user_permissions: Dict[UserId, Dict[Capability Mode]]) 

My gut feeling says that it would be nice to have a helper function to make it easy to get the actual UserItem and GroupItem objects from these dictionaries (primarily to see object names to make sense of who is getting assigned permissions) even though it is one quick call away via server.groups.get_by_id(). For example, if you are trying to display what users have what permissions, you are going to have to mentally jump back and forth between traversing this structure/parsing permissions and pulling the object from the server via its ID to get the human readable name of the object. Does that make sense?

[DepthDeluxe]

@t8y8 this little bit is terrible, looking for feedback here.

Trying to get the item type (Workbook, Datasource, Project) without having to explicitly send it every time. You had previously mentioned you want the Permissions module to be dumb to the type of . Figuring that if the function accepts item_type, then we will need to create an Enum for that which goes against your recommendation.

TBH it might be nice having each of the WorkbookItem, DatasourceItem, etc... to have an enum/type string associated with them so you could just use item.type instead of this.

[t8y8]

I was thinking about this over the weekend. Given no changes, I'd prefer explicit 'isinstance' checks because at least we're not relying on the name but that's not much better.

I think including a .type str or enum on the ModelItems is better. Maybe call it _xml_type if just to be extra conservative.

[DepthDeluxe]

@t8y8, I have done another draft of the API implementation. Simplified naming conventions and the way that capabilities are stored. GranteeCapabilityItem is now CapabilityItem. Now the PermissionsItem contains a map consisting of

(GranteeType, Server_ObjectId) -> CapabilityItem 

I also addressed some of the above comments in particular places where the code didn't change.

Please let me know what you think.

[t8y8]

Thanks, I will take a look as soon as aI can, definitely this week.

[t8y8]

Pinging myself -- Haven't forgotten you.

[DepthDeluxe]

@t8y8 bumping, did you forget me now? 😭

[t8y8]

I have not, just flooded with the 'day job' :(

Would you be willing to give me 'push' on your fork? I'd love to clone it and play around with a few scripts and try and collaborate there before merging back in here?

[DepthDeluxe]

@t8y8 no problem, been there 😅

I have granted you collaboration access to my repo, feel free to play around with it.

[mihaico]

any news on this feature?

[t8y8]

@DepthDeluxe thanks! I'm playing around with it, sorry this is taking time... you know how open source goes :/

[DepthDeluxe]

@t8y8 any updates?

[mihaico]

a capability setter would be nice to enable user defined permission changes

[mihaico]

project permissions definitions also includes default permissions for workbook and datasources access, it would be nice to have an url separation e.g. url = '{0}/{1}/default-permissions/{2}'.format(self.owner_baseurl(), item.id, default) (where default is 'workbooks' or 'datasources')

[t8y8]

@DepthDeluxe Hey!

# class PermissionsGrantee: # grantee_type = xpath-to-find-element-name # grantee_id = xpath-to-get-id # class WorkbookItem: # permissions: PermissionsCollection # class PermissionsCollection: # __init__(self, rules): # rules: List[PermissionsRule] (But your system is nice as well ->) # rules: Dict[Tuple[PermissionsGrantee.id, PermissionsGrantee.type]: PermissionsRule]] # class PermissionsRule: # grantee_type: PermissionsGrantee # grantee_id: str # permissions_map: Dict[Capability: Mode]) # class Capability: # capability_name: str # mode: str # class _PermissionsEndpoint: # def update(self, item, permissions): -> Item # def delete(self, item, permissions): -> Item # # This would iterate and call delete on the map, # # so calling update then delete then update would even out. 

Based on your 'development' branch, I believe this is pretty close to what you have.
Rename PermissionsItem to PermissionsCollection.
Rename CapabilityItem to PermissionsRule.
Let's make a PermissionsGrantee class that's responsible for parsing out the element name and for serializing itself
I think we can use xpath to get the type based on the tag name, rather than the try/except dance and then the ('Item').split.lower() dance

# capabilities[(grantee_type, grantee_id)] = capability_item (line 109 of permissions_item)

This logic to construct your list of rules is currently in the parser, and I think that should maybe move to business logic of the PermissionsCollection class but I haven't really cracked that yet, it just took me a long time to figure out how the dict was getting built. Open to ideas there!

So overall, I actually think this is close, and I have some ideas for how to move forward quickly:

• I will try to hack on this more tomorrow; but I actually would be available for a call/screen share to run through ideas with you
• I think you should trim the PR down to just the workbook type for now, so the reviews look less scary; once we feel good it's mostly copy paste for the other types
• What do you think of the comments above?

[t8y8]

@DepthDeluxe I tried pushing my in-progress tweaks to your fork in a new branch, but it says I'm still denied push

[DepthDeluxe]

@t8y8 it looks like you never accepted the original invite. It seems like you might be sent the link to your email? Something like that. Anyways I have canceled the invite and sent it again.

[DepthDeluxe]

@t8y8 I think what you are suggesting is definitely close to what I already have and seems good. I would be open to a call sometime this week to discuss details if necessary. Send me an email at colin@heinzmann.me to organize the call.

[irwando]

@t8y8 Made some extensive additions around permissions. Closing this old pull request.