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

Session cookies? (Till the browser is closed.) #35

Closed
michalrus opened this issue Sep 13, 2017 · 6 comments
Closed

Session cookies? (Till the browser is closed.) #35

michalrus opened this issue Sep 13, 2017 · 6 comments

Comments

@michalrus
Copy link

According to Wikipedia:

https://en.wikipedia.org/wiki/HTTP_cookie#Setting_a_cookie

… session cookies are the ones that have no MaxAge= or Expires=. They are cleared after the user closes their browser.

Could we have that? 🙏 I mean, expiry time can (should!) still be encrypted in session data, but some people might want to have the cookie cleared completely, after their browser window is closed (e.g. for the popular “[Don’t] Remember me” functionality).

So this is most probably a question of adding another field to AuthCookieSettings that would control whether this line is included or not at all:

("Max-Age", (BSC8.pack . show . n) acsMaxAge) :

@zohl
Copy link
Owner

zohl commented Sep 14, 2017

Hello,

this is definitely a good idea and should be implemented. I'm working on it, so you might expect it to be done in 1-2 days.

Unfortunately, it's not that simple as you might expect: for each call of functions that use AuthCookieSettings you would have to choose manually whether it's a session cookie or not. That's pretty fine for addSession as it is called once for every cookie and is not ok for cookied wrapper. We can employ cookie's metadata here and let the wrapper do the job :)

@michalrus
Copy link
Author

Awesome! 🎉 Thank you very much, and sorry for the naïve idea. 😅

@zohl
Copy link
Owner

zohl commented Sep 15, 2017

Things are getting complicated... It will take more time, than I expected. Hope, you will not get angry :)

To explain the situation, here is what's going on:
We have Max-Age and Expires attributes to take in account.

Expires attribute uses absolute time (in GMT format) and is deprecated.
Max-Age uses relative time (in seconds), is told to be replacement for Expires. However, older versions of IE do not support it, so Expires is still used, especially when the one needs cross-browser interoperability.
This means, we have three types of expiration -- Max-Age, Expires and session cookies.

The first attempt was to get advantage of cookies' fields that browser attaches to it's requests. It worked for Max-Age, but failed for Expires. For some (probably historical) reasons, firefox omits Expires attribute in requests.
Thus, this method is useless and we have to store a flag right into the cookie, so the core algorithm will be altered a little bit.

Cookies' internals haven't changed since the very beginning, so I think it's a good opportunity to revise it and make it easier to extend for such and similar cases.

As for API changes, there will be one additional argument to addSession family functions (of enum type).

@michalrus
Copy link
Author

michalrus commented Sep 17, 2017

Absolutely, take your time. =) For now, we can just use 30 min w/o “Remember me” and 2 weeks or so with it, but having real session cookies will be very nice.

That API change seems ideal.

Thank you for the wonderful support!

@zohl zohl mentioned this issue Sep 22, 2017
@zohl
Copy link
Owner

zohl commented Oct 23, 2017

Done,

you need to import SessionSettings(..), ExpirationType(..) and def (the last one from Data.Default).

Instead of addSession acs rs sk session you should write
addSession acs rs sk def {ssExpirationType = ...} session.
By default, there will be session cookies.
There were some internal changes and renamings, that might affect custom handlers. In most cases it's just replacing wmData to pwSession.

@michalrus
Copy link
Author

@zohl, excuse the pause in communication, but we had a somewhat hectic period.

Now I got some time to integrate this and… I just want to say that your support is amazing. ❤️

Everything works as expected! 😻

THANK YOU!!!

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

No branches or pull requests

2 participants