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

Make it possible to change session store implementation #1180

Merged
merged 4 commits into from
Aug 29, 2017

Conversation

cbxp
Copy link
Contributor

@cbxp cbxp commented Aug 23, 2017

There are some problems with the default cookie-based sessions, including:

  • It is very easy to overwrite changed session in concurrent AJAX request with unpredictable result
  • Session replay attacks are possible by sending previous version of the session until it expires

This PR makes it possible to implement other backends for session storage, e.g. cache or database.
This is similar to CacheImpl, where new caching implementaions can be created.

The default implementation is CookieSessionStore that extracted as-is from Scope.Session class. All Scope.Session API is left 100% unchanged, so no breaking changes.

No new implementations are provided as of now, but we can create a separate PR with cache-based implementation.

…SessionStore

This will allow adding of other session stores, e.g. cache or database.
… parameter. CookieSessionStore is the default.
@asolntsev asolntsev self-assigned this Aug 23, 2017
@asolntsev asolntsev added this to the 1.5.0 milestone Aug 23, 2017
@asolntsev
Copy link
Contributor

@xael-fry I think the change is good. Recommended to merge.

public static SessionStore sessionStore = createSessionStore();

private static SessionStore createSessionStore() {
String sessionStoreParameter = Play.configuration.getProperty("application.session.store", "cookie");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better if we just have application.session.storeClass which takes in the class instead of relying on some naming convention that has a "SessionStore" suffix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the simpler parameter is more consistent with other parameters, e.g. memcached

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the simpler parameter is more consistent with other parameters, e.g. memcached

@cbxp
Copy link
Contributor Author

cbxp commented Aug 25, 2017

I have made the requested change. How about a merge?

@cbxp
Copy link
Contributor Author

cbxp commented Aug 28, 2017

Note: build actually passed. For some reason it status was not updated here

@asolntsev asolntsev merged commit 450e080 into playframework:master Aug 29, 2017
@asolntsev asolntsev deleted the session-impl branch August 29, 2017 10:09
@xael-fry
Copy link
Member

@asolntsev Maybe documentation of application.session.storeClass should have been great before merging it

@asolntsev
Copy link
Contributor

@xael-fry Good catch. I will add it to the documentation.

@xael-fry
Copy link
Member

Should the name be application.session.store.class ?

@cbxp
Copy link
Contributor Author

cbxp commented Aug 30, 2017

It seems unlikely that there will be more general store parameters.
If any additional store would have its own parameters, they better be in its own namespace.
So we suggest to leave it as application.session.storeClass

Copy link
Member

@xael-fry xael-fry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

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

Successfully merging this pull request may close these issues.

None yet

5 participants