Skip to content

Commit 81d95dc

Browse files
iisisraeliisisrael
committed
Fix request and credentials handling in authenticator
1 parent 5059e4c commit 81d95dc

File tree

4 files changed

+112
-82
lines changed

4 files changed

+112
-82
lines changed

‎Security/Authentication/Authenticator/OAuthAuthenticator.php‎

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
use Symfony\Component\Security\Core\Exception\AccountStatusException;
2626
use Symfony\Component\Security\Core\Exception\AuthenticationException;
2727
use Symfony\Component\Security\Core\User\UserCheckerInterface;
28+
use Symfony\Component\Security\Core\User\UserInterface;
2829
use Symfony\Component\Security\Core\User\UserProviderInterface;
2930
use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface;
3031
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
@@ -76,42 +77,42 @@ public function __construct(
7677
*/
7778
public function authenticate(Request $request): UserPassportInterface
7879
{
79-
try {
80-
$token = $this->tokenStorage->getToken();
81-
$tokenString = $token->getToken();
80+
// remove the authorization header from the request on this check
81+
$tokenString = $this->serverService->getBearerToken($request, true);
82+
$accessToken = $scope = $user = $username = null;
8283

84+
try {
8385
$accessToken = $this->serverService->verifyAccessToken($tokenString);
84-
85-
/** @var \Symfony\Component\Security\Core\User\UserInterface **/
86+
$scope = $accessToken->getScope();
8687
$user = $accessToken->getUser();
88+
// allow for dependency on deprecated getUsername method
89+
$username = $user instanceof UserInterface
90+
? (method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername())
91+
: null
92+
;
93+
} catch (OAuth2AuthenticateException $e) {
94+
// do nothing - credentials will remain unresolved below
95+
}
8796

88-
if (null === $user) {
89-
throw new AuthenticationException('OAuth2 authentication failed');
90-
}
97+
// configure the passport badges, ensuring requisite string types
98+
$userBadge = new UserBadge($username ?? '');
99+
$credentials = new OAuthCredentials($tokenString ?? '', $scope ?? '');
91100

92-
// check the user
101+
// check the user if not null
102+
if ($user instanceof UserInterface) {
93103
try {
94104
$this->userChecker->checkPreAuth($user);
105+
106+
// mark the credentials as resolved
107+
$credentials->markResolved();
95108
} catch (AccountStatusException $e) {
96-
throw new OAuth2AuthenticateException(
97-
Response::HTTP_UNAUTHORIZED,
98-
OAuth2::TOKEN_TYPE_BEARER,
99-
$this->serverService->getVariable(OAuth2::CONFIG_WWW_REALM),
100-
'access_denied',
101-
$e->getMessage()
102-
);
109+
// do nothing - credentials remain unresolved
103110
}
104-
105-
return new Passport(
106-
new UserBadge($user->getUsername()),
107-
new OAuthCredentials($tokenString, $accessToken->getScope())
108-
);
109-
} catch (OAuth2ServerException $e) {
110-
throw new AuthenticationException('OAuth2 authentication failed', 0, $e);
111111
}
112112

113-
// this should never be reached
114-
throw new AuthenticationException('OAuth2 authentication failed');
113+
// passport will only be valid if all badges are resolved (user badge
114+
// is always resolved, credentials badge if passing the above check)
115+
return new Passport($userBadge, $credentials);
115116
}
116117

117118
/**
@@ -160,7 +161,7 @@ public function createAuthenticatedToken(PassportInterface $passport, string $fi
160161
$token->setToken($credentials->getTokenString());
161162
$token->setUser($user);
162163

163-
$credentials->markResolved();
164+
$this->tokenStorage->setToken($token);
164165

165166
return $token;
166167
}
@@ -194,6 +195,9 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio
194195
*/
195196
public function supports(Request $request): ?bool
196197
{
197-
return $this->tokenStorage->getToken() instanceof OAuthToken;
198+
// do not remove the authorization header from the request on this check
199+
$tokenString = $this->serverService->getBearerToken($request);
200+
201+
return is_string($tokenString) && !empty($tokenString);
198202
}
199203
}

‎Security/Authentication/Passport/OAuthCredentials.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,6 @@ public function getTokenString(): ?string
6969
public function markResolved(): void
7070
{
7171
$this->resolved = true;
72-
$this->scope = null;
73-
$this->tokenString = null;
7472
}
7573

7674
public function isResolved(): bool

‎Tests/Security/Authentication/Authenticator/OAuthAuthenticatorTest.php‎

Lines changed: 75 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
use Symfony\Component\Security\Core\Exception\CredentialsExpiredException;
2727
use Symfony\Component\Security\Core\Exception\DisabledException;
2828
use Symfony\Component\Security\Core\User\UserCheckerInterface;
29-
use Symfony\Component\Security\Core\User\UserInterface;
29+
use Symfony\Component\Security\Core\User\User;
3030
use Symfony\Component\Security\Core\User\UserProviderInterface;
3131
use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge;
3232
use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials;
@@ -50,7 +50,7 @@ class OAuthAuthenticatorTest extends \PHPUnit\Framework\TestCase
5050
protected $tokenStorage;
5151

5252
/**
53-
* @var \PHPUnit\Framework\MockObject\MockObject|UserInterface
53+
* @var \PHPUnit\Framework\MockObject\MockObject|User
5454
*/
5555
protected $user;
5656

@@ -69,16 +69,20 @@ public function setUp(): void
6969
$this->serverService = $this->getMockBuilder(OAuth2::class)
7070
->disableOriginalConstructor()
7171
->setMethods([
72+
'getBearerToken',
7273
'getVariable',
73-
'verifyAccessToken'
74+
'verifyAccessToken',
7475
])
7576
->getMock()
7677
;
7778
$this->tokenStorage = $this->getMockBuilder(TokenStorageInterface::class)->disableOriginalConstructor()->getMock();
78-
$this->user = $this->getMockBuilder(UserInterface::class)->disableOriginalConstructor()->getMock();
7979
$this->userChecker = $this->getMockBuilder(UserCheckerInterface::class)->disableOriginalConstructor()->getMock();
8080
$this->userProvider = $this->getMockBuilder(UserProviderInterface::class)->disableOriginalConstructor()->getMock();
8181

82+
// mock the core user object rather than the user interface that the new
83+
// getUserIdentifier method is used rather than the deprecated getUsername
84+
$this->user = $this->getMockBuilder(User::class)->disableOriginalConstructor()->getMock();
85+
8286
$this->authenticator = new OAuthAuthenticator(
8387
$this->serverService,
8488
$this->tokenStorage,
@@ -89,12 +93,15 @@ public function setUp(): void
8993

9094
public function testAuthenticateReturnsPassportIfValid(): void
9195
{
92-
// expect a token from the token storage
93-
$token = new OAuthToken();
94-
$token->setToken('mock_token_string');
95-
$this->tokenStorage->expects($this->once())
96-
->method('getToken')
97-
->will($this->returnValue($token))
96+
// expect the OAuth2 service to get the token from the request header,
97+
// flagging the authorization header to be removed at the same time
98+
$this->serverService->expects($this->once())
99+
->method('getBearerToken')
100+
->with(
101+
$this->isInstanceOf(Request::class),
102+
$this->equalTo(true)
103+
)
104+
->will($this->returnValue('mock_token_string'))
98105
;
99106

100107
// expect the OAuth2 service to verify the token, returning an access token
@@ -107,18 +114,18 @@ public function testAuthenticateReturnsPassportIfValid(): void
107114
->will($this->returnValue($accessToken))
108115
;
109116

117+
// expect the username from the user
118+
$this->user->expects($this->once())
119+
->method('getUserIdentifier')
120+
->will($this->returnValue('test_user'))
121+
;
122+
110123
// expect the user checker to pass
111124
$this->userChecker->expects($this->once())
112125
->method('checkPreAuth')
113126
->with($this->user)
114127
;
115128

116-
// expect the username from the user
117-
$this->user->expects($this->once())
118-
->method('getUsername')
119-
->will($this->returnValue('test_user'))
120-
;
121-
122129
$passport = $this->authenticator->authenticate(new Request());
123130

124131
$this->assertInstanceOf(Passport::class, $passport);
@@ -128,16 +135,20 @@ public function testAuthenticateReturnsPassportIfValid(): void
128135
$this->assertSame('test_user', $passport->getBadge(UserBadge::class)->getUserIdentifier());
129136
$this->assertSame('mock_token_string', $passport->getBadge(OAuthCredentials::class)->getTokenString());
130137
$this->assertSame(['ROLE_SCOPE_1', 'ROLE_SCOPE_2'], $passport->getBadge(OAuthCredentials::class)->getRoles($this->user));
138+
$this->assertTrue($passport->getBadge(OAuthCredentials::class)->isResolved());
131139
}
132140

133-
public function testAuthenticateReturnsTokenInvalidWhenNullData(): void
141+
public function testAuthenticateReturnsUnresolvedPassportWhenNullUser(): void
134142
{
135-
// expect a token from the token storage
136-
$token = new OAuthToken();
137-
$token->setToken('mock_token_string');
138-
$this->tokenStorage->expects($this->once())
139-
->method('getToken')
140-
->will($this->returnValue($token))
143+
// expect the OAuth2 service to get the token from the request header,
144+
// flagging the authorization header to be removed at the same time
145+
$this->serverService->expects($this->once())
146+
->method('getBearerToken')
147+
->with(
148+
$this->isInstanceOf(Request::class),
149+
$this->equalTo(true)
150+
)
151+
->will($this->returnValue('mock_token_string'))
141152
;
142153

143154
// expect the OAuth2 service to verify the token, returning an access
@@ -149,26 +160,29 @@ public function testAuthenticateReturnsTokenInvalidWhenNullData(): void
149160
->will($this->returnValue($accessToken))
150161
;
151162

152-
// expect an authentication exception
153-
$this->expectException(AuthenticationException::class);
154-
$this->expectExceptionMessage('OAuth2 authentication failed');
163+
// expect the null user value to not be processed
164+
$this->userChecker->expects($this->never())->method('checkPreAuth');
155165

156-
$this->authenticator->authenticate(new Request());
166+
$passport = $this->authenticator->authenticate(new Request());
167+
168+
// confirm that the returned passport won't pass validation
169+
$this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved());
157170
}
158171

159-
public function testAuthenticateTransformsOAuthServerException(): void
172+
public function testAuthenticateReturnsUnresolvedPassportWhenInvalidToken(): void
160173
{
161-
// expect a token from the token storage
162-
$token = new OAuthToken();
163-
$token->setToken('mock_token_string');
164-
$this->tokenStorage->expects($this->once())
165-
->method('getToken')
166-
->will($this->returnValue($token))
174+
// expect the OAuth2 service to get the token from the request header,
175+
// flagging the authorization header to be removed at the same time
176+
$this->serverService->expects($this->once())
177+
->method('getBearerToken')
178+
->with(
179+
$this->isInstanceOf(Request::class),
180+
$this->equalTo(true)
181+
)
182+
->will($this->returnValue('mock_token_string'))
167183
;
168184

169-
// expect the OAuth2 service to verify the token, returning an access
170-
// token, but without a related user
171-
$accessToken = new AccessToken();
185+
// expect the OAuth2 service to not verify the token, throwing an exception
172186
$this->serverService->expects($this->once())
173187
->method('verifyAccessToken')
174188
->with('mock_token_string')
@@ -182,21 +196,26 @@ public function testAuthenticateTransformsOAuthServerException(): void
182196
))
183197
;
184198

185-
// expect the thrown exception to be transformed into an authentication exception
186-
$this->expectException(AuthenticationException::class);
187-
$this->expectExceptionMessage('OAuth2 authentication failed');
199+
// expect the null user value to not be processed
200+
$this->userChecker->expects($this->never())->method('checkPreAuth');
201+
202+
$passport = $this->authenticator->authenticate(new Request());
188203

189-
$this->authenticator->authenticate(new Request());
204+
// confirm that the returned passport won't pass validation
205+
$this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved());
190206
}
191207

192208
public function testAuthenticateTransformsAccountStatusException(): void
193209
{
194-
// expect a token from the token storage
195-
$token = new OAuthToken();
196-
$token->setToken('mock_token_string');
197-
$this->tokenStorage->expects($this->once())
198-
->method('getToken')
199-
->will($this->returnValue($token))
210+
// expect the OAuth2 service to get the token from the request header,
211+
// flagging the authorization header to be removed at the same time
212+
$this->serverService->expects($this->once())
213+
->method('getBearerToken')
214+
->with(
215+
$this->isInstanceOf(Request::class),
216+
$this->equalTo(true)
217+
)
218+
->will($this->returnValue('mock_token_string'))
200219
;
201220

202221
// expect the OAuth2 service to verify the token, returning an access token
@@ -216,11 +235,10 @@ public function testAuthenticateTransformsAccountStatusException(): void
216235
->willThrowException(new DisabledException('User account is disabled.'))
217236
;
218237

219-
// expect the thrown exception to be transformed into an authentication exception
220-
$this->expectException(AuthenticationException::class);
221-
$this->expectExceptionMessage('OAuth2 authentication failed');
238+
$passport = $this->authenticator->authenticate(new Request());
222239

223-
$this->authenticator->authenticate(new Request());
240+
// confirm that the returned passport won't pass validation
241+
$this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved());
224242
}
225243

226244
public function testCreateAuthenticatedTokenWithValidPassport(): void
@@ -245,6 +263,12 @@ public function testCreateAuthenticatedTokenWithValidPassport(): void
245263
->will($this->returnValue(['ROLE_USER']))
246264
;
247265

266+
// expect a new authenticated token to be stored
267+
$this->tokenStorage->expects($this->once())
268+
->method('setToken')
269+
->with($this->isInstanceOf(OAuthToken::class))
270+
;
271+
248272
// configure the passport
249273
$passport = new Passport(
250274
new UserBadge('test_user'),

‎Tests/Security/Authentication/Passport/OAuthCredentialsTest.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,12 @@ public function testMarkResolved(): void
6262

6363
$credentials->markResolved();
6464

65+
// marking credentials as resolved should not change any other state,
66+
// as the transported data is still needed for creating the
67+
// authenticated token when the AuthenticatorManager progresses in
68+
// executing the OAuthAuthenticator
6569
$this->assertTrue($credentials->isResolved());
66-
$this->assertNull($credentials->getTokenString());
67-
$this->assertSame([], $credentials->getRoles($this->user));
70+
$this->assertSame('mock_token', $credentials->getTokenString());
71+
$this->assertSame(['ROLE_SCOPE_1', 'ROLE_SCOPE_2'], $credentials->getRoles($this->user));
6872
}
6973
}

0 commit comments

Comments
 (0)