-
Notifications
You must be signed in to change notification settings - Fork 265
feat: Support user defined or json defined scopes for impersonated token #1815
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
Conversation
4ec205f to
7705a7b
Compare
|
| if (json.containsKey("scopes")) { | ||
| scopes = ImmutableList.copyOf((List<String>) json.get("scopes")); | ||
| } |
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.
This scopes field was recently added and may not exist in existing ImpersonatedCred Json files. Check if it exists
| */ | ||
| @CanIgnoreReturnValue | ||
| public Builder setScopes(List<String> scopes) { | ||
| Preconditions.checkNotNull(scopes, "Scopes cannot be null"); |
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.
Constructor has a null check that threw a IllegalStateException runtime exception. Enforce this on the setter so users don't pass in an invalid data.
This never worked, so we don't expect any breakages or changes in behavior.
zhumin8
left a comment
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.
Change LGTM.
Got question about CLOUD_PLATFORM_SCOPE used for sourceCredentials, but I think it does not block this change.
| this.sourceCredentials = | ||
| this.sourceCredentials.createScoped(Arrays.asList(CLOUD_PLATFORM_SCOPE)); | ||
| this.sourceCredentials.createScoped( | ||
| Collections.singletonList(OAuth2Utils.CLOUD_PLATFORM_SCOPE)); |
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.
Does it work or is it better to switch to https://www.googleapis.com/auth/iam?
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.
I think either should work: https://cloud.google.com/iam/docs/reference/credentials/rest/v1/projects.serviceAccounts/generateAccessToken#authorization-scopes
Not sure if there is a preference. I can update if there is.


See b/450322374 for more information
Changes
scopesvalue from the JSON fileLots of smaller changes in test files from smaller refactors related to create a
CLOUD_PLATFORM_SCOPEconstant and such.