sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
6.99k stars 433 forks source link

A good authentication story #178

Closed Rich-Harris closed 6 years ago

Rich-Harris commented 6 years ago

Very broad issue title I know. Auth is a little tricky in the context of a universal framework. Currently I will often do this sort of thing:

// app/client.js
import * as auth from '../shared/auth.js';
import { init } from 'sapper/runtime.js';
import { routes } from './manifest/client.js';

auth.init().then(() => {
  init(document.querySelector('#sapper'), routes);
});
// shared/auth.js
export let user;

export function init() {
  return fetch(`/auth/user`, { credentials: 'include' })
    .then(r => r.json())
    .then(data => {
      user = data.user;
    });
}

// ...
<!-- routes/somepage.html -->

<script>
  import * as auth from '../shared/auth.js';

  export default {
    preload({ params, session }) {
      const user = session ? session.user : auth.user;
      if (!user) return this.redirect(302, '/login');

      return fetch(...).then(...);
    }
  };
</script>

This works, but it's not ideal. In particular, it doesn't work well with Store, because (since a single store instance backs every server-rendered page) you can't have a user object in your store, so you have to pass it around as a prop which is tedious (and problematic when it comes to logging out).

It would be nice to have a good story around this that felt idiomatic, worked on client and server without letting you accidentally do insecure things, and worked well with Store. Open to ideas!

ISNIT0 commented 6 years ago

Would love to see some progress on this, I've been futsing around with PassportJS and getting very upset about losing sessions - Now I can't prefetch on authenticated pages!

Anything I can help with on this issue?

Rich-Harris commented 6 years ago

@ISNIT0 At this stage it's more about figuring out potential solutions could look like at a high-level, rather than implementation work, so if you have any ideas for what a nice approach would look like then please share! You raise a good point about preloading for authenticated pages — I assume you're talking about this sort of thing...

preload({ ... }) {
  return fetch(`/someroute`, { credentials: 'include' }).then(...)
}

...where credentials: 'include' doesn't mean anything during SSR? I've been bitten by that and I haven't figured out a good solution.

ISNIT0 commented 6 years ago

Okay, I'll have a think!

Yes, you're right - that's what I was fighting... bit of a pain :)

Rich-Harris commented 6 years ago

Just thinking out loud for a moment... on the server, fetch doesn't send credentials, because that doesn't make sense in the context of node-fetch.

For a component like this...

<script>
  export default {
    preload({ params }) {
      return fetch('/auth/me.json', { credentials: 'include' })
        .then(r => r.json())
        .then(user => {
          return { user };
        });
    }
  };
</script>

...Svelte generates something like this:

function preload({ params }) {
  return fetch('/auth/me.json', { credentials: 'include' })
    .then(r => r.json())
    .then(user => {
      return { user };
    });
};

var SvelteComponent = {};

// ...

SvelteComponent.preload = preload;
return SvelteComponent;

If it instead generated this...

export default function(fetch) {
  function preload({ params }) {
    return fetch('/auth/me.json', { credentials: 'include' })
      .then(r => r.json())
      .then(user => {
        return { user };
      });
  };

  var SvelteComponent = {};

  // ...

  SvelteComponent.preload = preload;
  return SvelteComponent;
}

...then inside Sapper, we could do this sort of thing:

-const mod = route.module;
+const mod = route.module(configureFetch(req));

// later...
Promise.resolve(
  mod.preload ? mod.preload.call({
    redirect: (statusCode: number, location: string) => {
      redirect = { statusCode, location };
    },
    error: (statusCode: number, message: Error | string) => {
      error = { statusCode, message };
    }
  }, req) : {}
).catch(err => {
  error = { statusCode: 500, message: err };
}).then(preloaded => {
  // ...
});

In other words we could simulate sessions on the server by 'injecting' 'globals' into the component's scope. Feels very specific to this problem, but perhaps it applies to other things too. Would obviously require some changes to Svelte itself. A possible API:

const result = svelte.compile(source, {
  generate: 'ssr',
  wrap: ['fetch']
});

Would need to consider the impact on performance.

This doesn't solve the Store problem, of course.

Rich-Harris commented 6 years ago

Actually, scratch the above — we don't need to wrap the entire component, just preload, since methods and lifecycle hooks don't run on the server.

Rich-Harris commented 6 years ago

or — galaxy brain — fetch could be passed into preload:

<script>
  export default {
    preload({ params, fetch }) {
      return fetch('/auth/me.json', { credentials: 'include' })
        .then(r => r.json())
        .then(user => {
          return { user };
        });
    }
  };
</script>

On the browser it would just be window.fetch; on the server it would be a wrapper function that knew how to handle credentials: 'include'. This is nice and explicit and non-magical, and can easily be implemented without any controversial changes to Svelte itself.

(Still doesn't solve the Store problem, but it's orthogonal so worth doing anyway.)

ISNIT0 commented 6 years ago

I like this approach - makes it quite clear that fetch is special/different, but also without it being - as you say - magic.

Rich-Harris commented 6 years ago

@thgh noted that mixing properties in with the Request object might be a bit weird, so a couple of other possibilities:

// a second argument
export default {
  preload({ params }, { fetch }) {
    return fetch('/auth/me.json', { credentials: 'include' }).then(...)
  }
};

// using this.fetch
export default {
  preload({ params }) {
    return this.fetch('/auth/me.json', { credentials: 'include' }).then(...)
  }
};

The this.fetch option dovetails with the existing this.error and this.redirect methods.

jamesbirtles commented 6 years ago

None of these really work if you do the fetching outside of the component scope, e.g. in an imported function, method call, or buried deep in a library somewhere, which I think would be highly likely particularly in a larger app.

thgh commented 6 years ago

All in all, my vote goes to the first method preload ({ params, fetch })

Crisfole commented 6 years ago

I think there may be two separate issues that can be resolved:

  1. How to pass additional information via fetch.
  2. How to store that information in an interface accessible on both server and client in a manner specific to the current user. (Easy for client: just create a custom store that only loads on the client, a smidge harder on the server where you want to store a map from session token to the data that gets loaded on the client, but only allow access to the data the client has).

I'm not opposed to 1 being fixed with an additional parameter in preload, although I don't think that'll really solve the whole issue. Honestly I'd rather have a global hook for modifying fetch. (Something akin to JQuery's ajaxSetup).

How about making this a plugin, so it doesn't clutter Sapper or Svelte:

  1. Create a SessionStore class. We've already got sigils for shared store data ($) so double it for session information: $$user or use an ampersand: {{@user}}. The SessionStore would have to be initialized with a well defined interface for defining all the pieces that go into auth, but in a manner that allows each user to completely customize how it works. Fair warning: I don't speak TypeScript well, but I very much admire it. Also not sure about SessionType : Store...
interface SessionStrategy<KeyType, SessionType> where SessionType : Store {
  // Session Keys: Not necessarily a string, but most likely one.
  // Can be implemented on both client and server (JWT or similar for client).
  // Optional if this strategy doesn't need it:
  // (e.g.: setSessionKey not needed on client for cookie based sessions).

  // Get Session Key. (Assumes same params as `preload`)
  getSessionKey?({ params : any, query : any, request?: express.Request }) : Promise<KeyType>;

  // Set Session Key (by cookie, or magic, none of sapper's business).
  // This may require extra parameters so the request/stores can be altered.
  setSessionKey?(key: KeyType): Promise;

  // Fetch a Session, key can be null on client, required on server (to specify which)
  getSession(key?: KeyType) : Promise<SessionType>;

  // Create a session, returns session key (can be null on client).
  createSession(val: SessionType) : Promise<KeyType>

  // Update a Session, key can be null on client
  updateSession(key?: KeyType, val: SessionType) : Promise;

  // Delete the session, key can be null on client
  expireSession(key: KeyType) : Promise;

This would have to be implemented twice, once for the client and once for the server.

I'm still not 100% sure on the details, that's just first stab. But with this you'd have:

class SessionStore<K, T> extends Store {
  constructor({ client : SessionStrategy<K, T>, server : SessionStrategy<K, T> }) {
    this.strategy = process.browser ? client : server;
  }

  get(key, preloadParams) {
    var sessionKey = this.strategy.getSessionKey && this.strategy.getSessionKey(...preloadParams);
    var session = this.strategy.getSession(sessionKey);
     // This assumes Sessions are store implementations.
     // I can be persuaded a simple indexer is ok here.
    return session.get(key);
  }

  // ... rest of implementation similar...
}

Authentication/Authorization is tricky...

Rich-Harris commented 6 years ago

We had some further chats on this in the Gitter room, and hit upon a plan that seems workable:

// app/server.js
import fs from 'fs';
import polka from 'polka';
import compression from 'compression';
import sapper from 'sapper';
import serve from 'serve-static';
import authenticationMiddleware from './authenticationMiddleware.js';
import { Store } from 'svelte/store.js';
import { routes } from './manifest/server.js';

polka()
  .use(compression({ threshold: 0 }))
  .use(authenticationMiddleware()) // imagine it delegates to passport
  .use(serve('assets'))
  .use(sapper({
    routes,
    store: req => {
      return new Store({ // or `MySubclassedStore` or whatever
        user: req.session.passport && req.session.passport.user
      });
    }
  }))
  .listen(process.env.PORT);
// app/client.js
import { init } from 'sapper/runtime.js';
import { Store } from 'svelte/store.js';
import { routes } from './manifest/client.js';

fetch(`/auth/me.json`).then(r => r.json()).then(user => {
  // user would be null if not logged in
  const store = new Store({ user });
  init(target, routes, { store });
});

In other words, on the server there'd be a store per-request. On the client, it would be shared by all components. As well as allowing secure server-side authentication, it would remove the boilerplate currently associated with using Store in Sapper apps.

One remaining question is whether we'd want to serialize the server-side store for reinitialization on the client, similarly to how we reuse preload data. I guess that could look like this:

// app/client.js
import { init } from 'sapper/runtime.js';
import { Store } from 'svelte/store.js';
import { routes } from './manifest/client.js';

init(target, routes, {
  store: data => new Store(data)
});
Crisfole commented 6 years ago

I'd vote Yes on reinitializing

On Thu, Mar 15, 2018, 4:48 PM Rich Harris notifications@github.com wrote:

We had some further chats on this in the Gitter room, and hit upon a plan that seems workable:

// app/server.jsimport fs from 'fs';import polka from 'polka';import compression from 'compression';import sapper from 'sapper';import serve from 'serve-static';import authenticationMiddleware from './authenticationMiddleware.js';import { Store } from 'svelte/store.js';import { routes } from './manifest/server.js'; polka() .use(compression({ threshold: 0 })) .use(authenticationMiddleware()) // imagine it delegates to passport .use(serve('assets')) .use(sapper({ routes, store: req => { return new Store({ // or MySubclassedStore or whatever user: req.session.passport && req.session.passport.user }); } })) .listen(process.env.PORT);

// app/client.jsimport { init } from 'sapper/runtime.js';import { Store } from 'svelte/store.js';import { routes } from './manifest/client.js'; fetch(/auth/me.json).then(r => r.json()).then(user => { // user would be null if not logged in const store = new Store({ user }); init(target, routes, { store }); });

In other words, on the server there'd be a store per-request. On the client, it would be shared by all components. As well as allowing secure server-side authentication, it would remove the boilerplate currently associated with using Store in Sapper apps.

One remaining question is whether we'd want to serialize the server-side store for reinitialization on the client, similarly to how we reuse preload data. I guess that could look like this:

// app/client.jsimport { init } from 'sapper/runtime.js';import { Store } from 'svelte/store.js';import { routes } from './manifest/client.js'; init(target, routes, { store: data => new Store(data) });

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/178#issuecomment-373517843, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZONdf5hI6VjA_uCm6dmUV_IxBm5llMks5tetOJgaJpZM4Sjede .

Rich-Harris commented 6 years ago

Closing this, as we have both this.fetch and good support for Store. The fact that you have to pass around this.fetch to your internal API module/whatever is a nuisance, but I think it's basically unavoidable.

jobelenus commented 4 years ago

I've been playing around with auth in sapper, and I think there are some elements of this story missing. I'm not even close to an expert on Sapper, so I could easily be doing this all wrong.

So far, I have not been able to see how to redirect a request on the server. In SSR applications this is how devs expect to be able to work. I tried a res.redirect('/login') in a middleware in server.js that does not appear to work. And I don't believe we can redirect in the module prefetch either, correct (I honestly didn't even try it)?

This means we have to "redirect" on the client (which really means, load the login components). The typical way to "redirect" is to alter the window.location = '/login'. In order to access window, we have to be inside onMount but that means that we've already rendered content—which means a flash of content on whatever page they were loading.

In terms of developer experience it would also mean importing that "are you logged in" check into every single route. Doing this redirect in server.js could be a far more pleasing experience. I am looking to make this as good a developer experience as the rest of svelte and sapper are 😄

antony commented 4 years ago

Hi @jobelenus - I'm going to release a Sapper auth example soon, a recreation of the one we use on https://beyonk.com. I'll add a blog post / recipe on this when I've done it.

If you need some more help with auth come and chat to us in discord, but to address your immediate problems - have a look at my talk here: https://antony.github.io/svelte-meetup-talk-oct-2019/#1 and the attached demo app, and also take a look at @beyonk/sapper-rbac for robust route protection

jobelenus commented 4 years ago

Sounds good. I've been able to do everything I am looking to do—with the exception of redirect from the server side w/o a browser render/flash-of-content.

frederikhors commented 4 years ago

@antony any news on this?

kevmodrome commented 4 years ago

Antony did a talk at Svelte Society Day 2020 that you can watch here: https://www.youtube.com/watch?v=E47uUYvlhQc that is aclled 'Authentication with Sapper' 👍

lgs commented 4 years ago

@kevmodrome @Rich-Harris @natevaughan see also https://github.com/antony/sapper-authentication-demo

jobelenus commented 4 years ago

I'm taking a look at the code, and I don't see an http redirect happening if the user is not authenticated? Am I misunderstanding something?

allyraza commented 4 years ago

Safety Note I just finished watching the talk, please be careful with the examples from the talk (this one to be precise https://github.com/antony/sapper-authentication-demo) it is not very well thought out implementation It does not protect the routes it just hides them, using 2 different packages for jwt signing and verification could lead to unintended consequences ...

natevaughan commented 4 years ago

Watched @antony's talk and just want to quietly make the observation that much of the authentication complexity comes from using Sapper as an active middleware server. You have to think about authenticating both server and the client, which excludes a few authentication use cases and makes others more complex.

Everything gets simpler if you stop using Sapper as middleware and commit to the static export approach. Hydrate what you can know at build time (e.g. static data) at build time and hydrate what you can only know at runtime (e.g. user data) at runtime, client-side. Middleware has few if any advantages and adds whole lot of complexity, which becomes especially obvious in authentication.

jobelenus commented 4 years ago

I don’t think it makes anything simpler. You still have to auth. Doesn’t matter if it’s a second service, or Sapper.

On Sat, May 2, 2020 at 3:56 PM Nate Vaughan notifications@github.com wrote:

Watched @antony https://github.com/antony's talk and just want to quietly make the observation that much of the authentication complexity comes from using Sapper as an active middleware/rendering server. You have to think about authenticating both server and the client, which excludes a few authentication use cases and makes others more complex.

Everything gets simpler if you stop using Sapper as middleware and commit to the static export approach. Hydrate what you can know at build time (e.g. static data) at build time and hydrate what you can only know at runtime (e.g. user data) at runtime, client-side. Middleware has few if any advantages and adds whole lot of complexity, which becomes especially obvious in authentication.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/178#issuecomment-623005541, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQ7JK67CBHYK2W2JQOWULRPR3HBANCNFSM4EUN45PA .

natevaughan commented 4 years ago

Sure things get simpler if you ditch the middleware. You just auth the client with the resource server and don't have to think at all about the middleware server, what domain it sits on, and whether it has access to the cookies it needs to also access the resource server.

antony commented 4 years ago

@allyraza thanks for your thoughts. However you're wrong on both points:

The RBAC middleware does hide (prevent navigation) to your routes, which is the intent, since all secret data is securely protected in the API, not in the application.

The use of "two different packages" is also an incorrect conclusion. The hapi-auth-jwt2 plugin is a Hapi authentication plugin, which deals only with authentication in Hapi. The jsonwebtoken module is for creating and signing JWT, and verifying signatures.

Besides, even if I was using two different libraries, JWT is a standard, which means that both implementations would be compatible.

antony commented 4 years ago

@natevaughan you are of course correct, but my talk is about authentication in Sapper, the solution you propose has more build and infra complexity. The talk is intended for a person wanting to authenticate a regular Sapper app.

Tradeoffs.

antony commented 4 years ago

@jobelenus The demo app doesn't do any route protection. The discussion and implementation of route protection is discussed on the second to last slide via the @beyonk/sapper-rbac plugin, which is outside the scope of the talk.

This is probably why @allyraza is confused too. The demo doesn't have any route protection, just conditionall rendered links.

natevaughan commented 4 years ago

the solution you propose has more build and infra complexity

Disagree completely. What's simpler to deploy than a single build command that generates folder full of static assets that can be deployed on literally any file server or CDN in the world?

The only reason the Sapper middleware approach could be considered a "regular Sapper app" as you call it is that the docs are written with it as the default way to get started. Wish this community would stop talking about the middleware approach as the default.

antony commented 4 years ago

@natevaughan ah I understand. Yes this is an approach, but you lose any SSR support for authenticated pages, which wouldn't work for us, and I assume, a number of other use cases.

On Sat, 2 May 2020 at 21:33, Nate Vaughan notifications@github.com wrote:

the solution you propose has more build and infra complexity

Disagree completely. What's simpler to deploy than a single build command that generates folder full of static assets that can be deployed on literally any file server or CDN in the world?

The only reason the Sapper middleware approach could be considered a "regular Sapper app" as you call it is that the docs are written with it as the default way to get started. Wish this community would stop talking about the middleware approach as the default.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/178#issuecomment-623009718, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABVORPN6TUSHM3DDHQOY2DRPR7THANCNFSM4EUN45PA .

--


ꜽ . antony jones . http://www.enzy.org

natevaughan commented 4 years ago

What's one advantage that server side rendering brings to authenticated pages? Don't say SEO.

And I should add that you're only talking about the first view. Because every subsequent view is going to be rendered client-side with Sapper anyway.

edit I don't mean to come off as combative here, I have nothing but love for the Svelte and Sapper creators and volunteers and appreciate the discussion here. I'm on this boat, for better and/or worse.

antony commented 4 years ago

What's one advantage that server side rendering brings to authenticated pages? Don't say SEO.

Loading speed, progressive enhancement.

natevaughan commented 4 years ago

Loading speed

Really doubt you're going to get faster with middleware hydrating your data on a nodejs server at runtime than a ready-to-go static asset on a CDN near you (e.g. one generated by sapper export or routify build) and then hydrating the dynamic content client-side. Static assets can be cached all over the world and even in your browser and served by purpose built file servers that are the fastest in the world for what they do

I mean you certainly could devise a scenario where adding a middleware nodejs server to fetch and pre-hydrating the user's data would faster (again remember this is only on a hard refresh or the very very first view a user requests). But every one I can come up with is pretty far-fetched.

progressive enhancement

Actually, that's exactly where pre-hydrated static assets shine. You get a view (almost instantly I might add, no waiting on upstream data) that contains everything we already know what it should look like and just fill in the remaining dynamic parts client-side. I mean, that's exactly what Sapper does on every subequent view after the first one anyway.

Middleware has few, if any advantages for serving authenticated data, and tons of downsides.

jobelenus commented 4 years ago

Nate, I really dont know why you’re having the argument you’re having. Authentication needs to happen on a backend, period. Whether it’s a dapper backend, or other backend. Talking about CDN delivery is quite literally not the point here

Thank you Antony, I will check out that linked package!

On Sat, May 2, 2020 at 5:11 PM Nate Vaughan notifications@github.com wrote:

Loading speed

Really doubt you're going to get faster with middleware hydrating your data on a nodejs server at runtime than a ready-to-go static asset on a CDN near you (e.g. one generated by sapper export or routify build). Static assets can be cached all over the world and even in your browser and served by purpose built file servers that are the fastest in the world for what they do

I mean you certainly could devise a scenario where adding a middleware nodejs server to fetch and pre-hydrating the user's data would faster (again remember this is only on a hard refresh or the very very first view a user requests). But every one I can come up with is pretty far-fetched.

progressive enhancement

Actually, that's exactly where pre-hydrated static assets shine. You get a view that contains everything we already know what it should look like and just fill in the remaining dynamic parts client-side. I mean, that's exactly what Sapper does on every subequent view after the first one anyway.

Middleware has few, if any advantages for serving authenticated data, and tons of downsides.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/178#issuecomment-623013740, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQ7JKBABXZIL5K7L4OIUTRPSEBFANCNFSM4EUN45PA .

antony commented 4 years ago

All good. Thanks for your input and feedback everyone. We have done this to death. The talk is there for information and as a Base for your own experimentation.

For further discussion find me on the Svelte discord.

Locking this thread.