From 6ac574dada0dbacd766a7228128e725f0aa857c7 Mon Sep 17 00:00:00 2001
From: Daimona Eaytoy <daimona.wiki@gmail.com>
Date: Tue, 2 Jul 2024 22:43:09 +0200
Subject: [PATCH] Add missing permission check to canSeeLogDetailsForFilter

`canSeeLogDetails` should also be checked when a filter is protected, as
it is the base right for being able to see abuselog entries. With this
in mind, check that immediately at the beginning of the method, instead
of repeating calls. Also merge the conditionals, and return early when a
permission check fails.

Move a test up so that it comes immediately after its data provider, and
add test cases for a few combinations of rights.

Change-Id: Ic3cf58f43803bef8bf2d65566434baff145b3fd5
---
 includes/AbuseFilterPermissionManager.php     | 33 +++++-------
 .../unit/AbuseFilterPermissionManagerTest.php | 53 ++++++++++++-------
 2 files changed, 49 insertions(+), 37 deletions(-)

diff --git a/includes/AbuseFilterPermissionManager.php b/includes/AbuseFilterPermissionManager.php
index 85f73e96b..826ac0fc4 100644
--- a/includes/AbuseFilterPermissionManager.php
+++ b/includes/AbuseFilterPermissionManager.php
@@ -4,6 +4,7 @@ namespace MediaWiki\Extension\AbuseFilter;
 
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\Filter\AbstractFilter;
+use MediaWiki\Extension\AbuseFilter\Filter\Flags;
 use MediaWiki\Permissions\Authority;
 
 /**
@@ -168,27 +169,21 @@ class AbuseFilterPermissionManager {
 	 * @return bool
 	 */
 	public function canSeeLogDetailsForFilter( Authority $performer, int $privacyLevel ): bool {
-		if ( $privacyLevel ) {
-			$res = true;
-			if ( FilterUtils::isHidden( $privacyLevel ) ) {
-				if (
-					!$this->canSeeLogDetails( $performer ) ||
-					!$this->canViewPrivateFiltersLogs( $performer )
-				) {
-					$res = false;
-				}
-			}
-
-			if ( FilterUtils::isProtected( $privacyLevel ) ) {
-				if ( !$this->canViewProtectedVariables( $performer ) ) {
-					$res = false;
-				}
-			}
-
-			return $res;
+		if ( !$this->canSeeLogDetails( $performer ) ) {
+			return false;
 		}
 
-		return $this->canSeeLogDetails( $performer );
+		if ( $privacyLevel === Flags::FILTER_PUBLIC ) {
+			return true;
+		}
+		if ( FilterUtils::isHidden( $privacyLevel ) && !$this->canViewPrivateFiltersLogs( $performer ) ) {
+			return false;
+		}
+		if ( FilterUtils::isProtected( $privacyLevel ) && !$this->canViewProtectedVariables( $performer ) ) {
+			return false;
+		}
+
+		return true;
 	}
 
 	/**
diff --git a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php
index 302dccf28..93059de60 100644
--- a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php
+++ b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php
@@ -174,22 +174,53 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase {
 		$details = [ 0 => 'abusefilter-log-detail' ];
 		$private = [ 1 => 'abusefilter-log-private' ];
 		$protected = [ 2 => 'abusefilter-access-protected-vars' ];
+
 		yield 'filter hidden, not privileged' => [ Flags::FILTER_HIDDEN, [], false ];
 		yield 'filter hidden, details only' => [ Flags::FILTER_HIDDEN, $details, false ];
 		yield 'filter hidden, private logs only' => [ Flags::FILTER_HIDDEN, $private, false ];
 		yield 'filter hidden, details and private logs' => [ Flags::FILTER_HIDDEN, $details + $private, true ];
+
 		yield 'filter protected, not privileged' => [ Flags::FILTER_USES_PROTECTED_VARS, [], false ];
-		yield 'filter protected, privileged' => [ Flags::FILTER_USES_PROTECTED_VARS, $protected, true ];
+		yield 'filter protected, details only' => [ Flags::FILTER_USES_PROTECTED_VARS, $details, false ];
+		yield 'filter protected, protected logs only' => [ Flags::FILTER_USES_PROTECTED_VARS, $protected, false ];
+		yield 'filter protected, privileged' => [ Flags::FILTER_USES_PROTECTED_VARS, $details + $protected, true ];
+
+		$hiddenProtected = Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS;
+		yield 'filter hidden and protected, not privileged' => [ $hiddenProtected, [], false ];
+		yield 'filter hidden and protected, details only' => [ $hiddenProtected, $details, false ];
+		yield 'filter hidden and protected, private only' => [ $hiddenProtected, $private, false ];
+		yield 'filter hidden and protected, protected only' => [ $hiddenProtected, $protected, false ];
 		yield 'filter hidden and protected, details and private only' => [
-			Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS, $details + $private, false
+			$hiddenProtected, $details + $private, false
 		];
-		yield 'filter hidden and protected, protected only' => [
-			Flags::FILTER_HIDDEN | Flags::FILTER_USES_PROTECTED_VARS, $protected, false
+		yield 'filter hidden and protected, details and protected only' => [
+			$hiddenProtected, $details + $protected, false
 		];
+		yield 'filter hidden and protected, private and protected only' => [
+			$hiddenProtected, $private + $protected, false
+		];
+		yield 'filter hidden and protected, privileged' => [
+			$hiddenProtected, $details + $private + $protected, true
+		];
+
 		yield 'filter visible, not privileged' => [ Flags::FILTER_PUBLIC, [], false ];
 		yield 'filter visible, privileged' => [ Flags::FILTER_PUBLIC, $details, true ];
 	}
 
+	/**
+	 * @param int $privacyLevel
+	 * @param array $rights
+	 * @param bool $expected
+	 * @dataProvider provideCanSeeLogDetailsForFilter
+	 */
+	public function testCanSeeLogDetailsForFilter( int $privacyLevel, array $rights, bool $expected ) {
+		$performer = $this->mockRegisteredAuthorityWithPermissions( $rights );
+		$this->assertSame(
+			$expected,
+			$this->getPermMan()->canSeeLogDetailsForFilter( $performer, $privacyLevel )
+		);
+	}
+
 	public function provideCanViewProtectedVariables(): Generator {
 		$block = $this->createMock( DatabaseBlock::class );
 		$block->method( 'isSiteWide' )->willReturn( true );
@@ -261,20 +292,6 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase {
 		);
 	}
 
-	/**
-	 * @param int $privacyLevel
-	 * @param array $rights
-	 * @param bool $expected
-	 * @dataProvider provideCanSeeLogDetailsForFilter
-	 */
-	public function testCanSeeLogDetailsForFilter( int $privacyLevel, array $rights, bool $expected ) {
-		$performer = $this->mockRegisteredAuthorityWithPermissions( $rights );
-		$this->assertSame(
-			$expected,
-			$this->getPermMan()->canSeeLogDetailsForFilter( $performer, $privacyLevel )
-		);
-	}
-
 	public static function provideSimpleCases(): array {
 		return [
 			'not allowed' => [ false ],