Fix type for cert(...) function, to support standard downloadable serviceAccount.json

[charles-allen]

Motivation

The JavaScript implementation of cert(...) already supports snake_case account objects. I suspect this is the most popular format (for non-Cloud-Function usage) since GCP & Firebase console provide downloadable account configs as snake_case.

Every single TypeScript client of the Node Admin SDK must navigate this type error, which occurs at the very start of their SDK journey. It has already been raised as an issue 3 separate times. It's a trivial fix that increases the accuracy & readability of the codebase.

Issues

• Resolves https://github.com/firebase/firebase-admin-node/issues/522
• Resolves https://github.com/firebase/firebase-admin-node/issues/624
• Resolves https://github.com/firebase/firebase-admin-node/issues/903

Testing

• Make sure all existing tests pass => crosses fingers
• If you fixed a bug ... add a new test => n/a (compile-time)

API Changes

• This isn't a change; rather the API is already supporting snake_case and the TypeScript description is simply deficient

[google-cla[bot]]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[charles-allen]

@googlebot I signed it!

[google-cla[bot]]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[charles-allen]

@googlebot I fixed it

[hiranya911]

Been testing this solution and seems alright to me. I was a bit concerned about tsc potentially flagging an object that has more properties than the declared interface (since service account json contains many other fields), but that doesn't seem to be the case. It still needs to go through our internal API review process. I'll find some time for that in the coming weeks.

[charles-allen]

Hi @hiranya911,

I PR'd this implementation because it's a mirror of the existing camelCase interface (and I thought that would be the smallest possible increment).

More complete & explicit

If you're willing or even prefer to make a larger change, I think the best option is an interface that explicitly matches the expectations of the API (i.e everything that's required from a snake_case JSON would be marked required, and all the optional properties would be listed & explicitly marked optional):

export interface ServiceAccountJson {
  // I think these are all the required fields
  client_email: string // required
  private_key: string // required
  project_id: string // required
  // I think these are all the optional fields
  auth_provider_x509_cert_url?: string
  auth_uri?: string
  client_id?: string
  client_x509_cert_url?: string
  private_key_id?: string
  token_uri?: string
  type?: string
}

Other options

You could also use an index signature, but this opens the door for typos in any optional property (typos in required properties will get caught); personally I am not a fan:

export interface ServiceAccountJson {
  project_id: string // required
  private_key: string // required
  client_email: string // required
  [additional: string]: string // optional
}

Internal usage

If you need different "optional" status internally, you can do a lot with TypeScript:

// Example: types for merging passed JSON with default values
type ServiceAccountJsonDefaults = Required<Pick<ServiceAccountJson, 'type' | 'client_id'>>
type ServiceAccountJsonMerged = ServiceAccountJson & ServiceAccountJsonDefaults

const defaults: ServiceAccountJsonDefaults = { type: '...', client_id: '...' };
const merged: ServiceAccountMerged = Object.assign(defaults, serviceAccount);

Testing

I wrote previously that I couldn't write a test, but actually I think we could add a small dummy TypeScript program that imports the SDK (like the cases below), and add a step to the GitHub actions workflow to compile that program (compilation failure would fail the build).

You mentioned:

I was a bit concerned about tsc potentially flagging an object that has more properties than the declared interface (since service account json contains many other fields)

From the TypeScript docs - TL;DR: the "additional property check" is only when assigning/passing an object literal:

Object literals get special treatment and undergo excess property checking when assigning them to other variables, or passing them as arguments. If an object literal has any properties that the “target type” doesn’t have, you’ll get an error

There might be cases where a client constructs their config inline (e.g. case 3) where the currently proposed PR might fail as you described (I believe the more complete interface at the top of this comment would pass):

// case 1: static import
// - type inferred from JSON itself => checked structurally vs the interface
// - not passing a literal => excess property check skipped
import serviceAccount from 'service-account.json'
admin.initializeApp({ credential: admin.credential.cert(serviceAccount) })

// case 2: dynamic import
// - type asserted => actual JSON is never checked
// - not passing a literal => excess property check skipped
const serviceAccount: ServiceAccountJson = await import(configPath) // or require(...)
admin.initializeApp({ credential: admin.credential.cert(serviceAccount) })

// case 3: inline literal
// - type inferred from the literal => checked structurally vs the interface
// - passing a literal => excess property check might fail
admin.initializeApp({ credential: admin.credential.cert({
  project_id = process.env.FIREBASE_PROJECT_ID,
  private_key = process.env.FIREBASE_PRIVATE_KEY,
  client_email = process.env.FIREBASE_PROJECT_ID,
  another_property = process.env.FIREBASE_ANOTHER_PROPERTY // might fail on this line
}) })

[hiranya911]

I wrote previously that I couldn't write a test, but actually I think we could add a small dummy TypeScript program that imports the SDK (like the cases below), and add a step to the GitHub actions workflow to compile that program (compilation failure would fail the build).

I think you can (and should) add some unit tests to the existing test suite. Something similar to:

https://github.com/firebase/firebase-admin-node/blob/1862342636e816b61337262254cdfe0cc410c640/test/unit/firebase.spec.ts#L114-L117

You can convert the mocks.certificateObject into an instance of ServiceAccountJson and pass that into cert.

[charles-allen]

I think you can (and should) add some unit tests to the existing test suite. Something similar to: ...

I'll see if I can add something along those lines that verifies a snake_case config can be imported in all the standard ways, passed to cert, and most importantly:

1. that tsc is happy about it (the tests will fail the build step if this isn't the case)
2. that cover the excess property case

[charles-allen]

Sorry for the delay. Busy with work. I should have time to revisit this this week :)

[paul-uz]

@charles-allen any update on this? I encountered this issue just this week and it was bugging the heck out of me, pardon the pun!

[charles-allen]

@charles-allen any update on this? I encountered this issue just this week and it was bugging the heck out of me, pardon the pun!

I got sidetracked by work. But the next 2 weeks is the perfect time for me to revive it. I think I just need to copy the existing tests and swap camel for snake case.

Can't offer any guarantees on how much more time it would take to get accepted though.

[paul-uz]

@charles-allen no worries :)

[charles-allen]

@charles-allen no worries :)

Thanks for bumping it :) Good to get in back in my peripheral vision!

[tylim88]

it's been almost 2 years, any update?

[charles-allen]

it's been almost 2 years, any update?

I'm not working with Firebase any more, and I think Hiranya has left the Firebase team. This PR is probably horribly out-of-date. You're welcome to replace it with a new PR :)