From 35d4d9ab494efcafaa56a606dbf0eccaf9549bb2 Mon Sep 17 00:00:00 2001
From: Jakob Warkotsch <jakob.warkotsch@wikimedia.de>
Date: Mon, 23 Sep 2024 10:36:08 +0200
Subject: [PATCH] REST: Fix AbuseFilter error handling

This fixes a few issues pointed out in
I24b632b9b0172037d827167ccb1914769d947051:
 * detect AbuseFilter errors more accurately
 * rely on IApiMessage::getApiData() which is more stable than message
   params
 * fix filter ID type since they're not always ints
 * use more realistic test input

Bug: T374959
Change-Id: I3252ca5ca91e2d5e03a75a330e6f47c0a09bfd1c
---
 .../Exceptions/AbuseFilterException.php       |  6 +++---
 .../DataAccess/EntityUpdater.php              | 20 ++++++++++++++++---
 .../mocha/api-testing/AbuseFilterTest.js      |  4 ++--
 .../DataAccess/EntityUpdaterTest.php          | 16 +++++++++++++--
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/repo/rest-api/src/Domain/Services/Exceptions/AbuseFilterException.php b/repo/rest-api/src/Domain/Services/Exceptions/AbuseFilterException.php
index 7423a827fb..bb13788235 100644
--- a/repo/rest-api/src/Domain/Services/Exceptions/AbuseFilterException.php
+++ b/repo/rest-api/src/Domain/Services/Exceptions/AbuseFilterException.php
@@ -10,11 +10,11 @@
  */
 class AbuseFilterException extends Exception {
 
-	private int $filterId;
+	private string $filterId;
 	private string $filterDescription;
 
 	public function __construct(
-		int $filterId,
+		string $filterId,
 		string $filterDescription,
 		string $message = '',
 		int $code = 0,
@@ -25,7 +25,7 @@ public function __construct(
 		$this->filterDescription = $filterDescription;
 	}
 
-	public function getFilterId(): int {
+	public function getFilterId(): string {
 		return $this->filterId;
 	}
 
diff --git a/repo/rest-api/src/Infrastructure/DataAccess/EntityUpdater.php b/repo/rest-api/src/Infrastructure/DataAccess/EntityUpdater.php
index 1bca12c461..d575cab003 100644
--- a/repo/rest-api/src/Infrastructure/DataAccess/EntityUpdater.php
+++ b/repo/rest-api/src/Infrastructure/DataAccess/EntityUpdater.php
@@ -2,6 +2,7 @@
 
 namespace Wikibase\Repo\RestApi\Infrastructure\DataAccess;
 
+use IApiMessage;
 use LogicException;
 use MediaWiki\Context\IContextSource;
 use MediaWiki\Permissions\PermissionManager;
@@ -107,10 +108,12 @@ private function createOrUpdate(
 				throw new ResourceTooLargeException( $maxSizeInKiloBytes );
 			}
 
-			$abuseFilterError = $this->findErrorInStatus( $status, 'abusefilter' );
+			$abuseFilterError = $this->findAbuseFilterError( $status->getMessages() );
 			if ( $abuseFilterError ) {
-				[ $filterDescription, $filterId ] = $abuseFilterError->getParams();
-				throw new AbuseFilterException( (int)$filterId, $filterDescription );
+				throw new AbuseFilterException(
+					$abuseFilterError->getApiData()['abusefilter']['id'],
+					$abuseFilterError->getApiData()['abusefilter']['description']
+				);
 			}
 
 			if ( $this->isPreventedEdit( $status ) ) {
@@ -141,6 +144,17 @@ private function findErrorInStatus( Status $status, string $errorCode ): ?Messag
 		return null;
 	}
 
+	private function findAbuseFilterError( array $messages ): ?IApiMessage {
+		foreach ( $messages as $message ) {
+			if ( $message instanceof IApiMessage &&
+				in_array( $message->getApiCode(), [ 'abusefilter-warning', 'abusefilter-disallowed' ] ) ) {
+				return $message;
+			}
+		}
+
+		return null;
+	}
+
 	private function checkBotRightIfProvided( User $user, bool $isBot ): void {
 		// This is only a low-level safeguard and should be checked and handled properly before using this service.
 		if ( $isBot && !$this->permissionManager->userHasRight( $user, 'bot' ) ) {
diff --git a/repo/rest-api/tests/mocha/api-testing/AbuseFilterTest.js b/repo/rest-api/tests/mocha/api-testing/AbuseFilterTest.js
index e895f4a322..03573aa945 100644
--- a/repo/rest-api/tests/mocha/api-testing/AbuseFilterTest.js
+++ b/repo/rest-api/tests/mocha/api-testing/AbuseFilterTest.js
@@ -20,7 +20,7 @@ const config = require( 'api-testing/lib/config' )();
  *
  * @param {string} description
  * @param {string} rules
- * @return {Promise<number>} the filter ID
+ * @return {Promise<string>} the filter ID
  */
 async function createAbuseFilter( description, rules ) {
 	const rootClient = await action.root();
@@ -49,7 +49,7 @@ async function createAbuseFilter( description, rules ) {
 		wpFilterTags: ''
 	} );
 
-	return parseInt( new URL( createFilterResponse.headers.location ).searchParams.get( 'changedfilter' ) );
+	return new URL( createFilterResponse.headers.location ).searchParams.get( 'changedfilter' );
 }
 
 describe( 'Abuse Filter', () => {
diff --git a/repo/rest-api/tests/phpunit/Infrastructure/DataAccess/EntityUpdaterTest.php b/repo/rest-api/tests/phpunit/Infrastructure/DataAccess/EntityUpdaterTest.php
index d978cff5b5..416568d19e 100644
--- a/repo/rest-api/tests/phpunit/Infrastructure/DataAccess/EntityUpdaterTest.php
+++ b/repo/rest-api/tests/phpunit/Infrastructure/DataAccess/EntityUpdaterTest.php
@@ -230,10 +230,22 @@ public function testGivenResourceTooLarge_throwsCorrespondingException( EntityDo
 	 * @dataProvider provideEntity
 	 */
 	public function testGivenAbuseFilterMatch_throwsCorrespondingException( EntityDocument $entity ): void {
-		$filterId = 777;
+		$filterId = '777';
 		$filterDescription = 'bad word rejecting filter';
 
-		$errorStatus = EditEntityStatus::newFatal( 'abusefilter-disallowed', $filterDescription, $filterId );
+		$errorStatus = EditEntityStatus::newFatal(
+			\ApiMessage::create(
+				[ 'abusefilter-disallowed', $filterDescription, $filterId ],
+				'abusefilter-disallowed',
+				[
+					'abusefilter' => [
+						'id' => $filterId,
+						'description' => $filterDescription,
+						'actions' => 'disallow',
+					],
+				]
+			)
+		);
 
 		$editEntity = $this->createStub( EditEntity::class );
 		$editEntity->method( 'attemptSave' )->willReturn( $errorStatus );