Page MenuHomePhabricator

Fix limit handling in TemporaryAccountHandler query
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

TemporaryAccountHandler returns the IP addresses used by a temporary account.

A limit parameter can be passed. This means the maximum number of IP addresses that can be returned (as opposed to rows that can be read by the query).

A config, $wgCheckUserMaximumRowCount controls the maximum number of rows that can be read, for performance reasons.

The query currently takes the lowest of these two limits, and applies that as a limit on the query. The unique IPs from the resulting rows are then returned.

Instead, the config limit should be used in the query, but the parameter limit (if present) should be applied in PHP, after the unique IPs have been found from the rows returned by the query.

This should solve 2 problems with the limit handling (see also T324603#8584676):

  • Number of IPs returned is sometimes less than the limit parameter. This is because there can be multiple rows for the same actor-IP-timestamp, and the limit is being applied to the number of rows read.
  • A negative limit parameter causes a SQL error
Acceptance criteria
  • The API only returns fewer IPs than the limit parameter if there are actually fewer IPs (or if the limit parameter is higher than $wgCheckUserMaximumRowCount)
  • Passing a negative limit is handled by the API and returns an appropriate error message and code

Event Timeline

To achieve the first acceptance criteria, I would suggest copying some of the SQL query from Special:CheckUser's 'Get IP Addresses' (which can be found in CheckUserGetIPsPager::getQueryInfo) as this does what the acceptance criteria is asking for. This would mean no need for the PHP limiting and the IPs would be pre-grouped.

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

[mediawiki/extensions/CheckUser@master] Fix limit handling in TemporaryAccountHandler query

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

Hi I have a question regarding

Passing a negative limit is handled by the API and returns an appropriate error message and code

What do we consider an appropriate error message and code in this situation?
The result I have doesn't return an error but an empty array.
Do we want a message such as "You have entered a negative limit, please enter a positive number to see results" ?

Thanks!

AGueyte set the point value for this task to 3.Feb 21 2023, 4:39 PM

Instead, the config limit should be used in the query, but the parameter limit (if present) should be applied in PHP, after the unique IPs have been found from the rows returned by the query.

Why does this have to be the case (assuming that the method of one IP per row in the SQL result is followed)? The result from the DB isn't cached server-side, so I don't see why the user provided limit (after validation) can't be used directly on the SQL query.

To test the patch, visit the hook URL
http://localhost:8080/w/rest.php/checkuser/v0/temporaryaccount/*Unregistered%205?limit=5001

Use a temporary account user with multiple IPs, and add the limit param to test scenarios.

Change 890871 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Fix limit handling in TemporaryAccountHandler query

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

@AGueyte Please see the results below. Everything seems to be good but I wanted to check with you about the screenshots error wording when testing the limit being negative and limit above the maximum row count. If the error wording is correct for them, I can move this to Done. Thanks!

OS: macOS 13.0
Browser: Chrome 110
Environment: Local

Test**Results*
Limit Over
T328894_IPMasking_Admin_TempAccountHandler_Limit Over.png (785×3 px, 563 KB)
Limit Negative
T328894_IPMasking_Admin_TempAccountHandler_LimitNegative.png (688×3 px, 488 KB)
Limit Above MRC
T328894_IPMasking_Admin_TempAccountHandler_LimitAboveMRC.png (583×3 px, 428 KB)
Limit Below MRC
T328894_IPMasking_Admin_TempAccountHandler_LimitBelowMRC.png (715×3 px, 453 KB)
5 Different IPs & Different Temp Users
T328894_IPMasking_Admin_TempAccountHandler_5Temp5DiffIPS.png (1×2 px, 716 KB)
6 Different TempUsers w/same IP
T328894_IPMasking_Admin_TempAccountHandler_6Temp5SameIPS.png (1×2 px, 782 KB)
Same TempUser w/5 Different IPs
T328894_IPMasking_Admin_TempAccountHandler_1Temp5DiffIPS.png (1×2 px, 798 KB)

Thank you for that!

The wording is the one from the getParamSettings() function so I assume it's the one expected for the specific errors!