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

Feature: Option to remove server side auth #610

Merged
merged 37 commits into from
Apr 18, 2024

Conversation

KyleSmith0905
Copy link
Contributor

@KyleSmith0905 KyleSmith0905 commented Dec 14, 2023

Closes Contributes to #551.

This adds an option disableServerSideAuth that when enabled, would make useAuth calls not pass down the user's session on the server.

For example: If a website has caching, we don't want to send the user a page that says they're logged in as someone else.

Checklist:

  • issue number linked above after pound (#)
    • replace "Closes " with "Contributes to" or other if this PR does not close the issue
  • manually checked my feature / checking not applicable
  • wrote tests / testing not applicable
  • attached screenshots / screenshot not applicable

@zoey-kaiser
Copy link
Member

Hi @KyleSmith0905 👋

Thanks for your PR. Could you please provide more context on when this is needed and what your current setup looks like, that make it required to have this in place? Thanks!

@KyleSmith0905
Copy link
Contributor Author

What is the use case?

I have an app with ISR/prerender on most routes. It's rendered on the server and the response is cached on the edge. With what we currently have (and after this update if disableServerSideAuth is not true), the cached response may contain a user's authentication session that may be entirely separate from the user making the request.

Here is a repo I created to share the problem: https://github.com/KyleSmith0905/mre--nuxt-app/tree/using-isr

Why is it a global setting?

It was easier to implement Many times if you're using ISR another type of caching, it's likely going to be on other pages as well I guess.

Current implementation

I added a disableServerSideAuth option that prevents the server from getting the user session. It is by default false.

export default defineNuxtConfig({
  ...
  auth: {
    provider: {
      type: 'authjs'
    },
    disableServerSideAuth: true,
  }
})

@zoey-kaiser
Copy link
Member

Hi @KyleSmith0905 thanks for the detailed response!

Ill properly test this tomorrow! Do you also want to do me a favor and add it to the docs? That way I can see if I still get this merged for you before the new year!

@KyleSmith0905
Copy link
Contributor Author

KyleSmith0905 commented Dec 18, 2023

Sounds good

I added to the JSDoc documentation on the Nuxt Config page to https://sidebase.io/nuxt-auth/configuration/nuxt-config
I couldn't find any other places that fit 100% comfortably, besides putting it in a tip box on the getting started page?

@brandon-gs
Copy link

Hello there @KyleSmith0905

Can this work if I only have my root page using swr and cached with cloudflare, but other pages using SSR?

@KyleSmith0905
Copy link
Contributor Author

It would make every page use client side auth, sorry. :/

I figured that if someone was using caching strategies, they'd have caching on many more places.

I'm not familiar with playing around with routeRules, but I can envision that I can put a property in there for disabling server side auth.

I agree that it should be done that using routeRules though. So I might work on the weekend to try to see if it would work.

@brandon-gs
Copy link

brandon-gs commented Dec 29, 2023

I would greatly appreciate it if you could do that! I want to implement this in a project that is already quite large and has too many routes, so I would like to progressively implement prerendering (SSG) or SWR on pages where I see it necessary and leave some using SSR if required. So using the disableServerSideAuth option in routeRules would be extremely helpful for me!

@KyleSmith0905
Copy link
Contributor Author

  • Added an auth object to routeRules. I kept in the global option I had before. The route rules takes priority over the module rules.

  • Added a cached page in the playground project for all of the 3 providers to demonstrate route-based rules.

  • Added a lot more documentation (a whole separate page).

If there are any issues, let me know. I changed a lot of lines of code. I couldn't find anything breaking.

@brandon-gs
Copy link

brandon-gs commented Dec 31, 2023

Thank you so much for your effort I really appreciate it @KyleSmith0905!

It appears that test-playground-auth is failing because, in playground-authjs/nuxt.config.ts, inside the auth configuration, the port in baseURL should be set to 3001.

@zoey-kaiser
Copy link
Member

Thanks @KyleSmith0905 for continuing to push this into the new year!
I plan on reviewing + hopefully merging next Monday. Meanwhile, @brandon-gs do you think you could also do a review of the request you had? I think this PR is almost over the finish line!

@KyleSmith0905
Copy link
Contributor Author

What's the next step in the review process?

@zoey-kaiser
Copy link
Member

What's the next step in the review process?

Hi @KyleSmith0905! I have passed this onto @BracketJohn to review and decide if this fits into our vision of NuxtAuth. Sadly he is pretty busy at the moment, resulting in some delays.

I cannot give you an ETA at the moment. Is this PR super pressing for you? If it is, I could look into making a beta release with this PR for you and then we do a retrospective review (I would obviously not prefer this, but it is one of the options).

@KyleSmith0905
Copy link
Contributor Author

That's fine, I can wait. I just wanted to make sure it wasn't entirely forgotten. All is good.

KyleSmith0905 and others added 14 commits March 24, 2024 11:37
@KyleSmith0905
Copy link
Contributor Author

I truly appreciate your feedback. Your feedback showed much greater foresight than I've considered when initially developing it.

Also, I believe I'm ready for another review when you're free.

I created live deployments if you would like to see how they function.
Auth.js provider: https://nuxtauth-authjs.vercel.app/with-caching
Local provider: https://nuxtauth-local.vercel.app/with-caching

Would be neat to have Vercel deployments integrated into the CI, though I don't know if there are any caveats (I can delete my projects if you want to use the project ids nuxtauth-authjs and nuxtauth-local).

@zoey-kaiser
Copy link
Member

Hi @KyleSmith0905 👋

Thanks for all your great work (and the demos) we really appreciate it! It makes reviewing so much easier ❤️
I will do a functional review, based on your demos, but as @phoenix-ru has already begun the code review process, I will leave the code review to him. He is currently on vacation and will be back next week.

He can then get around to a review around Thursday and we can then look to (finally) merge your PR 💪

zoey-kaiser
zoey-kaiser previously approved these changes Mar 27, 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 😊

Copy link
Collaborator

@phoenix-ru phoenix-ru left a comment

Choose a reason for hiding this comment

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

One additional thing - in the demos you provided I noticed that Server Render Time on the https://nuxtauth-authjs.vercel.app/with-caching page is always changing across reloads. Is your preview using normal SSR or swr?

Overall it lgtm, don't want to hold this any longer

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
src/runtime/utils/kit.ts Outdated Show resolved Hide resolved
@KyleSmith0905
Copy link
Contributor Author

KyleSmith0905 commented Apr 7, 2024

Thanks, for the reviews. I've made edits accordingly.

I'm using different branches for the demo URLs (that are synced with this PR's branch), not sure what the problem was with those, but I deployed again and fixed it.

@phoenix-ru phoenix-ru merged commit 11a22c5 into sidebase:main Apr 18, 2024
4 checks passed
@phoenix-ru
Copy link
Collaborator

@KyleSmith0905 Thank you for your PR! It was successfully merged and we will probably schedule it for the next patch release.

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

Successfully merging this pull request may close these issues.

5 participants