Skip to content
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

Voter priority in reverse order from lowest to highest #21660

Closed
scoolen opened this issue Feb 18, 2017 · 3 comments
Closed

Voter priority in reverse order from lowest to highest #21660

scoolen opened this issue Feb 18, 2017 · 3 comments

Comments

@scoolen
Copy link
Contributor

scoolen commented Feb 18, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version v2.8.11

Although this is poorly documented, I understand that voter services can have a priority.

Relevant commit: 0643dc4

Similar to other DI tags with a priority attribute this was intended to be implemented as follows:

The priority value is optional and defaults to 0. The higher the priority, the sooner it gets executed.

The compiler pass responsible for adding all configured voters is the AddSecurityVoterPass which uses an SplPriorityQueue (max heap) to correctly order the voters one by one.

The priority queue is then converted to an array and passed to the AccessDecissionManager service definition by adding a method call for setVoters.

The problem is that right after the array conversion a ksort is done on the voters array. This actually reverses the prioritization done by SplPriorityQueue!

Result: voters are ordered from lowest to highest priority and are also processed in this order in the different decision strategies.

I think the priority queue was added later as an optimization for setting the prio as the indices of an associative array, but the obsolete call to ksort was never removed.

I double checked other compiler passes using priority attributes:

Compiler Pass A-OK unit tested impl
AddCacheWarmerPass yes yes set + ksort
AddSecurityVotersPass no no prio queue + ksort
ConfigCachePass yes yes set + ksort
ProfilerPass yes no prio queue
PropertyInfoPass yes yes set + ksort
SerializerPass yes yes set + ksort
@scoolen
Copy link
Contributor Author

scoolen commented Feb 18, 2017

Failing unit test

@linaori
Copy link
Contributor

linaori commented Feb 20, 2017

Could probably use something like the PriorityTaggedServiceTrait, but this can only be done in the master I guess

@xabbuh
Copy link
Member

xabbuh commented Feb 20, 2017

see #21679

fabpot added a commit that referenced this issue Feb 22, 2017
…(xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[SecurityBundle] fix priority ordering of security voters

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21660
| License       | MIT
| Doc PR        |

Could be updated in the `3.2` branch to make use of the `PriorityTaggedServiceTrait `.

Commits
-------

dcd19f3 fix priority ordering of security voters
@fabpot fabpot closed this as completed Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants