Skip to content
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

[Security] context listener: hardening user provider handling #21865

Merged
merged 1 commit into from Mar 6, 2017

Conversation

Projects
None yet
6 participants
@xabbuh
Copy link
Member

commented Mar 4, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #4498
License MIT
Doc PR

After the wrong fix in #21791 this is the second attempt to solve #4498. If more than one user provider support the user for the current context, all of them will be applied instead of returning prematurely when the first user provider does not find the logged in user.

@xabbuh xabbuh force-pushed the xabbuh:issue-4498 branch from 8d224e2 to 0fb0929 Mar 4, 2017

@slaci

This comment has been minimized.

Copy link

commented Mar 4, 2017

hi

With this patch this scenario will work out of the box. But to be honest I would say its just a quick hack, which is doable from the other way around (this patch is better than the other way).

The main problem is that, if you have eg 3 providers and you need a user from the last, then every user will have 3 queries for every page load (say we are using fos user bundle, and just query from the db). Because the provider will need to load the user from the db (or api or from hell) first to check if its supported. Then throw the exception, and the next provider will do the same, stacking queries.

So with this patch this feature will work, but won't be optimal cause this :(

I liked the other approach more, where I could define a provider to use on refresh (I know that wont work, cause bc break, so just saying :P).

Im sorry but I think the only way to solve it:

  • In the docs mention it is recommended to use only 1 userprovider per user classes. Just to remember, this whole thing is problem when people has more user providers for the same user class (type or something column in the same user table). If people use distinct user classes then everything works. OR
  • Store the used provider somewhere on login (well only session could work I think which is = token here), so no magic needed. The token interface could not be changed for obvious reasons, just a new interface could be introduced which would have the once removed getProvider method and tell people to use that if they want this scenario to work. But I guess that would be more like a new feature than bugfix, so you could not merge it into 2.7 (well just guessing).
  • EDIT: Or maybe use token attributes.. I don't know about those. Does not seem a clean solution, but the provider with a special key may be serialized there. Big hack, but possibility :)

If these points are not possible, I support the merge of this patch. At least people will not have errors, just a bit degraded performance sometimes that they may not even notice or could live with that.

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2017

In the docs mention it is recommended to use only 1 userprovider per user classes. Just to remember, this whole thing is problem when people has more user providers for the same user class (type or something column in the same user table). If people use distinct user classes then everything works.

Would you like to open an issue or even better create a pull request to improve the docs?

  • Store the used provider somewhere on login (well only session could work I think which is = token here), so no magic needed. The token interface could not be changed for obvious reasons, just a new interface could be introduced which would have the once removed getProvider method and tell people to use that if they want this scenario to work. But I guess that would be more like a new feature than bugfix, so you could not merge it into 2.7 (well just guessing).

  • EDIT: Or maybe use token attributes.. I don't know about those. Does not seem a clean solution, but the provider with a special key may be serialized there. Big hack, but possibility :)

That would indeed be better solutions, but will need to be done on master as it would be a new feature IMO.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Mar 6, 2017

@nicolas-grekas
Copy link
Member

left a comment

👍

@javiereguiluz
Copy link
Member

left a comment

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit 0fb0929 into symfony:2.7 Mar 6, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 6, 2017

bug #21865 [Security] context listener: hardening user provider handl…
…ing (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[Security] context listener: hardening user provider handling

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #4498
| License       | MIT
| Doc PR        |

After the wrong fix in #21791 this is the second attempt to solve #4498. If more than one user provider support the user for the current context, all of them will be applied instead of returning prematurely when the first user provider does not find the logged in user.

Commits
-------

0fb0929 context listener: hardening user provider handling

@xabbuh xabbuh deleted the xabbuh:issue-4498 branch Mar 6, 2017

@slaci

This comment has been minimized.

Copy link

commented Mar 8, 2017

You should define distinct user providers for all your different user classes! If you do otherwise, then the security system may query all supported user providers to find the right one when refreshing the user from the session, which occurs on every page load. This may lead to degraded performance, when you have multiple user providers for the same user!

@xabbuh I could imagine something like the above, into a warning block with Be aware or something start, in: http://symfony.com/doc/current/security/custom_provider.html or http://symfony.com/doc/current/security/multiple_user_providers.html or somewhere.
My english is bad, so I wouldn't pollute your docs :D I tried to keep it short... so I didn't try to put in the real world example (the type column in the user class), cause that would need a bit longer explanation. Or mention it is a limitation of the security system... thats not good to advertise :D

Just to note, this is a limitation in all versions, not just 2.7.

@xabbuh

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2017

@slaci Can you just open a new issue with your suggestion? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.