feat(storage): public access prevention#7423
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
google-cla
Bot
added
the
cla: yes
frankyn
force-pushed
the
publicaccessprevention
branch
5 times, most recently
from
1bfc1c5 to
9287c98
Compare
frankyn
changed the title
frankyn
added
the
status: blocked
| Storage storage = StorageOptions.newBuilder().setProjectId(projectId).build().getService(); | ||
| Bucket bucket = | ||
| storage.get( | ||
| bucketName, Storage.BucketGetOption.fields(Storage.BucketField.IAMCONFIGURATION)); |
There was a problem hiding this comment.
This field should be present regardless (unless something in Java specifically works differently).
There was a problem hiding this comment.
Testing, I thought only a subset of fields were selected in the Java client. I'll confirm.
There was a problem hiding this comment.
Field is always present! My mistake and fixed.
| public void testSetPublicAccessPreventionEnforced() { | ||
| SetPublicAccessPreventionEnforced.setPublicAccessPreventionEnforced(PROJECT_ID, BUCKET); | ||
| assertEquals( | ||
| storage.get(BUCKET).getIamConfiguration().getPublicAccessPrevention(), |
There was a problem hiding this comment.
Does this re-fetch from GCS?
| } | ||
|
|
||
| @Test | ||
| public void testSetPublicAccessPreventionUnspecified() { |
There was a problem hiding this comment.
Should these tests be verifying public access prevention without the API call to getPublicAccessPrevention? This is testing the setPublicAccessPreventionUnspecified function, but also relies on the functionality of getPublicAccessPrevention. Not sure what practices are on how small a unit should be for each unit test.
There was a problem hiding this comment.
Fair point, I'll decouple.
There was a problem hiding this comment.
Ah for these tests, I keep them simple for samples. This case is covered now by library integration tests.
product-auto-label
Bot
added
the
api: storage
BenWhitehead
left a comment
BenWhitehead
left a comment
There was a problem hiding this comment.
Couple small questions, but otherwise LGTM
| assertEquals( | ||
| storage.get(BUCKET).getIamConfiguration().getPublicAccessPrevention(), | ||
| BucketInfo.PublicAccessPrevention.ENFORCED); | ||
| storage |
There was a problem hiding this comment.
I think this call chain setting the Bucket back to UNSPECIFIED should be in a finally block so that it will execute even if the above sample run or assertion fails for any reason.
|
|
||
| @Test | ||
| public void testSetPublicAccessPreventionEnforced() { | ||
| SetPublicAccessPreventionEnforced.setPublicAccessPreventionEnforced(PROJECT_ID, BUCKET); |
There was a problem hiding this comment.
Can we call out here why we don't need to first set the bucket to UNSPECIFIED before this line? (In the test case for unspecified it does set enforced before setting UNSPACIFIED)
There was a problem hiding this comment.
attempted to address.
|
We would like to merge these samples next week after the client library has been released. Does that work for you? |
|
Public access prevention rollout has been delayed due to a bug surfaced during Googler preview. I will keep this PR updated as I learn new release timeline details. |
|
@BenWhitehead These samples can now be merged and released. |
BenWhitehead
force-pushed
the
publicaccessprevention
branch
from
173f558 to
7db7b43
Compare
4 participants
Samples for Public Access Prevention.
Blocked until PR (googleapis/java-storage#636) is released.