feat(storage): better error messages in signed URLs (#11741)

The error messages for signed URL functions (`CreateV2SignedUrl()`,
`CreateV4SignedUrl()` and `CreatedSignedPolicyDocument()`) were not
helpful when the signing account is missing.

google/cloud/storage/client.cc

@@ -22,6 +22,7 @@
 #include "google/cloud/internal/algorithm.h"
 #include "google/cloud/internal/curl_options.h"
 #include "google/cloud/internal/filesystem.h"
+#include "google/cloud/internal/make_status.h"
 #include "google/cloud/internal/opentelemetry.h"
 #include "google/cloud/log.h"
 #include <fstream>
@@ -357,7 +358,6 @@ StatusOr<Client::SignBlobResponseRaw> Client::SignBlobImpl(
     SigningAccount const& signing_account, std::string const& string_to_sign) {
   auto credentials = raw_client_->client_options().credentials();
 
-  std::string signing_account_email = SigningEmail(signing_account);
   // First try to sign locally.
   auto signed_blob = credentials->SignBlob(signing_account, string_to_sign);
   if (signed_blob) {
@@ -367,8 +367,19 @@ StatusOr<Client::SignBlobResponseRaw> Client::SignBlobImpl(
   // If signing locally fails that may be because the credentials do not
   // support signing, or because the signing account is different than the
   // credentials account. In either case, try to sign using the API.
+  // In this case, however, we want to validate the signing account, because
+  // otherwise the errors are almost impossible to troubleshoot.
+  auto signing_email = SigningEmail(signing_account);
+  if (signing_email.empty()) {
+    return google::cloud::internal::InvalidArgumentError(
+        "signing account cannot be empty."
+        " The client library was unable to fetch a valid signing email from"
+        " the configured credentials, and the application did not provide"
+        " a value in the `google::cloud::storage::SigningAccount` option.",
+        GCP_ERROR_INFO());
+  }
   internal::SignBlobRequest sign_request(
-      signing_account_email, internal::Base64Encode(string_to_sign), {});
+      std::move(signing_email), internal::Base64Encode(string_to_sign), {});
   auto response = raw_client_->SignBlob(sign_request);
   if (!response) return response.status();
   auto decoded = internal::Base64Decode(response->signed_blob);

google/cloud/storage/client_sign_policy_document_test.cc

@@ -137,6 +137,7 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignRemote) {
   auto client = ClientForMock();
   auto actual = client.CreateSignedPolicyDocument(
       CreatePolicyDocumentForTest(),
+      SigningAccount("test-only-invalid@example.com"),
       Options{}.set<UserProjectOption>("u-p-test"));
   ASSERT_STATUS_OK(actual);
   EXPECT_THAT(actual->signature, expected_signed_blob);
@@ -148,7 +149,10 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignPolicyTooManyFailures) {
   testing::TooManyFailuresStatusTest<internal::SignBlobResponse>(
       mock_, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateSignedPolicyDocument(CreatePolicyDocumentForTest())
+        return client
+            .CreateSignedPolicyDocument(
+                CreatePolicyDocumentForTest(),
+                SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");
@@ -161,7 +165,10 @@ TEST_F(CreateSignedPolicyDocRPCTest, SignPolicyPermanentFailure) {
   testing::PermanentFailureStatusTest<internal::SignBlobResponse>(
       client, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateSignedPolicyDocument(CreatePolicyDocumentForTest())
+        return client
+            .CreateSignedPolicyDocument(
+                CreatePolicyDocumentForTest(),
+                SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");

google/cloud/storage/client_sign_url_test.cc

@@ -29,6 +29,7 @@ namespace {
 
 using ::google::cloud::internal::CurrentOptions;
 using ::google::cloud::storage::testing::canonical_errors::TransientError;
+using ::google::cloud::testing_util::StatusIs;
 using ::testing::HasSubstr;
 using ::testing::Return;
 
@@ -101,6 +102,7 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) {
   auto client = ClientForMock();
   StatusOr<std::string> actual =
       client.CreateV2SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"),
                                Options{}.set<UserProjectOption>("u-p-test"));
   ASSERT_STATUS_OK(actual);
   EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_safe));
@@ -111,7 +113,9 @@ TEST_F(CreateSignedUrlTest, V2SignTooManyFailures) {
   testing::TooManyFailuresStatusTest<internal::SignBlobResponse>(
       mock_, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateV2SignedUrl("GET", "test-bucket", "test-object")
+        return client
+            .CreateV2SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");
@@ -123,7 +127,9 @@ TEST_F(CreateSignedUrlTest, V2SignPermanentFailure) {
   testing::PermanentFailureStatusTest<internal::SignBlobResponse>(
       client, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateV2SignedUrl("GET", "test-bucket", "test-object")
+        return client
+            .CreateV2SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");
@@ -247,17 +253,29 @@ TEST_F(CreateSignedUrlTest, V4SignRemote) {
   auto client = ClientForMock();
   StatusOr<std::string> actual =
       client.CreateV4SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"),
                                Options{}.set<UserProjectOption>("u-p-test"));
   ASSERT_STATUS_OK(actual);
   EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_hex));
 }
 
+TEST_F(CreateSignedUrlTest, V4SignRemoteNoSigningEmail) {
+  EXPECT_CALL(*mock_, SignBlob).Times(0);
+  auto client = ClientForMock();
+  auto const actual = client.CreateV4SignedUrl(
+      "GET", "test-bucket", "test-object", SigningAccount(""));
+  EXPECT_THAT(actual, StatusIs(StatusCode::kInvalidArgument,
+                               HasSubstr("signing account cannot be empty")));
+}
+
 /// @test Verify that CreateV4SignedUrl() + SignBlob() respects retry policies.
 TEST_F(CreateSignedUrlTest, V4SignTooManyFailures) {
   testing::TooManyFailuresStatusTest<internal::SignBlobResponse>(
       mock_, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateV4SignedUrl("GET", "test-bucket", "test-object")
+        return client
+            .CreateV4SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");
@@ -269,7 +287,9 @@ TEST_F(CreateSignedUrlTest, V4SignPermanentFailure) {
   testing::PermanentFailureStatusTest<internal::SignBlobResponse>(
       client, EXPECT_CALL(*mock_, SignBlob),
       [](Client& client) {
-        return client.CreateV4SignedUrl("GET", "test-bucket", "test-object")
+        return client
+            .CreateV4SignedUrl("GET", "test-bucket", "test-object",
+                               SigningAccount("test-only-invalid@example.com"))
             .status();
       },
       "SignBlob");