hyper icon indicating copy to clipboard operation
hyper copied to clipboard

Retief/cookiestore

Open wzimrin opened this issue 6 years ago • 3 comments

Added a session store for storing session info in an encrypted cookie, and added tests for session stores. This does include a breaking change to the SessionStore class (put needs to return the updated id), but that seems reasonable to me -- it's a one line fix.

wzimrin avatar Jan 21 '18 00:01 wzimrin

Looks like travis might be complaining about simple.JSON? I can swap it out easily enough, but I'm not sure what I should replace it with.

wzimrin avatar Jan 21 '18 06:01 wzimrin

Hi, thanks for the PR!

Regarding the session IDs: I haven't had time to look that closely, but it seems inconsistent with the SessionStore class. If a consumer of the session store use its operation to create a new ID, that would be a useless string, as the "real ID" would be the encrypted cookie with this store. That seems like a very confusing API.

I suggest we either adapt the type class to fit a broader use, or separate it into two different responsibilities, e.g. the existing SessionStore for a key-value-like database (Redis, Memcache, in-memory) which might be used by many session backends, and some other SessionStrategy or SessionBackend that works for both the cookie and database cases. This is just my first observation, might be misunderstanding something here.

@paluh input?

Thanks!

owickstrom avatar Feb 20 '18 06:02 owickstrom

The new definition of SessionStore should still work just fine with key-value like backends. Just return the id that was passed in and you are all set (see my changes to InMemorySessionStore). SessionStore users need to make sure to use the most recent key, but that doesn't add that much complexity. For example, saveSession needed a two line change to work with the new definition.

Once I updated the SessionStore definition, CookieStore plays nicely with that definition. Yes, newSessionID doesn't do very much. However, consumers don't need to care -- as long as they use the latest key, it works just like a standard key/value store.

That said, if you want to rework this code in some manner, be my guest. This was a learning exercise for me more than anything else, and if you want to redo or skip parts of it, have fun. You probably want to keep the bugfix on line 151 of Session.purs, and I think SessionSpec.purs does a decent job of testing InMemorySessionStore/getSession/saveSession/deleteSession even if you strip out the CookieStore tests. Beyond that, do what you want with the code.

wzimrin avatar Feb 25 '18 00:02 wzimrin