Skip to content

Commit 43088a1

Browse files
schebscheb
committed
Fix pre-poisoning of code-reuse cache, fixes #317 
1 parent 7bb83b3 commit 43088a1

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 string 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 string 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 string 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
@@ -14,15 +14,17 @@
1414
use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorCodeEvent;
1515
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface;
1616
use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderRegistry;
17+
use Scheb\TwoFactorBundle\Tests\EventDispatcherTestHelper;
1718
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
1819

1920
/**
2021
* @property CheckTwoFactorCodeListener $listener
2122
*/
2223
class CheckTwoFactorCodeListenerTest extends AbstractCheckCodeListenerTestSetup
2324
{
25+
use EventDispatcherTestHelper;
26+
2427
private MockObject&TwoFactorProviderRegistry $providerRegistry;
25-
private MockObject&EventDispatcherInterface $eventDispatcher;
2628

2729
protected function setUp(): void
2830
{
@@ -85,10 +87,10 @@ public function checkPassport_validCode_invalidateAndResolveCredentials(): void
8587
{
8688
$this->stubAllPreconditionsFulfilled();
8789

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

9395
$authenticationProvider = $this->stubTwoFactorAuthenticationProvider();
9496
$authenticationProvider
@@ -107,10 +109,10 @@ public function checkPassport_invalidCode_unresolvedCredentials(): void
107109
{
108110
$this->stubAllPreconditionsFulfilled();
109111

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

115117
$authenticationProvider = $this->stubTwoFactorAuthenticationProvider();
116118
$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 string MFA_CODE = '123456';
29+
private const string CACHE_KEY = 'scheb_two_factor_code_reuse.3468ccbd044e2fdfb0769b68b5c85d7660c6c12c';
3030
private const string 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)