Incorrect typing on createUser api in admin.auth leads to broken multi-factor auth

[jakebiesinger-storyhealth]

• Operating System version: Mac OS 11.2.3
• Firebase SDK version: 9.7.0
• Firebase Product: auth
• Node.js version: 12.21.0
• NPM version: 7.7.0

[REQUIRED] Step 3: Describe the problem

Typescript types are incorrect for CreateMultiFactorInfoRequest and CreatePhoneMultiFactorInfoRequest in admin.auth.createUser. I expect to be able to pass in a phone number but am not allowed by the type system. Instead, I have to cast the type as any.

Steps to reproduce:

admin.auth.createUser({
  displayName: 'somebody',
  email: '[email protected]'
  emailVerified: true,
  multiFactor: { enrolledFactors: [{ factorId: 'phone', phoneNumber: request.phoneNumber }] }
}

This yields the error

You can work around this by casting the object to an any, but the type system is still wrong here.

This was also raised in #1048 but for some reason that ticket was closed by the OP.

As a side-note, it feels like the code was written by someone who thought that extending a typescript interface would mean that instances of the derived class can be passed in instead of the base class, but with structural typing, this is not allowed.

[jakebiesinger-storyhealth]

Relevant code from admin sdk is here

firebase-admin-node/src/auth/index.ts

Line 449 in d961c3f

export interface CreateMultiFactorInfoRequest {

... notice that the interface extends

firebase-admin-node/src/auth/index.ts

Line 466 in d961c3f

export interface CreatePhoneMultiFactorInfoRequest extends CreateMultiFactorInfoRequest {

but the base type is the one that's required, not the derived type.

[hiranya911]

Thanks for raising this issue. I've noticed this problem in several places in our API. Here are some:

• CreateMultiFactorInfoRequest vs CreatePhoneMultiFactorInfoRequest
• UpdateMultiFactorInfoRequest vs UpdatePhoneMultiFactorInfoRequest
• AuthProviderConfig vs SAMLAuthProviderConfig vs OIDCAuthProviderConfig

The proposed solution of using union types is probably the right approach.

However, I'd also note that you don't need an any cast to workaround this limitation. You can explicitly type your argument:

const factor: admin.auth.CreatePhoneMultiFactorInfoRequest = {phoneNumber: '', factorId: 'phone'};
admin.auth().createUser({
  multiFactor: {enrolledFactors: [factor]},
});

Or even an in-place cast:

admin.auth().createUser({
  multiFactor: {enrolledFactors: [
    {phoneNumber: '', factorId: 'phone'} as CreatePhoneMultiFactorInfoRequest
  ]},
});

Regardless, it would be nice if we could just pass object literals to these APIs, and have them correctly typed by default.

@bojeil-google wdyt?

[bojeil-google]

I think union types sound good.👍
We just need to make sure not to introduce a breaking change by making that adjustment.

[hiranya911]

How about rearranging our typings like this?

export interface BaseCreateMultiFactorInfoRequest {
  displayName?: string;
}

export interface CreatePhoneMultiFactorInfoRequest extends BaseCreateMultiFactorInfoRequest {
  factorId: 'phone';
  phoneNumber: string;
}

export type CreateMultiFactorInfoRequest = | CreatePhoneMultiFactorInfoRequest;

// Same for update types

This also enforces that the factorId field on CreatePhoneMultiFactorInfoRequest must be set to "phone". Any semantically correct existing code should compile against the new typings, and therefore won't be a breaking change.

[bojeil-google]

How about rearranging our typings like this?

export interface BaseCreateMultiFactorInfoRequest {
  displayName?: string;
}

export interface CreatePhoneMultiFactorInfoRequest extends BaseCreateMultiFactorInfoRequest {
  factorId: 'phone';
  phoneNumber: string;
}

export type CreateMultiFactorInfoRequest = | CreatePhoneMultiFactorInfoRequest;

// Same for update types

This also enforces that the factorId field on CreatePhoneMultiFactorInfoRequest must be set to "phone". Any semantically correct existing code should compile against the new typings, and therefore won't be a breaking change.

I think that looks reasonable.