From 583ca423d2fc0ca61a53d0fdcb05c960ed807c86 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 19 Nov 2019 17:19:26 -0800 Subject: [PATCH 01/11] 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 7a9bc44500f707f7f211e83d02ade1af5c54d63d Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 16 Dec 2019 16:36:27 -0800 Subject: [PATCH 02/11] feat(storage): support requested_policy_version for get_iam_policy --- storage/google/cloud/storage/bucket.py | 14 +++++++++++- storage/tests/unit/test_bucket.py | 30 ++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 14c1d873525c..cda44206a4a0 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -1861,7 +1861,7 @@ def disable_website(self): """ return self.configure_website(None, None) - def get_iam_policy(self, client=None): + def get_iam_policy(self, client=None, requested_policy_version=None): """Retrieve the IAM policy for the bucket. See @@ -1874,6 +1874,15 @@ def get_iam_policy(self, client=None): :param client: Optional. The client to use. If not passed, falls back to the ``client`` stored on the current bucket. + :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. + :rtype: :class:`google.api_core.iam.Policy` :returns: the policy instance, based on the resource returned from the ``getIamPolicy`` API request. @@ -1884,6 +1893,9 @@ def get_iam_policy(self, client=None): if self.user_project is not None: query_params["userProject"] = self.user_project + if requested_policy_version is not None: + query_params["optionsRequestedPolicyVersion"] = requested_policy_version + info = client._connection.api_request( method="GET", path="%s/iam" % (self.path,), diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 4aeada6a0586..ce70f0e5787f 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -2083,6 +2083,36 @@ def test_get_iam_policy_w_user_project(self): self.assertEqual(kw[0]["path"], "%s/iam" % (PATH,)) self.assertEqual(kw[0]["query_params"], {"userProject": USER_PROJECT}) + def test_get_iam_policy_w_requested_policy_version(self): + from google.cloud.storage.iam import STORAGE_OWNER_ROLE + from google.api_core.iam import Policy + + NAME = "name" + PATH = "/b/%s" % (NAME,) + ETAG = "DEADBEEF" + VERSION = 1 + OWNER1 = "user:phred@example.com" + OWNER2 = "group:cloud-logs@google.com" + RETURNED = { + "resourceId": PATH, + "etag": ETAG, + "version": VERSION, + "bindings": [ + {"role": STORAGE_OWNER_ROLE, "members": [OWNER1, OWNER2]}, + ], + } + connection = _Connection(RETURNED) + client = _Client(connection, None) + bucket = self._make_one(client=client, name=NAME) + + policy = bucket.get_iam_policy(requested_policy_version=3) + + kw = connection._requested + self.assertEqual(len(kw), 1) + self.assertEqual(kw[0]["method"], "GET") + self.assertEqual(kw[0]["path"], "%s/iam" % (PATH,)) + self.assertEqual(kw[0]["query_params"], {"optionsRequestedPolicyVersion": 3}) + def test_set_iam_policy(self): import operator from google.cloud.storage.iam import STORAGE_OWNER_ROLE From 33898d766304ca7d3cdd35be0bd481e8e3c749f3 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Mon, 16 Dec 2019 18:00:30 -0800 Subject: [PATCH 03/11] add system-test --- api_core/google/api_core/iam.py | 2 +- storage/tests/system.py | 55 +++++++++++++++++++++++++++++++ storage/tests/unit/test_bucket.py | 2 ++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 869b715f45cb..4de0811fcf82 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -39,7 +39,7 @@ }, { "role": "roles/viewer", - "members": {"allUsers"} + "members": {"sericeAccount:account@iam.gserviceaccount.com"} "condition": { "title": "request_time", "description": "Requests made before 2021-01-01T00:00:00Z", diff --git a/storage/tests/system.py b/storage/tests/system.py index 2bfaa5b8f492..090b6e18c5d6 100644 --- a/storage/tests/system.py +++ b/storage/tests/system.py @@ -242,6 +242,61 @@ def test_bucket_update_labels(self): bucket.update() self.assertEqual(bucket.labels, {}) + def test_get_set_iam_policy(self): + import pytest + from google.cloud.storage.iam import STORAGE_OBJECT_VIEWER_ROLE + from google.api_core.exceptions import BadRequest, PreconditionFailed + from google.api_core.iam import InvalidOperationException + + bucket_name = "iam-policy" + unique_resource_id("-") + bucket = retry_429_503(Config.CLIENT.create_bucket)(bucket_name) + self.case_buckets_to_delete.append(bucket_name) + self.assertTrue(bucket.exists()) + + policy_no_version = bucket.get_iam_policy() + self.assertEqual(policy_no_version.version, 1) + + policy = bucket.get_iam_policy(requested_policy_version=3) + self.assertEqual(policy, policy_no_version) + + member = "serviceAccount:{}".format(Config.CLIENT.get_service_account_email()) + + BINDING_W_CONDITION = { + "role": STORAGE_OBJECT_VIEWER_ROLE, + "members": {member}, + "condition": { + "title": "always-true", + "description": "test condition always-true", + "expression": "true" + } + } + policy.bindings.append(BINDING_W_CONDITION) + + with pytest.raises(PreconditionFailed, match="enable uniform bucket-level access"): + bucket.set_iam_policy(policy) + + bucket.iam_configuration.uniform_bucket_level_access_enabled = True + bucket.patch() + + policy = bucket.get_iam_policy(requested_policy_version=3) + policy.bindings.append(BINDING_W_CONDITION) + + with pytest.raises(BadRequest, match="at least 3"): + bucket.set_iam_policy(policy) + + policy.version = 3 + returned_policy = bucket.set_iam_policy(policy) + self.assertEqual(returned_policy.version, 3) + self.assertEqual(returned_policy.bindings, policy.bindings) + + with pytest.raises(BadRequest, match="cannot be less than the existing policy version"): + bucket.get_iam_policy() + with pytest.raises(BadRequest, match="cannot be less than the existing policy version"): + bucket.get_iam_policy(requested_policy_version=2) + + fetched_policy = bucket.get_iam_policy(requested_policy_version=3) + self.assertEqual(fetched_policy.bindings, returned_policy.bindings) + @unittest.skipUnless(USER_PROJECT, "USER_PROJECT not set in environment.") def test_crud_bucket_with_requester_pays(self): new_bucket_name = "w-requester-pays" + unique_resource_id("-") diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index ce70f0e5787f..2f178e43af9f 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -2107,6 +2107,8 @@ def test_get_iam_policy_w_requested_policy_version(self): policy = bucket.get_iam_policy(requested_policy_version=3) + self.assertEqual(policy.version, VERSION) + kw = connection._requested self.assertEqual(len(kw), 1) self.assertEqual(kw[0]["method"], "GET") From 2dd85e102b4f4b224587b7f7780d9769fa2d8d0f Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 17 Dec 2019 13:02:52 -0800 Subject: [PATCH 04/11] add ref doc sample to get_iam_policy --- storage/google/cloud/storage/bucket.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index cda44206a4a0..26b607da0928 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -1882,10 +1882,35 @@ def get_iam_policy(self, client=None, requested_policy_version=None): 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.api_core.iam.Policy` :returns: the policy instance, based on the resource returned from the ``getIamPolicy`` API request. + + Example: + .. code-block:: python + from google.cloud.storage.iam import STORAGE_OBJECT_VIEWER_ROLE + + policy = bucket.get_iam_policy(requested_policy_version=3) + + policy.version = 3 + + # Add a binding to the policy via it's bindings property + policy.bindings.append({ + "role": STORAGE_OBJECT_VIEWER_ROLE, + "members": {"serviceAccount:account@project.iam.gserviceaccount.com", ...}, + # Optional: + "condition": { + "title": "prefix" + "description": "Objects matching prefix" + "expression": "resource.name.startsWith(\"projects/project-name/buckets/bucket-name/objects/prefix\")" + } + }) + + bucket.set_iam_policy(policy) """ client = self._require_client(client) query_params = {} From 40770451b3f812e5d8a2ba0547c14ecf30db6706 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 17 Dec 2019 15:04:14 -0800 Subject: [PATCH 05/11] add requested_policy_version to blob --- storage/google/cloud/storage/blob.py | 17 +++++++++- storage/tests/unit/test_blob.py | 47 +++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/storage/google/cloud/storage/blob.py b/storage/google/cloud/storage/blob.py index 4d9bed1c8488..e95d1239fcb7 100644 --- a/storage/google/cloud/storage/blob.py +++ b/storage/google/cloud/storage/blob.py @@ -1456,7 +1456,7 @@ def create_resumable_upload_session( except resumable_media.InvalidResponse as exc: _raise_from_invalid_response(exc) - def get_iam_policy(self, client=None): + def get_iam_policy(self, client=None, requested_policy_version=None): """Retrieve the IAM policy for the object. .. note: @@ -1475,6 +1475,18 @@ def get_iam_policy(self, client=None): :param client: Optional. The client to use. If not passed, falls back to the ``client`` stored on the current object's bucket. + :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.api_core.iam.Policy` :returns: the policy instance, based on the resource returned from the ``getIamPolicy`` API request. @@ -1486,6 +1498,9 @@ def get_iam_policy(self, client=None): if self.user_project is not None: query_params["userProject"] = self.user_project + if requested_policy_version is not None: + query_params["optionsRequestedPolicyVersion"] = requested_policy_version + info = client._connection.api_request( method="GET", path="%s/iam" % (self.path,), diff --git a/storage/tests/unit/test_blob.py b/storage/tests/unit/test_blob.py index 2609d546ef49..d3e36a2e7e68 100644 --- a/storage/tests/unit/test_blob.py +++ b/storage/tests/unit/test_blob.py @@ -1928,7 +1928,7 @@ def test_get_iam_policy(self): BLOB_NAME = "blob-name" PATH = "/b/name/o/%s" % (BLOB_NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" EDITOR1 = "domain:google.com" @@ -1973,6 +1973,45 @@ def test_get_iam_policy(self): }, ) + def test_get_iam_policy_w_requested_policy_version(self): + from google.cloud.storage.iam import STORAGE_OWNER_ROLE + from google.api_core.iam import Policy + + BLOB_NAME = "blob-name" + PATH = "/b/name/o/%s" % (BLOB_NAME,) + ETAG = "DEADBEEF" + VERSION = 3 + OWNER1 = "user:phred@example.com" + OWNER2 = "group:cloud-logs@google.com" + RETURNED = { + "resourceId": PATH, + "etag": ETAG, + "version": VERSION, + "bindings": [{"role": STORAGE_OWNER_ROLE, "members": [OWNER1, OWNER2]}], + } + after = ({"status": http_client.OK}, RETURNED) + EXPECTED = { + binding["role"]: set(binding["members"]) for binding in RETURNED["bindings"] + } + connection = _Connection(after) + client = _Client(connection) + bucket = _Bucket(client=client) + blob = self._make_one(BLOB_NAME, bucket=bucket) + + policy = blob.get_iam_policy() + + kw = connection._requested + self.assertEqual(len(kw), 1) + self.assertEqual( + kw[0], + { + "method": "GET", + "path": "%s/iam" % (PATH,), + "query_params": {"optionsRequestedPolicyVersion": 3}, + "_target_object": None, + }, + ) + def test_get_iam_policy_w_user_project(self): from google.api_core.iam import Policy @@ -1980,7 +2019,7 @@ def test_get_iam_policy_w_user_project(self): USER_PROJECT = "user-project-123" PATH = "/b/name/o/%s" % (BLOB_NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 RETURNED = { "resourceId": PATH, "etag": ETAG, @@ -2023,7 +2062,7 @@ def test_set_iam_policy(self): BLOB_NAME = "blob-name" PATH = "/b/name/o/%s" % (BLOB_NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" EDITOR1 = "domain:google.com" @@ -2074,7 +2113,7 @@ def test_set_iam_policy_w_user_project(self): USER_PROJECT = "user-project-123" PATH = "/b/name/o/%s" % (BLOB_NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 BINDINGS = [] RETURNED = {"etag": ETAG, "version": VERSION, "bindings": BINDINGS} after = ({"status": http_client.OK}, RETURNED) From c210d2995f5767749a17db81a2d3d58f3b796f26 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Tue, 17 Dec 2019 15:11:33 -0800 Subject: [PATCH 06/11] fix tests --- storage/tests/unit/test_blob.py | 4 ++-- storage/tests/unit/test_bucket.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/storage/tests/unit/test_blob.py b/storage/tests/unit/test_blob.py index d3e36a2e7e68..e6f2dac09ea1 100644 --- a/storage/tests/unit/test_blob.py +++ b/storage/tests/unit/test_blob.py @@ -1980,7 +1980,7 @@ def test_get_iam_policy_w_requested_policy_version(self): BLOB_NAME = "blob-name" PATH = "/b/name/o/%s" % (BLOB_NAME,) ETAG = "DEADBEEF" - VERSION = 3 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" RETURNED = { @@ -1998,7 +1998,7 @@ def test_get_iam_policy_w_requested_policy_version(self): bucket = _Bucket(client=client) blob = self._make_one(BLOB_NAME, bucket=bucket) - policy = blob.get_iam_policy() + policy = blob.get_iam_policy(requested_policy_version=3) kw = connection._requested self.assertEqual(len(kw), 1) diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 2f178e43af9f..e1fb8fc7699f 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -2014,7 +2014,7 @@ def test_get_iam_policy(self): NAME = "name" PATH = "/b/%s" % (NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" EDITOR1 = "domain:google.com" @@ -2058,7 +2058,7 @@ def test_get_iam_policy_w_user_project(self): USER_PROJECT = "user-project-123" PATH = "/b/%s" % (NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 RETURNED = { "resourceId": PATH, "etag": ETAG, @@ -2125,7 +2125,7 @@ def test_set_iam_policy(self): NAME = "name" PATH = "/b/%s" % (NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" EDITOR1 = "domain:google.com" @@ -2178,7 +2178,7 @@ def test_set_iam_policy_w_user_project(self): USER_PROJECT = "user-project-123" PATH = "/b/%s" % (NAME,) ETAG = "DEADBEEF" - VERSION = 17 + VERSION = 1 OWNER1 = "user:phred@example.com" OWNER2 = "group:cloud-logs@google.com" EDITOR1 = "domain:google.com" From cc717bd9f64e726a4e6d5e5259004eefe5e0ab79 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Fri, 20 Dec 2019 13:12:20 -0800 Subject: [PATCH 07/11] nit: typo --- api_core/google/api_core/iam.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index 4de0811fcf82..77d920be6fbf 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -39,7 +39,7 @@ }, { "role": "roles/viewer", - "members": {"sericeAccount:account@iam.gserviceaccount.com"} + "members": {"serviceAccount:account@iam.gserviceaccount.com"} "condition": { "title": "request_time", "description": "Requests made before 2021-01-01T00:00:00Z", From 62b003a589835263cd91ba70c79d20dd078d529c Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 13:27:08 -0800 Subject: [PATCH 08/11] blacken --- storage/tests/system.py | 16 +++++++++++----- storage/tests/unit/test_bucket.py | 4 +--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/storage/tests/system.py b/storage/tests/system.py index f3b99e42f9a4..fe29c8a5966a 100644 --- a/storage/tests/system.py +++ b/storage/tests/system.py @@ -287,12 +287,14 @@ def test_get_set_iam_policy(self): "condition": { "title": "always-true", "description": "test condition always-true", - "expression": "true" - } + "expression": "true", + }, } policy.bindings.append(BINDING_W_CONDITION) - with pytest.raises(PreconditionFailed, match="enable uniform bucket-level access"): + with pytest.raises( + PreconditionFailed, match="enable uniform bucket-level access" + ): bucket.set_iam_policy(policy) bucket.iam_configuration.uniform_bucket_level_access_enabled = True @@ -309,9 +311,13 @@ def test_get_set_iam_policy(self): self.assertEqual(returned_policy.version, 3) self.assertEqual(returned_policy.bindings, policy.bindings) - with pytest.raises(BadRequest, match="cannot be less than the existing policy version"): + with pytest.raises( + BadRequest, match="cannot be less than the existing policy version" + ): bucket.get_iam_policy() - with pytest.raises(BadRequest, match="cannot be less than the existing policy version"): + with pytest.raises( + BadRequest, match="cannot be less than the existing policy version" + ): bucket.get_iam_policy(requested_policy_version=2) fetched_policy = bucket.get_iam_policy(requested_policy_version=3) diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 132ab0ae5026..0261bac7bf00 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -2106,9 +2106,7 @@ def test_get_iam_policy_w_requested_policy_version(self): "resourceId": PATH, "etag": ETAG, "version": VERSION, - "bindings": [ - {"role": STORAGE_OWNER_ROLE, "members": [OWNER1, OWNER2]}, - ], + "bindings": [{"role": STORAGE_OWNER_ROLE, "members": [OWNER1, OWNER2]}], } connection = _Connection(RETURNED) client = _Client(connection, None) From 70a1886273407c80f9b9ba21d83ebb4349bd733e Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 13:30:43 -0800 Subject: [PATCH 09/11] fix docs build --- storage/google/cloud/storage/bucket.py | 32 ++++++++++++++------------ 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/storage/google/cloud/storage/bucket.py b/storage/google/cloud/storage/bucket.py index 8ca8eb0a2bfe..ed275e88fcec 100644 --- a/storage/google/cloud/storage/bucket.py +++ b/storage/google/cloud/storage/bucket.py @@ -1895,26 +1895,28 @@ def get_iam_policy(self, client=None, requested_policy_version=None): the ``getIamPolicy`` API request. Example: + .. code-block:: python - from google.cloud.storage.iam import STORAGE_OBJECT_VIEWER_ROLE - policy = bucket.get_iam_policy(requested_policy_version=3) + from google.cloud.storage.iam import STORAGE_OBJECT_VIEWER_ROLE + + policy = bucket.get_iam_policy(requested_policy_version=3) - policy.version = 3 + policy.version = 3 - # Add a binding to the policy via it's bindings property - policy.bindings.append({ - "role": STORAGE_OBJECT_VIEWER_ROLE, - "members": {"serviceAccount:account@project.iam.gserviceaccount.com", ...}, - # Optional: - "condition": { - "title": "prefix" - "description": "Objects matching prefix" - "expression": "resource.name.startsWith(\"projects/project-name/buckets/bucket-name/objects/prefix\")" - } - }) + # Add a binding to the policy via it's bindings property + policy.bindings.append({ + "role": STORAGE_OBJECT_VIEWER_ROLE, + "members": {"serviceAccount:account@project.iam.gserviceaccount.com", ...}, + # Optional: + "condition": { + "title": "prefix" + "description": "Objects matching prefix" + "expression": "resource.name.startsWith(\"projects/project-name/buckets/bucket-name/objects/prefix\")" + } + }) - bucket.set_iam_policy(policy) + bucket.set_iam_policy(policy) """ client = self._require_client(client) query_params = {} From 27c3e6a6eac4b5193c95b4950ffbc4d944965760 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 13:31:45 -0800 Subject: [PATCH 10/11] format docs --- api_core/google/api_core/iam.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api_core/google/api_core/iam.py b/api_core/google/api_core/iam.py index a7a1c00e236d..f1309360304c 100644 --- a/api_core/google/api_core/iam.py +++ b/api_core/google/api_core/iam.py @@ -210,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 a17cbc1caebd6966e0e36e843ba0dbab9165ce41 Mon Sep 17 00:00:00 2001 From: Jonathan Lui Date: Thu, 9 Jan 2020 13:54:24 -0800 Subject: [PATCH 11/11] remove unused variables --- storage/tests/system.py | 1 - storage/tests/unit/test_blob.py | 6 +----- storage/tests/unit/test_bucket.py | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/storage/tests/system.py b/storage/tests/system.py index fe29c8a5966a..d689c2f2c6d2 100644 --- a/storage/tests/system.py +++ b/storage/tests/system.py @@ -266,7 +266,6 @@ def test_get_set_iam_policy(self): import pytest from google.cloud.storage.iam import STORAGE_OBJECT_VIEWER_ROLE from google.api_core.exceptions import BadRequest, PreconditionFailed - from google.api_core.iam import InvalidOperationException bucket_name = "iam-policy" + unique_resource_id("-") bucket = retry_429_503(Config.CLIENT.create_bucket)(bucket_name) diff --git a/storage/tests/unit/test_blob.py b/storage/tests/unit/test_blob.py index e6f2dac09ea1..aa3819545920 100644 --- a/storage/tests/unit/test_blob.py +++ b/storage/tests/unit/test_blob.py @@ -1975,7 +1975,6 @@ def test_get_iam_policy(self): def test_get_iam_policy_w_requested_policy_version(self): from google.cloud.storage.iam import STORAGE_OWNER_ROLE - from google.api_core.iam import Policy BLOB_NAME = "blob-name" PATH = "/b/name/o/%s" % (BLOB_NAME,) @@ -1990,15 +1989,12 @@ def test_get_iam_policy_w_requested_policy_version(self): "bindings": [{"role": STORAGE_OWNER_ROLE, "members": [OWNER1, OWNER2]}], } after = ({"status": http_client.OK}, RETURNED) - EXPECTED = { - binding["role"]: set(binding["members"]) for binding in RETURNED["bindings"] - } connection = _Connection(after) client = _Client(connection) bucket = _Bucket(client=client) blob = self._make_one(BLOB_NAME, bucket=bucket) - policy = blob.get_iam_policy(requested_policy_version=3) + blob.get_iam_policy(requested_policy_version=3) kw = connection._requested self.assertEqual(len(kw), 1) diff --git a/storage/tests/unit/test_bucket.py b/storage/tests/unit/test_bucket.py index 0261bac7bf00..68399b3c8962 100644 --- a/storage/tests/unit/test_bucket.py +++ b/storage/tests/unit/test_bucket.py @@ -2094,7 +2094,6 @@ def test_get_iam_policy_w_user_project(self): def test_get_iam_policy_w_requested_policy_version(self): from google.cloud.storage.iam import STORAGE_OWNER_ROLE - from google.api_core.iam import Policy NAME = "name" PATH = "/b/%s" % (NAME,)