wikimedia/mediawiki-extensions-AbuseFilter
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git synced 2024-11-23 13:46:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Protected variable logs: fallback to accountname if user_name is not set

Browse source 
Why:

- For account creations and account autocreations, the user_name
  property is deliberately unset, to avoid displaying the IP address of
  an unregistered user. Instead, `accountname` is set with the newly
  created account name
- For logging that someone has seen a protected variable value, we need
  to record the username that was seen

What:

- Use `accountname` as a fallback in case `user_name` is not set, when
  logging protected variable access
- Update tests to cover this case.

Bug: T376885
Change-Id: I688a3529fac0ad8455977a0cfdb950f0105f550d
This commit is contained in:
Kosta Harlan 2024-10-15 16:31:09 +02:00
parent 08910fcf38
commit 05da3118aa
No known key found for this signature in database
GPG key ID: BC3D8915606A5ED9
4 changed files with 37 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
includes/Api/QueryAbuseLog.php
View file

@ -370,14 +370,16 @@ class QueryAbuseLog extends ApiQueryBase {
}
if ( $shouldLog ) {
// phan check - user_name should always exist but guarantee it does and just in case
// user_name or accountname should always exist -- 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'] ) ) {
if ( isset( $entry['details']['user_name'] ) ||
isset( $entry['details']['accountname'] )
) {
$logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger();
$logger->logViewProtectedVariableValue(
$performer->getUser(),
$entry['details']['user_name']
$entry['details']['user_name'] ?? $entry['details']['accountname']
);
} else {
foreach ( $usedProtectedVars as $protectedVariable ) {

2
includes/Special/SpecialAbuseLog.php
View file

@ -839,7 +839,7 @@ class SpecialAbuseLog extends AbuseFilterSpecialPage {
$logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger();
$logger->logViewProtectedVariableValue(
$userAuthority->getUser(),
$varsArray['user_name']
$varsArray['user_name'] ?? $varsArray['accountname']
);
}

2
includes/View/AbuseFilterViewExamine.php
View file

@ -341,7 +341,7 @@ class AbuseFilterViewExamine extends AbuseFilterView {
$logger = $this->abuseLoggerFactory->getProtectedVarsAccessLogger();
$logger->logViewProtectedVariableValue(
$userAuthority->getUser(),
$varsArray['user_name']
$varsArray['user_name'] ?? $varsArray['accountname']
);
}

39
tests/phpunit/integration/Api/QueryAbuseLogTest.php
View file

@ -32,7 +32,7 @@ class QueryAbuseLogTest extends ApiTestCase {
$this->addToAssertionCount( 1 );
}
public function testProtectedVariableValueAcces() {
public function testProtectedVariableValueAccess() {
// Add filter to query for
$filter = [
'id' => '1',
@ -54,14 +54,24 @@ class QueryAbuseLogTest extends ApiTestCase {
$this->createFilter( $filter );
// Insert a hit on the filter
AbuseFilterServices::getAbuseLoggerFactory()->newLogger(
$abuseFilterLoggerFactory = AbuseFilterServices::getAbuseLoggerFactory();
$abuseFilterLoggerFactory->newLogger(
$this->getExistingTestPage()->getTitle(),
$this->getTestUser()->getUser(),
VariableHolder::newFromArray( [
'action' => 'edit',
'user_unnamed_ip' => '1.2.3.4',
'user_name' => 'User1',
] )
] )
)->addLogEntries( [ 1 => [ 'warn' ] ] );
$abuseFilterLoggerFactory->newLogger(
$this->getExistingTestPage()->getTitle(),
$this->getTestUser()->getUser(),
VariableHolder::newFromArray( [
'action' => 'autocreateaccount',
'user_unnamed_ip' => '1.2.3.4',
'accountname' => 'User1',
] )
)->addLogEntries( [ 1 => [ 'warn' ] ] );
// Update afl_ip to a known value that can be used when it's reconstructed in the variable holder
@ -95,10 +105,13 @@ class QueryAbuseLogTest extends ApiTestCase {
$result = $this->doApiRequest( [
'action' => 'query',
'list' => 'abuselog',
'aflprop' => 'details'
'aflprop' => 'details',
'afldir' => 'older',
], null, null, $authorityCanViewProtectedVar );
$result = $result[0]['query']['abuselog'];
$this->assertSame( '', $result[0]['details']['user_unnamed_ip'] );
foreach ( $result as $row ) {
$this->assertSame( '', $row['details']['user_unnamed_ip'], 'IP is redacted' );
}
// Enable the preference for the user
$userOptions = new StaticUserOptionsLookup(
@ -119,12 +132,20 @@ class QueryAbuseLogTest extends ApiTestCase {
$result = $this->doApiRequest( [
'action' => 'query',
'list' => 'abuselog',
'aflprop' => 'details'
'aflprop' => 'details',
'afldir' => 'older',
], null, null, $authorityCanViewProtectedVar );
$result = $result[0]['query']['abuselog'];
$this->assertSame( '1.2.3.4', $result[0]['details']['user_unnamed_ip'] );
$this->resetServices();
foreach ( $result as $row ) {
$this->assertSame( '1.2.3.4', $row['details']['user_unnamed_ip'] );
if ( isset( $row['details']['accountname'] ) ) {
$this->assertSame( 'User1', $row['details']['accountname'] );
$this->assertArrayNotHasKey( 'user_name', $row['details'] );
} else {
$this->assertSame( 'User1', $row['details']['user_name'] );
$this->assertArrayNotHasKey( 'accountname', $row['details'] );
}
}
}
/**