Accomodate (future) user-defined 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.

Author: tseaver

core/google/cloud/iam.py

@@ -53,9 +53,49 @@ class Policy(object):
     def __init__(self, etag=None, version=None):
         self.etag = etag
         self.version = version
-        self.owners = set()
-        self.editors = set()
-        self.viewers = set()
+        self.bindings = {}
+
+    @property
+    def owners(self):
+        """Legacy access to owner role."""
+        result = set()
+        for role in self._OWNER_ROLES:
+            for member in self.bindings.get(role, ()):
+                result.add(member)
+        return result
+
+    @owners.setter
+    def owners(self, value):
+        """Update owners."""
+        self.bindings[OWNER_ROLE] = list(value)
+
+    @property
+    def editors(self):
+        """Legacy access to editor role."""
+        result = set()
+        for role in self._EDITOR_ROLES:
+            for member in self.bindings.get(role, ()):
+                result.add(member)
+        return result
+
+    @editors.setter
+    def editors(self, value):
+        """Update editors."""
+        self.bindings[EDITOR_ROLE] = list(value)
+
+    @property
+    def viewers(self):
+        """Legacy access to viewer role."""
+        result = set()
+        for role in self._VIEWER_ROLES:
+            for member in self.bindings.get(role, ()):
+                result.add(member)
+        return result
+
+    @viewers.setter
+    def viewers(self, value):
+        """Update viewers."""
+        self.bindings[VIEWER_ROLE] = list(value)
 
     @staticmethod
     def user(email):
@@ -123,22 +163,6 @@ def authenticated_users():
         """
         return 'allAuthenticatedUsers'
 
-    def _bind_custom_role(self, role, members):  # pylint: disable=no-self-use
-        """Bind an API-specific role to members.
-
-        Helper for :meth:`from_api_repr`.
-
-        :type role: str
-        :param role: role to bind.
-
-        :type members: set of str
-        :param members: member IDs to be bound to the role.
-
-        Subclasses may override.
-        """
-        raise ValueError(
-            'Unknown binding: role=%s, members=%s' % (role, members))
-
     @classmethod
     def from_api_repr(cls, resource):
         """Create a policy from the resource returned from the API.
@@ -154,47 +178,10 @@ def from_api_repr(cls, resource):
         policy = cls(etag, version)
         for binding in resource.get('bindings', ()):
             role = binding['role']
-            members = set(binding['members'])
-            if role in cls._OWNER_ROLES:
-                policy.owners |= members
-            elif role in cls._EDITOR_ROLES:
-                policy.editors |= members
-            elif role in cls._VIEWER_ROLES:
-                policy.viewers |= members
-            else:
-                policy._bind_custom_role(role, members)
+            members = sorted(binding['members'])
+            policy.bindings[role] = members
         return policy
 
-    def _role_bindings(self):
-        """Enumerate members bound to roles for the policy.
-
-        Helper for :meth:`to_api_repr`.
-
-        :rtype: list of mapping
-        :returns: zero or more mappings describing roles / members bound by
-                  the policy.
-
-        Subclasses may override.
-        """
-        bindings = []
-
-        if self.owners:
-            bindings.append(
-                {'role': OWNER_ROLE,
-                 'members': sorted(self.owners)})
-
-        if self.editors:
-            bindings.append(
-                {'role': EDITOR_ROLE,
-                 'members': sorted(self.editors)})
-
-        if self.viewers:
-            bindings.append(
-                {'role': VIEWER_ROLE,
-                 'members': sorted(self.viewers)})
-
-        return bindings
-
     def to_api_repr(self):
         """Construct a Policy resource.
 
@@ -209,9 +196,9 @@ def to_api_repr(self):
         if self.version is not None:
             resource['version'] = self.version
 
-        bindings = self._role_bindings()
-
-        if bindings:
-            resource['bindings'] = bindings
+        if len(self.bindings) > 0:
+            resource['bindings'] = [
+                {'role': role, 'members': members}
+                for role, members in sorted(self.bindings.items())]
 
         return resource

core/unit_tests/test_iam.py

@@ -33,6 +33,7 @@ def test_ctor_defaults(self):
         self.assertEqual(list(policy.owners), [])
         self.assertEqual(list(policy.editors), [])
         self.assertEqual(list(policy.viewers), [])
+        self.assertEqual(dict(policy.bindings), {})
 
     def test_ctor_explicit(self):
         VERSION = 17
@@ -43,6 +44,7 @@ def test_ctor_explicit(self):
         self.assertEqual(list(policy.owners), [])
         self.assertEqual(list(policy.editors), [])
         self.assertEqual(list(policy.viewers), [])
+        self.assertEqual(dict(policy.bindings), {})
 
     def test_user(self):
         EMAIL = 'phred@example.com'
@@ -87,6 +89,7 @@ def test_from_api_repr_only_etag(self):
         self.assertEqual(list(policy.owners), [])
         self.assertEqual(list(policy.editors), [])
         self.assertEqual(list(policy.viewers), [])
+        self.assertEqual(dict(policy.bindings), {})
 
     def test_from_api_repr_complete(self):
         from google.cloud.iam import (
@@ -95,8 +98,8 @@ def test_from_api_repr_complete(self):
             VIEWER_ROLE,
         )
 
-        OWNER1 = 'user:phred@example.com'
-        OWNER2 = 'group:cloud-logs@google.com'
+        OWNER1 = 'group:cloud-logs@google.com'
+        OWNER2 = 'user:phred@example.com'
         EDITOR1 = 'domain:google.com'
         EDITOR2 = 'user:phred@example.com'
         VIEWER1 = 'serviceAccount:1234-abcdef@service.example.com'
@@ -114,23 +117,31 @@ def test_from_api_repr_complete(self):
         policy = klass.from_api_repr(RESOURCE)
         self.assertEqual(policy.etag, 'DEADBEEF')
         self.assertEqual(policy.version, 17)
-        self.assertEqual(sorted(policy.owners), [OWNER2, OWNER1])
+        self.assertEqual(sorted(policy.owners), [OWNER1, OWNER2])
         self.assertEqual(sorted(policy.editors), [EDITOR1, EDITOR2])
         self.assertEqual(sorted(policy.viewers), [VIEWER1, VIEWER2])
-
-    def test_from_api_repr_bad_role(self):
-        BOGUS1 = 'user:phred@example.com'
-        BOGUS2 = 'group:cloud-logs@google.com'
+        self.assertEqual(
+            dict(policy.bindings), {
+                OWNER_ROLE: [OWNER1, OWNER2],
+                EDITOR_ROLE: [EDITOR1, EDITOR2],
+                VIEWER_ROLE: [VIEWER1, VIEWER2],
+            })
+
+    def test_from_api_repr_unknown_role(self):
+        USER = 'user:phred@example.com'
+        GROUP = 'group:cloud-logs@google.com'
         RESOURCE = {
             'etag': 'DEADBEEF',
             'version': 17,
             'bindings': [
-                {'role': 'nonesuch', 'members': [BOGUS1, BOGUS2]},
+                {'role': 'unknown', 'members': [USER, GROUP]},
             ],
         }
         klass = self._get_target_class()
-        with self.assertRaises(ValueError):
-            klass.from_api_repr(RESOURCE)
+        policy = klass.from_api_repr(RESOURCE)
+        self.assertEqual(policy.etag, 'DEADBEEF')
+        self.assertEqual(policy.version, 17)
+        self.assertEqual(policy.bindings, {'unknown': [GROUP, USER]})
 
     def test_to_api_repr_defaults(self):
         policy = self._make_one()
@@ -141,6 +152,7 @@ def test_to_api_repr_only_etag(self):
         self.assertEqual(policy.to_api_repr(), {'etag': 'DEADBEEF'})
 
     def test_to_api_repr_full(self):
+        import operator
         from google.cloud.iam import (
             OWNER_ROLE,
             EDITOR_ROLE,
@@ -153,20 +165,18 @@ def test_to_api_repr_full(self):
         EDITOR2 = 'user:phred@example.com'
         VIEWER1 = 'serviceAccount:1234-abcdef@service.example.com'
         VIEWER2 = 'user:phred@example.com'
-        EXPECTED = {
-            'etag': 'DEADBEEF',
-            'version': 17,
-            'bindings': [
-                {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]},
-                {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]},
-                {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]},
-            ],
-        }
+        BINDINGS = [
+            {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]},
+            {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]},
+            {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]},
+        ]
         policy = self._make_one('DEADBEEF', 17)
-        policy.owners.add(OWNER1)
-        policy.owners.add(OWNER2)
-        policy.editors.add(EDITOR1)
-        policy.editors.add(EDITOR2)
-        policy.viewers.add(VIEWER1)
-        policy.viewers.add(VIEWER2)
-        self.assertEqual(policy.to_api_repr(), EXPECTED)
+        policy.owners = [OWNER1, OWNER2]
+        policy.editors = [EDITOR1, EDITOR2]
+        policy.viewers = [VIEWER1, VIEWER2]
+        resource = policy.to_api_repr()
+        self.assertEqual(resource['etag'], 'DEADBEEF')
+        self.assertEqual(resource['version'], 17)
+        key = operator.itemgetter('role')
+        self.assertEqual(
+            sorted(resource['bindings'], key=key), sorted(BINDINGS, key=key))

pubsub/google/cloud/pubsub/iam.py

@@ -17,9 +17,11 @@
 https://cloud.google.com/pubsub/access_control#permissions
 """
 
-from google.cloud.iam import OWNER_ROLE
-from google.cloud.iam import EDITOR_ROLE
-from google.cloud.iam import VIEWER_ROLE
+# pylint: disable=unused-import
+from google.cloud.iam import OWNER_ROLE   # noqa - backward compat
+from google.cloud.iam import EDITOR_ROLE  # noqa - backward compat
+from google.cloud.iam import VIEWER_ROLE  # noqa - backward compat
+# pylint: enable=unused-import
 from google.cloud.iam import Policy as _BasePolicy
 
 # Pubsub-specific IAM roles
@@ -94,76 +96,32 @@ class Policy(_BasePolicy):
     See:
     https://cloud.google.com/pubsub/docs/reference/rest/Shared.Types/Policy
     https://cloud.google.com/pubsub/docs/reference/rest/Shared.Types/Binding
-
-    :type etag: str
-    :param etag: ETag used to identify a unique of the policy
-
-    :type version: int
-    :param version: unique version of the policy
     """
     _OWNER_ROLES = (OWNER_ROLE, PUBSUB_ADMIN_ROLE)
-    _EDITOR_ROLES = (EDITOR_ROLE, PUBSUB_EDITOR_ROLE)
-    _VIEWER_ROLES = (VIEWER_ROLE, PUBSUB_VIEWER_ROLE)
-
-    def __init__(self, etag=None, version=None):
-        super(Policy, self).__init__(etag, version)
-        self.publishers = set()
-        self.subscribers = set()
-
-    def _bind_custom_role(self, role, members):
-        """Bind an API-specific role to members.
-
-        Helper for :meth:`from_api_repr`.
-
-        :type role: str
-        :param role: role to bind.
-
-        :type members: set of str
-        :param members: member IDs to be bound to the role.
-
-        Subclasses may override.
-        """
-        if role == PUBSUB_PUBLISHER_ROLE:
-            self.publishers |= members
-        elif role == PUBSUB_SUBSCRIBER_ROLE:
-            self.subscribers |= members
-        else:
-            super(Policy, self)._bind_custom_role(role, members)
+    """Roles mapped onto our ``owners`` attribute."""
 
-    def _role_bindings(self):
-        """Enumerate members bound to roles for the policy.
-
-        Helper for :meth:`to_api_repr`.
-
-        :rtype: list of mapping
-        :returns: zero or more mappings describing roles / members bound by
-                  the policy.
-        """
-        bindings = []
-
-        if self.owners:
-            bindings.append(
-                {'role': PUBSUB_ADMIN_ROLE,
-                 'members': sorted(self.owners)})
-
-        if self.editors:
-            bindings.append(
-                {'role': PUBSUB_EDITOR_ROLE,
-                 'members': sorted(self.editors)})
-
-        if self.viewers:
-            bindings.append(
-                {'role': PUBSUB_VIEWER_ROLE,
-                 'members': sorted(self.viewers)})
-
-        if self.publishers:
-            bindings.append(
-                {'role': PUBSUB_PUBLISHER_ROLE,
-                 'members': sorted(self.publishers)})
-
-        if self.subscribers:
-            bindings.append(
-                {'role': PUBSUB_SUBSCRIBER_ROLE,
-                 'members': sorted(self.subscribers)})
+    _EDITOR_ROLES = (EDITOR_ROLE, PUBSUB_EDITOR_ROLE)
+    """Roles mapped onto our ``editors`` attribute."""
 
-        return bindings
+    _VIEWER_ROLES = (VIEWER_ROLE, PUBSUB_VIEWER_ROLE)
+    """Roles mapped onto our ``viewers`` attribute."""
+
+    @property
+    def publishers(self):
+        """Legacy access to owner role."""
+        return self.bindings.get(PUBSUB_PUBLISHER_ROLE, ())
+
+    @publishers.setter
+    def publishers(self, value):
+        """Update publishers."""
+        self.bindings[PUBSUB_PUBLISHER_ROLE] = list(value)
+
+    @property
+    def subscribers(self):
+        """Legacy access to owner role."""
+        return self.bindings.get(PUBSUB_SUBSCRIBER_ROLE, ())
+
+    @subscribers.setter
+    def subscribers(self, value):
+        """Update subscribers."""
+        self.bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value)