-
Notifications
You must be signed in to change notification settings - Fork 258
feat: Add TrustBoundaries support for ExternalAccounts. #1836
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.
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
base: feat-tb-sa
Are you sure you want to change the base?
feat: Add TrustBoundaries support for ExternalAccounts. #1836
Conversation
… and header value. # Conflicts: # oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java # oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java # oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java # Conflicts: # oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java # oauth2_http/java/com/google/auth/oauth2/GoogleCredentials.java # oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java # oauth2_http/java/com/google/auth/oauth2/OAuth2Credentials.java # oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java # oauth2_http/java/com/google/auth/oauth2/TrustBoundary.java
…e trust boundary enabler env variable
…ents regarding a separate mock for trust boundary.
fb18c0c to
458bad4
Compare
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java
Outdated
Show resolved
Hide resolved
…s universe domain.
oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/MockTokenServerTransport.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountCredentialsTest.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
…tion and format changes.
oauth2_http/java/com/google/auth/oauth2/ExternalAccountAuthorizedUserCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
|
I think things generally LGTM. I will do a second pass for the tests below. |
| AccessToken accessToken = this.impersonatedCredentials.refreshAccessToken(); | ||
| // After the impersonated credential refreshes, its trust boundary is | ||
| // also refreshed. That is the trust boundary we will use. | ||
| // We use the impersonated account's credential as the trust boundary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think the previous comment was more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great job! :)
01eb20d to
2071071
Compare
| protected void setTrustBoundary(TrustBoundary trustBoundary) { | ||
| this.trustBoundary = trustBoundary; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still needed?
| .setSubjectTokenType("subjectTokenType") | ||
| .build(); | ||
|
|
||
| awsCredential.refreshAccessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think most of the other test classes use .refresh()
| throw new IllegalStateException( | ||
| "The provided audience is not in a valid format for either a workload identity pool or a workforce pool."); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If possible, is there an official doc that we can link to with the format for workload pool and workforce pool?
| throw new IllegalStateException( | ||
| "The provided audience is not in the correct format for a workforce pool."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If possible, can we link to a public offical doc that has the format for workforce pool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I added a few nits that may need additional code changes (feel free to resolve if it's not possible). I can re-approve afterwards.
Added logic and unit tests for trust boundary for external accounts. This PR covers
This PR is a followup of the initial PR for Trust Boundaries for Service accounts.