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

Add a cookie based session store #504

Closed
danhyun opened this issue Nov 25, 2014 · 13 comments

Comments

@danhyun
Copy link
Member

commented Nov 25, 2014

ratpack-session currently supports a memory based session store, we should provide a CookieBasedSessionStore and CookieBasedSessionStorage as an alternative to MapBasedSession implementations.

@danhyun danhyun self-assigned this Nov 25, 2014
@danhyun danhyun added the in progress label Nov 26, 2014
@danhyun danhyun added this to the release-0.9.11 milestone Nov 28, 2014
@danhyun

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2014

Should keep #447 in mind

@danhyun

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2014

First pass 9105945

Decided to use existing DefaultSessionStorage and provide a serializer/deserializer and signer.

@danhyun

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2014

Needs to support configuration for signing algorithm and secret key

danhyun added a commit that referenced this issue Nov 30, 2014
danhyun added a commit that referenced this issue Nov 30, 2014
@ldaley ldaley modified the milestones: release-0.9.11, release-0.9.12 Dec 1, 2014
@ldaley

This comment has been minimized.

Copy link
Member

commented Dec 2, 2014

A couple of todos:

  1. Tests for session keys/values with meta chars (e.g. &=:)
  2. More documentation on the module, specifically about setting the key/algorithm and concerns there
  3. Bust up the storage binder handler, it's doing too much (i.e. extract a service that turns a session map to a string value and vice versa, should be session agnostic). This should be something that can be overridden by a guice binding, that is… there should be a public interface for this as well as a default impl
  4. We need to do something about non string values (at the moment we just blindly toString() the value
ldaley added a commit that referenced this issue Dec 2, 2014
ldaley added a commit that referenced this issue Dec 2, 2014
ldaley added a commit that referenced this issue Dec 2, 2014
danhyun added a commit that referenced this issue Dec 29, 2014
hashCode impl -> key.hashCode XOR value.hashCode
if key.equals(value) then hashCode == 0
so empty map has same hashcode as map where key.equals(value)
danhyun added a commit that referenced this issue Dec 29, 2014
adding SessionService and DefaultClient impl
@danhyun

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2014

Addressed 1 and 3, I'll be doing 2 soon.

WRT 4, do you think we should serialize the object? Cookie size limit is 4k bytes. Also I'm thinking about encrypting the whole cookie value so that nobody can read the contents.

danhyun added a commit that referenced this issue Dec 31, 2014
danhyun added a commit that referenced this issue Dec 31, 2014
danhyun added a commit that referenced this issue Dec 31, 2014
added configuration info, configuration example and concerns about using the client side sessions module
@danhyun

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2015

2 has been addressed. Still need to discuss 4.

wrt4: Doesn't make sense/is not practical to serialize object graphs, is toString() too simplistic? I'm not aware of any middle ground. Does it make sense to throw IllegalArgumentException if you don't explicitly pass a string as both key and value?

@ldaley ldaley modified the milestones: release-0.9.12, release-0.9.13 Jan 1, 2015
@ldaley

This comment has been minimized.

Copy link
Member

commented Jan 5, 2015

toString() is not round-trippable. That is, you can't recreate the object from the string.

It's certainly an option to only support strings when using client side storage. We can at least start there. Have to make sure this limitation is prominently documented.

@danhyun

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2015

We could offer to serialize/deserialize to JSON, I would rather not introduce Jackson as a dependency to sessions but it would be nice to treat the client session store as a true multi level map.

@ldaley

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

We wouldn't implicitly convert to JSON. The user would either need to tell us to do this (by way of a bi directional converter) or do it before giving us the value (i.e. store the JSON representation directly)

@ldaley

This comment has been minimized.

Copy link
Member

commented Jan 16, 2015

We should have a look at the new Spring Session API for inspiration.

@ylemoigne

This comment has been minimized.

Copy link

commented Feb 14, 2015

Note : the serialization problem made ClientSideSessionsModule not compatible with pac4j at this time.
(as the store return the "toString" of registered UserProfile, it end with ClassCastException in Pac4jProfileHandler.java:50)

@danhyun

This comment has been minimized.

Copy link
Member Author

commented May 1, 2015

Thanks to @zedar for addressing a bulk of the remaining issues as well as implementing really nice features. I'm going to close this ticket to mark that we have a solid basic client side session store but I'm going to open a new ticket to address serialization and pac4j here #664.

@danhyun danhyun closed this May 1, 2015
@danhyun danhyun removed the in progress label May 1, 2015
@ldaley

This comment has been minimized.

Copy link
Member

commented May 4, 2015

Docs need review.

@ldaley ldaley reopened this May 4, 2015
@ldaley ldaley modified the milestones: release-0.9.17, release-0.9.16 Jun 2, 2015
@ldaley ldaley closed this Jun 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.