[Security] #25091 add target user to SwitchUserListener #25092
Conversation
CHANGELOG-4.0.md
Outdated
| @@ -1,6 +1,8 @@ | |||
| CHANGELOG for 4.0.x | |||
| =================== | |||
|
|
|||
| * feature #25091 [Security] Add target user to SwitchUserListener (jwmickey) | |||
|
|
|||
There was a problem hiding this comment.
No need to add this, will be done automatically when the version is released after merging.
There was a problem hiding this comment.
@iltar is right. But you need to add the feature to the component's changelog instead.
There was a problem hiding this comment.
I will move the changelog note to the correct file, thank you.
|
|
||
| throw $exception; | ||
| if (false === $this->accessDecisionManager->decide($token, array($this->role), $user)) { | ||
| throw new AccessDeniedException(); | ||
| } | ||
|
|
||
| if (null !== $this->logger) { | ||
| $this->logger->info('Attempting to switch to user.', array('username' => $username)); |
There was a problem hiding this comment.
Won't this give the wrong username as result now?
There was a problem hiding this comment.
I wonder why we would need to do the change on line 129 instead.
There was a problem hiding this comment.
I don't think it will give the wrong username, but I did notice that I removed the line that sets attributes on the exception, so I'll add that back in.
There was a problem hiding this comment.
@iltar, my apologies, the code I submitted did indeed change the username for JSON requests. The problem stemmed from me originally starting from the 2.7 branch and then rebasing to master, and I ended up changing some code from 2.7 that was different in master without realizing... in any case, lesson learned and thanks for pointing this out
|
Can you please rebase on current |
This patch provides the target user to the SwitchUserListener's accessDecisionManager->decide() call as the $object parameter to give any registered voters extra information.
- moved message from the general changelog to the security component's changelog - restored exception attributes that were mistakenly removed
|
Is there anything else I need to do for this PR? |
| $exception = new AccessDeniedException(); | ||
| $exception->setAttributes($this->role); | ||
|
|
There was a problem hiding this comment.
Why should this change be reverted?
There was a problem hiding this comment.
I'm sorry, I don't follow. Are you saying that the $user param does not need to be passed to the accessDecisionManager->decide() call as the 3rd parameter?
There was a problem hiding this comment.
Oh that's a misunderstanding then. I was just referring to the blank line which should not have been removed.
|
Thank you @jwmickey and congratz for your first contribution on Symfony! |
|
Whoohoo! Thank you @chalasr, very exciting! |
This patch provides the target user to the SwitchUserListener's
accessDecisionManager->decide() call as the $object parameter to
give any registered voters extra information.