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

feat: implement GrpcStorageImpl#createDefaultAcl & GrpcStorageImpl#updateDefaultAcl #1806

Merged
merged 2 commits into from
Dec 14, 2022
Merged

feat: implement GrpcStorageImpl#createDefaultAcl & GrpcStorageImpl#updateDefaultAcl #1806

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb0f538
feat: implement GrpcStorageImpl#createDefaultAcl & GrpcStorageImpl#up…
BenWhitehead Dec 8, 2022
22818a5
chore: update to return status code 0 when the updated acl is not fou…
BenWhitehead Dec 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/Bucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1285,7 +1285,7 @@ public boolean deleteDefaultAcl(Entity entity) {
*
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
public Acl createDefaultAcl(Acl acl) {
return storage.createDefaultAcl(getName(), acl);
}
Expand All @@ -1304,7 +1304,7 @@ public Acl createDefaultAcl(Acl acl) {
*
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
public Acl updateDefaultAcl(Acl acl) {
return storage.updateDefaultAcl(getName(), acl);
}
Expand Down
52 changes: 50 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.common.io.BaseEncoding;
import com.google.common.io.ByteStreams;
import com.google.iam.v1.GetIamPolicyRequest;
Expand Down Expand Up @@ -936,12 +937,59 @@ public boolean deleteDefaultAcl(String bucket, Entity entity) {

@Override
public Acl createDefaultAcl(String bucket, Acl acl) {
return throwNotYetImplemented(fmtMethodName("createDefaultAcl", String.class, Acl.class));
return updateDefaultAcl(bucket, acl);
sydney-munro marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public Acl updateDefaultAcl(String bucket, Acl acl) {
return throwNotYetImplemented(fmtMethodName("updateDefaultAcl", String.class, Acl.class));
try {
com.google.storage.v2.Bucket resp = getBucketDefaultAcls(bucket);
ObjectAccessControl encode = codecs.objectAcl().encode(acl);
String entity = encode.getEntity();

Predicate<ObjectAccessControl> entityPredicate = objectAclEntityOrAltEq(entity);

ImmutableList<ObjectAccessControl> collect =
Streams.concat(
resp.getDefaultObjectAclList().stream().filter(entityPredicate.negate()),
Stream.of(encode))
.collect(ImmutableList.toImmutableList());

com.google.storage.v2.Bucket update =
com.google.storage.v2.Bucket.newBuilder()
.setName(bucketNameCodec.encode(bucket))
.addAllDefaultObjectAcl(collect)
.build();
Opts<BucketTargetOpt> opts =
Opts.from(
UnifiedOpts.fields(ImmutableSet.of(BucketField.DEFAULT_OBJECT_ACL)),
UnifiedOpts.metagenerationMatch(resp.getMetageneration()));
UpdateBucketRequest req =
opts.updateBucketsRequest()
.apply(UpdateBucketRequest.newBuilder())
.setBucket(update)
.build();

GrpcCallContext grpcCallContext = GrpcCallContext.createDefault();
com.google.storage.v2.Bucket updateResult =
Retrying.run(
getOptions(),
retryAlgorithmManager.getFor(req),
() -> storageClient.updateBucketCallable().call(req, grpcCallContext),
Decoder.identity());

//noinspection DataFlowIssue
Optional<Acl> first =
updateResult.getDefaultObjectAclList().stream()
.filter(entityPredicate)
.findFirst()
.map(codecs.objectAcl()::decode);

return first.orElseThrow(
() -> new StorageException(404, "Acl update call success, but not in response"));
Copy link
Member

Choose a reason for hiding this comment

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

When would this occur?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully never, but since we have to search for the updated acl in the response, if the response somehow doesn't included the updated acl we need to signal that somehow. 404 here doesn't feel totally correct, but I'm not sure 500 is better. What do you think?

Copy link
Member

@frankyn frankyn Dec 13, 2022 •
edited

Choose a reason for hiding this comment

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

Not sure it's necessary tbh, you can only have up to 100 acls associated to a bucket or object;
image

https://cloud.google.com/storage/docs/access-control/lists#:~:text=use%20IAM%20exclusively.-,What%20is%20an%20access%20control%20list%3F,of%20one%20or%20more%20entries.

Thinking we just rely on the service response code in this case unless I'm misunderstanding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I spoke with @frankyn offline, we came to the consensus that instead of returning an HTTP status code here, insead we will return undefined 0 to signal that it's purely a client side issue, not any system issue.

} catch (NotFoundException e) {
throw StorageException.coalesce(e);
}
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3530,7 +3530,7 @@ PostPolicyV4 generateSignedPostPolicyV4(
*
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
Acl createDefaultAcl(String bucket, Acl acl);

/**
Expand All @@ -3549,7 +3549,7 @@ PostPolicyV4 generateSignedPostPolicyV4(
*
* @throws StorageException upon failure
*/
@TransportCompatibility({Transport.HTTP})
@TransportCompatibility({Transport.HTTP, Transport.GRPC})
Acl updateDefaultAcl(String bucket, Acl acl);

/**
Expand Down
7 changes: 6 additions & 1 deletion google-cloud-storage/src/main/java/com/google/cloud/storage/UnifiedOpts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ public Mapper<ListObjectsRequest.Builder> listObjects() {
}

static final class Fields extends RpcOptVal<ImmutableSet<NamedField>>
implements ObjectSourceOpt, ObjectListOpt, BucketSourceOpt, BucketListOpt {
implements ObjectSourceOpt, ObjectListOpt, BucketSourceOpt, BucketTargetOpt, BucketListOpt {

/**
* Apiary and gRPC have differing handling of where the field selector is evaluated relative to
Expand Down Expand Up @@ -752,6 +752,11 @@ public Mapper<ListBucketsRequest.Builder> listBuckets() {
return b -> b.setReadMask(FieldMask.newBuilder().addAllPaths(getPaths()).build());
}

@Override
public Mapper<UpdateBucketRequest.Builder> updateBucket() {
return b -> b.setUpdateMask(FieldMask.newBuilder().addAllPaths(getPaths()).build());
}

@Override
public Mapper<GetObjectRequest.Builder> getObject() {
return b -> b.setReadMask(FieldMask.newBuilder().addAllPaths(getPaths()).build());
Expand Down
121 changes: 120 additions & 1 deletion google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITAccessTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import com.google.cloud.RetryHelper.RetryHelperException;
import com.google.cloud.http.BaseHttpServiceException;
import com.google.cloud.storage.Acl;
import com.google.cloud.storage.Acl.Entity;
import com.google.cloud.storage.Acl.Project.ProjectRole;
import com.google.cloud.storage.Acl.Role;
import com.google.cloud.storage.Acl.User;
import com.google.cloud.storage.Blob;
Expand Down Expand Up @@ -70,6 +72,7 @@
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.function.Predicate;
import java.util.stream.Collector;
import java.util.stream.Collectors;
import org.junit.Ignore;
Expand Down Expand Up @@ -185,12 +188,108 @@ public void bucket_defaultAcl_list_bucket404() {
assertThat(storageException.getCode()).isEqualTo(404);
}

@Test
public void bucket_defaultAcl_create() throws Exception {
BucketInfo bucketInfo = BucketInfo.newBuilder(generator.randomBucketName()).build();
try (TemporaryBucket tempB =
TemporaryBucket.newBuilder().setBucketInfo(bucketInfo).setStorage(storage).build()) {
BucketInfo bucket = tempB.getBucket();

Acl readAll = Acl.of(User.ofAllAuthenticatedUsers(), Role.READER);
Acl actual = retry429s(() -> storage.createDefaultAcl(bucket.getName(), readAll), storage);

assertThat(actual.getEntity()).isEqualTo(readAll.getEntity());
assertThat(actual.getRole()).isEqualTo(readAll.getRole());
assertThat(actual.getEtag()).isNotEmpty();

Bucket bucketUpdated =
storage.get(bucket.getName(), BucketGetOption.fields(BucketField.values()));
assertThat(bucketUpdated.getMetageneration()).isNotEqualTo(bucket.getMetageneration());

// etags change when updates happen, drop before our comparison
List<Acl> expectedAcls = dropEtags(bucket.getDefaultAcl());
List<Acl> actualAcls = dropEtags(bucketUpdated.getDefaultAcl());
assertThat(actualAcls).containsAtLeastElementsIn(expectedAcls);
assertThat(actualAcls).contains(readAll);
}
}

@Test
public void bucket_defaultAcl_create_bucket404() {
Acl readAll = Acl.of(User.ofAllAuthenticatedUsers(), Role.READER);
StorageException storageException =
assertThrows(
StorageException.class,
() ->
retry429s(
() -> storage.createDefaultAcl(bucket.getName() + "x", readAll), storage));

assertThat(storageException.getCode()).isEqualTo(404);
}

@Test
public void bucket_defaultAcl_update() throws Exception {
BucketInfo bucketInfo = BucketInfo.newBuilder(generator.randomBucketName()).build();
try (TemporaryBucket tempB =
TemporaryBucket.newBuilder().setBucketInfo(bucketInfo).setStorage(storage).build()) {
BucketInfo bucket = tempB.getBucket();

List<Acl> defaultAcls = bucket.getDefaultAcl();
assertThat(defaultAcls).isNotEmpty();

Predicate<Acl> isProjectEditor = hasProjectRole(ProjectRole.EDITORS);

//noinspection OptionalGetWithoutIsPresent
Acl projectEditorAsOwner =
defaultAcls.stream().filter(hasRole(Role.OWNER).and(isProjectEditor)).findFirst().get();

// lower the privileges of project editors to writer from owner
Entity entity = projectEditorAsOwner.getEntity();
Acl projectEditorAsReader = Acl.of(entity, Role.READER);

Acl actual =
retry429s(
() -> storage.updateDefaultAcl(bucket.getName(), projectEditorAsReader), storage);

assertThat(actual.getEntity()).isEqualTo(projectEditorAsReader.getEntity());
assertThat(actual.getRole()).isEqualTo(projectEditorAsReader.getRole());
assertThat(actual.getEtag()).isNotEmpty();

Bucket bucketUpdated =
storage.get(bucket.getName(), BucketGetOption.fields(BucketField.values()));
assertThat(bucketUpdated.getMetageneration()).isNotEqualTo(bucket.getMetageneration());

// etags change when updates happen, drop before our comparison
List<Acl> expectedAcls =
dropEtags(
bucket.getDefaultAcl().stream()
.filter(isProjectEditor.negate())
.collect(Collectors.toList()));
List<Acl> actualAcls = dropEtags(bucketUpdated.getDefaultAcl());
assertThat(actualAcls).containsAtLeastElementsIn(expectedAcls);
assertThat(actualAcls).doesNotContain(projectEditorAsOwner);
assertThat(actualAcls).contains(projectEditorAsReader);
}
}

@Test
public void bucket_defaultAcl_update_bucket404() {
Acl readAll = Acl.of(User.ofAllAuthenticatedUsers(), Role.READER);
StorageException storageException =
assertThrows(
StorageException.class,
() ->
retry429s(
() -> storage.updateDefaultAcl(bucket.getName() + "x", readAll), storage));

assertThat(storageException.getCode()).isEqualTo(404);
}

@Test
@CrossRun.Ignore(transports = Transport.GRPC)
public void testBucketDefaultAcl() {
// TODO: break this test up into each of the respective scenarios
// 2. Delete a default ACL for a specific entity
// 3. Create a default ACL for specific entity
// 4. Update default ACL to change role of a specific entity
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved

// according to https://cloud.google.com/storage/docs/access-control/lists#default
Expand Down Expand Up @@ -1026,4 +1125,24 @@ public boolean shouldRetry(Throwable previousThrowable, Object previousResponse)
}
}
}

private static ImmutableList<Acl> dropEtags(List<Acl> defaultAcls) {
return defaultAcls.stream()
.map(acl -> Acl.of(acl.getEntity(), acl.getRole()))
.collect(ImmutableList.toImmutableList());
}

private static Predicate<Acl> hasRole(Acl.Role expected) {
return acl -> acl.getRole().equals(expected);
}

private static Predicate<Acl> hasProjectRole(Acl.Project.ProjectRole expected) {
return acl -> {
Entity entity = acl.getEntity();
if (entity.getType().equals(Entity.Type.PROJECT)) {
return ((Acl.Project) entity).getProjectRole().equals(expected);
}
return false;
};
}
}