Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

maxperrimond
Copy link
Contributor

@maxperrimondmaxperrimond commentedDec 29, 2016
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?yes
Deprecations?no
Tests pass?no
Fixed tickets
LicenseMIT
Doc PRTODO

(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

linaori, Nicofuma, ogizanagi, ErikSaunier, yceruto, stephanvierkant, and Isinlor reacted with thumbs up emoji
Copy link
Contributor

@linaorilinaori left a 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)
Copy link
Contributor

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)
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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;
Copy link
Contributor

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
);
Copy link
Contributor

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?

Copy link
ContributorAuthor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should probably be final

@ro0NL
Copy link
Contributor

Maybe im missing it.. but isnt this about just the same as simply reporting fromyourVoterInterface::vote method? Whatever the report strategy is..

In other words.. what's the difference between doing it ininitialize vs.vote? The data is the same-ish, the calling order is the same... 😕

vote is already all about decision making, just not sure this can/should be captured with a standard...

@maxperrimondmaxperrimondforce-pushed thevoter-message branch 2 times, most recently from8028a1e to9aff6fdCompareDecember 30, 2016 02:09
@maxperrimond
Copy link
ContributorAuthor

@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 forinitialize, I know it's a bit a pollution and it's only for internal usage, that's why I tried to separate it from theVoterInterface but this method is in any case to be in decision making.

@ro0NL
Copy link
Contributor

ro0NL commentedDec 30, 2016
edited
Loading

I understand the usecase, im not sure a) if belongs to core and b) we really need 2 methods on this.

From my POVvote is about decision-making, and where context diverges in many, many (i've seen some voters :)), many contexts. It's where the true magic happens.

Given

if ($user->isBad()) {returnfalse;}if ($user->getAge() <12) {returnfalse;}returntrue;

These are 3 contexts already, not sure you propose the if/else structure ininitialize as well? But to me this is about at simple as adding a$logger->info() something above each return invote. And im not surethat should be standardized :)

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.

@linaori
Copy link
Contributor

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.

ogizanagi reacted with thumbs up emoji

@ro0NL
Copy link
Contributor

ro0NL commentedDec 30, 2016
edited
Loading

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 specialVoteResult...

//SomeVote::votereturn$yes ?self::GRANTED :newVoteResult(self::DENIED,'report');
jvasseur reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the3.x milestoneJan 2, 2017
@maxperrimond
Copy link
ContributorAuthor

Quite like for aVoteResult, I will do some changes soon for a try

@ro0NL
Copy link
Contributor

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?

@linaori
Copy link
Contributor

@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
Copy link
Contributor

ro0NL commentedJan 14, 2017
edited
Loading

We could always go withvote() : VoteResult of course :) (additional interface).

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.

@linaori
Copy link
Contributor

The "old" implementation could be nothing more than being decorated with a vote result without a message (for example)

@ro0NL
Copy link
Contributor

Exactly.

@maxperrimond
Copy link
ContributorAuthor

I made a new version of it withVoterResult as you mention, let me know what do you think of this version.
Cheerz

*
* @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)
Copy link
Contributor

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

Copy link
ContributorAuthor

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 ?

Copy link
Member

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

@chalasr
Copy link
Member

@maxperrimond Any chance for this PR to be finished?

@weaverryan
Copy link
Member

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 :)

stephanvierkant reacted with thumbs down emoji

@weaverryanweaverryan modified the milestones:3.4,4.1Sep 27, 2017
@fabpot
Copy link
Member

Based on@weaverryan's last comment, I'm closing this PR as the code won't be merged anyway.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@linaorilinaorilinaori requested changes

Assignees
No one assigned
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

9 participants
@maxperrimond@ro0NL@linaori@chalasr@weaverryan@fabpot@stof@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp