-
Notifications
You must be signed in to change notification settings - Fork 407
feat: Fix impersonated service account parsing exception #1862
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
Changes from 3 commits
5102751
a940307
175edd8
996941e
9bbe729
968225a
5c88efd
5d87876
178f906
dbf701a
a8d0c23
bcf2b33
4dff635
42b9c5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "delegates": [], | ||
| "service_account_impersonation_url": "", | ||
| "source_credentials": { | ||
| "client_id": "client_id", | ||
| "client_secret": "client_secret", | ||
| "refresh_token": "refresh_token", | ||
| "type": "authorized_user" | ||
| }, | ||
| "type": "impersonated_service_account" | ||
| } | ||
blue-hope marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ import { | |
| } from '../../../src/app/index'; | ||
| import { | ||
| RefreshTokenCredential, ServiceAccountCredential, | ||
| ComputeEngineCredential, getApplicationDefault, isApplicationDefault | ||
| ComputeEngineCredential, getApplicationDefault, isApplicationDefault, ImpersonatedServiceAccountCredential | ||
| } from '../../../src/app/credential-internal'; | ||
| import { HttpClient } from '../../../src/utils/api-request'; | ||
| import { Agent } from 'https'; | ||
|
|
@@ -424,6 +424,13 @@ describe('Credential', () => { | |
| expect(c).to.be.an.instanceof(ServiceAccountCredential); | ||
| }); | ||
|
|
||
| it('should return a ImpersonatedCredential with impersonated GOOGLE_APPLICATION_CREDENTIALS set', () => { | ||
| process.env.GOOGLE_APPLICATION_CREDENTIALS | ||
| = path.resolve(__dirname, '../../resources/mock.impersonated_key.json'); | ||
| const c = getApplicationDefault(); | ||
| expect(c).to.be.an.instanceof(ImpersonatedServiceAccountCredential); | ||
| }); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add another test to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think we can improve the test coverage for Let's also add another test in |
||
| it('should throw if explicitly pointing to an invalid path', () => { | ||
| process.env.GOOGLE_APPLICATION_CREDENTIALS = 'invalidpath'; | ||
| expect(() => getApplicationDefault()).to.throw(Error); | ||
|
|
||
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.
Correct me if I am wrong here, but it looks like by setting
ignoreMissingwe turn off theFailed to read credentials from fileerror. I think ifGOOGLE_APPLICATION_CREDENTIALSis set andgetApplicationDefault()is called we should still throw if it is an incorrect file path, right?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.
Sure, so I passed
ignoreMissingas false when checkGOOGLE_APPLICATION_CREDENTIALSon line 486. So we still can encounterFirebaseAppErrorwhen an incorrect file path is passed.