Page MenuHomePhabricator

Query condition in username migration is wrong?
Closed, ResolvedPublic

Description

Hi!

The wikimedia installation I maintain is using this great plugin for integrating Azure AD in our systems. It has been using the plugin since 1.31. It is configured to use migration of users from the previous SSO (Keycloak) by matching via username. This was working fine in 1.31. In 1.35 the code looks like this: (for matching already existing username with preferred username):

	/**
	 * @param string $username
	 * @return string|null
	 */
	public function getMigratedIdByUserName( string $username ): ?string {
		$dbr = wfGetDB( DB_REPLICA );
		$row = $dbr->selectRow(
			[
				'user',
				'openid_connect'
			],
			[
				'user_id'
			],
			[
				'user_name' => $username,
				'oidc_user' => null
			],
			__METHOD__,
			[],
			[
				'openid_connect' => [ 'LEFT JOIN', [ 'user_id=oidc_user' ] ]
			]
		);
		if ( $row !== false ) {
			return $row->user_id;
		}
		return null;
	}

I believe the condition:

				'oidc_user' => null

Is bogus and is making the code not do what it is intended to do. I removed this condition and now the migration of users is working properly.

Thanks in advance, Diego

Event Timeline

Thank you for reporting this. I agree that it looks incorrect and appears to have been introduced in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OpenIDConnect/+/426095/8/src/OpenIDConnect.php#342. Would you be able to submit a patch for this in gerrit?

I have no idea how to do that so it is more efficient for someone who already knows how to work with phabricator/gerrit to do it

You are very welcome to use developer access to submit proposed code changes as a Git branch directly into Gerrit (in the mediawiki/extensions/OpenIDConnect repository) which makes it easier to review and provide feedback. If you don't want to set up Git/Gerrit, you can also use the Gerrit Patch Uploader. Or wait for someone else. :)

Change 926668 had a related patch set uploaded (by Cicalese; author: Cicalese):

[mediawiki/extensions/OpenIDConnect@master] Remove bogus query condition in username migration

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

cicalese claimed this task.

Change 926668 merged by jenkins-bot:

[mediawiki/extensions/OpenIDConnect@master] Remove bogus query condition in username migration

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

Change 926771 had a related patch set uploaded (by Cicalese; author: Cicalese):

[mediawiki/extensions/OpenIDConnect@REL1_39] Remove bogus query condition in username migration

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

Change 926772 had a related patch set uploaded (by Cicalese; author: Cicalese):

[mediawiki/extensions/OpenIDConnect@REL1_40] Remove bogus query condition in username migration

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

Change 926771 merged by jenkins-bot:

[mediawiki/extensions/OpenIDConnect@REL1_39] Remove bogus query condition in username migration

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

Change 926772 merged by jenkins-bot:

[mediawiki/extensions/OpenIDConnect@REL1_40] Remove bogus query condition in username migration

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