From 0b3d0b3b6ddbab98758cfb6114b0003bca264f6f Mon Sep 17 00:00:00 2001 From: STran Date: Fri, 30 Aug 2024 03:29:15 -0700 Subject: [PATCH] Write protected variables access logs to CheckUser if installed Write logs related to temporary accounts to CheckUser if the extension is available so that logs are topically centralized. Bug: T373525 Depends-On: I35d50df7cd6754e29d964cc716fb3c42406272df Change-Id: Ic95f211f4db7ce6dc2d769d2f3af206f4a3935e4 --- includes/ProtectedVarsAccessLogger.php | 45 ++++--- .../ProtectedVarsAccessLoggerTest.php | 119 ++++++++++++++++++ 2 files changed, 150 insertions(+), 14 deletions(-) create mode 100644 tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php diff --git a/includes/ProtectedVarsAccessLogger.php b/includes/ProtectedVarsAccessLogger.php index f5df49305..21a5ffbb5 100644 --- a/includes/ProtectedVarsAccessLogger.php +++ b/includes/ProtectedVarsAccessLogger.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\AbuseFilter; use ManualLogEntry; +use MediaWiki\MediaWikiServices; use MediaWiki\Title\Title; use MediaWiki\User\UserIdentity; use Psr\Log\LoggerInterface; @@ -33,7 +34,7 @@ class ProtectedVarsAccessLogger { /** * @var string */ - private const LOG_TYPE = 'abusefilter-protected-vars'; + public const LOG_TYPE = 'abusefilter-protected-vars'; private LoggerInterface $logger; private IConnectionProvider $lbFactory; @@ -80,22 +81,38 @@ class ProtectedVarsAccessLogger { string $action, ?array $params = [] ): void { - $logEntry = $this->createManualLogEntry( $action ); - $logEntry->setPerformer( $performer ); - $logEntry->setTarget( Title::makeTitle( NS_USER, $target ) ); - $logEntry->setParameters( $params ); + if ( MediaWikiServices::getInstance()->getExtensionRegistry()->isLoaded( 'CheckUser' ) ) { + // Add the extension name to the action so that CheckUser has a clearer + // reference to the source in the message key + $action = 'af-' . $action; - try { - $dbw = $this->lbFactory->getPrimaryDatabase(); - $logEntry->insert( $dbw ); - } catch ( DBError $e ) { - $this->logger->critical( - 'AbuseFilter proctected variable log entry was not recorded. ' . - 'This means access to IPs can occur without being auditable. ' . - 'Immediate fix required.' + $logger = MediaWikiServices::getInstance() + ->getService( 'CheckUserTemporaryAccountLoggerFactory' ) + ->getLogger(); + $logger->logFromExternal( + $performer, + $target, + $action, + $params ); + } else { + $logEntry = $this->createManualLogEntry( $action ); + $logEntry->setPerformer( $performer ); + $logEntry->setTarget( Title::makeTitle( NS_USER, $target ) ); + $logEntry->setParameters( $params ); - throw $e; + try { + $dbw = $this->lbFactory->getPrimaryDatabase(); + $logEntry->insert( $dbw ); + } catch ( DBError $e ) { + $this->logger->critical( + 'AbuseFilter proctected variable log entry was not recorded. ' . + 'This means access to IPs can occur without being auditable. ' . + 'Immediate fix required.' + ); + + throw $e; + } } } diff --git a/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php new file mode 100644 index 000000000..cd6657dec --- /dev/null +++ b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php @@ -0,0 +1,119 @@ + [ + [ + 'logAction' => 'logAccessEnabled', + 'params' => [], + ], + [ + 'expectedCULogType' => 'af-change-access-enable', + 'expectedAFLogType' => 'change-access-enable', + ] + ]; + + yield 'disable access to protected vars values' => [ + [ + 'logAction' => 'logAccessDisabled', + 'params' => [] + ], + [ + 'expectedCULogType' => 'af-change-access-disable', + 'expectedAFLogType' => 'change-access-disable' + ] + ]; + } + + /** + * @dataProvider provideProtectedVarsLogTypes + */ + public function testProtectedVarsLoggerNoCUEnabled() { + $extensionRegistry = $this->createMock( ExtensionRegistry::class ); + $extensionRegistry->method( 'isLoaded' )->with( 'CheckUser' )->willReturn( false ); + $this->setService( 'ExtensionRegistry', $extensionRegistry ); + + $performer = $this->getTestSysop(); + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->logAccessEnabled( $performer->getUserIdentity() ); + + // Assert that the action wasn't inserted into CheckUsers' temp account logging table + $this->assertSame( + 0, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => 'af-change-access-enable', + 'log_type' => TemporaryAccountLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + // and also that it was inserted into abusefilter's protected vars logging table + $this->assertSame( + 1, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => 'change-access-enable', + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + + $this->resetServices(); + } + + /** + * @dataProvider provideProtectedVarsLogTypes + */ + public function testProtectedVarsLoggerSendingLogsToCUTable( $options, $expected ) { + $this->markTestSkippedIfExtensionNotLoaded( 'CheckUser' ); + + $performer = $this->getTestSysop(); + $logAction = $options['logAction']; + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->$logAction( $performer->getUserIdentity(), ...$options['params'] ); + + // Assert that the action was inserted into CheckUsers' temp account logging table + $this->assertSame( + 1, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => $expected['expectedCULogType'], + 'log_type' => TemporaryAccountLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + // and also that it wasn't inserted into abusefilter's protected vars logging table + $this->assertSame( + 0, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => $expected['expectedAFLogType'], + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + } +}