Skip to content

[Security] Severe: When following the docs, each user can access each other user's data #11505

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 Apr 29, 2019 · 6 comments

Comments

@ThomasLandauer
Copy link
Contributor

ThomasLandauer commented Apr 29, 2019

I'm upset today, since I followed https://symfony.com/doc/current/security.html and ended up with a ridiculous "security" system in which every user can access everything (i.e. every other users' data).

The relevant information on how to do it right is hidden in the chapter https://symfony.com/doc/current/security.html#access-control-lists-acls-securing-individual-database-objects
This entire chapter is problematic for many reasons:

  • Under the heading "Access Control Lists" it is recommended to avoid Access Control Lists.
  • The second part of the heading ("Securing individual Database Objects") is outright wrong, since this has nothing to do with database objects at all.
  • "Imagine you are designing a blog..." sounds more like the beginning of a fairy tale than the beginning of one of the most important parts on the entire page.
  • The real important information is just not there. It's hidden under a tiny link to "Voters" (which is a term that sounds only relevant for organizers of a public election).

Usually I don't expand so much on the shortcomings of any part of the docs. I try to improve it instead. But I've been asked the question "Why do you want to change it?" too often. So I'm trying it the other way round today.

So what needs to be done on this page IMHO:

  • Get rid of this "role" stuff! For the majority of users, it would be better if they've never heard about it, cause (a) they won't need it, and (b) it's highly misleading. Why? The term ROLE_USER suggests that we are talking about individual users. But we aren't. Anything "restricted" to ROLE_USER is open to any user. Unanswered (unanswerable?) question: Why do those "pseudo-individual" users then need separate passwords at all, when (in the end) it just doesn't make any difference??
  • The voters stuff should become a top-level heading and go up higher on the page, at least up to "Denying Access, Roles and other Authorization", maybe even up to 'The "User Provider"'.
  • In that voters chapter the easiest way to do it should be shown right away:
    if ($user->getId() !== $this->getUser()->getId()) {
        throw new AccessDeniedException();
    }
    ... and only refered to https://symfony.com/doc/current/security/voters.html for more complex situations.

I'd be willing to re-write it in principle, but I already have 5 PR's sitting around without much progress: https://github.com/symfony/symfony-docs/pulls/ThomasLandauer

@javiereguiluz
Copy link
Member

Thomas, I don't care if you are upset or had a bad day. The tone and wording of your issue is not acceptable. Please, reword it to avoid all the aggressive and insulting parts and then we'll consider it. Symfony Doc maintainers and contributor's deserve some respect and you should know better because you are also a contributor. Thanks.

@javiereguiluz
Copy link
Member

Thank you for rewording your original issue description. Now we'll consider the issue and we'll come with a reply to your questions/comments.

@xabbuh
Copy link
Member

xabbuh commented Apr 30, 2019

I do not agree on dropping the part about roles. We can of course discuss whether voters are as important as roles, but from my experience there are many real applications where you do never need to protect objects individually, but there are just certain groups of people that share the same level of permissions and for which roles are the solution that fits best. But we should of course proofread the chapter to see if it is clear here that roles are not the solution to protect individual objects.

I would still keep the voters in their own article, but we should maybe make it more clear in the section about roles that there is another tool to protect individual objects to make its discovery a bit easier.

@ThomasLandauer
Copy link
Contributor Author

Loosely related: #13406

@ThomasLandauer
Copy link
Contributor Author

Zero progress after almost a year.

@xabbuh: I didn't mean to drop roles completely. It's enough if you <sarcasm> hide it under a tiny link with a misleading title </sarcasm> - just like "Voters" ;-)

OK, now seriously: If roles are "advertised" so much, this question should be answered: What's the advantage of having different users at all, when ultimately they all have the same permissions?

@wouterj
Copy link
Member

wouterj commented Apr 11, 2020

Roles and permissions, as discussed in this issue, are 2 very different topics. See also https://wouterj.nl/2020/01/grant-on-permissions-not-roles

I do agree that voters should get a more prominent place in the main Security guide than it has now.
I however don't agree that this results in severely insecure applications. We don't claim anywhere that doing $this->denyAccessUnlessGranted("ROLE_USER") means the user can access only their own stuff. And the voters guide does imho show a good example to make sure users can only view their own stuff.

wouterj added a commit that referenced this issue Apr 16, 2020
This PR was merged into the 4.4 branch.

Discussion
----------

Renaming "Securing Individual Objects"

Reason: I never understood what "objects" referred to in "Securing Individual Objects". Renaming it to "Individual User Permissions" is another step forward at solving the "discoverability" problem I mentioned in #11505

However, I would still move it higher up on the page (above Roles), since I see it like this: Having *Individual* users is basic for any security system; grouping those users into something called Roles is advanced.

Second, since Voters are described on a separate page, I would also defer Roles to another page, off the main security page.

Commits
-------

bce240d Renaming "Securing Individual Objects"
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

No branches or pull requests

4 participants