From 38871faed6685e656c52cee7a562274bb1974126 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 14:22:02 -0400 Subject: [PATCH 1/7] Improve error messages when loading credentials. This fixes #974. The error messages when loading an invalid or missing credentials file were sometimes very obscure (typically some parsing problem in the JSON library). The new error messages should make it easier to diagnose the problem. --- .../oauth2/authorized_user_credentials.h | 58 +++++++++++---- .../authorized_user_credentials_test.cc | 59 ++++++++++++++- .../storage/oauth2/google_credentials.cc | 26 ++++--- .../storage/oauth2/google_credentials_test.cc | 71 ++++++++++++++++++- .../oauth2/service_account_credentials.h | 69 +++++++++++++----- .../service_account_credentials_test.cc | 59 ++++++++++++++- 6 files changed, 294 insertions(+), 48 deletions(-) diff --git a/google/cloud/storage/oauth2/authorized_user_credentials.h b/google/cloud/storage/oauth2/authorized_user_credentials.h index 62a5fb6f70728..f05cc7f684356 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials.h +++ b/google/cloud/storage/oauth2/authorized_user_credentials.h @@ -53,26 +53,49 @@ template class AuthorizedUserCredentials : public Credentials { public: - explicit AuthorizedUserCredentials(std::string const& contents) - : AuthorizedUserCredentials(contents, GoogleOAuthRefreshEndpoint()) {} + explicit AuthorizedUserCredentials(std::string const& contents, + std::string const& source) + : AuthorizedUserCredentials(contents, source, + GoogleOAuthRefreshEndpoint()) {} explicit AuthorizedUserCredentials(std::string const& content, + std::string const& source, std::string oauth_server) : expiration_time_() { HttpRequestBuilderType request_builder( std::move(oauth_server), storage::internal::GetDefaultCurlHandleFactory()); - auto credentials = storage::internal::nl::json::parse(content); + auto credentials = + storage::internal::nl::json::parse(content, nullptr, false); + if (credentials.is_null()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid AuthorizedUserCredentials, parsing failed on data from " + + source); + } + char const CLIENT_ID_KEY[] = "client_id"; + char const CLIENT_SECRET_KEY[] = "client_secret"; + char const REFRESH_TOKEN_KEY[] = "refresh_token"; + for (auto const& key : + {CLIENT_ID_KEY, CLIENT_SECRET_KEY, REFRESH_TOKEN_KEY}) { + if (credentials.count(key) == 0U) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid AuthorizedUserCredentials, the " + std::string(key) + + " field is missing on data loaded from " + source); + } + } std::string payload("grant_type=refresh_token"); payload += "&client_id="; payload += - request_builder.MakeEscapedString(credentials["client_id"]).get(); + request_builder.MakeEscapedString(credentials.value(CLIENT_ID_KEY, "")) + .get(); payload += "&client_secret="; - payload += - request_builder.MakeEscapedString(credentials["client_secret"]).get(); + payload += request_builder + .MakeEscapedString(credentials.value(CLIENT_SECRET_KEY, "")) + .get(); payload += "&refresh_token="; - payload += - request_builder.MakeEscapedString(credentials["refresh_token"]).get(); + payload += request_builder + .MakeEscapedString(credentials.value(REFRESH_TOKEN_KEY, "")) + .get(); payload_ = std::move(payload); request_ = request_builder.BuildRequest(); } @@ -92,16 +115,23 @@ class AuthorizedUserCredentials : public Credentials { // TODO(#516) - use retry policies to refresh the credentials. auto response = request_.MakeRequest(payload_); - if (200 != response.status_code) { + if (response.status_code >= 300) { + return false; + } + nl::json access_token = nl::json::parse(response.payload, nullptr, false); + if (access_token.is_discarded() or access_token.count("token_type") == 0U or + access_token.count("access_token") == 0U or + access_token.count("id_token") == 0U or + access_token.count("expires_in") == 0U) { return false; } - nl::json access_token = nl::json::parse(response.payload); std::string header = "Authorization: "; - header += access_token["token_type"].get_ref(); + header += access_token.value("token_type", ""); header += ' '; - header += access_token["access_token"].get_ref(); - std::string new_id = access_token["id_token"]; - auto expires_in = std::chrono::seconds(access_token["expires_in"]); + header += access_token.value("access_token", ""); + std::string new_id = access_token.value("id_token", ""); + auto expires_in = + std::chrono::seconds(access_token.value("expires_in", int(0))); auto new_expiration = std::chrono::system_clock::now() + expires_in - GoogleOAuthAccessTokenExpirationSlack(); // Do not update any state until all potential exceptions are raised. diff --git a/google/cloud/storage/oauth2/authorized_user_credentials_test.cc b/google/cloud/storage/oauth2/authorized_user_credentials_test.cc index 9c3c9f04ae0bc..1bae7ec00bdad 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials_test.cc +++ b/google/cloud/storage/oauth2/authorized_user_credentials_test.cc @@ -87,7 +87,7 @@ TEST_F(AuthorizedUserCredentialsTest, Simple) { "type": "magic_type" })"""; - AuthorizedUserCredentials credentials(config); + AuthorizedUserCredentials credentials(config, "test"); EXPECT_EQ("Authorization: Type access-token-value", credentials.AuthorizationHeader()); } @@ -137,7 +137,7 @@ TEST_F(AuthorizedUserCredentialsTest, Refresh) { "refresh_token": "1/THETOKEN", "type": "magic_type" })"""; - AuthorizedUserCredentials credentials(config); + AuthorizedUserCredentials credentials(config, "test"); EXPECT_EQ("Authorization: Type access-token-r1", credentials.AuthorizationHeader()); EXPECT_EQ("Authorization: Type access-token-r2", @@ -146,6 +146,61 @@ TEST_F(AuthorizedUserCredentialsTest, Refresh) { credentials.AuthorizationHeader()); } +/// @test Verify that invalid contents result in a readable error. +TEST_F(AuthorizedUserCredentialsTest, InvalidContents) { + std::string config = R"""( not-a-valid-json-string })"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + auto mock_builder = MockHttpRequestBuilder::mock; + EXPECT_CALL(*mock_builder, Constructor(GoogleOAuthRefreshEndpoint())) + .Times(1); + EXPECT_THROW( + try { + AuthorizedUserCredentials credentials( + config, "test-as-a-source"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Invalid AuthorizedUserCredentials")); + EXPECT_THAT(ex.what(), HasSubstr("test-as-a-source")); + throw; + }, + std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED( + AuthorizedUserCredentials(config, "test-as-a-source"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +/// @test Verify that missing fields result in a readable error. +TEST_F(AuthorizedUserCredentialsTest, MissingContents) { + std::string config = R"""({ + // "client_id": "a-client-id.example.com", + "client_secret": "a-123456ABCDEF", + "refresh_token": "1/THETOKEN", + "type": "magic_type" +})"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + auto mock_builder = MockHttpRequestBuilder::mock; + EXPECT_CALL(*mock_builder, Constructor(GoogleOAuthRefreshEndpoint())) + .Times(1); + EXPECT_THROW( + try { + AuthorizedUserCredentials credentials( + config, "test-as-a-source"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("the client_id field is missing")); + EXPECT_THAT(ex.what(), HasSubstr("test-as-a-source")); + throw; + }, + std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED( + AuthorizedUserCredentials(config, "test-as-a-source"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + } // namespace } // namespace oauth2 } // namespace STORAGE_CLIENT_NS diff --git a/google/cloud/storage/oauth2/google_credentials.cc b/google/cloud/storage/oauth2/google_credentials.cc index e5f30b247b899..b70eeea7443bd 100644 --- a/google/cloud/storage/oauth2/google_credentials.cc +++ b/google/cloud/storage/oauth2/google_credentials.cc @@ -29,18 +29,28 @@ std::shared_ptr GoogleDefaultCredentials() { auto path = GoogleAdcFilePathOrEmpty(); if (not path.empty()) { std::ifstream is(path); + if (not is.is_open()) { + google::cloud::internal::RaiseRuntimeError( + "Cannot open credentials file " + path); + } std::string contents(std::istreambuf_iterator{is}, {}); - auto cred_json = storage::internal::nl::json::parse(contents); + auto cred_json = + storage::internal::nl::json::parse(contents, nullptr, false); + if (cred_json.is_discarded()) { + google::cloud::internal::RaiseRuntimeError( + "Invalid contents in credentials file " + path); + } std::string cred_type = cred_json.value("type", "no type given"); if (cred_type == "authorized_user") { - return std::make_shared>(contents); + return std::make_shared>(contents, path); } if (cred_type == "service_account") { - return std::make_shared>(contents); + return std::make_shared>(contents, path); } google::cloud::internal::RaiseRuntimeError( "Unsupported credential type (" + cred_type + - ") when reading Application Default Credentials file."); + ") when reading Application Default Credentials file from " + path + + "."); } // TODO(#579): Check for implicit environment-based credentials if no ADC file @@ -58,24 +68,24 @@ std::shared_ptr> CreateAuthorizedUserCredentialsFromJsonFilePath(std::string const& path) { std::ifstream is(path); std::string contents(std::istreambuf_iterator{is}, {}); - return CreateAuthorizedUserCredentialsFromJsonContents(contents); + return std::make_shared>(contents, path); } std::shared_ptr> CreateAuthorizedUserCredentialsFromJsonContents(std::string const& contents) { - return std::make_shared>(contents); + return std::make_shared>(contents, "memory"); } std::shared_ptr> CreateServiceAccountCredentialsFromJsonFilePath(std::string const& path) { std::ifstream is(path); std::string contents(std::istreambuf_iterator{is}, {}); - return CreateServiceAccountCredentialsFromJsonContents(contents); + return std::make_shared>(contents, path); } std::shared_ptr> CreateServiceAccountCredentialsFromJsonContents(std::string const& contents) { - return std::make_shared>(contents); + return std::make_shared>(contents, "memory"); } } // namespace oauth2 diff --git a/google/cloud/storage/oauth2/google_credentials_test.cc b/google/cloud/storage/oauth2/google_credentials_test.cc index bde23172f2003..4cd1774d58768 100644 --- a/google/cloud/storage/oauth2/google_credentials_test.cc +++ b/google/cloud/storage/oauth2/google_credentials_test.cc @@ -25,9 +25,10 @@ namespace storage { inline namespace STORAGE_CLIENT_NS { namespace oauth2 { namespace { -using ::google::cloud::internal::SetEnv; -using ::google::cloud::internal::UnsetEnv; -using ::google::cloud::testing_util::EnvironmentVariableRestore; +using google::cloud::internal::SetEnv; +using google::cloud::internal::UnsetEnv; +using google::cloud::testing_util::EnvironmentVariableRestore; +using ::testing::HasSubstr; char const VAR_NAME[] = "GOOGLE_APPLICATION_CREDENTIALS"; @@ -178,6 +179,70 @@ TEST_F(GoogleCredentialsTest, LoadValidAnonymousCredentials) { EXPECT_EQ(typeid(*ptr), typeid(AnonymousCredentials)); } +TEST_F(GoogleCredentialsTest, LoadUnknownTypeCredentials) { + char const filename[] = "unknown-type-credentials.json"; + std::ofstream os(filename); + std::string contents_str = R"""({ + "type": "unknown_type" +})"""; + os << contents_str; + os.close(); + SetEnv(VAR_NAME, filename); + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW(try { + auto credentials = GoogleDefaultCredentials(); + } catch(std::runtime_error const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Unsupported credential type")); + EXPECT_THAT(ex.what(), HasSubstr(filename)); + throw; + }, std::runtime_error); +#else + EXPECT_DEATH_IF_SUPPORTED( + GoogleDefaultCredentials(), "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +TEST_F(GoogleCredentialsTest, LoadInvalidCredentials) { + char const filename[] = "invalid-credentials.json"; + std::ofstream os(filename); + std::string contents_str = R"""( not-a-json-object-string )"""; + os << contents_str; + os.close(); + SetEnv(VAR_NAME, filename); + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW(try { + auto credentials = GoogleDefaultCredentials(); + } catch(std::exception const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Invalid contents in credentials file")); + EXPECT_THAT(ex.what(), HasSubstr(filename)); + throw; + }, std::runtime_error); +#else + EXPECT_DEATH_IF_SUPPORTED( + GoogleDefaultCredentials(), "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +TEST_F(GoogleCredentialsTest, MissingCredentials) { + char const filename[] = "missing-credentials.json"; + SetEnv(VAR_NAME, filename); + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW(try { + auto credentials = GoogleDefaultCredentials(); + } catch(std::runtime_error const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Cannot open credentials file")); + EXPECT_THAT(ex.what(), HasSubstr(filename)); + throw; + }, std::runtime_error); +#else + EXPECT_DEATH_IF_SUPPORTED( + GoogleDefaultCredentials(), "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + } // namespace } // namespace oauth2 } // namespace STORAGE_CLIENT_NS diff --git a/google/cloud/storage/oauth2/service_account_credentials.h b/google/cloud/storage/oauth2/service_account_credentials.h index 2bb699f141a41..f8804c7c3a805 100644 --- a/google/cloud/storage/oauth2/service_account_credentials.h +++ b/google/cloud/storage/oauth2/service_account_credentials.h @@ -57,14 +57,35 @@ template class ServiceAccountCredentials : public Credentials { public: - explicit ServiceAccountCredentials(std::string const& content) - : ServiceAccountCredentials(content, GoogleOAuthRefreshEndpoint()) {} + explicit ServiceAccountCredentials(std::string const& content, + std::string const& source) + : ServiceAccountCredentials(content, source, + GoogleOAuthRefreshEndpoint()) {} explicit ServiceAccountCredentials(std::string const& content, + std::string const& source, std::string default_token_uri) : expiration_time_(), clock_() { namespace nl = storage::internal::nl; - nl::json credentials = nl::json::parse(content); + nl::json credentials = nl::json::parse(content, nullptr, false); + if (credentials.is_discarded()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, " + "parsing failed on data loaded from " + + source); + } + char const PRIVATE_KEY_ID_KEY[] = "private_key_id"; + char const PRIVATE_KEY_KEY[] = "private_key"; + char const TOKEN_URI_KEY[] = "token_uri"; + char const CLIENT_EMAIL_KEY[] = "client_email"; + for (auto const& key : {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, TOKEN_URI_KEY, + CLIENT_EMAIL_KEY}) { + if (credentials.count(key) == 0U) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, the " + std::string(key) + + " field is missing on data loaded from " + source); + } + } // Below, we construct a JWT refresh request used to obtain an access token. // The structure of a JWT is defined in RFC 7519 (see // https://tools.ietf.org/html/rfc7519), and Google-specific JWT validation @@ -72,31 +93,36 @@ class ServiceAccountCredentials : public Credentials { // https://cloud.google.com/endpoints/docs/frameworks/java/troubleshoot-jwt nl::json assertion_header = { {"alg", "RS256"}, - {"kid", credentials["private_key_id"].get_ref()}, + {"kid", credentials.value(PRIVATE_KEY_ID_KEY, "")}, {"typ", "JWT"}}; std::string scope = GoogleOAuthScopeCloudPlatform(); // Some credential formats (e.g. gcloud's ADC file) don't contain a // "token_uri" attribute in the JSON object. In this case, we try using the // default value. - char const TOKEN_URI_KEY[] = "token_uri"; std::string token_uri = credentials.value(TOKEN_URI_KEY, default_token_uri); - long int cur_time = static_cast( - std::chrono::system_clock::to_time_t(clock_.now())); - long int expiration_time = - cur_time + GoogleOAuthAccessTokenLifetime().count(); + + // As much as possible do the time arithmetic using the std::chrono types, + // convert to longs only when we are dealing with timestamps since the + // epoch. + auto now = clock_.now(); + auto expiration = now + GoogleOAuthAccessTokenLifetime(); + auto now_from_epoch = + static_cast(std::chrono::system_clock::to_time_t(now)); + auto expiration_from_epoch = + static_cast(std::chrono::system_clock::to_time_t(expiration)); nl::json assertion_payload = { - {"iss", credentials["client_email"].get_ref()}, + {"iss", credentials.value(CLIENT_EMAIL_KEY, "")}, {"scope", scope}, {"aud", token_uri}, - {"iat", cur_time}, + {"iat", now_from_epoch}, // Resulting access token should be expire after one hour. - {"exp", expiration_time}}; + {"exp", expiration_from_epoch}}; HttpRequestBuilderType request_builder( std::move(token_uri), storage::internal::GetDefaultCurlHandleFactory()); std::string svc_acct_private_key_pem = - credentials["private_key"].get_ref(); + credentials.value(PRIVATE_KEY_KEY, ""); // This is the value of grant_type for JSON-formatted service account // keyfiles downloaded from Cloud Console. std::string payload("grant_type="); @@ -144,18 +170,23 @@ class ServiceAccountCredentials : public Credentials { // TODO(#516) - use retry policies to refresh the credentials. auto response = request_.MakeRequest(payload_); - if (200 != response.status_code) { + if (response.status_code >= 300) { return false; } - nl::json access_token = nl::json::parse(response.payload); + nl::json access_token = nl::json::parse(response.payload, nullptr, false); + if (access_token.is_null() or access_token.count("token_type") == 0U or + access_token.count("access_token") == 0U or + access_token.count("expires_in") == 0U) { + return false; + } // Response should have the attributes "access_token", "expires_in", and // "token_type". std::string header = - "Authorization: " + - access_token["token_type"].get_ref() + " " + - access_token["access_token"].get_ref(); - auto expires_in = std::chrono::seconds(access_token["expires_in"]); + "Authorization: " + access_token.value("token_type", "") + " " + + access_token.value("access_token", ""); + auto expires_in = + std::chrono::seconds(access_token.value("expires_in", int(0))); auto new_expiration = std::chrono::system_clock::now() + expires_in - GoogleOAuthAccessTokenExpirationSlack(); // Do not update any state until all potential exceptions are raised. diff --git a/google/cloud/storage/oauth2/service_account_credentials_test.cc b/google/cloud/storage/oauth2/service_account_credentials_test.cc index 0e8c354ae3dd4..0f12d2bef19dc 100644 --- a/google/cloud/storage/oauth2/service_account_credentials_test.cc +++ b/google/cloud/storage/oauth2/service_account_credentials_test.cc @@ -126,7 +126,7 @@ TEST_F(ServiceAccountCredentialsTest, })); ServiceAccountCredentials credentials( - kJsonKeyfileContents); + kJsonKeyfileContents, "test"); // Calls Refresh to obtain the access token for our authorization header. EXPECT_EQ("Authorization: Type access-token-value", @@ -176,7 +176,7 @@ TEST_F(ServiceAccountCredentialsTest, })); ServiceAccountCredentials credentials( - kJsonKeyfileContents); + kJsonKeyfileContents, "test"); // Calls Refresh to obtain the access token for our authorization header. EXPECT_EQ("Authorization: Type access-token-r1", credentials.AuthorizationHeader()); @@ -188,6 +188,61 @@ TEST_F(ServiceAccountCredentialsTest, credentials.AuthorizationHeader()); } +/// @test Verify that invalid contents result in a readable error. +TEST_F(ServiceAccountCredentialsTest, InvalidContents) { + std::string config = R"""( not-a-valid-json-string )"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ServiceAccountCredentials credentials( + config, "test-as-a-source"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Invalid ServiceAccountCredentials")); + EXPECT_THAT(ex.what(), HasSubstr("test-as-a-source")); + throw; + }, + std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED(ServiceAccountCredentials( + config, "test-as-a-source"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +/// @test Verify that missing fields result in a readable error. +TEST_F(ServiceAccountCredentialsTest, MissingContents) { + // Note that the private_key field is missing here. + std::string contents = R"""({ + "type": "service_account", + "project_id": "foo-project", + "private_key_id": "a1a111aa1111a11a11a11aa111a111a1a1111111", + "client_email": "foo-email@foo-project.iam.gserviceaccount.com", + "client_id": "100000000000000000001", + "auth_uri": "https://accounts.google.com/o/oauth2/auth", + "token_uri": "https://oauth2.googleapis.com/token", + "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs", + "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/foo-email%40foo-project.iam.gserviceaccount.com" +})"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ServiceAccountCredentials credentials( + contents, "test-as-a-source"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("the private_key field is missing")); + EXPECT_THAT(ex.what(), HasSubstr("test-as-a-source")); + throw; + }, + std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED(ServiceAccountCredentials( + contents, "test-as-a-source"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + } // namespace } // namespace oauth2 } // namespace STORAGE_CLIENT_NS From 93007532c65cf9b052bebd959aeec730c9255823 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 17:54:31 -0400 Subject: [PATCH 2/7] WIP - Address review comments. --- google/cloud/storage/CMakeLists.txt | 1 + .../oauth2/service_account_credentials.cc | 66 +++++++++++++++++++ .../oauth2/service_account_credentials.h | 57 +++++++--------- .../service_account_credentials_test.cc | 10 +++ google/cloud/storage/storage_client.bzl | 1 + 5 files changed, 101 insertions(+), 34 deletions(-) create mode 100644 google/cloud/storage/oauth2/service_account_credentials.cc diff --git a/google/cloud/storage/CMakeLists.txt b/google/cloud/storage/CMakeLists.txt index 9537888a90ff4..739d0d4e121bd 100644 --- a/google/cloud/storage/CMakeLists.txt +++ b/google/cloud/storage/CMakeLists.txt @@ -181,6 +181,7 @@ add_library(storage_client oauth2/google_credentials.h oauth2/google_credentials.cc oauth2/service_account_credentials.h + oauth2/service_account_credentials.cc object_access_control.h object_access_control.cc object_metadata.h diff --git a/google/cloud/storage/oauth2/service_account_credentials.cc b/google/cloud/storage/oauth2/service_account_credentials.cc new file mode 100644 index 0000000000000..cd3784eea7e5c --- /dev/null +++ b/google/cloud/storage/oauth2/service_account_credentials.cc @@ -0,0 +1,66 @@ +// Copyright 2018 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/storage/oauth2/service_account_credentials.h" + +namespace google { +namespace cloud { +namespace storage { +inline namespace STORAGE_CLIENT_NS { +namespace oauth2 { + +ServiceAccountCredentialsInfo ParseServiceAccountCredentials( + std::string const& content, std::string const& source, + std::string const& default_token_uri) { + namespace nl = storage::internal::nl; + nl::json credentials = nl::json::parse(content, nullptr, false); + if (credentials.is_discarded()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, " + "parsing failed on data loaded from " + + source); + } + char const PRIVATE_KEY_ID_KEY[] = "private_key_id"; + char const PRIVATE_KEY_KEY[] = "private_key"; + char const TOKEN_URI_KEY[] = "token_uri"; + char const CLIENT_EMAIL_KEY[] = "client_email"; + for (auto const &key : {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, TOKEN_URI_KEY, + CLIENT_EMAIL_KEY}) { + if (credentials.count(key) == 0U) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, the " + std::string(key) + + " field is missing on data loaded from " + source); + } + if (credentials.value(key, "").empty()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, the " + std::string(key) + + " field is empty on data loaded from " + source); + } + } + return ServiceAccountCredentialsInfo{ + credentials.value(PRIVATE_KEY_ID_KEY, ""), + credentials.value(PRIVATE_KEY_KEY, ""), + // Some credential formats (e.g. gcloud's ADC file) don't contain a + // "token_uri" attribute in the JSON object. In this case, we try using the + // default value. + credentials.value(TOKEN_URI_KEY, default_token_uri), + credentials.value(CLIENT_EMAIL_KEY, ""), + }; +} + +} // namespace oauth2 +} // namespace STORAGE_CLIENT_NS +} // namespace storage +} // namespace cloud +} // namespace google diff --git a/google/cloud/storage/oauth2/service_account_credentials.h b/google/cloud/storage/oauth2/service_account_credentials.h index f8804c7c3a805..399faa4268a0d 100644 --- a/google/cloud/storage/oauth2/service_account_credentials.h +++ b/google/cloud/storage/oauth2/service_account_credentials.h @@ -32,6 +32,19 @@ namespace cloud { namespace storage { inline namespace STORAGE_CLIENT_NS { namespace oauth2 { +/// A plain object to hold the result of parsing a service account credentials. +struct ServiceAccountCredentialsInfo { + std::string private_key_id; + std::string private_key; + std::string token_uri; + std::string client_email; +}; + +/// Parse a JSON object as a ServiceAccountCredentials. +ServiceAccountCredentialsInfo ParseServiceAccountCredentials( + std::string const& content, std::string const& source, + std::string const& default_token_uri); + /** * A C++ wrapper for Google's Service Account Credentials. * @@ -64,43 +77,20 @@ class ServiceAccountCredentials : public Credentials { explicit ServiceAccountCredentials(std::string const& content, std::string const& source, - std::string default_token_uri) + std::string const& default_token_uri) : expiration_time_(), clock_() { namespace nl = storage::internal::nl; - nl::json credentials = nl::json::parse(content, nullptr, false); - if (credentials.is_discarded()) { - google::cloud::internal::RaiseInvalidArgument( - "Invalid ServiceAccountCredentials, " - "parsing failed on data loaded from " + - source); - } - char const PRIVATE_KEY_ID_KEY[] = "private_key_id"; - char const PRIVATE_KEY_KEY[] = "private_key"; - char const TOKEN_URI_KEY[] = "token_uri"; - char const CLIENT_EMAIL_KEY[] = "client_email"; - for (auto const& key : {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, TOKEN_URI_KEY, - CLIENT_EMAIL_KEY}) { - if (credentials.count(key) == 0U) { - google::cloud::internal::RaiseInvalidArgument( - "Invalid ServiceAccountCredentials, the " + std::string(key) + - " field is missing on data loaded from " + source); - } - } + auto info = + ParseServiceAccountCredentials(content, source, default_token_uri); // Below, we construct a JWT refresh request used to obtain an access token. // The structure of a JWT is defined in RFC 7519 (see // https://tools.ietf.org/html/rfc7519), and Google-specific JWT validation // logic is further described at: // https://cloud.google.com/endpoints/docs/frameworks/java/troubleshoot-jwt nl::json assertion_header = { - {"alg", "RS256"}, - {"kid", credentials.value(PRIVATE_KEY_ID_KEY, "")}, - {"typ", "JWT"}}; + {"alg", "RS256"}, {"kid", info.private_key_id}, {"typ", "JWT"}}; std::string scope = GoogleOAuthScopeCloudPlatform(); - // Some credential formats (e.g. gcloud's ADC file) don't contain a - // "token_uri" attribute in the JSON object. In this case, we try using the - // default value. - std::string token_uri = credentials.value(TOKEN_URI_KEY, default_token_uri); // As much as possible do the time arithmetic using the std::chrono types, // convert to longs only when we are dealing with timestamps since the @@ -112,17 +102,16 @@ class ServiceAccountCredentials : public Credentials { auto expiration_from_epoch = static_cast(std::chrono::system_clock::to_time_t(expiration)); nl::json assertion_payload = { - {"iss", credentials.value(CLIENT_EMAIL_KEY, "")}, + {"iss", info.client_email}, {"scope", scope}, - {"aud", token_uri}, + {"aud", info.token_uri}, {"iat", now_from_epoch}, // Resulting access token should be expire after one hour. {"exp", expiration_from_epoch}}; HttpRequestBuilderType request_builder( - std::move(token_uri), storage::internal::GetDefaultCurlHandleFactory()); - std::string svc_acct_private_key_pem = - credentials.value(PRIVATE_KEY_KEY, ""); + std::move(info.token_uri), + storage::internal::GetDefaultCurlHandleFactory()); // This is the value of grant_type for JSON-formatted service account // keyfiles downloaded from Cloud Console. std::string payload("grant_type="); @@ -131,8 +120,8 @@ class ServiceAccountCredentials : public Credentials { .MakeEscapedString("urn:ietf:params:oauth:grant-type:jwt-bearer") .get(); payload += "&assertion="; - payload += MakeJWTAssertion(assertion_header, assertion_payload, - svc_acct_private_key_pem); + payload += + MakeJWTAssertion(assertion_header, assertion_payload, info.private_key); payload_ = std::move(payload); request_builder.AddHeader( diff --git a/google/cloud/storage/oauth2/service_account_credentials_test.cc b/google/cloud/storage/oauth2/service_account_credentials_test.cc index 0f12d2bef19dc..f684a197830de 100644 --- a/google/cloud/storage/oauth2/service_account_credentials_test.cc +++ b/google/cloud/storage/oauth2/service_account_credentials_test.cc @@ -188,6 +188,16 @@ TEST_F(ServiceAccountCredentialsTest, credentials.AuthorizationHeader()); } +/// @test Verify that nl::json::parse() failures are reported as is_discarded. +TEST_F(ServiceAccountCredentialsTest, JsonParsingFailure) { + std::string config = R"""( not-a-valid-json-string )"""; + // The documentation for nl::json::parse() is a bit ambiguous, so wrote a + // little test to verify it works as I expected. + internal::nl::json parsed = internal::nl::json::parse(config, nullptr, false); + EXPECT_TRUE(parsed.is_discarded()); + EXPECT_FALSE(parsed.is_null()); +} + /// @test Verify that invalid contents result in a readable error. TEST_F(ServiceAccountCredentialsTest, InvalidContents) { std::string config = R"""( not-a-valid-json-string )"""; diff --git a/google/cloud/storage/storage_client.bzl b/google/cloud/storage/storage_client.bzl index 8f9735da220bf..4881ec36c54b2 100644 --- a/google/cloud/storage/storage_client.bzl +++ b/google/cloud/storage/storage_client.bzl @@ -103,6 +103,7 @@ storage_client_SRCS = [ "oauth2/anonymous_credentials.cc", "oauth2/google_application_default_credentials_file.cc", "oauth2/google_credentials.cc", + "oauth2/service_account_credentials.cc", "object_access_control.cc", "object_metadata.cc", "object_rewriter.cc", From a82e4ac2378b4789241b85ea8b8cf8d454a445d8 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 21:58:44 -0400 Subject: [PATCH 3/7] Address comments about ServiceAccountCredentials. --- .../oauth2/service_account_credentials.cc | 25 ++-- .../service_account_credentials_test.cc | 121 ++++++++++++++++++ 2 files changed, 137 insertions(+), 9 deletions(-) diff --git a/google/cloud/storage/oauth2/service_account_credentials.cc b/google/cloud/storage/oauth2/service_account_credentials.cc index cd3784eea7e5c..a62416c554aab 100644 --- a/google/cloud/storage/oauth2/service_account_credentials.cc +++ b/google/cloud/storage/oauth2/service_account_credentials.cc @@ -27,33 +27,40 @@ ServiceAccountCredentialsInfo ParseServiceAccountCredentials( nl::json credentials = nl::json::parse(content, nullptr, false); if (credentials.is_discarded()) { google::cloud::internal::RaiseInvalidArgument( - "Invalid ServiceAccountCredentials, " - "parsing failed on data loaded from " + - source); + "Invalid ServiceAccountCredentials," + " parsing failed on data loaded from " + + source); } char const PRIVATE_KEY_ID_KEY[] = "private_key_id"; char const PRIVATE_KEY_KEY[] = "private_key"; char const TOKEN_URI_KEY[] = "token_uri"; char const CLIENT_EMAIL_KEY[] = "client_email"; - for (auto const &key : {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, TOKEN_URI_KEY, - CLIENT_EMAIL_KEY}) { + for (auto const& key : + {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, CLIENT_EMAIL_KEY}) { if (credentials.count(key) == 0U) { google::cloud::internal::RaiseInvalidArgument( "Invalid ServiceAccountCredentials, the " + std::string(key) + - " field is missing on data loaded from " + source); + " field is missing on data loaded from " + source); } if (credentials.value(key, "").empty()) { google::cloud::internal::RaiseInvalidArgument( "Invalid ServiceAccountCredentials, the " + std::string(key) + - " field is empty on data loaded from " + source); + " field is empty on data loaded from " + source); } } + // The token_uri field may be missing, but may not be empty: + if (credentials.count(TOKEN_URI_KEY) != 0U and + credentials.value(TOKEN_URI_KEY, "").empty()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid ServiceAccountCredentials, the " + std::string(TOKEN_URI_KEY) + + " field is empty on data loaded from " + source); + } return ServiceAccountCredentialsInfo{ credentials.value(PRIVATE_KEY_ID_KEY, ""), credentials.value(PRIVATE_KEY_KEY, ""), // Some credential formats (e.g. gcloud's ADC file) don't contain a - // "token_uri" attribute in the JSON object. In this case, we try using the - // default value. + // "token_uri" attribute in the JSON object. In this case, we try using + // the default value. credentials.value(TOKEN_URI_KEY, default_token_uri), credentials.value(CLIENT_EMAIL_KEY, ""), }; diff --git a/google/cloud/storage/oauth2/service_account_credentials_test.cc b/google/cloud/storage/oauth2/service_account_credentials_test.cc index f684a197830de..72d47f7fdab91 100644 --- a/google/cloud/storage/oauth2/service_account_credentials_test.cc +++ b/google/cloud/storage/oauth2/service_account_credentials_test.cc @@ -253,6 +253,127 @@ TEST_F(ServiceAccountCredentialsTest, MissingContents) { #endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS } +/// @test Verify that parsing a service account JSON string works. +TEST_F(ServiceAccountCredentialsTest, ParseSimple) { + std::string contents = R"""({ + "type": "service_account", + "private_key_id": "not-a-key-id-just-for-testing", + "private_key": "not-a-valid-key-just-for-testing", + "client_email": "test-only@test-group.example.com", + "token_uri": "https://oauth2.googleapis.com/token" +})"""; + + auto actual = ParseServiceAccountCredentials(contents, "test-data", "unused"); + EXPECT_EQ("not-a-key-id-just-for-testing", actual.private_key_id); + EXPECT_EQ("not-a-valid-key-just-for-testing", actual.private_key); + EXPECT_EQ("test-only@test-group.example.com", actual.client_email); + EXPECT_EQ("https://oauth2.googleapis.com/token", actual.token_uri); +} + +/// @test Verify that parsing a service account JSON string works. +TEST_F(ServiceAccountCredentialsTest, ParseDefaultTokenUri) { + std::string contents = R"""({ + "type": "service_account", + "private_key_id": "not-a-key-id-just-for-testing", + "private_key": "not-a-valid-key-just-for-testing", + "client_email": "test-only@test-group.example.com" +})"""; + + auto actual = ParseServiceAccountCredentials( + contents, "test-data", "https://oauth2.googleapis.com/token"); + EXPECT_EQ("not-a-key-id-just-for-testing", actual.private_key_id); + EXPECT_EQ("not-a-valid-key-just-for-testing", actual.private_key); + EXPECT_EQ("test-only@test-group.example.com", actual.client_email); + EXPECT_EQ("https://oauth2.googleapis.com/token", actual.token_uri); +} + +/// @test Verify that invalid contents result in a readable error. +TEST_F(ServiceAccountCredentialsTest, ParseInvalid) { + std::string contents = R"""( not-a-valid-json-string )"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseServiceAccountCredentials(contents, "test-data", "unused"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Invalid ServiceAccountCredentials")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED( + ParseServiceAccountCredentials(contents, "test-data", "unused"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +/// @test Parsing a service account JSON string should detect empty fields. +TEST_F(ServiceAccountCredentialsTest, ParseEmptyField) { + std::string contents = R"""({ + "type": "service_account", + "private_key_id": "not-a-key-id-just-for-testing", + "private_key": "not-a-valid-key-just-for-testing", + "client_email": "test-only@test-group.example.com", + "token_uri": "https://oauth2.googleapis.com/token" +})"""; + + for (auto const& field : + {"private_key_id", "private_key", "client_email", "token_uri"}) { + internal::nl::json json = internal::nl::json::parse(contents); + json[field] = ""; +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseServiceAccountCredentials(json.dump(), "test-data", ""); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr(field)); + EXPECT_THAT(ex.what(), HasSubstr(" field is empty")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument) << "field=" << field; +#else + EXPECT_DEATH_IF_SUPPORTED( + ParseServiceAccountCredentials(json.dump(), "test-data", "unused"), + "exceptions are disabled") << "field=" << field; +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + } +} + +/// @test Parsing a service account JSON string should detect missing fields. +TEST_F(ServiceAccountCredentialsTest, ParseMissingField) { + std::string contents = R"""({ + "type": "service_account", + "private_key_id": "not-a-key-id-just-for-testing", + "private_key": "not-a-valid-key-just-for-testing", + "client_email": "test-only@test-group.example.com", + "token_uri": "https://oauth2.googleapis.com/token" +})"""; + + for (auto const& field : + {"private_key_id", "private_key", "client_email"}) { + internal::nl::json json = internal::nl::json::parse(contents); + json.erase(field); +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseServiceAccountCredentials(json.dump(), "test-data", ""); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr(field)); + EXPECT_THAT(ex.what(), HasSubstr(" field is missing")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument) << "field=" << field; +#else + EXPECT_DEATH_IF_SUPPORTED( + ParseServiceAccountCredentials(json.dump(), "test-data", "unused"), + "exceptions are disabled") << "field=" << field; +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + } +} + } // namespace } // namespace oauth2 } // namespace STORAGE_CLIENT_NS From 1c5001239f0e5eac60b82b0797a309ced9c39006 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 22:24:33 -0400 Subject: [PATCH 4/7] Address review comments for AuthorizedUserCredentials. --- google/cloud/storage/CMakeLists.txt | 1 + .../oauth2/authorized_user_credentials.cc | 57 +++++++++ .../oauth2/authorized_user_credentials.h | 36 +++--- .../authorized_user_credentials_test.cc | 110 +++++++++++++++++- google/cloud/storage/storage_client.bzl | 1 + 5 files changed, 179 insertions(+), 26 deletions(-) create mode 100644 google/cloud/storage/oauth2/authorized_user_credentials.cc diff --git a/google/cloud/storage/CMakeLists.txt b/google/cloud/storage/CMakeLists.txt index 739d0d4e121bd..368aa53688c7c 100644 --- a/google/cloud/storage/CMakeLists.txt +++ b/google/cloud/storage/CMakeLists.txt @@ -174,6 +174,7 @@ add_library(storage_client oauth2/anonymous_credentials.h oauth2/anonymous_credentials.cc oauth2/authorized_user_credentials.h + oauth2/authorized_user_credentials.cc oauth2/credential_constants.h oauth2/credentials.h oauth2/google_application_default_credentials_file.h diff --git a/google/cloud/storage/oauth2/authorized_user_credentials.cc b/google/cloud/storage/oauth2/authorized_user_credentials.cc new file mode 100644 index 0000000000000..aa87c7bbe03de --- /dev/null +++ b/google/cloud/storage/oauth2/authorized_user_credentials.cc @@ -0,0 +1,57 @@ +// Copyright 2018 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/storage/oauth2/authorized_user_credentials.h" + +namespace google { +namespace cloud { +namespace storage { +inline namespace STORAGE_CLIENT_NS { +namespace oauth2 { +AuthorizedUserCredentialsInfo ParseAuthorizedUserCredentials( + std::string const& content, std::string const& source) { + auto credentials = + storage::internal::nl::json::parse(content, nullptr, false); + if (credentials.is_discarded()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid AuthorizedUserCredentials, parsing failed on data from " + + source); + } + + char const CLIENT_ID_KEY[] = "client_id"; + char const CLIENT_SECRET_KEY[] = "client_secret"; + char const REFRESH_TOKEN_KEY[] = "refresh_token"; + for (auto const& key : + {CLIENT_ID_KEY, CLIENT_SECRET_KEY, REFRESH_TOKEN_KEY}) { + if (credentials.count(key) == 0U) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid AuthorizedUserCredentials, the " + std::string(key) + + " field is missing on data loaded from " + source); + } + if (credentials.value(key, "").empty()) { + google::cloud::internal::RaiseInvalidArgument( + "Invalid AuthorizedUserCredentials, the " + std::string(key) + + " field is empty on data loaded from " + source); + } + } + return AuthorizedUserCredentialsInfo{ + credentials.value(CLIENT_ID_KEY, ""), + credentials.value(CLIENT_SECRET_KEY, ""), + credentials.value(REFRESH_TOKEN_KEY, "")}; +} +} // namespace oauth2 +} // namespace STORAGE_CLIENT_NS +} // namespace storage +} // namespace cloud +} // namespace google diff --git a/google/cloud/storage/oauth2/authorized_user_credentials.h b/google/cloud/storage/oauth2/authorized_user_credentials.h index f05cc7f684356..2706f59e1a7bf 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials.h +++ b/google/cloud/storage/oauth2/authorized_user_credentials.h @@ -30,6 +30,17 @@ namespace cloud { namespace storage { inline namespace STORAGE_CLIENT_NS { namespace oauth2 { +/// A plain object to hold the result of parsing authorized user credentials. +struct AuthorizedUserCredentialsInfo { + std::string client_id; + std::string client_secret; + std::string refresh_token; +}; + +/// Parse a JSON object string as an AuthorizedUserCredentials. +AuthorizedUserCredentialsInfo ParseAuthorizedUserCredentials( + std::string const& content, std::string const& source); + /** * A C++ wrapper for Google's Authorized User Credentials. * @@ -65,36 +76,19 @@ class AuthorizedUserCredentials : public Credentials { HttpRequestBuilderType request_builder( std::move(oauth_server), storage::internal::GetDefaultCurlHandleFactory()); - auto credentials = - storage::internal::nl::json::parse(content, nullptr, false); - if (credentials.is_null()) { - google::cloud::internal::RaiseInvalidArgument( - "Invalid AuthorizedUserCredentials, parsing failed on data from " + - source); - } - char const CLIENT_ID_KEY[] = "client_id"; - char const CLIENT_SECRET_KEY[] = "client_secret"; - char const REFRESH_TOKEN_KEY[] = "refresh_token"; - for (auto const& key : - {CLIENT_ID_KEY, CLIENT_SECRET_KEY, REFRESH_TOKEN_KEY}) { - if (credentials.count(key) == 0U) { - google::cloud::internal::RaiseInvalidArgument( - "Invalid AuthorizedUserCredentials, the " + std::string(key) + - " field is missing on data loaded from " + source); - } - } + auto info = ParseAuthorizedUserCredentials(content, source); std::string payload("grant_type=refresh_token"); payload += "&client_id="; payload += - request_builder.MakeEscapedString(credentials.value(CLIENT_ID_KEY, "")) + request_builder.MakeEscapedString(info.client_id) .get(); payload += "&client_secret="; payload += request_builder - .MakeEscapedString(credentials.value(CLIENT_SECRET_KEY, "")) + .MakeEscapedString(info.client_secret) .get(); payload += "&refresh_token="; payload += request_builder - .MakeEscapedString(credentials.value(REFRESH_TOKEN_KEY, "")) + .MakeEscapedString(info.refresh_token) .get(); payload_ = std::move(payload); request_ = request_builder.BuildRequest(); diff --git a/google/cloud/storage/oauth2/authorized_user_credentials_test.cc b/google/cloud/storage/oauth2/authorized_user_credentials_test.cc index 1bae7ec00bdad..79816322ab226 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials_test.cc +++ b/google/cloud/storage/oauth2/authorized_user_credentials_test.cc @@ -165,16 +165,15 @@ TEST_F(AuthorizedUserCredentialsTest, InvalidContents) { }, std::invalid_argument); #else - EXPECT_DEATH_IF_SUPPORTED( - AuthorizedUserCredentials(config, "test-as-a-source"), - "exceptions are disabled"); + EXPECT_DEATH_IF_SUPPORTED(AuthorizedUserCredentials( + config, "test-as-a-source"), + "exceptions are disabled"); #endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS } /// @test Verify that missing fields result in a readable error. TEST_F(AuthorizedUserCredentialsTest, MissingContents) { std::string config = R"""({ - // "client_id": "a-client-id.example.com", "client_secret": "a-123456ABCDEF", "refresh_token": "1/THETOKEN", "type": "magic_type" @@ -194,13 +193,114 @@ TEST_F(AuthorizedUserCredentialsTest, MissingContents) { throw; }, std::invalid_argument); +#else + EXPECT_DEATH_IF_SUPPORTED(AuthorizedUserCredentials( + config, "test-as-a-source"), + "exceptions are disabled"); +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS +} + +/// @test Verify that parsing works in the easy case. +TEST_F(AuthorizedUserCredentialsTest, ParseSimple) { + std::string contents = R"""({ + "client_id": "a-client-id.example.com", + "client_secret": "a-123456ABCDEF", + "refresh_token": "1/THETOKEN", + "type": "magic_type" +})"""; + + auto actual = ParseAuthorizedUserCredentials(contents, "test-data"); + EXPECT_EQ("a-client-id.example.com", actual.client_id); + EXPECT_EQ("a-123456ABCDEF", actual.client_secret); + EXPECT_EQ("1/THETOKEN", actual.refresh_token); +} +/// @test Verify that invalid contents result in a readable error. +TEST_F(AuthorizedUserCredentialsTest, ParseInvalid) { + std::string contents = R"""( not-a-valid-json-string )"""; + +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseAuthorizedUserCredentials(contents, "test-data"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr("Invalid AuthorizedUserCredentials")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument); #else EXPECT_DEATH_IF_SUPPORTED( - AuthorizedUserCredentials(config, "test-as-a-source"), + ParseAuthorizedUserCredentials(contents, "test-data"), "exceptions are disabled"); #endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS } +/// @test Parsing a service account JSON string should detect empty fields. +TEST_F(AuthorizedUserCredentialsTest, ParseEmptyField) { + std::string contents = R"""({ + "client_id": "a-client-id.example.com", + "client_secret": "a-123456ABCDEF", + "refresh_token": "1/THETOKEN", + "type": "magic_type" +})"""; + + for (auto const& field : {"client_id", "client_secret", "refresh_token"}) { + internal::nl::json json = internal::nl::json::parse(contents); + json[field] = ""; +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseAuthorizedUserCredentials(json.dump(), "test-data"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr(field)); + EXPECT_THAT(ex.what(), HasSubstr(" field is empty")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument) + << "field=" << field; +#else + EXPECT_DEATH_IF_SUPPORTED( + ParseAuthorizedUserCredentials(json.dump(), "test-data"), + "exceptions are disabled") + << "field=" << field; +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + } +} + +/// @test Parsing a service account JSON string should detect missing fields. +TEST_F(AuthorizedUserCredentialsTest, ParseMissingField) { + std::string contents = R"""({ + "client_id": "a-client-id.example.com", + "client_secret": "a-123456ABCDEF", + "refresh_token": "1/THETOKEN", + "type": "magic_type" +})"""; + + for (auto const& field : {"client_id", "client_secret", "refresh_token"}) { + internal::nl::json json = internal::nl::json::parse(contents); + json.erase(field); +#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + EXPECT_THROW( + try { + ParseAuthorizedUserCredentials(json.dump(), "test-data"); + } catch (std::invalid_argument const& ex) { + EXPECT_THAT(ex.what(), HasSubstr(field)); + EXPECT_THAT(ex.what(), HasSubstr(" field is missing")); + EXPECT_THAT(ex.what(), HasSubstr("test-data")); + throw; + }, + std::invalid_argument) + << "field=" << field; +#else + EXPECT_DEATH_IF_SUPPORTED( + ParseAuthorizedUserCredentials(json.dump(), "test-data"), + "exceptions are disabled") + << "field=" << field; +#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS + } +} + } // namespace } // namespace oauth2 } // namespace STORAGE_CLIENT_NS diff --git a/google/cloud/storage/storage_client.bzl b/google/cloud/storage/storage_client.bzl index 4881ec36c54b2..43713cd9d48fd 100644 --- a/google/cloud/storage/storage_client.bzl +++ b/google/cloud/storage/storage_client.bzl @@ -101,6 +101,7 @@ storage_client_SRCS = [ "list_objects_reader.cc", "notification_metadata.cc", "oauth2/anonymous_credentials.cc", + "oauth2/authorized_user_credentials.cc", "oauth2/google_application_default_credentials_file.cc", "oauth2/google_credentials.cc", "oauth2/service_account_credentials.cc", From 71f2b977409d3de740b0489f5997cef15e7df8a3 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 22:26:55 -0400 Subject: [PATCH 5/7] Use is_abandoned to detect parse errors. --- google/cloud/storage/oauth2/service_account_credentials.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/storage/oauth2/service_account_credentials.h b/google/cloud/storage/oauth2/service_account_credentials.h index 399faa4268a0d..c7bd715373d6b 100644 --- a/google/cloud/storage/oauth2/service_account_credentials.h +++ b/google/cloud/storage/oauth2/service_account_credentials.h @@ -164,7 +164,7 @@ class ServiceAccountCredentials : public Credentials { } nl::json access_token = nl::json::parse(response.payload, nullptr, false); - if (access_token.is_null() or access_token.count("token_type") == 0U or + if (access_token.is_discarded() or access_token.count("token_type") == 0U or access_token.count("access_token") == 0U or access_token.count("expires_in") == 0U) { return false; From 5723dfa0cb57065cd3922c7f2dac1d4e7cf3cd77 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 22:27:28 -0400 Subject: [PATCH 6/7] Apply clang-format(1). --- .../storage/oauth2/authorized_user_credentials.h | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/google/cloud/storage/oauth2/authorized_user_credentials.h b/google/cloud/storage/oauth2/authorized_user_credentials.h index 2706f59e1a7bf..1c696f2f508e6 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials.h +++ b/google/cloud/storage/oauth2/authorized_user_credentials.h @@ -79,17 +79,11 @@ class AuthorizedUserCredentials : public Credentials { auto info = ParseAuthorizedUserCredentials(content, source); std::string payload("grant_type=refresh_token"); payload += "&client_id="; - payload += - request_builder.MakeEscapedString(info.client_id) - .get(); + payload += request_builder.MakeEscapedString(info.client_id).get(); payload += "&client_secret="; - payload += request_builder - .MakeEscapedString(info.client_secret) - .get(); + payload += request_builder.MakeEscapedString(info.client_secret).get(); payload += "&refresh_token="; - payload += request_builder - .MakeEscapedString(info.refresh_token) - .get(); + payload += request_builder.MakeEscapedString(info.refresh_token).get(); payload_ = std::move(payload); request_ = request_builder.BuildRequest(); } From 7f9255ea4f041dbd93d8092e5d82af7bc830634a Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Wed, 3 Oct 2018 22:31:49 -0400 Subject: [PATCH 7/7] Make clang-tidy(1) happy. --- .../oauth2/authorized_user_credentials.cc | 14 +++++------ .../oauth2/service_account_credentials.cc | 24 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/google/cloud/storage/oauth2/authorized_user_credentials.cc b/google/cloud/storage/oauth2/authorized_user_credentials.cc index aa87c7bbe03de..4eebb1d054019 100644 --- a/google/cloud/storage/oauth2/authorized_user_credentials.cc +++ b/google/cloud/storage/oauth2/authorized_user_credentials.cc @@ -29,11 +29,11 @@ AuthorizedUserCredentialsInfo ParseAuthorizedUserCredentials( source); } - char const CLIENT_ID_KEY[] = "client_id"; - char const CLIENT_SECRET_KEY[] = "client_secret"; - char const REFRESH_TOKEN_KEY[] = "refresh_token"; + char const client_id_key[] = "client_id"; + char const client_secret_key[] = "client_secret"; + char const refresh_token_key[] = "refresh_token"; for (auto const& key : - {CLIENT_ID_KEY, CLIENT_SECRET_KEY, REFRESH_TOKEN_KEY}) { + {client_id_key, client_secret_key, refresh_token_key}) { if (credentials.count(key) == 0U) { google::cloud::internal::RaiseInvalidArgument( "Invalid AuthorizedUserCredentials, the " + std::string(key) + @@ -46,9 +46,9 @@ AuthorizedUserCredentialsInfo ParseAuthorizedUserCredentials( } } return AuthorizedUserCredentialsInfo{ - credentials.value(CLIENT_ID_KEY, ""), - credentials.value(CLIENT_SECRET_KEY, ""), - credentials.value(REFRESH_TOKEN_KEY, "")}; + credentials.value(client_id_key, ""), + credentials.value(client_secret_key, ""), + credentials.value(refresh_token_key, "")}; } } // namespace oauth2 } // namespace STORAGE_CLIENT_NS diff --git a/google/cloud/storage/oauth2/service_account_credentials.cc b/google/cloud/storage/oauth2/service_account_credentials.cc index a62416c554aab..411d51088a47f 100644 --- a/google/cloud/storage/oauth2/service_account_credentials.cc +++ b/google/cloud/storage/oauth2/service_account_credentials.cc @@ -31,12 +31,12 @@ ServiceAccountCredentialsInfo ParseServiceAccountCredentials( " parsing failed on data loaded from " + source); } - char const PRIVATE_KEY_ID_KEY[] = "private_key_id"; - char const PRIVATE_KEY_KEY[] = "private_key"; - char const TOKEN_URI_KEY[] = "token_uri"; - char const CLIENT_EMAIL_KEY[] = "client_email"; + char const private_key_id_key[] = "private_key_id"; + char const private_key_key[] = "private_key"; + char const token_uri_key[] = "token_uri"; + char const client_email_key[] = "client_email"; for (auto const& key : - {PRIVATE_KEY_ID_KEY, PRIVATE_KEY_KEY, CLIENT_EMAIL_KEY}) { + {private_key_id_key, private_key_key, client_email_key}) { if (credentials.count(key) == 0U) { google::cloud::internal::RaiseInvalidArgument( "Invalid ServiceAccountCredentials, the " + std::string(key) + @@ -49,20 +49,20 @@ ServiceAccountCredentialsInfo ParseServiceAccountCredentials( } } // The token_uri field may be missing, but may not be empty: - if (credentials.count(TOKEN_URI_KEY) != 0U and - credentials.value(TOKEN_URI_KEY, "").empty()) { + if (credentials.count(token_uri_key) != 0U and + credentials.value(token_uri_key, "").empty()) { google::cloud::internal::RaiseInvalidArgument( - "Invalid ServiceAccountCredentials, the " + std::string(TOKEN_URI_KEY) + + "Invalid ServiceAccountCredentials, the " + std::string(token_uri_key) + " field is empty on data loaded from " + source); } return ServiceAccountCredentialsInfo{ - credentials.value(PRIVATE_KEY_ID_KEY, ""), - credentials.value(PRIVATE_KEY_KEY, ""), + credentials.value(private_key_id_key, ""), + credentials.value(private_key_key, ""), // Some credential formats (e.g. gcloud's ADC file) don't contain a // "token_uri" attribute in the JSON object. In this case, we try using // the default value. - credentials.value(TOKEN_URI_KEY, default_token_uri), - credentials.value(CLIENT_EMAIL_KEY, ""), + credentials.value(token_uri_key, default_token_uri), + credentials.value(client_email_key, ""), }; }