[Security][Firewall] Passing the newly generated security token to the event during user switching #21951
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
klandaika force-pushed the include-security-token-in-switch-user branch 2 times, most recently from 0ed7be1 to 45e339d Compare 
| private $token; | ||
| | ||
| public function __construct(Request $request, UserInterface $targetUser) | ||
| public function __construct(Request $request, UserInterface $targetUser, TokenInterface $token) |
There was a problem hiding this comment.
adding a new required argument is a BC break. Instead, you should either make it optional, or rely solely on the setter
There was a problem hiding this comment.
Got it. Made it optional.
klandaika force-pushed the include-security-token-in-switch-user branch from 45e339d to c1e056a Compare 
| Would you like me to do anything else in regards to this? |
xabbuh reviewed Apr 19, 2017
| } | ||
| | ||
| /** | ||
| * @return TokenInterface |
xabbuh reviewed Apr 19, 2017
| } | ||
| | ||
| /** | ||
| * @param TokenInterface $token |
| Can you also add a test that checks that the token can be replaced? |
klandaika force-pushed the include-security-token-in-switch-user branch 4 times, most recently from 2e32f4b to c5d89e1 Compare | Did all that was requested. Please check again. |
| Moving to milestone 3.4 as 3.3 is about to be closed. |
xabbuh reviewed Jun 3, 2017
| ----- | ||
| | ||
| * 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. |
There was a problem hiding this comment.
This must now be done under a new 3.4.0 headline
klandaika force-pushed the include-security-token-in-switch-user branch 2 times, most recently from 429cbed to 6fbbf28 Compare klandaika changed the title klandaika force-pushed the include-security-token-in-switch-user branch 2 times, most recently from d83e7cc to b32b205 Compare klandaika changed the title klandaika force-pushed the include-security-token-in-switch-user branch from eec0683 to ee25877 Compare xabbuh requested changes Jun 27, 2017
| $this->tokenStorage->setToken($token); | ||
| $this->request->query->set('_switch_user', 'kuba'); | ||
| | ||
| $this->accessDecisionManager->expects($this->once()) |
There was a problem hiding this comment.
$this->any() (it does not matter how often the method is called)
| $this->userProvider->expects($this->once()) | ||
| ->method('loadUserByUsername')->with('kuba') | ||
| ->will($this->returnValue($user)); | ||
| $this->userChecker->expects($this->once()) |
There was a problem hiding this comment.
This was done in previous test, figured since accessDecisionManager calls checkPostAuth we should have it mocked to expect to be called.
There was a problem hiding this comment.
Removed, from test, but let me know if you want me to put it back.
| ->method('decide')->with($token, array('ROLE_ALLOWED_TO_SWITCH')) | ||
| ->will($this->returnValue(true)); | ||
| | ||
| $this->userProvider->expects($this->once()) |
| })); | ||
| | ||
| $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', | ||
| $this->accessDecisionManager, null, '_switch_user', 'ROLE_ALLOWED_TO_SWITCH', $dispatcher); |
| { | ||
| $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); | ||
| $user = new User('username', 'password', array()); | ||
| $replacedToken = new UsernamePasswordToken('replaced', '', 'key', array('ROLE_BAR')); |
There was a problem hiding this comment.
key should be provider123 (or the other way around) to be consistent with the SwitchUserListener definition below
There was a problem hiding this comment.
instead of replaced I would pass the created User object as the first argument instead
| | ||
| public function testSwitchUserWithReplacedToken() | ||
| { | ||
| $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); |
There was a problem hiding this comment.
key should be provider123 (or the other way around) to be consistent with the SwitchUserListener definition below
| public function testSwitchUserWithReplacedToken() | ||
| { | ||
| $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); | ||
| $user = new User('username', 'password', array()); |
There was a problem hiding this comment.
instead of username I would pass the below created User object as the first argument instead
| 3.4.0 | ||
| ----- | ||
| | ||
| * added `TokenInterface` to `\Symfony\Component\Security\Http\Event\SwitchUserEvent` to allow listeners to switch out |
There was a problem hiding this comment.
I think it's more important to document that you can change the token being used by calling the setToken() method:
- added a
setToken()method to theSwitchUserEventclass to allow to replace the created token while switching users
klandaika force-pushed the include-security-token-in-switch-user branch from ee25877 to 364078b Compare 
| Seems legit to me. @klandaika would you mind to rebase this PR against the 3.4 branch? |
klandaika force-pushed the include-security-token-in-switch-user branch from 364078b to 1a8a401 Compare xabbuh reviewed Sep 26, 2017
| $switchEvent = new SwitchUserEvent($request, $token->getUser()); | ||
| $switchEvent = new SwitchUserEvent($request, $token->getUser(), $token); | ||
| $this->dispatcher->dispatch(SecurityEvents::SWITCH_USER, $switchEvent); | ||
| //use the token from the event in case any listeners have replaced it. |
xabbuh reviewed Sep 26, 2017
| 3.4.0 | ||
| ----- | ||
| | ||
| * added a setToken() method to the SwitchUserEvent class to allow to replace the created token while switching users |
There was a problem hiding this comment.
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 xabbuh approved these changes Sep 26, 2017
…witching. Event allows listeners to easily switch out the token if custom token updates are required
klandaika force-pushed the include-security-token-in-switch-user branch from 1a8a401 to 4205f1b Compare 
chalasr approved these changes Sep 26, 2017
| Thank you @klandaika. |
chalasr pushed a commit that referenced this pull request Sep 26, 2017
…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 switching 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. 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 referenced Oct 18, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
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.
Event allows listeners to easily switch out the token if custom token updates are required
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_listenerservice 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_userevent that gets thrown by Symfony'sSwitchUserListener