From 44acf706812bc77812e6648c2cc28af36e172a14 Mon Sep 17 00:00:00 2001 From: VirtualTam Date: Wed, 25 Oct 2017 23:03:31 +0200 Subject: 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 --- tests/LoginManagerTest.php | 199 ++++++++++++++++++++++++++++++++++++++ tests/utils/FakeConfigManager.php | 35 ++++++- 2 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 tests/LoginManagerTest.php (limited to 'tests') diff --git a/tests/LoginManagerTest.php b/tests/LoginManagerTest.php new file mode 100644 index 00000000..4159038e --- /dev/null +++ b/tests/LoginManagerTest.php @@ -0,0 +1,199 @@ +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, + " 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)); + } +} diff --git a/tests/utils/FakeConfigManager.php b/tests/utils/FakeConfigManager.php index f29760cb..85434de7 100644 --- a/tests/utils/FakeConfigManager.php +++ b/tests/utils/FakeConfigManager.php @@ -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; } } -- cgit v1.2.3