From 6c9556d2907fb515e835244d75caab55cbfc2cb6 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 19 Nov 2019 17:19:26 -0800 Subject: [PATCH 01/10] feat(api_core): support version 3 policy bindings --- api_core/google/api_core/iam.py | 251 +++++++++++++++++++---- api_core/tests/unit/test_iam.py | 150 ++++++++++++-- bigtable/google/cloud/bigtable/policy.py | 54 ++++- bigtable/tests/unit/test_policy.py | 84 +++++++- 4 files changed, 467 insertions(+), 72 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 04680eb58869..869b715f45cb 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -21,19 +21,38 @@ .. code-block:: python # ``get_iam_policy`` returns a :class:'~google.api_core.iam.Policy`. - policy = resource.get_iam_policy() - - phred = policy.user("phred@example.com") - admin_group = policy.group("admins@groups.example.com") - account = policy.service_account("account-1234@accounts.example.com") - policy["roles/owner"] = [phred, admin_group, account] - policy["roles/editor"] = policy.authenticated_users() - policy["roles/viewer"] = policy.all_users() + policy = resource.get_iam_policy(requested_policy_version=3) + + phred = "user:phred@example.com" + admin_group = "group:admins@groups.example.com" + account = "serviceAccount:account-1234@accounts.example.com" + + policy.version = 3 + policy.bindings = [ + { + "role": "roles/owner", + "members": {phred, admin_group, account} + }, + { + "role": "roles/editor", + "members": {"allAuthenticatedUsers"} + }, + { + "role": "roles/viewer", + "members": {"allUsers"} + "condition": { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", + "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" + } + } + ] resource.set_iam_policy(policy) """ import collections +import operator import warnings try: @@ -53,18 +72,41 @@ """Generic role implying rights to access an object.""" _ASSIGNMENT_DEPRECATED_MSG = """\ -Assigning to '{}' is deprecated. Replace with 'policy[{}] = members.""" +Assigning to '{}' is deprecated. Use the `policy.bindings` property to modify bindings instead.""" + +_FACTORY_DEPRECATED_MSG = """\ +Factory method {0} is deprecated. Replace with '{0}'.""" + +_DICT_ACCESS_MSG = """\ +Dict access is not supported on policies with version > 1 or with conditional bindings.""" + + +class InvalidOperationException(Exception): + """Raised when trying to use Policy class as a dict.""" + + pass class Policy(collections_abc.MutableMapping): """IAM Policy - See - https://cloud.google.com/iam/reference/rest/v1/Policy - Args: etag (Optional[str]): ETag used to identify a unique of the policy - version (Optional[int]): unique version of the policy + version (Optional[int]): The syntax schema version of the policy. + + Note: + Using conditions in bindings requires the policy's version to be set + to `3` or greater, depending on the versions that are currently supported. + + Accessing the policy using dict operations will raise InvalidOperationException + when the policy's version is set to 3. + + Use the policy.bindings getter/setter to retrieve and modify the policy's bindings. + + See: + IAM Policy https://cloud.google.com/iam/reference/rest/v1/Policy + Policy versions https://cloud.google.com/iam/docs/policies#versions + Conditions overview https://cloud.google.com/iam/docs/conditions-overview. """ _OWNER_ROLES = (OWNER_ROLE,) @@ -79,31 +121,109 @@ class Policy(collections_abc.MutableMapping): def __init__(self, etag=None, version=None): self.etag = etag self.version = version - self._bindings = collections.defaultdict(set) + self._bindings = [] def __iter__(self): - return iter(self._bindings) + self.__check_version__() + return (binding["role"] for binding in self._bindings) def __len__(self): + self.__check_version__() return len(self._bindings) def __getitem__(self, key): - return self._bindings[key] + self.__check_version__() + for b in self._bindings: + if b["role"] == key: + return b["members"] + return set() def __setitem__(self, key, value): - self._bindings[key] = set(value) + self.__check_version__() + value = set(value) + for binding in self._bindings: + if binding["role"] == key: + binding["members"] = value + return + self._bindings.append({"role": key, "members": value}) def __delitem__(self, key): - del self._bindings[key] + self.__check_version__() + for b in self._bindings: + if b["role"] == key: + self._bindings.remove(b) + return + raise KeyError(key) + + def __check_version__(self): + """Raise InvalidOperationException if version is greater than 1 or policy contains conditions.""" + raise_version = self.version is not None and self.version > 1 + + if raise_version or self._contains_conditions(): + raise InvalidOperationException(_DICT_ACCESS_MSG) + + def _contains_conditions(self): + for b in self._bindings: + if b.get("condition") is not None: + return True + return False + + @property + def bindings(self): + """:obj:`list` of :obj:`dict`: The policy's bindings list. + :obj:`dict` Binding: + role (str): Role that is assigned to `members`. + members (:obj:`set` of str): Specifies the identities associated to this binding. + condition (dict of str:str): Specifies a condition under which this binding will apply. + + :obj:`dict` Condition: + title (str): Title for the condition. + description (:obj:str, optional): Description of the condition. + expression: A CEL expression. + + See: + Policy versions https://cloud.google.com/iam/docs/policies#versions + Conditions overview https://cloud.google.com/iam/docs/conditions-overview. + + Example: + .. code-block:: python + USER = "user:phred@example.com" + ADMIN_GROUP = "group:admins@groups.example.com" + SERVICE_ACCOUNT = "serviceAccount:account-1234@accounts.example.com" + + # Set policy's version to 3 before setting bindings containing conditions. + policy.version = 3 + + policy.bindings = [ + { + "role": "roles/viewer", + "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, + "condition": { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", # Optional + "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" + } + }, + ... + ] + """ + return self._bindings + + @bindings.setter + def bindings(self, bindings): + self._bindings = bindings @property def owners(self): """Legacy access to owner role. - DEPRECATED: use ``policy["roles/owners"]`` instead.""" + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to access bindings instead. + """ result = set() for role in self._OWNER_ROLES: - for member in self._bindings.get(role, ()): + for member in self.get(role, ()): result.add(member) return frozenset(result) @@ -111,7 +231,10 @@ def owners(self): def owners(self, value): """Update owners. - DEPRECATED: use ``policy["roles/owners"] = value`` instead.""" + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to access bindings instead. + """ warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format("owners", OWNER_ROLE), DeprecationWarning ) @@ -121,10 +244,13 @@ def owners(self, value): def editors(self): """Legacy access to editor role. - DEPRECATED: use ``policy["roles/editors"]`` instead.""" + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to access bindings instead. + """ result = set() for role in self._EDITOR_ROLES: - for member in self._bindings.get(role, ()): + for member in self.get(role, ()): result.add(member) return frozenset(result) @@ -132,7 +258,10 @@ def editors(self): def editors(self, value): """Update editors. - DEPRECATED: use ``policy["roles/editors"] = value`` instead.""" + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to modify bindings instead. + """ warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format("editors", EDITOR_ROLE), DeprecationWarning, @@ -143,11 +272,13 @@ def editors(self, value): def viewers(self): """Legacy access to viewer role. - DEPRECATED: use ``policy["roles/viewers"]`` instead + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to modify bindings instead. """ result = set() for role in self._VIEWER_ROLES: - for member in self._bindings.get(role, ()): + for member in self.get(role, ()): result.add(member) return frozenset(result) @@ -155,7 +286,9 @@ def viewers(self): def viewers(self, value): """Update viewers. - DEPRECATED: use ``policy["roles/viewers"] = value`` instead. + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + + DEPRECATED: use `policy.bindings` to modify bindings instead. """ warnings.warn( _ASSIGNMENT_DEPRECATED_MSG.format("viewers", VIEWER_ROLE), @@ -172,7 +305,12 @@ def user(email): Returns: str: A member string corresponding to the given user. + + DEPRECATED: set the role `user:{email}` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("user:{email}"), DeprecationWarning, + ) return "user:%s" % (email,) @staticmethod @@ -184,7 +322,13 @@ def service_account(email): Returns: str: A member string corresponding to the given service account. + + DEPRECATED: set the role `serviceAccount:{email}` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("serviceAccount:{email}"), + DeprecationWarning, + ) return "serviceAccount:%s" % (email,) @staticmethod @@ -196,7 +340,12 @@ def group(email): Returns: str: A member string corresponding to the given group. + + DEPRECATED: set the role `group:{email}` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("group:{email}"), DeprecationWarning, + ) return "group:%s" % (email,) @staticmethod @@ -208,7 +357,12 @@ def domain(domain): Returns: str: A member string corresponding to the given domain. + + DEPRECATED: set the role `domain:{email}` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("domain:{email}"), DeprecationWarning, + ) return "domain:%s" % (domain,) @staticmethod @@ -217,7 +371,12 @@ def all_users(): Returns: str: A member string representing all users. + + DEPRECATED: set the role `allUsers` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("allUsers"), DeprecationWarning, + ) return "allUsers" @staticmethod @@ -226,7 +385,12 @@ def authenticated_users(): Returns: str: A member string representing all authenticated users. + + DEPRECATED: set the role `allAuthenticatedUsers` in the binding instead. """ + warnings.warn( + _FACTORY_DEPRECATED_MSG.format("allAuthenticatedUsers"), DeprecationWarning, + ) return "allAuthenticatedUsers" @classmethod @@ -242,10 +406,11 @@ def from_api_repr(cls, resource): version = resource.get("version") etag = resource.get("etag") policy = cls(etag, version) - for binding in resource.get("bindings", ()): - role = binding["role"] - members = sorted(binding["members"]) - policy[role] = members + policy.bindings = resource.get("bindings", []) + + for binding in policy.bindings: + binding["members"] = set(binding.get("members", ())) + return policy def to_api_repr(self): @@ -262,13 +427,21 @@ def to_api_repr(self): if self.version is not None: resource["version"] = self.version - if self._bindings: - bindings = resource["bindings"] = [] - for role, members in sorted(self._bindings.items()): - if members: - bindings.append({"role": role, "members": sorted(set(members))}) - - if not bindings: - del resource["bindings"] + if self._bindings and len(self._bindings) > 0: + bindings = [] + for binding in self._bindings: + if binding["members"]: + new_binding = { + "role": binding["role"], + "members": sorted(binding["members"]) + } + if binding.get("condition"): + new_binding["condition"] = binding["condition"] + bindings.append(new_binding) + + if bindings: + # Sort bindings by role + key = operator.itemgetter("role") + resource["bindings"] = sorted(bindings, key=key) return resource diff --git a/api_core/tests/unit/test_iam.py b/api_core/tests/unit/test_iam.py index 199c38907983..c9752c4f9c1f 100644 --- a/api_core/tests/unit/test_iam.py +++ b/api_core/tests/unit/test_iam.py @@ -14,6 +14,8 @@ import pytest +from google.api_core.iam import _DICT_ACCESS_MSG, InvalidOperationException + class TestPolicy: @staticmethod @@ -37,7 +39,7 @@ def test_ctor_defaults(self): assert dict(policy) == {} def test_ctor_explicit(self): - VERSION = 17 + VERSION = 1 ETAG = "ETAG" empty = frozenset() policy = self._make_one(ETAG, VERSION) @@ -53,6 +55,21 @@ def test___getitem___miss(self): policy = self._make_one() assert policy["nonesuch"] == set() + def test___getitem___version3(self): + policy = self._make_one("DEADBEEF", 3) + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy["role"] + + def test___getitem___with_conditions(self): + USER = "user:phred@example.com" + CONDITION = {"expression": "2 > 1"} + policy = self._make_one("DEADBEEF", 1) + policy.bindings = [ + {"role": "role/reader", "members": [USER], "condition": CONDITION} + ] + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy["role/reader"] + def test___setitem__(self): USER = "user:phred@example.com" PRINCIPALS = set([USER]) @@ -62,18 +79,70 @@ def test___setitem__(self): assert len(policy) == 1 assert dict(policy) == {"rolename": PRINCIPALS} + def test__set_item__overwrite(self): + USER = "user:phred@example.com" + ALL_USERS = "allUsers" + MEMBERS = set([ALL_USERS]) + policy = self._make_one() + policy["rolename"] = [USER] + policy["rolename"] = [ALL_USERS] + assert policy["rolename"] == MEMBERS + assert len(policy) == 1 + assert dict(policy) == {"rolename": MEMBERS} + + def test___setitem___version3(self): + policy = self._make_one("DEADBEEF", 3) + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy["role/reader"] = ["user:phred@example.com"] + + def test___setitem___with_conditions(self): + USER = "user:phred@example.com" + CONDITION = {"expression": "2 > 1"} + policy = self._make_one("DEADBEEF", 1) + policy.bindings = [ + {"role": "role/reader", "members": set([USER]), "condition": CONDITION} + ] + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy["role/reader"] = ["user:phred@example.com"] + def test___delitem___hit(self): policy = self._make_one() - policy._bindings["rolename"] = ["phred@example.com"] - del policy["rolename"] - assert len(policy) == 0 - assert dict(policy) == {} + policy.bindings = [ + {"role": "to/keep", "members": set(["phred@example.com"])}, + {"role": "to/remove", "members": set(["phred@example.com"])} + ] + del policy["to/remove"] + assert len(policy) == 1 + assert dict(policy) == {"to/keep": set(["phred@example.com"])} def test___delitem___miss(self): policy = self._make_one() with pytest.raises(KeyError): del policy["nonesuch"] + def test___delitem___version3(self): + policy = self._make_one("DEADBEEF", 3) + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + del policy["role/reader"] + + def test___delitem___with_conditions(self): + USER = "user:phred@example.com" + CONDITION = {"expression": "2 > 1"} + policy = self._make_one("DEADBEEF", 1) + policy.bindings = [ + {"role": "role/reader", "members": set([USER]), "condition": CONDITION} + ] + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + del policy["role/reader"] + + def test_bindings_property(self): + USER = "user:phred@example.com" + CONDITION = {"expression": "2 > 1"} + policy = self._make_one() + BINDINGS = [{"role": "role/reader", "members": set([USER]), "condition": CONDITION}] + policy.bindings = BINDINGS + assert policy.bindings == BINDINGS + def test_owners_getter(self): from google.api_core.iam import OWNER_ROLE @@ -94,7 +163,7 @@ def test_owners_setter(self): with warnings.catch_warnings(record=True) as warned: policy.owners = [MEMBER] - warning, = warned + (warning,) = warned assert warning.category is DeprecationWarning assert policy[OWNER_ROLE] == expected @@ -118,7 +187,7 @@ def test_editors_setter(self): with warnings.catch_warnings(record=True) as warned: policy.editors = [MEMBER] - warning, = warned + (warning,) = warned assert warning.category is DeprecationWarning assert policy[EDITOR_ROLE] == expected @@ -142,41 +211,77 @@ def test_viewers_setter(self): with warnings.catch_warnings(record=True) as warned: policy.viewers = [MEMBER] - warning, = warned + (warning,) = warned assert warning.category is DeprecationWarning assert policy[VIEWER_ROLE] == expected def test_user(self): + import warnings + EMAIL = "phred@example.com" MEMBER = "user:%s" % (EMAIL,) policy = self._make_one() - assert policy.user(EMAIL) == MEMBER + with warnings.catch_warnings(record=True) as warned: + assert policy.user(EMAIL) == MEMBER + + (warning,) = warned + assert warning.category is DeprecationWarning def test_service_account(self): + import warnings + EMAIL = "phred@example.com" MEMBER = "serviceAccount:%s" % (EMAIL,) policy = self._make_one() - assert policy.service_account(EMAIL) == MEMBER + with warnings.catch_warnings(record=True) as warned: + assert policy.service_account(EMAIL) == MEMBER + + (warning,) = warned + assert warning.category is DeprecationWarning def test_group(self): + import warnings + EMAIL = "phred@example.com" MEMBER = "group:%s" % (EMAIL,) policy = self._make_one() - assert policy.group(EMAIL) == MEMBER + with warnings.catch_warnings(record=True) as warned: + assert policy.group(EMAIL) == MEMBER + + (warning,) = warned + assert warning.category is DeprecationWarning def test_domain(self): + import warnings + DOMAIN = "example.com" MEMBER = "domain:%s" % (DOMAIN,) policy = self._make_one() - assert policy.domain(DOMAIN) == MEMBER + with warnings.catch_warnings(record=True) as warned: + assert policy.domain(DOMAIN) == MEMBER + + (warning,) = warned + assert warning.category is DeprecationWarning def test_all_users(self): + import warnings + policy = self._make_one() - assert policy.all_users() == "allUsers" + with warnings.catch_warnings(record=True) as warned: + assert policy.all_users() == "allUsers" + + (warning,) = warned + assert warning.category is DeprecationWarning def test_authenticated_users(self): + import warnings + policy = self._make_one() - assert policy.authenticated_users() == "allAuthenticatedUsers" + with warnings.catch_warnings(record=True) as warned: + assert policy.authenticated_users() == "allAuthenticatedUsers" + + (warning,) = warned + assert warning.category is DeprecationWarning def test_from_api_repr_only_etag(self): empty = frozenset() @@ -201,7 +306,7 @@ def test_from_api_repr_complete(self): VIEWER2 = "user:phred@example.com" RESOURCE = { "etag": "DEADBEEF", - "version": 17, + "version": 1, "bindings": [ {"role": OWNER_ROLE, "members": [OWNER1, OWNER2]}, {"role": EDITOR_ROLE, "members": [EDITOR1, EDITOR2]}, @@ -211,7 +316,7 @@ def test_from_api_repr_complete(self): klass = self._get_target_class() policy = klass.from_api_repr(RESOURCE) assert policy.etag == "DEADBEEF" - assert policy.version == 17 + assert policy.version == 1 assert policy.owners, frozenset([OWNER1 == OWNER2]) assert policy.editors, frozenset([EDITOR1 == EDITOR2]) assert policy.viewers, frozenset([VIEWER1 == VIEWER2]) @@ -220,19 +325,24 @@ def test_from_api_repr_complete(self): EDITOR_ROLE: set([EDITOR1, EDITOR2]), VIEWER_ROLE: set([VIEWER1, VIEWER2]), } + assert policy.bindings == [ + {"role": OWNER_ROLE, "members": set([OWNER1, OWNER2])}, + {"role": EDITOR_ROLE, "members": set([EDITOR1, EDITOR2])}, + {"role": VIEWER_ROLE, "members": set([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, + "version": 1, "bindings": [{"role": "unknown", "members": [USER, GROUP]}], } klass = self._get_target_class() policy = klass.from_api_repr(RESOURCE) assert policy.etag == "DEADBEEF" - assert policy.version == 17 + assert policy.version == 1 assert dict(policy), {"unknown": set([GROUP == USER])} def test_to_api_repr_defaults(self): @@ -276,13 +386,13 @@ def test_to_api_repr_full(self): {"role": EDITOR_ROLE, "members": [EDITOR1, EDITOR2]}, {"role": VIEWER_ROLE, "members": [VIEWER1, VIEWER2]}, ] - policy = self._make_one("DEADBEEF", 17) + policy = self._make_one("DEADBEEF", 1) with warnings.catch_warnings(record=True): policy.owners = [OWNER1, OWNER2] policy.editors = [EDITOR1, EDITOR2] policy.viewers = [VIEWER1, VIEWER2] resource = policy.to_api_repr() assert resource["etag"] == "DEADBEEF" - assert resource["version"] == 17 + assert resource["version"] == 1 key = operator.itemgetter("role") assert sorted(resource["bindings"], key=key) == sorted(BINDINGS, key=key) diff --git a/bigtable/google/cloud/bigtable/policy.py b/bigtable/google/cloud/bigtable/policy.py index 9fea7bbc5a0e..65be0158a006 100644 --- a/bigtable/google/cloud/bigtable/policy.py +++ b/bigtable/google/cloud/bigtable/policy.py @@ -72,6 +72,22 @@ class Policy(BasePolicy): If no etag is provided in the call to setIamPolicy, then the existing policy is overwritten blindly. + :type version: int + :param version: The syntax schema version of the policy. + + Note: + Using conditions in bindings requires the policy's version to be set + to `3` or greater, depending on the versions that are currently supported. + + Accessing the policy using dict operations will raise InvalidOperationException + when the policy's version is set to 3. + + Use the policy.bindings getter/setter to retrieve and modify the policy's bindings. + + See: + IAM Policy https://cloud.google.com/iam/reference/rest/v1/Policy + Policy versions https://cloud.google.com/iam/docs/policies#versions + Conditions overview https://cloud.google.com/iam/docs/conditions-overview. """ def __init__(self, etag=None, version=None): @@ -83,6 +99,8 @@ def __init__(self, etag=None, version=None): def bigtable_admins(self): """Access to bigtable.admin role memebers + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + For example: .. literalinclude:: snippets.py @@ -90,7 +108,7 @@ def bigtable_admins(self): :end-before: [END bigtable_admins_policy] """ result = set() - for member in self._bindings.get(BIGTABLE_ADMIN_ROLE, ()): + for member in self.get(BIGTABLE_ADMIN_ROLE, ()): result.add(member) return frozenset(result) @@ -98,6 +116,8 @@ def bigtable_admins(self): def bigtable_readers(self): """Access to bigtable.reader role memebers + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + For example: .. literalinclude:: snippets.py @@ -105,7 +125,7 @@ def bigtable_readers(self): :end-before: [END bigtable_readers_policy] """ result = set() - for member in self._bindings.get(BIGTABLE_READER_ROLE, ()): + for member in self.get(BIGTABLE_READER_ROLE, ()): result.add(member) return frozenset(result) @@ -113,6 +133,8 @@ def bigtable_readers(self): def bigtable_users(self): """Access to bigtable.user role memebers + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + For example: .. literalinclude:: snippets.py @@ -120,7 +142,7 @@ def bigtable_users(self): :end-before: [END bigtable_users_policy] """ result = set() - for member in self._bindings.get(BIGTABLE_USER_ROLE, ()): + for member in self.get(BIGTABLE_USER_ROLE, ()): result.add(member) return frozenset(result) @@ -128,6 +150,8 @@ def bigtable_users(self): def bigtable_viewers(self): """Access to bigtable.viewer role memebers + Raise InvalidOperationException if version is greater than 1 or policy contains conditions. + For example: .. literalinclude:: snippets.py @@ -135,7 +159,7 @@ def bigtable_viewers(self): :end-before: [END bigtable_viewers_policy] """ result = set() - for member in self._bindings.get(BIGTABLE_VIEWER_ROLE, ()): + for member in self.get(BIGTABLE_VIEWER_ROLE, ()): result.add(member) return frozenset(result) @@ -152,8 +176,17 @@ def from_pb(cls, policy_pb): """ policy = cls(policy_pb.etag, policy_pb.version) - for binding in policy_pb.bindings: - policy[binding.role] = sorted(binding.members) + policy.bindings = bindings = [] + for binding_pb in policy_pb.bindings: + binding = {"role": binding_pb.role, "members": set(binding_pb.members)} + condition = binding_pb.condition + if condition and condition.expression: + binding["condition"] = { + "title": condition.title, + "description": condition.description, + "expression": condition.expression, + } + bindings.append(binding) return policy @@ -169,8 +202,13 @@ def to_pb(self): etag=self.etag, version=self.version or 0, bindings=[ - policy_pb2.Binding(role=role, members=sorted(self[role])) - for role in self + policy_pb2.Binding( + role=binding["role"], + members=sorted(binding["members"]), + condition=binding.get("condition"), + ) + for binding in self.bindings + if binding["members"] ], ) diff --git a/bigtable/tests/unit/test_policy.py b/bigtable/tests/unit/test_policy.py index 74b19e49b29a..f91a96cddc56 100644 --- a/bigtable/tests/unit/test_policy.py +++ b/bigtable/tests/unit/test_policy.py @@ -38,7 +38,7 @@ def test_ctor_defaults(self): self.assertEqual(dict(policy), {}) def test_ctor_explicit(self): - VERSION = 17 + VERSION = 1 ETAG = b"ETAG" empty = frozenset() policy = self._make_one(ETAG, VERSION) @@ -108,7 +108,7 @@ def test_from_pb_non_empty(self): from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE ETAG = b"ETAG" - VERSION = 17 + VERSION = 1 members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] empty = frozenset() message = policy_pb2.Policy( @@ -127,6 +127,46 @@ def test_from_pb_non_empty(self): self.assertEqual(len(policy), 1) self.assertEqual(dict(policy), {BIGTABLE_ADMIN_ROLE: set(members)}) + def test_from_pb_with_condition(self): + import pytest + from google.iam.v1 import policy_pb2 + from google.api_core.iam import InvalidOperationException, _DICT_ACCESS_MSG + from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE + + ETAG = b"ETAG" + VERSION = 3 + members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] + empty = frozenset() + BINDINGS = [ + { + "role": BIGTABLE_ADMIN_ROLE, + "members": members, + "condition": { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", + "expression": 'request.time < timestamp("2021-01-01T00:00:00Z")', + }, + } + ] + message = policy_pb2.Policy(etag=ETAG, version=VERSION, bindings=BINDINGS,) + klass = self._get_target_class() + policy = klass.from_pb(message) + self.assertEqual(policy.etag, ETAG) + self.assertEqual(policy.version, VERSION) + self.assertEqual(policy.bindings[0]["role"], BIGTABLE_ADMIN_ROLE) + self.assertEqual(policy.bindings[0]["members"], set(members)) + self.assertEqual(policy.bindings[0]["condition"], BINDINGS[0]["condition"]) + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy.bigtable_admins + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy.bigtable_readers + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy.bigtable_users + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + policy.bigtable_viewers + with pytest.raises(InvalidOperationException, match=_DICT_ACCESS_MSG): + len(policy) + def test_to_pb_empty(self): from google.iam.v1 import policy_pb2 @@ -139,7 +179,7 @@ def test_to_pb_explicit(self): from google.iam.v1 import policy_pb2 from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE - VERSION = 17 + VERSION = 1 ETAG = b"ETAG" members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] policy = self._make_one(ETAG, VERSION) @@ -154,8 +194,42 @@ def test_to_pb_explicit(self): self.assertEqual(policy.to_pb(), expected) + def test_to_pb_with_condition(self): + from google.iam.v1 import policy_pb2 + from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE + + VERSION = 3 + ETAG = b"ETAG" + members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] + condition = { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", + "expression": 'request.time < timestamp("2021-01-01T00:00:00Z")', + } + policy = self._make_one(ETAG, VERSION) + policy.bindings = [ + { + "role": BIGTABLE_ADMIN_ROLE, + "members": set(members), + "condition": condition, + } + ] + expected = policy_pb2.Policy( + etag=ETAG, + version=VERSION, + bindings=[ + policy_pb2.Binding( + role=BIGTABLE_ADMIN_ROLE, + members=sorted(members), + condition=condition, + ) + ], + ) + + self.assertEqual(policy.to_pb(), expected) + def test_from_api_repr_wo_etag(self): - VERSION = 17 + VERSION = 1 empty = frozenset() resource = {"version": VERSION} klass = self._get_target_class() @@ -187,7 +261,7 @@ def test_from_api_repr_w_etag(self): self.assertEqual(dict(policy), {}) def test_to_api_repr_wo_etag(self): - VERSION = 17 + VERSION = 1 resource = {"version": VERSION} policy = self._make_one(version=VERSION) self.assertEqual(policy.to_api_repr(), resource) From 3342650dbb80bf8b4f556dbe0980568008270ae7 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 6 Jan 2020 15:17:41 -0800 Subject: [PATCH 02/10] fix(doc): fix documenting bindings structure --- api_core/google/api_core/iam.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 869b715f45cb..0d870896b044 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -171,15 +171,14 @@ def _contains_conditions(self): @property def bindings(self): """:obj:`list` of :obj:`dict`: The policy's bindings list. - :obj:`dict` Binding: + + A binding is specified by a dictionary with keys: role (str): Role that is assigned to `members`. members (:obj:`set` of str): Specifies the identities associated to this binding. condition (dict of str:str): Specifies a condition under which this binding will apply. - - :obj:`dict` Condition: - title (str): Title for the condition. - description (:obj:str, optional): Description of the condition. - expression: A CEL expression. + title (str): Title for the condition. + description (:obj:str, optional): Description of the condition. + expression: A CEL expression. See: Policy versions https://cloud.google.com/iam/docs/policies#versions From 1265c7e9380d87cbeb8dbffd0c388ab79233be5c Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 6 Jan 2020 16:00:10 -0800 Subject: [PATCH 03/10] try fixing docs --- api_core/google/api_core/iam.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 0d870896b044..742aebfb2bb5 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -176,9 +176,9 @@ def bindings(self): role (str): Role that is assigned to `members`. members (:obj:`set` of str): Specifies the identities associated to this binding. condition (dict of str:str): Specifies a condition under which this binding will apply. - title (str): Title for the condition. - description (:obj:str, optional): Description of the condition. - expression: A CEL expression. + - title (str): Title for the condition. + - description (:obj:str, optional): Description of the condition. + - expression: A CEL expression. See: Policy versions https://cloud.google.com/iam/docs/policies#versions From 69319001f2620a8e32f535d3f6fb2f945846664b Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 6 Jan 2020 17:43:51 -0800 Subject: [PATCH 04/10] fix pytype error --- api_core/google/api_core/iam.py | 37 ++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 742aebfb2bb5..8b3f89b501bf 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -173,22 +173,27 @@ def bindings(self): """:obj:`list` of :obj:`dict`: The policy's bindings list. A binding is specified by a dictionary with keys: - role (str): Role that is assigned to `members`. - members (:obj:`set` of str): Specifies the identities associated to this binding. - condition (dict of str:str): Specifies a condition under which this binding will apply. - - title (str): Title for the condition. - - description (:obj:str, optional): Description of the condition. - - expression: A CEL expression. + role (str): Role that is assigned to `members`. + members (:obj:`set` of str): Specifies the identities associated to this binding. + condition (dict of str:str): Specifies a condition under which this binding will apply. + - title (str): Title for the condition. + - description (:obj:str, optional): Description of the condition. + - expression: A CEL expression. See: - Policy versions https://cloud.google.com/iam/docs/policies#versions - Conditions overview https://cloud.google.com/iam/docs/conditions-overview. + Policy versions https://cloud.google.com/iam/docs/policies#versions + Conditions overview https://cloud.google.com/iam/docs/conditions-overview. Example: .. code-block:: python USER = "user:phred@example.com" ADMIN_GROUP = "group:admins@groups.example.com" SERVICE_ACCOUNT = "serviceAccount:account-1234@accounts.example.com" + condition = { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", # Optional + "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" + } # Set policy's version to 3 before setting bindings containing conditions. policy.version = 3 @@ -197,11 +202,7 @@ def bindings(self): { "role": "roles/viewer", "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, - "condition": { - "title": "request_time", - "description": "Requests made before 2021-01-01T00:00:00Z", # Optional - "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" - } + "condition": CONDITION }, ... ] @@ -429,13 +430,15 @@ def to_api_repr(self): if self._bindings and len(self._bindings) > 0: bindings = [] for binding in self._bindings: - if binding["members"]: + members = binding.get("members") + if members: new_binding = { "role": binding["role"], - "members": sorted(binding["members"]) + "members": sorted(members) } - if binding.get("condition"): - new_binding["condition"] = binding["condition"] + condition = binding.get("condition") + if condition: + new_binding["condition"] = condition bindings.append(new_binding) if bindings: From da80589d75903813f68850951c89d489153a7d60 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 6 Jan 2020 17:49:56 -0800 Subject: [PATCH 05/10] fill test coverage --- api_core/tests/unit/test_iam.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/api_core/tests/unit/test_iam.py b/api_core/tests/unit/test_iam.py index c9752c4f9c1f..788392e48cad 100644 --- a/api_core/tests/unit/test_iam.py +++ b/api_core/tests/unit/test_iam.py @@ -381,16 +381,19 @@ def test_to_api_repr_full(self): EDITOR2 = "user:phred@example.com" VIEWER1 = "serviceAccount:1234-abcdef@service.example.com" VIEWER2 = "user:phred@example.com" + CONDITION = { + "title": "title", + "description": "description", + "expression": "true" + } BINDINGS = [ {"role": OWNER_ROLE, "members": [OWNER1, OWNER2]}, {"role": EDITOR_ROLE, "members": [EDITOR1, EDITOR2]}, {"role": VIEWER_ROLE, "members": [VIEWER1, VIEWER2]}, + {"role": VIEWER_ROLE, "members": [VIEWER1, VIEWER2], "condition": CONDITION}, ] policy = self._make_one("DEADBEEF", 1) - with warnings.catch_warnings(record=True): - policy.owners = [OWNER1, OWNER2] - policy.editors = [EDITOR1, EDITOR2] - policy.viewers = [VIEWER1, VIEWER2] + policy.bindings = BINDINGS resource = policy.to_api_repr() assert resource["etag"] == "DEADBEEF" assert resource["version"] == 1 From ff2667a96ec5bd6c41dd679ebd0caf7e9aaec02f Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Wed, 8 Jan 2020 16:27:22 -0800 Subject: [PATCH 06/10] indent docs --- api_core/google/api_core/iam.py | 40 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 8b3f89b501bf..d485556ed1ae 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -186,26 +186,26 @@ def bindings(self): Example: .. code-block:: python - USER = "user:phred@example.com" - ADMIN_GROUP = "group:admins@groups.example.com" - SERVICE_ACCOUNT = "serviceAccount:account-1234@accounts.example.com" - condition = { - "title": "request_time", - "description": "Requests made before 2021-01-01T00:00:00Z", # Optional - "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" - } - - # Set policy's version to 3 before setting bindings containing conditions. - policy.version = 3 - - policy.bindings = [ - { - "role": "roles/viewer", - "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, - "condition": CONDITION - }, - ... - ] + USER = "user:phred@example.com" + ADMIN_GROUP = "group:admins@groups.example.com" + SERVICE_ACCOUNT = "serviceAccount:account-1234@accounts.example.com" + CONDITION = { + "title": "request_time", + "description": "Requests made before 2021-01-01T00:00:00Z", # Optional + "expression": "request.time < timestamp(\"2021-01-01T00:00:00Z\")" + } + + # Set policy's version to 3 before setting bindings containing conditions. + policy.version = 3 + + policy.bindings = [ + { + "role": "roles/viewer", + "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, + "condition": CONDITION + }, + ... + ] """ return self._bindings From f84ea6fdbf855b5f6cf5eaa58888fb9fb746c377 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Wed, 8 Jan 2020 16:45:10 -0800 Subject: [PATCH 07/10] fix docs --- api_core/google/api_core/iam.py | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index d485556ed1ae..a7a1c00e236d 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -170,22 +170,33 @@ def _contains_conditions(self): @property def bindings(self): - """:obj:`list` of :obj:`dict`: The policy's bindings list. + """The policy's list of bindings. A binding is specified by a dictionary with keys: - role (str): Role that is assigned to `members`. - members (:obj:`set` of str): Specifies the identities associated to this binding. - condition (dict of str:str): Specifies a condition under which this binding will apply. - - title (str): Title for the condition. - - description (:obj:str, optional): Description of the condition. - - expression: A CEL expression. + + * role (str): Role that is assigned to `members`. + + * members (:obj:`set` of str): Specifies the identities associated to this binding. + + * condition (:obj:`dict` of str:str): Specifies a condition under which this binding will apply. + + * title (str): Title for the condition. + + * description (:obj:str, optional): Description of the condition. + + * expression: A CEL expression. + + Type: + :obj:`list` of :obj:`dict` See: Policy versions https://cloud.google.com/iam/docs/policies#versions Conditions overview https://cloud.google.com/iam/docs/conditions-overview. Example: + .. code-block:: python + USER = "user:phred@example.com" ADMIN_GROUP = "group:admins@groups.example.com" SERVICE_ACCOUNT = "serviceAccount:account-1234@accounts.example.com" @@ -199,10 +210,10 @@ def bindings(self): policy.version = 3 policy.bindings = [ - { - "role": "roles/viewer", - "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, - "condition": CONDITION + { + "role": "roles/viewer", + "members": {USER, ADMIN_GROUP, SERVICE_ACCOUNT}, + "condition": CONDITION }, ... ] From ef37fde284e68e9180415834f683a98c8f405347 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Wed, 8 Jan 2020 18:16:40 -0800 Subject: [PATCH 08/10] improve test coverage --- api_core/tests/unit/test_iam.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/api_core/tests/unit/test_iam.py b/api_core/tests/unit/test_iam.py index 788392e48cad..21d04ad769e6 100644 --- a/api_core/tests/unit/test_iam.py +++ b/api_core/tests/unit/test_iam.py @@ -80,15 +80,18 @@ def test___setitem__(self): assert dict(policy) == {"rolename": PRINCIPALS} def test__set_item__overwrite(self): + GROUP = "group:test@group.com" USER = "user:phred@example.com" ALL_USERS = "allUsers" MEMBERS = set([ALL_USERS]) + GROUPS = set([GROUP]) policy = self._make_one() - policy["rolename"] = [USER] - policy["rolename"] = [ALL_USERS] - assert policy["rolename"] == MEMBERS - assert len(policy) == 1 - assert dict(policy) == {"rolename": MEMBERS} + policy["first"] = [GROUP] + policy["second"] = [USER] + policy["second"] = [ALL_USERS] + assert policy["second"] == MEMBERS + assert len(policy) == 2 + assert dict(policy) == {"first": GROUPS, "second": MEMBERS} def test___setitem___version3(self): policy = self._make_one("DEADBEEF", 3) From f89760c87a82094bd4864ed98bf887908d773c45 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 11:35:15 -0800 Subject: [PATCH 09/10] linty --- api_core/tests/unit/test_iam.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api_core/tests/unit/test_iam.py b/api_core/tests/unit/test_iam.py index 21d04ad769e6..896e10de2264 100644 --- a/api_core/tests/unit/test_iam.py +++ b/api_core/tests/unit/test_iam.py @@ -375,7 +375,6 @@ def test_to_api_repr_binding_w_duplicates(self): def test_to_api_repr_full(self): import operator - import warnings from google.api_core.iam import OWNER_ROLE, EDITOR_ROLE, VIEWER_ROLE OWNER1 = "group:cloud-logs@google.com" From 22eb912833c7735bf2eddf5d19733ab0e8403a0f Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 11:38:31 -0800 Subject: [PATCH 10/10] remove unused variable --- bigtable/tests/unit/test_policy.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bigtable/tests/unit/test_policy.py b/bigtable/tests/unit/test_policy.py index f91a96cddc56..63f9ba03fb23 100644 --- a/bigtable/tests/unit/test_policy.py +++ b/bigtable/tests/unit/test_policy.py @@ -136,7 +136,6 @@ def test_from_pb_with_condition(self): ETAG = b"ETAG" VERSION = 3 members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] - empty = frozenset() BINDINGS = [ { "role": BIGTABLE_ADMIN_ROLE,