fix: downscope credentials used for IAM AuthN login #999
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
enocom approved these changes Sep 30, 2022
Contributor Author
| Looks like the Code Coverage check wants me to add a test |
Member
| One option for writing a test: extract the logic here into a method that accepts a Credentials object. Then you could test what happens when that token does not supported GoogleCredentials, what happens when the credentials are scoped, etc. |
kurtisvg suggested changes Sep 30, 2022
| | ||
| if (enableIamAuth) { | ||
| try { | ||
| credentials.get().refresh(); |
Contributor
There was a problem hiding this comment.
Can we refresh after we downscope?
Contributor Author
There was a problem hiding this comment.
Unfortunately no, we get a nullpointer if we do that.
shubha-rajan force-pushed the downscoping branch 3 times, most recently from c52260a to 53e4480 Compare 
shubha-rajan force-pushed the downscoping branch from 274f5f7 to 25a371a Compare shubha-rajan added the kokoro:force-run 
kokoro-team removed the kokoro:force-run shubha-rajan force-pushed the downscoping branch from 25a371a to a7ff4af Compare shubha-rajan added the kokoro:force-run kokoro-team removed the kokoro:force-run shubha-rajan force-pushed the downscoping branch 5 times, most recently from 6e24cb5 to f670014 Compare kurtisvg suggested changes Oct 31, 2022
Comment on lines 530 to 531
| OAuth2Credentials creds = credentials.get(); | ||
| creds.refresh(); | ||
| GoogleCredentials downscoped = getDownscopedCredentials(creds); | ||
| downscoped.refresh(); |
Contributor
There was a problem hiding this comment.
I'd like to better understand why we need to refresh twice here?
shubha-rajan force-pushed the downscoping branch 2 times, most recently from f4d0267 to ace1c22 Compare 
shubha-rajan force-pushed the downscoping branch from 0012692 to f909ac8 Compare kurtisvg approved these changes Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Relevant issues: