Page MenuHomePhabricator

Title::checkUserBlock should call User::isBlockedFrom for every action that requires unblock
Closed, ResolvedPublic

Description

Problem
in Title::checkUserBlock the title is not checked for a block if the action is not create or edit. This means other actions like move-subpages returns an error for partially blocked users.

$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;

This will return an error because $user->getBlock()->prevents( $action ) will return null

Solution
The code above should be replaced with:

$block = $user->getBlock( $useReplica );

// The block may explicitly allow an action (like "read" or "upload").
if ( $block && $block->prevents( $action ) === false ) {
  return $errors;
}

// Determine if the user is blocked from this action on this page.
try {
  // @todo FIXME: Pass the relevant context into this function.
  $action = Action::factory( $action, WikiPage::factory( $this ), RequestContext::getMain() );
} catch ( Exception $e ) {
  $action = null;
}

// If no action object is returned, assume that the action requires unblock
// which is the default.
if ( !$action || $action->requiresUnblock() ) {
  if ( $user->isBlockedFrom( $this, $useReplica ) ) {
    // @todo FIXME: Pass the relevant context into this function.
    $errors[] = $block
      ? $block->getPermissionsError( RequestContext::getMain() )
      : [ 'actionblockedtext' ];
  }
}

Related Objects

Event Timeline

dbarratt created this task.Nov 6 2018, 3:47 PM
Restricted Application added subscribers: MGChecker, Aklapper. · View Herald TranscriptNov 6 2018, 3:47 PM
dbarratt updated the task description. (Show Details)Nov 6 2018, 3:50 PM
dbarratt updated the task description. (Show Details)

Change 471993 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/core@master] Title::checkUserBlock should call User::isBlockedFrom for every action

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

dbarratt updated the task description. (Show Details)Nov 6 2018, 4:07 PM
Oshwah added a subscriber: Oshwah.Nov 6 2018, 4:56 PM
TBolliger triaged this task as High priority.Nov 7 2018, 5:20 PM
dbarratt claimed this task.Nov 8 2018, 4:40 PM

Change 471993 merged by jenkins-bot:
[mediawiki/core@master] Title::checkUserBlock should call User::isBlockedFrom for every action

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

dbarratt closed this task as Resolved.Nov 19 2018, 11:30 PM
dbarratt moved this task from Review to Done on the Anti-Harassment (AHT Sprint 33) board.

Are we planning to reinstate this once T211048 is resolved?

Are we planning to reinstate this once T211048 is resolved?

I imagine so — since we're fixing the side-effects and not altering this strategy. This patch fixes some important release blockers.

Change 478056 had a related patch set uploaded (by Dbarratt; owner: Tchanders):
[mediawiki/core@master] Revert "Revert "Title::checkUserBlock should call User::isBlockedFrom for every action""

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

dbarratt updated the task description. (Show Details)Jan 4 2019, 12:38 AM
dbarratt updated the task description. (Show Details)Jan 4 2019, 3:53 PM
dbarratt added a comment.EditedJan 4 2019, 9:27 PM

I found a small problem in the patch. Since Title::userCan() and Title::getUserPermissionsErrors() expect the result of Action::getRestriction() not Action::getName() and since those two values do not have to be the same, constructing the action by name, rather than by restriction could have unexpected (overly restrictive) results.

The actions (in core) where this is the case are:

  • MarkpatrolledAction Name: markpatrolled Restriction: patrol
  • RevertAction Name: revert Restriction: upload

Since actions can only be loaded by name, and restrictions are what get passed into the method, this means these actions (or any other in extensions) will have the default value of true (i.e. a partial block will block the action).

There isn't a way to construct actions by a restriction. I was thinking we could add the actions to the service container, but that doesn't support tagging (or any other categorization mechanism) T212976. We could deprecate Action::getRestriction() and make a new, static method. That way we could loop through all of the action classes and create a map of restrictions to actions. If we are going to do that, we might as well do the same thing with Action::requiresUnblock() to avoid having to construct it at all. Or we could accept that when they do not match, that they will require unblocking. 😕

dbarratt added a subscriber: Tgr.EditedJan 4 2019, 11:26 PM

As @Tgr pointed out on the patch, this change does introduce a small inverted code flow.

The reason for this is because Action::checkCanExecute() makes a call to Title::getUserPermissionsErrors(). Then (with this patch) we are instantiating a new instance of the same Action that called this method in order to determine if the block should or should not affect the action that is being checked.

A "simple" solution to this problem would be to deprecated Action::requiresUnblock() and create a new static method. This way, metadata on the Action can be retrieved without having to instantiate a new instance of the action.

Alternatively, it would be better to resolve T212341 and create some sort of ActionPermission class that would contain the authorization checks for a particular action.

dbarratt renamed this task from Title::checkUserBlock should call User::isBlockedFrom for every action to Title::checkUserBlock should call User::isBlockedFrom for every action that requires unblock.Jan 5 2019, 12:18 AM

Change 478056 merged by jenkins-bot:
[mediawiki/core@master] Revert "Revert "Title::checkUserBlock should call User::isBlockedFrom for every action""

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

dbarratt closed this task as Resolved.Jan 8 2019, 9:16 PM
dbarratt moved this task from Review to Done on the Anti-Harassment (Alef — א) board.