[13.x] Improve issuing PATs (#1780)

* improve issuing pat * improve tests * upgrade guide * Update UPGRADE.md * Update ClientRepository.php --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Aug 19, 2024 · 8375604 · 8375604
1 parent 61644b3
commit 8375604
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 129 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -53,13 +53,13 @@ When authenticating users via bearer tokens, the `User` model's `token` method n
 
 ### Personal Access Client Table and Model Removal
 
-PR: https://github.com/laravel/passport/pull/1749
+PR: https://github.com/laravel/passport/pull/1749, https://github.com/laravel/passport/pull/1780
 
 Passport's `oauth_personal_access_clients` table has been redundant and unnecessary for several release cycles. Therefore, this release of Passport no longer interacts with this table or its corresponding model. If you wish, you may create a migration that drops this table:
 
     Schema::drop('oauth_personal_access_clients');
 
-In addition, the `Laravel\Passport\PersonalAccessClient` model, `Passport::$personalAccessClientModel` property, `Passport::usePersonalAccessClientModel()`, `Passport::personalAccessClientModel()`, and `Passport::personalAccessClient()` methods have been removed.
+In addition, the `passport.personal_access_client` configuration value, `Laravel\Passport\PersonalAccessClient` model, `Passport::$personalAccessClientModel` property, `Passport::usePersonalAccessClientModel()`, `Passport::personalAccessClientModel()`, and `Passport::personalAccessClient()` methods have been removed.
 
 ## Upgrading To 12.0 From 11.x
 

diff --git a/config/passport.php b/config/passport.php
@@ -43,20 +43,4 @@
 
     'connection' => env('PASSPORT_CONNECTION'),
 
-    /*
-    |--------------------------------------------------------------------------
-    | Personal Access Client
-    |--------------------------------------------------------------------------
-    |
-    | If you enable client hashing, you should set the personal access client
-    | ID and unhashed secret within your environment file. The values will
-    | get used while issuing fresh personal access tokens to your users.
-    |
-    */
-
-    'personal_access_client' => [
-        'id' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_ID'),
-        'secret' => env('PASSPORT_PERSONAL_ACCESS_CLIENT_SECRET'),
-    ],
-
 ];
diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php
@@ -36,17 +36,7 @@ public function getClientEntity(string $clientIdentifier): ?ClientEntityInterfac
     {
         $record = $this->clients->findActive($clientIdentifier);
 
-        if (! $record) {
-            return null;
-        }
-
-        return new Client(
-            $clientIdentifier,
-            $record->name,
-            $record->redirect_uris,
-            $record->confidential(),
-            $record->provider
-        );
+        return $record ? $this->fromClientModel($record) : null;
     }
 
     /**
@@ -81,4 +71,28 @@ protected function verifySecret(string $clientSecret, string $storedHash): bool
     {
         return $this->hasher->check($clientSecret, $storedHash);
     }
+
+    /**
+     * Get the personal access client for the given provider.
+     */
+    public function getPersonalAccessClientEntity(string $provider): ?ClientEntityInterface
+    {
+        return $this->fromClientModel(
+            $this->clients->personalAccessClient($provider)
+        );
+    }
+
+    /**
+     * Create a new client entity from the given client model instance.
+     */
+    protected function fromClientModel(ClientModel $model): ClientEntityInterface
+    {
+        return new Client(
+            $model->getKey(),
+            $model->name,
+            $model->redirect_uris,
+            $model->confidential(),
+            $model->provider
+        );
+    }
 }
diff --git a/src/Bridge/PersonalAccessGrant.php b/src/Bridge/PersonalAccessGrant.php
@@ -5,7 +5,6 @@
 use DateInterval;
 use League\OAuth2\Server\Exception\OAuthServerException;
 use League\OAuth2\Server\Grant\AbstractGrant;
-use League\OAuth2\Server\RequestEvent;
 use League\OAuth2\Server\ResponseTypes\ResponseTypeInterface;
 use Psr\Http\Message\ServerRequestInterface;
 
@@ -20,20 +19,17 @@ public function respondToAccessTokenRequest(
         DateInterval $accessTokenTTL
     ): ResponseTypeInterface {
         // Validate request
-        $client = $this->validateClient($request);
-
-        if (! $client->isConfidential()) {
-            $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));
+        if (! $userIdentifier = $this->getRequestParameter('user_id', $request)) {
+            throw OAuthServerException::invalidRequest('user_id');
+        }
 
-            throw OAuthServerException::invalidClient($request);
+        if (! $provider = $this->getRequestParameter('provider', $request)) {
+            throw OAuthServerException::invalidRequest('provider');
         }
 
-        $scopes = $this->validateScopes($this->getRequestParameter('scope', $request, $this->defaultScope));
-        $userIdentifier = $this->getRequestParameter('user_id', $request);
+        $client = $this->clientRepository->getPersonalAccessClientEntity($provider);
 
-        if (! $userIdentifier) {
-            throw OAuthServerException::invalidRequest('user_id');
-        }
+        $scopes = $this->validateScopes($this->getRequestParameter('scope', $request, $this->defaultScope));
 
         // Finalize the requested scopes
         $scopes = $this->scopeRepository->finalizeScopes(

diff --git a/src/ClientRepository.php b/src/ClientRepository.php
@@ -4,6 +4,7 @@
 
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Support\Str;
+use RuntimeException;
 
 class ClientRepository
 {
@@ -76,6 +77,29 @@ public function activeForUser($userId)
         })->values();
     }
 
+    /*
+     * Get the latest active personal access client for the given user provider.
+     *
+     * @throws \RuntimeException
+     */
+    public function personalAccessClient(string $provider): Client
+    {
+        return Passport::client()
+            ->where('revoked', false)
+            ->whereNull('user_id')
+            ->where(function ($query) use ($provider) {
+                $query->when($provider === config('auth.guards.api.provider'), function ($query) {
+                    $query->orWhereNull('provider');
+                })->orWhere('provider', $provider);
+            })
+            ->latest()
+            ->get()
+            ->first(fn (Client $client) => $client->hasGrantType('personal_access'))
+            ?? throw new RuntimeException(
+                "Personal access client not found for '$provider' user provider. Please create one."
+            );
+    }
+
     /**
      * Store a new client.
      *

diff --git a/src/Console/ClientCommand.php b/src/Console/ClientCommand.php
@@ -68,19 +68,13 @@ protected function createPersonalAccessClient(ClientRepository $clients)
 
         $provider = $this->option('provider') ?: $this->choice(
             'Which user provider should this client use to retrieve users?',
-            array_keys(config('auth.providers')),
+            collect(config('auth.guards'))->where('driver', 'passport')->pluck('provider')->all(),
             config('auth.guards.api.provider')
         );
 
-        $client = $clients->createPersonalAccessGrantClient($name, $provider);
+        $clients->createPersonalAccessGrantClient($name, $provider);
 
         $this->components->info('Personal access client created successfully.');
-
-        if (! config('passport.personal_access_client')) {
-            $this->components->info('Next, define the `PASSPORT_PERSONAL_ACCESS_CLIENT_ID` and `PASSPORT_PERSONAL_ACCESS_CLIENT_SECRET` environment variables using the values below.');
-        }
-
-        $this->outputClientDetails($client);
     }
 
     /**
@@ -98,7 +92,7 @@ protected function createPasswordClient(ClientRepository $clients)
 
         $provider = $this->option('provider') ?: $this->choice(
             'Which user provider should this client use to retrieve users?',
-            array_keys(config('auth.providers')),
+            collect(config('auth.guards'))->where('driver', 'passport')->pluck('provider')->all(),
             config('auth.guards.api.provider')
         );
 

diff --git a/src/PersonalAccessTokenFactory.php b/src/PersonalAccessTokenFactory.php
@@ -7,7 +7,6 @@
 use Nyholm\Psr7\Response;
 use Nyholm\Psr7\ServerRequest;
 use Psr\Http\Message\ServerRequestInterface;
-use RuntimeException;
 
 class PersonalAccessTokenFactory
 {
@@ -18,13 +17,6 @@ class PersonalAccessTokenFactory
      */
     protected $server;
 
-    /**
-     * The client repository instance.
-     *
-     * @var \Laravel\Passport\ClientRepository
-     */
-    protected $clients;
-
     /**
      * The token repository instance.
      *
@@ -43,20 +35,17 @@ class PersonalAccessTokenFactory
      * Create a new personal access token factory instance.
      *
      * @param  \League\OAuth2\Server\AuthorizationServer  $server
-     * @param  \Laravel\Passport\ClientRepository  $clients
      * @param  \Laravel\Passport\TokenRepository  $tokens
      * @param  \Lcobucci\JWT\Parser  $jwt
      * @return void
      */
     public function __construct(AuthorizationServer $server,
-                                ClientRepository $clients,
                                 TokenRepository $tokens,
                                 JwtParser $jwt)
     {
         $this->jwt = $jwt;
         $this->tokens = $tokens;
         $this->server = $server;
-        $this->clients = $clients;
     }
 
     /**
@@ -96,20 +85,9 @@ public function make($userId, string $name, array $scopes, string $provider)
      */
     protected function createRequest($userId, array $scopes, string $provider)
     {
-        $config = config("passport.personal_access_client.$provider", config('passport.personal_access_client'));
-
-        $client = isset($config['id']) ? $this->clients->findActive($config['id']) : null;
-
-        if (! $client || ($client->provider && $client->provider !== $provider)) {
-            throw new RuntimeException(
-                'Personal access client not found. Please create one and set the `PASSPORT_PERSONAL_ACCESS_CLIENT_ID` and `PASSPORT_PERSONAL_ACCESS_CLIENT_SECRET` environment variables.'
-            );
-        }
-
         return (new ServerRequest('POST', 'not-important'))->withParsedBody([
             'grant_type' => 'personal_access',
-            'client_id' => $config['id'] ?? null,
-            'client_secret' => $config['secret'] ?? null,
+            'provider' => $provider,
             'user_id' => $userId,
             'scope' => implode(' ', $scopes),
         ]);

diff --git a/tests/Feature/AccessTokenControllerTest.php b/tests/Feature/AccessTokenControllerTest.php
@@ -269,49 +269,6 @@ public function testGettingCustomResponseType()
         $this->assertArrayHasKey('id_token', $decodedResponse);
         $this->assertSame('foo_bar_open_id_token', $decodedResponse['id_token']);
     }
-
-    public function testPersonalAccessTokenRequestIsDisabled()
-    {
-        $user = UserFactory::new()->create([
-            'email' => '[email protected]',
-            'password' => $this->app->make(Hasher::class)->make('foobar123'),
-        ]);
-
-        /** @var Client $client */
-        $client = ClientFactory::new()->asPersonalAccessTokenClient()->create();
-
-        config([
-            'passport.personal_access_client.id' => $client->getKey(),
-            'passport.personal_access_client.secret' => $client->plainSecret,
-        ]);
-
-        $response = $this->post(
-            '/oauth/token',
-            [
-                'grant_type' => 'personal_access',
-                'client_id' => $client->getKey(),
-                'client_secret' => $client->plainSecret,
-                'user_id' => $user->getKey(),
-                'scope' => '',
-            ]
-        );
-
-        $response->assertStatus(400);
-
-        $decodedResponse = $response->decodeResponseJson()->json();
-
-        $this->assertArrayNotHasKey('token_type', $decodedResponse);
-        $this->assertArrayNotHasKey('expires_in', $decodedResponse);
-        $this->assertArrayNotHasKey('access_token', $decodedResponse);
-
-        $this->assertArrayHasKey('error', $decodedResponse);
-        $this->assertSame('unsupported_grant_type', $decodedResponse['error']);
-        $this->assertArrayHasKey('error_description', $decodedResponse);
-
-        $token = $user->createToken('test');
-
-        $this->assertInstanceOf(\Laravel\Passport\PersonalAccessTokenResult::class, $token);
-    }
 }
 
 class IdTokenResponse extends \League\OAuth2\Server\ResponseTypes\BearerTokenResponse

diff --git a/...eature/PersonalAccessTokenFactoryTest.php → tests/Feature/PersonalAccessGrantTest.php b/...eature/PersonalAccessTokenFactoryTest.php → tests/Feature/PersonalAccessGrantTest.php
@@ -2,7 +2,6 @@
 
 namespace Laravel\Passport\Tests\Feature;
 
-use Illuminate\Contracts\Hashing\Hasher;
 use Illuminate\Foundation\Auth\User as Authenticatable;
 use Illuminate\Support\Facades\DB;
 use Laravel\Passport\Client;
@@ -13,25 +12,17 @@
 use Orchestra\Testbench\Concerns\WithLaravelMigrations;
 use Workbench\Database\Factories\UserFactory;
 
-class PersonalAccessTokenFactoryTest extends PassportTestCase
+class PersonalAccessGrantTest extends PassportTestCase
 {
     use WithLaravelMigrations;
 
     public function testIssueToken()
     {
-        $user = UserFactory::new()->create([
-            'email' => '[email protected]',
-            'password' => $this->app->make(Hasher::class)->make('foobar123'),
-        ]);
+        $user = UserFactory::new()->create();
 
         /** @var Client $client */
         $client = ClientFactory::new()->asPersonalAccessTokenClient()->create();
 
-        config([
-            'passport.personal_access_client.id' => $client->getKey(),
-            'passport.personal_access_client.secret' => $client->plainSecret,
-        ]);
-
         Passport::tokensCan([
             'foo' => 'Do foo',
             'bar' => 'Do bar',
@@ -56,9 +47,6 @@ public function testIssueTokenWithDifferentProviders()
             'auth.guards.api-admins' => ['driver' => 'passport', 'provider' => 'admins'],
             'auth.providers.customers' => ['driver' => 'eloquent', 'model' => CustomerProviderStub::class],
             'auth.guards.api-customers' => ['driver' => 'passport', 'provider' => 'customers'],
-            'passport.personal_access_client' => ['id' => $client->getKey(), 'secret' => $client->plainSecret],
-            'passport.personal_access_client.admins' => ['id' => $adminClient->getKey(), 'secret' => $adminClient->plainSecret],
-            'passport.personal_access_client.customers' => ['id' => $customerClient->getKey(), 'secret' => $customerClient->plainSecret],
         ]);
 
         $user = UserFactory::new()->create();
@@ -97,6 +85,28 @@ public function testIssueTokenWithDifferentProviders()
         $this->assertEquals([$adminToken->token->id], $adminTokens);
         $this->assertEquals([$customerToken->token->id], $customerTokens);
     }
+
+    public function testPersonalAccessTokenRequestIsDisabled()
+    {
+        $user = UserFactory::new()->create();
+        $client = ClientFactory::new()->asPersonalAccessTokenClient()->create();
+
+        $response = $this->post('/oauth/token', [
+            'grant_type' => 'personal_access',
+            'provider' => $user->getProvider(),
+            'user_id' => $user->getKey(),
+            'scope' => '',
+        ]);
+
+        $response->assertStatus(400);
+        $json = $response->json();
+
+        $this->assertSame('unsupported_grant_type', $json['error']);
+        $this->assertArrayHasKey('error_description', $json);
+        $this->assertArrayNotHasKey('access_token', $json);
+
+        $this->assertInstanceOf(PersonalAccessTokenResult::class, $user->createToken('test'));
+    }
 }
 
 class AdminProviderStub extends Authenticatable

diff --git a/tests/Unit/BridgeClientRepositoryTest.php b/tests/Unit/BridgeClientRepositoryTest.php
@@ -210,6 +210,7 @@ public function test_without_grant_types()
 class BridgeClientRepositoryTestClientStub extends \Laravel\Passport\Client
 {
     protected $attributes = [
+        'id' => 1,
         'name' => 'Client',
         'redirect_uris' => '["http://localhost"]',
         'secret' => '$2y$10$WgqU4wQpfsARCIQk.nPSOOiNkrMpPVxQiLCFUt8comvQwh1z6WFMG',