From 7582f0213e9e62f75d2931e5d59ba494c632917c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Mon, 1 May 2017 08:36:49 +0200 Subject: [PATCH] Do not disable password reset for blocks meant to force login Also remove resetpassword right (killed in I3ab5962d) from tests. Bug: T161860 Change-Id: Ic7e7e9b4ff7fe94001578a895962ef732b690384 --- includes/user/PasswordReset.php | 36 +++++++- .../includes/user/PasswordResetTest.php | 88 +++++++++++++------ 2 files changed, 95 insertions(+), 29 deletions(-) diff --git a/includes/user/PasswordReset.php b/includes/user/PasswordReset.php index 4ee256c495..dd16fb78ba 100644 --- a/includes/user/PasswordReset.php +++ b/includes/user/PasswordReset.php @@ -100,9 +100,10 @@ class PasswordReset implements LoggerAwareInterface { } elseif ( !$user->isAllowed( 'editmyprivateinfo' ) ) { // Maybe not all users have permission to change private data $status = StatusValue::newFatal( 'badaccess' ); - } elseif ( $user->isBlocked() ) { + } elseif ( $this->isBlocked( $user ) ) { // Maybe the user is blocked (check this here rather than relying on the parent - // method as we have a more specific error message to use here + // method as we have a more specific error message to use here and we want to + // ignore some types of blocks) $status = StatusValue::newFatal( 'blocked-mailpassword' ); } @@ -250,6 +251,37 @@ class PasswordReset implements LoggerAwareInterface { return StatusValue::newGood( $passwords ); } + /** + * Check whether the user is blocked. + * Ignores certain types of system blocks that are only meant to force users to log in. + * @param User $user + * @return bool + * @since 1.30 + */ + protected function isBlocked( User $user ) { + $block = $user->getBlock() ?: $user->getGlobalBlock(); + if ( !$block ) { + return false; + } + $type = $block->getSystemBlockType(); + if ( in_array( $type, [ null, 'global-block' ], true ) ) { + // Normal block. Maybe it was meant for someone else and the user just needs to log in; + // or maybe it was issued specifically to prevent some IP from messing with password + // reset? Go out on a limb and use the registration allowed flag to decide. + return $block->prevents( 'createaccount' ); + } elseif ( $type === 'proxy' ) { + // we disallow actions through proxy even if the user is logged in + // so it makes sense to disallow password resets as well + return true; + } elseif ( in_array( $type, [ 'dnsbl', 'wgSoftBlockRanges' ], true ) ) { + // these are just meant to force login so let's not prevent that + return false; + } else { + // some extension - we'll have to guess + return true; + } + } + /** * @param string $email * @return User[] diff --git a/tests/phpunit/includes/user/PasswordResetTest.php b/tests/phpunit/includes/user/PasswordResetTest.php index 3363bca524..53f02df69c 100644 --- a/tests/phpunit/includes/user/PasswordResetTest.php +++ b/tests/phpunit/includes/user/PasswordResetTest.php @@ -10,8 +10,7 @@ class PasswordResetTest extends PHPUnit_Framework_TestCase { * @dataProvider provideIsAllowed */ public function testIsAllowed( $passwordResetRoutes, $enableEmail, - $allowsAuthenticationDataChange, $canEditPrivate, $canSeePassword, - $userIsBlocked, $isAllowed + $allowsAuthenticationDataChange, $canEditPrivate, $block, $globalBlock, $isAllowed ) { $config = new HashConfig( [ 'PasswordResetRoutes' => $passwordResetRoutes, @@ -25,13 +24,12 @@ class PasswordResetTest extends PHPUnit_Framework_TestCase { $user = $this->getMockBuilder( User::class )->getMock(); $user->expects( $this->any() )->method( 'getName' )->willReturn( 'Foo' ); - $user->expects( $this->any() )->method( 'isBlocked' )->willReturn( $userIsBlocked ); + $user->expects( $this->any() )->method( 'getBlock' )->willReturn( $block ); + $user->expects( $this->any() )->method( 'getGlobalBlock' )->willReturn( $globalBlock ); $user->expects( $this->any() )->method( 'isAllowed' ) - ->will( $this->returnCallback( function ( $perm ) use ( $canEditPrivate, $canSeePassword ) { + ->will( $this->returnCallback( function ( $perm ) use ( $canEditPrivate ) { if ( $perm === 'editmyprivateinfo' ) { return $canEditPrivate; - } elseif ( $perm === 'passwordreset' ) { - return $canSeePassword; } else { $this->fail( 'Unexpected permission check' ); } @@ -44,67 +42,103 @@ class PasswordResetTest extends PHPUnit_Framework_TestCase { public function provideIsAllowed() { return [ - [ + 'no routes' => [ 'passwordResetRoutes' => [], 'enableEmail' => true, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => true, - 'canSeePassword' => true, - 'userIsBlocked' => false, + 'block' => null, + 'globalBlock' => null, 'isAllowed' => false, ], - [ + 'email disabled' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => false, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => true, - 'canSeePassword' => true, - 'userIsBlocked' => false, + 'block' => null, + 'globalBlock' => null, 'isAllowed' => false, ], - [ + 'auth data change disabled' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, 'allowsAuthenticationDataChange' => false, 'canEditPrivate' => true, - 'canSeePassword' => true, - 'userIsBlocked' => false, + 'block' => null, + 'globalBlock' => null, 'isAllowed' => false, ], - [ + 'cannot edit private data' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => false, - 'canSeePassword' => true, - 'userIsBlocked' => false, + 'block' => null, + 'globalBlock' => null, 'isAllowed' => false, ], - [ + 'blocked with account creation disabled' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => true, - 'canSeePassword' => true, - 'userIsBlocked' => true, + 'block' => new Block( [ 'createAccount' => true ] ), + 'globalBlock' => null, 'isAllowed' => false, ], - [ + 'blocked w/o account creation disabled' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => true, - 'canSeePassword' => false, - 'userIsBlocked' => false, + 'block' => new Block( [] ), + 'globalBlock' => null, 'isAllowed' => true, ], - [ + 'using blocked proxy' => [ 'passwordResetRoutes' => [ 'username' => true ], 'enableEmail' => true, 'allowsAuthenticationDataChange' => true, 'canEditPrivate' => true, - 'canSeePassword' => true, - 'userIsBlocked' => false, + 'block' => new Block( [ 'systemBlock' => 'proxy' ] ), + 'globalBlock' => null, + 'isAllowed' => false, + ], + 'globally blocked with account creation disabled' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => null, + 'globalBlock' => new Block( [ 'systemBlock' => 'global-block', 'createAccount' => true ] ), + 'isAllowed' => false, + ], + 'globally blocked with account creation not disabled' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => null, + 'globalBlock' => new Block( [ 'systemBlock' => 'global-block', 'createAccount' => false ] ), + 'isAllowed' => true, + ], + 'blocked via wgSoftBlockRanges' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => new Block( [ 'systemBlock' => 'wgSoftBlockRanges', 'anonOnly' => true ] ), + 'globalBlock' => null, + 'isAllowed' => true, + ], + 'all OK' => [ + 'passwordResetRoutes' => [ 'username' => true ], + 'enableEmail' => true, + 'allowsAuthenticationDataChange' => true, + 'canEditPrivate' => true, + 'block' => null, + 'globalBlock' => null, 'isAllowed' => true, ], ]; -- 2.20.1