From b66daede0a4d15c10d9367ce535b5c36d10fd843 Mon Sep 17 00:00:00 2001 From: STran Date: Thu, 12 Sep 2024 07:45:57 -0700 Subject: [PATCH] Log specific views of protected variables Like CheckUser, AbuseFilter should also log when specific protected logs are viewed. - Add support for debouncing logs to reduce log spam - Log when AbuseFilterViewExamine with protected variables available is accessed - Log when SpecialAbuseLog with protected variables available is accessed - Log when QueryAbuseLog with protected variables available is accessed Bug: T365743 Change-Id: If31a71ea5c7e2dd7c5d26ad37dc474787a7d5b1a --- extension.json | 16 ++- i18n/en.json | 4 +- i18n/qqq.json | 4 +- includes/AbuseFilterPermissionManager.php | 10 ++ includes/AbuseLoggerFactory.php | 23 +++- includes/Api/QueryAbuseLog.php | 51 ++++++- .../ProtectedVarsAccessLogFormatter.php | 41 ++++++ includes/ProtectedVarsAccessLogger.php | 127 +++++++++++++++--- includes/ServiceWiring.php | 1 + includes/Special/SpecialAbuseFilter.php | 2 + includes/Special/SpecialAbuseLog.php | 59 ++++++-- includes/View/AbuseFilterViewExamine.php | 51 +++++-- .../integration/Api/QueryAbuseLogTest.php | 9 ++ .../ProtectedVarsAccessLoggerTest.php | 80 +++++++++++ .../unit/AbuseFilterPermissionManagerTest.php | 21 +++ tests/phpunit/unit/AbuseLoggerFactoryTest.php | 2 + 16 files changed, 442 insertions(+), 59 deletions(-) create mode 100644 includes/LogFormatter/ProtectedVarsAccessLogFormatter.php diff --git a/extension.json b/extension.json index ffe5351a4..6a77937a8 100644 --- a/extension.json +++ b/extension.json @@ -85,7 +85,8 @@ "AbuseFilterVariablesBlobStore", "AbuseFilterSpecsFormatter", "AbuseFilterVariablesFormatter", - "AbuseFilterVariablesManager" + "AbuseFilterVariablesManager", + "AbuseFilterAbuseLoggerFactory" ] }, "AbuseFilter": { @@ -133,7 +134,12 @@ "suppress/unhide-afl": "MediaWiki\\Extension\\AbuseFilter\\LogFormatter\\AbuseFilterSuppressLogFormatter", "rights/blockautopromote": "MediaWiki\\Extension\\AbuseFilter\\LogFormatter\\AbuseFilterRightsLogFormatter", "rights/restoreautopromote": "MediaWiki\\Extension\\AbuseFilter\\LogFormatter\\AbuseFilterRightsLogFormatter", - "abusefilter-protected-vars/*": "LogFormatter" + "abusefilter-protected-vars/*": { + "class": "MediaWiki\\Extension\\AbuseFilter\\LogFormatter\\ProtectedVarsAccessLogFormatter", + "services": [ + "UserFactory" + ] + } }, "ActionFilteredLogs": { "abusefilter": { @@ -162,6 +168,9 @@ "change-access": [ "change-access-enable", "change-access-disable" + ], + "view-protected-variable-value": [ + "view-protected-variable-value" ] } }, @@ -231,7 +240,8 @@ "AbuseFilterPermissionManager", "AbuseFilterVariablesBlobStore", "AbuseFilterVariablesManager", - "UserFactory" + "UserFactory", + "AbuseFilterAbuseLoggerFactory" ] }, "abusefilters": { diff --git a/i18n/en.json b/i18n/en.json index 89176495f..497f7486f 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -638,6 +638,8 @@ "abusefilter-protected-vars-log-header": "This is a log of:\n# Viewing protected variables in log details\n# Changing user access levels for viewing protected variables", "logentry-abusefilter-protected-vars-change-access-enable": "$1 enabled {{GENDER:$2|his|her|their}} own access to view protected variables", "logentry-abusefilter-protected-vars-change-access-disable": "$1 disabled {{GENDER:$2|his|her|their}} own access to view protected variables", + "logentry-abusefilter-protected-vars-view-protected-var-value": "$1 viewed protected variables associated with $3", "log-action-filter-abusefilter-protected-vars": "Type of action:", - "log-action-filter-abusefilter-protected-vars-change-access": "Change access" + "log-action-filter-abusefilter-protected-vars-change-access": "Change access", + "log-action-filter-abusefilter-protected-vars-view-protected-variable-value": "View protected variable values" } diff --git a/i18n/qqq.json b/i18n/qqq.json index 6bd12d21e..2ee8464f9 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -683,6 +683,8 @@ "abusefilter-protected-vars-log-header": "Appears on top of [[Special:Log/abusefilter-protected-vars]].", "logentry-abusefilter-protected-vars-change-access-enable": "{{Logentry|[[Special:Log/abusefilter-protected-vars]]}}\n\nUsed if a user's state of access was enabled.", "logentry-abusefilter-protected-vars-change-access-disable": "{{Logentry|[[Special:Log/abusefilter-protected-vars]]}}\n\nUsed if a user's state of access was disabled.", + "logentry-abusefilter-protected-vars-view-protected-var-value": "{{Logentry|[[Special:Log/abusefilter-protected-vars]]}}\n\nUsed if user viewed the value of a protected variable.", "log-action-filter-abusefilter-protected-vars": "{{doc-log-action-filter-type|abusefilter-protected-vars}}\n{{related|Log-action-filter}}", - "log-action-filter-abusefilter-protected-vars-change-access": "{{doc-log-action-filter-action|abusefilter-protected-vars|change-access}}" + "log-action-filter-abusefilter-protected-vars-change-access": "{{doc-log-action-filter-action|abusefilter-protected-vars|change-access}}", + "log-action-filter-abusefilter-protected-vars-view-protected-variable-value": "{{doc-log-action-filter-action|abusefilter-protected-vars|view-protected-variable-value}}" } diff --git a/includes/AbuseFilterPermissionManager.php b/includes/AbuseFilterPermissionManager.php index 6cf4c573e..795ca7ff3 100644 --- a/includes/AbuseFilterPermissionManager.php +++ b/includes/AbuseFilterPermissionManager.php @@ -123,6 +123,16 @@ class AbuseFilterPermissionManager { ); } + /** + * Return all used protected variables from an array of variables. Ignore user permissions. + * + * @param string[] $usedVariables + * @return string[] + */ + public function getUsedProtectedVariables( array $usedVariables ): array { + return array_intersect( $usedVariables, $this->protectedVariables ); + } + /** * Check if the filter uses variables that the user is not allowed to use (i.e., variables that are protected, if * the user can't view protected variables), and return them. diff --git a/includes/AbuseLoggerFactory.php b/includes/AbuseLoggerFactory.php index 747873d4d..001a596dd 100644 --- a/includes/AbuseLoggerFactory.php +++ b/includes/AbuseLoggerFactory.php @@ -7,6 +7,7 @@ use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; use MediaWiki\Extension\AbuseFilter\Variables\VariablesBlobStore; use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager; use MediaWiki\Title\Title; +use MediaWiki\User\ActorStore; use MediaWiki\User\User; use Psr\Log\LoggerInterface; use Wikimedia\Rdbms\LBFactory; @@ -14,6 +15,14 @@ use Wikimedia\Rdbms\LBFactory; class AbuseLoggerFactory { public const SERVICE_NAME = 'AbuseFilterAbuseLoggerFactory'; + /** + * The default amount of time after which a duplicate log entry can be inserted. 24 hours (in + * seconds). + * + * @var int + */ + private const DEFAULT_DEBOUNCE_DELAY = 24 * 60 * 60; + /** @var CentralDBManager */ private $centralDBManager; /** @var FilterLookup */ @@ -26,6 +35,8 @@ class AbuseLoggerFactory { private $editRevUpdater; /** @var LBFactory */ private $lbFactory; + /** @var ActorStore */ + private $actorStore; /** @var ServiceOptions */ private $options; /** @var string */ @@ -42,6 +53,7 @@ class AbuseLoggerFactory { * @param VariablesManager $varManager * @param EditRevUpdater $editRevUpdater * @param LBFactory $lbFactory + * @param ActorStore $actorStore * @param ServiceOptions $options * @param string $wikiID * @param string $requestIP @@ -54,6 +66,7 @@ class AbuseLoggerFactory { VariablesManager $varManager, EditRevUpdater $editRevUpdater, LBFactory $lbFactory, + ActorStore $actorStore, ServiceOptions $options, string $wikiID, string $requestIP, @@ -65,6 +78,7 @@ class AbuseLoggerFactory { $this->varManager = $varManager; $this->editRevUpdater = $editRevUpdater; $this->lbFactory = $lbFactory; + $this->actorStore = $actorStore; $this->options = $options; $this->wikiID = $wikiID; $this->requestIP = $requestIP; @@ -72,12 +86,17 @@ class AbuseLoggerFactory { } /** + * @param int $delay * @return ProtectedVarsAccessLogger */ - public function getProtectedVarsAccessLogger() { + public function getProtectedVarsAccessLogger( + int $delay = self::DEFAULT_DEBOUNCE_DELAY + ) { return new ProtectedVarsAccessLogger( $this->logger, - $this->lbFactory + $this->lbFactory, + $this->actorStore, + $delay ); } diff --git a/includes/Api/QueryAbuseLog.php b/includes/Api/QueryAbuseLog.php index bff3471c4..4408cde4b 100644 --- a/includes/Api/QueryAbuseLog.php +++ b/includes/Api/QueryAbuseLog.php @@ -23,6 +23,7 @@ use ApiQuery; use ApiQueryBase; use InvalidArgumentException; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; +use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\CentralDBNotAvailableException; use MediaWiki\Extension\AbuseFilter\Filter\FilterNotFoundException; use MediaWiki\Extension\AbuseFilter\Filter\Flags; @@ -64,6 +65,8 @@ class QueryAbuseLog extends ApiQueryBase { /** @var UserFactory */ private $userFactory; + private AbuseLoggerFactory $abuseLoggerFactory; + /** * @param ApiQuery $query * @param string $moduleName @@ -72,6 +75,7 @@ class QueryAbuseLog extends ApiQueryBase { * @param VariablesBlobStore $afVariablesBlobStore * @param VariablesManager $afVariablesManager * @param UserFactory $userFactory + * @param AbuseLoggerFactory $abuseLoggerFactory */ public function __construct( ApiQuery $query, @@ -80,7 +84,8 @@ class QueryAbuseLog extends ApiQueryBase { AbuseFilterPermissionManager $afPermManager, VariablesBlobStore $afVariablesBlobStore, VariablesManager $afVariablesManager, - UserFactory $userFactory + UserFactory $userFactory, + AbuseLoggerFactory $abuseLoggerFactory ) { parent::__construct( $query, $moduleName, 'afl' ); $this->afFilterLookup = $afFilterLookup; @@ -88,6 +93,7 @@ class QueryAbuseLog extends ApiQueryBase { $this->afVariablesBlobStore = $afVariablesBlobStore; $this->afVariablesManager = $afVariablesManager; $this->userFactory = $userFactory; + $this->abuseLoggerFactory = $abuseLoggerFactory; } /** @@ -120,6 +126,10 @@ class QueryAbuseLog extends ApiQueryBase { $this->checkUserRightsAny( 'abusefilter-log-detail' ); } + $canViewPrivate = $this->afPermManager->canViewPrivateFiltersLogs( $performer ); + $canViewProtected = $this->afPermManager->canViewProtectedVariables( $performer ); + $canViewProtectedValues = $this->afPermManager->canViewProtectedVariableValues( $performer ); + // Map of [ [ id, global ], ... ] $searchFilters = []; // Match permissions for viewing events on private filters to SpecialAbuseLog (bug 42814) @@ -139,9 +149,6 @@ class QueryAbuseLog extends ApiQueryBase { } } - $canViewPrivate = $this->afPermManager->canViewPrivateFiltersLogs( $performer ); - $canViewProtected = $this->afPermManager->canViewProtectedVariables( $performer ); - $canViewProtectedValues = $this->afPermManager->canViewProtectedVariableValues( $performer ); if ( !$canViewPrivate || !$canViewProtected || !$canViewProtectedValues ) { foreach ( $searchFilters as [ $filterID, $global ] ) { try { @@ -345,12 +352,42 @@ class QueryAbuseLog extends ApiQueryBase { $varManager = $this->afVariablesManager; $entry['details'] = $varManager->exportAllVars( $vars ); - if ( !$this->afPermManager->canViewProtectedVariableValues( $performer ) ) { - foreach ( $this->afPermManager->getProtectedVariables() as $protectedVariable ) { + $usedProtectedVars = $this->afPermManager + ->getUsedProtectedVariables( array_keys( $entry['details'] ) ); + if ( $usedProtectedVars ) { + // Unset the variable if the user can't see protected variables + // Additionally, a protected variable is considered used if the key exists + // but since it can have a null value, check isset before logging access + $shouldLog = false; + foreach ( $usedProtectedVars as $protectedVariable ) { if ( isset( $entry['details'][$protectedVariable] ) ) { - $entry['details'][$protectedVariable] = ''; + if ( $canViewProtectedValues ) { + $shouldLog = true; + } else { + $entry['details'][$protectedVariable] = ''; + } } } + + if ( $shouldLog ) { + // phan check - user_name should always exist but guarantee it does and just in case + // if it doesn't, unset the protected variables since they shouldn't be accessed if + // the access isn't logged + if ( isset( $entry['details']['user_name'] ) ) { + $logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger(); + $logger->logViewProtectedVariableValue( + $performer->getUser(), + $entry['details']['user_name'] + ); + } else { + foreach ( $usedProtectedVars as $protectedVariable ) { + if ( isset( $entry['details'][$protectedVariable] ) ) { + $entry['details'][$protectedVariable] = ''; + } + } + } + + } } } } diff --git a/includes/LogFormatter/ProtectedVarsAccessLogFormatter.php b/includes/LogFormatter/ProtectedVarsAccessLogFormatter.php new file mode 100644 index 000000000..8a657fe08 --- /dev/null +++ b/includes/LogFormatter/ProtectedVarsAccessLogFormatter.php @@ -0,0 +1,41 @@ +userFactory = $userFactory; + } + + /** + * @inheritDoc + */ + protected function getMessageParameters() { + $params = parent::getMessageParameters(); + + // Replace temporary user page link with contributions page link. + // Don't use LogFormatter::makeUserLink, because that adds tools links. + if ( $this->entry->getSubtype() === ProtectedVarsAccessLogger::ACTION_VIEW_PROTECTED_VARIABLE_VALUE ) { + $tempUserName = $this->entry->getTarget()->getText(); + $params[2] = Message::rawParam( + Linker::userLink( 0, $this->userFactory->newUnsavedTempUser( $tempUserName ) ) + ); + } + + return $params; + } +} diff --git a/includes/ProtectedVarsAccessLogger.php b/includes/ProtectedVarsAccessLogger.php index 21a5ffbb5..744e6bcc7 100644 --- a/includes/ProtectedVarsAccessLogger.php +++ b/includes/ProtectedVarsAccessLogger.php @@ -5,8 +5,10 @@ namespace MediaWiki\Extension\AbuseFilter; use ManualLogEntry; use MediaWiki\MediaWikiServices; use MediaWiki\Title\Title; +use MediaWiki\User\ActorStore; use MediaWiki\User\UserIdentity; use Psr\Log\LoggerInterface; +use Wikimedia\Assert\Assert; use Wikimedia\Rdbms\DBError; use Wikimedia\Rdbms\IConnectionProvider; @@ -31,6 +33,13 @@ class ProtectedVarsAccessLogger { */ public const ACTION_CHANGE_ACCESS_DISABLED = 'change-access-disable'; + /** + * Represents a user viewing the value of a protected variable + * + * @var string + */ + public const ACTION_VIEW_PROTECTED_VARIABLE_VALUE = 'view-protected-var-value'; + /** * @var string */ @@ -38,17 +47,28 @@ class ProtectedVarsAccessLogger { private LoggerInterface $logger; private IConnectionProvider $lbFactory; + private ActorStore $actorStore; + private int $delay; /** * @param LoggerInterface $logger * @param IConnectionProvider $lbFactory + * @param ActorStore $actorStore + * @param int $delay The number of seconds after which a duplicate log entry can be + * created for a debounced log */ public function __construct( LoggerInterface $logger, - IConnectionProvider $lbFactory + IConnectionProvider $lbFactory, + ActorStore $actorStore, + int $delay ) { + Assert::parameter( $delay > 0, 'delay', 'delay must be positive' ); + $this->logger = $logger; $this->lbFactory = $lbFactory; + $this->actorStore = $actorStore; + $this->delay = $delay; } /** @@ -57,7 +77,7 @@ class ProtectedVarsAccessLogger { * @param UserIdentity $performer */ public function logAccessEnabled( UserIdentity $performer ): void { - $this->log( $performer, $performer->getName(), self::ACTION_CHANGE_ACCESS_ENABLED ); + $this->log( $performer, $performer->getName(), self::ACTION_CHANGE_ACCESS_ENABLED, false ); } /** @@ -66,21 +86,54 @@ class ProtectedVarsAccessLogger { * @param UserIdentity $performer */ public function logAccessDisabled( UserIdentity $performer ): void { - $this->log( $performer, $performer->getName(), self::ACTION_CHANGE_ACCESS_DISABLED ); + $this->log( $performer, $performer->getName(), self::ACTION_CHANGE_ACCESS_DISABLED, false ); + } + + /** + * Log when the user views the values of protected variables + * + * @param UserIdentity $performer + * @param string $target + * @param int|null $timestamp + */ + public function logViewProtectedVariableValue( + UserIdentity $performer, + string $target, + ?int $timestamp = null + ): void { + if ( !$timestamp ) { + $timestamp = (int)wfTimestamp(); + } + $this->log( + $performer, + $target, + self::ACTION_VIEW_PROTECTED_VARIABLE_VALUE, + true, + $timestamp + ); } /** * @param UserIdentity $performer * @param string $target * @param string $action + * @param bool $shouldDebounce + * @param int|null $timestamp * @param array|null $params */ private function log( UserIdentity $performer, string $target, string $action, + bool $shouldDebounce, + ?int $timestamp = null, ?array $params = [] ): void { + if ( !$timestamp ) { + $timestamp = (int)wfTimestamp(); + } + + // Log to CheckUser's temporary accounts log if CU is installed 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 @@ -93,25 +146,63 @@ class ProtectedVarsAccessLogger { $performer, $target, $action, - $params + $params, + $shouldDebounce, + $timestamp ); } else { - $logEntry = $this->createManualLogEntry( $action ); - $logEntry->setPerformer( $performer ); - $logEntry->setTarget( Title::makeTitle( NS_USER, $target ) ); - $logEntry->setParameters( $params ); + $dbw = $this->lbFactory->getPrimaryDatabase(); + $shouldLog = false; - 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.' - ); + // If the log is debounced, check against the logging table before logging + if ( $shouldDebounce ) { + $timestampMinusDelay = $timestamp - $this->delay; + $actorId = $this->actorStore->findActorId( $performer, $dbw ); + if ( !$actorId ) { + $shouldLog = true; + } else { + $logline = $dbw->newSelectQueryBuilder() + ->select( '*' ) + ->from( 'logging' ) + ->where( [ + 'log_type' => self::LOG_TYPE, + 'log_action' => $action, + 'log_actor' => $actorId, + 'log_namespace' => NS_USER, + 'log_title' => $target, + $dbw->expr( 'log_timestamp', '>', $dbw->timestamp( $timestampMinusDelay ) ), + ] ) + ->caller( __METHOD__ ) + ->fetchRow(); - throw $e; + if ( !$logline ) { + $shouldLog = true; + } + } + } else { + // If the log isn't debounced then it should always be logged + $shouldLog = true; + } + + // Actually write to logging table + if ( $shouldLog ) { + $logEntry = $this->createManualLogEntry( $action ); + $logEntry->setPerformer( $performer ); + $logEntry->setTarget( Title::makeTitle( NS_USER, $target ) ); + $logEntry->setParameters( $params ); + $logEntry->setTimestamp( wfTimestamp( TS_MW, $timestamp ) ); + + try { + $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/includes/ServiceWiring.php b/includes/ServiceWiring.php index 71b9762bb..ed9d1de13 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -254,6 +254,7 @@ return [ $services->get( VariablesManager::SERVICE_NAME ), $services->get( EditRevUpdater::SERVICE_NAME ), $services->getDBLoadBalancerFactory(), + $services->getActorStore(), new ServiceOptions( AbuseLogger::CONSTRUCTOR_OPTIONS, $services->getMainConfig() diff --git a/includes/Special/SpecialAbuseFilter.php b/includes/Special/SpecialAbuseFilter.php index a7612a49f..363b352b0 100644 --- a/includes/Special/SpecialAbuseFilter.php +++ b/includes/Special/SpecialAbuseFilter.php @@ -3,6 +3,7 @@ namespace MediaWiki\Extension\AbuseFilter\Special; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; +use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\CentralDBManager; use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesFactory; use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry; @@ -67,6 +68,7 @@ class SpecialAbuseFilter extends AbuseFilterSpecialPage { VariablesFormatter::SERVICE_NAME, VariablesManager::SERVICE_NAME, VariableGeneratorFactory::SERVICE_NAME, + AbuseLoggerFactory::SERVICE_NAME ], AbuseFilterViewHistory::class => [ 'UserNameUtils', diff --git a/includes/Special/SpecialAbuseLog.php b/includes/Special/SpecialAbuseLog.php index 9ded5616f..bf7be7f3f 100644 --- a/includes/Special/SpecialAbuseLog.php +++ b/includes/Special/SpecialAbuseLog.php @@ -9,6 +9,7 @@ use ManualLogEntry; use MediaWiki\Cache\LinkBatchFactory; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; use MediaWiki\Extension\AbuseFilter\AbuseFilterServices; +use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\CentralDBNotAvailableException; use MediaWiki\Extension\AbuseFilter\Consequences\ConsequencesRegistry; use MediaWiki\Extension\AbuseFilter\Filter\FilterNotFoundException; @@ -133,6 +134,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { /** @var VariablesManager */ private $varManager; + private AbuseLoggerFactory $abuseLoggerFactory; + /** * @param LBFactory $lbFactory * @param LinkBatchFactory $linkBatchFactory @@ -144,6 +147,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { * @param SpecsFormatter $specsFormatter * @param VariablesFormatter $variablesFormatter * @param VariablesManager $varManager + * @param AbuseLoggerFactory $abuseLoggerFactory */ public function __construct( LBFactory $lbFactory, @@ -155,7 +159,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { VariablesBlobStore $varBlobStore, SpecsFormatter $specsFormatter, VariablesFormatter $variablesFormatter, - VariablesManager $varManager + VariablesManager $varManager, + AbuseLoggerFactory $abuseLoggerFactory ) { parent::__construct( self::PAGE_NAME, 'abusefilter-log', $afPermissionManager ); $this->lbFactory = $lbFactory; @@ -169,6 +174,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { $this->variablesFormatter = $variablesFormatter; $this->variablesFormatter->setMessageLocalizer( $this ); $this->varManager = $varManager; + $this->abuseLoggerFactory = $abuseLoggerFactory; } /** @@ -738,12 +744,14 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { ->fetchRow(); $error = null; + $privacyLevel = Flags::FILTER_PUBLIC; if ( !$row ) { $error = 'abusefilter-log-nonexistent'; } else { $filterID = $row->afl_filter_id; $global = $row->afl_global; + $privacyLevel = $row->af_hidden; if ( $global ) { try { $privacyLevel = AbuseFilterServices::getFilterLookup()->getFilter( $filterID, $global ) @@ -752,8 +760,6 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { // Conservatively assume that it's hidden and protected, like in AbuseLogPager::doFormatRow $privacyLevel = Flags::FILTER_HIDDEN & Flags::FILTER_USES_PROTECTED_VARS; } - } else { - $privacyLevel = $row->af_hidden; } if ( !$this->afPermissionManager->canSeeLogDetailsForFilter( $performer, $privacyLevel ) ) { @@ -794,6 +800,8 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { // Load data $vars = $this->varBlobStore->loadVarDump( $row ); + $varsArray = $this->varManager->dumpAllVars( $vars, true ); + $shouldLogProtectedVarAccess = false; // If a non-protected filter and a protected filter have overlapping conditions, // it's possible for a hit to contain a protected variable and for that variable @@ -802,17 +810,40 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { // We shouldn't block access to the details of an otherwise public filter hit so // instead only check for access to the protected variables and redact them if the user // shouldn't see them. - if ( !$this->afPermissionManager->canViewProtectedVariableValues( $performer ) ) { - $varsArray = $this->varManager->dumpAllVars( $vars, true ); - foreach ( $this->afPermissionManager->getProtectedVariables() as $protectedVariable ) { - if ( isset( $varsArray[$protectedVariable] ) ) { + $userAuthority = $this->getAuthority(); + $canViewProtectedVars = $this->afPermissionManager->canViewProtectedVariableValues( $userAuthority ); + foreach ( $this->afPermissionManager->getProtectedVariables() as $protectedVariable ) { + if ( isset( $varsArray[$protectedVariable] ) ) { + if ( !$canViewProtectedVars ) { $varsArray[$protectedVariable] = ''; + } else { + // Protected variables in protected filters logs access in the general permission check + // Log access to non-protected filters that happen to expose protected variables here + if ( !FilterUtils::isProtected( $privacyLevel ) ) { + $shouldLogProtectedVarAccess = true; + } } } - $vars = VariableHolder::newFromArray( $varsArray ); + } + $vars = VariableHolder::newFromArray( $varsArray ); + + // Log if protected variables are accessed + if ( + FilterUtils::isProtected( $privacyLevel ) && + $canViewProtectedVars + ) { + $shouldLogProtectedVarAccess = true; } - $out->addJsConfigVars( 'wgAbuseFilterVariables', $this->varManager->dumpAllVars( $vars, true ) ); + if ( $shouldLogProtectedVarAccess ) { + $logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger(); + $logger->logViewProtectedVariableValue( + $userAuthority->getUser(), + $varsArray['user_name'] + ); + } + + $out->addJsConfigVars( 'wgAbuseFilterVariables', $varsArray ); $out->addModuleStyles( 'mediawiki.interface.helpers.styles' ); // Diff, if available @@ -886,7 +917,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { * or an error and no row. */ public static function getPrivateDetailsRow( Authority $authority, $id ) { - $afPermManager = AbuseFilterServices::getPermissionManager(); + $afPermissionManager = AbuseFilterServices::getPermissionManager(); $dbr = MediaWikiServices::getInstance()->getConnectionProvider()->getReplicaDatabase(); $row = $dbr->newSelectQueryBuilder() @@ -914,7 +945,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { $privacyLevel = $row->af_hidden; } - if ( !$afPermManager->canSeeLogDetailsForFilter( $authority, $privacyLevel ) ) { + if ( !$afPermissionManager->canSeeLogDetailsForFilter( $authority, $privacyLevel ) ) { $status->fatal( 'abusefilter-log-cannot-see-details' ); return $status; } @@ -1167,15 +1198,15 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage { /** * @param stdClass $row * @param Authority $authority - * @param AbuseFilterPermissionManager $afPermManager + * @param AbuseFilterPermissionManager $afPermissionManager * @return string One of the self::VISIBILITY_* constants */ public static function getEntryVisibilityForUser( stdClass $row, Authority $authority, - AbuseFilterPermissionManager $afPermManager + AbuseFilterPermissionManager $afPermissionManager ): string { - if ( $row->afl_deleted && !$afPermManager->canSeeHiddenLogEntries( $authority ) ) { + if ( $row->afl_deleted && !$afPermissionManager->canSeeHiddenLogEntries( $authority ) ) { return self::VISIBILITY_HIDDEN; } if ( !$row->afl_rev_id ) { diff --git a/includes/View/AbuseFilterViewExamine.php b/includes/View/AbuseFilterViewExamine.php index 1b9b01501..300e6c9d1 100644 --- a/includes/View/AbuseFilterViewExamine.php +++ b/includes/View/AbuseFilterViewExamine.php @@ -7,6 +7,7 @@ use LogicException; use MediaWiki\Context\IContextSource; use MediaWiki\Extension\AbuseFilter\AbuseFilterChangesList; use MediaWiki\Extension\AbuseFilter\AbuseFilterPermissionManager; +use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory; use MediaWiki\Extension\AbuseFilter\CentralDBNotAvailableException; use MediaWiki\Extension\AbuseFilter\EditBox\EditBoxBuilderFactory; use MediaWiki\Extension\AbuseFilter\Filter\Flags; @@ -62,6 +63,8 @@ class AbuseFilterViewExamine extends AbuseFilterView { */ private $varGeneratorFactory; + private AbuseLoggerFactory $abuseLoggerFactory; + /** * @param LBFactory $lbFactory * @param AbuseFilterPermissionManager $afPermManager @@ -71,6 +74,7 @@ class AbuseFilterViewExamine extends AbuseFilterView { * @param VariablesFormatter $variablesFormatter * @param VariablesManager $varManager * @param VariableGeneratorFactory $varGeneratorFactory + * @param AbuseLoggerFactory $abuseLoggerFactory * @param IContextSource $context * @param LinkRenderer $linkRenderer * @param string $basePageName @@ -85,6 +89,7 @@ class AbuseFilterViewExamine extends AbuseFilterView { VariablesFormatter $variablesFormatter, VariablesManager $varManager, VariableGeneratorFactory $varGeneratorFactory, + AbuseLoggerFactory $abuseLoggerFactory, IContextSource $context, LinkRenderer $linkRenderer, string $basePageName, @@ -99,6 +104,7 @@ class AbuseFilterViewExamine extends AbuseFilterView { $this->variablesFormatter->setMessageLocalizer( $context ); $this->varManager = $varManager; $this->varGeneratorFactory = $varGeneratorFactory; + $this->abuseLoggerFactory = $abuseLoggerFactory; } /** @@ -290,19 +296,22 @@ class AbuseFilterViewExamine extends AbuseFilterView { return; } + $shouldLogProtectedVarAccess = false; + // Logs that reveal the values of protected variables are gated behind: // 1. the `abusefilter-access-protected-vars` right // 2. agreement to the `abusefilter-protected-vars-view-agreement` preference $userAuthority = $this->getAuthority(); - if ( FilterUtils::isProtected( $privacyLevel ) && - !$this->afPermManager->canViewProtectedVariableValues( $userAuthority ) - ) { - $out->addWikiMsg( 'abusefilter-examine-protected-vars-permission' ); - return; + $canViewProtectedVars = $this->afPermManager->canViewProtectedVariableValues( $userAuthority ); + if ( FilterUtils::isProtected( $privacyLevel ) ) { + if ( !$canViewProtectedVars ) { + $out->addWikiMsg( 'abusefilter-examine-protected-vars-permission' ); + return; + } else { + $shouldLogProtectedVarAccess = true; + } } - $vars = $this->varBlobStore->loadVarDump( $row ); - // If a non-protected filter and a protected filter have overlapping conditions, // it's possible for a hit to contain a protected variable and for that variable // to be dumped and displayed on a detail page that wouldn't be considered @@ -310,18 +319,34 @@ class AbuseFilterViewExamine extends AbuseFilterView { // We shouldn't block access to the details of an otherwise public filter hit so // instead only check for access to the protected variables and redact them if the // user shouldn't see them. - if ( !$this->afPermManager->canViewProtectedVariableValues( $userAuthority ) ) { - $varsArray = $this->varManager->dumpAllVars( $vars, true ); - foreach ( $this->afPermManager->getProtectedVariables() as $protectedVariable ) { - if ( isset( $varsArray[$protectedVariable] ) ) { + $vars = $this->varBlobStore->loadVarDump( $row ); + $varsArray = $this->varManager->dumpAllVars( $vars, true ); + + foreach ( $this->afPermManager->getProtectedVariables() as $protectedVariable ) { + if ( isset( $varsArray[$protectedVariable] ) ) { + if ( !$canViewProtectedVars ) { $varsArray[$protectedVariable] = ''; + } else { + // Protected variable in protected filters logs access in the general permission check + // Log access to non-protected filters that happen to expose protected variables here + if ( !FilterUtils::isProtected( $privacyLevel ) ) { + $shouldLogProtectedVarAccess = true; + } } } - $vars = VariableHolder::newFromArray( $varsArray ); + } + $vars = VariableHolder::newFromArray( $varsArray ); + + if ( $shouldLogProtectedVarAccess ) { + $logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger(); + $logger->logViewProtectedVariableValue( + $userAuthority->getUser(), + $varsArray['user_name'] + ); } $out->addJsConfigVars( [ - 'wgAbuseFilterVariables' => $this->varManager->dumpAllVars( $vars, true ), + 'wgAbuseFilterVariables' => $varsArray, 'abuseFilterExamine' => [ 'type' => 'log', 'id' => $logid ] ] ); $this->showExaminer( $vars ); diff --git a/tests/phpunit/integration/Api/QueryAbuseLogTest.php b/tests/phpunit/integration/Api/QueryAbuseLogTest.php index a33f7ff52..5598d4352 100644 --- a/tests/phpunit/integration/Api/QueryAbuseLogTest.php +++ b/tests/phpunit/integration/Api/QueryAbuseLogTest.php @@ -11,6 +11,7 @@ use MediaWiki\Extension\AbuseFilter\Filter\Specs; use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; use MediaWiki\Permissions\SimpleAuthority; use MediaWiki\Tests\Api\ApiTestCase; +use MediaWiki\User\ActorStore; use MediaWiki\User\Options\StaticUserOptionsLookup; use MediaWiki\User\UserIdentityValue; use Wikimedia\TestingAccessWrapper; @@ -59,6 +60,7 @@ class QueryAbuseLogTest extends ApiTestCase { VariableHolder::newFromArray( [ 'action' => 'edit', 'user_unnamed_ip' => '1.2.3.4', + 'user_name' => 'User1', ] ) )->addLogEntries( [ 1 => [ 'warn' ] ] ); @@ -108,6 +110,11 @@ class QueryAbuseLogTest extends ApiTestCase { ); $this->setService( 'UserOptionsLookup', $userOptions ); + // Actor store needs to return a valid actor_id for the logs querying generates + $actorStore = $this->createMock( ActorStore::class ); + $actorStore->method( 'acquireActorId' )->willReturn( 1 ); + $this->setService( 'ActorStore', $actorStore ); + // Assert that the ip is now visible $result = $this->doApiRequest( [ 'action' => 'query', @@ -116,6 +123,8 @@ class QueryAbuseLogTest extends ApiTestCase { ], null, null, $authorityCanViewProtectedVar ); $result = $result[0]['query']['abuselog']; $this->assertSame( '1.2.3.4', $result[0]['details']['user_unnamed_ip'] ); + + $this->resetServices(); } /** diff --git a/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php index 0a58e2b8a..f6a241b4c 100644 --- a/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php +++ b/tests/phpunit/integration/ProtectedVarsAccessLoggerTest.php @@ -117,4 +117,84 @@ class ProtectedVarsAccessLoggerTest extends MediaWikiIntegrationTestCase { ->fetchField() ); } + + public function testDebouncedLogs_CUDisabled() { + $extensionRegistry = $this->createMock( ExtensionRegistry::class ); + $extensionRegistry->method( 'isLoaded' )->with( 'CheckUser' )->willReturn( false ); + $this->setService( 'ExtensionRegistry', $extensionRegistry ); + + // Run the same action twice + $performer = $this->getTestSysop(); + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->logViewProtectedVariableValue( $performer->getUserIdentity(), '~2024-01', (int)wfTimestamp() ); + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->logViewProtectedVariableValue( $performer->getUserIdentity(), '~2024-01', (int)wfTimestamp() ); + + // 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-view-protected-var-value', + 'log_type' => TemporaryAccountLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + // and also that it only inserted once into abusefilter's protected vars logging table + $this->assertSame( + 1, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => 'view-protected-var-value', + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + + $this->resetServices(); + } + + public function testDebouncedLogs_CUEnabled() { + $this->markTestSkippedIfExtensionNotLoaded( 'CheckUser' ); + + // Run the same action twice + $performer = $this->getTestSysop(); + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->logViewProtectedVariableValue( $performer->getUserIdentity(), '~2024-01', (int)wfTimestamp() ); + AbuseFilterServices::getAbuseLoggerFactory() + ->getProtectedVarsAccessLogger() + ->logViewProtectedVariableValue( $performer->getUserIdentity(), '~2024-01', (int)wfTimestamp() ); + + // Assert that the action only inserted once into CheckUsers' temp account logging table + $this->assertSame( + 1, + (int)$this->getDb()->newSelectQueryBuilder() + ->select( 'COUNT(*)' ) + ->from( 'logging' ) + ->where( [ + 'log_action' => 'af-view-protected-var-value', + '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' => 'view-protected-var-value', + 'log_type' => ProtectedVarsAccessLogger::LOG_TYPE, + ] ) + ->fetchField() + ); + } } diff --git a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php index 1842dcb85..528b75f81 100644 --- a/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php +++ b/tests/phpunit/unit/AbuseFilterPermissionManagerTest.php @@ -291,6 +291,27 @@ class AbuseFilterPermissionManagerTest extends MediaWikiUnitTestCase { ); } + public function provideTestGetUsedProtectedVariables(): Generator { + $userCheckedPreference = new UserIdentityValue( 1, 'User1' ); + $userUncheckedPreference = new UserIdentityValue( 2, 'User2' ); + yield 'uses protected variables' => [ + [ 'user_unnamed_ip', 'user_name' ], [ 'user_unnamed_ip' ] + ]; + yield 'no protected variables' => [ + [ 'user_name' ], [] + ]; + } + + /** + * @dataProvider provideTestGetUsedProtectedVariables + */ + public function testGetUsedProtectedVariables( array $usedVariables, $expected ) { + $this->assertSame( + $expected, + $this->getPermMan()->getUsedProtectedVariables( $usedVariables ) + ); + } + public static function provideGetForbiddenVariables(): Generator { yield 'cannot view, protected vars' => [ [ diff --git a/tests/phpunit/unit/AbuseLoggerFactoryTest.php b/tests/phpunit/unit/AbuseLoggerFactoryTest.php index 1fd357d34..818123bb1 100644 --- a/tests/phpunit/unit/AbuseLoggerFactoryTest.php +++ b/tests/phpunit/unit/AbuseLoggerFactoryTest.php @@ -14,6 +14,7 @@ use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder; use MediaWiki\Extension\AbuseFilter\Variables\VariablesBlobStore; use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager; use MediaWiki\Title\Title; +use MediaWiki\User\ActorStore; use MediaWiki\User\User; use MediaWikiUnitTestCase; use Psr\Log\LoggerInterface; @@ -33,6 +34,7 @@ class AbuseLoggerFactoryTest extends MediaWikiUnitTestCase { $this->createMock( VariablesManager::class ), $this->createMock( EditRevUpdater::class ), $this->createMock( LBFactory::class ), + $this->createMock( ActorStore::class ), new ServiceOptions( AbuseLogger::CONSTRUCTOR_OPTIONS, [