Rework return_path session key handling

- Add new IHandleSessions::pop() method
- Remove redirection from Authentication::setForUser()
- Add explicit return_path form parameter to Login::form()

src/Core/Session.php

@@ -44,6 +44,11 @@ class Session
 		return DI::session()->get($name, $defaults);
 	}
 
+	public static function pop($name, $defaults = null)
+	{
+		return DI::session()->pop($name, $defaults);
+	}
+
 	public static function set($name, $value)
 	{
 		DI::session()->set($name, $value);

src/Core/Session/Capability/IHandleSessions.php

@@ -54,6 +54,16 @@ interface IHandleSessions
 	 */
 	public function get(string $name, $defaults = null);
 
+	/**
+	 * Retrieves a value from the provided key if it exists and removes it from session
+	 *
+	 * @param string $name
+	 * @param mixed  $defaults
+	 *
+	 * @return mixed
+	 */
+	public function pop(string $name, $defaults = null);
+
 	/**
 	 * Sets a single session variable.
 	 * Overrides value of existing key.

src/Core/Session/Type/AbstractSession.php

@@ -52,6 +52,20 @@ class AbstractSession implements IHandleSessions
 		return $_SESSION[$name] ?? $defaults;
 	}
 
+	/**
+	 * {@inheritDoc}
+	 */
+	public function pop(string $name, $defaults = null)
+	{
+		$value = $defaults;
+		if ($this->exists($name)) {
+			$value = $this->get($name);
+			$this->remove($name);
+		}
+
+		return $value;
+	}
+
 	/**
 	 * {@inheritDoc}
 	 */

src/Module/Security/Login.php

@@ -21,54 +21,76 @@
 
 namespace Friendica\Module\Security;
 
+use Friendica\App;
 use Friendica\BaseModule;
+use Friendica\Core\Config\Capability\IManageConfigValues;
 use Friendica\Core\Hook;
+use Friendica\Core\L10n;
 use Friendica\Core\Renderer;
-use Friendica\Core\Session;
+use Friendica\Core\Session\Capability\IHandleSessions;
 use Friendica\DI;
 use Friendica\Module\Register;
+use Friendica\Module\Response;
+use Friendica\Security\Authentication;
+use Friendica\Util\Profiler;
+use Psr\Log\LoggerInterface;
 
 /**
  * Login module
  */
 class Login extends BaseModule
 {
+	/** @var Authentication */
+	private $auth;
+
+	/** @var IManageConfigValues */
+	private $config;
+
+	/** @var IHandleSessions */
+	private $session;
+
+	public function __construct(Authentication $auth, IManageConfigValues $config, IHandleSessions $session, L10n $l10n, App\BaseURL $baseUrl, App\Arguments $args, LoggerInterface $logger, Profiler $profiler, Response $response, array $server, array $parameters = [])
+	{
+		parent::__construct($l10n, $baseUrl, $args, $logger, $profiler, $response, $server, $parameters);
+
+		$this->auth    = $auth;
+		$this->config  = $config;
+		$this->session = $session;
+	}
+
 	protected function content(array $request = []): string
 	{
-		$return_path = $_REQUEST['return_path'] ?? '' ;
+		$return_path = $request['return_path'] ?? $this->session->pop('return_path', '') ;
 
 		if (local_user()) {
-			DI::baseUrl()->redirect($return_path);
+			$this->baseUrl->redirect($return_path);
-		} elseif (!empty($return_path)) {
-			Session::set('return_path', $return_path);
 		}
 
-		return self::form(Session::get('return_path'), intval(DI::config()->get('config', 'register_policy')) !== \Friendica\Module\Register::CLOSED);
+		return self::form($return_path, intval($this->config->get('config', 'register_policy')) !== \Friendica\Module\Register::CLOSED);
 	}
 
 	protected function post(array $request = [])
 	{
-		$return_path = Session::get('return_path');
+		$this->session->clear();
-		Session::clear();
-		Session::set('return_path', $return_path);
 
 		// OpenId Login
 		if (
-			empty($_POST['password'])
+			empty($request['password'])
-			&& (!empty($_POST['openid_url'])
+			&& (!empty($request['openid_url'])
-				|| !empty($_POST['username']))
+				|| !empty($request['username']))
 		) {
-			$openid_url = trim(($_POST['openid_url'] ?? '') ?: $_POST['username']);
+			$openid_url = trim(($request['openid_url'] ?? '') ?: $request['username']);
 
-			DI::auth()->withOpenId($openid_url, !empty($_POST['remember']));
+			$this->auth->withOpenId($openid_url, !empty($request['remember']));
 		}
 
-		if (!empty($_POST['auth-params']) && $_POST['auth-params'] === 'login') {
+		if (!empty($request['auth-params']) && $request['auth-params'] === 'login') {
-			DI::auth()->withPassword(
+			$this->auth->withPassword(
 				DI::app(),
-				trim($_POST['username']),
+				trim($request['username']),
-				trim($_POST['password']),
+				trim($request['password']),
-				!empty($_POST['remember'])
+				!empty($request['remember']),
+				$request['return_path'] ?? ''
 			);
 		}
 	}
@@ -76,26 +98,23 @@ class Login extends BaseModule
 	/**
 	 * Wrapper for adding a login box.
 	 *
-	 * @param string $return_path  The path relative to the base the user should be sent
+	 * @param string|null $return_path The path relative to the base the user should be sent back to after login completes.
-	 *                             back to after login completes
+	 * @param bool        $register    If $register == true provide a registration link.
-	 * @param bool   $register     If $register == true provide a registration link.
+	 *                                 This will almost always depend on the value of config.register_policy.
-	 *                             This will most always depend on the value of config.register_policy.
-	 * @param array  $hiddens      optional
 	 *
 	 * @return string Returns the complete html for inserting into the page
 	 *
 	 * @throws \Friendica\Network\HTTPException\InternalServerErrorException
+	 * @throws \Friendica\Network\HTTPException\ServiceUnavailableException
 	 * @hooks 'login_hook' string $o
 	 */
-	public static function form($return_path = null, $register = false, $hiddens = [])
+	public static function form(string $return_path = null, bool $register = false): string
 	{
-		$o = '';
-
 		$noid = DI::config()->get('system', 'no_openid');
 
 		if ($noid) {
-			Session::remove('openid_identity');
+			DI::session()->remove('openid_identity');
-			Session::remove('openid_attributes');
+			DI::session()->remove('openid_attributes');
 		}
 
 		$reg = false;
@@ -107,10 +126,6 @@ class Login extends BaseModule
 			];
 		}
 
-		if (is_null($return_path)) {
-			$return_path = DI::args()->getQueryString();
-		}
-
 		if (local_user()) {
 			$tpl = Renderer::getMarkupTemplate('logout.tpl');
 		} else {
@@ -122,13 +137,12 @@ class Login extends BaseModule
 			);
 
 			$tpl = Renderer::getMarkupTemplate('login.tpl');
-			$_SESSION['return_path'] = $return_path;
 		}
 
-		if (!empty(Session::get('openid_identity'))) {
+		if (!empty(DI::session()->get('openid_identity'))) {
 			$openid_title = DI::l10n()->t('Your OpenID: ');
 			$openid_readonly = true;
-			$identity = Session::get('openid_identity');
+			$identity = DI::session()->get('openid_identity');
 			$username_desc = DI::l10n()->t('Please enter your username and password to add the OpenID to your existing account.');
 		} else {
 			$openid_title = DI::l10n()->t('Or login using OpenID: ');
@@ -137,7 +151,7 @@ class Login extends BaseModule
 			$username_desc = '';
 		}
 
-		$o .= Renderer::replaceMacros(
+		$o = Renderer::replaceMacros(
 			$tpl,
 			[
 				'$dest_url'     => DI::baseUrl()->get(true) . '/login',
@@ -151,7 +165,7 @@ class Login extends BaseModule
 				'$openid'       => !$noid,
 				'$lopenid'      => ['openid_url', $openid_title, $identity, '', $openid_readonly],
 
-				'$hiddens'      => $hiddens,
+				'$hiddens'      => ['return_path' => $return_path ?? DI::args()->getQueryString()],
 
 				'$register'     => $reg,
 
@@ -174,14 +188,14 @@ class Login extends BaseModule
 	/**
 	 * Get the URL to the register page and add OpenID parameters to it
 	 */
-	private static function getRegisterURL()
+	private static function getRegisterURL(): string
 	{
-		if (empty(Session::get('openid_identity'))) {
+		if (empty(DI::session()->get('openid_identity'))) {
 			return 'register';
 		}
 
 		$args = [];
-		$attr = Session::get('openid_attributes', []);
+		$attr = DI::session()->get('openid_attributes', []);
 
 		if (is_array($attr) && count($attr)) {
 			foreach ($attr as $k => $v) {
@@ -218,7 +232,7 @@ class Login extends BaseModule
 			$args['photo'] = $photo;
 		}
 
-		$args['openid_url'] = trim(Session::get('openid_identity'));
+		$args['openid_url'] = trim(DI::session()->get('openid_identity'));
 
 		return 'register?' . http_build_query($args);
 	}

src/Module/Security/OpenID.php

@@ -73,9 +73,7 @@ class OpenID extends BaseModule
 
 					DI::auth()->setForUser(DI::app(), $user, true, true);
 
-					// just in case there was no return url set
+					$this->baseUrl->redirect(DI::session()->pop('return_path', ''));
-					// and we fell through
-					DI::baseUrl()->redirect();
 				}
 
 				// Successful OpenID login - but we can't match it to an existing account.
@@ -84,7 +82,7 @@ class OpenID extends BaseModule
 				$session->set('openid_identity', $authId);
 
 				// Detect the server URL
-				$open_id_obj = new LightOpenID(DI::baseUrl()->getHostName());
+				$open_id_obj = new LightOpenID(DI::baseUrl()->getHostname());
 				$open_id_obj->identity = $authId;
 				$session->set('openid_server', $open_id_obj->discover($open_id_obj->identity));
 

src/Module/Security/TwoFactor/Recovery.php

@@ -73,6 +73,8 @@ class Recovery extends BaseModule
 				info($this->t('Remaining recovery codes: %d', RecoveryCode::countValidForUser(local_user())));
 
 				$this->auth->setForUser($this->app, User::getById($this->app->getLoggedInUserId()), true, true);
+
+				$this->baseUrl->redirect($this->session->pop('return_path', ''));
 			} else {
 				notice($this->t('Invalid code, please retry.'));
 			}

src/Module/Security/TwoFactor/Trust.php

@@ -102,6 +102,7 @@ class Trust extends BaseModule
 
 			try {
 				$this->auth->setForUser($this->app, User::getById($this->app->getLoggedInUserId()), true, true);
+				$this->baseUrl->redirect($this->session->pop('return_path', ''));
 			} catch (FoundException | TemporaryRedirectException | MovedPermanentlyException $e) {
 				// exception wanted!
 				throw $e;
@@ -122,7 +123,7 @@ class Trust extends BaseModule
 				$trustedBrowser = $this->trustedBrowserRepository->selectOneByHash($this->cookie->get('2fa_cookie_hash'));
 				if (!$trustedBrowser->trusted) {
 					$this->auth->setForUser($this->app, User::getById($this->app->getLoggedInUserId()), true, true);
-					$this->baseUrl->redirect();
+					$this->baseUrl->redirect($this->session->pop('return_path', ''));
 				}
 			} catch (TrustedBrowserNotFoundException $exception) {
 				$this->logger->notice('Trusted Browser of the cookie not found.', ['cookie_hash' => $this->cookie->get('trusted'), 'uid' => $this->app->getLoggedInUserId(), 'exception' => $exception]);

src/Security/Authentication.php

@@ -244,15 +244,19 @@ class Authentication
 	/**
 	 * Attempts to authenticate using login/password
 	 *
-	 * @param App    $a        The Friendica Application context
+	 * @param App    $a           The Friendica Application context
-	 * @param string $username User name
+	 * @param string $username
-	 * @param string $password Clear password
+	 * @param string $password    Clear password
-	 * @param bool   $remember Whether to set the session remember flag
+	 * @param bool   $remember    Whether to set the session remember flag
+	 * @param string $return_path The relative path to redirect the user to after authentication
 	 *
-	 * @throws HttpException\InternalServerErrorException In case of Friendica internal exceptions
+	 * @throws HTTPException\ForbiddenException
-	 * @throws Exception A general Exception (like SQL Grammar exceptions)
+	 * @throws HTTPException\FoundException
+	 * @throws HTTPException\InternalServerErrorException In case of Friendica internal exceptions
+	 * @throws HTTPException\MovedPermanentlyException
+	 * @throws HTTPException\TemporaryRedirectException
 	 */
-	public function withPassword(App $a, string $username, string $password, bool $remember)
+	public function withPassword(App $a, string $username, string $password, bool $remember, string $return_path = '')
 	{
 		$record = null;
 
@@ -289,8 +293,6 @@ class Authentication
 
 		$this->setForUser($a, $record, true, true);
 
-		$return_path = $this->session->get('return_path', '');
-		$this->session->remove('return_path');
 
 		$this->baseUrl->redirect($return_path);
 	}
@@ -382,10 +384,6 @@ class Authentication
 
 		if ($login_initial) {
 			Hook::callAll('logged_in', $user_record);
-
-			if (DI::args()->getModuleName() !== 'home' && $this->session->exists('return_path')) {
-				$this->baseUrl->redirect($this->session->get('return_path'));
-			}
 		}
 	}