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

Refactor login / ban authentication steps 

Relates to https://github.com/shaarli/Shaarli/issues/324

Added:
- Add the `LoginManager` class to manage logins and bans

Changed:
- Refactor IP ban management
- Simplify logic
- Avoid using globals, inject dependencies

Fixed:
- Use `ban_duration` instead of `ban_after` when setting a new ban

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
This commit is contained in:
VirtualTam 2017-10-25 23:03:31 +02:00
parent a381c373b3
commit 44acf70681
6 changed files with 385 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

134
application/LoginManager.php Normal file
View File

@ -0,0 +1,134 @@
<?php
namespace Shaarli;
/**
* User login management
*/
class LoginManager
{
protected $globals = [];
protected $configManager = null;
protected $banFile = '';
/**
* Constructor
*
* @param array $globals The $GLOBALS array (reference)
* @param ConfigManager $configManager Configuration Manager instance.
*/
public function __construct(& $globals, $configManager)
{
$this->globals = &$globals;
$this->configManager = $configManager;
$this->banFile = $this->configManager->get('resource.ban_file', 'data/ipbans.php');
$this->readBanFile();
}
/**
* Read a file containing banned IPs
*/
protected function readBanFile()
{
if (! file_exists($this->banFile)) {
return;
}
include $this->banFile;
}
/**
* Write the banned IPs to a file
*/
protected function writeBanFile()
{
if (! array_key_exists('IPBANS', $this->globals)) {
return;
}
file_put_contents(
$this->banFile,
"<?php\n\$GLOBALS['IPBANS']=" . var_export($this->globals['IPBANS'], true) . ";\n?>"
);
}
/**
* Handle a failed login and ban the IP after too many failed attempts
*
* @param array $server The $_SERVER array
*/
public function handleFailedLogin($server)
{
$ip = $server['REMOTE_ADDR'];
$trusted = $this->configManager->get('security.trusted_proxies', []);
if (in_array($ip, $trusted)) {
$ip = getIpAddressFromProxy($server, $trusted);
if (! $ip) {
// the IP is behind a trusted forward proxy, but is not forwarded
// in the HTTP headers, so we do nothing
return;
}
}
// increment the fail count for this IP
if (isset($this->globals['IPBANS']['FAILURES'][$ip])) {
$this->globals['IPBANS']['FAILURES'][$ip]++;
} else {
$this->globals['IPBANS']['FAILURES'][$ip] = 1;
}
if ($this->globals['IPBANS']['FAILURES'][$ip] >= $this->configManager->get('security.ban_after')) {
$this->globals['IPBANS']['BANS'][$ip] = time() + $this->configManager->get('security.ban_duration', 1800);
logm(
$this->configManager->get('resource.log'),
$server['REMOTE_ADDR'],
'IP address banned from login'
);
}
$this->writeBanFile();
}
/**
* Handle a successful login
*
* @param array $server The $_SERVER array
*/
public function handleSuccessfulLogin($server)
{
$ip = $server['REMOTE_ADDR'];
// FIXME unban when behind a trusted proxy?
unset($this->globals['IPBANS']['FAILURES'][$ip]);
unset($this->globals['IPBANS']['BANS'][$ip]);
$this->writeBanFile();
}
/**
* Check if the user can login from this IP
*
* @param array $server The $_SERVER array
*
* @return bool true if the user is allowed to login
*/
public function canLogin($server)
{
$ip = $server['REMOTE_ADDR'];
if (! isset($this->globals['IPBANS']['BANS'][$ip])) {
// the user is not banned
return true;
}
if ($this->globals['IPBANS']['BANS'][$ip] > time()) {
// the user is still banned
return false;
}
// the ban has expired, the user can attempt to log in again
logm($this->configManager->get('resource.log'), $server['REMOTE_ADDR'], 'Ban lifted.');
unset($this->globals['IPBANS']['FAILURES'][$ip]);
unset($this->globals['IPBANS']['BANS'][$ip]);
$this->writeBanFile();
return true;
}
}

116
index.php
View File

@ -78,6 +78,7 @@ require_once 'application/Updater.php';
use \Shaarli\Languages;
use \Shaarli\ThemeUtils;
use \Shaarli\Config\ConfigManager;
use \Shaarli\LoginManager;
use \Shaarli\SessionManager;
// Ensure the PHP version is supported
@ -122,6 +123,7 @@ if (isset($_COOKIE['shaarli']) && !SessionManager::checkId($_COOKIE['shaarli']))
}
$conf = new ConfigManager();
$loginManager = new LoginManager($GLOBALS, $conf);
$sessionManager = new SessionManager($_SESSION, $conf);
// LC_MESSAGES isn't defined without php-intl, in this case use LC_COLLATE locale instead.
@ -293,108 +295,22 @@ function logout() {
setcookie('shaarli_staySignedIn', FALSE, 0, WEB_PATH);
}
// ------------------------------------------------------------------------------------------
// Brute force protection system
// Several consecutive failed logins will ban the IP address for 30 minutes.
if (!is_file($conf->get('resource.ban_file', 'data/ipbans.php'))) {
// FIXME! globals
file_put_contents(
$conf->get('resource.ban_file', 'data/ipbans.php'),
"<?php\n\$GLOBALS['IPBANS']=".var_export(array('FAILURES'=>array(),'BANS'=>array()),true).";\n?>"
);
}
include $conf->get('resource.ban_file', 'data/ipbans.php');
/**
* Signal a failed login. Will ban the IP if too many failures:
*
* @param ConfigManager $conf Configuration Manager instance.
*/
function ban_loginFailed($conf)
{
$ip = $_SERVER['REMOTE_ADDR'];
$trusted = $conf->get('security.trusted_proxies', array());
if (in_array($ip, $trusted)) {
$ip = getIpAddressFromProxy($_SERVER, $trusted);
if (!$ip) {
return;
}
}
$gb = $GLOBALS['IPBANS'];
if (! isset($gb['FAILURES'][$ip])) {
$gb['FAILURES'][$ip]=0;
}
$gb['FAILURES'][$ip]++;
if ($gb['FAILURES'][$ip] > ($conf->get('security.ban_after') - 1))
{
$gb['BANS'][$ip] = time() + $conf->get('security.ban_after', 1800);
logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'IP address banned from login');
}
$GLOBALS['IPBANS'] = $gb;
file_put_contents(
$conf->get('resource.ban_file', 'data/ipbans.php'),
"<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
);
}
/**
* Signals a successful login. Resets failed login counter.
*
* @param ConfigManager $conf Configuration Manager instance.
*/
function ban_loginOk($conf)
{
$ip = $_SERVER['REMOTE_ADDR'];
$gb = $GLOBALS['IPBANS'];
unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]);
$GLOBALS['IPBANS'] = $gb;
file_put_contents(
$conf->get('resource.ban_file', 'data/ipbans.php'),
"<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
);
}
/**
* Checks if the user CAN login. If 'true', the user can try to login.
*
* @param ConfigManager $conf Configuration Manager instance.
*
* @return bool: true if the user is allowed to login.
*/
function ban_canLogin($conf)
{
$ip=$_SERVER["REMOTE_ADDR"]; $gb=$GLOBALS['IPBANS'];
if (isset($gb['BANS'][$ip]))
{
// User is banned. Check if the ban has expired:
if ($gb['BANS'][$ip]<=time())
{ // Ban expired, user can try to login again.
logm($conf->get('resource.log'), $_SERVER['REMOTE_ADDR'], 'Ban lifted.');
unset($gb['FAILURES'][$ip]); unset($gb['BANS'][$ip]);
file_put_contents(
$conf->get('resource.ban_file', 'data/ipbans.php'),
"<?php\n\$GLOBALS['IPBANS']=".var_export($gb,true).";\n?>"
);
return true; // Ban has expired, user can login.
}
return false; // User is banned.
}
return true; // User is not banned.
}
// ------------------------------------------------------------------------------------------
// Process login form: Check if login/password is correct.
if (isset($_POST['login']))
{
if (!ban_canLogin($conf)) die(t('I said: NO. You are banned for the moment. Go away.'));
if (! $loginManager->canLogin($_SERVER)) {
die(t('I said: NO. You are banned for the moment. Go away.'));
}
if (isset($_POST['password'])
&& $sessionManager->checkToken($_POST['token'])
&& (check_auth($_POST['login'], $_POST['password'], $conf))
) { // Login/password is OK.
ban_loginOk($conf);
) {
// Login/password is OK.
$loginManager->handleSuccessfulLogin($_SERVER);
// If user wants to keep the session cookie even after the browser closes:
if (!empty($_POST['longlastingsession']))
{
if (!empty($_POST['longlastingsession'])) {
$_SESSION['longlastingsession'] = 31536000; // (31536000 seconds = 1 year)
$expiration = time() + $_SESSION['longlastingsession']; // calculate relative cookie expiration (1 year from now)
setcookie('shaarli_staySignedIn', STAY_SIGNED_IN_TOKEN, $expiration, WEB_PATH);
@ -437,10 +353,8 @@ if (isset($_POST['login']))
}
}
header('Location: ?'); exit;
}
else
{
ban_loginFailed($conf);
} else {
$loginManager->handleFailedLogin($_SERVER);
$redir = '&username='. urlencode($_POST['login']);
if (isset($_GET['post'])) {
$redir .= '&post=' . urlencode($_GET['post']);
@ -684,8 +598,9 @@ function showLinkList($PAGE, $LINKSDB, $conf, $pluginManager) {
* @param LinkDB $LINKSDB
* @param History $history instance
* @param SessionManager $sessionManager SessionManager instance
* @param LoginManager $loginManager LoginManager instance
*/
function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager)
function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager, $loginManager)
{
$updater = new Updater(
read_updates_file($conf->get('resource.updates')),
@ -761,6 +676,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager)
$PAGE->assign('returnurl',(isset($_SERVER['HTTP_REFERER']) ? escape($_SERVER['HTTP_REFERER']):''));
// add default state of the 'remember me' checkbox
$PAGE->assign('remember_user_default', $conf->get('privacy.remember_user_default'));
$PAGE->assign('user_can_login', $loginManager->canLogin($_SERVER));
$PAGE->renderPage('loginform');
exit;
}
@ -2330,7 +2246,7 @@ $response = $app->run(true);
if ($response->getStatusCode() == 404 && strpos($_SERVER['REQUEST_URI'], '/api/v1') === false) {
// We use UTF-8 for proper international characters handling.
header('Content-Type: text/html; charset=utf-8');
renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager);
renderPage($conf, $pluginManager, $linkDb, $history, $sessionManager, $loginManager);
} else {
$app->respond($response);
}

199
tests/LoginManagerTest.php Normal file
View File

@ -0,0 +1,199 @@
<?php
namespace Shaarli;
require_once 'tests/utils/FakeConfigManager.php';
use \PHPUnit\Framework\TestCase;
/**
* Test coverage for LoginManager
*/
class LoginManagerTest extends TestCase
{
protected $configManager = null;
protected $loginManager = null;
protected $banFile = 'sandbox/ipbans.php';
protected $logFile = 'sandbox/shaarli.log';
protected $globals = [];
protected $ipAddr = '127.0.0.1';
protected $server = [];
protected $trustedProxy = '10.1.1.100';
/**
* Prepare or reset test resources
*/
public function setUp()
{
if (file_exists($this->banFile)) {
unlink($this->banFile);
}
$this->configManager = new \FakeConfigManager([
'resource.ban_file' => $this->banFile,
'resource.log' => $this->logFile,
'security.ban_after' => 4,
'security.ban_duration' => 3600,
'security.trusted_proxies' => [$this->trustedProxy],
]);
$this->globals = &$GLOBALS;
unset($this->globals['IPBANS']);
$this->loginManager = new LoginManager($this->globals, $this->configManager);
$this->server['REMOTE_ADDR'] = $this->ipAddr;
}
/**
* Wipe test resources
*/
public function tearDown()
{
unset($this->globals['IPBANS']);
}
/**
* Instantiate a LoginManager and load ban records
*/
public function testReadBanFile()
{
file_put_contents(
$this->banFile,
"<?php\n\$GLOBALS['IPBANS']=array('FAILURES' => array('127.0.0.1' => 99));\n?>"
);
new LoginManager($this->globals, $this->configManager);
$this->assertEquals(99, $this->globals['IPBANS']['FAILURES']['127.0.0.1']);
}
/**
* Record a failed login attempt
*/
public function testHandleFailedLogin()
{
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
}
/**
* Record a failed login attempt - IP behind a trusted proxy
*/
public function testHandleFailedLoginBehindTrustedProxy()
{
$server = [
'REMOTE_ADDR' => $this->trustedProxy,
'HTTP_X_FORWARDED_FOR' => $this->ipAddr,
];
$this->loginManager->handleFailedLogin($server);
$this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->loginManager->handleFailedLogin($server);
$this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
}
/**
* Record a failed login attempt - IP behind a trusted proxy but not forwarded
*/
public function testHandleFailedLoginBehindTrustedProxyNoIp()
{
$server = [
'REMOTE_ADDR' => $this->trustedProxy,
];
$this->loginManager->handleFailedLogin($server);
$this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
$this->loginManager->handleFailedLogin($server);
$this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
}
/**
* Record a failed login attempt and ban the IP after too many failures
*/
public function testHandleFailedLoginBanIp()
{
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(1, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(3, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(4, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertFalse($this->loginManager->canLogin($this->server));
// handleFailedLogin is not supposed to be called at this point:
// - no login form should be displayed once an IP has been banned
// - yet this could happen when using custom templates / scripts
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(5, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertFalse($this->loginManager->canLogin($this->server));
}
/**
* Nothing to do
*/
public function testHandleSuccessfulLogin()
{
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->loginManager->handleSuccessfulLogin($this->server);
$this->assertTrue($this->loginManager->canLogin($this->server));
}
/**
* Erase failure records after successfully logging in from this IP
*/
public function testHandleSuccessfulLoginAfterFailure()
{
$this->loginManager->handleFailedLogin($this->server);
$this->loginManager->handleFailedLogin($this->server);
$this->assertEquals(2, $this->globals['IPBANS']['FAILURES'][$this->ipAddr]);
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->loginManager->handleSuccessfulLogin($this->server);
$this->assertTrue($this->loginManager->canLogin($this->server));
$this->assertFalse(isset($this->globals['IPBANS']['FAILURES'][$this->ipAddr]));
$this->assertFalse(isset($this->globals['IPBANS']['BANS'][$this->ipAddr]));
}
/**
* The IP is not banned
*/
public function testCanLoginIpNotBanned()
{
$this->assertTrue($this->loginManager->canLogin($this->server));
}
/**
* The IP is banned
*/
public function testCanLoginIpBanned()
{
// ban the IP for an hour
$this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10;
$this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600;
$this->assertFalse($this->loginManager->canLogin($this->server));
}
/**
* The IP is banned, and the ban duration is over
*/
public function testCanLoginIpBanExpired()
{
// ban the IP for an hour
$this->globals['IPBANS']['FAILURES'][$this->ipAddr] = 10;
$this->globals['IPBANS']['BANS'][$this->ipAddr] = time() + 3600;
$this->assertFalse($this->loginManager->canLogin($this->server));
// lift the ban
$this->globals['IPBANS']['BANS'][$this->ipAddr] = time() - 3600;
$this->assertTrue($this->loginManager->canLogin($this->server));
}
}

35
tests/utils/FakeConfigManager.php
View File

@ -5,8 +5,41 @@
*/
class FakeConfigManager
{
public static function get($key)
protected $values = [];
/**
* Initialize with test values
*
* @param array $values Initial values
*/
public function __construct($values = [])
{
$this->values = $values;
}
/**
* Set a given value
*
* @param string $key Key of the value to set
* @param mixed $value Value to set
*/
public function set($key, $value)
{
$this->values[$key] = $value;
}
/**
* Get a given configuration value
*
* @param string $key Index of the value to retrieve
*
* @return mixed The value if set, else the name of the key
*/
public function get($key)
{
if (isset($this->values[$key])) {
return $this->values[$key];
}
return $key;
}
}

2
tpl/default/loginform.html
View File

@ -5,7 +5,7 @@
</head>
<body>
{include="page.header"}
{if="!ban_canLogin($conf)"}
{if="!$user_can_login"}
<div class="pure-g pure-alert pure-alert-error pure-alert-closable center">
<div class="pure-u-2-24"></div>
<div class="pure-u-20-24">

2
tpl/vintage/loginform.html
View File

@ -2,7 +2,7 @@
<html>
<head>{include="includes"}</head>
<body
{if="ban_canLogin($conf)"}
{if="$user_can_login"}
{if="empty($username)"}
onload="document.loginform.login.focus();"
{else}