Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Storage: add 'bucket_policy_only' IAM property #7066

Merged
merged 14 commits into from
Feb 6, 2019
Merged

Storage: add 'bucket_policy_only' IAM property #7066

merged 14 commits into from
Feb 6, 2019

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jan 7, 2019

@frankyn Still working on system tests. I can test creating a new bucket with BPO enabled, but am unable to update a newly-created non-BPO bucket to have BPO set: the PATCH request returns:

           google.api_core.exceptions.BadRequest: 400 PATCH https://www.googleapis.com/storage/v1/b/set-bpo-1546903462976?projection=full: Cannot disable object policies because default object acls are not empty

@tseaver tseaver added api: storage Issues related to the Cloud Storage API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 7, 2019
@tseaver tseaver requested a review from frankyn January 7, 2019 23:33
@tseaver tseaver requested a review from crwilcox as a code owner January 7, 2019 23:33
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 7, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Jan 8, 2019

@frankyn I've added system tests exercising the feature. PTAL.

frankyn
frankyn suggested changes Jan 9, 2019
View reviewed changes
storage/tests/system.py Outdated Show resolved Hide resolved
storage/tests/system.py Outdated Show resolved Hide resolved
storage/tests/system.py Outdated
with self.assertRaises(exceptions.BadRequest):
bucket.acl.reload()

# XXX This should raise, but doesn't as of 2018-01-08T19:00:00Z

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

storage/google/cloud/storage/bucket.py
@@ -272,6 +272,82 @@ def from_api_repr(cls, resource):
return instance


class IAMConfiguration(dict):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3b43092

storage/tests/system.py Outdated
#with self.assertRaises(exceptions.BadRequest):
# bucket.acl.clear()

# XXX The blob ACL get / set stuff raises 403, rather than 400.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frankyn 693838f Removes the additional / problematic assertions about error responses for other blob / bucket ACL operations under BPO.

@tseaver
Copy link
Contributor Author

tseaver commented Jan 17, 2019

@frankyn PTAL.

frankyn
frankyn suggested changes Jan 19, 2019
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

two requests on related documentation. Thanks @tseaver overall LGTM. Please don't merge until given the OK.

storage/google/cloud/storage/bucket.py Outdated
"""When was the bucket configured to allow only IAM policy?

:rtype: Union[:class:`datetime.datetime`, None]
:returns: (readonly) the time the bucket's IAM-only policy was set.
Copy link
Member

Choose a reason for hiding this comment

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

Use discovery document description for lockedTime:
The deadline time for changing iamConfiguration.bucketPolicyOnly.enabled from true to false in RFC 3339 format. iamConfiguration.bucketPolicyOnly.enabled may be changed from true to false until the locked time, after which the field is immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c071753

storage/google/cloud/storage/bucket.py Outdated

@property
def bucket_policy_only(self):
"""Is the bucket configured to allow only IAM policy?
Copy link
Member

Choose a reason for hiding this comment

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

From discovery document. "If set, access checks only use bucket-level IAM policies or above."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c071753

@tseaver
Copy link
Contributor Author

tseaver commented Jan 21, 2019

@frankyn Please remove the 'do not merge' label when the back-end is ready for me to merge this PR.

@frankyn
Copy link
Member

frankyn commented Jan 22, 2019

Acking, thank you @tseaver!

@tseaver tseaver changed the title [WIP] Storage: bucket policy only feature Storage: bucket policy only feature Jan 22, 2019
@tseaver tseaver changed the title Storage: bucket policy only feature Storage: add 'bucket_policy_only' IAM property Jan 22, 2019
frankyn
frankyn reviewed Jan 22, 2019
View reviewed changes
storage/google/cloud/storage/bucket.py Outdated
self.bucket._patch_property("iamConfiguration", self)

@property
def locked_time(self):
Copy link
Member

@frankyn frankyn Jan 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @tseaver, I just realized this isn't scoped according to BucketPolicyOnly only. Could you move this to be at the same context of bucket_policy_only?

I'm writing samples and I missed this during review. I'm thinking it should have the following pattern.

iam_configuration.bucket_policy_only_enabled
iam_configuration.bucket_policy_only_locked_time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c84f099

@frankyn
Copy link
Member

frankyn commented Jan 23, 2019

Thanks @tseaver LGTM. I'll remove the label when it's time to release.

frankyn
frankyn approved these changes Feb 6, 2019
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

The whitelist is no longer required. LGTM'ing.

@frankyn frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 6, 2019
@tseaver tseaver merged commit f9f05fa into googleapis:master Feb 6, 2019
@tseaver tseaver deleted the storage-bucket_policy_only-feature branch February 6, 2019 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chestercun chestercun chestercun left review comments

@frankyn frankyn frankyn approved these changes

@crwilcox crwilcox Awaiting requested review from crwilcox

Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tseaver @frankyn @chestercun @googlebot