Merge pull request #35783 from nextcloud/feat/inject-controller-services-into-methods

Inject services into controller methods

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(),

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,
 			);
 		});
 

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;

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'

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);