[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
0ed7be1 to
45e339d
Compare
|
|
||
| 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.
45e339d to
c1e056a
Compare
|
Would you like me to do anything else in regards to this? |
xabbuh
reviewed
Apr 19, 2017
| @@ -46,4 +49,20 @@ public function getTargetUser() | |||
| { | |||
| return $this->targetUser; | |||
| } | |||
|
|
|||
| /** | |||
| * @return TokenInterface | |||
xabbuh
reviewed
Apr 19, 2017
| } | ||
|
|
||
| /** | ||
| * @param TokenInterface $token |
|
Can you also add a test that checks that the token can be replaced? |
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
| @@ -4,6 +4,8 @@ CHANGELOG | |||
| 3.3.0 | |||
| ----- | |||
|
|
|||
| * 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
429cbed to
6fbbf28
Compare
d83e7cc to
b32b205
Compare
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
| @@ -13,6 +13,8 @@ CHANGELOG | |||
| 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
ee25877 to
364078b
Compare
|
Seems legit to me. @klandaika would you mind to rebase this PR against the 3.4 branch? |
364078b to
1a8a401
Compare
xabbuh
reviewed
Sep 26, 2017
| $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
| @@ -4,6 +4,8 @@ CHANGELOG | |||
| 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
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
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