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

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

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 );
+	}
 }

includes/Auth/LoginAttemptCounter.php

@@ -0,0 +1,116 @@
+<?php
+
+namespace MediaWiki\Extension\ConfirmEdit\Auth;
+
+use BagOStuff;
+use MediaWiki\Extension\ConfirmEdit\CaptchaTriggers;
+use MediaWiki\Extension\ConfirmEdit\SimpleCaptcha\SimpleCaptcha;
+use MediaWiki\MediaWikiServices;
+use MediaWiki\User\UserNameUtils;
+use ObjectCache;
+use User;
+
+/**
+ * Helper to count login attempts per IP and per username.
+ *
+ * @internal
+ */
+class LoginAttemptCounter {
+	private SimpleCaptcha $captcha;
+
+	public function __construct( SimpleCaptcha $captcha ) {
+		$this->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 )
+		);
+	}
+}

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 );
+			}
+		};
+	}
+
 }