From 2b4974b697ca937d37f407c429d792c467af7ec5 Mon Sep 17 00:00:00 2001 From: sephster Date: Tue, 13 Nov 2018 18:18:07 +0000 Subject: [PATCH 1/7] Change to use invalid_grant --- src/Grant/PasswordGrant.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Grant/PasswordGrant.php b/src/Grant/PasswordGrant.php index 1d00998b..412ac117 100644 --- a/src/Grant/PasswordGrant.php +++ b/src/Grant/PasswordGrant.php @@ -81,11 +81,13 @@ class PasswordGrant extends AbstractGrant protected function validateUser(ServerRequestInterface $request, ClientEntityInterface $client) { $username = $this->getRequestParameter('username', $request); + if (is_null($username)) { throw OAuthServerException::invalidRequest('username'); } $password = $this->getRequestParameter('password', $request); + if (is_null($password)) { throw OAuthServerException::invalidRequest('password'); } @@ -96,10 +98,11 @@ class PasswordGrant extends AbstractGrant $this->getIdentifier(), $client ); + if ($user instanceof UserEntityInterface === false) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::USER_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidCredentials(); + throw OAuthServerException::invalidGrant(); } return $user; From 685dc6edea755e587675eae6bcfa43f8ec959d8f Mon Sep 17 00:00:00 2001 From: sephster Date: Tue, 13 Nov 2018 18:19:20 +0000 Subject: [PATCH 2/7] Update test --- tests/Grant/PasswordGrantTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Grant/PasswordGrantTest.php b/tests/Grant/PasswordGrantTest.php index c90a83db..378fee6f 100644 --- a/tests/Grant/PasswordGrantTest.php +++ b/tests/Grant/PasswordGrantTest.php @@ -145,6 +145,7 @@ class PasswordGrantTest extends TestCase /** * @expectedException \League\OAuth2\Server\Exception\OAuthServerException + * @expectedExceptionCode 10 */ public function testRespondToRequestBadCredentials() { From a93696271625a33ab60940d192e2f13dd38b7c11 Mon Sep 17 00:00:00 2001 From: sephster Date: Tue, 13 Nov 2018 18:27:03 +0000 Subject: [PATCH 3/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b8b6ecba..0cb7da30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `issueAccessToken()` in the Abstract Grant no longer sets access token client, user ID or scopes. These values should already have been set when calling `getNewToken()` (PR #919) - No longer need to enable PKCE with `enableCodeExchangeProof` flag. Any client sending a code challenge will initiate PKCE checks. (PR #938) - Function `getClientEntity()` no longer performs client validation (PR #938) +- Password Grant now returns an invalid_grant error instead of invalid_credentials if a user cannot be validated (PR #967) ### Removed - `enableCodeExchangeProof` flag (PR #938) From ec8a663a8188a32ecc44afba069a95bd5674632b Mon Sep 17 00:00:00 2001 From: Chris Tanaskoski Date: Thu, 29 Nov 2018 09:28:36 +0100 Subject: [PATCH 4/7] Added test for respondToAccessTokenRequest using Http Basic Auth for client credentials --- tests/Grant/AuthCodeGrantTest.php | 70 +++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 3ea7105a..ff051087 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -606,6 +606,76 @@ class AuthCodeGrantTest extends TestCase $this->assertInstanceOf(RefreshTokenEntityInterface::class, $response->getRefreshToken()); } + public function testRespondToAccessTokenRequestUsingHttpBasicAuth() + { + $client = new ClientEntity(); + $client->setIdentifier('foo'); + $client->setRedirectUri('http://foo/bar'); + $client->setConfidential(); + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + + $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); + $scopeEntity = new ScopeEntity(); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity); + $scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0); + + $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); + $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); + $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf(); + + $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); + $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf(); + $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); + + $grant = new AuthCodeGrant( + $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new \DateInterval('PT10M') + ); + $grant->setClientRepository($clientRepositoryMock); + $grant->setScopeRepository($scopeRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); + $grant->setEncryptionKey($this->cryptStub->getKey()); + $grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); + + $request = new ServerRequest( + [], + [], + null, + 'POST', + 'php://input', + [ + 'Authorization' => 'Basic Zm9vOmJhcg==', + ], + [], + [], + [ + 'grant_type' => 'authorization_code', + 'redirect_uri' => 'http://foo/bar', + 'code' => $this->cryptStub->doEncrypt( + json_encode( + [ + 'auth_code_id' => uniqid(), + 'client_id' => 'foo', + 'expire_time' => time() + 3600, + 'user_id' => 123, + 'scopes' => ['foo'], + 'redirect_uri' => 'http://foo/bar', + ] + ) + ), + ] + ); + + /** @var StubResponseType $response */ + $response = $grant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M')); + + $this->assertInstanceOf(AccessTokenEntityInterface::class, $response->getAccessToken()); + $this->assertInstanceOf(RefreshTokenEntityInterface::class, $response->getRefreshToken()); + } + public function testRespondToAccessTokenRequestForPublicClient() { $client = new ClientEntity(); From b6955a6c65c84e4b4c4faa360ae70ea9477ecd7c Mon Sep 17 00:00:00 2001 From: Chris Tanaskoski Date: Thu, 29 Nov 2018 09:33:12 +0100 Subject: [PATCH 5/7] Fixed respondToAccessTokenRequest such that it accepts client_id through request body and Http Basic Auth --- src/Grant/AbstractGrant.php | 33 ++++++++++++++++++++++++--------- src/Grant/AuthCodeGrant.php | 6 +----- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php index 4531a6a3..65ab16cf 100644 --- a/src/Grant/AbstractGrant.php +++ b/src/Grant/AbstractGrant.php @@ -171,15 +171,7 @@ abstract class AbstractGrant implements GrantTypeInterface */ protected function validateClient(ServerRequestInterface $request) { - list($basicAuthUser, $basicAuthPassword) = $this->getBasicAuthCredentials($request); - - $clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser); - - if (is_null($clientId)) { - throw OAuthServerException::invalidRequest('client_id'); - } - - $clientSecret = $this->getRequestParameter('client_secret', $request, $basicAuthPassword); + list($clientId, $clientSecret) = $this->getClientCredentials($request); if ($this->clientRepository->validateClient($clientId, $clientSecret, $this->getIdentifier()) === false) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); @@ -199,6 +191,29 @@ abstract class AbstractGrant implements GrantTypeInterface return $client; } + /** + * Gets the client credentials from the request from the request body or + * the Http Basic Authorization header + * + * @param ServerRequestInterface $request + * + * @return array + */ + protected function getClientCredentials(ServerRequestInterface $request) + { + list($basicAuthUser, $basicAuthPassword) = $this->getBasicAuthCredentials($request); + + $clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser); + + if (is_null($clientId)) { + throw OAuthServerException::invalidRequest('client_id'); + } + + $clientSecret = $this->getRequestParameter('client_secret', $request, $basicAuthPassword); + + return [$clientId, $clientSecret]; + } + /** * Validate redirectUri from the request. * If a redirect URI is provided ensure it matches what is pre-registered diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 974f8373..c340fe96 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -90,11 +90,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant ResponseTypeInterface $responseType, \DateInterval $accessTokenTTL ) { - $clientId = $this->getRequestParameter('client_id', $request, null); - - if ($clientId === null) { - throw OAuthServerException::invalidRequest('client_id'); - } + list($clientId) = $this->getClientCredentials($request); $client = $this->clientRepository->getClientEntity($clientId); From fd65bf9e544262949baa46fa2ad9ca2f4e75e787 Mon Sep 17 00:00:00 2001 From: sephster Date: Mon, 10 Dec 2018 22:51:58 +0000 Subject: [PATCH 6/7] Streamline tests --- tests/Grant/AuthCodeGrantTest.php | 42 ++++++++++--------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index ff051087..3abf232c 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -610,35 +610,30 @@ class AuthCodeGrantTest extends TestCase { $client = new ClientEntity(); $client->setIdentifier('foo'); - $client->setRedirectUri('http://foo/bar'); - $client->setConfidential(); $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); $clientRepositoryMock->method('getClientEntity')->willReturn($client); $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); - $scopeEntity = new ScopeEntity(); - $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(new ScopeEntity()); $scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0); $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); - $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf(); $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); - $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf(); $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); - $grant = new AuthCodeGrant( + $authCodeGrant = new AuthCodeGrant( $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), - $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + $refreshTokenRepositoryMock, new \DateInterval('PT10M') ); - $grant->setClientRepository($clientRepositoryMock); - $grant->setScopeRepository($scopeRepositoryMock); - $grant->setAccessTokenRepository($accessTokenRepositoryMock); - $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); - $grant->setEncryptionKey($this->cryptStub->getKey()); - $grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); + + $authCodeGrant->setClientRepository($clientRepositoryMock); + $authCodeGrant->setScopeRepository($scopeRepositoryMock); + $authCodeGrant->setAccessTokenRepository($accessTokenRepositoryMock); + $authCodeGrant->setEncryptionKey($this->cryptStub->getKey()); + $authCodeGrant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); $request = new ServerRequest( [], @@ -647,7 +642,7 @@ class AuthCodeGrantTest extends TestCase 'POST', 'php://input', [ - 'Authorization' => 'Basic Zm9vOmJhcg==', + //'Authorization' => 'Basic Zm9vOmJhcg==', ], [], [], @@ -670,7 +665,7 @@ class AuthCodeGrantTest extends TestCase ); /** @var StubResponseType $response */ - $response = $grant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M')); + $response = $authCodeGrant->respondToAccessTokenRequest($request, new StubResponseType(), new \DateInterval('PT10M')); $this->assertInstanceOf(AccessTokenEntityInterface::class, $response->getAccessToken()); $this->assertInstanceOf(RefreshTokenEntityInterface::class, $response->getRefreshToken()); @@ -1035,27 +1030,16 @@ class AuthCodeGrantTest extends TestCase public function testRespondToAccessTokenRequestExpiredCode() { - $client = new ClientEntity(); - $client->setIdentifier('foo'); - $client->setRedirectUri('http://foo/bar'); - $client->setConfidential(); $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); - $clientRepositoryMock->method('getClientEntity')->willReturn($client); - - $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); - $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf(); - - $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); - $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf(); + $clientRepositoryMock->method('getClientEntity')->willReturn(new ClientEntity()); $grant = new AuthCodeGrant( $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), new \DateInterval('PT10M') ); + $grant->setClientRepository($clientRepositoryMock); - $grant->setAccessTokenRepository($accessTokenRepositoryMock); - $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); $grant->setEncryptionKey($this->cryptStub->getKey()); $request = new ServerRequest( From 894724c45b903c1865143c2f672a1b21cb8dd07a Mon Sep 17 00:00:00 2001 From: sephster Date: Mon, 10 Dec 2018 23:01:45 +0000 Subject: [PATCH 7/7] Remove invalid commenting --- tests/Grant/AuthCodeGrantTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 3abf232c..c7f08a1a 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -642,7 +642,7 @@ class AuthCodeGrantTest extends TestCase 'POST', 'php://input', [ - //'Authorization' => 'Basic Zm9vOmJhcg==', + 'Authorization' => 'Basic Zm9vOmJhcg==', ], [], [],