Page MenuHomePhabricator

ipblock-exempt does not allow account creation when blocked
Open, MediumPublic

Description

When an IP is blocked, the underlying user cannot create accounts if they have the ipblock-exempt right (incl sysops)

From my test-wiki (just cloned from gerrit):

https://i.imgur.com/3DKnyq1.png - Blocked w/o sysop


https://i.imgur.com/aDeEwmR.png - Blocked with sysop

https://i.imgur.com/iaKvsk4.png - Block options

Only block option was account creation disabled. Tested IPv4 and IPv6 blocks. Tested range and directed blocks.

More info: https://en.wikipedia.org/wiki/Wikipedia:Administrators%27_noticeboard/Archive297#All_user_accounts_are_restricted_from_account_creation_while_doing_so_from_a_blocked_IP_or_range

Event Timeline

SQL created this task.Mar 10 2018, 4:37 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 10 2018, 4:37 AM
SQL updated the task description. (Show Details)Mar 10 2018, 4:44 AM
1997kB added a subscriber: 1997kB.Mar 10 2018, 7:34 AM
JJMC89 added a subscriber: JJMC89.Mar 10 2018, 9:08 AM
Nick added a subscriber: Nick.Mar 12 2018, 12:19 PM
SQL updated the task description. (Show Details)Mar 18 2018, 3:49 PM
1997kB triaged this task as Unbreak Now! priority.May 18 2018, 1:34 PM
Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptMay 18 2018, 1:35 PM

Is anyone working on it?

No.

Is this a regression? Why is it "unbreak now"?

matmarex updated the task description. (Show Details)May 18 2018, 3:54 PM
matmarex lowered the priority of this task from Unbreak Now! to Needs Triage.May 18 2018, 4:25 PM

It seems that AuthManager checks the user rights too stringently (when calling isBlockedFromCreateAccount()). In addition to checking whether the account creator can create the new account (AuthManager::checkAccountCreatePermissions()), it also checks whether the new account can create itself (CheckBlocksSecondaryAuthenticationProvider::testUserForCreation()). I think that is incorrect, but perhaps there is a reason for this (or perhaps it has always been this way, and you're in fact requesting a new feature).

matmarex removed a subscriber: matmarex.May 18 2018, 4:31 PM
BRPever removed a subscriber: BRPever.

Today, I was not able to reproduce it on https://test.wikipedia.org (1.33.0-wmf.20 (rMWf929e2a50696) 23:04, 6 March 2019). I tried a non-anonymous block of an IP range and both an anonymous and a non-anonymous block for a single IP.

But, I can reproduce it on https://en.wikipedia.beta.wmflabs.org (1.33.0-alpha (278ac40) 23:37, 10 March 2019).

Work is ongoing for partial blocks, but I don't know if that is related.

Izno added a subscriber: Izno.May 5 2019, 8:29 PM

Per T15611: Blocking account creation on an IP address should apply to logged in users too it is intentional that blocks that disable createaccount should have that applied to logged in users; however, I don't see ipblock-exempt mentioned. r41150 was by @aaron with follow-up by @tstarling.

mediawiki/includes/user/User.php
	/**
	 * Get whether the user is explicitly blocked from account creation.
	 * @return bool|AbstractBlock
	 */
	public function isBlockedFromCreateAccount() {
		$this->getBlockedStatus();
		if ( $this->mBlock && $this->mBlock->appliesToRight( 'createaccount' ) ) {
			return $this->mBlock;
		}
		# T15611: if the IP address the user is trying to create an account from is
		# blocked with createaccount disabled, prevent new account creation there even
		# when the user is logged in
		if ( $this->mBlockedFromCreateAccount === false && !$this->isAllowed( 'ipblock-exempt' ) ) {
			$this->mBlockedFromCreateAccount = DatabaseBlock::newFromTarget(
				null, $this->getRequest()->getIP()
			);
		}
		return $this->mBlockedFromCreateAccount instanceof AbstractBlock
			&& $this->mBlockedFromCreateAccount->appliesToRight( 'createaccount' )
			? $this->mBlockedFromCreateAccount
			: false;
	}

(That's all the further that I'm able to get investigating this.)

I have just encounted this issue when trying to create an account for a trainee at the university where I am Wikimedian in Residence, via en.Wikipedia. I have "IP Block exempt" and "account creator" rights on that project.

dom_walden added a comment.EditedOct 28 2019, 3:42 PM

It should be noted that DatabaseBlock::newFromTarget (used here) chooses the most specific IP block. Blocking an IP range from creating accounts might not have the desired affect if there are more specific IP blocks which allow account creation. This is only in the case of wanting to block admins from creating accounts, which we may not want to do in the first place (as mentioned above).

Also, for testing purposes, I believe the corresponding API call that checks whether a particular $username can be created is: w/api.php?action=query&list=users&usprop=cancreate&ususers=$username. This also exhibits the same bug/behaviour as in the description.

Wondering if there's any progress on this? I see that blocking logged-in users from creating accounts on account-creation-blocked IPs is intentional per T15611, and I agree, but surely that was not also meant to block ipblock-exempt users? ipblock-exempt should exempt users from ALL IP block settings - if we didn't want to grant users full exemption from IP blocks we would not give them the permission.

Ammarpad triaged this task as High priority.

Change 555282 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Fix AuthManager::canCreateAccount method

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

Ammarpad removed Ammarpad as the assignee of this task.Dec 26 2019, 9:02 AM
Ammarpad raised the priority of this task from High to Needs Triage.
Ammarpad removed a project: Patch-For-Review.

I may try this again but not now

Cannot reproduce. However, normal users (no IPBE) cannot create accounts, even with softblocks.

Aha...with sysop, the form shows, but doesn't allow me to create the account anyway - even with my steward account.

I've digged into this a little. AuthManager::canCreateAccount calls CheckBlocksSecondaryAuthenticationProvider with the "to be created" user, which doesn't have IPBE. I think CheckBlocksSecondaryAuthenticationProvider shouldn't be called at all, given the special page itself checks for permissions :-).

Given this is a core issue, calling CPT to help to decide about the best fix.

Anomie added a subscriber: Anomie.

TL;DR: Removing CheckBlocksSecondaryAuthenticationProvider::testUserForCreation() and at the same time adding a call to $user->isBlockedFromCreateAccount() into AuthManager::autoCreateUser() seems like the thing to do here.

I've digged into this a little. AuthManager::canCreateAccount calls CheckBlocksSecondaryAuthenticationProvider with the "to be created" user, which doesn't have IPBE. I think CheckBlocksSecondaryAuthenticationProvider shouldn't be called at all,

As stated, that doesn't really make sense. What I think you mean is something along the lines of "CheckBlocksSecondaryAuthenticationProvider::testUserForCreation() shouldn't call $user->isBlockedFromCreateAccount()", which would reduce it to the no-op implementation in its parent class.

It does seem that always checking ->isBlockedFromCreateAccount() on the to-be-created user was a bug. Looking back at rMWd245bd25aef1: Add AuthManager, originally that was only done on auto-creation (where it does make sense, and we need to make sure that continues to happen). Then PS90 accidentally made it apply always, and PS101 further obscured the issue. While we could go back to having CheckBlocksSecondaryAuthenticationProvider only do the test on autocreation, probably when PS101 introduced checkAccountCreatePermissions() it should have just added the IP-block check directly to AuthManager::autoCreateUser() too to match.

given the special page itself checks for permissions :-).

AuthManager::checkAccountCreatePermissions() is what checks the relevant permission. Which is good, because just the special page checking wouldn't work for account creations via the Action API.

The special page may check too, in order to provide a better error message than AuthManager generates for common cases.

nnikkhoui triaged this task as Medium priority.
Pchelolo removed nnikkhoui as the assignee of this task.Apr 15 2020, 4:26 PM
Pchelolo added a subscriber: nnikkhoui.
Ammarpad claimed this task.Fri, May 22, 1:49 PM