NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[SecurityBundle] Set`UserValueResolver`'s priority higher than`EntityValueResolver`#48032

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:6.2fromkbond:user-value-resolver-priority

Nov 9, 2022

Merged

[SecurityBundle] Set`UserValueResolver`'s priority higher than`EntityValueResolver`#48032

nicolas-grekas merged 1 commit intosymfony:6.2fromkbond:user-value-resolver-priority

Nov 9, 2022

Conversation

Copy link

Member

kbond commentedOct 28, 2022•
edited
Loading

Q	A
Branch?	6.2
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets	n/a
License	MIT
Doc PR	n/a

UserValueResolver's priority is currently40 andEntityValueResolver's priority is110 (configured in doctrine-bundle).

Currently, to use theCurrentUser attribute andMapEntity (whenauto_mapping is enabled), you need to do the following to have it work:

publicfunction postAction(  #[CurrentUser]  #[MapEntity(disabled:true)]User$user,Post$post)

This removes this need for#[MapEntity(disabled: true)] but I'm not sure the larger impact of increasing the priority ofUserValueResolver. Here is some context as to why the priorities are they way they are:

SetUserValueResolver's priority higher thanEntityValueResolver

48499b9

kbond requested review fromchalasr andwouterj ascode owners

October 28, 2022 18:19

carsonbot added Status: Needs Review SecurityBundle labels

Oct 28, 2022

carsonbot added this to the6.2 milestone

Oct 28, 2022

Copy link

carsonbot commentedOct 29, 2022

Hey!

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

Cheers!

Carsonbot

nicolas-grekas mentioned this pull request

Nov 9, 2022

Revert "Lowering the EntityValueResolver to be lower than the #[CurrentUser] feature"doctrine/DoctrineBundle#1578

Merged

nicolas-grekas approved these changes

Nov 9, 2022

View reviewed changes

Copy link

Member

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.

Related todoctrine/DoctrineBundle#1578

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 9, 2022

Copy link

Member

stof commentedNov 9, 2022

The impact would be that if you have a parameter typehinted asUserInterface or tagged asCurrentUser that has the same name than a request attribute, the UserValueResolver would now apply instead of the RequestAttributeResolver. But this is more likely to fix bugs (or at least fix DX confusion) than to introduce it (if you tag asCurrentUser, you make the intent clear that you expect the current user and not the request attribute nameduser)