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

Retief/cookiestore #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wzimrin
Copy link

@wzimrin wzimrin commented Jan 21, 2018

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.

CookieStore stores session info in encrypted/hmac'ed cookies.  To do
this, I need to be able to update the id when I save a session, so I
updated SessionStore class.  I also added some tests for session stores.
 These found a bug in Hyper.Session.deleteSession (the delete call
wasn't actually being run), and I fixed it.
@wzimrin
Copy link
Author

wzimrin commented Jan 21, 2018

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.

@owickstrom
Copy link
Collaborator

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!

@wzimrin
Copy link
Author

wzimrin commented Feb 25, 2018

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.

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

Successfully merging this pull request may close these issues.

2 participants