From c1aff817bb2df563d0fa2b60e62ba7e517e3da16 Mon Sep 17 00:00:00 2001 From: Samuel Gfeller Date: Mon, 16 Oct 2023 18:04:57 +0200 Subject: [PATCH] Retrieve user ip and id in middleware [SLE-193] --- composer.json | 2 +- config/local/env.test.php | 2 - config/middleware.php | 5 ++- .../Data/UserNetworkSessionData.php | 15 ++++++++ .../UserNetworkSessionDataMiddleware.php | 37 +++++++++++++++++++ .../Authentication/Service/LoginVerifier.php | 7 ++-- .../Security/Service/LoginRequestFinder.php | 6 ++- .../Security/Service/SecurityLoginChecker.php | 3 -- .../User/Service/UserActivityManager.php | 8 ++-- .../AuthenticationLoggerRepository.php | 4 +- .../LoginLogFinderRepository.php | 12 +++--- tests/Traits/AppTestTrait.php | 3 -- 12 files changed, 78 insertions(+), 26 deletions(-) create mode 100644 src/Application/Data/UserNetworkSessionData.php create mode 100644 src/Application/Middleware/UserNetworkSessionDataMiddleware.php diff --git a/composer.json b/composer.json index a425b401..6b5eae64 100644 --- a/composer.json +++ b/composer.json @@ -9,7 +9,7 @@ "php-di/php-di": "^7.0", "ext-pdo": "*", "ext-json": "*", - "cakephp/database": "^4", + "cakephp/database": "4.4.17", "odan/session": "^6", "slim/php-view": "^3.0", "selective/basepath": "^2.0", diff --git a/config/local/env.test.php b/config/local/env.test.php index fe70a1aa..27218b52 100644 --- a/config/local/env.test.php +++ b/config/local/env.test.php @@ -8,8 +8,6 @@ // Optional setting but used in unit test $settings['security']['global_daily_email_threshold'] = 300; $settings['security']['global_monthly_email_threshold'] = 1000; -// Throttling of successful requests has to be enabled as it's tested -$settings['security']['throttle_login_success'] = true; // Using the null adapter to prevent emails from actually being sent $settings['smtp']['type'] = 'null'; diff --git a/config/middleware.php b/config/middleware.php index bea789ea..d1c5885f 100644 --- a/config/middleware.php +++ b/config/middleware.php @@ -16,7 +16,10 @@ // Language middleware has to be after PhpViewExtensionMiddleware as it needs the $route parameter $app->add(\App\Application\Middleware\LocaleMiddleware::class); - // Put everything possible before PhpViewExtensionMiddleware as if there is an error in a middleware, + // Retrieve and store ip address, user agent and user id (has to be BEFORE SessionStartMiddleware as it is using it) + $app->add(\App\Application\Middleware\UserNetworkSessionDataMiddleware::class); + + // ? Put everything possible before PhpViewExtensionMiddleware as if there is an error in a middleware, // the error page (and layout as well as everything else) needs this middleware loaded to work. $app->add(PhpViewExtensionMiddleware::class); diff --git a/src/Application/Data/UserNetworkSessionData.php b/src/Application/Data/UserNetworkSessionData.php new file mode 100644 index 00000000..7579c71d --- /dev/null +++ b/src/Application/Data/UserNetworkSessionData.php @@ -0,0 +1,15 @@ +getServerParams()['REMOTE_ADDR']; + $userAgent = $request->getServerParams()['HTTP_USER_AGENT']; + + // Add ip address to the ipAddressData DTO object + $this->clientNetworkData->ipAddress = $ipAddress; + $this->clientNetworkData->userAgent = $userAgent; + $this->clientNetworkData->userId = $this->session->get('user_id'); + + return $handler->handle($request); + } +} diff --git a/src/Domain/Authentication/Service/LoginVerifier.php b/src/Domain/Authentication/Service/LoginVerifier.php index 5392785c..f53e4b6b 100644 --- a/src/Domain/Authentication/Service/LoginVerifier.php +++ b/src/Domain/Authentication/Service/LoginVerifier.php @@ -2,6 +2,7 @@ namespace App\Domain\Authentication\Service; +use App\Application\Data\UserNetworkSessionData; use App\Domain\Authentication\Exception\InvalidCredentialsException; use App\Domain\Security\Service\SecurityLoginChecker; use App\Domain\User\Enum\UserActivity; @@ -22,8 +23,8 @@ public function __construct( private readonly AuthenticationLoggerRepository $authenticationLoggerRepository, private readonly LoginNonActiveUserHandler $loginNonActiveUserHandler, private readonly UserActivityManager $userActivityManager, + private readonly UserNetworkSessionData $ipAddressData, ) { - } /** @@ -58,7 +59,7 @@ public function getUserIdIfAllowedToLogin( // Insert login success request $this->authenticationLoggerRepository->logLoginRequest( $dbUser->email, - $_SERVER['REMOTE_ADDR'], + $this->ipAddressData->ipAddress, true, $dbUser->id ); @@ -81,7 +82,7 @@ public function getUserIdIfAllowedToLogin( // Password not correct or user not existing - insert login request for ip $this->authenticationLoggerRepository->logLoginRequest( $userLoginValues['email'], - $_SERVER['REMOTE_ADDR'], + $this->ipAddressData->ipAddress, false, $dbUser->id ); diff --git a/src/Domain/Security/Service/LoginRequestFinder.php b/src/Domain/Security/Service/LoginRequestFinder.php index 3de1e273..d628bde9 100644 --- a/src/Domain/Security/Service/LoginRequestFinder.php +++ b/src/Domain/Security/Service/LoginRequestFinder.php @@ -2,6 +2,7 @@ namespace App\Domain\Security\Service; +use App\Application\Data\UserNetworkSessionData; use App\Domain\Settings; use App\Infrastructure\SecurityLogging\LoginLogFinderRepository; @@ -11,6 +12,7 @@ class LoginRequestFinder public function __construct( private readonly LoginLogFinderRepository $loginRequestFinderRepository, + private readonly UserNetworkSessionData $ipAddressData, Settings $settings ) { $this->securitySettings = $settings->get('security'); @@ -31,7 +33,7 @@ public function findLoginLogEntriesInTimeLimit(string $email): array // Stats concerning given email in last timespan return $this->loginRequestFinderRepository->getLoginSummaryFromEmailAndIp( $email, - $_SERVER['REMOTE_ADDR'], + $this->ipAddressData->ipAddress, $this->securitySettings['timespan'] ); } @@ -47,7 +49,7 @@ public function findLatestLoginRequestTimestamp(string $email): int { $createdAt = $this->loginRequestFinderRepository->findLatestLoginTimestampFromUserOrIp( $email, - $_SERVER['REMOTE_ADDR'] + $this->ipAddressData->ipAddress, ); if ($createdAt) { return (new \DateTime($createdAt))->format('U'); diff --git a/src/Domain/Security/Service/SecurityLoginChecker.php b/src/Domain/Security/Service/SecurityLoginChecker.php index d2fe402c..c7506202 100644 --- a/src/Domain/Security/Service/SecurityLoginChecker.php +++ b/src/Domain/Security/Service/SecurityLoginChecker.php @@ -83,7 +83,6 @@ public function performLoginSecurityCheck(string $email, ?string $reCaptchaRespo */ private function performLoginCheck(array $loginsByIp, array $loginsByEmail, string $email): void { - $throttleSuccess = $this->securitySettings['throttle_login_success']; // Reverse order to compare fails the longest delay first and then go down from there krsort($this->securitySettings['login_throttle_rule']); // Fails on specific user or coming from specific IP @@ -91,9 +90,7 @@ private function performLoginCheck(array $loginsByIp, array $loginsByEmail, stri // Check that there aren't more login successes or failures than tolerated if ( ($loginsByIp['failures'] >= $requestLimit && $loginsByIp['failures'] !== 0) - || ($throttleSuccess && $loginsByIp['successes'] >= $requestLimit && $loginsByIp['successes'] !== 0) || ($loginsByEmail['failures'] >= $requestLimit && $loginsByEmail['failures'] !== 0) - || ($throttleSuccess && $loginsByEmail['successes'] >= $requestLimit && $loginsByEmail['successes'] !== 0) ) { // If truthy means: too many ip fails OR too many ip successes // OR too many failed login tries on specific user OR too many succeeding login requests on specific user diff --git a/src/Domain/User/Service/UserActivityManager.php b/src/Domain/User/Service/UserActivityManager.php index 1b83aff3..a6ee7912 100644 --- a/src/Domain/User/Service/UserActivityManager.php +++ b/src/Domain/User/Service/UserActivityManager.php @@ -2,6 +2,7 @@ namespace App\Domain\User\Service; +use App\Application\Data\UserNetworkSessionData; use App\Domain\User\Data\UserActivityData; use App\Domain\User\Enum\UserActivity; use App\Infrastructure\User\UserActivityRepository; @@ -12,6 +13,7 @@ class UserActivityManager public function __construct( private readonly UserActivityRepository $userActivityRepository, private readonly SessionInterface $session, + private readonly UserNetworkSessionData $userNetworkSessionData, ) { } @@ -34,9 +36,9 @@ public function addUserActivity( ?int $userId = null, ): int { $userActivity = new UserActivityData(); - $userActivity->ipAddress = $_SERVER['REMOTE_ADDR']; - $userActivity->userAgent = $_SERVER['HTTP_USER_AGENT'] ?? null; - $userActivity->userId = $this->session->get('user_id') ?? $userId; + $userActivity->ipAddress = $this->userNetworkSessionData->ipAddress; + $userActivity->userAgent = $this->userNetworkSessionData->userAgent; + $userActivity->userId = $this->userNetworkSessionData->userId ?? $userId; $userActivity->action = $userActivityAction; $userActivity->table = $table; $userActivity->rowId = $rowId; diff --git a/src/Infrastructure/SecurityLogging/AuthenticationLoggerRepository.php b/src/Infrastructure/SecurityLogging/AuthenticationLoggerRepository.php index 3712688e..abb3da34 100644 --- a/src/Infrastructure/SecurityLogging/AuthenticationLoggerRepository.php +++ b/src/Infrastructure/SecurityLogging/AuthenticationLoggerRepository.php @@ -15,12 +15,12 @@ public function __construct( * Insert new login request. * * @param string $email - * @param string $ip + * @param ?string $ip * @param bool $success whether login request was a successful login or not * @param int|null $userId * @return string */ - public function logLoginRequest(string $email, string $ip, bool $success, ?int $userId = null): string + public function logLoginRequest(string $email, ?string $ip, bool $success, ?int $userId = null): string { $query = $this->queryFactory->newQuery(); $query->insert(['email', 'ip_address', 'is_success', 'user_id', 'created_at']) diff --git a/src/Infrastructure/SecurityLogging/LoginLogFinderRepository.php b/src/Infrastructure/SecurityLogging/LoginLogFinderRepository.php index df175fe1..c08ea3d2 100644 --- a/src/Infrastructure/SecurityLogging/LoginLogFinderRepository.php +++ b/src/Infrastructure/SecurityLogging/LoginLogFinderRepository.php @@ -23,16 +23,16 @@ public function __construct( * logins_by_ip: array{successes: int, failures: int}, * } */ - public function getLoginSummaryFromEmailAndIp(string $email, string $ip, int $seconds): array + public function getLoginSummaryFromEmailAndIp(string $email, ?string $ip, int $seconds): array { - $summary = ['logins_by_ip' => [], 'logins_by_email' => []]; + $summary = ['logins_by_ip' => ['successes' => 0, 'failures' => 0], 'logins_by_email' => []]; // Only return values if not empty string as it doesn't represent a user request if ($email !== '') { - $summary['logins_by_email'] = $this->getLoginRequestStats(['email' => $email], $seconds); + $summary['logins_by_email'] = $this->getLoginRequestSummary(['email' => $email], $seconds); } - if ($ip !== '') { - $summary['logins_by_ip'] = $this->getLoginRequestStats(['ip_address' => $ip], $seconds); + if ($ip && $ip !== '') { + $summary['logins_by_ip'] = $this->getLoginRequestSummary(['ip_address' => $ip], $seconds); } return $summary; @@ -46,7 +46,7 @@ public function getLoginSummaryFromEmailAndIp(string $email, string $ip, int $se * * @return array{successes: int, failures: int} */ - private function getLoginRequestStats(array $whereEmailOrIpArr, int $seconds): array + private function getLoginRequestSummary(array $whereEmailOrIpArr, int $seconds): array { $query = $this->queryFactory->newQuery(); $query->select( diff --git a/tests/Traits/AppTestTrait.php b/tests/Traits/AppTestTrait.php index 679fad1b..91805cdb 100644 --- a/tests/Traits/AppTestTrait.php +++ b/tests/Traits/AppTestTrait.php @@ -64,9 +64,6 @@ protected function setUp(): void $this->insertFixtures([UserRoleFixture::class]); } - // Per default not set when script executed with cli and used at least in all security checks - $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; - // XDebug start_with_request produces errors when testing (SLE-102) // if (!isset($_ENV['AUTO_XDEBUG_DISABLED'])) { // Disable xdebug.start_with_request (when already disabled, delay is approx 200ms for 80 tests)