From 54392fab817a1a65175936b6c49c85ad054ff0fb Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Sun, 8 Dec 2019 22:45:34 +0100 Subject: [PATCH] Move Cookie to own class (with tests) Move Authentication to App namespace --- include/api.php | 2 +- index.php | 2 +- mod/dfrn_poll.php | 4 +- mod/openid.php | 2 +- src/App.php | 4 +- src/{Core => App}/Authentication.php | 81 +++++++------ src/Core/Session.php | 68 +---------- src/Model/User.php | 2 + src/Model/User/Cookie.php | 159 +++++++++++++++++++++++++ src/Module/Delegation.php | 2 +- src/Module/Login.php | 2 +- src/Module/Logout.php | 2 +- src/Module/TwoFactor/Recovery.php | 2 +- src/Module/TwoFactor/Verify.php | 2 +- src/Network/FKOAuth1.php | 2 +- tests/src/Model/User/CookieTest.php | 171 +++++++++++++++++++++++++++ 16 files changed, 394 insertions(+), 113 deletions(-) rename src/{Core => App}/Authentication.php (89%) create mode 100644 src/Model/User/Cookie.php create mode 100644 tests/src/Model/User/CookieTest.php diff --git a/include/api.php b/include/api.php index f4b95733e9..77bce65a39 100644 --- a/include/api.php +++ b/include/api.php @@ -12,7 +12,7 @@ use Friendica\Content\ContactSelector; use Friendica\Content\Feature; use Friendica\Content\Text\BBCode; use Friendica\Content\Text\HTML; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Config; use Friendica\Core\Hook; use Friendica\Core\L10n; diff --git a/index.php b/index.php index 00ec0edb75..dbdac5fcbf 100644 --- a/index.php +++ b/index.php @@ -23,5 +23,5 @@ $a->runFrontend( $dice->create(\Friendica\App\Module::class), $dice->create(\Friendica\App\Router::class), $dice->create(\Friendica\Core\Config\PConfiguration::class), - $dice->create(\Friendica\Core\Authentication::class) + $dice->create(\Friendica\App\Authentication::class) ); diff --git a/mod/dfrn_poll.php b/mod/dfrn_poll.php index 892aecacb7..8479217195 100644 --- a/mod/dfrn_poll.php +++ b/mod/dfrn_poll.php @@ -6,7 +6,7 @@ use Friendica\App; use Friendica\BaseObject; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Config; use Friendica\Core\L10n; use Friendica\Core\Logger; @@ -24,7 +24,7 @@ function dfrn_poll_init(App $a) { /** @var Authentication $authentication */ $authentication = BaseObject::getClass(Authentication::class); - $authentication->withSession($a, $_COOKIE); + $authentication->withSession($a); $dfrn_id = $_GET['dfrn_id'] ?? ''; $type = ($_GET['type'] ?? '') ?: 'data'; diff --git a/mod/openid.php b/mod/openid.php index 0c21f7a31c..fc7336a548 100644 --- a/mod/openid.php +++ b/mod/openid.php @@ -5,7 +5,7 @@ use Friendica\App; use Friendica\BaseObject; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Config; use Friendica\Core\L10n; use Friendica\Core\Logger; diff --git a/src/App.php b/src/App.php index 243f5ba075..7a52139d6a 100644 --- a/src/App.php +++ b/src/App.php @@ -8,7 +8,7 @@ use Exception; use Friendica\App\Arguments; use Friendica\App\BaseURL; use Friendica\App\Page; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Config\Cache\ConfigCache; use Friendica\Core\Config\Configuration; use Friendica\Core\Config\PConfiguration; @@ -720,7 +720,7 @@ class App Model\Profile::openWebAuthInit($token); } - $auth->withSession($this, $_COOKIE); + $auth->withSession($this); if (empty($_SESSION['authenticated'])) { header('X-Account-Management-Status: none'); diff --git a/src/Core/Authentication.php b/src/App/Authentication.php similarity index 89% rename from src/Core/Authentication.php rename to src/App/Authentication.php index 39de73ce02..99231cb776 100644 --- a/src/Core/Authentication.php +++ b/src/App/Authentication.php @@ -4,11 +4,15 @@ * @file /src/Core/Authentication.php */ -namespace Friendica\Core; +namespace Friendica\App; use Exception; use Friendica\App; use Friendica\Core\Config\Configuration; +use Friendica\Core\Hook; +use Friendica\Core\PConfig; +use Friendica\Core\Session; +use Friendica\Core\System; use Friendica\Database\Database; use Friendica\Database\DBA; use Friendica\Model\User; @@ -35,6 +39,8 @@ class Authentication private $dba; /** @var LoggerInterface */ private $logger; + /** @var User\Cookie */ + private $cookie; /** * Authentication constructor. @@ -44,63 +50,62 @@ class Authentication * @param L10n $l10n * @param Database $dba * @param LoggerInterface $logger + * @param User\Cookie $cookie */ - public function __construct(Configuration $config, App\BaseURL $baseUrl, L10n $l10n, Database $dba, LoggerInterface $logger) + public function __construct(Configuration $config, App\BaseURL $baseUrl, L10n $l10n, Database $dba, LoggerInterface $logger, User\Cookie $cookie) { $this->config = $config; $this->baseUrl = $baseUrl; $this->l10n = $l10n; $this->dba = $dba; $this->logger = $logger; + $this->cookie = $cookie; } /** * @brief Tries to auth the user from the cookie or session * * @param App $a The Friendica Application context - * @param array $cookie The $_COOKIE array * * @throws HttpException\InternalServerErrorException In case of Friendica internal exceptions * @throws Exception In case of general exceptions (like SQL Grammar) */ - public function withSession(App $a, array $cookie) + public function withSession(App $a) { - // When the "Friendica" cookie is set, take the value to authenticate and renew the cookie. - if (isset($cookie["Friendica"])) { - $data = json_decode($cookie["Friendica"]); - if (isset($data->uid)) { + $data = $this->cookie->getData(); - $user = $this->dba->selectFirst( - 'user', - [], - [ - 'uid' => $data->uid, - 'blocked' => false, - 'account_expired' => false, - 'account_removed' => false, - 'verified' => true, - ] - ); - if (DBA::isResult($user)) { - if (!Session::checkCookie($data->hash, $user)) { - $this->logger->notice("Hash doesn't fit.", ['user' => $data->uid]); - Session::delete(); - $this->baseUrl->redirect(); - } + // When the "Friendica" cookie is set, take the value to authenticate and renew the cookie. + if (isset($data) && isset($data->uid)) { + + $user = $this->dba->selectFirst( + 'user', + [], + [ + 'uid' => $data->uid, + 'blocked' => false, + 'account_expired' => false, + 'account_removed' => false, + 'verified' => true, + ] + ); + if (DBA::isResult($user)) { + if (!$this->cookie->check($data->hash, + $user['password'] ?? '', + $user['prvKey'] ?? '')) { + $this->logger->notice("Hash doesn't fit.", ['user' => $data->uid]); + Session::delete(); + $this->baseUrl->redirect(); + } - // Renew the cookie - // Expires after 7 days by default, - // can be set via system.auth_cookie_lifetime - $authcookiedays = $this->config->get('system', 'auth_cookie_lifetime', 7); - Session::setCookie($authcookiedays * 24 * 60 * 60, $user); + // Renew the cookie + $this->cookie->set($user['uid'], $user['password'], $user['prvKey']); - // Do the authentification if not done by now - if (!Session::get('authenticated')) { - $this->setForUser($a, $user); + // Do the authentification if not done by now + if (!Session::get('authenticated')) { + $this->setForUser($a, $user); - if ($this->config->get('system', 'paranoia')) { - Session::set('addr', $data->ip); - } + if ($this->config->get('system', 'paranoia')) { + Session::set('addr', $data->ip); } } } @@ -241,7 +246,7 @@ class Authentication } if (!$remember) { - Session::setCookie(0); // 0 means delete on browser exit + $this->cookie->clear(); } // if we haven't failed up this point, log them in. @@ -343,7 +348,7 @@ class Authentication */; if (Session::get('remember')) { $a->getLogger()->info('Injecting cookie for remembered user ' . $user_record['nickname']); - Session::setCookie(604800, $user_record); + $this->cookie->set($user_record['uid'], $user_record['password'], $user_record['prvKey']); Session::remove('remember'); } } diff --git a/src/Core/Session.php b/src/Core/Session.php index 02e10482d5..542307a5ca 100644 --- a/src/Core/Session.php +++ b/src/Core/Session.php @@ -6,10 +6,12 @@ namespace Friendica\Core; use Friendica\App; +use Friendica\BaseObject; use Friendica\Core\Session\CacheSessionHandler; use Friendica\Core\Session\DatabaseSessionHandler; use Friendica\Database\DBA; use Friendica\Model\Contact; +use Friendica\Model\User; use Friendica\Util\Strings; /** @@ -171,73 +173,15 @@ class Session return $_SESSION['authenticated']; } - /** - * @brief Calculate the hash that is needed for the "Friendica" cookie - * - * @param array $user Record from "user" table - * - * @return string Hashed data - * @throws \Friendica\Network\HTTPException\InternalServerErrorException - */ - private static function getCookieHashForUser($user) - { - return hash_hmac( - "sha256", - hash_hmac("sha256", $user["password"], $user["prvkey"]), - Config::get("system", "site_prvkey") - ); - } - - /** - * @brief Set the "Friendica" cookie - * - * @param int $time - * @param array $user Record from "user" table - * @throws \Friendica\Network\HTTPException\InternalServerErrorException - */ - public static function setCookie($time, $user = []) - { - if ($time != 0) { - $time = $time + time(); - } - - if ($user) { - $value = json_encode([ - "uid" => $user["uid"], - "hash" => self::getCookieHashForUser($user), - "ip" => ($_SERVER['REMOTE_ADDR'] ?? '') ?: '0.0.0.0' - ]); - } else { - $value = ""; - } - - setcookie("Friendica", $value, $time, "/", "", (Config::get('system', 'ssl_policy') == App\BaseURL::SSL_POLICY_FULL), true); - } - - /** - * @brief Checks if the "Friendica" cookie is set - * - * @param string $hash - * @param array $user Record from "user" table - * - * @return boolean True, if the cookie is set - * - * @throws \Friendica\Network\HTTPException\InternalServerErrorException - */ - public static function checkCookie(string $hash, array $user) - { - return hash_equals( - self::getCookieHashForUser($user), - $hash - ); - } - /** * @brief Kills the "Friendica" cookie and all session data */ public static function delete() { - self::setCookie(-3600); // make sure cookie is deleted on browser close, as a security measure + /** @var User\Cookie $cookie */ + $cookie = BaseObject::getClass(User\Cookie::class); + $cookie->clear(); + $_SESSION = []; session_unset(); session_destroy(); } diff --git a/src/Model/User.php b/src/Model/User.php index 7ecf4a576c..14cb09207b 100644 --- a/src/Model/User.php +++ b/src/Model/User.php @@ -9,12 +9,14 @@ namespace Friendica\Model; use DivineOmega\PasswordExposed; use Exception; +use Friendica\App; use Friendica\Core\Config; use Friendica\Core\Hook; use Friendica\Core\L10n; use Friendica\Core\Logger; use Friendica\Core\PConfig; use Friendica\Core\Protocol; +use Friendica\Core\Session; use Friendica\Core\System; use Friendica\Core\Worker; use Friendica\Database\DBA; diff --git a/src/Model/User/Cookie.php b/src/Model/User/Cookie.php new file mode 100644 index 0000000000..d8404e41e3 --- /dev/null +++ b/src/Model/User/Cookie.php @@ -0,0 +1,159 @@ +remoteAddr = $server['REMOTE_ADDR']; + } + + $this->sslEnabled = $config->get('system', 'ssl_policy') === App\BaseURL::SSL_POLICY_FULL; + $this->sitePrivateKey = $config->get('system', 'site_prvkey'); + + $authCookieDays = $config->get('system', 'auth_cookie_lifetime', + self::DEFAULT_EXPIRE); + $this->lifetime = $authCookieDays * 24 * 60 * 60; + $this->cookie = $cookie; + } + + /** + * Checks if the Friendica cookie is set for a user + * + * @param string $hash The cookie hash + * @param string $password The user password + * @param string $privateKey The private Key of the user + * + * @return boolean True, if the cookie is set + * + */ + public function check(string $hash, string $password, string $privateKey) + { + return hash_equals( + $this->getHash($password, $privateKey), + $hash + ); + } + + /** + * Set the Friendica cookie for a user + * + * @param int $uid The user id + * @param string $password The user password + * @param string $privateKey The user private key + * @param int|null $seconds optional the seconds + * + * @return bool + */ + public function set(int $uid, string $password, string $privateKey, int $seconds = null) + { + if (!isset($seconds)) { + $seconds = $this->lifetime; + } elseif (isset($seconds) && $seconds != 0) { + $seconds = $seconds + time(); + } + + $value = json_encode([ + 'uid' => $uid, + 'hash' => $this->getHash($password, $privateKey), + 'ip' => $this->remoteAddr, + ]); + + return $this->setCookie(self::NAME, $value, $seconds, + '/', '', $this->sslEnabled, true); + } + + /** + * Returns the data of the Friendicas user cookie + * + * @return mixed|null The JSON data, null if not set + */ + public function getData() + { + // When the "Friendica" cookie is set, take the value to authenticate and renew the cookie. + if (isset($this->cookie[self::NAME])) { + $data = json_decode($this->cookie[self::NAME]); + if (!empty($data)) { + return $data; + } + } + + return null; + } + + /** + * Clears the Friendica cookie of this user after leaving the page + */ + public function clear() + { + // make sure cookie is deleted on browser close, as a security measure + return $this->setCookie(self::NAME, '', -3600, + '/', '', $this->sslEnabled, true); + } + + /** + * Calculate the hash that is needed for the Friendica cookie + * + * @param string $password The user password + * @param string $privateKey The private key of the user + * + * @return string Hashed data + */ + private function getHash(string $password, string $privateKey) + { + return hash_hmac( + 'sha256', + hash_hmac('sha256', $password, $privateKey), + $this->sitePrivateKey + ); + } + + /** + * Send a cookie - protected, internal function for test-mocking possibility + * + * @link https://php.net/manual/en/function.setcookie.php + * + * @param string $name + * @param string $value [optional] + * @param int $expire [optional] + * @param string $path [optional] + * @param string $domain [optional] + * @param bool $secure [optional] + * @param bool $httponly [optional]

+ * + * @return bool If output exists prior to calling this function, + * + * @since 4.0 + * @since 5.0 + */ + protected function setCookie(string $name, string $value = null, int $expire = null, + string $path = null, string $domain = null, + bool $secure = null, bool $httponly = null) + { + return setcookie($name, $value, $expire, $path, $domain, $secure, $httponly); + } +} diff --git a/src/Module/Delegation.php b/src/Module/Delegation.php index 2e3a5cafea..7d2e686725 100644 --- a/src/Module/Delegation.php +++ b/src/Module/Delegation.php @@ -3,7 +3,7 @@ namespace Friendica\Module; use Friendica\BaseModule; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Hook; use Friendica\Core\L10n; use Friendica\Core\Renderer; diff --git a/src/Module/Login.php b/src/Module/Login.php index 8ecf3e40c8..d7c537839d 100644 --- a/src/Module/Login.php +++ b/src/Module/Login.php @@ -7,7 +7,7 @@ namespace Friendica\Module; use Friendica\BaseModule; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Config; use Friendica\Core\Hook; use Friendica\Core\L10n; diff --git a/src/Module/Logout.php b/src/Module/Logout.php index 877a8cda05..89910107d0 100644 --- a/src/Module/Logout.php +++ b/src/Module/Logout.php @@ -6,7 +6,7 @@ namespace Friendica\Module; use Friendica\BaseModule; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Cache; use Friendica\Core\Hook; use Friendica\Core\L10n; diff --git a/src/Module/TwoFactor/Recovery.php b/src/Module/TwoFactor/Recovery.php index f1454469f2..371b7d7d53 100644 --- a/src/Module/TwoFactor/Recovery.php +++ b/src/Module/TwoFactor/Recovery.php @@ -3,7 +3,7 @@ namespace Friendica\Module\TwoFactor; use Friendica\BaseModule; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\L10n; use Friendica\Core\Renderer; use Friendica\Core\Session; diff --git a/src/Module/TwoFactor/Verify.php b/src/Module/TwoFactor/Verify.php index e4a0b2ff18..62b988ef99 100644 --- a/src/Module/TwoFactor/Verify.php +++ b/src/Module/TwoFactor/Verify.php @@ -3,7 +3,7 @@ namespace Friendica\Module\TwoFactor; use Friendica\BaseModule; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\L10n; use Friendica\Core\PConfig; use Friendica\Core\Renderer; diff --git a/src/Network/FKOAuth1.php b/src/Network/FKOAuth1.php index 81c0c1b291..a3dde38d26 100644 --- a/src/Network/FKOAuth1.php +++ b/src/Network/FKOAuth1.php @@ -5,7 +5,7 @@ namespace Friendica\Network; use Friendica\BaseObject; -use Friendica\Core\Authentication; +use Friendica\App\Authentication; use Friendica\Core\Logger; use Friendica\Core\Session; use Friendica\Database\DBA; diff --git a/tests/src/Model/User/CookieTest.php b/tests/src/Model/User/CookieTest.php new file mode 100644 index 0000000000..05fc26c2ac --- /dev/null +++ b/tests/src/Model/User/CookieTest.php @@ -0,0 +1,171 @@ +config = \Mockery::mock(Configuration::class); + } + + public function testInstance() + { + $this->config->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(1)->once(); + $this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn('1235')->once(); + $this->config->shouldReceive('get')->with('system', 'auth_cookie_lifetime', Cookie::DEFAULT_EXPIRE)->andReturn('7')->once(); + + $cookie = new Cookie($this->config, []); + $this->assertInstanceOf(Cookie::class, $cookie); + } + + public function dataGet() + { + return [ + 'default' => [ + 'cookieData' => [ + Cookie::NAME => json_encode([ + 'uid' => -1, + 'hash' => 12345, + 'ip' => '127.0.0.1', + ]) + ], + 'hasValues' => true, + 'uid' => -1, + 'hash' => 12345, + 'ip' => '127.0.0.1', + ], + 'missing' => [ + 'cookieData' => [ + + ], + 'hasValues' => false, + 'uid' => null, + 'hash' => null, + 'ip' => null, + ], + 'invalid' => [ + 'cookieData' => [ + Cookie::NAME => 'test', + ], + 'hasValues' => false, + 'uid' => null, + 'hash' => null, + 'ip' => null, + ], + 'incomplete' => [ + 'cookieData' => [ + Cookie::NAME => json_encode([ + 'uid' => -1, + 'hash' => 12345, + ]) + ], + 'hasValues' => true, + 'uid' => -1, + 'hash' => 12345, + 'ip' => null, + ], + ]; + } + + /** + * @dataProvider dataGet + */ + public function testGet(array $cookieData, bool $hasValues, $uid, $hash, $ip) + { + $this->config->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(1)->once(); + $this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn('1235')->once(); + $this->config->shouldReceive('get')->with('system', 'auth_cookie_lifetime', Cookie::DEFAULT_EXPIRE)->andReturn('7')->once(); + + $cookie = new Cookie($this->config, [], $cookieData); + $this->assertInstanceOf(Cookie::class, $cookie); + + $assertData = $cookie->getData(); + + if (!$hasValues) { + $this->assertEmpty($assertData); + } else { + $this->assertNotEmpty($assertData); + if (isset($uid)) { + $this->assertObjectHasAttribute('uid', $assertData); + $this->assertEquals($uid, $assertData->uid); + } else { + $this->assertObjectNotHasAttribute('uid', $assertData); + } + if (isset($hash)) { + $this->assertObjectHasAttribute('hash', $assertData); + $this->assertEquals($hash, $assertData->hash); + } else { + $this->assertObjectNotHasAttribute('hash', $assertData); + } + if (isset($ip)) { + $this->assertObjectHasAttribute('ip', $assertData); + $this->assertEquals($ip, $assertData->ip); + } else { + $this->assertObjectNotHasAttribute('ip', $assertData); + } + } + } + + public function dataCheck() + { + return [ + 'default' => [ + 'serverPrivateKey' => 'serverkey', + 'userPrivateKey' => 'userkey', + 'password' => 'test', + 'assertHash' => 'e9b4eb16275a2907b5659d22905b248221d0517dde4a9d5c320b8fe051b1267b', + 'assertTrue' => true, + ], + 'emptyUser' => [ + 'serverPrivateKey' => 'serverkey', + 'userPrivateKey' => '', + 'password' => '', + 'assertHash' => '', + 'assertTrue' => false, + ], + 'invalid' => [ + 'serverPrivateKey' => 'serverkey', + 'userPrivateKey' => 'bla', + 'password' => 'nope', + 'assertHash' => 'real wrong!', + 'assertTrue' => false, + ] + ]; + } + + /** + * @dataProvider dataCheck + */ + public function testCheck(string $serverPrivateKey, string $userPrivateKey, string $password, string $assertHash, bool $assertTrue) + { + $this->config->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(1)->once(); + $this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn($serverPrivateKey)->once(); + $this->config->shouldReceive('get')->with('system', 'auth_cookie_lifetime', Cookie::DEFAULT_EXPIRE)->andReturn('7')->once(); + + $cookie = new Cookie($this->config, []); + $this->assertInstanceOf(Cookie::class, $cookie); + + $this->assertEquals($assertTrue, $cookie->check($assertHash, $password, $userPrivateKey)); + } + + public function testSet() + { + $this->markTestIncomplete('Needs mocking of setcookie() first.'); + } + + public function testClear() + { + $this->markTestIncomplete('Needs mocking of setcookie() first.'); + } +} -- 2.39.5