From 2e44a19c3f9c2eebd55bbc13a532cbfcef65880f Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Thu, 30 May 2024 18:23:42 +0330 Subject: [PATCH] [13.x] Always hash client secret (#1745) * always hash client secret * update upgrade guide * fix static analys * fix a bug on getting PAN client id * Update UPGRADE.md --------- Co-authored-by: Taylor Otwell --- UPGRADE.md | 10 +++++++ src/Bridge/ClientRepository.php | 14 +++++---- src/Client.php | 9 ++---- src/Console/ClientCommand.php | 10 ++----- src/Console/HashCommand.php | 9 ++---- src/Http/Controllers/ClientController.php | 13 ++------- src/Passport.php | 19 ------------ src/PersonalAccessTokenFactory.php | 11 +++---- tests/Feature/AccessTokenControllerTest.php | 12 ++++---- tests/Feature/Console/HashCommand.php | 9 ++---- ...ridgeClientRepositoryHashedSecretsTest.php | 29 ------------------- tests/Unit/BridgeClientRepositoryTest.php | 14 +++++---- tests/Unit/PersonalAccessTokenFactoryTest.php | 13 ++------- 13 files changed, 50 insertions(+), 122 deletions(-) delete mode 100644 tests/Unit/BridgeClientRepositoryHashedSecretsTest.php diff --git a/UPGRADE.md b/UPGRADE.md index 6127c47ae..eed1fb255 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 Secrets Hashed by Default + +PR: https://github.com/laravel/passport/pull/1745 + +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 + +In light of this change, the `Passport::$hashesClientSecrets` property and `Passport::hashClientSecrets()` method has been removed. + ## Upgrading To 12.0 From 11.x ### Migration Changes 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..4fc509338 100644 --- a/src/Client.php +++ b/src/Client.php @@ -4,6 +4,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; @@ -129,13 +130,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..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,19 +87,16 @@ 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) { - $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_id' => $this->clients->getPersonalAccessClientId(), + '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..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,8 @@ 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([ 'access_token' => 'foo', @@ -58,16 +58,7 @@ 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'; }