firebase / firebase-js-sdk

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

Deleting a user which is no longer the current user seems to trigger onAuthStateChanges now. #5705

Closed jaunt closed 2 years ago

jaunt commented 2 years ago

Browser version: Chrome Version 95.0.4638.69 Firebase SDK version: 9.2 Compatibility Mode Firebase UI version: 6.0

This is in regards to handling anonymous-upgrade-merge-conflict in firebase UI.

Before I updated to 9.2 compatibility mode, I was on firebaseui 5.0 and firebase 8.x. Deleting the anon user as shown in the code below did so silently (onAuthStateChanged was not called). This is makes sense as the old "merged" user has been logged in at that point.

After updating to 6.0 and 9.2, when anonymousUser.delete() is called, it actually logs out the no longer anonymous real user, since onAuthStateChanged is called with user==null and that triggers my entire sign-out logic.

Is this expected behaviour? If so I'm not sure how I can delete the unneeded anon user in this case.

The code below is based on the firebase UI example and was working fine until I upgraded to 9.2 compatibility mode.

       signInFailure: (error) => {
            // For merge conflicts, the error.code will be
            // 'firebaseui/anonymous-upgrade-merge-conflict'.
            if (
              error.code != "firebaseui/anonymous-upgrade-merge-conflict"
            ) {
              return Promise.resolve();
            }

            // The credential the user tried to sign in with.
            let cred = error.credential;
            let anonymousUser = firebase.auth().currentUser;
            let newUser;

            return firebase
              .auth()
              .signInWithCredential(cred)
              .then(function (data) {
                newUser = data.user;
                me.signInComplete(newUser, {
                  upgraded: false,
                  merged: true,
                });
                anonymousUser.delete(); // causes new user to be signed out!
              })
google-oss-bot commented 2 years ago

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

jbalidiong commented 2 years ago

Hi @jaunt, thanks for the report. I was able to reproduce the behavior now. Let me check what we can do for this issue or bring someone here that can provide more context about it. I’ll update this thread if I have any information to share.

jbalidiong commented 2 years ago

Upon further checking, it seems that your code has incomplete implementation when upgrading an anonymous user account to a permanent user account. If we follow the code snippet, it will log in the authenticated user using Firebase UI, however it will not copy the data from the anonymous user account to the permanent user account. You may validate it using the User UID. If the anonymous user account is properly upgraded to a permanent user account, the UID should not change.

Here is a snippet for v8:

signInFailure: function (error) {
          // Temp variable to hold the anonymous user data if needed.
          let data = null;
          // Hold a reference to the anonymous current user.
          let anonymousUser = firebase.auth().currentUser;
          // For merge conflicts, the error.code will be
          // 'firebaseui/anonymous-upgrade-merge-conflict'.
          if (error.code != 'firebaseui/anonymous-upgrade-merge-conflict') {
            return Promise.resolve();
          }
          // The credential the user tried to sign in with.
          var cred = error.credential;
          // Copy data from anonymous user to permanent user and delete anonymous
          // user.
          anonymousUser.linkWithCredential(cred);
          // Finish sign-in after data is copied.
          return firebase
            .auth()
            .signInWithCredential(cred)
        }

For v9 compat:

signInFailure: function (error) {
          // Temp variable to hold the anonymous user data if needed.
          let data = null;

          console.log(error)
          // Hold a reference to the anonymous current user.
          let anonymousUser = firebaseAuth.currentUser;
          // For merge conflicts, the error.code will be
          // 'firebaseui/anonymous-upgrade-merge-conflict'.
          if (error.code != 'firebaseui/anonymous-upgrade-merge-conflict') {
            return Promise.resolve();
          }
          // The credential the user tried to sign in with.
          var cred = error.credential;
          // Copy data from anonymous user to permanent user and delete anonymous
          // user.
          anonymousUser.linkWithCredential(cred)
          // Finish sign-in after data is copied. 
          return firebaseAuth.signInWithCredential(cred)
          .then(anonymousUser.delete())
        }

With the code snippet, you should properly upgrade your anonymous user.

jaunt commented 2 years ago

@jbalidiong Ok thanks. I find that very confusing as this is an anonymous user, so why would I want to link their credential? I was under the impression that that function was meant for linking say email with google or facebook etc. If this is the correct behaviour, I'd suggest firebaseUI needs to update their example, as it doesn't do what you are suggesting.

https://github.com/firebase/firebaseui-web#upgrading-anonymous-users

jbalidiong commented 2 years ago

Hi @jaunt, apologies there might be some confusion. Could you share more details of the use-case for this issue? Also, please provide a minimal but complete example to run it locally.

jaunt commented 2 years ago

@jbalidiong No worries at all. I'm probably doing something wrong!

Anyway, the use-case is pretty much exactly as described in the firebaseui-web example I posted in the last message.

"When an anonymous user signs in or signs up with a permanent account, you want to be sure the user can continue with what they were doing before signing up."

I notice in their docs, they do say they use linkWithCredential if you set autoUpgradeAnonymousUsers: true, which I am doing.

The difference I'm seeing between firebase 8 and 9 is that now when I try to delete the old anon user, it logs the current user out. But maybe I don't need to delete that user?

I'll see if I can carve out some time to make an example but might take some time.

Thanks for the help.

jbalidiong commented 2 years ago

@jaunt, I've adjusted the code (following the implementation in the documentation). I removed the additional linkWithCredential() since it's already doing the linking using Firebase UI and remove the delete(). Since Firebase UI used linkWithCredential(), your anonymous user will be upgraded to a permanent account.

Because it is the same user (anonymous linked to a permanent account), you don't need to use delete(). If you'll check the Authentication console, you won't find the anonymous user. However, if you'll track the User UID, the anonymous UID now has an identifier (the credentials you previously used to link).

jaunt commented 2 years ago

@jbalidiong Great, that makes sense. I'll update my code and let you know if anything is unexpected. I bet it will be fine. Thanks again.

jbalidiong commented 2 years ago

I'm going to close this issue out for now, but if you have new findings please feel free to comment/reopen.