firebase / firebase-js-sdk

Firebase Javascript SDK
https://firebase.google.com/docs/web/setup
Other
4.86k stars 891 forks source link

multiFactor throws internal error when reloadListener exists on user. #7709

Open hoongtong opened 1 year ago

hoongtong commented 1 year ago

Operating System

macOS 12.6

Browser Version

Chrome/117.0.5938.149

Firebase SDK Version

10.5.0

Firebase SDK Product:

Auth, Database, Firestore

Describe your project's tooling

CRACO react app

Describe the problem

During the upgrade from v8.0.0 to v10.5.0 we also migrated to the modular api. In our application we (partially) enrolled multi-factor authentication and therefore need to check if the user has enrolledFactors and show the enrollment flow when applicable.

However, we encountered that the previous check to get the enrolledFactors were failing after the migration. Prior to the migration the enrolled factors were obtained via user.multiFactor.enrolledFactors. This is equivalent to multiFactor(user).enrolledFactors. But there is an assertion that checks whether that the reloadListener on the user object is null. Why is the assertion there? It means you cannot check the multiFactor once the reloadListener is set or we have to clear it ourselves.

The internal error:

assert.ts:136 Uncaught FirebaseError: Firebase: Error (auth/internal-error).
    at createErrorInternal (assert.ts:136:55)
    at _assert (assert.ts:167:11)
    at UserImpl._onReload (user_impl.ts:154:5)
    at new MultiFactorUserImpl (mfa_user.ts:37:10)
    at MultiFactorUserImpl._fromUser (mfa_user.ts:47:12)
    at multiFactor (mfa_user.ts:121:27)

Steps and code to reproduce issue

When we update claims on the user, we force a refresh of the JWT:

 await firebaseUser.getIdTokenResult(true);
 setFirebaseUser(_.clone(firebaseUser));

This updates the local state firebaseUser and following check fails as user has a reloadListener.

  const userHasMultifactorEnabled = useMemo((): boolean => {
    return firebaseUser ? multiFactor(firebaseUser).enrolledFactors.length > 0 : false;
  }, [firebaseUser]);

Do we need to manually clear the reloadListener or check on reloadListener in userHasMultifactorEnabled? Can we get some clarification on the assertion?

Workaround:

  const userHasMultifactorEnabled = useMemo((): boolean => {
    try {
      return firebaseUser ? multiFactor(firebaseUser).enrolledFactors.length > 0 : false;
    } catch (error) {
      return true;
    }
  }, [firebaseUser]);
bhparijat commented 1 year ago

The reloadListener ensures that the latest MFA enrollment information is reflected on the user. Could you explain why it's required to clone the user and is it possible to reuse the same user instance?

philly-vanilly commented 8 months ago

@bhparijat

The reloadListener ensures that the latest MFA enrollment information is reflected on the user

how exactly is it ensuring that? by throwing a generic "internal" error? My code looks like this:

import { getAuth, multiFactor } from "firebase/auth";
...
this.user = getAuth(this.firebaseApp).currentUser;
this.multiFactorUser = multiFactor(this.user);

I am not cloning anything. I also did not upgrade firebase, as I am still on version 9.15 What I did, however, was update the app setup over the course of 3 months and it is impossible to find the relevant change with an error description like that.

philly-vanilly commented 8 months ago

Updating to firebase 10, still the same problem:

FirebaseError: Firebase: Error (auth/internal-error).
    at createErrorInternal (firebase_auth.js?v=1794b601:695:37)
    at _assert (firebase_auth.js?v=1794b601:701:11)
    at Proxy._onReload (firebase_auth.js?v=1794b601:1683:5)
    at new _MultiFactorUserImpl (firebase_auth.js?v=1794b601:4828:10)
    at _MultiFactorUserImpl._fromUser (firebase_auth.js?v=1794b601:4835:12)
    at multiFactor (firebase_auth.js?v=1794b601:4867:63)

Upon debugging _onReload in the browser I see that this.reloadListener (where "this" is a _UserImpl) is null. I assume this causes the issue? What exactly could cause the reloadListener to be null? I am not setting it anywhere manually in my app. When it worked, many commits ago, the reloadListener at the same spot was resolved to

(userInfo) => {
      if (userInfo.mfaInfo) {
        this.enrolledFactors = userInfo.mfaInfo.map((enrollment) => MultiFactorInfoImpl._fromServerResponse(user.auth, enrollment));
      }
    }

This is some code from firebase, I have no idea how it is attached to the user object.

Zachacious commented 8 months ago

I was able to work around this by manually setting the reloadListener field of the User to null before passing it to multiFactor. For the moment, everything seems to work as expected.