Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedApr 18, 2019•
edited
Loading

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#26379
LicenseMIT
Doc PRtodo

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

Need to add a test. added

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

would deserve a test case

@SimperfitSimperfitforce-pushed thebugfix/logout-remember-me-when-user-changed branch 2 times, most recently from30460c0 toaefe84cCompareApril 22, 2019 06:02
@aschempp
Copy link
Contributor

Checking the return value ofrefreshUser does not necessarily mean that the user has changed. TherefreshUser explicitly tracks$userDeauthenticated, but it also returnsnull 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
Copy link
ContributorAuthor

Simperfit commentedApr 24, 2019 via email

if the user has been deauthenticated I think we should delete therememberme token but as speaking there is a pending PR (see#31138) maybe instead of doing itlike 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> .
aschempp reacted with thumbs up emoji

@Simperfit
Copy link
ContributorAuthor

@chalasr WDYT ?

@Simperfit
Copy link
ContributorAuthor

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

@Simperfit
Copy link
ContributorAuthor

@fabpot this is ready

@SimperfitSimperfitforce-pushed thebugfix/logout-remember-me-when-user-changed branch fromaefe84c tobcc51f2CompareJuly 17, 2019 19:17
@Simperfit
Copy link
ContributorAuthor

cc @chalasr@linaori

<argumenttype="service"id="logger"on-invalid="null" />
<argumenttype="service"id="event_dispatcher"on-invalid="null" />
<argumenttype="service"id="security.authentication.trust_resolver" />
<argumenttype="service"id="security.authentication.rememberme"on-invalid="null" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There is no such global service but oneRememberMeServices per context (firewall).
This argument needs to be wired fromSecurityExtension. SeeRememberMeFactory which is responsible for the service registration.

$listener->handle(newGetResponseEvent($this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(),$request, HttpKernelInterface::MASTER_REQUEST));

$this->assertNull($tokenStorage->getToken());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

norkunasand others added7 commitsJuly 25, 2019 13:54
…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 branchsymfony#32714/cc@TobionCommits-------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 insymfony#32695Commits-------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
nicolas-grekasand others added18 commitsAugust 6, 2019 09:11
* 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'ssymfony#32973 on 4.4 :PCommits-------7a44ed6 removed unneeded phpdocs
…able is not set (j92)This PR was squashed before being merged into the 4.4 branch (closessymfony#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     <!-- seehttps://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:```bashFOO=${VARIABLE:-default}```When VARIABLE is not set```bashFOO=${VARIABLE:-default} #FOO=default```When VARIABLE is set:```bashVARIABLE=testFOO=${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
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#12073Commits-------a4178f1 [HttpClient] add "max_duration" option
…mponent (yceruto)This PR was squashed before being merged into the 4.4 branch (closessymfony#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       | MITCommits-------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        | -cfhttps://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
@chalasr
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr requested changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixxlyrixx is a code owner

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

+2 more reviewers

@aschemppaschemppaschempp left review comments

@linaorilinaorilinaori left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

28 participants

@Simperfit@aschempp@chalasr@nicolas-grekas@linaori@stanislav-reshetnev@carsonbot@norkunas@yceruto@Tobion@fabpot@OskarStark@Koc@ogizanagi@xabbuh@lyrixx@paxal@vincenttouzet@Arman-Hosseini@dbu@fancyweb@ro0NL@luispabon@jderusse@Raulnet@srsbiz@pierredup@j92
[8]ページ先頭
©2009-2025 Movatter.jp