Page MenuHomePhabricator

Prepare OATHAuth and WebAuthn extensions for IP Masking
Closed, ResolvedPublic

Description

A preliminary investigation (T326759) has found that the OATHAuth and WebAuthn extensions may be affected by IP Masking

Event Timeline

pmiazga changed the task status from Open to In Progress.Jan 15 2024, 3:54 PM
pmiazga claimed this task.
pmiazga triaged this task as Medium priority.

The WebAuthn extension requires one change to fully support IPMasking. The only place that required investigation is checkPermissions method - https://gerrit.wikimedia.org/g/mediawiki/extensions/WebAuthn/+/122deaa8a99b868c08f9fa1e5e5d4ab0a4387bf3/src/Api/WebAuthn.php#112

	protected function checkPermissions( string $func ): void {
		$registered = $this->getRegisteredFunctions();
		$functionConfig = $registered[$func];

		$mustBeLoggedIn = $functionConfig['mustBeLoggedIn'];
		if ( $mustBeLoggedIn === true ) {
			$user = $this->getUser();
			if ( !$user->isRegistered() ) {
				$this->dieWithError( [ 'apierror-mustbeloggedin', $this->msg( 'action-oathauth-enable' ) ] );
			}
		}

		$funcPermissions = $functionConfig['permissions'];
		if ( $funcPermissions ) {
			$this->checkUserRightsAny( $funcPermissions );
		}
	}

We need to change from if ( !$user->isRegistered() ) { to if ( !$user->isNamed() ) { to throw present this error to both anon and temp users.

Change 991081 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/extensions/WebAuthn@master] Temp users require to create account in order to use WebAuthn

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

Change 991084 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/extensions/OATHAuth@master] OATH validation is available only to named users

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

In the OATHAuth extension we are required to change two places:

a) https://gerrit.wikimedia.org/g/mediawiki/extensions/OATHAuth/+/320d0c5d17d1fb902059ced24ccb6dc16bb33864/src/Api/Module/ApiOATHValidate.php#63
b) https://gerrit.wikimedia.org/g/mediawiki/extensions/OATHAuth/+/320d0c5d17d1fb902059ced24ccb6dc16bb33864/src/Api/Module/ApiQueryOATH.php#90

In both places we check if ( !$user->isAnon() ) -> but OATH is available only for named users -> therefore those checks should be rewritten to
if ( $user->isNamed() ).

The OATHAuth extension uses $user->name() in multiple places but does not display it to other users. It is used only for server logging, creating cache keys or selecting the correct gender form when displaying messages.

Change 991084 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] OATH validation is available only to named users

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

Change 991081 merged by jenkins-bot:

[mediawiki/extensions/WebAuthn@master] Temp users require to create account in order to use WebAuthn

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

In OATHAuth, the current permission configuration (in extension.json) adds oathauth-enable to user, that should probably be named.

I can find nothing else with a quick grep. disableOATHAuthForUser.php could maybe complain if the user parameter is a temp user, but then it's pretty unlikely that anyone would invoke it with a temp user.

In OATHAuth, the current permission configuration (in extension.json) adds oathauth-enable to user, that should probably be named.

I can find nothing else with a quick grep. disableOATHAuthForUser.php could maybe complain if the user parameter is a temp user, but then it's pretty unlikely that anyone would invoke it with a temp user.

Thanks, that's a great find. I'll look into it as I'm wondering if changing this can break things - I mean mess up exiting permissions as we change from user to named.

Sorry, I double-checked and temporary users are not actually part of user (filed T355741: 'named' user group is pointless about that). So nothing left to do here.