Page MenuHomePhabricator

Update AbuseFilter with the right user verification
Closed, ResolvedPublic3 Estimated Story Points

Description

Following the investigation on AbuseFilter for IP Masking, update the following files to reflect the implementation of Temporary Account Users.

ConsequencesExecutor::
specializeParameters()



Define the expiry block by user type


$consParams[$filter][$name] = [
'expiry' => $user->isRegistered() ? $parameters[2] : $parameters[1],
'blocktalk' => $parameters[0] === 'blocktalk'
];



Suggestion:
'expiry' => $user->isNamed() ? $parameters[2] : $parameters[1]
,
The temp and anon user get the same expiration time for blocks


BlockAutopromote::execute()


Verify user identity before giving them blockautopromote rights

!$target->isRegistered()


Suggestion:
!$target->isNamed()


Don’t let temp and anon users go through the process of getting blockautopromote rights

CheckUserHandler::onCheckUserInsertChangesRow()


if ( $user->isRegistered() && $this->filterUser->getUserIdentity()->getId() == $user->getId() )


Suggestions:
$user->isNamed() - The filter user shouldn’t be a temp user.

CheckUserHandler::onCheckUserInsertLogEventRow()

Logs action should only be by a named user

$user->isNamed()

CheckUserHandler::
onCheckUserInsertPrivateEventRow()


Log action should only be by a named user
$user->isNamed()

RCVariableGenerator::addCreateAccountVars()



Case of object type UserIdentity - only can call ->isTemp() and isRegistered

if ( $userIdentity->isRegistered() && $name !== $userIdentity->getName() ) {
$this->addUserVars( $userIdentity, $this->rc );
}



Suggestion:


$userIsNamed = $userIdentity->isRegistered() && !$userIdentity->isTemp()
if ( $userIsNamed && $name !== $userIdentity->getName() ) {
$this->addUserVars( $userIdentity, $this->rc );
}

LazyVariableComputer::compute()

if ( !$user->isRegistered() )

Suggestion: !$user->isNamed()

LazyVariableComputerTest::provideUserRelatedVars()

$user->method( 'isRegistered' )->willReturn( true );

Suggestion: $user->method( ‘isNamed )->willReturn( true );




QueryAbuselog::execute()



if ( isset( $params['user'] ) ) {
$u = User::newFromName( $params['user'] );
if ( $u ) {
// Username normalisation
$params['user'] = $u->getName();
$userId = $u->getId();
} elseif ( IPUtils::isIPAddress( $params['user'] ) ) {
// It's an IP, sanitize it
$params['user'] = IPUtils::sanitizeIP( $params['user'] );
$userId = 0;
}

Sanitizing IPs for the log


Suggestion: Eventually remove the elseif condition?

Event Timeline

AGueyte set the point value for this task to 1.

Change 911301 had a related patch set uploaded (by AGueyte; author: AGueyte):

[mediawiki/extensions/AbuseFilter@master] Update AbuseFilter with the right user verification for Temporary Users

https://gerrit.wikimedia.org/r/911301

Noting here that being able to use the IP of logged-out users is a very important feature that AF currently provides. The expectation is that user_name will be the IP of logged-out users, and many filters make this assumption. Finding all of them is going to be hard, but for instance, all filters using ip_in_range or ip_in_ranges are going to be affected; see partial list of wikis in the following NDA paste: P47270.

Also, many filter use "user" in user_groups (or variations thereof) to check if a user is logged-in or not. Temp users are in the user group, so this is also going to fail.

These are just the first two examples I could think of, but there could be more. Updating the AF codebase might be relatively easy; updating all the existing filters, not so much I believe. Maybe it'd be a little bit easier if AF also provided a user_is_temp variable or something, but that still wouldn't address some issues like accessing the user's IP.

AGueyte changed the point value for this task from 1 to 3.May 4 2023, 5:45 PM

Change 911301 had a related patch set uploaded (by Tchanders; author: AGueyte):

[mediawiki/extensions/AbuseFilter@master] Update user type checks to handle temporary users

https://gerrit.wikimedia.org/r/911301

Tchanders closed this task as a duplicate of T307060: [IP Masking] Temporary account AbuseFilter support.

Mistake - wrong task. This one isn't a duplicate.

Change 911301 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Update user type checks to handle temporary users

https://gerrit.wikimedia.org/r/911301

matej_suchanek subscribed.

Will user rights management be possible with temporary users?
See Degroup::execute:

includes/Consequences/Consequence/Degroup.php
$user = $this->parameters->getUser();
if ( !$user->isRegistered() ) {
	return false;
}

Maybe this one was forgotten?

Change 924519 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/AbuseFilter@master] Degroup: Return early if user is a temporary user

https://gerrit.wikimedia.org/r/924519

@matej_suchanek Thanks for pointing that out - looks like it was just missed.

Temporary users won't have groups T330816: [Epic] Temporary users should not be assigned to user groups, so the patch above updates this.

@AGueyte @Tchanders I imported new user blanking articles into my Local. When I'm not logged in, I get an error when I try to blank out an article as seen in the .webm. When I have a temp user though, it seems that the abuse filter works, along with dewiki too, as seen in my screenshots.

Test link: Local
Beta: https://de.wikipedia.beta.wmflabs.org/wiki/Dogobert_Dock

Local: Not logged in- Error

Local: Temp User- WAD. (*Unregistered 32 was created after the .webm to test out temp user.)

T335062_AbuseFilter_LocalTemp_Working.png (801×3 px, 179 KB)

de Wiki Beta

T335062_AbuseFilter_DeBeta_Working.png (1×1 px, 200 KB)

@GMikesell-WMF Ah yes, I should've added this to my testing notes... It's the same issue that was reported in T335064 (which has now been merged into another task, and will be fixed when we do that work).

The error happens when you trigger an abuse filter and try to create a new temporary user at the same time. The way to work around it is:

  • Create a temporary user by doing something successfully first
  • Once the temporary user is created, you can then use it to trigger the abuse filter

Moving back to code review since there's one more patch:

Change 924519 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/AbuseFilter@master] Degroup: Return early if user is a temporary user

https://gerrit.wikimedia.org/r/924519

Change 924519 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Degroup: Return early if user is a temporary user

https://gerrit.wikimedia.org/r/924519

@AGueyte Other than the issue below, I came up with the same results from my previous comments. If you want me to create a ticket, please let me know. I can move this ticket to Done since the other issue will be fixed in T335064: Investigate AbuseFilter Hook with CheckUser

`Different Browsers, blocking the IP Address- Left side is Chrome 114 with a different IP address than Safari 16.4 (right side). On the left with Chrome, I logged in as an Admin. I then did a partial block of the IP address of Safari when it goes to the "Dog" article. On the Safari side, I go to the "Beer" article since it's not blocked to make a temp user. I then go to the "Dog" article which I was able to still edit even though it should be blocked.

Blocking the temp user- Same block config as above but using the temp user name this time and when it went to the "Dog" article, the user was blocked

T335062_IPMasking_AbuseFilter_BlockTemp.png (791×1 px, 291 KB)

Hi @GMikesell-WMF, thanks for this.

Do you get the same result if you block the user from everything and not specifically a page?
If not, would you mind creating a ticket to investigate this issue?
Thank you!

@AGueyte Nope, I get the same block message as blocking the temp user screenshot from above. I created T339947: IP Masking- Special:Block w/ Different Browsers; Partial Site and will move this task to Done. Thanks!

Closing this - note that T331653 is still open tracking the next round of product investigation into AbuseFilter