From 1902efbcc76631f614ef2803c8aefb6ed73feb54 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mat=C4=9Bj=20Such=C3=A1nek?= <matejsuchanek97@gmail.com>
Date: Sun, 1 Aug 2021 11:05:36 +0200
Subject: [PATCH] BlockUser: Restore blocking autoblocked IP addresses

This seems to be a regression from 166ed5c.
DatabaseBlock::newFromTarget may return an autoblock for an IP
unless there is an existing manual block. If so, ignore it
because it is allowed to have an IP address both manually
blocked and autoblocked (see ipb_address_unique index).

Also add an integration test case.

Bug: T287798
Change-Id: I7c9a66ba0ffe759f43f4d0821c30fb94649d3dea
---
 includes/block/BlockUser.php                  | 12 +++++
 .../includes/block/BlockUserTest.php          | 49 +++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/includes/block/BlockUser.php b/includes/block/BlockUser.php
index 93d0c8ab382..b0519d25b98 100644
--- a/includes/block/BlockUser.php
+++ b/includes/block/BlockUser.php
@@ -555,6 +555,18 @@ class BlockUser {
 		// xxx: there is an identical call at the beginning of ::placeBlock
 		$priorBlock = DatabaseBlock::newFromTarget( $this->target, null, /*fromPrimary=*/true );
 
+		// T287798: we are blocking an IP that is currently autoblocked
+		// we can ignore the block because ipb_address_unique allows the IP address
+		// be both manually blocked and autoblocked
+		// this will work as long as DatabaseBlock::newLoad prefers manual IP blocks
+		// over autoblocks
+		if ( $priorBlock !== null
+			&& $priorBlock->getType() === AbstractBlock::TYPE_AUTO
+			&& $this->targetType === AbstractBlock::TYPE_IP
+		) {
+			$priorBlock = null;
+		}
+
 		$isReblock = false;
 		if ( $priorBlock !== null ) {
 			// Reblock only if the caller wants so
diff --git a/tests/phpunit/integration/includes/block/BlockUserTest.php b/tests/phpunit/integration/includes/block/BlockUserTest.php
index eb38e499a77..ab9da68d324 100644
--- a/tests/phpunit/integration/includes/block/BlockUserTest.php
+++ b/tests/phpunit/integration/includes/block/BlockUserTest.php
@@ -211,4 +211,53 @@ class BlockUserTest extends MediaWikiIntegrationTestCase {
 		$this->assertSame( $priorBlock->getId(), $hookPriorBlock->getId() );
 	}
 
+	/**
+	 * @covers MediaWiki\Block\BlockUser::placeBlockInternal
+	 */
+	public function testIPBlockAllowedAutoblockPreserved() {
+		$blockStatus = $this->blockUserFactory->newBlockUser(
+			$this->user,
+			$this->mockAnonUltimateAuthority(),
+			'infinity',
+			'test block with autoblocking',
+			[ 'isAutoblocking' => true ]
+		)->placeBlockUnsafe();
+		$this->assertTrue( $blockStatus->isOK() );
+		$block = $blockStatus->getValue();
+
+		$target = '1.2.3.4';
+		$autoBlockId = $block->doAutoblock( $target );
+		$this->assertNotFalse( $autoBlockId );
+
+		$hookPriorBlock = false;
+		$this->setTemporaryHook(
+			'BlockIpComplete',
+			static function ( $block, $legacyUser, $priorBlock )
+			use ( &$hookPriorBlock )
+			{
+				$hookPriorBlock = $priorBlock;
+			}
+		);
+
+		$IPBlockStatus = $this->blockUserFactory->newBlockUser(
+			$target,
+			$this->mockAnonUltimateAuthority(),
+			'infinity',
+			'test IP block'
+		)->placeBlockUnsafe();
+		$this->assertTrue( $IPBlockStatus->isOK() );
+		$IPBlock = $IPBlockStatus->getValue();
+		$this->assertInstanceOf( DatabaseBlock::class, $IPBlock );
+		$this->assertNull( $hookPriorBlock );
+
+		$blockIds = array_map(
+			static function ( DatabaseBlock $block ) {
+				return $block->getId();
+			},
+			DatabaseBlock::newListFromTarget( $target, null, /*fromPrimary=*/true )
+		);
+		$this->assertContains( $autoBlockId, $blockIds );
+		$this->assertContains( $IPBlock->getId(), $blockIds );
+	}
+
 }