Factor common IAM policy bits into 'google.cloud.iam'. #3188

tseaver · 2017-03-22T20:31:08Z

Toward #1679.

Pubsub-specific roles, permissions left behind in google.cloud.pubsub.iam.

google.cloud.pubsub.iam.Policy subclasses the core one, extending it to deal with the pubsub-specific roles.

theacodes · 2017-03-22T20:35:49Z

core/google/cloud/iam.py

@@ -0,0 +1,217 @@
+# Copyright 2016 Google Inc.


elibixby · 2017-03-22T21:10:07Z

I had a previous proposal to do this which got abandoned due to slow review cycle, but it's here: See #2031

The key takeaway: the API representation of bindings is really gross to interact with. Client side it would be much nicer if bindings were a dict with roles as keys, and member sets as values. So instead of code like:

def add_role(policy, my_role, my_member):
  roles = [binding['role'] for binding in policy.bindings]
  if my_role in roles:
     members = policy.bindings[roles.index(my_role)]
     if my_member not in members:
       members.append(my_member)
  else:
     policy.bindings.append({'role': my_role, 'members': [my_member]})
  return policy

We had code like:

def add_role(policy, my_role, my_member):
  return policy.bindings.setdefault(my_role, set()).add(my_member)

core/google/cloud/iam.py

+        """
+        return 'allAuthenticatedUsers'
+
+    def _bind_custom_role(self, role, members):


core/google/cloud/iam.py

+        self.viewers = set()
+
+    @staticmethod
+    def user(email):


core/google/cloud/iam.py

+    def __init__(self, etag=None, version=None):
+        self.etag = etag
+        self.version = version
+        self.owners = set()


elibixby · 2017-03-22T21:36:55Z

Additionally note that Roles are an API resource, and should be represented with an object. What does the resource.queryGrantableRoles method return? Raw JSON?

Both resource.queryGrantableRoles and resource.testIamPermissions are part of the IAM meta API and must be implemented on every resource that supports IAM.

Role resources will be user managed with the addition of custom roles (currently in alpha), and will have a list of iam permissions that they grant, which will be important for user protection of custom methods, and for identifying the necessary roles that a member needs to be granted. (combining resource.testIamPermissions and resource.queryGrantableRoles)

Further explanation of suggestions like these can be found in comment threads in #2031

EDIT: Slight correction: testIamPermissions is indeed part of the meta API, but queryGrantableRoles calls directly to the IAM API, but can be called with ANY one platform resource string, so it might make more sense to have it as a method on a Resource object that implements an IAM mixin anyway.

EDIT2: This can probably be tackled in a followup PR, but should be considered when defining the interface? Do we want to make policy.bindings a map <string: set> or a map <Role: set> I think likely still the former, since we wont be receiving role metadata along with a policy, but it's worth consideration.

pubsub/google/cloud/pubsub/iam.py

-    @staticmethod
-    def user(email):
-        """Factory method for a user member.
+    def _bind_custom_role(self, role, members):


tseaver · 2017-03-23T17:41:59Z

@elibixby I think I have addressed your concerns about supporting user-defined roles in b625017.

re: queryGrantableRoles: We can't propely target implementing support for not-yet-released APIs in the public repository: my current writ is to get storage up to the same level of IAM support that pubsub currently has. I will be adding storage-specific implementations of get_iam_policy, set_iam_policy, and test_iam_permissions in a separate PR.

elibixby · 2017-03-23T17:48:01Z

@tseaver queryGrantableRoles isn't unreleased...https://cloud.google.com/iam/reference/rest/v1/roles/queryGrantableRoles

But I agree that this can be tackled in a follow-up PR.

Long term I still believe a common implementatoin for One Platform APIs will save you the most heartache. Storage is an oddball because it is so old (i.e. not a One Platform API) and IAM support was "backported" in a slightly weird way. But going forward all OP APIs will share a common IAM signature, and a Mixin could provide drop in support for that.

core/google/cloud/iam.py

-                policy.viewers |= members
-            else:
-                policy._bind_custom_role(role, members)
+            members = sorted(binding['members'])


core/google/cloud/iam.py

-            resource['bindings'] = bindings
+        if len(self.bindings) > 0:
+            resource['bindings'] = [
+                {'role': role, 'members': members}


core/google/cloud/iam.py

+        for role in self._OWNER_ROLES:
+            for member in self.bindings.get(role, ()):
+                result.add(member)
+        return result


lukesneeringer · 2017-03-23T22:37:24Z

EDIT @lukesneeringer I think doing @tseaver approach and then moving to my approach immediately before GA is a reasonable approach, thoughts @tseaver?

This is fine with me also.

lukesneeringer · 2017-03-30T14:52:33Z

@tseaver Checking on the status of this. :-)

Pubsub-specific roles, permissions left behind in 'google.cloud.pubsub.iam'. 'google.cloud.pubsub.iam.Policy' subclasses the core one, extending it to deal with the pubsub-specific roles.

- google.cloud.iam.Policy holds a 'bindings' mapping, which doesn't enforce using known roles. - Its 'owners', 'editors', and 'viewers' are now properties which indirect over that 'bindings' attribute. Note that this is a breaking change, as users who relied on mutating one of those sets (rather than re-assigning it) will need to update.

- Don't pass roles w/ empty members to back-end. - De-duplicate role members when passing to back-end.

- Re-assign 'policy.viewers'/'policy.editors', rather than mutating them in place.

tseaver · 2017-04-05T22:14:18Z

@lukesneeringer I'm back at this, but can't figure out what the desired resolution would be. Here is a sketch at a way forward:

Return frozenset instances from the getters for owners, editors, and viewers (breaking anybody who expects to mutate them in place, rather than re-assigning them).
Emit DeprecationWarning from the setters for those properties, pointing people to the desired surface.

Then, the question is what the desired surface would be: I'm really leery of exposing the mutable bindings attribute as a dict: it allows us no way to do any checking at all. We could:

Make the Policy class dict-like, with roles as keys and lists of principals as values. The __setitem__ implementation would give us the hook to do any required checking.
Make Policy.bindings a dict-like object with a similar __setitem__.
Add a Policy.bind(role, *principals) method, and use whatever backing store we like.

Updating them in place never actually worked (they were sets created on the fly), but at least we give an appropriate error now if the user tries.

lukesneeringer · 2017-04-06T14:35:42Z

My personal preference is the first option (making Policy dict-like and defining __setitem__ to include appropriate validation).

tseaver · 2017-04-06T15:47:17Z

@lukesneeringer That is my preference as well. Update on the way.

Keys are roles, values are lists of principals.

tseaver · 2017-04-06T17:27:45Z

@lukesneeringer OK, I've updated Policy to be dict-like, and added deprecations for assignment to the "named" role attributes.

lukesneeringer

Approved, but giving @elibixby a chance to weigh in before merge.

core/google/cloud/iam.py

+        return self._bindings[key]
+
+    def __setitem__(self, key, value):
+        self._bindings[key] = value


lukesneeringer · 2017-04-12T22:12:36Z

Merging this when CI goes green.

…b-iam-for-storage Factor common IAM policy bits into 'google.cloud.iam'.

tseaver added api: pubsub Issues related to the Pub/Sub API. auth labels Mar 22, 2017

tseaver requested review from daspecster and dhermes March 22, 2017 20:31

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 22, 2017

tseaver mentioned this pull request Mar 22, 2017

Add IAM support to APIs which support it #1679

Closed

6 tasks

dhermes reviewed Mar 22, 2017

View reviewed changes

core/google/cloud/iam.py Outdated

@@ -0,0 +1,217 @@

# Copyright 2016 Google Inc.

This comment was marked as spam.

Sign in to view

elibixby reviewed Mar 22, 2017

View reviewed changes

core/google/cloud/iam.py Outdated

def __init__(self, etag=None, version=None):

self.etag = etag

self.version = version

self.owners = set()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

elibixby reviewed Mar 22, 2017

View reviewed changes

pubsub/google/cloud/pubsub/iam.py Outdated

@staticmethod

def user(email):

"""Factory method for a user member.

def _bind_custom_role(self, role, members):

This comment was marked as spam.

Sign in to view

elibixby reviewed Mar 23, 2017

View reviewed changes

core/google/cloud/iam.py Outdated

policy.viewers |= members

else:

policy._bind_custom_role(role, members)

members = sorted(binding['members'])

This comment was marked as spam.

Sign in to view

elibixby reviewed Mar 23, 2017

View reviewed changes

core/google/cloud/iam.py Outdated

resource['bindings'] = bindings

if len(self.bindings) > 0:

resource['bindings'] = [

{'role': role, 'members': members}

This comment was marked as spam.

Sign in to view

elibixby reviewed Mar 23, 2017

View reviewed changes

tseaver added 6 commits April 5, 2017 17:22

Factor common IAM policy bits into 'google.cloud.iam'.

ce42bae

Pubsub-specific roles, permissions left behind in 'google.cloud.pubsub.iam'. 'google.cloud.pubsub.iam.Policy' subclasses the core one, extending it to deal with the pubsub-specific roles.

Add API documentation for 'google.cloud.iam'.

743c5d5

Address review:

a01dae4

- Don't pass roles w/ empty members to back-end. - De-duplicate role members when passing to back-end.

Update pubsub snipets to accomodate changed semantics.

cc7c7c7

- Re-assign 'policy.viewers'/'policy.editors', rather than mutating them in place.

Accomodate noxification.

38f8976

Return frozensets from named Policy properties.

2196802

Updating them in place never actually worked (they were sets created on the fly), but at least we give an appropriate error now if the user tries.

tseaver added 2 commits April 6, 2017 11:49

Make IAM Policy objects dict-line.

77ed12b

Keys are roles, values are lists of principals.

Deprecate assignment to legacy role attributes.

e7654b6

lukesneeringer approved these changes Apr 6, 2017

View reviewed changes

elibixby reviewed Apr 7, 2017

View reviewed changes

core/google/cloud/iam.py Outdated

return self._bindings[key]

def __setitem__(self, key, value):

self._bindings[key] = value

This comment was marked as spam.

Sign in to view

lukesneeringer self-assigned this Apr 12, 2017

Coerce role-principal values to frozenset.

cf4e4a9

tseaver merged commit 3b8c9f6 into googleapis:master Apr 17, 2017

tseaver deleted the 1679-generalize-pubsub-iam-for-storage branch April 17, 2017 20:13

This was referenced Apr 24, 2017

Fixing broken logging system test. #3325

Merged

Cut releases of core, error reporting, pubsub, spanner and storage. #3340

Merged

richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017

Merge pull request googleapis#3188 from tseaver/1679-generalize-pubsu…

f270f5c

…b-iam-for-storage Factor common IAM policy bits into 'google.cloud.iam'.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor common IAM policy bits into 'google.cloud.iam'. #3188

Factor common IAM policy bits into 'google.cloud.iam'. #3188

tseaver commented Mar 22, 2017

theacodes commented Mar 22, 2017

This comment was marked as spam.

elibixby commented Mar 22, 2017 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

elibixby commented Mar 22, 2017 •

edited

Loading

This comment was marked as spam.

tseaver commented Mar 23, 2017

elibixby commented Mar 23, 2017 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

lukesneeringer commented Mar 23, 2017

lukesneeringer commented Mar 30, 2017

tseaver commented Apr 5, 2017

lukesneeringer commented Apr 6, 2017

tseaver commented Apr 6, 2017

tseaver commented Apr 6, 2017

lukesneeringer left a comment

This comment was marked as spam.

lukesneeringer commented Apr 12, 2017

Factor common IAM policy bits into 'google.cloud.iam'. #3188

Factor common IAM policy bits into 'google.cloud.iam'. #3188

Conversation

tseaver commented Mar 22, 2017

theacodes commented Mar 22, 2017

This comment was marked as spam.

elibixby commented Mar 22, 2017 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

elibixby commented Mar 22, 2017 • edited Loading

This comment was marked as spam.

tseaver commented Mar 23, 2017

elibixby commented Mar 23, 2017 • edited Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

lukesneeringer commented Mar 23, 2017

lukesneeringer commented Mar 30, 2017

tseaver commented Apr 5, 2017

lukesneeringer commented Apr 6, 2017

tseaver commented Apr 6, 2017

tseaver commented Apr 6, 2017

lukesneeringer left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

lukesneeringer commented Apr 12, 2017

elibixby commented Mar 22, 2017 •

edited

Loading

elibixby commented Mar 22, 2017 •

edited

Loading

elibixby commented Mar 23, 2017 •

edited

Loading