From 583ca423d2fc0ca61a53d0fdcb05c960ed807c86 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 19 Nov 2019 17:19:26 -0800 Subject: [PATCH 1/5] iam proposal #3 maintain compatibility with defaultdict remove in place raise KeyError on delete update deprecation for dict-key access and factory methods clean up maintain compatibility - removing duplicate in __setitems__ check for conditions for dict access remove empty binding fix test accessing private var _bindings fix(tests): change version to make existing tests pass tests: add tests for getitem, delitem, setitem on v3 and conditions test policy.bindings property fixlint black sort bindings by role when converting to api repr add deprecation warning for iam factory methods update deprecation message for role methods make Policy#bindings.members a set update policy docs fix docs make docs better fix: Bigtable policy class to use Policy.bindings add from_pb with conditions test add to_pb condition test blacken fix policy __delitem__ add docs on dict access do not modify binding in to_apr_repr --- 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 396dbfcf0734a058f38469b49b462dc597c908fa Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 17 Dec 2019 17:34:47 -0800 Subject: [PATCH 2/5] feat(bigtable): support requested_policy_version to instance --- bigtable/google/cloud/bigtable/instance.py | 25 ++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/bigtable/google/cloud/bigtable/instance.py b/bigtable/google/cloud/bigtable/instance.py index 8a664778577a..96e43b7b51a8 100644 --- a/bigtable/google/cloud/bigtable/instance.py +++ b/bigtable/google/cloud/bigtable/instance.py @@ -23,6 +23,7 @@ from google.protobuf import field_mask_pb2 +from google.cloud.bigtable_admin_v2.types import bigtable_instance_admin_pb2 from google.cloud.bigtable_admin_v2.types import instance_pb2 from google.api_core.exceptions import NotFound @@ -434,7 +435,7 @@ def delete(self): """ self._client.instance_admin_client.delete_instance(name=self.name) - def get_iam_policy(self): + def get_iam_policy(self, requested_policy_version=None): """Gets the access control policy for an instance resource. For example: @@ -443,11 +444,31 @@ def get_iam_policy(self): :start-after: [START bigtable_get_iam_policy] :end-before: [END bigtable_get_iam_policy] + :type requested_policy_version: int or ``NoneType`` + :param requested_policy_version: Optional. The version of IAM policies to request. + If a policy with a condition is requested without + setting this, the server will return an error. + This must be set to a value of 3 to retrieve IAM + policies containing conditions. This is to prevent + client code that isn't aware of IAM conditions from + interpreting and modifying policies incorrectly. + The service might return a policy with version lower + than the one that was requested, based on the + feature syntax in the policy fetched. + :rtype: :class:`google.cloud.bigtable.policy.Policy` :returns: The current IAM policy of this instance """ + args = {"resource": self.name} + + if requested_policy_version is not None: + args["options"] = bigtable_instance_admin_pb2.GetPolicyOptions( + requested_policy_version_dummy=requested_policy_version + ) + instance_admin_client = self._client.instance_admin_client - resp = instance_admin_client.get_iam_policy(resource=self.name) + + resp = instance_admin_client.get_iam_policy(**args) return Policy.from_pb(resp) def set_iam_policy(self, policy): From 1d44a60c1467c03ca8486b2927c11ff96f4c9b83 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 19 Dec 2019 12:56:12 -0800 Subject: [PATCH 3/5] fix passing requested_policy_version to pb2 --- bigtable/google/cloud/bigtable/instance.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bigtable/google/cloud/bigtable/instance.py b/bigtable/google/cloud/bigtable/instance.py index 96e43b7b51a8..dbdd20640918 100644 --- a/bigtable/google/cloud/bigtable/instance.py +++ b/bigtable/google/cloud/bigtable/instance.py @@ -23,8 +23,7 @@ from google.protobuf import field_mask_pb2 -from google.cloud.bigtable_admin_v2.types import bigtable_instance_admin_pb2 -from google.cloud.bigtable_admin_v2.types import instance_pb2 +from google.cloud.bigtable_admin_v2.types import instance_pb2, options_pb2 from google.api_core.exceptions import NotFound @@ -460,10 +459,9 @@ def get_iam_policy(self, requested_policy_version=None): :returns: The current IAM policy of this instance """ args = {"resource": self.name} - if requested_policy_version is not None: - args["options"] = bigtable_instance_admin_pb2.GetPolicyOptions( - requested_policy_version_dummy=requested_policy_version + args["options_"] = options_pb2.GetPolicyOptions( + requested_policy_version=requested_policy_version ) instance_admin_client = self._client.instance_admin_client From b05043b2742a51945194ccdef54a5414c9f8740c Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 19 Dec 2019 12:56:21 -0800 Subject: [PATCH 4/5] add unit test --- bigtable/tests/unit/test_instance.py | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/bigtable/tests/unit/test_instance.py b/bigtable/tests/unit/test_instance.py index 397e54d570d0..b129d4edc825 100644 --- a/bigtable/tests/unit/test_instance.py +++ b/bigtable/tests/unit/test_instance.py @@ -633,6 +633,44 @@ def test_get_iam_policy(self): for found, expected in zip(sorted(admins), sorted(members)): self.assertEqual(found, expected) + def test_get_iam_policy_w_requested_policy_version(self): + from google.cloud.bigtable_admin_v2.gapic import bigtable_instance_admin_client + from google.iam.v1 import policy_pb2, options_pb2 + from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE + + credentials = _make_credentials() + client = self._make_client( + project=self.PROJECT, credentials=credentials, admin=True + ) + instance = self._make_one(self.INSTANCE_ID, client) + + version = 1 + etag = b"etag_v1" + members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] + bindings = [{"role": BIGTABLE_ADMIN_ROLE, "members": members}] + iam_policy = policy_pb2.Policy(version=version, etag=etag, bindings=bindings) + + # Patch the stub used by the API method. + instance_api = mock.create_autospec( + bigtable_instance_admin_client.BigtableInstanceAdminClient + ) + client._instance_admin_client = instance_api + instance_api.get_iam_policy.return_value = iam_policy + + # Perform the method and check the result. + result = instance.get_iam_policy(requested_policy_version=3) + + instance_api.get_iam_policy.assert_called_once_with( + resource=instance.name, + options_=options_pb2.GetPolicyOptions(requested_policy_version=3), + ) + self.assertEqual(result.version, version) + self.assertEqual(result.etag, etag) + admins = result.bigtable_admins + self.assertEqual(len(admins), len(members)) + for found, expected in zip(sorted(admins), sorted(members)): + self.assertEqual(found, expected) + def test_set_iam_policy(self): from google.cloud.bigtable_admin_v2.gapic import bigtable_instance_admin_client from google.iam.v1 import policy_pb2 From f98e6b7fac013f411f8413a031c35cbfee165dde Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 19 Dec 2019 12:56:21 -0800 Subject: [PATCH 5/5] add unit test --- bigtable/tests/unit/test_instance.py | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/bigtable/tests/unit/test_instance.py b/bigtable/tests/unit/test_instance.py index 397e54d570d0..b129d4edc825 100644 --- a/bigtable/tests/unit/test_instance.py +++ b/bigtable/tests/unit/test_instance.py @@ -633,6 +633,44 @@ def test_get_iam_policy(self): for found, expected in zip(sorted(admins), sorted(members)): self.assertEqual(found, expected) + def test_get_iam_policy_w_requested_policy_version(self): + from google.cloud.bigtable_admin_v2.gapic import bigtable_instance_admin_client + from google.iam.v1 import policy_pb2, options_pb2 + from google.cloud.bigtable.policy import BIGTABLE_ADMIN_ROLE + + credentials = _make_credentials() + client = self._make_client( + project=self.PROJECT, credentials=credentials, admin=True + ) + instance = self._make_one(self.INSTANCE_ID, client) + + version = 1 + etag = b"etag_v1" + members = ["serviceAccount:service_acc1@test.com", "user:user1@test.com"] + bindings = [{"role": BIGTABLE_ADMIN_ROLE, "members": members}] + iam_policy = policy_pb2.Policy(version=version, etag=etag, bindings=bindings) + + # Patch the stub used by the API method. + instance_api = mock.create_autospec( + bigtable_instance_admin_client.BigtableInstanceAdminClient + ) + client._instance_admin_client = instance_api + instance_api.get_iam_policy.return_value = iam_policy + + # Perform the method and check the result. + result = instance.get_iam_policy(requested_policy_version=3) + + instance_api.get_iam_policy.assert_called_once_with( + resource=instance.name, + options_=options_pb2.GetPolicyOptions(requested_policy_version=3), + ) + self.assertEqual(result.version, version) + self.assertEqual(result.etag, etag) + admins = result.bigtable_admins + self.assertEqual(len(admins), len(members)) + for found, expected in zip(sorted(admins), sorted(members)): + self.assertEqual(found, expected) + def test_set_iam_policy(self): from google.cloud.bigtable_admin_v2.gapic import bigtable_instance_admin_client from google.iam.v1 import policy_pb2