Repository navigation
feat: support external_account_authorized_user credentials - #16581
Merged
scotthart merged 4 commits intoOct 23, 2026
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ExternalAccountAuthorizedUserCredentials and its configuration parser to support external account authorized user configurations, along with comprehensive unit tests. The reviewer feedback highlights a critical thread-safety issue where concurrent calls to GetToken can cause a data race on info_.refresh_token during token rotation, recommending the use of std::mutex to protect this state. Additionally, several style guide violations were identified, including the use of default parameters in an internal namespace constructor and the use of auto where it obscures StatusOr<T> and other return types.
Comment on lines
+152
to
+156
| private: | ||
| ExternalAccountAuthorizedUserInfo info_; | ||
| HttpClientFactory client_factory_; | ||
| Options options_; | ||
| }; |
There was a problem hiding this comment.
Since ExternalAccountAuthorizedUserCredentials::GetToken modifies info_.refresh_token during refresh token rotation, this class has mutable state. To prevent data races when multiple threads call GetToken concurrently, we must protect access to info_.refresh_token using a std::mutex.
private:
mutable std::mutex mu_;
ExternalAccountAuthorizedUserInfo info_;
HttpClientFactory client_factory_;
Options options_;
};
Contributor
Author
There was a problem hiding this comment.
Done — added mutable std::mutex mu_; to protect info_.refresh_token.
Comment on lines
+384
to
+433
| StatusOr<AccessToken> ExternalAccountAuthorizedUserCredentials::GetToken( | ||
| std::chrono::system_clock::time_point tp) { | ||
| internal::Base64Encoder encoder; | ||
| for (unsigned char const c : | ||
| absl::StrCat(info_.client_id, ":", info_.client_secret)) { | ||
| encoder.PushBack(c); | ||
| } | ||
| auto request = | ||
| rest_internal::RestRequest(info_.token_url) | ||
| .AddHeader("content-type", "application/x-www-form-urlencoded") | ||
| .AddHeader("Authorization", | ||
| absl::StrCat("Basic ", std::move(encoder).FlushAndPad())); | ||
|
|
||
| auto form_data = std::vector<std::pair<std::string, std::string>>{ | ||
| {"grant_type", "refresh_token"}, | ||
| {"refresh_token", info_.refresh_token}, | ||
| }; | ||
|
|
||
| auto client = client_factory_(options_); | ||
| rest_internal::RestContext unused; | ||
| auto response = client->Post(unused, request, form_data); | ||
| if (!response) return std::move(response).status(); | ||
| if (IsHttpError(**response)) return AsStatus(std::move(**response)); | ||
| auto payload = rest_internal::ReadAll(std::move(**response).ExtractPayload()); | ||
| if (!payload) return std::move(payload).status(); | ||
|
|
||
| auto ec = internal::ErrorContext({ | ||
| {"token_url", info_.token_url}, | ||
| }); | ||
|
|
||
| auto access = nlohmann::json::parse(*payload, nullptr, false); | ||
| if (!access.is_object()) { | ||
| return InvalidArgumentError( | ||
| "token refresh response cannot be parsed as JSON object", | ||
| GCP_ERROR_INFO().WithContext(ec)); | ||
| } | ||
| auto token = | ||
| ValidateStringField(access, "access_token", "token-refresh-response", ec); | ||
| if (!token) return std::move(token).status(); | ||
| auto expires_in = | ||
| ValidateIntField(access, "expires_in", "token-refresh-response", ec); | ||
| if (!expires_in) return std::move(expires_in).status(); | ||
| auto refresh_token = ValidateStringField(access, "refresh_token", | ||
| "token-refresh-response", "", ec); | ||
| if (!refresh_token) return std::move(refresh_token).status(); | ||
| if (!refresh_token->empty()) { | ||
| info_.refresh_token = *std::move(refresh_token); | ||
| } | ||
| return AccessToken{*std::move(token), tp + std::chrono::seconds(*expires_in)}; | ||
| } |
There was a problem hiding this comment.
This suggestion addresses two issues:
- Thread Safety: Protects
info_.refresh_tokenwith a mutex lock to prevent data races during concurrent token refreshes. - Style Guide Adherence: Replaces
autowith explicit types forStatusOr<T>and other return types to comply with the repository style guide (lines 104-108).
StatusOr<AccessToken> ExternalAccountAuthorizedUserCredentials::GetToken(
std::chrono::system_clock::time_point tp) {
std::string refresh_token;
{
std::lock_guard<std::mutex> lk(mu_);
refresh_token = info_.refresh_token;
}
internal::Base64Encoder encoder;
for (unsigned char const c :
absl::StrCat(info_.client_id, ":", info_.client_secret)) {
encoder.PushBack(c);
}
rest_internal::RestRequest request =
rest_internal::RestRequest(info_.token_url)
.AddHeader("content-type", "application/x-www-form-urlencoded")
.AddHeader("Authorization",
absl::StrCat("Basic ", std::move(encoder).FlushAndPad()));
std::vector<std::pair<std::string, std::string>> form_data = {
{"grant_type", "refresh_token"},
{"refresh_token", std::move(refresh_token)},
};
std::unique_ptr<rest_internal::RestClient> client = client_factory_(options_);
rest_internal::RestContext unused;
StatusOr<std::unique_ptr<rest_internal::HttpResponse>> response =
client->Post(unused, request, form_data);
if (!response) return std::move(response).status();
if (IsHttpError(**response)) return AsStatus(std::move(**response));
StatusOr<std::string> payload =
rest_internal::ReadAll(std::move(**response).ExtractPayload());
if (!payload) return std::move(payload).status();
internal::ErrorContext const ec = internal::ErrorContext({
{"token_url", info_.token_url},
});
nlohmann::json access = nlohmann::json::parse(*payload, nullptr, false);
if (!access.is_object()) {
return InvalidArgumentError(
"token refresh response cannot be parsed as JSON object",
GCP_ERROR_INFO().WithContext(ec));
}
StatusOr<std::string> token =
ValidateStringField(access, "access_token", "token-refresh-response", ec);
if (!token) return std::move(token).status();
StatusOr<std::int64_t> expires_in =
ValidateIntField(access, "expires_in", "token-refresh-response", ec);
if (!expires_in) return std::move(expires_in).status();
StatusOr<std::string> new_refresh_token = ValidateStringField(
access, "refresh_token", "token-refresh-response", "", ec);
if (!new_refresh_token) return std::move(new_refresh_token).status();
if (!new_refresh_token->empty()) {
std::lock_guard<std::mutex> lk(mu_);
info_.refresh_token = *std::move(new_refresh_token);
}
return AccessToken{*std::move(token), tp + std::chrono::seconds(*expires_in)};
}References
- Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr, domain objects, protobuf messages/fields, or function return types. (link)
Contributor
Author
There was a problem hiding this comment.
Done — protected info_.refresh_token with std::lock_guard<std::mutex> and replaced auto with explicit types.
Comment on lines
+140
to
+142
| ExternalAccountAuthorizedUserCredentials( | ||
| ExternalAccountAuthorizedUserInfo info, HttpClientFactory client_factory, | ||
| Options options = {}); |
There was a problem hiding this comment.
The repository style guide (lines 35-40) disallows default parameters in internal namespaces (such as oauth2_internal). Please split this constructor into two explicit overloads to conform to the style guide.
ExternalAccountAuthorizedUserCredentials(
ExternalAccountAuthorizedUserInfo info, HttpClientFactory client_factory);
ExternalAccountAuthorizedUserCredentials(
ExternalAccountAuthorizedUserInfo info, HttpClientFactory client_factory,
Options options);References
- Disallow Default Parameters in internal Namespaces: Reject default argument values in functions, member functions, and constructors declared in internal namespaces. (link)
Contributor
Author
There was a problem hiding this comment.
Done — split into two explicit constructor overloads without default parameters.
Comment on lines
+338
to
+374
| StatusOr<ExternalAccountAuthorizedUserInfo> | ||
| ParseExternalAccountAuthorizedUserConfiguration( | ||
| std::string const& configuration, internal::ErrorContext const& ec) { | ||
| auto json = nlohmann::json::parse(configuration, nullptr, false); | ||
| if (!json.is_object()) { | ||
| return InvalidArgumentError( | ||
| "external account authorized user configuration was not a JSON object", | ||
| GCP_ERROR_INFO().WithContext(ec)); | ||
| } | ||
| auto type = ValidateStringField(json, "type", "credentials-file", ec); | ||
| if (!type) return std::move(type).status(); | ||
| if (*type != "external_account_authorized_user") { | ||
| return InvalidArgumentError( | ||
| "mismatched type (" + *type + | ||
| ") in external account authorized user configuration", | ||
| GCP_ERROR_INFO().WithContext(ec)); | ||
| } | ||
| auto client_id = | ||
| ValidateStringField(json, "client_id", "credentials-file", ec); | ||
| if (!client_id) return std::move(client_id).status(); | ||
| auto client_secret = | ||
| ValidateStringField(json, "client_secret", "credentials-file", ec); | ||
| if (!client_secret) return std::move(client_secret).status(); | ||
| auto refresh_token = | ||
| ValidateStringField(json, "refresh_token", "credentials-file", ec); | ||
| if (!refresh_token) return std::move(refresh_token).status(); | ||
| auto token_url = | ||
| ValidateStringField(json, "token_url", "credentials-file", ec); | ||
| if (!token_url) return std::move(token_url).status(); | ||
| auto universe_domain = GetUniverseDomainFromCredentialsJson(json); | ||
| if (!universe_domain) return std::move(universe_domain).status(); | ||
|
|
||
| return ExternalAccountAuthorizedUserInfo{ | ||
| *std::move(client_id), *std::move(client_secret), | ||
| *std::move(refresh_token), *std::move(token_url), | ||
| *std::move(universe_domain)}; | ||
| } |
There was a problem hiding this comment.
The repository style guide (lines 104-108) rejects the use of auto when it hides StatusOr<T> or other domain/return types. Please use explicit types instead of auto for these fields.
StatusOr<ExternalAccountAuthorizedUserInfo>
ParseExternalAccountAuthorizedUserConfiguration(
std::string const& configuration, internal::ErrorContext const& ec) {
nlohmann::json json = nlohmann::json::parse(configuration, nullptr, false);
if (!json.is_object()) {
return InvalidArgumentError(
"external account authorized user configuration was not a JSON object",
GCP_ERROR_INFO().WithContext(ec));
}
StatusOr<std::string> type = ValidateStringField(json, "type", "credentials-file", ec);
if (!type) return std::move(type).status();
if (*type != "external_account_authorized_user") {
return InvalidArgumentError(
"mismatched type (" + *type +
") in external account authorized user configuration",
GCP_ERROR_INFO().WithContext(ec));
}
StatusOr<std::string> client_id =
ValidateStringField(json, "client_id", "credentials-file", ec);
if (!client_id) return std::move(client_id).status();
StatusOr<std::string> client_secret =
ValidateStringField(json, "client_secret", "credentials-file", ec);
if (!client_secret) return std::move(client_secret).status();
StatusOr<std::string> refresh_token =
ValidateStringField(json, "refresh_token", "credentials-file", ec);
if (!refresh_token) return std::move(refresh_token).status();
StatusOr<std::string> token_url =
ValidateStringField(json, "token_url", "credentials-file", ec);
if (!token_url) return std::move(token_url).status();
StatusOr<std::string> universe_domain = GetUniverseDomainFromCredentialsJson(json);
if (!universe_domain) return std::move(universe_domain).status();
return ExternalAccountAuthorizedUserInfo{
*std::move(client_id), *std::move(client_secret),
*std::move(refresh_token), *std::move(token_url),
*std::move(universe_domain)};
}References
- Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr, domain objects, protobuf messages/fields, or function return types. (link)
Contributor
Author
There was a problem hiding this comment.
Done — replaced auto with explicit types.
Comment on lines
+63
to
+70
| if (cred_type == "external_account_authorized_user") { | ||
| auto info = ParseExternalAccountAuthorizedUserConfiguration( | ||
| contents, internal::ErrorContext{}); | ||
| if (!info) return std::move(info).status(); | ||
| return std::unique_ptr<Credentials>( | ||
| std::make_unique<ExternalAccountAuthorizedUserCredentials>( | ||
| *std::move(info), std::move(client_factory), options)); | ||
| } |
There was a problem hiding this comment.
The repository style guide (lines 104-108) rejects the use of auto when it hides StatusOr<T>. Please use explicit types instead of auto for info.
if (cred_type == "external_account_authorized_user") {
StatusOr<ExternalAccountAuthorizedUserInfo> info =
ParseExternalAccountAuthorizedUserConfiguration(
contents, internal::ErrorContext{});
if (!info) return std::move(info).status();
return std::unique_ptr<Credentials>(
std::make_unique<ExternalAccountAuthorizedUserCredentials>(
*std::move(info), std::move(client_factory), options));
}References
- Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr, domain objects, protobuf messages/fields, or function return types. (link)
Contributor
Author
There was a problem hiding this comment.
Done — replaced auto with StatusOr<ExternalAccountAuthorizedUserInfo>.
Comment on lines
+135
to
+154
| auto const json = | ||
| nlohmann::json::parse(cfg.json_object(), nullptr, false); | ||
| if (json.is_object()) { | ||
| auto const it = json.find("type"); | ||
| if (it != json.end() && it->is_string() && | ||
| *it == "external_account_authorized_user") { | ||
| auto info = | ||
| oauth2_internal::ParseExternalAccountAuthorizedUserConfiguration( | ||
| cfg.json_object(), ec); | ||
| if (!info) { | ||
| result = MakeErrorCredentials(std::move(info).status()); | ||
| return; | ||
| } | ||
| auto creds = std::make_shared< | ||
| oauth2_internal::ExternalAccountAuthorizedUserCredentials>( | ||
| *std::move(info), client_factory_, cfg.options()); | ||
| result = Decorate(std::move(creds), std::move(client_factory_), | ||
| cfg.options()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The repository style guide (lines 104-108) rejects the use of auto when it hides StatusOr<T> or other return types. Please use explicit types instead of auto.
nlohmann::json const json =
nlohmann::json::parse(cfg.json_object(), nullptr, false);
if (json.is_object()) {
nlohmann::json::const_iterator const it = json.find("type");
if (it != json.end() && it->is_string() &&
*it == "external_account_authorized_user") {
StatusOr<oauth2_internal::ExternalAccountAuthorizedUserInfo> info =
oauth2_internal::ParseExternalAccountAuthorizedUserConfiguration(
cfg.json_object(), ec);
if (!info) {
result = MakeErrorCredentials(std::move(info).status());
return;
}
std::shared_ptr<oauth2_internal::Credentials> creds =
std::make_shared<
oauth2_internal::ExternalAccountAuthorizedUserCredentials>(
*std::move(info), client_factory_, cfg.options());
result = Decorate(std::move(creds), std::move(client_factory_),
cfg.options());
return;
}
}References
- Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr, domain objects, protobuf messages/fields, or function return types. (link)
Contributor
Author
There was a problem hiding this comment.
Done — replaced auto with explicit types.
colinmoy
approved these changes
Oct 21, 2026
Contributor
Author
|
@colinmoy Can we please |
Collaborator
|
/gcbrun |
sachinpro
force-pushed
the
feat-external-account-authorized-user
branch
from
October 21, 2026 18:19
6cc466a to
48b4a01
Compare
Contributor
Author
|
@colinmoy I updated the PR because gcc-oldest-pr was failing. Please |
Collaborator
|
/gcbrun |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16581 +/- ##
==========================================
+ Coverage 92.38% 92.41% +0.02%
==========================================
Files 2266 2266
Lines 219636 220009 +373
==========================================
+ Hits 202904 203313 +409
+ Misses 16732 16696 -36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
scotthart
requested changes
Oct 21, 2026
|
|
||
| void visit(ExternalAccountConfig const& cfg) override { | ||
| auto const ec = internal::ErrorContext(); | ||
| nlohmann::json const json = |
Member
There was a problem hiding this comment.
This block of code seems to duplicate most of the error checking below. Also, it introduces a new call to json::parse such that in non-external account authorized user instances, we end up parsing the same json twice. Let's look at trying to work this new credential into the existing external account cred flow.
Contributor
Author
There was a problem hiding this comment.
I have removed all the changes from unified_rest_credentials.cc since we are now handling it inside ParseExternalAccountConfiguration.
If it looks good, please /gcbrun.
…ernal account flow
Member
|
/gcbrun |
scotthart
approved these changes
Oct 22, 2026
When an external account credential configuration cannot be parsed or is unsupported by gRPC C++'s internal parser (such as external_account_authorized_user), GrpcExternalAccountCredentials() returns nullptr. Passing nullptr directly into grpc::CompositeChannelCredentials() causes a segmentation fault. Check for null and return a GrpcErrorCredentialsAuthentication with InvalidArgumentError instead of crashing.
Contributor
Author
|
@scotthart @colinmoy I added a minor commit to fix a crash when unsupported config is sent to gRPC. Please I have verified this e2e on GCD. |
Member
|
/gcbrun |
Member
|
/gharun |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds support for
"type": "external_account_authorized_user"credentials (OAuth 2.0 refresh token flow for Workforce Identity Federation external accounts) when loading credentials via Application Default Credentials (MakeGoogleDefaultCredentials) orMakeExternalAccountCredentials."type": "external_account_authorized_user"configurations (client_id,client_secret,refresh_token,token_url, and optionaluniverse_domain).ExternalAccountAuthorizedUserCredentials::GetTokenusing HTTP Basic Authentication (client_id:client_secret) andgrant_type=refresh_token, including optionalrefresh_tokenrotation in the token response.external_account_authorized_userintoLoadCredsFromString(ADC) andExternalAccountConfiginunified_rest_credentials.cc.MapCredentials.