Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

UserDef: creating from id 0 should be unknown user, not context ip

Browse source 
The User class sets the name to be the context ip when creating based
on id 0, and the switch to UserIdentity(Value) matched that behavior,
but its likely unexpected for callers - treat 0 the same as other
ids that don't correspond to existing users by returning a
UserIdentityValue with an id 0 and the name "Unknown user".

Bug: T288311
Change-Id: I9763e4c1e17de8930e0f982ea868405db20a8efd
This commit is contained in:
DannyS712 2021-07-27 22:39:38 +00:00
parent 46ef24f70a
commit 5da80e9abf
2 changed files with 21 additions and 49 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
includes/ParamValidator/TypeDef/UserDef.php
View file

@ -8,7 +8,6 @@ use MediaWiki\User\UserIdentity;
use MediaWiki\User\UserIdentityLookup; use MediaWiki\User\UserIdentityLookup;
use MediaWiki\User\UserIdentityValue; use MediaWiki\User\UserIdentityValue;
use MediaWiki\User\UserNameUtils; use MediaWiki\User\UserNameUtils;
use RequestContext;
use TitleParser; use TitleParser;
use Wikimedia\IPUtils; use Wikimedia\IPUtils;
use Wikimedia\Message\MessageValue; use Wikimedia\Message\MessageValue;
@ -164,26 +163,19 @@ class UserDef extends TypeDef {
private function processUser( string $value ): array { private function processUser( string $value ): array {
// A user ID? // A user ID?
if ( preg_match( '/^#(\d+)$/D', $value, $m ) ) { if ( preg_match( '/^#(\d+)$/D', $value, $m ) ) {
// We match the behavior of trying to create a User object // This used to use the IP address of the current request if the
// based on the id. The name is: // id was 0, to match the behavior of User objects, but was switched
// * the IP address of the current request, if the id is 0 // to "Unknown user" because the former behavior is likely unexpected.
// * the name of the relevant user in the database, if one exists // If the id corresponds to a user in the database, use that user, otherwise
// * the name 'Unknown user' if it does not exist // return a UserIdentityValue with id 0 (regardless of the input id) and
// the name "Unknown user"
$userId = (int)$m[1]; $userId = (int)$m[1];
if ( $userId === 0 ) { if ( $userId !== 0 ) {
// TODO we probably shouldn't be falling back to the request IP, // Check the database.
// but if we really want to we shouldn't be doing it by accessing $userIdentity = $this->userIdentityLookup->getUserIdentityByUserId( $userId );
// the global state like this if ( $userIdentity ) {
$currentIP = RequestContext::getMain()->getRequest()->getIP(); return [ 'id', $userIdentity ];
return [ }
'id',
new UserIdentityValue( 0, IPUtils::sanitizeIP( $currentIP ) )
];
}
// Check the database.
$userIdentity = $this->userIdentityLookup->getUserIdentityByUserId( $userId );
if ( $userIdentity ) {
return [ 'id', $userIdentity ];
} }
// Fall back to "Unknown user" // Fall back to "Unknown user"
return [ return [

38
tests/phpunit/includes/ParamValidator/TypeDef/UserDefTest.php
View file

@ -7,8 +7,6 @@ use MediaWiki\MediaWikiServices;
use MediaWiki\User\UserIdentity; use MediaWiki\User\UserIdentity;
use MediaWiki\User\UserIdentityLookup; use MediaWiki\User\UserIdentityLookup;
use MediaWiki\User\UserIdentityValue; use MediaWiki\User\UserIdentityValue;
use RequestContext;
use WebRequest;
use Wikimedia\Message\DataMessageValue; use Wikimedia\Message\DataMessageValue;
use Wikimedia\ParamValidator\ParamValidator; use Wikimedia\ParamValidator\ParamValidator;
use Wikimedia\ParamValidator\SimpleCallbacks; use Wikimedia\ParamValidator\SimpleCallbacks;
@ -413,35 +411,12 @@ class UserDefTest extends TypeDefTestCase {
$this->assertSame( $expectName, $actual->getName() ); $this->assertSame( $expectName, $actual->getName() );
} }
public function testProcessUser_id0() { /**
// User created by id, when that id is 0, falls back to context ip * @dataProvider provideMissingId
// Use try-finally so that we don't interfere with other tests */
$oldRequest = RequestContext::getMain()->getRequest(); public function testProcessUser_missingId( $missingId ) {
try {
$mockRequest = $this->createMock( WebRequest::class );
$mockRequest->method( 'getIP' )->willReturn( '200.1.2.3' );
RequestContext::getMain()->setRequest( $mockRequest );
$userDef = $this->getInstance( new SimpleCallbacks( [] ), [] );
$res = $userDef->validate(
'', // $name, unused here
'#0',
[
UserDef::PARAM_ALLOWED_USER_TYPES => [ 'id' ],
UserDef::PARAM_RETURN_OBJECT => true,
], // $settings
[] // $options, unused here
);
$this->assertUserIdentity( $res, 0, '200.1.2.3' );
} finally {
RequestContext::getMain()->setRequest( $oldRequest );
}
}
public function testProcessUser_missingId() {
// User created by id, does not exist, falls back to "Unknown user" // User created by id, does not exist, falls back to "Unknown user"
// See our mock UserIdentityLookup for which ids and names exist // See our mock UserIdentityLookup for which ids and names exist
$missingId = 6;
$userDef = $this->getInstance( new SimpleCallbacks( [] ), [] ); $userDef = $this->getInstance( new SimpleCallbacks( [] ), [] );
$res = $userDef->validate( $res = $userDef->validate(
'', // $name, unused here '', // $name, unused here
@ -457,6 +432,11 @@ class UserDefTest extends TypeDefTestCase {
$this->assertUserIdentity( $res, 0, "Unknown user" ); $this->assertUserIdentity( $res, 0, "Unknown user" );
} }
public function provideMissingId() {
yield "0 no longer matches request ip" => [ 0 ];
yield "Id with no user" => [ 6 ];
}
public function testProcessUser_validId() { public function testProcessUser_validId() {
// User created by id, does exist // User created by id, does exist
// See our mock UserIdentityLookup for which ids and names exist // See our mock UserIdentityLookup for which ids and names exist