From ce42baee26036f3c9afa625f4d9bd71846fa7160 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 22 Mar 2017 15:14:35 -0400 Subject: [PATCH 01/10] Factor common IAM policy bits into 'google.cloud.iam'. 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. --- core/google/cloud/iam.py | 217 ++++++++++++++++++++++++++++++ core/unit_tests/test_iam.py | 172 +++++++++++++++++++++++ pubsub/google/cloud/pubsub/iam.py | 154 +++++---------------- pubsub/tests/unit/test_iam.py | 130 ++++-------------- 4 files changed, 445 insertions(+), 228 deletions(-) create mode 100644 core/google/cloud/iam.py create mode 100644 core/unit_tests/test_iam.py diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py new file mode 100644 index 000000000000..1303b3c42625 --- /dev/null +++ b/core/google/cloud/iam.py @@ -0,0 +1,217 @@ +# Copyright 2017 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Non-API-specific IAM policy definitions + +For allowed roles / permissions, see: +https://cloud.google.com/iam/docs/understanding-roles +""" + +# Generic IAM roles + +OWNER_ROLE = 'roles/owner' +"""Generic role implying all rights to an object.""" + +EDITOR_ROLE = 'roles/editor' +"""Generic role implying rights to modify an object.""" + +VIEWER_ROLE = 'roles/viewer' +"""Generic role implying rights to access an object.""" + + +class Policy(object): + """IAM Policy + + See: + https://cloud.google.com/iam/reference/rest/v1/Policy + + :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,) + """Roles mapped onto our ``owners`` attribute.""" + + _EDITOR_ROLES = (EDITOR_ROLE,) + """Roles mapped onto our ``editors`` attribute.""" + + _VIEWER_ROLES = (VIEWER_ROLE,) + """Roles mapped onto our ``viewers`` attribute.""" + + def __init__(self, etag=None, version=None): + self.etag = etag + self.version = version + self.owners = set() + self.editors = set() + self.viewers = set() + + @staticmethod + def user(email): + """Factory method for a user member. + + :type email: str + :param email: E-mail for this particular user. + + :rtype: str + :returns: A member string corresponding to the given user. + """ + return 'user:%s' % (email,) + + @staticmethod + def service_account(email): + """Factory method for a service account member. + + :type email: str + :param email: E-mail for this particular service account. + + :rtype: str + :returns: A member string corresponding to the given service account. + """ + return 'serviceAccount:%s' % (email,) + + @staticmethod + def group(email): + """Factory method for a group member. + + :type email: str + :param email: An id or e-mail for this particular group. + + :rtype: str + :returns: A member string corresponding to the given group. + """ + return 'group:%s' % (email,) + + @staticmethod + def domain(domain): + """Factory method for a domain member. + + :type domain: str + :param domain: The domain for this member. + + :rtype: str + :returns: A member string corresponding to the given domain. + """ + return 'domain:%s' % (domain,) + + @staticmethod + def all_users(): + """Factory method for a member representing all users. + + :rtype: str + :returns: A member string representing all users. + """ + return 'allUsers' + + @staticmethod + def authenticated_users(): + """Factory method for a member representing all authenticated users. + + :rtype: str + :returns: A member string representing all 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. + + :type resource: dict + :param resource: resource returned from the ``getIamPolicy`` API. + + :rtype: :class:`Policy` + :returns: the parsed policy + """ + version = resource.get('version') + etag = resource.get('etag') + 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) + 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. + + :rtype: dict + :returns: a resource to be passed to the ``setIamPolicy`` API. + """ + resource = {} + + if self.etag is not None: + resource['etag'] = self.etag + + if self.version is not None: + resource['version'] = self.version + + bindings = self._role_bindings() + + if bindings: + resource['bindings'] = bindings + + return resource diff --git a/core/unit_tests/test_iam.py b/core/unit_tests/test_iam.py new file mode 100644 index 000000000000..d4d270ad8dc2 --- /dev/null +++ b/core/unit_tests/test_iam.py @@ -0,0 +1,172 @@ +# Copyright 2017 Google Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + + +class TestPolicy(unittest.TestCase): + + @staticmethod + def _get_target_class(): + from google.cloud.iam import Policy + + return Policy + + def _make_one(self, *args, **kw): + return self._get_target_class()(*args, **kw) + + def test_ctor_defaults(self): + policy = self._make_one() + self.assertIsNone(policy.etag) + self.assertIsNone(policy.version) + self.assertEqual(list(policy.owners), []) + self.assertEqual(list(policy.editors), []) + self.assertEqual(list(policy.viewers), []) + + def test_ctor_explicit(self): + VERSION = 17 + ETAG = 'ETAG' + policy = self._make_one(ETAG, VERSION) + self.assertEqual(policy.etag, ETAG) + self.assertEqual(policy.version, VERSION) + self.assertEqual(list(policy.owners), []) + self.assertEqual(list(policy.editors), []) + self.assertEqual(list(policy.viewers), []) + + def test_user(self): + EMAIL = 'phred@example.com' + MEMBER = 'user:%s' % (EMAIL,) + policy = self._make_one() + self.assertEqual(policy.user(EMAIL), MEMBER) + + def test_service_account(self): + EMAIL = 'phred@example.com' + MEMBER = 'serviceAccount:%s' % (EMAIL,) + policy = self._make_one() + self.assertEqual(policy.service_account(EMAIL), MEMBER) + + def test_group(self): + EMAIL = 'phred@example.com' + MEMBER = 'group:%s' % (EMAIL,) + policy = self._make_one() + self.assertEqual(policy.group(EMAIL), MEMBER) + + def test_domain(self): + DOMAIN = 'example.com' + MEMBER = 'domain:%s' % (DOMAIN,) + policy = self._make_one() + self.assertEqual(policy.domain(DOMAIN), MEMBER) + + def test_all_users(self): + policy = self._make_one() + self.assertEqual(policy.all_users(), 'allUsers') + + def test_authenticated_users(self): + policy = self._make_one() + self.assertEqual(policy.authenticated_users(), 'allAuthenticatedUsers') + + def test_from_api_repr_only_etag(self): + RESOURCE = { + 'etag': 'ACAB', + } + klass = self._get_target_class() + policy = klass.from_api_repr(RESOURCE) + self.assertEqual(policy.etag, 'ACAB') + self.assertIsNone(policy.version) + self.assertEqual(list(policy.owners), []) + self.assertEqual(list(policy.editors), []) + self.assertEqual(list(policy.viewers), []) + + def test_from_api_repr_complete(self): + from google.cloud.iam import ( + OWNER_ROLE, + EDITOR_ROLE, + VIEWER_ROLE, + ) + + OWNER1 = 'user:phred@example.com' + OWNER2 = 'group:cloud-logs@google.com' + EDITOR1 = 'domain:google.com' + EDITOR2 = 'user:phred@example.com' + VIEWER1 = 'serviceAccount:1234-abcdef@service.example.com' + VIEWER2 = 'user:phred@example.com' + RESOURCE = { + 'etag': 'DEADBEEF', + 'version': 17, + 'bindings': [ + {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]}, + {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, + {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, + ], + } + klass = self._get_target_class() + 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.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' + RESOURCE = { + 'etag': 'DEADBEEF', + 'version': 17, + 'bindings': [ + {'role': 'nonesuch', 'members': [BOGUS1, BOGUS2]}, + ], + } + klass = self._get_target_class() + with self.assertRaises(ValueError): + klass.from_api_repr(RESOURCE) + + def test_to_api_repr_defaults(self): + policy = self._make_one() + self.assertEqual(policy.to_api_repr(), {}) + + def test_to_api_repr_only_etag(self): + policy = self._make_one('DEADBEEF') + self.assertEqual(policy.to_api_repr(), {'etag': 'DEADBEEF'}) + + def test_to_api_repr_full(self): + from google.cloud.iam import ( + OWNER_ROLE, + EDITOR_ROLE, + VIEWER_ROLE, + ) + + 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' + 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]}, + ], + } + 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) diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index 53c0f36579f3..b650f82486e0 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/pubsub/google/cloud/pubsub/iam.py @@ -17,16 +17,10 @@ https://cloud.google.com/pubsub/access_control#permissions """ -# Generic IAM roles - -OWNER_ROLE = 'roles/owner' -"""Generic role implying all rights to an object.""" - -EDITOR_ROLE = 'roles/editor' -"""Generic role implying rights to modify an object.""" - -VIEWER_ROLE = 'roles/viewer' -"""Generic role implying rights to access an object.""" +from google.cloud.iam import OWNER_ROLE +from google.cloud.iam import EDITOR_ROLE +from google.cloud.iam import VIEWER_ROLE +from google.cloud.iam import Policy as _BasePolicy # Pubsub-specific IAM roles @@ -94,8 +88,8 @@ """Permission: update subscriptions.""" -class Policy(object): - """Combined IAM Policy / Bindings. +class Policy(_BasePolicy): + """IAM Policy / Bindings. See: https://cloud.google.com/pubsub/docs/reference/rest/Shared.Types/Policy @@ -107,125 +101,44 @@ class Policy(object): :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): - self.etag = etag - self.version = version - self.owners = set() - self.editors = set() - self.viewers = set() + super(Policy, self).__init__(etag, version) self.publishers = set() self.subscribers = set() - @staticmethod - def user(email): - """Factory method for a user member. + def _bind_custom_role(self, role, members): + """Bind an API-specific role to members. - :type email: str - :param email: E-mail for this particular user. + Helper for :meth:`from_api_repr`. - :rtype: str - :returns: A member string corresponding to the given user. - """ - return 'user:%s' % (email,) - - @staticmethod - def service_account(email): - """Factory method for a service account member. + :type role: str + :param role: role to bind. - :type email: str - :param email: E-mail for this particular service account. + :type members: set of str + :param members: member IDs to be bound to the role. - :rtype: str - :returns: A member string corresponding to the given service account. + Subclasses may override. """ - return 'serviceAccount:%s' % (email,) - - @staticmethod - def group(email): - """Factory method for a group member. + 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) - :type email: str - :param email: An id or e-mail for this particular group. - - :rtype: str - :returns: A member string corresponding to the given group. - """ - return 'group:%s' % (email,) - - @staticmethod - def domain(domain): - """Factory method for a domain member. - - :type domain: str - :param domain: The domain for this member. - - :rtype: str - :returns: A member string corresponding to the given domain. - """ - return 'domain:%s' % (domain,) + def _role_bindings(self): + """Enumerate members bound to roles for the policy. - @staticmethod - def all_users(): - """Factory method for a member representing all users. + Helper for :meth:`to_api_repr`. - :rtype: str - :returns: A member string representing all users. + :rtype: list of mapping + :returns: zero or more mappings describing roles / members bound by + the policy. """ - return 'allUsers' - - @staticmethod - def authenticated_users(): - """Factory method for a member representing all authenticated users. - - :rtype: str - :returns: A member string representing all authenticated users. - """ - return 'allAuthenticatedUsers' - - @classmethod - def from_api_repr(cls, resource): - """Create a policy from the resource returned from the API. - - :type resource: dict - :param resource: resource returned from the ``getIamPolicy`` API. - - :rtype: :class:`Policy` - :returns: the parsed policy - """ - version = resource.get('version') - etag = resource.get('etag') - policy = cls(etag, version) - for binding in resource.get('bindings', ()): - role = binding['role'] - members = set(binding['members']) - if role in (OWNER_ROLE, PUBSUB_ADMIN_ROLE): - policy.owners |= members - elif role in (EDITOR_ROLE, PUBSUB_EDITOR_ROLE): - policy.editors |= members - elif role in (VIEWER_ROLE, PUBSUB_VIEWER_ROLE): - policy.viewers |= members - elif role == PUBSUB_PUBLISHER_ROLE: - policy.publishers |= members - elif role == PUBSUB_SUBSCRIBER_ROLE: - policy.subscribers |= members - else: - raise ValueError('Unknown role: %s' % (role,)) - return policy - - def to_api_repr(self): - """Construct a Policy resource. - - :rtype: dict - :returns: a resource to be passed to the ``setIamPolicy`` API. - """ - resource = {} - - if self.etag is not None: - resource['etag'] = self.etag - - if self.version is not None: - resource['version'] = self.version - bindings = [] if self.owners: @@ -253,7 +166,4 @@ def to_api_repr(self): {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': sorted(self.subscribers)}) - if bindings: - resource['bindings'] = bindings - - return resource + return bindings diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index 1d73277c270d..394317218486 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -48,111 +48,33 @@ def test_ctor_explicit(self): self.assertEqual(list(policy.publishers), []) self.assertEqual(list(policy.subscribers), []) - def test_user(self): - EMAIL = 'phred@example.com' - MEMBER = 'user:%s' % (EMAIL,) - policy = self._make_one() - self.assertEqual(policy.user(EMAIL), MEMBER) - - def test_service_account(self): - EMAIL = 'phred@example.com' - MEMBER = 'serviceAccount:%s' % (EMAIL,) - policy = self._make_one() - self.assertEqual(policy.service_account(EMAIL), MEMBER) - - def test_group(self): - EMAIL = 'phred@example.com' - MEMBER = 'group:%s' % (EMAIL,) - policy = self._make_one() - self.assertEqual(policy.group(EMAIL), MEMBER) - - def test_domain(self): - DOMAIN = 'example.com' - MEMBER = 'domain:%s' % (DOMAIN,) - policy = self._make_one() - self.assertEqual(policy.domain(DOMAIN), MEMBER) - - def test_all_users(self): - policy = self._make_one() - self.assertEqual(policy.all_users(), 'allUsers') - - def test_authenticated_users(self): + def test__bind_custom_role_publisher(self): + from google.cloud.pubsub.iam import ( + PUBSUB_PUBLISHER_ROLE, + ) + PUBLISHER = 'user:phred@example.com' policy = self._make_one() - self.assertEqual(policy.authenticated_users(), 'allAuthenticatedUsers') + policy._bind_custom_role(PUBSUB_PUBLISHER_ROLE, set([PUBLISHER])) - def test_from_api_repr_only_etag(self): - RESOURCE = { - 'etag': 'ACAB', - } - klass = self._get_target_class() - policy = klass.from_api_repr(RESOURCE) - self.assertEqual(policy.etag, 'ACAB') - self.assertIsNone(policy.version) - self.assertEqual(list(policy.owners), []) - self.assertEqual(list(policy.editors), []) - self.assertEqual(list(policy.viewers), []) + self.assertEqual(sorted(policy.publishers), [PUBLISHER]) - def test_from_api_repr_complete(self): + def test__bind_custom_role_subscriber(self): from google.cloud.pubsub.iam import ( - OWNER_ROLE, - EDITOR_ROLE, - VIEWER_ROLE, - PUBSUB_PUBLISHER_ROLE, PUBSUB_SUBSCRIBER_ROLE, ) - - OWNER1 = 'user:phred@example.com' - OWNER2 = 'group:cloud-logs@google.com' - EDITOR1 = 'domain:google.com' - EDITOR2 = 'user:phred@example.com' - VIEWER1 = 'serviceAccount:1234-abcdef@service.example.com' - VIEWER2 = 'user:phred@example.com' - PUBLISHER = 'user:phred@example.com' SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' - RESOURCE = { - 'etag': 'DEADBEEF', - 'version': 17, - 'bindings': [ - {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]}, - {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, - {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, - {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, - {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': [SUBSCRIBER]}, - ], - } - klass = self._get_target_class() - 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.editors), [EDITOR1, EDITOR2]) - self.assertEqual(sorted(policy.viewers), [VIEWER1, VIEWER2]) - self.assertEqual(sorted(policy.publishers), [PUBLISHER]) - self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) + policy = self._make_one() + policy._bind_custom_role(PUBSUB_SUBSCRIBER_ROLE, set([SUBSCRIBER])) - def test_from_api_repr_bad_role(self): - BOGUS1 = 'user:phred@example.com' - BOGUS2 = 'group:cloud-logs@google.com' - RESOURCE = { - 'etag': 'DEADBEEF', - 'version': 17, - 'bindings': [ - {'role': 'nonesuch', 'members': [BOGUS1, BOGUS2]}, - ], - } - klass = self._get_target_class() - with self.assertRaises(ValueError): - klass.from_api_repr(RESOURCE) + self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) - def test_to_api_repr_defaults(self): + def test__bind_custom_role_unknown(self): policy = self._make_one() - self.assertEqual(policy.to_api_repr(), {}) - - def test_to_api_repr_only_etag(self): - policy = self._make_one('DEADBEEF') - self.assertEqual(policy.to_api_repr(), {'etag': 'DEADBEEF'}) + USER = 'user:phred@example.com' + with self.assertRaises(ValueError): + policy._bind_custom_role('nonesuch', set([USER])) - def test_to_api_repr_full(self): + def test__role_bindings(self): from google.cloud.pubsub.iam import ( PUBSUB_ADMIN_ROLE, PUBSUB_EDITOR_ROLE, @@ -169,17 +91,13 @@ def test_to_api_repr_full(self): VIEWER2 = 'user:phred@example.com' PUBLISHER = 'user:phred@example.com' SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' - EXPECTED = { - 'etag': 'DEADBEEF', - 'version': 17, - 'bindings': [ - {'role': PUBSUB_ADMIN_ROLE, 'members': [OWNER1, OWNER2]}, - {'role': PUBSUB_EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, - {'role': PUBSUB_VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, - {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, - {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': [SUBSCRIBER]}, - ], - } + EXPECTED = [ + {'role': PUBSUB_ADMIN_ROLE, 'members': [OWNER1, OWNER2]}, + {'role': PUBSUB_EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, + {'role': PUBSUB_VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, + {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, + {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': [SUBSCRIBER]}, + ] policy = self._make_one('DEADBEEF', 17) policy.owners.add(OWNER1) policy.owners.add(OWNER2) @@ -189,4 +107,4 @@ def test_to_api_repr_full(self): policy.viewers.add(VIEWER2) policy.publishers.add(PUBLISHER) policy.subscribers.add(SUBSCRIBER) - self.assertEqual(policy.to_api_repr(), EXPECTED) + self.assertEqual(policy._role_bindings(), EXPECTED) From ea63f8cf595dcdb41b049f77a0e68c341ce462b8 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 23 Mar 2017 13:25:44 -0400 Subject: [PATCH 02/10] 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. --- core/google/cloud/iam.py | 111 +++++++++++-------------- core/unit_tests/test_iam.py | 62 ++++++++------ pubsub/google/cloud/pubsub/iam.py | 102 +++++++---------------- pubsub/tests/system.py | 8 +- pubsub/tests/unit/test_iam.py | 53 ++---------- pubsub/tests/unit/test_subscription.py | 36 ++++---- pubsub/tests/unit/test_topic.py | 36 ++++---- 7 files changed, 171 insertions(+), 237 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index 1303b3c42625..452d969e9805 100644 --- a/core/google/cloud/iam.py +++ b/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 diff --git a/core/unit_tests/test_iam.py b/core/unit_tests/test_iam.py index d4d270ad8dc2..c9b62ee0455b 100644 --- a/core/unit_tests/test_iam.py +++ b/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)) diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index b650f82486e0..d7aedc2d3d56 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/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) diff --git a/pubsub/tests/system.py b/pubsub/tests/system.py index 54c4ba7a895c..93ad19b76647 100644 --- a/pubsub/tests/system.py +++ b/pubsub/tests/system.py @@ -252,7 +252,9 @@ def test_topic_iam_policy(self): if topic.check_iam_permissions([PUBSUB_TOPICS_GET_IAM_POLICY]): policy = topic.get_iam_policy() - policy.viewers.add(policy.user('jjg@google.com')) + viewers = set(policy.viewers) + viewers.add(policy.user('jjg@google.com')) + policy.viewers = viewers new_policy = topic.set_iam_policy(policy) self.assertEqual(new_policy.viewers, policy.viewers) @@ -280,6 +282,8 @@ def test_subscription_iam_policy(self): if subscription.check_iam_permissions( [PUBSUB_SUBSCRIPTIONS_GET_IAM_POLICY]): policy = subscription.get_iam_policy() - policy.viewers.add(policy.user('jjg@google.com')) + viewers = set(policy.viewers) + viewers.add(policy.user('jjg@google.com')) + policy.viewers = viewers new_policy = subscription.set_iam_policy(policy) self.assertEqual(new_policy.viewers, policy.viewers) diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index 394317218486..16231a0d73c7 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -48,63 +48,26 @@ def test_ctor_explicit(self): self.assertEqual(list(policy.publishers), []) self.assertEqual(list(policy.subscribers), []) - def test__bind_custom_role_publisher(self): + def test_publishers_setter(self): from google.cloud.pubsub.iam import ( PUBSUB_PUBLISHER_ROLE, ) PUBLISHER = 'user:phred@example.com' policy = self._make_one() - policy._bind_custom_role(PUBSUB_PUBLISHER_ROLE, set([PUBLISHER])) + policy.publishers = [PUBLISHER] self.assertEqual(sorted(policy.publishers), [PUBLISHER]) + self.assertEqual( + policy.bindings, {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]}) - def test__bind_custom_role_subscriber(self): + def test_subscribers_setter(self): from google.cloud.pubsub.iam import ( PUBSUB_SUBSCRIBER_ROLE, ) SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' policy = self._make_one() - policy._bind_custom_role(PUBSUB_SUBSCRIBER_ROLE, set([SUBSCRIBER])) + policy.subscribers = [SUBSCRIBER] self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) - - def test__bind_custom_role_unknown(self): - policy = self._make_one() - USER = 'user:phred@example.com' - with self.assertRaises(ValueError): - policy._bind_custom_role('nonesuch', set([USER])) - - def test__role_bindings(self): - from google.cloud.pubsub.iam import ( - PUBSUB_ADMIN_ROLE, - PUBSUB_EDITOR_ROLE, - PUBSUB_VIEWER_ROLE, - PUBSUB_PUBLISHER_ROLE, - PUBSUB_SUBSCRIBER_ROLE, - ) - - 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' - VIEWER2 = 'user:phred@example.com' - PUBLISHER = 'user:phred@example.com' - SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' - EXPECTED = [ - {'role': PUBSUB_ADMIN_ROLE, 'members': [OWNER1, OWNER2]}, - {'role': PUBSUB_EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, - {'role': PUBSUB_VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, - {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, - {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': [SUBSCRIBER]}, - ] - 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) - policy.publishers.add(PUBLISHER) - policy.subscribers.add(SUBSCRIBER) - self.assertEqual(policy._role_bindings(), EXPECTED) + self.assertEqual( + policy.bindings, {PUBSUB_SUBSCRIBER_ROLE: [SUBSCRIBER]}) diff --git a/pubsub/tests/unit/test_subscription.py b/pubsub/tests/unit/test_subscription.py index 42fb23d9ae68..f16f0ad9126e 100644 --- a/pubsub/tests/unit/test_subscription.py +++ b/pubsub/tests/unit/test_subscription.py @@ -523,11 +523,12 @@ def test_get_iam_policy_w_alternate_client(self): self.assertEqual(api._got_iam_policy, self.SUB_PATH) def test_set_iam_policy_w_bound_client(self): + import operator from google.cloud.pubsub.iam import Policy from google.cloud.pubsub.iam import ( - PUBSUB_ADMIN_ROLE, - PUBSUB_EDITOR_ROLE, - PUBSUB_VIEWER_ROLE, + OWNER_ROLE, + EDITOR_ROLE, + VIEWER_ROLE, PUBSUB_PUBLISHER_ROLE, PUBSUB_SUBSCRIBER_ROLE, ) @@ -544,9 +545,9 @@ def test_set_iam_policy_w_bound_client(self): 'etag': 'DEADBEEF', 'version': 17, 'bindings': [ - {'role': PUBSUB_ADMIN_ROLE, 'members': [OWNER1, OWNER2]}, - {'role': PUBSUB_EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, - {'role': PUBSUB_VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, + {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]}, + {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, + {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, {'role': PUBSUB_SUBSCRIBER_ROLE, 'members': [SUBSCRIBER]}, ], @@ -560,14 +561,11 @@ def test_set_iam_policy_w_bound_client(self): topic = _Topic(self.TOPIC_NAME, client=client) subscription = self._make_one(self.SUB_NAME, topic) policy = Policy('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) - policy.publishers.add(PUBLISHER) - policy.subscribers.add(SUBSCRIBER) + policy.owners = [OWNER1, OWNER2] + policy.editors = [EDITOR1, EDITOR2] + policy.viewers = [VIEWER1, VIEWER2] + policy.publishers = [PUBLISHER] + policy.subscribers = [SUBSCRIBER] new_policy = subscription.set_iam_policy(policy) @@ -578,7 +576,15 @@ def test_set_iam_policy_w_bound_client(self): self.assertEqual(sorted(new_policy.viewers), [VIEWER1, VIEWER2]) self.assertEqual(sorted(new_policy.publishers), [PUBLISHER]) self.assertEqual(sorted(new_policy.subscribers), [SUBSCRIBER]) - self.assertEqual(api._set_iam_policy, (self.SUB_PATH, POLICY)) + self.assertEqual(len(api._set_iam_policy), 2) + self.assertEqual(api._set_iam_policy[0], self.SUB_PATH) + resource = api._set_iam_policy[1] + self.assertEqual(resource['etag'], POLICY['etag']) + self.assertEqual(resource['version'], POLICY['version']) + key = operator.itemgetter('role') + self.assertEqual( + sorted(resource['bindings'], key=key), + sorted(POLICY['bindings'], key=key)) def test_set_iam_policy_w_alternate_client(self): from google.cloud.pubsub.iam import Policy diff --git a/pubsub/tests/unit/test_topic.py b/pubsub/tests/unit/test_topic.py index 01864fa24fdd..2c90432195c2 100644 --- a/pubsub/tests/unit/test_topic.py +++ b/pubsub/tests/unit/test_topic.py @@ -509,11 +509,12 @@ def test_get_iam_policy_w_alternate_client(self): self.assertEqual(api._got_iam_policy, self.TOPIC_PATH) def test_set_iam_policy_w_bound_client(self): + import operator from google.cloud.pubsub.iam import Policy from google.cloud.pubsub.iam import ( - PUBSUB_ADMIN_ROLE, - PUBSUB_EDITOR_ROLE, - PUBSUB_VIEWER_ROLE, + OWNER_ROLE, + EDITOR_ROLE, + VIEWER_ROLE, PUBSUB_PUBLISHER_ROLE, PUBSUB_SUBSCRIBER_ROLE, ) @@ -530,11 +531,11 @@ def test_set_iam_policy_w_bound_client(self): 'etag': 'DEADBEEF', 'version': 17, 'bindings': [ - {'role': PUBSUB_ADMIN_ROLE, + {'role': OWNER_ROLE, 'members': [OWNER1, OWNER2]}, - {'role': PUBSUB_EDITOR_ROLE, + {'role': EDITOR_ROLE, 'members': [EDITOR1, EDITOR2]}, - {'role': PUBSUB_VIEWER_ROLE, + {'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]}, {'role': PUBSUB_PUBLISHER_ROLE, 'members': [PUBLISHER]}, @@ -551,14 +552,11 @@ def test_set_iam_policy_w_bound_client(self): api._set_iam_policy_response = RESPONSE topic = self._make_one(self.TOPIC_NAME, client=client) policy = Policy('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) - policy.publishers.add(PUBLISHER) - policy.subscribers.add(SUBSCRIBER) + policy.owners = [OWNER1, OWNER2] + policy.editors = [EDITOR1, EDITOR2] + policy.viewers = [VIEWER1, VIEWER2] + policy.publishers = [PUBLISHER] + policy.subscribers = [SUBSCRIBER] new_policy = topic.set_iam_policy(policy) @@ -569,7 +567,15 @@ def test_set_iam_policy_w_bound_client(self): self.assertEqual(sorted(new_policy.viewers), [VIEWER1, VIEWER2]) self.assertEqual(sorted(new_policy.publishers), [PUBLISHER]) self.assertEqual(sorted(new_policy.subscribers), [SUBSCRIBER]) - self.assertEqual(api._set_iam_policy, (self.TOPIC_PATH, POLICY)) + self.assertEqual(len(api._set_iam_policy), 2) + self.assertEqual(api._set_iam_policy[0], self.TOPIC_PATH) + resource = api._set_iam_policy[1] + self.assertEqual(resource['etag'], POLICY['etag']) + self.assertEqual(resource['version'], POLICY['version']) + key = operator.itemgetter('role') + self.assertEqual( + sorted(resource['bindings'], key=key), + sorted(POLICY['bindings'], key=key)) def test_set_iam_policy_w_alternate_client(self): from google.cloud.pubsub.iam import Policy From 743c5d514da9682f3f7792c36ec85fa10f8decd8 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 23 Mar 2017 14:45:58 -0400 Subject: [PATCH 03/10] Add API documentation for 'google.cloud.iam'. --- docs/google-cloud-api.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/google-cloud-api.rst b/docs/google-cloud-api.rst index bb2fd2842e9f..195a79c5abb2 100644 --- a/docs/google-cloud-api.rst +++ b/docs/google-cloud-api.rst @@ -29,3 +29,10 @@ Environment Variables .. automodule:: google.cloud.environment_vars :members: :show-inheritance: + +IAM Support +~~~~~~~~~~~ + +.. automodule:: google.cloud.iam + :members: + :show-inheritance: From a01dae40749524e79375d47d8d2097b5090f63b9 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 23 Mar 2017 14:59:10 -0400 Subject: [PATCH 04/10] Address review: - Don't pass roles w/ empty members to back-end. - De-duplicate role members when passing to back-end. --- core/google/cloud/iam.py | 11 ++++++++--- core/unit_tests/test_iam.py | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index 452d969e9805..e19928ff6b34 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -197,8 +197,13 @@ def to_api_repr(self): resource['version'] = self.version if len(self.bindings) > 0: - resource['bindings'] = [ - {'role': role, 'members': members} - for role, members in sorted(self.bindings.items())] + bindings = resource['bindings'] = [] + for role, members in sorted(self.bindings.items()): + if len(members) > 0: + bindings.append( + {'role': role, 'members': sorted(set(members))}) + + if len(bindings) == 0: + del resource['bindings'] return resource diff --git a/core/unit_tests/test_iam.py b/core/unit_tests/test_iam.py index c9b62ee0455b..be3719de464c 100644 --- a/core/unit_tests/test_iam.py +++ b/core/unit_tests/test_iam.py @@ -151,6 +151,22 @@ def test_to_api_repr_only_etag(self): policy = self._make_one('DEADBEEF') self.assertEqual(policy.to_api_repr(), {'etag': 'DEADBEEF'}) + def test_to_api_repr_binding_wo_members(self): + policy = self._make_one() + policy.bindings['empty'] = [] + self.assertEqual(policy.to_api_repr(), {}) + + def test_to_api_repr_binding_w_duplicates(self): + from google.cloud.iam import OWNER_ROLE + + OWNER = 'group:cloud-logs@google.com' + policy = self._make_one() + policy.owners = [OWNER, OWNER] + self.assertEqual( + policy.to_api_repr(), { + 'bindings': [{'role': OWNER_ROLE, 'members': [OWNER]}], + }) + def test_to_api_repr_full(self): import operator from google.cloud.iam import ( From cc7c7c74b5b9776ae3a86149e75faba9d11b15d0 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 23 Mar 2017 15:08:23 -0400 Subject: [PATCH 05/10] Update pubsub snipets to accomodate changed semantics. - Re-assign 'policy.viewers'/'policy.editors', rather than mutating them in place. --- docs/pubsub_snippets.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/pubsub_snippets.py b/docs/pubsub_snippets.py index 7255694d15f1..8584dedcd34d 100644 --- a/docs/pubsub_snippets.py +++ b/docs/pubsub_snippets.py @@ -124,9 +124,9 @@ def topic_iam_policy(client, to_delete): # [START topic_set_iam_policy] ALL_USERS = policy.all_users() - policy.viewers.add(ALL_USERS) + policy.viewers = [ALL_USERS] LOGS_GROUP = policy.group('cloud-logs@google.com') - policy.editors.add(LOGS_GROUP) + policy.editors = [LOGS_GROUP] new_policy = topic.set_iam_policy(policy) # API request # [END topic_set_iam_policy] @@ -395,9 +395,9 @@ def subscription_iam_policy(client, to_delete): # [START subscription_set_iam_policy] ALL_USERS = policy.all_users() - policy.viewers.add(ALL_USERS) + policy.viewers = [ALL_USERS] LOGS_GROUP = policy.group('cloud-logs@google.com') - policy.editors.add(LOGS_GROUP) + policy.editors = [LOGS_GROUP] new_policy = subscription.set_iam_policy(policy) # API request # [END subscription_set_iam_policy] From 38f8976b6c5d1fda03ae61f19c1eb599e809b049 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 5 Apr 2017 17:42:20 -0400 Subject: [PATCH 06/10] Accomodate noxification. --- core/{unit_tests => tests/unit}/test_iam.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename core/{unit_tests => tests/unit}/test_iam.py (100%) diff --git a/core/unit_tests/test_iam.py b/core/tests/unit/test_iam.py similarity index 100% rename from core/unit_tests/test_iam.py rename to core/tests/unit/test_iam.py From 2196802e18f9f2e145e7cc8ba45a43bc450e840a Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Wed, 5 Apr 2017 19:43:07 -0400 Subject: [PATCH 07/10] Return frozensets from named Policy properties. 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. --- core/google/cloud/iam.py | 6 +++--- core/tests/unit/test_iam.py | 3 +++ pubsub/google/cloud/pubsub/iam.py | 4 ++-- pubsub/tests/unit/test_iam.py | 5 +++++ 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index e19928ff6b34..ce2d3e445ba0 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -62,7 +62,7 @@ def owners(self): for role in self._OWNER_ROLES: for member in self.bindings.get(role, ()): result.add(member) - return result + return frozenset(result) @owners.setter def owners(self, value): @@ -76,7 +76,7 @@ def editors(self): for role in self._EDITOR_ROLES: for member in self.bindings.get(role, ()): result.add(member) - return result + return frozenset(result) @editors.setter def editors(self, value): @@ -90,7 +90,7 @@ def viewers(self): for role in self._VIEWER_ROLES: for member in self.bindings.get(role, ()): result.add(member) - return result + return frozenset(result) @viewers.setter def viewers(self, value): diff --git a/core/tests/unit/test_iam.py b/core/tests/unit/test_iam.py index be3719de464c..425e3572bc34 100644 --- a/core/tests/unit/test_iam.py +++ b/core/tests/unit/test_iam.py @@ -30,8 +30,11 @@ def test_ctor_defaults(self): policy = self._make_one() self.assertIsNone(policy.etag) self.assertIsNone(policy.version) + self.assertIsInstance(policy.owners, frozenset) self.assertEqual(list(policy.owners), []) + self.assertIsInstance(policy.editors, frozenset) self.assertEqual(list(policy.editors), []) + self.assertIsInstance(policy.viewers, frozenset) self.assertEqual(list(policy.viewers), []) self.assertEqual(dict(policy.bindings), {}) diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index d7aedc2d3d56..b0093d263871 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/pubsub/google/cloud/pubsub/iam.py @@ -109,7 +109,7 @@ class Policy(_BasePolicy): @property def publishers(self): """Legacy access to owner role.""" - return self.bindings.get(PUBSUB_PUBLISHER_ROLE, ()) + return frozenset(self.bindings.get(PUBSUB_PUBLISHER_ROLE, ())) @publishers.setter def publishers(self, value): @@ -119,7 +119,7 @@ def publishers(self, value): @property def subscribers(self): """Legacy access to owner role.""" - return self.bindings.get(PUBSUB_SUBSCRIBER_ROLE, ()) + return frozenset(self.bindings.get(PUBSUB_SUBSCRIBER_ROLE, ())) @subscribers.setter def subscribers(self, value): diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index 16231a0d73c7..cdb750fd2573 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -30,10 +30,15 @@ def test_ctor_defaults(self): policy = self._make_one() self.assertIsNone(policy.etag) self.assertIsNone(policy.version) + self.assertIsInstance(policy.owners, frozenset) self.assertEqual(list(policy.owners), []) + self.assertIsInstance(policy.editors, frozenset) self.assertEqual(list(policy.editors), []) + self.assertIsInstance(policy.viewers, frozenset) self.assertEqual(list(policy.viewers), []) + self.assertIsInstance(policy.publishers, frozenset) self.assertEqual(list(policy.publishers), []) + self.assertIsInstance(policy.subscribers, frozenset) self.assertEqual(list(policy.subscribers), []) def test_ctor_explicit(self): From 77ed12b55dfb1b9cc838668366927fdb680646c4 Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 6 Apr 2017 11:49:10 -0400 Subject: [PATCH 08/10] Make IAM Policy objects dict-line. Keys are roles, values are lists of principals. --- core/google/cloud/iam.py | 42 +++++++++++++++++++++++-------- core/tests/unit/test_iam.py | 39 +++++++++++++++++++++++----- pubsub/google/cloud/pubsub/iam.py | 8 +++--- pubsub/tests/unit/test_iam.py | 4 +-- 4 files changed, 70 insertions(+), 23 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index ce2d3e445ba0..d2987986123b 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -17,6 +17,8 @@ https://cloud.google.com/iam/docs/understanding-roles """ +import collections + # Generic IAM roles OWNER_ROLE = 'roles/owner' @@ -29,7 +31,7 @@ """Generic role implying rights to access an object.""" -class Policy(object): +class Policy(collections.MutableMapping): """IAM Policy See: @@ -53,49 +55,64 @@ class Policy(object): def __init__(self, etag=None, version=None): self.etag = etag self.version = version - self.bindings = {} + self._bindings = {} + + def __iter__(self): + return iter(self._bindings) + + def __len__(self): + return len(self._bindings) + + def __getitem__(self, key): + return self._bindings[key] + + def __setitem__(self, key, value): + self._bindings[key] = value + + def __delitem__(self, key): + del self._bindings[key] @property def owners(self): """Legacy access to owner role.""" result = set() for role in self._OWNER_ROLES: - for member in self.bindings.get(role, ()): + for member in self._bindings.get(role, ()): result.add(member) return frozenset(result) @owners.setter def owners(self, value): """Update owners.""" - self.bindings[OWNER_ROLE] = list(value) + 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, ()): + for member in self._bindings.get(role, ()): result.add(member) return frozenset(result) @editors.setter def editors(self, value): """Update editors.""" - self.bindings[EDITOR_ROLE] = list(value) + 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, ()): + for member in self._bindings.get(role, ()): result.add(member) return frozenset(result) @viewers.setter def viewers(self, value): """Update viewers.""" - self.bindings[VIEWER_ROLE] = list(value) + self._bindings[VIEWER_ROLE] = list(value) @staticmethod def user(email): @@ -179,7 +196,7 @@ def from_api_repr(cls, resource): for binding in resource.get('bindings', ()): role = binding['role'] members = sorted(binding['members']) - policy.bindings[role] = members + policy._bindings[role] = members return policy def to_api_repr(self): @@ -196,9 +213,9 @@ def to_api_repr(self): if self.version is not None: resource['version'] = self.version - if len(self.bindings) > 0: + if len(self._bindings) > 0: bindings = resource['bindings'] = [] - for role, members in sorted(self.bindings.items()): + for role, members in sorted(self._bindings.items()): if len(members) > 0: bindings.append( {'role': role, 'members': sorted(set(members))}) @@ -207,3 +224,6 @@ def to_api_repr(self): del resource['bindings'] return resource + + +collections.MutableMapping.register(Policy) diff --git a/core/tests/unit/test_iam.py b/core/tests/unit/test_iam.py index 425e3572bc34..f2b162143b4d 100644 --- a/core/tests/unit/test_iam.py +++ b/core/tests/unit/test_iam.py @@ -36,7 +36,8 @@ def test_ctor_defaults(self): self.assertEqual(list(policy.editors), []) self.assertIsInstance(policy.viewers, frozenset) self.assertEqual(list(policy.viewers), []) - self.assertEqual(dict(policy.bindings), {}) + self.assertEqual(len(policy), 0) + self.assertEqual(dict(policy), {}) def test_ctor_explicit(self): VERSION = 17 @@ -47,7 +48,33 @@ 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), {}) + self.assertEqual(len(policy), 0) + self.assertEqual(dict(policy), {}) + + def test___getitem___miss(self): + policy = self._make_one() + with self.assertRaises(KeyError): + policy['nonesuch'] + + def test___setitem__(self): + USER = 'user:phred@example.com' + policy = self._make_one() + policy['rolename'] = [USER] + self.assertEqual(policy['rolename'], [USER]) + self.assertEqual(len(policy), 1) + self.assertEqual(dict(policy), {'rolename': [USER]}) + + def test___delitem___hit(self): + policy = self._make_one() + policy._bindings['rolename'] = ['phred@example.com'] + del policy['rolename'] + self.assertEqual(len(policy), 0) + self.assertEqual(dict(policy), {}) + + def test___delitem___miss(self): + policy = self._make_one() + with self.assertRaises(KeyError): + del policy['nonesuch'] def test_user(self): EMAIL = 'phred@example.com' @@ -92,7 +119,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), {}) + self.assertEqual(dict(policy), {}) def test_from_api_repr_complete(self): from google.cloud.iam import ( @@ -124,7 +151,7 @@ def test_from_api_repr_complete(self): self.assertEqual(sorted(policy.editors), [EDITOR1, EDITOR2]) self.assertEqual(sorted(policy.viewers), [VIEWER1, VIEWER2]) self.assertEqual( - dict(policy.bindings), { + dict(policy), { OWNER_ROLE: [OWNER1, OWNER2], EDITOR_ROLE: [EDITOR1, EDITOR2], VIEWER_ROLE: [VIEWER1, VIEWER2], @@ -144,7 +171,7 @@ def test_from_api_repr_unknown_role(self): policy = klass.from_api_repr(RESOURCE) self.assertEqual(policy.etag, 'DEADBEEF') self.assertEqual(policy.version, 17) - self.assertEqual(policy.bindings, {'unknown': [GROUP, USER]}) + self.assertEqual(dict(policy), {'unknown': [GROUP, USER]}) def test_to_api_repr_defaults(self): policy = self._make_one() @@ -156,7 +183,7 @@ def test_to_api_repr_only_etag(self): def test_to_api_repr_binding_wo_members(self): policy = self._make_one() - policy.bindings['empty'] = [] + policy['empty'] = [] self.assertEqual(policy.to_api_repr(), {}) def test_to_api_repr_binding_w_duplicates(self): diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index b0093d263871..704da83b3d6a 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/pubsub/google/cloud/pubsub/iam.py @@ -109,19 +109,19 @@ class Policy(_BasePolicy): @property def publishers(self): """Legacy access to owner role.""" - return frozenset(self.bindings.get(PUBSUB_PUBLISHER_ROLE, ())) + return frozenset(self._bindings.get(PUBSUB_PUBLISHER_ROLE, ())) @publishers.setter def publishers(self, value): """Update publishers.""" - self.bindings[PUBSUB_PUBLISHER_ROLE] = list(value) + self._bindings[PUBSUB_PUBLISHER_ROLE] = list(value) @property def subscribers(self): """Legacy access to owner role.""" - return frozenset(self.bindings.get(PUBSUB_SUBSCRIBER_ROLE, ())) + return frozenset(self._bindings.get(PUBSUB_SUBSCRIBER_ROLE, ())) @subscribers.setter def subscribers(self, value): """Update subscribers.""" - self.bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value) + self._bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value) diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index cdb750fd2573..4b7b63259186 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -63,7 +63,7 @@ def test_publishers_setter(self): self.assertEqual(sorted(policy.publishers), [PUBLISHER]) self.assertEqual( - policy.bindings, {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]}) + dict(policy), {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]}) def test_subscribers_setter(self): from google.cloud.pubsub.iam import ( @@ -75,4 +75,4 @@ def test_subscribers_setter(self): self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) self.assertEqual( - policy.bindings, {PUBSUB_SUBSCRIBER_ROLE: [SUBSCRIBER]}) + dict(policy), {PUBSUB_SUBSCRIBER_ROLE: [SUBSCRIBER]}) From e7654b6b19bbd2b7ae42afa1c2d7f6b10c9532ef Mon Sep 17 00:00:00 2001 From: Tres Seaver Date: Thu, 6 Apr 2017 12:27:21 -0400 Subject: [PATCH 09/10] Deprecate assignment to legacy role attributes. --- core/google/cloud/iam.py | 13 ++++++++ core/tests/unit/test_iam.py | 52 +++++++++++++++++++++++++++++++ pubsub/google/cloud/pubsub/iam.py | 11 +++++++ pubsub/tests/unit/test_iam.py | 8 +++-- 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index d2987986123b..7aa34fa83f67 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -18,6 +18,7 @@ """ import collections +import warnings # Generic IAM roles @@ -30,6 +31,9 @@ VIEWER_ROLE = 'roles/viewer' """Generic role implying rights to access an object.""" +_ASSIGNMENT_DEPRECATED_MSG = """\ +Assigning to '{}' is deprecated. Replace with 'policy[{}] = members.""" + class Policy(collections.MutableMapping): """IAM Policy @@ -84,6 +88,9 @@ def owners(self): @owners.setter def owners(self, value): """Update owners.""" + warnings.warn( + _ASSIGNMENT_DEPRECATED_MSG.format('owners', OWNER_ROLE), + DeprecationWarning) self._bindings[OWNER_ROLE] = list(value) @property @@ -98,6 +105,9 @@ def editors(self): @editors.setter def editors(self, value): """Update editors.""" + warnings.warn( + _ASSIGNMENT_DEPRECATED_MSG.format('editors', EDITOR_ROLE), + DeprecationWarning) self._bindings[EDITOR_ROLE] = list(value) @property @@ -112,6 +122,9 @@ def viewers(self): @viewers.setter def viewers(self, value): """Update viewers.""" + warnings.warn( + _ASSIGNMENT_DEPRECATED_MSG.format('viewers', VIEWER_ROLE), + DeprecationWarning) self._bindings[VIEWER_ROLE] = list(value) @staticmethod diff --git a/core/tests/unit/test_iam.py b/core/tests/unit/test_iam.py index f2b162143b4d..888f6968be3c 100644 --- a/core/tests/unit/test_iam.py +++ b/core/tests/unit/test_iam.py @@ -76,6 +76,58 @@ def test___delitem___miss(self): with self.assertRaises(KeyError): del policy['nonesuch'] + def test_owners_getter(self): + from google.cloud.iam import OWNER_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + policy[OWNER_ROLE] = [MEMBER] + self.assertIsInstance(policy.owners, frozenset) + self.assertEqual(list(policy.owners), [MEMBER]) + + def test_owners_setter(self): + import warnings + from google.cloud.iam import OWNER_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + with warnings.catch_warnings(): + policy.owners = [MEMBER] + self.assertEqual(list(policy[OWNER_ROLE]), [MEMBER]) + + def test_editors_getter(self): + from google.cloud.iam import EDITOR_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + policy[EDITOR_ROLE] = [MEMBER] + self.assertIsInstance(policy.editors, frozenset) + self.assertEqual(list(policy.editors), [MEMBER]) + + def test_editors_setter(self): + import warnings + from google.cloud.iam import EDITOR_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + with warnings.catch_warnings(): + policy.editors = [MEMBER] + self.assertEqual(list(policy[EDITOR_ROLE]), [MEMBER]) + + def test_viewers_getter(self): + from google.cloud.iam import VIEWER_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + policy[VIEWER_ROLE] = [MEMBER] + self.assertIsInstance(policy.viewers, frozenset) + self.assertEqual(list(policy.viewers), [MEMBER]) + + def test_viewers_setter(self): + import warnings + from google.cloud.iam import VIEWER_ROLE + MEMBER = 'user:phred@example.com' + policy = self._make_one() + with warnings.catch_warnings(): + warnings.simplefilter('always') + policy.viewers = [MEMBER] + self.assertEqual(list(policy[VIEWER_ROLE]), [MEMBER]) + def test_user(self): EMAIL = 'phred@example.com' MEMBER = 'user:%s' % (EMAIL,) diff --git a/pubsub/google/cloud/pubsub/iam.py b/pubsub/google/cloud/pubsub/iam.py index 704da83b3d6a..e92f2151dc05 100644 --- a/pubsub/google/cloud/pubsub/iam.py +++ b/pubsub/google/cloud/pubsub/iam.py @@ -17,12 +17,15 @@ https://cloud.google.com/pubsub/access_control#permissions """ +import warnings + # 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 +from google.cloud.iam import _ASSIGNMENT_DEPRECATED_MSG # Pubsub-specific IAM roles @@ -114,6 +117,10 @@ def publishers(self): @publishers.setter def publishers(self, value): """Update publishers.""" + warnings.warn( + _ASSIGNMENT_DEPRECATED_MSG.format( + 'publishers', PUBSUB_PUBLISHER_ROLE), + DeprecationWarning) self._bindings[PUBSUB_PUBLISHER_ROLE] = list(value) @property @@ -124,4 +131,8 @@ def subscribers(self): @subscribers.setter def subscribers(self, value): """Update subscribers.""" + warnings.warn( + _ASSIGNMENT_DEPRECATED_MSG.format( + 'subscribers', PUBSUB_SUBSCRIBER_ROLE), + DeprecationWarning) self._bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value) diff --git a/pubsub/tests/unit/test_iam.py b/pubsub/tests/unit/test_iam.py index 4b7b63259186..3bf4aaa922f0 100644 --- a/pubsub/tests/unit/test_iam.py +++ b/pubsub/tests/unit/test_iam.py @@ -54,24 +54,28 @@ def test_ctor_explicit(self): self.assertEqual(list(policy.subscribers), []) def test_publishers_setter(self): + import warnings from google.cloud.pubsub.iam import ( PUBSUB_PUBLISHER_ROLE, ) PUBLISHER = 'user:phred@example.com' policy = self._make_one() - policy.publishers = [PUBLISHER] + with warnings.catch_warnings(): + policy.publishers = [PUBLISHER] self.assertEqual(sorted(policy.publishers), [PUBLISHER]) self.assertEqual( dict(policy), {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]}) def test_subscribers_setter(self): + import warnings from google.cloud.pubsub.iam import ( PUBSUB_SUBSCRIBER_ROLE, ) SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com' policy = self._make_one() - policy.subscribers = [SUBSCRIBER] + with warnings.catch_warnings(): + policy.subscribers = [SUBSCRIBER] self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER]) self.assertEqual( From cf4e4a909166b1fd14bab2b3d6bcf746b37a71d3 Mon Sep 17 00:00:00 2001 From: Luke Sneeringer Date: Wed, 12 Apr 2017 15:11:56 -0700 Subject: [PATCH 10/10] Coerce role-principal values to frozenset. --- core/google/cloud/iam.py | 2 +- core/tests/unit/test_iam.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/core/google/cloud/iam.py b/core/google/cloud/iam.py index 7aa34fa83f67..4747b39bbf07 100644 --- a/core/google/cloud/iam.py +++ b/core/google/cloud/iam.py @@ -71,7 +71,7 @@ def __getitem__(self, key): return self._bindings[key] def __setitem__(self, key, value): - self._bindings[key] = value + self._bindings[key] = frozenset(value) def __delitem__(self, key): del self._bindings[key] diff --git a/core/tests/unit/test_iam.py b/core/tests/unit/test_iam.py index 888f6968be3c..9f1bd9b3904f 100644 --- a/core/tests/unit/test_iam.py +++ b/core/tests/unit/test_iam.py @@ -58,11 +58,12 @@ def test___getitem___miss(self): def test___setitem__(self): USER = 'user:phred@example.com' + PRINCIPALS = frozenset([USER]) policy = self._make_one() policy['rolename'] = [USER] - self.assertEqual(policy['rolename'], [USER]) + self.assertEqual(policy['rolename'], PRINCIPALS) self.assertEqual(len(policy), 1) - self.assertEqual(dict(policy), {'rolename': [USER]}) + self.assertEqual(dict(policy), {'rolename': PRINCIPALS}) def test___delitem___hit(self): policy = self._make_one()