Skip to content
This repository

Make it possible to set keep alive sessions #2171

Closed
emninet opened this Issue September 14, 2011 · 24 comments

10 participants

Drak Florin Patan Rui Marinho Arjen Brouwer Victor Berchet Miha Vrhovnik Christophe Coevoet Thomas Stähle Fabien Potencier

As far as I understood it from the doc and experienced it in my S2 app, the sessions are not keep alive ones.
Even if it may be a security weakness at some points and in some business contexts, many users will ask for keep alive sessions, and do not want to be forced to reconnect after a given amount of time.

Do you think it could be possible to implement this in the next future of S2?
With a new parameter such as idle_lifetime?

Thanks

Florin Patan

@manuzorch I don't get it. There's a lifetime option for the session. Isn't that enough?

@dlsniper: no that is not always enough.
Some use cases require keep alive sessions, meaning the session is never killed as long as the user is active.
The standard session lifetime should be considered as an "idle session timeout": the session lifetime before the user is disconnected if there is no activity of the user.
Otherwise the session is kept alive.

Rui Marinho

I think what @manuzorch means is that cookie_lifetime sets a definitive value and not a delta value updated on every request. Sometimes it's useful for that value to be updated on the last request of the user in order to keep the session alive.

@ruimarinho: yes that is exactly what I was meaning but better said.

Arjen Brouwer

Someone implemented a Listener that migrates the Session each kernel request.

See: http://forum.symfony-project.org/viewtopic.php?t=36827&p=125930

Rui Marinho

@arjenjb, the author updated the post with this "EDIT: Yea, don't do this... it seems to break the CSFR tokens and all forms become invalid", so it doesn't look like a viable alternative.

I have developed my own Firewall listener. I tried to implement this notion of session migrate in it but noticed too many problems most notably this problem of CRSF tokens becoming invalid. So I decided not to "play" anymore with sessions and proposed this feature of "keep alive sessions" to be added to the core of Symfony.

Drak
Collaborator
drak commented March 20, 2012

This should not be necessary but I realise why this PR has come about. It's because from Symfony2 FrameworkBundle, there is no way to configure the garbage collection. Let me explain the logic.

The cookie lifetime simply defines how long the cookie will live on the remote client browser. However, that does not determine how long the cookie will remain in the webserver' session storage. The fact these two are different allows for very nice security settings to be devised.

So there are two variables involved. The session workflow is like this - any time a session is started, that session will ultimately be saved to storage. So regardless of the lifetime of the cookie, we can count how 'fresh' the session is, i.e. when it was last accessed. This means we can always calculate 'session idle time'.

For example, if you have a sensitive application (banking for example), then you might want idle sessions to expire after 5 or ten minutes.

This is where the garbage collection comes in. There are three values set by PHP,

  • gc_maxlifetime - refers to how long a session may remain idle for.
  • gc_probability and gc_divisor - these create a percentage chance of PHP calling the garbage collection routines. For example, a probability of 1 and divisor of 100 means 1 in 100 chance of calling GC when a session is started.

So in order to control "keep-alive", you simply need to extend the period of the gc_maxlifetime because GC will not expire session that were saved less than the gc_maxlifetime setting. I will submit a PR which allows these values to be configured.

Victor Berchet
vicb commented March 20, 2012

@drak online banking is a great example here: my online bank used to logged me out automatically after 5min or so which was very annoying some times.

What this issue propose is to change this to be automatically logged out after 5min of inactivity (you can check your account for 1h provided your are active at least once every 5mn).

I don't think #3659 solves this ?

Drak
Collaborator
drak commented March 20, 2012

@vicd - as annoying as it may be, your bank has designed it this way exactly because they dont want people leaving unattended terminals. So they would set the GC maxlifetime to 300 seconds.

What this issue propose is to change this to be automatically logged out after 5min of inactivity (you can check your account for 1h provided your are active at least once every 5mn).

That is exactly what #3659 does. You see, each time a request is made (and a session starts), that session is saved - that means each request automatically 'resets' the idle timer and therefor, you can check your account for 1h providing you use the app once every 5 mins which is the required behaviour (assuming the idle time is set to 300 seconds).

Miha Vrhovnik

@drak the problem is not the gc, but that session cookie lifetime is set to now + cookie_lifeteime at first request and then it stays the same. What @manuzorch is suggesting is that session expiration date should be moved for cookie_lifetime each time the request is made.
AFAIR this was suggested more than once and it was always shoot down, the reason being it's to expensive to do that on each request.

edit: After thinking about this a bit more. Are you suggesting that cookie_lifetime should be set to arbitrary high number and then control the expiration with session garbage collection?

Drak
Collaborator
drak commented March 21, 2012

After thinking about this a bit more. Are you suggesting that cookie_lifetime should be set to arbitrary high number and then control the expiration with session garbage collection?

Yes, that's the idea. Basically, so long as a cookie_lifetime is specified the cookie will always be extended for that lifetime on each page request. By making the cookie_lifetime longer than the gc_maxlifetime (i.e. idle time) you get fine control over session workflow.

If you want users to remain logged in forever, simply set both GC and cookie lifetime to something very high, like a year or whatever. If you want users to remain logged in only for max idle time, cookie to say a day, and idle time to 20 mins or whatever.

There are more possibilities once you get into more application level stuff, but this is the gist of it.

Rui Marinho

@drak, is it possible that relying on this strategy might mean sessions won't expire exactly at the idle time we want to? My concern, which I'm not sure it's based on precise ideas, is that setting a gc_maxlifetime to say 5 minutes might actually mean a higher idle time count, since the GC only has a certain probability of running.

Also, you mentioned that the GC might run when a new session is started. What happens when there are no new session for a long period?

Lastly, regarding:

so long as a cookie_lifetime is specified the cookie will always be extended for that lifetime on each page request

Is this what's expected with the current code or after your PR?

Drak
Collaborator
drak commented March 26, 2012

@ruimarinho - you are right, actually, this method of idling is potentially not precise, but based on the fact that servers are hit all the time with requests, so the GC is very very likely to run, especially if the settings are tweaked for your traffic settings. However, yes, explicit expiry of idle session on login is also an option, something I will address in separate PR.

Fabien Potencier fabpot referenced this issue from a commit March 26, 2012
Fabien Potencier merged branch drak/session_gc (PR #3659)
Commits
-------

cdba4cf [FrameworkBundle] Change XSD to allow string replacements on session args.
52f7955 [FrameworkBundle] Remove default from gc_* session configuration keys.
749593d [FrameworkBundle] Allow configuration of session garbage collection for session 'keep-alive'.

Discussion
----------

[2.1][FrameworkBundle] Allow configuration of session garbage collection

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2171
Todo: -

---------------------------------------------------------------------------

by drak at 2012-03-21T21:56:20Z

@fabpot - this PR is ready for merge.  It basically allows configuration of some session ini values that are necessary in controlling the session behaviour.

---------------------------------------------------------------------------

by dlsniper at 2012-03-21T22:57:18Z

@drak shouldn't all the options here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L266 be available for configuration, or am I just reading the source wrong and they already are?

In this case should I make a separate PR to cover the rest or could you do it in this one?

---------------------------------------------------------------------------

by fabpot at 2012-03-23T14:56:22Z

@drak: the discussion is the ticket is very interesting and I think it should be part of a cookbook in the documentation. Can you take care of that before I merge this PR? Thanks.

---------------------------------------------------------------------------

by drak at 2012-03-25T15:32:59Z

@fabpot - yes - it's on the todo list.  Will update this PR when done.

---------------------------------------------------------------------------

by drak at 2012-03-26T19:45:13Z

@fabpot - this is ready for merging, the documentation is done (the PR is in but I'll tweak it, but no need to wait to merge this PR).  I will also add something extra to cookbook (I wrote docs for the component).
1bb6e0d
Rui Marinho

@drak, are you planning on dispatching any kind of event when the lifetime expires? The UI question you rised regarding specifically tailored messages for the user when the session expires is very common and they would surely help there.

Drak
Collaborator
drak commented March 29, 2012

@ruimarinho - For the component, there is currently no plan to make a coupling to the dispatcher although I am toying with introducing a couple of EventDipatcherAware things. If you look at PR #3718 you will see I have added meta-data to the session so it is possible to make calculations and then do as you wish. I think this is very much in line with how a component should behave (giving lots of flexibility without sticking to a single implementation).

The only thing remaining to decide is if one should be able to change the lifetime of an existing session cookie. Overall, since this PR it's clear that we can do everything we need with either a lifetime of zero (which sets the session cookie to die after the browser is closed) or a large lifetime, of a year+ and let everything else happen server side. Ultimately all processing needs to happen server side as client side is not reliable or secure.

Drak
Collaborator
drak commented March 30, 2012

I've made significant progress on #3718 but I do want to show you something I have so far decided to leave out of the commit which allows you to extend the life of the current session cookie.

    /**
     * Changes the current session to expire at given time.
     *
     * To convert the existing session to expire when the browser closes,
     * specify a time of 0.
     *
     * This method emits a new session cookie.
     *
     * @param integer Expiry time from now in seconds (as a Unix timestamp).
     */
    public function changeSessionExpiry($lifetime)
    {
        $cookie = session_get_cookie_params();
        setcookie(session_name(), session_id(), time() + $lifetime, $cookie['path'], $cookie['domain'], $cookie['secure'], $cookie['httponly']);
    }

After a lot of thought so far, I don't see this as necessary but it might make a good cook-book entry.

Miha Vrhovnik

I believe that #3178 is supposed to be #3718

Drak
Collaborator
drak commented March 31, 2012

@mvrhov Correct, I have updated my comment.

Fabien Potencier fabpot referenced this issue from a commit April 03, 2012
Fabien Potencier merged branch drak/sessionmeta (PR #3718)
Commits
-------

8a0e6d2 [HttpFoundation] Update changelog.
4fc04fa [HttpFoundation] Renamed MetaBag to MetadataBag
2f03b31 [HttpFoundation] Added the ability to change the session cookie lifetime on migrate().
39141e8 [HttpFoundation] Add ability to force the lifetime (allows update of session cookie expiry-time)
ec3f88f [HttpFoundation] Add methods to interface
402254c [HttpFoundation] Changed meta-data responsibility to SessionStorageInterface
d9fd14f [HttpFoundation] Refactored for moved tests location.
29bd787 [HttpFoundation] Added some basic meta-data to Session

Discussion
----------

[2.1][HttpFoundation] Added some basic meta-data to Session

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2171
Todo: -

Session data is stored as an encoded string against a single id.  If we want to store meta-data about the session, that data has to be stored as part of the session data to ensure the meta-data can persist using any session save handler.

This patch makes it much easier to determine the logic of session expiration.  In general a session expiry can be dealt with by the gc handlers, however, in some applications more specific expiry rules might be required.

Session expiry may also be more complex than a simple, session was idle for x seconds.  For example, in Zikula there are three security settings, Low, Medium and High.  The rules for session expiry are more complex as under the Medium setting, a session will expire after x minutes idle time, unless the rememberme option was ticked on login.  If so, the session will not idle.  This gives the user some control over their experience.  Under the high security setting, then there is no option, sessions will expire after the idle time is reached and login the UI has the rememberme checkbox removed.

The other advantage is that under this methodology, there can be a UI experience on expiry, like "Sorry, your session expired due to being idle for 10 minutes".

Keeping in the spirit of Symfony2 Components, I am seeking to make session handling flexible enough to accommodate these general requirements without specifically covering expiration rules. It would mean that it would be up to the implementing application to specifcally check and expire session after starting it.

Expiration might look something like this:

    $session->start();
    if (time() - $session->getMetadataBag()->getLastUpdate() > $maxIdleTime) {
        $session->invalidate();
        throw new SessionExpired();
    }

This commit also brings the ability to change the `cookie_lifetime` when migrating a session. This means one could move from a default of browser only session cookie to long-lived cookie when changing from a anonymous to a logged in user for example.

    $session->migrate($destroy, $lifetime);

---------------------------------------------------------------------------

by drak at 2012-03-30T18:18:43Z

@fabpot I have removed [WIP] status.

---------------------------------------------------------------------------

by drak at 2012-03-31T13:34:57Z

NB: This PR has been rebased and the tests relocated as per recent master changes.

---------------------------------------------------------------------------

by drak at 2012-04-03T02:16:43Z

@fabpot - ping
b9de0be
Christophe Coevoet
Collaborator
stof commented April 03, 2012

@drak can this issue be closed ?

Drak
Collaborator
drak commented April 05, 2012
Thomas Stähle

I am wondering about the status of this discussion/request. I am facing the same problem, that i need a idle timeout not a fixed timeout, as it is the most common behaviour everywhere on non critical sites. I know that sf 2.1 will be released in near future which maybe has everything implemented already (with all refactoring) but i would be happy if these settings would be introduced to 2.0, as well.

Fabien Potencier
Owner
fabpot commented July 01, 2012

@drak: Am I correct if I say that this use case should be documented properly in a cookbook entry? If so, can you create a ticket on the symfony doc repository so that we can close this issue?

Drak drak referenced this issue in symfony/symfony-docs July 01, 2012
Closed

Ensure Session idle timeouts are documented #1512

Drak drak closed this July 01, 2012
Drak
Collaborator
drak commented July 01, 2012

Closing, ticket opened in docs repo.

mmucklo mmucklo referenced this issue from a commit March 26, 2012
Fabien Potencier merged branch drak/session_gc (PR #3659)
Commits
-------

cdba4cf [FrameworkBundle] Change XSD to allow string replacements on session args.
52f7955 [FrameworkBundle] Remove default from gc_* session configuration keys.
749593d [FrameworkBundle] Allow configuration of session garbage collection for session 'keep-alive'.

Discussion
----------

[2.1][FrameworkBundle] Allow configuration of session garbage collection

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2171
Todo: -

---------------------------------------------------------------------------

by drak at 2012-03-21T21:56:20Z

@fabpot - this PR is ready for merge.  It basically allows configuration of some session ini values that are necessary in controlling the session behaviour.

---------------------------------------------------------------------------

by dlsniper at 2012-03-21T22:57:18Z

@drak shouldn't all the options here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L266 be available for configuration, or am I just reading the source wrong and they already are?

In this case should I make a separate PR to cover the rest or could you do it in this one?

---------------------------------------------------------------------------

by fabpot at 2012-03-23T14:56:22Z

@drak: the discussion is the ticket is very interesting and I think it should be part of a cookbook in the documentation. Can you take care of that before I merge this PR? Thanks.

---------------------------------------------------------------------------

by drak at 2012-03-25T15:32:59Z

@fabpot - yes - it's on the todo list.  Will update this PR when done.

---------------------------------------------------------------------------

by drak at 2012-03-26T19:45:13Z

@fabpot - this is ready for merging, the documentation is done (the PR is in but I'll tweak it, but no need to wait to merge this PR).  I will also add something extra to cookbook (I wrote docs for the component).
215ca17
mmucklo mmucklo referenced this issue from a commit April 03, 2012
Fabien Potencier merged branch drak/sessionmeta (PR #3718)
Commits
-------

8a0e6d2 [HttpFoundation] Update changelog.
4fc04fa [HttpFoundation] Renamed MetaBag to MetadataBag
2f03b31 [HttpFoundation] Added the ability to change the session cookie lifetime on migrate().
39141e8 [HttpFoundation] Add ability to force the lifetime (allows update of session cookie expiry-time)
ec3f88f [HttpFoundation] Add methods to interface
402254c [HttpFoundation] Changed meta-data responsibility to SessionStorageInterface
d9fd14f [HttpFoundation] Refactored for moved tests location.
29bd787 [HttpFoundation] Added some basic meta-data to Session

Discussion
----------

[2.1][HttpFoundation] Added some basic meta-data to Session

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2171
Todo: -

Session data is stored as an encoded string against a single id.  If we want to store meta-data about the session, that data has to be stored as part of the session data to ensure the meta-data can persist using any session save handler.

This patch makes it much easier to determine the logic of session expiration.  In general a session expiry can be dealt with by the gc handlers, however, in some applications more specific expiry rules might be required.

Session expiry may also be more complex than a simple, session was idle for x seconds.  For example, in Zikula there are three security settings, Low, Medium and High.  The rules for session expiry are more complex as under the Medium setting, a session will expire after x minutes idle time, unless the rememberme option was ticked on login.  If so, the session will not idle.  This gives the user some control over their experience.  Under the high security setting, then there is no option, sessions will expire after the idle time is reached and login the UI has the rememberme checkbox removed.

The other advantage is that under this methodology, there can be a UI experience on expiry, like "Sorry, your session expired due to being idle for 10 minutes".

Keeping in the spirit of Symfony2 Components, I am seeking to make session handling flexible enough to accommodate these general requirements without specifically covering expiration rules. It would mean that it would be up to the implementing application to specifcally check and expire session after starting it.

Expiration might look something like this:

    $session->start();
    if (time() - $session->getMetadataBag()->getLastUpdate() > $maxIdleTime) {
        $session->invalidate();
        throw new SessionExpired();
    }

This commit also brings the ability to change the `cookie_lifetime` when migrating a session. This means one could move from a default of browser only session cookie to long-lived cookie when changing from a anonymous to a logged in user for example.

    $session->migrate($destroy, $lifetime);

---------------------------------------------------------------------------

by drak at 2012-03-30T18:18:43Z

@fabpot I have removed [WIP] status.

---------------------------------------------------------------------------

by drak at 2012-03-31T13:34:57Z

NB: This PR has been rebased and the tests relocated as per recent master changes.

---------------------------------------------------------------------------

by drak at 2012-04-03T02:16:43Z

@fabpot - ping
5639824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.