From b93543ef0002a88507285d3c79a9775ed756ff91 Mon Sep 17 00:00:00 2001 From: Kosta Harlan Date: Wed, 15 May 2024 10:23:29 +0200 Subject: [PATCH] ConfirmEditHandler: Use SimpleCaptcha API to invoke CAPTCHA display Why: - The previous attempt to integrate AbuseFilter with ConfirmEdit set a flag on the request object (I110a5f5321649dcf85993a0c209ab70b9886057c) didn't work in WMF production because in WMF, we load ConfirmEdit first, followed by AbuseFilter. Therefore any flag set in an AbuseFilter hook is ignored by ConfirmEdit What: - Remove implementation of ConfirmEditTriggersCaptchaHook, as this does not work when AbuseFilter is loaded after ConfirmEdit. - Repurpose onConfirmEditTriggersCaptcha to handle non-edit actions only - Implement the EditFilterMergedContent hook and call SimpleCaptcha's public confirmEditMerged method if CaptchaConsequence has specified that a CAPTCHA should be displayed, and if the CAPTCHA has not already been solved Soft-Depends-On: Idc47bdae8007da938f31e1c0f33e9be4813f41d7 Bug: T20110 Change-Id: I7dd3a7c41606dcf5123518c2d3d0f4355f5edfd3 --- extension.json | 3 +- .../Hooks/Handlers/ConfirmEditHandler.php | 64 +++++++++++++++---- .../Hooks/ConfirmEditHandlerTest.php | 54 ++++++++++++---- 3 files changed, 94 insertions(+), 27 deletions(-) diff --git a/extension.json b/extension.json index 4b3c6797f..15fe985fe 100644 --- a/extension.json +++ b/extension.json @@ -400,7 +400,7 @@ } }, "Hooks": { - "EditFilterMergedContent": "FilteredActions", + "EditFilterMergedContent": [ "FilteredActions", "ConfirmEdit" ], "GetAutoPromoteGroups": "AutoPromoteGroups", "TitleMove": "FilteredActions", "ArticleDelete": "FilteredActions", @@ -418,7 +418,6 @@ "CheckUserInsertChangesRow": "CheckUser", "CheckUserInsertPrivateEventRow": "CheckUser", "CheckUserInsertLogEventRow": "CheckUser", - "ConfirmEditTriggersCaptcha": "ConfirmEdit", "UserMergeAccountFields": "UserMerge", "BeforeCreateEchoEvent": "Echo", "ParserOutputStashForEdit": "FilteredActions", diff --git a/includes/Hooks/Handlers/ConfirmEditHandler.php b/includes/Hooks/Handlers/ConfirmEditHandler.php index 2facce09b..beb065794 100644 --- a/includes/Hooks/Handlers/ConfirmEditHandler.php +++ b/includes/Hooks/Handlers/ConfirmEditHandler.php @@ -2,24 +2,64 @@ namespace MediaWiki\Extension\AbuseFilter\Hooks\Handlers; -use MediaWiki\Context\RequestContext; -use MediaWiki\Extension\ConfirmEdit\AbuseFilter\CaptchaConsequence; -use MediaWiki\Extension\ConfirmEdit\Hooks\ConfirmEditTriggersCaptchaHook; -use MediaWiki\Page\PageIdentity; +use Content; +use ExtensionRegistry; +use MediaWiki\Context\IContextSource; +use MediaWiki\Extension\ConfirmEdit\Hooks; +use MediaWiki\Hook\EditFilterMergedContentHook; +use MediaWiki\Status\Status; +use MediaWiki\User\User; /** * Integration with Extension:ConfirmEdit, if loaded. */ -class ConfirmEditHandler implements ConfirmEditTriggersCaptchaHook { +class ConfirmEditHandler implements EditFilterMergedContentHook { /** @inheritDoc */ - public function onConfirmEditTriggersCaptcha( string $action, ?PageIdentity $page, bool &$result ) { - // Check the request to see if ConfirmEdit's CaptchaConsequence was triggered, as - // that sets a property on the request. If so, we know that we want to trigger the CAPTCHA. - // Note that users with 'skipcaptcha' right will not be shown the CAPTCHA even if this - // return true. - if ( RequestContext::getMain()->getRequest()->getBool( CaptchaConsequence::FLAG ) ) { - $result = true; + public function onEditFilterMergedContent( + IContextSource $context, Content $content, Status $status, $summary, User $user, $minoredit + ) { + if ( !ExtensionRegistry::getInstance()->isLoaded( 'ConfirmEdit' ) ) { + return true; } + $simpleCaptcha = Hooks::getInstance(); + // FIXME: Remove method_exists checks after I3484d66298bc9f49dfbe003a0605e2ac1a092e10 is merged + if ( !method_exists( $simpleCaptcha, 'shouldForceShowCaptcha' ) || + !method_exists( $simpleCaptcha, 'isCaptchaSolved' ) || + !method_exists( $simpleCaptcha, 'editFilterMergedContentHandlerAlreadyInvoked' ) ) { + return true; + } + // In WMF production, AbuseFilter is loaded after ConfirmEdit. That means, + // Extension:ConfirmEdit's EditFilterMergedContent hook has already run, and that hook + // is responsible for deciding whether to show a CAPTCHA via the SimpleCaptcha::confirmEditMerged + // method. + // Here, we look to see if: + // 1. CaptchaConsequence in AbuseFilter modified the global SimpleCaptcha instance to say that + // we should force showing a Captcha + // 2. that the Captcha hasn't yet been solved + // 3. ConfirmEdit's EditFilterMergedContent handler has already run (ConfirmEdit was loaded + // ahead of AbuseFilter via wfLoadExtension()) + // If all conditions are true, we invoke SimpleCaptcha's ConfirmEditMerged method, which + // will run in a narrower scope (not invoking ConfirmEdit's onConfirmEditTriggersCaptcha hook, + // for example), and will just make sure that the status is modified to present a CAPTCHA to + // the user. + // FIXME: Remove phan suppression after I3484d66298bc9f49dfbe003a0605e2ac1a092e10 is merged + // @phan-suppress-next-line PhanUndeclaredMethod + if ( $simpleCaptcha->shouldForceShowCaptcha() && + // @phan-suppress-next-line PhanUndeclaredMethod + !$simpleCaptcha->isCaptchaSolved() && + // @phan-suppress-next-line PhanUndeclaredMethod + $simpleCaptcha->editFilterMergedContentHandlerAlreadyInvoked() ) { + return $simpleCaptcha->confirmEditMerged( + $context, + $content, + $status, + $summary, + $user, + $minoredit + ); + } + return true; } + } diff --git a/tests/phpunit/integration/Hooks/ConfirmEditHandlerTest.php b/tests/phpunit/integration/Hooks/ConfirmEditHandlerTest.php index 5f90b2ce2..859dacfe2 100644 --- a/tests/phpunit/integration/Hooks/ConfirmEditHandlerTest.php +++ b/tests/phpunit/integration/Hooks/ConfirmEditHandlerTest.php @@ -2,35 +2,63 @@ namespace MediaWiki\Extension\AbuseFilter\Tests\Integration\Hooks; +use MediaWiki\Context\RequestContext; use MediaWiki\Extension\AbuseFilter\Consequences\Parameters; use MediaWiki\Extension\AbuseFilter\Hooks\Handlers\ConfirmEditHandler; use MediaWiki\Extension\ConfirmEdit\AbuseFilter\CaptchaConsequence; -use MediaWiki\Page\PageIdentity; +use MediaWiki\Extension\ConfirmEdit\Hooks; +use MediaWiki\Status\Status; +use MediaWiki\Title\Title; +use MediaWiki\User\User; use MediaWikiIntegrationTestCase; /** * @covers \MediaWiki\Extension\AbuseFilter\Hooks\Handlers\ConfirmEditHandler + * @group Database */ class ConfirmEditHandlerTest extends MediaWikiIntegrationTestCase { - public function testConfirmEditTriggersCaptcha() { + public function testOnEditFilterMergedContent() { $this->markTestSkippedIfExtensionNotLoaded( 'ConfirmEdit' ); $confirmEditHandler = new ConfirmEditHandler(); - $result = false; - $confirmEditHandler->onConfirmEditTriggersCaptcha( - 'edit', - $this->createMock( PageIdentity::class ), - $result + $status = Status::newGood(); + $title = $this->createMock( Title::class ); + $title->method( 'canExist' )->willReturn( true ); + $context = RequestContext::getMain(); + $context->setTitle( $title ); + $confirmEditHandler->onEditFilterMergedContent( + $context, + $this->createMock( \Content::class ), + $status, + '', + $this->createMock( User::class ), + false ); - $this->assertFalse( $result, 'The default is to not show a CAPTCHA' ); + $this->assertStatusGood( $status, 'The default is to not show a CAPTCHA' ); + $simpleCaptcha = Hooks::getInstance(); + // FIXME: Remove this method_exists check after Idc47bdae8007da938f31e1c0f33e9be4813f41d7 is merged + if ( method_exists( $simpleCaptcha, 'setEditFilterMergedContentHandlerInvoked' ) ) { + $simpleCaptcha->setEditFilterMergedContentHandlerInvoked(); + } + $simpleCaptcha->setAction( 'edit' ); $captchaConsequence = new CaptchaConsequence( $this->createMock( Parameters::class ) ); $captchaConsequence->execute(); - $confirmEditHandler->onConfirmEditTriggersCaptcha( - 'edit', - $this->createMock( PageIdentity::class ), - $result + $confirmEditHandler->onEditFilterMergedContent( + $context, + $this->createMock( \Content::class ), + $status, + '', + $this->createMock( User::class ), + false ); - $this->assertTrue( $result, 'CaptchaConsequence specifies that a CAPTCHA should be shown' ); + // FIXME: Remove this method_exists check after Idc47bdae8007da938f31e1c0f33e9be4813f41d7 is merged + if ( method_exists( $simpleCaptcha, 'shouldForceShowCaptcha' ) ) { + $this->assertStatusError( + 'captcha-edit-fail', + $status, + 'CaptchaConsequence specifies that a CAPTCHA should be shown' + ); + } } }