Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] #25091 add target user to SwitchUserListener#25092
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
CHANGELOG-4.0.md Outdated
| =================== | ||
| * feature#25091[Security] Add target user to SwitchUserListener (jwmickey) | ||
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.
No need to add this, will be done automatically when the version is released after merging.
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.
@iltar is right. But you need to add the feature to the component's changelog instead.
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 will move the changelog note to the correct file, thank you.
| } | ||
| if (null !==$this->logger) { | ||
| $this->logger->info('Attempting to switch to user.',array('username' =>$username)); |
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.
Won't this give the wrong username as result now?
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 wonder why we would need to do the change on line 129 instead.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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
xabbuh commentedNov 26, 2017
Can you please rebase on current |
This patch provides the target user to the SwitchUserListener'saccessDecisionManager->decide() call as the $object parameter togive any registered voters extra information.
- moved message from the general changelog to the security component'schangelog- restored exception attributes that were mistakenly removed
jwmickey commentedDec 11, 2017
Is there anything else I need to do for this PR? |
| if (false ===$this->accessDecisionManager->decide($token,array($this->role),$user)) { | ||
| $exception =newAccessDeniedException(); | ||
| $exception->setAttributes($this->role); | ||
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 change should be reverted
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.
Why should this change be reverted?
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.
Because it's not needed.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh that's a misunderstanding then. I was just referring to the blank line which should not have been removed.
xabbuh left a comment
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.
apart from my minor comment
chalasr commentedJan 18, 2018
Thank you@jwmickey and congratz for your first contribution on Symfony! |
jwmickey commentedJan 18, 2018
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.