From 6fa936e78917c38bc918fcd1cc11088d9cb41916 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 21 May 2024 19:34:32 +0330 Subject: [PATCH 1/5] always hash client secret --- src/Bridge/ClientRepository.php | 14 +++++---- src/Client.php | 10 ++----- src/Console/ClientCommand.php | 10 ++----- src/Console/HashCommand.php | 9 ++---- src/Http/Controllers/ClientController.php | 13 ++------- src/Passport.php | 19 ------------ src/PersonalAccessTokenFactory.php | 4 +-- tests/Feature/AccessTokenControllerTest.php | 12 ++++---- tests/Feature/Console/HashCommand.php | 9 ++---- ...ridgeClientRepositoryHashedSecretsTest.php | 29 ------------------- tests/Unit/BridgeClientRepositoryTest.php | 14 +++++---- tests/Unit/PersonalAccessTokenFactoryTest.php | 5 +--- 12 files changed, 36 insertions(+), 112 deletions(-) delete mode 100644 tests/Unit/BridgeClientRepositoryHashedSecretsTest.php diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php index 1c21ce623..0420808e4 100644 --- a/src/Bridge/ClientRepository.php +++ b/src/Bridge/ClientRepository.php @@ -2,9 +2,9 @@ namespace Laravel\Passport\Bridge; +use Illuminate\Contracts\Hashing\Hasher; use Laravel\Passport\Client as ClientModel; use Laravel\Passport\ClientRepository as ClientModelRepository; -use Laravel\Passport\Passport; use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Repositories\ClientRepositoryInterface; @@ -15,12 +15,18 @@ class ClientRepository implements ClientRepositoryInterface */ protected ClientModelRepository $clients; + /** + * The hasher implementation. + */ + protected Hasher $hasher; + /** * Create a new repository instance. */ - public function __construct(ClientModelRepository $clients) + public function __construct(ClientModelRepository $clients, Hasher $hasher) { $this->clients = $clients; + $this->hasher = $hasher; } /** @@ -83,8 +89,6 @@ protected function handlesGrant(ClientModel $record, string $grantType): bool */ protected function verifySecret(string $clientSecret, string $storedHash): bool { - return Passport::$hashesClientSecrets - ? password_verify($clientSecret, $storedHash) - : hash_equals($storedHash, $clientSecret); + return $this->hasher->check($clientSecret, $storedHash); } } diff --git a/src/Client.php b/src/Client.php index 16e9dfff4..6be9812e4 100644 --- a/src/Client.php +++ b/src/Client.php @@ -4,7 +4,7 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; -use Illuminate\Support\Str; +use Illuminate\Support\Facades\Hash; use Laravel\Passport\Database\Factories\ClientFactory; class Client extends Model @@ -129,13 +129,7 @@ public function setSecretAttribute($value) { $this->plainSecret = $value; - if (is_null($value) || ! Passport::$hashesClientSecrets) { - $this->attributes['secret'] = $value; - - return; - } - - $this->attributes['secret'] = password_hash($value, PASSWORD_BCRYPT); + $this->attributes['secret'] = is_null($value) ? $value : Hash::make($value); } /** diff --git a/src/Console/ClientCommand.php b/src/Console/ClientCommand.php index 58c52ed64..ee4ba91fd 100644 --- a/src/Console/ClientCommand.php +++ b/src/Console/ClientCommand.php @@ -5,7 +5,6 @@ use Illuminate\Console\Command; use Laravel\Passport\Client; use Laravel\Passport\ClientRepository; -use Laravel\Passport\Passport; use Symfony\Component\Console\Attribute\AsCommand; #[AsCommand(name: 'passport:client')] @@ -164,12 +163,9 @@ protected function createAuthCodeClient(ClientRepository $clients) */ protected function outputClientDetails(Client $client) { - if (Passport::$hashesClientSecrets) { - $this->line('Here is your new client secret. This is the only time it will be shown so don\'t lose it!'); - $this->line(''); - } + $this->components->warn('Here is your new client secret. This is the only time it will be shown so don\'t lose it!'); - $this->components->twoColumnDetail('Client ID', $client->getKey()); - $this->components->twoColumnDetail('Client secret', $client->plainSecret); + $this->components->twoColumnDetail('Client ID', $client->getKey()); + $this->components->twoColumnDetail('Client Secret', $client->plainSecret); } } diff --git a/src/Console/HashCommand.php b/src/Console/HashCommand.php index 4e55b1350..65cf62c32 100644 --- a/src/Console/HashCommand.php +++ b/src/Console/HashCommand.php @@ -3,6 +3,7 @@ namespace Laravel\Passport\Console; use Illuminate\Console\Command; +use Illuminate\Support\Facades\Hash; use Laravel\Passport\Passport; use Symfony\Component\Console\Attribute\AsCommand; @@ -30,17 +31,11 @@ class HashCommand extends Command */ public function handle() { - if (! Passport::$hashesClientSecrets) { - $this->components->warn('Please enable client hashing yet in your AppServiceProvider before continuing.'); - - return; - } - if ($this->option('force') || $this->confirm('Are you sure you want to hash all client secrets? This cannot be undone.')) { $model = Passport::clientModel(); foreach ((new $model)->whereNotNull('secret')->cursor() as $client) { - if (password_get_info($client->secret)['algo'] === PASSWORD_BCRYPT) { + if (Hash::isHashed($client->secret) && ! Hash::needsRehash($client->secret)) { continue; } diff --git a/src/Http/Controllers/ClientController.php b/src/Http/Controllers/ClientController.php index a36be44c1..acff352f9 100644 --- a/src/Http/Controllers/ClientController.php +++ b/src/Http/Controllers/ClientController.php @@ -7,7 +7,6 @@ use Illuminate\Http\Response; use Laravel\Passport\ClientRepository; use Laravel\Passport\Http\Rules\RedirectRule; -use Laravel\Passport\Passport; class ClientController { @@ -60,13 +59,7 @@ public function forUser(Request $request) { $userId = $request->user()->getAuthIdentifier(); - $clients = $this->clients->activeForUser($userId); - - if (Passport::$hashesClientSecrets) { - return $clients; - } - - return $clients->makeVisible('secret'); + return $this->clients->activeForUser($userId); } /** @@ -88,9 +81,7 @@ public function store(Request $request) null, false, false, (bool) $request->input('confidential', true) ); - if (Passport::$hashesClientSecrets) { - return ['plainSecret' => $client->plainSecret] + $client->toArray(); - } + $client->secret = $client->plainSecret; return $client->makeVisible('secret'); } diff --git a/src/Passport.php b/src/Passport.php index 728cc20d9..d24f768c5 100644 --- a/src/Passport.php +++ b/src/Passport.php @@ -156,13 +156,6 @@ class Passport */ public static $decryptsCookies = true; - /** - * Indicates if client secrets will be hashed. - * - * @var bool - */ - public static $hashesClientSecrets = false; - /** * The callback that should be used to generate JWT encryption keys. * @@ -651,18 +644,6 @@ public static function refreshToken() return new static::$refreshTokenModel; } - /** - * Configure Passport to hash client credential secrets. - * - * @return static - */ - public static function hashClientSecrets() - { - static::$hashesClientSecrets = true; - - return new static; - } - /** * Specify the callback that should be invoked to generate encryption keys for encrypting JWT tokens. * diff --git a/src/PersonalAccessTokenFactory.php b/src/PersonalAccessTokenFactory.php index 15ac840ad..7983c09d0 100644 --- a/src/PersonalAccessTokenFactory.php +++ b/src/PersonalAccessTokenFactory.php @@ -94,12 +94,10 @@ public function make($userId, $name, array $scopes = []) */ protected function createRequest($client, $userId, array $scopes) { - $secret = Passport::$hashesClientSecrets ? $this->clients->getPersonalAccessClientSecret() : $client->secret; - return (new ServerRequest('POST', 'not-important'))->withParsedBody([ 'grant_type' => 'personal_access', 'client_id' => $client->getKey(), - 'client_secret' => $secret, + 'client_secret' => $this->clients->getPersonalAccessClientSecret(), 'user_id' => $userId, 'scope' => implode(' ', $scopes), ]); diff --git a/tests/Feature/AccessTokenControllerTest.php b/tests/Feature/AccessTokenControllerTest.php index 215558e9a..b39cf7cc2 100644 --- a/tests/Feature/AccessTokenControllerTest.php +++ b/tests/Feature/AccessTokenControllerTest.php @@ -33,7 +33,7 @@ public function testGettingAccessTokenWithClientCredentialsGrant() [ 'grant_type' => 'client_credentials', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret, + 'client_secret' => $client->plainSecret, ] ); @@ -76,7 +76,7 @@ public function testGettingAccessTokenWithClientCredentialsGrantInvalidClientSec [ 'grant_type' => 'client_credentials', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret.'foo', + 'client_secret' => $client->plainSecret.'foo', ] ); @@ -120,7 +120,7 @@ public function testGettingAccessTokenWithPasswordGrant() [ 'grant_type' => 'password', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret, + 'client_secret' => $client->plainSecret, 'username' => $user->email, 'password' => $password, ] @@ -169,7 +169,7 @@ public function testGettingAccessTokenWithPasswordGrantWithInvalidPassword() [ 'grant_type' => 'password', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret, + 'client_secret' => $client->plainSecret, 'username' => $user->email, 'password' => $password.'foo', ] @@ -213,7 +213,7 @@ public function testGettingAccessTokenWithPasswordGrantWithInvalidClientSecret() [ 'grant_type' => 'password', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret.'foo', + 'client_secret' => $client->plainSecret.'foo', 'username' => $user->email, 'password' => $password, ] @@ -258,7 +258,7 @@ public function testGettingCustomResponseType() [ 'grant_type' => 'client_credentials', 'client_id' => $client->getKey(), - 'client_secret' => $client->secret, + 'client_secret' => $client->plainSecret, ] ); diff --git a/tests/Feature/Console/HashCommand.php b/tests/Feature/Console/HashCommand.php index fae29cab1..8494129b6 100644 --- a/tests/Feature/Console/HashCommand.php +++ b/tests/Feature/Console/HashCommand.php @@ -4,23 +4,18 @@ use Illuminate\Contracts\Hashing\Hasher; use Illuminate\Support\Str; -use Laravel\Passport\Client; -use Laravel\Passport\Passport; +use Laravel\Passport\Database\Factories\ClientFactory; use Laravel\Passport\Tests\Feature\PassportTestCase; class HashCommand extends PassportTestCase { public function test_it_can_properly_hash_client_secrets() { - $client = factory(Client::class)->create(['secret' => $secret = Str::random(40)]); + $client = ClientFactory::new()->create(['secret' => $secret = Str::random(40)]); $hasher = $this->app->make(Hasher::class); - Passport::hashClientSecrets(); - $this->artisan('passport:hash', ['--force' => true]); $this->assertTrue($hasher->check($secret, $client->refresh()->secret)); - - Passport::$hashesClientSecrets = false; } } diff --git a/tests/Unit/BridgeClientRepositoryHashedSecretsTest.php b/tests/Unit/BridgeClientRepositoryHashedSecretsTest.php deleted file mode 100644 index e94017977..000000000 --- a/tests/Unit/BridgeClientRepositoryHashedSecretsTest.php +++ /dev/null @@ -1,29 +0,0 @@ -shouldReceive('findActive') - ->with(1) - ->andReturn(new BridgeClientRepositoryHashedTestClientStub); - - $this->clientModelRepository = $clientModelRepository; - $this->repository = new BridgeClientRepository($clientModelRepository); - } -} - -class BridgeClientRepositoryHashedTestClientStub extends BridgeClientRepositoryTestClientStub -{ - public $secret = '$2y$10$WgqU4wQpfsARCIQk.nPSOOiNkrMpPVxQiLCFUt8comvQwh1z6WFMG'; -} diff --git a/tests/Unit/BridgeClientRepositoryTest.php b/tests/Unit/BridgeClientRepositoryTest.php index 4b8b678c5..18b8c2ffe 100644 --- a/tests/Unit/BridgeClientRepositoryTest.php +++ b/tests/Unit/BridgeClientRepositoryTest.php @@ -2,10 +2,10 @@ namespace Laravel\Passport\Tests\Unit; +use Illuminate\Contracts\Hashing\Hasher; use Laravel\Passport\Bridge\Client; use Laravel\Passport\Bridge\ClientRepository as BridgeClientRepository; use Laravel\Passport\ClientRepository; -use Laravel\Passport\Passport; use Mockery as m; use PHPUnit\Framework\TestCase; @@ -23,15 +23,17 @@ class BridgeClientRepositoryTest extends TestCase protected function setUp(): void { - Passport::$hashesClientSecrets = false; - $clientModelRepository = m::mock(ClientRepository::class); $clientModelRepository->shouldReceive('findActive') ->with(1) - ->andReturn(new BridgeClientRepositoryTestClientStub); + ->andReturn($client = new BridgeClientRepositoryTestClientStub); + + $hasher = m::mock(Hasher::class); + $hasher->shouldReceive('check')->with('secret', $client->secret)->andReturn(true); + $hasher->shouldReceive('check')->withAnyArgs()->andReturn(false); $this->clientModelRepository = $clientModelRepository; - $this->repository = new BridgeClientRepository($clientModelRepository); + $this->repository = new BridgeClientRepository($clientModelRepository, $hasher); } protected function tearDown(): void @@ -188,7 +190,7 @@ class BridgeClientRepositoryTestClientStub extends \Laravel\Passport\Client public $redirect = 'http://localhost'; - public $secret = 'secret'; + public $secret = '$2y$10$WgqU4wQpfsARCIQk.nPSOOiNkrMpPVxQiLCFUt8comvQwh1z6WFMG'; public $personal_access_client = false; diff --git a/tests/Unit/PersonalAccessTokenFactoryTest.php b/tests/Unit/PersonalAccessTokenFactoryTest.php index f20eaecef..01fb33edf 100644 --- a/tests/Unit/PersonalAccessTokenFactoryTest.php +++ b/tests/Unit/PersonalAccessTokenFactoryTest.php @@ -35,6 +35,7 @@ public function test_access_token_can_be_created() $factory = new PersonalAccessTokenFactory($server, $clients, $tokens, $jwt); $clients->shouldReceive('personalAccessClient')->andReturn($client = new PersonalAccessTokenFactoryTestClientStub); + $clients->shouldReceive('getPersonalAccessClientSecret')->andReturn('secret'); $server->shouldReceive('respondToAccessTokenRequest')->andReturn($response = m::mock(ResponseInterface::class)); $response->shouldReceive('getBody->__toString')->andReturn(json_encode([ 'access_token' => 'foo', @@ -61,13 +62,9 @@ public function test_access_token_can_be_created() class PersonalAccessTokenFactoryTestClientStub extends Client { public $id = 1; - - public $secret = 'something'; } class PersonalAccessTokenFactoryTestModelStub extends Token { public $id = 1; - - public $secret = 'something'; } From ea5a55223711277ff0463e88750ab967272d9bc7 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 21 May 2024 20:03:38 +0330 Subject: [PATCH 2/5] update upgrade guide --- UPGRADE.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index 6127c47ae..f2e7befcd 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -16,6 +16,16 @@ PR: https://github.com/laravel/passport/pull/1734 The `league/oauth2-server` Composer package which is utilized internally by Passport has been updated to 9.0, which adds additional types to method signatures. To ensure your application is compatible, you should review this package's complete [changelog](https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#900---released-2024-05-13). +### Client Secret + +PR: https://github.com/laravel/passport/pull/1745 + +Passport now always hashes clients' secret using `Hash` facade, you may call the following command if you need to rehash your clients' secrets: + + php artisan passport:hash + +Therefore, `Passport::$hashesClientSecrets` property and `Passport::hashClientSecrets()` method has been removed. + ## Upgrading To 12.0 From 11.x ### Migration Changes From 71201e015103cf0b5e1f69de78842e7e412e5021 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 21 May 2024 20:22:00 +0330 Subject: [PATCH 3/5] fix static analys --- src/Client.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Client.php b/src/Client.php index 6be9812e4..4fc509338 100644 --- a/src/Client.php +++ b/src/Client.php @@ -5,6 +5,7 @@ use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Str; use Laravel\Passport\Database\Factories\ClientFactory; class Client extends Model From 5dcff22d681bbd89814eec7633a7106271336b1e Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 21 May 2024 21:26:55 +0330 Subject: [PATCH 4/5] fix a bug on getting PAN client id --- src/PersonalAccessTokenFactory.php | 7 +++---- tests/Unit/PersonalAccessTokenFactoryTest.php | 8 +------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/PersonalAccessTokenFactory.php b/src/PersonalAccessTokenFactory.php index 7983c09d0..6e3cd2685 100644 --- a/src/PersonalAccessTokenFactory.php +++ b/src/PersonalAccessTokenFactory.php @@ -69,7 +69,7 @@ public function __construct(AuthorizationServer $server, public function make($userId, $name, array $scopes = []) { $response = $this->dispatchRequestToAuthorizationServer( - $this->createRequest($this->clients->personalAccessClient(), $userId, $scopes) + $this->createRequest($userId, $scopes) ); $token = tap($this->findAccessToken($response), function ($token) use ($userId, $name) { @@ -87,16 +87,15 @@ public function make($userId, $name, array $scopes = []) /** * Create a request instance for the given client. * - * @param \Laravel\Passport\Client $client * @param mixed $userId * @param array $scopes * @return \Psr\Http\Message\ServerRequestInterface */ - protected function createRequest($client, $userId, array $scopes) + protected function createRequest($userId, array $scopes) { return (new ServerRequest('POST', 'not-important'))->withParsedBody([ 'grant_type' => 'personal_access', - 'client_id' => $client->getKey(), + 'client_id' => $this->clients->getPersonalAccessClientId(), 'client_secret' => $this->clients->getPersonalAccessClientSecret(), 'user_id' => $userId, 'scope' => implode(' ', $scopes), diff --git a/tests/Unit/PersonalAccessTokenFactoryTest.php b/tests/Unit/PersonalAccessTokenFactoryTest.php index 01fb33edf..2c55353d1 100644 --- a/tests/Unit/PersonalAccessTokenFactoryTest.php +++ b/tests/Unit/PersonalAccessTokenFactoryTest.php @@ -2,7 +2,6 @@ namespace Laravel\Passport\Tests\Unit; -use Laravel\Passport\Client; use Laravel\Passport\ClientRepository; use Laravel\Passport\PersonalAccessTokenFactory; use Laravel\Passport\PersonalAccessTokenResult; @@ -34,7 +33,7 @@ public function test_access_token_can_be_created() $factory = new PersonalAccessTokenFactory($server, $clients, $tokens, $jwt); - $clients->shouldReceive('personalAccessClient')->andReturn($client = new PersonalAccessTokenFactoryTestClientStub); + $clients->shouldReceive('getPersonalAccessClientId')->andReturn('1'); $clients->shouldReceive('getPersonalAccessClientSecret')->andReturn('secret'); $server->shouldReceive('respondToAccessTokenRequest')->andReturn($response = m::mock(ResponseInterface::class)); $response->shouldReceive('getBody->__toString')->andReturn(json_encode([ @@ -59,11 +58,6 @@ public function test_access_token_can_be_created() } } -class PersonalAccessTokenFactoryTestClientStub extends Client -{ - public $id = 1; -} - class PersonalAccessTokenFactoryTestModelStub extends Token { public $id = 1; From 175db459ec9674316588ca8df061a2dd1b03733f Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Thu, 30 May 2024 09:53:13 -0500 Subject: [PATCH 5/5] Update UPGRADE.md --- UPGRADE.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index f2e7befcd..eed1fb255 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -16,15 +16,15 @@ PR: https://github.com/laravel/passport/pull/1734 The `league/oauth2-server` Composer package which is utilized internally by Passport has been updated to 9.0, which adds additional types to method signatures. To ensure your application is compatible, you should review this package's complete [changelog](https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#900---released-2024-05-13). -### Client Secret +### Client Secrets Hashed by Default PR: https://github.com/laravel/passport/pull/1745 -Passport now always hashes clients' secret using `Hash` facade, you may call the following command if you need to rehash your clients' secrets: +Passport now always hashes client secrets using Laravel's `Hash` facade. If you are currently storing your client secrets in plain text, you may invoke the `passport:hash` Artisan command to hash all of your existing client secrets: php artisan passport:hash -Therefore, `Passport::$hashesClientSecrets` property and `Passport::hashClientSecrets()` method has been removed. +In light of this change, the `Passport::$hashesClientSecrets` property and `Passport::hashClientSecrets()` method has been removed. ## Upgrading To 12.0 From 11.x