Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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
[Security][Firewall] Passing the newly generated security token to the event during user switching#21951
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0ed7be1 to45e339dCompare| public function __construct(Request $request, UserInterface $targetUser) | ||
| public function __construct(Request $request, UserInterface $targetUser, TokenInterface $token) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
45e339d toc1e056aCompareWould you like me to do anything else in regards to this? |
| @@ -46,4 +49,20 @@ public function getTargetUser() | |||
| { | |||
| return $this->targetUser; | |||
| } | |||
| /** | |||
| * @return TokenInterface | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TokenInterface|null
| } | ||
| /** | ||
| * @param TokenInterface $token |
There was a problem hiding this comment.
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
Can you also add a test that checks that the token can be replaced? |
2e32f4b toc5d89e1CompareDid all that was requested. Please check again. |
Moving to milestone 3.4 as 3.3 is about to be closed. |
| @@ -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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated
429cbed to6fbbf28Compared83e7cc tob32b205Compareeec0683 toee25877Compare| $this->tokenStorage->setToken($token); | ||
| $this->request->query->set('_switch_user', 'kuba'); | ||
| $this->accessDecisionManager->expects($this->once()) |
There was a problem hiding this comment.
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)
| $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.
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
klandaikaAug 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ->method('decide')->with($token, array('ROLE_ALLOWED_TO_SWITCH')) | ||
| ->will($this->returnValue(true)); | ||
| $this->userProvider->expects($this->once()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
any()
| })); | ||
| $listener = new SwitchUserListener($this->tokenStorage, $this->userProvider, $this->userChecker, 'provider123', | ||
| $this->accessDecisionManager, null, '_switch_user', 'ROLE_ALLOWED_TO_SWITCH', $dispatcher); |
There was a problem hiding this comment.
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
| { | ||
| $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.
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
There was a problem hiding this comment.
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
| public function testSwitchUserWithReplacedToken() | ||
| { | ||
| $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); |
There was a problem hiding this comment.
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
| public function testSwitchUserWithReplacedToken() | ||
| { | ||
| $token = new UsernamePasswordToken('username', '', 'key', array('ROLE_FOO')); | ||
| $user = new User('username', 'password', array()); |
There was a problem hiding this comment.
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
| @@ -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.
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 a
setToken()method to theSwitchUserEventclass to allow to replace the created token while switching users
ee25877 to364078bCompareSeems legit to me.@klandaika would you mind to rebase this PR against the 3.4 branch? |
364078b to1a8a401Compare| $this->dispatcher->dispatch(SecurityEvents::SWITCH_USER, $switchEvent); | ||
| //use the token from the event in case any listeners have replaced it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing space after//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
| @@ -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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
…witching.Event allows listeners to easily switch out the token if custom token updates are required
1a8a401 to4205f1bCompareThank you@klandaika. |
…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.
Uh oh!
There was an error while loading.Please reload this page.
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