Merge pull request #167 from trikoder/add-support-for-public-clients

Add full support for public clients for auth code grant (PKCE)
trikoder · Feb 24, 2020 · 8a71f55 · 8a71f55
2 parents 3c741ca + 0077c20
commit 8a71f55
Show file tree

Hide file tree

Showing 17 changed files with 377 additions and 24 deletions.
diff --git a/Command/CreateClientCommand.php b/Command/CreateClientCommand.php
@@ -4,6 +4,7 @@
 
 namespace Trikoder\Bundle\OAuth2Bundle\Command;
 
+use InvalidArgumentException;
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
@@ -67,13 +68,27 @@ protected function configure(): void
                 InputArgument::OPTIONAL,
                 'The client secret'
             )
+            ->addOption(
+                'public',
+                null,
+                InputOption::VALUE_NONE,
+                'Create a public client.'
+            )
         ;
     }
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
         $io = new SymfonyStyle($input, $output);
-        $client = $this->buildClientFromInput($input);
+
+        try {
+            $client = $this->buildClientFromInput($input);
+        } catch (InvalidArgumentException $exception) {
+            $io->error($exception->getMessage());
+
+            return 1;
+        }
+
         $this->clientManager->save($client);
         $io->success('New oAuth2 client created successfully.');
 
@@ -89,7 +104,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int
     private function buildClientFromInput(InputInterface $input): Client
     {
         $identifier = $input->getArgument('identifier') ?? hash('md5', random_bytes(16));
-        $secret = $input->getArgument('secret') ?? hash('sha512', random_bytes(32));
+
+        $isPublic = $input->getOption('public');
+
+        if (null !== $input->getArgument('secret') && $isPublic) {
+            throw new InvalidArgumentException('The client cannot have a secret and be public.');
+        }
+
+        $secret = $isPublic ? null : $input->getArgument('secret') ?? hash('sha512', random_bytes(32));
 
         $client = new Client($identifier, $secret);
         $client->setActive(true);

diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php
@@ -99,6 +99,10 @@ private function createAuthorizationServerNode(): NodeDefinition
                     ->info('Whether to enable the authorization code grant')
                     ->defaultTrue()
                 ->end()
+                ->booleanNode('require_code_challenge_for_public_clients')
+                    ->info('Whether to require code challenge for public clients for the auth code grant')
+                    ->defaultTrue()
+                ->end()
                 ->booleanNode('enable_implicit_grant')
                     ->info('Whether to enable the implicit grant')
                     ->defaultTrue()

diff --git a/DependencyInjection/TrikoderOAuth2Extension.php b/DependencyInjection/TrikoderOAuth2Extension.php
@@ -199,15 +199,17 @@ private function configureGrants(ContainerBuilder $container, array $config): vo
             ])
         ;
 
-        $container
-            ->getDefinition(AuthCodeGrant::class)
-            ->replaceArgument('$authCodeTTL', new Definition(DateInterval::class, [$config['auth_code_ttl']]))
-            ->addMethodCall('disableRequireCodeChallengeForPublicClients') // TODO: Make this grant option configurable
+        $authCodeGrantDefinition = $container->getDefinition(AuthCodeGrant::class);
+        $authCodeGrantDefinition->replaceArgument('$authCodeTTL', new Definition(DateInterval::class, [$config['auth_code_ttl']]))
             ->addMethodCall('setRefreshTokenTTL', [
                 new Definition(DateInterval::class, [$config['refresh_token_ttl']]),
             ])
         ;
 
+        if (false === $config['require_code_challenge_for_public_clients']) {
+            $authCodeGrantDefinition->addMethodCall('disableRequireCodeChallengeForPublicClients');
+        }
+
         $container
             ->getDefinition(ImplicitGrant::class)
             ->replaceArgument('$accessTokenTTL', new Definition(DateInterval::class, [$config['access_token_ttl']]))

diff --git a/League/Entity/Client.php b/League/Entity/Client.php
@@ -13,12 +13,6 @@ final class Client implements ClientEntityInterface
     use EntityTrait;
     use ClientTrait;
 
-    public function __construct()
-    {
-        // TODO: Add support for confidential clients
-        $this->isConfidential = true;
-    }
-
     /**
      * {@inheritdoc}
      */
@@ -34,4 +28,9 @@ public function setRedirectUri(array $redirectUri): void
     {
         $this->redirectUri = $redirectUri;
     }
+
+    public function setConfidential(bool $isConfidential): void
+    {
+        $this->isConfidential = $isConfidential;
+    }
 }
diff --git a/League/Repository/ClientRepository.php b/League/Repository/ClientRepository.php
@@ -54,11 +54,7 @@ public function validateClient($clientIdentifier, $clientSecret, $grantType)
             return false;
         }
 
-        if (null === $clientSecret) {
-            return true;
-        }
-
-        if (hash_equals($client->getSecret(), (string) $clientSecret)) {
+        if (!$client->isConfidential() || hash_equals($client->getSecret(), (string) $clientSecret)) {
             return true;
         }
 
@@ -70,6 +66,7 @@ private function buildClientEntity(ClientModel $client): ClientEntity
         $clientEntity = new ClientEntity();
         $clientEntity->setIdentifier($client->getIdentifier());
         $clientEntity->setRedirectUri(array_map('strval', $client->getRedirectUris()));
+        $clientEntity->setConfidential($client->isConfidential());
 
         return $clientEntity;
     }

diff --git a/Model/Client.php b/Model/Client.php
@@ -12,7 +12,7 @@ class Client
     private $identifier;
 
     /**
-     * @var string
+     * @var string|null
      */
     private $secret;
 
@@ -36,7 +36,7 @@ class Client
      */
     private $active = true;
 
-    public function __construct(string $identifier, string $secret)
+    public function __construct(string $identifier, ?string $secret)
     {
         $this->identifier = $identifier;
         $this->secret = $secret;
@@ -52,7 +52,7 @@ public function getIdentifier(): string
         return $this->identifier;
     }
 
-    public function getSecret(): string
+    public function getSecret(): ?string
     {
         return $this->secret;
     }
@@ -113,4 +113,9 @@ public function setActive(bool $active): self
 
         return $this;
     }
+
+    public function isConfidential(): bool
+    {
+        return !empty($this->secret);
+    }
 }
diff --git a/README.md b/README.md
@@ -85,6 +85,9 @@ This package is currently in the active development.
             # Whether to enable the authorization code grant
             enable_auth_code_grant: true
 
+            # Whether to require code challenge for public clients for the auth code grant
+            require_code_challenge_for_public_clients: true
+
             # Whether to enable the implicit grant
             enable_implicit_grant: true
         resource_server:      # Required

diff --git a/Resources/config/doctrine/model/Client.orm.xml b/Resources/config/doctrine/model/Client.orm.xml
@@ -4,7 +4,7 @@
                    https://raw.github.com/doctrine/doctrine2/master/doctrine-mapping.xsd">
     <entity name="Trikoder\Bundle\OAuth2Bundle\Model\Client" table="oauth2_client">
         <id name="identifier" type="string" length="32" />
-        <field name="secret" type="string" length="128" />
+        <field name="secret" type="string" length="128" nullable="true" />
         <field name="redirectUris" type="oauth2_redirect_uri" nullable="true" />
         <field name="grants" type="oauth2_grant" nullable="true" />
         <field name="scopes" type="oauth2_scope" nullable="true" />

diff --git a/Tests/Acceptance/AuthorizationEndpointTest.php b/Tests/Acceptance/AuthorizationEndpointTest.php
@@ -12,8 +12,10 @@
 use Trikoder\Bundle\OAuth2Bundle\Manager\ClientManagerInterface;
 use Trikoder\Bundle\OAuth2Bundle\Manager\RefreshTokenManagerInterface;
 use Trikoder\Bundle\OAuth2Bundle\Manager\ScopeManagerInterface;
+use Trikoder\Bundle\OAuth2Bundle\Model\AuthorizationCode;
 use Trikoder\Bundle\OAuth2Bundle\OAuth2Events;
 use Trikoder\Bundle\OAuth2Bundle\Tests\Fixtures\FixtureFactory;
+use Trikoder\Bundle\OAuth2Bundle\Tests\TestHelper;
 
 final class AuthorizationEndpointTest extends AbstractAcceptanceTest
 {
@@ -68,6 +70,117 @@ public function testSuccessfulCodeRequest(): void
         $this->assertEquals('foobar', $query['state']);
     }
 
+    public function testSuccessfulPKCEAuthCodeRequest(): void
+    {
+        $state = bin2hex(random_bytes(20));
+        $codeVerifier = bin2hex(random_bytes(64));
+        $codeChallengeMethod = 'S256';
+
+        $codeChallenge = strtr(
+            rtrim(base64_encode(hash('sha256', $codeVerifier, true)), '='),
+            '+/',
+            '-_'
+        );
+
+        $this->client
+            ->getContainer()
+            ->get('event_dispatcher')
+            ->addListener(OAuth2Events::AUTHORIZATION_REQUEST_RESOLVE, function (AuthorizationRequestResolveEvent $event) use ($state, $codeChallenge, $codeChallengeMethod): void {
+                $this->assertSame($state, $event->getState());
+                $this->assertSame($codeChallenge, $event->getCodeChallenge());
+                $this->assertSame($codeChallengeMethod, $event->getCodeChallengeMethod());
+
+                $event->resolveAuthorization(AuthorizationRequestResolveEvent::AUTHORIZATION_APPROVED);
+            });
+
+        timecop_freeze(new DateTimeImmutable());
+
+        try {
+            $this->client->request(
+                'GET',
+                '/authorize',
+                [
+                    'client_id' => FixtureFactory::FIXTURE_PUBLIC_CLIENT,
+                    'response_type' => 'code',
+                    'scope' => '',
+                    'state' => $state,
+                    'code_challenge' => $codeChallenge,
+                    'code_challenge_method' => $codeChallengeMethod,
+                ]
+            );
+        } finally {
+            timecop_return();
+        }
+
+        $response = $this->client->getResponse();
+
+        $this->assertSame(302, $response->getStatusCode());
+        $redirectUri = $response->headers->get('Location');
+
+        $this->assertStringStartsWith(FixtureFactory::FIXTURE_CLIENT_FIRST_REDIRECT_URI, $redirectUri);
+        $query = [];
+        parse_str(parse_url($redirectUri, PHP_URL_QUERY), $query);
+        $this->assertArrayHasKey('state', $query);
+        $this->assertSame($state, $query['state']);
+
+        $this->assertArrayHasKey('code', $query);
+        $payload = json_decode(TestHelper::decryptPayload($query['code']), true);
+
+        $this->assertArrayHasKey('code_challenge', $payload);
+        $this->assertArrayHasKey('code_challenge_method', $payload);
+        $this->assertSame($codeChallenge, $payload['code_challenge']);
+        $this->assertSame($codeChallengeMethod, $payload['code_challenge_method']);
+
+        /** @var AuthorizationCode|null $authCode */
+        $authCode = $this->client
+            ->getContainer()
+            ->get('doctrine.orm.entity_manager')
+            ->getRepository(AuthorizationCode::class)
+            ->findOneBy(['identifier' => $payload['auth_code_id']]);
+
+        $this->assertInstanceOf(AuthorizationCode::class, $authCode);
+        $this->assertSame(FixtureFactory::FIXTURE_PUBLIC_CLIENT, $authCode->getClient()->getIdentifier());
+    }
+
+    public function testAuthCodeRequestWithPublicClientWithoutCodeChallengeWhenTheChallengeIsRequiredForPublicClients(): void
+    {
+        $this->client
+            ->getContainer()
+            ->get('event_dispatcher')
+            ->addListener(OAuth2Events::AUTHORIZATION_REQUEST_RESOLVE, function (AuthorizationRequestResolveEvent $event): void {
+                $this->fail('This event should not have been dispatched.');
+            });
+
+        timecop_freeze(new DateTimeImmutable());
+
+        try {
+            $this->client->request(
+                'GET',
+                '/authorize',
+                [
+                    'client_id' => FixtureFactory::FIXTURE_PUBLIC_CLIENT,
+                    'response_type' => 'code',
+                    'scope' => '',
+                    'state' => bin2hex(random_bytes(20)),
+                ]
+            );
+        } finally {
+            timecop_return();
+        }
+
+        $response = $this->client->getResponse();
+
+        $this->assertSame(400, $response->getStatusCode());
+
+        $this->assertSame('application/json', $response->headers->get('Content-Type'));
+
+        $jsonResponse = json_decode($response->getContent(), true);
+
+        $this->assertSame('invalid_request', $jsonResponse['error']);
+        $this->assertSame('The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed.', $jsonResponse['message']);
+        $this->assertSame('Code challenge must be provided for public clients', $jsonResponse['hint']);
+    }
+
     public function testSuccessfulTokenRequest(): void
     {
         $this->client

diff --git a/Tests/Acceptance/CreateClientCommandTest.php b/Tests/Acceptance/CreateClientCommandTest.php
@@ -35,11 +35,66 @@ public function testCreateClientWithIdentifier(): void
         $this->assertStringContainsString('New oAuth2 client created successfully', $output);
         $this->assertStringContainsString('foobar', $output);
 
+        /** @var Client $client */
         $client = $this->client
             ->getContainer()
             ->get(ClientManagerInterface::class)
             ->find('foobar');
         $this->assertInstanceOf(Client::class, $client);
+        $this->assertTrue($client->isConfidential());
+        $this->assertNotEmpty($client->getSecret());
+    }
+
+    public function testCreatePublicClientWithIdentifier(): void
+    {
+        $clientIdentifier = 'foobar test';
+        $command = $this->application->find('trikoder:oauth2:create-client');
+        $commandTester = new CommandTester($command);
+        $commandTester->execute([
+            'command' => $command->getName(),
+            'identifier' => $clientIdentifier,
+            '--public' => true,
+        ]);
+
+        $this->assertSame(0, $commandTester->getStatusCode());
+
+        $output = $commandTester->getDisplay();
+        $this->assertStringContainsString('New oAuth2 client created successfully', $output);
+        $this->assertStringContainsString($clientIdentifier, $output);
+
+        /** @var Client $client */
+        $client = $this->client
+            ->getContainer()
+            ->get(ClientManagerInterface::class)
+            ->find($clientIdentifier);
+        $this->assertInstanceOf(Client::class, $client);
+        $this->assertFalse($client->isConfidential());
+        $this->assertNull($client->getSecret());
+    }
+
+    public function testCannotCreatePublicClientWithSecret(): void
+    {
+        $clientIdentifier = 'foobar test';
+        $command = $this->application->find('trikoder:oauth2:create-client');
+        $commandTester = new CommandTester($command);
+        $commandTester->execute([
+            'command' => $command->getName(),
+            'identifier' => $clientIdentifier,
+            'secret' => 'foo',
+            '--public' => true,
+        ]);
+
+        $this->assertSame(1, $commandTester->getStatusCode());
+
+        $output = $commandTester->getDisplay();
+        $this->assertStringContainsString('The client cannot have a secret and be public.', $output);
+        $this->assertStringNotContainsString($clientIdentifier, $output);
+
+        $client = $this->client
+            ->getContainer()
+            ->get(ClientManagerInterface::class)
+            ->find($clientIdentifier);
+        $this->assertNull($client);
     }
 
     public function testCreateClientWithSecret(): void
@@ -54,12 +109,15 @@ public function testCreateClientWithSecret(): void
 
         $output = $commandTester->getDisplay();
         $this->assertStringContainsString('New oAuth2 client created successfully', $output);
+
+        /** @var Client $client */
         $client = $this->client
             ->getContainer()
             ->get(ClientManagerInterface::class)
             ->find('foobar');
         $this->assertInstanceOf(Client::class, $client);
         $this->assertSame('quzbaz', $client->getSecret());
+        $this->assertTrue($client->isConfidential());
     }
 
     public function testCreateClientWithRedirectUris(): void