Merge pull request #11714 from nupplaphil/bug/trust

[2FA] Add a lot of log-points

src/App/BaseURL.php

@@ -444,6 +444,10 @@ class BaseURL
 	 * @param string $toUrl The destination URL (Default is empty, which is the default page of the Friendica node)
 	 * @param bool   $ssl   if true, base URL will try to get called with https:// (works just for relative paths)
 	 *
+	 * @throws HTTPException\FoundException
+	 * @throws HTTPException\MovedPermanentlyException
+	 * @throws HTTPException\TemporaryRedirectException
+	 *
 	 * @throws HTTPException\InternalServerErrorException In Case the given URL is not relative to the Friendica node
 	 */
 	public function redirect(string $toUrl = '', bool $ssl = false)

src/Core/System.php

@@ -441,6 +441,12 @@ class System
 	 *
 	 * @param string $url  The new Location to redirect
 	 * @param int    $code The redirection code, which is used (Default is 302)
+	 *
+	 * @throws FoundException
+	 * @throws MovedPermanentlyException
+	 * @throws TemporaryRedirectException
+	 *
+	 * @throws \Friendica\Network\HTTPException\InternalServerErrorException
 	 */
 	public static function externalRedirect($url, $code = 302)
 	{

src/Module/Security/TwoFactor/SignOut.php

@@ -108,7 +108,7 @@ class SignOut extends BaseModule
 				info($this->t('Logged out.'));
 				$this->baseUrl->redirect();
 			}
-		} catch (NotFoundException $exception) {
+		} catch (TwoFactor\Exception\TrustedBrowserNotFoundException $exception) {
 			$this->cookie->clear();
 			$this->session->clear();
 

src/Module/Security/TwoFactor/Trust.php

@@ -29,7 +29,9 @@ use Friendica\Core\Session\Capability\IHandleSessions;
 use Friendica\Model\User;
 use Friendica\Model\User\Cookie;
 use Friendica\Module\Response;
-use Friendica\Network\HTTPException\NotFoundException;
+use Friendica\Network\HTTPException\FoundException;
+use Friendica\Network\HTTPException\MovedPermanentlyException;
+use Friendica\Network\HTTPException\TemporaryRedirectException;
 use Friendica\Security\Authentication;
 use Friendica\Util\Profiler;
 use Friendica\Security\TwoFactor;
@@ -53,23 +55,24 @@ class Trust extends BaseModule
 	/** @var TwoFactor\Factory\TrustedBrowser  */
 	protected $trustedBrowserFactory;
 	/** @var TwoFactor\Repository\TrustedBrowser  */
-	protected $trustedBrowserRepositoy;
+	protected $trustedBrowserRepository;
 
 	public function __construct(App $app, Authentication $auth, L10n $l10n, App\BaseURL $baseUrl, App\Arguments $args, LoggerInterface $logger, Profiler $profiler, IHandleSessions $session, Cookie $cookie, TwoFactor\Factory\TrustedBrowser $trustedBrowserFactory, TwoFactor\Repository\TrustedBrowser $trustedBrowserRepositoy, Response $response, array $server, array $parameters = [])
 	{
 		parent::__construct($l10n, $baseUrl, $args, $logger, $profiler, $response, $server, $parameters);
 
-		$this->app                     = $app;
+		$this->app                      = $app;
-		$this->auth                    = $auth;
+		$this->auth                     = $auth;
-		$this->session                 = $session;
+		$this->session                  = $session;
-		$this->cookie                  = $cookie;
+		$this->cookie                   = $cookie;
-		$this->trustedBrowserFactory   = $trustedBrowserFactory;
+		$this->trustedBrowserFactory    = $trustedBrowserFactory;
-		$this->trustedBrowserRepositoy = $trustedBrowserRepositoy;
+		$this->trustedBrowserRepository = $trustedBrowserRepositoy;
 	}
 
 	protected function post(array $request = [])
 	{
 		if (!local_user() || !$this->session->get('2fa')) {
+			$this->logger->info('Invalid call', ['request' => $request]);
 			return;
 		}
 
@@ -82,16 +85,27 @@ class Trust extends BaseModule
 				case 'trust':
 				case 'dont_trust':
 					$trustedBrowser = $this->trustedBrowserFactory->createForUserWithUserAgent(local_user(), $this->server['HTTP_USER_AGENT'], $action === 'trust');
-					$this->trustedBrowserRepositoy->save($trustedBrowser);
+					try {
+						$this->trustedBrowserRepository->save($trustedBrowser);
 
-					// The string is sent to the browser to be sent back with each request
+						// The string is sent to the browser to be sent back with each request
-					if (!$this->cookie->set('2fa_cookie_hash', $trustedBrowser->cookie_hash)) {
+						if (!$this->cookie->set('2fa_cookie_hash', $trustedBrowser->cookie_hash)) {
-						notice($this->t('Couldn\'t save browser to Cookie.'));
+							notice($this->t('Couldn\'t save browser to Cookie.'));
-					};
+						};
+					} catch (TwoFactor\Exception\TrustedBrowserPersistenceException $exception) {
+						$this->logger->warning('Unexpected error when saving the trusted browser.', ['trustedBrowser' => $trustedBrowser, 'exception' => $exception]);
+					}
 					break;
 			}
 
-			$this->auth->setForUser($this->app, User::getById($this->app->getLoggedInUserId()), true, true);
+			try {
+				$this->auth->setForUser($this->app, User::getById($this->app->getLoggedInUserId()), true, true);
+			} catch (FoundException | TemporaryRedirectException | MovedPermanentlyException $e) {
+				// exception wanted!
+				throw $e;
+			} catch (\Exception $e) {
+				$this->logger->warning('Unexpected error during authentication.', ['user' => $this->app->getLoggedInUserId(), 'exception' => $exception]);
+			}
 		}
 	}
 
@@ -103,13 +117,17 @@ class Trust extends BaseModule
 
 		if ($this->cookie->get('2fa_cookie_hash')) {
 			try {
-				$trustedBrowser = $this->trustedBrowserRepositoy->selectOneByHash($this->cookie->get('2fa_cookie_hash'));
+				$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();
 				}
-			} catch (NotFoundException $exception) {
+			} catch (TwoFactor\Exception\TrustedBrowserNotFoundException $exception) {
 				$this->logger->notice('Trusted Browser of the cookie not found.', ['cookie_hash' => $this->cookie->get('trusted'), 'uid' => $this->app->getLoggedInUserId(), 'exception' => $exception]);
+			} catch (TwoFactor\Exception\TrustedBrowserPersistenceException $exception) {
+				$this->logger->warning('Unexpected persistence exception.', ['cookie_hash' => $this->cookie->get('trusted'), 'uid' => $this->app->getLoggedInUserId(), 'exception' => $exception]);
+			} catch (\Exception $exception) {
+				$this->logger->warning('Unexpected exception.', ['cookie_hash' => $this->cookie->get('trusted'), 'uid' => $this->app->getLoggedInUserId(), 'exception' => $exception]);
 			}
 		}
 

src/Module/Settings/TwoFactor/Trusted.php

@@ -77,7 +77,7 @@ class Trusted extends BaseSettings
 			self::checkFormSecurityTokenRedirectOnError('settings/2fa/trusted', 'settings_2fa_trusted');
 
 			switch ($_POST['action']) {
-				case 'remove_all' :
+				case 'remove_all':
 					$this->trustedBrowserRepo->removeAllForUser(local_user());
 					info($this->t('Trusted browsers successfully removed.'));
 					$this->baseUrl->redirect('settings/2fa/trusted?t=' . self::getFormSecurityToken('settings_2fa_password'));
@@ -118,9 +118,9 @@ class Trusted extends BaseSettings
 			$result = $parser->parse($trustedBrowser->user_agent);
 
 			$uaData = [
-				'os' => $result->os->family,
+				'os'              => $result->os->family,
-				'device' => $result->device->family,
+				'device'          => $result->device->family,
-				'browser' => $result->ua->family,
+				'browser'         => $result->ua->family,
 				'trusted_labeled' => $trustedBrowser->trusted ? $this->t('Yes') : $this->t('No'),
 			];
 
@@ -128,21 +128,21 @@ class Trusted extends BaseSettings
 		}, $trustedBrowsers->getArrayCopy());
 
 		return Renderer::replaceMacros(Renderer::getMarkupTemplate('settings/twofactor/trusted_browsers.tpl'), [
-			'$form_security_token' => self::getFormSecurityToken('settings_2fa_trusted'),
+			'$form_security_token'     => self::getFormSecurityToken('settings_2fa_trusted'),
 			'$password_security_token' => self::getFormSecurityToken('settings_2fa_password'),
 
-			'$title'               => $this->t('Two-factor Trusted Browsers'),
+			'$title'            => $this->t('Two-factor Trusted Browsers'),
-			'$message'             => $this->t('Trusted browsers are individual browsers you chose to skip two-factor authentication to access Friendica. Please use this feature sparingly, as it can negate the benefit of two-factor authentication.'),
+			'$message'          => $this->t('Trusted browsers are individual browsers you chose to skip two-factor authentication to access Friendica. Please use this feature sparingly, as it can negate the benefit of two-factor authentication.'),
-			'$device_label'        => $this->t('Device'),
+			'$device_label'     => $this->t('Device'),
-			'$os_label'            => $this->t('OS'),
+			'$os_label'         => $this->t('OS'),
-			'$browser_label'       => $this->t('Browser'),
+			'$browser_label'    => $this->t('Browser'),
-			'$trusted_label'       => $this->t('Trusted'),
+			'$trusted_label'    => $this->t('Trusted'),
-			'$created_label'       => $this->t('Created At'),
+			'$created_label'    => $this->t('Created At'),
-			'$last_used_label'     => $this->t('Last Use'),
+			'$last_used_label'  => $this->t('Last Use'),
-			'$remove_label'        => $this->t('Remove'),
+			'$remove_label'     => $this->t('Remove'),
-			'$remove_all_label'    => $this->t('Remove All'),
+			'$remove_all_label' => $this->t('Remove All'),
 
-			'$trusted_browsers'    => $trustedBrowserDisplay,
+			'$trusted_browsers' => $trustedBrowserDisplay,
 		]);
 	}
 }

src/Security/Authentication.php

@@ -304,8 +304,13 @@ class Authentication
 	 * @param bool  $interactive
 	 * @param bool  $login_refresh
 	 *
+	 * @throws HTTPException\FoundException
+	 * @throws HTTPException\MovedPermanentlyException
+	 * @throws HTTPException\TemporaryRedirectException
+	 * @throws HTTPException\ForbiddenException
+
 	 * @throws HTTPException\InternalServerErrorException In case of Friendica specific exceptions
-	 * @throws Exception In case of general Exceptions (like SQL Grammar exceptions)
+	 *
 	 */
 	public function setForUser(App $a, array $user_record, bool $login_initial = false, bool $interactive = false, bool $login_refresh = false)
 	{

src/Security/TwoFactor/Exception/TrustedBrowserNotFoundException.php

@@ -0,0 +1,32 @@
+<?php
+/**
+ * @copyright Copyright (C) 2010-2022, the Friendica project
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Friendica\Security\TwoFactor\Exception;
+
+use Exception;
+
+class TrustedBrowserNotFoundException extends \RuntimeException
+{
+	public function __construct($message = '', Exception $previous = null)
+	{
+		parent::__construct($message, 404, $previous);
+	}
+}

src/Security/TwoFactor/Exception/TrustedBrowserPersistenceException.php

@@ -0,0 +1,32 @@
+<?php
+/**
+ * @copyright Copyright (C) 2010-2022, the Friendica project
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Friendica\Security\TwoFactor\Exception;
+
+use Throwable;
+
+class TrustedBrowserPersistenceException extends \RuntimeException
+{
+	public function __construct($message = "", Throwable $previous = null)
+	{
+		parent::__construct($message, 500, $previous);
+	}
+}

src/Security/TwoFactor/Factory/TrustedBrowser.php

@@ -27,6 +27,11 @@ use Friendica\Util\Strings;
 
 class TrustedBrowser extends BaseFactory
 {
+	/**
+	 * Creates a new Trusted Browser based on the current user environment
+	 *
+	 * @throws \Exception In case something really unexpected happens
+	 */
 	public function createForUserWithUserAgent(int $uid, string $userAgent, bool $trusted): \Friendica\Security\TwoFactor\Model\TrustedBrowser
 	{
 		$trustedHash = Strings::getRandomHex();

src/Security/TwoFactor/Model/TrustedBrowser.php

@@ -67,6 +67,12 @@ class TrustedBrowser extends BaseEntity
 		$this->last_used   = $last_used;
 	}
 
+	/**
+	 * Records if the trusted browser was used
+	 *
+	 * @return void
+	 * @throws \Exception unexpected DateTime exception happened
+	 */
 	public function recordUse()
 	{
 		$this->last_used = DateTimeFormat::utcNow();

src/Security/TwoFactor/Repository/TrustedBrowser.php

@@ -21,10 +21,8 @@
 
 namespace Friendica\Security\TwoFactor\Repository;
 
-use Friendica\Security\TwoFactor\Model;
+use Friendica\Security\TwoFactor;
-use Friendica\Security\TwoFactor\Collection\TrustedBrowsers;
 use Friendica\Database\Database;
-use Friendica\Network\HTTPException\NotFoundException;
 use Psr\Log\LoggerInterface;
 
 class TrustedBrowser
@@ -35,83 +33,122 @@ class TrustedBrowser
 	/** @var LoggerInterface  */
 	protected $logger;
 
-	/** @var \Friendica\Security\TwoFactor\Factory\TrustedBrowser  */
+	/** @var TwoFactor\Factory\TrustedBrowser  */
 	protected $factory;
 
 	protected static $table_name = '2fa_trusted_browser';
 
-	public function __construct(Database $database, LoggerInterface $logger, \Friendica\Security\TwoFactor\Factory\TrustedBrowser $factory = null)
+	public function __construct(Database $database, LoggerInterface $logger, TwoFactor\Factory\TrustedBrowser $factory = null)
 	{
-		$this->db = $database;
+		$this->db      = $database;
-		$this->logger = $logger;
+		$this->logger  = $logger;
-		$this->factory = $factory ?? new \Friendica\Security\TwoFactor\Factory\TrustedBrowser($logger);
+		$this->factory = $factory ?? new TwoFactor\Factory\TrustedBrowser($logger);
 	}
 
 	/**
 	 * @param string $cookie_hash
-	 * @return Model\TrustedBrowser|null
+	 *
-	 * @throws \Exception
+	 * @return TwoFactor\Model\TrustedBrowser|null
+	 *
+	 * @throws TwoFactor\Exception\TrustedBrowserPersistenceException
+	 * @throws TwoFactor\Exception\TrustedBrowserNotFoundException
 	 */
-	public function selectOneByHash(string $cookie_hash): Model\TrustedBrowser
+	public function selectOneByHash(string $cookie_hash): TwoFactor\Model\TrustedBrowser
 	{
-		$fields = $this->db->selectFirst(self::$table_name, [], ['cookie_hash' => $cookie_hash]);
+		try {
+			$fields = $this->db->selectFirst(self::$table_name, [], ['cookie_hash' => $cookie_hash]);
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('Internal server error when retrieving cookie hash \'%s\'', $cookie_hash));
+		}
 		if (!$this->db->isResult($fields)) {
-			throw new NotFoundException('');
+			throw new TwoFactor\Exception\TrustedBrowserNotFoundException(sprintf('Cookie hash \'%s\' not found', $cookie_hash));
 		}
 
 		return $this->factory->createFromTableRow($fields);
 	}
 
-	public function selectAllByUid(int $uid): TrustedBrowsers
+	/**
+	 * @param int $uid
+	 *
+	 * @return TwoFactor\Collection\TrustedBrowsers
+	 *
+	 * @throws TwoFactor\Exception\TrustedBrowserPersistenceException
+	 */
+	public function selectAllByUid(int $uid): TwoFactor\Collection\TrustedBrowsers
 	{
-		$rows = $this->db->selectToArray(self::$table_name, [], ['uid' => $uid]);
+		try {
+			$rows = $this->db->selectToArray(self::$table_name, [], ['uid' => $uid]);
 
-		$trustedBrowsers = [];
+			$trustedBrowsers = [];
-		foreach ($rows as $fields) {
+			foreach ($rows as $fields) {
-			$trustedBrowsers[] = $this->factory->createFromTableRow($fields);
+				$trustedBrowsers[] = $this->factory->createFromTableRow($fields);
+			}
+			return new TwoFactor\Collection\TrustedBrowsers($trustedBrowsers);
+
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('selection for uid \'%s\' wasn\'t successful.', $uid));
 		}
-
-		return new TrustedBrowsers($trustedBrowsers);
 	}
 
 	/**
-	 * @param Model\TrustedBrowser $trustedBrowser
+	 * @param TwoFactor\Model\TrustedBrowser $trustedBrowser
+	 *
 	 * @return bool
-	 * @throws \Exception
+	 *
+	 * @throws TwoFactor\Exception\TrustedBrowserPersistenceException
 	 */
-	public function save(Model\TrustedBrowser $trustedBrowser): bool
+	public function save(TwoFactor\Model\TrustedBrowser $trustedBrowser): bool
 	{
-		return $this->db->insert(self::$table_name, $trustedBrowser->toArray(), $this->db::INSERT_UPDATE);
+		try {
+			return $this->db->insert(self::$table_name, $trustedBrowser->toArray(), $this->db::INSERT_UPDATE);
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('Couldn\'t save trusted Browser with cookie_hash \'%s\'', $trustedBrowser->cookie_hash));
+		}
 	}
 
 	/**
-	 * @param Model\TrustedBrowser $trustedBrowser
+	 * @param TwoFactor\Model\TrustedBrowser $trustedBrowser
+	 *
 	 * @return bool
-	 * @throws \Exception
+	 *
+	 * @throws TwoFactor\Exception\TrustedBrowserPersistenceException
 	 */
-	public function remove(Model\TrustedBrowser $trustedBrowser): bool
+	public function remove(TwoFactor\Model\TrustedBrowser $trustedBrowser): bool
 	{
-		return $this->db->delete(self::$table_name, ['cookie_hash' => $trustedBrowser->cookie_hash]);
+		try {
+			return $this->db->delete(self::$table_name, ['cookie_hash' => $trustedBrowser->cookie_hash]);
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('Couldn\'t delete trusted Browser with cookie hash \'%s\'', $trustedBrowser->cookie_hash));
+		}
 	}
 
 	/**
 	 * @param int    $local_user
 	 * @param string $cookie_hash
+	 *
 	 * @return bool
-	 * @throws \Exception
+	 *
+	 * @throws TwoFactor\Exception\TrustedBrowserPersistenceException
 	 */
 	public function removeForUser(int $local_user, string $cookie_hash): bool
 	{
-		return $this->db->delete(self::$table_name, ['cookie_hash' => $cookie_hash,'uid' => $local_user]);
+		try {
+			return $this->db->delete(self::$table_name, ['cookie_hash' => $cookie_hash, 'uid' => $local_user]);
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('Couldn\'t delete trusted Browser for user \'%s\' and cookie hash \'%s\'', $local_user, $cookie_hash));
+		}
 	}
 
 	/**
 	 * @param int $local_user
 	 * @return bool
-	 * @throws \Exception
 	 */
 	public function removeAllForUser(int $local_user): bool
 	{
-		return $this->db->delete(self::$table_name, ['uid' => $local_user]);
+		try {
+			return $this->db->delete(self::$table_name, ['uid' => $local_user]);
+		} catch (\Exception $exception) {
+			throw new TwoFactor\Exception\TrustedBrowserPersistenceException(sprintf('Couldn\'t delete trusted Browsers for user \'%s\'', $local_user));
+		}
 	}
 }