review pr feedback

thephpleague · Dec 12, 2021 · 172f5f9 · 172f5f9
1 parent 064952f
commit 172f5f9
Show file tree

Hide file tree

Showing 16 changed files with 57 additions and 62 deletions.
diff --git a/docs/index.md b/docs/index.md
@@ -72,8 +72,8 @@ For implementation into Symfony projects, please see [bundle documentation](docs
             # Whether to require code challenge for public clients for the auth code grant
             require_code_challenge_for_public_clients: true
 
-            # Whether to disable access token saving to persistence layer (default to false)
-            disable_access_token_saving: true
+            # Whether to enable access token saving to persistence layer (default to true)
+            persist_access_token: true
 
         resource_server:      # Required
 

diff --git a/phpunit.xml.dist b/phpunit.xml.dist
@@ -13,6 +13,8 @@
         <env name="SHELL_VERBOSITY" value="-1" />
         <env name="SYMFONY_DEPRECATIONS_HELPER" value="max[self]=0" />
         <env name="SYMFONY_PHPUNIT_REMOVE" value="" />
+
+        <ini name="memory_limit" value="512M" />
     </php>
 
     <testsuites>

diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
@@ -106,9 +106,9 @@ private function createAuthorizationServerNode(): NodeDefinition
                     ->info('Whether to enable the implicit grant')
                     ->defaultTrue()
                 ->end()
-                ->booleanNode('disable_access_token_saving')
-                    ->info('Whether to disable access token saving to persistence layer')
-                    ->defaultFalse()
+                ->booleanNode('persist_access_token')
+                    ->info('Whether to enable access token saving to persistence layer')
+                    ->defaultTrue()
                 ->end()
             ->end()
         ;

diff --git a/src/DependencyInjection/LeagueOAuth2ServerExtension.php b/src/DependencyInjection/LeagueOAuth2ServerExtension.php
@@ -210,13 +210,13 @@ private function configureGrants(ContainerBuilder $container, array $config): vo
 
     private function configureAccessTokenSaving(LoaderInterface $loader, ContainerBuilder $container, array $config): void
     {
-        if ($config['disable_access_token_saving']) {
-            $loader->load('access_token/null.php');
-        } else {
+        if ($config['persist_access_token']) {
             $loader->load('access_token/default.php');
+        } else {
+            $loader->load('access_token/null.php');
         }
 
-        $container->setParameter('league.oauth2_server.authorization_server.disable_access_token_saving', $config['disable_access_token_saving']);
+        $container->setParameter('league.oauth2_server.authorization_server.disable_access_token_saving', !$config['persist_access_token']);
     }
 
     /**
@@ -231,20 +231,19 @@ private function configurePersistence(LoaderInterface $loader, ContainerBuilder
         $persistenceConfig = current($config['persistence']);
         $persistenceMethod = key($config['persistence']);
 
-        $disableAccessTokenSaving = $config['authorization_server']['disable_access_token_saving'];
         switch ($persistenceMethod) {
             case 'in_memory':
                 $loader->load('storage/in_memory.php');
-                $this->configureInMemoryPersistence($container, $disableAccessTokenSaving);
+                $this->configureInMemoryPersistence($container, $config);
                 break;
             case 'doctrine':
                 $loader->load('storage/doctrine.php');
-                $this->configureDoctrinePersistence($container, $config, $persistenceConfig, $disableAccessTokenSaving);
+                $this->configureDoctrinePersistence($container, $config, $persistenceConfig);
                 break;
         }
     }
 
-    private function configureDoctrinePersistence(ContainerBuilder $container, array $config, array $persistenceConfig, bool $disableAccessTokenSaving): void
+    private function configureDoctrinePersistence(ContainerBuilder $container, array $config, array $persistenceConfig): void
     {
         $entityManagerName = $persistenceConfig['entity_manager'];
 
@@ -255,7 +254,7 @@ private function configureDoctrinePersistence(ContainerBuilder $container, array
         $container
             ->findDefinition(AccessTokenManager::class)
             ->replaceArgument(0, $entityManager)
-            ->replaceArgument('$disableAccessTokenSaving', $disableAccessTokenSaving)
+            ->replaceArgument(1, $config['authorization_server']['persist_access_token'])
         ;
 
         $container
@@ -282,23 +281,24 @@ private function configureDoctrinePersistence(ContainerBuilder $container, array
         $container
             ->findDefinition(Driver::class)
             ->replaceArgument(0, $config['client']['classname'])
+            ->replaceArgument(1, $config['authorization_server']['persist_access_token'])
         ;
 
         $container->setParameter('league.oauth2_server.persistence.doctrine.enabled', true);
         $container->setParameter('league.oauth2_server.persistence.doctrine.manager', $entityManagerName);
 
-        if ($disableAccessTokenSaving) {
-            $container->setParameter('league.oauth2_server.persistence.doctrine.access_token.disabled', true);
-        } else {
+        if ($config['authorization_server']['persist_access_token']) {
             $container->setParameter('league.oauth2_server.persistence.doctrine.access_token.enabled', true);
+        } else {
+            $container->setParameter('league.oauth2_server.persistence.doctrine.access_token.disabled', true);
         }
     }
 
-    private function configureInMemoryPersistence(ContainerBuilder $container, bool $disableAccessTokenSaving): void
+    private function configureInMemoryPersistence(ContainerBuilder $container, array $config): void
     {
         $container
             ->findDefinition(InMemoryAccessTokenManager::class)
-            ->replaceArgument('$disableAccessTokenSaving', $disableAccessTokenSaving)
+            ->replaceArgument(0, $config['authorization_server']['persist_access_token'])
         ;
         $container->setParameter('league.oauth2_server.persistence.in_memory.enabled', true);
     }

diff --git a/src/Manager/Doctrine/AccessTokenManager.php b/src/Manager/Doctrine/AccessTokenManager.php
@@ -16,17 +16,17 @@ final class AccessTokenManager implements AccessTokenManagerInterface
     private $entityManager;
 
     /** @var bool */
-    private $disableAccessTokenSaving;
+    private $persistAccessToken;
 
-    public function __construct(EntityManagerInterface $entityManager, bool $disableAccessTokenSaving)
+    public function __construct(EntityManagerInterface $entityManager, bool $persistAccessToken)
     {
         $this->entityManager = $entityManager;
-        $this->disableAccessTokenSaving = $disableAccessTokenSaving;
+        $this->persistAccessToken = $persistAccessToken;
     }
 
     public function find(string $identifier): ?AccessToken
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return null;
         }
 
@@ -35,7 +35,7 @@ public function find(string $identifier): ?AccessToken
 
     public function save(AccessToken $accessToken): void
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return;
         }
 
@@ -45,7 +45,7 @@ public function save(AccessToken $accessToken): void
 
     public function clearExpired(): int
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return 0;
         }
 

diff --git a/src/Manager/InMemory/AccessTokenManager.php b/src/Manager/InMemory/AccessTokenManager.php
@@ -15,19 +15,19 @@ final class AccessTokenManager implements AccessTokenManagerInterface
     private $accessTokens = [];
 
     /** @var bool */
-    private $disableAccessTokenSaving;
+    private $persistAccessToken;
 
-    public function __construct(bool $disableAccessTokenSaving)
+    public function __construct(bool $persistAccessToken)
     {
-        $this->disableAccessTokenSaving = $disableAccessTokenSaving;
+        $this->persistAccessToken = $persistAccessToken;
     }
 
     /**
      * @psalm-mutation-free
      */
     public function find(string $identifier): ?AccessToken
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return null;
         }
 
@@ -36,7 +36,7 @@ public function find(string $identifier): ?AccessToken
 
     public function save(AccessToken $accessToken): void
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return;
         }
 
@@ -45,7 +45,7 @@ public function save(AccessToken $accessToken): void
 
     public function clearExpired(): int
     {
-        if ($this->disableAccessTokenSaving) {
+        if (!$this->persistAccessToken) {
             return 0;
         }
 

diff --git a/src/Persistence/Mapping/Driver.php b/src/Persistence/Mapping/Driver.php
@@ -25,9 +25,13 @@ class Driver implements MappingDriver
      */
     private $clientClass;
 
-    public function __construct(string $clientClass)
+    /** @var bool */
+    private $persistAccessToken;
+
+    public function __construct(string $clientClass, bool $persistAccessToken)
     {
         $this->clientClass = $clientClass;
+        $this->persistAccessToken = $persistAccessToken;
     }
 
     public function loadMetadataForClass($className, ClassMetadata $metadata): void
@@ -63,11 +67,11 @@ public function getAllClassNames(): array
         return array_merge(
             [
                 AbstractClient::class,
-                AccessToken::class,
                 AuthorizationCode::class,
                 RefreshToken::class,
             ],
-            Client::class === $this->clientClass ? [Client::class] : []
+            Client::class === $this->clientClass ? [Client::class] : [],
+            $this->persistAccessToken ? [AccessToken::class] : [],
         );
     }
 

diff --git a/src/Repository/NullAccessTokenRepository.php b/src/Repository/NullAccessTokenRepository.php
@@ -14,7 +14,7 @@ final class NullAccessTokenRepository implements AccessTokenRepositoryInterface
     /**
      * {@inheritdoc}
      */
-    public function getNewToken(ClientEntityInterface $clientEntity, array $scopes, $userIdentifier = null)
+    public function getNewToken(ClientEntityInterface $clientEntity, array $scopes, $userIdentifier = null): AccessTokenEntityInterface
     {
         /** @var int|string|null $userIdentifier */
         $accessToken = new AccessTokenEntity();

diff --git a/src/Resources/config/access_token/default.php b/src/Resources/config/access_token/default.php
@@ -2,25 +2,13 @@
 
 declare(strict_types=1);
 
+use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
 use League\Bundle\OAuth2ServerBundle\Converter\ScopeConverterInterface;
-use League\Bundle\OAuth2ServerBundle\League\Repository\AccessTokenRepository;
 use League\Bundle\OAuth2ServerBundle\Manager\AccessTokenManagerInterface;
 use League\Bundle\OAuth2ServerBundle\Manager\ClientManagerInterface;
+use League\Bundle\OAuth2ServerBundle\Repository\AccessTokenRepository;
 use League\OAuth2\Server\Repositories\AccessTokenRepositoryInterface;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
-use Symfony\Component\DependencyInjection\Loader\Configurator\ReferenceConfigurator;
-
-// BC Layer for < 5.1 versions
-if (!function_exists('service')) {
-    function service(string $id): ReferenceConfigurator
-    {
-        $fn = function_exists('Symfony\Component\DependencyInjection\Loader\Configurator\service')
-            ? 'Symfony\Component\DependencyInjection\Loader\Configurator\service'
-            : 'Symfony\Component\DependencyInjection\Loader\Configurator\ref';
-
-        return ($fn)($id);
-    }
-}
 
 return static function (ContainerConfigurator $container): void {
     $container->services()

diff --git a/src/Resources/config/access_token/null.php b/src/Resources/config/access_token/null.php
@@ -2,8 +2,8 @@
 
 declare(strict_types=1);
 
-use League\Bundle\OAuth2ServerBundle\League\Repository\AccessTokenRepository;
-use League\Bundle\OAuth2ServerBundle\League\Repository\NullAccessTokenRepository;
+use League\Bundle\OAuth2ServerBundle\Repository\AccessTokenRepository;
+use League\Bundle\OAuth2ServerBundle\Repository\NullAccessTokenRepository;
 use League\OAuth2\Server\Repositories\AccessTokenRepositoryInterface;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 

diff --git a/src/Resources/config/services.php b/src/Resources/config/services.php
@@ -2,6 +2,10 @@
 
 declare(strict_types=1);
 
+use function Symfony\Component\DependencyInjection\Loader\Configurator\abstract_arg;
+use function Symfony\Component\DependencyInjection\Loader\Configurator\param;
+use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
+use function Symfony\Component\DependencyInjection\Loader\Configurator\tagged_iterator;
 use League\Bundle\OAuth2ServerBundle\AuthorizationServer\GrantConfigurator;
 use League\Bundle\OAuth2ServerBundle\Command\ClearExpiredTokensCommand;
 use League\Bundle\OAuth2ServerBundle\Command\CreateClientCommand;
@@ -53,10 +57,6 @@
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\HttpFoundation\RequestStack;
 use Symfony\Component\Security\Core\Security;
-use function Symfony\Component\DependencyInjection\Loader\Configurator\abstract_arg;
-use function Symfony\Component\DependencyInjection\Loader\Configurator\param;
-use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
-use function Symfony\Component\DependencyInjection\Loader\Configurator\tagged_iterator;
 
 return static function (ContainerConfigurator $container): void {
     $container->services()

diff --git a/src/Resources/config/storage/doctrine.php b/src/Resources/config/storage/doctrine.php
@@ -23,6 +23,7 @@
         ->set('league.oauth2_server.persistence.driver', Driver::class)
             ->args([
                 null,
+                null,
             ])
         ->alias(Driver::class, 'league.oauth2_server.persistence.driver')
 
@@ -38,7 +39,7 @@
         ->set('league.oauth2_server.manager.doctrine.access_token', AccessTokenManager::class)
             ->args([
                 null,
-                '$disableAccessTokenSaving' => null,
+                null,
             ])
         ->alias(AccessTokenManagerInterface::class, 'league.oauth2_server.manager.doctrine.access_token')
         ->alias(AccessTokenManager::class, 'league.oauth2_server.manager.doctrine.access_token')

diff --git a/src/Resources/config/storage/in_memory.php b/src/Resources/config/storage/in_memory.php
@@ -26,7 +26,7 @@
 
         ->set('league.oauth2_server.manager.in_memory.access_token', AccessTokenManager::class)
             ->args([
-                '$disableAccessTokenSaving' => null,
+                null,
             ])
         ->alias(AccessTokenManagerInterface::class, 'league.oauth2_server.manager.in_memory.access_token')
         ->alias(AccessTokenManager::class, 'league.oauth2_server.manager.in_memory.access_token')

diff --git a/tests/Acceptance/DoctrineAccessTokenManagerTest.php b/tests/Acceptance/DoctrineAccessTokenManagerTest.php
@@ -21,7 +21,7 @@ public function testClearExpired(): void
         /** @var EntityManagerInterface $em */
         $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
 
-        $doctrineAccessTokenManager = new DoctrineAccessTokenManager($em, false);
+        $doctrineAccessTokenManager = new DoctrineAccessTokenManager($em, true);
 
         $client = new Client('client', 'client', 'secret');
         $em->persist($client);
@@ -47,7 +47,7 @@ public function testClearExpiredWithoutSavingAccessToken(): void
         /** @var EntityManagerInterface $em */
         $em = $this->client->getContainer()->get('doctrine.orm.entity_manager');
 
-        $doctrineAccessTokenManager = new DoctrineAccessTokenManager($em, true);
+        $doctrineAccessTokenManager = new DoctrineAccessTokenManager($em, false);
 
         $client = new Client('name', 'client', 'secret');
         $em->persist($client);

diff --git a/tests/Integration/AbstractIntegrationTest.php b/tests/Integration/AbstractIntegrationTest.php
@@ -110,7 +110,7 @@ protected function setUp(): void
         $this->eventDispatcher = new EventDispatcher();
         $this->scopeManager = new ScopeManager();
         $this->clientManager = new ClientManager($this->eventDispatcher);
-        $this->accessTokenManager = new AccessTokenManager(false);
+        $this->accessTokenManager = new AccessTokenManager(true);
         $this->refreshTokenManager = new RefreshTokenManager();
         $this->authCodeManager = new AuthorizationCodeManager();
 

diff --git a/tests/Unit/InMemoryAccessTokenManagerTest.php b/tests/Unit/InMemoryAccessTokenManagerTest.php
@@ -16,7 +16,7 @@ final class InMemoryAccessTokenManagerTest extends TestCase
 {
     public function testClearExpired(): void
     {
-        $inMemoryAccessTokenManager = new InMemoryAccessTokenManager(false);
+        $inMemoryAccessTokenManager = new InMemoryAccessTokenManager(true);
 
         $testData = $this->buildClearExpiredTestData();
 
@@ -34,7 +34,7 @@ public function testClearExpired(): void
 
     public function testClearExpiredWithoutSavingAccessToken(): void
     {
-        $inMemoryAccessTokenManager = new InMemoryAccessTokenManager(true);
+        $inMemoryAccessTokenManager = new InMemoryAccessTokenManager(false);
 
         $testData = $this->buildClearExpiredTestData();