search

firebase / firebase-js-sdk

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

FR: Change behavior of onAuthStateChanged. #4133

Open heckad opened 3 years ago

heckad commented 3 years ago

[REQUIRED] Describe your environment

Problem: If a user was logged and try to open spa app on page wich required auth access the app should waiting until auth will be confirmed and only then call callback onAuthStateChanged with the user object. This check can be quite long (more than 5 seconds with slow 3g).

Propose: Call callback when we find user information in storage with this user object without waiting for the end of the auth check. If auth check return that user was logout than call callback with user null.

Thanks!

hsubox76 commented 3 years ago

I'm not sure I understand what the request is. Is the problem the same as the one you are asking about in the comment here: https://github.com/firebase/firebase-js-sdk/issues/462#issuecomment-735518608? If so, does the answer there solve your problem?

Or is this a separate issue?

heckad commented 3 years ago

Yes, this is the same problem. I tried your solution, and they didn't help because if a user wants to get access to the private route, then the router will wait until verification competes and the user will see a white screen all time. If I remove check verification from the route, then route redirects the user to a login page. Because I need the auth call callback less than 100ms even they can't get information from google.

gustavopch commented 2 years ago

@hsubox76 I think I have a similar use case, although it's a multi-page application instead.

Whenever the user navigates to a new page, everything including Firebase is initialized from scratch. I have some JS code that will dynamically render the page by setting document.body.innerHTML, but it can only run after the auth state is known. The code is somewhat like this:

<header>
  <script type="module">
    onAuthStateChanged(getAuth(), user => {
      document.body.innerHTML = `Hello, ${user.displayName}!`
    })
  <script>
</header>
<body></body>

Even on a fast connection, it takes 300ms for the callback to be called, so the browser won't wait and will show the blank page (because the body is empty in the HTML itself). So this is what the user sees in the screen:

On the other hand, if onAuthStateChanged invoked the callback immediately with cached data (from IndexedDB or Local Storage), paint holding would take place and the user would see this in the screen:

Summarizing: if onAuthStateChanged loaded immediately with cached data, there wouldn't be a white flash between navigations (when loading a new page). That would provide a much superior UX for multi-page applications. I know the initial auth state would potentially be stale/expired, but in that case the callback would be called again with fresh data. IIRC, that's just how Firestore's snapshot listeners work (edit: actually not, a new listener takes about 120ms to receive the first snapshot here; edit 2: about 40ms with persistence enabled which is still too slow for paint holding to work).

gustavopch commented 2 years ago

I found that this is where the problem happens:

https://github.com/firebase/firebase-js-sdk/blob/cdada6c68f9740d13dd6674bcb658e28e68253b6/packages/auth/src/core/user/reload.ts#L31-L75

Firebase indeed has the auth state stored locally (the user parameter), but it forces a network request to check if it should be invalidated (_logoutIfInvalidated()). That request is what takes a lot of time. The listeners will only be notified after it completes. TBH, I'm not even sure if there's much of a point in storing the auth state locally if it's always requested again from the server, and not used for optimistic UI.

Instead, I think it could immediately notify the listeners with the cached auth state and THEN proceed to check if the invalidation is actually needed (most times it won't be, at least for my app).

I understand that for some apps the invalidation may be important for security, but if that's the case, then at least add an option to initializeApp() where we can opt in to using the cached auth state. It can also consider how much time ago it was last checked. If there was a session in the last 24 hours, use the cached auth state, otherwise not.

Please consider following my suggestion. It's a low-hanging fruit that will make all MPAs using Firebase Auth a lot faster.

I'll even leave a draft diff here so you have something to start with 😊:

 export async function _reloadWithoutSaving(user: UserInternal): Promise<void> {
   const auth = user.auth;
+
+  // Notify listeners with the cached user before checking if the invalidation is needed
+ // so the app can initialize fast.
+  user._notifyReloadListener(user);
+
+  // Wrap in a promise that's not awaited so it doesn't block the function termination.
+  Promise.resolve(async () => {
     const idToken = await user.getIdToken();
     const response = await _logoutIfInvalidated(
       user,
       getAccountInfo(auth, { idToken })
     );

I know this diff would break other places using the _reloadWithoutSaving() method, but I hope it can at least illustrate my suggestion. In reality, you'd probably add a options: { acceptCached?: boolean } | undefined parameter and only pass { acceptCached: true } when Firebase is being initialized.

sam-gc commented 2 years ago

Hi @gustavopch, thanks for digging into this more! My main concern is that this proposal blurs the lines between onAuthStateChanged() and onIdTokenChanged() (the former only updates when the user's uid changes, the latter updates whenever the token itself updates).

With that said, there may be a way to deliver this functionality using a different/new API. I will discuss this more closely with the team.

gustavopch commented 2 years ago

@sam-gc That makes sense. I'll explain my actual use case. Pseudo-code:

let unsubscribe
onAuthStateChanged(getAuth(), authState => {
  unsubscribe?.()
  unsubscribe = onSnapshot(
    doc(getFirestore(), `users/${authState.uid}`)
    user => { console.log('User loaded') }, 
  )
})

As you can see, I can only add the Firestore listener once I know the ID of the current user, so even when I have Firestore persistence enabled, it won't load the user document as fast as it could. I'm also not sure if there's any code inside Firestore that would wait for the auth state to be known even if my own code wasn't wrapped in an onAuthStateChanged(), so that's something to keep in mind too.

gustavopch commented 2 years ago

@sam-gc Well, maybe simply populate getAuth().currentUser with the cached auth state upon initialization? That way, you don't need to mess with onAuthStateChanged(). So the code above would need to be adapted to:

let unsubscribe
const handleAuthState = authState => {
  unsubscribe?.()
  unsubscribe = onSnapshot(
    doc(getFirestore(), `users/${authState.uid}`)
    user => { console.log('User loaded') }, 
  )
}

handleAuthState(getAuth().currentUser)
onAuthStateChanged(getAuth(), handleAuthState)

Note: added unsubscribe to the snippet so people using it as a reference in the future don't forget.

sam-gc commented 2 years ago

The problem with either is that it changes the way the API operates, which is a breaking change. The team is exploring other options that avoid changing existing functionality (i.e. by adding this as a new feature).

One thing to note is that if the token is too old, the database read will fail on the server with a permission denied error. I'm not sure how this will interact with the local cache

gustavopch commented 2 years ago

One thing to note is that if the token is too old, the database read will fail on the server with a permission denied error.

@sam-gc Here's how I think it would work:

Assuming most times the token can be refreshed, then most times it will work seamlessly. In cases where the token can't be refreshed, then the user may see the cached data for a moment and then be forced to re-authenticate. Of course, that's assuming the developer wants to use the cached data, but it would be optional.

cbreezier commented 1 year ago

Hi, any movement on this? @sam-gc

TBH, I'm not even sure if there's much of a point in storing the auth state locally if it's always requested again from the server, and not used for optimistic UI.

Wholly agreed here.

Right now, the predominant workaround is to store some value in local storage (eg, a boolean, or maybe even the entire auth.currentUser object) and manually check that to determine "logged-in-ness" faster than the sdk can re-resolve the current user. This is so clunky and forces us to re-implement logic and duplicate data that is already tracked by the sdk.

99.99% of the time, the user will be successfully validated, and only in 0.01% of the time might the user be deleted, or credentials revoked. We should optimise for the common case instead of the rare one.

The problem with either is that it changes the way the API operates, which is a breaking change.

I don't see how pre-populating the user with the stored data from IndexDB changes the way the API operates. The onAuthStateChanged will still correctly notify us about changes to the auth state. Even auth.currentUser doesn't break any definitions. In both cases, the APIs just work off of the most current information that they can, and correct themselves as soon as new information is available.

Edit: even today, onAuthStateChanged and auth.currentUser aren't guaranteed to serve fresh data. If a user has a browser tab open overnight and their credentials are revoked, when they view the browser in the morning they'll still be "logged in" and referencing stale data, until the sdk can re-validate everything asynchronously in the background.

pcl commented 1 year ago

I'm running into this issue as well.

TBH, I'm not even sure if there's much of a point in storing the auth state locally if it's always requested again from the server, and not used for optimistic UI.

Wholly agreed here.

In my experience, if a device is offline, then the request to refresh the user will fail fast and the app will launch quickly. So IMO it's great that the cache exists, and is certainly not just there as an optimization.

However, when on a slow network (such as a 3G connection), the refresh request has to time out before anything else can proceed, which is frustrating.

I'm currently hacking around this problem by intercepting fetch (which I already do for logging purposes) and adding a 2-second timeout to the requests to identitytoolkit.googleapis.com and securetoken.googleapis.com.

It'd be great if the initialization sequence didn't block on this refresh, or if there were some other way to fetch the current user object from local storage while the initialization progresses. I don't really want to add my own user cache on top of the one that Firebase Auth provides, since that presents a bunch of additional synchronization issues to manage.

cbreezier commented 1 year ago

I just wanted to add that other than the slow 3G connection use case, anyone in a region where Firebase Auth isn't replicated to will also feel a lot of pain.

My customers are primarily in Australia, and the 300-600ms latency makes every single page refresh really clunky feeling, because I can't render anything if I don't know if the user is logged in or not.

cbreezier commented 10 months ago

@sam-gc Hello, friendly bump - any movement or new information on this issue?

chdsbd commented 9 months ago

This is a really annoying bug in Firebase. I've been trying to dig though the firebase sdk to see how to fix this, but it's pretty esoteric

Looking in the Chrome stack trace, I think onSnapshot waits for this auth call to complete before continuing to load: https://github.com/firebase/firebase-js-sdk/blob/086df7c7e0299cedd9f3cff9080f46ca25cab7cd/packages/firestore/src/remote/persistent_stream.ts#L446-L461

wahibimoh commented 8 months ago

Any update on this? this was raised on 2020 and still no progress what so ever. All we are asking for is to give developers the option to trust the cached user, seriously, how hard can this be to implement? Cached user is already trusted in offline mode, so just give us this option.

This is killing the whole firebase ecosystem for MPA.