Creating the custom Voter

[colejarczyk]

This is a small thing. In Voter example $user object is an instance of Symfony\Component\Security\Core\User\UserInterface. Then in a switch statement there is a call to method getId() on $user, but there is no such method in UserInterface.

[weaverryan]

Hey @colejarczyk!

You're absolutely right, but it's done on purpose. The vote() method - which is an abstract method that comes from Symfony - has the UserInterface type-hint - so our concrete implementation must have that same type-hint. But in reality, in our application, we know what our concrete User instance is - e.g. It's AppBundle\Entity\Post, and we know what methods it has on it - like getId() and getOwner(). So, it's no problem.

But, is this unclear in the docs? Are are you just looking very closely at things? :)

Cheers!

[colejarczyk]

I found it by accident. I used this example to learn voters and thought that this maybe is not on purpose. If it is, then it's ok. It's very clear for me.

I can only suggest to change method from getId to getUsername to keep with UserInterface, but this is just only a suggestion.

[weaverryan]

Thanks @colejarczyk :). Actually, using getId() is on purpose - because you should feel save and secure using methods that are not on UserInterface because you know what concrete User class you have in your application. That would be totally different if you were creating a voter that would be shared between projects - but for your project, you have more information about what that User class actually is :).

Cheers!

[wouterj]

@weaverryan I'm not sure about this one. It is indeed true that $post->getAuthor()->getId() is a safe call, but $user->getId() isn't. We only know that $user is an instance of UserInterface.

[weaverryan]

@wouterj I disagree - we must assume that the User we're getting here is our concrete User class - else we can't perform any of our business logic. But for clarity, we could add a check at the top to make sure User is our User class and throw an Exception of it's not ("Hey programmer guy/gal, you I think you misconfigured something, because the logged in user is not our User instance somehow").

[xabbuh]

I think we should add that kind of error handling. Indeed, if you configured everything in your application correctly, this will never happen. But we all know how difficult the Security component can be (especially for newcomers) and tracking the cause of the error may become hard without a proper error message. :)

[colejarczyk]

Maybe something like this can solve this problem? What do you think?

// make sure there is a user object (i.e. that the user is logged in)
$userClassName = get_class($post->getOwner());
if (!$user instanceof $userClassName) {
    return VoterInterface::ACCESS_DENIED;
}

[wouterj]

I think we have to throw an exception and just make things simple like !$user instanceof User (where User is the User entity).

[xabbuh]

agreed with this simple approach

[weaverryan]

+1 - it adds clarity, which I like

if (!$user instanceof User) {
    throw new \LogicException('The user is somehow not our User class! This should never happen!');
}

[wouterj]

Btw, I think we have to be a bit carefull with "this should never happen". There are some people, @iltar for instance, who are using a different user object for the security than in their application. This adds a clear separation.

[weaverryan]

Ok, then just take off the second sentence:

if (!$user instanceof User) {
    throw new \LogicException('The user is somehow not our User class!');
}

[linaori]

I think it should respect the UserInterface, hence my implementation would be compatible. A side note, if getUsername() gets renamed to getIdentifier(), that could return a getId().

[weaverryan]

@iltar Why should someone make their business logic only rely on UserInterface? If we extend that argument, then when calling $this->getUser() in a controller, we should also only rely on the UserInterface methods.

[linaori]

That's one of the reason I don't like using the Symfony Controller. Regarding voters, it all depends on your implementation I suppose. In my case I use $token->getUsername() to retrieve the client identifier, which I then use to retrieve a Client object as my client is the one with permissions, not my session user (which in most cases happens to be an entity).

Most of this logic is all abstracted but we still have to find a decent way of composing voters. Currently we have created around 40 CAN_* roles, put in about 4 listeners. This is of course becoming a mess.

[weaverryan]

PR at #5317!

[harikt]

@weaverryan could you add a flag hasPR for easy lookups ?

Thank you.