-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NextAuth should not invalidate the client-side session when the browser is offline #2215
Comments
Hi there, I wouldn't flag this as a bug, more of an enhancement as this has been the expected behavior until now. The thing you experience happens, because we re-fetch the session when the tab/window gets refocused. Currently this behavior cannot be turned off, but in v4, there will be an option for it. See #2171 ( |
Thanks for pointing me to that enhancement. However, the more I think about this, the stronger I feel that current behavior is incorrect. And to be clear, my main concern is with the way NextAuth handles network failures / offline mode and not the 'visibilitychange' event. Let's imagine you have an email client app that uses the following session settings:
These settings establish the following contract: But what is happening is that NextAuth evicts client-side session data even when a single keep-alive call to the server fails and returns no fresh data. Now imagine you are commuting to work by train reading emails, and all of a sudden, your favorite email app tells you "Hey, your session is no longer good, and you need to sign in again" all just because the train went through a tunnel and you were offline for couple minutes. This is a bad user experience in my books, and I do not see a clear way for an app developer to work this around unless NextAuth correctly handles failed keep-alive / session refresh calls. What are your thoughts on this? |
Here is a PR that I believe addresses this issue. |
Thank you for your detailed example. I can see how it would be a problem, but I still think that any request to a new session should invalidate the old one as well. Let's say the session contains an The problem you are describing can be worked around in user land in my opinion. You could set up an event listener for network changes, and whenever you experience that the network is unavailable, you could clear the polling by setting Suggestion for import * as React from "react"
import { Provider } from "next-auth/client"
function useOnlineStatus() {
const [online, setOnline] = React.useState(() => navigator.onLine)
React.useEffect(() => {
const handler = () => setOnline(navigator.onLine)
window.addEventListener("online", handler)
window.addEventListener("offline", handler)
return () => {
window.removeEventListener("online", handler)
window.removeEventListener("offline", handler)
}
}, [])
return online
}
export default function App({ Component, pageProps: { session, ...pageProps } }) {
// Poll session every five minutes, but stop polling when the internet drops.
const keepAlive = useOnlineStatus() ? 5 * 60 : 0
return (
<Provider options={{ keepAlive }}>
<Component {...pageProps} />
</Provider>
)
} UPDATE: I checked, and our current implementation wouldn't actually work, since we return early if To be frank, I see a MUCH bigger problem. We use See the relevant current code here: Lines 283 to 298 in 84094b0
And the new code here: Lines 259 to 270 in 3a48b8e
Note that the new implementation would work with my suggestion, as it is correctly defined in a |
I'm more than happy to create a PR against the v4 branch as the code change would be pretty much identical.
And that's exactly what's happening because NextAuth Provider is sitting at the top of the components tree in the _app.js, and triggers a re-render of the whole app when session state changes. It is entirely legit behavior when the session state changes (a user signs in or signs out in another tab), but it's not quite logical when an opportunistic keepAlive call fails due to network or server error. Maybe we are treating keepAlive calls differently. In my view, keepAlive calls are opportunistic "heartbeats" that are meant to renew session expiration TTL and fetch an updated session state if it changes. But it's totally okay to skip some heartbeats when the user is offline. For example, users would expect their email client app or book reading app to stay open and mounted in the browser when they briefly go offline. Would you be more open to adding a configuration option that would define keepAlive semantic and allow it to work both ways? |
Thank you for the creative proposal utilizing navigator.onLine. However, it can't reliably cover all possible use-cases of why keepAlive call may fail. Think of a network issue anywhere between your router and Next backend server (your direct ISP or any intermediate ISP), or intermittent issue with the backend server itself causing it to respond with 500 error. navigator.onLine would miss this kind of issue, but keepAlive call would still fail. |
In this case I would setup keepAlive interval of at least 1/10th of access_token TTL, and if it happens that all 10 heartbeats couldn't reach the server then I would deal with the expired access token on the application level (and it would be much easier problem to solve rather than not being able to differentiate user sign out vs failed keepAlive call as both flip the session to null) |
Thanks for the additional info. You bring up logical reasons, so I am willing to give in, but I would like someone else's opinion on the matter. Would be nice to check how others have implemented this:
Before opening a new PR, would you be up to creating a small codesandbox.io example using |
Here is code sandbox example using swr and react-query - https://codesandbox.io/s/nextauth-2215-gxoks With both libraries the app stays mounted and displays previously fetched data. |
Some options to handle offline-ness would be pretty helpful. I ended up solving this with useSWR #596 (comment) |
Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks! |
Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks! |
It seems this behavior is unchanged in the current release (v4.9.0). I'm using next-auth with SWR on one of my projects where I keep custom access tokens in the JWT token. The fact that the session is invalidated when a request fails makes it hard to utilize SWR's
Perhaps we could add a new flag similar to the SWR's |
Thanks for posting this @jozefhruska, I'd wondered what the latest version was up to. I only had success with this issue when I stripped all of those built in functions out and just handled it myself. So using SWR, getting rid of the local storage stuff, and managing the cookies myself. Works a charm now. |
I'm still using my own patch package based on PR #2237, and it's working like a charm - no random session invalidations due to network errors whatsoever, all next-auth functionality fully preserved, and no extra SWR hoops to jump. Happy to share it if you are still on v3 and would like to try it out. |
@igordanchenko I'm still on v3 and need to handle offline robustly, please hook me up with your patch package |
@TimNZ, here you go. patch-package (npm, yarn v1): https://gist.github.com/igordanchenko/554f1572da8f24611adfa8960d2774b0#file-next-auth-3-29-10-patch yarn patch (yarn v3): https://gist.github.com/igordanchenko/554f1572da8f24611adfa8960d2774b0#file-next-auth-npm-3-29-10-12aff06fed-patch |
@igordanchenko [spends 20 mins going down the rabbit hole of patch-package and figuring out a patch file name that works 😅 ] But it doesn't work as I expected. My goal in applying that patch was to stop useSession() clearing the session on an offline/internet disconnected scenario as discussed above. My _app.tsx usage
What should I be doing? |
I'm pretty sure the behavior you are seeing is cause by Here is the logic implemented in this patch: const newClientSessionData = await getSession(
{ triggerEvent: !triggeredByStorageEvent },
// fail over to the current session if the update wasn't triggered by a storage event or max age timeout
!triggeredByStorageEvent && (clientMaxAge === 0 || currentTime < clientLastSync + clientMaxAge) ? clientSession : null
) |
Yeah I think that too. I set it to 0 after I commented and will probably leave it that way for now until I feel like reassessing the implications around freshness of client data in the UI. |
I've updated my project to next-auth@4.10.3 today, and unfortunately, v4 handles unreliable network conditions even worse than v3. It takes just one Here is an updated patch for v4: https://gist.github.com/igordanchenko/e1e40c2da14ccdc2823c7fc31a5b2de2 This patch changes next-auth's bail-out-on-any-network-error implementation to use-stale-while-revalidate semantics. With this patch, the Installation:
Patch source code: igordanchenko@0918a5f |
Good on ya mate, I honestly don't understand how you can be expected to handle real customers without this. Is nextauth only for people doing hacky projects or for people building actual businesses? |
@balazsorban44 Is a fix planned for this issue? |
I've also just run into this issue and was taken by surprised by the current behaviour. In our case, this is affecting users on mobile devices where the connection may be intermittent and the browser suspended at any point leading to one-off errors. The patch looks great, what I would really love to see is another status that will show the difference between "loading" (not yet known), "authenticated" (good for sure), "unauthenticated" (we have positive proof that there isn't a valid session) and an additional "limbo" state where recent attempt(s) to validate the session have failed for a non specific reason (offline, temporary server errors). For now, we have reverted to using long lifetime access tokens with Auth0 rather than shorter lived access tokens periodically refreshed which would be my preference. |
Same issue. Following! |
@Sliffcak Since I last commented on this issue, a new |
@jozefhruska, the Here is a simple demo illustrating this issue - https://pnlkxs-3000.preview.csb.app (source code)
And here is the same demo with a patched |
Running into this as well using next-auth 4.24.5 - the patch discussed above seems like reasonable behavior for SessionProvider to have but has fallen out of date and is a brittle long term solution.. Has anyone solved this problem in another way or have a more recent issue open with next-auth regarding this? I've poked around a bit but haven't seen anything 😬 |
Well, it's been almost 3 years with no alternative solution in sight, so I'm treating the above patch as a permanent solution for my projects that still use |
Thanks for the patch fix @igordanchenko! |
Now that it's shown that it cannot be worked around in userland and that SWR nor react query invalidate all data on network errors either can the fix be finally accepted to this project @balazsorban44? Are there some arguments for keeping this behaviour that are left uncovered? Shouldn't it at least be an option to allow not invalidating sessions like this? Imagine if Netflix logged people out every time they have connectivity issues. |
Description 🐜
NextAuth invalidates the client-side session when the browser is offline and the NextAuth client cannot communicate with the NextAuth backend. From an application perspective, the expectation is that the NextAuth library must provide the last-known session state unless session TTL runs out or the NextAuth client receives an authoritative response from the NextAuth backend.
How to reproduce ☕️
Screenshots / Logs 📽
Logs in browser console:
Environment 🖥
Contributing 🙌🏽
I'm more than happy to author a PR but I'm just not sure about the logic behind
next-auth/src/client/index.js
Line 106 in 5a89ab6
The text was updated successfully, but these errors were encountered: