From a902e7d52967a544d673b8a912a7d3000bd4eaf5 Mon Sep 17 00:00:00 2001 From: Thorsten Rinne Date: Fri, 1 Nov 2024 12:57:01 +0100 Subject: [PATCH] refactor: split up large methods into smaller ones (#3202) --- phpmyfaq/src/phpMyFAQ/Network.php | 7 +- phpmyfaq/src/phpMyFAQ/User/Tracking.php | 163 +++++++++++++++--------- tests/phpMyFAQ/User/TrackingTest.php | 89 +++++++++++++ 3 files changed, 192 insertions(+), 67 deletions(-) create mode 100644 tests/phpMyFAQ/User/TrackingTest.php diff --git a/phpmyfaq/src/phpMyFAQ/Network.php b/phpmyfaq/src/phpMyFAQ/Network.php index 86f2cf0f7a..caeef39fe5 100644 --- a/phpmyfaq/src/phpMyFAQ/Network.php +++ b/phpmyfaq/src/phpMyFAQ/Network.php @@ -39,13 +39,12 @@ public function __construct(private Configuration $configuration) /** * Performs a check if an IPv4 or IPv6 address is banned. * - * @param string $ip IPv4 or IPv6 address - * + * @param string $ipAddress IPv4 or IPv6 address * @return bool false, if not banned */ - public function isBanned(string $ip): bool + public function isBanned(string $ipAddress): bool { $bannedIps = explode(' ', (string) $this->configuration->get('security.bannedIPs')); - return IpUtils::checkIp($ip, $bannedIps); + return IpUtils::checkIp($ipAddress, $bannedIps); } } diff --git a/phpmyfaq/src/phpMyFAQ/User/Tracking.php b/phpmyfaq/src/phpMyFAQ/User/Tracking.php index ea2975af9d..9bde4ae9ef 100644 --- a/phpmyfaq/src/phpMyFAQ/User/Tracking.php +++ b/phpmyfaq/src/phpMyFAQ/User/Tracking.php @@ -9,6 +9,7 @@ use phpMyFAQ\Filter; use phpMyFAQ\Network; use phpMyFAQ\Strings; +use Symfony\Component\HttpFoundation\HeaderBag; use Symfony\Component\HttpFoundation\IpUtils; use Symfony\Component\HttpFoundation\Request; @@ -40,22 +41,14 @@ public static function getInstance( /** * @throws Exception */ - public function log(string $action, int|string|null $data = null): void + public function log(string $action, int|string|null $data = null): bool { if (!$this->configuration->get('main.enableUserTracking')) { - return; + return false; } - $bots = 0; - $banned = false; - $this->currentSessionId = Filter::filterVar( - $this->request->query->get(UserSession::KEY_NAME_SESSION_ID), - FILTER_VALIDATE_INT - ); - $cookieId = Filter::filterVar( - $this->request->query->get(UserSession::COOKIE_NAME_SESSION_ID), - FILTER_VALIDATE_INT - ); + $this->initializeSessionId(); + $cookieId = $this->getCookieId(); if (!is_null($cookieId)) { $this->userSession->setCurrentSessionId($cookieId); @@ -65,78 +58,117 @@ public function log(string $action, int|string|null $data = null): void $this->userSession->setCurrentSessionId(0); } + $bots = $this->countBots(); + $remoteAddress = $this->getRemoteAddress(); + $banned = $this->isBanned($remoteAddress); + + if (0 === $bots && false === $banned) { + $this->handleSession($cookieId, $remoteAddress, $action, $data); + } + + return true; + } + + private function initializeSessionId(): void + { + $sessionId = $this->request->query->get(UserSession::KEY_NAME_SESSION_ID); + if ($sessionId) { + $this->currentSessionId = $sessionId; + } + } + + private function getCookieId(): ?int + { + return Filter::filterVar( + $this->request->query->get(UserSession::COOKIE_NAME_SESSION_ID), + FILTER_VALIDATE_INT + ); + } + + private function countBots(): int + { + $bots = 0; foreach ($this->getBotIgnoreList() as $bot) { - if (Strings::strstr($this->request->headers->get('user-agent'), $bot)) { + if (Strings::strstr($this->getRequestHeaders()->get('user-agent') ?? 1, $bot)) { ++$bots; } } + return $bots; + } - // if we're running behind a reverse proxy like nginx/varnish, fix the client IP + public function getRemoteAddress(): string + { $remoteAddress = $this->request->getClientIp(); $localAddresses = ['127.0.0.1', '::1']; - if (in_array($remoteAddress, $localAddresses) && $this->request->headers->has('X-Forwarded-For')) { - $remoteAddress = $this->request->headers->get('X-Forwarded-For'); + if (in_array($remoteAddress, $localAddresses) && $this->getRequestHeaders()->has('X-Forwarded-For')) { + $remoteAddress = $this->getRequestHeaders()->get('X-Forwarded-For'); } - // clean up as well - $remoteAddress = preg_replace('([^0-9a-z:.]+)i', '', (string) $remoteAddress); - - // Anonymize IP address - $remoteAddress = IpUtils::anonymize($remoteAddress); + return preg_replace('([^0-9a-z:.]+)i', '', (string)$remoteAddress); + } + private function isBanned(string $remoteAddress): bool + { $network = new Network($this->configuration); - if ($network->isBanned($remoteAddress)) { - $banned = true; - } + return $network->isBanned(IpUtils::anonymize($remoteAddress)); + } - if (0 === $bots && false === $banned) { - if ($this->currentSessionId === null) { - $this->currentSessionId = $this->configuration->getDb()->nextId( - Database::getTablePrefix() . 'faqsessions', - 'sid' - ); - // Check: force the session cookie to contains the current $sid - if (!is_null($cookieId) && (!$cookieId != $this->userSession->getCurrentSessionId())) { - $this->userSession->setCookie( - UserSession::COOKIE_NAME_SESSION_ID, - $this->userSession->getCurrentSessionId() - ); - } - - $query = sprintf( - "INSERT INTO %sfaqsessions (sid, user_id, ip, time) VALUES (%d, %d, '%s', %d)", - Database::getTablePrefix(), - $this->userSession->getCurrentSessionId(), - CurrentUser::getCurrentUser($this->configuration)->getUserId(), - $remoteAddress, - $this->request->server->get('REQUEST_TIME') + /** + * @throws Exception + */ + private function handleSession(?int $cookieId, string $remoteAddress, string $action, int|string|null $data): void + { + if ($this->currentSessionId === null) { + $this->currentSessionId = $this->configuration->getDb()->nextId( + Database::getTablePrefix() . 'faqsessions', + 'sid' + ); + + if (!is_null($cookieId) && (!$cookieId != $this->userSession->getCurrentSessionId())) { + $this->userSession->setCookie( + UserSession::COOKIE_NAME_SESSION_ID, + $this->userSession->getCurrentSessionId() ); - - $this->configuration->getDb()->query($query); } - $data = $this->userSession->getCurrentSessionId() . ';' . - str_replace(';', ',', $action) . ';' . - $data . ';' . - $remoteAddress . ';' . - str_replace(';', ',', $this->request->server->get('QUERY_STRING') ?? '') . ';' . - str_replace(';', ',', $this->request->server->get('HTTP_REFERER') ?? '') . ';' . - str_replace(';', ',', urldecode((string) $this->request->server->get('HTTP_USER_AGENT'))) . ';' . - $this->request->server->get('REQUEST_TIME') . ";\n"; + $query = sprintf( + "INSERT INTO %sfaqsessions (sid, user_id, ip, time) VALUES (%d, %d, '%s', %d)", + Database::getTablePrefix(), + $this->userSession->getCurrentSessionId(), + CurrentUser::getCurrentUser($this->configuration)->getUserId(), + $remoteAddress, + $this->request->server->get('REQUEST_TIME') + ); - $file = PMF_ROOT_DIR . '/content/core/data/tracking' . date('dmY'); + $this->configuration->getDb()->query($query); + } - if (!is_file($file)) { - touch($file); - } + $this->writeTrackingData($action, $data, $remoteAddress); + } - if (!is_writable($file)) { - $this->configuration->getLogger()->error('Cannot write to ' . $file); - } + private function writeTrackingData(string $action, int|string|null $data, string $remoteAddress): void + { + $data = $this->userSession->getCurrentSessionId() . ';' . + str_replace(';', ',', $action) . ';' . + $data . ';' . + $remoteAddress . ';' . + str_replace(';', ',', $this->request->server->get('QUERY_STRING') ?? '') . ';' . + str_replace(';', ',', $this->request->server->get('HTTP_REFERER') ?? '') . ';' . + str_replace(';', ',', urldecode((string) $this->request->server->get('HTTP_USER_AGENT'))) . ';' . + $this->request->server->get('REQUEST_TIME') . ";\n"; + + $file = PMF_ROOT_DIR . '/content/core/data/tracking' . date('dmY'); + + if (!is_file($file)) { + touch($file); + } - file_put_contents($file, $data, FILE_APPEND | LOCK_EX); + if (!is_writable($file)) { + $this->configuration->getLogger()->error('Cannot write to ' . $file); } + + file_put_contents($file, $data, FILE_APPEND | LOCK_EX); } /** @@ -147,4 +179,9 @@ private function getBotIgnoreList(): array { return explode(',', (string) $this->configuration->get('main.botIgnoreList')); } + + private function getRequestHeaders(): HeaderBag + { + return $this->request->headers; + } } diff --git a/tests/phpMyFAQ/User/TrackingTest.php b/tests/phpMyFAQ/User/TrackingTest.php new file mode 100644 index 0000000000..0ade829d14 --- /dev/null +++ b/tests/phpMyFAQ/User/TrackingTest.php @@ -0,0 +1,89 @@ +configurationMock = $this->createMock(Configuration::class); + $this->requestMock = $this->createMock(Request::class); + $this->requestMock->headers = new HeaderBag([ + 'X-Forwarded-For' => '192.168.1.1' + ]); + $this->requestMock->method('getClientIp')->willReturn('127.0.0.1'); + $this->userSessionMock = $this->createMock(UserSession::class); + $this->databaseMock = $this->createMock(DatabaseDriver::class); + + $this->configurationMock->method('getDb')->willReturn($this->databaseMock); + $this->tracking = Tracking::getInstance($this->configurationMock, $this->requestMock, $this->userSessionMock); + } + + public function testInitializeSessionId(): void + { + $this->requestMock->query = new InputBag([ + UserSession::KEY_NAME_SESSION_ID => 123, + UserSession::COOKIE_NAME_SESSION_ID => 456 + ]); + + $reflection = new ReflectionClass($this->tracking); + $method = $reflection->getMethod('initializeSessionId'); + $method->setAccessible(true); + $method->invoke($this->tracking); + + $property = $reflection->getProperty('currentSessionId'); + $property->setAccessible(true); + $this->assertEquals(123, $property->getValue($this->tracking)); + } + + public function testGetCookieId(): void + { + $this->requestMock->query = new InputBag([ + UserSession::KEY_NAME_SESSION_ID => 123, + UserSession::COOKIE_NAME_SESSION_ID => 456 + ]); + + $reflection = new ReflectionClass($this->tracking); + $method = $reflection->getMethod('getCookieId'); + $method->setAccessible(true); + $result = $method->invoke($this->tracking); + + $this->assertEquals(456, $result); + } + + /** + * @throws \ReflectionException + */ + public function testCountBots(): void + { + $this->configurationMock->method('get')->with('main.botIgnoreList')->willReturn('bot1,bot2'); + $this->requestMock->headers = new HeaderBag([ + 'user-agent' => 'bot1' + ]); + + $reflection = new ReflectionClass($this->tracking); + $method = $reflection->getMethod('countBots'); + $method->setAccessible(true); + $result = $method->invoke($this->tracking); + + $this->assertEquals(1, $result); + } +}