[Auth] Fix error typing and bring MFA error into alignment with the current API standard (#5616)

sam-gc · PabloLION · kevinthecheung · web-flow · commit 69ff8eb549e4 · 2021-10-19T12:38:15.000-07:00
* fix interface AuthError (#5609) * fix interface AuthError change interface AuthError to correct structure * remove `declare` * Refactor MFA error to properly follow the new format * Add changeset * Add doc comment to MultiFactorError.customData * Apply suggestions from code review Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com> * Update changeset to patch Co-authored-by: Pablion <36828324+Pablion@users.noreply.github.com> Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com>
diff --git a/.changeset/fresh-ways-think.md b/.changeset/fresh-ways-think.md
@@ -0,0 +1,5 @@
+---
+"@firebase/auth": patch
+---
+
+Fix the public `AuthError` typing, and update the `MultiFactorError` implementation to follow the new standard (all fields listed under `customData`)
diff --git a/common/api-review/auth.api.md b/common/api-review/auth.api.md
@@ -120,10 +120,12 @@ export class AuthCredential {
 
 // @public
 export interface AuthError extends FirebaseError {
-    readonly appName: string;
-    readonly email?: string;
-    readonly phoneNumber?: string;
-    readonly tenantid?: string;
+    readonly customData: {
+        readonly appName: string;
+        readonly email?: string;
+        readonly phoneNumber?: string;
+        readonly tenantId?: string;
+    };
 }
 
 // @public
@@ -443,7 +445,9 @@ export interface MultiFactorAssertion {
 
 // @public
 export interface MultiFactorError extends AuthError {
-    readonly operationType: typeof OperationType[keyof typeof OperationType];
+    readonly customData: AuthError['customData'] & {
+        readonly operationType: typeof OperationType[keyof typeof OperationType];
+    };
 }
 
 // @public
diff --git a/packages/auth/src/api/index.ts b/packages/auth/src/api/index.ts
@@ -197,7 +197,7 @@ export async function _performSignInRequest<T, V extends IdTokenResponse>(
   )) as V;
   if ('mfaPendingCredential' in serverResponse) {
     _fail(auth, AuthErrorCode.MFA_REQUIRED, {
-      serverResponse
+      _serverResponse: serverResponse
     });
   }
 
diff --git a/packages/auth/src/core/errors.ts b/packages/auth/src/core/errors.ts
@@ -398,7 +398,7 @@ export interface NamedErrorParams {
   phoneNumber?: string;
   tenantId?: string;
   user?: User;
-  serverResponse?: object;
+  _serverResponse?: object;
 }
 
 /**
@@ -431,7 +431,7 @@ export interface AuthErrorParams extends GenericAuthErrorParams {
   [AuthErrorCode.NO_AUTH_EVENT]: { appName?: AppName };
   [AuthErrorCode.MFA_REQUIRED]: {
     appName: AppName;
-    serverResponse: IdTokenMfaResponse;
+    _serverResponse: IdTokenMfaResponse;
   };
   [AuthErrorCode.INVALID_CORDOVA_CONFIGURATION]: {
     appName: AppName;
diff --git a/packages/auth/src/core/strategies/credential.test.ts b/packages/auth/src/core/strategies/credential.test.ts
@@ -139,16 +139,15 @@ describe('core/strategies/credential', () => {
       stub(authCredential, '_getIdTokenResponse').returns(
         Promise.reject(
           _createError(auth, AuthErrorCode.MFA_REQUIRED, {
-            serverResponse
+            _serverResponse: serverResponse
           })
         )
       );
       const error = await expect(
         signInWithCredential(auth, authCredential)
       ).to.be.rejectedWith(MultiFactorError);
-      expect(error.operationType).to.eq(OperationType.SIGN_IN);
-      expect(error.serverResponse).to.eql(serverResponse);
-      expect(error.user).to.be.undefined;
+      expect(error.customData.operationType).to.eq(OperationType.SIGN_IN);
+      expect(error.customData._serverResponse).to.eql(serverResponse);
     });
   });
 
diff --git a/packages/auth/src/core/user/reauthenticate.test.ts b/packages/auth/src/core/user/reauthenticate.test.ts
@@ -148,16 +148,15 @@ describe('core/user/reauthenticate', () => {
     stub(credential, '_getReauthenticationResolver').returns(
       Promise.reject(
         _createError(user.auth, AuthErrorCode.MFA_REQUIRED, {
-          serverResponse
+          _serverResponse: serverResponse
         })
       )
     );
     const error = await expect(
       _reauthenticate(user, credential)
     ).to.be.rejectedWith(MultiFactorError);
-    expect(error.operationType).to.eq(OperationType.REAUTHENTICATE);
-    expect(error.serverResponse).to.eql(serverResponse);
-    expect(error.user).to.eq(user);
+    expect(error.customData.operationType).to.eq(OperationType.REAUTHENTICATE);
+    expect(error.customData._serverResponse).to.eql(serverResponse);
   });
 
   it('should return a valid user credential', async () => {
diff --git a/packages/auth/src/mfa/mfa_error.ts b/packages/auth/src/mfa/mfa_error.ts
@@ -25,16 +25,15 @@ import { AuthCredential } from '../core/credentials';
 import { IdTokenMfaResponse } from '../api/authentication/mfa';
 import { OperationType } from '../model/enums';
 
+export type MultiFactorErrorData = MultiFactorErrorPublic['customData'] & {
+  _serverResponse: IdTokenMfaResponse;
+};
+
 export class MultiFactorError
   extends FirebaseError
   implements MultiFactorErrorPublic
 {
-  readonly name = 'FirebaseError';
-  readonly code: string;
-  readonly appName: string;
-  readonly serverResponse: IdTokenMfaResponse;
-
-  readonly tenantId?: string;
+  readonly customData: MultiFactorErrorData;
 
   private constructor(
     auth: AuthInternal,
@@ -45,11 +44,12 @@ export class MultiFactorError
     super(error.code, error.message);
     // https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work
     Object.setPrototypeOf(this, MultiFactorError.prototype);
-    this.appName = auth.name;
-    this.code = error.code;
-    this.tenantId = auth.tenantId ?? undefined;
-    this.serverResponse = error.customData!
-      .serverResponse as IdTokenMfaResponse;
+    this.customData = {
+      appName: auth.name,
+      tenantId: auth.tenantId ?? undefined,
+      _serverResponse: error.customData!._serverResponse as IdTokenMfaResponse,
+      operationType,
+    };
   }
 
   static _fromErrorAndOperation(
diff --git a/packages/auth/src/mfa/mfa_resolver.test.ts b/packages/auth/src/mfa/mfa_resolver.test.ts
@@ -54,7 +54,7 @@ describe('core/mfa/mfa_resolver/MultiFactorResolver', () => {
     auth = await testAuth();
     auth.tenantId = 'tenant-id';
     underlyingError = _createError(auth, AuthErrorCode.MFA_REQUIRED, {
-      serverResponse: {
+      _serverResponse: {
         localId: 'local-id',
         mfaPendingCredential: 'mfa-pending-credential',
         mfaInfo: [
diff --git a/packages/auth/src/mfa/mfa_resolver.ts b/packages/auth/src/mfa/mfa_resolver.ts
@@ -49,17 +49,18 @@ export class MultiFactorResolverImpl implements MultiFactorResolver {
     error: MultiFactorErrorInternal
   ): MultiFactorResolverImpl {
     const auth = _castAuth(authExtern);
-    const hints = (error.serverResponse.mfaInfo || []).map(enrollment =>
+    const serverResponse = error.customData._serverResponse;
+    const hints = (serverResponse.mfaInfo || []).map(enrollment =>
       MultiFactorInfoImpl._fromServerResponse(auth, enrollment)
     );
 
     _assert(
-      error.serverResponse.mfaPendingCredential,
+      serverResponse.mfaPendingCredential,
       auth,
       AuthErrorCode.INTERNAL_ERROR
     );
     const session = MultiFactorSessionImpl._fromMfaPendingCredential(
-      error.serverResponse.mfaPendingCredential
+      serverResponse.mfaPendingCredential
     );
 
     return new MultiFactorResolverImpl(
@@ -70,12 +71,12 @@ export class MultiFactorResolverImpl implements MultiFactorResolver {
       ): Promise<UserCredentialInternal> => {
         const mfaResponse = await assertion._process(auth, session);
         // Clear out the unneeded fields from the old login response
-        delete error.serverResponse.mfaInfo;
-        delete error.serverResponse.mfaPendingCredential;
+        delete serverResponse.mfaInfo;
+        delete serverResponse.mfaPendingCredential;
 
         // Use in the new token & refresh token in the old response
         const idTokenResponse = {
-          ...error.serverResponse,
+          ...serverResponse,
           idToken: mfaResponse.idToken,
           refreshToken: mfaResponse.refreshToken
         };
@@ -129,9 +130,9 @@ export function getMultiFactorResolver(
 ): MultiFactorResolver {
   const authModular = getModularInstance(auth);
   const errorInternal = error as MultiFactorErrorInternal;
-  _assert(error.operationType, authModular, AuthErrorCode.ARGUMENT_ERROR);
+  _assert(error.customData.operationType, authModular, AuthErrorCode.ARGUMENT_ERROR);
   _assert(
-    errorInternal.serverResponse?.mfaPendingCredential,
+    errorInternal.customData._serverResponse?.mfaPendingCredential,
     authModular,
     AuthErrorCode.ARGUMENT_ERROR
   );
diff --git a/packages/auth/src/model/public_types.ts b/packages/auth/src/model/public_types.ts
@@ -121,21 +121,24 @@ export type NextOrObserver<T> = NextFn<T | null> | Observer<T | null>;
  * @public
  */
 export interface AuthError extends FirebaseError {
-  /** The name of the Firebase App which triggered this error.  */
-  readonly appName: string;
-  /** The email of the user's account, used for sign-in/linking. */
-  readonly email?: string;
-  /** The phone number of the user's account, used for sign-in/linking. */
-  readonly phoneNumber?: string;
-  /**
-   * The tenant ID being used for sign-in/linking.
-   *
-   * @remarks
-   * If you use {@link signInWithRedirect} to sign in,
-   * you have to set the tenant ID on {@link Auth} instance again as the tenant ID is not persisted
-   * after redirection.
-   */
-  readonly tenantid?: string;
+  /** Details about the Firebase Auth error.  */
+  readonly customData: {
+    /** The name of the Firebase App which triggered this error.  */
+    readonly appName: string;
+    /** The email address of the user's account, used for sign-in and linking. */
+    readonly email?: string;
+    /** The phone number of the user's account, used for sign-in and linking. */
+    readonly phoneNumber?: string;
+    /**
+     * The tenant ID being used for sign-in and linking.
+     *
+     * @remarks
+     * If you use {@link signInWithRedirect} to sign in,
+     * you have to set the tenant ID on the {@link Auth} instance again as the tenant ID is not persisted
+     * after redirection.
+     */
+    readonly tenantId?: string;
+  };
 }
 
 /**
@@ -599,11 +602,14 @@ export interface MultiFactorAssertion {
  *
  * @public
  */
-export interface MultiFactorError extends AuthError {
-  /**
-   * The type of operation (e.g., sign-in, link, or reauthenticate) during which the error was raised.
-   */
-  readonly operationType: typeof OperationTypeMap[keyof typeof OperationTypeMap];
+ export interface MultiFactorError extends AuthError {
+   /** Details about the MultiFactorError. */
+  readonly customData: AuthError['customData'] & {
+    /**
+     * The type of operation (sign-in, linking, or re-authentication) that raised the error.
+     */
+    readonly operationType: typeof OperationTypeMap[keyof typeof OperationTypeMap];
+  }
 }
 
 /**

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@firebase/auth": patch
 +---
++
 +Fix the public `AuthError` typing, and update the `MultiFactorError` implementation to follow the new standard (all fields listed under `customData`)
Original file line number	Diff line number	Diff line change
`@@ -197,7 +197,7 @@ export async function _performSignInRequest<T, V extends IdTokenResponse>(`
`197`	`197`	`)) as V;`
`198`	`198`	`if ('mfaPendingCredential' in serverResponse) {`
`199`	`199`	`_fail(auth, AuthErrorCode.MFA_REQUIRED, {`
`200`		`- serverResponse`
	`200`	`+ _serverResponse: serverResponse`
`201`	`201`	`});`
`202`	`202`	`}`
`203`	`203`