From c4c354e2df792be41a9674344c7700ee716bb19b Mon Sep 17 00:00:00 2001 From: sephster Date: Mon, 1 Jul 2019 19:17:43 +0100 Subject: [PATCH] Fix phpstan issues --- phpstan.neon | 2 -- .../BearerTokenValidator.php | 2 +- src/CryptTrait.php | 18 +++++++--- src/Entities/Traits/AccessTokenTrait.php | 2 +- src/Exception/OAuthServerException.php | 4 ++- src/Grant/AbstractGrant.php | 2 +- src/Grant/AuthCodeGrant.php | 32 ++++++++++------- .../ClientRepositoryInterface.php | 2 +- src/Repositories/ScopeRepositoryInterface.php | 2 +- src/Repositories/UserRepositoryInterface.php | 2 +- src/RequestTypes/AuthorizationRequest.php | 2 +- src/ResponseTypes/BearerTokenResponse.php | 35 +++++++++++-------- tests/Exception/OAuthServerExceptionTest.php | 4 ++- tests/Grant/ImplicitGrantTest.php | 6 ++-- tests/Utils/CryptKeyTest.php | 10 ++++++ 15 files changed, 79 insertions(+), 46 deletions(-) diff --git a/phpstan.neon b/phpstan.neon index 5cd9d80d..ba1fb491 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,8 +1,6 @@ includes: - vendor/phpstan/phpstan-phpunit/extension.neon - vendor/phpstan/phpstan-phpunit/rules.neon - - vendor/phpstan/phpstan-phpunit/strictRules.neon - - vendor/phpstan/phpstan-strict-rules/rules.neon services: - class: LeagueTests\PHPStan\AbstractGrantExtension diff --git a/src/AuthorizationValidators/BearerTokenValidator.php b/src/AuthorizationValidators/BearerTokenValidator.php index b2035ccc..7218f413 100644 --- a/src/AuthorizationValidators/BearerTokenValidator.php +++ b/src/AuthorizationValidators/BearerTokenValidator.php @@ -63,7 +63,7 @@ class BearerTokenValidator implements AuthorizationValidatorInterface } $header = $request->getHeader('authorization'); - $jwt = trim(preg_replace('/^(?:\s+)?Bearer\s/', '', $header[0])); + $jwt = trim((string) preg_replace('/^(?:\s+)?Bearer\s/', '', $header[0])); try { // Attempt to parse and validate the JWT diff --git a/src/CryptTrait.php b/src/CryptTrait.php index 1196e9dc..5709940d 100644 --- a/src/CryptTrait.php +++ b/src/CryptTrait.php @@ -19,7 +19,7 @@ use LogicException; trait CryptTrait { /** - * @var string|Key + * @var string|Key|null */ protected $encryptionKey; @@ -39,9 +39,13 @@ trait CryptTrait return Crypto::encrypt($unencryptedData, $this->encryptionKey); } - return Crypto::encryptWithPassword($unencryptedData, $this->encryptionKey); + if (is_string($this->encryptionKey)) { + return Crypto::encryptWithPassword($unencryptedData, $this->encryptionKey); + } + + throw new LogicException('Encryption key not set when attempting to encrypt'); } catch (Exception $e) { - throw new LogicException($e->getMessage(), null, $e); + throw new LogicException($e->getMessage(), 0, $e); } } @@ -61,9 +65,13 @@ trait CryptTrait return Crypto::decrypt($encryptedData, $this->encryptionKey); } - return Crypto::decryptWithPassword($encryptedData, $this->encryptionKey); + if (is_string($this->encryptionKey)) { + return Crypto::decryptWithPassword($encryptedData, $this->encryptionKey); + } + + throw new LogicException('Encryption key not set when attempting to decrypt'); } catch (Exception $e) { - throw new LogicException($e->getMessage(), null, $e); + throw new LogicException($e->getMessage(), 0, $e); } } diff --git a/src/Entities/Traits/AccessTokenTrait.php b/src/Entities/Traits/AccessTokenTrait.php index 872e8c18..e9757264 100644 --- a/src/Entities/Traits/AccessTokenTrait.php +++ b/src/Entities/Traits/AccessTokenTrait.php @@ -48,7 +48,7 @@ trait AccessTokenTrait ->setIssuedAt(time()) ->setNotBefore(time()) ->setExpiration($this->getExpiryDateTime()->getTimestamp()) - ->setSubject($this->getUserIdentifier()) + ->setSubject((string) $this->getUserIdentifier()) ->set('scopes', $this->getScopes()) ->sign(new Sha256(), new Key($privateKey->getKeyPath(), $privateKey->getPassPhrase())) ->getToken(); diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 3c3c7129..8e628baa 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -308,7 +308,9 @@ class OAuthServerException extends Exception $response = $response->withHeader($header, $content); } - $response->getBody()->write(json_encode($payload, $jsonOptions)); + $responseBody = json_encode($payload, $jsonOptions) ?: 'JSON encoding of payload failed'; + + $response->getBody()->write($responseBody); return $response->withStatus($this->getHttpStatusCode()); } diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php index aed0e6c7..0ac9e395 100644 --- a/src/Grant/AbstractGrant.php +++ b/src/Grant/AbstractGrant.php @@ -185,7 +185,7 @@ abstract class AbstractGrant implements GrantTypeInterface throw OAuthServerException::invalidClient($request); } - $client = $this->clientRepository->getClientEntity($clientId); + $client = $this->getClientEntityOrFail($clientId, $request); // If a redirect URI is provided ensure it matches what is pre-registered $redirectUri = $this->getRequestParameter('redirect_uri', $request, null); diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index a11828d7..f85a0898 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -142,19 +142,21 @@ class AuthCodeGrant extends AbstractAuthorizeGrant ); } - if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) { - $codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method]; + if (property_exists($authCodePayload, 'code_challenge_method')) { + if (isset($this->codeChallengeVerifiers[$authCodePayload->code_challenge_method])) { + $codeChallengeVerifier = $this->codeChallengeVerifiers[$authCodePayload->code_challenge_method]; - if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) { - throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.'); + if ($codeChallengeVerifier->verifyCodeChallenge($codeVerifier, $authCodePayload->code_challenge) === false) { + throw OAuthServerException::invalidGrant('Failed to verify `code_verifier`.'); + } + } else { + throw OAuthServerException::serverError( + sprintf( + 'Unsupported code challenge method `%s`', + $authCodePayload->code_challenge_method + ) + ); } - } else { - throw OAuthServerException::serverError( - sprintf( - 'Unsupported code challenge method `%s`', - $authCodePayload->code_challenge_method - ) - ); } } @@ -351,12 +353,18 @@ class AuthCodeGrant extends AbstractAuthorizeGrant 'code_challenge_method' => $authorizationRequest->getCodeChallengeMethod(), ]; + $jsonPayload = json_encode($payload); + + if ($jsonPayload === false) { + throw new LogicException('An error was encountered when JSON encoding the authorization request response'); + } + $response = new RedirectResponse(); $response->setRedirectUri( $this->makeRedirectUri( $finalRedirectUri, [ - 'code' => $this->encrypt(json_encode($payload)), + 'code' => $this->encrypt($jsonPayload), 'state' => $authorizationRequest->getState(), ] ) diff --git a/src/Repositories/ClientRepositoryInterface.php b/src/Repositories/ClientRepositoryInterface.php index 47b6083a..7eef494f 100644 --- a/src/Repositories/ClientRepositoryInterface.php +++ b/src/Repositories/ClientRepositoryInterface.php @@ -21,7 +21,7 @@ interface ClientRepositoryInterface extends RepositoryInterface * * @param string $clientIdentifier The client's identifier * - * @return ClientEntityInterface + * @return ClientEntityInterface|null */ public function getClientEntity($clientIdentifier); diff --git a/src/Repositories/ScopeRepositoryInterface.php b/src/Repositories/ScopeRepositoryInterface.php index 52db05de..997aac2c 100644 --- a/src/Repositories/ScopeRepositoryInterface.php +++ b/src/Repositories/ScopeRepositoryInterface.php @@ -22,7 +22,7 @@ interface ScopeRepositoryInterface extends RepositoryInterface * * @param string $identifier The scope identifier * - * @return ScopeEntityInterface + * @return ScopeEntityInterface|null */ public function getScopeEntityByIdentifier($identifier); diff --git a/src/Repositories/UserRepositoryInterface.php b/src/Repositories/UserRepositoryInterface.php index 0a9efef0..8ad49aa7 100644 --- a/src/Repositories/UserRepositoryInterface.php +++ b/src/Repositories/UserRepositoryInterface.php @@ -22,7 +22,7 @@ interface UserRepositoryInterface extends RepositoryInterface * @param string $grantType The grant type used * @param ClientEntityInterface $clientEntity * - * @return UserEntityInterface + * @return UserEntityInterface|null */ public function getUserEntityByUserCredentials( $username, diff --git a/src/RequestTypes/AuthorizationRequest.php b/src/RequestTypes/AuthorizationRequest.php index 5faa45d4..6441e144 100644 --- a/src/RequestTypes/AuthorizationRequest.php +++ b/src/RequestTypes/AuthorizationRequest.php @@ -111,7 +111,7 @@ class AuthorizationRequest } /** - * @return UserEntityInterface + * @return UserEntityInterface|null */ public function getUser() { diff --git a/src/ResponseTypes/BearerTokenResponse.php b/src/ResponseTypes/BearerTokenResponse.php index ddcadd63..41a10170 100644 --- a/src/ResponseTypes/BearerTokenResponse.php +++ b/src/ResponseTypes/BearerTokenResponse.php @@ -14,6 +14,7 @@ namespace League\OAuth2\Server\ResponseTypes; use League\OAuth2\Server\Entities\AccessTokenEntityInterface; use League\OAuth2\Server\Entities\RefreshTokenEntityInterface; use Psr\Http\Message\ResponseInterface; +use \LogicException; class BearerTokenResponse extends AbstractResponseType { @@ -31,23 +32,27 @@ class BearerTokenResponse extends AbstractResponseType ]; if ($this->refreshToken instanceof RefreshTokenEntityInterface) { - $refreshToken = $this->encrypt( - json_encode( - [ - 'client_id' => $this->accessToken->getClient()->getIdentifier(), - 'refresh_token_id' => $this->refreshToken->getIdentifier(), - 'access_token_id' => $this->accessToken->getIdentifier(), - 'scopes' => $this->accessToken->getScopes(), - 'user_id' => $this->accessToken->getUserIdentifier(), - 'expire_time' => $this->refreshToken->getExpiryDateTime()->getTimestamp(), - ] - ) - ); + $refreshTokenPayload = json_encode([ + 'client_id' => $this->accessToken->getClient()->getIdentifier(), + 'refresh_token_id' => $this->refreshToken->getIdentifier(), + 'access_token_id' => $this->accessToken->getIdentifier(), + 'scopes' => $this->accessToken->getScopes(), + 'user_id' => $this->accessToken->getUserIdentifier(), + 'expire_time' => $this->refreshToken->getExpiryDateTime()->getTimestamp(), + ]); - $responseParams['refresh_token'] = $refreshToken; + if ($refreshTokenPayload === false) { + throw new LogicException('Error encountered JSON encoding the refresh token payload'); + } + + $responseParams['refresh_token'] = $this->encrypt($refreshTokenPayload); } - $responseParams = array_merge($this->getExtraParams($this->accessToken), $responseParams); + $responseParams = json_encode(array_merge($this->getExtraParams($this->accessToken), $responseParams)); + + if ($responseParams === false) { + throw new LogicException('Error encountered JSON encoding response parameters'); + } $response = $response ->withStatus(200) @@ -55,7 +60,7 @@ class BearerTokenResponse extends AbstractResponseType ->withHeader('cache-control', 'no-store') ->withHeader('content-type', 'application/json; charset=UTF-8'); - $response->getBody()->write(json_encode($responseParams)); + $response->getBody()->write($responseParams); return $response; } diff --git a/tests/Exception/OAuthServerExceptionTest.php b/tests/Exception/OAuthServerExceptionTest.php index 82ecef02..eb2b2ad3 100644 --- a/tests/Exception/OAuthServerExceptionTest.php +++ b/tests/Exception/OAuthServerExceptionTest.php @@ -85,7 +85,9 @@ class OAuthServerExceptionTest extends TestCase $previous = new Exception('This is the previous'); $exceptionWithPrevious = OAuthServerException::accessDenied(null, null, $previous); - $this->assertSame('This is the previous', $exceptionWithPrevious->getPrevious()->getMessage()); + $previousMessage = $exceptionWithPrevious->getPrevious() !== null ? $exceptionWithPrevious->getPrevious()->getMessage() : null; + + $this->assertSame('This is the previous', $previousMessage); } public function testDoesNotHavePrevious() diff --git a/tests/Grant/ImplicitGrantTest.php b/tests/Grant/ImplicitGrantTest.php index 558923b5..e9523669 100644 --- a/tests/Grant/ImplicitGrantTest.php +++ b/tests/Grant/ImplicitGrantTest.php @@ -273,7 +273,7 @@ class ImplicitGrantTest extends TestCase $accessToken = new AccessTokenEntity(); $accessToken->setClient($client); - /** @var AccessTokenRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject $accessTokenRepositoryMock */ + /** @var AccessTokenRepositoryInterface|\PHPUnit\Framework\MockObject\MockObject $accessTokenRepositoryMock */ $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); $accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken); $accessTokenRepositoryMock->expects($this->at(0))->method('persistNewAccessToken')->willThrowException(UniqueTokenIdentifierConstraintViolationException::create()); @@ -298,7 +298,7 @@ class ImplicitGrantTest extends TestCase $authRequest->setGrantTypeId('authorization_code'); $authRequest->setUser(new UserEntity()); - /** @var AccessTokenRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject $accessTokenRepositoryMock */ + /** @var AccessTokenRepositoryInterface|\PHPUnit\Framework\MockObject\MockObject $accessTokenRepositoryMock */ $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); $accessTokenRepositoryMock->method('persistNewAccessToken')->willThrowException(OAuthServerException::serverError('something bad happened')); @@ -325,7 +325,7 @@ class ImplicitGrantTest extends TestCase $authRequest->setGrantTypeId('authorization_code'); $authRequest->setUser(new UserEntity()); - /** @var AccessTokenRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject $accessTokenRepositoryMock */ + /** @var AccessTokenRepositoryInterface|\PHPUnit\Framework\MockObject\MockObject $accessTokenRepositoryMock */ $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); $accessTokenRepositoryMock->method('persistNewAccessToken')->willThrowException(UniqueTokenIdentifierConstraintViolationException::create()); diff --git a/tests/Utils/CryptKeyTest.php b/tests/Utils/CryptKeyTest.php index 1fe79d20..e9799c08 100644 --- a/tests/Utils/CryptKeyTest.php +++ b/tests/Utils/CryptKeyTest.php @@ -26,6 +26,11 @@ class CryptKeyTest extends TestCase public function testKeyFileCreation() { $keyContent = file_get_contents(__DIR__ . '/../Stubs/public.key'); + + if (!is_string($keyContent)) { + $this->fail('The public key stub is not a string'); + } + $key = new CryptKey($keyContent); $this->assertEquals( @@ -34,6 +39,11 @@ class CryptKeyTest extends TestCase ); $keyContent = file_get_contents(__DIR__ . '/../Stubs/private.key.crlf'); + + if (!is_string($keyContent)) { + $this->fail('The private key (crlf) stub is not a string'); + } + $key = new CryptKey($keyContent); $this->assertEquals(