-
Notifications
You must be signed in to change notification settings - Fork 381
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
feat: Fix impersonated service account parsing exception #1862
Changes from all 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" | ||
} |
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'; | ||
|
@@ -59,6 +59,17 @@ const MOCK_REFRESH_TOKEN_CONFIG = { | |
type: 'authorized_user', | ||
refresh_token: 'test_token', | ||
}; | ||
const MOCK_IMPERSONATED_TOKEN_CONFIG = { | ||
delegates: [], | ||
service_account_impersonation_url: '', | ||
source_credentials: { | ||
client_id: 'test_client_id', | ||
client_secret: 'test_client_secret', | ||
refresh_token: 'test_refresh_token', | ||
type: 'authorized_user' | ||
}, | ||
type: 'impersonated_service_account' | ||
} | ||
|
||
const ONE_HOUR_IN_SECONDS = 60 * 60; | ||
const FIVE_MINUTES_IN_SECONDS = 5 * 60; | ||
|
@@ -424,6 +435,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); | ||
|
@@ -538,6 +556,15 @@ describe('Credential', () => { | |
expect(isApplicationDefault(c)).to.be.true; | ||
}); | ||
|
||
it('should return true for ImpersonatedServiceAccountCredential loaded from GOOGLE_APPLICATION_CREDENTIALS', () => { | ||
process.env.GOOGLE_APPLICATION_CREDENTIALS = path.resolve( | ||
__dirname, '../../resources/mock.impersonated_key.json' | ||
); | ||
const c = getApplicationDefault(); | ||
expect(c).is.instanceOf(ImpersonatedServiceAccountCredential); | ||
expect(isApplicationDefault(c)).to.be.true; | ||
}); | ||
|
||
it('should return true for credential loaded from gcloud SDK', () => { | ||
if (!fs.existsSync(GCLOUD_CREDENTIAL_PATH)) { | ||
// tslint:disable-next-line:no-console | ||
|
@@ -570,6 +597,11 @@ describe('Credential', () => { | |
expect(isApplicationDefault(c)).to.be.false; | ||
}); | ||
|
||
it('should return false for explicitly loaded ImpersonatedServiceAccountCredential', () => { | ||
const c = new ImpersonatedServiceAccountCredential(MOCK_IMPERSONATED_TOKEN_CONFIG); | ||
expect(isApplicationDefault(c)).to.be.false; | ||
}); | ||
|
||
it('should return false for custom credential', () => { | ||
const c: Credential = { | ||
getAccessToken: () => { | ||
|
@@ -636,5 +668,15 @@ describe('Credential', () => { | |
expect(stub.args[0][0].httpAgent).to.equal(agent); | ||
}); | ||
}); | ||
|
||
it('ImpersonatedServiceAccountCredential should use the provided HTTP Agent', () => { | ||
const agent = new Agent(); | ||
const c = new ImpersonatedServiceAccountCredential(MOCK_IMPERSONATED_TOKEN_CONFIG, agent); | ||
return c.getAccessToken().then((token) => { | ||
expect(token.access_token).to.equal(expectedToken); | ||
expect(stub).to.have.been.calledOnce; | ||
expect(stub.args[0][0].httpAgent).to.equal(agent); | ||
}); | ||
}); | ||
}); | ||
}); |
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
ignoreMissing
we turn off theFailed to read credentials from file
error. I think ifGOOGLE_APPLICATION_CREDENTIALS
is 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
ignoreMissing
as false when checkGOOGLE_APPLICATION_CREDENTIALS
on line 486. So we still can encounterFirebaseAppError
when an incorrect file path is passed.