Knah-Tsaeb/MyShaarli
Knah-Tsaeb/MyShaarli
1
0
Fork 0
Code Issues Pull requests Releases Wiki Activity

Use PSR-3 logger for login attempts

Browse source 
Fixes #1122
This commit is contained in:
ArthurHoaro 2020-10-20 11:47:07 +02:00
parent ca5e98da48
commit b38a1b0209
15 changed files with 170 additions and 124 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
application/Utils.php
View file

@ -4,21 +4,23 @@
*/
/**
* Logs a message to a text file
* Format log using provided data.
*
* The log format is compatible with fail2ban.
* @param string $message the message to log
* @param string|null $clientIp the client's remote IPv4/IPv6 address
*
* @param string $logFile where to write the logs
* @param string $clientIp the client's remote IPv4/IPv6 address
* @param string $message the message to log
* @return string Formatted message to log
*/
function logm($logFile, $clientIp, $message)
function format_log(string $message, string $clientIp = null): string
{
file_put_contents(
$logFile,
date('Y/m/d H:i:s').' - '.$clientIp.' - '.strval($message).PHP_EOL,
FILE_APPEND
);
$out = $message;
if (!empty($clientIp)) {
// Note: we keep the first dash to avoid breaking fail2ban configs
$out = '- ' . $clientIp . ' - ' . $out;
}
return $out;
}
/**

10
application/container/ContainerBuilder.php
View file

@ -5,6 +5,7 @@ declare(strict_types=1);
namespace Shaarli\Container;
use malkusch\lock\mutex\FlockMutex;
use Psr\Log\LoggerInterface;
use Shaarli\Bookmark\BookmarkFileService;
use Shaarli\Bookmark\BookmarkServiceInterface;
use Shaarli\Config\ConfigManager;
@ -49,6 +50,9 @@ class ContainerBuilder
/** @var LoginManager */
protected $login;
/** @var LoggerInterface */
protected $logger;
/** @var string|null */
protected $basePath = null;
@ -56,12 +60,14 @@ class ContainerBuilder
ConfigManager $conf,
SessionManager $session,
CookieManager $cookieManager,
LoginManager $login
LoginManager $login,
LoggerInterface $logger
) {
$this->conf = $conf;
$this->session = $session;
$this->login = $login;
$this->cookieManager = $cookieManager;
$this->logger = $logger;
}
public function build(): ShaarliContainer
@ -72,6 +78,7 @@ class ContainerBuilder
$container['sessionManager'] = $this->session;
$container['cookieManager'] = $this->cookieManager;
$container['loginManager'] = $this->login;
$container['logger'] = $this->logger;
$container['basePath'] = $this->basePath;
$container['plugins'] = function (ShaarliContainer $container): PluginManager {
@ -99,6 +106,7 @@ class ContainerBuilder
return new PageBuilder(
$container->conf,
$container->sessionManager->getSession(),
$container->logger,
$container->bookmarkService,
$container->sessionManager->generateToken(),
$container->loginManager->isLoggedIn()

2
application/container/ShaarliContainer.php
View file

@ -4,6 +4,7 @@ declare(strict_types=1);
namespace Shaarli\Container;
use Psr\Log\LoggerInterface;
use Shaarli\Bookmark\BookmarkServiceInterface;
use Shaarli\Config\ConfigManager;
use Shaarli\Feed\FeedBuilder;
@ -36,6 +37,7 @@ use Slim\Container;
* @property History $history
* @property HttpAccess $httpAccess
* @property LoginManager $loginManager
* @property LoggerInterface $logger
* @property MetadataRetriever $metadataRetriever
* @property NetscapeBookmarkUtils $netscapeBookmarkUtils
* @property callable $notFoundHandler Overrides default Slim exception display

1
application/front/controller/visitor/LoginController.php
View file

@ -65,7 +65,6 @@ class LoginController extends ShaarliVisitorController
}
if (!$this->container->loginManager->checkCredentials(
$this->container->environment['REMOTE_ADDR'],
client_ip_id($this->container->environment),
$request->getParam('login'),
$request->getParam('password')

29
application/render/PageBuilder.php
View file

@ -3,7 +3,7 @@
namespace Shaarli\Render;
use Exception;
use exceptions\MissingBasePathException;
use Psr\Log\LoggerInterface;
use RainTPL;
use Shaarli\ApplicationUtils;
use Shaarli\Bookmark\BookmarkServiceInterface;
@ -35,6 +35,9 @@ class PageBuilder
*/
protected $session;
/** @var LoggerInterface */
protected $logger;
/**
* @var BookmarkServiceInterface $bookmarkService instance.
*/
@ -54,17 +57,25 @@ class PageBuilder
* PageBuilder constructor.
* $tpl is initialized at false for lazy loading.
*
* @param ConfigManager $conf Configuration Manager instance (reference).
* @param array $session $_SESSION array
* @param BookmarkServiceInterface $linkDB instance.
* @param string $token Session token
* @param bool $isLoggedIn
* @param ConfigManager $conf Configuration Manager instance (reference).
* @param array $session $_SESSION array
* @param LoggerInterface $logger
* @param null $linkDB instance.
* @param null $token Session token
* @param bool $isLoggedIn
*/
public function __construct(&$conf, $session, $linkDB = null, $token = null, $isLoggedIn = false)
{
public function __construct(
ConfigManager &$conf,
array $session,
LoggerInterface $logger,
$linkDB = null,
$token = null,
$isLoggedIn = false
) {
$this->tpl = false;
$this->conf = $conf;
$this->session = $session;
$this->logger = $logger;
$this->bookmarkService = $linkDB;
$this->token = $token;
$this->isLoggedIn = $isLoggedIn;
@ -98,7 +109,7 @@ class PageBuilder
$this->tpl->assign('newVersion', escape($version));
$this->tpl->assign('versionError', '');
} catch (Exception $exc) {
logm($this->conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], $exc->getMessage());
$this->logger->error(format_log('Error: ' . $exc->getMessage(), client_ip_id($_SERVER)));
$this->tpl->assign('newVersion', '');
$this->tpl->assign('versionError', escape($exc->getMessage()));
}

28
application/security/BanManager.php
View file

@ -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;

69
application/security/LoginManager.php
View file

@ -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;
}

19
index.php
View file

@ -25,9 +25,12 @@ require_once 'application/Utils.php';
require_once __DIR__ . '/init.php';
use Katzgrau\KLogger\Logger;
use Psr\Log\LogLevel;
use Shaarli\Config\ConfigManager;
use Shaarli\Container\ContainerBuilder;
use Shaarli\Languages;
use Shaarli\Security\BanManager;
use Shaarli\Security\CookieManager;
use Shaarli\Security\LoginManager;
use Shaarli\Security\SessionManager;
@ -48,10 +51,22 @@ if ($conf->get('dev.debug', false)) {
});
}
$logger = new Logger(
dirname($conf->get('resource.log')),
!$conf->get('dev.debug') ? LogLevel::INFO : LogLevel::DEBUG,
['filename' => basename($conf->get('resource.log'))]
);
$sessionManager = new SessionManager($_SESSION, $conf, session_save_path());
$sessionManager->initialize();
$cookieManager = new CookieManager($_COOKIE);
$loginManager = new LoginManager($conf, $sessionManager, $cookieManager);
$banManager = new BanManager(
$conf->get('security.trusted_proxies', []),
$conf->get('security.ban_after'),
$conf->get('security.ban_duration'),
$conf->get('resource.ban_file', 'data/ipbans.php'),
$logger
);
$loginManager = new LoginManager($conf, $sessionManager, $cookieManager, $banManager, $logger);
$loginManager->generateStaySignedInToken($_SERVER['REMOTE_ADDR']);
// Sniff browser language and set date format accordingly.
@ -71,7 +86,7 @@ date_default_timezone_set($conf->get('general.timezone', 'UTC'));
$loginManager->checkLoginState(client_ip_id($_SERVER));
$containerBuilder = new ContainerBuilder($conf, $sessionManager, $cookieManager, $loginManager);
$containerBuilder = new ContainerBuilder($conf, $sessionManager, $cookieManager, $loginManager, $logger);
$container = $containerBuilder->build();
$app = new App($container);

36
tests/UtilsTest.php
View file

@ -63,41 +63,25 @@ class UtilsTest extends \Shaarli\TestCase
}
/**
* Log a message to a file - IPv4 client address
* Format a log a message - IPv4 client address
*/
public function testLogmIp4()
public function testFormatLogIp4()
{
$logMessage = 'IPv4 client connected';
logm(self::$testLogFile, '127.0.0.1', $logMessage);
list($date, $ip, $message) = $this->getLastLogEntry();
$message = 'IPv4 client connected';
$log = format_log($message, '127.0.0.1');
$this->assertInstanceOf(
'DateTime',
DateTime::createFromFormat(self::$dateFormat, $date)
);
$this->assertTrue(
filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4) !== false
);
$this->assertEquals($logMessage, $message);
static::assertSame('- 127.0.0.1 - IPv4 client connected', $log);
}
/**
* Log a message to a file - IPv6 client address
* Format a log a message - IPv6 client address
*/
public function testLogmIp6()
public function testFormatLogIp6()
{
$logMessage = 'IPv6 client connected';
logm(self::$testLogFile, '2001:db8::ff00:42:8329', $logMessage);
list($date, $ip, $message) = $this->getLastLogEntry();
$message = 'IPv6 client connected';
$log = format_log($message, '2001:db8::ff00:42:8329');
$this->assertInstanceOf(
'DateTime',
DateTime::createFromFormat(self::$dateFormat, $date)
);
$this->assertTrue(
filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false
);
$this->assertEquals($logMessage, $message);
static::assertSame('- 2001:db8::ff00:42:8329 - IPv6 client connected', $log);
}
/**

5
tests/container/ContainerBuilderTest.php
View file

@ -4,6 +4,7 @@ declare(strict_types=1);
namespace Shaarli\Container;
use Psr\Log\LoggerInterface;
use Shaarli\Bookmark\BookmarkServiceInterface;
use Shaarli\Config\ConfigManager;
use Shaarli\Feed\FeedBuilder;
@ -55,7 +56,8 @@ class ContainerBuilderTest extends TestCase
$this->conf,
$this->sessionManager,
$this->cookieManager,
$this->loginManager
$this->loginManager,
$this->createMock(LoggerInterface::class)
);
}
@ -73,6 +75,7 @@ class ContainerBuilderTest extends TestCase
static::assertInstanceOf(History::class, $container->history);
static::assertInstanceOf(HttpAccess::class, $container->httpAccess);
static::assertInstanceOf(LoginManager::class, $container->loginManager);
static::assertInstanceOf(LoggerInterface::class, $container->logger);
static::assertInstanceOf(MetadataRetriever::class, $container->metadataRetriever);
static::assertInstanceOf(NetscapeBookmarkUtils::class, $container->netscapeBookmarkUtils);
static::assertInstanceOf(PageBuilder::class, $container->pageBuilder);

2
tests/front/controller/visitor/LoginControllerTest.php
View file

@ -195,7 +195,7 @@ class LoginControllerTest extends TestCase
$this->container->loginManager
->expects(static::once())
->method('checkCredentials')
->with('1.2.3.4', '1.2.3.4', 'bob', 'pass')
->with('1.2.3.4', 'bob', 'pass')
->willReturn(true)
;
$this->container->loginManager->method('getStaySignedInToken')->willReturn(bin2hex(random_bytes(8)));

3
tests/security/BanManagerTest.php
View file

@ -3,6 +3,7 @@
namespace Shaarli\Security;
use Psr\Log\LoggerInterface;
use Shaarli\FileUtils;
use Shaarli\TestCase;
@ -387,7 +388,7 @@ class BanManagerTest extends TestCase
3,
1800,
$this->banFile,
$this->logFile
$this->createMock(LoggerInterface::class)
);
}
}

51
tests/security/LoginManagerTest.php
View file

@ -2,6 +2,8 @@
namespace Shaarli\Security;
use Psr\Log\LoggerInterface;
use Shaarli\FakeConfigManager;
use Shaarli\TestCase;
/**
@ -9,7 +11,7 @@ use Shaarli\TestCase;
*/
class LoginManagerTest extends TestCase
{
/** @var \FakeConfigManager Configuration Manager instance */
/** @var FakeConfigManager Configuration Manager instance */
protected $configManager = null;
/** @var LoginManager Login Manager instance */
@ -60,6 +62,9 @@ class LoginManagerTest extends TestCase
/** @var CookieManager */
protected $cookieManager;
/** @var BanManager */
protected $banManager;
/**
* Prepare or reset test resources
*/
@ -71,7 +76,7 @@ class LoginManagerTest extends TestCase
$this->passwordHash = sha1($this->password . $this->login . $this->salt);
$this->configManager = new \FakeConfigManager([
$this->configManager = new FakeConfigManager([
'credentials.login' => $this->login,
'credentials.hash' => $this->passwordHash,
'credentials.salt' => $this->salt,
@ -91,18 +96,29 @@ class LoginManagerTest extends TestCase
return $this->cookie[$key] ?? null;
});
$this->sessionManager = new SessionManager($this->session, $this->configManager, 'session_path');
$this->loginManager = new LoginManager($this->configManager, $this->sessionManager, $this->cookieManager);
$this->banManager = $this->createMock(BanManager::class);
$this->loginManager = new LoginManager(
$this->configManager,
$this->sessionManager,
$this->cookieManager,
$this->banManager,
$this->createMock(LoggerInterface::class)
);
$this->server['REMOTE_ADDR'] = $this->ipAddr;
}
/**
* Record a failed login attempt
*/
public function testHandleFailedLogin()
public function testHandleFailedLogin(): void
{
$this->banManager->expects(static::exactly(2))->method('handleFailedAttempt');
$this->banManager->method('isBanned')->willReturn(true);
$this->loginManager->handleFailedLogin($this->server);
$this->loginManager->handleFailedLogin($this->server);
$this->assertFalse($this->loginManager->canLogin($this->server));
static::assertFalse($this->loginManager->canLogin($this->server));
}
/**
@ -114,8 +130,13 @@ class LoginManagerTest extends TestCase
'REMOTE_ADDR' => $this->trustedProxy,
'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
];
$this->banManager->expects(static::exactly(2))->method('handleFailedAttempt');
$this->banManager->method('isBanned')->willReturn(true);
$this->loginManager->handleFailedLogin($server);
$this->loginManager->handleFailedLogin($server);
$this->assertFalse($this->loginManager->canLogin($server));
}
@ -196,10 +217,16 @@ class LoginManagerTest extends TestCase
*/
public function testCheckLoginStateNotConfigured()
{
$configManager = new \FakeConfigManager([
$configManager = new FakeConfigManager([
'resource.ban_file' => $this->banFile,
]);
$loginManager = new LoginManager($configManager, null, $this->cookieManager);
$loginManager = new LoginManager(
$configManager,
$this->sessionManager,
$this->cookieManager,
$this->banManager,
$this->createMock(LoggerInterface::class)
);
$loginManager->checkLoginState('');
$this->assertFalse($loginManager->isLoggedIn());
@ -270,7 +297,7 @@ class LoginManagerTest extends TestCase
public function testCheckCredentialsWrongLogin()
{
$this->assertFalse(
$this->loginManager->checkCredentials('', '', 'b4dl0g1n', $this->password)
$this->loginManager->checkCredentials('', 'b4dl0g1n', $this->password)
);
}
@ -280,7 +307,7 @@ class LoginManagerTest extends TestCase
public function testCheckCredentialsWrongPassword()
{
$this->assertFalse(
$this->loginManager->checkCredentials('', '', $this->login, 'b4dp455wd')
$this->loginManager->checkCredentials('', $this->login, 'b4dp455wd')
);
}
@ -290,7 +317,7 @@ class LoginManagerTest extends TestCase
public function testCheckCredentialsWrongLoginAndPassword()
{
$this->assertFalse(
$this->loginManager->checkCredentials('', '', 'b4dl0g1n', 'b4dp455wd')
$this->loginManager->checkCredentials('', 'b4dl0g1n', 'b4dp455wd')
);
}
@ -300,7 +327,7 @@ class LoginManagerTest extends TestCase
public function testCheckCredentialsGoodLoginAndPassword()
{
$this->assertTrue(
$this->loginManager->checkCredentials('', '', $this->login, $this->password)
$this->loginManager->checkCredentials('', $this->login, $this->password)
);
}
@ -311,7 +338,7 @@ class LoginManagerTest extends TestCase
{
$this->configManager->set('ldap.host', 'dummy');
$this->assertFalse(
$this->loginManager->checkCredentials('', '', $this->login, $this->password)
$this->loginManager->checkCredentials('', $this->login, $this->password)
);
}

5
tests/security/SessionManagerTest.php
View file

@ -2,6 +2,7 @@
namespace Shaarli\Security;
use Shaarli\FakeConfigManager;
use Shaarli\TestCase;
/**
@ -12,7 +13,7 @@ class SessionManagerTest extends TestCase
/** @var array Session ID hashes */
protected static $sidHashes = null;
/** @var \FakeConfigManager ConfigManager substitute for testing */
/** @var FakeConfigManager ConfigManager substitute for testing */
protected $conf = null;
/** @var array $_SESSION array for testing */
@ -34,7 +35,7 @@ class SessionManagerTest extends TestCase
*/
protected function setUp(): void
{
$this->conf = new \FakeConfigManager([
$this->conf = new FakeConfigManager([
'credentials.login' => 'johndoe',
'credentials.salt' => 'salt',
'security.session_protection_disabled' => false,

10
tests/utils/FakeConfigManager.php
View file

@ -1,9 +1,13 @@
<?php
namespace Shaarli;
use Shaarli\Config\ConfigManager;
/**
* Fake ConfigManager
*/
class FakeConfigManager
class FakeConfigManager extends ConfigManager
{
protected $values = [];
@ -23,7 +27,7 @@ class FakeConfigManager
* @param string $key Key of the value to set
* @param mixed $value Value to set
*/
public function set($key, $value)
public function set($key, $value, $write = false, $isLoggedIn = false)
{
$this->values[$key] = $value;
}
@ -35,7 +39,7 @@ class FakeConfigManager
*
* @return mixed The value if set, else the name of the key
*/
public function get($key)
public function get($key, $default = '')
{
if (isset($this->values[$key])) {
return $this->values[$key];