feat: Fix impersonated service account parsing exception #1862

blue-hope · 2022-08-10T16:00:11Z

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 · 2022-08-16T09:49:34Z

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

nathan-stable-auto · 2022-08-31T00:28:09Z

@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 · 2022-10-12T09:11:43Z

@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 · 2022-10-18T15:20:59Z

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 · 2022-10-18T16:20:53Z

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

IchordeDionysos · 2022-11-13T14:02:00Z

@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 · 2022-11-18T19:57:50Z

src/app/credential-internal.ts

-function credentialFromFile(filePath: string, httpAgent?: Agent): Credential {
- const credentialsFile = readCredentialFile(filePath);
+function credentialFromFile(filePath: string, httpAgent?: Agent, ignoreMissing?: boolean): Credential | null {
+ const credentialsFile = readCredentialFile(filePath, ignoreMissing);


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?

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.

test/resources/mock.impersonated_key.json

lahirumaramba · 2022-11-18T20:06:59Z

test/unit/app/credential-internal.spec.ts

+ const c = getApplicationDefault();
+ expect(c).to.be.an.instanceof(ImpersonatedServiceAccountCredential);
+ });
+


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

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 · 2022-11-18T20:19:30Z

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 · 2022-11-18T20:24:01Z

@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 · 2022-11-22T12:44:12Z

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

blue-hope · 2022-11-23T04:41:59Z

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

lahirumaramba

Thank you @blue-hope! LGTM!

blue-hope · 2022-12-21T03:14:00Z

@lahirumaramba When this will be released? 😄

Klaitos · 2023-01-12T14:29:54Z

Definitively need this

lahirumaramba · 2023-01-19T17:44:23Z

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!

fix: Fix impersonated service account parsing exception

5102751

Merge branch 'master' into master

a940307

Merge branch 'master' into master

175edd8

branks approved these changes Oct 4, 2022

View reviewed changes

blue-hope added 2 commits October 6, 2022 13:57

Merge branch 'master' into master

996941e

Merge branch 'master' into master

9bbe729

lahirumaramba self-assigned this Oct 17, 2022

lahirumaramba self-requested a review October 17, 2022 21:52

Merge branch 'master' into master

968225a

blue-hope added 3 commits October 24, 2022 19:37

Merge branch 'master' into master

5c88efd

Merge branch 'master' into master

5d87876

Merge branch 'master' into master

178f906

lahirumaramba requested changes Nov 18, 2022

View reviewed changes

lahirumaramba assigned blue-hope and unassigned lahirumaramba Nov 18, 2022

fix: Add new line

dbf701a

lahirumaramba approved these changes Nov 21, 2022

View reviewed changes

lahirumaramba added the release-note label Nov 21, 2022

lahirumaramba changed the title ~~fix: Fix impersonated service account parsing exception~~ feat: Fix impersonated service account parsing exception Nov 21, 2022

feat: Add test for implicit credential

a8d0c23

Merge branch 'master' into master

bcf2b33

lahirumaramba approved these changes Dec 1, 2022

View reviewed changes

Merge branch 'master' into master

4dff635

Merge branch 'master' into master

42b9c5a

lahirumaramba merged commit c81f572 into firebase:master Jan 13, 2023

This was referenced Jan 18, 2023

chore: Increase test coverage for service account impersonation support #2047

Merged

[chore] Release 11.5.0 #2049

Merged

This was referenced Jan 19, 2023

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

Closed

Logged with gcloud impersonate service account will cause initializeApp error #1703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fix impersonated service account parsing exception #1862

feat: Fix impersonated service account parsing exception #1862

blue-hope commented Aug 10, 2022 •

edited by lahirumaramba

blue-hope commented Aug 16, 2022

nathan-stable-auto commented Aug 31, 2022

blue-hope commented Oct 12, 2022 •

edited

lahirumaramba commented Oct 18, 2022

blue-hope commented Oct 18, 2022

IchordeDionysos commented Nov 13, 2022

lahirumaramba left a comment

lahirumaramba Nov 18, 2022

blue-hope Nov 19, 2022

lahirumaramba Nov 18, 2022

lahirumaramba Nov 18, 2022

lahirumaramba commented Nov 18, 2022 •

edited

lahirumaramba commented Nov 18, 2022

lahirumaramba left a comment

blue-hope commented Nov 22, 2022

blue-hope commented Nov 23, 2022 •

edited

lahirumaramba left a comment

blue-hope commented Dec 21, 2022

Klaitos commented Jan 12, 2023

lahirumaramba commented Jan 19, 2023

feat: Fix impersonated service account parsing exception #1862

feat: Fix impersonated service account parsing exception #1862

Conversation

blue-hope commented Aug 10, 2022 • edited by lahirumaramba

Discussion

Testing

API Changes

blue-hope commented Aug 16, 2022

nathan-stable-auto commented Aug 31, 2022

blue-hope commented Oct 12, 2022 • edited

lahirumaramba commented Oct 18, 2022

blue-hope commented Oct 18, 2022

IchordeDionysos commented Nov 13, 2022

lahirumaramba left a comment

Choose a reason for hiding this comment

lahirumaramba Nov 18, 2022

Choose a reason for hiding this comment

blue-hope Nov 19, 2022

Choose a reason for hiding this comment

lahirumaramba Nov 18, 2022

Choose a reason for hiding this comment

lahirumaramba Nov 18, 2022

Choose a reason for hiding this comment

lahirumaramba commented Nov 18, 2022 • edited

lahirumaramba commented Nov 18, 2022

lahirumaramba left a comment

Choose a reason for hiding this comment

blue-hope commented Nov 22, 2022

blue-hope commented Nov 23, 2022 • edited

lahirumaramba left a comment

Choose a reason for hiding this comment

blue-hope commented Dec 21, 2022

Klaitos commented Jan 12, 2023

lahirumaramba commented Jan 19, 2023

blue-hope commented Aug 10, 2022 •

edited by lahirumaramba

blue-hope commented Oct 12, 2022 •

edited

lahirumaramba commented Nov 18, 2022 •

edited

blue-hope commented Nov 23, 2022 •

edited