Skip to content

Commit

Permalink
Store policy bindings as sets, not frozensets. (#3308)
Browse files Browse the repository at this point in the history 
The legacy accessors still return frozensets, as they cannot safely be
mutated in plcae.
  • Loading branch information
@tseaver @lukesneeringer
tseaver authored and lukesneeringer committed Apr 19, 2017
1 parent 331e616 commit 1633962
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 55 deletions.
10 changes: 5 additions & 5 deletions core/google/cloud/iam.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def __getitem__(self, key):
return self._bindings[key]

def __setitem__(self, key, value):
self._bindings[key] = frozenset(value)
self._bindings[key] = set(value)

def __delitem__(self, key):
del self._bindings[key]
Expand All @@ -91,7 +91,7 @@ def owners(self, value):
warnings.warn(
_ASSIGNMENT_DEPRECATED_MSG.format('owners', OWNER_ROLE),
DeprecationWarning)
self._bindings[OWNER_ROLE] = list(value)
self[OWNER_ROLE] = value

@property
def editors(self):
Expand All @@ -108,7 +108,7 @@ def editors(self, value):
warnings.warn(
_ASSIGNMENT_DEPRECATED_MSG.format('editors', EDITOR_ROLE),
DeprecationWarning)
self._bindings[EDITOR_ROLE] = list(value)
self[EDITOR_ROLE] = value

@property
def viewers(self):
Expand All @@ -125,7 +125,7 @@ def viewers(self, value):
warnings.warn(
_ASSIGNMENT_DEPRECATED_MSG.format('viewers', VIEWER_ROLE),
DeprecationWarning)
self._bindings[VIEWER_ROLE] = list(value)
self[VIEWER_ROLE] = value

@staticmethod
def user(email):
Expand Down Expand Up @@ -209,7 +209,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[role] = members
return policy

def to_api_repr(self):
Expand Down
64 changes: 35 additions & 29 deletions core/tests/unit/test_iam.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,26 @@ def _make_one(self, *args, **kw):
return self._get_target_class()(*args, **kw)

def test_ctor_defaults(self):
empty = frozenset()
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(policy.owners, empty)
self.assertEqual(policy.editors, empty)
self.assertEqual(policy.viewers, empty)
self.assertEqual(len(policy), 0)
self.assertEqual(dict(policy), {})

def test_ctor_explicit(self):
VERSION = 17
ETAG = 'ETAG'
empty = frozenset()
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), [])
self.assertEqual(policy.owners, empty)
self.assertEqual(policy.editors, empty)
self.assertEqual(policy.viewers, empty)
self.assertEqual(len(policy), 0)
self.assertEqual(dict(policy), {})

Expand All @@ -58,7 +57,7 @@ def test___getitem___miss(self):

def test___setitem__(self):
USER = 'user:phred@example.com'
PRINCIPALS = frozenset([USER])
PRINCIPALS = set([USER])
policy = self._make_one()
policy['rolename'] = [USER]
self.assertEqual(policy['rolename'], PRINCIPALS)
Expand All @@ -80,54 +79,59 @@ def test___delitem___miss(self):
def test_owners_getter(self):
from google.cloud.iam import OWNER_ROLE
MEMBER = 'user:phred@example.com'
expected = frozenset([MEMBER])
policy = self._make_one()
policy[OWNER_ROLE] = [MEMBER]
self.assertIsInstance(policy.owners, frozenset)
self.assertEqual(list(policy.owners), [MEMBER])
self.assertEqual(policy.owners, expected)

def test_owners_setter(self):
import warnings
from google.cloud.iam import OWNER_ROLE
MEMBER = 'user:phred@example.com'
expected = set([MEMBER])
policy = self._make_one()
with warnings.catch_warnings():
warnings.simplefilter('always')
policy.owners = [MEMBER]
self.assertEqual(list(policy[OWNER_ROLE]), [MEMBER])
self.assertEqual(policy[OWNER_ROLE], expected)

def test_editors_getter(self):
from google.cloud.iam import EDITOR_ROLE
MEMBER = 'user:phred@example.com'
expected = frozenset([MEMBER])
policy = self._make_one()
policy[EDITOR_ROLE] = [MEMBER]
self.assertIsInstance(policy.editors, frozenset)
self.assertEqual(list(policy.editors), [MEMBER])
self.assertEqual(policy.editors, expected)

def test_editors_setter(self):
import warnings
from google.cloud.iam import EDITOR_ROLE
MEMBER = 'user:phred@example.com'
expected = set([MEMBER])
policy = self._make_one()
with warnings.catch_warnings():
warnings.simplefilter('always')
policy.editors = [MEMBER]
self.assertEqual(list(policy[EDITOR_ROLE]), [MEMBER])
self.assertEqual(policy[EDITOR_ROLE], expected)

def test_viewers_getter(self):
from google.cloud.iam import VIEWER_ROLE
MEMBER = 'user:phred@example.com'
expected = frozenset([MEMBER])
policy = self._make_one()
policy[VIEWER_ROLE] = [MEMBER]
self.assertIsInstance(policy.viewers, frozenset)
self.assertEqual(list(policy.viewers), [MEMBER])
self.assertEqual(policy.viewers, expected)

def test_viewers_setter(self):
import warnings
from google.cloud.iam import VIEWER_ROLE
MEMBER = 'user:phred@example.com'
expected = set([MEMBER])
policy = self._make_one()
with warnings.catch_warnings():
warnings.simplefilter('always')
policy.viewers = [MEMBER]
self.assertEqual(list(policy[VIEWER_ROLE]), [MEMBER])
self.assertEqual(policy[VIEWER_ROLE], expected)

def test_user(self):
EMAIL = 'phred@example.com'
Expand Down Expand Up @@ -162,16 +166,17 @@ def test_authenticated_users(self):
self.assertEqual(policy.authenticated_users(), 'allAuthenticatedUsers')

def test_from_api_repr_only_etag(self):
empty = frozenset()
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(policy.owners, empty)
self.assertEqual(policy.editors, empty)
self.assertEqual(policy.viewers, empty)
self.assertEqual(dict(policy), {})

def test_from_api_repr_complete(self):
Expand All @@ -196,18 +201,19 @@ def test_from_api_repr_complete(self):
{'role': VIEWER_ROLE, 'members': [VIEWER1, VIEWER2]},
],
}
empty = frozenset()
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), [OWNER1, OWNER2])
self.assertEqual(sorted(policy.editors), [EDITOR1, EDITOR2])
self.assertEqual(sorted(policy.viewers), [VIEWER1, VIEWER2])
self.assertEqual(policy.owners, frozenset([OWNER1, OWNER2]))
self.assertEqual(policy.editors, frozenset([EDITOR1, EDITOR2]))
self.assertEqual(policy.viewers, frozenset([VIEWER1, VIEWER2]))
self.assertEqual(
dict(policy), {
OWNER_ROLE: [OWNER1, OWNER2],
EDITOR_ROLE: [EDITOR1, EDITOR2],
VIEWER_ROLE: [VIEWER1, VIEWER2],
OWNER_ROLE: set([OWNER1, OWNER2]),
EDITOR_ROLE: set([EDITOR1, EDITOR2]),
VIEWER_ROLE: set([VIEWER1, VIEWER2]),
})

def test_from_api_repr_unknown_role(self):
Expand All @@ -224,7 +230,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(dict(policy), {'unknown': [GROUP, USER]})
self.assertEqual(dict(policy), {'unknown': set([GROUP, USER])})

def test_to_api_repr_defaults(self):
policy = self._make_one()
Expand Down
4 changes: 2 additions & 2 deletions pubsub/google/cloud/pubsub/iam.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def publishers(self, value):
_ASSIGNMENT_DEPRECATED_MSG.format(
'publishers', PUBSUB_PUBLISHER_ROLE),
DeprecationWarning)
self._bindings[PUBSUB_PUBLISHER_ROLE] = list(value)
self[PUBSUB_PUBLISHER_ROLE] = value

@property
def subscribers(self):
Expand All @@ -135,4 +135,4 @@ def subscribers(self, value):
_ASSIGNMENT_DEPRECATED_MSG.format(
'subscribers', PUBSUB_SUBSCRIBER_ROLE),
DeprecationWarning)
self._bindings[PUBSUB_SUBSCRIBER_ROLE] = list(value)
self[PUBSUB_SUBSCRIBER_ROLE] = value
37 changes: 18 additions & 19 deletions pubsub/tests/unit/test_iam.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,56 +27,55 @@ def _make_one(self, *args, **kw):
return self._get_target_class()(*args, **kw)

def test_ctor_defaults(self):
empty = frozenset()
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), [])
self.assertEqual(policy.owners, empty)
self.assertEqual(policy.editors, empty)
self.assertEqual(policy.viewers, empty)
self.assertEqual(policy.publishers, empty)
self.assertEqual(policy.subscribers, empty)

def test_ctor_explicit(self):
VERSION = 17
ETAG = 'ETAG'
empty = frozenset()
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), [])
self.assertEqual(list(policy.publishers), [])
self.assertEqual(list(policy.subscribers), [])
self.assertEqual(policy.owners, empty)
self.assertEqual(policy.editors, empty)
self.assertEqual(policy.viewers, empty)
self.assertEqual(policy.publishers, empty)
self.assertEqual(policy.subscribers, empty)

def test_publishers_setter(self):
import warnings
from google.cloud.pubsub.iam import (
PUBSUB_PUBLISHER_ROLE,
)
PUBLISHER = 'user:phred@example.com'
expected = set([PUBLISHER])
policy = self._make_one()
with warnings.catch_warnings():
policy.publishers = [PUBLISHER]

self.assertEqual(sorted(policy.publishers), [PUBLISHER])
self.assertEqual(policy.publishers, frozenset(expected))
self.assertEqual(
dict(policy), {PUBSUB_PUBLISHER_ROLE: [PUBLISHER]})
dict(policy), {PUBSUB_PUBLISHER_ROLE: expected})

def test_subscribers_setter(self):
import warnings
from google.cloud.pubsub.iam import (
PUBSUB_SUBSCRIBER_ROLE,
)
SUBSCRIBER = 'serviceAccount:1234-abcdef@service.example.com'
expected = set([SUBSCRIBER])
policy = self._make_one()
with warnings.catch_warnings():
policy.subscribers = [SUBSCRIBER]

self.assertEqual(sorted(policy.subscribers), [SUBSCRIBER])
self.assertEqual(policy.subscribers, frozenset(expected))
self.assertEqual(
dict(policy), {PUBSUB_SUBSCRIBER_ROLE: [SUBSCRIBER]})
dict(policy), {PUBSUB_SUBSCRIBER_ROLE: expected})

0 comments on commit 1633962

Please sign in to comment.