firebase / firebase-js-sdk

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

FR: please add function Auth.getCurrentUser() as a promise that will return current user or null #462

Closed Thaina closed 6 months ago

Thaina commented 6 years ago

Auth.currentUser object is unreliable to get user from persistence storage. If the authentication system could be pending and don't really have ensure finishing endpoint then you should have a promise for it

So I want to have promise function Auth.getCurrentUser() in addition to onAuthStateChange that would eventually return user but would return null if no user exist in persistence storage or user require sign in from expired token

google-oss-bot commented 6 years ago

Hmmm this issue does not seem to follow the issue template. Make sure you provide all the required information.

bojeil-google commented 6 years ago

You can easily implement that on your own with a couple of lines:

function getCurrentUser(auth) {
  return new Promise((resolve, reject) => {
     const unsubscribe = auth.onAuthStateChanged(user => {
        unsubscribe();
        resolve(user);
     }, reject);
  });
}
Thaina commented 6 years ago

@bojeil-google If the user does not signed in will that promise would ever fired for null?

And I think this function is really common enough to included in the SDK

bojeil-google commented 6 years ago

Yes that promise will fire. onAuthStateChanged always triggers with the initial state even when it is null. That is what you care for here, the current state. After that is determined, you unsubscribe.

Thaina commented 6 years ago

I see but anyway if it really just a couple line and will be the same for everyone so why it not be inside SDK. Did you not consider it useful?

And also that function would fire only one time at first call and will not fire again in second call am I right? What I want from getCurrentUser() function is consistently return null if it already checking in persistence storage, or return the actual current user after that

At least it should have a function like getPreparedAuthStated() or isPreparedAuthFinished

bojeil-google commented 6 years ago

I have no idea what you mean by: " consistently return null if it already checking in persistence storage, or return the actual current user after that".

Thaina commented 6 years ago

@bojeil-google If I don't misunderstand, the onAuthStateChanged in promise function you present will work only on the first call. If I call that function on the second time it will never return because the auth state never change again after that

This is not what I want from getCurrentUser(). I want this function to actively checking that user is not really signed in before. I want it to be promise just in case that firebase auth does not finish initializing yet, in that case the user was signed in before but firebase itself are not ready. So return promise is better solution

What I want is

I want this consistence behavior for getCurrentUser() function

But if I don't misunderstand, the function you present will never resolve if that function was called after firebase auth preparation is finished unless the user try to signed in

Thaina commented 6 years ago

If I misunderstand above, which means onAuthStateChanged will always fire once whenever it get called for the current user without the need of signIn or signOut action. Then I would like to request that this behavior should be mentioned in the document

bojeil-google commented 6 years ago

Please read the documentation, you presume some incorrect assumptions. If we were to implement a promise that returns the current user, it will be done as I described. onAuthStateChanged will always trigger every time it is set. It is super easy to test that. If it is called while Auth is still initializing and loading state, it will wait for it to complete before resolving. If you call it after, it will still trigger immediately with the current user.

If you want some custom function or listener that triggers based on your own custom requirements, we provide all the APIs you need to build that.

If you are experiencing issues with the behavior of onAuthStateChanged listener, please file the bugs and we will make sure to address them.

Thaina commented 6 years ago

@bojeil-google I know that if I call onAuthStateChanged before initializing and loading state it would be triggered. However I read the document and haven't seen mentioning about what happen if I register onAuthStateChanged after it finished initializing and loading state but the user is not signed in yet

I know that it would be surely be triggered whenever user would sign in by any means. But if user never sign in at all would it trigger that it change from null to null?

Why is that?

Is it changed from null as auth not initialized to null as user not signed in ?

And if I register onAuthStateChanged after finished initialized why it still trigger when user AuthState was not changed at all?

In the document

https://firebase.google.com/docs/reference/js/firebase.auth.Auth

I have seen the explanation only these

Adds an observer for changes to the user's sign-in state.

Prior to 4.0.0, this triggered the observer when users were signed in, signed out, or when the user's ID token changed in situations such as token expiry or password change. After 4.0.0, the observer is only triggered on sign-in or sign-out.

At this line

the observer is only triggered on sign-in or sign-out

is very pinpoint that if user not doing any signin/signout at all it should not be triggered just from registering the observer

Also in here is not mention about that too

https://firebase.google.com/docs/auth/web/manage-users

What document you think I should read?

Thaina commented 6 years ago

@bojeil-google Do you have a link to the document you mention about this behavior?

bojeil-google commented 6 years ago

This is the reference for onAuthStateChanged: https://firebase.google.com/docs/reference/js/firebase.auth.Auth#onAuthStateChanged

This is the guide on how to use it: https://firebase.google.com/docs/auth/web/manage-users#get_the_currently_signed_in_user

Any time you call this listener after initial state has been determined, it will trigger with the current state (null if no user signed in, and with a user if a user is signed in). If you don't believe, then just try it out yourself.

Thaina commented 6 years ago

@bojeil-google It not that I would not belief but it the API name itself is misleading so I got confused. I test it and it seem the real behavior is like you stated. But because it not mentioned in the document I don't know I should rely on it or not

That's why I said if I misunderstand then it should be documented about this behavior. Because the API name is "onAuthStateChanged" but it will also firing one time when the auth state not really changed

bojeil-google commented 6 years ago

Fair enough. We should update the documentation and references to mention that. Thanks for pointing that out.

chrismcmacken commented 6 years ago

I will šŸ‘ this request, I just wasted a lot of time trying to find a method on the api to load the currently logged in user on page load before I found this issue. It needs to be mentioned that this will also load the user from the persistence if it is persisted.

atishpatel commented 5 years ago

Here is modified version of @bojeil-google function that can be used anytime to get a promise that resolves into current user or null.

let userLoaded = false;
function getCurrentUser(auth) {
  return new Promise((resolve, reject) => {
     if (userLoaded) {
          resolve(firebase.auth().currentUser);
     }
     const unsubscribe = auth.onAuthStateChanged(user => {
        userLoaded = true;
        unsubscribe();
        resolve(user);
     }, reject);
  });
}
JesusCuenca commented 5 years ago

I've šŸ‘ this request, too. As the Thaina, I also think the documentation about onAuthStateChanged is not clear about the discussed behavior, and even the API name itself is misleading. To me, it seems that an action from the user is needed in order to trigger the function, very much like the HTML onClick event.

SumNeuron commented 5 years ago

@atishpatel I've tried that but it deosnt work for me :/

holmberd commented 5 years ago

onAuthStateChanged is called when the user auth state changes and when the firebase.auth is first initialized.

Does anyone know if firebase.auth().currentUser caches the user, with or without persistence enabled. Or if requires a new network request API call everytime?

bojeil-google commented 5 years ago

firebase.auth().currentUser caches the user regardless of persistence.

SumNeuron commented 5 years ago

No idea... I'm using nuxt and have this as a pluggin:

// PLUGIN FILE
import firebaseConfig from '~/firebase'
import firebase from 'firebase/app'

if (!firebaseConfig) {
  throw new Error('missing firebase configuration file')
}

export default ({store, redirect}) => {
  if (!firebase.apps.length) {
    firebase.initializeApp(firebaseConfig)
  }
  return firebase.auth().onAuthStateChanged((user) => {
    if (user) {
      store.commit('firebase/setUser', user)
    }
  })
}

// VUEX STORE MODULE 

...

setUser(state, user) {
    state.user = user
    return this.dispatch('firebase/setProfileRef', state.user.uid)
  }

setProfileRef: firestoreAction(({ bindFirestoreRef }, uid) => {
    return bindFirestoreRef('profile', firebase.firestore().collection('profiles').doc(uid))
  }),
...
SumNeuron commented 5 years ago

sometimes causes inf recursion šŸ™ƒ

jhoughjr commented 5 years ago

bump. I'm finding many hours wasted on the auth state. I vote for a simple API the consistently gives us the user or null when we need it.

zerobytes commented 5 years ago

See, authStateChanged has a purpose, and therefore it can be used to workaround the fact that .auth().currentUser is not a promise and it's value is not trustable (because it depends on asynchronous tasks), it's purpose is to watch changes at the auth state. getCurrentUser, method that doesn't exists currently, has a different purpose: to simply get the current user despite how many changes happened at the application until now.

Dont take this wrong, please, but having methods implicates in having closed scopes of use, it's a basic pattern rule.

Either making it possible to use async .auth.currentUser or .auth().getCurrentUser().then would be desired implementation.

holmberd commented 5 years ago

@zerobytes I just wrap it in a promise before calling it from other services.

function getUser() {
  return new Promise((resolve, reject) => {
    let user = this.firebase.auth().currentUser;
    return user ? resolve(user) : reject(new Error('No user signed in')); 
}
zerobytes commented 5 years ago

Hey @holmberd, I understood what you did, but it has no point in having a promise if your action is not asynchronous, What we intent to do with getCurrentUser() is having a promise which will await until firebase auth has actually finished the process of identifying the current user. This task happens asynchronously which is the reason why an already authenticated app can fail get firebase.auth().currentUser, as firebase auth has not yet finished the process of setting it.

@atishpatel Answer is more like what we need, and will do the job, but it's still a workaround. Firebase SDK should have it as a promise.

holmberd commented 5 years ago

What we intent to do with getCurrentUser() is having a promise which will await until firebase auth has actually finished the process of identifying the current user. This task happens asynchronously which is the reason why an already authenticated app can fail get firebase.auth().currentUser, as firebase auth has not yet finished the process of setting it.

I haven't seen any such failures occur and therefore can't confirm. But it does look like there is a fair share of ambiguity in regards to the API usage in general.

As I see it getCurrentUser is only guaranteed to be set after a firebase.auth().onAuthStateChanged event when a user has signed in, or in the resolved promise of firebase.auth().signIn*(provider). Where the latter is more useful if you are checking if the signed in user is a new user additionalUserInfo.isNewUser.

For me onAuthStateChanged gate-keeps any services relying on the signed in user state and maintain the asynchronous flow.

Thaina commented 5 years ago

@holmberd I guess you might not know but, firebase library has an initialize duration in the loading time that, even user have been logged in and even have persistent logged in data stored in the local session, the currentUser would still be null until firebase system finish initialized itself

And this issue was voicing that, we don't really want a long term listener for the gate-keeps functionality you talk about. We want just a Promise or a Thenable to be that kind of gate-keep in the async function. We just want to call await getCurrentUser() and if it is null we then just show the logged-in button or launch the redirect flow. But we just need to call it one time and don't want to have it keep listening. So why we need to have it as listener that we need to unsubscribe by ourselves? It make our code unnecessary more bloated

holmberd commented 5 years ago

@Thaina I have not experienced that issue. I initialize firebase before initializing any services depending on it. Could you show me an example of such behaviour to test?

Thaina commented 5 years ago

@holmberd You can just read from the doc

https://firebase.google.com/docs/auth/web/manage-users

The recommended way to get the current user is by setting an observer on the Auth object:

By using an observer, you ensure that the Auth object isn't in an intermediate stateā€”such as initializationā€”when you get the current user. When you use signInWithRedirect, the onAuthStateChanged observer waits until getRedirectResult resolves before triggering.

It's always mentioned since the beginning

ensure that the Auth object isn't in an intermediate stateā€”such as initialization

holmberd commented 5 years ago

@Thaina I see, yes I'm aware of this behaviour. I don't see the problem to be honest.

Thaina commented 5 years ago

@holmberd The problem lies in this code

function getUser() {
  return new Promise((resolve, reject) => {
    let user = this.firebase.auth().currentUser;
    return user ? resolve(user) : reject(new Error('No user signed in')); 
}

Your code does not ensure that the current user aren't exist. Because if we call your code when the auth object still initialize it will just resolve null even the user has been logged in already. Which is the point of this issue and the request of people

Simply put, you don't see the problem and don't have a problem with this behaviour. But other people include me did. We need a getCurrentUser() as a promise function instead of .currentUser

holmberd commented 5 years ago

@Thaina If you don't take the asynchronous nature of the library in consideration when calling the API and setting up your services, then yes you will run into these kinds of problems.

The code examples provided by bojeil-google and atishpatel will do what you want.

Thaina commented 5 years ago

@holmberd I think you misunderstand everything in this issue

Promise is asynchronous in nature

It was this library that expose currentUser field without considering asynchronous nature of itself

This library actually should consider its own asynchronous nature. And so it should provide getCurrentUser() that would return Promise<User> which will be asynchronous

proof666 commented 4 years ago

Any news?

I faced this issue too. When already logged in user refresh page the observer onAuthStateChanged returns null, and after 1-2 seconds fired one more time with logged user info.

But when in fires first time, my code decide that user should be redirected to login page.

What can you guys advise on such behavior? Wait 1-2 seconds before listen onAuthStateChanged?

I think it is not a proper solution...

wti806 commented 4 years ago

@proof666 As @bojeil-google mentioned: 'If it is called while Auth is still initializing and loading state, it will wait for it to complete before resolving. ' If you have a logged in user, refreshing the page should only trigger the listener once with that user. I tested myself and verified the behavior.

quantuminformation commented 4 years ago

Seems like this is a whole lot more complex than it needs to be, Can't it be a sync read of the indexDB? Could that really be that slow?

Thaina commented 4 years ago

@QuantumInformation There was a lot more preparation need to be done in firebase system before it could return proper user. I don't know what they kept in indexDB but I guess it is not a complete userdata. And if the system was online the user will most likely expect the fresh userdata

quantuminformation commented 4 years ago

This used to work, but now it never fires (sorry if this is of topic)

https://github.com/QuantumInformation/svelte-fullstack-starter/blob/master/src/stores/firebaseUserStore.js#L38

const unsubscribe = firebase.auth().onAuthStateChanged(user => {
            userLoaded = true;
            unsubscribe();
            resolve(user);
        }, reject);

Now the app just doesn't do any network requests locally, any idea what could have happened? No errors either. the indexDB looks like this.

image

AmitJoki commented 4 years ago

Any time you call this listener after initial state has been determined, it will trigger with the current state (null if no user signed in, and with a user if a user is signed in). If you don't believe, then just try it out yourself.

Actually, I've a signInWithCustomToken enabled with LinkedIn OAuth and I've done

console.log(firebase.auth().currentUser);

And I get null first and just a few seconds later, I get the user object. This is when I've already signed in. And this behavior is consistent. I get null and then after a few seconds, the user.

Same behavior with the authStateChanged as well. There's a delay which we cannot control.

If firebase.auth().currentUser were a promise, I could do:

showLoader();
firebase.auth().getCurrentUser().then(function(user){
  if(user) stopLoader();
  else redirectToLogIn();
});

Now, it ain't possible.

AmitJoki commented 4 years ago

Here's the demo. https://webm.red/view/mkpV.webm

kwbtdisk commented 4 years ago

My solution for this topic, a bit ugly but it works like a charm on my side :D

  async _fetchLoginStateWithWaitingUntil4Sec() {
    // AuthCheck, Fire on load the app into the browser
    // check userinfo every 250ms until 4000ms passed
    const waitMSec = 250
    const loopMax = 4000/waitMSec
    for (let i=0; loopMax > i; i++) {
      console.log(`_fetchLoginStateWithWaitingUntil3Sec: ${i}`)
      const user = firebase.auth().currentUser
      if(user) {
        console.log(`_fetchLoginStateWithWaitingUntil3Sec: user:`, user)
        return user
      }
      await this._sleep(waitMSec)
    }
  }

  async _sleep(msec) {
    return new Promise(resolve => setTimeout(resolve, msec))
  }

currentUser will be fetched around 1500ms. (of course it depends on the network conditions)

proof666 commented 4 years ago

@proof666 As @bojeil-google mentioned: 'If it is called while Auth is still initializing and loading state, it will wait for it to complete before resolving. ' If you have a logged in user, refreshing the page should only trigger the listener once with that user. I tested myself and verified the behavior.

@wti806, You can see in @AmitJoki demo that it is not as @bojeil-google mentioned.

Everyone who encountered a problem is trying to tell about this. But those who have not encountered the problem sacredly believe in the documentation and refuse to believe in any arguments. Instead, they suggest wrapping the synchronous method in a promise (what does it change?) It looks weird for a library of "google"-level.

Probably the problem is reproduced only under certain conditions, for example, when the page loads too quickly and the firebase does not have time to initialize. If you set a timeout of at least 1 second, the problem stops reproducing. So when your app starts to render when firebase already initialied you does not have any problems. But some apps start to render too fast, when firebase does not initilized yet <--- here is the problem.

andrefedev commented 4 years ago

Firebase Auth persists the data in "IndexedDB", additionally every time the app is initialized for the first time it makes a call to the api of "googleapis.com/identitytoolkit", I suppose that it is a way to verify that the information persisted in the browser be valid. Until the answer is complete we will have that currentUser will remain "null", Although the user has already logged in previously.

@bojeil-google, I would like to find some help in the documentation to handle this problem.

gamliela commented 4 years ago

After spending a few hours/days on these issues, I think I understand some of the frustration. I also understand why wrapping currentUser doesn't really make any difference. Let me try to rephrase the problem from a different point of view.

  1. currentUser is poorly documented. It doesn't mention the fact that this value can be null during initialisation.
  2. onAuthStateChanged is poorly documented. It doesn't mention the fact the observer will be called at least once, after initialisation.
  3. However, there are other docs that provide more details for these methods. So unless you read all documentation carefully you're in trouble.
  4. Given that the recommended approach to get current user relies on onAuthStateChanged, it is not clear what could be a valid use-case for currentUser. There is no API that tells us when initialisation has finished, so it is not possible to know when it is safe to use currentUser. Maybe this method should get removed completely from the API?
  5. Not really related to this topic, but I wish the documentation would provide more information about the relation between onAuthStateChanged and getRedirectResult. It seems that onAuthStateChanged might wait to getRedirectResult to get resolved; but how do we know if we need to call getRedirectResult? Does error of getRedirectResult get delegated to onAuthStateChanged error parameter? etc.
Thaina commented 4 years ago

@gamliela Very appreciate your understanding but I would like to add that onAuthStateChanged has its own usage and pattern. It was designed to be used in reactive styled application, such that when you have logged in profile on top of the page, you could let it listened and change UI immediately when user change their logged in account and status

However, in many situation when our app don't really need to be reactive, we just need to check a currentUser once and continue doing other things. onAuthStateChanged just become friction in async/await code

So IMO, the actual complete solution is to provide both API for covering each situation. And should deprecate direct currentUser while promote getCurrentUser() instead

philBrown commented 4 years ago

I totally agree with @Thaina. The issue here is mixing a runtime event subscriber like onAuthStateChanged with a one-time event handler for auth state initialised that is only required at application startup.

Handling each of those events requires totally separate logic and as such, should be facilitated by separate methods.

gamliela commented 4 years ago

@Thaina and @philBrown I actually support that view. I'm sorry it wasn't clear from my earlier comment. I can't see a lot of value in a synchronous version of getCurrentUser. While obtaining current user can be done technically using onAuthStateChanged, it's not ideal in terms of developer experience. A better solution would be to replace it with asynchronous getCurrentUser (which will guarantee to resolve after initialisation).

Another approach could be to introduce another observer like onAuthStateInitialised. This can serve other needs except fetching the current user. Then you could do something like:

onAuthStateInitialised(() => console.log(app.auth().currentUser))
Thaina commented 4 years ago

@gamliela Well, that I was not being clear on my side. I just mention that I would like it to be getCurrentUser as a promise so it actually being asynchronous

Currently now we don't have getCurrentUser function at all, we only have currentUser field

rockodragon commented 4 years ago

I'm having this intermittent issue as well forcing a user to login when they shouldn't have to. Agree getCurrentUser would be more deterministic.