[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172

Simperfit · 2019-04-18T17:32:36Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#26379
License	MIT
Doc PR	todo

As far as I understand this is fixing the current wrong behaviour

~~Need to add a test.~~ added

nicolas-grekas

would deserve a test case

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

aschempp · 2019-04-24T06:29:54Z

Checking the return value of refreshUser does not necessarily mean that the user has changed. The refreshUser explicitly tracks $userDeauthenticated, but it also returns null if the user was not found by any provider. I don't think the rememberme should be invalidated in that case.

Instead of hardcoding rememberme, how about firing an event on user change? There is currently a user logout event, but there is no way to listen to an "automated" logout.

Simperfit · 2019-04-24T07:03:06Z

if the user has been deauthenticated I think we should delete the rememberme token but as speaking there is a pending PR (see #31138) maybe instead of doing it like this we could directly listen to that new event and delete the cookie. Both solution fit the issues and fix it. Le mer. 24 avr. 2019 à 08:30, Andreas Schempp <notifications@github.com> a écrit :

…

Checking the return value of refreshUser does not necessarily mean that the user has changed. The refreshUser explicitly tracks $userDeauthenticated, but it also returns null if the user was not found by any provider. I don't think the rememberme should be invalidated in that case. Instead of hardcoding rememberme, how about firing an event on user change? There is currently a user logout event, but there is no way to listen to an "automated" logout. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31172 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2KV4U57DEACFGX3MV4ZADPR747TANCNFSM4HG66URQ> .

Simperfit · 2019-05-12T06:02:59Z

@chalasr WDYT ?

Simperfit · 2019-05-18T05:55:22Z

Do I let it like this, or Do I add a subscriber to listen to the new event ?

Simperfit · 2019-06-04T10:14:23Z

@fabpot this is ready

Simperfit · 2019-07-19T07:17:20Z

cc @chalasr @linaori

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

chalasr · 2019-07-21T20:25:41Z

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

@@ -45,6 +45,7 @@
            <argument type="service" id="logger" on-invalid="null" />
            <argument type="service" id="event_dispatcher" on-invalid="null" />
            <argument type="service" id="security.authentication.trust_resolver" />
+            <argument type="service" id="security.authentication.rememberme" on-invalid="null" />


There is no such global service but one RememberMeServices per context (firewall).
This argument needs to be wired from SecurityExtension. See RememberMeFactory which is responsible for the service registration.

chalasr · 2019-07-21T20:31:56Z

src/Symfony/Component/Security/Http/Tests/Firewall/ContextListenerTest.php

+        $listener->handle(new GetResponseEvent($this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(), $request, HttpKernelInterface::MASTER_REQUEST));
+
+        $this->assertNull($tokenStorage->getToken());
+    }


I don't see anything about the REMEMBERME cookie in this test, which is what needs to be cleared.
#24805 might help writing a relevant test case for this.

@Tobion

…ion (yceruto) This PR was merged into the 4.4 branch. Discussion ---------- [TwigBundle] Update tests inline with master version | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#32714 (comment) | License | MIT | Doc PR | - Preparing to remove the `Resources/views` for TwigBundle in master branch symfony#32714 /cc @Tobion Commits ------- 28a7ab8 [TwigBundle] Update tests inline with master version

…roller and mark it as internal (yceruto) This PR was merged into the 4.4 branch. Discussion ---------- [WebProfilerBundle] Rename the new exception controller and mark it as internal | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#32695 (comment) | License | MIT | Doc PR | - I missed some important details in symfony#32695 Commits ------- ba24a51 Rename the new exception controller and mark it as internal

…ing DST (xabbuh) This PR was merged into the 4.4 branch. Discussion ---------- [Form] use a reference date to handle times during DST | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony#18366 | License | MIT | Doc PR | Commits ------- 39c98b9 use a reference date to handle times during DST

* 3.4: bump phpunit-bridge cache-id Use assertStringContainsString when needed Use assert assertContainsEquals when needed Use assertEqualsWithDelta when required

* 4.3: bump phpunit-bridge cache-id Use assertStringContainsString when needed Use assert assertContainsEquals when needed Use assertEqualsWithDelta when required

This PR was merged into the 4.4 branch. Discussion ---------- "An instance of X" phpdocs removal | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - That's symfony#32973 on 4.4 :P Commits ------- 7a44ed6 removed unneeded phpdocs

…able is not set (j92) This PR was squashed before being merged into the 4.4 branch (closes symfony#31546). Discussion ---------- [Dotenv] Use default value when referenced variable is not set | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#11956  In bash you have the option to define a default variable like this: ```bash FOO=${VARIABLE:-default} ``` When VARIABLE is not set ```bash FOO=${VARIABLE:-default} #FOO=default ``` When VARIABLE is set: ```bash VARIABLE=test FOO=${VARIABLE:-default} #FOO=test ``` If others find this also a good idea, I will write documentation and add the Doc PR. But first I would like some feedback to check if anyone agrees with this feature. Commits ------- 790dbad [Dotenv] Use default value when referenced variable is not set

* 4.3: fix merge

This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] add "max_duration" option | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#32765 | License | MIT | Doc PR | symfony/symfony-docs#12073 Commits ------- a4178f1 [HttpClient] add "max_duration" option

…mponent (yceruto) This PR was squashed before being merged into the 4.4 branch (closes symfony#32964). Discussion ---------- add yceruto as code owner of the ErrorRenderer component | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Commits ------- df6e73d add yceruto as code owner of the ErrorRenderer component

… PHP-CS-Fixer config (fancyweb) This PR was merged into the 4.4 branch. Discussion ---------- Ignore ErrorHandler DebugClassLoaderTest class in PHP-CS-Fixer config | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - It must be ignored like the `Debug` one. Commits ------- ed916a4 Ignore ErrorHandler DebugClassLoaderTest class in PHP-CS-Fixer config

…ancyweb) This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] Relax max duration test assertion | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - cf https://travis-ci.org/symfony/symfony/jobs/568304532 - It looks like with the curl client, the actual duration can be a little less than the configured max duration. Note that the same test did not fail on the PR... Commits ------- 3cbb978 [HttpClient] Relax max duration test assertion

This PR was merged into the 4.4 branch. Discussion ---------- [Console] Check for ErrorHandler classes | Q | A | ------------- | --- | Branch? | 4. | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 48b358d [Console] Check for ErrorHandler classes

This PR was merged into the 4.4 branch. Discussion ---------- [Mailer] fix getName() when transport is null | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- ee4192e fix getName() when transport is null

…n_user_change" option

chalasr · 2019-11-28T06:41:09Z

Closing in favor of #34671, thanks for the attempt.

carsonbot added Status: Needs Review Security Bug Deprecation labels Apr 18, 2019

Simperfit mentioned this pull request Apr 18, 2019

[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #26379

Closed

Simperfit marked this pull request as ready for review April 19, 2019 07:36

nicolas-grekas added this to the 3.4 milestone Apr 19, 2019

nicolas-grekas reviewed Apr 19, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/ContextListener.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Http/Firewall/ContextListener.php Outdated Show resolved Hide resolved

Simperfit force-pushed the bugfix/logout-remember-me-when-user-changed branch 2 times, most recently from 30460c0 to aefe84c Compare April 22, 2019 06:02

leofeyer mentioned this pull request May 9, 2019

Use DeauthenticatedEvent contao/contao#475

Closed

chalasr removed the Deprecation label May 18, 2019

Simperfit force-pushed the bugfix/logout-remember-me-when-user-changed branch from aefe84c to bcc51f2 Compare July 17, 2019 19:17

linaori reviewed Jul 19, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Firewall/ContextListener.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Http/Firewall/ContextListener.php Outdated Show resolved Hide resolved

chalasr requested changes Jul 21, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 21, 2019

norkunas and others added 7 commits July 25, 2019 13:54

Fix pluralizing "season"

bf608aa

[TwigBundle] Update tests inline with master version

28a7ab8

Rename the new exception controller and mark it as internal

ba24a51

[Messenger] expire delay queue and fix auto_setup logic

7aee83a

nicolas-grekas and others added 18 commits August 6, 2019 09:11

Merge branch '3.4' into 4.3

5f96c0d

* 3.4: bump phpunit-bridge cache-id Use assertStringContainsString when needed Use assert assertContainsEquals when needed Use assertEqualsWithDelta when required

Merge branch '4.3' into 4.4

34e8676

* 4.3: bump phpunit-bridge cache-id Use assertStringContainsString when needed Use assert assertContainsEquals when needed Use assertEqualsWithDelta when required

[Dotenv] Use default value when referenced variable is not set

790dbad

fix merge

36466a3

Merge branch '4.3' into 4.4

405c64c

* 4.3: fix merge

Ignore ErrorHandler DebugClassLoaderTest class in PHP-CS-Fixer config

ed916a4

[HttpClient] Relax max duration test assertion

3cbb978

add yceruto as code owner of the ErrorRenderer component

df6e73d

[Security] REMEMBERME cookie does not get deleted using the "logout_o…

21ba558

…n_user_change" option

wi

38ab8e2

Simperfit force-pushed the bugfix/logout-remember-me-when-user-changed branch from bcc51f2 to 38ab8e2 Compare August 8, 2019 12:36

Simperfit requested review from dunglas, lyrixx, sroze and xabbuh as code owners August 8, 2019 12:36

carsonbot added Status: Needs Review and removed Status: Needs Work labels Aug 8, 2019

linaori mentioned this pull request Aug 10, 2019

4.3.3 - EquatableInterface does not take effect if using the "remember_me" feature #33109

Closed

chalasr added Status: Needs Work and removed Status: Needs Review labels Aug 11, 2019

chalasr closed this Nov 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172

[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172

Simperfit commented Apr 18, 2019 •

edited

nicolas-grekas left a comment

aschempp commented Apr 24, 2019

Simperfit commented Apr 24, 2019 via email

Simperfit commented May 12, 2019

Simperfit commented May 18, 2019

Simperfit commented Jun 4, 2019

Simperfit commented Jul 19, 2019

chalasr Jul 21, 2019

chalasr Jul 21, 2019

chalasr commented Nov 28, 2019

[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172

[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172

Conversation

Simperfit commented Apr 18, 2019 • edited

nicolas-grekas left a comment

Choose a reason for hiding this comment

aschempp commented Apr 24, 2019

Simperfit commented Apr 24, 2019 via email

Simperfit commented May 12, 2019

Simperfit commented May 18, 2019

Simperfit commented Jun 4, 2019

Simperfit commented Jul 19, 2019

chalasr Jul 21, 2019

Choose a reason for hiding this comment

chalasr Jul 21, 2019

Choose a reason for hiding this comment

chalasr commented Nov 28, 2019

Simperfit commented Apr 18, 2019 •

edited