Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/Controller/AuthorizationController.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,42 @@ public function indexAction(Request $request): Response

$response = $this->server->completeAuthorizationRequest($authRequest, $serverResponse);
} catch (OAuthServerException $e) {
// https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1
// If the request fails due to a missing, invalid, or mismatching
// redirection URI, or if the client identifier is missing or invalid,
// the authorization server SHOULD inform the resource owner of the
// error and MUST NOT automatically redirect the user-agent to the
// invalid redirection URI.
//
// If the resource owner denies the access request or if the request
// fails for reasons other than a missing or invalid redirection URI,
// the authorization server informs the client by adding the following
// parameters to the query component of the redirection URI using the
// "application/x-www-form-urlencoded" format
//
// so if redirectUri is not already set, we try to set request redirect_uri params, fallback to first redirectUri of client
/** @psalm-suppress RiskyTruthyFalsyComparison !empty($e->getHint()),empty($e->getRedirectUri()) we really want to check null and empty */
if (!empty($client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use explicit checks instead of empty()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit checks now used on $client

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant everywhere in the patch. I try to avoid using empty() for the reasons mentioned here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok no problem, I understand
new commit pushed

&& ('invalid_client' === $e->getErrorType()
|| ('invalid_request' === $e->getErrorType() && !empty($e->getHint())
&& !\in_array(sscanf($e->getHint() ?? '', 'Check the `%s` parameter')[0] ?? null, ['client_id', 'client_secret', 'redirect_uri'])))
&& empty($e->getRedirectUri())) {
/** @var \League\Bundle\OAuth2ServerBundle\Model\ClientInterface $client */
$redirectUri = $request->query->get('redirect_uri', // query string has priority
(string)$request->request->get('redirect_uri', // then we check body to support POST request
$client->getRedirectUris()[0]?->__toString() ?? '')); // then first client redirect uri
if (!empty($redirectUri)) {
$e = new OAuthServerException(
$e->getMessage(),
$e->getCode(),
$e->getErrorType(),
$e->getHttpStatusCode(),
$e->getHint(),
$redirectUri,
$e->getPrevious(),
);
}
}
$response = $e->generateHttpResponse($serverResponse);
}

Expand Down
16 changes: 8 additions & 8 deletions tests/Acceptance/AuthorizationEndpointTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,15 @@ public function testAuthCodeRequestWithClientWhoIsNotAllowedToMakeARequestWithPl

$response = $this->client->getResponse();

$this->assertSame(400, $response->getStatusCode());

$this->assertSame('application/json', $response->headers->get('Content-Type'));

$jsonResponse = json_decode($response->getContent(), true);
$this->assertSame(302, $response->getStatusCode());
$redirectUri = $response->headers->get('Location');

$this->assertSame('invalid_request', $jsonResponse['error']);
$this->assertSame('The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.', $jsonResponse['error_description']);
$this->assertSame('Plain code challenge method is not allowed for this client', $jsonResponse['hint']);
$this->assertStringStartsWith(FixtureFactory::FIXTURE_CLIENT_FIRST_REDIRECT_URI, $redirectUri);
$query = [];
parse_str(parse_url($redirectUri, \PHP_URL_QUERY), $query);
$this->assertSame('invalid_request', $query['error']);
$this->assertSame('The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.', $query['error_description']);
$this->assertSame('Plain code challenge method is not allowed for this client', $query['hint']);
}

public function testAuthCodeRequestWithClientWhoIsAllowedToMakeARequestWithPlainCodeChallengeMethod(): void
Expand Down