From 57425e32fcb288633a886a9ae0990a7f27bdfc5c Mon Sep 17 00:00:00 2001 From: athakor Date: Thu, 26 Nov 2020 09:55:44 +0530 Subject: [PATCH 01/18] feat: add support of public access prevention --- .../com/google/cloud/storage/BucketInfo.java | 27 ++++++++++++++++++- .../google/cloud/storage/BucketInfoTest.java | 4 +++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 602be08ac9..752ba7a280 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -112,6 +112,7 @@ public static class IamConfiguration implements Serializable { private Boolean isUniformBucketLevelAccessEnabled; private Long uniformBucketLevelAccessLockedTime; + private String publicAccessPrevention; @Override public boolean equals(Object o) { @@ -125,12 +126,16 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(isUniformBucketLevelAccessEnabled, uniformBucketLevelAccessLockedTime); + return Objects.hash( + isUniformBucketLevelAccessEnabled, + uniformBucketLevelAccessLockedTime, + publicAccessPrevention); } private IamConfiguration(Builder builder) { this.isUniformBucketLevelAccessEnabled = builder.isUniformBucketLevelAccessEnabled; this.uniformBucketLevelAccessLockedTime = builder.uniformBucketLevelAccessLockedTime; + this.publicAccessPrevention = builder.publicAccessPrevention; } public static Builder newBuilder() { @@ -141,6 +146,7 @@ public Builder toBuilder() { Builder builder = new Builder(); builder.isUniformBucketLevelAccessEnabled = isUniformBucketLevelAccessEnabled; builder.uniformBucketLevelAccessLockedTime = uniformBucketLevelAccessLockedTime; + builder.publicAccessPrevention = publicAccessPrevention; return builder; } @@ -164,6 +170,10 @@ public Long getUniformBucketLevelAccessLockedTime() { return uniformBucketLevelAccessLockedTime; } + public String getPublicAccessPrevention() { + return publicAccessPrevention; + } + Bucket.IamConfiguration toPb() { Bucket.IamConfiguration iamConfiguration = new Bucket.IamConfiguration(); @@ -176,6 +186,7 @@ Bucket.IamConfiguration toPb() { : new DateTime(uniformBucketLevelAccessLockedTime)); iamConfiguration.setUniformBucketLevelAccess(uniformBucketLevelAccess); + iamConfiguration.setPublicAccessPrevention(publicAccessPrevention); return iamConfiguration; } @@ -188,6 +199,7 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { return newBuilder() .setIsUniformBucketLevelAccessEnabled(uniformBucketLevelAccess.getEnabled()) .setUniformBucketLevelAccessLockedTime(lockedTime == null ? null : lockedTime.getValue()) + .setPublicAccessPrevention(iamConfiguration.getPublicAccessPrevention()) .build(); } @@ -195,6 +207,7 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { public static class Builder { private Boolean isUniformBucketLevelAccessEnabled; private Long uniformBucketLevelAccessLockedTime; + private String publicAccessPrevention; /** Deprecated in favor of setIsUniformBucketLevelAccessEnabled(). */ @Deprecated @@ -235,6 +248,18 @@ Builder setUniformBucketLevelAccessLockedTime(Long uniformBucketLevelAccessLocke return this; } + /** + * Sets the bucket's Public Access Prevention configuration. Currently, 'unspecified' and + * 'enforced' are supported. + * + * @see public-access-prevention + */ + public Builder setPublicAccessPrevention(String publicAccessPrevention) { + this.publicAccessPrevention = publicAccessPrevention; + return this; + } + /** Builds an {@code IamConfiguration} object */ public IamConfiguration build() { return new IamConfiguration(this); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 8def7c306d..7521bdb8a5 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -72,10 +72,12 @@ public class BucketInfoTest { LifecycleAction.newDeleteAction(), LifecycleCondition.newBuilder().setAge(5).build())); private static final String INDEX_PAGE = "index.html"; + private static final String PUBLIC_ACCESS_PREVENTION_ENFORCED = "enforced"; private static final BucketInfo.IamConfiguration IAM_CONFIGURATION = BucketInfo.IamConfiguration.newBuilder() .setIsUniformBucketLevelAccessEnabled(true) .setUniformBucketLevelAccessLockedTime(System.currentTimeMillis()) + .setPublicAccessPrevention(PUBLIC_ACCESS_PREVENTION_ENFORCED) .build(); private static final BucketInfo.Logging LOGGING = BucketInfo.Logging.newBuilder() @@ -358,11 +360,13 @@ public void testIamConfiguration() { BucketInfo.IamConfiguration.newBuilder() .setIsUniformBucketLevelAccessEnabled(true) .setUniformBucketLevelAccessLockedTime(System.currentTimeMillis()) + .setPublicAccessPrevention(PUBLIC_ACCESS_PREVENTION_ENFORCED) .build() .toPb(); assertEquals(Boolean.TRUE, iamConfiguration.getUniformBucketLevelAccess().getEnabled()); assertNotNull(iamConfiguration.getUniformBucketLevelAccess().getLockedTime()); + assertEquals(PUBLIC_ACCESS_PREVENTION_ENFORCED, iamConfiguration.getPublicAccessPrevention()); } @Test From 29b8e138e157b3b4f1f244c0ec8b4648a75afb5c Mon Sep 17 00:00:00 2001 From: athakor Date: Thu, 3 Dec 2020 12:05:38 +0530 Subject: [PATCH 02/18] feat: add integration test and fix review changes --- .../com/google/cloud/storage/BucketInfo.java | 42 ++++++++++++---- .../google/cloud/storage/BucketInfoTest.java | 8 +-- .../cloud/storage/it/ITStorageTest.java | 49 +++++++++++++++++++ 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 752ba7a280..c5f4362711 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -101,6 +101,21 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) private final String locationType; private final Logging logging; + public enum PublicAccessPrevention { + ENFORCED("enforced"), + UNSPECIFIED("unspecified"); + + private final String value; + + PublicAccessPrevention(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + } + /** * The Bucket's IAM Configuration. * @@ -112,7 +127,7 @@ public static class IamConfiguration implements Serializable { private Boolean isUniformBucketLevelAccessEnabled; private Long uniformBucketLevelAccessLockedTime; - private String publicAccessPrevention; + private PublicAccessPrevention publicAccessPrevention; @Override public boolean equals(Object o) { @@ -170,7 +185,8 @@ public Long getUniformBucketLevelAccessLockedTime() { return uniformBucketLevelAccessLockedTime; } - public String getPublicAccessPrevention() { + /** Returns the Public Access Prevention. * */ + public PublicAccessPrevention getPublicAccessPrevention() { return publicAccessPrevention; } @@ -186,7 +202,8 @@ Bucket.IamConfiguration toPb() { : new DateTime(uniformBucketLevelAccessLockedTime)); iamConfiguration.setUniformBucketLevelAccess(uniformBucketLevelAccess); - iamConfiguration.setPublicAccessPrevention(publicAccessPrevention); + iamConfiguration.setPublicAccessPrevention( + publicAccessPrevention == null ? null : publicAccessPrevention.getValue()); return iamConfiguration; } @@ -195,11 +212,15 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { Bucket.IamConfiguration.UniformBucketLevelAccess uniformBucketLevelAccess = iamConfiguration.getUniformBucketLevelAccess(); DateTime lockedTime = uniformBucketLevelAccess.getLockedTime(); + String publicAccessPrevention = iamConfiguration.getPublicAccessPrevention(); return newBuilder() .setIsUniformBucketLevelAccessEnabled(uniformBucketLevelAccess.getEnabled()) .setUniformBucketLevelAccessLockedTime(lockedTime == null ? null : lockedTime.getValue()) - .setPublicAccessPrevention(iamConfiguration.getPublicAccessPrevention()) + .setPublicAccessPrevention( + publicAccessPrevention == null + ? null + : PublicAccessPrevention.valueOf(publicAccessPrevention)) .build(); } @@ -207,7 +228,7 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { public static class Builder { private Boolean isUniformBucketLevelAccessEnabled; private Long uniformBucketLevelAccessLockedTime; - private String publicAccessPrevention; + private PublicAccessPrevention publicAccessPrevention; /** Deprecated in favor of setIsUniformBucketLevelAccessEnabled(). */ @Deprecated @@ -249,14 +270,17 @@ Builder setUniformBucketLevelAccessLockedTime(Long uniformBucketLevelAccessLocke } /** - * Sets the bucket's Public Access Prevention configuration. Currently, 'unspecified' and - * 'enforced' are supported. + * Sets the bucket's Public Access Prevention configuration. Currently supported options are + * 'unspecified' (default) or 'enforced'. * * @see public-access-prevention */ - public Builder setPublicAccessPrevention(String publicAccessPrevention) { - this.publicAccessPrevention = publicAccessPrevention; + public Builder setPublicAccessPrevention(PublicAccessPrevention publicAccessPrevention) { + this.publicAccessPrevention = + publicAccessPrevention == null + ? publicAccessPrevention.UNSPECIFIED + : publicAccessPrevention; return this; } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 7521bdb8a5..115f101b07 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -77,7 +77,7 @@ public class BucketInfoTest { BucketInfo.IamConfiguration.newBuilder() .setIsUniformBucketLevelAccessEnabled(true) .setUniformBucketLevelAccessLockedTime(System.currentTimeMillis()) - .setPublicAccessPrevention(PUBLIC_ACCESS_PREVENTION_ENFORCED) + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) .build(); private static final BucketInfo.Logging LOGGING = BucketInfo.Logging.newBuilder() @@ -360,13 +360,15 @@ public void testIamConfiguration() { BucketInfo.IamConfiguration.newBuilder() .setIsUniformBucketLevelAccessEnabled(true) .setUniformBucketLevelAccessLockedTime(System.currentTimeMillis()) - .setPublicAccessPrevention(PUBLIC_ACCESS_PREVENTION_ENFORCED) + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) .build() .toPb(); assertEquals(Boolean.TRUE, iamConfiguration.getUniformBucketLevelAccess().getEnabled()); assertNotNull(iamConfiguration.getUniformBucketLevelAccess().getLockedTime()); - assertEquals(PUBLIC_ACCESS_PREVENTION_ENFORCED, iamConfiguration.getPublicAccessPrevention()); + assertEquals( + BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), + iamConfiguration.getPublicAccessPrevention()); } @Test diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 80685a2775..336c46739f 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3235,6 +3235,55 @@ public void testEnableAndDisableUniformBucketLevelAccessOnExistingBucket() throw } } + @Test + public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Exception { + String papBucket = RemoteStorageHelper.generateBucketName(); + try { + BucketInfo.IamConfiguration iamConfiguration = + BucketInfo.IamConfiguration.newBuilder() + .setIsUniformBucketLevelAccessEnabled(true) + .build(); + Bucket bucket = + storage.create( + Bucket.newBuilder(papBucket).setIamConfiguration(iamConfiguration).build()); + assertEquals( + BucketInfo.PublicAccessPrevention.UNSPECIFIED.getValue(), + bucket.getIamConfiguration().getPublicAccessPrevention().getValue()); + assertTrue(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + + bucket + .toBuilder() + .setIamConfiguration( + iamConfiguration + .toBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) + .build()) + .build() + .update(); + Bucket remoteBucket = + storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); + assertEquals( + BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), + remoteBucket.getIamConfiguration().getPublicAccessPrevention().getValue()); + + remoteBucket + .toBuilder() + .setIamConfiguration( + iamConfiguration.toBuilder().setIsUniformBucketLevelAccessEnabled(false).build()) + .build() + .update(); + remoteBucket = + storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); + assertEquals( + BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), + remoteBucket.getIamConfiguration().getPublicAccessPrevention().getValue()); + assertFalse(remoteBucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + + } finally { + RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); + } + } + @Test public void testUploadUsingSignedURL() throws Exception { String blobName = "test-signed-url-upload"; From 6292407af9094f8d889eece41ca37de815d7f732 Mon Sep 17 00:00:00 2001 From: athakor Date: Thu, 3 Dec 2020 12:52:43 +0530 Subject: [PATCH 03/18] fix: build --- .../src/main/java/com/google/cloud/storage/BucketInfo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index c5f4362711..45760dd8e9 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -220,7 +220,7 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { .setPublicAccessPrevention( publicAccessPrevention == null ? null - : PublicAccessPrevention.valueOf(publicAccessPrevention)) + : PublicAccessPrevention.valueOf(publicAccessPrevention.toUpperCase())) .build(); } From 6c066593050d96bf8df06255f8f41b8d20b27712 Mon Sep 17 00:00:00 2001 From: athakor Date: Thu, 17 Dec 2020 12:54:50 +0530 Subject: [PATCH 04/18] feat: address review changes --- .../com/google/cloud/storage/BucketInfo.java | 7 +-- .../google/cloud/storage/BucketInfoTest.java | 1 - .../cloud/storage/it/ITStorageTest.java | 46 +++++++++++++++---- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 45760dd8e9..4871dc6c1a 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -271,16 +271,13 @@ Builder setUniformBucketLevelAccessLockedTime(Long uniformBucketLevelAccessLocke /** * Sets the bucket's Public Access Prevention configuration. Currently supported options are - * 'unspecified' (default) or 'enforced'. + * 'unspecified' or 'enforced'. * * @see public-access-prevention */ public Builder setPublicAccessPrevention(PublicAccessPrevention publicAccessPrevention) { - this.publicAccessPrevention = - publicAccessPrevention == null - ? publicAccessPrevention.UNSPECIFIED - : publicAccessPrevention; + this.publicAccessPrevention = publicAccessPrevention; return this; } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 115f101b07..90b35b03d6 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -72,7 +72,6 @@ public class BucketInfoTest { LifecycleAction.newDeleteAction(), LifecycleCondition.newBuilder().setAge(5).build())); private static final String INDEX_PAGE = "index.html"; - private static final String PUBLIC_ACCESS_PREVENTION_ENFORCED = "enforced"; private static final BucketInfo.IamConfiguration IAM_CONFIGURATION = BucketInfo.IamConfiguration.newBuilder() .setIsUniformBucketLevelAccessEnabled(true) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 336c46739f..44e30ba4a2 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3246,11 +3246,32 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex Bucket bucket = storage.create( Bucket.newBuilder(papBucket).setIamConfiguration(iamConfiguration).build()); + assertNull(bucket.getIamConfiguration().getPublicAccessPrevention()); + + // Set unspecified public access prevention on bucket + bucket + .toBuilder() + .setIamConfiguration( + iamConfiguration + .toBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) + .build()) + .build() + .update(); + Bucket unspecifiedPublicAccessPreventionBucket = + storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( BucketInfo.PublicAccessPrevention.UNSPECIFIED.getValue(), - bucket.getIamConfiguration().getPublicAccessPrevention().getValue()); - assertTrue(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + unspecifiedPublicAccessPreventionBucket + .getIamConfiguration() + .getPublicAccessPrevention() + .getValue()); + assertTrue( + unspecifiedPublicAccessPreventionBucket + .getIamConfiguration() + .isUniformBucketLevelAccessEnabled()); + // Set enforced public access prevention on bucket bucket .toBuilder() .setIamConfiguration( @@ -3260,24 +3281,33 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .build()) .build() .update(); - Bucket remoteBucket = + Bucket enforcedPublicAccessPreventionBucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), - remoteBucket.getIamConfiguration().getPublicAccessPrevention().getValue()); + enforcedPublicAccessPreventionBucket + .getIamConfiguration() + .getPublicAccessPrevention() + .getValue()); - remoteBucket + enforcedPublicAccessPreventionBucket .toBuilder() .setIamConfiguration( iamConfiguration.toBuilder().setIsUniformBucketLevelAccessEnabled(false).build()) .build() .update(); - remoteBucket = + enforcedPublicAccessPreventionBucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), - remoteBucket.getIamConfiguration().getPublicAccessPrevention().getValue()); - assertFalse(remoteBucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + enforcedPublicAccessPreventionBucket + .getIamConfiguration() + .getPublicAccessPrevention() + .getValue()); + assertFalse( + enforcedPublicAccessPreventionBucket + .getIamConfiguration() + .isUniformBucketLevelAccessEnabled()); } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); From 0a434467cfec481c9e099f3a82a694e60efc3112 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Fri, 12 Feb 2021 09:33:20 -0800 Subject: [PATCH 05/18] refactor pap tests --- .../cloud/storage/it/ITStorageTest.java | 77 +++++-------------- 1 file changed, 19 insertions(+), 58 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 44e30ba4a2..d0baaddeba 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3239,76 +3239,37 @@ public void testEnableAndDisableUniformBucketLevelAccessOnExistingBucket() throw public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { + Bucket bucket = storage.create(Bucket.newBuilder(papBucket).build()); + + // Set unspecified public access prevention on bucket BucketInfo.IamConfiguration iamConfiguration = BucketInfo.IamConfiguration.newBuilder() - .setIsUniformBucketLevelAccessEnabled(true) + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) .build(); - Bucket bucket = - storage.create( - Bucket.newBuilder(papBucket).setIamConfiguration(iamConfiguration).build()); - assertNull(bucket.getIamConfiguration().getPublicAccessPrevention()); - - // Set unspecified public access prevention on bucket - bucket - .toBuilder() - .setIamConfiguration( - iamConfiguration - .toBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) - .build()) - .build() - .update(); Bucket unspecifiedPublicAccessPreventionBucket = - storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); + bucket.toBuilder().setIamConfiguration(iamConfiguration).build().update(); + assertEquals( - BucketInfo.PublicAccessPrevention.UNSPECIFIED.getValue(), + BucketInfo.PublicAccessPrevention.UNSPECIFIED, unspecifiedPublicAccessPreventionBucket .getIamConfiguration() - .getPublicAccessPrevention() - .getValue()); - assertTrue( - unspecifiedPublicAccessPreventionBucket - .getIamConfiguration() - .isUniformBucketLevelAccessEnabled()); + .getPublicAccessPrevention()); // Set enforced public access prevention on bucket - bucket - .toBuilder() - .setIamConfiguration( - iamConfiguration - .toBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) - .build()) - .build() - .update(); Bucket enforcedPublicAccessPreventionBucket = - storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); - assertEquals( - BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), - enforcedPublicAccessPreventionBucket - .getIamConfiguration() - .getPublicAccessPrevention() - .getValue()); + bucket + .toBuilder() + .setIamConfiguration( + iamConfiguration + .toBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) + .build()) + .build() + .update(); - enforcedPublicAccessPreventionBucket - .toBuilder() - .setIamConfiguration( - iamConfiguration.toBuilder().setIsUniformBucketLevelAccessEnabled(false).build()) - .build() - .update(); - enforcedPublicAccessPreventionBucket = - storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( - BucketInfo.PublicAccessPrevention.ENFORCED.getValue(), - enforcedPublicAccessPreventionBucket - .getIamConfiguration() - .getPublicAccessPrevention() - .getValue()); - assertFalse( - enforcedPublicAccessPreventionBucket - .getIamConfiguration() - .isUniformBucketLevelAccessEnabled()); - + BucketInfo.PublicAccessPrevention.ENFORCED, + enforcedPublicAccessPreventionBucket.getIamConfiguration().getPublicAccessPrevention()); } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); } From 7e8a6bdfa68509916a9fe207922506f7d468dfc9 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Fri, 12 Feb 2021 10:50:02 -0800 Subject: [PATCH 06/18] address feedback --- .../cloud/storage/it/ITStorageTest.java | 105 +++++++++++++----- 1 file changed, 78 insertions(+), 27 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index d0baaddeba..25f88b1c86 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3239,37 +3239,88 @@ public void testEnableAndDisableUniformBucketLevelAccessOnExistingBucket() throw public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { - Bucket bucket = storage.create(Bucket.newBuilder(papBucket).build()); + Bucket bucket = + storage.create( + Bucket.newBuilder(papBucket) + .setIamConfiguration( + BucketInfo.IamConfiguration.newBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) + .build()) + .build()); + // Making bucket public should fail. + try { + storage.setIamPolicy( + papBucket, + Policy.newBuilder() + .setVersion(3) + .setBindings( + ImmutableList.of( + com.google.cloud.Binding.newBuilder() + .setRole("roles/storage.objectViewer") + .addMembers("allUsers") + .build())) + .build()); + fail("pap: expected adding AllUsers policy to bucket should fail"); + } catch (StorageException storageException) { + // Expected storage exception. + } - // Set unspecified public access prevention on bucket - BucketInfo.IamConfiguration iamConfiguration = - BucketInfo.IamConfiguration.newBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) - .build(); - Bucket unspecifiedPublicAccessPreventionBucket = - bucket.toBuilder().setIamConfiguration(iamConfiguration).build().update(); + // Making object public via ACL should fail. + try { + // Create a public object + bucket.create( + "pap-test-object", + "".getBytes(), + Bucket.BlobTargetOption.predefinedAcl(Storage.PredefinedAcl.PUBLIC_READ)); + fail("pap: expected adding AllUsers ACL to object should fail"); + } catch (StorageException storageException) { + // Expected storage exception. + } + // Update PAP setting to unspecified should work and not affect UBLA setting. + bucket + .toBuilder() + .setIamConfiguration( + bucket + .getIamConfiguration() + .toBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) + .build()) + .build() + .update(); + bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.values())); assertEquals( - BucketInfo.PublicAccessPrevention.UNSPECIFIED, - unspecifiedPublicAccessPreventionBucket - .getIamConfiguration() - .getPublicAccessPrevention()); - - // Set enforced public access prevention on bucket - Bucket enforcedPublicAccessPreventionBucket = - bucket - .toBuilder() - .setIamConfiguration( - iamConfiguration - .toBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) - .build()) - .build() - .update(); + bucket.getIamConfiguration().getPublicAccessPrevention(), + BucketInfo.PublicAccessPrevention.UNSPECIFIED); + assertFalse(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + assertFalse(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); - assertEquals( - BucketInfo.PublicAccessPrevention.ENFORCED, - enforcedPublicAccessPreventionBucket.getIamConfiguration().getPublicAccessPrevention()); + // Now, making object public or making bucket public should succeed. + try { + // Create a public object + bucket.create( + "pap-test-object", + "".getBytes(), + Bucket.BlobTargetOption.predefinedAcl(Storage.PredefinedAcl.PUBLIC_READ)); + } catch (StorageException storageException) { + fail("pap: expected adding AllUsers ACL to object to succeed"); + } + + try { + storage.setIamPolicy( + papBucket, + Policy.newBuilder() + .setVersion(3) + .setBindings( + ImmutableList.of( + com.google.cloud.Binding.newBuilder() + .setRole("roles/storage.objectViewer") + .addMembers("allUsers") + .build())) + .build()); + } catch (StorageException storageException) { + fail("pap: expected adding AllUsers policy to bucket to succeed"); + } } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); } From 19c21dfc0d0e7683b0570ece9656973979e93a91 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Fri, 12 Feb 2021 11:05:11 -0800 Subject: [PATCH 07/18] address feedback --- .../google/cloud/storage/it/ITStorageTest.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 25f88b1c86..626ff908bb 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3288,7 +3288,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .build()) .build() .update(); - bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.values())); + bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( bucket.getIamConfiguration().getPublicAccessPrevention(), BucketInfo.PublicAccessPrevention.UNSPECIFIED); @@ -3306,6 +3306,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex fail("pap: expected adding AllUsers ACL to object to succeed"); } + // Now, making bucket public should succeed. try { storage.setIamPolicy( papBucket, @@ -3321,6 +3322,21 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex } catch (StorageException storageException) { fail("pap: expected adding AllUsers policy to bucket to succeed"); } + + // Updating UBLA should not affect PAP setting. + bucket = + bucket + .toBuilder() + .setIamConfiguration( + bucket + .getIamConfiguration() + .toBuilder() + .setIsUniformBucketLevelAccessEnabled(true) + .build()) + .build() + .update(); + assertTrue(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + assertTrue(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); } From 8a5f76c7127bc97f3a6012b9803aecdff1ac34db Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Fri, 12 Feb 2021 11:06:29 -0800 Subject: [PATCH 08/18] address feedback --- .../test/java/com/google/cloud/storage/it/ITStorageTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 626ff908bb..a016a60598 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3337,6 +3337,9 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .update(); assertTrue(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); assertTrue(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); + assertEquals( + bucket.getIamConfiguration().getPublicAccessPrevention(), + BucketInfo.PublicAccessPrevention.UNSPECIFIED); } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); } From 2a550135aa50ec4ed85ea2e5162eaa2b9930f722 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 2 Mar 2021 12:58:21 -0800 Subject: [PATCH 09/18] s/AllUsers/allUsers --- .../java/com/google/cloud/storage/it/ITStorageTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index a016a60598..a67f35992c 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3260,7 +3260,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .addMembers("allUsers") .build())) .build()); - fail("pap: expected adding AllUsers policy to bucket should fail"); + fail("pap: expected adding allUsers policy to bucket should fail"); } catch (StorageException storageException) { // Expected storage exception. } @@ -3272,7 +3272,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex "pap-test-object", "".getBytes(), Bucket.BlobTargetOption.predefinedAcl(Storage.PredefinedAcl.PUBLIC_READ)); - fail("pap: expected adding AllUsers ACL to object should fail"); + fail("pap: expected adding allUsers ACL to object should fail"); } catch (StorageException storageException) { // Expected storage exception. } @@ -3303,7 +3303,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex "".getBytes(), Bucket.BlobTargetOption.predefinedAcl(Storage.PredefinedAcl.PUBLIC_READ)); } catch (StorageException storageException) { - fail("pap: expected adding AllUsers ACL to object to succeed"); + fail("pap: expected adding allUsers ACL to object to succeed"); } // Now, making bucket public should succeed. @@ -3320,7 +3320,7 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .build())) .build()); } catch (StorageException storageException) { - fail("pap: expected adding AllUsers policy to bucket to succeed"); + fail("pap: expected adding allUsers policy to bucket to succeed"); } // Updating UBLA should not affect PAP setting. From ae1724a84d5598ca46750837e59037518da921b9 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 2 Mar 2021 14:52:22 -0800 Subject: [PATCH 10/18] address feedback --- .../main/java/com/google/cloud/storage/BucketInfo.java | 7 ++++--- .../java/com/google/cloud/storage/it/ITStorageTest.java | 8 ++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 4871dc6c1a..86e000e7ca 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -103,6 +103,7 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) public enum PublicAccessPrevention { ENFORCED("enforced"), + // Default value for Public Access Prevention UNSPECIFIED("unspecified"); private final String value; @@ -125,9 +126,9 @@ public String getValue() { public static class IamConfiguration implements Serializable { private static final long serialVersionUID = -8671736104909424616L; - private Boolean isUniformBucketLevelAccessEnabled; - private Long uniformBucketLevelAccessLockedTime; - private PublicAccessPrevention publicAccessPrevention; + private final Boolean isUniformBucketLevelAccessEnabled; + private final Long uniformBucketLevelAccessLockedTime; + private final PublicAccessPrevention publicAccessPrevention; @Override public boolean equals(Object o) { diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index a67f35992c..1a5552a6cf 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3262,7 +3262,9 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex .build()); fail("pap: expected adding allUsers policy to bucket should fail"); } catch (StorageException storageException) { - // Expected storage exception. + // Creating a bucket with roles/storage.objectViewer is not + // allowed when publicAccessPrevention is enabled. + assertEquals(storageException.getCode(), 412); } // Making object public via ACL should fail. @@ -3274,7 +3276,9 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex Bucket.BlobTargetOption.predefinedAcl(Storage.PredefinedAcl.PUBLIC_READ)); fail("pap: expected adding allUsers ACL to object should fail"); } catch (StorageException storageException) { - // Expected storage exception. + // Creating an object with allUsers roles/storage.viewer permission + // is not allowed. When Public Access Prevention is enabled. + assertEquals(storageException.getCode(), 412); } // Update PAP setting to unspecified should work and not affect UBLA setting. From 09992ba8b7294a5a9c1435bad6846fb4ef38d78a Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 9 Mar 2021 14:42:07 -0800 Subject: [PATCH 11/18] address documentation feedback --- .../main/java/com/google/cloud/storage/BucketInfo.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 86e000e7ca..93944387a6 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -101,6 +101,12 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) private final String locationType; private final Logging logging; + /** + * Public Access Prevention enum with expected values. + * + * @see public-access-prevention + */ public enum PublicAccessPrevention { ENFORCED("enforced"), // Default value for Public Access Prevention @@ -122,6 +128,8 @@ public String getValue() { * * @see uniform * bucket-level access + * @see public-access-prevention */ public static class IamConfiguration implements Serializable { private static final long serialVersionUID = -8671736104909424616L; From 740061167d24206bed00801421413f160dd3f777 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 9 Mar 2021 14:48:58 -0800 Subject: [PATCH 12/18] split up integration tests --- .../cloud/storage/it/ITStorageTest.java | 75 ++++++++++++------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index 1a5552a6cf..db1e0a4cb9 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3235,18 +3235,21 @@ public void testEnableAndDisableUniformBucketLevelAccessOnExistingBucket() throw } } + private Bucket generatePublicAccessPreventionBucket(String bucketName) { + return storage.create( + Bucket.newBuilder(bucketName) + .setIamConfiguration( + BucketInfo.IamConfiguration.newBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) + .build()) + .build()); + } + @Test - public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Exception { + public void testEnforcedPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { - Bucket bucket = - storage.create( - Bucket.newBuilder(papBucket) - .setIamConfiguration( - BucketInfo.IamConfiguration.newBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) - .build()) - .build()); + Bucket bucket = generatePublicAccessPreventionBucket(papBucket); // Making bucket public should fail. try { storage.setIamPolicy( @@ -3280,24 +3283,16 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex // is not allowed. When Public Access Prevention is enabled. assertEquals(storageException.getCode(), 412); } + } finally { + RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); + } + } - // Update PAP setting to unspecified should work and not affect UBLA setting. - bucket - .toBuilder() - .setIamConfiguration( - bucket - .getIamConfiguration() - .toBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) - .build()) - .build() - .update(); - bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); - assertEquals( - bucket.getIamConfiguration().getPublicAccessPrevention(), - BucketInfo.PublicAccessPrevention.UNSPECIFIED); - assertFalse(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); - assertFalse(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); + @Test + public void testUnspecifiedPublicAccessPreventionOnBucket() throws Exception { + String papBucket = RemoteStorageHelper.generateBucketName(); + try { + Bucket bucket = generatePublicAccessPreventionBucket(papBucket); // Now, making object public or making bucket public should succeed. try { @@ -3326,6 +3321,34 @@ public void testUnspecifiedAndEnforcedPublicAccessPreventionOnBucket() throws Ex } catch (StorageException storageException) { fail("pap: expected adding allUsers policy to bucket to succeed"); } + } finally { + RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); + } + } + + @Test + public void testUblaWithPublicAccessPreventionOnBucket() throws Exception { + String papBucket = RemoteStorageHelper.generateBucketName(); + try { + Bucket bucket = generatePublicAccessPreventionBucket(papBucket); + + // Update PAP setting to unspecified should work and not affect UBLA setting. + bucket + .toBuilder() + .setIamConfiguration( + bucket + .getIamConfiguration() + .toBuilder() + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) + .build()) + .build() + .update(); + bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); + assertEquals( + bucket.getIamConfiguration().getPublicAccessPrevention(), + BucketInfo.PublicAccessPrevention.UNSPECIFIED); + assertFalse(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + assertFalse(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); // Updating UBLA should not affect PAP setting. bucket = From c8df1386b468db18739054e0738c55ba3a723aff Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Tue, 9 Mar 2021 16:46:52 -0800 Subject: [PATCH 13/18] address issue in test set up --- .../cloud/storage/it/ITStorageTest.java | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java index db1e0a4cb9..a4c87c71fe 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java @@ -3235,12 +3235,15 @@ public void testEnableAndDisableUniformBucketLevelAccessOnExistingBucket() throw } } - private Bucket generatePublicAccessPreventionBucket(String bucketName) { + private Bucket generatePublicAccessPreventionBucket(String bucketName, boolean enforced) { return storage.create( Bucket.newBuilder(bucketName) .setIamConfiguration( BucketInfo.IamConfiguration.newBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) + .setPublicAccessPrevention( + enforced + ? BucketInfo.PublicAccessPrevention.ENFORCED + : BucketInfo.PublicAccessPrevention.UNSPECIFIED) .build()) .build()); } @@ -3249,7 +3252,7 @@ private Bucket generatePublicAccessPreventionBucket(String bucketName) { public void testEnforcedPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { - Bucket bucket = generatePublicAccessPreventionBucket(papBucket); + Bucket bucket = generatePublicAccessPreventionBucket(papBucket, true); // Making bucket public should fail. try { storage.setIamPolicy( @@ -3292,7 +3295,7 @@ public void testEnforcedPublicAccessPreventionOnBucket() throws Exception { public void testUnspecifiedPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { - Bucket bucket = generatePublicAccessPreventionBucket(papBucket); + Bucket bucket = generatePublicAccessPreventionBucket(papBucket, false); // Now, making object public or making bucket public should succeed. try { @@ -3327,26 +3330,31 @@ public void testUnspecifiedPublicAccessPreventionOnBucket() throws Exception { } @Test - public void testUblaWithPublicAccessPreventionOnBucket() throws Exception { + public void testUBLAWithPublicAccessPreventionOnBucket() throws Exception { String papBucket = RemoteStorageHelper.generateBucketName(); try { - Bucket bucket = generatePublicAccessPreventionBucket(papBucket); + Bucket bucket = generatePublicAccessPreventionBucket(papBucket, false); + assertEquals( + bucket.getIamConfiguration().getPublicAccessPrevention(), + BucketInfo.PublicAccessPrevention.UNSPECIFIED); + assertFalse(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); + assertFalse(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); - // Update PAP setting to unspecified should work and not affect UBLA setting. + // Update PAP setting to ENFORCED and should not affect UBLA setting. bucket .toBuilder() .setIamConfiguration( bucket .getIamConfiguration() .toBuilder() - .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.UNSPECIFIED) + .setPublicAccessPrevention(BucketInfo.PublicAccessPrevention.ENFORCED) .build()) .build() .update(); bucket = storage.get(papBucket, Storage.BucketGetOption.fields(BucketField.IAMCONFIGURATION)); assertEquals( bucket.getIamConfiguration().getPublicAccessPrevention(), - BucketInfo.PublicAccessPrevention.UNSPECIFIED); + BucketInfo.PublicAccessPrevention.ENFORCED); assertFalse(bucket.getIamConfiguration().isUniformBucketLevelAccessEnabled()); assertFalse(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); @@ -3366,7 +3374,7 @@ public void testUblaWithPublicAccessPreventionOnBucket() throws Exception { assertTrue(bucket.getIamConfiguration().isBucketPolicyOnlyEnabled()); assertEquals( bucket.getIamConfiguration().getPublicAccessPrevention(), - BucketInfo.PublicAccessPrevention.UNSPECIFIED); + BucketInfo.PublicAccessPrevention.ENFORCED); } finally { RemoteStorageHelper.forceDelete(storage, papBucket, 1, TimeUnit.MINUTES); } From 125da9f42a512b56c50dd4a8c81bcffb68d2623d Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 11 Mar 2021 22:07:00 -0800 Subject: [PATCH 14/18] Apply suggestions from code review Co-authored-by: BenWhitehead --- .../src/main/java/com/google/cloud/storage/BucketInfo.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 93944387a6..43e71b4a30 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -109,7 +109,9 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) */ public enum PublicAccessPrevention { ENFORCED("enforced"), - // Default value for Public Access Prevention + /** + * Default value for Public Access Prevention + */ UNSPECIFIED("unspecified"); private final String value; @@ -280,7 +282,7 @@ Builder setUniformBucketLevelAccessLockedTime(Long uniformBucketLevelAccessLocke /** * Sets the bucket's Public Access Prevention configuration. Currently supported options are - * 'unspecified' or 'enforced'. + * {@link PublicAccessPrevention#UNSPECIFIED} or {@link PublicAccessPrevention#ENFORCED} * * @see public-access-prevention From 2da9fa4b251c7f8157f82b96ac0fb38934a3ea06 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Mon, 22 Mar 2021 09:40:55 -0700 Subject: [PATCH 15/18] fail valueOf gracefully --- .../com/google/cloud/storage/BucketInfo.java | 21 ++++++++++++------- .../google/cloud/storage/BucketInfoTest.java | 17 +++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 43e71b4a30..cd0d1f6828 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -109,9 +109,7 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) */ public enum PublicAccessPrevention { ENFORCED("enforced"), - /** - * Default value for Public Access Prevention - */ + /** Default value for Public Access Prevention */ UNSPECIFIED("unspecified"); private final String value; @@ -225,13 +223,22 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { DateTime lockedTime = uniformBucketLevelAccess.getLockedTime(); String publicAccessPrevention = iamConfiguration.getPublicAccessPrevention(); + PublicAccessPrevention publicAccessPreventionValue = null; + + try { + if (publicAccessPrevention != null) { + publicAccessPreventionValue = + PublicAccessPrevention.valueOf(publicAccessPrevention.toUpperCase()); + } + } catch (IllegalArgumentException ex) { + throw new IllegalArgumentException( + "IamConfiguration: Received an unexpected value of " + publicAccessPrevention); + } + return newBuilder() .setIsUniformBucketLevelAccessEnabled(uniformBucketLevelAccess.getEnabled()) .setUniformBucketLevelAccessLockedTime(lockedTime == null ? null : lockedTime.getValue()) - .setPublicAccessPrevention( - publicAccessPrevention == null - ? null - : PublicAccessPrevention.valueOf(publicAccessPrevention.toUpperCase())) + .setPublicAccessPrevention(publicAccessPreventionValue) .build(); } diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 90b35b03d6..ec4e7a5357 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import com.google.api.client.util.DateTime; import com.google.api.services.storage.model.Bucket; @@ -370,6 +371,22 @@ public void testIamConfiguration() { iamConfiguration.getPublicAccessPrevention()); } + @Test + public void testPapValueOfIamConfiguration() { + try { + Bucket.IamConfiguration iamConfiguration = new Bucket.IamConfiguration(); + Bucket.IamConfiguration.UniformBucketLevelAccess uniformBucketLevelAccess = + new Bucket.IamConfiguration.UniformBucketLevelAccess(); + iamConfiguration.setUniformBucketLevelAccess(uniformBucketLevelAccess); + iamConfiguration.setPublicAccessPrevention("random-string"); + BucketInfo.IamConfiguration.fromPb(iamConfiguration); + fail("Expected an IllegalArgumentException when passing in a bad"); + } catch (IllegalArgumentException ex) { + // expected IllegalArgumentException because random-string does not map to an enum value + assertTrue(ex.getMessage().contains("random-string")); + } + } + @Test public void testLogging() { Bucket.Logging logging = From a042d43bf2b4e8125a6daf0893d1d755db3402a1 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Thu, 6 May 2021 18:45:03 -0400 Subject: [PATCH 16/18] fix: add UNKNOWN enum value to PublicAccessPrevention --- .../com/google/cloud/storage/BucketInfo.java | 23 +++++++++++-------- .../google/cloud/storage/BucketInfoTest.java | 23 ++++++++----------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index cd0d1f6828..2fbba55777 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -110,7 +110,8 @@ public com.google.api.services.storage.model.Bucket apply(BucketInfo bucketInfo) public enum PublicAccessPrevention { ENFORCED("enforced"), /** Default value for Public Access Prevention */ - UNSPECIFIED("unspecified"); + UNSPECIFIED("unspecified"), + UNKNOWN(null); private final String value; @@ -121,6 +122,15 @@ public enum PublicAccessPrevention { public String getValue() { return value; } + + public static PublicAccessPrevention parse(String value) { + String upper = value.toUpperCase(); + try { + return valueOf(upper); + } catch (IllegalArgumentException ignore) { + return UNKNOWN; + } + } } /** @@ -224,15 +234,8 @@ static IamConfiguration fromPb(Bucket.IamConfiguration iamConfiguration) { String publicAccessPrevention = iamConfiguration.getPublicAccessPrevention(); PublicAccessPrevention publicAccessPreventionValue = null; - - try { - if (publicAccessPrevention != null) { - publicAccessPreventionValue = - PublicAccessPrevention.valueOf(publicAccessPrevention.toUpperCase()); - } - } catch (IllegalArgumentException ex) { - throw new IllegalArgumentException( - "IamConfiguration: Received an unexpected value of " + publicAccessPrevention); + if (publicAccessPrevention != null) { + publicAccessPreventionValue = PublicAccessPrevention.parse(publicAccessPrevention); } return newBuilder() diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index ec4e7a5357..537b14a850 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -20,7 +20,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import com.google.api.client.util.DateTime; import com.google.api.services.storage.model.Bucket; @@ -32,11 +31,13 @@ import com.google.cloud.storage.BucketInfo.CreatedBeforeDeleteRule; import com.google.cloud.storage.BucketInfo.DeleteRule; import com.google.cloud.storage.BucketInfo.DeleteRule.Type; +import com.google.cloud.storage.BucketInfo.IamConfiguration; import com.google.cloud.storage.BucketInfo.IsLiveDeleteRule; import com.google.cloud.storage.BucketInfo.LifecycleRule; import com.google.cloud.storage.BucketInfo.LifecycleRule.LifecycleAction; import com.google.cloud.storage.BucketInfo.LifecycleRule.LifecycleCondition; import com.google.cloud.storage.BucketInfo.NumNewerVersionsDeleteRule; +import com.google.cloud.storage.BucketInfo.PublicAccessPrevention; import com.google.cloud.storage.BucketInfo.RawDeleteRule; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -373,18 +374,14 @@ public void testIamConfiguration() { @Test public void testPapValueOfIamConfiguration() { - try { - Bucket.IamConfiguration iamConfiguration = new Bucket.IamConfiguration(); - Bucket.IamConfiguration.UniformBucketLevelAccess uniformBucketLevelAccess = - new Bucket.IamConfiguration.UniformBucketLevelAccess(); - iamConfiguration.setUniformBucketLevelAccess(uniformBucketLevelAccess); - iamConfiguration.setPublicAccessPrevention("random-string"); - BucketInfo.IamConfiguration.fromPb(iamConfiguration); - fail("Expected an IllegalArgumentException when passing in a bad"); - } catch (IllegalArgumentException ex) { - // expected IllegalArgumentException because random-string does not map to an enum value - assertTrue(ex.getMessage().contains("random-string")); - } + Bucket.IamConfiguration iamConfiguration = new Bucket.IamConfiguration(); + Bucket.IamConfiguration.UniformBucketLevelAccess uniformBucketLevelAccess = + new Bucket.IamConfiguration.UniformBucketLevelAccess(); + iamConfiguration.setUniformBucketLevelAccess(uniformBucketLevelAccess); + iamConfiguration.setPublicAccessPrevention("random-string"); + IamConfiguration fromPb = IamConfiguration.fromPb(iamConfiguration); + + assertEquals(PublicAccessPrevention.UNKNOWN, fromPb.getPublicAccessPrevention()); } @Test From 21b7659a79e517928987c5eea75b467ad4e85ac2 Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 May 2021 12:15:59 -0400 Subject: [PATCH 17/18] fix: add comment to unknown --- .../src/main/java/com/google/cloud/storage/BucketInfo.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java index 2fbba55777..75c4ff25ec 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/BucketInfo.java @@ -43,6 +43,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; +import java.util.EnumSet; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -111,6 +112,10 @@ public enum PublicAccessPrevention { ENFORCED("enforced"), /** Default value for Public Access Prevention */ UNSPECIFIED("unspecified"), + /** + * If the api returns a value that isn't defined in {@link PublicAccessPrevention} this value + * will be returned. + */ UNKNOWN(null); private final String value; From 37e19e36a2a8832099344c933621145e5981ad4a Mon Sep 17 00:00:00 2001 From: BenWhitehead Date: Fri, 7 May 2021 15:20:01 -0400 Subject: [PATCH 18/18] fix: add test to ensure publicAccessPrevention is absent from serialized value when set to UNKNOWN --- .../google/cloud/storage/BucketInfoTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java index 537b14a850..f8ed5cd0ec 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/BucketInfoTest.java @@ -18,9 +18,12 @@ import static com.google.cloud.storage.Acl.Project.ProjectRole.VIEWERS; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import com.google.api.client.json.JsonGenerator; +import com.google.api.client.json.jackson2.JacksonFactory; import com.google.api.client.util.DateTime; import com.google.api.services.storage.model.Bucket; import com.google.api.services.storage.model.Bucket.Lifecycle.Rule; @@ -41,6 +44,8 @@ import com.google.cloud.storage.BucketInfo.RawDeleteRule; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import java.io.IOException; +import java.io.StringWriter; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -372,6 +377,24 @@ public void testIamConfiguration() { iamConfiguration.getPublicAccessPrevention()); } + @Test + public void testPublicAccessPrevention_ensureAbsentWhenUnknown() throws IOException { + StringWriter stringWriter = new StringWriter(); + JsonGenerator jsonGenerator = JacksonFactory.getDefaultInstance() + .createJsonGenerator(stringWriter); + + jsonGenerator.serialize(BucketInfo.IamConfiguration.newBuilder() + .setIsUniformBucketLevelAccessEnabled(true) + .setUniformBucketLevelAccessLockedTime(System.currentTimeMillis()) + .setPublicAccessPrevention(PublicAccessPrevention.UNKNOWN) + .build() + .toPb() + ); + jsonGenerator.flush(); + + assertFalse(stringWriter.getBuffer().toString().contains("publicAccessPrevention")); + } + @Test public void testPapValueOfIamConfiguration() { Bucket.IamConfiguration iamConfiguration = new Bucket.IamConfiguration();