Skip to content

Commit 238a787

Browse files
SephsterSephster
authored
Merge pull request #1298 from hafezdivandari/patch-1
Fix "state" parameter of error responses
2 parents 1d82315 + fbecb61 commit 238a787

File tree

6 files changed

+142
-42
lines changed

6 files changed

+142
-42
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
88
### Fixed
99
- Auto-generated event emitter is now persisted. Previously, a new emitter was generated every time (PR #1428)
1010
- Fixed bug where you could not omit a redirect uri even if one had not been specified during the auth request (PR #1428)
11+
- Fixed bug where "state" parameter wasn't present on `invalid_scope` error response and wasn't on fragment part of `access_denied` redirect URI on Implicit grant (PR #1298)
1112

1213
## [9.0.0] - released 2024-05-13
1314
### Added

‎src/Grant/AbstractAuthorizeGrant.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
namespace League\OAuth2\Server\Grant;
1616

17+
use League\OAuth2\Server\Entities\ClientEntityInterface;
1718
use League\OAuth2\Server\RequestTypes\AuthorizationRequest;
1819
use League\OAuth2\Server\RequestTypes\AuthorizationRequestInterface;
1920

@@ -35,4 +36,14 @@ protected function createAuthorizationRequest(): AuthorizationRequestInterface
3536
{
3637
return new AuthorizationRequest();
3738
}
39+
40+
/**
41+
* Get the client redirect URI.
42+
*/
43+
protected function getClientRedirectUri(ClientEntityInterface $client): string
44+
{
45+
return is_array($client->getRedirectUri())
46+
? $client->getRedirectUri()[0]
47+
: $client->getRedirectUri();
48+
}
3849
}

‎src/Grant/AuthCodeGrant.php‎

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -280,17 +280,16 @@ public function validateAuthorizationRequest(ServerRequestInterface $request): A
280280
throw OAuthServerException::invalidClient($request);
281281
}
282282

283-
$defaultClientRedirectUri = is_array($client->getRedirectUri())
284-
? $client->getRedirectUri()[0]
285-
: $client->getRedirectUri();
283+
$stateParameter = $this->getQueryStringParameter('state', $request);
286284

287285
$scopes = $this->validateScopes(
288286
$this->getQueryStringParameter('scope', $request, $this->defaultScope),
289-
$redirectUri ?? $defaultClientRedirectUri
287+
$this->makeRedirectUri(
288+
$redirectUri ?? $this->getClientRedirectUri($client),
289+
$stateParameter !== null ? ['state' => $stateParameter] : []
290+
)
290291
);
291292

292-
$stateParameter = $this->getQueryStringParameter('state', $request);
293-
294293
$authorizationRequest = $this->createAuthorizationRequest();
295294
$authorizationRequest->setGrantTypeId($this->getIdentifier());
296295
$authorizationRequest->setClient($client);
@@ -354,7 +353,7 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth
354353
}
355354

356355
$finalRedirectUri = $authorizationRequest->getRedirectUri()
357-
?? $this->getClientRedirectUri($authorizationRequest);
356+
?? $this->getClientRedirectUri($authorizationRequest->getClient());
358357

359358
// The user approved the client, redirect them back with an auth code
360359
if ($authorizationRequest->isAuthorizationApproved() === true) {
@@ -408,14 +407,4 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth
408407
)
409408
);
410409
}
411-
412-
/**
413-
* Get the client redirect URI if not set in the request.
414-
*/
415-
private function getClientRedirectUri(AuthorizationRequestInterface $authorizationRequest): string
416-
{
417-
return is_array($authorizationRequest->getClient()->getRedirectUri())
418-
? $authorizationRequest->getClient()->getRedirectUri()[0]
419-
: $authorizationRequest->getClient()->getRedirectUri();
420-
}
421410
}

‎src/Grant/ImplicitGrant.php‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -111,24 +111,24 @@ public function validateAuthorizationRequest(ServerRequestInterface $request): A
111111
if ($redirectUri !== null) {
112112
$this->validateRedirectUri($redirectUri, $client, $request);
113113
} elseif (
114-
is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1
115-
|| $client->getRedirectUri() === ''
114+
$client->getRedirectUri() === '' ||
115+
(is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1)
116116
) {
117117
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));
118118
throw OAuthServerException::invalidClient($request);
119-
} else {
120-
$redirectUri = is_array($client->getRedirectUri())
121-
? $client->getRedirectUri()[0]
122-
: $client->getRedirectUri();
123119
}
124120

121+
$stateParameter = $this->getQueryStringParameter('state', $request);
122+
125123
$scopes = $this->validateScopes(
126124
$this->getQueryStringParameter('scope', $request, $this->defaultScope),
127-
$redirectUri
125+
$this->makeRedirectUri(
126+
$redirectUri ?? $this->getClientRedirectUri($client),
127+
$stateParameter !== null ? ['state' => $stateParameter] : [],
128+
$this->queryDelimiter
129+
)
128130
);
129131

130-
$stateParameter = $this->getQueryStringParameter('state', $request);
131-
132132
$authorizationRequest = $this->createAuthorizationRequest();
133133
$authorizationRequest->setGrantTypeId($this->getIdentifier());
134134
$authorizationRequest->setClient($client);
@@ -152,11 +152,8 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth
152152
throw new LogicException('An instance of UserEntityInterface should be set on the AuthorizationRequest');
153153
}
154154

155-
$clientRegisteredRedirectUri = is_array($authorizationRequest->getClient()->getRedirectUri())
156-
? $authorizationRequest->getClient()->getRedirectUri()[0]
157-
: $authorizationRequest->getClient()->getRedirectUri();
158-
159-
$finalRedirectUri = $authorizationRequest->getRedirectUri() ?? $clientRegisteredRedirectUri;
155+
$finalRedirectUri = $authorizationRequest->getRedirectUri()
156+
?? $this->getClientRedirectUri($authorizationRequest->getClient());
160157

161158
// The user approved the client, redirect them back with an access token
162159
if ($authorizationRequest->isAuthorizationApproved() === true) {
@@ -199,7 +196,8 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth
199196
$finalRedirectUri,
200197
[
201198
'state' => $authorizationRequest->getState(),
202-
]
199+
],
200+
$this->queryDelimiter
203201
)
204202
);
205203
}

‎tests/Grant/AuthCodeGrantTest.php‎

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,50 @@ public function testValidateAuthorizationRequestInvalidCodeChallengeMethod(): vo
469469
$grant->validateAuthorizationRequest($request);
470470
}
471471

472+
public function testValidateAuthorizationRequestInvalidScopes(): void
473+
{
474+
$client = new ClientEntity();
475+
$client->setRedirectUri(self::REDIRECT_URI);
476+
$client->setConfidential();
477+
478+
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
479+
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
480+
481+
$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
482+
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(null);
483+
484+
$grant = new AuthCodeGrant(
485+
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
486+
$this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
487+
new DateInterval('PT10M')
488+
);
489+
490+
$grant->setClientRepository($clientRepositoryMock);
491+
$grant->setScopeRepository($scopeRepositoryMock);
492+
$grant->setDefaultScope(self::DEFAULT_SCOPE);
493+
494+
$request = (new ServerRequest())->withQueryParams([
495+
'response_type' => 'code',
496+
'client_id' => 'foo',
497+
'redirect_uri' => self::REDIRECT_URI,
498+
'scope' => 'foo',
499+
'state' => 'foo',
500+
]);
501+
502+
try {
503+
$grant->validateAuthorizationRequest($request);
504+
} catch (OAuthServerException $e) {
505+
self::assertSame(5, $e->getCode());
506+
self::assertSame('invalid_scope', $e->getErrorType());
507+
self::assertSame('https://foo/bar?state=foo', $e->getRedirectUri());
508+
509+
return;
510+
}
511+
512+
$this->expectException(OAuthServerException::class);
513+
$this->expectExceptionCode(5);
514+
}
515+
472516
public function testCompleteAuthorizationRequest(): void
473517
{
474518
$client = new ClientEntity();
@@ -529,6 +573,7 @@ public function testCompleteAuthorizationRequestDenied(): void
529573
$authRequest->setClient($client);
530574
$authRequest->setGrantTypeId('authorization_code');
531575
$authRequest->setUser(new UserEntity());
576+
$authRequest->setState('foo');
532577

533578
$authCodeRepository = $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock();
534579
$authCodeRepository->method('getNewAuthCode')->willReturn(new AuthCodeEntity());
@@ -540,10 +585,18 @@ public function testCompleteAuthorizationRequestDenied(): void
540585
);
541586
$grant->setEncryptionKey($this->cryptStub->getKey());
542587

588+
try {
589+
$grant->completeAuthorizationRequest($authRequest);
590+
} catch (OAuthServerException $e) {
591+
self::assertSame(9, $e->getCode());
592+
self::assertSame('access_denied', $e->getErrorType());
593+
self::assertSame('http://foo/bar?state=foo', $e->getRedirectUri());
594+
595+
return;
596+
}
597+
543598
$this->expectException(OAuthServerException::class);
544599
$this->expectExceptionCode(9);
545-
546-
$grant->completeAuthorizationRequest($authRequest);
547600
}
548601

549602
public function testRespondToAccessTokenRequest(): void

‎tests/Grant/ImplicitGrantTest.php‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ public function testValidateAuthorizationRequest(): void
9595
$grant->setDefaultScope(self::DEFAULT_SCOPE);
9696

9797
$request = (new ServerRequest())->withQueryParams([
98-
'response_type' => 'code',
98+
'response_type' => 'token',
9999
'client_id' => 'foo',
100100
'redirect_uri' => self::REDIRECT_URI,
101101
]);
@@ -120,7 +120,7 @@ public function testValidateAuthorizationRequestRedirectUriArray(): void
120120
$grant->setDefaultScope(self::DEFAULT_SCOPE);
121121

122122
$request = (new ServerRequest())->withQueryParams([
123-
'response_type' => 'code',
123+
'response_type' => 'token',
124124
'client_id' => 'foo',
125125
'redirect_uri' => self::REDIRECT_URI,
126126
]);
@@ -135,7 +135,7 @@ public function testValidateAuthorizationRequestMissingClientId(): void
135135
$grant = new ImplicitGrant(new DateInterval('PT10M'));
136136
$grant->setClientRepository($clientRepositoryMock);
137137

138-
$request = (new ServerRequest())->withQueryParams(['response_type' => 'code']);
138+
$request = (new ServerRequest())->withQueryParams(['response_type' => 'token']);
139139

140140
$this->expectException(OAuthServerException::class);
141141
$this->expectExceptionCode(3);
@@ -152,7 +152,7 @@ public function testValidateAuthorizationRequestInvalidClientId(): void
152152
$grant->setClientRepository($clientRepositoryMock);
153153

154154
$request = (new ServerRequest())->withQueryParams([
155-
'response_type' => 'code',
155+
'response_type' => 'token',
156156
'client_id' => 'foo',
157157
]);
158158

@@ -173,7 +173,7 @@ public function testValidateAuthorizationRequestBadRedirectUriString(): void
173173
$grant->setClientRepository($clientRepositoryMock);
174174

175175
$request = (new ServerRequest())->withQueryParams([
176-
'response_type' => 'code',
176+
'response_type' => 'token',
177177
'client_id' => 'foo',
178178
'redirect_uri' => 'http://bar',
179179
]);
@@ -195,7 +195,7 @@ public function testValidateAuthorizationRequestBadRedirectUriArray(): void
195195
$grant->setClientRepository($clientRepositoryMock);
196196

197197
$request = (new ServerRequest())->withQueryParams([
198-
'response_type' => 'code',
198+
'response_type' => 'token',
199199
'client_id' => 'foo',
200200
'redirect_uri' => 'http://bar',
201201
]);
@@ -206,6 +206,45 @@ public function testValidateAuthorizationRequestBadRedirectUriArray(): void
206206
$grant->validateAuthorizationRequest($request);
207207
}
208208

209+
public function testValidateAuthorizationRequestInvalidScopes(): void
210+
{
211+
$client = new ClientEntity();
212+
$client->setRedirectUri(self::REDIRECT_URI);
213+
214+
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
215+
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
216+
217+
$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
218+
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(null);
219+
220+
$grant = new ImplicitGrant(new DateInterval('PT10M'));
221+
222+
$grant->setClientRepository($clientRepositoryMock);
223+
$grant->setScopeRepository($scopeRepositoryMock);
224+
$grant->setDefaultScope(self::DEFAULT_SCOPE);
225+
226+
$request = (new ServerRequest())->withQueryParams([
227+
'response_type' => 'token',
228+
'client_id' => 'foo',
229+
'redirect_uri' => self::REDIRECT_URI,
230+
'scope' => 'foo',
231+
'state' => 'foo',
232+
]);
233+
234+
try {
235+
$grant->validateAuthorizationRequest($request);
236+
} catch (OAuthServerException $e) {
237+
self::assertSame(5, $e->getCode());
238+
self::assertSame('invalid_scope', $e->getErrorType());
239+
self::assertSame('https://foo/bar#state=foo', $e->getRedirectUri());
240+
241+
return;
242+
}
243+
244+
$this->expectException(OAuthServerException::class);
245+
$this->expectExceptionCode(5);
246+
}
247+
209248
public function testCompleteAuthorizationRequest(): void
210249
{
211250
$client = new ClientEntity();
@@ -248,6 +287,7 @@ public function testCompleteAuthorizationRequestDenied(): void
248287
$authRequest->setClient($client);
249288
$authRequest->setGrantTypeId('authorization_code');
250289
$authRequest->setUser(new UserEntity());
290+
$authRequest->setState('foo');
251291

252292
$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
253293
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
@@ -261,10 +301,18 @@ public function testCompleteAuthorizationRequestDenied(): void
261301
$grant->setAccessTokenRepository($accessTokenRepositoryMock);
262302
$grant->setScopeRepository($scopeRepositoryMock);
263303

304+
try {
305+
$grant->completeAuthorizationRequest($authRequest);
306+
} catch (OAuthServerException $e) {
307+
self::assertSame(9, $e->getCode());
308+
self::assertSame('access_denied', $e->getErrorType());
309+
self::assertSame('https://foo/bar#state=foo', $e->getRedirectUri());
310+
311+
return;
312+
}
313+
264314
$this->expectException(OAuthServerException::class);
265315
$this->expectExceptionCode(9);
266-
267-
$grant->completeAuthorizationRequest($authRequest);
268316
}
269317

270318
public function testAccessTokenRepositoryUniqueConstraintCheck(): void

0 commit comments

Comments
 (0)