[TASK] Avoid $GLOBALS['TYPO3_REQUEST'] usage in ext:felogin

This patch removes all usages of `$GLOBALS['TYPO3_REQUEST']` in ext:felogin classes, by passing the extbase request object to methods requiring access to request parameters. As a result, the class `ServerRequestHandler` has been removed, which was a wrapper class for `$GLOBALS['TYPO3_REQUEST']`. Besides the removal of `$GLOBALS['TYPO3_REQUEST']`, the following cleanup and modernisation tasks have been made: - Use constructor property promotion - Use constructor dependency injection - Marked `LoginController` as `@internal` - Corrected some comments in tests and method doc headers - Simplify mock creation in some tests Resolves: #99777 Releases: main Signed-off-by: Torben Hansen <derhansen@gmail.com> Change-Id: I94034fdcc83fc78c34173b119d829279f8260de8 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/77652 Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch> Tested-by: core-ci <typo3@b13.com> Reviewed-by: Benjamin Franzke <bfr@qbus.de> Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
TYPO3 · Feb 4, 2023 · c01cd1c · c01cd1c
1 parent 1c77c83
commit c01cd1c
Show file tree

Hide file tree

Showing 15 changed files with 229 additions and 244 deletions.
diff --git a/typo3/sysext/felogin/Classes/Controller/AbstractLoginFormController.php b/typo3/sysext/felogin/Classes/Controller/AbstractLoginFormController.php
@@ -21,6 +21,9 @@
 use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Extbase\Mvc\Controller\ActionController;
 
+/*
+ * @internal this is a concrete TYPO3 implementation and solely used for EXT:felogin and not part of TYPO3's Core API.
+ */
 abstract class AbstractLoginFormController extends ActionController
 {
     /**

diff --git a/typo3/sysext/felogin/Classes/Controller/LoginController.php b/typo3/sysext/felogin/Classes/Controller/LoginController.php
@@ -22,7 +22,6 @@
 use TYPO3\CMS\Core\Context\Context;
 use TYPO3\CMS\Core\Context\UserAspect;
 use TYPO3\CMS\Core\Security\RequestToken;
-use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Extbase\Http\ForwardResponse;
 use TYPO3\CMS\FrontendLogin\Configuration\RedirectConfiguration;
 use TYPO3\CMS\FrontendLogin\Event\BeforeRedirectEvent;
@@ -31,11 +30,12 @@
 use TYPO3\CMS\FrontendLogin\Event\LogoutConfirmedEvent;
 use TYPO3\CMS\FrontendLogin\Event\ModifyLoginFormViewEvent;
 use TYPO3\CMS\FrontendLogin\Redirect\RedirectHandler;
-use TYPO3\CMS\FrontendLogin\Redirect\ServerRequestHandler;
 use TYPO3\CMS\FrontendLogin\Service\UserService;
 
 /**
  * Used for plugin login
+ *
+ * @internal this is a concrete TYPO3 implementation and solely used for EXT:felogin and not part of TYPO3's Core API.
  */
 class LoginController extends AbstractLoginFormController
 {
@@ -51,18 +51,18 @@ class LoginController extends AbstractLoginFormController
 
     public function __construct(
         protected RedirectHandler $redirectHandler,
-        protected ServerRequestHandler $requestHandler,
-        protected UserService $userService
+        protected UserService $userService,
+        protected Context $context
     ) {
-        $this->userAspect = GeneralUtility::makeInstance(Context::class)->getAspect('frontend.user');
+        $this->userAspect = $context->getAspect('frontend.user');
     }
 
     /**
      * Initialize redirects
      */
     public function initializeAction(): void
     {
-        $this->loginType = (string)$this->requestHandler->getPropertyFromGetAndPost('logintype');
+        $this->loginType = (string)($this->request->getParsedBody()['logintype'] ?? $this->request->getQueryParams()['logintype'] ?? '');
         $this->configuration = RedirectConfiguration::fromSettings($this->settings);
 
         if ($this->isLoginOrLogoutInProgress() && !$this->isRedirectDisabled()) {
@@ -72,6 +72,7 @@ public function initializeAction(): void
             }
 
             $this->redirectUrl = $this->redirectHandler->processRedirect(
+                $this->request,
                 $this->loginType,
                 $this->configuration,
                 $this->request->hasArgument('redirectReferrer') ? $this->request->getArgument('redirectReferrer') : ''
@@ -104,9 +105,9 @@ public function loginAction(): ResponseInterface
                 'cookieWarning' => $this->showCookieWarning,
                 'messageKey' => $this->getStatusMessageKey(),
                 'permaloginStatus' => $this->getPermaloginStatus(),
-                'redirectURL' => $this->redirectHandler->getLoginFormRedirectUrl($this->configuration, $this->isRedirectDisabled()),
+                'redirectURL' => $this->redirectHandler->getLoginFormRedirectUrl($this->request, $this->configuration, $this->isRedirectDisabled()),
                 'redirectReferrer' => $this->request->hasArgument('redirectReferrer') ? (string)$this->request->getArgument('redirectReferrer') : '',
-                'referer' => $this->requestHandler->getPropertyFromGetAndPost('referer'),
+                'referer' => (string)($this->request->getParsedBody()['referer'] ?? $this->request->getQueryParams()['referer'] ?? ''),
                 'noRedirect' => $this->isRedirectDisabled(),
                 'requestToken' => RequestToken::create('core/user-auth/fe')
                     ->withMergedParams(['pid' => implode(',', $this->getStorageFolders())]),
@@ -147,15 +148,20 @@ public function overviewAction(bool $showLoginMessage = false): ResponseInterfac
     public function logoutAction(int $redirectPageLogout = 0): ResponseInterface
     {
         if (($redirectResponse = $this->handleRedirect()) !== null) {
-            return $this->handleRedirect();
+            return $redirectResponse;
         }
 
         $this->view->assignMultiple(
             [
                 'cookieWarning' => $this->showCookieWarning,
                 'user' => $this->userService->getFeUserData(),
                 'noRedirect' => $this->isRedirectDisabled(),
-                'actionUri' => $this->redirectHandler->getLogoutFormRedirectUrl($this->configuration, $redirectPageLogout, $this->isRedirectDisabled()),
+                'actionUri' => $this->redirectHandler->getLogoutFormRedirectUrl(
+                    $this->request,
+                    $this->configuration,
+                    $redirectPageLogout,
+                    $this->isRedirectDisabled()
+                ),
             ]
         );
 

diff --git a/typo3/sysext/felogin/Classes/Controller/PasswordRecoveryController.php b/typo3/sysext/felogin/Classes/Controller/PasswordRecoveryController.php
@@ -50,7 +50,7 @@ public function __construct(
     }
 
     /**
-     * Shows the recovery form. If $userIdentifier is set an email will be sent, if the corresponding user exists and
+     * Shows the recovery form. If $userIdentifier is set, an email will be sent, if the corresponding user exists and
      * has a valid email address set.
      */
     public function recoveryAction(string $userIdentifier = null): ResponseInterface
@@ -67,7 +67,7 @@ public function recoveryAction(string $userIdentifier = null): ResponseInterface
         if ($userData && GeneralUtility::validEmail($userData['email'])) {
             $hash = $this->recoveryConfiguration->getForgotHash();
             $this->userRepository->updateForgotHashForUserByUid($userData['uid'], GeneralUtility::hmac($hash));
-            $this->recoveryService->sendRecoveryEmail($userData, $hash);
+            $this->recoveryService->sendRecoveryEmail($this->request, $userData, $hash);
         }
 
         if ($this->exposeNoneExistentUser($userData)) {

diff --git a/typo3/sysext/felogin/Classes/Domain/Repository/FrontendUserGroupRepository.php b/typo3/sysext/felogin/Classes/Domain/Repository/FrontendUserGroupRepository.php
@@ -19,7 +19,6 @@
 
 use TYPO3\CMS\Core\Database\Connection;
 use TYPO3\CMS\Core\Database\ConnectionPool;
-use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\FrontendLogin\Service\UserService;
 
 /**
@@ -30,10 +29,10 @@ class FrontendUserGroupRepository
     protected Connection $connection;
     protected string $table;
 
-    public function __construct(UserService $userService)
+    public function __construct(UserService $userService, ConnectionPool $connectionPool)
     {
         $this->table = $userService->getFeUserGroupTable();
-        $this->connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable($this->getTable());
+        $this->connection = $connectionPool->getConnectionForTable($this->getTable());
     }
 
     public function getTable(): string

diff --git a/typo3/sysext/felogin/Classes/Domain/Repository/FrontendUserRepository.php b/typo3/sysext/felogin/Classes/Domain/Repository/FrontendUserRepository.php
@@ -20,7 +20,6 @@
 use TYPO3\CMS\Core\Context\Context;
 use TYPO3\CMS\Core\Database\Connection;
 use TYPO3\CMS\Core\Database\ConnectionPool;
-use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\FrontendLogin\Service\UserService;
 
 /**
@@ -30,9 +29,12 @@ class FrontendUserRepository
 {
     protected Connection $connection;
 
-    public function __construct(protected UserService $userService, protected Context $context)
-    {
-        $this->connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable($this->getTable());
+    public function __construct(
+        protected UserService $userService,
+        protected Context $context,
+        ConnectionPool $connectionPool
+    ) {
+        $this->connection = $connectionPool->getConnectionForTable($this->getTable());
     }
 
     public function getTable(): string

diff --git a/typo3/sysext/felogin/Classes/Redirect/RedirectHandler.php b/typo3/sysext/felogin/Classes/Redirect/RedirectHandler.php
@@ -19,7 +19,9 @@
 
 use TYPO3\CMS\Core\Authentication\LoginType;
 use TYPO3\CMS\Core\Context\Context;
+use TYPO3\CMS\Extbase\Mvc\RequestInterface;
 use TYPO3\CMS\FrontendLogin\Configuration\RedirectConfiguration;
+use TYPO3\CMS\FrontendLogin\Validation\RedirectUrlValidator;
 
 /**
  * Resolve felogin related redirects based on the current login type and the selected configuration (redirect mode)
@@ -31,8 +33,8 @@ class RedirectHandler
     protected bool $userIsLoggedIn = false;
 
     public function __construct(
-        protected ServerRequestHandler $requestHandler,
         protected RedirectModeHandler $redirectModeHandler,
+        protected RedirectUrlValidator $redirectUrlValidator,
         Context $context
     ) {
         $this->userIsLoggedIn = (bool)$context->getPropertyFromAspect('frontend.user', 'isLoggedIn');
@@ -41,20 +43,20 @@ public function __construct(
     /**
      * Process redirect modes. This method searches for a redirect url using all configured modes and returns it.
      */
-    public function processRedirect(string $loginType, RedirectConfiguration $configuration, string $redirectModeReferrer): string
+    public function processRedirect(RequestInterface $request, string $loginType, RedirectConfiguration $configuration, string $redirectModeReferrer): string
     {
         if ($this->isUserLoginFailedAndLoginErrorActive($configuration->getModes(), $loginType)) {
-            return $this->redirectModeHandler->redirectModeLoginError($configuration->getPageOnLoginError());
+            return $this->redirectModeHandler->redirectModeLoginError($request, $configuration->getPageOnLoginError());
         }
 
         $redirectUrlList = [];
         foreach ($configuration->getModes() as $redirectMode) {
             $redirectUrl = '';
 
             if ($loginType === LoginType::LOGIN) {
-                $redirectUrl = $this->handleSuccessfulLogin($redirectMode, $configuration->getPageOnLogin(), $configuration->getDomains(), $redirectModeReferrer);
+                $redirectUrl = $this->handleSuccessfulLogin($request, $redirectMode, $configuration->getPageOnLogin(), $configuration->getDomains(), $redirectModeReferrer);
             } elseif ($loginType === LoginType::LOGOUT) {
-                $redirectUrl = $this->handleSuccessfulLogout($redirectMode, $configuration->getPageOnLogout());
+                $redirectUrl = $this->handleSuccessfulLogout($request, $redirectMode, $configuration->getPageOnLogout());
             }
 
             if ($redirectUrl !== '') {
@@ -68,31 +70,31 @@ public function processRedirect(string $loginType, RedirectConfiguration $config
     /**
      * Get alternative logout form redirect url if logout and page not accessible
      */
-    protected function getLogoutRedirectUrl(array $redirectModes, int $redirectPageLogout = 0): string
+    protected function getLogoutRedirectUrl(RequestInterface $request, array $redirectModes, int $redirectPageLogout = 0): string
     {
         if ($this->userIsLoggedIn && $this->isRedirectModeActive($redirectModes, RedirectMode::LOGOUT)) {
-            return $this->redirectModeHandler->redirectModeLogout($redirectPageLogout);
+            return $this->redirectModeHandler->redirectModeLogout($request, $redirectPageLogout);
         }
-        return $this->getGetpostRedirectUrl($redirectModes);
+        return $this->getGetpostRedirectUrl($request, $redirectModes);
     }
 
     /**
      * Is used for alternative redirect urls on redirect mode "getpost"
      */
-    protected function getGetpostRedirectUrl(array $redirectModes): string
+    protected function getGetpostRedirectUrl(RequestInterface $request, array $redirectModes): string
     {
         return $this->isRedirectModeActive($redirectModes, RedirectMode::GETPOST)
-            ? $this->requestHandler->getRedirectUrlRequestParam()
+            ? $this->getRedirectUrlRequestParam($request)
             : '';
     }
 
     /**
      * Handle redirect mode logout
      */
-    protected function handleSuccessfulLogout(string $redirectMode, int $redirectPageLogout): string
+    protected function handleSuccessfulLogout(RequestInterface $request, string $redirectMode, int $redirectPageLogout): string
     {
         if ($redirectMode === RedirectMode::LOGOUT) {
-            return $this->redirectModeHandler->redirectModeLogout($redirectPageLogout);
+            return $this->redirectModeHandler->redirectModeLogout($request, $redirectPageLogout);
         }
         return '';
     }
@@ -119,7 +121,7 @@ protected function fetchReturnUrlFromList(array $redirectUrlList, string $redire
     /**
      * Generate redirect_url for case that the user was successfully logged in
      */
-    protected function handleSuccessfulLogin(string $redirectMode, int $redirectPageLogin = 0, string $domains = '', string $redirectModeReferrer = ''): string
+    protected function handleSuccessfulLogin(RequestInterface $request, string $redirectMode, int $redirectPageLogin = 0, string $domains = '', string $redirectModeReferrer = ''): string
     {
         if (!$this->userIsLoggedIn) {
             return '';
@@ -128,22 +130,22 @@ protected function handleSuccessfulLogin(string $redirectMode, int $redirectPage
         // Logintype is needed because the login-page wouldn't be accessible anymore after a login (would always redirect)
         switch ($redirectMode) {
             case RedirectMode::GROUP_LOGIN:
-                $redirectUrl = $this->redirectModeHandler->redirectModeGroupLogin();
+                $redirectUrl = $this->redirectModeHandler->redirectModeGroupLogin($request);
                 break;
             case RedirectMode::USER_LOGIN:
-                $redirectUrl = $this->redirectModeHandler->redirectModeUserLogin();
+                $redirectUrl = $this->redirectModeHandler->redirectModeUserLogin($request);
                 break;
             case RedirectMode::LOGIN:
-                $redirectUrl = $this->redirectModeHandler->redirectModeLogin($redirectPageLogin);
+                $redirectUrl = $this->redirectModeHandler->redirectModeLogin($request, $redirectPageLogin);
                 break;
             case RedirectMode::GETPOST:
-                $redirectUrl = $this->requestHandler->getRedirectUrlRequestParam();
+                $redirectUrl = $this->getRedirectUrlRequestParam($request);
                 break;
             case RedirectMode::REFERER:
-                $redirectUrl = $this->redirectModeHandler->redirectModeReferrer($redirectModeReferrer);
+                $redirectUrl = $this->redirectModeHandler->redirectModeReferrer($request, $redirectModeReferrer);
                 break;
             case RedirectMode::REFERER_DOMAINS:
-                $redirectUrl = $this->redirectModeHandler->redirectModeRefererDomains($domains, $redirectModeReferrer);
+                $redirectUrl = $this->redirectModeHandler->redirectModeRefererDomains($request, $domains, $redirectModeReferrer);
                 break;
             default:
                 $redirectUrl = '';
@@ -167,22 +169,42 @@ protected function isRedirectModeActive(array $redirectModes, string $mode): boo
     /**
      * Returns the redirect Url that should be used in login form template for GET/POST redirect mode
      */
-    public function getLoginFormRedirectUrl(RedirectConfiguration $configuration, bool $redirectDisabled): string
-    {
+    public function getLoginFormRedirectUrl(
+        RequestInterface $request,
+        RedirectConfiguration $configuration,
+        bool $redirectDisabled
+    ): string {
         if (!$redirectDisabled) {
-            return $this->getGetpostRedirectUrl($configuration->getModes());
+            return $this->getGetpostRedirectUrl($request, $configuration->getModes());
         }
         return '';
     }
 
     /**
      * Returns the redirect Url that should be used in logout form
      */
-    public function getLogoutFormRedirectUrl(RedirectConfiguration $configuration, int $redirectPageLogout, bool $redirectDisabled): string
-    {
+    public function getLogoutFormRedirectUrl(
+        RequestInterface $request,
+        RedirectConfiguration $configuration,
+        int $redirectPageLogout,
+        bool $redirectDisabled
+    ): string {
         if (!$redirectDisabled) {
-            return $this->getLogoutRedirectUrl($configuration->getModes(), $redirectPageLogout);
+            return $this->getLogoutRedirectUrl($request, $configuration->getModes(), $redirectPageLogout);
         }
-        return $this->requestHandler->getRedirectUrlRequestParam();
+        return $this->getRedirectUrlRequestParam($request);
+    }
+
+    /**
+     * Returns validated redirect url contained in request param return_url or redirect_url
+     */
+    private function getRedirectUrlRequestParam(RequestInterface $request): string
+    {
+        // If config.typolinkLinkAccessRestrictedPages is set, the var is return_url
+        $returnUrlFromRequest = (string)($request->getParsedBody()['return_url'] ?? $request->getQueryParams()['return_url'] ?? null);
+        $redirectUrlFromRequest = (string)($request->getParsedBody()['redirect_url'] ?? $request->getQueryParams()['redirect_url'] ?? null);
+        $redirectUrl = $returnUrlFromRequest ?: $redirectUrlFromRequest;
+
+        return $this->redirectUrlValidator->isValid($request, $redirectUrl) ? $redirectUrl : '';
     }
 }