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 ability for voters to explain their vote#59771

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:7.3fromnicolas-grekas:sec-vote-reasons
Feb 17, 2025

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedFeb 13, 2025•
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?no
Deprecations?no
IssuesFix#27995,#26343,#35592,#43147
LicenseMIT

This 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

image

Internally, this provides a new access audit log infrastructure that relies on newAccessDecision andVote objects.

Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from#58107 /#46493 as much as possible.

j-schumann, deguif, alexandre-daubois, 94noni, SVillette, alamirault, chalasr, cybernet, nuryagdym, and andreybolonin reacted with thumbs up emojirvanlaak and macghriogair reacted with hooray emoji
Copy link
Member

@alexandre-dauboisalexandre-daubois left a comment

Choose a reason for hiding this comment

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

Just a thought at first read:Vote andAccessDecision really look like value objects.

Should we make:

  • AccessDecision::isGranted readonly and set with a constructor? The decision may not change when the object is created, and if so, a new instance may be created instead because that's another decision actually ;
  • The same thing forVote::$result andVote::$voter? Here again, if one of these property has to change after instantiation, then it may be more correct to create a new instance because this is not the "same" vote anymore.

@nicolas-grekas
Copy link
MemberAuthor

@alexandre-daubois making the properties readonly would break the FC layer: setting the results multiple times is how an updated implementation can still give proper feedback when given a non-updated one (voters/access decision managers/strategies.)

alexandre-daubois reacted with thumbs up emoji

@nicolas-grekasnicolas-grekasforce-pushed thesec-vote-reasons branch 3 times, most recently fromfae982d to20e7b35CompareFebruary 14, 2025 13:26
try {
return $this->userSecurityChecker->isGrantedForUser($user, $attribute, $subject, $accessDecision);
} catch (AuthenticationCredentialsNotFoundException) {
return false;
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

added for parity with isGranted() above

@nicolas-grekasnicolas-grekasforce-pushed thesec-vote-reasons branch 2 times, most recently from69264d6 to2f4b88eCompareFebruary 14, 2025 15:24
@nicolas-grekas
Copy link
MemberAuthor

This PR should be ready for review.
I fixed tests, so that it's green, but I'll add more.

Status: needs review

public function __construct(
private TokenStorageInterface $tokenStorage,
private AccessDecisionManagerInterface $accessDecisionManager,
) {
}

final public function isGranted(mixed $attribute, mixed $subject = null): bool
final public function isGranted(mixed $attribute, mixed $subject = null, ?AccessDecision $accessDecision = null): bool
Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasFeb 14, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Note that wecould decide to not expose AccessDecision objects when using (User)AuthorizationCheckerInterface.
Any opinions on this?

Having the AccessDecision available toisGranted[ForUser]() methods would allow userland to get the audit log more easily. It's also used right now inAbstractController::denyUnlessGranted(), to compute a better error message. Using the AccessDecisionManager instead of the (User)AuthorizationChecker would be the alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Having it in(User)AuthorizationCheckerInterface looks useful enough

@94noni
Copy link
Contributor

What a ride for those PRs :)
Bravo to all devs includes in the process 👏

rvanlaak reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@nicolas-grekas.

rvanlaak, alamirault, and OskarStark reacted with hooray emoji

@fabpotfabpot merged commit2962567 intosymfony:7.3Feb 17, 2025
8 of 10 checks passed
@nicolas-grekasnicolas-grekas deleted the sec-vote-reasons branchFebruary 17, 2025 14:12
@fabpotfabpot mentioned this pull requestMay 2, 2025
@kevinpapst
Copy link

The methodVote:addReason() was added, but the code calls$vote->reasons[] all the time. Would be great if we could replace the actual Vote instance with a custom implementation that does not store the reasons. This code can be called hundreds of times and there is no benefit in production to actually store these information.

@nicolas-grekas
Copy link
MemberAuthor

We'd need benchmarks to consider any perf/mem issue here. To me, this should be negligible.
Also: this can be used in prod to compute nice user error messages if one wants to (some will).

@j-schumann
Copy link

Also: this can be used in prod to compute nice user error messages if one wants to (some will).

This is our primary/only reason why we wanted this feature.

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

@j-schumannj-schumannj-schumann left review comments

@gharlangharlangharlan left review comments

@stofstofstof left review comments

@alexandre-dauboisalexandre-dauboisalexandre-daubois left review comments

@alamiraultalamiraultalamirault left review comments

@fabpotfabpotfabpot approved these changes

@94noni94noni94noni approved these changes

@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
11 participants
@nicolas-grekas@94noni@fabpot@kevinpapst@j-schumann@gharlan@stof@alexandre-daubois@chalasr@alamirault@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp