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

A good authentication story #178

Closed
Rich-Harris opened this issue Mar 8, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@Rich-Harris
Copy link
Member

commented Mar 8, 2018

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

This comment has been minimized.

Copy link

commented Mar 9, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2018

@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

This comment has been minimized.

Copy link

commented Mar 10, 2018

Okay, I'll have a think!

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

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

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

This comment has been minimized.

Copy link

commented Mar 12, 2018

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

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2018

@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.

@UnwrittenFun

This comment has been minimized.

Copy link

commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2018

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

  • Only need to change one line of code, even if multiple fetches
  • When creating separate api module, just need to pass fetch as a parameter => price of isomorphic js
  • No new Sapper specific methods (this.fetch)
  • Can be implemented as an express/... middleware express-session-fetch
@Crisfole

This comment has been minimized.

Copy link

commented Mar 15, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2018

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

This comment has been minimized.

Copy link

commented Mar 15, 2018

Rich-Harris added a commit that referenced this issue Mar 17, 2018

Rich-Harris added a commit that referenced this issue Mar 17, 2018

Merge pull request #202 from sveltejs/gh-178-store
add server- and client-side store management (#178)

Rich-Harris added a commit that referenced this issue Mar 17, 2018

Rich-Harris added a commit that referenced this issue Mar 19, 2018

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

commented May 4, 2018

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.

@Rich-Harris Rich-Harris closed this May 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.