Handle special case of application default credentials location#444
Handle special case of application default credentials location#444hiranya911 merged 4 commits intofirebase:masterfrom
Conversation
|
ADC protocol only requires that the variable point to a service account json file: https://cloud.google.com/docs/authentication/production
However, in practice many implementations are lenient about this, and allow both service accounts and refresh token files. I'm ok with making the Node.js implementation do the same. But right now there's a CI failure, and we cannot proceed until that is fixed. |
|
Thank you. This is important in environments where you may pass your local application default credentials into a docker container in a dev environment but pass a service account key in production. I didn't realize you aren't able to test refresh token validity in the CI environment until I read the other tests better. I've rewritten the test to use Sinon and mock the readFileSync in the case where there is no refresh token present during the test. I've rebased into a single commit. |
When the GOOGLE_APPLICATION_CREDENTIALS environment variable is pointed to the refresh token file created by 'gcloud auth application-default login', Firebase admin would error as it tried to parse it as a certificate. This fix doesn't attempt to parse the file as a certificate if the variable points to the refresh token file and instead just attempts refresh token file parsing
hiranya911
left a comment
There was a problem hiding this comment.
Couple of changes suggested. Also please update the CHANGELOG file.
src/auth/credential.ts
Outdated
|
|
||
| constructor(httpAgent?: Agent) { | ||
| if (process.env.GOOGLE_APPLICATION_CREDENTIALS) { | ||
| if (process.env.GOOGLE_APPLICATION_CREDENTIALS && |
There was a problem hiding this comment.
If we are going to implement this we should be consistent with other languages. Specifically, there's no reason to special case GCLOUD_CREDENTIALS_PATH. Rather, we should do something like:
if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
const fileContent = readFile(process.env.GOOGLE_APPLICATION_CREDENTIALS);
if (fileContent.type === 'authorized_user') {
// create refresh token credential
} else if (fileContent.type === 'service_account') {
// create cert credential
} else {
// throw error
}
}
test/unit/auth/credential.spec.ts
Outdated
| it('should parse valid token if app def creds point to default refresh token loc', () => { | ||
| process.env.GOOGLE_APPLICATION_CREDENTIALS = GCLOUD_CREDENTIAL_PATH; | ||
|
|
||
| let readFileSyncStub; |
There was a problem hiding this comment.
There's an fsStub variable already in scope for these tests. See if we can use that here.
|
Updated. Quite a bit more code to handle that and still keep all existing test cases passing but I did it and added a few more to cover the other code paths I added. |
hiranya911
left a comment
There was a problem hiding this comment.
I think this can be simplified, if we don't try to merge the existing logic in the constructor with the new logic. See my comments.
src/auth/credential.ts
Outdated
| if (refreshToken) { | ||
| this.credential_ = new RefreshTokenCredential(refreshToken, httpAgent); | ||
| return; | ||
| if (typeof process.env.GOOGLE_APPLICATION_CREDENTIALS === 'string' && |
There was a problem hiding this comment.
This doesn't need to be this complex. You only have to modify the first if block in the existing constructor. Rest of the constructor can remain unchanged. Something like:
constructor(httpAgent?: Agent) {
if (process.env.GOOGLE_APPLICATION_CREDENTIALS) {
this.credential_ = credentialFromFile(process.env.GOOGLE_APPLICATION_CREDENTIALS, httpAgent);
return;
}
// Rest is unchanged
}
function credentialFromFile(filePath: string, httpAgent?: Agent): Credential {
const credentialsFile = readCredentialFile(filePath);
if (typeof credentialsFile !== 'object') {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Failed to parse contents of the credentials file as an object',
);
}
if (credentialsFile.type === 'service_account') {
return new CertCredential(credentialsFile, httpAgent);
}
if (credentialsFile.type === 'authorized_user') {
return new RefreshTokenCredential(credentialsFile, httpAgent);
}
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Invalid contents in the credentials file',
);
}
function readCredentialFile(filePath: string): {[key: string]: any} {
if (typeof filePath !== 'string') {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Failed to parse credentials file: TypeError: path must be a string',
);
}
let fileText: string;
try {
fileText = fs.readFileSync(filePath, 'utf8');
} catch (error) {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
`Failed to read credentials from file ${filePath}: ` + error,
);
}
try {
return JSON.parse(fileText);
} catch (error) {
throw new FirebaseAppError(
AppErrorCodes.INVALID_CREDENTIAL,
'Failed to parse contents of the credentials file as an object: ' + error,
);
}
}
test/unit/auth/credential.spec.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should parse valid token if app def creds point to default refresh token loc', () => { |
There was a problem hiding this comment.
Lets spell out words in full: should create a RefreshTokenCredential if application default credentials variable point to gcloud credentials path.
|
Changes made as requested |
hiranya911
left a comment
There was a problem hiding this comment.
Thanks @yinzara. The Code LGTM 👍 . Please address the 2 comments I've left on tests, and also update the CHANGELOG file, and then I can merge.
When the GOOGLE_APPLICATION_CREDENTIALS environment variable is pointed to
the refresh token file created by 'gcloud auth application-default login',
Firebase admin would error as it tried to parse it as a certificate.
This fix doesn't attempt to parse the file as a certificate if the
variable points to the refresh token file and instead just attempts
refresh token file parsing. Test case added, ran, and verified. Eslint rules passed.