[Security] Idle sessions expiration. #12807

ajgarlag · 2014-12-02T15:53:39Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	symfony/symfony-docs#4576

This new firewall allows you to block long running sessions.

When the session cookie is valid until the browser is closed (cookie_lifetime = 0), but the user left the browser open for a long time, the session will be invalidated if the max_idle_time is exceeded. The max_idle_time value should be lesser or equal than the one from session.gc_maxlifetime.

If an expiration_url is set, the user agent will be redirected to this URL after the session invalidation. If this parameter is not set, a SessionExpiredException will be thrown.

xabbuh · 2014-12-02T18:52:09Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml

@@ -28,6 +28,8 @@

        <parameter key="security.authentication.switchuser_listener.class">Symfony\Component\Security\Http\Firewall\SwitchUserListener</parameter>

+        <parameter key="security.authentication.sessionexpiration_listener.class">Symfony\Component\Security\Http\Firewall\SessionExpirationListener</parameter>


Better don't use a parameter for the class name.

Why not? All other firewall listeners clases are defined using a parameter.

Because such class names are deprecated and will be removed in 3.0, so we don't introduce new ones.

Ok, didn't know!

ajgarlag · 2014-12-29T07:49:42Z

I've just rebased 2.7 branch to get green tests again.

With components enviroment option the tests fail trying to autoload new classes, not included with the component installation. Is there anything I can do here?

stof · 2015-01-04T08:31:24Z

src/Symfony/Bundle/SecurityBundle/Tests/Functional/SessionExpirationTest.php

+namespace Symfony\Bundle\SecurityBundle\Tests\Functional;
+
+/**
+ * @author Antonio J. García Lagar <aj@garcialagar.es>


we generally don't put @author tags on test files

ajgarlag · 2015-01-04T17:16:39Z

@stof Thanks for your review. I think all errors you found were fixed.

Insert a reference to PR symfony/symfony#12807

stof · 2015-01-18T16:04:14Z

src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml

@@ -258,6 +258,15 @@
            <argument type="service" id="event_dispatcher" on-invalid="null"/>
        </service>

+        <service id="security.authentication.sessionexpiration_listener" class="Symfony\Component\Security\Http\Firewall\SessionExpirationListener" public="false" abstract="true">


I would call it session_expiration_listener (separating all words with underscores, not only half of them)

fabpot · 2016-06-16T16:03:21Z

ping @symfony/deciders What about this feature? Is it something we want in core?

xabbuh · 2016-06-18T06:09:34Z

src/Symfony/Bundle/SecurityBundle/Tests/Functional/app/SessionExpiration/config.yml

+            anonymous: ~
+            logout:
+                target: /second/target
+                path: /second/logout


What do we need this firewall for?

…alue

Use `Psr\Log\LoggerInterface` instead of the deprecated `Symfony\Component\HttpKernel\Log\LoggerInterface`

weaverryan · 2017-09-26T18:52:55Z

What's the advantage of this over using session.gc_maxlifetime and allowing the session to expire naturally? Is it that you can expire your login, but before expiring your session? Or is it because you want to be able to control this on a firewall-by-firewall basis?

I just want to make sure I understand the need for this. :)

ajgarlag · 2017-09-27T06:45:36Z

The session.gc_maxlifetime param only indicates to the garbage collector that sessions older than that can be safely removed. But the garbage collector is started with a probability of session.gc_probability divided by session.gc_divisor, so you couldn't really control it until PHP 7.1 (this PR is 3 years old) where there is a new session_gc function that you can call to launch the garbage collection of session data.

Another problem with session.gc_maxlifetime is that if you use the native session save handler, you must use a filesystem that modifies the access time of the session file. Otherwise, the max lifetime will be computed from the session start, and not from last access.

Another advantage of this PR is that when an expired session is detected, you can control how your app should react, catching the SessionExpiredException. With the garbage collection mechanism, when it is launched, it removes data from all sessions, not only for the one active in that request, so if an user which session data have been removed returns to your app his session is gone, you cannot identify him an provide an user friendly message like Your session have expired due inactivity.

You can read a more detailed explanation in https://stackoverflow.com/questions/520237/how-do-i-expire-a-php-session-after-30-minutes

weaverryan

This is a sound PR imo. And so, I'd like to see it merged :). I just sent you some feedback @ajgarlag. Sorry for the long delays - sometimes security stuff (due to complexity, sensitivity) is one that is the most difficult to review. So, thanks for your patience. I only see one potential blocker, and it's about the logout handlers inside the listener.

weaverryan · 2017-09-27T14:31:43Z

...ymfony/Bundle/SecurityBundle/Tests/Functional/Bundle/SimpleLoginBundle/SimpleLoginBundle.php

+    {
+        return 'FormLoginBundle';
+    }
+}


Could we just use the FormLoginBundle directly in the tests? Then we could remove this file and the new after_login.html.twig. That bundle already gives us a functional /profile URL, which is all we really need I think.

weaverryan · 2017-09-27T14:32:41Z

src/Symfony/Bundle/SecurityBundle/Tests/Functional/SessionExpirationTest.php

+        $client->request('GET', '/protected_resource');
+        $this->assertEquals(200, $client->getResponse()->getStatusCode());
+
+        sleep(5); //Wait for session to expire


// Wait (add a space)

weaverryan · 2017-09-27T14:33:46Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

+                ->canBeUnset()
+                ->children()
+                    ->integerNode('max_idle_time')->defaultValue(ini_get('session.gc_maxlifetime'))->min(1)->end()
+                    ->scalarNode('expiration_url')->defaultNull()->end()


What about expiration_path? This seems more consistent with the keys under form_login, for example. And this could also be a URL, correct? It looks like you set it up that way :)

weaverryan · 2017-09-27T14:35:47Z

src/Symfony/Bundle/SecurityBundle/composer.json

@@ -17,7 +17,7 @@
    ],
    "require": {
        "php": ">=5.5.9",
-        "symfony/security": "~3.1,>=3.1.2",
+        "symfony/security": "~3.2",


Can be removed - on the 3.4 branch, this is already updated to "symfony/security": "~3.4|~4.0",

weaverryan · 2017-09-27T14:37:34Z

src/Symfony/Component/Security/Http/Firewall/SessionExpirationListener.php

+        }
+
+        if (null !== $this->logger) {
+            $this->logger->info(sprintf("Expired session detected for user named '%s'", $token->getUsername()));


nit pick, but use ' around the string and then "%s" inside. We almost always wrap strings in single quotes (sometimes this isn't followed, but usually when we need "\n")

weaverryan · 2017-09-27T14:43:10Z

src/Symfony/Component/Security/Http/Firewall/SessionExpirationListener.php

+        }
+
+        $this->tokenStorage->setToken(null);
+        $session->invalidate();


Ok, so this is the trickiest part of this PR. You're effectively imitating what LogoutListener does + some of its logout handlers (e.g. SessionLogoutHandler). So then (to play devil's advocate), why not actually inject the logout handlers and use them here? We are logging the user out... so shouldn't those handlers be called? And to go further, couldn't we use the normal DefaultLogoutSuccessHandler to figure out where to take the user next?

The disadvantage to this is that you couldn't do something different based on a normal user logout versus a session expiration logout. But, is that a problem? And if it is, could we dispatch a special event (e.g. security.session_expired) in this class, before calling the logout handlers?

After 3 years, my vision is slightly different. I'll remove the $tokenStorage->setToken(null) so this listener will only detect the session expiration and optionally (enabled by default) will invalidate the session.

For me, the logout is an interactive user action and different from the session expiration case.
If you need to force the logout you could disable the session invalidation and redirect the user to the logout path. What do you think?

Hmm... so then the purpose of this listener would be to only invalidate the session after a certain amount of idle time. If that's the case... then this is kind of unrelated to security, right? This could just be a general framework feature of the session: a framework-level way to invalidate sessions that are idle. Wdyt?

@weaverryan It took me some time to review this, as the history behind this PR is too old. It is the result of an split of #12009, which was a reimplementation of #786, into #12807(this PR) and #12810, so I decided to write a recap

We have to implement 2 different things here: first, a feature to detect idle sessions, and last, to provide a default behavior once the expired session is detected.

Detecting idle sessions

It is now implemented as a firewall listener, so it can be enabled or disabled for different firewalls in the same app, but if the session is invalidated inside one firewall, it will logout from any firewall that uses the ContextListener to restore the token.

If we decide that this feature is not related to security, but to the framework, we can implement it as a kernel.request event listener that, IMO, should be executed with higher priority than the firewall one, so the user is not authenticated from an expired session. I'm not sure which component (HttpKernel?) should provide this listener.

Anyway, once the expired session is detected, I think that a new event should be dispatched (currently not implemented). Your proposed name security.session_expired it's only an option if we decide that this feature is a security concern.

Default behavior on expired sessions

For me, the best option here is to invalidate the session (should we allow not to override this?), and redirect the user to a given path that will return an accurate response depending on the given request (html, json, etc...) or to throw a SessionExpiredException in no path is defined. If we implement this feature as a kernel event listener, this behavior should be hooked to the new session_expired event.

In a previous comment you proposed to force an user logout calling the LogoutSuccessHandler and all defined LogoutHandlers, but I see two problems here:

In the app there could be more than one firewall, so; which logout should be executed when an expired session is detected if we decide that this option should not be implemented as a firewall listener, but as a kernel listener?

If we decide to execute the logout from the current firewall, (in this case the expired session listener should be a firewall listener, or has to be executed after the firewall listener), what should be done if there is no logout listener defined for the current path?

Hey @ajgarlag!

Sorry for the slow reply again :p.

After reading your reply, it seems to me that this PR indeed has nothing to do with security. We're simply implementing a framework-level "session timeout" feature. This is necessary because there's no reliable way to do this with php.ini configuration. If there were, then we would just have the user do that.

With that in mind, this 100% seems like "session" configuration. And so, it would be configured under the framework.session configuration. And I don't think ANY other configuration is necessary - no need to have expiration_url. Nope, we would simply invalidate the user's session and that's it (e.g. allow the request to continue).

IF we want to allow the user to hook into this further, then dispatching the event we discussed may make sense. I WOULD do that, then we can see what others think.

To summarize (and tell me if you disagree):

This is not related to security at all - implement it under the framework.session configuration. The listener would be configured in FrameworkBundle, but the listener/subscriber class itself would live in the HttpKernel component.

When a session "expires", simply invalidate the session and do nothing else. Well, let's also dispatch a new event, and we can see if people think it's a good idea.

nicolas-grekas · 2019-09-29T10:56:06Z

I'm chiming in a bit randomly so my comment might be totally wrong.
What's the difference with using a remember me cookie here?
Right now, all session storages update the last-access timestamp and enforce gc_maxlifetime.
In order to detect idle sessions, one can set gc_maxlifetime to the idle time they want + rely on a remember-me cookie to reboot a fresh session after the idle timeout expired, providing adjusted role management + privilege upgrade path, using only existing mechanisms from core, isn't it?

fabpot · 2020-06-25T09:08:06Z

Looks like @nicolas-grekas is right. Closing.

This was referenced Dec 2, 2014

[Security] Session concurrency control #12810

Closed

[WIP] [Security] Session concurrency control #12009

Closed

[WCM] Idle sessions expiration cookbook recipe. symfony/symfony-docs#4576

Closed

xabbuh reviewed Dec 2, 2014
View reviewed changes

fabpot added the Security label Dec 7, 2014

ajgarlag force-pushed the feature/session-expiration branch from 0c81e2d to b69255a Compare December 29, 2014 07:38

stof reviewed Jan 4, 2015
View reviewed changes

fabpot force-pushed the 2.7 branch from 77b6cca to dcfc338 Compare January 8, 2015 13:06

ajgarlag added a commit to ajgarlag/AjglSessionExpiration that referenced this pull request Jan 9, 2015

Reference to Symfony PR

c88507a

Insert a reference to PR symfony/symfony#12807

ajgarlag added a commit to ajgarlag/AjglSessionExpirationBundle that referenced this pull request Jan 9, 2015

Reference to Symfony PR

dc0065c

Insert a reference to PR symfony/symfony#12807

stof reviewed Jan 18, 2015
View reviewed changes

ajgarlag force-pushed the feature/session-expiration branch from e596c70 to ab2b434 Compare March 24, 2015 20:16

javiereguiluz added the Feature label Mar 3, 2016

xabbuh reviewed Jun 18, 2016
View reviewed changes

ajgarlag added 6 commits September 9, 2016 17:35

Removes warning when max idle time exceeds session.gc_maxlifetime v…

f62e061

…alue

Removes author tag from test docblock

2f32566

Checks that logger exists before logging a message

98e1c09

Rename session expiration listener identifier

e70e491

Remove deprecated Symfony\Component\HttpKernel\Log\LoggerInterface

ddfa80c

Use `Psr\Log\LoggerInterface` instead of the deprecated `Symfony\Component\HttpKernel\Log\LoggerInterface`

Fix failing test

d573605

ajgarlag force-pushed the feature/session-expiration branch from 036ed33 to 1b18bdd Compare September 9, 2016 16:13

ajgarlag changed the base branch from 2.7 to master September 9, 2016 16:14

ajgarlag force-pushed the feature/session-expiration branch from 1b18bdd to c877aad Compare September 12, 2016 07:07

ajgarlag added 5 commits September 12, 2016 09:24

Fix YAML syntax

ffd3415

Use ClockMock for time sensitive tests.

2cfe9b1

Fix the functional test config to make it more simple

598d0e8

CS fixes suggested by fabbot

d5dc414

Increase symfony/security minimun required version

eae647e

ajgarlag force-pushed the feature/session-expiration branch from 53eb071 to eae647e Compare September 12, 2016 07:30

xabbuh requested changes Sep 19, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

weaverryan requested changes Sep 27, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.4, 4.1 Sep 28, 2017

aschempp mentioned this pull request Dec 19, 2017

[RFC] Security fixes (step 2) contao/core-bundle#1238

Merged

28 tasks

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

fabpot closed this Jun 25, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

		@@ -28,6 +28,8 @@

		<parameter key="security.authentication.switchuser_listener.class">Symfony\Component\Security\Http\Firewall\SwitchUserListener</parameter>

		<parameter key="security.authentication.sessionexpiration_listener.class">Symfony\Component\Security\Http\Firewall\SessionExpirationListener</parameter>

Uh oh!

[Security] Idle sessions expiration. #12807

[Security] Idle sessions expiration. #12807

Uh oh!

Conversation

ajgarlag commented Dec 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgarlag commented Dec 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgarlag commented Jan 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Sep 26, 2017

Uh oh!

ajgarlag commented Sep 27, 2017

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Sep 29, 2019

Uh oh!

fabpot commented Jun 25, 2020

Uh oh!

Uh oh!