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] Be able to know the reasons of the denied access#40711

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
azjezz wants to merge2 commits intosymfony:5.4fromazjezz:vote

Conversation

azjezz
Copy link
Contributor

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?yes
TicketsFix#27995,#26343,#35592
LicenseMIT
Doc PRn/a

This is a PR continuing the work started by@maidmaid and@noniagriconomie in#35592


About the voters, the gif of the the original issue sums up the situation well :

no

Currently, the voters can only say yes or no. This PR allows to give a reason from the voters.

Before :

class PostVoterextends Voter{// ...protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token)    {if ($subject->getAuthor() !==$token->getUser()) {returnfalse;        }if ($subject->getStatus() !=='draft') {returnfalse;        }returntrue;    }}

After:

class PostVoterextends Voter{// ...protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token)    {if ($subject->getAuthor() !==$token->getUser()) {return$this->deny('You are not the author.')        }if ($subject->getStatus() !=='draft') {return$this->deny('The post is no more a draft.')        }return$this->grant();    }}

A voter returns no more an int based on the constant ACCESS_{GRANTED,ABSTAIN,DENIED} but a Vote object which contains the access result and the reason. Thus, the security panel of the profiler can display the reason of each vote.

security

The access decision manager returns no more a bool for the final verdict but an AccessDecision object which contains the final verdict and all the votes. Thus, the 403 response can display all the denied raisons.

security-ex


TODO:

  • add BC Layer
  • migrate existing voters
  • update old tests
  • add new tests

chalasr, apfelbox, OskarStark, noniagriconomie, deguif, kbsali, skmedix, onEXHovia, maidmaid, Warxcell, and maxhelias reacted with thumbs up emojiOskarStark, Warxcell, maxhelias, danielchodusov, and yellow1912 reacted with heart emoji
Co-Authored-By: Dany Maillard <danymaillard93b@gmail.com>Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>
@carsonbot

This comment has been minimized.

@azjezzazjezz changed the titleVote[Security] Be able to know the reasons of the denied accessApr 6, 2021
@azjezzazjezz marked this pull request as ready for reviewApril 6, 2021 09:36
@azjezzazjezzforce-pushed thevote branch 2 times, most recently fromb56fa30 to93e9c87CompareApril 6, 2021 09:40
@chalasrchalasr added this to the5.x milestoneApr 6, 2021
@noniagriconomie
Copy link
Contributor

Tank you for continuing this@azjezz I'll review very soon
Also to be mentioned, all the initial work has been done by@maidmaid, I've just done some reviews like others ppl :)

Copy link
Contributor

@noniagriconomienoniagriconomie left a comment

Choose a reason for hiding this comment

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

some early reviews

@@ -33,7 +33,7 @@ interface VoterInterface
* @param mixed $subject The subject to secure
* @param array $attributes An array of attributes associated with the method being invoked
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
* @return int|Vote either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
Copy link
Contributor

@noniagriconomienoniagriconomieApr 6, 2021
edited
Loading

Choose a reason for hiding this comment

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

related to comment onVoter/Voter.php

use AccessTrait;

private $reason;
private $parameters;
Copy link
Contributor

Choose a reason for hiding this comment

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

i would call this$context like in workflow, wdyt?

@@ -206,4 +216,22 @@ private function decidePriority(TokenInterface $token, array $attributes, $objec

return $this->allowIfAllAbstainDecisions;
}

private function decideIfAllAbstainDecisions(): AccessDecision
Copy link
Contributor

Choose a reason for hiding this comment

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

i would directly write this code, the function seem not mandatory

@@ -75,7 +108,7 @@ abstract protected function supports(string $attribute, $subject);
*
* @param mixed $subject
*
* @returnbool
* @returnVote Returning a boolean is deprecated since Symfony 5.1. Return a Vote object instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, 5.3
also, shouldnt we create someting like#35592 (comment) to make it BC?

@azjezzazjezzforce-pushed thevote branch 13 times, most recently frombd948f7 to5489fe2CompareApril 6, 2021 16:35
@azjezzazjezzforce-pushed thevote branch 8 times, most recently fromb5f1e4c to5b89b7eCompareApril 9, 2021 13:28
@noniagriconomie
Copy link
Contributor

👋 ping@azjezz will you be able to rebase and finish for v5.4? thx

@fabpot
Copy link
Member

Let's close for now as there is no more work here.@azjezz Feel free to reopen if you'd like to finish it.

@fabpotfabpot closed thisSep 21, 2021
@yellow1912
Copy link

I would really like to see this feature merged. Is it possible for me to pick up the work?@fabpot@azjezz I can use the current 5.4 base, merge the changes made.

@azjezz
Copy link
ContributorAuthor

@yellow1912 feel free to do so, I'm currently unable to do so due to lack of time :)

yellow1912 reacted with thumbs up emoji

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

@OskarStarkOskarStarkOskarStark left review comments

@noniagriconomienoniagriconomienoniagriconomie left review comments

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@wouterjwouterjAwaiting requested review from wouterj

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[Security][DX] Be able to know why exactly SecurityVoter returns false
7 participants
@azjezz@carsonbot@noniagriconomie@fabpot@yellow1912@OskarStark@chalasr

[8]ページ先頭

©2009-2025 Movatter.jp