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

fix: makes default token url universe aware #1383

Merged
merged 6 commits into from
Apr 11, 2024
Merged

fix: makes default token url universe aware #1383

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
88c6632
fix: makes default token url universe aware
aeitzman Apr 9, 2024
d0fc130
lint and add test
aeitzman Apr 9, 2024
24422f5
Update oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredent…
aeitzman Apr 9, 2024
5842c8f
add back else
aeitzman Apr 9, 2024
f5766a3
Merge branch 'main' into universe-aware-default
aeitzman Apr 10, 2024
b7cc2fe
move code into override
aeitzman Apr 11, 2024
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
17 changes: 15 additions & 2 deletions oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public abstract class ExternalAccountCredentials extends GoogleCredentials {
static final String EXTERNAL_ACCOUNT_FILE_TYPE = "external_account";
static final String EXECUTABLE_SOURCE_KEY = "executable";

static final String DEFAULT_TOKEN_URL = "https://sts.googleapis.com/v1/token";
static final String DEFAULT_TOKEN_URL = "https://sts.{UNIVERSE_DOMAIN}/v1/token";
static final String PROGRAMMATIC_METRICS_HEADER_VALUE = "programmatic";

private final String transportFactoryClassName;
Expand Down Expand Up @@ -235,7 +235,20 @@ protected ExternalAccountCredentials(ExternalAccountCredentials.Builder builder)
this.serviceAccountImpersonationUrl = builder.serviceAccountImpersonationUrl;
this.clientId = builder.clientId;
this.clientSecret = builder.clientSecret;
this.tokenUrl = builder.tokenUrl == null ? DEFAULT_TOKEN_URL : builder.tokenUrl;

if (builder.tokenUrl == null) {
try {
this.tokenUrl = DEFAULT_TOKEN_URL.replace("{UNIVERSE_DOMAIN}", this.getUniverseDomain());
} catch (IOException e) {
// Throwing an IOException would be a breaking change, so wrap it here.
// This should not happen for this credential type.
Copy link

@westarle westarle Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a place we can read this value for this credential type that doesn't offer an exception (and this dead code)?

Can we override getUniverseDomain() and strip the exception if it is always available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could override it, I think we would still have the dead code in the overridden function, but I may be misunderstanding what you mean by strip the exception, do you just mean doing this? -

  @Override
  public String getUniverseDomain() {
    try {
      return super.getUniverseDomain();
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }

The other easier option would be to make universeDomain protected instead of private in GoogleCredentials so the external account credential could just read it directly instead of calling the parent getUniverseDomain() function.

Copy link

Choose a reason for hiding this comment

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

The example you put is what I meant -- this implementation has a more precise definition of getUniverseDomain that can embed the fact that it will not throw. This will help this codepath and any other users avoid a hard decision.

universeDomain can't be protected in GoogleCredentials because it isn't usable by subclasses of GCE credential types, and those credential types can't hide it. It probably isn't set until a successful request to MDS.

I understand the override is kind of ugly but it is a consequence of our deep type hierarchy.

throw new IllegalStateException(
"Error occurred when attempting to retrieve universe domain.", e);
}
} else {
this.tokenUrl = builder.tokenUrl;
}

this.scopes =
(builder.scopes == null || builder.scopes.isEmpty())
? Arrays.asList(CLOUD_PLATFORM_SCOPE)
Expand Down
62 changes: 62 additions & 0 deletions oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountCredentialsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,44 @@ public void constructor_builder_defaultTokenUrl() {
assertEquals(STS_URL, credentials.getTokenUrl());
}

@Test
public void constructor_builder_defaultTokenUrlwithUniverseDomain() {
HashMap<String, Object> credentialSource = new HashMap<>();
credentialSource.put("file", "file");

ExternalAccountCredentials credentials =
IdentityPoolCredentials.newBuilder()
.setHttpTransportFactory(transportFactory)
.setAudience(
"//iam.googleapis.com/locations/global/workforcePools/pool/providers/provider")
.setSubjectTokenType("subjectTokenType")
.setCredentialSource(new TestCredentialSource(credentialSource))
.setUniverseDomain("testdomain.org")
.build();

assertEquals("https://sts.testdomain.org/v1/token", credentials.getTokenUrl());
}

@Test
public void constructor_builder_getUniverseDomainFails() {
HashMap<String, Object> credentialSource = new HashMap<>();
credentialSource.put("file", "file");

try {
UniverseDomainErrorTestCredentials.newBuilder()
.setHttpTransportFactory(transportFactory)
.setAudience(
"//iam.googleapis.com/locations/global/workforcePools/pool/providers/provider")
.setSubjectTokenType("subjectTokenType")
.setUniverseDomain("testdomain.org")
.build();
fail("Should not be able to continue without exception.");
} catch (IllegalStateException exception) {
assertEquals(
"Error occurred when attempting to retrieve universe domain.", exception.getMessage());
}
}

@Test
public void constructor_builder_subjectTokenTypeEnum() {
HashMap<String, Object> credentialSource = new HashMap<>();
Expand Down Expand Up @@ -1350,4 +1388,28 @@ public String retrieveSubjectToken() {
return "subjectToken";
}
}

static class UniverseDomainErrorTestCredentials extends TestExternalAccountCredentials {
protected UniverseDomainErrorTestCredentials(TestExternalAccountCredentials.Builder builder) {
super(builder);
}

public static UniverseDomainErrorTestCredentials.Builder newBuilder() {
return new UniverseDomainErrorTestCredentials.Builder();
}

static class Builder extends TestExternalAccountCredentials.Builder {
Builder() {}

@Override
public UniverseDomainErrorTestCredentials build() {
return new UniverseDomainErrorTestCredentials(this);
}
}

@Override
public String getUniverseDomain() throws IOException {
throw new IOException("Test error");
}
}
}
Loading