google.api_core.iam.Policy.__getitem__ does not correctly save empty bindings

[rvandegrift]

We recently tried to upgrade a tool from 1.15.0 to 1.26.1 and found it breaks some code that handles IAM policies. The commit that introduces the bug was released in 1.16.0: fd47fda#diff-7cc73ea72342c139ff54060be9ff25b2f792f9225e0cc0f501dca9dbed9c4741 -

The new __getitem__ implementation returns a new empty set() for roles not in the current policy. But it doesn't save that set in the bindings. So if the user manipulates it, the policy isn't actually updated. That breaks code written like this:

policy = resource.get_iam_policy()
policy['roles/storage.objectAdmin'].add(principal)
bucket.set_iam_policy(policy)

This worked fine on v1.15.0 because of the use of defaultdict. But now, this adds the principal to a set that's not used by the policy.

Something like the following (untested) patch should do the trick:

diff --git a/google/api_core/iam.py b/google/api_core/iam.py
index f130936..d650336 100644
--- a/google/api_core/iam.py
+++ b/google/api_core/iam.py
@@ -136,7 +136,9 @@ class Policy(collections_abc.MutableMapping):
         for b in self._bindings:
             if b["role"] == key:
                 return b["members"]
-        return set()
+
+        self[key] = set()
+        return self[key]

     def __setitem__(self, key, value):
         self.__check_version__()

[jceresini]

I wanted to add that, the example code also does not fail, so the issue was identified elsewhere and troubleshooting the cause was quite difficult.

What I mean is, since policy['roles/storage.objectAdmin'].add(principal) doesn't actually modify the bindings, the subsequent call to set_iam_policy submits an unchanged policy to the IAM api, which returns a successful response but changes nothing.

[busunkim96]

Thank you for the report! @rvandegrift @jceresini

CC @frankyn since I believe this bit of code is used in the storage library.