8000 Clarify VoterInterface with multiple attributes by BenMorel · Pull Request #32956 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Clarify VoterInterface with multiple attributes #32956

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

Closed
wants to merge 1 commit into from
Closed

Clarify VoterInterface with multiple attributes #32956

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ interface VoterInterface
* This method must return one of the following constants:
* ACCESS_GRANTED, ACCESS_DENIED, or ACCESS_ABSTAIN.
*
* If more than one attribute is passed, the voter must grant access if access is granted to any of
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is not a hard requirement for any voter and it's totally fine to implement a custom voter that behaves differently. I would suggest that we move this comment into the docblock of the Voter class instead.

Copy link
Contributor Author
@BenMorel BenMorel Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would be very careful with this approach, as it might bring a lot of confusion.

There clearly are conflicting views here. Here's how the Twig doc for is_granted() evolved in the last few months:

  • on 9 Oct 2018, Improve is_granted documentation symfony-docs#10461 by @Jean85 changed the wording to:

    Returns true if the current user has the required role, if only one is passed; if more than one is passed, with an array, the behavior depends on how the Access Decision Manager is configured; by default, the user has to have at least one of the passed roles.

  • changed the same day by @javiereguiluz to:

    Returns true if the current user has the given role. If several roles are
    passed in an array, returns true if the user has all of them or at least one
    of them, depending on the value of this option:
    security.access_decision_manager.strategy.

  • on 25 Oct 2018, following this comment on #10461 by @xabbuh:

    The access decision strategy defines how the result looks like when multiple voters voted on a certain attribute. It does not affect the internal behaviour of the RoleVoter.

    fix RoleVoter behaviour description symfony-docs#10580 by @xabbuh changed it to:

    Returns true if the current user has the given role. If several roles are passed in an array, true is returned if the user has at least one of them.

  • on 18 July 2019, [Security] Better explain what happens when multiple roles are passed symfony-docs#11988 by @javiereguiluz changed it back to:

    Returns true if the current user has the given role. If several roles are passed in an array, true is returned if the user has at least one of them or all of them, depending on the Access Decision Manager strategy.

As you can see, on the assumption that Twig's is_granted() follows the same rules as VoterInterface::vote(), there is no current consensus on how is_granted() should behave, and worse, there are regressions in the docs, ignoring relevant past comments:

  • @Jean85 and @javiereguiluz think that the behaviour is influenced by the Access Decision Manager strategy, although @xabbuh proved that it's not; @Jean85 admitted that this was a mistake.

  • regardless of the point above, @xabbuh now thinks that it's fine for voters to behave as they wish on this point, upvoted by @ro0NL

  • the current Twig doc is clearly in infringement with the source code anyway, as it says that:

    true is returned if the user has at least one of them or all of them, depending on the Access Decision Manager strategy

    which directly contradicts @xabbuh's argument:

    the access decision strategy defines how the result looks like when multiple voters voted on a certain attribute (...) not the internal behaviour of the RoleVoter.

  • and the Voter class is opinionated about what should happen:

    grant access as soon as at least one attribute returns a positive response

IMHO it was an unfortunate design decision to allow multiple values in the first place (forcing isGranted('X') || isGranted('Y') or isGranted('X') && isGranted('Y') would have probably been much less confusing), now one will always wonder what the behaviour is should they pass multiple values, and going to the docs or the source code will only make things more confusing, until they're all in sync on this point.

Now, to make things right without breaking BC, I would recommend to apply the "less bad" fix:

  • choose one, and only one, appropriate, documented behaviour here (most likely the at least one of them strategy)
  • update all docs (Symfony Security component, Twig, source code) to reflect this fact
  • maybe find a wa 5E8B y to ensure that another PR a few months down the road won't change this again (how?)

* the given attributes.
*
* @param TokenInterface $token A TokenInterface instance
* @param mixed $subject The subject to secure
* @param array $attributes An array of attributes associated with the method being invoked
Expand Down
0