What's the alternative right now for rehashing? Is there any?

AUTHENTICATION_SUCCESS_SENSITIVE

makes sense, the current name is too specific to one use case, isn't it?
AUTHENTICATION_SUCCESS_WITH_CREDENTIALS?

👍 For adding this event. I agree with @nicolas-grekas that the name is a bit specific atm. I like AUTHENTICATION_SUCCESS_SENSITIVE the most from the suggestions (passwords still are credentials while hashed imo, so I think the second suggestion doesn't really warn the user that's quite sensitive data).

"What's the alternative right now for rehashing? Is there any?" -@nicolas-grekas

I've seen implementations handle it inside the password encoder, but this requires injection an entity manager or other persistence layer inside the password encoder implementation, which is a deal breaker for me. IMHO, the password encoder should not be in the business of persisting anything.

"The name is a bit specific atm. AUTHENTICATION_SUCCESS_SENSITIVE?" -@wouterj
"Current name is too specific. AUTHENTICATION_SUCCESS_WITH_CREDENTIALS?" -@nicolas-grekas

I prefer AUTHENTICATION_SUCCESS_SENSITIVE and that is what I've gone ahead and edited this PR to implement. If there is a strong preference otherwise, please advise. I've also included unit tests now. Thoughts on this implementation? Any changes I should make?

Otherwise, I think this is pretty much set, pending any additional reviews.

This will be a bit difficult to do for guard. A guard authenticator has getCredentials in the PreAuthenticatedGuardToken, which usually returns an array of which 1 key is password (average use-case). In my case, I have 3 authenticators on a single firewall. 2/3 use password, the 3rd doesn't have a password (one time token). They are on the same firewall, same firewall context and use the same PreAuthenticatedGuardToken. The only difference is that I have no password key in the 3rd case. I know I can make a bunch of if-statements in my listener, but it feels nasty.

Also in your code I noticed that you use $result instead of $token, which means with guard, you can never get the credentials. Here's a scenario where I have a two factor code (scheb/two-factor-bundle). If it wasn't a two factor token, it would've been a PostAuthenticatedGuardToken directly:

"I know I can make a bunch of if statements in my listener, but it feels nasty." -@iltar

Effectively, whether the control statement is in your own listener or within a symfony-core implementation, such logic does need to exist somewhere, right? What are you suggesting as an alternative?

"Also in your code I noticed that you use $result instead of $token." -@iltar

You're referring to diff-70fc43e7af23f097f94d4a0036888896R97, correct? My usage of $result instead of $token was taken directly from the implementation of AUTHENTICATION_SUCCESS. I thought that the authenticate() method on a provider returned a token? You're saying for guard auth providers they do not? Why is that?

Effectively, whether the control statement is in your own listener or within a symfony-core implementation, such logic does need to exist somewhere, right? What are you suggesting as an alternative?

I'm not exactly sure if this can be solved in an easy fashion. I just felt it was worthy of mentioning, either for the docs or perhaps to inspire an idea.

You're referring to diff-70fc43e7af23f097f94d4a0036888896R97, correct?

Correct.

I thought that the authenticate() method on a provider returned a token? You're saying for guard auth providers they do not? Why is that?

They do return a token, this token just doesn't have the password in it.

In your change, you're using the authenticated token, not the token that contains the information to authenticated. The even you've borrowed the code from, uses the authenticated token as it would not make sense to use the raw data.

edit:

Thanks for the clarifications @iltar. Keeping some of the concerns you mentioned earlier at the forefront of my mind, perhaps it would make sense to create a more feature-rich AuthenticationSensitiveEvent class implementation that offloads some of the more complex tasks away from the user.

Here is an example draft of one such event implementation, where some of the most common operations for this new event (such as password rehashing) should be relatively easy to implement thanks to the expanded context provided to the constructor and the additional helper methods exposed on it:

<?php

/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */

namespace Symfony\Component\Security\Core\Event;

use Symfony\Component\EventDispatcher\Event;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

/**
 * This is an authentication event that includes sensitive data.
 *
 * @author Rob Frawley 2nd <rmf@src.run>
 */
class AuthenticationSensitiveEvent extends Event
{
    private $finalToken;
    private $priorToken;
    private $authenticationProvider;

    public function __construct(TokenInterface $finalToken, TokenInterface $priorToken, string $authenticationProvider = null)
    {
        $this->priorToken = $priorToken;
        $this->finalToken = $finalToken;
        $this->authenticationProvider = $authenticationProvider;
    }

    public function getAuthenticationToken(): TokenInterface
    {
        return $this->getFinalToken();
    }

    public function getPriorToken(): TokenInterface
    {
        return $this->priorToken;
    }

    public function getFinalToken(): TokenInterface
    {
        return $this->finalToken;
    }

    public function isTokenAuthenticated(bool $considerPriorToken = false): bool
    {
        return $this->finalToken->isAuthenticated() || ($considerPriorToken && $this->priorToken->isAuthenticated());
    }

    public function getAuthenticationProvider(): ?string
    {
        return $this->authenticationProvider;
    }

    public function isAuthenticationProvider(string $provider): bool
    {
        return $this->authenticationProvider === $provider;
    }

    public function getTokenPasswordCredentials(\Closure $extractor = null, bool $considerRequestedToken = true)
    {
        return $this->normalizeTokenPasswordCredentials($this->finalToken, $extractor) ?: (
            true === $considerRequestedToken
                ? $this->normalizeTokenPasswordCredentials($this->priorToken, $extractor)
                : null
        );
    }

    private function normalizeTokenPasswordCredentials(TokenInterface $token, \Closure $extractor = null): ?string
    {
        return ($extractor ?? function (TokenInterface $token): ?string {
            return $this->extractTokenPasswordCredentials($token);
        })($token, $this);
    }

    private function extractTokenPasswordCredentials(TokenInterface $token): ?string
    {
        $cred = $token->getCredentials();

        if (is_array($cred) && null !== $pass = $this->searchTokenCredentialsArrayForPasswordElement($cred)) {
            return $pass;
        }

        return $cred ?: null;
    }

    private function searchTokenCredentialsArrayForPasswordElement(array $credentials): ?string
    {
        foreach (['password', 'secret', 'api_key'] as $index) {
            if (isset($credentials[$index]) && false === empty($credentials[$index])) {
                return $credentials[$index];
            }
        }

        return null;
    }
}

I believe most of the above functionsality is generic enough for countless purposes while still providing meaningful impact on lessoning the burden on developers looking to handle common tasks like password rehashing. Given, the above is a rough draft and I'm just looking for feedback ATM, but this does work relatively well for the original goal of this PR: to streamline password rehashing and expose an event for this use-case.

As a further illustration, browse the below event listener utilizing a AuthenticationSensitiveEvent event. Operations such as retrieving the prior/final token object, checking authentication state, exposing the authentication provider responsible for creating the final token, and password extraction from the multiple possible formats of a token's credentials value, should all help to create a better DX experience.

<?php

use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\Security\Core\AuthenticationEvents;
use Symfony\Component\Security\Core\Authentication\Provider\UserAuthenticationProvider;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Event\AuthenticationSensitiveEvent;

$listener = function (AuthenticationSensitiveEvent $event) {
    ### Token Objects: Final and Prior

    // Easily get the final authentication token using either method (they are synonyms atm).
    $finalToken = $event->getAuthenticationToken();
    $finalToken = $event->getFinalToken();

    // Or, get the prior authentication token as originally passed into the authentication
    // manager, before any authentication provider performed operations on it.
    $priorToken = $event->getPriorToken();

    ### Token Authentication: State

    // Check if the authentication token (the final token) is authenticated.
    $isAuthenticated = $event->isTokenAuthenticated();
    // Or, check if *either* the final or the prior authentication token is authenticated.
    $isAuthenticated = $event->isTokenAuthenticated(true);

    ### Token Credentials: Extract Password

    // Easily extract the token's password from its credentials, regardless of format.
    // All core token credential types are supported, including strings (the most common)
    // and credential arrays (as seen in the guard tokens). Supported indexes for pass
    // when token credentials is an array are 'password', 'secret', and 'api_key'.
    $password = $event->getTokenPasswordCredentials();

    // Or, pass an "extraction" closure as the first argument and perform any custom
    // logic required on the token to resolve the password.
    $password = $event->getTokenPasswordCredentials(function (TokenInterface $token): ?string {
        return $token->getCredentials()['some_custom_index_string'] ?? null;
    });

    // For both examples above, be sure to check for a `null` return to ensure the
    // password resolution was successful.
    if (null !== $password) {
        // Perform work if password was extracted...
    }

    ### Authentication Provider: Fully Qualified Class Name

    // Get the fully qualified class name for the authentication provider that resolved
    // the final token value (use in a `switch` or other control structure to perform
    // specific actions depending on the authentication provider that handled the token).
    $providerClassName = $event->getAuthenticationProvider();

    // Or, quickly check if the authentication provider equals your passed class name
    // argument.
    if ($event->isAuthenticationProvider(UserAuthenticationProvider::class)) {
        // Do work if `UserAuthenticationProvider::class` is the authentication provider
        // that resolved the final token...
    }

    ### Everything Else: Use Your Imagination

    // Lastly, perform any other operations on either token that regularly occur in
    // existing code...
    $event->getFinalToken()->getUser()->callMyCustomMethodOnUserObject();

};

// Start, setup, and run dispatcher manually using above defined `$listener` anonymous
// function (normally handled internally by Symfony; only to illustrate example usage).
$dispatcher = new EventDispatcher();
$dispatcher->addListener(
    AuthenticationEvents::AUTHENTICATION_SUCCESS_SENSITIVE,
    $listener
);
$dispatcher->dispatch(
    AuthenticationEvents::AUTHENTICATION_SUCCESS_SENSITIVE,
    new AuthenticationSensitiveEvent($finalToken, $priorToken, $providerClassName)
);

@iltar, @wouterj, @nicolas-grekas: Any thoughts on the current implementation and the drafted additions here?

Some feedback:

• I don't know if it's useful to pass the authentication provider, in terms for Guard, this would be guard, thus still of no use, I don't see a way of fetching the internal authenticator either, so this might be a bit problematic to solve.

Or, check if either the final or the prior authentication token is authenticated.

• The previous token (at least in guard), is never authenticated. You're never authenticated when you're trying to authenticate, so I don't think this check makes sense to have

$event->getTokenPasswordCredentials(); I like this idea, but it's convention based. In case of the "old" tokens like UsernamePasswordToken, the contents of the credentials is a string, the old password, where the user(name) is the username filled in. It's really hard to facilitate all edge cases in here. Perhaps it would be enough to explain in the documentation that you'll need to manually check this value instead? The event as is, kinda mixes a bit of domain logic, which is error prone.

One of the issues I foresee, is the ability to determine which guard authenticator was used to generate the token. @weaverryan made it so that secured_area would have secured_area_$i as provider key in the Pre token. It would have secured_area in the Post token. If we would be able to track back which guard authenticator was used via the token, things might be easier. It would probably mean that the Pre token should get a more user-friendly name that can be referred back to the actual guard implementation used. Perhaps the class name or service id?

I'm personally okay with the event looking like this:

public function __construct(TokenInterface $authenticationAttemptToken, TokenInterface $authenticatedToken)
// or if you wish to follow the guard naming convention
public function __construct(TokenInterface $preAuthenticationToken, TokenInterface $postAuthenticationToken)

So long story short: this would have to be fixed in guard to be a lot more useful, as currently the providers are sequence based, this is fragile. On your current PR, If you'd fix the event to have pre and post tokens, it should be good enough for the core imo. Judging on this being a feature for 4.2, we have enough time to fix Guard along with it.

Edit: perhaps the authenticator class would be an option to pass along?

perhaps the authenticator class would be an option to pass along?

@iltar: The current version is passing along the authentication provider class name; that's the third constructor argument. Is that what you mean? This allows you to easily compare it with whatever provider class name you need to to determine the logic.

Here is the latest version, updated per some of your other comments and generally simplified/refactored (I've also edited all properties/methods to be much more explicit in their naming, so they should be clearer):

Edit: Removed huge code block as recent commit #b3d37d2bfae1f61a6cd1450ed5e3e1e2e6bb7124 provides the same context.

I've pushed some working changes as #b3d37d2bfae1f61a6cd1450ed5e3e1e2e6bb7124 so taking a look at that should provide some insight into how this is coming together (especially the tests, which are incomplete but helpful).

I've addressed all reviews up to this point and refactored the event's password extraction handling (to make it both simpler and more capable) and implemented comprehensive unit tests for it, as well.

Does anyone have additional thoughts on how this is coming together?

@iltar any idea when this will be merged in? Is it waiting on more reviews?

@timwhite I have no idea, I'm merely a contributor and I have no say in those things. Before it can be merged, you'll have to solve the conflicts (and rebase your branch). @chalasr can do more with this PR 👍

IMO this is still a much needed feature for the people that want to upgrade their legacy passwords or rehash them when the cost is lower than the required cost.

Would it be possible to pick this up? Or would it be okay that I make a new PR for this if @robfrawley is okay with that?

cc @chalasr

@chalasr what branch you you want me to rebase it from? I will make a new PR so this can be merged.

@rjd22 should be master, this one has the good target but is outdated. No emergency though

Will pick this up tomorrow at EUFOSSA 👍

Thank you very much for your pull request!

As part of the Symfony EU funded hackathon (https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming), we were able to assemble most of the core team and big contributors in one place. Our goal is to finish as many open issues and PRs as possible.

Your commits will not be lost and you will therefore get credit for your work. All that will happen is that your commits will be moved to a new PR (see #30955) where all remaining concerns will be addressed.

Without your work this would not have been possible. So thank you once again!