From adaa31f0ae471b2d44bb35b532e801acb72f724b Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 9 May 2024 13:33:30 +0000 Subject: [PATCH 01/14] feat(storage): Respect custom endpoint for SignedUrl --- google/cloud/storage/client.cc | 6 +++++- google/cloud/storage/client.h | 2 ++ google/cloud/storage/client_sign_url_test.cc | 11 +++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 8727768182b5..d4a21e04c64f 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -389,7 +389,7 @@ StatusOr Client::SignUrlV2( std::string signature = curl.MakeEscapedString(encoded).get(); std::ostringstream os; - os << "https://storage.googleapis.com/" << request.bucket_name(); + os << ExternalUrl() << request.bucket_name(); if (!request.object_name().empty()) { os << '/' << curl.MakeEscapedString(request.object_name()).get(); } @@ -481,6 +481,10 @@ std::string CreateRandomPrefixName(std::string const& prefix) { "abcdefghijklmnopqrstuvwxyz"); } +std::string Client::ExternalUrl() { + return connection_->client_options().endpoint() + "/"; +} + namespace internal { Client ClientImplDetails::CreateWithDecorations( diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index 01f9cb79a547..91d2f2335f92 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -3492,6 +3492,8 @@ class Client { StatusOr SignPolicyDocumentV4( internal::PolicyDocumentV4Request request); + std::string ExternalUrl(); + std::shared_ptr connection_; }; diff --git a/google/cloud/storage/client_sign_url_test.cc b/google/cloud/storage/client_sign_url_test.cc index 680642ccb73c..d620cf07f545 100644 --- a/google/cloud/storage/client_sign_url_test.cc +++ b/google/cloud/storage/client_sign_url_test.cc @@ -107,6 +107,17 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) { EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_safe)); } +/// @test Verify that CreateV2SignedUrl() respects the custom endpoint. +TEST_F(CreateSignedUrlTest, V2SignCustomEndpoint) { + std::string custom_endpoint = "https://storage.mydomain.com"; + + Client client(Options{}.set(custom_endpoint)); + StatusOr actual = + client.CreateV2SignedUrl("GET", "test-bucket", "test-object"); + ASSERT_STATUS_OK(actual); + EXPECT_THAT(*actual, HasSubstr(custom_endpoint)); +} + // This is a placeholder service account JSON file that is inactive. It's fine // for it to be public. constexpr char kJsonKeyfileContentsForV4[] = R"""({ From 638e026aa3396e423e40a6a5c0bbc0f19702266c Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 9 May 2024 15:08:35 +0000 Subject: [PATCH 02/14] 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 d4a21e04c64f..884dfb578e04 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -389,7 +389,7 @@ StatusOr 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(); } namespace internal { diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index 91d2f2335f92..831968e83ebd 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -3492,7 +3492,7 @@ class Client { StatusOr SignPolicyDocumentV4( internal::PolicyDocumentV4Request request); - std::string ExternalUrl(); + std::string ExternalUrl() const; std::shared_ptr connection_; }; diff --git a/google/cloud/storage/client_sign_url_test.cc b/google/cloud/storage/client_sign_url_test.cc index d620cf07f545..bb9649f0e5ae 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 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(MakeServiceAccountCredentials(kJsonKeyfileContents)) + .set(custom_endpoint); + Client client(options); + StatusOr 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(MakeServiceAccountCredentials(kJsonKeyfileContents)) + .set(custom_ud); + Client client(options); - Client client(Options{}.set(custom_endpoint)); StatusOr 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 From 301ff2e487bff4b29c7cab0effd028baa584390a Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 15 May 2024 11:53:38 +0000 Subject: [PATCH 03/14] custom endpoint for v4 SignedUrl --- google/cloud/storage/client.cc | 4 +- google/cloud/storage/client.h | 5 +- google/cloud/storage/client_sign_url_test.cc | 58 +++++++++++++++++-- .../storage/internal/signed_url_requests.cc | 11 ++-- .../storage/internal/signed_url_requests.h | 21 ++++--- 5 files changed, 78 insertions(+), 21 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 884dfb578e04..aa6a510aa348 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -389,7 +389,7 @@ StatusOr Client::SignUrlV2( std::string signature = curl.MakeEscapedString(encoded).get(); std::ostringstream os; - os << ExternalUrl() << '/' << request.bucket_name(); + os << Endpoint() << '/' << request.bucket_name(); if (!request.object_name().empty()) { os << '/' << curl.MakeEscapedString(request.object_name()).get(); } @@ -481,7 +481,7 @@ std::string CreateRandomPrefixName(std::string const& prefix) { "abcdefghijklmnopqrstuvwxyz"); } -std::string Client::ExternalUrl() const{ +std::string Client::Endpoint() const { return connection_->options().get(); } diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index 831968e83ebd..ea57e7993152 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -2995,8 +2995,9 @@ class Client { Options&&... options) { google::cloud::internal::OptionsSpan const span( SpanOptions(std::forward(options)...)); + auto const host = Endpoint(); internal::V4SignUrlRequest request(std::move(verb), std::move(bucket_name), - std::move(object_name)); + std::move(object_name), std::move(host)); request.set_multiple_options(std::forward(options)...); return SignUrlV4(std::move(request)); } @@ -3492,7 +3493,7 @@ class Client { StatusOr SignPolicyDocumentV4( internal::PolicyDocumentV4Request request); - std::string ExternalUrl() const; + std::string Endpoint() const; std::shared_ptr connection_; }; diff --git a/google/cloud/storage/client_sign_url_test.cc b/google/cloud/storage/client_sign_url_test.cc index bb9649f0e5ae..8e68a9edeedb 100644 --- a/google/cloud/storage/client_sign_url_test.cc +++ b/google/cloud/storage/client_sign_url_test.cc @@ -113,9 +113,11 @@ TEST_F(CreateSignedUrlTest, V2SignRemote) { /// @test Verify that CreateV2SignedUrl() respects the custom endpoint. TEST_F(CreateSignedUrlTest, V2SignCustomEndpoint) { auto const custom_endpoint = std::string{"https://storage.mydomain.com"}; - - Options options = Options{}.set(MakeServiceAccountCredentials(kJsonKeyfileContents)) - .set(custom_endpoint); + + Options options = Options{} + .set( + MakeServiceAccountCredentials(kJsonKeyfileContents)) + .set(custom_endpoint); Client client(options); StatusOr actual = client.CreateV2SignedUrl("GET", "test-bucket", "test-object"); @@ -126,8 +128,11 @@ TEST_F(CreateSignedUrlTest, V2SignCustomEndpoint) { TEST_F(CreateSignedUrlTest, V2SignCustomUniverseDomain) { auto const custom_ud = std::string{"mydomain.com"}; - Options options = Options{}.set(MakeServiceAccountCredentials(kJsonKeyfileContents)) - .set(custom_ud); + Options options = + Options{} + .set( + MakeServiceAccountCredentials(kJsonKeyfileContents)) + .set(custom_ud); Client client(options); StatusOr actual = @@ -259,6 +264,49 @@ TEST_F(CreateSignedUrlTest, V4SignRemote) { EXPECT_THAT(*actual, HasSubstr(expected_signed_blob_hex)); } +/// @test Verify that CreateV4SignedUrl() respects the custom endpoint. +TEST_F(CreateSignedUrlTest, V4SignCustomEndpoint) { + auto const custom_endpoint = std::string{"https://storage.mydomain.com"}; + std::string const bucket_name = "test-bucket"; + std::string const object_name = "test-object"; + std::string const date = "2019-02-01T09:00:00Z"; + auto const valid_for = std::chrono::seconds(10); + + Options options = + Options{} + .set( + MakeServiceAccountCredentials(kJsonKeyfileContentsForV4)) + .set(custom_endpoint); + Client client(options); + + auto actual = client.CreateV4SignedUrl( + "GET", bucket_name, object_name, + SignedUrlTimestamp(google::cloud::internal::ParseRfc3339(date).value()), + SignedUrlDuration(valid_for)); + EXPECT_THAT(actual, IsOkAndHolds(StartsWith(custom_endpoint))); +} + +/// @test Verify that CreateV4SignUrl() respects the custom universe domain. +TEST_F(CreateSignedUrlTest, V4SignCustomUniverseDomain) { + auto const custom_ud = std::string{"mydomain.com"}; + std::string const bucket_name = "test-bucket"; + std::string const object_name = "test-object"; + std::string const date = "2019-02-01T09:00:00Z"; + auto const valid_for = std::chrono::seconds(10); + + Options options = + Options{} + .set( + MakeServiceAccountCredentials(kJsonKeyfileContentsForV4)) + .set(custom_ud); + Client client(options); + auto actual = client.CreateV4SignedUrl( + "GET", bucket_name, object_name, + SignedUrlTimestamp(google::cloud::internal::ParseRfc3339(date).value()), + SignedUrlDuration(valid_for)); + EXPECT_THAT(actual, IsOkAndHolds(HasSubstr(custom_ud))); +} + TEST_F(CreateSignedUrlTest, V4SignRemoteNoSigningEmail) { EXPECT_CALL(*mock_, SignBlob).Times(0); auto client = ClientForMock(); diff --git a/google/cloud/storage/internal/signed_url_requests.cc b/google/cloud/storage/internal/signed_url_requests.cc index e923fb414945..65c7616f82f3 100644 --- a/google/cloud/storage/internal/signed_url_requests.cc +++ b/google/cloud/storage/internal/signed_url_requests.cc @@ -235,19 +235,20 @@ Status V4SignUrlRequest::Validate() { } std::string V4SignUrlRequest::Hostname() { + auto const host = common_request_.host(); if (virtual_host_name_) { - return common_request_.bucket_name() + ".storage.googleapis.com"; + return common_request_.bucket_name() + "." + host; } if (domain_named_bucket_) { return *domain_named_bucket_; } - return "storage.googleapis.com"; + return host; } std::string V4SignUrlRequest::HostnameWithBucket() { - return scheme_ + "://" + Hostname() + - (SkipBucketInPath() ? std::string() - : ("/" + common_request_.bucket_name())); + return Hostname() + (SkipBucketInPath() + ? std::string() + : ("/" + common_request_.bucket_name())); } std::chrono::system_clock::time_point V4SignUrlRequest::DefaultTimestamp() { diff --git a/google/cloud/storage/internal/signed_url_requests.h b/google/cloud/storage/internal/signed_url_requests.h index 6fd63111425d..7bd3bee86fd1 100644 --- a/google/cloud/storage/internal/signed_url_requests.h +++ b/google/cloud/storage/internal/signed_url_requests.h @@ -38,14 +38,16 @@ class SignUrlRequestCommon { public: SignUrlRequestCommon() = default; SignUrlRequestCommon(std::string verb, std::string bucket_name, - std::string object_name) + std::string object_name, std::string host) : verb_(std::move(verb)), bucket_name_(std::move(bucket_name)), - object_name_(std::move(object_name)) {} + object_name_(std::move(object_name)), + host_(std::move(host)) {} std::string const& verb() const { return verb_; } std::string const& bucket_name() const { return bucket_name_; } std::string const& object_name() const { return object_name_; } + std::string const& host() const { return host_; } std::string const& sub_resource() const { return sub_resource_; } std::map const& extension_headers() const { return extension_headers_; @@ -94,6 +96,7 @@ class SignUrlRequestCommon { std::string verb_; std::string bucket_name_; std::string object_name_; + std::string host_; std::string sub_resource_; std::map extension_headers_; std::multimap query_parameters_; @@ -108,10 +111,11 @@ class SignUrlRequestCommon { class V2SignUrlRequest { public: V2SignUrlRequest() = default; - explicit V2SignUrlRequest(std::string verb, std::string bucket_name, - std::string object_name) + explicit V2SignUrlRequest( + std::string verb, std::string bucket_name, std::string object_name, + std::string host = "https:://storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), - std::move(object_name)), + std::move(object_name), std::move(host)), expiration_time_(DefaultExpirationTime()) {} std::string const& verb() const { return common_request_.verb(); } @@ -121,6 +125,7 @@ class V2SignUrlRequest { std::string const& object_name() const { return common_request_.object_name(); } + std::string const& host() const { return common_request_.host(); } std::string const& sub_resource() const { return common_request_.sub_resource(); } @@ -221,9 +226,10 @@ class V4SignUrlRequest { public: V4SignUrlRequest() : expires_(0) {} explicit V4SignUrlRequest(std::string verb, std::string bucket_name, - std::string object_name) + std::string object_name, + std::string host = "https://storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), - std::move(object_name)), + std::move(object_name), std::move(host)), scheme_("https"), timestamp_(DefaultTimestamp()), expires_(DefaultExpires()), @@ -236,6 +242,7 @@ class V4SignUrlRequest { std::string const& object_name() const { return common_request_.object_name(); } + std::string const& host() const { return common_request_.host(); } std::vector ObjectNameParts() const; From 84931527f9a133d90f461a34c64e8441734f532d Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 16 May 2024 15:01:47 +0000 Subject: [PATCH 04/14] hostname should return only the host without scheme and changes for PolicyDocument --- google/cloud/storage/client.cc | 7 +++++++ google/cloud/storage/client.h | 4 +++- .../client_sign_policy_document_test.cc | 20 ++++++++++++++++--- .../internal/policy_document_request.cc | 6 ++---- .../internal/policy_document_request.h | 8 +++++++- .../storage/internal/signed_url_requests.cc | 8 ++++---- 6 files changed, 40 insertions(+), 13 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index aa6a510aa348..9bc583ca0dc5 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -460,6 +460,7 @@ StatusOr Client::SignPolicyDocumentV4( if (!signed_blob) { return signed_blob.status(); } + request.SetHost(Host()); std::string signature = google::cloud::internal::HexEncode(signed_blob->signed_blob); auto required_fields = request.RequiredFormFields(); @@ -485,6 +486,12 @@ std::string Client::Endpoint() const { return connection_->options().get(); } +std::string Client::Host() const { + auto const host = Endpoint(); + auto const prefix = std::string{"https://"}; + return host.substr(prefix.length()); +} + namespace internal { Client ClientImplDetails::CreateWithDecorations( diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index ea57e7993152..18bbf28e0dd4 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -2995,7 +2995,7 @@ class Client { Options&&... options) { google::cloud::internal::OptionsSpan const span( SpanOptions(std::forward(options)...)); - auto const host = Endpoint(); + auto const host = Host(); internal::V4SignUrlRequest request(std::move(verb), std::move(bucket_name), std::move(object_name), std::move(host)); request.set_multiple_options(std::forward(options)...); @@ -3495,6 +3495,8 @@ class Client { std::string Endpoint() const; + std::string Host() const; + std::shared_ptr connection_; }; diff --git a/google/cloud/storage/client_sign_policy_document_test.cc b/google/cloud/storage/client_sign_policy_document_test.cc index af20656d94bf..7d3ebe2a9b13 100644 --- a/google/cloud/storage/client_sign_policy_document_test.cc +++ b/google/cloud/storage/client_sign_policy_document_test.cc @@ -33,6 +33,7 @@ using ::google::cloud::internal::CurrentOptions; using ::google::cloud::storage::testing::canonical_errors::TransientError; using ::testing::HasSubstr; using ::testing::Return; +using ::testing::StartsWith; constexpr char kJsonKeyfileContents[] = R"""({ "type": "service_account", @@ -58,9 +59,12 @@ std::string Dec64(std::string const& s) { return std::string(res.begin(), res.end()); }; -Client CreateClientForTest() { - return Client(Options{}.set( - MakeServiceAccountCredentials(kJsonKeyfileContents))); +Client CreateClientForTest( + std::string endpoint = "https://storage.googleapis.com") { + return Client(Options{} + .set( + MakeServiceAccountCredentials(kJsonKeyfileContents)) + .set(std::move(endpoint))); } /** @@ -255,6 +259,16 @@ TEST(CreateSignedPolicyDocTest, SignV4VirtualHostname) { EXPECT_EQ("https://test-bucket.storage.googleapis.com/", actual->url); } +TEST(CreateSignedPolicyDocTest, SignV4CustomEndpoint) { + auto const custom_endpoint = std::string{"https://storage.mydomain.com"}; + auto client = CreateClientForTest(std::move(custom_endpoint)); + auto actual = + client.GenerateSignedPostPolicyV4(CreatePolicyDocumentV4ForTest()); + + ASSERT_STATUS_OK(actual); + EXPECT_THAT(actual->url, StartsWith("https://storage.mydomain.com")); +} + } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage diff --git a/google/cloud/storage/internal/policy_document_request.cc b/google/cloud/storage/internal/policy_document_request.cc index 4cbbab5e5e59..8a7ef8fbaba9 100644 --- a/google/cloud/storage/internal/policy_document_request.cc +++ b/google/cloud/storage/internal/policy_document_request.cc @@ -275,11 +275,9 @@ std::string PolicyDocumentV4Request::Url() const { return scheme_ + "://" + *bucket_bound_domain_ + "/"; } if (virtual_host_name_) { - return scheme_ + "://" + policy_document().bucket + - ".storage.googleapis.com/"; + return scheme_ + "://" + policy_document().bucket + "." + host_ + "/"; } - return scheme_ + "://storage.googleapis.com/" + policy_document().bucket + - "/"; + return scheme_ + "://" + host_ + "/" + policy_document().bucket + "/"; } std::string PolicyDocumentV4Request::Credentials() const { diff --git a/google/cloud/storage/internal/policy_document_request.h b/google/cloud/storage/internal/policy_document_request.h index d82649789539..4931c903ea59 100644 --- a/google/cloud/storage/internal/policy_document_request.h +++ b/google/cloud/storage/internal/policy_document_request.h @@ -104,7 +104,8 @@ std::ostream& operator<<(std::ostream& os, PolicyDocumentRequest const& r); class PolicyDocumentV4Request { public: - PolicyDocumentV4Request() : scheme_("https") {} + PolicyDocumentV4Request() + : scheme_("https"), host_("storage.googleapis.com") {} explicit PolicyDocumentV4Request(PolicyDocumentV4 document) : PolicyDocumentV4Request() { document_ = std::move(document); @@ -125,6 +126,8 @@ class PolicyDocumentV4Request { return signing_account_delegates_; } + std::string host() { return host_; } + void SetOption(SigningAccount const& o) { signing_account_ = o; } void SetOption(SigningAccountDelegates const& o) { @@ -176,6 +179,8 @@ class PolicyDocumentV4Request { signing_email_ = std::move(signing_email); } + void SetHost(std::string host) { host_ = std::move(host); } + std::string Credentials() const; std::map RequiredFormFields() const; @@ -191,6 +196,7 @@ class PolicyDocumentV4Request { absl::optional bucket_bound_domain_; std::string scheme_; bool virtual_host_name_{false}; + std::string host_; }; std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r); diff --git a/google/cloud/storage/internal/signed_url_requests.cc b/google/cloud/storage/internal/signed_url_requests.cc index 65c7616f82f3..5262092275df 100644 --- a/google/cloud/storage/internal/signed_url_requests.cc +++ b/google/cloud/storage/internal/signed_url_requests.cc @@ -235,7 +235,7 @@ Status V4SignUrlRequest::Validate() { } std::string V4SignUrlRequest::Hostname() { - auto const host = common_request_.host(); + auto host = common_request_.host(); if (virtual_host_name_) { return common_request_.bucket_name() + "." + host; } @@ -246,9 +246,9 @@ std::string V4SignUrlRequest::Hostname() { } std::string V4SignUrlRequest::HostnameWithBucket() { - return Hostname() + (SkipBucketInPath() - ? std::string() - : ("/" + common_request_.bucket_name())); + return scheme_ + "://" + Hostname() + + (SkipBucketInPath() ? std::string() + : ("/" + common_request_.bucket_name())); } std::chrono::system_clock::time_point V4SignUrlRequest::DefaultTimestamp() { From 5c2a5338da3e737694622e1db3fe0da079754b2a Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Mon, 20 May 2024 14:54:08 +0000 Subject: [PATCH 05/14] Host default should be hostname not endpoint --- google/cloud/storage/internal/signed_url_requests.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/storage/internal/signed_url_requests.h b/google/cloud/storage/internal/signed_url_requests.h index 7bd3bee86fd1..e619b02467c0 100644 --- a/google/cloud/storage/internal/signed_url_requests.h +++ b/google/cloud/storage/internal/signed_url_requests.h @@ -113,7 +113,7 @@ class V2SignUrlRequest { V2SignUrlRequest() = default; explicit V2SignUrlRequest( std::string verb, std::string bucket_name, std::string object_name, - std::string host = "https:://storage.googleapis.com") + std::string host = "storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), std::move(object_name), std::move(host)), expiration_time_(DefaultExpirationTime()) {} @@ -227,7 +227,7 @@ class V4SignUrlRequest { V4SignUrlRequest() : expires_(0) {} explicit V4SignUrlRequest(std::string verb, std::string bucket_name, std::string object_name, - std::string host = "https://storage.googleapis.com") + std::string host = "storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), std::move(object_name), std::move(host)), scheme_("https"), From 70e0603ddae552f79384099369d252b4d6fce0f6 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Mon, 20 May 2024 15:09:47 +0000 Subject: [PATCH 06/14] fix ci failures --- google/cloud/storage/internal/signed_url_requests.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/google/cloud/storage/internal/signed_url_requests.h b/google/cloud/storage/internal/signed_url_requests.h index e619b02467c0..eb162fe014f1 100644 --- a/google/cloud/storage/internal/signed_url_requests.h +++ b/google/cloud/storage/internal/signed_url_requests.h @@ -111,9 +111,9 @@ class SignUrlRequestCommon { class V2SignUrlRequest { public: V2SignUrlRequest() = default; - explicit V2SignUrlRequest( - std::string verb, std::string bucket_name, std::string object_name, - std::string host = "storage.googleapis.com") + explicit V2SignUrlRequest(std::string verb, std::string bucket_name, + std::string object_name, + std::string host = "storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), std::move(object_name), std::move(host)), expiration_time_(DefaultExpirationTime()) {} From cd09dc54845d2d24b907562594044239bd5cdf34 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 22 May 2024 13:39:12 +0000 Subject: [PATCH 07/14] Fix conformance test failure --- google/cloud/storage/client.cc | 10 +++++++--- .../cloud/storage/tests/signed_url_conformance_test.cc | 3 +++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index 9bc583ca0dc5..dc1271a5a954 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -23,6 +23,7 @@ #include "google/cloud/internal/make_status.h" #include "google/cloud/internal/opentelemetry.h" #include "google/cloud/log.h" +#include "absl/strings/str_split.h" #include #include #include @@ -487,9 +488,12 @@ std::string Client::Endpoint() const { } std::string Client::Host() const { - auto const host = Endpoint(); - auto const prefix = std::string{"https://"}; - return host.substr(prefix.length()); + auto endpoint = Endpoint(); + auto host = absl::string_view(endpoint); + if (!absl::ConsumePrefix(&host, "https://")) { + absl::ConsumePrefix(&host, "http://"); + } + return std::string(host); } namespace internal { diff --git a/google/cloud/storage/tests/signed_url_conformance_test.cc b/google/cloud/storage/tests/signed_url_conformance_test.cc index 8df28bfe4381..60323690afbd 100644 --- a/google/cloud/storage/tests/signed_url_conformance_test.cc +++ b/google/cloud/storage/tests/signed_url_conformance_test.cc @@ -24,6 +24,7 @@ #include "google/cloud/internal/time_utils.h" #include "google/cloud/terminate_handler.h" #include "google/cloud/testing_util/status_matchers.h" +#include "google/cloud/testing_util/scoped_environment.h" #include #include #include @@ -75,6 +76,7 @@ class V4SignedUrlConformanceTest class V4PostPolicyConformanceTest : public V4SignedUrlConformanceTest {}; TEST_P(V4SignedUrlConformanceTest, V4SignJson) { + testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", absl::nullopt); auto creds = oauth2::CreateServiceAccountCredentialsFromJsonFilePath( service_account_key_filename_); ASSERT_STATUS_OK(creds); @@ -176,6 +178,7 @@ INSTANTIATE_TEST_SUITE_P( }())); TEST_P(V4PostPolicyConformanceTest, V4PostPolicy) { + testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", absl::nullopt); auto creds = oauth2::CreateServiceAccountCredentialsFromJsonFilePath( service_account_key_filename_); ASSERT_STATUS_OK(creds); From 4aa989bf2c46f9b3f04f356e4ed34e4a27ef2ff2 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Wed, 22 May 2024 13:52:04 +0000 Subject: [PATCH 08/14] fix other ci failures --- google/cloud/storage/tests/signed_url_conformance_test.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/google/cloud/storage/tests/signed_url_conformance_test.cc b/google/cloud/storage/tests/signed_url_conformance_test.cc index 60323690afbd..eb9371030795 100644 --- a/google/cloud/storage/tests/signed_url_conformance_test.cc +++ b/google/cloud/storage/tests/signed_url_conformance_test.cc @@ -23,8 +23,8 @@ #include "google/cloud/internal/getenv.h" #include "google/cloud/internal/time_utils.h" #include "google/cloud/terminate_handler.h" -#include "google/cloud/testing_util/status_matchers.h" #include "google/cloud/testing_util/scoped_environment.h" +#include "google/cloud/testing_util/status_matchers.h" #include #include #include @@ -76,7 +76,8 @@ class V4SignedUrlConformanceTest class V4PostPolicyConformanceTest : public V4SignedUrlConformanceTest {}; TEST_P(V4SignedUrlConformanceTest, V4SignJson) { - testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", absl::nullopt); + testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", + absl::nullopt); auto creds = oauth2::CreateServiceAccountCredentialsFromJsonFilePath( service_account_key_filename_); ASSERT_STATUS_OK(creds); @@ -178,7 +179,8 @@ INSTANTIATE_TEST_SUITE_P( }())); TEST_P(V4PostPolicyConformanceTest, V4PostPolicy) { - testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", absl::nullopt); + testing_util::ScopedEnvironment endpoint("CLOUD_STORAGE_EMULATOR_ENDPOINT", + absl::nullopt); auto creds = oauth2::CreateServiceAccountCredentialsFromJsonFilePath( service_account_key_filename_); ASSERT_STATUS_OK(creds); From 9b9334b0a78fadcda5539f0f212907d5bbc19bd5 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 10:46:17 +0000 Subject: [PATCH 09/14] address review comments --- google/cloud/storage/client.cc | 5 +-- google/cloud/storage/client.h | 9 ++++-- .../internal/policy_document_request.cc | 6 ++-- .../internal/policy_document_request.h | 10 +++--- .../internal/policy_document_request_test.cc | 24 ++++++++++++++ .../storage/internal/signed_url_requests.cc | 6 ++-- .../storage/internal/signed_url_requests.h | 32 +++++++++++-------- .../internal/signed_url_requests_test.cc | 9 ++++++ 8 files changed, 73 insertions(+), 28 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index dc1271a5a954..b2c54c0bfc4a 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -461,7 +461,6 @@ StatusOr Client::SignPolicyDocumentV4( if (!signed_blob) { return signed_blob.status(); } - request.SetHost(Host()); std::string signature = google::cloud::internal::HexEncode(signed_blob->signed_blob); auto required_fields = request.RequiredFormFields(); @@ -487,7 +486,9 @@ std::string Client::Endpoint() const { return connection_->options().get(); } -std::string Client::Host() const { +// This can be optimized to not have a lot of string copies. +// But the code is rarely used and not in any critical path. +std::string Client::EndpointAuthority() const { auto endpoint = Endpoint(); auto host = absl::string_view(endpoint); if (!absl::ConsumePrefix(&host, "https://")) { diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index 18bbf28e0dd4..f25723a28813 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -2995,9 +2995,9 @@ class Client { Options&&... options) { google::cloud::internal::OptionsSpan const span( SpanOptions(std::forward(options)...)); - auto const host = Host(); internal::V4SignUrlRequest request(std::move(verb), std::move(bucket_name), - std::move(object_name), std::move(host)); + std::move(object_name), + EndpointAuthority()); request.set_multiple_options(std::forward(options)...); return SignUrlV4(std::move(request)); } @@ -3085,6 +3085,7 @@ class Client { SpanOptions(std::forward(options)...)); internal::PolicyDocumentV4Request request(std::move(document)); request.set_multiple_options(std::forward(options)...); + request.SetEndpointAuthority(EndpointAuthority()); return SignPolicyDocumentV4(std::move(request)); } @@ -3493,9 +3494,11 @@ class Client { StatusOr SignPolicyDocumentV4( internal::PolicyDocumentV4Request request); + // The configured endpoint, including any scheme and port. std::string Endpoint() const; - std::string Host() const; + // The hostname:port part of the configured endpoint. + std::string EndpointAuthority() const; std::shared_ptr connection_; }; diff --git a/google/cloud/storage/internal/policy_document_request.cc b/google/cloud/storage/internal/policy_document_request.cc index 8a7ef8fbaba9..cbbdfc6796f1 100644 --- a/google/cloud/storage/internal/policy_document_request.cc +++ b/google/cloud/storage/internal/policy_document_request.cc @@ -275,9 +275,11 @@ std::string PolicyDocumentV4Request::Url() const { return scheme_ + "://" + *bucket_bound_domain_ + "/"; } if (virtual_host_name_) { - return scheme_ + "://" + policy_document().bucket + "." + host_ + "/"; + return scheme_ + "://" + policy_document().bucket + "." + + endpoint_authority_ + "/"; } - return scheme_ + "://" + host_ + "/" + policy_document().bucket + "/"; + return scheme_ + "://" + endpoint_authority_ + "/" + + policy_document().bucket + "/"; } std::string PolicyDocumentV4Request::Credentials() const { diff --git a/google/cloud/storage/internal/policy_document_request.h b/google/cloud/storage/internal/policy_document_request.h index 4931c903ea59..7c56f51f88ff 100644 --- a/google/cloud/storage/internal/policy_document_request.h +++ b/google/cloud/storage/internal/policy_document_request.h @@ -105,7 +105,7 @@ std::ostream& operator<<(std::ostream& os, PolicyDocumentRequest const& r); class PolicyDocumentV4Request { public: PolicyDocumentV4Request() - : scheme_("https"), host_("storage.googleapis.com") {} + : scheme_("https"), endpoint_authority_("storage.googleapis.com") {} explicit PolicyDocumentV4Request(PolicyDocumentV4 document) : PolicyDocumentV4Request() { document_ = std::move(document); @@ -126,7 +126,7 @@ class PolicyDocumentV4Request { return signing_account_delegates_; } - std::string host() { return host_; } + std::string endpoint_authority() { return endpoint_authority_; } void SetOption(SigningAccount const& o) { signing_account_ = o; } @@ -179,7 +179,9 @@ class PolicyDocumentV4Request { signing_email_ = std::move(signing_email); } - void SetHost(std::string host) { host_ = std::move(host); } + void SetEndpointAuthority(std::string endpoint_authority) { + endpoint_authority_ = std::move(endpoint_authority); + } std::string Credentials() const; @@ -196,7 +198,7 @@ class PolicyDocumentV4Request { absl::optional bucket_bound_domain_; std::string scheme_; bool virtual_host_name_{false}; - std::string host_; + std::string endpoint_authority_; }; std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r); diff --git a/google/cloud/storage/internal/policy_document_request_test.cc b/google/cloud/storage/internal/policy_document_request_test.cc index defe689cc814..aa7fd11d35b8 100644 --- a/google/cloud/storage/internal/policy_document_request_test.cc +++ b/google/cloud/storage/internal/policy_document_request_test.cc @@ -28,6 +28,7 @@ namespace { using ::google::cloud::testing_util::IsOkAndHolds; using ::google::cloud::testing_util::StatusIs; using ::testing::ElementsAre; +using ::testing::StartsWith; TEST(PolicyDocumentRequest, SigningAccount) { PolicyDocumentRequest request; @@ -153,6 +154,29 @@ TEST(PolicyDocumentV4Request, RequiredFormFields) { EXPECT_EQ(expected_fields, req.RequiredFormFields()); } +TEST(PolicyDocumentV4Request, Url) { + PolicyDocumentV4 doc; + doc.bucket = "test-bucket"; + PolicyDocumentV4Request request(doc); + auto const custom_endpoint_authority = std::string{"mydomain.com"}; + request.SetEndpointAuthority(custom_endpoint_authority); + EXPECT_THAT(request.Url(), + StartsWith("https://" + custom_endpoint_authority)); +} + +TEST(PolicyDocumentV4Request, UrlWithVirtualHostName) { + PolicyDocumentV4 doc; + doc.bucket = "test-bucket"; + auto const custom_endpoint_authority = std::string{"mydomain.com"}; + PolicyDocumentV4Request request(doc); + request.SetOption(VirtualHostname(true)); + request.SetEndpointAuthority(custom_endpoint_authority); + + auto const expected_url = std::string{"https://" + doc.bucket + "." + + custom_endpoint_authority + "/"}; + EXPECT_EQ(request.Url(), expected_url); +} + } // namespace } // namespace internal GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END diff --git a/google/cloud/storage/internal/signed_url_requests.cc b/google/cloud/storage/internal/signed_url_requests.cc index 5262092275df..2003b6dc9411 100644 --- a/google/cloud/storage/internal/signed_url_requests.cc +++ b/google/cloud/storage/internal/signed_url_requests.cc @@ -235,14 +235,14 @@ Status V4SignUrlRequest::Validate() { } std::string V4SignUrlRequest::Hostname() { - auto host = common_request_.host(); + auto endpoint_authority = common_request_.endpoint_authority(); if (virtual_host_name_) { - return common_request_.bucket_name() + "." + host; + return common_request_.bucket_name() + "." + endpoint_authority; } if (domain_named_bucket_) { return *domain_named_bucket_; } - return host; + return endpoint_authority; } std::string V4SignUrlRequest::HostnameWithBucket() { diff --git a/google/cloud/storage/internal/signed_url_requests.h b/google/cloud/storage/internal/signed_url_requests.h index eb162fe014f1..2df11421a4ef 100644 --- a/google/cloud/storage/internal/signed_url_requests.h +++ b/google/cloud/storage/internal/signed_url_requests.h @@ -38,16 +38,16 @@ class SignUrlRequestCommon { public: SignUrlRequestCommon() = default; SignUrlRequestCommon(std::string verb, std::string bucket_name, - std::string object_name, std::string host) + std::string object_name, std::string endpoint_authority) : verb_(std::move(verb)), bucket_name_(std::move(bucket_name)), object_name_(std::move(object_name)), - host_(std::move(host)) {} + endpoint_authority_(std::move(endpoint_authority)) {} std::string const& verb() const { return verb_; } std::string const& bucket_name() const { return bucket_name_; } std::string const& object_name() const { return object_name_; } - std::string const& host() const { return host_; } + std::string const& endpoint_authority() const { return endpoint_authority_; } std::string const& sub_resource() const { return sub_resource_; } std::map const& extension_headers() const { return extension_headers_; @@ -96,7 +96,7 @@ class SignUrlRequestCommon { std::string verb_; std::string bucket_name_; std::string object_name_; - std::string host_; + std::string endpoint_authority_; std::string sub_resource_; std::map extension_headers_; std::multimap query_parameters_; @@ -111,11 +111,11 @@ class SignUrlRequestCommon { class V2SignUrlRequest { public: V2SignUrlRequest() = default; - explicit V2SignUrlRequest(std::string verb, std::string bucket_name, - std::string object_name, - std::string host = "storage.googleapis.com") + explicit V2SignUrlRequest( + std::string verb, std::string bucket_name, std::string object_name, + std::string endpoint_authority = "storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), - std::move(object_name), std::move(host)), + std::move(object_name), std::move(endpoint_authority)), expiration_time_(DefaultExpirationTime()) {} std::string const& verb() const { return common_request_.verb(); } @@ -125,7 +125,9 @@ class V2SignUrlRequest { std::string const& object_name() const { return common_request_.object_name(); } - std::string const& host() const { return common_request_.host(); } + std::string const& endpoint_authority() const { + return common_request_.endpoint_authority(); + } std::string const& sub_resource() const { return common_request_.sub_resource(); } @@ -225,11 +227,11 @@ std::ostream& operator<<(std::ostream& os, V2SignUrlRequest const& r); class V4SignUrlRequest { public: V4SignUrlRequest() : expires_(0) {} - explicit V4SignUrlRequest(std::string verb, std::string bucket_name, - std::string object_name, - std::string host = "storage.googleapis.com") + explicit V4SignUrlRequest( + std::string verb, std::string bucket_name, std::string object_name, + std::string endpoint_authority = "storage.googleapis.com") : common_request_(std::move(verb), std::move(bucket_name), - std::move(object_name), std::move(host)), + std::move(object_name), std::move(endpoint_authority)), scheme_("https"), timestamp_(DefaultTimestamp()), expires_(DefaultExpires()), @@ -242,7 +244,9 @@ class V4SignUrlRequest { std::string const& object_name() const { return common_request_.object_name(); } - std::string const& host() const { return common_request_.host(); } + std::string const& endpoint_authority() const { + return common_request_.endpoint_authority(); + } std::vector ObjectNameParts() const; diff --git a/google/cloud/storage/internal/signed_url_requests_test.cc b/google/cloud/storage/internal/signed_url_requests_test.cc index 8687e29d2952..c8e5f578071d 100644 --- a/google/cloud/storage/internal/signed_url_requests_test.cc +++ b/google/cloud/storage/internal/signed_url_requests_test.cc @@ -28,6 +28,7 @@ namespace { using ::google::cloud::testing_util::StatusIs; using ::testing::ElementsAre; using ::testing::HasSubstr; +using ::testing::StartsWith; TEST(V2SignedUrlRequests, Sign) { V2SignUrlRequest request("GET", "test-bucket", "test-object"); @@ -641,6 +642,14 @@ TEST(V4SignedUrlRequests, BucketBoundHostnameReset) { EXPECT_EQ(expected, actual); } +TEST(V4SignedUrlRequests, CustomEndpoint) { + auto const custom_endpoint_authority = std::string{"mydomain.com"}; + V4SignUrlRequest request("GET", "test-bucket", "test-object", + custom_endpoint_authority); + ASSERT_STATUS_OK(request.Validate()); + EXPECT_THAT(request.Hostname(), StartsWith(custom_endpoint_authority)); +} + TEST(DefaultCtorsWork, Trivial) { EXPECT_FALSE(ExpirationTime().has_value()); EXPECT_FALSE(AddExtensionHeaderOption().has_value()); From 7bd4dd978d96964346c140b0be277a09921c4e19 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 10:57:02 +0000 Subject: [PATCH 10/14] further corrections --- google/cloud/storage/client.cc | 8 ++++---- google/cloud/storage/client_sign_policy_document_test.cc | 2 +- .../storage/internal/policy_document_request_test.cc | 4 ++-- google/cloud/storage/internal/signed_url_requests_test.cc | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/client.cc b/google/cloud/storage/client.cc index b2c54c0bfc4a..25e4271f8a35 100644 --- a/google/cloud/storage/client.cc +++ b/google/cloud/storage/client.cc @@ -490,11 +490,11 @@ std::string Client::Endpoint() const { // But the code is rarely used and not in any critical path. std::string Client::EndpointAuthority() const { auto endpoint = Endpoint(); - auto host = absl::string_view(endpoint); - if (!absl::ConsumePrefix(&host, "https://")) { - absl::ConsumePrefix(&host, "http://"); + auto endpoint_authority = absl::string_view(endpoint); + if (!absl::ConsumePrefix(&endpoint_authority, "https://")) { + absl::ConsumePrefix(&endpoint_authority, "http://"); } - return std::string(host); + return std::string(endpoint_authority); } namespace internal { diff --git a/google/cloud/storage/client_sign_policy_document_test.cc b/google/cloud/storage/client_sign_policy_document_test.cc index 7d3ebe2a9b13..a12b89d00691 100644 --- a/google/cloud/storage/client_sign_policy_document_test.cc +++ b/google/cloud/storage/client_sign_policy_document_test.cc @@ -266,7 +266,7 @@ TEST(CreateSignedPolicyDocTest, SignV4CustomEndpoint) { client.GenerateSignedPostPolicyV4(CreatePolicyDocumentV4ForTest()); ASSERT_STATUS_OK(actual); - EXPECT_THAT(actual->url, StartsWith("https://storage.mydomain.com")); + EXPECT_THAT(actual->url, StartsWith(custom_endpoint)); } } // namespace diff --git a/google/cloud/storage/internal/policy_document_request_test.cc b/google/cloud/storage/internal/policy_document_request_test.cc index aa7fd11d35b8..48d126f15539 100644 --- a/google/cloud/storage/internal/policy_document_request_test.cc +++ b/google/cloud/storage/internal/policy_document_request_test.cc @@ -158,7 +158,7 @@ TEST(PolicyDocumentV4Request, Url) { PolicyDocumentV4 doc; doc.bucket = "test-bucket"; PolicyDocumentV4Request request(doc); - auto const custom_endpoint_authority = std::string{"mydomain.com"}; + auto const custom_endpoint_authority = std::string{"storage.mydomain.com"}; request.SetEndpointAuthority(custom_endpoint_authority); EXPECT_THAT(request.Url(), StartsWith("https://" + custom_endpoint_authority)); @@ -167,7 +167,7 @@ TEST(PolicyDocumentV4Request, Url) { TEST(PolicyDocumentV4Request, UrlWithVirtualHostName) { PolicyDocumentV4 doc; doc.bucket = "test-bucket"; - auto const custom_endpoint_authority = std::string{"mydomain.com"}; + auto const custom_endpoint_authority = std::string{"storage.mydomain.com"}; PolicyDocumentV4Request request(doc); request.SetOption(VirtualHostname(true)); request.SetEndpointAuthority(custom_endpoint_authority); diff --git a/google/cloud/storage/internal/signed_url_requests_test.cc b/google/cloud/storage/internal/signed_url_requests_test.cc index c8e5f578071d..22b04c822efb 100644 --- a/google/cloud/storage/internal/signed_url_requests_test.cc +++ b/google/cloud/storage/internal/signed_url_requests_test.cc @@ -643,7 +643,7 @@ TEST(V4SignedUrlRequests, BucketBoundHostnameReset) { } TEST(V4SignedUrlRequests, CustomEndpoint) { - auto const custom_endpoint_authority = std::string{"mydomain.com"}; + auto const custom_endpoint_authority = std::string{"storage.mydomain.com"}; V4SignUrlRequest request("GET", "test-bucket", "test-object", custom_endpoint_authority); ASSERT_STATUS_OK(request.Validate()); From d10e9c738bfa42311be3c1b36a491031d1cfa205 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 11:51:35 +0000 Subject: [PATCH 11/14] fix ci failure --- google/cloud/storage/client_sign_policy_document_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/client_sign_policy_document_test.cc b/google/cloud/storage/client_sign_policy_document_test.cc index a12b89d00691..e70e585544d6 100644 --- a/google/cloud/storage/client_sign_policy_document_test.cc +++ b/google/cloud/storage/client_sign_policy_document_test.cc @@ -261,7 +261,7 @@ TEST(CreateSignedPolicyDocTest, SignV4VirtualHostname) { TEST(CreateSignedPolicyDocTest, SignV4CustomEndpoint) { auto const custom_endpoint = std::string{"https://storage.mydomain.com"}; - auto client = CreateClientForTest(std::move(custom_endpoint)); + auto client = CreateClientForTest(custom_endpoint); auto actual = client.GenerateSignedPostPolicyV4(CreatePolicyDocumentV4ForTest()); From 7e09ce2e062b9f9f02d56989d42f350d271664a7 Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 14:20:15 +0000 Subject: [PATCH 12/14] Add new field endpoint_authority to operator<< --- google/cloud/storage/internal/policy_document_request.cc | 7 ++++++- google/cloud/storage/internal/policy_document_request.h | 2 +- .../cloud/storage/internal/policy_document_request_test.cc | 4 +++- google/cloud/storage/internal/signed_url_requests.cc | 5 +++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/google/cloud/storage/internal/policy_document_request.cc b/google/cloud/storage/internal/policy_document_request.cc index cbbdfc6796f1..da4bbbdb041a 100644 --- a/google/cloud/storage/internal/policy_document_request.cc +++ b/google/cloud/storage/internal/policy_document_request.cc @@ -342,7 +342,12 @@ std::map PolicyDocumentV4Request::RequiredFormFields() } std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r) { - return os << "PolicyDocumentRequest={" << r.StringToSign() << "}"; + using nlohmann::json; + json j; + j["endpoint_authority"] = r.endpoint_authority(); + + return os << "PolicyDocumentRequest={" << std::move(j.dump()) << "," + << r.StringToSign() << "}"; } } // namespace internal diff --git a/google/cloud/storage/internal/policy_document_request.h b/google/cloud/storage/internal/policy_document_request.h index 7c56f51f88ff..17e9fc96afb9 100644 --- a/google/cloud/storage/internal/policy_document_request.h +++ b/google/cloud/storage/internal/policy_document_request.h @@ -126,7 +126,7 @@ class PolicyDocumentV4Request { return signing_account_delegates_; } - std::string endpoint_authority() { return endpoint_authority_; } + std::string endpoint_authority() const { return endpoint_authority_; } void SetOption(SigningAccount const& o) { signing_account_ = o; } diff --git a/google/cloud/storage/internal/policy_document_request_test.cc b/google/cloud/storage/internal/policy_document_request_test.cc index 48d126f15539..6c2af683023a 100644 --- a/google/cloud/storage/internal/policy_document_request_test.cc +++ b/google/cloud/storage/internal/policy_document_request_test.cc @@ -120,7 +120,9 @@ TEST(PolicyDocumentV4Request, Printing) { std::stringstream stream; stream << req; EXPECT_EQ( - "PolicyDocumentRequest={{\"conditions\":[{\"bucket\":\"test-bucket\"},{" + "PolicyDocumentRequest={{\"endpoint_authority\":\"storage.googleapis." + "com\"}," + "{\"conditions\":[{\"bucket\":\"test-bucket\"},{" "\"key\":\"test-object\"},{\"x-goog-date\":\"20100616T111111Z\"},{\"x-" "goog-credential\":\"/20100616/auto/storage/" "goog4_request\"},{\"x-goog-algorithm\":\"GOOG4-RSA-SHA256\"}]," diff --git a/google/cloud/storage/internal/signed_url_requests.cc b/google/cloud/storage/internal/signed_url_requests.cc index 2003b6dc9411..43b4406c4cfb 100644 --- a/google/cloud/storage/internal/signed_url_requests.cc +++ b/google/cloud/storage/internal/signed_url_requests.cc @@ -100,7 +100,8 @@ std::string V2SignUrlRequest::StringToSign() const { } std::ostream& operator<<(std::ostream& os, V2SignUrlRequest const& r) { - return os << "SingUrlRequest={" << r.StringToSign() << "}"; + return os << "SingUrlRequest={" << r.endpoint_authority() << "," + << r.StringToSign() << "}"; } namespace { @@ -319,7 +320,7 @@ std::string V4SignUrlRequest::PayloadHashValue() const { } std::ostream& operator<<(std::ostream& os, V4SignUrlRequest const& r) { - return os << "V4SignUrlRequest={" + return os << "V4SignUrlRequest={" << r.endpoint_authority() << "," << r.CanonicalRequest("placeholder-client-id") << "," << r.StringToSign("placeholder-client-id") << "}"; } From 206e5eb6db6c06ba771c3bcd34e46ded75f88d3b Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 15:38:35 +0000 Subject: [PATCH 13/14] Move endpoint_authority to constructor --- google/cloud/storage/client.h | 4 ++-- .../storage/internal/policy_document_request.cc | 8 ++------ .../storage/internal/policy_document_request.h | 13 ++++++++----- .../internal/policy_document_request_test.cc | 9 +++------ 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/google/cloud/storage/client.h b/google/cloud/storage/client.h index f25723a28813..3934938e46f0 100644 --- a/google/cloud/storage/client.h +++ b/google/cloud/storage/client.h @@ -3083,9 +3083,9 @@ class Client { PolicyDocumentV4 document, Options&&... options) { google::cloud::internal::OptionsSpan const span( SpanOptions(std::forward(options)...)); - internal::PolicyDocumentV4Request request(std::move(document)); + internal::PolicyDocumentV4Request request(std::move(document), + EndpointAuthority()); request.set_multiple_options(std::forward(options)...); - request.SetEndpointAuthority(EndpointAuthority()); return SignPolicyDocumentV4(std::move(request)); } diff --git a/google/cloud/storage/internal/policy_document_request.cc b/google/cloud/storage/internal/policy_document_request.cc index da4bbbdb041a..c077c916e95e 100644 --- a/google/cloud/storage/internal/policy_document_request.cc +++ b/google/cloud/storage/internal/policy_document_request.cc @@ -342,12 +342,8 @@ std::map PolicyDocumentV4Request::RequiredFormFields() } std::ostream& operator<<(std::ostream& os, PolicyDocumentV4Request const& r) { - using nlohmann::json; - json j; - j["endpoint_authority"] = r.endpoint_authority(); - - return os << "PolicyDocumentRequest={" << std::move(j.dump()) << "," - << r.StringToSign() << "}"; + return os << "PolicyDocumentRequest={endpoint_authority=" + << r.endpoint_authority() << "," << r.StringToSign() << "}"; } } // namespace internal diff --git a/google/cloud/storage/internal/policy_document_request.h b/google/cloud/storage/internal/policy_document_request.h index 17e9fc96afb9..c752518f16aa 100644 --- a/google/cloud/storage/internal/policy_document_request.h +++ b/google/cloud/storage/internal/policy_document_request.h @@ -105,7 +105,14 @@ std::ostream& operator<<(std::ostream& os, PolicyDocumentRequest const& r); class PolicyDocumentV4Request { public: PolicyDocumentV4Request() - : scheme_("https"), endpoint_authority_("storage.googleapis.com") {} + : PolicyDocumentV4Request("storage.googleapis.com") {} + explicit PolicyDocumentV4Request(std::string endpoint_authority) + : scheme_("https"), endpoint_authority_(std::move(endpoint_authority)) {} + PolicyDocumentV4Request(PolicyDocumentV4 document, + std::string endpoint_authority) + : PolicyDocumentV4Request(std::move(endpoint_authority)) { + document_ = std::move(document); + } explicit PolicyDocumentV4Request(PolicyDocumentV4 document) : PolicyDocumentV4Request() { document_ = std::move(document); @@ -179,10 +186,6 @@ class PolicyDocumentV4Request { signing_email_ = std::move(signing_email); } - void SetEndpointAuthority(std::string endpoint_authority) { - endpoint_authority_ = std::move(endpoint_authority); - } - std::string Credentials() const; std::map RequiredFormFields() const; diff --git a/google/cloud/storage/internal/policy_document_request_test.cc b/google/cloud/storage/internal/policy_document_request_test.cc index 6c2af683023a..207c3f4a4b2a 100644 --- a/google/cloud/storage/internal/policy_document_request_test.cc +++ b/google/cloud/storage/internal/policy_document_request_test.cc @@ -120,8 +120,7 @@ TEST(PolicyDocumentV4Request, Printing) { std::stringstream stream; stream << req; EXPECT_EQ( - "PolicyDocumentRequest={{\"endpoint_authority\":\"storage.googleapis." - "com\"}," + "PolicyDocumentRequest={endpoint_authority=storage.googleapis.com,," "{\"conditions\":[{\"bucket\":\"test-bucket\"},{" "\"key\":\"test-object\"},{\"x-goog-date\":\"20100616T111111Z\"},{\"x-" "goog-credential\":\"/20100616/auto/storage/" @@ -159,9 +158,8 @@ TEST(PolicyDocumentV4Request, RequiredFormFields) { TEST(PolicyDocumentV4Request, Url) { PolicyDocumentV4 doc; doc.bucket = "test-bucket"; - PolicyDocumentV4Request request(doc); auto const custom_endpoint_authority = std::string{"storage.mydomain.com"}; - request.SetEndpointAuthority(custom_endpoint_authority); + PolicyDocumentV4Request request(doc, custom_endpoint_authority); EXPECT_THAT(request.Url(), StartsWith("https://" + custom_endpoint_authority)); } @@ -170,9 +168,8 @@ TEST(PolicyDocumentV4Request, UrlWithVirtualHostName) { PolicyDocumentV4 doc; doc.bucket = "test-bucket"; auto const custom_endpoint_authority = std::string{"storage.mydomain.com"}; - PolicyDocumentV4Request request(doc); + PolicyDocumentV4Request request(doc, custom_endpoint_authority); request.SetOption(VirtualHostname(true)); - request.SetEndpointAuthority(custom_endpoint_authority); auto const expected_url = std::string{"https://" + doc.bucket + "." + custom_endpoint_authority + "/"}; From feb02f88a572cee51097a119814254c069af08db Mon Sep 17 00:00:00 2001 From: bajajnehaa Date: Thu, 23 May 2024 16:23:19 +0000 Subject: [PATCH 14/14] fix typo --- google/cloud/storage/internal/policy_document_request_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/internal/policy_document_request_test.cc b/google/cloud/storage/internal/policy_document_request_test.cc index 207c3f4a4b2a..ad04ee24fd46 100644 --- a/google/cloud/storage/internal/policy_document_request_test.cc +++ b/google/cloud/storage/internal/policy_document_request_test.cc @@ -120,7 +120,7 @@ TEST(PolicyDocumentV4Request, Printing) { std::stringstream stream; stream << req; EXPECT_EQ( - "PolicyDocumentRequest={endpoint_authority=storage.googleapis.com,," + "PolicyDocumentRequest={endpoint_authority=storage.googleapis.com," "{\"conditions\":[{\"bucket\":\"test-bucket\"},{" "\"key\":\"test-object\"},{\"x-goog-date\":\"20100616T111111Z\"},{\"x-" "goog-credential\":\"/20100616/auto/storage/"