Skip to content

Close #213: honor post-success listeners that may block authentication #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
giosh94mhz wants to merge 3 commits into
base: 7.x
Choose a base branch
from
Open

Close #213: honor post-success listeners that may block authentication #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
951ee9a
Unit tests: fixed expections for Token attributes
giosh94mhz May 15, 2026
6adf4cb
Close #213: honor post-success listeners that may block authentication
giosh94mhz May 15, 2026
a26ff7b
Issue #213: set token as completed right before the COMPLETE event
giosh94mhz May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/bundle/Security/Http/Authenticator/TwoFactorAuthenticator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,22 @@
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
use Symfony\Component\Security\Http\Authenticator\Passport\Passport;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
use Symfony\Contracts\Service\ResetInterface;
use function assert;
use function class_exists;
use function count;
use function spl_object_hash;

/**
* @final
*/
class TwoFactorAuthenticator implements AuthenticatorInterface, InteractiveAuthenticatorInterface
class TwoFactorAuthenticator implements AuthenticatorInterface, InteractiveAuthenticatorInterface, ResetInterface
{
public const FLAG_2FA_COMPLETE = '2fa_complete';

private readonly LoggerInterface $logger;
/** @var TwoFactorTokenInterface[] */
private array $onCompleteTokens = [];

public function __construct(
private readonly TwoFactorFirewallConfig $twoFactorFirewallConfig,
Expand All @@ -53,6 +58,11 @@ public function __construct(
$this->logger = $logger ?? new NullLogger();
}

public function reset(): void
{
$this->onCompleteTokens = [];
}

public function supports(Request $request): bool|null
{
return $this->twoFactorFirewallConfig->isCheckPathRequest($request);
Expand Down Expand Up @@ -113,6 +123,7 @@ public function createToken(Passport $passport, string $firewallName): TokenInte
if ($this->isAuthenticationComplete($twoFactorToken)) {
$authenticatedToken = $twoFactorToken->getAuthenticatedToken(); // Authentication complete, unwrap the token
$authenticatedToken->setAttribute(self::FLAG_2FA_COMPLETE, true);
$this->onCompleteTokens[spl_object_hash($authenticatedToken)] = $twoFactorToken;

return $authenticatedToken;
}
Expand All @@ -122,7 +133,10 @@ public function createToken(Passport $passport, string $firewallName): TokenInte

private function isAuthenticationComplete(TwoFactorTokenInterface $token): bool
{
return !$this->twoFactorFirewallConfig->isMultiFactor() || $token->allTwoFactorProvidersAuthenticated();
return !$this->twoFactorFirewallConfig->isMultiFactor()
|| $token->allTwoFactorProvidersAuthenticated()
// The current provider is the last provider. It will be completed in onAuthenticationSuccess.
|| 1 === count($token->getTwoFactorProviders());
}

public function onAuthenticationSuccess(Request $request, TokenInterface $token, string $firewallName): Response|null
Expand All @@ -132,11 +146,25 @@ public function onAuthenticationSuccess(Request $request, TokenInterface $token,

// When it's still a TwoFactorTokenInterface, keep showing the auth form
if ($token instanceof TwoFactorTokenInterface) {
$currentProvider = $token->getCurrentTwoFactorProvider();
if (null !== $currentProvider) {
$token->setTwoFactorProviderComplete($currentProvider);
}

$this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::REQUIRE, $request, $token);

return $this->authenticationRequiredHandler->onAuthenticationRequired($request, $token);
}

$twoFactorToken = $this->onCompleteTokens[spl_object_hash($token)] ?? null;
if (null !== $twoFactorToken) {
unset($this->onCompleteTokens[spl_object_hash($token)]);
$currentProvider = $twoFactorToken->getCurrentTwoFactorProvider();
if (null !== $currentProvider) {
$twoFactorToken->setTwoFactorProviderComplete($currentProvider);
}
}
Copy link
Copy Markdown
Owner

@scheb scheb May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this part isn't pretty. Feels like a "hack".

I'd say we don't need this necessarily. When the 2fa token is completed, the TwoFactorToken is replaced by the authenticated token. So changing the "complete" state for the last provider is not really necessary, since we're ditching the TwoFactorToken anyways. I checked, if we're removing this part, 2fa is still working fine, also in the multi-factor case. The important part is to call setTwoFactorProviderComplete in line 145.

So I'd say we're going with your "option 1" for when the 2fa process is finally completed:

doing nothing, as the token will be forgotten right away (also, the TokenStorage no longer contains it)

This will introduce a this small behavioral change, that you have previously described:

Updating users may experience changes in TwoFactorTokenInterface::getCurrentTwoFactorProvider, e.g. from null to totp (though this is likely a fix rather than an issue, IMO).

I'd be okay with that. Also agree, that this may rather be a "fix".

Copy link
Copy Markdown
Author

@giosh94mhz giosh94mhz May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented out code fragments for the final, clean implementation.

I removed most of the comments and updated the one you pointed out

count($token->getTwoFactorProviders()) <= 1

Had to check 1 === count($token->getTwoFactorProviders()) as some other test failed otherwise

So I'd say we're going with your "option 1" for when the 2fa process is finally completed: [...]

Once I implemented the "token trick" I was no longer sure it was a good idea of doing nothing until the next "major". It's a minor BC, but I don't like to break other people code.

keep a local map like "SymfonyToken => TwoFactorToken", so that we can retrieve TwoFactorToken

I've done the above solution (option 2), that is fully encapsulated in TwoFactorAuthenticator.

IMO, we may flag method allTwoFactorProvidersAuthenticated as @deprecated so that in the next major you may remove it, and revert my 2nd commit with the trick.

What do you think?

Copy link
Copy Markdown
Owner

@scheb scheb May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with that. Will have another look, once I find some time.


$this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::COMPLETE, $request, $token);

return $this->successHandler->onAuthenticationSuccess($request, $token);
Expand Down
2 changes: 1 addition & 1 deletion src/bundle/Security/Http/EventListener/AbstractCheckCodeListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function checkPassport(CheckPassportEvent $event): void
return;
}

$token->setTwoFactorProviderComplete($providerName);
// Badge is resolved, but authentication for provider is not complete yet
$credentialsBadge->markResolved();
}

Expand Down
35 changes: 29 additions & 6 deletions tests/Security/Http/Authenticator/TwoFactorAuthenticatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace Scheb\TwoFactorBundle\Tests\Security\Http\Authenticator;

use InvalidArgumentException;
use PHPUnit\Framework\Attributes\Test;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
Expand All @@ -30,8 +31,10 @@
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\RememberMeBadge;
use Symfony\Component\Security\Http\Authenticator\Passport\Passport;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
use function array_key_exists;
use function assert;
use function method_exists;
use function sprintf;

class TwoFactorAuthenticatorTest extends TestCase
{
Expand Down Expand Up @@ -193,12 +196,30 @@ private function stubIsCheckPath(bool $isCheckPath): void
->willReturn($isCheckPath);
}

private function expect2faCompleteFlagSet(MockObject $authenticatedToken): void
private function stubAttributes(MockObject $authenticatedToken): void
{
$attributes = [];
$authenticatedToken
->expects($this->any())
->method('setAttribute')
->with(TwoFactorAuthenticator::FLAG_2FA_COMPLETE, true);
->willReturnCallback(static function ($name, $value) use (&$attributes) {
$attributes[$name] = $value;

return null;
});
$authenticatedToken
->method('getAttribute')
->willReturnCallback(static function ($name) use (&$attributes) {
return array_key_exists($name, $attributes);
});
$authenticatedToken
->method('getAttribute')
->willReturnCallback(static function ($name) use (&$attributes) {
if (!array_key_exists($name, $attributes)) {
throw new InvalidArgumentException(sprintf('This token has no "%s" attribute.', $name));
}

return $attributes[$name];
});
}

#[Test]
Expand Down Expand Up @@ -347,10 +368,11 @@ public function createToken_multiFactorAuthenticationIsComplete_returnAuthentica
$twoFactorToken = $this->createTwoFactorToken($authenticatedToken, true);
$passport = $this->createPassportWithTwoFactorCredentials($twoFactorToken);

$this->expect2faCompleteFlagSet($authenticatedToken);

$this->stubAttributes($authenticatedToken);
$returnValue = $this->authenticator->createToken($passport, self::FIREWALL_NAME);

$this->assertSame($authenticatedToken, $returnValue);
$this->assertTrue($authenticatedToken->getAttribute(TwoFactorAuthenticator::FLAG_2FA_COMPLETE));
}

#[Test]
Expand All @@ -361,10 +383,11 @@ public function createToken_noMultiFactorAuthentication_returnAuthenticatedToken
$twoFactorToken = $this->createTwoFactorToken($authenticatedToken, false);
$passport = $this->createPassportWithTwoFactorCredentials($twoFactorToken);

$this->expect2faCompleteFlagSet($authenticatedToken);
$this->stubAttributes($authenticatedToken);

$returnValue = $this->authenticator->createToken($passport, self::FIREWALL_NAME);
$this->assertSame($authenticatedToken, $returnValue);
$this->assertTrue($authenticatedToken->getAttribute(TwoFactorAuthenticator::FLAG_2FA_COMPLETE));
}

#[Test]
Expand Down
2 changes: 1 addition & 1 deletion tests/Security/TwoFactor/Event/AuthenticationTokenListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public function onAuthenticationTokenCreated_tokenFlagged2faComplete_notChangeTo
$this->stubTwoFactorConditionsFulfilled(true);
$authenticatedToken = $this->createMock(TokenInterface::class);
$authenticatedToken
->expects($this->any())
->expects($this->atLeastOnce())
->method('hasAttribute')
->with(TwoFactorAuthenticator::FLAG_2FA_COMPLETE)
->willReturn(true);
Expand Down
Loading