Skip to content

[Security] Voters: What's the advantage over doing it in the controller? #13406

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

Closed
ThomasLandauer opened this issue Mar 23, 2020 · 12 comments
Closed
Labels

Comments

@ThomasLandauer
Copy link
Contributor

The entire example used throughout https://symfony.com/doc/4.4/security/voters.html (namely checking if a certain user is allowed to edit this specific post) can be achieved with 2 lines in the "edit"-Controller:

if ($post->getOwner() !== $this->getUser()) {
    throw new AccessDeniedException();
}

So what's the advantage of setting up this complicated voting system?? This should be explained on the page.

@xabbuh
Copy link
Member

xabbuh commented Mar 24, 2020

Looking at the lines of code used in the voter I would have said that it should be obvious to spot while you rather not want to place such a check in each individual controller. Do you think it would makes it more clear if there also was a sentence explaining that voter centralise the logic so that there is only one place to update when requirements change?

@ThomasLandauer
Copy link
Contributor Author

In some cases (like in the "Post" example), there is only one controller/route to protect. So you will have to change it in just one place anyway. So the question is not about "centralizing" it, but about "outsourcing" it.

So the question is: In which situations is one approach smarter than the other? What are the relevant questions you have to ask yourself?

@xabbuh
Copy link
Member

xabbuh commented Mar 25, 2020

From my experience I would never inline such a check in controllers. Using voters always has the advantage of having a few places where you control your access control rules which allows you to quite easily spot how permissions are distributed. Furthermore, you have a central place where you can adjust checks without risking to forget a place.

@dbrumann
Copy link
Contributor

How does this relate to the docs? Are you proposing a discussion section on when to use/not use features? I can't think of any other part in the docs where we discuss when/how components are "justified".

@ThomasLandauer
Copy link
Contributor Author

Well, take look at e.g. at https://symfony.com/doc/current/forms.html#creating-forms-in-controllers

"Creating Forms in Controllers" is explained first, "Creating Form Classes" as alternative. Plus:

Symfony recommends to put as little logic as possible in controllers. That's why it's better to move complex forms to dedicated classes instead of defining them in controller actions. Besides, forms defined in classes can be reused in multiple actions and services.

This is a clear recommendation, along with a reason (=reuseability). So why not having some pros and cons for alternative approaches in the Security docs too?

@ThomasLandauer
Copy link
Contributor Author

I created a (somehow) related gist: https://gist.github.com/ThomasLandauer/668d7353dc5794da62be4cec9e8091ab

@ThomasLandauer
Copy link
Contributor Author

Just for the records: What I had in mind, is not explained in today's PR ;-)

@wouterj
Copy link
Member

wouterj commented Apr 16, 2020

I think it's best to always do these things in a voter. The big difference between the form and security examples is that forms are usable in controllers, the Security Authorization system isn't and would involve writing your own logic (your example doesn't use anything from Symfony Security).

In #13522 I made Voters the more clear go-to abstraction for non-role based permissions. I think that's good enough.

@wouterj
Copy link
Member

wouterj commented Apr 16, 2020

For the record: There are always 101 ways to solve a problem in software. What way is best depends completely on the type of application, the experience/size of the dev team, and many more parameters. So if the if-statement works and is the best in your case, feel free to go with it! 😃

@ThomasLandauer
Copy link
Contributor Author

Well, I appreciate there's at least some progress here :-) And your PR is certainly a step in the right direction! See also #13553

My example above uses $this->getUser() from Security. And all I'm asking for is a short note (1-2 sentences) saying: If you have just 1-2 controllers, it's ok to do it like this: ... But if you want to do it more professional, see here: ...

If you agree in principle, I'll look for an appropriate place and come up with a PR. Just don't want to waste my time, if you won't merge it ;-)

@wouterj
Copy link
Member

wouterj commented Apr 16, 2020

My example above uses $this->getUser() from Security. And all I'm asking for is a short note (1-2 sentences) saying: If you have just 1-2 controllers, it's ok to do it like this: ... But if you want to do it more professional, see here: ...

Ah, then I misunderstood. I thought you wanted to first discuss doing things without using isGranted() and then introducing that method.
I think I would agree with a little tip saying "Hey, if your permission is simple and does not need to be reused, feel free to do a small if statement and throw the correct exception.". I think https://symfony.com/doc/current/security/voters.html#how-symfony-uses-voters is probably the best place, as it's just before we explain the Voters, but just after we introduced "security permissions".

@ThomasLandauer
Copy link
Contributor Author

See #13554

xabbuh added a commit that referenced this issue Aug 3, 2020
…auer, wouterj)

This PR was merged into the 3.4 branch.

Discussion
----------

Explaining controllers as viable alternative

See #13406 (comment)

Please wrap it in a "caution" box, or whatever you think is appropriate.

I'm unsure about how much context of the controller should be shown in the code, since the entire controller is only shown later. If you think it's too confusing now, I'd suggest to show the controller as "skeleton" above this note.

Commits
-------

15c4adc Update voters.rst
bb0872a Update voters.rst
577dfa2 Update voters.rst
fdfd1ad Update security/voters.rst
221bbb1 Update voters.rst
164aec4 [#13554] Slightly reworded the tip
2bc5c54 Update voters.rst
40bbd81 Explaining controllers as viable alternative
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants