From 2c6fe245218010606c34c46ce788498e2de1aedc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Thu, 24 Aug 2023 00:52:30 +0200 Subject: [PATCH] Move login attempt counting to a separate class This has nothing to do with CAPTCHA generation, and the only thing it needs from the SimpleCaptcha class is checking whether a CAPTCHA on bad login is enabled at all. Also improve comments in CaptchaPreAuthenticationProvider. I found the session flag business really difficult to understand. Change-Id: I8200531718aaa11effcb07539204e1a05ed432e0 --- SimpleCaptcha/SimpleCaptcha.php | 96 --------------- .../Auth/CaptchaPreAuthenticationProvider.php | 41 ++++--- includes/Auth/LoginAttemptCounter.php | 116 ++++++++++++++++++ .../CaptchaPreAuthenticationProviderTest.php | 56 ++++++--- 4 files changed, 178 insertions(+), 131 deletions(-) create mode 100644 includes/Auth/LoginAttemptCounter.php diff --git a/SimpleCaptcha/SimpleCaptcha.php b/SimpleCaptcha/SimpleCaptcha.php index 62dfae91..e9e66cc5 100644 --- a/SimpleCaptcha/SimpleCaptcha.php +++ b/SimpleCaptcha/SimpleCaptcha.php @@ -4,7 +4,6 @@ namespace MediaWiki\Extension\ConfirmEdit\SimpleCaptcha; use ApiBase; use ApiEditPage; -use BagOStuff; use Content; use ExtensionRegistry; use HTMLForm; @@ -31,9 +30,7 @@ use MediaWiki\Revision\SlotRecord; use MediaWiki\Status\Status; use MediaWiki\Title\Title; use MediaWiki\User\User; -use MediaWiki\User\UserNameUtils; use Message; -use ObjectCache; use OOUI\FieldLayout; use OOUI\HiddenInputWidget; use OOUI\NumberInputWidget; @@ -352,72 +349,6 @@ class SimpleCaptcha { return true; } - /** - * Increase bad login counter after a failed login. - * The user might be required to solve a captcha if the count is high. - * @param string $username - * TODO use Throttler - */ - public function increaseBadLoginCounter( $username ) { - global $wgCaptchaBadLoginExpiration, $wgCaptchaBadLoginPerUserExpiration; - - $cache = ObjectCache::getLocalClusterInstance(); - - if ( $this->triggersCaptcha( CaptchaTriggers::BAD_LOGIN ) ) { - $key = $this->badLoginKey( $cache ); - $cache->incrWithInit( $key, $wgCaptchaBadLoginExpiration ); - } - - if ( $this->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) && $username ) { - $key = $this->badLoginPerUserKey( $username, $cache ); - $cache->incrWithInit( $key, $wgCaptchaBadLoginPerUserExpiration ); - } - } - - /** - * Reset bad login counter after a successful login. - * @param string $username - */ - public function resetBadLoginCounter( $username ) { - if ( $this->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) && $username ) { - $cache = ObjectCache::getLocalClusterInstance(); - $cache->delete( $this->badLoginPerUserKey( $username, $cache ) ); - } - } - - /** - * Check if a bad login has already been registered for this - * IP address. If so, require a captcha. - * @return bool - * @private - */ - public function isBadLoginTriggered() { - global $wgCaptchaBadLoginAttempts; - - $cache = ObjectCache::getLocalClusterInstance(); - return $this->triggersCaptcha( CaptchaTriggers::BAD_LOGIN ) - && (int)$cache->get( $this->badLoginKey( $cache ) ) >= $wgCaptchaBadLoginAttempts; - } - - /** - * Is the per-user captcha triggered? - * - * @param User|string $u User object, or name - * @return bool - */ - public function isBadLoginPerUserTriggered( $u ) { - global $wgCaptchaBadLoginPerUserAttempts; - - $cache = ObjectCache::getLocalClusterInstance(); - - if ( is_object( $u ) ) { - $u = $u->getName(); - } - $badLoginPerUserKey = $this->badLoginPerUserKey( $u, $cache ); - return $this->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) - && (int)$cache->get( $badLoginPerUserKey ) >= $wgCaptchaBadLoginPerUserAttempts; - } - /** * Check if the current IP is allowed to skip captchas. This checks * the whitelist from two sources. @@ -503,33 +434,6 @@ class SimpleCaptcha { return $validIPs; } - /** - * Internal cache key for badlogin checks. - * @param BagOStuff $cache - * @return string - */ - private function badLoginKey( BagOStuff $cache ) { - global $wgRequest; - $ip = $wgRequest->getIP(); - - return $cache->makeGlobalKey( 'captcha', 'badlogin', 'ip', $ip ); - } - - /** - * Cache key for badloginPerUser checks. - * @param string $username - * @param BagOStuff $cache - * @return string - */ - private function badLoginPerUserKey( $username, BagOStuff $cache ) { - $userNameUtils = MediaWikiServices::getInstance()->getUserNameUtils(); - $username = $userNameUtils->getCanonical( $username, UserNameUtils::RIGOR_USABLE ) ?: $username; - - return $cache->makeGlobalKey( - 'captcha', 'badlogin', 'user', md5( $username ) - ); - } - /** * Check if the submitted form matches the captcha session data provided * by the plugin when the form was generated. diff --git a/includes/Auth/CaptchaPreAuthenticationProvider.php b/includes/Auth/CaptchaPreAuthenticationProvider.php index 3069317f..cd342e5b 100644 --- a/includes/Auth/CaptchaPreAuthenticationProvider.php +++ b/includes/Auth/CaptchaPreAuthenticationProvider.php @@ -39,24 +39,27 @@ class CaptchaPreAuthenticationProvider extends AbstractPreAuthenticationProvider } break; case AuthManager::ACTION_LOGIN: - // Captcha is shown on login when there were too many failed attempts from the - // current IP or user. The latter is a bit awkward because we don't know the + $loginCounter = $this->getLoginAttemptCounter( $captcha ); + // Captcha is shown on login when there were too many failed attempts from the current IP + // or using a given username. The latter is a bit awkward because we don't know the // username yet. The username from the last successful login is stored in a cookie, - // but we still must make sure to not lock out other usernames so we use a session - // flag. This will result in confusing error messages if the browser cannot persist - // the session, but then login would be impossible anyway so no big deal. + // but we still must make sure to not lock out other usernames, so after the first + // failed login attempt using a username that needs a captcha, set a session flag + // to display a captcha on login from that point on. This will result in confusing + // error messages if the browser cannot persist the session (because we'll never show + // a required captcha field), but then login would be impossible anyway so no big deal. // If the username ends to be one that does not trigger the captcha, that will - // result in weird behavior (if the user leaves the captcha field open, they get - // a required field error, if they fill it with an invalid answer, it will pass) + // result in weird behavior (if the user leaves the captcha field empty, they get + // a required field error; if they fill it with an invalid answer, it will pass) // - again, not a huge deal. $session = $this->manager->getRequest()->getSession(); - $sessionFlag = $session->get( 'ConfirmEdit:loginCaptchaPerUserTriggered' ); + $userProbablyNeedsCaptcha = $session->get( 'ConfirmEdit:loginCaptchaPerUserTriggered' ); $suggestedUsername = $session->suggestLoginUsername(); if ( - $captcha->isBadLoginTriggered() - || $sessionFlag - || $suggestedUsername && $captcha->isBadLoginPerUserTriggered( $suggestedUsername ) + $loginCounter->isBadLoginTriggered() + || $userProbablyNeedsCaptcha + || $suggestedUsername && $loginCounter->isBadLoginPerUserTriggered( $suggestedUsername ) ) { $needed = true; $captcha->setAction( 'badlogin' ); @@ -85,11 +88,12 @@ class CaptchaPreAuthenticationProvider extends AbstractPreAuthenticationProvider public function testForAuthentication( array $reqs ) { $captcha = Hooks::getInstance(); $username = AuthenticationRequest::getUsernameFromRequests( $reqs ); + $loginCounter = $this->getLoginAttemptCounter( $captcha ); $success = true; $isBadLoginPerUserTriggered = $username ? - $captcha->isBadLoginPerUserTriggered( $username ) : false; + $loginCounter->isBadLoginPerUserTriggered( $username ) : false; - if ( $captcha->isBadLoginTriggered() || $isBadLoginPerUserTriggered ) { + if ( $loginCounter->isBadLoginTriggered() || $isBadLoginPerUserTriggered ) { $captcha->setAction( 'badlogin' ); $captcha->setTrigger( "post-badlogin login '$username'" ); $success = $this->verifyCaptcha( $captcha, $reqs, new User() ); @@ -105,6 +109,7 @@ class CaptchaPreAuthenticationProvider extends AbstractPreAuthenticationProvider if ( $isBadLoginPerUserTriggered ) { $session = $this->manager->getRequest()->getSession(); + // A captcha is needed to log in with this username, so display it on the next attempt. $session->set( 'ConfirmEdit:loginCaptchaPerUserTriggered', true ); } @@ -146,15 +151,16 @@ class CaptchaPreAuthenticationProvider extends AbstractPreAuthenticationProvider */ public function postAuthentication( $user, AuthenticationResponse $response ) { $captcha = Hooks::getInstance(); + $loginCounter = $this->getLoginAttemptCounter( $captcha ); switch ( $response->status ) { case AuthenticationResponse::PASS: case AuthenticationResponse::RESTART: $session = $this->manager->getRequest()->getSession(); $session->remove( 'ConfirmEdit:loginCaptchaPerUserTriggered' ); - $captcha->resetBadLoginCounter( $user ? $user->getName() : null ); + $loginCounter->resetBadLoginCounter( $user ? $user->getName() : null ); break; case AuthenticationResponse::FAIL: - $captcha->increaseBadLoginCounter( $user ? $user->getName() : null ); + $loginCounter->increaseBadLoginCounter( $user ? $user->getName() : null ); break; } } @@ -189,4 +195,9 @@ class CaptchaPreAuthenticationProvider extends AbstractPreAuthenticationProvider } return Status::newFatal( $message ); } + + protected function getLoginAttemptCounter( SimpleCaptcha $captcha ): LoginAttemptCounter { + // Overridable for testing + return new LoginAttemptCounter( $captcha ); + } } diff --git a/includes/Auth/LoginAttemptCounter.php b/includes/Auth/LoginAttemptCounter.php new file mode 100644 index 00000000..79196187 --- /dev/null +++ b/includes/Auth/LoginAttemptCounter.php @@ -0,0 +1,116 @@ +captcha = $captcha; + } + + /** + * Increase bad login counter after a failed login. + * The user might be required to solve a captcha if the count is high. + * @param string $username + * TODO use Throttler + */ + public function increaseBadLoginCounter( $username ) { + global $wgCaptchaBadLoginExpiration, $wgCaptchaBadLoginPerUserExpiration; + + $cache = ObjectCache::getLocalClusterInstance(); + + if ( $this->captcha->triggersCaptcha( CaptchaTriggers::BAD_LOGIN ) ) { + $key = $this->badLoginKey( $cache ); + $cache->incrWithInit( $key, $wgCaptchaBadLoginExpiration ); + } + + if ( $this->captcha->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) && $username ) { + $key = $this->badLoginPerUserKey( $username, $cache ); + $cache->incrWithInit( $key, $wgCaptchaBadLoginPerUserExpiration ); + } + } + + /** + * Reset bad login counter after a successful login. + * @param string $username + */ + public function resetBadLoginCounter( $username ) { + if ( $this->captcha->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) && $username ) { + $cache = ObjectCache::getLocalClusterInstance(); + $cache->delete( $this->badLoginPerUserKey( $username, $cache ) ); + } + } + + /** + * Check if a bad login has already been registered for this + * IP address. If so, require a captcha. + * @return bool + */ + public function isBadLoginTriggered() { + global $wgCaptchaBadLoginAttempts; + + $cache = ObjectCache::getLocalClusterInstance(); + return $this->captcha->triggersCaptcha( CaptchaTriggers::BAD_LOGIN ) + && (int)$cache->get( $this->badLoginKey( $cache ) ) >= $wgCaptchaBadLoginAttempts; + } + + /** + * Is the per-user captcha triggered? + * + * @param User|string $u User object, or name + * @return bool + */ + public function isBadLoginPerUserTriggered( $u ) { + global $wgCaptchaBadLoginPerUserAttempts; + + $cache = ObjectCache::getLocalClusterInstance(); + + if ( is_object( $u ) ) { + $u = $u->getName(); + } + $badLoginPerUserKey = $this->badLoginPerUserKey( $u, $cache ); + return $this->captcha->triggersCaptcha( CaptchaTriggers::BAD_LOGIN_PER_USER ) + && (int)$cache->get( $badLoginPerUserKey ) >= $wgCaptchaBadLoginPerUserAttempts; + } + + /** + * Internal cache key for badlogin checks. + * @param BagOStuff $cache + * @return string + */ + private function badLoginKey( BagOStuff $cache ) { + global $wgRequest; + $ip = $wgRequest->getIP(); + + return $cache->makeGlobalKey( 'captcha', 'badlogin', 'ip', $ip ); + } + + /** + * Cache key for badloginPerUser checks. + * @param string $username + * @param BagOStuff $cache + * @return string + */ + private function badLoginPerUserKey( $username, BagOStuff $cache ) { + $userNameUtils = MediaWikiServices::getInstance()->getUserNameUtils(); + $username = $userNameUtils->getCanonical( $username, UserNameUtils::RIGOR_USABLE ) ?: $username; + + return $cache->makeGlobalKey( + 'captcha', 'badlogin', 'user', md5( $username ) + ); + } +} diff --git a/tests/phpunit/CaptchaPreAuthenticationProviderTest.php b/tests/phpunit/CaptchaPreAuthenticationProviderTest.php index ba514faf..6bb35084 100644 --- a/tests/phpunit/CaptchaPreAuthenticationProviderTest.php +++ b/tests/phpunit/CaptchaPreAuthenticationProviderTest.php @@ -4,6 +4,7 @@ use MediaWiki\Auth\AuthManager; use MediaWiki\Auth\UsernameAuthenticationRequest; use MediaWiki\Extension\ConfirmEdit\Auth\CaptchaAuthenticationRequest; use MediaWiki\Extension\ConfirmEdit\Auth\CaptchaPreAuthenticationProvider; +use MediaWiki\Extension\ConfirmEdit\Auth\LoginAttemptCounter; use MediaWiki\Extension\ConfirmEdit\Hooks; use MediaWiki\Extension\ConfirmEdit\SimpleCaptcha\SimpleCaptcha; use MediaWiki\Extension\ConfirmEdit\Store\CaptchaHashStore; @@ -125,19 +126,20 @@ class CaptchaPreAuthenticationProviderTest extends MediaWikiIntegrationTestCase return false; } ); CaptchaStore::get()->store( '345', [ 'question' => '2+2', 'answer' => '4' ] ); - $captcha = $this->getMockBuilder( SimpleCaptcha::class ) + $loginAttemptCounter = $this->getMockBuilder( LoginAttemptCounter::class ) ->onlyMethods( [ 'isBadLoginTriggered', 'isBadLoginPerUserTriggered' ] ) + ->disableOriginalConstructor() ->getMock(); - $captcha->expects( $this->any() )->method( 'isBadLoginTriggered' ) + $loginAttemptCounter->expects( $this->any() )->method( 'isBadLoginTriggered' ) ->willReturn( $isBadLoginTriggered ); - $captcha->expects( $this->any() )->method( 'isBadLoginPerUserTriggered' ) + $loginAttemptCounter->expects( $this->any() )->method( 'isBadLoginPerUserTriggered' ) ->willReturn( $isBadLoginPerUserTriggered ); - $this->setMwGlobals( 'wgCaptcha', $captcha ); - TestingAccessWrapper::newFromClass( Hooks::class )->instanceCreated = true; - $provider = new CaptchaPreAuthenticationProvider(); + $provider = $this->getProvider(); + $provider->loginAttemptCounter = $loginAttemptCounter; $this->initProvider( $provider, null, null, $this->getServiceContainer()->getAuthManager() ); $status = $provider->testForAuthentication( $req ? [ $req ] : [] ); + $this->assertEquals( $result, $status->isGood() ); } @@ -192,39 +194,43 @@ class CaptchaPreAuthenticationProviderTest extends MediaWikiIntegrationTestCase $captcha = new SimpleCaptcha(); $user = User::newFromName( 'Foo' ); $anotherUser = User::newFromName( 'Bar' ); - $provider = new CaptchaPreAuthenticationProvider(); + $provider = $this->getProvider(); + $loginAttemptCounter = new LoginAttemptCounter( $captcha ); + $provider->loginAttemptCounter = $loginAttemptCounter; $this->initProvider( $provider, null, null, $this->getServiceContainer()->getAuthManager() ); - $this->assertFalse( $captcha->isBadLoginTriggered() ); - $this->assertFalse( $captcha->isBadLoginPerUserTriggered( $user ) ); + $this->assertFalse( $loginAttemptCounter->isBadLoginTriggered() ); + $this->assertFalse( $loginAttemptCounter->isBadLoginPerUserTriggered( $user ) ); $provider->postAuthentication( $user, \MediaWiki\Auth\AuthenticationResponse::newFail( wfMessage( '?' ) ) ); - $this->assertTrue( $captcha->isBadLoginTriggered() ); - $this->assertTrue( $captcha->isBadLoginPerUserTriggered( $user ) ); - $this->assertFalse( $captcha->isBadLoginPerUserTriggered( $anotherUser ) ); + $this->assertTrue( $loginAttemptCounter->isBadLoginTriggered() ); + $this->assertTrue( $loginAttemptCounter->isBadLoginPerUserTriggered( $user ) ); + $this->assertFalse( $loginAttemptCounter->isBadLoginPerUserTriggered( $anotherUser ) ); $provider->postAuthentication( $user, \MediaWiki\Auth\AuthenticationResponse::newPass( 'Foo' ) ); - $this->assertFalse( $captcha->isBadLoginPerUserTriggered( $user ) ); + $this->assertFalse( $loginAttemptCounter->isBadLoginPerUserTriggered( $user ) ); } public function testPostAuthentication_disabled() { $this->setTriggers( [] ); $captcha = new SimpleCaptcha(); + $loginAttemptCounter = new LoginAttemptCounter( $captcha ); $user = User::newFromName( 'Foo' ); - $provider = new CaptchaPreAuthenticationProvider(); + $provider = $this->getProvider(); + $provider->loginAttemptCounter = $loginAttemptCounter; $this->initProvider( $provider, null, null, $this->getServiceContainer()->getAuthManager() ); - $this->assertFalse( $captcha->isBadLoginTriggered() ); - $this->assertFalse( $captcha->isBadLoginPerUserTriggered( $user ) ); + $this->assertFalse( $loginAttemptCounter->isBadLoginTriggered() ); + $this->assertFalse( $loginAttemptCounter->isBadLoginPerUserTriggered( $user ) ); $provider->postAuthentication( $user, \MediaWiki\Auth\AuthenticationResponse::newFail( wfMessage( '?' ) ) ); - $this->assertFalse( $captcha->isBadLoginTriggered() ); - $this->assertFalse( $captcha->isBadLoginPerUserTriggered( $user ) ); + $this->assertFalse( $loginAttemptCounter->isBadLoginTriggered() ); + $this->assertFalse( $loginAttemptCounter->isBadLoginPerUserTriggered( $user ) ); } /** @@ -293,8 +299,8 @@ class CaptchaPreAuthenticationProviderTest extends MediaWikiIntegrationTestCase } protected function blockLogin( $username ) { - $captcha = new SimpleCaptcha(); - $captcha->increaseBadLoginCounter( $username ); + $counter = new LoginAttemptCounter( new SimpleCaptcha() ); + $counter->increaseBadLoginCounter( $username ); } protected function flagSession() { @@ -311,4 +317,14 @@ class CaptchaPreAuthenticationProviderTest extends MediaWikiIntegrationTestCase $this->setMwGlobals( 'wgCaptchaTriggers', $captchaTriggers ); } + private function getProvider(): CaptchaPreAuthenticationProvider { + return new class() extends CaptchaPreAuthenticationProvider { + public ?LoginAttemptCounter $loginAttemptCounter = null; + + protected function getLoginAttemptCounter( SimpleCaptcha $captcha ): LoginAttemptCounter { + return $this->loginAttemptCounter ?: parent::getLoginAttemptCounter( $captcha ); + } + }; + } + }