From 501876a87a17b7156f4a3737d58c26f4228d0d0d Mon Sep 17 00:00:00 2001
From: bajajnehaa <bajajnehaa@google.com>
Date: Thu, 9 May 2024 15:08:35 +0000
Subject: [PATCH] address review comments and one more test for custom universe
 domain

---
 google/cloud/storage/client.cc               |  6 ++---
 google/cloud/storage/client.h                |  2 +-
 google/cloud/storage/client_sign_url_test.cc | 25 ++++++++++++++++----
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc
index d4a21e04c64f7..884dfb578e047 100644
--- a/google/cloud/storage/client.cc
+++ b/google/cloud/storage/client.cc
@@ -389,7 +389,7 @@ StatusOr<std::string> Client::SignUrlV2(
   std::string signature = curl.MakeEscapedString(encoded).get();
 
   std::ostringstream os;
-  os << ExternalUrl() << request.bucket_name();
+  os << ExternalUrl() << '/' << request.bucket_name();
   if (!request.object_name().empty()) {
     os << '/' << curl.MakeEscapedString(request.object_name()).get();
   }
@@ -481,8 +481,8 @@ std::string CreateRandomPrefixName(std::string const& prefix) {
                                                   "abcdefghijklmnopqrstuvwxyz");
 }
 
-std::string Client::ExternalUrl() {
-  return connection_->client_options().endpoint() + "/";
+std::string Client::ExternalUrl() const{
+  return connection_->options().get<RestEndpointOption>();
 }
 
 namespace internal {
diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h
index 91d2f2335f923..831968e83ebd9 100644
--- a/google/cloud/storage/client.h
+++ b/google/cloud/storage/client.h
@@ -3492,7 +3492,7 @@ class Client {
   StatusOr<PolicyDocumentV4Result> SignPolicyDocumentV4(
       internal::PolicyDocumentV4Request request);
 
-  std::string ExternalUrl();
+  std::string ExternalUrl() const;
 
   std::shared_ptr<internal::StorageConnection> connection_;
 };
diff --git a/google/cloud/storage/client_sign_url_test.cc b/google/cloud/storage/client_sign_url_test.cc
index d620cf07f5457..bb9649f0e5ae2 100644
--- a/google/cloud/storage/client_sign_url_test.cc
+++ b/google/cloud/storage/client_sign_url_test.cc
@@ -18,6 +18,7 @@
 #include "google/cloud/storage/testing/canonical_errors.h"
 #include "google/cloud/storage/testing/client_unit_test.h"
 #include "google/cloud/testing_util/status_matchers.h"
+#include "google/cloud/universe_domain_options.h"
 #include <gmock/gmock.h>
 
 namespace google {
@@ -28,9 +29,11 @@ namespace {
 
 using ::google::cloud::internal::CurrentOptions;
 using ::google::cloud::storage::testing::canonical_errors::TransientError;
+using ::google::cloud::testing_util::IsOkAndHolds;
 using ::google::cloud::testing_util::StatusIs;
 using ::testing::HasSubstr;
 using ::testing::Return;
+using ::testing::StartsWith;
 
 constexpr char kJsonKeyfileContents[] = R"""({
       "type": "service_account",
@@ -109,13 +112,27 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) {
 
 /// @test Verify that CreateV2SignedUrl() respects the custom endpoint.
 TEST_F(CreateSignedUrlTest, V2SignCustomEndpoint) {
-  std::string custom_endpoint = "https://storage.mydomain.com";
+  auto const custom_endpoint = std::string{"https://storage.mydomain.com"};
+  
+  Options options = Options{}.set<UnifiedCredentialsOption>(MakeServiceAccountCredentials(kJsonKeyfileContents))
+                             .set<RestEndpointOption>(custom_endpoint);
+  Client client(options);
+  StatusOr<std::string> actual =
+      client.CreateV2SignedUrl("GET", "test-bucket", "test-object");
+  EXPECT_THAT(actual, IsOkAndHolds(StartsWith(custom_endpoint)));
+}
+
+/// @test Verify that CreateV2SignedUrl() respects the custom universe domain.
+TEST_F(CreateSignedUrlTest, V2SignCustomUniverseDomain) {
+  auto const custom_ud = std::string{"mydomain.com"};
+
+  Options options = Options{}.set<UnifiedCredentialsOption>(MakeServiceAccountCredentials(kJsonKeyfileContents))
+                             .set<google::cloud::internal::UniverseDomainOption>(custom_ud);
+  Client client(options);
 
-  Client client(Options{}.set<RestEndpointOption>(custom_endpoint));
   StatusOr<std::string> actual =
       client.CreateV2SignedUrl("GET", "test-bucket", "test-object");
-  ASSERT_STATUS_OK(actual);
-  EXPECT_THAT(*actual, HasSubstr(custom_endpoint));
+  EXPECT_THAT(actual, IsOkAndHolds(HasSubstr(custom_ud)));
 }
 
 // This is a placeholder service account JSON file that is inactive. It's fine