Skip to content

[Security] AbstractVoter should not abstain when object == null #16556

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.

Sign up for GitHub

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

Jump to bottom
Closed
lyrixx opened this issue Nov 16, 2015 · 4 comments
Closed

[Security] AbstractVoter should not abstain when object == null #16556

lyrixx opened this issue Nov 16, 2015 · 4 comments
Labels
Security

Comments

@lyrixx
Copy link
Member

lyrixx commented Nov 16, 2015

I was migrating our voters to use the AbstractVoter and I notice something strange.
In 2.7 and in 2.8, if nothing is passed for the object argument, the voter votes ACCESS_ABSTAIN.

IMHO, this is wrong. For example, If a voter votes for POST_CREATE, I may have no $object parameters. and so the abstract voter should let the voter decide.

What do you think?

ping @inoryy @weaverryan @wouterj @Koc
@Koc
Copy link
Contributor

Koc commented Nov 16, 2015

I agree with you, but this logic was added when this voter was created d3bafc6#diff-c46e9d9da2a0b7aa68e78b8e58369481R61 and changing behavior can be a BC-break.

@lyrixx
Copy link
Member Author

lyrixx commented Nov 16, 2015

@Koc Yes, I saw this diff. But I don't understand why. And this Logic prevents me to use the AbstractVoter so for me it's a bug.

@inoryy
Copy link
Contributor

inoryy commented Nov 16, 2015

@lyrixx your use case makes sense and in fact when I looked up the original PR, I've found that somebody has already encountered this issue here, I'm not sure if any action was taken though /cc @weaverryan

@weaverryan
Copy link
Member

@lyrixx it was an oversight in the original AbstractVoter. So yes, it's a bug, but it needs to be fixed while keeping BC. I think there's another issue about this, where I suggested renaming this to AbstractObjectVoter and creating some other new abstract class with the desired functionality. So I think it's valid - someone should work on it for a future release.

@lyrixx lyrixx mentioned this issue Nov 19, 2015
Merged
nicolas-grekas added a commit that referenced this issue Nov 26, 2015
@nicolas-grekas
feature #16601 [Security] Deprecate "AbstractVoter" in favor of "Vote…
a58df11 
…r" (nicolas-grekas, lyrixx)

This PR was merged into the 2.8 branch.

Discussion
----------

[Security] Deprecate "AbstractVoter" in favor of "Voter"

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #16556, #16558, #16554
| License       | MIT
| Doc PR        | -

Commits
-------

fd8b87c [Security] Deprecate "AbstractVoter" in favor of "Voter"
d3c6d93 [Security] Revert changes made between 2.7 and 2.8-beta
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@weaverryan @Koc @inoryy @nicolas-grekas @lyrixx @xabbuh