[SecurityBundle] only pass relevant user provider #21791
Conversation
|
Being able to pass multiple user providers to the context listener doesn't make much sense. I suggest to deprecate this possibility entirely (see #21792). |
|
Thank you @xabbuh. |
This PR was merged into the 2.7 branch. Discussion ---------- [SecurityBundle] only pass relevant user provider | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #4498, #12465, #20401, #21737 | License | MIT | Doc PR | There is no need for the context listener to be aware of all the configured user providers. It must only use the provider for the current firewall (the one identified by the context key passed to the constructor) to refresh the user from the session. Commits ------- d97e07f [SecurityBundle] only pass relevant user provider
|
Just a note, regarding my comment on a related issue: If people follow this link: http://symfony.com/doc/current/security/multiple_user_providers.html Here http_basic has a different provider than the default one. In this PR you pass the default provider to the context listener, which is And if you have say 2 providers and you just put in 1 under eg basic_auth and dont set the default (under the firewall) then the listener will use the first registered provider (which may be different). These could not be fixed without bc break I think, just wanted to noted these. But these will cause surprise for some people I guess. |
|
@slaci The variable name is misleading. In fact, the |
|
Nope. I tried it with latest symfony 2.7 (2.7.24) and downloaded your file to the right location and it works as I described. Here is what I did: https://gist.github.com/slaci/f7cb3ee6f0d9144db6d16e20dee0ab83 So here, the root page is in the Symfony cannot get the default provider from http_basic, because there may be this config: main:
http_basic:
provider: first
form_login:
provider: secondNow tell me, which one I used to login (which one should be the default)? This is why I miss the provider from the token. This PR may bring a little bc break cause this. If someone used multiple providers and didnt define the default for the firewalls, then there will be surprises I think :) Anyway I like the PR, works better than before, just a bit different and some suprise factors that you should be aware :) But ofc I could be wrong. |
|
Ah, I misunderstood what you meant. In the meantime, @stof pointed me to the fact that the provider cannot only be configured per firewall, but also per authenticator (see #21792 (comment)). And that's the same with your example. I think this really is an issue and we should fix it as soon as possible or revert my PR and reopen #4498. |
|
will be reverted by #21798 |
…r provider (xabbuh)" (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)" | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21791 | License | MIT | Doc PR | #21791 was a mistake as pointed out by @slaci (see #21791 (comment)) and @stof (see #21792 (comment)). Commits ------- f6637dd Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)"
…ders in context listener (xabbuh)" (xabbuh) This PR was merged into the 3.3-dev branch. Discussion ---------- Revert "feature #21792 [Security] deprecate multiple providers in context listener (xabbuh)" | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21791 | License | MIT | Doc PR | #21792 was a mistake as pointed out by @slaci (see #21791 (comment)) and @stof (see #21792 (comment)). Commits ------- 3cfa0c7 Revert "feature #21792 [Security] deprecate multiple providers in context listener (xabbuh)"
* 2.7: Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)" [DependencyInjection] inline conditional statements.
* 2.8: Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)" [DependencyInjection] inline conditional statements.
* 3.2: Revert "bug #21791 [SecurityBundle] only pass relevant user provider (xabbuh)" [DependencyInjection] inline conditional statements.
…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
There is no need for the context listener to be aware of all the configured user providers. It must only use the provider for the current firewall (the one identified by the context key passed to the constructor) to refresh the user from the session.