8000 Permissions Support 2.0 by t8y8 · Pull Request #429 · tableau/server-client-python · GitHub
[go: up one dir, main page]
Skip to content

Permissions Support 2.0 #429

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Jul 27, 2019
Merged

Permissions Support 2.0 #429

merged 22 commits into from
Jul 27, 2019

Conversation

t8y8
Copy link
Collaborator
@t8y8 t8y8 commented May 7, 2019 •
edited
Loading

This is an old massive PR that slowly works on getting permissions into tsc.

@DepthDeluxe contributed a lot (most) of the initial code, but I've forked and simplified a lot of the interfaces. I think we can still build the convenience functions they want but this is a simpler starting point.

I have two main open questions:

  1. What do we do with Grantees? The GranteeUser/Group object only needs an 'id' to be passed into a PermissionsRule. However, the Group and User Item classes have a bunch of required (and non-nullable) properties. It's nice that we can just pass in a UserItem or GroupItem from a query, into PermissionsRules but I ALSO want to enable the ability to just create the users/groups you need based on ID. Currently I have this subtyped and overriding the problematic properties, but we could also just change UserItem/GroupItem to be more flexible. A middle ground is some kind of classmethod that constructs a UserItem/GroupItem out of only ID (a classmethod of some sort). What do you think?

  2. How should we handle Default Permissions? Help Link
    I have a few ideas, one is to just have a default permissions endpoint that's largely copy/paste from the permissions endpoint, and then have an update method on it that takes (ResourceType, List[PermissionsRule]). Alternatively I can reuse most of the existing code if I just make a update_datasource_default_permissions, update_workbook_default_permissions, etc -- on to the permissions endpoint, and then somehow only enable that for projects (via an optional parameter in the init for the permissions endpoint? Via a classmethod? something else?) I need to prototype here more.

/cc @shinchris @RussTheAerialist @gaoang2148

If I can get answers to those two, I can implement the remaining stuff and start on the tests.

DepthDeluxe and others added 5 commits May 6, 2019 22:29
@DepthDeluxe
adding permissions support for workbook, datasource, project
19fd25f 
* _PermissionsEndpoint to be used inside Endpoint that has the calls
* Added a couple of classes related to permissions, followed naming
convention of API
@t8y8
first rename
2239612
@t8y8
checkpoint
28a66d0
fix merge conflict
751901b
checkpoint 1 of refactor, woo!
0670e42
t8y8
t8y8 commented May 7, 2019
View reviewed changes
@shinchris
Copy link
Contributor

Thanks for tackling the permissions work!

Here are my thoughts for the two questions above:

  1. My initial thought was that creating an item based on ID would go against our existing patterns, but I can't come up with a better way to avoid that due to the design of the permissions requests. I think the subtyped grantees are good since we can just pass in a UserItem or GroupItem from a query into PermissionsRules, rather than extracting out the IDs and creating new grantee objects. I think that would be better than making changes to make UserItem and GroupItem more flexible.

  2. I think the first idea of adding a default permissions function to PermissionsEndpoint and then calling that from WorkbookEndpoint/DatasourceEndpoint with the ResourceType would be cleaner with the structure we have now. Maybe we'll also support flows some day.

/cc @RussTheAerialist @gaoang2148

@t8y8

This comment has been minimized.

Sign in to view
gaoang2148
gaoang2148 suggested changes Jun 7, 2019
View reviewed changes
Copy link
Contributor
@gaoang2148 gaoang2148 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the things!

(Make the update methods all consistent-like.
convert strings to constants

@t8y8 t8y8 requested a review from gaoang2148 June 11, 2019 23:33
@t8y8
Copy link
Collaborator Author
t8y8 commented Jun 11, 2019 •
edited
Loading

Aaaaand done.

I've made the calls all consistent, and implemented the Reference pattern.

I decided not to subtype things because they all need a tag_name that they take from the Item class, and they all need an ID passed in to the constructor... the subtypes added no value at all!

Maybe someday they will, we can safely refactor this then, but for now, this implements the Reference method with takes the tag_name defined from the real content-type class, and then an ID.

Ran a manual test and the few tests in the suite, I'll add more tests but wanted to get this out for review so I can start adding the other content types.

/cc @RussTheAerialist @gaoang2148 @shinchris

@t8y8 t8y8 changed the title [WIP] Permissions Support 2.0 Permissions Support 2.0 Jun 12, 2019
@t8y8 t8y8 mentioned this pull request Jul 15, 2019
Closed
@shinchris
Copy link
Contributor

🚀
@gaoang2148 @RussTheAerialist

@t8y8 t8y8 merged commit 122fe1a into tableau:development Jul 27, 2019
@t8y8 t8y8 deleted the pr-1 branch July 27, 2019 07:10
@irwando irwando mentioned this pull request Jul 30, 2019
Closed
@shinchris shinchris mentioned this pull request Oct 4, 2019
Merged
shinchris pushed a commit that referenced this pull request Oct 4, 2019
Merge pull request #507 from tableau/development
b260ebc 
Release v0.9
## 0.9 (4 Oct 2019)

* Added Metadata API endpoints (#431)
* Added site settings for Data Catalog and Prep Conductor (#434)
* Added new fields to ViewItem (#331)
* Added support and samples for Tableau Server Personal Access Tokens (#465)
* Added Permissions endpoints (#429)
* Added tags to ViewItem (#470)
* Added Databases and Tables endpoints (#445)
* Added Flow endpoints (#494)
* Added ability to filter projects by topLevelProject attribute (#497)
* Improved server_info endpoint error handling (#439)
* Improved Pager to take in keyword arguments (#451)
* Fixed UUID serialization error while publishing workbook (#449)
* Fixed materalized views in request body for update_workbook (#461)
@bcantoni bcantoni mentioned this pull request Nov 19, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shinchris shinchris shinchris left review comments

@gaoang2148 gaoang2148 Awaiting requested review from gaoang2148

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t8y8 @shinchris @gaoang2148 @DepthDeluxe
0