From 51381f006759a80b616834704b3ddc757d0d7417 Mon Sep 17 00:00:00 2001
From: STran <stran@wikimedia.org>
Date: Mon, 23 Sep 2024 03:38:57 -0700
Subject: [PATCH] Bugfix: Fix minor issues with protected vars logging

- Fix an issue where if a user didn't have view permissions they could
  get the preference check error (a preference they wouldn't have) on
  SpecialAbuseLog
- Fix an issue where the `change-access` hadn't been updated to the used
  disabled/enabled log types
- Fix an issue where a ProtectedVarsAccessLoggerTest test wasn't
  correctly using the data provider data
- Improve naming since ProtectedVarsAccessLogger exists in its own test
  file instead of being a subset of tests on AbuseLoggerTest

Bug: T371798
Change-Id: I53f22855e63d9e1339361a5c9ee7886e0f74714a
---
 extension.json                                        |  3 ++-
 includes/Special/SpecialAbuseLog.php                  |  5 ++++-
 .../integration/ProtectedVarsAccessLoggerTest.php     | 11 ++++++-----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/extension.json b/extension.json
index 8d7f7c22a..ffe5351a4 100644
--- a/extension.json
+++ b/extension.json
@@ -160,7 +160,8 @@
 		},
 		"abusefilter-protected-vars": {
 			"change-access": [
-				"change-access"
+				"change-access-enable",
+				"change-access-disable"
 			]
 		}
 	},
diff --git a/includes/Special/SpecialAbuseLog.php b/includes/Special/SpecialAbuseLog.php
index ec8d617c6..9ded5616f 100644
--- a/includes/Special/SpecialAbuseLog.php
+++ b/includes/Special/SpecialAbuseLog.php
@@ -767,9 +767,12 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage {
 				}
 			}
 
+			// Only show the preference error if another error isn't already set
+			// as this error shouldn't take precedence over a view permission error
 			if (
 				FilterUtils::isProtected( $privacyLevel ) &&
-				!$this->afPermissionManager->canViewProtectedVariableValues( $performer )
+				!$this->afPermissionManager->canViewProtectedVariableValues( $performer ) &&
+				!$error
 			) {
 				$error = 'abusefilter-examine-protected-vars-permission';
 			}
diff --git a/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php
index cd6657dec..0a58e2b8a 100644
--- a/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php
+++ b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php
@@ -41,15 +41,16 @@ class ProtectedVarsAccessLoggerTest extends MediaWikiIntegrationTestCase {
 	/**
 	 * @dataProvider provideProtectedVarsLogTypes
 	 */
-	public function testProtectedVarsLoggerNoCUEnabled() {
+	public function testLogs_CUDisabled( $options, $expected ) {
 		$extensionRegistry = $this->createMock( ExtensionRegistry::class );
 		$extensionRegistry->method( 'isLoaded' )->with( 'CheckUser' )->willReturn( false );
 		$this->setService( 'ExtensionRegistry', $extensionRegistry );
 
 		$performer = $this->getTestSysop();
+		$logAction = $options['logAction'];
 		AbuseFilterServices::getAbuseLoggerFactory()
 			->getProtectedVarsAccessLogger()
-			->logAccessEnabled( $performer->getUserIdentity() );
+			->$logAction( $performer->getUserIdentity(), ...$options['params'] );
 
 		// Assert that the action wasn't inserted into CheckUsers' temp account logging table
 		$this->assertSame(
@@ -58,7 +59,7 @@ class ProtectedVarsAccessLoggerTest extends MediaWikiIntegrationTestCase {
 				->select( 'COUNT(*)' )
 				->from( 'logging' )
 				->where( [
-					'log_action' => 'af-change-access-enable',
+					'log_action' => $expected['expectedCULogType'],
 					'log_type' => TemporaryAccountLogger::LOG_TYPE,
 					] )
 				->fetchField()
@@ -70,7 +71,7 @@ class ProtectedVarsAccessLoggerTest extends MediaWikiIntegrationTestCase {
 				->select( 'COUNT(*)' )
 				->from( 'logging' )
 				->where( [
-					'log_action' => 'change-access-enable',
+					'log_action' => $expected['expectedAFLogType'],
 					'log_type' => ProtectedVarsAccessLogger::LOG_TYPE,
 					] )
 				->fetchField()
@@ -82,7 +83,7 @@ class ProtectedVarsAccessLoggerTest extends MediaWikiIntegrationTestCase {
 	/**
 	 * @dataProvider provideProtectedVarsLogTypes
 	 */
-	public function testProtectedVarsLoggerSendingLogsToCUTable( $options, $expected ) {
+	public function testLogs_CUEnabled( $options, $expected ) {
 		$this->markTestSkippedIfExtensionNotLoaded( 'CheckUser' );
 
 		$performer = $this->getTestSysop();