Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages when loading credentials. #1212

Merged
merged 7 commits into from
Oct 4, 2018
Merged

Improve error messages when loading credentials. #1212

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
38871fa
Improve error messages when loading credentials.
coryan Oct 3, 2018
9300753
WIP - Address review comments.
coryan Oct 3, 2018
a82e4ac
Address comments about ServiceAccountCredentials.
coryan Oct 4, 2018
1c50012
Address review comments for AuthorizedUserCredentials.
coryan Oct 4, 2018
71f2b97
Use is_abandoned to detect parse errors.
coryan Oct 4, 2018
5723dfa
Apply clang-format(1).
coryan Oct 4, 2018
7f9255e
Make clang-tidy(1) happy.
coryan Oct 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 44 additions & 14 deletions google/cloud/storage/oauth2/authorized_user_credentials.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,49 @@ template <typename HttpRequestBuilderType =
storage::internal::CurlRequestBuilder>
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this new logic could be factored out into its own function, which would (probably) make testing it easier. If you had something like ParseAndValidateCredentialContent(), it could check if the content is valid/parseable JSON, check that the required fields exist, and make sure that those fields aren't empty (meaning we don't have to change the .get() calls below)... returning the nl::json object if all went well, or raising/dying at the first error it finds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored, let me know what you think.

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();
}
Expand All @@ -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
coryan marked this conversation as resolved.
Show resolved Hide resolved
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<std::string const&>();
header += access_token.value("token_type", "");
coryan marked this conversation as resolved.
Show resolved Hide resolved
header += ' ';
header += access_token["access_token"].get_ref<std::string const&>();
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.
Expand Down
59 changes: 57 additions & 2 deletions google/cloud/storage/oauth2/authorized_user_credentials_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ TEST_F(AuthorizedUserCredentialsTest, Simple) {
"type": "magic_type"
})""";

AuthorizedUserCredentials<MockHttpRequestBuilder> credentials(config);
AuthorizedUserCredentials<MockHttpRequestBuilder> credentials(config, "test");
EXPECT_EQ("Authorization: Type access-token-value",
credentials.AuthorizationHeader());
}
Expand Down Expand Up @@ -137,7 +137,7 @@ TEST_F(AuthorizedUserCredentialsTest, Refresh) {
"refresh_token": "1/THETOKEN",
"type": "magic_type"
})""";
AuthorizedUserCredentials<MockHttpRequestBuilder> credentials(config);
AuthorizedUserCredentials<MockHttpRequestBuilder> credentials(config, "test");
EXPECT_EQ("Authorization: Type access-token-r1",
credentials.AuthorizationHeader());
EXPECT_EQ("Authorization: Type access-token-r2",
Expand All @@ -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<MockHttpRequestBuilder> 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<MockHttpRequestBuilder>(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<MockHttpRequestBuilder> 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<MockHttpRequestBuilder>(config, "test-as-a-source"),
"exceptions are disabled");
#endif // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
}

} // namespace
} // namespace oauth2
} // namespace STORAGE_CLIENT_NS
Expand Down
26 changes: 18 additions & 8 deletions google/cloud/storage/oauth2/google_credentials.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,28 @@ std::shared_ptr<Credentials> 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<char>{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<AuthorizedUserCredentials<>>(contents);
return std::make_shared<AuthorizedUserCredentials<>>(contents, path);
}
if (cred_type == "service_account") {
return std::make_shared<ServiceAccountCredentials<>>(contents);
return std::make_shared<ServiceAccountCredentials<>>(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
Expand All @@ -58,24 +68,24 @@ std::shared_ptr<AuthorizedUserCredentials<>>
CreateAuthorizedUserCredentialsFromJsonFilePath(std::string const& path) {
std::ifstream is(path);
std::string contents(std::istreambuf_iterator<char>{is}, {});
return CreateAuthorizedUserCredentialsFromJsonContents(contents);
return std::make_shared<AuthorizedUserCredentials<>>(contents, path);
}

std::shared_ptr<AuthorizedUserCredentials<>>
CreateAuthorizedUserCredentialsFromJsonContents(std::string const& contents) {
return std::make_shared<AuthorizedUserCredentials<>>(contents);
return std::make_shared<AuthorizedUserCredentials<>>(contents, "memory");
}

std::shared_ptr<ServiceAccountCredentials<>>
CreateServiceAccountCredentialsFromJsonFilePath(std::string const& path) {
std::ifstream is(path);
std::string contents(std::istreambuf_iterator<char>{is}, {});
return CreateServiceAccountCredentialsFromJsonContents(contents);
return std::make_shared<ServiceAccountCredentials<>>(contents, path);
}

std::shared_ptr<ServiceAccountCredentials<>>
CreateServiceAccountCredentialsFromJsonContents(std::string const& contents) {
return std::make_shared<ServiceAccountCredentials<>>(contents);
return std::make_shared<ServiceAccountCredentials<>>(contents, "memory");
}

} // namespace oauth2
Expand Down
71 changes: 68 additions & 3 deletions google/cloud/storage/oauth2/google_credentials_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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
Expand Down
Loading