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

[Security][Firewall] Passing the newly generated security token to the event during user switching#21951

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

chalasr merged 1 commit intosymfony:3.4fromklandaika:include-security-token-in-switch-user

Sep 26, 2017

Merged

[Security][Firewall] Passing the newly generated security token to the event during user switching#21951

chalasr merged 1 commit intosymfony:3.4fromklandaika:include-security-token-in-switch-user

Sep 26, 2017

Conversation

Copy link

Contributor

klandaika commentedMar 9, 2017•
edited
Loading

Event allows listeners to easily switch out the token if custom token updates are required

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

Updated SwitchUserEvent to include the generated security Token. Allows the listeners to replace the token with their own (in case an application has some custom logic for token generation). The SwitchUserListener will now use the token returned by the event, so if token was not changed the self generated token will be used. If token was changed in the event then the new token would get used.

Reasons for this feature

In our current project users can have different Role sets depending on which organization they switch to. OurUser->getRoles() always returns ["ROLE_USER"] and after login user is presented with choice of organizations they want to work in. Based on selected organization roles get updated with then stored token.

Without the change proposed in this PR. The only way we can setup the proper roles during user switch is by replacingsecurity.authentication.switchuser_listener service with our own implementation of the listener.

With the proposed change, we can replace the security token with the one having all the roles we require directly inside our listener forsecurity.switch_user event that gets thrown by Symfony'sSwitchUserListener

carsonbot added Status: Needs Review Security Feature labels

Mar 9, 2017

klandaika force-pushed theinclude-security-token-in-switch-user branch 2 times, most recently from0ed7be1 to45e339dCompare

March 9, 2017 17:56

nicolas-grekas reviewed

Mar 10, 2017

View reviewed changes

src/Symfony/Component/Security/Http/Event/SwitchUserEvent.php Outdated

		private$token;

		publicfunction__construct(Request$request,UserInterface$targetUser)
		publicfunction__construct(Request$request,UserInterface$targetUser,TokenInterface$token)

Copy link

Member

nicolas-grekasMar 10, 2017

Choose a reason for hiding this comment

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

adding a new required argument is a BC break. Instead, you should either make it optional, or rely solely on the setter

Copy link

ContributorAuthor

klandaikaMar 10, 2017

Choose a reason for hiding this comment

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

Got it. Made it optional.

nicolas-grekas added this to the3.3 milestone

Mar 10, 2017

klandaika force-pushed theinclude-security-token-in-switch-user branch from45e339d toc1e056aCompare

March 10, 2017 17:11

Copy link

ContributorAuthor

klandaika commentedMar 15, 2017

Would you like me to do anything else in regards to this?

xabbuh reviewed

Apr 19, 2017

View reviewed changes

src/Symfony/Component/Security/Http/Event/SwitchUserEvent.php Outdated

		}

		/**
		* @return TokenInterface

Copy link

Member

xabbuhApr 19, 2017

Choose a reason for hiding this comment

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

TokenInterface|null

xabbuh reviewed

Apr 19, 2017

View reviewed changes

src/Symfony/Component/Security/Http/Event/SwitchUserEvent.php Outdated

		}

		/**
		* @param TokenInterface $token

Copy link

Member

xabbuhApr 19, 2017

Choose a reason for hiding this comment

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

this docblock is not needed

Copy link

Member

xabbuh commentedApr 19, 2017

Can you also add a test that checks that the token can be replaced?

klandaika force-pushed theinclude-security-token-in-switch-user branch 4 times, most recently from2e32f4b toc5d89e1Compare

April 19, 2017 16:47

Copy link

ContributorAuthor

klandaika commentedApr 19, 2017

Did all that was requested. Please check again.

Copy link

Member

nicolas-grekas commentedApr 28, 2017

Moving to milestone 3.4 as 3.3 is about to be closed.

nicolas-grekas modified the milestones:3.4,3.3

Apr 28, 2017

xabbuh reviewed

Jun 3, 2017

View reviewed changes

src/Symfony/Component/Security/CHANGELOG.md Outdated

		-----

		* added`TokenInterface` to`\Symfony\Component\Security\Http\Event\SwitchUserEvent` to allow listeners to switch out
		the token when custom token generation is required by application.

Copy link

Member

xabbuhJun 3, 2017

Choose a reason for hiding this comment

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

This must now be done under a new3.4.0 headline

Copy link

ContributorAuthor

klandaikaJun 5, 2017

Choose a reason for hiding this comment

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

Updated

klandaika force-pushed theinclude-security-token-in-switch-user branch 2 times, most recently from429cbed to6fbbf28Compare

June 3, 2017 14:55

klandaika changed the title~~[Security][Firewall] Passing the newly generated security token to the event during user switching~~WIP: [Security][Firewall] Passing the newly generated security token to the event during user switching

Jun 3, 2017

klandaika force-pushed theinclude-security-token-in-switch-user branch 2 times, most recently fromd83e7cc tob32b205Compare

June 5, 2017 20:32

klandaika changed the title~~WIP: [Security][Firewall] Passing the newly generated security token to the event during user switching~~[Security][Firewall] Passing the newly generated security token to the event during user switching

Jun 5, 2017

klandaika force-pushed theinclude-security-token-in-switch-user branch fromeec0683 toee25877Compare

June 26, 2017 15:15

xabbuh requested changes

Jun 27, 2017

View reviewed changes

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

		$this->tokenStorage->setToken($token);
		$this->request->query->set('_switch_user','kuba');

		$this->accessDecisionManager->expects($this->once())

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

$this->any() (it does not matter how often the method is called)

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

		$this->userProvider->expects($this->once())
		->method('loadUserByUsername')->with('kuba')
		->will($this->returnValue($user));
		$this->userChecker->expects($this->once())

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

this doesn't look necessary

Copy link

ContributorAuthor

klandaikaAug 22, 2017•
edited
Loading

Choose a reason for hiding this comment

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

This was done in previous test, figured since accessDecisionManager callscheckPostAuth we should have it mocked to expect to be called.

Copy link

ContributorAuthor

klandaikaAug 22, 2017

Choose a reason for hiding this comment

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

Removed, from test, but let me know if you want me to put it back.

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

		->method('decide')->with($token,array('ROLE_ALLOWED_TO_SWITCH'))
		->will($this->returnValue(true));

		$this->userProvider->expects($this->once())

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

any()

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

		}));

		$listener =newSwitchUserListener($this->tokenStorage,$this->userProvider,$this->userChecker,'provider123',
		$this->accessDecisionManager,null,'_switch_user','ROLE_ALLOWED_TO_SWITCH',$dispatcher);

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

please do not wrap lines here

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

		{
		$token =newUsernamePasswordToken('username','','key',array('ROLE_FOO'));
		$user =newUser('username','password',array());
		$replacedToken =newUsernamePasswordToken('replaced','','key',array('ROLE_BAR'));

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

key should beprovider123 (or the other way around) to be consistent with theSwitchUserListener definition below

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

instead ofreplaced I would pass the createdUser object as the first argument instead

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


		publicfunctiontestSwitchUserWithReplacedToken()
		{
		$token =newUsernamePasswordToken('username','','key',array('ROLE_FOO'));

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

key should beprovider123 (or the other way around) to be consistent with theSwitchUserListener definition below

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

		publicfunctiontestSwitchUserWithReplacedToken()
		{
		$token =newUsernamePasswordToken('username','','key',array('ROLE_FOO'));
		$user =newUser('username','password',array());

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

instead ofusername I would pass the below createdUser object as the first argument instead

src/Symfony/Component/Security/CHANGELOG.md Outdated

		3.4.0
		-----

		* added`TokenInterface` to`\Symfony\Component\Security\Http\Event\SwitchUserEvent` to allow listeners to switch out

Copy link

Member

xabbuhJun 27, 2017

Choose a reason for hiding this comment

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

I think it's more important to document that you can change the token being used by calling thesetToken() method:

added asetToken() method to theSwitchUserEvent class to allow to replace the created token while switching users

klandaika force-pushed theinclude-security-token-in-switch-user branch fromee25877 to364078bCompare

August 22, 2017 15:32

chalasr self-requested a review

September 2, 2017 13:40

Copy link

Member

chalasr commentedSep 2, 2017

Seems legit to me.@klandaika would you mind to rebase this PR against the 3.4 branch?

klandaika force-pushed theinclude-security-token-in-switch-user branch from364078b to1a8a401Compare

September 20, 2017 13:43

Copy link

Member

nicolas-grekas commentedSep 26, 2017

ping@chalasr @xabbuh for review

xabbuh reviewed

Sep 26, 2017

View reviewed changes

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

		$switchEvent =newSwitchUserEvent($request,$token->getUser());
		$switchEvent =newSwitchUserEvent($request,$token->getUser(),$token);
		$this->dispatcher->dispatch(SecurityEvents::SWITCH_USER,$switchEvent);
		//use the token from the event in case any listeners have replaced it.

Copy link

Member

xabbuhSep 26, 2017

Choose a reason for hiding this comment

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

missing space after//

Copy link

ContributorAuthor

klandaikaSep 26, 2017

Choose a reason for hiding this comment

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

Fixed

xabbuh reviewed

Sep 26, 2017

View reviewed changes

src/Symfony/Component/Security/CHANGELOG.md Outdated

		3.4.0
		-----

		* added a setToken() method to the SwitchUserEvent class to allow to replace the created token while switching users

Copy link

Member

xabbuhSep 26, 2017

Choose a reason for hiding this comment

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

the method and class names should be enclosed with backticks:

* added a`setToken()` method to the`SwitchUserEvent` class to allow to replace the created token while switching users

Copy link

ContributorAuthor

klandaikaSep 26, 2017

Choose a reason for hiding this comment

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

Fixed

xabbuh approved these changes

Sep 26, 2017

View reviewed changes

Passing the newly generated security token to the event during user s…

4205f1b

…witching.Event allows listeners to easily switch out the token if custom token updates are required

klandaika force-pushed theinclude-security-token-in-switch-user branch from1a8a401 to4205f1bCompare

September 26, 2017 20:05

chalasr changed the base branch frommaster to3.4

September 26, 2017 20:13

chalasr approved these changes

Sep 26, 2017

View reviewed changes

Copy link

Member

chalasr commentedSep 26, 2017

Thank you@klandaika.

chalasr merged commit4205f1b intosymfony:3.4

Sep 26, 2017

chalasr pushed a commit that referenced this pull request

Sep 26, 2017

feature#21951[Security][Firewall] Passing the newly generated secur…

084e49f

…ity token to the event during user switching (klandaika)This PR was merged into the 3.4 branch.Discussion----------[Security][Firewall] Passing the newly generated security token to the event during user switchingEvent allows listeners to easily switch out the token if custom token updates are required| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |Updated SwitchUserEvent to include the generated security Token. Allows the listeners to replace the token with their own (in case an application has some custom logic for token generation). The SwitchUserListener will now use the token returned by the event, so if token was not changed the self generated token will be used. If token was changed in the event then the new token would get used.Reasons for this feature--------------------------In our current project users can have different Role sets depending on which organization they switch to. Our `User->getRoles()` always returns ["ROLE_USER"] and after login user is presented with choice of organizations they want to work in. Based on selected organization roles get updated with then stored token.Without the change proposed in this PR. The only way we can setup the proper roles during user switch is by replacing `security.authentication.switchuser_listener` service with our own implementation of the listener.With the proposed change, we can replace the security token with the one having all the roles we require directly inside our listener for `security.switch_user` event that gets thrown by Symfony's `SwitchUserListener`Commits-------4205f1b Passing the newly generated security token to the event during user switching.

This was referencedOct 18, 2017

Release v3.4.0-BETA1#24609

Merged

Release v4.0.0-BETA1#24612

Merged