Skip to content

Commit 16f232d

Browse files
fabpotfabpot
committed
bug #51104 [Security] Fix loading user from UserBadge (guillaumesmo)
This PR was merged into the 6.3 branch. Discussion ---------- [Security] Fix loading user from UserBadge | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #50511 | License | MIT | Doc PR | none Fixed a breaking change from https://github.com/symfony/symfony/pull/48272/files#diff-de9707bb338188f62878f2ebd42e7a7bf9547f6d0bf07a4fcd9c386c263c601b Commits ------- 21532cb6bc Fix breaking change in AccessTokenAuthenticator
2 parents 1696026 + 1d6ce2c commit 16f232d

File tree

7 files changed

+203
-5
lines changed

7 files changed

+203
-5
lines changed

‎AccessToken/Oidc/OidcTokenHandler.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface;
2828
use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\InvalidSignatureException;
2929
use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\MissingClaimException;
30+
use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader;
3031
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
3132

3233
/**
@@ -93,7 +94,7 @@ public function getUserBadgeFrom(string $accessToken): UserBadge
9394
}
9495

9596
// UserLoader argument can be overridden by a UserProvider on AccessTokenAuthenticator::authenticate
96-
return new UserBadge($claims[$this->claim], fn () => $this->createUser($claims), $claims);
97+
return new UserBadge($claims[$this->claim], new FallbackUserLoader(fn () => $this->createUser($claims)), $claims);
9798
} catch (\Exception $e) {
9899
$this->logger?->error('An error occurred while decoding and validating the token.', [
99100
'error' => $e->getMessage(),

‎AccessToken/Oidc/OidcUserInfoTokenHandler.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
1616
use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface;
1717
use Symfony\Component\Security\Http\AccessToken\Oidc\Exception\MissingClaimException;
18+
use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader;
1819
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
1920
use Symfony\Contracts\HttpClient\HttpClientInterface;
2021

@@ -48,7 +49,7 @@ public function getUserBadgeFrom(string $accessToken): UserBadge
4849
}
4950

5051
// UserLoader argument can be overridden by a UserProvider on AccessTokenAuthenticator::authenticate
51-
return new UserBadge($claims[$this->claim], fn () => $this->createUser($claims), $claims);
52+
return new UserBadge($claims[$this->claim], new FallbackUserLoader(fn () => $this->createUser($claims)), $claims);
5253
} catch (\Exception $e) {
5354
$this->logger?->error('An error occurred on OIDC server.', [
5455
'error' => $e->getMessage(),

‎Authenticator/AccessTokenAuthenticator.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public function authenticate(Request $request): Passport
5959
}
6060

6161
$userBadge = $this->accessTokenHandler->getUserBadgeFrom($accessToken);
62-
if ($this->userProvider) {
62+
if ($this->userProvider && (null === $userBadge->getUserLoader() || $userBadge->getUserLoader() instanceof FallbackUserLoader)) {
6363
$userBadge->setUserLoader($this->userProvider->loadUserByIdentifier(...));
6464
}
6565

‎Authenticator/FallbackUserLoader.php

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Security\Http\Authenticator;
13+
14+
use Symfony\Component\Security\Core\User\UserInterface;
15+
16+
/**
17+
* This wrapper serves as a marker interface to indicate badge user loaders that should not be overridden by the
18+
* default user provider.
19+
*
20+
* @internal
21+
*/
22+
final class FallbackUserLoader
23+
{
24+
public function __construct(private $inner)
25+
{
26+
}
27+
28+
public function __invoke(mixed ...$args): ?UserInterface
29+
{
30+
return ($this->inner)(...$args);
31+
}
32+
}

‎Tests/AccessToken/Oidc/OidcTokenHandlerTest.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
2222
use Symfony\Component\Security\Core\User\OidcUser;
2323
use Symfony\Component\Security\Http\AccessToken\Oidc\OidcTokenHandler;
24+
use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader;
2425
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
2526

2627
/**
@@ -61,7 +62,7 @@ public function testGetsUserIdentifierFromSignedToken(string $claim, string $exp
6162
))->getUserBadgeFrom($token);
6263
$actualUser = $userBadge->getUserLoader()();
6364

64-
$this->assertEquals(new UserBadge($expected, fn () => $expectedUser, $claims), $userBadge);
65+
$this->assertEquals(new UserBadge($expected, new FallbackUserLoader(fn () => $expectedUser), $claims), $userBadge);
6566
$this->assertInstanceOf(OidcUser::class, $actualUser);
6667
$this->assertEquals($expectedUser, $actualUser);
6768
$this->assertEquals($claims, $userBadge->getAttributes());

‎Tests/AccessToken/Oidc/OidcUserInfoTokenHandlerTest.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
1717
use Symfony\Component\Security\Core\User\OidcUser;
1818
use Symfony\Component\Security\Http\AccessToken\Oidc\OidcUserInfoTokenHandler;
19+
use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader;
1920
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
2021
use Symfony\Contracts\HttpClient\HttpClientInterface;
2122
use Symfony\Contracts\HttpClient\ResponseInterface;
@@ -47,7 +48,7 @@ public function testGetsUserIdentifierFromOidcServerResponse(string $claim, stri
4748
$userBadge = (new OidcUserInfoTokenHandler($clientMock, null, $claim))->getUserBadgeFrom($accessToken);
4849
$actualUser = $userBadge->getUserLoader()();
4950

50-
$this->assertEquals(new UserBadge($expected, fn () => $expectedUser, $claims), $userBadge);
51+
$this->assertEquals(new UserBadge($expected, new FallbackUserLoader(fn () => $expectedUser), $claims), $userBadge);
5152
$this->assertInstanceOf(OidcUser::class, $actualUser);
5253
$this->assertEquals($expectedUser, $actualUser);
5354
$this->assertEquals($claims, $userBadge->getAttributes());

‎Tests/Authenticator/AccessTokenAuthenticatorTest.php

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Authenticator;
13+
14+
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
17+
use Symfony\Component\Security\Core\User\InMemoryUser;
18+
use Symfony\Component\Security\Core\User\InMemoryUserProvider;
19+
use Symfony\Component\Security\Http\AccessToken\AccessTokenExtractorInterface;
20+
use Symfony\Component\Security\Http\AccessToken\AccessTokenHandlerInterface;
21+
use Symfony\Component\Security\Http\Authenticator\AccessTokenAuthenticator;
22+
use Symfony\Component\Security\Http\Authenticator\FallbackUserLoader;
23+
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
24+
25+
class AccessTokenAuthenticatorTest extends TestCase
26+
{
27+
private AccessTokenHandlerInterface $accessTokenHandler;
28+
private AccessTokenExtractorInterface $accessTokenExtractor;
29+
private InMemoryUserProvider $userProvider;
30+
31+
protected function setUp(): void
32+
{
33+
$this->accessTokenHandler = $this->createMock(AccessTokenHandlerInterface::class);
34+
$this->accessTokenExtractor = $this->createMock(AccessTokenExtractorInterface::class);
35+
$this->userProvider = new InMemoryUserProvider(['test' => ['password' => 's$cr$t']]);
36+
}
37+
38+
public function testAuthenticateWithoutAccessToken()
39+
{
40+
$this->expectException(BadCredentialsException::class);
41+
$this->expectExceptionMessage('Invalid credentials.');
42+
43+
$request = Request::create('/test');
44+
45+
$this->accessTokenExtractor
46+
->expects($this->once())
47+
->method('extractAccessToken')
48+
->with($request)
49+
->willReturn(null);
50+
51+
$authenticator = new AccessTokenAuthenticator(
52+
$this->accessTokenHandler,
53+
$this->accessTokenExtractor,
54+
);
55+
56+
$authenticator->authenticate($request);
57+
}
58+
59+
public function testAuthenticateWithoutProvider()
60+
{
61+
$request = Request::create('/test');
62+
63+
$this->accessTokenExtractor
64+
->expects($this->once())
65+
->method('extractAccessToken')
66+
->with($request)
67+
->willReturn('test');
68+
$this->accessTokenHandler
69+
->expects($this->once())
70+
->method('getUserBadgeFrom')
71+
->with('test')
72+
->willReturn(new UserBadge('john', fn () => new InMemoryUser('john', null)));
73+
74+
$authenticator = new AccessTokenAuthenticator(
75+
$this->accessTokenHandler,
76+
$this->accessTokenExtractor,
77+
$this->userProvider,
78+
);
79+
80+
$passport = $authenticator->authenticate($request);
81+
82+
$this->assertEquals('john', $passport->getUser()->getUserIdentifier());
83+
}
84+
85+
public function testAuthenticateWithoutUserLoader()
86+
{
87+
$request = Request::create('/test');
88+
89+
$this->accessTokenExtractor
90+
->expects($this->once())
91+
->method('extractAccessToken')
92+
->with($request)
93+
->willReturn('test');
94+
$this->accessTokenHandler
95+
->expects($this->once())
96+
->method('getUserBadgeFrom')
97+
->with('test')
98+
->willReturn(new UserBadge('test'));
99+
100+
$authenticator = new AccessTokenAuthenticator(
101+
$this->accessTokenHandler,
102+
$this->accessTokenExtractor,
103+
$this->userProvider,
104+
);
105+
106+
$passport = $authenticator->authenticate($request);
107+
108+
$this->assertEquals('test', $passport->getUser()->getUserIdentifier());
109+
}
110+
111+
public function testAuthenticateWithUserLoader()
112+
{
113+
$request = Request::create('/test');
114+
115+
$this->accessTokenExtractor
116+
->expects($this->once())
117+
->method('extractAccessToken')
118+
->with($request)
119+
->willReturn('test');
120+
$this->accessTokenHandler
121+
->expects($this->once())
122+
->method('getUserBadgeFrom')
123+
->with('test')
124+
->willReturn(new UserBadge('john', fn () => new InMemoryUser('john', null)));
125+
126+
$authenticator = new AccessTokenAuthenticator(
127+
$this->accessTokenHandler,
128+
$this->accessTokenExtractor,
129+
$this->userProvider,
130+
);
131+
132+
$passport = $authenticator->authenticate($request);
133+
134+
$this->assertEquals('john', $passport->getUser()->getUserIdentifier());
135+
}
136+
137+
public function testAuthenticateWithFallbackUserLoader()
138+
{
139+
$request = Request::create('/test');
140+
141+
$this->accessTokenExtractor
142+
->expects($this->once())
143+
->method('extractAccessToken')
144+
->with($request)
145+
->willReturn('test');
146+
$this->accessTokenHandler
147+
->expects($this->once())
148+
->method('getUserBadgeFrom')
149+
->with('test')
150+
->willReturn(new UserBadge('test', new FallbackUserLoader(fn () => new InMemoryUser('john', null))));
151+
152+
$authenticator = new AccessTokenAuthenticator(
153+
$this->accessTokenHandler,
154+
$this->accessTokenExtractor,
155+
$this->userProvider,
156+
);
157+
158+
$passport = $authenticator->authenticate($request);
159+
160+
$this->assertEquals('test', $passport->getUser()->getUserIdentifier());
161+
}
162+
}

0 commit comments

Comments
 (0)