diff --git a/composer.json b/composer.json index 54af5b45..a4597df7 100644 --- a/composer.json +++ b/composer.json @@ -21,11 +21,12 @@ "league/oauth2-server": "^9.2", "nyholm/psr7": "^1.4", "psr/http-factory": "^1.0", - "symfony/deprecation-contracts": "^3", + "symfony/deprecation-contracts": "^3.0", "symfony/event-dispatcher": "^6.4|^7.0|^8.0", "symfony/filesystem": "^6.4|^7.0|^8.0", "symfony/framework-bundle": "^6.4|^7.0|^8.0", "symfony/psr-http-message-bridge": "^6.4|^7.0|^8.0", + "symfony/password-hasher": "^6.4|^7.0|^8.0", "symfony/security-bundle": "^6.4|^7.0|^8.0" }, "require-dev": { diff --git a/config/services.php b/config/services.php index be416c4f..e9b8dc98 100644 --- a/config/services.php +++ b/config/services.php @@ -13,6 +13,7 @@ use League\Bundle\OAuth2ServerBundle\Command\DeleteClientCommand; use League\Bundle\OAuth2ServerBundle\Command\GenerateKeyPairCommand; use League\Bundle\OAuth2ServerBundle\Command\ListClientsCommand; +use League\Bundle\OAuth2ServerBundle\Command\RehashClientSecretsCommand; use League\Bundle\OAuth2ServerBundle\Command\UpdateClientCommand; use League\Bundle\OAuth2ServerBundle\Controller\AuthorizationController; use League\Bundle\OAuth2ServerBundle\Controller\TokenController; @@ -59,6 +60,7 @@ use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher; return static function (ContainerConfigurator $container): void { $container->services() @@ -67,6 +69,7 @@ ->set('league.oauth2_server.repository.client', ClientRepository::class) ->args([ service(ClientManagerInterface::class), + service('league.oauth2_server.password_hasher'), ]) ->alias(ClientRepositoryInterface::class, 'league.oauth2_server.repository.client') ->alias(ClientRepository::class, 'league.oauth2_server.repository.client') @@ -233,6 +236,7 @@ ->args([ service(ClientManagerInterface::class), null, + service('league.oauth2_server.password_hasher'), ]) ->tag('console.command', ['command' => 'league:oauth2-server:create-client']) ->alias(CreateClientCommand::class, 'league.oauth2_server.command.create_client') @@ -277,6 +281,14 @@ ->tag('console.command', ['command' => 'league:oauth2-server:generate-keypair']) ->alias(GenerateKeyPairCommand::class, 'league.oauth2_server.command.generate_keypair') + ->set('league.oauth2_server.command.rehash_client_secrets', RehashClientSecretsCommand::class) + ->args([ + service(ClientManagerInterface::class), + service('league.oauth2_server.password_hasher'), + ]) + ->tag('console.command', ['command' => 'league:oauth2-server:rehash-client-secrets']) + ->alias(RehashClientSecretsCommand::class, 'league.oauth2_server.command.rehash_client_secrets') + // Utility services ->set('league.oauth2_server.converter.user', UserConverter::class) ->alias(UserConverterInterface::class, 'league.oauth2_server.converter.user') @@ -321,5 +333,8 @@ ]) ->set('league.oauth2_server.factory.http_foundation', HttpFoundationFactory::class) + + // Password hasher for client secrets + ->set('league.oauth2_server.password_hasher', NativePasswordHasher::class) ; }; diff --git a/src/Command/CreateClientCommand.php b/src/Command/CreateClientCommand.php index c8fed82f..7f3d7cdf 100644 --- a/src/Command/CreateClientCommand.php +++ b/src/Command/CreateClientCommand.php @@ -17,6 +17,7 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; +use Symfony\Component\PasswordHasher\PasswordHasherInterface; #[AsCommand(name: 'league:oauth2-server:create-client', description: 'Creates a new OAuth2 client')] final class CreateClientCommand extends Command @@ -31,12 +32,19 @@ final class CreateClientCommand extends Command */ private $clientFqcn; - public function __construct(ClientManagerInterface $clientManager, string $clientFqcn) + private ?PasswordHasherInterface $passwordHasher = null; + + public function __construct(ClientManagerInterface $clientManager, string $clientFqcn, ?PasswordHasherInterface $passwordHasher = null) { parent::__construct(); $this->clientManager = $clientManager; $this->clientFqcn = $clientFqcn; + $this->passwordHasher = $passwordHasher; + + if (null === $this->passwordHasher) { + trigger_deprecation('league/oauth2-server-bundle', '1.2', 'Not passing a "%s" to "%s" is deprecated since version 1.2 and will be required in 2.0.', PasswordHasherInterface::class, __CLASS__); + } } protected function configure(): void @@ -99,7 +107,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int $io = new SymfonyStyle($input, $output); try { - $client = $this->buildClientFromInput($input); + $plainSecret = $this->resolvePlainSecret($input); + $client = $this->buildClientFromInput($input, $plainSecret); } catch (\InvalidArgumentException $exception) { $io->error($exception->getMessage()); @@ -111,27 +120,36 @@ protected function execute(InputInterface $input, OutputInterface $output): int $headers = ['Identifier', 'Secret']; $rows = [ - [$client->getIdentifier(), $client->getSecret()], + [$client->getIdentifier(), $plainSecret], ]; $io->table($headers, $rows); return 0; } - private function buildClientFromInput(InputInterface $input): ClientInterface + private function resolvePlainSecret(InputInterface $input): ?string { - $name = $input->getArgument('name'); - $identifier = (string) $input->getArgument('identifier') ?: hash('md5', random_bytes(16)); $isPublic = $input->getOption('public'); if ($isPublic && null !== $input->getArgument('secret')) { throw new \InvalidArgumentException('The client cannot have a secret and be public.'); } - $secret = $isPublic ? null : $input->getArgument('secret') ?? hash('sha512', random_bytes(32)); + return $isPublic ? null : $input->getArgument('secret') ?? bin2hex(random_bytes(32)); + } + + private function buildClientFromInput(InputInterface $input, ?string $plainSecret): ClientInterface + { + $name = $input->getArgument('name'); + $identifier = (string) $input->getArgument('identifier') ?: hash('md5', random_bytes(16)); + + $hashedSecret = $plainSecret; + if ($this->passwordHasher && \is_string($plainSecret)) { + $hashedSecret = $this->passwordHasher->hash($plainSecret); + } /** @var AbstractClient $client */ - $client = new $this->clientFqcn($name, $identifier, $secret); + $client = new $this->clientFqcn($name, $identifier, $hashedSecret); $client->setActive(true); $client->setAllowPlainTextPkce($input->getOption('allow-plain-text-pkce')); diff --git a/src/Command/ListClientsCommand.php b/src/Command/ListClientsCommand.php index f04ed1c2..f1f7de2b 100644 --- a/src/Command/ListClientsCommand.php +++ b/src/Command/ListClientsCommand.php @@ -66,6 +66,12 @@ protected function configure(): void 'Finds by allowed scope for client. Use this option multiple times to find by multiple scopes.', [] ) + ->addOption( + 'reveal-secret', + null, + InputOption::VALUE_NONE, + 'Reveal the client secret in the output.' + ) ; } @@ -107,7 +113,7 @@ private function drawTable(InputInterface $input, OutputInterface $output, array { $io = new SymfonyStyle($input, $output); $columns = $this->getColumns($input); - $rows = $this->getRows($clients, $columns); + $rows = $this->getRows($clients, $columns, $input->getOption('reveal-secret')); $io->table($columns, $rows); } @@ -117,13 +123,13 @@ private function drawTable(InputInterface $input, OutputInterface $output, array * * @return array> */ - private function getRows(array $clients, array $columns): array + private function getRows(array $clients, array $columns, bool $revealSecret): array { - return array_map(static function (ClientInterface $client) use ($columns): array { + return array_map(static function (ClientInterface $client) use ($columns, $revealSecret): array { $values = [ 'name' => $client->getName(), 'identifier' => $client->getIdentifier(), - 'secret' => $client->getSecret(), + 'secret' => $revealSecret ? $client->getSecret() : ($client->isConfidential() ? '****' : '(public)'), 'scope' => implode(', ', $client->getScopes()), 'redirect uri' => implode(', ', $client->getRedirectUris()), 'grant type' => implode(', ', $client->getGrants()), diff --git a/src/Command/RehashClientSecretsCommand.php b/src/Command/RehashClientSecretsCommand.php new file mode 100644 index 00000000..6a3ed10b --- /dev/null +++ b/src/Command/RehashClientSecretsCommand.php @@ -0,0 +1,61 @@ +clientManager = $clientManager; + $this->hasher = $hasher; + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $migrated = $alreadyHashed = $public = 0; + + foreach ($this->clientManager->list(null) as $client) { + if (!$client->isConfidential()) { + ++$public; + continue; + } + + $secret = $client->getSecret() ?? ''; + + if (!$this->hasher->needsRehash($secret)) { + ++$alreadyHashed; + continue; + } + + $client->setSecret($this->hasher->hash($secret)); + $this->clientManager->save($client); + ++$migrated; + } + + $io->success(\sprintf( + 'Migration complete: %d secret(s) rehashed, %d already hashed, %d public client(s) skipped.', + $migrated, + $alreadyHashed, + $public, + )); + + return Command::SUCCESS; + } +} diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index 4221c8c8..d6a281d6 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -277,6 +277,10 @@ private function createClientNode(): NodeDefinition ->thenInvalid(\sprintf('%%s must be a %s', AbstractClient::class)) ->end() ->end() + ->booleanNode('allow_plaintext_secrets') + ->info('Whether to allow plaintext client secrets.') + ->defaultTrue() + ->end() ->end() ; diff --git a/src/DependencyInjection/LeagueOAuth2ServerExtension.php b/src/DependencyInjection/LeagueOAuth2ServerExtension.php index 9b77bbff..eaef7e77 100644 --- a/src/DependencyInjection/LeagueOAuth2ServerExtension.php +++ b/src/DependencyInjection/LeagueOAuth2ServerExtension.php @@ -45,6 +45,8 @@ use Symfony\Component\DependencyInjection\Extension\PrependExtensionInterface; use Symfony\Component\DependencyInjection\Loader\PhpFileLoader; use Symfony\Component\DependencyInjection\Reference; +use Symfony\Component\PasswordHasher\Hasher\MigratingPasswordHasher; +use Symfony\Component\PasswordHasher\Hasher\PlaintextPasswordHasher; final class LeagueOAuth2ServerExtension extends Extension implements PrependExtensionInterface, CompilerPassInterface { @@ -75,6 +77,18 @@ public function load(array $configs, ContainerBuilder $container): void ->replaceArgument(1, $config['client']['classname']) ; + if ($config['client']['allow_plaintext_secrets']) { + trigger_deprecation('league/oauth2-server-bundle', '1.2', 'Setting "client.allow_plaintext_secrets" config option to "true" is deprecated. Use the `league:oauth2-server:rehash-client-secrets` command to rehash existing client secrets and set this option to "false" afterwards.'); + $container->register('league.oauth2_server.password_hasher.plaintext', PlaintextPasswordHasher::class); + $container->register('league.oauth2_server.password_hasher.migrating', MigratingPasswordHasher::class) + ->setDecoratedService('league.oauth2_server.password_hasher') + ->setArguments([ + new Reference('league.oauth2_server.password_hasher.migrating.inner'), + new Reference('league.oauth2_server.password_hasher.plaintext'), + ]) + ; + } + $container ->findDefinition(GenerateKeyPairCommand::class) ->replaceArgument(1, $config['authorization_server']['private_key']) diff --git a/src/Model/AbstractClient.php b/src/Model/AbstractClient.php index 9f8a95ef..276dd431 100644 --- a/src/Model/AbstractClient.php +++ b/src/Model/AbstractClient.php @@ -62,6 +62,11 @@ public function getSecret(): ?string return $this->secret; } + public function setSecret(string $secret): void + { + $this->secret = $secret; + } + /** * @return list */ diff --git a/src/Model/ClientInterface.php b/src/Model/ClientInterface.php index cea6f754..502ab43b 100644 --- a/src/Model/ClientInterface.php +++ b/src/Model/ClientInterface.php @@ -10,6 +10,7 @@ /** * @method string getName() + * @method void setSecret(string $secret) */ interface ClientInterface { @@ -20,6 +21,8 @@ public function getIdentifier(): string; public function getSecret(): ?string; + // public function setSecret(string $secret): void; + /** * @return list */ diff --git a/src/Persistence/Mapping/Driver.php b/src/Persistence/Mapping/Driver.php index e9c85ae5..8e8c45e0 100644 --- a/src/Persistence/Mapping/Driver.php +++ b/src/Persistence/Mapping/Driver.php @@ -97,7 +97,7 @@ private function buildAbstractClientMetadata(ORMClassMetadata $metadata): void (new ClassMetadataBuilder($metadata)) ->setMappedSuperClass() ->createField('name', 'string')->length(128)->build() - ->createField('secret', 'string')->length(128)->nullable(true)->build() + ->createField('secret', 'string')->length(255)->nullable(true)->build() ->createField('redirectUris', 'oauth2_redirect_uri')->nullable(true)->build() ->createField('grants', 'oauth2_grant')->nullable(true)->build() ->createField('scopes', 'oauth2_scope')->nullable(true)->build() diff --git a/src/Repository/ClientRepository.php b/src/Repository/ClientRepository.php index af84b730..c9484818 100644 --- a/src/Repository/ClientRepository.php +++ b/src/Repository/ClientRepository.php @@ -9,6 +9,7 @@ use League\Bundle\OAuth2ServerBundle\Model\ClientInterface; use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Repositories\ClientRepositoryInterface; +use Symfony\Component\PasswordHasher\PasswordHasherInterface; final class ClientRepository implements ClientRepositoryInterface { @@ -17,9 +18,16 @@ final class ClientRepository implements ClientRepositoryInterface */ private $clientManager; - public function __construct(ClientManagerInterface $clientManager) + private ?PasswordHasherInterface $passwordHasher; + + public function __construct(ClientManagerInterface $clientManager, ?PasswordHasherInterface $passwordHasher = null) { $this->clientManager = $clientManager; + $this->passwordHasher = $passwordHasher; + + if (null === $this->passwordHasher) { + trigger_deprecation('league/oauth2-server-bundle', '1.2', 'Not passing a "%s" to "%s" is deprecated since version 1.2 and will be required in 2.0.', PasswordHasherInterface::class, __CLASS__); + } } public function getClientEntity(string $clientIdentifier): ?ClientEntityInterface @@ -33,7 +41,7 @@ public function getClientEntity(string $clientIdentifier): ?ClientEntityInterfac return $this->buildClientEntity($client); } - public function validateClient(string $clientIdentifier, ?string $clientSecret, ?string $grantType): bool + public function validateClient(string $clientIdentifier, #[\SensitiveParameter] ?string $clientSecret, ?string $grantType): bool { $client = $this->clientManager->find($clientIdentifier); @@ -49,11 +57,29 @@ public function validateClient(string $clientIdentifier, ?string $clientSecret, return false; } - if (!$client->isConfidential() || hash_equals((string) $client->getSecret(), (string) $clientSecret)) { + if (!$client->isConfidential()) { return true; } - return false; + $storedSecret = (string) $client->getSecret(); + $inputSecret = (string) $clientSecret; + + if (null === $this->passwordHasher) { + return hash_equals($storedSecret, $inputSecret); + } + + $secretIsValid = $this->passwordHasher->verify($storedSecret, $inputSecret); + + if ($secretIsValid && $this->passwordHasher->needsRehash($storedSecret)) { + if (!method_exists($client, 'setSecret')) { + trigger_deprecation('league/oauth2-server-bundle', '1.2', 'Not implementing method "setSecret()" in client "%s" is deprecated. This method will be required in 2.0.', $client::class); + } else { + $client->setSecret($this->passwordHasher->hash($inputSecret)); + $this->clientManager->save($client); + } + } + + return $secretIsValid; } private function buildClientEntity(ClientInterface $client): ClientEntity diff --git a/tests/Acceptance/CreateClientCommandTest.php b/tests/Acceptance/CreateClientCommandTest.php index 391c7767..48a68c6a 100644 --- a/tests/Acceptance/CreateClientCommandTest.php +++ b/tests/Acceptance/CreateClientCommandTest.php @@ -8,6 +8,7 @@ use League\Bundle\OAuth2ServerBundle\Model\Client; use League\Bundle\OAuth2ServerBundle\ValueObject\Scope; use Symfony\Component\Console\Tester\CommandTester; +use Symfony\Component\PasswordHasher\PasswordHasherInterface; final class CreateClientCommandTest extends AbstractAcceptanceTest { @@ -125,7 +126,9 @@ public function testCreateClientWithSecret(): void ->get(ClientManagerInterface::class) ->find('foobar'); $this->assertInstanceOf(Client::class, $client); - $this->assertSame('quzbaz', $client->getSecret()); + /** @var PasswordHasherInterface $passwordHasher */ + $passwordHasher = static::getContainer()->get('league.oauth2_server.password_hasher'); + $this->assertTrue($passwordHasher->verify($client->getSecret() ?? '', 'quzbaz')); $this->assertTrue($client->isConfidential()); $this->assertFalse($client->isPlainTextPkceAllowed()); } diff --git a/tests/Acceptance/CustomPersistenceManagerTest.php b/tests/Acceptance/CustomPersistenceManagerTest.php index 4ceb71bb..71c132e3 100644 --- a/tests/Acceptance/CustomPersistenceManagerTest.php +++ b/tests/Acceptance/CustomPersistenceManagerTest.php @@ -60,7 +60,8 @@ public function testSuccessfulClientCredentialsRequest(): void $this->accessTokenManager->expects(self::atLeastOnce())->method('save'); $this->client->getContainer()->set('test.access_token_manager', $this->accessTokenManager); - $this->clientManager->expects(self::atLeastOnce())->method('find')->with('foo')->willReturn(new Client('name', 'foo', 'secret')); + $hashedSecret = $this->client->getContainer()->get('league.oauth2_server.password_hasher')->hash('secret'); + $this->clientManager->expects(self::atLeastOnce())->method('find')->with('foo')->willReturn(new Client('name', 'foo', $hashedSecret)); $this->client->getContainer()->set('test.client_manager', $this->clientManager); $this->client->request('POST', '/token', [ @@ -79,7 +80,8 @@ public function testSuccessfulPasswordRequest(): void $this->accessTokenManager->expects(self::atLeastOnce())->method('save'); $this->client->getContainer()->set('test.access_token_manager', $this->accessTokenManager); - $this->clientManager->expects(self::atLeastOnce())->method('find')->with('foo')->willReturn(new Client('name', 'foo', 'secret')); + $hashedSecret = $this->client->getContainer()->get('league.oauth2_server.password_hasher')->hash('secret'); + $this->clientManager->expects(self::atLeastOnce())->method('find')->with('foo')->willReturn(new Client('name', 'foo', $hashedSecret)); $this->client->getContainer()->set('test.client_manager', $this->clientManager); $eventDispatcher = $this->client->getContainer()->get('event_dispatcher'); @@ -101,7 +103,8 @@ public function testSuccessfulPasswordRequest(): void public function testSuccessfulRefreshTokenRequest(): void { - $client = new Client('name', 'foo', 'secret'); + $hashedSecret = $this->client->getContainer()->get('league.oauth2_server.password_hasher')->hash('secret'); + $client = new Client('name', 'foo', $hashedSecret); $accessToken = new AccessToken('access_token', new \DateTimeImmutable('+1 hour'), $client, 'user', []); $refreshToken = new RefreshToken('refresh_token', new \DateTimeImmutable('+1 month'), $accessToken); @@ -128,7 +131,8 @@ public function testSuccessfulRefreshTokenRequest(): void public function testSuccessfulAuthorizationCodeRequest(): void { - $client = new Client('name', 'foo', 'secret'); + $hashedSecret = $this->client->getContainer()->get('league.oauth2_server.password_hasher')->hash('secret'); + $client = new Client('name', 'foo', $hashedSecret); $client->setRedirectUris(new RedirectUri('https://example.org/oauth2/redirect-uri')); $authCode = new AuthorizationCode('authorization_code', new \DateTimeImmutable('+2 minute'), $client, 'user', []); diff --git a/tests/Acceptance/DoctrineAccessTokenManagerTest.php b/tests/Acceptance/DoctrineAccessTokenManagerTest.php index ed1b71e9..77889dc0 100644 --- a/tests/Acceptance/DoctrineAccessTokenManagerTest.php +++ b/tests/Acceptance/DoctrineAccessTokenManagerTest.php @@ -74,8 +74,8 @@ private function buildClearExpiredTestData(Client $client): array $validAccessTokens = [ $this->buildAccessToken('1111', '+1 day', $client), $this->buildAccessToken('2222', '+1 hour', $client), - $this->buildAccessToken('3333', '+1 second', $client), - $this->buildAccessToken('4444', 'now', $client), + $this->buildAccessToken('3333', '+1 minute', $client), + $this->buildAccessToken('4444', '+1 second', $client), ]; $expiredAccessTokens = [ diff --git a/tests/Acceptance/RehashClientSecretsCommandTest.php b/tests/Acceptance/RehashClientSecretsCommandTest.php new file mode 100644 index 00000000..6354cb67 --- /dev/null +++ b/tests/Acceptance/RehashClientSecretsCommandTest.php @@ -0,0 +1,128 @@ +getConnection(); + + $this->insertClientWithSecret($connection, 'client-plain', 'plain-text-secret'); + + $commandTester = $this->runCommand(); + + $this->assertSame(0, $commandTester->getStatusCode()); + $this->assertStringContainsString('1 secret(s) rehashed', $commandTester->getDisplay()); + + $hashedSecret = $connection->fetchOne('SELECT secret FROM oauth2_client WHERE identifier = ?', ['client-plain']); + $this->assertStringStartsWith('$2y$', $hashedSecret); + $this->assertTrue($this->getHasher()->verify($hashedSecret, 'plain-text-secret')); + } + + public function testSkipAlreadyHashedSecrets(): void + { + $connection = $this->getConnection(); + + // Pre-hash with the same hasher the command uses so needsRehash() returns false + $hashedByHasher = $this->getHasher()->hash('already-hashed'); + $this->insertClientWithSecret($connection, 'client-hashed', $hashedByHasher); + + $commandTester = $this->runCommand(); + + $this->assertSame(0, $commandTester->getStatusCode()); + $this->assertStringContainsString('0 secret(s) rehashed', $commandTester->getDisplay()); + $this->assertStringContainsString('1 already hashed', $commandTester->getDisplay()); + + $storedSecret = $connection->fetchOne('SELECT secret FROM oauth2_client WHERE identifier = ?', ['client-hashed']); + $this->assertSame($hashedByHasher, $storedSecret); + } + + public function testSkipPublicClients(): void + { + $connection = $this->getConnection(); + + $connection->insert('oauth2_client', [ + 'identifier' => 'client-public', + 'name' => 'Public Client', + 'secret' => null, + 'active' => 1, + 'allowPlainTextPkce' => 0, + ]); + + $commandTester = $this->runCommand(); + + $this->assertSame(0, $commandTester->getStatusCode()); + $this->assertStringContainsString('0 secret(s) rehashed', $commandTester->getDisplay()); + } + + public function testMixedClients(): void + { + $connection = $this->getConnection(); + $hasher = $this->getHasher(); + + $this->insertClientWithSecret($connection, 'plain-1', 'secret-one'); + $this->insertClientWithSecret($connection, 'plain-2', 'secret-two'); + + // Pre-hash with the same hasher so needsRehash() returns false + $this->insertClientWithSecret($connection, 'already-hashed', $hasher->hash('hashed-secret')); + + $connection->insert('oauth2_client', [ + 'identifier' => 'public-client', + 'name' => 'Public', + 'secret' => null, + 'active' => 1, + 'allowPlainTextPkce' => 0, + ]); + + $commandTester = $this->runCommand(); + + $this->assertStringContainsString('2 secret(s) rehashed', $commandTester->getDisplay()); + $this->assertStringContainsString('1 already hashed', $commandTester->getDisplay()); + + $this->assertTrue($hasher->verify( + $connection->fetchOne('SELECT secret FROM oauth2_client WHERE identifier = ?', ['plain-1']), + 'secret-one' + )); + $this->assertTrue($hasher->verify( + $connection->fetchOne('SELECT secret FROM oauth2_client WHERE identifier = ?', ['plain-2']), + 'secret-two' + )); + } + + private function getConnection(): Connection + { + return $this->client->getContainer()->get('database_connection'); + } + + private function getHasher(): PasswordHasherInterface + { + return $this->client->getContainer()->get('league.oauth2_server.password_hasher'); + } + + private function insertClientWithSecret(Connection $connection, string $identifier, string $secret): void + { + $connection->insert('oauth2_client', [ + 'identifier' => $identifier, + 'name' => 'Test Client', + 'secret' => $secret, + 'active' => 1, + 'allowPlainTextPkce' => 0, + ]); + } + + private function runCommand(): CommandTester + { + $command = $this->application->find('league:oauth2-server:rehash-client-secrets'); + $commandTester = new CommandTester($command); + $commandTester->execute([]); + + return $commandTester; + } +} diff --git a/tests/Acceptance/resource/list-clients-with-client-having-no-secret.txt b/tests/Acceptance/resource/list-clients-with-client-having-no-secret.txt index 940bed08..fd93987f 100644 --- a/tests/Acceptance/resource/list-clients-with-client-having-no-secret.txt +++ b/tests/Acceptance/resource/list-clients-with-client-having-no-secret.txt @@ -1,5 +1,5 @@ - -------- ------------ -------- ------- -------------- ------------ - name identifier secret scope redirect uri grant type - -------- ------------ -------- ------- -------------- ------------ - client foobar rock - -------- ------------ -------- ------- -------------- ------------ + -------- ------------ ---------- ------- -------------- ------------ + name identifier secret scope redirect uri grant type + -------- ------------ ---------- ------- -------------- ------------ + client foobar (public) rock + -------- ------------ ---------- ------- -------------- ------------ diff --git a/tests/Acceptance/resource/list-clients.txt b/tests/Acceptance/resource/list-clients.txt index eca977d7..cd6cdcbc 100644 --- a/tests/Acceptance/resource/list-clients.txt +++ b/tests/Acceptance/resource/list-clients.txt @@ -1,5 +1,5 @@ -------- ------------ -------- ------- -------------- ------------ name identifier secret scope redirect uri grant type -------- ------------ -------- ------- -------------- ------------ - client foobar quzbaz rock + client foobar **** rock -------- ------------ -------- ------- -------------- ------------ diff --git a/tests/Acceptance/resource/list-filters-clients.txt b/tests/Acceptance/resource/list-filters-clients.txt index d9e98e1d..9e6e29e8 100644 --- a/tests/Acceptance/resource/list-filters-clients.txt +++ b/tests/Acceptance/resource/list-filters-clients.txt @@ -1,6 +1,6 @@ - -------- ------------ ----------------- ----------------------------- -------------- ------------ - name identifier secret scope redirect uri grant type - -------- ------------ ----------------- ----------------------------- -------------- ------------ - client client-b client-b-secret client-b-scope, other-scope - -------- ------------ ----------------- ----------------------------- -------------- ------------ + -------- ------------ -------- ----------------------------- -------------- ------------ + name identifier secret scope redirect uri grant type + -------- ------------ -------- ----------------------------- -------------- ------------ + client client-b **** client-b-scope, other-scope + -------- ------------ -------- ----------------------------- -------------- ------------ diff --git a/tests/Fixtures/FixtureFactory.php b/tests/Fixtures/FixtureFactory.php index 8fbbd439..1bf00693 100644 --- a/tests/Fixtures/FixtureFactory.php +++ b/tests/Fixtures/FixtureFactory.php @@ -257,19 +257,19 @@ private static function createClients(): array { $clients = []; - $clients[] = (new Client('name', self::FIXTURE_CLIENT_FIRST, 'secret')) + $clients[] = (new Client('name', self::FIXTURE_CLIENT_FIRST, '$2y$13$C5G/H.dXzMuOfnV4qC4C1.SBvEf2jcUYXE.xNneP4bSKY6ccDkPWe')) // 'secret' ->setRedirectUris(new RedirectUri(self::FIXTURE_CLIENT_FIRST_REDIRECT_URI)); - $clients[] = (new Client('name', self::FIXTURE_CLIENT_SECOND, 'top_secret')) + $clients[] = (new Client('name', self::FIXTURE_CLIENT_SECOND, '$2y$13$nTT7fkuXnJc9TQIl0wW61uVuMzzOTgE.isRssjlLb55HdYas4jLty')) // 'top_secret' ->setRedirectUris(new RedirectUri(self::FIXTURE_CLIENT_SECOND_REDIRECT_URI)); - $clients[] = (new Client('name', self::FIXTURE_CLIENT_INACTIVE, 'woah')) + $clients[] = (new Client('name', self::FIXTURE_CLIENT_INACTIVE, '$2y$13$bb75Y48uFgQiREiXtiJNkee9Q7kVcSpgw.fW0tTyM5FicvEppalV.')) // 'woah' ->setActive(false); - $clients[] = (new Client('name', self::FIXTURE_CLIENT_RESTRICTED_GRANTS, 'wicked')) + $clients[] = (new Client('name', self::FIXTURE_CLIENT_RESTRICTED_GRANTS, '$2y$13$aBEPx05Ba9P3ierHACN1Gu4uG6UAxnCU2qPhg9875MmmKF1n3csYW')) // 'wicked' ->setGrants(new Grant('password')); - $clients[] = (new Client('name', self::FIXTURE_CLIENT_RESTRICTED_SCOPES, 'beer')) + $clients[] = (new Client('name', self::FIXTURE_CLIENT_RESTRICTED_SCOPES, '$2y$13$5NX9iiSlH3IRTl223/qVzu/CuyvxKq7rbLhFouVBrsCAZT5zYMBy6')) // 'beer' ->setRedirectUris(new RedirectUri(self::FIXTURE_CLIENT_FIRST_REDIRECT_URI)) ->setScopes(new Scope(self::FIXTURE_SCOPE_SECOND)); diff --git a/tests/Integration/AbstractIntegrationTest.php b/tests/Integration/AbstractIntegrationTest.php index ef91fb65..de2048b1 100644 --- a/tests/Integration/AbstractIntegrationTest.php +++ b/tests/Integration/AbstractIntegrationTest.php @@ -48,6 +48,8 @@ use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\PasswordHasher\Hasher\NativePasswordHasher; +use Symfony\Component\PasswordHasher\PasswordHasherInterface; use Symfony\Contracts\EventDispatcher\EventDispatcherInterface; abstract class AbstractIntegrationTest extends TestCase @@ -97,6 +99,11 @@ abstract class AbstractIntegrationTest extends TestCase */ private $psrFactory; + /** + * @var PasswordHasherInterface + */ + protected $passwordHasher; + /** * @var bool */ @@ -110,10 +117,11 @@ protected function setUp(): void $this->accessTokenManager = new AccessTokenManager(true); $this->refreshTokenManager = new RefreshTokenManager(); $this->authCodeManager = new AuthorizationCodeManager(); + $this->passwordHasher = new NativePasswordHasher(); $scopeConverter = new ScopeConverter(); $scopeRepository = new ScopeRepository($this->scopeManager, $this->clientManager, $scopeConverter, $this->eventDispatcher); - $clientRepository = new ClientRepository($this->clientManager); + $clientRepository = new ClientRepository($this->clientManager, $this->passwordHasher); $accessTokenRepository = new AccessTokenRepository($this->accessTokenManager, $this->clientManager, $scopeConverter); $refreshTokenRepository = new RefreshTokenRepository($this->refreshTokenManager, $this->accessTokenManager); $userConverter = new UserConverter(); diff --git a/tests/TestKernel.php b/tests/TestKernel.php index f2495b78..8dd1002d 100644 --- a/tests/TestKernel.php +++ b/tests/TestKernel.php @@ -184,6 +184,9 @@ public function registerContainerConfiguration(LoaderInterface $loader): void ], ], 'persistence' => $this->persistenceConfig ?? ['doctrine' => ['entity_manager' => 'default']], + 'client' => [ + 'allow_plaintext_secrets' => false, + ], ]); $this->configureControllers($container); diff --git a/tests/Unit/ClientRepositoryTest.php b/tests/Unit/ClientRepositoryTest.php new file mode 100644 index 00000000..e39313dd --- /dev/null +++ b/tests/Unit/ClientRepositoryTest.php @@ -0,0 +1,148 @@ +hasher = new NativePasswordHasher(); + $this->clientManager = new ClientManager(new EventDispatcher()); + $this->repository = new ClientRepository($this->clientManager, $this->hasher); + } + + /** + * @group legacy + */ + public function testValidateClientSucceedsWithoutPasswordHasher(): void + { + $plainSecret = 'my-plain-secret'; + $client = new Client('My App', 'my-client', $plainSecret); + $this->clientManager->save($client); + + $repositoryWithoutHasher = new ClientRepository($this->clientManager); + + $this->assertTrue( + $repositoryWithoutHasher->validateClient('my-client', $plainSecret, null) + ); + } + + /** + * Regression test: a client created via CreateClientCommand stores its secret + * pre-hashed with NativePasswordHasher::hash() (which uses SHA-512+bcrypt for + * long passwords). validateClient() must verify the plain secret correctly. + */ + public function testValidateClientSucceedsWithSecretHashedByHasher(): void + { + $plainSecret = 'my-plain-secret'; + $client = new Client('My App', 'my-client', $this->hasher->hash($plainSecret)); + $this->clientManager->save($client); + + $this->assertTrue( + $this->repository->validateClient('my-client', $plainSecret, null) + ); + } + + /** + * Regression test for the original bug: a long secret (>72 bytes, as produced by + * CreateClientCommand's sha512 fallback) must survive a round-trip through + * hash() and verify() without silent truncation. + */ + public function testValidateClientSucceedsWithLongSecretOver72Bytes(): void + { + // Simulate the auto-generated secret from CreateClientCommand (sha512 = 128 hex chars). + $longSecret = hash('sha512', random_bytes(32)); + $this->assertGreaterThan(72, \strlen($longSecret)); + + $client = new Client('My App', 'my-client-long', $this->hasher->hash($longSecret)); + $this->clientManager->save($client); + + $this->assertTrue( + $this->repository->validateClient('my-client-long', $longSecret, null) + ); + } + + public function testValidateClientFailsWithWrongSecret(): void + { + $client = new Client('My App', 'my-client', $this->hasher->hash('my-plain-secret')); + $this->clientManager->save($client); + + $this->assertFalse( + $this->repository->validateClient('my-client', 'wrong-secret', null) + ); + } + + /** + * Plain-text migration path: clients whose secrets were stored as plain text + * (before hashing was introduced) are verified and then automatically + * rehashed on first successful validation. + */ + public function testValidateClientSucceedsWithPlainTextLegacySecret(): void + { + $client = new Client('Legacy App', 'legacy-client', 'plain-text-secret'); + $this->clientManager->save($client); + + $migratingHasher = new MigratingPasswordHasher($this->hasher, new PlaintextPasswordHasher()); + $repositoryWithMigratingHasher = new ClientRepository($this->clientManager, $migratingHasher); + + $this->assertTrue( + $repositoryWithMigratingHasher->validateClient('legacy-client', 'plain-text-secret', null) + ); + + // After successful plain-text validation the secret must have been upgraded to a hash. + $upgraded = $this->clientManager->find('legacy-client'); + $this->assertNotSame('plain-text-secret', $upgraded->getSecret()); + $this->assertTrue($this->hasher->verify($upgraded->getSecret(), 'plain-text-secret')); + } + + /** + * Simulates what Doctrine does when hydrating a client from the database: + * the $secret field is set directly to the stored hash value via reflection, + * bypassing the constructor. setSecret() replicates this path. + */ + public function testValidateClientSucceedsWhenSecretSetDirectlyAsHash(): void + { + $plainSecret = 'my-plain-secret'; + + $client = new Client('My App', 'my-client-db', null); + $client->setSecret($this->hasher->hash($plainSecret)); + $this->clientManager->save($client); + + $this->assertTrue( + $this->repository->validateClient('my-client-db', $plainSecret, null) + ); + } + + public function testValidateClientReturnsFalseForUnknownClient(): void + { + $this->assertFalse( + $this->repository->validateClient('nonexistent', 'any-secret', null) + ); + } + + public function testValidateClientReturnsFalseForInactiveClient(): void + { + $client = (new Client('My App', 'inactive-client', $this->hasher->hash('secret')))->setActive(false); + $this->clientManager->save($client); + + $this->assertFalse( + $this->repository->validateClient('inactive-client', 'secret', null) + ); + } +} diff --git a/tests/Unit/ExtensionTest.php b/tests/Unit/ExtensionTest.php index 4fb34602..0db6d22d 100644 --- a/tests/Unit/ExtensionTest.php +++ b/tests/Unit/ExtensionTest.php @@ -196,6 +196,9 @@ private function getValidConfiguration(array $options = []): array // Pick one for valid config: // 'persistence' => ['doctrine' => []] 'persistence' => ['in_memory' => 1], + 'client' => [ + 'allow_plaintext_secrets' => $options['allow_plaintext_secrets'] ?? false, + ], ], ]; }