Fix: DI-related issues (#548)

* Use EventDispatcherInterface instead of EventDispatcher

* Remove container from AuthorizeController

* Session can be null in AuthorizeController

* Remove BaseCommand and inject dependencies for commands

* Fix comment

* Fix tests for commands

* Fix tests for AuthorizeController

Author: alanpoulain

Command/BaseCommand.php

@@ -11,13 +11,30 @@
 
 namespace FOS\OAuthServerBundle\Command;
 
+use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use FOS\OAuthServerBundle\Model\TokenManagerInterface;
 use FOS\OAuthServerBundle\Model\AuthCodeManagerInterface;
 
-class CleanCommand extends BaseCommand
+class CleanCommand extends Command
 {
+    private $accessTokenManager;
+    private $refreshTokenManager;
+    private $authCodeManager;
+
+    public function __construct(
+        TokenManagerInterface $accessTokenManager,
+        TokenManagerInterface $refreshTokenManager,
+        AuthCodeManagerInterface $authCodeManager)
+    {
+        parent::__construct();
+
+        $this->accessTokenManager = $accessTokenManager;
+        $this->refreshTokenManager = $refreshTokenManager;
+        $this->authCodeManager = $authCodeManager;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -41,19 +58,9 @@ protected function configure()
      */
     protected function execute(InputInterface $input, OutputInterface $output)
     {
-        $services = [
-            'fos_oauth_server.access_token_manager'  => 'Access token',
-            'fos_oauth_server.refresh_token_manager' => 'Refresh token',
-            'fos_oauth_server.auth_code_manager'     => 'Auth code',
-        ];
-
-        foreach ($services as $service => $name) {
-            /** @var $instance TokenManagerInterface */
-            $instance = $this->getContainer()->get($service);
-            if ($instance instanceof TokenManagerInterface || $instance instanceof AuthCodeManagerInterface) {
-                $result = $instance->deleteExpired();
-                $output->writeln(sprintf('Removed <info>%d</info> items from <comment>%s</comment> storage.', $result, $name));
-            }
+        foreach ([$this->accessTokenManager, $this->refreshTokenManager, $this->authCodeManager] as $service) {
+            $result = $service->deleteExpired();
+            $output->writeln(sprintf('Removed <info>%d</info> items from <comment>%s</comment> storage.', $result, get_class($service)));
         }
     }
 }

Command/CleanCommand.php

@@ -11,14 +11,24 @@
 
 namespace FOS\OAuthServerBundle\Command;
 
+use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use FOS\OAuthServerBundle\Model\ClientManagerInterface;
 
-class CreateClientCommand extends BaseCommand
+class CreateClientCommand extends Command
 {
+    private $clientManager;
+
+    public function __construct(ClientManagerInterface $clientManager)
+    {
+        parent::__construct();
+
+        $this->clientManager = $clientManager;
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -61,18 +71,14 @@ protected function execute(InputInterface $input, OutputInterface $output)
 
         $io->title('Client Credentials');
 
-        // Get the client manager
-        /** @var ClientManagerInterface $clientManager */
-        $clientManager = $this->getContainer()->get('fos_oauth_server.client_manager.default');
-
         // Create a new client
-        $client = $clientManager->createClient();
+        $client = $this->clientManager->createClient();
 
         $client->setRedirectUris($input->getOption('redirect-uri'));
         $client->setAllowedGrantTypes($input->getOption('grant-type'));
 
         // Save the client
-        $clientManager->updateClient($client);
+        $this->clientManager->updateClient($client);
 
         // Give the credentials back to the user
         $headers = ['Client ID', 'Client Secret'];

Command/CreateClientCommand.php

@@ -17,10 +17,8 @@
 use FOS\OAuthServerBundle\Model\ClientManagerInterface;
 use OAuth2\OAuth2;
 use OAuth2\OAuth2ServerException;
-use Symfony\Component\DependencyInjection\ContainerAwareInterface;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Bundle\FrameworkBundle\Templating\EngineInterface;
-use Symfony\Component\EventDispatcher\EventDispatcher;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\Form\Form;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
@@ -37,18 +35,13 @@
  *
  * @author Chris Jones <[email protected]>
  */
-class AuthorizeController implements ContainerAwareInterface
+class AuthorizeController
 {
     /**
      * @var ClientInterface
      */
     private $client;
 
-    /**
-     * @var ContainerInterface
-     */
-    protected $container;
-
     /**
      * @var SessionInterface
      */
@@ -100,48 +93,38 @@ class AuthorizeController implements ContainerAwareInterface
     private $templateEngineType;
 
     /**
-     * @var EventDispatcher
+     * @var EventDispatcherInterface
      */
     private $eventDispatcher;
 
-    /**
-     * Sets the container.
-     *
-     * @param ContainerInterface|null $container A ContainerInterface instance or null
-     */
-    public function setContainer(ContainerInterface $container = null)
-    {
-        $this->container = $container;
-    }
-
     /**
      * This controller had been made as a service due to support symfony 4 where all* services are private by default.
-     * Thus, there is considered a bad practice to fetch services directly from container.
-     * @todo This controller could be refactored to do not rely on so many dependencies
+     * Thus, this is considered a bad practice to fetch services directly from container.
+     * @todo This controller could be refactored to not rely on so many dependencies
      *
-     * @param RequestStack           $requestStack
-     * @param SessionInterface       $session
-     * @param Form                   $authorizeForm
-     * @param AuthorizeFormHandler   $authorizeFormHandler
-     * @param OAuth2                 $oAuth2Server
-     * @param EngineInterface        $templating
-     * @param TokenStorageInterface  $tokenStorage
-     * @param UrlGeneratorInterface  $router
-     * @param ClientManagerInterface $clientManager
-     * @param EventDispatcher        $eventDispatcher
-     * @param string                 $templateEngineType
+     * @param RequestStack             $requestStack
+     * @param Form                     $authorizeForm
+     * @param AuthorizeFormHandler     $authorizeFormHandler
+     * @param OAuth2                   $oAuth2Server
+     * @param EngineInterface          $templating
+     * @param TokenStorageInterface    $tokenStorage
+     * @param UrlGeneratorInterface    $router
+     * @param ClientManagerInterface   $clientManager
+     * @param EventDispatcherInterface $eventDispatcher
+     * @param SessionInterface         $session
+     * @param string                   $templateEngineType
      */
     public function __construct(
         RequestStack $requestStack,
-        SessionInterface $session,
         Form $authorizeForm,
         AuthorizeFormHandler $authorizeFormHandler,
         OAuth2 $oAuth2Server,
         EngineInterface $templating,
         TokenStorageInterface $tokenStorage,
         UrlGeneratorInterface $router,
         ClientManagerInterface $clientManager,
-        EventDispatcher $eventDispatcher,
+        EventDispatcherInterface $eventDispatcher,
+        SessionInterface $session = null,
         $templateEngineType = 'twig'
     ) {
         $this->requestStack = $requestStack;
@@ -168,7 +151,7 @@ public function authorizeAction(Request $request)
             throw new AccessDeniedException('This user does not have access to this section.');
         }
 
-        if (true === $this->session->get('_fos_oauth_server.ensure_logout')) {
+        if ($this->session && true === $this->session->get('_fos_oauth_server.ensure_logout')) {
             $this->session->invalidate(600);
             $this->session->set('_fos_oauth_server.ensure_logout', true);
         }
@@ -209,7 +192,7 @@ public function authorizeAction(Request $request)
      */
     protected function processSuccess(UserInterface $user, AuthorizeFormHandler $formHandler, Request $request)
     {
-        if (true === $this->session->get('_fos_oauth_server.ensure_logout')) {
+        if ($this->session && true === $this->session->get('_fos_oauth_server.ensure_logout')) {
             $this->tokenStorage->setToken(null);
             $this->session->invalidate();
         }

Controller/AuthorizeController.php

@@ -25,7 +25,6 @@
 
         <service id="fos_oauth_server.controller.authorize" class="FOS\OAuthServerBundle\Controller\AuthorizeController" public="true">
             <argument type="service" id="request_stack" />
-            <argument type="service" id="session" />
             <argument type="service" id="fos_oauth_server.authorize.form" />
             <argument type="service" id="fos_oauth_server.authorize.form.handler" />
             <argument type="service" id="fos_oauth_server.server" />
@@ -34,6 +33,7 @@
             <argument type="service" id="router" />
             <argument type="service" id="fos_oauth_server.client_manager" />
             <argument type="service" id="event_dispatcher" />
+            <argument type="service" id="session" on-invalid="null" />
             <argument>%fos_oauth_server.template.engine%</argument>
         </service>
     </services>

Resources/config/authorize.xml

@@ -30,10 +30,14 @@
         <service id="fos_oauth_server.controller.token" alias="FOS\OAuthServerBundle\Controller\TokenController" public="true" />
 
         <service id="fos_oauth_server.clean_command" class="FOS\OAuthServerBundle\Command\CleanCommand">
+            <argument type="service" id="fos_oauth_server.access_token_manager" />
+            <argument type="service" id="fos_oauth_server.refresh_token_manager" />
+            <argument type="service" id="fos_oauth_server.auth_code_manager" />
             <tag name="console.command" />
         </service>
 
         <service id="fos_oauth_server.create_client_command" class="FOS\OAuthServerBundle\Command\CreateClientCommand">
+            <argument type="service" id="fos_oauth_server.client_manager.default" />
             <tag name="console.command" />
         </service>
     </services>