feat(app framework)!: Inject services into controller methods

Usually Nextcloud DI goes through constructor injection. This has the
implication that each instance of a class builds the full DI tree. That
is the injected services, their services, etc. Occasionally there is a
service that is only needed for one controller method. Then the DI tree
is build regardless if used or not.

If services are injected into the method, we only build the DI tree if
that method gets executed.

This is also how Laravel allows injection.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index ef1c1fbd94f5076ce77134e13b61b87f60377b02..8fd994ae6483b2a7dc0f0c0da235c1f8120c5fb6 100644 (file)
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -71,7 +71,6 @@ class LoginController extends Controller {
        private IURLGenerator $urlGenerator;
        private Defaults $defaults;
        private Throttler $throttler;
-       private Chain $loginChain;
        private IInitialStateService $initialStateService;
        private WebAuthnManager $webAuthnManager;
        private IManager $manager;
@@ -86,7 +85,6 @@ class LoginController extends Controller {
                                                                IURLGenerator $urlGenerator,
                                                                Defaults $defaults,
                                                                Throttler $throttler,
-                                                               Chain $loginChain,
                                                                IInitialStateService $initialStateService,
                                                                WebAuthnManager $webAuthnManager,
                                                                IManager $manager,
@@ -99,7 +97,6 @@ class LoginController extends Controller {
                $this->urlGenerator = $urlGenerator;
                $this->defaults = $defaults;
                $this->throttler = $throttler;
-               $this->loginChain = $loginChain;
                $this->initialStateService = $initialStateService;
                $this->webAuthnManager = $webAuthnManager;
                $this->manager = $manager;
@@ -290,15 +287,10 @@ class LoginController extends Controller {
         * @NoCSRFRequired
         * @BruteForceProtection(action=login)
         *
-        * @param string $user
-        * @param string $password
-        * @param string $redirect_url
-        * @param string $timezone
-        * @param string $timezone_offset
-        *
         * @return RedirectResponse
         */
-       public function tryLogin(string $user,
+       public function tryLogin(Chain $loginChain,
+                                                        string $user,
                                                         string $password,
                                                         string $redirect_url = null,
                                                         string $timezone = '',
@@ -330,7 +322,7 @@ class LoginController extends Controller {
                        $timezone,
                        $timezone_offset
                );
-               $result = $this->loginChain->process($data);
+               $result = $loginChain->process($data);
                if (!$result->isSuccess()) {
                        return $this->createLoginFailedResponse(
                                $data->getUsername(),

diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php
index 462f9b978fd91c213b6995cf282694eb8be20eb8..6e0e452bccd40706859708b9ec2370bef82e4b33 100644 (file)
--- a/lib/private/AppFramework/DependencyInjection/DIContainer.php
+++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php
@@ -191,7 +191,8 @@ class DIContainer extends SimpleContainer implements IAppContainer {
                                $c->get(IConfig::class),
                                $c->get(IDBConnection::class),
                                $c->get(LoggerInterface::class),
-                               $c->get(EventLogger::class)
+                               $c->get(EventLogger::class),
+                               $c,
                        );
                });
 

diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php
index aaaf2ba65602f218abaa20fac74b2a7b4a9316a4..cc288edc96611123d5eca91bee832d41b891454d 100644 (file)
--- a/lib/private/AppFramework/Http/Dispatcher.php
+++ b/lib/private/AppFramework/Http/Dispatcher.php
@@ -42,6 +42,7 @@ use OCP\AppFramework\Http\Response;
 use OCP\Diagnostics\IEventLogger;
 use OCP\IConfig;
 use OCP\IRequest;
+use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -73,6 +74,8 @@ class Dispatcher {
        /** @var IEventLogger */
        private $eventLogger;
 
+       private ContainerInterface $appContainer;
+
        /**
         * @param Http $protocol the http protocol with contains all status headers
         * @param MiddlewareDispatcher $middlewareDispatcher the dispatcher which
@@ -92,7 +95,8 @@ class Dispatcher {
                                                                IConfig $config,
                                                                ConnectionAdapter $connection,
                                                                LoggerInterface $logger,
-                                                               IEventLogger $eventLogger) {
+                                                               IEventLogger $eventLogger,
+                                                               ContainerInterface $appContainer) {
                $this->protocol = $protocol;
                $this->middlewareDispatcher = $middlewareDispatcher;
                $this->reflector = $reflector;
@@ -101,6 +105,7 @@ class Dispatcher {
                $this->connection = $connection;
                $this->logger = $logger;
                $this->eventLogger = $eventLogger;
+               $this->appContainer = $appContainer;
        }
 
 
@@ -216,6 +221,8 @@ class Dispatcher {
                                $value = false;
                        } elseif ($value !== null && \in_array($type, $types, true)) {
                                settype($value, $type);
+                       } elseif ($value === null && $type !== null && $this->appContainer->has($type)) {
+                               $value = $this->appContainer->get($type);
                        }
 
                        $arguments[] = $value;

diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index 8a206a429bd3b24133b1988ca79b6bcfce1142d9..2c8aedf99c4cb777aee761faa561edff49b1713d 100644 (file)
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -78,9 +78,6 @@ class LoginControllerTest extends TestCase {
        /** @var Throttler|MockObject */
        private $throttler;
 
-       /** @var LoginChain|MockObject */
-       private $chain;
-
        /** @var IInitialStateService|MockObject */
        private $initialStateService;
 
@@ -104,7 +101,6 @@ class LoginControllerTest extends TestCase {
                $this->twoFactorManager = $this->createMock(Manager::class);
                $this->defaults = $this->createMock(Defaults::class);
                $this->throttler = $this->createMock(Throttler::class);
-               $this->chain = $this->createMock(LoginChain::class);
                $this->initialStateService = $this->createMock(IInitialStateService::class);
                $this->webAuthnManager = $this->createMock(\OC\Authentication\WebAuthn\Manager::class);
                $this->notificationManager = $this->createMock(IManager::class);
@@ -134,7 +130,6 @@ class LoginControllerTest extends TestCase {
                        $this->urlGenerator,
                        $this->defaults,
                        $this->throttler,
-                       $this->chain,
                        $this->initialStateService,
                        $this->webAuthnManager,
                        $this->notificationManager,
@@ -448,11 +443,11 @@ class LoginControllerTest extends TestCase {
                $this->assertEquals($expectedResponse, $this->loginController->showLoginForm('0', ''));
        }
 
-       public function testLoginWithInvalidCredentials() {
+       public function testLoginWithInvalidCredentials(): void {
                $user = 'MyUserName';
                $password = 'secret';
                $loginPageUrl = '/login?redirect_url=/apps/files';
-
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -464,7 +459,7 @@ class LoginControllerTest extends TestCase {
                        '/apps/files'
                );
                $loginResult = LoginResult::failure($loginData, LoginController::LOGIN_MSG_INVALIDPASSWORD);
-               $this->chain->expects($this->once())
+               $loginChain->expects($this->once())
                        ->method('process')
                        ->with($this->equalTo($loginData))
                        ->willReturn($loginResult);
@@ -479,7 +474,7 @@ class LoginControllerTest extends TestCase {
                $expected = new RedirectResponse($loginPageUrl);
                $expected->throttle(['user' => 'MyUserName']);
 
-               $response = $this->loginController->tryLogin($user, $password, '/apps/files');
+               $response = $this->loginController->tryLogin($loginChain, $user, $password, '/apps/files');
 
                $this->assertEquals($expected, $response);
        }
@@ -487,7 +482,7 @@ class LoginControllerTest extends TestCase {
        public function testLoginWithValidCredentials() {
                $user = 'MyUserName';
                $password = 'secret';
-
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -498,7 +493,7 @@ class LoginControllerTest extends TestCase {
                        $password
                );
                $loginResult = LoginResult::success($loginData);
-               $this->chain->expects($this->once())
+               $loginChain->expects($this->once())
                        ->method('process')
                        ->with($this->equalTo($loginData))
                        ->willReturn($loginResult);
@@ -508,7 +503,7 @@ class LoginControllerTest extends TestCase {
                        ->willReturn('/default/foo');
 
                $expected = new RedirectResponse('/default/foo');
-               $this->assertEquals($expected, $this->loginController->tryLogin($user, $password));
+               $this->assertEquals($expected, $this->loginController->tryLogin($loginChain, $user, $password));
        }
 
        public function testLoginWithoutPassedCsrfCheckAndNotLoggedIn(): void {
@@ -519,7 +514,7 @@ class LoginControllerTest extends TestCase {
                        ->willReturn('jane');
                $password = 'secret';
                $originalUrl = 'another%20url';
-
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -533,7 +528,7 @@ class LoginControllerTest extends TestCase {
                $this->userSession->expects($this->never())
                        ->method('createRememberMeToken');
 
-               $response = $this->loginController->tryLogin('Jane', $password, $originalUrl);
+               $response = $this->loginController->tryLogin($loginChain, 'Jane', $password, $originalUrl);
 
                $expected = new RedirectResponse('');
                $expected->throttle(['user' => 'Jane']);
@@ -549,7 +544,7 @@ class LoginControllerTest extends TestCase {
                $password = 'secret';
                $originalUrl = 'another url';
                $redirectUrl = 'http://localhost/another url';
-
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -571,7 +566,7 @@ class LoginControllerTest extends TestCase {
                        ->with('remember_login_cookie_lifetime')
                        ->willReturn(1234);
 
-               $response = $this->loginController->tryLogin('Jane', $password, $originalUrl);
+               $response = $this->loginController->tryLogin($loginChain, 'Jane', $password, $originalUrl);
 
                $expected = new RedirectResponse($redirectUrl);
                $this->assertEquals($expected, $response);
@@ -581,7 +576,7 @@ class LoginControllerTest extends TestCase {
                $user = 'MyUserName';
                $password = 'secret';
                $redirectUrl = 'https://next.cloud/apps/mail';
-
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -593,7 +588,7 @@ class LoginControllerTest extends TestCase {
                        '/apps/mail'
                );
                $loginResult = LoginResult::success($loginData);
-               $this->chain->expects($this->once())
+               $loginChain->expects($this->once())
                        ->method('process')
                        ->with($this->equalTo($loginData))
                        ->willReturn($loginResult);
@@ -606,12 +601,13 @@ class LoginControllerTest extends TestCase {
                        ->willReturn($redirectUrl);
                $expected = new RedirectResponse($redirectUrl);
 
-               $response = $this->loginController->tryLogin($user, $password, '/apps/mail');
+               $response = $this->loginController->tryLogin($loginChain, $user, $password, '/apps/mail');
 
                $this->assertEquals($expected, $response);
        }
 
        public function testToNotLeakLoginName() {
+               $loginChain = $this->createMock(LoginChain::class);
                $this->request
                        ->expects($this->once())
                        ->method('passesCSRFCheck')
@@ -624,7 +620,7 @@ class LoginControllerTest extends TestCase {
                        '/apps/files'
                );
                $loginResult = LoginResult::failure($loginData, LoginController::LOGIN_MSG_INVALIDPASSWORD);
-               $this->chain->expects($this->once())
+               $loginChain->expects($this->once())
                        ->method('process')
                        ->with($this->equalTo($loginData))
                        ->willReturnCallback(function (LoginData $data) use ($loginResult) {
@@ -643,6 +639,7 @@ class LoginControllerTest extends TestCase {
                $expected->throttle(['user' => 'john']);
 
                $response = $this->loginController->tryLogin(
+                       $loginChain,
                        'john@doe.com',
                        'just wrong',
                        '/apps/files'

diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php
index 016bd7efb58da08ee56f556133544b75f230b13a..7f81701a8b3ded426dd7d7667d2bce5aa17565eb 100644 (file)
--- a/tests/lib/AppFramework/Http/DispatcherTest.php
+++ b/tests/lib/AppFramework/Http/DispatcherTest.php
@@ -36,6 +36,7 @@ use OCP\Diagnostics\IEventLogger;
 use OCP\IConfig;
 use OCP\IRequest;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Container\ContainerInterface;
 use Psr\Log\LoggerInterface;
 use OCP\IRequestId;
 
@@ -104,10 +105,10 @@ class DispatcherTest extends \Test\TestCase {
        private $config;
        /** @var LoggerInterface|MockObject  */
        private $logger;
-       /**
-        * @var IEventLogger|MockObject
-        */
+       /** @var IEventLogger|MockObject */
        private $eventLogger;
+       /** @var ContainerInterface|MockObject */
+       private $container;
 
        protected function setUp(): void {
                parent::setUp();
@@ -116,6 +117,7 @@ class DispatcherTest extends \Test\TestCase {
                $this->config = $this->createMock(IConfig::class);
                $this->logger = $this->createMock(LoggerInterface::class);
                $this->eventLogger = $this->createMock(IEventLogger::class);
+               $this->container = $this->createMock(ContainerInterface::class);
                $app = $this->getMockBuilder(
                        'OC\AppFramework\DependencyInjection\DIContainer')
                        ->disableOriginalConstructor()
@@ -154,7 +156,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container,
                );
 
                $this->response = $this->createMock(Response::class);
@@ -330,7 +333,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);
 
@@ -362,7 +366,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);
 
@@ -397,7 +402,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);
 
@@ -431,7 +437,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);
 
@@ -466,7 +473,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);
 
@@ -503,7 +511,8 @@ class DispatcherTest extends \Test\TestCase {
                        $this->config,
                        \OC::$server->getDatabaseConnection(),
                        $this->logger,
-                       $this->eventLogger
+                       $this->eventLogger,
+                       $this->container
                );
                $controller = new TestController('app', $this->request);