SimpleCaptcha: Allow invoking CAPTCHA display from other extensions

Why:

- In the production WMF deployment of AbuseFilter and ConfirmEdit, we
  load ConfirmEdit first, then AbuseFilter. That means that
  ConfirmEdit's onEditFilterMergedContent hook fires before
  AbuseFilter's. The problem is that AbuseFilter uses
  onEditFilterMergedContent to evaluate its rules and consequences, so
  an AbuseFilter rule that defines a "showcaptcha" consequence becomes a
  no-op, as it fires after ConfirmEdit has already decided to show or
  not show a CAPTCHA to a user.
 - All of that is to say: we need a way to tell ConfirmEdit to show a
   CAPTCHA at the time that AbuseFilter's consequences are invoked,
   which could be before or after ConfirmEdit's EditFilterMergedContent
   hook invocation, depending on how the wiki has decided to load the
   extensions

What:

- Define a flag for "shouldForceShowCaptcha", that other extensions can
  set on the SimpleCaptcha base class to indicate that ConfirmEdit must
  show a CAPTCHA (users with "skipcaptcha" right are still exempt)
- Check the isCaptchaSolved() and shouldForShowCaptcha() flags in
  ::triggersCaptcha, and also check if ConfirmEdit's
  EditFilterMergedContent hook already ran
- In CaptchaConsequence, set the forceShowCaptcha property on the
  SimpleCaptcha base class
- [misc] Add getter/setter for the captchaSolved property and the other
  new class properties

Depends-On: I7dd3a7c41606dcf5123518c2d3d0f4355f5edfd3
Bug: T20110
Change-Id: Idc47bdae8007da938f31e1c0f33e9be4813f41d7

SimpleCaptcha/SimpleCaptcha.php

@@ -47,8 +47,14 @@ use WikiPage;
 class SimpleCaptcha {
 	protected static $messagePrefix = 'captcha-';
 
+	/** @var bool Override to force showing the CAPTCHA to users who don't have "skipcaptcha" right. */
+	private bool $forceShowCaptcha = false;
+
 	/** @var bool|null Was the CAPTCHA already passed and if yes, with which result? */
-	private $captchaSolved = null;
+	private ?bool $captchaSolved = null;
+
+	/** @var bool Flag to indicate whether the onEditFilterMergedContent hook was invoked. */
+	private bool $editFilterMergedContentHandlerCalled = false;
 
 	/** @var bool[] Activate captchas status list for a pages by key */
 	private $activatedCaptchas = [];
@@ -471,6 +477,11 @@ class SimpleCaptcha {
 	 * @return bool True, if the action should trigger a CAPTCHA, false otherwise
 	 */
 	public function triggersCaptcha( $action, $title = null ) {
+		// Captcha was already solved, we don't need to check anything else.
+		if ( $this->isCaptchaSolved() ) {
+			return false;
+		}
+
 		global $wgCaptchaTriggers, $wgCaptchaTriggersOnNamespace;
 
 		$result = false;
@@ -492,6 +503,12 @@ class SimpleCaptcha {
 			$result = $wgCaptchaTriggersOnNamespace[$title->getNamespace()][$action];
 		}
 
+		// SimpleCaptcha has been instructed to force showing the CAPTCHA, no need to
+		// check what other hook implementations think.
+		if ( $this->shouldForceShowCaptcha() ) {
+			return true;
+		}
+
 		$hookRunner = new HookRunner(
 			MediaWikiServices::getInstance()->getHookContainer()
 		);
@@ -647,6 +664,49 @@ class SimpleCaptcha {
 		return false;
 	}
 
+	public function isCaptchaSolved(): ?bool {
+		return $this->captchaSolved;
+	}
+
+	protected function setCaptchaSolved( ?bool $captchaSolved ): void {
+		$this->captchaSolved = $captchaSolved;
+	}
+
+	/**
+	 * @return bool True if an override is set to force showing a CAPTCHA
+	 *  to the user. Note that users with "skipcaptcha" right may still
+	 *  bypass this override.
+	 */
+	public function shouldForceShowCaptcha(): bool {
+		return $this->forceShowCaptcha;
+	}
+
+	/**
+	 * @param bool $forceShowCaptcha True if the caller wants to force showing
+	 *  a CAPTCHA to the user. Note that users with "skipcaptcha" right may
+	 *  still bypass this override.
+	 * @return void
+	 */
+	public function setForceShowCaptcha( bool $forceShowCaptcha ): void {
+		$this->forceShowCaptcha = $forceShowCaptcha;
+	}
+
+	/**
+	 * @return bool Was the EditFilterMergedContent hook implementation already
+	 * invoked?
+	 */
+	public function editFilterMergedContentHandlerAlreadyInvoked(): bool {
+		return $this->editFilterMergedContentHandlerCalled;
+	}
+
+	/**
+	 * @return void Set a flag on the class stating that EditFilterMergedContent handler
+	 * was already run.
+	 */
+	public function setEditFilterMergedContentHandlerInvoked(): void {
+		$this->editFilterMergedContentHandlerCalled = true;
+	}
+
 	/**
 	 * Filter callback function for URL whitelisting
 	 * @param string $url string to check
@@ -985,8 +1045,8 @@ class SimpleCaptcha {
 	protected function passCaptcha( $index, $word ) {
 		// Don't check the same CAPTCHA twice in one session,
 		// if the CAPTCHA was already checked - Bug T94276
-		if ( $this->captchaSolved !== null ) {
-			return $this->captchaSolved;
+		if ( $this->isCaptchaSolved() !== null ) {
+			return (bool)$this->isCaptchaSolved();
 		}
 
 		$info = $this->retrieveCaptcha( $index );
@@ -994,12 +1054,12 @@ class SimpleCaptcha {
 			if ( $this->keyMatch( $word, $info ) ) {
 				$this->log( "passed" );
 				$this->clearCaptcha( $index );
-				$this->captchaSolved = true;
+				$this->setCaptchaSolved( true );
 				return true;
 			} else {
 				$this->clearCaptcha( $index );
 				$this->log( "bad form input" );
-				$this->captchaSolved = false;
+				$this->setCaptchaSolved( false );
 				return false;
 			}
 		} else {

includes/AbuseFilter/CaptchaConsequence.php

@@ -2,21 +2,18 @@
 
 namespace MediaWiki\Extension\ConfirmEdit\AbuseFilter;
 
-use MediaWiki\Context\RequestContext;
 use MediaWiki\Extension\AbuseFilter\Consequences\Consequence\Consequence;
+use MediaWiki\Extension\ConfirmEdit\Hooks;
 
 /**
- * Will inform ConfirmEdit extension to show a CAPTCHA.
+ * Show a CAPTCHA to the user before they can proceed with an action.
  */
 class CaptchaConsequence extends Consequence {
 
-	public const FLAG = 'wgAbuseFilterCaptchaConsequence';
-
 	public function execute(): bool {
-		// This consequence was triggered, so we need to set a global flag
-		// which Extension:ConfirmEdit will read in order to decide if a
-		// CAPTCHA should be shown to the user in onConfirmEditTriggersCaptcha
-		RequestContext::getMain()->getRequest()->setVal( self::FLAG, true );
+		// This consequence was triggered, so we need to set a flag
+		// on the SimpleCaptcha instance to force showing the CAPTCHA.
+		Hooks::getInstance()->setForceShowCaptcha( true );
 		return true;
 	}
 }

includes/Hooks.php

@@ -92,7 +92,12 @@ class Hooks implements
 	public function onEditFilterMergedContent( IContextSource $context, Content $content, Status $status,
 		$summary, User $user, $minorEdit
 	) {
-		return self::getInstance()->confirmEditMerged( $context, $content, $status, $summary,
+		$simpleCaptcha = self::getInstance();
+		// Set a flag indicating that ConfirmEdit's implementation of
+		// EditFilterMergedContent ran. This can be checked by other extensions
+		// e.g. AbuseFilter.
+		$simpleCaptcha->setEditFilterMergedContentHandlerInvoked();
+		return $simpleCaptcha->confirmEditMerged( $context, $content, $status, $summary,
 			$user, $minorEdit );
 	}
 

tests/phpunit/SimpleCaptcha/CaptchaTest.php

@@ -8,6 +8,7 @@ use MediaWiki\Request\WebRequest;
 use MediaWiki\Title\Title;
 use MediaWiki\User\User;
 use Wikimedia\ScopedCallback;
+use Wikimedia\TestingAccessWrapper;
 
 /**
  * @covers \MediaWiki\Extension\ConfirmEdit\SimpleCaptcha\SimpleCaptcha
@@ -186,4 +187,33 @@ class CaptchaTest extends MediaWikiIntegrationTestCase {
 		]
 		);
 	}
+
+	public function testTriggersCaptchaReturnsEarlyIfCaptchaSolved() {
+		$this->setMwGlobals( [
+			'wgCaptchaTriggers' => [
+				'edit' => true,
+			]
+		] );
+		$testObject = new SimpleCaptcha();
+		/** @var SimpleCaptcha|TestingAccessWrapper $wrapper */
+		$wrapper = TestingAccessWrapper::newFromObject( $testObject );
+		$wrapper->captchaSolved = true;
+		$this->assertFalse( $wrapper->triggersCaptcha( 'edit' ), 'CAPTCHA is not triggered if already solved' );
+	}
+
+	public function testForceShowCaptcha() {
+		$this->setMwGlobals( [
+			'wgCaptchaTriggers' => [
+				'edit' => false,
+			]
+		] );
+		$testObject = new SimpleCaptcha();
+		/** @var SimpleCaptcha|TestingAccessWrapper $wrapper */
+		$wrapper = TestingAccessWrapper::newFromObject( $testObject );
+		$this->assertFalse(
+			$wrapper->triggersCaptcha( 'edit' ), 'CAPTCHA is not triggered by edit action in this configuration'
+		);
+		$wrapper->setForceShowCaptcha( true );
+		$this->assertTrue( $wrapper->triggersCaptcha( 'edit' ), 'Force showing a CAPTCHA if flag is set' );
+	}
 }

tests/phpunit/integration/AbuseFilter/CaptchaConsequenceTest.php

@@ -2,9 +2,9 @@
 
 namespace MediaWiki\Extension\ConfirmEdit\Test\Integration\AbuseFilter;
 
-use MediaWiki\Context\RequestContext;
 use MediaWiki\Extension\AbuseFilter\Consequences\Parameters;
 use MediaWiki\Extension\ConfirmEdit\AbuseFilter\CaptchaConsequence;
+use MediaWiki\Extension\ConfirmEdit\Hooks;
 use MediaWikiIntegrationTestCase;
 
 /**
@@ -16,16 +16,10 @@ class CaptchaConsequenceTest extends MediaWikiIntegrationTestCase {
 		$parameters = $this->createMock( Parameters::class );
 		$parameters->method( 'getAction' )->willReturn( 'edit' );
 		$captchaConsequence = new CaptchaConsequence( $parameters );
-		$request = RequestContext::getMain();
-		$this->assertNull( $request->getRequest()->getVal(
-			CaptchaConsequence::FLAG
-		) );
+		$simpleCaptcha = Hooks::getInstance();
+		$this->assertFalse( $simpleCaptcha->shouldForceShowCaptcha() );
 		$captchaConsequence->execute();
-		$this->assertTrue(
-			$request->getRequest()->getBool(
-				CaptchaConsequence::FLAG
-			)
-		);
+		$this->assertTrue( $simpleCaptcha->shouldForceShowCaptcha() );
 	}
 
 }