Skip to content

Added access decision strategy to respect voter priority #34548

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

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

aschempp
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR will happily do if this is of interest/to be merged 🙃

The priority-based access decision strategy will decide based on the first voter that does not abstain from the decision. Security voters can be registered with priority (PriorityTaggedServiceTrait), so a voter with higher priority can overrule other voters.

In Contao CMS, the core system should provide security voters that provide the "default permissions", but extensions/bundles can override almost anything and therefore need to be able to override the core decision. None of the existing strategies allow for something like that.

/ping @chalasr @Toflar @leofeyer @ausi
#SymfonyHackday

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

missing composer.json update: the bundle is not compatible with security-core < 5.1 now
OR the bundle should add this choice conditionnaly.
LGTM feature-wise.

@aschempp
Copy link
Contributor Author

regarding if/elseif, I just implemented it the same way the other strategies are implemented 🤷‍♂😊 let me know if I should change it.

Sorry I don't understand what you're referring to with the composer.json and security-core?

@nicolas-grekas
Copy link
Member

Sorry I don't understand what you're referring to with the composer.json and security-core?

have a look at deps=low failures ;)

@nicolas-grekas
Copy link
Member

I just implemented it the same way the other strategies are implemented

only AccessDecisionManager does it this way, and honeslty the switch looks less readable than some early return statements to me...

@aschempp
Copy link
Contributor Author

only AccessDecisionManager does it this way, and honeslty the switch looks less readable than some early return statements to me...

Should I change the other statements too then?

@nicolas-grekas
Copy link
Member

Should I change the other statements too then?

On branch 3.4 then, as all CS changes (to reduce future merge conflicts).
And only if you're up to (not doing it is fine too :) )

@aschempp
Copy link
Contributor Author

now that I see it, it really looks much better 👍

chalasr added a commit that referenced this pull request Dec 15, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

CS for AccessDecisionManager

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #34548
| License       | MIT
| Doc PR        | -

As discussed in #34548 with @nicolas-grekas here's a CS change for the `AccessDecisionManager`

Commits
-------

b3742ec CS
@chalasr
Copy link
Member

chalasr commented Dec 15, 2019

@aschempp Can you add an entry to security and security-bundle CHANGELOG files?

@aschempp
Copy link
Contributor Author

Updated in 0c78239. Let me know if that makes sense, I never did CHANGELOG files before 🙃

@chalasr chalasr force-pushed the feature/security-priority-strategy branch from 0c78239 to 0b8028a Compare December 18, 2019 13:26
@chalasr
Copy link
Member

chalasr commented Dec 18, 2019

Thank you @aschempp.

@fabpot fabpot mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants