From efb7d21b52eb033530e80e5e49d175e6e3b031f4 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Fri, 2 Oct 2020 17:50:59 +0200 Subject: Add strict types for bookmarks management Parameters typing and using strict types overall increase the codebase quality by enforcing the a given parameter will have the expected type. It also removes the need to unnecessary unit tests checking methods behavior with invalid input. --- application/security/LoginManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'application/security') diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php index d74c3118..65048f10 100644 --- a/application/security/LoginManager.php +++ b/application/security/LoginManager.php @@ -118,7 +118,7 @@ class LoginManager * * @return true when the user is logged in, false otherwise */ - public function isLoggedIn() + public function isLoggedIn(): bool { if ($this->openShaarli) { return true; -- cgit v1.2.3 From b38a1b0209f546d4824a0db81a34c4e30fcdebaf Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Tue, 20 Oct 2020 11:47:07 +0200 Subject: Use PSR-3 logger for login attempts Fixes #1122 --- application/security/BanManager.php | 28 +++++++------- application/security/LoginManager.php | 69 +++++++++++++++-------------------- 2 files changed, 43 insertions(+), 54 deletions(-) (limited to 'application/security') diff --git a/application/security/BanManager.php b/application/security/BanManager.php index 68190c54..f72c8b7b 100644 --- a/application/security/BanManager.php +++ b/application/security/BanManager.php @@ -3,6 +3,7 @@ namespace Shaarli\Security; +use Psr\Log\LoggerInterface; use Shaarli\FileUtils; /** @@ -28,8 +29,8 @@ class BanManager /** @var string Path to the file containing IP bans and failures */ protected $banFile; - /** @var string Path to the log file, used to log bans */ - protected $logFile; + /** @var LoggerInterface Path to the log file, used to log bans */ + protected $logger; /** @var array List of IP with their associated number of failed attempts */ protected $failures = []; @@ -40,18 +41,19 @@ class BanManager /** * BanManager constructor. * - * @param array $trustedProxies List of allowed proxies IP - * @param int $nbAttempts Number of allowed failed attempt before the ban - * @param int $banDuration Ban duration in seconds - * @param string $banFile Path to the file containing IP bans and failures - * @param string $logFile Path to the log file, used to log bans + * @param array $trustedProxies List of allowed proxies IP + * @param int $nbAttempts Number of allowed failed attempt before the ban + * @param int $banDuration Ban duration in seconds + * @param string $banFile Path to the file containing IP bans and failures + * @param LoggerInterface $logger PSR-3 logger to save login attempts in log directory */ - public function __construct($trustedProxies, $nbAttempts, $banDuration, $banFile, $logFile) { + public function __construct($trustedProxies, $nbAttempts, $banDuration, $banFile, LoggerInterface $logger) { $this->trustedProxies = $trustedProxies; $this->nbAttempts = $nbAttempts; $this->banDuration = $banDuration; $this->banFile = $banFile; - $this->logFile = $logFile; + $this->logger = $logger; + $this->readBanFile(); } @@ -78,11 +80,7 @@ class BanManager if ($this->failures[$ip] >= $this->nbAttempts) { $this->bans[$ip] = time() + $this->banDuration; - logm( - $this->logFile, - $server['REMOTE_ADDR'], - 'IP address banned from login: '. $ip - ); + $this->logger->info(format_log('IP address banned from login: '. $ip, $ip)); } $this->writeBanFile(); } @@ -138,7 +136,7 @@ class BanManager unset($this->failures[$ip]); } unset($this->bans[$ip]); - logm($this->logFile, $server['REMOTE_ADDR'], 'Ban lifted for: '. $ip); + $this->logger->info(format_log('Ban lifted for: '. $ip, $ip)); $this->writeBanFile(); return false; diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php index 65048f10..426e785e 100644 --- a/application/security/LoginManager.php +++ b/application/security/LoginManager.php @@ -2,6 +2,7 @@ namespace Shaarli\Security; use Exception; +use Psr\Log\LoggerInterface; use Shaarli\Config\ConfigManager; /** @@ -31,26 +32,30 @@ class LoginManager protected $staySignedInToken = ''; /** @var CookieManager */ protected $cookieManager; + /** @var LoggerInterface */ + protected $logger; /** * Constructor * - * @param ConfigManager $configManager Configuration Manager instance + * @param ConfigManager $configManager Configuration Manager instance * @param SessionManager $sessionManager SessionManager instance - * @param CookieManager $cookieManager CookieManager instance + * @param CookieManager $cookieManager CookieManager instance + * @param BanManager $banManager + * @param LoggerInterface $logger Used to log login attempts */ - public function __construct($configManager, $sessionManager, $cookieManager) - { + public function __construct( + ConfigManager $configManager, + SessionManager $sessionManager, + CookieManager $cookieManager, + BanManager $banManager, + LoggerInterface $logger + ) { $this->configManager = $configManager; $this->sessionManager = $sessionManager; $this->cookieManager = $cookieManager; - $this->banManager = new BanManager( - $this->configManager->get('security.trusted_proxies', []), - $this->configManager->get('security.ban_after'), - $this->configManager->get('security.ban_duration'), - $this->configManager->get('resource.ban_file', 'data/ipbans.php'), - $this->configManager->get('resource.log') - ); + $this->banManager = $banManager; + $this->logger = $logger; if ($this->configManager->get('security.open_shaarli') === true) { $this->openShaarli = true; @@ -129,48 +134,34 @@ class LoginManager /** * Check user credentials are valid * - * @param string $remoteIp Remote client IP address * @param string $clientIpId Client IP address identifier * @param string $login Username * @param string $password Password * * @return bool true if the provided credentials are valid, false otherwise */ - public function checkCredentials($remoteIp, $clientIpId, $login, $password) + public function checkCredentials($clientIpId, $login, $password) { - // Check login matches config - if ($login !== $this->configManager->get('credentials.login')) { - return false; - } - // Check credentials try { $useLdapLogin = !empty($this->configManager->get('ldap.host')); - if ((false === $useLdapLogin && $this->checkCredentialsFromLocalConfig($login, $password)) - || (true === $useLdapLogin && $this->checkCredentialsFromLdap($login, $password)) + if ($login === $this->configManager->get('credentials.login') + && ( + (false === $useLdapLogin && $this->checkCredentialsFromLocalConfig($login, $password)) + || (true === $useLdapLogin && $this->checkCredentialsFromLdap($login, $password)) + ) ) { - $this->sessionManager->storeLoginInfo($clientIpId); - logm( - $this->configManager->get('resource.log'), - $remoteIp, - 'Login successful' - ); - return true; + $this->sessionManager->storeLoginInfo($clientIpId); + $this->logger->info(format_log('Login successful', $clientIpId)); + + return true; } - } - catch(Exception $exception) { - logm( - $this->configManager->get('resource.log'), - $remoteIp, - 'Exception while checking credentials: ' . $exception - ); + } catch(Exception $exception) { + $this->logger->info(format_log('Exception while checking credentials: ' . $exception, $clientIpId)); } - logm( - $this->configManager->get('resource.log'), - $remoteIp, - 'Login failed for user ' . $login - ); + $this->logger->info(format_log('Login failed for user ' . $login, $clientIpId)); + return false; } -- cgit v1.2.3 From c2cd15dac2bfaebe6d32f7649fbdedc07400fa08 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Fri, 16 Oct 2020 13:34:59 +0200 Subject: Move utils classes to Shaarli\Helper namespace and folder --- application/security/BanManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'application/security') diff --git a/application/security/BanManager.php b/application/security/BanManager.php index f72c8b7b..288cbde0 100644 --- a/application/security/BanManager.php +++ b/application/security/BanManager.php @@ -4,7 +4,7 @@ namespace Shaarli\Security; use Psr\Log\LoggerInterface; -use Shaarli\FileUtils; +use Shaarli\Helper\FileUtils; /** * Class BanManager -- cgit v1.2.3 From d3f6d525253eb7bb041d9436cbf213c10524a85c Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Wed, 28 Oct 2020 14:02:08 +0100 Subject: Fix compatiliby issue on login with PHP 7.1 session_set_cookie_params does not return any value in PHP 7.1 --- application/security/SessionManager.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'application/security') diff --git a/application/security/SessionManager.php b/application/security/SessionManager.php index 36df8c1c..96bf193c 100644 --- a/application/security/SessionManager.php +++ b/application/security/SessionManager.php @@ -293,9 +293,12 @@ class SessionManager return session_start(); } - public function cookieParameters(int $lifeTime, string $path, string $domain): bool + /** + * Be careful, return type of session_set_cookie_params() changed between PHP 7.1 and 7.2. + */ + public function cookieParameters(int $lifeTime, string $path, string $domain): void { - return session_set_cookie_params($lifeTime, $path, $domain); + session_set_cookie_params($lifeTime, $path, $domain); } public function regenerateId(bool $deleteOldSession = false): bool -- cgit v1.2.3 From 53054b2bf6a919fd4ff9b44b6ad1986f21f488b6 Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Tue, 22 Sep 2020 20:25:47 +0200 Subject: Apply PHP Code Beautifier on source code for linter automatic fixes --- application/security/BanManager.php | 8 ++++---- application/security/LoginManager.php | 16 ++++++++++------ application/security/SessionManager.php | 3 ++- 3 files changed, 16 insertions(+), 11 deletions(-) (limited to 'application/security') diff --git a/application/security/BanManager.php b/application/security/BanManager.php index 288cbde0..7077af5b 100644 --- a/application/security/BanManager.php +++ b/application/security/BanManager.php @@ -1,6 +1,5 @@ trustedProxies = $trustedProxies; $this->nbAttempts = $nbAttempts; $this->banDuration = $banDuration; @@ -80,7 +80,7 @@ class BanManager if ($this->failures[$ip] >= $this->nbAttempts) { $this->bans[$ip] = time() + $this->banDuration; - $this->logger->info(format_log('IP address banned from login: '. $ip, $ip)); + $this->logger->info(format_log('IP address banned from login: ' . $ip, $ip)); } $this->writeBanFile(); } @@ -136,7 +136,7 @@ class BanManager unset($this->failures[$ip]); } unset($this->bans[$ip]); - $this->logger->info(format_log('Ban lifted for: '. $ip, $ip)); + $this->logger->info(format_log('Ban lifted for: ' . $ip, $ip)); $this->writeBanFile(); return false; diff --git a/application/security/LoginManager.php b/application/security/LoginManager.php index 426e785e..b795b80e 100644 --- a/application/security/LoginManager.php +++ b/application/security/LoginManager.php @@ -1,4 +1,5 @@ sessionManager->storeLoginInfo($clientIpId); - } elseif ($this->sessionManager->hasSessionExpired() + } elseif ( + $this->sessionManager->hasSessionExpired() || $this->sessionManager->hasClientIpChanged($clientIpId) ) { $this->sessionManager->logout(); @@ -145,7 +147,8 @@ class LoginManager // Check credentials try { $useLdapLogin = !empty($this->configManager->get('ldap.host')); - if ($login === $this->configManager->get('credentials.login') + if ( + $login === $this->configManager->get('credentials.login') && ( (false === $useLdapLogin && $this->checkCredentialsFromLocalConfig($login, $password)) || (true === $useLdapLogin && $this->checkCredentialsFromLdap($login, $password)) @@ -156,7 +159,7 @@ class LoginManager return true; } - } catch(Exception $exception) { + } catch (Exception $exception) { $this->logger->info(format_log('Exception while checking credentials: ' . $exception, $clientIpId)); } @@ -174,7 +177,8 @@ class LoginManager * * @return bool true if the provided credentials are valid, false otherwise */ - public function checkCredentialsFromLocalConfig($login, $password) { + public function checkCredentialsFromLocalConfig($login, $password) + { $hash = sha1($password . $login . $this->configManager->get('credentials.salt')); return $login == $this->configManager->get('credentials.login') @@ -193,14 +197,14 @@ class LoginManager */ public function checkCredentialsFromLdap($login, $password, $connect = null, $bind = null) { - $connect = $connect ?? function($host) { + $connect = $connect ?? function ($host) { $resource = ldap_connect($host); ldap_set_option($resource, LDAP_OPT_PROTOCOL_VERSION, 3); return $resource; }; - $bind = $bind ?? function($handle, $dn, $password) { + $bind = $bind ?? function ($handle, $dn, $password) { return ldap_bind($handle, $dn, $password); }; diff --git a/application/security/SessionManager.php b/application/security/SessionManager.php index 96bf193c..f957b91a 100644 --- a/application/security/SessionManager.php +++ b/application/security/SessionManager.php @@ -1,4 +1,5 @@ conf->get('credentials.salt')); + $token = sha1(uniqid('', true) . '_' . mt_rand() . $this->conf->get('credentials.salt')); $this->session['tokens'][$token] = 1; return $token; } -- cgit v1.2.3