Page MenuHomePhabricator

CheckUser API is case-sensitive to the first character of the username
Closed, ResolvedPublic

Description

Running a check through API using the "userips" function for cutarget= 'Admin' gave back results, but running it with cutarget = 'admin' returned no results.

Event Timeline

Huji created this task.Aug 27 2017, 10:15 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 27 2017, 10:15 AM

@Huji 2 questions. Can you verify that this is still the case, and is the function userips (i.e. is the extra "a" a typo?)

Huji updated the task description. (Show Details)Jul 20 2019, 2:24 AM

That was a typo (I updated the task). The issue persists.

The relevant API code starts with

$user_id = User::idFromName( $target );
if ( !$user_id ) {
	$this->dieWithError(
		[ 'nosuchusershort', wfEscapeWikiText( $target ) ], 'nosuchuser'
	);
}

Are you getting this nosuchusershort error? If so, the scope of this issue is more than just CU

Huji added a comment.Jul 20 2019, 8:43 PM

No, I don't. Here is what I get for running a check for "admin":

{
    "batchcomplete": "",
    "query": {
        "checkuser": {
            "userips": []
        }
    }
}

And for "Admin":

{
    "batchcomplete": "",
    "query": {
        "checkuser": {
            "userips": [
                {
                    "end": "2019-07-20T20:40:15Z",
                    "editcount": 123,
                    "address": "IP.RE.DAC.TED"
                }
            ]
        }
    }
}

The API code then continues with $this->addWhereFld( 'cuc_user_text', $target );, which creates the case sensitive match. While the username could be fixed if it is uppercase, the fact that user_id has already been confirmed to exist means that user_id can be used instead: $this->addWhereFld( 'cuc_user', $user_id );. This also has the benefit that there is a key for the cuc_user column, so it may speed up performance. Thoughts?

Huji added a subscriber: Legoktm.EditedJul 21 2019, 12:16 AM

Hmmm. The only case for using cuc_user_text would be if we were thinking that a user maybe renamed but CU data might not have been synced yet. But I don't think that should be the case ever since the onRenameUserSQL hook exists (since rECHU7b16c55a4a0def59a910d4bb4bead07706a23028 implemented in 2016).

@Legoktm do you have any other thoughts? If not, I am going to change the code.

DannyS712 moved this task from Unsorted to Next on the User-DannyS712 board.

Change 524687 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/extensions/CheckUser@master] Lookup users by their ID not their username

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

Huji claimed this task.Jul 21 2019, 8:00 PM

Change 524687 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Lookup users by their ID not their username

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

Huji closed this task as Resolved.Jul 23 2019, 2:14 AM