Skip to content

Refresh provider on tab refocus calls getSession instead of refresh #765

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

Closed
mbellamyy opened this issue Jun 5, 2024 · 4 comments
Closed
Labels
bug A bug that needs to be resolved p3 Minor issue

Comments

@mbellamyy
Copy link

mbellamyy commented Jun 5, 2024

Environment



Reproduction

not needed, code is obvious

Describe the bug

This is actually two issues.

First issue is here. Let's assume refresh provider is chosen.

if (enableRefreshPeriodically !== false) {
const intervalTime =
enableRefreshPeriodically === true ? 1000 : enableRefreshPeriodically
this.refetchIntervalTimer = setInterval(() => {
if (data.value) {
getSession()
}
}, intervalTime)
}
if (runtimeConfig.provider.type === 'refresh') {
const intervalTime = runtimeConfig.provider.token.maxAgeInSeconds! * 1000
const { refresh, refreshToken } = useAuth()
this.refreshTokenIntervalTimer = setInterval(() => {
if (refreshToken.value) {
refresh()
}
}, intervalTime)
}

We're creating two intervals. One of them is calling getSession and the other is calling refresh. But the refresh implementation calls getSession at the end itself... So the first interval might be redundant for refresh? I know the interval times are different. So this is not a hugely imporant part of this issue.

Now the second (and more important) part of the issue:

visibilityHandler () {
// Listen for when the page is visible, if the user switches tabs
// and makes our tab visible again, re-fetch the session, but only if
// this feature is not disabled.
if (this.config?.enableRefreshOnWindowFocus && document.visibilityState === 'visible') {
useAuth().getSession()
}
},

This function is the tab refocus refresher. It is calling getSession which is acceptable in the local provider, but for refresh I think it should call refresh, which in turn calls getSession later.
If the token is expired (but can be refreshed), this method is missing it and logging the user out.

Additional context

No response

Logs

No response

@mbellamyy mbellamyy added bug A bug that needs to be resolved pending An issue waiting for triage labels Jun 5, 2024
phoenix-ru added a commit that referenced this issue Jun 6, 2024
@phoenix-ru phoenix-ru added p3 Minor issue and removed pending An issue waiting for triage labels Jun 6, 2024
@ThomasSimonsen20
Copy link

@phoenix-ru Hey Phoenix sorry to ping, I see you started working on a PR regarding this, is this something you are still working on? I am facing a similar issue as above and I believe your PR would fix that issue :)

I am sadly not experienced enough to figure out if I could help with anything in your PR, as I can barely understand what's going on :D

@phoenix-ru
Copy link
Collaborator

Hi @ThomasSimonsen20 , I was on a vacation 🙂
I will possibly come back to this in 2 days or next week, as it's awaiting review from @zoey-kaiser

@ThomasSimonsen20
Copy link

Thank you for the update, hope you had a nice vacation :)

zoey-kaiser added a commit that referenced this issue Jun 27, 2024
@zoey-kaiser
Copy link
Member

Hi @mbellamyy and @ThomasSimonsen20 👋

Sorry for the ping, but I wanted to quickly let you know that we have just released https://github.com./sidebase/nuxt-auth/releases/tag/0.8.0-alpha.3 in which you can now define your own custom refresh logic! Feel free to check out the release notes for more information!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug that needs to be resolved p3 Minor issue
Projects
None yet
Development

No branches or pull requests

4 participants