feat: Fix impersonated service account parsing exception

[blue-hope]

Discussion

• related to [FR] Is it possible to use impersonated service account? #1861

Testing

• test for checking impersonated service account added.

API Changes

• no change on public API

RELEASE NOTE: Added support for initializing the SDK with service account impersonation in Application Default Credentials.

[blue-hope]

Can I get any feedback on this? Or discussion on #1861 ?

[nathan-stable-auto]

@blue-hope thanks for the PR!

@lahirumaramba I notice you self-assigned the related issue #1703 - would you be able provide any input on how to move this forward? Thanks very much!

[blue-hope]

@lahirumaramba
I hope this feature will be dealt with soon.
I would be so appreciated if you could give us a feedback or review.

[lahirumaramba]

Thank you everyone for your patience! Hey @blue-hope Thank you so much for the contribution! Just adding this note to let you know that I have started the review process. We also plan to migrate the credentials implementation to google-auth-library-nodejs (see #1377) and once that is done firebase-admin will reach feature parity with google-auth-library-nodejs. In the meantime, I think it is reasonable to merge this change.

[blue-hope]

Thank you so much for reviewing.
(+ Just curious, when will that google-auth-library-nodejs implementation be finished?)

[IchordeDionysos]

@lahirumaramba just a side-note on the migration to google-auth-library-nodejs:
Right now, it's possible to do the following using a local service account:

Run: gcloud auth application-default login

initializeFirebase({project: 'project'});

getFirestore().doc('foo/bar').get();

However, trying to authenticate with Identity Toolkit / Firebase Auth would fail:

initializeFirebase({project: 'project'});

getAuth().getUser('uid'); // -> throws an error

Would the migration to google-auth-library-nodejs library fix this problem?

[lahirumaramba]

Hi @blue-hope, Thank you again for your contributions. It looks pretty good overall... I think we can improve the test coverage. Added a few comments and one question. Please take a look. Thanks!

[lahirumaramba]

Correct me if I am wrong here, but it looks like by setting ignoreMissing we turn off the Failed to read credentials from file error. I think if GOOGLE_APPLICATION_CREDENTIALS is set and getApplicationDefault() is called we should still throw if it is an incorrect file path, right?

[blue-hope]

Sure, so I passed ignoreMissing as false when check GOOGLE_APPLICATION_CREDENTIALS on line 486. So we still can encounter FirebaseAppError when an incorrect file path is passed.

[lahirumaramba]

Let's add another test to httpAgent:

  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);
    });
  });

[lahirumaramba]

Actually, I think we can improve the test coverage for ImpersonatedServiceAccountCredential. Please see describe('RefreshTokenCredential', () => { for an example.

Let's also add another test in describe('isApplicationDefault()', () => {

  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 false for explicitly loaded ImpersonatedServiceAccountCredential', () => {
    const c = new ImpersonatedServiceAccountCredential(mockImpersonatedAccount);
    expect(isApplicationDefault(c)).to.be.false;
  });

[lahirumaramba]

Thank you so much for reviewing. (+ Just curious, when will that google-auth-library-nodejs implementation be finished?)

I can't promise a timeline for this, but we are starting the initial work this year. It will be a bit complex migration so we plan to perform it in multiple stages. We will use #1377 to track any progress.

[lahirumaramba]

@lahirumaramba just a side-note on the migration to google-auth-library-nodejs: Right now, it's possible to do the following using a local service account:

Run: gcloud auth application-default login

initializeFirebase({project: 'project'});

getFirestore().doc('foo/bar').get();

However, trying to authenticate with Identity Toolkit / Firebase Auth would fail:

initializeFirebase({project: 'project'});

getAuth().getUser('uid'); // -> throws an error

Would the migration to google-auth-library-nodejs library fix this problem?

Hi @IchordeDionysos, This looks a bit strange. gcloud auth application-default login should set GCLOUD_CREDENTIAL_PATH and load ADC. If this is reproducible, please file a new issue with a code sample and any error logs. Thanks!

[lahirumaramba]

Thanks for addressing the review feedback! LGTM!
Let me know if you can add the missing unit tests. As an alternative, we can merge this as it is and I am happy to add the missing tests in a follow-up PR.

[blue-hope]

@lahirumaramba I'll add missing tests and notice to you before merge, thanks!

[blue-hope]

@lahirumaramba New test added. I missed checking implicit discovered credential, thanks a lot.

[lahirumaramba]

Thank you @blue-hope! LGTM!

[blue-hope]

@lahirumaramba When this will be released? 😄

[Klaitos]

Definitively need this

[lahirumaramba]

Thank you @blue-hope for your contribution. This feature is now included in the v11.5.0 release.

Thanks folks for your patience on this! Try out the new feature if you get a chance and let us know what you think.
If you encounter any issues, please open a new issue on Github. Thank you!