[Security] REMEMBERME cookie does not get deleted using the "logout_on_user_change" option #31172
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
30460c0 to
aefe84c
Compare
|
Checking the return value of 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. |
|
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>
.
|
|
@chalasr WDYT ? |
|
Do I let it like this, or Do I add a subscriber to listen to the new event ? |
|
@fabpot this is ready |
aefe84c to
bcc51f2
Compare
linaori
reviewed
Jul 19, 2019
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
| @@ -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 was a problem hiding this comment.
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.
| $listener->handle(new GetResponseEvent($this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(), $request, HttpKernelInterface::MASTER_REQUEST)); | ||
|
|
||
| $this->assertNull($tokenStorage->getToken()); | ||
| } |
There was a problem hiding this comment.
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.
…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 <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#11956 <!-- required for new features --> 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
bcc51f2 to
38ab8e2
Compare
|
Closing in favor of #34671, thanks for the attempt. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I understand this is fixing the current wrong behaviour
Need to add a test.added