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] added voter report behavior#21094
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
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 version constraints own't be correct between the bundle (service config) and the component, can you verify this?
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function __construct(array $voters = array(), $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) | ||
public function __construct(array $voters = array(),VoterContextFactoryInterface $contextFactory,$strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) |
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 is a BC break
* use for translating | ||
* report messages | ||
*/ | ||
public function __construct(VoteReportCollectorInterface $collector, TranslatorInterface $translator, $translationDomain = null) |
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.
Constructor.
can be removed, it adds nothing of value. The$translationDomain
should probably just go on 1 line.
/** | ||
* @author Maxime Perrimond <max.perrimond@gmail.com> | ||
*/ | ||
class VoteReportBuilder implements VoteReportBuilderInterface |
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.
Should probably be final
/** | ||
* @author Maxime Perrimond <max.perrimond@gmail.com> | ||
*/ | ||
class VoteReport implements VoteReportInterface |
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.
Should probably be final
* | ||
* @author Maxime Perrimond <max.perrimond@gmail.com> | ||
*/ | ||
class VoterContextFactory implements VoterContextFactoryInterface |
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.
Should probably be final
* | ||
* @author Maxime Perrimond <max.perrimond@gmail.com> | ||
*/ | ||
class VoterContext implements VoterContextInterface |
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.
Should probably be final
/** | ||
* @var null|string | ||
*/ | ||
protected $translationDomain; |
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.
Unless there's a very good reason to make them protected, please make properties private
$this->message, | ||
$this->parameters, | ||
$this->translationDomain | ||
); |
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.
Is there a reason everything is on its own line in this method?
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 thought, it's gonna be too long but no actually
/** | ||
* @author Maxime Perrimond <max.perrimond@gmail.com> | ||
*/ | ||
class VoteReportCollector implements \IteratorAggregate, VoteReportCollectorInterface |
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.
Should probably be final
Maybe im missing it.. but isnt this about just the same as simply reporting fromyour In other words.. what's the difference between doing it in
|
8028a1e
to9aff6fd
Compare@ro0NL Same as constraint validation in the form component, I wanted to give more verbosity on voters. For example as, a voter can support one or several objects and attributes, it can choose a decision without knowing the exact reason on your side. class MyVoterextends Voter{// ...protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token) {$user =$token->getUser();if ($user->isBad()) {returnfalse; }if ($user->getAge() <12) {returnfalse; }returntrue; }} Here, for example, the voter can say no on an action with the current user but you can't tell that the user was a bad person or too young. After for |
ro0NL commentedDec 30, 2016 • 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.
I understand the usecase, im not sure a) if belongs to core and b) we really need 2 methods on this. From my POV Given if ($user->isBad()) {returnfalse;}if ($user->getAge() <12) {returnfalse;}returntrue; These are 3 contexts already, not sure you propose the if/else structure in Besides, i would do plain english i guess. Not translated; i think it's rather technical info (basically logs). Using many voters will give better context by default. But then again; im not sure whyyou exactly want to separate this in 2 methods. |
I'm very happy to get a context of why it's not allowed. Imagine a list of options, but some are not allowed. As a person seeing those options, I want to know why they are not allowed. Currently I have to write a custom implementation on top of the voters (or next to it) to determine why, duplicating the domain logic. |
ro0NL commentedDec 30, 2016 • 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.
Ok that makes sense :) Still the calling order of both is the same.. meaning we dont really "initialize" all voters beforehand, not giving context beforehand. What about a special //SomeVote::votereturn$yes ?self::GRANTED :newVoteResult(self::DENIED,'report'); |
Quite like for a |
Basically it would allow to follow all authorization checks for a given request, and collect them for the profiler. Meaning we can follow each decision in detail, additionally with custom user messages. @maxperrimond@iltar wdyt? |
@ro0NL I like the idea of a vote report, but not the way it's implemented there. I don't like multiple return types, even if it enhances DX. What about a second "introspectable" base Voter? It could be very similar to the original voter but has a different way to report: publicfunction reportVoteOnAttribute(string$attribute,$subject,TokenInterface$token):VoteReport; |
ro0NL commentedJan 14, 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.
We could always go with Im just not sure if duplicating logic to provide detailed reports is convenient? You end up creating a common method providing the result + report.. edit: i understand if the purpose is to report about context or so? And not so much about the actual vote result. |
The "old" implementation could be nothing more than being decorated with a vote result without a message (for example) |
Exactly. |
I made a new version of it with |
* | ||
* @throws \InvalidArgumentException | ||
*/ | ||
public function __construct(array $voters = array(), $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) | ||
public function __construct(array $voters = array(),VoteReportBuilderInterface $reportBuilder,$strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) |
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.
Note that this is still a BC break
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.
Maybe I should set it and the behavior as optional to avoid the BC Break ?
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.
it must be optional, and it must be at the end. Shifting all arguments is a BC break
@maxperrimond Any chance for this PR to be finished? |
Hmm... I like the idea... but it's a lot of code in core to accomplish this. I'm moving to the 4.1 milestone. But, unless we find a simpler implementation (after all, this is effectively "fancy logging"), I'm 👎. So, I hope you can prove me wrong with something simpler :) |
Based on@weaverryan's last comment, I'm closing this PR as the code won't be merged anyway. |
Uh oh!
There was an error while loading.Please reload this page.
(Note: I didn't wanted to create this PR now on master but on my fork 😞, so it's still a bit work in progress).
This is a proposal of a new feature on Voters, the goal is to provide a way for a voter to do a kind of reporting why it choose this decision at a specific context.
For example on of our project, we use voters but we liked to know which voter and why was against this action and report it through a response or any kind. This can be used also in the profiler for some debugging.
PS: I will fill more this description soon, feed backs are welcome to continue this or not