feat: Support retrieving ID Token from IAM endpoint for ServiceAccountCredentials#1433
feat: Support retrieving ID Token from IAM endpoint for ServiceAccountCredentials#1433
Conversation
zhumin8
left a comment
There was a problem hiding this comment.
The high level logic seems reasonable to me, still trying to get a better understanding of the id-token workflow. I will give it a second try after I do some readings on the context.
|
|
||
| @Test | ||
| public void idTokenWithAudience_incorrect() throws IOException { | ||
| public void idTokenWithAudience_oauthFlow_incorrect() throws IOException { |
There was a problem hiding this comment.
Maybe something like idTokenWithAudience_oauthFlow_incorrectAudience() to be more specific on the test?
There was a problem hiding this comment.
Sounds good, will update!
…als.java Co-authored-by: Min Zhu <zhumin@google.com>
zhumin8
left a comment
There was a problem hiding this comment.
LGTM.
only a few nit picks.
| String assertion = | ||
| createAssertionForIdToken( | ||
| jsonFactory, currentTime, tokenServerUri.toString(), targetAudience); | ||
| createAssertionForIdToken(currentTime, tokenServerUri.toString(), targetAudience); |
There was a problem hiding this comment.
For readability, perhaps rename tokenServerUri tooauthIdTokenUri in parallel to iamIdTokenUri?
| URI iamIdTokenUri = | ||
| URI.create( | ||
| String.format( | ||
| OAuth2Utils.IAM_ID_TOKEN_ENDPOINT_FORMAT, getUniverseDomain(), clientEmail)); |
There was a problem hiding this comment.
nit: Can we be consistent with iamIdTokenUri and tokenServerUri? either both declare as class variable or both inside respective get endpoint method.
There was a problem hiding this comment.
tokenServerUri is a customizable from setters and can't be declared as a class variable. iamIdTokenUri probably can be initialized in the constructor and I'll move it up there.
There was a problem hiding this comment.
On second thought, getUniverseDomain() throws an IOException and adding a checked exception to the constructor is a breaking change. I think it would be better to leave it here.
|
|
See b/340613207 for more information.