Skip to content

Commit e10d895

Browse files
schebscheb
committed
Fix pre-poisoning of code-reuse cache, fixes #317
Signed-off-by: Christian Scheb <git@christianscheb.de>
1 parent 30f4fcd commit e10d895

6 files changed

Lines changed: 115 additions & 48 deletions

File tree

‎doc/events.rst‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,24 @@ Event class: ``\Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEven
8080

8181
Is dispatched right before authentication provider is used to check the code.
8282

83+
``scheb_two_factor.authentication.code_valid``
84+
~~~~~~~~~~~~~~~~~~~~~~~~~~
85+
86+
Constant: ``Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorAuthenticationEvents::CODE_VALID``
87+
88+
Event class: ``\Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent``
89+
90+
Is dispatch right after the authentication provider validating the code, when the code was *valid*.
91+
92+
``scheb_two_factor.authentication.code_invalid``
93+
~~~~~~~~~~~~~~~~~~~~~~~~~~
94+
95+
Constant: ``Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorAuthenticationEvents::CODE_INVALID``
96+
97+
Event class: ``\Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent``
98+
99+
Is dispatch right after the authentication provider validating the code, when the code was *invalid*.
100+
83101
``scheb_two_factor.authentication.code_reused``
84102
~~~~~~~~~~~~~~~~~~~~~~~~~~
85103

‎src/bundle/Security/Http/EventListener/CheckTwoFactorCodeListener.php‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,21 @@ protected function isValidCode(string $providerName, object $user, string $code)
4545
throw $exception;
4646
}
4747

48-
if (!$authenticationProvider->validateAuthenticationCode($user, $code)) {
49-
throw new InvalidTwoFactorCodeException(InvalidTwoFactorCodeException::MESSAGE);
48+
if ($authenticationProvider->validateAuthenticationCode($user, $code)) {
49+
$this->eventDispatcher->dispatch(
50+
new TwoFactorCodeEvent($user, $code),
51+
TwoFactorAuthenticationEvents::CODE_VALID,
52+
);
53+
54+
return true;
5055
}
5156

52-
return true;
57+
$this->eventDispatcher->dispatch(
58+
new TwoFactorCodeEvent($user, $code),
59+
TwoFactorAuthenticationEvents::CODE_INVALID,
60+
);
61+
62+
throw new InvalidTwoFactorCodeException(InvalidTwoFactorCodeException::MESSAGE);
5363
}
5464

5565
/**

‎src/bundle/Security/Http/EventListener/CheckTwoFactorCodeReuseListener.php‎

Lines changed: 42 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,40 +30,65 @@ public function __construct(
3030

3131
public function checkForCodeReuse(TwoFactorCodeEvent $event): void
3232
{
33-
if (null === $this->cache) {
33+
$cache = $this->getCachePool();
34+
if (null === $cache) {
3435
return;
3536
}
3637

37-
if (!$this->cache instanceof CacheItemPoolInterface) {
38-
if ($this->logger instanceof LoggerInterface) {
39-
$this->logger->error('Your reuse-cache seems to be configured wrongly! Provide a CacheItemPoolInterface as the cache object if you want to disallow reusing 2FA-codes!');
40-
}
38+
$cacheItem = $cache->getItem($this->getCacheKey($event));
39+
// phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed
40+
if ($cacheItem->isHit()) {
41+
$this->eventDispatcher->dispatch(
42+
new TwoFactorCodeReusedEvent($event->getUser(), $event->getCode()),
43+
TwoFactorAuthenticationEvents::CODE_REUSED,
44+
);
45+
}
46+
}
4147

48+
public function rememberCode(TwoFactorCodeEvent $event): void
49+
{
50+
$cache = $this->getCachePool();
51+
if (null === $cache) {
4252
return;
4353
}
4454

45-
$cacheKey = 'scheb_two_factor_code_reuse.'
46-
.sha1($event->getUser()->getUserIdentifier().'.'.$event->getCode());
47-
48-
$cacheItem = $this->cache->getItem($cacheKey);
55+
$cacheItem = $cache->getItem($this->getCacheKey($event));
4956
$cacheItem->expiresAfter($this->cacheDuration);
5057
$cacheItem->set(true);
51-
$this->cache->save($cacheItem);
58+
$cache->save($cacheItem);
59+
}
5260

53-
// phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed
54-
if ($cacheItem->isHit()) {
55-
$this->eventDispatcher->dispatch(
56-
new TwoFactorCodeReusedEvent($event->getUser(), $event->getCode()),
57-
TwoFactorAuthenticationEvents::CODE_REUSED,
58-
);
61+
private function getCachePool(): CacheItemPoolInterface|null
62+
{
63+
if (null === $this->cache) {
64+
return null;
65+
}
66+
67+
if (!$this->cache instanceof CacheItemPoolInterface) {
68+
if ($this->logger instanceof LoggerInterface) {
69+
$this->logger->error('Your reuse-cache seems to be configured wrongly! Provide a CacheItemPoolInterface as the cache object if you want to disallow reusing 2FA-codes!');
70+
}
71+
72+
return null;
5973
}
74+
75+
return $this->cache;
76+
}
77+
78+
private function getCacheKey(TwoFactorCodeEvent $event): string
79+
{
80+
return 'scheb_two_factor_code_reuse.'
81+
.sha1($event->getUser()->getUserIdentifier().'.'.$event->getCode());
6082
}
6183

6284
/**
6385
* {@inheritDoc}
6486
*/
6587
public static function getSubscribedEvents(): array
6688
{
67-
return [TwoFactorAuthenticationEvents::CHECK => ['checkForCodeReuse', self::LISTENER_PRIORITY]];
89+
return [
90+
TwoFactorAuthenticationEvents::CHECK => ['checkForCodeReuse', self::LISTENER_PRIORITY],
91+
TwoFactorAuthenticationEvents::CODE_VALID => ['rememberCode', self::LISTENER_PRIORITY],
92+
];
6893
}
6994
}

‎src/bundle/Security/TwoFactor/Event/TwoFactorAuthenticationEvents.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,16 @@ class TwoFactorAuthenticationEvents
4747
*/
4848
public const CHECK = 'scheb_two_factor.authentication.check';
4949

50+
/**
51+
* Right after a two-factor authentication code was checked, when the code was valid.
52+
*/
53+
public const CODE_VALID = 'scheb_two_factor.authentication.code_valid';
54+
55+
/**
56+
* Right after a two-factor authentication code was checked, when the code was invalid.
57+
*/
58+
public const CODE_INVALID = 'scheb_two_factor.authentication.code_invalid';
59+
5060
/**
5161
* When the two-factor code has been used already.
5262
*/

‎tests/Security/Http/EventListener/CheckTwoFactorCodeListenerTest.php‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,17 @@
1515
use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent;
1616
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface;
1717
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderRegistry;
18+
use Scheb\TwoFactorBundle\Tests\EventDispatcherTestHelper;
1819
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
1920

2021
/**
2122
* @property CheckTwoFactorCodeListener $listener
2223
*/
2324
class CheckTwoFactorCodeListenerTest extends AbstractCheckCodeListenerTestSetup
2425
{
26+
use EventDispatcherTestHelper;
27+
2528
private MockObject|BackupCodeManagerInterface $providerRegistry;
26-
private MockObject|EventDispatcherInterface $eventDispatcher;
2729

2830
protected function setUp(): void
2931
{
@@ -86,10 +88,10 @@ public function checkPassport_validCode_invalidateAndResolveCredentials(): void
8688
{
8789
$this->stubAllPreconditionsFulfilled();
8890

89-
$this->eventDispatcher
90-
->expects($this->once())
91-
->method('dispatch')
92-
->with(new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CHECK);
91+
$this->expectDispatchConsecutiveEvents([
92+
[new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CHECK],
93+
[new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CODE_VALID],
94+
]);
9395

9496
$authenticationProvider = $this->stubTwoFactorAuthenticationProvider();
9597
$authenticationProvider
@@ -108,10 +110,10 @@ public function checkPassport_invalidCode_unresolvedCredentials(): void
108110
{
109111
$this->stubAllPreconditionsFulfilled();
110112

111-
$this->eventDispatcher
112-
->expects($this->once())
113-
->method('dispatch')
114-
->with(new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CHECK);
113+
$this->expectDispatchConsecutiveEvents([
114+
[new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CHECK],
115+
[new TwoFactorCodeEvent($this->user, self::CODE), TwoFactorAuthenticationEvents::CODE_INVALID],
116+
]);
115117

116118
$authenticationProvider = $this->stubTwoFactorAuthenticationProvider();
117119
$authenticationProvider

‎tests/Security/Http/EventListener/CheckTwoFactorCodeReuseListenerTest.php‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeReusedEvent;
1818
use Scheb\TwoFactorBundle\Tests\TestCase;
1919
use stdClass;
20-
use Symfony\Component\Cache\CacheItem;
2120
use Symfony\Component\Security\Core\User\UserInterface;
2221
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
2322

@@ -27,6 +26,7 @@
2726
class CheckTwoFactorCodeReuseListenerTest extends TestCase
2827
{
2928
private const MFA_CODE = '123456';
29+
private const CACHE_KEY = 'scheb_two_factor_code_reuse.3468ccbd044e2fdfb0769b68b5c85d7660c6c12c';
3030
private const USER_IDENTIFIER = 'jdoe@example.com';
3131

3232
private MockObject|EventDispatcherInterface $eventDispatcher;
@@ -44,7 +44,21 @@ protected function setUp(): void
4444
$this->logger = $this->createMock(LoggerInterface::class);
4545
}
4646

47-
protected function expectDoNothing(): void
47+
private function stubCacheItemIsHit(CacheItemPoolInterface&MockObject $cacheProvider, bool $isHit): void
48+
{
49+
$cacheItem = $this->createMock(CacheItemInterface::class);
50+
$cacheItem
51+
->expects($this->any())
52+
->method('isHit')
53+
->willReturn($isHit);
54+
55+
$cacheProvider->expects($this->once())
56+
->method('getItem')
57+
->with(self::CACHE_KEY)
58+
->willReturn($cacheItem);
59+
}
60+
61+
private function expectDoNothing(): void
4862
{
4963
$this->eventDispatcher
5064
->expects($this->never())
@@ -79,13 +93,7 @@ public function checkForCodeReuse_invalidCacheProvider_nothingHappensButLogged()
7993
public function checkForCodeReuse_validCacheProviderAndNoHit_nothingHappens(): void
8094
{
8195
$cacheProvider = $this->createMock(CacheItemPoolInterface::class);
82-
$cacheItem = new CacheItem();
83-
$cacheItem->set(true);
84-
$cacheItem->expiresAfter(60);
85-
$cacheProvider->expects($this->once())
86-
->method('getItem')
87-
->with('scheb_two_factor_code_reuse.3468ccbd044e2fdfb0769b68b5c85d7660c6c12c')
88-
->willReturn($cacheItem);
96+
$this->stubCacheItemIsHit($cacheProvider, false);
8997

9098
$listener = new CheckTwoFactorCodeReuseListener($this->eventDispatcher, $cacheProvider, 20, $this->logger);
9199
$this->expectDoNothing();
@@ -97,13 +105,7 @@ public function checkForCodeReuse_validCacheProviderAndNoHit_nothingHappens(): v
97105
public function checkForCodeReuse_validCacheProviderWithCacheHit_eventIsDispatched(): void
98106
{
99107
$cacheProvider = $this->createMock(CacheItemPoolInterface::class);
100-
101-
$cacheItem = $this->createMock(CacheItemInterface::class);
102-
$cacheItem->expects($this->once())->method('isHit')->willReturn(true);
103-
104-
$cacheProvider->expects($this->once())
105-
->method('getItem')
106-
->willReturn($cacheItem);
108+
$this->stubCacheItemIsHit($cacheProvider, true);
107109

108110
$this->eventDispatcher->expects($this->once())
109111
->method('dispatch')
@@ -115,7 +117,7 @@ public function checkForCodeReuse_validCacheProviderWithCacheHit_eventIsDispatch
115117
}
116118

117119
#[Test]
118-
public function checkForCodeReuse_validCacheProviderNoCacheHit_cacheItemIsSaved(): void
120+
public function rememberCode_validCacheProvider_cacheItemIsSaved(): void
119121
{
120122
$cacheItem = new class implements CacheItemInterface {
121123
private mixed $value;
@@ -171,12 +173,12 @@ public function expiresAfter(DateInterval|int|null $time): static
171173
$cacheItem->expiresAfter(60);
172174
$cacheProvider->expects($this->once())
173175
->method('getItem')
174-
->with('scheb_two_factor_code_reuse.3468ccbd044e2fdfb0769b68b5c85d7660c6c12c')
176+
->with(self::CACHE_KEY)
175177
->willReturn($cacheItem);
176178

177179
$listener = new CheckTwoFactorCodeReuseListener($this->eventDispatcher, $cacheProvider, 20, $this->logger);
178180

179-
$listener->checkForCodeReuse(new TwoFactorCodeEvent($this->user, self::MFA_CODE));
181+
$listener->rememberCode(new TwoFactorCodeEvent($this->user, self::MFA_CODE));
180182

181183
$this->assertSame(20, $cacheItem->expiresAfter);
182184
$this->assertSame(true, $cacheItem->get());

0 commit comments

Comments
 (0)