From 067f06b1660db7a6b29dddaf6896649474e27fff Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Mon, 1 Aug 2022 11:38:54 -0400 Subject: [PATCH] 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 | 5 + .../Session/Capability/IHandleSessions.php | 10 ++ src/Core/Session/Type/AbstractSession.php | 14 +++ src/Module/Security/Login.php | 98 +++++++++++-------- src/Module/Security/OpenID.php | 6 +- src/Module/Security/TwoFactor/Recovery.php | 2 + src/Module/Security/TwoFactor/Trust.php | 3 +- src/Security/Authentication.php | 24 +++-- 8 files changed, 102 insertions(+), 60 deletions(-) diff --git a/src/Core/Session.php b/src/Core/Session.php index aa8de99d5c..059cd499c0 100644 --- a/src/Core/Session.php +++ b/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); diff --git a/src/Core/Session/Capability/IHandleSessions.php b/src/Core/Session/Capability/IHandleSessions.php index 98c46ad4d9..d0b649845b 100644 --- a/src/Core/Session/Capability/IHandleSessions.php +++ b/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. diff --git a/src/Core/Session/Type/AbstractSession.php b/src/Core/Session/Type/AbstractSession.php index a24b6e478e..0e2f884a32 100644 --- a/src/Core/Session/Type/AbstractSession.php +++ b/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} */ diff --git a/src/Module/Security/Login.php b/src/Module/Security/Login.php index 080c3ef3ca..80320403a6 100644 --- a/src/Module/Security/Login.php +++ b/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); - } elseif (!empty($return_path)) { - Session::set('return_path', $return_path); + $this->baseUrl->redirect($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'); - Session::clear(); - Session::set('return_path', $return_path); + $this->session->clear(); // OpenId Login if ( - empty($_POST['password']) - && (!empty($_POST['openid_url']) - || !empty($_POST['username'])) + empty($request['password']) + && (!empty($request['openid_url']) + || !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') { - DI::auth()->withPassword( + if (!empty($request['auth-params']) && $request['auth-params'] === 'login') { + $this->auth->withPassword( DI::app(), - trim($_POST['username']), - trim($_POST['password']), - !empty($_POST['remember']) + trim($request['username']), + trim($request['password']), + !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 - * back to after login completes - * @param bool $register If $register == true provide a registration link. - * This will most always depend on the value of config.register_policy. - * @param array $hiddens optional + * @param string|null $return_path The path relative to the base the user should be sent back to after login completes. + * @param bool $register If $register == true provide a registration link. + * This will almost always depend on the value of config.register_policy. * * @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'); - Session::remove('openid_attributes'); + DI::session()->remove('openid_identity'); + 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); } diff --git a/src/Module/Security/OpenID.php b/src/Module/Security/OpenID.php index fec00f8ea1..7dbb765c6d 100644 --- a/src/Module/Security/OpenID.php +++ b/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 - // and we fell through - DI::baseUrl()->redirect(); + $this->baseUrl->redirect(DI::session()->pop('return_path', '')); } // 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)); diff --git a/src/Module/Security/TwoFactor/Recovery.php b/src/Module/Security/TwoFactor/Recovery.php index ca7215da43..6d8d91dbea 100644 --- a/src/Module/Security/TwoFactor/Recovery.php +++ b/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.')); } diff --git a/src/Module/Security/TwoFactor/Trust.php b/src/Module/Security/TwoFactor/Trust.php index 4ba4ba69b3..1f5fb7418f 100644 --- a/src/Module/Security/TwoFactor/Trust.php +++ b/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]); diff --git a/src/Security/Authentication.php b/src/Security/Authentication.php index 9f45516f7d..d1e64534c6 100644 --- a/src/Security/Authentication.php +++ b/src/Security/Authentication.php @@ -244,15 +244,19 @@ class Authentication /** * Attempts to authenticate using login/password * - * @param App $a The Friendica Application context - * @param string $username User name - * @param string $password Clear password - * @param bool $remember Whether to set the session remember flag + * @param App $a The Friendica Application context + * @param string $username + * @param string $password Clear password + * @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 Exception A general Exception (like SQL Grammar exceptions) + * @throws HTTPException\ForbiddenException + * @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')); - } } } -- 2.39.5