Skip to content

fix: securityPostDenormalize not working because clone is made after denormalization #5182

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 2 commits into from
Nov 15, 2022

Conversation

LoicBoursin
Copy link
Contributor

@LoicBoursin LoicBoursin commented Nov 12, 2022

Q A
Branch? 3.0
Tickets #5054
License MIT

Support for security_post_denormalize was added in #4184.

At that moment, the clone to populate previousObject was made before denormalization. It worked well.

This commit moved the clone after denormalization.

After upgrading to 3.0, using security_post_denormalize on a an ApiProperty was not working anymore. Moving the $previousObject line to its first position fixed the issue on my project.

I believe this issue will be fixed too.

@soyuka
Copy link
Member

soyuka commented Nov 14, 2022

Would it be possible to have a non regression test?

@soyuka soyuka merged commit d188135 into api-platform:3.0 Nov 15, 2022
@soyuka
Copy link
Member

soyuka commented Nov 15, 2022

nice catch thanks @LoicBoursin !

@FPDK
Copy link

FPDK commented Nov 16, 2022

Thanks for the fix. For anyone still having issues, check your allow_if_all_abstain and strategy security setting:

security:   
   access_decision_manager:   
      strategy: priority   
      allow_if_all_abstain: true   

If I have allow_if_all_abstain set to false in my project, one of my voters will produce a false positive of sorts in the Symfony profiler. This is even when the voter's supports() returns false, or voteOnAttribute() returns true.

@soyuka
Copy link
Member

soyuka commented Nov 16, 2022

@FPDK thanks for that, do you think it could be added to the api platform documentation?

@FPDK
Copy link

FPDK commented Nov 25, 2022

@soyuka If I can find the time, I'll try to test it in a blank project first before I create a PR on the docs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants