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

sessions #46

Closed
Rich-Harris opened this issue Oct 20, 2020 · 17 comments
Closed

sessions #46

Rich-Harris opened this issue Oct 20, 2020 · 17 comments
Labels
feature request New feature or request
Milestone

Comments

@Rich-Harris
Copy link
Member

Something people seem to trip over a bit is the fact that session, despite being a writable store, doesn't get persisted. I wonder if we can address that:

<script>
  import { stores } from '$tbd';
  const { session } = stores();

  let name;

  async function update_username(name) {
    // optimistic — update the client-side store, then persist
    // (rolls back in case of failure)
    session.update($session => ({
      ...$session,
      user: { ...$session.user, name }
    }));

    session.persist();

    // pessimistic — wait until success before updating
    // client-side store
    session.persist($session => {
      ...$session,
      user: { ...$session.user, name }
    });
  }
</script>

<!-- pretend i did this properly, with a progressively enhanced <form> -->
<input bind:value={name}>
<button on:click={() => update_username(name)}>
  Update
</button>

This requires that the developer add some persistence logic. Perhaps in the same file where we define logic for getting a session (#9), we have a place to put logic for persistence:

// src/session/index.js
import { parse } from 'cookie';
import * as db from './db.js';

export async function get(headers) {
  const cookies = parse(headers.cookie);
  const user = await db.get(cookies.session_id);
  return { user };
}

export async function persist(headers, data) {
  const cookies = parse(headers.cookie);
  const user = await db.set(cookies.session_id, data); // validate and store
  return { user };
}

Glossing over some details but what do folks think?

@Rich-Harris
Copy link
Member Author

I suppose it doesn't even really need to be session.persist, it could just be session.set, which persists automatically if you've created that function (and rolls back automatically, etc).

That way you could do

<script>
  import { stores } from '$tbd';
  const { session } = stores();

  let name;

  async function update_username(name) {
    // optimistic — update the client-side store, then persist
    // (rolls back in case of failure)
+    $session.user.name = name;
-    session.update($session => ({
-      ...$session,
-      user: { ...$session.user, name }
-    }));
-
-    session.persist();

    // pessimistic — wait until success before updating
    // client-side store
-    session.persist($session => {
+    session.update($session => {
      ...$session,
      user: { ...$session.user, name }
-    });
+    }, { optimistic: false });
  }
</script>

<!-- pretend i did this properly, with a progressively enhanced <form> -->
<input bind:value={name}>
<button on:click={() => update_username(name)}>
  Update
</button>

(if we did that, we'd probably want to export get and set from src/session/index.js rather than get and persist)

@Conduitry
Copy link
Member

To persist (🙄) my comments from chat:

This doesn't immediately seem like a silly idea.

Having the server call get anew to get the persisted value (rather than relying on the value returned by persist/set) seems safer, even though it's slightly less efficient. We don't want to turn the current confusion of 'I updated $session on the client, why didn't it persist through a page reload?' to the more complicated 'I updated $session on the client, and wrote a persist function that I incorrectly thought was persisting the data, why didn't it persist through a page reload?'

Perhaps persist/set should also receive the current session value as an parameter, so that it can tell what it's being asked to change.

We'd want to be able to only include the client-side code for handling this if the app specifies a server-side persist/set function anyway, so this is something that would be safe to push to post-launch.

@benmccann
Copy link
Member

I'm not really following the example. The first bit of code looks like a .svelte file, but then it's calling the session.persist which writes to the database, and seems like it would not work on the client-side. I wouldn't expect any writable stores to be persisted to the database because one can be a client-side thing and the other can't be.

I think the biggest issue with session that I've seen people trip up over is that when you change it, it re-renders your app which can be unexpected.

@Rich-Harris
Copy link
Member Author

@benmccann session.persist(...) POSTs to a generated endpoint that calls the set/persist/whatever function the developer adds to src/session/index.js. Generally speaking you wouldn't call it during SSR, it's something that would be called inside event handlers. (Maybe it should even be an error to call it during SSR.)

I think the biggest issue with session that I've seen people trip up over is that when you change it, it re-renders your app which can be unexpected.

It's a writable store! If someone doesn't want it to trigger reactivity then... they shouldn't write to it :)

@arxpoetica
Copy link
Member

I'm curious about this. Like @Conduitry said, it's probably not a bad idea. I could go either way. On the one hand, implementing session behavior might arguably be something someone should do as part of setting up auth or something related, and not necessarily a part of SvelteKit.

On the other hand, it's also a frequently enough employed use case that having a blessed way of handling it is probably a good idea.

Agree about either the update or get approach. Not sure which I agree with. Probably need to see more implementation. 😆

@ehrencrona
Copy link
Contributor

It would be helpful to think about this in terms of which use cases it would support. The Sapper docs and the examples above only talk about storing the current user, but if that's the main use case maybe there should be more specialized support for authentication/authorization instead?

You could imagine using this for storing settings (e.g. dark mode) you want available on the server side, but as soon as you have authentication you'd want to store the settings with the user rather than the session.

What other use cases are there?

A couple of other aspects:

  • if we offer endpoints for storing and retrieving potentially personal data we'd need to worry about security; only keying it on a random session ID feels potentially dangerous
  • would we need to worry about to how to clear old, expired sessions from storage?

@Rich-Harris
Copy link
Member Author

maybe there should be more specialized support for authentication/authorization instead?

Authentication definitely needs to be made as simple as possible, because at present it's a nightmare (not just in Sapper apps, but generally). I don't know that it can be solved at the framework level though without introducing a lot of opinions. I think the best we can do is provide a flexible enough API that it's easy to plug in packages that deal with authentication.

What other use cases are there?

Shopping carts? I'm new to the site, browsing as a guest, I want my cart to persist even though I haven't registered/logged in yet:

export async function get(headers) {
  const cookies = cookie.parse(headers['cookie']);

  const user = await db.get_user(cookies.session_id);
  const cart = await (user ? db.get_cart_for_user(user.id) : db.get_cart_for_guest(cookies.session_id));

  return {
    user: user && {
      // only expose public info
      name: user.name,
      email: user.email,
      avatar: user.avatar
    },
    cart
  };
}

Of course you could have a /cart.json endpoint instead, but it would be slightly more complicated as you'd need some way to represent individuals who aren't logged in without exposing their cookies.

only keying it on a random session ID feels potentially dangerous

Can you elaborate? This is just how auth works, no?

would we need to worry about to how to clear old, expired sessions from storage?

I don't think it need be solved at the framework level — I'm imagining that the implementation of db.get_user above (for example) would check to see if the session was expired. Periodically you'd want to purge expired sessions from the database to save space, but this can happen whenever (e.g. svelte.dev purges expired sessions whenever the server starts, i.e. whenever we deploy a new version)


This is a bit of a tangent, but: speaking of shopping carts, one of my bugbears with a lot of ecommerce sites is that I often want to look at products in multiple tabs, which means my cart usually gets out of sync between them. Is there a case where you wouldn't want sessions to be synchronised across tabs with localStorage events? (or, going further, using similar logic to SWR?)


This is also tangential to session.persist, but a couple of things occur to me:

  • the example code thus far takes a headers argument. Should it just be a pre-parsed cookies object? You might want to use Authorization headers with endpoints, for example, but they're not much help when browsing to a page. Feels like just passing cookies would simplify things somewhat
  • there's currently no place to set headers['cookie'], which seems like a bit of an oversight

@ehrencrona
Copy link
Contributor

ehrencrona commented Oct 21, 2020

only keying it on a random session ID feels potentially dangerous

Can you elaborate? This is just how auth works, no?

Yes, but authentication always times out. If we time out the session ID within a short period then, yes, I think we'd be as safe.

We would also need to make sure the session ID has very high entropy and so cannot be guessed. We might also want to block mass requests for different IDs fishing for one that is valid.

But I'm no security expert; someone who is should look at it, if we build something someone might store personal data with.

shopping carts

I still think the problem is that if the user is logged in, the data should be associated with the user rather than the session. So you'd want some way to move the data from a non-logged in session to a logged in user. Maybe there could be a getSessionId method that you can overwrite to return a user ID if logged in or something (or a changeSessionId or what have you). But that goes in the direction of more directly supporting authentication. (Oh, and then you definitely have the synchronized session scenario, because you'd like to synchronize the client-side session on all devices the user logs in on)

Is there a case where you wouldn't want sessions to be synchronised across tabs with localStorage events?

Maybe; imagine a notes application where you store new drafts of notes in the session. Then you start two new, different drafts in different tabs. Maybe far-fetched, and of course you could work around it even with a synchronized session.

@Conduitry
Copy link
Member

This feature wouldn't make the framework any more responsible for the actual session handling than Sapper already is - which is pretty much none. Sapper already assumes you have some mechanism for handling that, and you just give Sapper a function for deriving what it should consider the session data from the req object. That part, I'm assuming, is going to be essentially unchanged in sveltekit.

What this proposed feature is suggesting is just a slightly automated way to have an endpoint that's responsible for updates to this session data. What you write in the server-side persist function is completely your responsibility, and would be similar to what you would write for the endpoint if you were implementing it manually.

Sapper isn't and sveltekit wouldn't be responsible for anything about session storage or the unique keys or cleaning up or anything.

@arxpoetica
Copy link
Member

Should it just be a pre-parsed cookies object?

@Rich-Harris I assume the people who use JWT (and not cookies) could just rig it up a different way? (I use both.)

@Rich-Harris
Copy link
Member Author

That part, I'm assuming, is going to be essentially unchanged in sveltekit.

Yep — at present session is derived from req, but since req won't be available (because it doesn't exist in e.g. AWS Lambda), it would be derived from cookies instead. (Is there a situation where cookies alone would be insufficient?)

I still think the problem is that if the user is logged in, the data should be associated with the user rather than the session. So you'd want some way to move the data from a non-logged in session to a logged in user.

Your own /login (or whatever) endpoint would be responsible for associating a user with the relevant cookie (which may be an argument for not automatically parsing cookies? since the endpoint would only have access to the raw request headers) and whatever other logic is necessary (e.g. merging the user's existing cart from a previous logged-in session with their current cart from their until-now-not-logged-in session). SvelteKit doesn't need to have an opinion about that.

@arxpoetica not totally sure I follow? JWT implies manually adding an Authorization header — it applies to programmatic requests but not clicking around a website, for which cookies are the only game in town. Or are you saying that you typically derive a session from Authorization but only for programmatic requests? If so then perhaps that (along with the point above, about non-session endpoints needing to read cookies themselves) is an argument for just passing the headers object to the handler:

// src/session/index.js
import { parse } from 'cookie';
import * as db from './db.js';

export function get({ cookie }) {
  const cookies = parse(cookie);
  return db.get_user(cookies.sid);
}

@Conduitry
Copy link
Member

At work, we have a Sapper app that is single-instance multi-tenant, and it receives information about the tenant for this request via custom HTTP headers that are injected by another service that requests run through first - and I'm handling this in Sapper by exposing those headers in the $session store, so I can access them wherever they're needed. HTTP headers are also being used during E2E tests for things like simulating accessing the app at different times. So I'm definitely -1 on only exposing the cookies, because that seems to come from a fairly narrow view of what session can be and how your site is going to be accessed - and it would also break this thing that I need at work.

I feel like I'm also slightly against saying what's provided to user code for sessions/endpoints/etc needs to be a common denominator of what's possible in all conceivable deployment environments or adapters. I'm not sure what this means for the API, but if I have no intention of ever deploying to AWS Lambdas and I am inconvenienced by an API limitation that exists in all environments because of AWS Lambdas, that's going to be irritating.

Combining these two points, I don't currently have a situation in mind where having access to the HTTP headers (for session) would be insufficient, or where having access to HTTP headers and a readable stream (for endpoints) would be insufficient - but I'm concerned about a situation where someone only ever intends to build a Node server and really just wants to be able to access req and res directly, and can't.

If there's another issue where these conversations should happen, I'm happy to move over there.

@Rich-Harris
Copy link
Member Author

Since this has turned into a more general thread about session handling, I think this is a fine venue for a conversation about cookies vs all headers etc. And I'm glad you brought up the readable stream thing as I'm about to open a separate issue about body parsing.

In a more general sense, I think it's important to be future-proof, but if that means potentially having different APIs for different platforms then there's a pretty significant cost to that. So I think we should limit our design to the simplest thing that meets the use cases we can imagine, while being as imaginative as possible. (We can always break things in a future version if we really have to.)

The one thing the code examples above don't illustrate is setting a Set-Cookie header on the response, for situations where you want to immediately set a cookie for brand new users so that you can persist their session without them having to log in. I don't know what the best approach to that is.

@Rich-Harris Rich-Harris changed the title session.persist sessions Oct 22, 2020
@Rich-Harris
Copy link
Member Author

Rich-Harris commented Oct 22, 2020

Just realised there's a big gap in all the code above: endpoints are likely to need data that shouldn't be public. For example, an endpoint might need a user's access token to communicate with some third party service on their behalf, and that token shouldn't be sent to the client. (In the case of svelte.dev, we store GitHub access tokens in our session database.)

So an endpoint signature like this...

export function get({ params }, session) {
  // ...
}

...doesn't make sense, or at least session doesn't have the same meaning here as it would in a page. Instead, we need to generate a context (🐃 ) for the request from the headers — an arbitrary object that can include a user's private information — and a session from the context.

The point at which the context is generated is probably the right place to add a Set-Cookie header (if you want to set one immediately rather than waiting for login), so I'm envisaging something like this but with better names for everything:

// src/setup/index.js
import { parse } from 'cookie';
import uuid from '@lukeed/uuid';
import * as db from './db.js';

export async function prepare(request_headers) {
  const cookies = parse(request_headers['cookie']);

  const response_headers = {
    'x-foo': 'potato'
  };

  if (!cookies.sid) {
    response_headers['set-cookie'] = `sid=${uuid()}; HttpOnly`;
  }

  return {
    context: {
      user: (await db.get_user(cookies.sid)) || { guest: true }
    },
    headers: response_headers
  };
}

export function getSession(context) {
  return {
    user: {
      // strip out access tokens and what-have-you
      id: context.user.id,
      name: context.user.name,
      avatar: context.user.avatar,
      guest: context.user.guest
    }
  };
}

export async function setSession(context, session) {
  // we could get the previous session with getSession if we were so inclined
  await db.set_user(context.user.id, session.user);
}

I would love for someone to tell me a) if this is a sensible design and b) what to call everything

@Rich-Harris
Copy link
Member Author

Have gone ahead and implemented prepare and getSession, which are used if exported from src/setup/index.js (or index.ts, or whatever).

setSession involves a bit more work:

  • server and client need to agree on a magic URL to post session updates to — /__session__?
  • need to figure out the client-side details — should session.set cause setSession to be invoked? (What happens if there's an error?) Or should it only happen on an explicit session.persist(...) (and probably therefore persistSession rather than setSession)? Should there also be a session.refresh() method?

@rmunn
Copy link
Contributor

rmunn commented Sep 6, 2021

I have a design for persistSession (rather than setSession), and for $session.refresh(), sketched out in #1726 (comment). Rather than reproduce that comment here, I'd suggest making comments on #1726 so that the discussion on session persistence and refreshing can be in one place instead of spread between multiple issues.

@Rich-Harris
Copy link
Member Author

Going to close this in favour of #1726 as much of the thread is rather out of date

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

No branches or pull requests

6 participants