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

Partially revert "fix voting on multiple roles behavior description" due to CVE-2020-5275#13457

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
wouterj merged 0 commits intosymfony:4.4fromajgarlag:cve-2020-5275
Dec 24, 2021

Conversation

@ajgarlag
Copy link
Contributor

This partially reverts commit4b76554.

Seesymfony/symfony@c935e4a

@ajgarlagajgarlag changed the base branch frommaster to4.4March 31, 2020 06:08
Copy link
Contributor

@dbrumanndbrumann left a comment

Choose a reason for hiding this comment

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

👍 I like it. Thanks.

@javiereguiluz
Copy link
Member

This is security-related, so we'd like to merge this as soon as possible.@wouterj could you please check these proposed changes? Thanks!

@xabbuh
Copy link
Member

What if we simply do not show the use of multiple attributes here, but use an expression which checks if the user is granted at least one of them to avoid the ambiguity here?

@ajgarlag
Copy link
ContributorAuthor

What if we simply do not show the use of multiple attributes here, but use an expression which checks if the user is granted at least one of them to avoid the ambiguity here?

The main problem for me is that in YAML and PHP configuration formats, the key is calledroles instead ofrole so I prefer to have it properly documented.

Maybe we could add an advice to use an expressión and avoid further problems.

@javiereguiluz
Copy link
Member

After#15503 is merged, we'll need to check if this is still relevant and if we need to fix it. Thanks!

@ajgarlag
Copy link
ContributorAuthor

After#15503 is merged, we'll need to check if this is still relevant and if we need to fix it. Thanks!

@javiereguiluz#15503 is merged and I think this is still relevant. I've rebased the PR.

javiereguiluz reacted with thumbs up emoji

@wouterjwouterj merged commitacb5fa2 intosymfony:4.4Dec 24, 2021
@wouterj
Copy link
Member

wouterj commentedDec 24, 2021
edited
Loading

Hi@ajgarlag!

I'm sorry for the long wait here. I honestly wasn't sure what to do with these changes, as I do see your point about "roles" (and also being able to pass an array of attributes to this setting).

For now, I've decided to follow@xabbuh's suggestion. The reason is that an array of roles does not have a strict OR or AND relation, nor does this depend on the strategy. For full information:

  • The strategy determines how the votes of all voters are merged. E.g.RoleVoter might return DENIED, whereasAuthenticatedVoter returns GRANTED.
  • However, the list or roles is provided to each voter and the voter itself determines how to turn the list of roles into a single vote. So if you have multiple attributes that are handled by the same voter (e.g. 2ROLE_* attributes), the strategy is no longer important and the relationship is hardcoded in the voter (in the case of roles without using role_hierarchy, it's OR).
  • For roles, this becomes even more confusing as plain roles are handled byRoleVoter, but hierarchical roles are handled byRoleHierarchyVoter. So if a ROLE_ADMIN is also a ROLE_USER (through the hierarchy) and you use['ROLE_USER', 'ROLE_ADMIN'], you end up with a completely confusing situation (where both voters merge the attributes into a single vote using hardcoded logic, and the Access decision manager merges the two votes based on the strategy).

This is exactly the reason why we removed voting on more than one attribute at the same time. Unfortunately, it's the worst change I've ever contributed to Symfony (implementation wise), which means we still have the "roles" setting and ability to use an array of roles inaccess_control. Let's see if we can fix this for 6.x :)

Anyway, in case you're still reading this message: Thanks for remaining active in this PR! If you have any questions, feel free to comment :)

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

Reviewers

1 more reviewer

@dbrumanndbrumanndbrumann approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ajgarlag@javiereguiluz@xabbuh@wouterj@dbrumann@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp