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] Add the ability for voter to return decision reason#46493

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
alamirault wants to merge5 commits intosymfony:7.3fromalamirault:access-decision

Conversation

alamirault
Copy link
Contributor

@alamiraultalamirault commentedMay 29, 2022•
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?yes
TicketsFix#27995,#26343,#35592,#43147
LicenseMIT
Doc PR/

This PR continue the work started by@maidmaid and@noniagriconomie in#35592, continued by@yellow1912 in#43147.

It allow to have informations about AccessDecisionManager and Voters (And understand why we have an AccessDeniedException with clear infos).

I rebased on6.26.36.4 7.1 and adapt code in some cases

image
image

TODO:

  • Update Changelog.md
  • Update Upgrade-x.md
  • PR Documentation

Gemorroj, Jibbarth, deguif, nklth, zajca, Guuzen, rvanlaak, Mirgen, priyadi, AsfalothDE, and 4 more reacted with thumbs up emoji94noni, Kern046, maidmaid, and rvanlaak reacted with rocket emojiOskarStark and MatTheCat reacted with eyes emoji
/**
* {@inheritdoc}
*/
public function getDecision(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): AccessDecision
Copy link
ContributorAuthor

@alamiraultalamiraultMay 29, 2022•
edited
Loading

Choose a reason for hiding this comment

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

$allowMultipleAttributes is indecide method (but not in interface). I think we can remove this arg and the check WDYT ?

Original comment:
Special case for AccessListener, do not remove the right side of the condition before 6.0

related toGHSA-g4m9-5hpf-hx72

@@ -34,6 +36,8 @@ interface VoterInterface
* @param array $attributes An array of attributes associated with the method being invoked
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
*
* @deprecated since Symfony 6.2, use {@see getVote()} instead.
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Note sure if is the best way.
Also naming is probably not the best, maybevoteDecision is better ?

Copy link
Contributor

Choose a reason for hiding this comment

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

what aboutdoVote() ?
and when doing v7.0 we can revert tovote()

@carsonbot
Copy link

Hey!

I think@NicoHaase has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Contributor

@94noni94noni left a comment

Choose a reason for hiding this comment

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

Thx continuing this ! Was on my head for this summer glad you rebegin it!
I can obviously help review & test when on a laptop again

also do not forget to upgrade the upgrade file for new features desc

@@ -34,6 +36,8 @@ interface VoterInterface
* @param array $attributes An array of attributes associated with the method being invoked
*
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
*
* @deprecated since Symfony 6.2, use {@see getVote()} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

what aboutdoVote() ?
and when doing v7.0 we can revert tovote()

*/
public function getDecision(TokenInterface $token, array $attributes, mixed $object = null, bool $allowMultipleAttributes = false): AccessDecision
{
// Special case for AccessListener, do not remove the right side of the condition before 6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

to remove then now?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Related to#46493 (comment)

This was not removed on 6.x. I don't know why, and its impacts (CVE-2020-5275)

return $this->messages;
}

public function getMessage(): string
Copy link
Contributor

Choose a reason for hiding this comment

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

what about a more specific name?
like getReasonMessage or something like that?

@alamirault
Copy link
ContributorAuthor

Hi all,

I rebased on branch 6.3.

This feature improve a lot DX and I hope it could be merge before 7.0. (6.3 ? 😃)

Feedback/review are welcomed !

(@94noni Thanks for your first review, i'm waiting other review to see what are the best namming)

94noni reacted with thumbs up emojirvanlaak reacted with hooray emoji

@rvanlaak
Copy link
Contributor

rvanlaak commentedAug 24, 2023•
edited
Loading

This PR is really cool and provides extremely valuable insights in the voting process.@nicolas-grekas given you modified the milestone from 6.3 to 6.4 in May, any idea on what it would still take to deliver it with 6.4 (aside of getting CI green)?

94noni and deguif reacted with thumbs up emoji

@94noni
Copy link
Contributor

94noni commentedAug 24, 2023•
edited
Loading

I think this PR need to be rebases on 6.4 as 6.3 is live
But as seen in PR history ,@alamirault already done that multiple times and perhaps is waiting more reviewers to continuing investing time and effort on it :)

friendly ping@chalasr as « codeowner » on security related code

alamirault reacted with thumbs up emoji

@alamirault
Copy link
ContributorAuthor

I rebased on 6.4.

Considering the number of upvote on issue, the feature is highly expected.

Reviewer are welcomed ! 🙏

rvanlaak, j-schumann, and deguif reacted with hooray emoji

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I never opened this PR and I'm not expert in the domain, but now that I reviewed it, I'm wondering if all those deprecations aren't too much. It feels like we're throwing out a subsystem to replace it by a more capable one. That might be too costly for the community.

I'm looking for an approach that'd augment the existing voters instead of requiring everyone to rewrite theirs. This means that if a voter doesn't expose reasons, it would be fine writing them as today. And more capable ones could opt-in for exposing more detailed info.

Here is one idea: we allow voteOnAttribute to return int|string, string being meant as the reason to deny.
Then, we'd need another convention to allow thevote() method to give this info back to the caller. To do that, could we use an attribute on the token object? Another idea: we could document that voters may accept a last?string &$reason argument, and if they do, they should populate it with the reason phrase. This would be in the interface as an@param so that we don't force implementing it.

The same reasoning would then propagate to access decisions.

WDYT?

@94noni
Copy link
Contributor

thank for stopping by and for commenting here :)@nicolas-grekas

obviously since thefirst idea/and PR of this idea of the security voting return reason, the codebase evolves as well as the langage and also implementations ideas/developpers involved

this feature is, to me at least, still very relevant (think of workflow transition blocker message, etc) and can add lot of value

i am not the code owner of this PR but have reviewed a lot this one, as well as all priors one, and I dont mind the implementation details as much as the feature is easily integrated/maintainable in the current codebase for core team and obviously for the upgrade path for developpers

@alamirault i am opened for a discussion if you need so, perhaps we can find good compromise to make this advance!

cheers :)

@yellow1912
Copy link

It's been such a long time, and I was busy with personal work. That said, I think this is still relevant. And perhaps with the new direction we can pick it up again. There have been so many changes in the code base that merging does not seem like an option now. The way to move ahead is to probably start from scratch with the latest code base.

@xabbuhxabbuh modified the milestones:7.1,7.2May 15, 2024
@eltharin
Copy link
Contributor

eltharin commentedAug 7, 2024•
edited
Loading

HI,
I don't think there are most f changes to do, I put your work in a 7.2 branch I made some updates for a personal idea and it works.
here is my work :https://github.com/eltharin/symfony/tree/add_messages_and_score_on_votes
I suppress deprecated to keep all projects in place safe, make some little touchs, set a scoring vote capability and it's OK, I don't finish to copy all test beacause I want keep all existing to keep backward compatibility so juste add new

@nicolas-grekas
Copy link
Member

I took over in#59771, thanks for giving this a try!

fabpot added a commit that referenced this pull requestFeb 17, 2025
…e (nicolas-grekas)This PR was merged into the 7.3 branch.Discussion----------[Security] Add ability for voters to explain their vote| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        |Fix#27995,#26343,#35592,#43147| License       | MITThis PR takes over#58107, which itself took over from#46493, etc.It takes the only approach I could think of that would preserve BC.The visible tip of what this achieves is:![image](https://github.com/user-attachments/assets/67bd0678-868f-40ed-b2c6-3b1f10ffe101)![image](https://github.com/user-attachments/assets/715814d8-e4de-47f0-aa0e-c412092389ff)Internally, this provides a new access audit log infrastructure that relies on new `AccessDecision` and `Vote` objects.Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from#58107 /#46493 as much as possible.Commits-------c6eb9ae [Security] Add ability for voters to explain their vote
@alamiraultalamirault deleted the access-decision branchMarch 4, 2025 11:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@94noni94noni94noni left review comments

@wouterjwouterjAwaiting requested review from wouterj

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[Security][DX] Be able to know why exactly SecurityVoter returns false
9 participants
@alamirault@carsonbot@rvanlaak@94noni@yellow1912@eltharin@nicolas-grekas@fabpot@xabbuh
[8]ページ先頭
©2009-2025 Movatter.jp