Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security][SecurityBundle] Add voter individual decisions to profiler #27914

Merged
merged 1 commit into from Oct 28, 2018

Conversation

@l-vo
Copy link
Contributor

l-vo commented Jul 10, 2018

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
Screenshot profiler after PR:
capture d ecran 2018-10-10 a 21 35 30

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jul 10, 2018

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

@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Jul 10, 2018

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

This comment has been minimized.

Copy link
@curry684

curry684 Jul 10, 2018

Contributor
  • $reflectionClass

This comment has been minimized.

Copy link
@l-vo

l-vo Jul 10, 2018

Author Contributor

@curry684 Fixed, thank you :)

@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Jul 10, 2018

@javiereguiluz : screenshots added, thanks !

@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch 3 times, most recently from 51369d7 to 59761f6 Jul 10, 2018
@l-vo l-vo changed the title Add voter individual decisions to profiler [Security][SecurityBundle] Add voter individual decisions to profiler Jul 10, 2018
@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch 2 times, most recently from 3aa0f2d to 3a82646 Jul 11, 2018
@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from 3a82646 to 2175182 Jul 12, 2018
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Jul 13, 2018

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

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jul 13, 2018

@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-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from 2175182 to b1b73d9 Jul 14, 2018
@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Jul 14, 2018

Add PR changes to CHANGELOG files

@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Jul 14, 2018

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

@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from b1b73d9 to f3157da Jul 14, 2018
@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from f3157da to 6794076 Aug 26, 2018
@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from 8a28b3d to 42b0de2 Oct 19, 2018
@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Oct 19, 2018

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

@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Oct 19, 2018

status: needs review

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Oct 28, 2018

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

@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Oct 28, 2018

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

Copy link
Member

chalasr left a comment

Good for 4.2 IMHO

@chalasr chalasr added this to RFR in Profiler panels Oct 28, 2018
@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from 42b0de2 to 5af1d0c Oct 28, 2018
@l-vo

This comment has been minimized.

Copy link
Contributor Author

l-vo commented Oct 28, 2018

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

@l-vo l-vo force-pushed the l-vo:add_voter_votes_to_profiler branch from 5af1d0c to 8abb056 Oct 28, 2018
@fabpot
fabpot approved these changes Oct 28, 2018
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Oct 28, 2018

Thank you @l-vo.

@fabpot fabpot merged commit 8abb056 into symfony:master Oct 28, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Oct 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-vo l-vo deleted the l-vo:add_voter_votes_to_profiler branch Oct 28, 2018
@chalasr chalasr moved this from RFR to DONE in Profiler panels Oct 29, 2018
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
nicolas-grekas added a commit that referenced this pull request Nov 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-L66

The 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.