From 83228bdcd52e47c1824cfcc394b74bed97eb10a0 Mon Sep 17 00:00:00 2001 From: Dave Marshall Date: Mon, 27 Mar 2017 12:11:25 +0100 Subject: [PATCH 1/9] Change case for implict grant token_type --- src/Grant/ImplicitGrant.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Grant/ImplicitGrant.php b/src/Grant/ImplicitGrant.php index 62a48147..466f32ce 100644 --- a/src/Grant/ImplicitGrant.php +++ b/src/Grant/ImplicitGrant.php @@ -200,7 +200,7 @@ class ImplicitGrant extends AbstractAuthorizeGrant $finalRedirectUri, [ 'access_token' => (string) $accessToken->convertToJWT($this->privateKey), - 'token_type' => 'bearer', + 'token_type' => 'Bearer', 'expires_in' => $accessToken->getExpiryDateTime()->getTimestamp() - (new \DateTime())->getTimestamp(), 'state' => $authorizationRequest->getState(), ], From 80fc8e654b6ab6ba66000ddd7b95f8d7203c2443 Mon Sep 17 00:00:00 2001 From: Alex Bilbie Date: Wed, 19 Jul 2017 07:57:47 +0100 Subject: [PATCH 2/9] Trigger E_USER_NOTICE instead of throwing an exception if key cannot be chmod to 600 --- src/CryptKey.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/CryptKey.php b/src/CryptKey.php index c707b223..557d6dc7 100644 --- a/src/CryptKey.php +++ b/src/CryptKey.php @@ -50,12 +50,13 @@ class CryptKey // Attempt to correct the permissions if (chmod($keyPath, 0600) === false) { // @codeCoverageIgnoreStart - throw new \LogicException( + trigger_error( sprintf( 'Key file "%s" permissions are not correct, should be 600 instead of %s, unable to automatically resolve the issue', $keyPath, $keyPathPerms - ) + ), + E_USER_NOTICE ); // @codeCoverageIgnoreEnd } From a1b8d87b473e9b956b80e7e20b31ae20dc670d30 Mon Sep 17 00:00:00 2001 From: Alex Bilbie Date: Wed, 19 Jul 2017 07:58:56 +0100 Subject: [PATCH 3/9] Updated changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca978bc7..2b532e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 6.0.1 (released 2017-07-19) + +To address feedback from the security release the following change has been made: + +* If an RSA key cannot be chmod'ed to 600 then it will now throw a E_USER_NOTICE instead of an exception. + ## 6.0.0 (released 2017-07-01) * Breaking change: The `AuthorizationServer` constructor now expects an encryption key string instead of a public key From ecc07abb3367e34c3090aa67212421dee9bac905 Mon Sep 17 00:00:00 2001 From: Benjamin Dieleman Date: Thu, 27 Jul 2017 17:27:36 +0200 Subject: [PATCH 4/9] Updated PHPDoc about the unicity violation exception throwing UniqueTokenIdentifierConstraintViolationException can be thrown when persisting tokens --- src/Repositories/AccessTokenRepositoryInterface.php | 3 +++ src/Repositories/AuthCodeRepositoryInterface.php | 3 +++ src/Repositories/RefreshTokenRepositoryInterface.php | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/Repositories/AccessTokenRepositoryInterface.php b/src/Repositories/AccessTokenRepositoryInterface.php index 04e98bc6..72ddf1f4 100644 --- a/src/Repositories/AccessTokenRepositoryInterface.php +++ b/src/Repositories/AccessTokenRepositoryInterface.php @@ -12,6 +12,7 @@ namespace League\OAuth2\Server\Repositories; use League\OAuth2\Server\Entities\AccessTokenEntityInterface; use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Entities\ScopeEntityInterface; +use League\OAuth2\Server\Exception\UniqueTokenIdentifierConstraintViolationException; /** * Access token interface. @@ -33,6 +34,8 @@ interface AccessTokenRepositoryInterface extends RepositoryInterface * Persists a new access token to permanent storage. * * @param AccessTokenEntityInterface $accessTokenEntity + * + * @throws UniqueTokenIdentifierConstraintViolationException */ public function persistNewAccessToken(AccessTokenEntityInterface $accessTokenEntity); diff --git a/src/Repositories/AuthCodeRepositoryInterface.php b/src/Repositories/AuthCodeRepositoryInterface.php index 4d23439a..2dc285b8 100644 --- a/src/Repositories/AuthCodeRepositoryInterface.php +++ b/src/Repositories/AuthCodeRepositoryInterface.php @@ -10,6 +10,7 @@ namespace League\OAuth2\Server\Repositories; use League\OAuth2\Server\Entities\AuthCodeEntityInterface; +use League\OAuth2\Server\Exception\UniqueTokenIdentifierConstraintViolationException; /** * Auth code storage interface. @@ -27,6 +28,8 @@ interface AuthCodeRepositoryInterface extends RepositoryInterface * Persists a new auth code to permanent storage. * * @param AuthCodeEntityInterface $authCodeEntity + * + * @throws UniqueTokenIdentifierConstraintViolationException */ public function persistNewAuthCode(AuthCodeEntityInterface $authCodeEntity); diff --git a/src/Repositories/RefreshTokenRepositoryInterface.php b/src/Repositories/RefreshTokenRepositoryInterface.php index d7e686cc..0c0697bf 100644 --- a/src/Repositories/RefreshTokenRepositoryInterface.php +++ b/src/Repositories/RefreshTokenRepositoryInterface.php @@ -10,6 +10,7 @@ namespace League\OAuth2\Server\Repositories; use League\OAuth2\Server\Entities\RefreshTokenEntityInterface; +use League\OAuth2\Server\Exception\UniqueTokenIdentifierConstraintViolationException; /** * Refresh token interface. @@ -27,6 +28,8 @@ interface RefreshTokenRepositoryInterface extends RepositoryInterface * Create a new refresh token_name. * * @param RefreshTokenEntityInterface $refreshTokenEntity + * + * @throws UniqueTokenIdentifierConstraintViolationException */ public function persistNewRefreshToken(RefreshTokenEntityInterface $refreshTokenEntity); From 0f1ddaaacf2018002f2f7840555e47982c200ac2 Mon Sep 17 00:00:00 2001 From: Mathieu Alorent Date: Sat, 29 Jul 2017 17:41:44 +0200 Subject: [PATCH 5/9] Fix #772 - PR should be based on master branch --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0391ae5e..af439e0a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,7 +1,7 @@ Thanks for contributing to this project. -**Please submit your pull request against the `develop` branch only.** +**Please submit your pull request against the `master` branch only.** Please ensure that you run `phpunit` from the project root after you've made any changes. From 79038ced785e3fc733408b6ba8a88be7d0c30b59 Mon Sep 17 00:00:00 2001 From: Hugo Hamon Date: Wed, 2 Aug 2017 17:55:11 +0200 Subject: [PATCH 6/9] [BC Break] Fixes invalid code challenge method payload key name I guess this change might be a BC break for existing and active authorization tokens when they're validated by the server. The good thing is that an authorization token has a very short expiration time and is used once to request an access token. --- src/Grant/AuthCodeGrant.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 594bc7ab..a138366f 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -312,14 +312,14 @@ class AuthCodeGrant extends AbstractAuthorizeGrant ); $payload = [ - 'client_id' => $authCode->getClient()->getIdentifier(), - 'redirect_uri' => $authCode->getRedirectUri(), - 'auth_code_id' => $authCode->getIdentifier(), - 'scopes' => $authCode->getScopes(), - 'user_id' => $authCode->getUserIdentifier(), - 'expire_time' => (new \DateTime())->add($this->authCodeTTL)->format('U'), - 'code_challenge' => $authorizationRequest->getCodeChallenge(), - 'code_challenge_method ' => $authorizationRequest->getCodeChallengeMethod(), + 'client_id' => $authCode->getClient()->getIdentifier(), + 'redirect_uri' => $authCode->getRedirectUri(), + 'auth_code_id' => $authCode->getIdentifier(), + 'scopes' => $authCode->getScopes(), + 'user_id' => $authCode->getUserIdentifier(), + 'expire_time' => (new \DateTime())->add($this->authCodeTTL)->format('U'), + 'code_challenge' => $authorizationRequest->getCodeChallenge(), + 'code_challenge_method' => $authorizationRequest->getCodeChallengeMethod(), ]; $response = new RedirectResponse(); From 2aca909d203e8a925da8c3a3f16a803683cecf04 Mon Sep 17 00:00:00 2001 From: Yannick de Lange Date: Tue, 1 Aug 2017 14:59:21 +0200 Subject: [PATCH 7/9] Removed chmod from CryptKey and add toggle to disable checking --- src/CryptKey.php | 27 +++++++++++---------------- tests/AuthorizationServerTest.php | 7 +++++++ 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/CryptKey.php b/src/CryptKey.php index 557d6dc7..2ede9e33 100644 --- a/src/CryptKey.php +++ b/src/CryptKey.php @@ -29,8 +29,9 @@ class CryptKey /** * @param string $keyPath * @param null|string $passPhrase + * @param bool $keyPermissionsCheck */ - public function __construct($keyPath, $passPhrase = null) + public function __construct($keyPath, $passPhrase = null, $keyPermissionsCheck = true) { if (preg_match(self::RSA_KEY_PATTERN, $keyPath)) { $keyPath = $this->saveKeyToFile($keyPath); @@ -44,21 +45,15 @@ class CryptKey throw new \LogicException(sprintf('Key path "%s" does not exist or is not readable', $keyPath)); } - // Verify the permissions of the key - $keyPathPerms = decoct(fileperms($keyPath) & 0777); - if ($keyPathPerms !== '600') { - // Attempt to correct the permissions - if (chmod($keyPath, 0600) === false) { - // @codeCoverageIgnoreStart - trigger_error( - sprintf( - 'Key file "%s" permissions are not correct, should be 600 instead of %s, unable to automatically resolve the issue', - $keyPath, - $keyPathPerms - ), - E_USER_NOTICE - ); - // @codeCoverageIgnoreEnd + if ($keyPermissionsCheck === true) { + // Verify the permissions of the key + $keyPathPerms = decoct(fileperms($keyPath) & 0777); + if (in_array($keyPathPerms, ['600', '660'], true) === false) { + trigger_error(sprintf( + 'Key file "%s" permissions are not correct, should be 600 or 660 instead of %s', + $keyPath, + $keyPathPerms + ), E_USER_NOTICE); } } diff --git a/tests/AuthorizationServerTest.php b/tests/AuthorizationServerTest.php index 7b921ff9..91ca9e4b 100644 --- a/tests/AuthorizationServerTest.php +++ b/tests/AuthorizationServerTest.php @@ -26,6 +26,13 @@ use Zend\Diactoros\ServerRequestFactory; class AuthorizationServerTest extends \PHPUnit_Framework_TestCase { + public function setUp() + { + // Make sure the keys have the correct permissions. + chmod(__DIR__ . '/Stubs/private.key', 0600); + chmod(__DIR__ . '/Stubs/public.key', 0600); + } + public function testRespondToRequestInvalidGrantType() { $server = new AuthorizationServer( From c86c7dde70cecc459cb3592f8a862389e3d841d0 Mon Sep 17 00:00:00 2001 From: Alex Bilbie Date: Thu, 3 Aug 2017 16:07:11 +0100 Subject: [PATCH 8/9] Fix #759 --- src/Exception/OAuthServerException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php index 6cd82bb3..45e03c07 100644 --- a/src/Exception/OAuthServerException.php +++ b/src/Exception/OAuthServerException.php @@ -152,7 +152,7 @@ class OAuthServerException extends \Exception */ public static function invalidRefreshToken($hint = null) { - return new static('The refresh token is invalid.', 8, 'invalid_request', 400, $hint); + return new static('The refresh token is invalid.', 8, 'invalid_request', 401, $hint); } /** From 925776958fc3f5278e74363663c20147af32b668 Mon Sep 17 00:00:00 2001 From: Alex Bilbie Date: Thu, 3 Aug 2017 16:09:23 +0100 Subject: [PATCH 9/9] Updated changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b532e9c..958a941e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 6.0.2 (released 2017-08-03) + +* An invalid refresh token that can't be decrypted now returns a HTTP 401 error instead of HTTP 400 (Issue #759) +* Removed chmod from CryptKey and add toggle to disable checking (Issue #776) +* Fixes invalid code challenge method payload key name (Issue #777) + ## 6.0.1 (released 2017-07-19) To address feedback from the security release the following change has been made: