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][SecurityBundle] Add voter individual decisions to profiler#27914

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

Merged
fabpot merged 1 commit intosymfony:masterfroml-vo:add_voter_votes_to_profiler
Oct 28, 2018

Conversation

@l-vo
Copy link
Contributor

@l-vol-vo commentedJul 10, 2018•
edited
Loading

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

On the profiler (security tab), the decisions made by the AccessDecisionManager are displayed with the voters registered in the application. But there is no information about which voter was really used for each AccessDecisionManager choice and which result the voters returned.

This PR allows to display for each AccessDecisionManager choice, the voters implicated and the vote they returned.

Screenshot profiler before PR:
capture d ecran 2018-07-10 a 13 26 46
Screenshot profiler after PR:
capture d ecran 2018-10-10 a 21 35 30

stloyd, theofidry, ismail1432, mykiwi, andreybolonin, ScullWM, and aivus reacted with thumbs up emojiro0NL reacted with hooray emoji
@javiereguiluz
Copy link
Member

@l-vo this is something we definitely want to improve. Thanks! If possible, please paste a before/after screenshot comparison.

@l-vo
Copy link
ContributorAuthor

l-vo commentedJul 10, 2018•
edited
Loading

I didn't understand what CHANGELOG file I should update ? There is not CHANGELOG.md file and the CHANGELOG-4.1.md not seems to me the good file since I think my PR is for the 4.2 version.
EDIT: Found the file, I Iooked for it in the symfony project root, not in the component/bundle... I'm going to add changelog shortly.

$decisionLog =$this->accessDecisionManager->getDecisionLog();

// Voter constants
$reflexionClass =new \ReflectionClass(VoterInterface::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • $reflectionClass

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@curry684 Fixed, thank you :)

@l-vo
Copy link
ContributorAuthor

@javiereguiluz : screenshots added, thanks !

javiereguiluz reacted with thumbs up emoji

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch 3 times, most recently from51369d7 to59761f6CompareJuly 10, 2018 12:02
@l-vol-vo changed the titleAdd voter individual decisions to profiler[Security][SecurityBundle] Add voter individual decisions to profilerJul 10, 2018
@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch 2 times, most recently from3aa0f2d to3a82646CompareJuly 11, 2018 19:47
@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch from3a82646 to2175182CompareJuly 12, 2018 20:10
@ro0NL
Copy link
Contributor

ro0NL commentedJul 13, 2018•
edited
Loading

I'd like to see the "Show/hide voter details" toggle per row in the initial table (right column), toggling a single new table of voter details underneath it. Yes, there'd be always 1 active table with voter details.

Currently it looks cluttered, and unclear which table belongs to what.

edit; but wait for@javiereguiluz's opion 😉

@javiereguiluz
Copy link
Member

@ro0NL that's exactly what I was thinking. It's like the "Show more" link that GitHub uses in some places:

show-more

My idea was going to wait until this PR was merged and then tweak its design a bit in another PR. I have some sketches ready, but need time to polish it.

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch from2175182 tob1b73d9CompareJuly 14, 2018 14:29
@l-vo
Copy link
ContributorAuthor

Add PR changes to CHANGELOG files

@l-vo
Copy link
ContributorAuthor

@ro0NL@javiereguiluz ok for doing that in this way, I was not really inspired for how changing the current interface efficiently.

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch fromb1b73d9 tof3157daCompareJuly 14, 2018 19:32
@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch fromf3157da to6794076CompareAugust 26, 2018 08:03
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

shouldVoteEvent +VoteListener also marked@internal? Tend to believe at least the event should kept internal.

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch from8a28b3d to42b0de2CompareOctober 19, 2018 12:28
@l-vo
Copy link
ContributorAuthor

@ro0NL It makes sense, done, thanks :)
@fabpot Last modifications done, thanks !

@l-vo
Copy link
ContributorAuthor

status: needs review

@chalasr
Copy link
Member

Great work so far, but#27914 (comment) is not fully resolved, right?

@l-vo
Copy link
ContributorAuthor

l-vo commentedOct 28, 2018•
edited
Loading

@chalasr It is fully resolved. This problem occured whenTraceableVoter stored consecutive votes of its decorated voter. Using events and storing this information at AccessDecisionManager level solves this issue.

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Good for 4.2 IMHO

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch from42b0de2 to5af1d0cCompareOctober 28, 2018 17:31
@l-vo
Copy link
ContributorAuthor

Fixed some minors things,VoteListener class description and SecurityCHANGELOG (see self-review above). No code modified.

@l-vol-voforce-pushed theadd_voter_votes_to_profiler branch from5af1d0c to8abb056CompareOctober 28, 2018 17:49
@fabpot
Copy link
Member

Thank you@l-vo.

@fabpotfabpot merged commit8abb056 intosymfony:masterOct 28, 2018
fabpot added a commit that referenced this pull requestOct 28, 2018
…ons to profiler (l-vo)This PR was merged into the 4.2-dev branch.Discussion----------[Security][SecurityBundle] Add voter individual decisions to profiler| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |On the profiler (security tab), the decisions made by the AccessDecisionManager are displayed with the voters registered in the application. But there is no information about which voter was really used for each AccessDecisionManager choice and which result the voters returned.This PR allows to display for each AccessDecisionManager choice, the voters implicated and the vote they returned.Screenshot profiler before PR:![capture d ecran 2018-07-10 a 13 26 46](https://user-images.githubusercontent.com/15314293/42507425-7320156c-8445-11e8-9f73-a91bdae2eca5.png)Screenshot profiler after PR:<img width="1093" alt="capture d ecran 2018-10-10 a 21 35 30" src="https://user-images.githubusercontent.com/15314293/46761807-c16c4a00-ccd5-11e8-85b1-8cc0ea3eb9b8.png">Commits-------8abb056 [Security][SecurityBundle] Add voter individual decisions to profiler
@l-vol-vo deleted the add_voter_votes_to_profiler branchOctober 28, 2018 18:37
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
nicolas-grekas added a commit that referenced this pull requestNov 11, 2018
…mata)This PR was merged into the 4.2-dev branch.Discussion----------[SecurityBundle] unhide debug security voter services| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| License       | MIT#27914 introduces `testThatVotersAreNotDecoratedWithoutDebugMode()` which tests if decorated services exist but uses a bad service name without starting dot.Definition in the compiler pass :https://github.com/symfony/symfony/blob/a4204cd685c02377e6e2fbfc7ece98b5563644d9/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php#L58-L66The expected services are hidden and their name start with a dot. So the test will always pass, now it can fails :)Commits-------4677bb4 [SecurityBundle] unhide debug security voter services
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof requested changes

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

@chalasrchalasrchalasr approved these changes

+3 more reviewers

@ro0NLro0NLro0NL left review comments

@rpkamprpkamprpkamp left review comments

@curry684curry684curry684 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

12 participants

@l-vo@javiereguiluz@ro0NL@fabpot@chalasr@stof@rpkamp@curry684@xabbuh@nicolas-grekas@ogizanagi@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp