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

Support SSR #256

Closed
raymondsze opened this issue Dec 12, 2021 · 14 comments
Closed

Support SSR #256

raymondsze opened this issue Dec 12, 2021 · 14 comments
Labels
enhancement New feature or request

Comments

@raymondsze
Copy link

There are plenty of code access the window object which make the server build fail. It doesn't happen in oidc-client-js.
For example, window.LocalStorage, even I provide a custom CookieStorage, as the source code default value is using window.LocalStorage, the code would fail.
Also, when I call userManager.getUser(), it init a timer which make use of window.setInterval and window.clearInterval, which make it fail in server side build.
Is it possible to change the source code to check if window object exists before access it?

@kherock
Copy link
Collaborator

kherock commented Dec 13, 2021

This library only targets browsers at this time, though there are a few unnecessary uses of window that can be removed. The current uses of window.setInterval and others are actually workarounds to some type errors with the initial TS port. We should be able to clear those up.

I'm not super confident that this library can gracefully target a fully server-side implementation in its current form. I think you're better off using panva/node-openid-client for server-side applications, though I'd be happy to review an implementation of a cookie-based storage adapter if you have one to contribute.

@pamapa
Copy link
Member

pamapa commented Dec 13, 2021

We should do the obvious things (e.g. deal with localStorage and sessionStorage), such that not everybody needs to do it like this: https://github.com/johnsonandjohnson/Bodiless-JS/blob/b9d9addcc9117ab6942306265deb87d832ace9fd/packages/bodiless-oidc/src/UserManager.ts#L49-#L63. The SSRStorage there is simply a memory storage, which we anyway already have: InMemoryWebStorage. I will provide a patch for this.

@raymondsze
Copy link
Author

raymondsze commented Dec 13, 2021

This library only targets browsers at this time, though there are a few unnecessary uses of window that can be removed. The current uses of window.setInterval and others are actually workarounds to some type errors with the initial TS port. We should be able to clear those up.

I'm not super confident that this library can gracefully target a fully server-side implementation in its current form. I think you're better off using panva/node-openid-client for server-side applications, though I'd be happy to review an implementation of a cookie-based storage adapter if you have one to contribute.

The main purpose of server-side rendering is to resolve the user from cookies on server-side so that we can base on the user authentication to render the corresponding content during SSR. Its different from node-openid-client which is really do the oidc process on server side. To be clear, SSR only need a similar functionality as the webstorage provided that can resolve the user (also check if the user is expired). My current way to do it is grab the webstorage from the user manager settings, and do get("user.<authority>.<client_id>") with JSON.parse to get the user.

And I use a trick to solve the UserManager problem with if (typeof window === 'undefined') global.LocalStorage = null.

@matthetherington
Copy link

You guys rock, thanks for taking on this project.

I'm using this in a static Next.js app via the typeof window === 'undefined' workaround mentioned above to defer instantiation until the code is running on the client.

The only issues I've ran into so far are the JSON parsing error in #257 which you beat me to fixing! The other is that it's a bit difficult to get oidc-client-ts working with next but it looks like you've already done a bit there too

If possible, it would be really nice if oidc-client-ts could be instantiated without accessing any browser-specific APIs as it would open it up to users of Next. The most popular next auth library requires you to deploy it along with a backing node service unfortunately; it won't work if you only deploy the static files to a CDN via next export.

@raymondsze
Copy link
Author

You guys rock, thanks for taking on this project.

I'm using this in a static Next.js app via the typeof window === 'undefined' workaround mentioned above to defer instantiation until the code is running on the client.

The only issues I've ran into so far are the JSON parsing error in #257 which you beat me to fixing! The other is that it's a bit difficult to get oidc-client-ts working with next but it looks like you've already done a bit there too

If possible, it would be really nice if oidc-client-ts could be instantiated without accessing any browser-specific APIs as it would open it up to users of Next. The most popular next auth library requires you to deploy it along with a backing node service unfortunately; it won't work if you only deploy the static files to a CDN via next export.

I use it with Next.js too. I would like to achieve the SSR if server is available on Next.js side. The only storage that server can access like what frontend do is cookie storage. I created a cookie storage with help of "universal-cookie", it share the same api interface between frontend and backend. Then I pass it to the user manager settings to initialize the user manager in server side. Frontend rendering use the reaact-cookie to access the "universal-cookie". Assuming both stored cookies are the same, the SSR should match the one in Frontend side.

@brockallen
Copy link
Contributor

Sorry for butting in here, but I never understood this request. If you're doing SSR, then why aren't you using a server-side library? Given the session management and protocol mechanics, a SSR app should do things differently than a client-side JS app.

Also, unrelated, I never understood why people wanted to put their access tokens into a cookie from a client-side app -- you're just exposing them to potential leakage and putting end users at more risk.

But, admittedly, I don't understand how some of the JS frameworks blend and blur the line between SSR and how their client-side frameworks do their rendering.

But yea, my feedback here is really think thru where you're exposing the tokens and if that's really what you want.

@matthetherington
Copy link

matthetherington commented Dec 14, 2021

@brockallen We aren't actually using the SSR functionality in next - ie there is no node backend serving our app.

Next allows you to "static export" a site, so it will render your app at build time, emit your site as static files and then you can just serve it from a CDN.

https://nextjs.org/docs/advanced-features/static-html-export

So we can't make use of any of the server-side libraries unfortunately.

Also, unrelated, I never understood why people wanted to put their access tokens into a cookie from a client-side app -- you're just exposing them to potential leakage and putting end users at more risk.

Completely agree. If we had a server then I'd go for a regular authorisation code flow and keep the tokens server side, but we only have a SPA served from a CDN, and our API. For that situation, an authorisation code flow with PKCE is the preferred approach, which exposes the tokens to the client. I'd choose a different approach personally but it's not my call.

@brockallen
Copy link
Contributor

Next allows you to "static export" a site, so it will render your app at build time, emit your site as static files and then you can just serve it from a CDN.

Ah, ok, I think I understand -- so the build process actually runs some of the app and captures the DOM and turns that into static files?

Again, sorry for the tangent to educate me on this thread.

@kherock
Copy link
Collaborator

kherock commented Dec 14, 2021

^ @brockallen I also want to echo your concerns, I appreciate the feedback. I think what's maybe attractive about this library compared to others designed for servers is that it has a good client-side API for initiating an authorization code flow. It's also possible to initiate a server-side flow this way using cookies, but it feels wrong to use JS-based cookies, especially for auth.

If you're on Next.js using server rendering, the client shouldn't need any auth/user context beyond the data that you supply with getServerSideProps. If you need to initiate a server-side authorization code flow, the client-side app probably shouldn't be the one doing that, the server should via a page with getServerSideProps that sets an httpOnly cookie and returns a redirect.

@matthetherington your use case is the same as mine, my main motivator for maintaining this library is actually for a couple of static Next.js apps. My question for you is what would an "instantiated" client instance look like? Currently when you construct a UserManager, it fetches metadata from the configured authority. During a static build, this would be a waste of resources because we don't have a way to serialize that data for the UserManager instance on the client to reuse.

@matthetherington-pa
Copy link

matthetherington-pa commented Dec 15, 2021

@brockallen yep exactly that. Next does an initial first-pass render, which gets emitted as static files. The client requests the HTML along with the JS, and renders the HTML right away. When the JS has loaded it becomes interactive.

@kherock I mostly write C# code day-to-day and you can't have async constructors in that, so stuff like you are describing usually goes in a method that you call following instantiation. Instantiating a class is normally side-effect free.

So translated to this, I'm imagining an API (in react) something along the lines of:

// can be instantiated on the server without erroring
const userManager = new UserManager(config); 

const App = () => {
  //useEffects only get executed on the client-side
  useEffect(() => { 
    userManager.fetchMetadata()
  }, []);
  
  const redirectToLogin = () => {
    userManager.signinRedirect();
  };
  
  <button onClick={redirectToLogin}>Login</button>
};

export default App;

@brockallen
Copy link
Contributor

Next does an initial first-pass render, which gets emitted as static files. The client requests the HTML along with the JS, and renders the HTML right away. When the JS has loaded it becomes interactive.

So then how does that work if the HTML needs to render the user's display name (or something else if not logged in)? That's a challenge when this server rendered stuff doesn't have access to the session.

@pamapa
Copy link
Member

pamapa commented Dec 15, 2021

Minimal SSR support is now merged, i wont do more than that. I fully agree with @brockallen and @kherock!

The library can now be used to "static export" for the likes of search machine requests or other request where no user login is required.

@matthetherington-pa
Copy link

matthetherington-pa commented Dec 16, 2021

So then how does that work if the HTML needs to render the user's display name (or something else if not logged in)? That's a challenge when this server rendered stuff doesn't have access to the session.

@brockallen it doesn't 😄 like you said, you'd need a server with access to the session to do that, which would dynamically generate the markup and send it down to the client.

As it's just static files served from a CDN, it's the same markup for every user. So for the authenticated pages, we are pretty much just serving up a loading spinner via the html that's in the CDN and then when the JS bundle is loaded it'll redirect to the OIDC Provider. When the OIDC Provider redirects back and we have the user's identity, then the client-side JS can render their display name to the DOM.

The benefits are more tangible for the pages that don't require auth but you still get a much quicker first-paint. There's a good write-up of the performance aspects here: https://blog.logrocket.com/next-js-vs-create-react-app/

It's talking about SSR but a lot of the same benefits apply, minus the server-side data fetching.

@pamapa
Copy link
Member

pamapa commented Dec 16, 2021

I think we now have done what can be done. I will close this issue unless somebody speaks up.

@pamapa pamapa changed the title Support SSR with cookiesStorage Support SSR Dec 16, 2021
@pamapa pamapa added the enhancement New feature or request label Dec 16, 2021
@pamapa pamapa closed this as completed Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants