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

⚠️ Issue - On demand rendering for protected page #6884

Closed
eric-burel opened this issue Feb 14, 2024 · 6 comments
Closed

⚠️ Issue - On demand rendering for protected page #6884

eric-burel opened this issue Feb 14, 2024 · 6 comments
Assignees

Comments

@eric-burel
Copy link
Contributor

eric-burel commented Feb 14, 2024

Hi folks,

I am checking out the rendering modes documentation, and I've noticed that they on-demand rendering is recommended for protected pages. I've studied this topic a lot, and I think this recommendation is not correct.
On-demand rendering is necessary when your render depends on the specific content of an HTTP request. However, when protecting a route, you don't really use the request specificity, but rather a derived value shared by many users : "is the current user authorized", creating two groups, authorized users and the others.

In this situation, static rendering is actually the optimal choice. You simply have to couple it with an upstream access check. In Next.js this is done via an Edge middleware or a custom server. In Remix you'd do that with a custom entrypoint, or whatever proxy you use upstream if you can't use a custom entrypoint. Edit : I see you have middlewares out-of-the-box too, so they can be used already for the same goal. I am however not totally yet confident with how it would work on Vercel according to this doc, but that's besides the point.

If people prefer on-demand rendering for protected pages, it's mostly because this kind of protected static rendering is not easy to setup. But that's because the current generation of frameworks bases static rendering upon the URL only, which is not totally optimal, this idiosyncracy doesn't make them right.

I suggest reviewing this page of the documentation

  • "The same HTML document is sent to the browser for every visitor" => maybe just give a hint that with a more complex setup, it can be done for groups of users too
  • "Protected pages: Restrict access to a page based on user privileges, by handling user access on the server." => maybe also give a hint that in certain scenarios, if the protected content is the same for multiple users, static rendering can be used
  • And finally add a section about static rendering with protected content => basically you have to verify authorization before the user hits the URL, and redirect them accordingly. Works great if your app is behind a proxy or whatever upstream security check you may have.

A few resources:

@lilnasy
Copy link
Contributor

lilnasy commented Feb 22, 2024

Thanks for bringing this up. Protecting statically generated pages is a problem we are actively considering our options for. It comes up frequently from starlight users.

You are right, edge middleware would be a better tool for the job than on-demand rendering the entire site. However, I have to hesitate in making it the recommendation. Very few of our users are deploying on netlify and vercel, which are the only two platforms where it works out of the box.

I am however not totally yet confident with how it would work on Vercel according to this doc, but that's besides the point.

It seems like that page could be edited (or the adapter's behavior adjusted), because vercel-edge-middleware.js and <root>/middleware.ts are relevant only to very specific use-cases. Generally, you would make src/middleware.ts run as an edge middleware, and it happens automatically when you set edgeMiddleware to true.

@eric-burel
Copy link
Contributor Author

eric-burel commented Feb 22, 2024

@lilnasy regarding edge middlewares, you could actually replace that by any kind of server as long as it's upfront to your app. In Next.js for instance you could simply use a custom server, so a classical Node.js server, and add a middleware (a Connect middleware not an edge one, so a (req, res, next) function) just before the ones that returns the right page.
It's just that doing the check at page-level is slightly too late, anything before that does the job, eventhough edge middlewares are indeed optimal.

/middleware.ts are relevant only to very specific use-cases

If I understand Astro's middleware to be very similar to Next middlewares, to me static personnalisation is exactly one of the valid specific use-cases, similarly to A/B testing etc.

@sarah11918
Copy link
Member

I will also thank you for bringing this up, and comment here that we are actively working on this kind of issue as we investigate a proper authenticated Starlight setup: where your content is static (docs, articles etc.) but you want to restrict access (e.g. to paid membership for a course).

We don't yet have a good way to do this, but @TheOtterlord is exploring our options on this front to see what the "happy path" in an Astro project would be. I'm mentioning him here to put your links on his radar!

When we have figured out an implementation that we like, and have it working for Starlight, then we will absolutely be publishing advice for this. Until then, we are not comfortable adding content to the docs. We do have a saying that "not everything that is true belongs in docs" and a problem that in all likelihood has a solution, but which we have not yet solved ourselves, counts as something that does not necessarily set our audience up for success by mentioning it. 😄

I will let @TheOtterlord decide whether having this issue open, for further discussion on the topic, is helpful. Or, if this is closed, to at least mention this issue when a PR is eventually made.

Normally issues are used as an "inbox" in docs, and if an issue is already being worked on or we have a plan to address it, we will tend to close the issue. (Not because it is solved, but because it's in active development, we don't need the reminder!) Otterlord has been evaluating potential solutions for this for some time, so this is not only a great suggestion but also something that we hope to have a solution for that we are comfortable with and confident!

@eric-burel
Copy link
Contributor Author

eric-burel commented Mar 4, 2024

Thanks for you answer @sarah11918, no problem for closing the issue while this is being worked on, this architecture needs a bit of thinking indeed, in particular you may want the authentication + redirection to be less expensive than doing a naive check while rendering dynamically, which is not actually guaranteed and may depend on the underlying infrastructure (eg if you check payments from a DB, your VM running the app might end up being closer to the database than an upfront proxy and you get better perfs despite using a dynamic render).
By the way I've published a more up-to-date summary for Next.js comparing alternatives: Secure statically rendered paid content in Next.js (with the App Router)

@lilnasy
Copy link
Contributor

lilnasy commented Mar 4, 2024

@eric-burel These are all great points of feedback, thanks!

Roadmap discussions would be a more visible place for them. It's where we expect both the maintainers, and the interested users to give feedback on features. Docs get involved at a later point.

@sarah11918
Copy link
Member

Now that this has been put on everyone's radar, I'll close the issue so that no one jumps in thinking there's something actionable here at the moment! Thanks for the discussion everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants