Skip to content
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

fix: authState getting wiped on page reload on static websites #785

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

Vijayabhaskar96
Copy link
Contributor

@Vijayabhaskar96 Vijayabhaskar96 commented Jul 3, 2024

Previous PR: #712

🔗 Linked issue

#551
This issue occurs because data (data from getSession()) is not stored in the browser, which is not a problem in SSR with server because the server takes care of it, but on a statically generated website, this data is lost along with rawToken even if the token is stored in the browser cookies if you refresh the webpage.

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR stores the retrieved data from getSession() and stores it in the browser cookie auth:sessionCookie and reloads the data and rawToken back on the client side if they're undefined.
This PR fixes the data issue, but you still need to set the prerender:false for middleware protected routes in routeRules so that client-side middleware is forced to run and load the states.

📝 Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

src/runtime/plugin.ts Outdated Show resolved Hide resolved
@zoey-kaiser zoey-kaiser changed the title updated fix authState getting wiped on page reload on static websites fi: authState getting wiped on page reload on static websites Jul 5, 2024
@zoey-kaiser zoey-kaiser changed the title fi: authState getting wiped on page reload on static websites fix: authState getting wiped on page reload on static websites Jul 5, 2024
@zoey-kaiser zoey-kaiser added bug A bug that needs to be resolved provider-local An issue with the local provider provider-refresh An issue with the refresh provider and removed provider-refresh An issue with the refresh provider labels Jul 19, 2024
@Geekimo
Copy link

Geekimo commented Aug 7, 2024

Hello @zoey-kaiser,
is there anything we can do to help get this fix shipped ?

Thanks !

@zoey-kaiser
Copy link
Member

Hi @Geekimo 👋

I would like to get a review by @phoenix-ru in before merging this PR 😊

@Geekimo
Copy link

Geekimo commented Aug 8, 2024

@zoey-kaiser Thanks for your reply ☺️

@thorge
Copy link

thorge commented Aug 23, 2024

Any news on this fix or can we help? I use a local provider and having auth work with ssg would be highly appreciated :-)

@Geekimo
Copy link

Geekimo commented Sep 4, 2024

Hello @zoey-kaiser and @phoenix-ru,
Sorry to bother you about this fix, but I'm currently holding a deployment to production due to this bug.
I'd really love to help you out to get this released.

Kind regards.

Copy link

pkg-pr-new bot commented Sep 18, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@785

commit: 0f7e51a

zoey-kaiser
zoey-kaiser previously approved these changes Sep 18, 2024
Copy link
Member

@zoey-kaiser zoey-kaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally approved 🥳

@zoey-kaiser
Copy link
Member

Hi @Vijayabhaskar96 👋

I wanted to push this PR into the next release, but sadly the authjs tests are failing. We will finalize the PRs for the release tomorrow. For the case that we are not able to include it yet, you can directly install your PR as an npm package using:

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@785

@Geekimo
Copy link

Geekimo commented Oct 1, 2024

Hello @zoey-kaiser,
will this PR be included in the next release (if yes, when is it planned ?)
Regards

@thorge
Copy link

thorge commented Oct 21, 2024

Is there a specific reason why this wasn't included in the 0.9.4 release?

@thorge
Copy link

thorge commented Nov 18, 2024

Sorry, I don’t want to come across as impatient, but we’re currently holding back a production site until this fix is implemented. Is there already an idea of when this will be released?

Comment on lines +609 to +614
// Augment types
declare module 'nuxt/schema' {
interface PublicRuntimeConfig {
auth: ModuleOptionsNormalized
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question as to why was this added?

Comment on lines +144 to +147
sessionCookie.value = {
lastRefreshedAt: lastRefreshedAt.value,
data: data.value
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing user data in the cookie may lead to a GDPR violation if the getSession endpoint returns any personal information (which is often the case). It undoubtedly spawns the "protecting personal data" requirement for anyone using the module, even if they weren't affected by the SSG issue in the first place.

https://gdpr.eu/cookies/

I don't find it very attractive to introduce such a burden to NuxtAuth :/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe introducing an option to use SessionStorage nor LocalStorage would be great instead of using cookies ?

Comment on lines +51 to +80
function handleLocalAuth(config: ProviderLocal): void {
const sessionCookie = useCookie<SessionCookie | null>(
'auth:sessionCookie'
)
const cookieToken = useCookie<string | null>(
config.token?.cookieName ?? 'auth.token'
)

if (sessionCookie?.value && !rawToken?.value && cookieToken?.value) {
restoreSessionFromCookie(sessionCookie, cookieToken)
}
}

function restoreSessionFromCookie(
sessionCookie: CookieRef<SessionCookie | null>,
cookieToken: CookieRef<string | null>
): void {
try {
loading.value = true
const sessionData = sessionCookie.value
lastRefreshedAt.value = sessionData?.lastRefreshedAt
data.value = sessionData?.data
rawToken.value = cookieToken.value
}
catch (error) {
console.error('Failed to parse session data from cookie:', error)
}
finally {
loading.value = false
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am struggling to understand as to why saving/restoring the session data in the cookie is needed in the first place? I read both the original issue #551 and the PRs around it, but still haven't found a valid reason for this approach.

I think there should be a less hacky way of solving it

@phoenix-ru
Copy link
Collaborator

@Geekimo @thorge @Vijayabhaskar96 Hi, sorry for keeping you waiting as this PR has been quite controversial for me. I just left my concerns in the review above. I don't want to give any false promises about merging the PR soon, as I do not feel confident at all in the patch.

If you trust it, you can use it at your risk by using the latest commit (see the message from bot -> click on commit):

pnpm add https://pkg.pr.new/@sidebase/nuxt-auth@0f7e51a

I know it's one of the long-standing issues in the local provider and I will try to come back at it with a different approach within 2024.

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 provider-local An issue with the local provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants