[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 <[email protected]>

Author: hafezdivandari

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

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);
     }
 }

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);
     }
 
     /**

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('<comment>Here is your new client secret. This is the only time it will be shown so don\'t lose it!</comment>');
-            $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('<comment>Client ID</comment>', $client->getKey());
-        $this->components->twoColumnDetail('<comment>Client secret</comment>', $client->plainSecret);
+        $this->components->twoColumnDetail('Client ID', $client->getKey());
+        $this->components->twoColumnDetail('Client Secret', $client->plainSecret);
     }
 }

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;
                 }
 

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');
     }

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.
      *

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),
         ]);

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,
             ]
         );
 

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;
     }
 }

tests/Unit/BridgeClientRepositoryHashedSecretsTest.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;