Skip to content

fix(oauth2_http): scope sourceCredentials in ImpersonatedCredentials constructor - #14452

Draft
lsirac wants to merge 1 commit into
mainfrom
fix/b-563846234-impersonated-source-concurrency
Draft

lsirac wants to merge 1 commit into
mainfrom
fix/b-563846234-impersonated-source-concurrency

Conversation

@lsirac

@lsirac lsirac commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Scope sourceCredentials with CLOUD_PLATFORM_SCOPE once inside the ImpersonatedCredentials(Builder) constructor when getAccessToken() is null, and make sourceCredentials final.

Previously, ImpersonatedCredentials.refreshAccessToken() checked this.sourceCredentials.getAccessToken() == null and reassigned this.sourceCredentials = this.sourceCredentials.createScoped(...) without synchronization. Concurrent cold-start callers created separate sourceCredentials instances with separate OAuth2Credentials locks, causing redundant source token refreshes and repeating createScoped on every refresh when sourceCredentials used self-signed JWTs. Scoping sourceCredentials once in the constructor ensures all callers share the same final sourceCredentials instance and its single-flight refresh lock.

Testing

  • Unit tests (ImpersonatedCredentialsTest, ExternalAccountCredentialsTest, and full oauth2_http test suite) and live 16-thread cold-start verification against Google Cloud STS and IAMCredentials: https://paste.googleplex.com/6574850316042240

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request moves the scoping logic of sourceCredentials from refreshAccessToken() to the constructor of ImpersonatedCredentials to prevent concurrent cold starts from scoping and refreshing the source credentials multiple times. It also introduces a sourceCredentialsScoped flag in the Builder to track scoping status and adds a corresponding integration test. The reviewer pointed out that the use of firstNonNull when scoping credentials in the constructor is redundant and could cause compilation errors due to a missing import, suggesting a direct assignment instead.

Comment on lines +541 to +545
sourceCredentials =
firstNonNull(
sourceCredentials.createScoped(
Collections.singletonList(OAuth2Utils.CLOUD_PLATFORM_SCOPE)),
sourceCredentials);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The use of firstNonNull here is redundant because GoogleCredentials.createScoped is guaranteed to return a non-null GoogleCredentials instance (either this or a newly created scoped instance). Additionally, since no new import for firstNonNull (e.g., from Guava's MoreObjects) was added in this file, this could lead to a compilation error if it is not already statically imported. We can safely simplify this by directly assigning the result of createScoped.

      sourceCredentials =
          sourceCredentials.createScoped(
              Collections.singletonList(OAuth2Utils.CLOUD_PLATFORM_SCOPE));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant