Check provider supports user class #29653
Conversation
|
You may want to switch back to \get_class(). I used a variable under the impression that calling that method repeatedly is SLIGHTLY less performant since it's done several times throughout that method. |
|
This pull request is meant to be an example solution for #29652. However, I believe it is good enough to be merged if you agree that it is a satisfactory solution. |
|
@chalasr - The appveyor build failed because of an issue with PHP unit unrelated to this pull request. |
But this is documented to be the expected implementation; Not sure what we're gaining here 🤔 |
|
@ro0NL - What brought me to this point was unexpected behavior. Even if documented, it's not clear. Here was my scenario: I created a firewall with a user provider and everything was great for a year and a half. Being stateless, the rest api firewall didn't need the refreshUser method so I didn't bother doing anything inside but returning user (without noticing how my original class was built or looking at the comment you pointed out - my mistake there obviously). I was having a problem with my main firewall after that point because I assumed it would only use the relevant user provider, but even though my API firewall only applied to a specific host/pattern, the user provider for that firewall was still being tested. If it had run the supports method (like this pull request changes), it wouldn't have done that even with my initial mistake. Frankly, I don't know why there is a supports method if it's not being utilized for this purpose... Ideally, this is just one step in the right direction. I think it should only loop through user providers for the current applicable firewalls. Since we define a user provider per firewall, I think that would be the natural way for it to work. Otherwise why even define a user provider on the firewall if it's just going to try to use them all anyway? Furthermore, in the case of a stateless firewall, requiring the refreshUser method via the interface seems strange. |
|
Im not sure we want to undo
The thing im skeptical about is now it looks like you support that user, but you have to rely on supportsClass() to be called first. Im not sure we want to go that way as a new feature. |
|
I'd appreciate it if you and a couple other contributors could discuss the three things I mentioned. I'd be interested to see what you guys come up with, but I think there is room for improvement. Thanks for listening and looking into this for me! |
|
It appears |
| @@ -164,11 +164,21 @@ protected function refreshUser(TokenInterface $token) | |||
| $userDeauthenticated = false; | |||
|
|
|||
| foreach ($this->userProviders as $provider) { | |||
|
|
|||
| $providerClass = \get_class($provider); | |||
There was a problem hiding this comment.
These changes should be reverted.
| } | ||
|
|
||
| try { | ||
| $userClass = \get_class($user); | ||
| if( !$provider->supportsClass($userClass) ) |
There was a problem hiding this comment.
I would move this outside the try ... catch block. IMO there is no need to use an exception for this. We can instead just continue:
foreach ($this->userProviders as $provider) {
if (!$provider->supportsClass(\get_class($user)) {
continue;
}
// ...
}| @@ -4,6 +4,7 @@ CHANGELOG | |||
| 4.2.0 | |||
| ----- | |||
|
|
|||
| * added a `supportsClass()` check prior to `refreshUser()` for each user provider | |||
There was a problem hiding this comment.
Should be reverted, if this is a bugfix it should be based off the 3.4 branch. And for bugfixes we do not add entries to the components changelog files. If this is a new feature, it will be part of 4.3.
|
rebase needed + comments to address Status: needs work |
|
I'm 👎 on this PR, as it is implicitly adding overhead to every call which is in most environments not needed. The reason the check is not done by default is that it doesn't matter if your configuration is correct. I don't think we should make all Symfony production environments slower to be able to debug a configuration issue that will never get past QA a bit easier. |
|
Closing as there is no more activity here, feel free to reopen/take over. Thanks |
This feature checks that a user is compatible with a user provider prior to running refreshUser(). It eliminates the need for this logic inside the refreshUser method.