Page MenuHomePhabricator

Title::checkUserBlock does not return an an error if User::isBlockedFrom() returns true and Block::prevents('edit') returns false
Closed, DuplicatePublic

Description

Problem
If an extension has implemented UserIsBlockedFrom and that extension returns true dynamically (i.e. not necessarily when a user has a block), but Block::prevents('edit') continues to return true (or the user does not have a block at all), then Title::checkUserBlock will return an empty array (indicating no errors):

$useSlave = ( $rigor !== 'secure' );
if ( ( $action == 'edit' || $action == 'create' )
	&& !$user->isBlockedFrom( $this, $useSlave )
) {
	// Don't block the user from editing their own talk page unless they've been
	// explicitly blocked from that too.
} elseif ( $user->isBlocked() && $user->getBlock()->prevents( $action ) !== false ) {
	// @todo FIXME: Pass the relevant context into this function.
	$errors[] = $user->getBlock()->getPermissionsError( RequestContext::getMain() );
}

return $errors;

Solution
If User::isBlockedFrom returns false then the check should always return an error. It should return the block if the user has a block, or it should return a standard error message if they do not.

Event Timeline

dbarratt created this task.Oct 1 2018, 4:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 1 2018, 4:59 PM
dbarratt updated the task description. (Show Details)Oct 1 2018, 5:01 PM
dbarratt added a project: Anti-Harassment.
Restricted Application added a subscriber: MGChecker. · View Herald TranscriptOct 1 2018, 5:01 PM
dbarratt updated the task description. (Show Details)Oct 1 2018, 5:02 PM
Anomie added a subscriber: Anomie.Oct 1 2018, 5:13 PM

If User::isBlockedFrom returns false then the check should always return an error.

I think you have something backwards in there. If User::isBlockedFrom returns false, doesn't that mean the user isn't blocked from the page and therefore it shouldn't return an error?

dbarratt closed this task as Invalid.Oct 1 2018, 5:22 PM

If User::isBlockedFrom returns false then the check should always return an error.

I think you have something backwards in there. If User::isBlockedFrom returns false, doesn't that mean the user isn't blocked from the page and therefore it shouldn't return an error?

oooo! you're right, then this isn't a problem. sorry, I was looking at it and had it backwards.

dbarratt reopened this task as Open.Oct 1 2018, 5:25 PM

Oh, no, I just wrote it wrong, but it is a bug, if it returns true (as in, you are blocked), but prevents() returns false then you'd get no error.

dbarratt updated the task description. (Show Details)Oct 1 2018, 5:25 PM

Dropping Anti-Harassment since this shouldn't be affected by partial blocks.

Anomie added a comment.EditedOct 1 2018, 6:28 PM

if it returns true (as in, you are blocked), but prevents() returns false then you'd get no error.

Remember that blocks don't have to prevent every action. It's legitimate for prevents( 'createaccount' ) to return false, for example, if the block doesn't have ipb_create_account set and we don't want to return an error for that case.