[Security] Fix context listener misbehaviour #20401

Einenlum · 2016-11-03T14:28:35Z

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

The implementation of ContextListener is not logical.

Here is the actual difference between the ContextListener and the ChainUserProvider.
The ContextListener will only loop if the provider throws an UnsupportedUserException:

foreach ($this->userProviders as $provider) {
    try {
        // …
        return $token;
    } catch (UnsupportedUserException $e) {
        // let's try the next user provider
    } catch (UsernameNotFoundException $e) {
        if (null !== $this->logger) {
            $this->logger->warning('Username could not be found in the selected user provider.', array('username' => $e->getUsername(), 'provider' => get_class($provider)));
        }

        return;
    }
}

The ChainUserProvider on the other hand, will try to refresh the user with every provider even if the user was not found :

foreach ($this->providers as $provider) {
    try {
        return $provider->refreshUser($user);
    } catch (UnsupportedUserException $e) {
        // try next one
    } catch (UsernameNotFoundException $e) {
        $supportedUserFound = true;
        // try next one
    }
}

This is not only undocumented, it is also very disturbing. This leads to strange behaviours: If you had not set a chain provider, the login is successful but right after the redirection you're anonymous again without form error.

We really need to fix this incoherent behaviour. ChainProvider should be necessary only if you want to use multiple providers for a specific firewall.

aguidis · 2016-11-03T14:40:43Z

👍

Nek- · 2016-11-03T15:08:01Z

src/Symfony/Component/Security/Http/Firewall/ContextListener.php

@@ -166,13 +166,13 @@ protected function refreshUser(TokenInterface $token)

                return $token;
            } catch (UnsupportedUserException $e) {
-                // let's try the next user provider
+                continue;


why drop the comment (which is accurate) and add a continue (which is useless) ?

@Nek- I could also let it as it. But continue seems more appropriate to me (for now we just want to continue the loop and not go further). But just a matter of taste.

xabbuh · 2016-11-05T11:04:50Z

Imo the context listener should rather not try to use all user providers to refresh the user, but should only use the user provider that was configured for the active firewall.

Einenlum · 2016-11-06T09:20:44Z

@xabbuh Agreed. But the question still remain when there is no provider configured for the firewall, doesn't it?

xabbuh · 2016-11-06T09:38:04Z

@Einenlum It should then use the first configured user provider like it's done elsewhere.

slaci · 2016-12-09T15:22:37Z

This is very optimistic solution :P If you have 3 providers and you need the 3th, then the first two will query, fetch or something their backends on every page load. For example 3 query per page load just to refresh the user... and can be worse if you have rest api providers or anything else slow.

And the continue doesn't do anything there, if you just use that exception (UnsupportedException) then you are doing the same thing :)

Checking the firewall's provider is tricky too, because:

It seems very hard to check which firewall are we in. In 3.2 there is a new FirewallConfig or FirewallContext class which may help, dont know.
The firewall may have a provider key, but one firewall may have more than one authentication method which can overwrite that provider key. For example form_login and basic may have distinct providers in the same firewall, if someone like suffering.

So you don't really know, which method (provider) the user came from.

The only solution would be to save the provider in the token or user, but as fabpot said in the connected issue, it was there once, and they removed it for unkown reasons. Changing the tokeninterface would be big bc break, so no much hope :P

nicolas-grekas · 2016-12-27T14:28:04Z

Note that BC breaks are not allowed, so you'll need another approach if you need this in the core.

xabbuh · 2017-02-27T21:26:03Z

closing in favour of #21791

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

xabbuh · 2017-03-04T12:39:13Z

@Einenlum Can you check if #21865 fixes your issue?

Fix context listener misbehaviour

1ca8c75

carsonbot added Status: Needs Review Bug BC Break labels Nov 3, 2016

Einenlum changed the title ~~Fix context listener misbehaviour~~ [Security] Fix context listener misbehaviour Nov 3, 2016

Nek- suggested changes Nov 3, 2016

View reviewed changes

nicolas-grekas added this to the 3.x milestone Dec 27, 2016

xabbuh mentioned this pull request Feb 27, 2017

[SecurityBundle] only pass relevant user provider #21791

Merged

xabbuh closed this Feb 27, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Fix context listener misbehaviour #20401

[Security] Fix context listener misbehaviour #20401

Einenlum commented Nov 3, 2016 •

edited

Loading

aguidis commented Nov 3, 2016

Nek- Nov 3, 2016

Einenlum Nov 3, 2016 •

edited

Loading

xabbuh commented Nov 5, 2016 •

edited

Loading

Einenlum commented Nov 6, 2016 •

edited

Loading

xabbuh commented Nov 6, 2016

slaci commented Dec 9, 2016

nicolas-grekas commented Dec 27, 2016

xabbuh commented Feb 27, 2017

xabbuh commented Mar 4, 2017

[Security] Fix context listener misbehaviour #20401

[Security] Fix context listener misbehaviour #20401

Conversation

Einenlum commented Nov 3, 2016 • edited Loading

aguidis commented Nov 3, 2016

Nek- Nov 3, 2016

Choose a reason for hiding this comment

Einenlum Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

xabbuh commented Nov 5, 2016 • edited Loading

Einenlum commented Nov 6, 2016 • edited Loading

xabbuh commented Nov 6, 2016

slaci commented Dec 9, 2016

nicolas-grekas commented Dec 27, 2016

xabbuh commented Feb 27, 2017

xabbuh commented Mar 4, 2017

Einenlum commented Nov 3, 2016 •

edited

Loading

Einenlum Nov 3, 2016 •

edited

Loading

xabbuh commented Nov 5, 2016 •

edited

Loading

Einenlum commented Nov 6, 2016 •

edited

Loading