NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Security] Fix logout#24805

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:2.7fromMatTheCat:ticket_7104

May 15, 2018

Merged

[Security] Fix logout#24805

nicolas-grekas merged 1 commit intosymfony:2.7fromMatTheCat:ticket_7104

May 15, 2018

Conversation

Copy link

Contributor

MatTheCat commentedNov 3, 2017•
edited
Loading

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#6751,#7104
License	MIT

carsonbot added Status: Needs Review Security Bug labels

Nov 3, 2017

chalasr added this to the2.7 milestone

Nov 3, 2017

Copy link

Member

chalasr commentedNov 3, 2017•
edited
Loading

~~👎 because#7104 (comment), sorry~~

chalasr reviewed

Nov 3, 2017

View reviewed changes

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


		$listener->handle($event);

		$hasResponse =$event->hasResponse();

Copy link

Member

Choose a reason for hiding this comment

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

this means that it won't stop at the first listener setting the response anymore, allowing others to set the response

Copy link

ContributorAuthor

MatTheCatNov 3, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Nope onlyLogoutListener can override the response if it has been set (seehttps://github.com/symfony/symfony/pull/24805/files#diff-31ca8a8ce837591218082b00363149fcR65). Andhandle is called so#7104 (comment) doesn't apply.

Copy link

Member

Choose a reason for hiding this comment

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

Can you add a test case for this? with multiple listeners if possible

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I'll try but I don't know how to do it yet

chalasr requested a review fromstof

November 3, 2017 13:00

chalasr approved these changes

Nov 6, 2017

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 6, 2017

nicolas-grekas reviewed

Nov 12, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated

		$listeners[] =newReference('security.access_listener');

		// Logout listener
		if (isset($logoutListener)) {

Copy link

Member

Choose a reason for hiding this comment

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

comment should be removed (low value)
the "isset" shold be replaced bynull !== $logoutListener (which implies setting the var to null when appropriate)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Done

Copy link

ContributorAuthor

MatTheCat commentedNov 12, 2017

I guessTraceableFirewallListener will need to be updated when merging in 3.4/4.0

Copy link

ContributorAuthor

MatTheCat commentedNov 14, 2017

Is there something missing for this PR to be merged?

xabbuh reviewed

Nov 16, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated

		// Access listener
		$listeners[] =newReference('security.access_listener');

		if (null !==$logoutListenerId) {

Copy link

Member

Choose a reason for hiding this comment

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

Can't we just move the whole block that starts withif (isset($firewall['logout'])) { here?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Nope because factories rely on the definition being in the container so we must create it before but still register last.

xabbuh reviewed

Nov 16, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Outdated

		}

		// Determine default entry point
		$configuredEntryPoint =isset($firewall['entry_point']) ?$firewall['entry_point'] :null;

Copy link

Member

Choose a reason for hiding this comment

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

should probably be moved back as this is not needed

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Done

stof requested changes

Nov 17, 2017

View reviewed changes

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


		if ($event->hasResponse()) {
		break;
		}elseif ($hasResponse) {

Copy link

Member

Choose a reason for hiding this comment

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

justif as the previousif stops the execution path withbreak already.

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

		$exceptionListener->register($this->dispatcher);
		}

		$hasResponse =false;

Copy link

Member

Choose a reason for hiding this comment

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

You don't need to create this variable. Just use$event->hasResponse() in the right place.

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Nov 17, 2017

Copy link

Member

stof commentedNov 17, 2017

Note that this will defeat the lazy-loading added in 3.4, as the loop will always iterate until it reaches the logout listener, and it is at the end.

Copy link

ContributorAuthor

MatTheCat commentedDec 3, 2017

@chalasr @nicolas-grekas @stof @xabbuh I tried to exclude the logout listener from others to allow lazy-loading. I guess it introduced BC breaks so I would like you to review this.

Copy link

ContributorAuthor

MatTheCat commentedDec 3, 2017•
edited
Loading

I don't understand why tests fail on PHP > 7.0, seems like a routing issue... Tests pass on my computer with PHP 7.1.12

chalasr self-requested a review

December 12, 2017 01:24

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Dec 12, 2017

Copy link

ContributorAuthor

MatTheCat commentedDec 15, 2017

Should I duplicate this PR to get more visibility? The logout listener never did work so I thought this would have got more attention.

Copy link

Member

@MatTheCat No need for reopening a new one.
Sorry for not giving you more feedback yet, but be sure that this has my attention.
That's an important bug which exists for a long time, if we can fix it, we need to do it right.
I think it's close to be ok, you tried a lot of approaches and that's great. What prevents me to approve is the fact we need to change some public api, hence I want to review it more throughly, check it out and see if we can do that more transparently.
Note that as of 3.3FirewallContext::getContext() is deprecated in favor of separatedgetListeners() andgetExceptionListener(), which means we would need to add a new public method when merging this up to 3.3, hence I'm not comfortable with this as is. Please be a bit more a patient, I will give more inputs as soon as I have time.
Thanks for your understanding.

MatTheCat mentioned this pull request

Dec 21, 2017

[Security] Call logout handlers even if token is null#24769

Closed

Copy link

ContributorAuthor

MatTheCat commentedDec 27, 2017

I rebased one last time. People impacted by this issue will have to disablelogout in their configuration and call their handler in the controller of their logout route.

chalasr reviewed

Dec 27, 2017

View reviewed changes

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


		$this->assertNull($cookieJar->get('REMEMBERME'));
		}
		}

This comment was marked as resolved.

Copy link

Member

chalasr commentedDec 27, 2017•
edited
Loading

Given that this implies public API changes in patch releases, I think that the previous version would be better (passing a fake token to logout handlers if no one exists in the token storage).
@stof @xabbuh what do you think?

Copy link

Member

chalasr commentedMay 15, 2018

Does that mean mean I have to increment symfony/security version?

yea, SecurityBundle'ssymfony/security requirement should be"~2.7.47|~2.8.40" (#24805 (review))

chalasr reviewed

May 15, 2018

View reviewed changes

src/Symfony/Component/Security/Http/FirewallMapInterface.php Outdated

		/**
		* Returns the authentication listeners,andthe exception listenerto use
		* for the given request.
		* Returns the authentication listeners, the exception listenerand the logout

Copy link

Member

chalasrMay 15, 2018

Choose a reason for hiding this comment

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

just chatted a bit with@nicolas-grekas, let's revert the interface changes now (changelog update not needed anymore), we need to change this on 4.1 with proper deprecation.

Copy link

Member

nicolas-grekasMay 15, 2018

Choose a reason for hiding this comment

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

agreed: should be reverted and reintroduced in 4.1 with a deprecation notice

Copy link

ContributorAuthor

MatTheCatMay 15, 2018

Choose a reason for hiding this comment

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

okay done

chalasr approved these changes

May 15, 2018

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

👍

carsonbot added Status: Reviewed and removed Status: Needs Work labels

May 15, 2018

stof approved these changes

May 15, 2018

View reviewed changes

Copy link

Member

stof commentedMay 15, 2018

After merging this (and once it is propagated until master), we should get another PR against master to deprecate the case of returning only 2 elements in the firewall map. This can be checked in the Firewall class.

[Security] Fix logout

9e88eb5

Copy link

Member

nicolas-grekas commentedMay 15, 2018

Thank you@MatTheCat.

nicolas-grekas merged commit9e88eb5 intosymfony:2.7

May 15, 2018

nicolas-grekas added a commit that referenced this pull request

May 15, 2018

bug#24805[Security] Fix logout (MatTheCat)

15a7bbd

This PR was squashed before being merged into the 2.7 branch (closes#24805).Discussion----------[Security] Fix logout| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | no| Fixed tickets |#6751,#7104| License       | MITCommits-------9e88eb5 [Security] Fix logout

Copy link

Member

chalasr commentedMay 15, 2018

I'll take care of the master PR.

MacDada reviewed

May 15, 2018

View reviewed changes

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

		}

		if (null !==$logoutListener) {
		$logoutListener->handle($event);

Copy link

Contributor

MacDadaMay 15, 2018

Choose a reason for hiding this comment

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

😍

Copy link

ContributorAuthor

MatTheCat commentedMay 15, 2018

Yay thanks!

nicolas-grekas reviewed

May 15, 2018

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php

		private$logoutListener;

		publicfunction__construct(array$listeners,ExceptionListener$exceptionListener =null)
		publicfunction__construct(array$listeners,ExceptionListener$exceptionListener =null,LogoutListener$logoutListener =null)

Copy link

Member

nicolas-grekasMay 15, 2018

Choose a reason for hiding this comment

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

fun fact: on 3.4 & up, this classalready has a 3rd argument:FirewallConfig $config
To prevent any funnier things, I suggest adding this argument in 2.7 (but ignoring its value).
WDYT? Any better idea?

Copy link

ContributorAuthor

MatTheCatMay 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Can we do this on concerned branches only?

Copy link

Member

chalasrMay 16, 2018

Choose a reason for hiding this comment

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

we could change for a setter on 2.7, not sure it's better. Here is what you propose#27280

Copy link

Member

nicolas-grekasMay 16, 2018

Choose a reason for hiding this comment

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

I finally managed to merge, see86a9c73#diff-cf0390616c24425d612bcf9ee1555111
@chalasr when doing your PR against master, please also deprecate passing a FirewallConfig as 3rd arg there.

MatTheCat deleted the ticket_7104 branch

May 15, 2018 21:57

chalasr mentioned this pull request

May 16, 2018

[SecurityBundle] Fix FirewallContext constructor 3.4 compatibility#27280

Closed

This was referencedMay 21, 2018

Release v2.7.47#27327

Merged

Release v2.8.40#27328

Merged

Release v3.4.10#27329

Merged

Release v4.0.10#27330

Merged

MatTheCat mentioned this pull request

May 21, 2018

Logout handler won't be called when stateless is true.#5245

Closed

fabpot mentioned this pull request

May 21, 2018

Release v4.1.0-BETA2#27331

Merged

chalasr mentioned this pull request

May 22, 2018

[Security][SecurityBundle] FirewallMap/FirewallContext deprecations#27336

Merged

nreynis mentioned this pull request

May 25, 2018

Logout broken in Symfony 3.4.10hslavich/OneloginSamlBundle#53

Closed

nicolas-grekas added a commit that referenced this pull request

May 25, 2018

feature#27336[Security][SecurityBundle] FirewallMap/FirewallContext…

d314735

… deprecations (chalasr)This PR was merged into the 4.2-dev branch.Discussion----------[Security][SecurityBundle] FirewallMap/FirewallContext deprecations| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes/no <!-- don't forget to 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#... <!-- required for new features -->Next to#24805.Commits-------a71ba78 [Security][SecurityBundle] FirewallMap/FirewallContext deprecations