Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 59 additions & 14 deletions storage/google/cloud/storage/bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,12 @@ class IAMConfiguration(dict):
:type bucket: :class:`Bucket`
:params bucket: Bucket for which this instance is the policy.

:type uniform_bucket_level_access_enabled: bool
:params bucket_policy_only_enabled: (optional) whether the IAM-only policy is enabled for the bucket.

:type uniform_bucket_level_locked_time: :class:`datetime.datetime`
:params uniform_bucket_level_locked_time: (optional) When the bucket's IAM-only policy was ehabled. This value should normally only be set by the back-end API.

:type bucket_policy_only_enabled: bool
:params bucket_policy_only_enabled: (optional) whether the IAM-only policy is enabled for the bucket.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should bucket_policy_only_enabled and bucket_policy_only_locked_time be documented as deprecated?


Expand All @@ -292,12 +298,32 @@ def __init__(
bucket,
bucket_policy_only_enabled=False,
bucket_policy_only_locked_time=None,
uniform_bucket_level_access_enabled=False,
uniform_bucket_level_access_locked_time=None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering should match the order documented above. The constructor probably needs to check for / resolve conflicts between uniform_bucket_level_access_enabled / bucket_policy_only_enabled and uniform_bucket_level_access_locked_time / bucket_policy_only_locked_time first, which would simplify the code setting up data.

):
data = {"bucketPolicyOnly": {"enabled": bucket_policy_only_enabled}}
data = {"bucketPolicyOnly": {}, "uniformBucketLevelAccess": {}}
if bucket_policy_only_enabled:
data["bucketPolicyOnly"]["enabled"] = bucket_policy_only_enabled
data["uniformBucketLevelAccess"]["enabled"] = bucket_policy_only_enabled
if bucket_policy_only_locked_time is not None:
data["bucketPolicyOnly"]["lockedTime"] = _datetime_to_rfc3339(
bucket_policy_only_locked_time
)
data["bucketPolicyOnly"]["lockedTime"] = _datetime_to_rfc3339(
bucket_policy_only_locked_time
)
data["uniformBucketLevelAccess"]["lockedTime"] = _datetime_to_rfc3339(
bucket_policy_only_locked_time
)

if uniform_bucket_level_access_enabled:
data["bucketPolicyOnly"]["enabled"] = uniform_bucket_level_access_enabled
data["uniformBucketLevelAccess"]["enabled"] = uniform_bucket_level_access_enabled
if uniform_bucket_level_access_locked_time is not None:
data["bucketPolicyOnly"]["lockedTime"] = _datetime_to_rfc3339(
uniform_bucket_level_access_locked_time
)
data["uniformBucketLevelAccess"]["lockedTime"] = _datetime_to_rfc3339(
uniform_bucket_level_access_locked_time
)

super(IAMConfiguration, self).__init__(data)
self._bucket = bucket

Expand All @@ -314,6 +340,7 @@ def from_api_repr(cls, resource, bucket):
:rtype: :class:`IAMConfiguration`
:returns: Instance created from resource.
"""

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray line.

instance = cls(bucket)
instance.update(resource)
return instance
Expand All @@ -329,36 +356,54 @@ def bucket(self):

@property
def bucket_policy_only_enabled(self):
"""Alias for uniform_bucket_level_access_enabled"""
self.uniform_bucket_level_access_enabled()

@property
def uniform_bucket_level_access_enabled(self):
"""If set, access checks only use bucket-level IAM policies or above.

:rtype: bool
:returns: whether the bucket is configured to allow only IAM.
"""
bpo = self.get("bucketPolicyOnly", {})
return bpo.get("enabled", False)
ubla = self.get("uniformBucketLevelAccess", {})
return ubla.get("enabled", False)

@bucket_policy_only_enabled.setter
def bucket_policy_only_enabled(self, value):
@uniform_bucket_level_access_enabled.setter
def uniform_bucket_level_access_enabled(self, value):
ubla = self.setdefault("uniformBucketLevelAccess", {})
ubla["enabled"] = bool(value)
#### THIS IS A WORKAROUND ####
bpo = self.setdefault("bucketPolicyOnly", {})
bpo["enabled"] = bool(value)
#### THIS IS A WORKAROUND ####
self.bucket._patch_property("iamConfiguration", self)

@bucket_policy_only_enabled.setter
def bucket_policy_only_enabled(self, value):
self.uniform_bucket_level_access_enabled(value)

@property
def bucket_policy_only_locked_time(self):
"""Deadline for changing :attr:`bucket_policy_only_enabled` from true to false.
"""Alias for uniform_bucket_level_access."""
return self.uniform_bucket_level_access_locked_time()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't call the property, just return it, e.g.:

Suggested change
return self.uniform_bucket_level_access_locked_time()
return self.uniform_bucket_level_access_locked_time


@property
def uniform_bucket_level_access_locked_time(self):
"""Deadline for changing :attr:`uniform_bucket_level_access_enabled` from true to false.

If the bucket's :attr:`bucket_policy_only_enabled` is true, this property
If the bucket's :attr:`uniform_bucket_level_access_enabled` is true, this property
is time time after which that setting becomes immutable.

If the bucket's :attr:`bucket_policy_only_enabled` is false, this property
If the bucket's :attr:`uniform_bucket_level_access_enabled` is false, this property
is ``None``.

:rtype: Union[:class:`datetime.datetime`, None]
:returns: (readonly) Time after which :attr:`bucket_policy_only_enabled` will
:returns: (readonly) Time after which :attr:`uniform_bucket_level_access_enabled` will
be frozen as true.
"""
bpo = self.get("bucketPolicyOnly", {})
stamp = bpo.get("lockedTime")
ubla = self.get("uniformBucketLevelAccess", {})
stamp = ubla.get("lockedTime")
if stamp is not None:
stamp = _rfc3339_to_datetime(stamp)
return stamp
Expand Down
26 changes: 13 additions & 13 deletions storage/tests/system.py
Original file line number Diff line number Diff line change
Expand Up @@ -1647,13 +1647,13 @@ def tearDown(self):
bucket = Config.CLIENT.bucket(bucket_name)
retry_429_harder(bucket.delete)(force=True)

def test_new_bucket_w_bpo(self):
new_bucket_name = "new-w-bpo" + unique_resource_id("-")
def test_new_bucket_w_ubla(self):
new_bucket_name = "new-w-ubla" + unique_resource_id("-")
self.assertRaises(
exceptions.NotFound, Config.CLIENT.get_bucket, new_bucket_name
)
bucket = Config.CLIENT.bucket(new_bucket_name)
bucket.iam_configuration.bucket_policy_only_enabled = True
bucket.iam_configuration.uniform_bucket_level_access_enabled = True
retry_429(bucket.create)()
self.case_buckets_to_delete.append(new_bucket_name)

Expand Down Expand Up @@ -1683,9 +1683,8 @@ def test_new_bucket_w_bpo(self):
with self.assertRaises(exceptions.BadRequest):
blob_acl.save()

@unittest.skipUnless(False, "Back-end fix for BPO/UBLA expected 2019-07-12")
def test_bpo_set_unset_preserves_acls(self):
new_bucket_name = "bpo-acls" + unique_resource_id("-")
new_bucket_name = "ubla-acls" + unique_resource_id("-")
self.assertRaises(
exceptions.NotFound, Config.CLIENT.get_bucket, new_bucket_name
)
Expand All @@ -1697,29 +1696,30 @@ def test_bpo_set_unset_preserves_acls(self):
payload = b"DEADBEEF"
blob.upload_from_string(payload)

# Preserve ACLs before setting BPO
# Preserve ACLs before setting UBLA
bucket_acl_before = list(bucket.acl)
blob_acl_before = list(bucket.acl)

# Set BPO
bucket.iam_configuration.bucket_policy_only_enabled = True
# Set UBLA
bucket.iam_configuration.uniform_bucket_level_access_enabled = True
bucket.patch()

self.assertTrue(bucket.iam_configuration.bucket_policy_only_enabled)
self.assertTrue(bucket.iam_configuration.uniform_bucket_level_access_enabled)

# While BPO is set, cannot get / set ACLs
# While UBLA is set, cannot get / set ACLs
with self.assertRaises(exceptions.BadRequest):
bucket.acl.reload()

# Clear BPO
bucket.iam_configuration.bucket_policy_only_enabled = False
# Clear UBLA
bucket.iam_configuration.uniform_bucket_level_access_enabled = False
bucket.patch()

# Query ACLs after clearing BPO
# Query ACLs after clearing UBLA
bucket.acl.reload()
bucket_acl_after = list(bucket.acl)
blob.acl.reload()
blob_acl_after = list(bucket.acl)

self.assertEqual(bucket_acl_before, bucket_acl_after)
self.assertEqual(blob_acl_before, blob_acl_after)

50 changes: 25 additions & 25 deletions storage/tests/unit/test_bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ def test_ctor_defaults(self):
config = self._make_one(bucket)

self.assertIs(config.bucket, bucket)
self.assertFalse(config.bucket_policy_only_enabled)
self.assertIsNone(config.bucket_policy_only_locked_time)
self.assertFalse(config.uniform_bucket_level_access_enabled)
self.assertIsNone(config.uniform_bucket_level_access_locked_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to retain tests for the deprecated properties (which would have exposed the "can't call properties" bugs I noted above).


def test_ctor_explicit(self):
import datetime
Expand All @@ -208,12 +208,12 @@ def test_ctor_explicit(self):
now = datetime.datetime.utcnow().replace(tzinfo=pytz.UTC)

config = self._make_one(
bucket, bucket_policy_only_enabled=True, bucket_policy_only_locked_time=now
bucket, uniform_bucket_level_access_enabled=True, uniform_bucket_level_access_locked_time=now
)

self.assertIs(config.bucket, bucket)
self.assertTrue(config.bucket_policy_only_enabled)
self.assertEqual(config.bucket_policy_only_locked_time, now)
self.assertTrue(config.uniform_bucket_level_access_enabled)
self.assertEqual(config.uniform_bucket_level_access_locked_time, now)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, we need to continue to assert values for the deprecated properties here.

We also need separate testcases for passing bucket_policy_only_enabled / bucket_policy_only_locked_time to the constructor.


def test_from_api_repr_w_empty_resource(self):
klass = self._get_target_class()
Expand All @@ -223,30 +223,30 @@ def test_from_api_repr_w_empty_resource(self):
config = klass.from_api_repr(resource, bucket)

self.assertIs(config.bucket, bucket)
self.assertFalse(config.bucket_policy_only_enabled)
self.assertIsNone(config.bucket_policy_only_locked_time)
self.assertFalse(config.uniform_bucket_level_access_enabled)
self.assertIsNone(config.uniform_bucket_level_access_locked_time)

def test_from_api_repr_w_empty_bpo(self):
def test_from_api_repr_w_empty_ubla(self):
klass = self._get_target_class()
bucket = self._make_bucket()
resource = {"bucketPolicyOnly": {}}
resource = {"uniformBucketLevelAccess": {}}

config = klass.from_api_repr(resource, bucket)

self.assertIs(config.bucket, bucket)
self.assertFalse(config.bucket_policy_only_enabled)
self.assertIsNone(config.bucket_policy_only_locked_time)
self.assertFalse(config.uniform_bucket_level_access_enabled)
self.assertIsNone(config.uniform_bucket_level_access_locked_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to retain the assertions about the deprecated properties here.

Also, do we need a separate testcase for a resource which contains only an empty bucketPolicyOnly mapping? I.e., can we guarantee that the back-end won't return such a resource?


def test_from_api_repr_w_disabled(self):
klass = self._get_target_class()
bucket = self._make_bucket()
resource = {"bucketPolicyOnly": {"enabled": False}}
resource = {"uniformBucketLevelAccess": {"enabled": False}}

config = klass.from_api_repr(resource, bucket)

self.assertIs(config.bucket, bucket)
self.assertFalse(config.bucket_policy_only_enabled)
self.assertIsNone(config.bucket_policy_only_locked_time)
self.assertFalse(config.uniform_bucket_level_access_enabled)
self.assertIsNone(config.uniform_bucket_level_access_locked_time)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve the assertions about the deprecated properties here, too.

Also, do we need a separate testcase for a resource which contains only a populated bucketPolicyOnly mapping? I.e., can we guarantee that the back-end won't return such a resource?


def test_from_api_repr_w_enabled(self):
import datetime
Expand All @@ -257,7 +257,7 @@ def test_from_api_repr_w_enabled(self):
bucket = self._make_bucket()
now = datetime.datetime.utcnow().replace(tzinfo=pytz.UTC)
resource = {
"bucketPolicyOnly": {
"uniformBucketLevelAccess": {
"enabled": True,
"lockedTime": _datetime_to_rfc3339(now),
}
Expand All @@ -266,16 +266,16 @@ def test_from_api_repr_w_enabled(self):
config = klass.from_api_repr(resource, bucket)

self.assertIs(config.bucket, bucket)
self.assertTrue(config.bucket_policy_only_enabled)
self.assertEqual(config.bucket_policy_only_locked_time, now)
self.assertTrue(config.uniform_bucket_level_access_enabled)
self.assertEqual(config.uniform_bucket_level_access_locked_time, now)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve the assertions about the deprecated properties here, too.

Also, do we need a separate testcase for a resource which contains only a populated bucketPolicyOnly mapping? I.e., can we guarantee that the back-end won't return such a resource?


def test_bucket_policy_only_enabled_setter(self):
def test_uniform_bucket_level_access_enabled_setter(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to preserve separate test cases for the deprecated properties' setters.

bucket = self._make_bucket()
config = self._make_one(bucket)

config.bucket_policy_only_enabled = True
config.uniform_bucket_level_access_enabled = True

self.assertTrue(config["bucketPolicyOnly"]["enabled"])
self.assertTrue(config["uniformBucketLevelAccess"]["enabled"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve the assertion about the deprecated prroperty here, too.

bucket._patch_property.assert_called_once_with("iamConfiguration", config)


Expand Down Expand Up @@ -1249,8 +1249,8 @@ def test_iam_configuration_policy_missing(self):

self.assertIsInstance(config, IAMConfiguration)
self.assertIs(config.bucket, bucket)
self.assertFalse(config.bucket_policy_only_enabled)
self.assertIsNone(config.bucket_policy_only_locked_time)
self.assertFalse(config.uniform_bucket_level_access_enabled)
self.assertIsNone(config.uniform_bucket_level_access_locked_time)

def test_iam_configuration_policy_w_entry(self):
import datetime
Expand All @@ -1262,7 +1262,7 @@ def test_iam_configuration_policy_w_entry(self):
NAME = "name"
properties = {
"iamConfiguration": {
"bucketPolicyOnly": {
"uniformBucketLevelAccess": {
"enabled": True,
"lockedTime": _datetime_to_rfc3339(now),
}
Expand All @@ -1274,8 +1274,8 @@ def test_iam_configuration_policy_w_entry(self):

self.assertIsInstance(config, IAMConfiguration)
self.assertIs(config.bucket, bucket)
self.assertTrue(config.bucket_policy_only_enabled)
self.assertEqual(config.bucket_policy_only_locked_time, now)
self.assertTrue(config.uniform_bucket_level_access_enabled)
self.assertEqual(config.uniform_bucket_level_access_locked_time, now)

def test_lifecycle_rules_getter_unknown_action_type(self):
NAME = "name"
Expand Down