You probably have to rebase, you have a bunch of commits in here that are not related to your PR

Yes sorry rebased it now onto fresh master, the patch was based on current 3.1 branch.

@iltar I've changed the PR and removed the custom exception. Instead of it, I added code that removes the token for the case when there is a BadCredentialsException in order to prevent a infinite loop.

Should / could this be optional and driven by a security.yml config that defaults to false?

Any update on this? It's an important issue as changing a password to log out any other session is a very common use case and should be the default behaviour on any website.
I think this PR got the two key points right. The only small issue is that the user will be loaded from DB twice but I guess it's acceptable given it's not the general use case.
The other solution would be not to trust an unauthenticated token from the AuthenticationProvider? What would be the purpose of this flag?
Thank you

So after some debugging of mentioned "incorrect" behavior (not being logged out on other sessions after changing the password) I came to the same conclusions: isAuthenticated is being correctly set to false after hasUserChanged notices changed password (it starts from ContextListener). But then AccessListener comes into play and reauthenticates the token.
This is clearly not the way it should work and not the way it is mentioned in the docs.

However, I have not yet tested the patch here.

Any update here?

I don't think this PR should be accepted, imo the flow is working as intended. The current behavior is to log the user out whenever anything changes. This is correct because the user is not the same anymore. If you want to divert from this behavior, implement the EquatableInterface and customize behavior.

@iltar have you seen my comment above? It should log out whenever anything (or, like, a password) changes but it does not.

imo the flow is working as intended. The current behavior is to log the user out whenever anything changes.

No, that is not the current behavior. But I would expect this, too.

See the comment of @eXtreme.

@eXtreme I've been digging in the security component quite often last weeks and it works exactly as I just described, with the difference that I have the EquatableInterface which returns false only when the ID changes. This behavior works for me. Perhaps you can reproduce it in a fork?

@iltar ok I will try later today or tomorrow. The app runs on 2.8 so I will use the same version.

@iltar have you followed the entire trail of this back to the original issue report? As others mentioned, this is not the current behaviour. #17023 - might be it also works for ID (can you share your fork), same as for enabled field, but doesn't for other properties.

Security is such an important aspect and shouldn't be ignored - imo this should be picked up by the core team as a high priority (it has been almost 2 years already)

@iltar you can use this branch: https://github.com/gharlan/symfony-demo/tree/issue-19033

Go to backend, login, click on the "CHANGE PASSWORD" link.

I don't have a proper environment to run or test stuff at the moment. Digging into the code, this part looks like the problem to me: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php#L70-L86
I also found that the refreshUser is never called upon, which I think is normal to use in the other authenticators.

This part in the authenticator, is what causes the problem imo. There's no distinction between the current and actual user, because it always fetches the current user from the current token:
https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php#L77-L80

What should happen imo, is a refreshUser() call at that position and return that one. As this behavior is working fine for the Guard implementations for example, I recommend the AbstractToken stays the way it is. If anything needs fixing, it's the Dao/User authenticators.

@iltar
my attempt is here:
https://github.com/eXtreme/symfony-demo/tree/issue-19033
eXtreme/symfony-demo@9f22a02
using a form and a controller etc :P but on 2.8 as I mentioned above, but I guess it's the same on latest branches

simple reproduce:

1. log in as the same user on different browsers
2. in one browser, go to http://localhost:8000/en/profile/change-password
3. change a password to a different one
4. refresh the page on the second browser

I have not managed to reproduce the same behavior I debugged in my real app. BUT. Changing the passwords sets authenticated to false on the second browser. Which... logically makes sense, well, you are not authenticated but still logged in (?). So checking IS_AUTHENTICATED_FULLY should fail right? You can guard your critical app parts by this check and prevent abuse, right? Nope. After adding

if (!$this->get('security.authorization_checker')->isGranted('IS_AUTHENTICATED_FULLY')) {
    throw $this->createAccessDeniedException();
}

to Admin\BlogController::indexAction and refreshing the page on the second browser.. you are being reauthenticated. The same thing happens using, for example @Security("has_role('ROLE_ADMIN') and is_granted('IS_AUTHENTICATED_FULLY')"). So with these checks for "IS_AUTHENTICATED_FULLY" and changing the password on the first browser and then refreshing the page on the second browser you are reauthenticated. I've put a breakpoint on AbstractToken::setAuthenticated and on the second browser there is a call to set it to false and then a call to set it true. The first one is called by ContextListener::refreshUser, the second one by UserAuthenticationProvider.

In my real app I mentioned in the comment above it was AccessListener which reset the token to authenticated. But maybe it is because I use JMS SecurityExtraBundle? Not sure.

There's a flag that might alter behavior of the the access decision manager, could that change this particular behavior?

@iltar I'm out of office so I will test my fork tomorrow.

@iltar consensus or unanimous, it does not change anything in this case.

@eXtreme I mean this line of code, by default alwaysAuthenticate is false, but I have a feeling this is set to true for you: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php#L60

@iltar it's symfony-demo, so it should be default false

@iltar I checked this. It is false, at least in my demo branch.

So the issue is really just within the DaoAuthenticator then. What if you would remove just those lines, would that fix the issue as well?

https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php#L77-L80

I see no reason to not always fetch the new data tbh.

@iltar still no luck.

btw. we can move to Slack, not to spam here

So after some fiddling around with @eXtreme on slack, we have found out the following:

• AuthenticationManagerInterface have to manually implement logic to verify if the token can be authenticated. Only Guard has done this right, because it splits the token in pre and post.

• When a token is deauthenticated because the user has changed, it will trigger a call in the AuthorizationChecker::isGranted() method. This will happily reauthenticate the user in the same request as it was just determined that the user had been changed.

        if ($this->alwaysAuthenticate || !$token->isAuthenticated()) {
            $this->tokenStorage->setToken($token = $this->authenticationManager->authenticate($token));
        }

• The UserAuthenticationProvider uses a UsernamePasswordToken, which is unable to detect whether or not the user has been deauthenticated when going outside of the scope of the setUser method.

Possible solution

In theory a check could be added to the ContextListener which triggers an authentication exception. There's a new exception for this in 2.8 but not in 2.7 (where this problem also exists). There's 1 flaw in the following patch, it doesn't show the users why they have been logged out.

diff --git a/src/Symfony/Component/Security/Http/Firewall/ContextListener.php b/src/Symfony/Component/Security/Http/Firewall/ContextListener.php
index b443fa1..42b950d 100644
--- a/src/Symfony/Component/Security/Http/Firewall/ContextListener.php
+++ b/src/Symfony/Component/Security/Http/Firewall/ContextListener.php
@@ -157,6 +157,12 @@ class ContextListener implements ListenerInterface
                 $refreshedUser = $provider->refreshUser($user);
                 $token->setUser($refreshedUser);

+                // tokens can be deauthenticated if the user has been changed.
+                if (!$token->isAuthenticated()) {
+                    $this->logger->debug('Token was deauthenticated after trying to refresh it.', array('username' => $refreshedUser->getUsername(), 'provider' => get_class($provider)));
+                    return null;
+                }
+
                 if (null !== $this->logger) {
                     $this->logger->debug('User was reloaded from a user provider.', array('username' => $refreshedUser->getUsername(), 'provider' => get_class($provider)));
                 }

This is how Guard solved this. The return null should probably become an exception or something in this patch. @weaverryan perhaps you can confirm this?

@iltar haha, yes, someone finally noticed this in Guard :). I noticed this problem when making Guard, and so I made sure to make Guard not suffer from this. But, it's a workaround in Guard for sure.

About your patch, initially, I like it, but I'd need to review this again in more detail - I can't remember all the moving pieces. In the Guard provider, I throw an instance of AccountStatusException because this triggers a "logout" in the ExceptionListener:

But in ContextListener, we should just be able to nullify the token in handle(). Perhaps:

        if ($token instanceof TokenInterface) {
            $token = $this->refreshUser($token);
        } elseif (null !== $token) {
            if (null !== $this->logger) {
                $this->logger->warning('Expected a security token from the session, got something else.', array('key' => $this->sessionKey, 'received' => $token));
            }
            $token = null;
        }

       // -> right HERE, check if the token is authenticated, and if not, $token = null.
       
        $this->tokenStorage->setToken($token);

Hopefully that helps... a bit :)

Hopefully someone on this PR can answer this clearly. From my investigations, refreshUser isn't being called, and so ANY field changing in a user entity, causes the session to be forced to log back in. But everywhere I'm reading, I'm seeing conflicting information about it should refresh the user, vs logout the user.
Is this PR fixing it so it correctly reloads the user when a change occurs, or is it also logging it out? I'm trying to use Guards, @weaverryan do I understand correctly that Guards force a logout?

So? Is anyone interested in picking this one up?

I would love if someone could open a PR for this. I don't think this PR is the correct way - it would only fix it for some parts of the system. I think the solution proposed by @iltar and I is a good approach to try: (#19033 (comment) and comment above).

Anyone want to open a PR?

@weaverryan I can try, but I'm a bit concerned about the change as it does 2 things:

• It changes behavior that might break the majority of apps, by logging out users (I can't predict this)
• It fixes a long standing bug

First point indicates 3.4, second point indicates 2.7, which one should I open it against?

I would definitely do this change in 3.4.

Closing in favor of #23882.