Page MenuHomePhabricator

Update features for IP Masking
Open, Needs TriagePublic

Description

IP Masking will affect lots of our products, features, tools, gadgets, etc. This task is for tracking work to update those things ahead of IP Masking being enabled on WMF sites.

Since T300263: [IP Masking] Create temporary account on first edit it is possible to enable a config in MediaWiki so that anonymous edits are no longer assigned to IP addresses, and are instead assigned to temporary accounts. (This has not yet been enabled in WMF production as of the time of writing.)

How do temporary accounts work?

Highlights:

  • On editing without logging in, users will be warned that they will be given a temporary account
  • The temporary account name will follow a pattern defined in config
  • The temporary account user will not be able to set preferences
  • The same temporary account user may edit from more than one IP address
  • The temporary account persists for as long as the session (which is persisted via a cookie, the same as for registered user accounts)

Technical highlights (see also T300263 and related patches):

  • Temporary users are stored in the same way as regular users; they are recognised as temporary by checking their name
  • User::isAnon and mw.user.isAnon return false for a temporary account; a user's temporary status can be checked using User::isTemp
  • IP addresses are still stored in the CheckUser extension, but no longer via the username in core
What will be affected

IP Masking is likely to affect anything that:

  • Does something different for registered vs anonymous accounts
  • Identifies an anonymous user by checking the user name or ID
  • Does anything with a user's IP address, having got it from the username

IP Masking could affect anything that makes use of user accounts or usernames, in ways not anticipated above.

See T326759: Investigate: Which WMF deployed code might be affected by IP Masking? for a list of searches performed to find features that might be affected.

Where to test
  • Temporary accounts are enabled on https://de.wikipedia.beta.wmflabs.org
  • In LocalSettings.php, you can set $wgAutoCreateTempUser['enabled'] = true
  • On Patch Demo, you can check the "Enable temporary user account creation" checkbox

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
In Progress Niharika
OpenNone
OpenTchanders
Resolvedsbassett
ResolvedDaimona
OpenNone
OpenNone
OpenNone
OpenNone
OpenKStoller-WMF
Resolved TThoabala
ResolvedSBisson
ResolvedNikerabbit
ResolvedKrinkle
Resolveddaniel
OpenNone
ResolvedGehel
OpenCparle
OpenNone
DeclinedNone
ResolvedJdlrobson
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
ResolvedDaimona
OpenNone
OpenNone
Resolvedmatmarex
OpenNone
OpenDaimona
Resolvedmatmarex
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedEjegg
OpenNone
ResolvedMilimetric
DuplicateNone
OpenNone
OpenNone
OpenNone
ResolvedDaimona
Resolvedmatmarex
OpenNone
Resolvedpmiazga
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Hello @Tchanders @Niharika and thanks for identifying what fixes are needed in MediaWiki! This is great, but I'm still wondering how this will effect external tools heavily relied upon by volunteers (see previous questions: 1, 2). Examples include GUC and XTools GlobalContribs. These allow you to query by IP/IP range as well. With unregistered users now being given a temporary account and not actually being stored as an IP, we no longer can query for global contributions of the range that account belongs to, since the CheckUser data isn't available on Toolforge. Is there a solution in store for this problem?

Also is there any timeline as to when roughly we think the IP masking project will start taking effect on our cluster? This would help us fit the maintenance work into our annual planning, etc. For Community-Tech specifically, fixing our external tools (if needed) may be the larger time sink than fixing what we maintain in MediaWiki.

Technical question. $wgAutoCreateTempUser specifies a list of “actions” where a temporary account should be auto-created, with “edit” currently being the only supported action. Should other extensions that make edits, such as Wikibase (T328454), also use the “edit” action or a separate one (e.g. “wikibase-edit”)? (I think StructuredDiscussions/Flow T342831 and ProofreadPage T326940 are two other extensions where this question applies; there might be more I couldn’t think of.)

Technical question. $wgAutoCreateTempUser specifies a list of “actions” where a temporary account should be auto-created, with “edit” currently being the only supported action. Should other extensions that make edits, such as Wikibase (T328454), also use the “edit” action or a separate one (e.g. “wikibase-edit”)? (I think StructuredDiscussions/Flow T342831 and ProofreadPage T326940 are two other extensions where this question applies; there might be more I couldn’t think of.)

It should be acceptable to do either - use the edit action or add another action to the array.

Creation of the temporary account would still need to be implemented, however. At the moment, EditPage creates the temporary user, and just uses the config to check whether it should or not. Simply adding another action won't cause the temporary user to be created automatically.

We're looking into how we can improve temporary user creation for actions other than edit here: T336187: [S] Investigate: Creating temp user on non-EditPage actions.

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

MediaWiki Core Special Pages provide two helper methods requireLogin and requireNamedUser. Those methods check user type and redirect to UserLogin in case we have a user with the wrong type.

Idea:

Do not redirect temporary accounts to the login page - as those accounts are already logged in. They cannot log in, but without a registered account they cannot perform action they want. We should redirect them to create an account. Shout out to @Tgr for the idea.

Current flow:

SpecialPage::requireLogin() which would throw a UserNotLoggedIn exception if a user is an anon.
SpecialPage::requreNamedUser() throws a UserNotLoggedIn exception if a user is not named (anon or temp).
Then, the UserNotLoggedIn exception handles the redirect, always redirecting to UserLogin

Developers, based on their expectations use

  • $this->requireLogin() if SpecialPage is not available for anon
  • $this->requireNamedUser() if SpecialPage is available only for registered.
Proposed solutions:
Update in exception handling

Let the UserNotLoggedIn exception decide where to redirect users.
The UserNotLoggedIn can check if a user is anon. If yes leave the existing flow, otherwise if a user is a temporary user - redirect to the CreateAccount page.
This requires a small update here https://gerrit.wikimedia.org/g/mediawiki/core/+/ffecee7d38e17fe7f66eb1e3c6c8b3d25ee48efa/includes/exception/UserNotLoggedIn.php#102

This change would affect all places that are already updated logic. Although IMHO it's a good direction - it might be confusing for some. Also, devs wouldn't be able to specify different error messages for anon and temp user.

Selecting the route at the exception level would make the entire system consistent, as we have some extensions that use the UserNotLoggedIn exception directly.

Update the requiredNamedUser method

Update requiredNamedUser to trigger the UserNotLoggedIn with a different route. This way if in the code someone calls $this->requireNamedUser() it would redirect to CreateAccount both for anons and registered users. In case devs want to show different messages for anons and temp users, they would need to do something like this:

$this->requireLogin('message-for-anon'); // will redirect to UserLogin only if anon user
$this->requireNamedUser('message-for-temporary); // will redirect to CreateAccount

The difference between solution 1 is that we could specify the message, but in case we want to keep redirecting anons to login (preserve existing behaviour) we would need always to call both methods. From what I see, most of the already updated logic (https://codesearch.wmcloud.org/search/?q=requireNamedUser&files=&excludeFiles=&repos=) rewrite from $this->requireLogin() to $this->requiredNamedUser().

Most likely this would cause changing the signature of UserNotLoggedIn into

	public function __construct(
		$reasonMsg = 'exception-nologin-text',
		$titleMsg = 'exception-nologin',
		$params = [],
		$route = 'Userlogin'
	) {

Then the requireNamedUser would pass the Special page title, it could also pass different routes depending on anon/temp check which would make it work similarly to solution 1. But it wouldn't be global - eg the code that uses exception directly would still redirect only to the UserLogin page.

Additional method

Provide a third method in the SpecialPage that would take three params $reasonIfAnon, $reasonIfTemp, and $titleMsg and let that method pick the route. The requireLogin and requireNamedUser would stay the same - eg redirect to UserLogin.

The problem I see with this solution is accidental complexity - we will have two multiple methods to redirect anon/temp users - different methods will redirect to different URLs which makes system behaviour inconsistent.

Personally, I'm leaning towards solution 1, but I see solution 2 as a very solid candidate. The last solution doesn't make sense.

There is some direct usage of UserNotLoggedIn but it's minimal, so there would be some but not much benefit to handling at the exception level due to additional coverage.

The annoying part is going to be message handling. It would be nice to have separate messages for requiring login and requiring signup as they are quite different workflows. One can craft messages that work in both scenarios ("only registered users can X") but it feels awkward. So IMO there needs to be a way to specify two different messages. On the SpecialPage level that leaves two options:

$this->requireLogin( 'message-for-anon' ); // will redirect to UserLogin only if anon user
$this->requireNamedUser( 'message-for-temporary' ); // will redirect to CreateAccount

which has the drawback that current code already using requireNamedUser with a message that talks about logging in will have confusing results; and

$this->requireNamedUser([
    'anon' => 'message-for-anon',
    'temp' => 'message-for-temporary',
]);

which is uglier.
On the exception level, we could add a route parameter or create another extension but IMO there is no reason not to have UserNotLoggedIn choose the right method. We can either leave it to callers to do something like

if ( $user->isAnon() ) {
    throw new UserNotLoggedIn( 'message-for-anon' );
} elseif ( $user->isTemp() ) {
    throw new UserNotLoggedIn( 'message-for-temporary' );
}

or change the constuctor signature to

throw new UserNotLoggedIn( [
    'anon' => 'message-for-anon',
    'temp' => 'message-for-temporary',
] );

(and use B/C behavior when the first argument is a string).

I think an elegant interface is worth more than avoiding some slightly confusing messages that can be quickly fixed so I'd take the first (non-array) option for both.