Page MenuHomePhabricator

not possible to set email when email blocked
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Have an account without email address
  2. Use an admin account to block the user. in this case, I used a partial block without specified pages, but with email and account creation block, so the user can edit. This is the logentry:

(Block log); 19:09 Luke081515 talk contribs blocked Luke081515.2 talk contribs from specified non-editing actions with an expiration time of 3 hours (account creation blocked, email disabled) ‎(test)

  1. Go now to the preferences of the blocked user and try to add an email
  2. You receive this message:

There are no multiple blocks against this IP, only my autoblock.

While sending an email should not be allowed of course, it should be still possible for the user to change his email settings.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 15 2019, 7:18 PM
DannyS712 added a subscriber: DannyS712.EditedJun 16 2019, 7:08 AM

I tried tracking down where this is set - in User.php, there is:

public function setEmail( $str ) {
	$this->load();
	if ( $str == $this->mEmail ) {
		return;
	}
	$this->invalidateEmail();
	$this->mEmail = $str;
	Hooks::run( 'UserSetEmail', [ $this, &$this->mEmail ] );
}

as well as, separately in the same file,

public function isBlockedFromEmailuser() {
	$this->getBlockedStatus();
	return $this->mBlock && $this->mBlock->appliesToRight( 'sendemail' );
}

but the two don't appear to interact in an obvious way

JJMC89 added a subscriber: JJMC89.

Tagging Anti-Harassment since this could be related to partial blocks or other block refactoring the team has done.

Restricted Application added a subscriber: MGChecker. · View Herald TranscriptJun 16 2019, 7:40 AM

I think this has to do with the confirmation email that is sent when you change your email. I suppose someone could use the conformation email to harass you, though if the other person already has your email address, there's nothing stoping them from using a bazillion other systems to send you an email.

Thanks for reporting. It looks like it has been this way for a while (e.g. rolling back to 1.33.0, before the refactoring). @Niharika @SPoore would you have any concerns about allowing a user who is blocked from sending email to change their email address?

The multiple block message is a separate problem, filed as T225919. (It shouldn't be due to the autoblock, but could be due to e.g. tracking the block in the cookie.)

@Tchanders I think that it's fine to allow users to change/add an email address even if they are blocked. At least then they would be able to receive emails from other users and have a way to communicate about the block and/or receive guidance.

@dbarratt I think you're right:

public function setEmailWithConfirmation( $str ) {
...
		if ( $wgEmailAuthentication && $type === 'changed' ) {
			// Send the user an email notifying the user of the change in registered
			// email address on their previous email address
			$change = $str != '' ? 'changed' : 'removed';
			$notificationResult = $this->sendMail(
				wfMessage( 'notificationemail_subject_' . $change )->text(),
				wfMessage( 'notificationemail_body_' . $change,
					$this->getRequest()->getIP(),
					$this->getName(),
					$str )->text()
			);
		}
		$this->setEmail( $str );
		if ( $str !== '' && $wgEmailAuthentication ) {
			// Send a confirmation request to the new address if needed
			$result = $this->sendConfirmationMail( $type );

...

is there any way to override the block with an optional parameter (like ...sendMail(..., 'overrideBlock' => true)) so that in instances like this the block can be ignored programmatically?

@Tchanders I think that it's fine to allow users to change/add an email address even if they are blocked.

The "risk" is that someone might use the email confirmation system to harass someone else with a bunch of emails (which would otherwise be legitimate) and we wouldn't have a way to stop them from doing that. Of course you would have to know the user's email in order to carry this out, but if you did know it, you could abuse our email systems.

@Tchanders I think that it's fine to allow users to change/add an email address even if they are blocked.

The "risk" is that someone might use the email confirmation system to harass someone else with a bunch of emails (which would otherwise be legitimate) and we wouldn't have a way to stop them from doing that. Of course you would have to know the user's email in order to carry this out, but if you did know it, you could abuse our email systems.

I think that risk is pretty low considering people can be abused with password reset email spam (T145952: Reduce password reset spam) with just the username. It's not too frequent so I suspect the risk in our case is much lower.

See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/514755 for the patch that introduced this. Affects only "email disabled" blocks, through.

The patched linked above added

if ( $user->isBlockedFromEmailuser() ) {
	throw new UserBlockedError( $user->getBlock() );
}

which means that it would be fairly simple to resolve this task - simple remove this error. The question becomes, however, whether or not users who are blocked with email disabled should be able to change their own email. Personally, I would say that they should be able to change their emails, since if it is being used for harassment the account can just be globally locked, especially since accounts' emails are global and thus the "email user" feature can be used on any other wiki.

Tagging with Security, since this is de-facto request for reverting a security patch.

Reedy triaged this task as Low priority.Jul 5 2019, 6:33 PM

@DannyS712 @Urbanecm - IMO there's very much a cost/benefit question here for the edge case described within the task description, but leaving the ping limiter in place from r514755 is probably sufficient for now. I think the Security-Team would be ok with a partial revert of r514755 where:

if ( $user->isBlockedFromEmailuser() ) {
	throw new UserBlockedError( $user->getBlock() );
}

was removed. But if we see similar abuse as to what was reported in T209794, we might have to add the check again.

Change 521299 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Allow setting email even when blocked from sending emails

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

@sbassett I have uploaded a patch that removes the lines, per your comment above

sbassett added a subscriber: Reedy.Jul 8 2019, 3:33 PM

@DannyS712 - looks good, thanks. I +1'd for now. I'd like @Reedy to have a look as well, but I believe he's on vacation. Happy to +2 by tomorrow.

DannyS712 claimed this task.Jul 8 2019, 3:33 PM
DannyS712 moved this task from Unsorted to Awaiting review and deployment on the User-DannyS712 board.

@DannyS712 - Sorry for the delay, I'd been working on another incident. And it potentially had some implications for this revert, so I wanted to think a bit more on it. For now, I think this is still fine to revert as long as we keep an eye on it. I've gone ahead and +2'd the patch.

Change 521299 merged by jenkins-bot:
[mediawiki/core@master] Allow setting email even when blocked from sending emails

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

DannyS712 closed this task as Resolved.Aug 2 2019, 4:48 PM

Change was merged and deployed