feat: V4 POST policy #177

JesseLovelace · 2020-03-12T20:15:58Z

Adds V4 post policy. Will add documentation once implementation is approved

Fixes: #280 #88

…datev4

codecov · 2020-03-12T20:22:12Z

Codecov Report

Merging #177 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #177   +/-   ##
=========================================
  Coverage     62.43%   62.43%           
  Complexity      552      552           
=========================================
  Files            31       31           
  Lines          5007     5007           
  Branches        449      449           
=========================================
  Hits           3126     3126           
  Misses         1712     1712           
  Partials        169      169

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da40d03...da40d03. Read the comment docs.

elharo

Can you add documentation in this PR? That would make the code here easier to understand and review.

elharo · 2020-03-13T12:35:25Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    /**
+     * Use a virtual hosted-style hostname, which adds the bucket into the host portion of the URI
+     * rather than the path, e.g. 'https://mybucket.storage.googleapis.com/...'. The bucket name
+     * will be obtained from the resource passed in.


will be --> is
per Google Tech Writing Guidelines
https://developers.google.com/style/tense

elharo · 2020-03-13T12:35:37Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    }
+
+    /**
+     * Provides a service account signer to sign the policy. If not provided an attempt will be made


will be --> is

elharo · 2020-03-13T12:36:03Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+
+    /**
+     * Generate a path-style URL, which places the bucket name in the path portion of the URL
+     * instead of in the hostname, e.g 'https://storage.googleapis.com/mybucket/...'. Note that this


delete "Note that"
https://developers.google.com/style/tone

elharo · 2020-03-13T12:38:11Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    }
+
+    /**
+     * Generate a path-style URL, which places the bucket name in the path portion of the URL


Generate --> Generates
https://developers.google.com/style/reference-verbs

elharo · 2020-03-13T12:38:44Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    /**
+     * Use a bucket-bound hostname, which replaces the storage.googleapis.com host with the name of
+     * a CNAME bucket, e.g. a bucket named 'gcs-subdomain.my.domain.tld', or a Google Cloud Load
+     * Balancer which routes to a bucket you own, e.g. 'my-load-balancer-domain.tld'. Note that this


delete "Note that"

elharo · 2020-03-13T12:48:55Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

+
+    boolean usePathStyle = shouldUsePathStyleForSignedUrl(optionMap);
+
+    String url =


The ternary operator is hard to read when broken across multiple lines. Consider if else.

Elliotte's comment here makes sense. Probably want to use an if-else in this case.

elharo · 2020-03-13T12:51:31Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java

@@ -0,0 +1,202 @@
+/*
+ * Copyright 2019 Google LLC


Should be 2020.

elharo · 2020-03-13T12:52:05Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java

+
+package com.google.cloud.storage;
+
+import static com.google.cloud.storage.Storage.V4ConditionType.MATCHES;


Use non-static imports here as these are not well-known names.

elharo · 2020-03-13T12:52:36Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java

+  private final ServiceAccountCredentials serviceAccountCredentials;
+
+  /**
+   * @param testData The serialized test data representing the test case.


nit: no cap and no period since it's not a complete sentence

elharo · 2020-03-13T12:53:13Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java

+  }
+
+  /**
+   * Load all of the tests and return a {@code Collection<Object[]>} representing the set of tests.


Load --> Loads

frankyn

I'm still reviewing your PR. One ask is to add examples of how your code is expected to be used into Surface Review Doc

frankyn · 2020-03-13T22:05:42Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+    }
+  }
+
+  class V4PostConditions {


+1 could you split this up into separate files for maintainability as well?

frankyn · 2020-03-13T22:08:00Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java

@@ -104,14 +105,14 @@ public void test() {

    SignUrlOption style = SignUrlOption.withPathStyle();

-    if (testData.getUrlStyle().equals(SigningV4Test.UrlStyle.VIRTUAL_HOSTED_STYLE)) {
+    if (testData.getUrlStyle().equals(UrlStyle.VIRTUAL_HOSTED_STYLE)) {


can you split out this into a separte PR if possible?

For this specifically, I mean to split out conformance test changes into a separate PR.

frankyn

Thanks again for your patience @JesseLovelace.

frankyn · 2020-03-20T03:17:27Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java

@@ -104,14 +105,14 @@ public void test() {

    SignUrlOption style = SignUrlOption.withPathStyle();

-    if (testData.getUrlStyle().equals(SigningV4Test.UrlStyle.VIRTUAL_HOSTED_STYLE)) {
+    if (testData.getUrlStyle().equals(UrlStyle.VIRTUAL_HOSTED_STYLE)) {


For this specifically, I mean to split out conformance test changes into a separate PR.

frankyn · 2020-03-20T03:18:43Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

@@ -1255,6 +1260,447 @@ public static SignUrlOption withQueryParams(Map<String, String> queryParams) {
    }
  }

+  class V4PostPolicyOption implements Serializable {


Can you split all V4PostPolicy helper classes into a separate file, except what's needed for generateV4PresignedPostPolicy ease of use such as V4PostPolicyOption?

frankyn · 2020-03-20T03:24:44Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

+    return generateV4PresignedPostPolicy(
+        blobInfo, V4PostFields.newBuilder().build(), duration, options);
+  }
+


Put V4 at the end of the name please.

Please do this for other helpers as well that use V4 for POST Policy. From meeting.

frankyn

@JesseLovelace adding name change request for top level method name.

frankyn · 2020-03-24T00:45:06Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

@@ -2532,6 +2978,13 @@ Blob create(
   */
  URL signUrl(BlobInfo blobInfo, long duration, TimeUnit unit, SignUrlOption... options);

+  V4PostPolicy generateV4PresignedPostPolicy(


Using consistent naming across languages, please use: generateSignedPostPolicyV4.

…datev4

frankyn

Some surface changes requested. Open to discussing them as well.

frankyn · 2020-04-07T20:13:21Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * @param duration how long until the form expires, in milliseconds
+   * @param options optional post policy options
+   */
+  PostPolicyV4 generatePresignedPostPolicyV4(


use generateSignedPostPolicyV4

frankyn · 2020-04-07T20:15:00Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   */
+  PostPolicyV4 generatePresignedPostPolicyV4(
+      BlobInfo blobInfo,
+      PostFieldsV4 fields,


fields and conditions should be optional. I want to say these should be under options.

frankyn · 2020-04-07T20:17:45Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+            this.value = value;
+        }
+
+        public static ConditionV4 newV4Condition(ConditionV4Type type, String element, String value) {


This doesn't translate correctly for ContentRangeLengths because element and value are start and end respectively.

frankyn · 2020-04-07T20:19:46Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+        }
+
+        public static ConditionV4 newV4Condition(ConditionV4Type type, String element, String value) {
+            return new ConditionV4(type, element, value);


Given PostConditionV4 requires users to use a related method. Should this class have a private static method helper as well? Also not sure how this is different from private ConditionV4()?

…datev4

frankyn

Small nits overall LGTM.

Please also add integration tests.

frankyn · 2020-04-09T21:42:12Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+/**
+ * Presigned V4 post policy.
+ *
+ * @see <a href="https://cloud.google.com/storage/docs/xml-api/post-object" POST Object</a>


Missing >

frankyn · 2020-04-09T21:42:38Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+    /**
+     * Class representing which fields to specify in a V4 POST request.
+     *
+     * @see <a href=https://cloud.google.com/storage/docs/xml-api/post-object#form_fields> POST Object Form fields</a>


No double quote " around the URL.

frankyn · 2020-04-09T21:44:06Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+                return this;
+            }
+
+            public Builder addCustomCondition(ConditionV4Type type, String field, String value) {


Suggest removing this until we get a feature request for this method.

We actually use it in the generate method, but I can change it to package private

package private sgtm, thanks for clarifying!

frankyn · 2020-04-09T21:53:11Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * PostFieldsV4 fields = PostFieldsV4.newBuilder().setAcl("public-read").build();
+   * PostConditionsV4 conditions = PostConditionsV4.newBuilder().addContentTypeCondition(ConditionV4Type.MATCHES, "image/jpeg").build();
+   *
+   * PostPolicyV4 policy = storage.generatePresignedPostPolicyV4(


uses old, instead of generateSignedPostPolicyV4.

frankyn · 2020-04-09T21:53:40Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   *
+   * PostPolicyV4 policy = storage.generatePresignedPostPolicyV4(
+   *     BlobInfo.newBuilder("my-bucket", "my-object").build(),
+   *     fields, conditions, 7, TimeUnit.DAYS);


Could you add an example of how to send a POST request with the policy result?

frankyn · 2020-04-09T21:55:14Z

google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java

+
+    boolean usePathStyle = shouldUsePathStyleForSignedUrl(optionMap);
+
+    String url =


Elliotte's comment here makes sense. Probably want to use an if-else in this case.

frankyn · 2020-04-09T21:55:43Z

google-cloud-storage/src/test/java/com/google/cloud/storage/V4PostPolicyTest.java

@@ -0,0 +1,202 @@
+/*
+ * Copyright 2019 Google LLC


Should be 2020.

frankyn · 2020-04-09T21:57:23Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+  PostPolicyV4 generateSignedPostPolicyV4(
+          BlobInfo blobInfo, long duration, TimeUnit unit, PostConditionsV4 conditions, PostPolicyV4Option... options);
+
+  PostPolicyV4 generateSignedPostPolicyV4(


Add integration tests.

frankyn · 2020-04-09T21:58:35Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+            public Builder setSuccessActionStatus(int successActionStatus) {
+                this.successActionStatus = "" + successActionStatus;
+                return this;
+            }


Custom field setter should be available here.

frankyn · 2020-04-09T21:59:17Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+                return this;
+            }
+
+            public Builder addCustomMetadataCondition(ConditionV4Type type, String field, String value) {


this should be in fields instead.

frankyn

Thanks @JesseLovelace, added a few nit comments. Mainly the integration test should send the request to GCS and make sure it's successful.

frankyn · 2020-04-22T17:03:20Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

+   * for(Map.Entry<String, String> entry : policy.getFields().entrySet()) {
+   *     request.addHeader(entry.getKey(), entry.getValue());
+   * }
+   * client.execute(request);


This example is missing the file to upload*

frankyn · 2020-04-22T17:05:45Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

@@ -67,6 +67,8 @@
 import com.google.cloud.storage.CopyWriter;
 import com.google.cloud.storage.HmacKey;
 import com.google.cloud.storage.HttpMethod;
+import com.google.cloud.storage.PostPolicyV4;
+import com.google.cloud.storage.PostPolicyV4.*;


Explicitly import the needed classes and not use *.

frankyn · 2020-04-22T17:07:21Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

@@ -3208,4 +3210,72 @@ public void testBucketLogging() throws ExecutionException, InterruptedException
      RemoteStorageHelper.forceDelete(storage, loggingBucket, 5, TimeUnit.SECONDS);
    }
  }
+
+  @Test
+  public void testSignedPostPolicyV4() {


This is more of a unit test so I would say it's good to keep. I'd like to see a request similar to what you added into your documentation above:

PostFieldsV4 fields = PostFieldsV4.newBuilder().setAcl("public-read").build(); PostConditionsV4 conditions = PostConditionsV4.newBuilder().addContentTypeCondition(ConditionV4Type.MATCHES, "image/jpeg").build(); PostPolicyV4 policy = storage.generateSignedPostPolicyV4( BlobInfo.newBuilder("my-bucket", "my-object").build(), 7, TimeUnit.DAYS, fields, conditions); HttpClient client = HttpClientBuilder.create().build(); HttpPost request = new HttpPost(policy.getUrl()); for(Map.Entry<String, String> entry : policy.getFields().entrySet()) { request.addHeader(entry.getKey(), entry.getValue()); } // Missing data to upload in request client.execute(request);

frankyn · 2020-04-22T17:08:57Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

@@ -2532,6 +2634,88 @@ Blob create(
   */
  URL signUrl(BlobInfo blobInfo, long duration, TimeUnit unit, SignUrlOption... options);

+  /**
+   * Generates a URL and a map of fields that can be specified in a form to submit a POST request.


in an HTML form
You can link to relevant documentation here: https://cloud.google.com/storage/docs/xml-api/post-object#usage_and_examples

…datev4

frankyn · 2020-04-24T22:11:12Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PostPolicyV4.java

+        return this;
+      }
+
+      public Builder addBucketCondition(ConditionV4Type type, String bucket) {


Please remove this condition.

frankyn · 2020-04-24T22:19:38Z

pom.xml

+      <dependency>
+        <groupId>com.google.code.gson</groupId>
+        <artifactId>gson</artifactId>
+        <version>2.8.6</version>


what's the scope for gson artifact?

…datev4

frankyn

LGTM, let's get it merge after you fix the Binary Compat issue.

dmitry-fa · 2020-04-30T12:58:57Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

@@ -56,7 +57,6 @@
 *
 * @see <a href="https://cloud.google.com/storage/docs">Google Cloud Storage</a>
 */
-@InternalExtensionOnly
 public interface Storage extends Service<StorageOptions> {


@JesseLovelace , @frankyn

InternalExtensionOnly is necessary, otherwise this change will break API.

Definitely missed this during review, thank you @dmitry-fa!

dmitry-fa · 2020-04-30T13:07:42Z

google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java

   * @return the updated blob
   * @throws StorageException upon failure
-   * @see <a
-   *     href="https://cloud.google.com/storage/docs/json_api/v1/objects/update">https://cloud.google.com/storage/docs/json_api/v1/objects/update</a>
   */
  Blob update(BlobInfo blobInfo);


These changes revert fixes made for #252.

cc: @frankyn

Addressed issues. Thanks @dmitry-fa

dmitry-fa

Thanks for addressing my notes!

JesseLovelace added 15 commits February 3, 2020 16:24

Update V4 signature to pass conformance tests

a76845a

Merge branch 'master' of github.com:googleapis/java-storage

9d6261a

Merge branch 'master' of github.com:googleapis/java-storage

73a2da5

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

25ce31d

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

aca896e

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

161fcb5

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

a485ac0

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

9cf145a

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

362ce2c

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

ee7c507

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

140c26c

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

821dddb

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

440a1ca

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

0105b9d

…datev4

Merge remote-tracking branch 'origin' into updatev4

2ed70e6

JesseLovelace requested a review from frankyn March 12, 2020 20:15

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 12, 2020

Merge branch 'master' into updatev4

8db9aa2

elharo reviewed Mar 13, 2020

View reviewed changes

frankyn requested changes Mar 13, 2020

View reviewed changes

frankyn requested changes Mar 20, 2020

View reviewed changes

frankyn requested changes Mar 24, 2020

View reviewed changes

JesseLovelace and others added 2 commits April 7, 2020 11:03

Merge branch 'master' into updatev4

18a3108

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

165811d

…datev4

frankyn requested changes Apr 7, 2020

View reviewed changes

JesseLovelace and others added 3 commits April 9, 2020 14:02

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

391772c

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

fe965fc

…datev4

Merge branch 'master' into updatev4

00c9dc3

frankyn requested changes Apr 9, 2020

View reviewed changes

frankyn requested changes Apr 22, 2020

View reviewed changes

JesseLovelace added 2 commits April 24, 2020 11:12

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

52519a7

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

5b53ebe

…datev4

frankyn requested changes Apr 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into updatev4

15a0f4c

frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 27, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2020

JesseLovelace and others added 7 commits April 27, 2020 15:57

Merge remote-tracking branch 'origin/master' into updatev4

cea97ff

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

721edba

…datev4

Merge branch 'master' into updatev4

bbbee21

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

a5e48d9

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

14c66f3

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

4b49abf

…datev4

Merge branch 'updatev4' of github.com:googleapis/java-storage into up…

7be0222

…datev4

frankyn approved these changes Apr 29, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into updatev4

2274bda

dmitry-fa suggested changes Apr 30, 2020

View reviewed changes

unrevert InternalExtensionOnly

a413ae6

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 30, 2020

frankyn added 3 commits April 30, 2020 08:30

revert accidental deletion of comment updates

c323ac9

revert fix part 2

1d959fb

revert fix part 3

da40d03

frankyn requested a review from dmitry-fa April 30, 2020 15:42

dmitry-fa approved these changes Apr 30, 2020

View reviewed changes

JesseLovelace merged commit 32d8ffa into master Apr 30, 2020

JesseLovelace deleted the updatev4 branch April 30, 2020 18:36


		boolean usePathStyle = shouldUsePathStyleForSignedUrl(optionMap);

		String url =


		package com.google.cloud.storage;

		import static com.google.cloud.storage.Storage.V4ConditionType.MATCHES;

feat: V4 POST policy #177

feat: V4 POST policy #177

Conversation

JesseLovelace commented Mar 12, 2020 • edited by frankyn

codecov bot commented Mar 12, 2020 • edited

Codecov Report

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-fa Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-fa left a comment

Choose a reason for hiding this comment

JesseLovelace commented Mar 12, 2020 •

edited by frankyn

codecov bot commented Mar 12, 2020 •

edited

dmitry-fa Apr 30, 2020 •

edited