Repository: googleapis/java-storage
Pull request #114 (2020-02-04 00:29:06)

feat: update V4 signature to pass conformance tests
This updates the conformance tests and adds the consistency changes necessary to make them pass, as per https://github.com/googleapis/java-storage/issues/84 and https://github.com/googleapis/java-storage/issues/85

codecov[bot]: # [Codecov](https://codecov.io/gh/googleapis/java-storage/pull/114?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@a76845a`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/googleapis/java-storage/pull/114/graphs/tree.svg?width=650&token=ssm139YOdp&height=150&src=pr)](https://codecov.io/gh/googleapis/java-storage/pull/114?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##             master    #114   +/-   ##
========================================
  Coverage          ?   63.6%           
  Complexity        ?     538           
========================================
  Files             ?      30           
  Lines             ?    4726           
  Branches          ?     425           
========================================
  Hits              ?    3006           
  Misses            ?    1562           
  Partials          ?     158
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/googleapis/java-storage/pull/114?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/googleapis/java-storage/pull/114?src=pr&el=footer). Last update [a76845a...25ce31d](https://codecov.io/gh/googleapis/java-storage/pull/114?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/google-cloud-storage/pom.xml b/google-cloud-storage/pom.xml
index 20c60263..c2b734ab 100644
--- a/google-cloud-storage/pom.xml
+++ b/google-cloud-storage/pom.xml
@@ -34,7 +34,7 @@
     <dependency>
       <groupId>com.google.api-client</groupId>
       <artifactId>google-api-client</artifactId>
-      <version>1.30.7</version>
+      <version>1.30.8</version>
     </dependency>
     <dependency>
       <groupId>com.google.apis</groupId>
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/SignatureInfo.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/SignatureInfo.java
index f26157d9..03630c61 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/SignatureInfo.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/SignatureInfo.java
@@ -169,7 +169,9 @@ public class SignatureInfo {
     canonicalRequest
         .append(serializer.serializeHeaderNames(canonicalizedExtensionHeaders))
         .append(COMPONENT_SEPARATOR);
-    canonicalRequest.append("UNSIGNED-PAYLOAD");
+
+    String userProvidedHash = canonicalizedExtensionHeaders.get("X-Goog-Content-SHA256");
+    canonicalRequest.append(userProvidedHash == null ? "UNSIGNED-PAYLOAD" : userProvidedHash);
 
     return Hashing.sha256()
         .hashString(canonicalRequest.toString(), StandardCharsets.UTF_8)
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
index 77c88e97..98f304dd 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/Storage.java
@@ -161,6 +161,21 @@ public interface Storage extends Service<StorageOptions> {
     }
   }
 
+  enum UriScheme {
+    HTTP("http"),
+    HTTPS("https");
+
+    private final String scheme;
+
+    UriScheme(String scheme) {
+      this.scheme = scheme;
+    }
+
+    public String getScheme() {
+      return scheme;
+    }
+  }
+
   /** Class for specifying bucket target options. */
   class BucketTargetOption extends Option {
 
@@ -1038,6 +1053,7 @@ public interface Storage extends Service<StorageOptions> {
       HOST_NAME,
       PATH_STYLE,
       VIRTUAL_HOSTED_STYLE,
+      BUCKET_BOUND_HOST_NAME,
       QUERY_PARAMS
     }
 
@@ -1160,6 +1176,44 @@ public interface Storage extends Service<StorageOptions> {
       return new SignUrlOption(Option.PATH_STYLE, "");
     }
 
+    /**
+     * 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
+     * cannot be used alongside {@code withVirtualHostedStyle()} or {@code withPathStyle()}. This
+     * method signature uses HTTP for the URI scheme, and is equivalent to calling {@code
+     * withBucketBoundHostname("...", UriScheme.HTTP).}
+     *
+     * @see <a href="https://cloud.google.com/storage/docs/request-endpoints#cname">CNAME
+     *     Redirects</a>
+     * @see <a
+     *     href="https://cloud.google.com/load-balancing/docs/https/adding-backend-buckets-to-load-balancers">
+     *     GCLB Redirects</a>
+     */
+    public static SignUrlOption withBucketBoundHostname(String bucketBoundHostname) {
+      return withBucketBoundHostname(bucketBoundHostname, UriScheme.HTTP);
+    }
+
+    /**
+     * 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
+     * cannot be used alongside {@code withVirtualHostedStyle()} or {@code withPathStyle()}. The
+     * bucket name itself should not include the URI scheme (http or https), so it is specified via
+     * a local enum.
+     *
+     * @see <a href="https://cloud.google.com/storage/docs/request-endpoints#cname">CNAME
+     *     Redirects</a>
+     * @see <a
+     *     href="https://cloud.google.com/load-balancing/docs/https/adding-backend-buckets-to-load-balancers">
+     *     GCLB Redirects</a>
+     */
+    public static SignUrlOption withBucketBoundHostname(
+        String bucketBoundHostname, UriScheme uriScheme) {
+      return new SignUrlOption(
+          Option.BUCKET_BOUND_HOST_NAME, uriScheme.getScheme() + "://" + bucketBoundHostname);
+    }
+
     /**
      * Use if the URL should contain additional query parameters.
      *
diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java
index 63c19855..d76aa0d2 100644
--- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java
+++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java
@@ -660,8 +660,10 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
 
     checkArgument(
         !(optionMap.containsKey(SignUrlOption.Option.VIRTUAL_HOSTED_STYLE)
-            && optionMap.containsKey(SignUrlOption.Option.PATH_STYLE)),
-        "Cannot specify both the VIRTUAL_HOSTED_STYLE and PATH_STYLE SignUrlOptions together.");
+            && optionMap.containsKey(SignUrlOption.Option.PATH_STYLE)
+            && optionMap.containsKey(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME)),
+        "Only one of VIRTUAL_HOSTED_STYLE, PATH_STYLE, or BUCKET_BOUND_HOST_NAME SignUrlOptions can be"
+            + " specified.");
 
     String bucketName = slashlessBucketNameFromBlobInfo(blobInfo);
     String escapedBlobName = "";
@@ -676,6 +678,10 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
             ? STORAGE_XML_URI_SCHEME + "://" + getBaseStorageHostName(optionMap)
             : STORAGE_XML_URI_SCHEME + "://" + bucketName + "." + getBaseStorageHostName(optionMap);
 
+    if (optionMap.containsKey(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME)) {
+      storageXmlHostName = (String) optionMap.get(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME);
+    }
+
     String stPath =
         usePathStyle
             ? constructResourceUriPath(bucketName, escapedBlobName, optionMap)
@@ -753,9 +759,7 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
       }
       return pathBuilder.toString();
     }
-    if (!escapedBlobName.startsWith(PATH_DELIMITER)) {
-      pathBuilder.append(PATH_DELIMITER);
-    }
+    pathBuilder.append(PATH_DELIMITER);
     pathBuilder.append(escapedBlobName);
     return pathBuilder.toString();
   }
@@ -776,7 +780,8 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
   private boolean shouldUsePathStyleForSignedUrl(EnumMap<SignUrlOption.Option, Object> optionMap) {
     // TODO(#6362): If we decide to change the default style used to generate URLs, switch this
     // logic to return false unless PATH_STYLE was explicitly specified.
-    if (optionMap.containsKey(SignUrlOption.Option.VIRTUAL_HOSTED_STYLE)) {
+    if (optionMap.containsKey(SignUrlOption.Option.VIRTUAL_HOSTED_STYLE)
+        || optionMap.containsKey(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME)) {
       return false;
     }
     return true;
@@ -836,7 +841,8 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
         extHeadersBuilder.put(
             "host",
             slashlessBucketNameFromBlobInfo(blobInfo) + "." + getBaseStorageHostName(optionMap));
-      } else if (optionMap.containsKey(SignUrlOption.Option.HOST_NAME)) {
+      } else if (optionMap.containsKey(SignUrlOption.Option.HOST_NAME)
+          || optionMap.containsKey(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME)) {
         extHeadersBuilder.put("host", getBaseStorageHostName(optionMap));
       }
     }
@@ -868,9 +874,14 @@ final class StorageImpl extends BaseService<StorageOptions> implements Storage {
   /** Returns the hostname used to send requests to Cloud Storage, e.g. "storage.googleapis.com". */
   private String getBaseStorageHostName(Map<SignUrlOption.Option, Object> optionMap) {
     String specifiedBaseHostName = (String) optionMap.get(SignUrlOption.Option.HOST_NAME);
+    String bucketBoundHostName =
+        (String) optionMap.get(SignUrlOption.Option.BUCKET_BOUND_HOST_NAME);
     if (!Strings.isNullOrEmpty(specifiedBaseHostName)) {
       return specifiedBaseHostName.replaceFirst("http(s)?://", "");
     }
+    if (!Strings.isNullOrEmpty(bucketBoundHostName)) {
+      return bucketBoundHostName.replaceFirst("http(s)?://", "");
+    }
     return STORAGE_XML_URI_HOST_NAME;
   }
 
diff --git a/pom.xml b/pom.xml
index 725ce940..de0421be 100644
--- a/pom.xml
+++ b/pom.xml
@@ -212,7 +212,7 @@
       <dependency>
         <groupId>com.google.cloud</groupId>
         <artifactId>google-cloud-conformance-tests</artifactId>
-        <version>0.0.4</version>
+        <version>0.0.5</version>
         <scope>test</scope>
       </dependency>
     </dependencies>
@@ -225,7 +225,10 @@
           <groupId>org.apache.maven.plugins</groupId>
           <artifactId>maven-dependency-plugin</artifactId>
           <configuration>
-            <ignoredUnusedDeclaredDependencies>org.objenesis:objenesis</ignoredUnusedDeclaredDependencies>
+            <ignoredUnusedDeclaredDependencies>
+              <ignoredUnusedDeclaredDependency>org.hamcrest:hamcrest</ignoredUnusedDeclaredDependency>
+              <ignoredUnusedDeclaredDependency>org.objenesis:objenesis</ignoredUnusedDeclaredDependency>
+            </ignoredUnusedDeclaredDependencies>
           </configuration>
         </plugin>
       </plugins>


Test patch:
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
index 34f2478d..59e1f8d2 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java
@@ -1808,6 +1808,7 @@ public class StorageImplTest {
     String expectedUrl =
         new StringBuilder("https://storage.googleapis.com/")
             .append(BUCKET_NAME1)
+            .append("/")
             .append(expectedResourcePath)
             .append("?GoogleAccessId=")
             .append(ACCOUNT)
@@ -1825,6 +1826,7 @@ public class StorageImplTest {
         .append(42L + 1209600)
         .append("\n/")
         .append(BUCKET_NAME1)
+        .append("/")
         .append(expectedResourcePath);
 
     Signature signer = Signature.getInstance("SHA256withRSA");
@@ -1857,6 +1859,7 @@ public class StorageImplTest {
     String expectedUrl =
         new StringBuilder("https://example.com/")
             .append(BUCKET_NAME1)
+            .append("/")
             .append(escapedBlobName)
             .append("?GoogleAccessId=")
             .append(ACCOUNT)
@@ -1874,6 +1877,7 @@ public class StorageImplTest {
         .append(42L + 1209600)
         .append("\n/")
         .append(BUCKET_NAME1)
+        .append("/")
         .append(escapedBlobName);
 
     Signature signer = Signature.getInstance("SHA256withRSA");
@@ -2019,6 +2023,7 @@ public class StorageImplTest {
       String expectedUrl =
           new StringBuilder("https://storage.googleapis.com/")
               .append(BUCKET_NAME1)
+              .append("/")
               .append(expectedBlobName)
               .append("?GoogleAccessId=")
               .append(ACCOUNT)
@@ -2036,6 +2041,7 @@ public class StorageImplTest {
           .append(42L + 1209600)
           .append("\n/")
           .append(BUCKET_NAME1)
+          .append("/")
           .append(expectedBlobName);
 
       Signature signer = Signature.getInstance("SHA256withRSA");
@@ -2075,6 +2081,7 @@ public class StorageImplTest {
       String expectedUrl =
           new StringBuilder("https://example.com/")
               .append(BUCKET_NAME1)
+              .append("/")
               .append(expectedBlobName)
               .append("?GoogleAccessId=")
               .append(ACCOUNT)
@@ -2092,6 +2099,7 @@ public class StorageImplTest {
           .append(42L + 1209600)
           .append("\n/")
           .append(BUCKET_NAME1)
+          .append("/")
           .append(expectedBlobName);
 
       Signature signer = Signature.getInstance("SHA256withRSA");
@@ -2243,6 +2251,7 @@ public class StorageImplTest {
     String expectedUrl =
         new StringBuilder("https://storage.googleapis.com/")
             .append(BUCKET_NAME1)
+            .append("/")
             .append(escapedBlobName)
             .append("?GoogleAccessId=")
             .append(ACCOUNT)
@@ -2260,6 +2269,7 @@ public class StorageImplTest {
         .append(42L + 1209600)
         .append("\n/")
         .append(BUCKET_NAME1)
+        .append("/")
         .append(escapedBlobName);
 
     Signature signer = Signature.getInstance("SHA256withRSA");
@@ -2293,6 +2303,7 @@ public class StorageImplTest {
     String expectedUrl =
         new StringBuilder("https://example.com/")
             .append(BUCKET_NAME1)
+            .append("/")
             .append(escapedBlobName)
             .append("?GoogleAccessId=")
             .append(ACCOUNT)
@@ -2310,6 +2321,7 @@ public class StorageImplTest {
         .append(42L + 1209600)
         .append("\n/")
         .append(BUCKET_NAME1)
+        .append("/")
         .append(escapedBlobName);
 
     Signature signer = Signature.getInstance("SHA256withRSA");
diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java
index 8e1ad310..d45cb6dc 100644
--- a/google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java
+++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/V4SigningTest.java
@@ -16,11 +16,8 @@
 
 package com.google.cloud.storage;
 
-import static org.hamcrest.core.Is.is;
-import static org.hamcrest.core.IsNot.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assume.assumeThat;
 
 import com.google.api.core.ApiClock;
 import com.google.auth.oauth2.ServiceAccountCredentials;
@@ -94,11 +91,6 @@ public class V4SigningTest {
 
   @Test
   public void test() {
-    assumeThat(
-        "Test skipped until b/136171758 is resolved",
-        testName.getMethodName(),
-        is(not("test[Headers should be trimmed]")));
-
     Storage storage =
         RemoteStorageHelper.create()
             .getOptions()
@@ -110,6 +102,19 @@ public class V4SigningTest {
 
     BlobInfo blob = BlobInfo.newBuilder(testData.getBucket(), testData.getObject()).build();
 
+    SignUrlOption style = SignUrlOption.withPathStyle();
+
+    if (testData.getUrlStyle().equals(SigningV4Test.UrlStyle.VIRTUAL_HOSTED_STYLE)) {
+      style = SignUrlOption.withVirtualHostedStyle();
+    } else if (testData.getUrlStyle().equals(SigningV4Test.UrlStyle.PATH_STYLE)) {
+      style = SignUrlOption.withPathStyle();
+    } else if (testData.getUrlStyle().equals(SigningV4Test.UrlStyle.BUCKET_BOUND_DOMAIN)) {
+      style =
+          SignUrlOption.withBucketBoundHostname(
+              testData.getBucketBoundDomain(),
+              Storage.UriScheme.valueOf(testData.getScheme().toUpperCase()));
+    }
+
     final String signedUrl =
         storage
             .signUrl(
@@ -118,7 +123,9 @@ public class V4SigningTest {
                 TimeUnit.SECONDS,
                 SignUrlOption.httpMethod(HttpMethod.valueOf(testData.getMethod())),
                 SignUrlOption.withExtHeaders(testData.getHeadersMap()),
-                SignUrlOption.withV4Signature())
+                SignUrlOption.withV4Signature(),
+                SignUrlOption.withQueryParams(testData.getQueryParametersMap()),
+                style)
             .toString();
     assertEquals(testData.getExpectedUrl(), signedUrl);
   }