3 files changed, 61 insertions, 64 deletions

diff --git a/application/security/BanManager.php b/application/security/BanManager.php
index 68190c54..7077af5b 100644
--- a/application/security/BanManager.php
+++ b/application/security/BanManager.php
@@ -1,9 +1,9 @@
 <?php
 
-
 namespace Shaarli\Security;
 
-use Shaarli\FileUtils;
+use Psr\Log\LoggerInterface;
+use Shaarli\Helper\FileUtils;
 
 /**
  * Class BanManager
@@ -28,8 +28,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 +40,20 @@ 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 d74c3118..b795b80e 100644
--- a/application/security/LoginManager.php
+++ b/application/security/LoginManager.php
@@ -1,7 +1,9 @@
 <?php
+
 namespace Shaarli\Security;
 
 use Exception;
+use Psr\Log\LoggerInterface;
 use Shaarli\Config\ConfigManager;
 
 /**
@@ -31,26 +33,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;
@@ -101,7 +107,8 @@ class LoginManager
             // The user client has a valid stay-signed-in cookie
             // Session information is updated with the current client information
             $this->sessionManager->storeLoginInfo($clientIpId);
-        } elseif ($this->sessionManager->hasSessionExpired()
+        } elseif (
+            $this->sessionManager->hasSessionExpired()
             || $this->sessionManager->hasClientIpChanged($clientIpId)
         ) {
             $this->sessionManager->logout();
@@ -118,7 +125,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;
@@ -129,48 +136,35 @@ 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;
     }
 
@@ -183,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')
@@ -202,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 36df8c1c..f957b91a 100644
--- a/application/security/SessionManager.php
+++ b/application/security/SessionManager.php
@@ -1,4 +1,5 @@
 <?php
+
 namespace Shaarli\Security;
 
 use Shaarli\Config\ConfigManager;
@@ -79,7 +80,7 @@ class SessionManager
      */
     public function generateToken()
     {
-        $token = sha1(uniqid('', true) .'_'. mt_rand() . $this->conf->get('credentials.salt'));
+        $token = sha1(uniqid('', true) . '_' . mt_rand() . $this->conf->get('credentials.salt'));
         $this->session['tokens'][$token] = 1;
         return $token;
     }
@@ -293,9 +294,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