diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index f373adbded9..2896e44c1b2 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -338,6 +338,8 @@ because of Phabricator reports. * wfLocalFile(), deprecated since 1.34, now emits deprecation warnings. * wfFindFile(), deprecated since 1.34, now emits deprecation warnings. * User::getRights(), deprecated since 1.34, now emits deprecation warnings. +* User::changeableGroups and ::changeableByGroup were deprecated, use + corresponding methods in UserGroupManager instead. * … === Other changes in 1.37 === diff --git a/includes/user/User.php b/includes/user/User.php index 25a47aa9f7c..221ec671f60 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -4067,72 +4067,12 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact * 'remove' => [ removablegroups ], * 'add-self' => [ addablegroups to self ], * 'remove-self' => [ removable groups from self ] ] - * @suppress PhanTypeComparisonFromArray False positives with $wgGroupsAddToSelf + * @deprecated since 1.37 Use UserGroupManager::getGroupsChangeableByGroup instead. */ public static function changeableByGroup( $group ) { - global $wgAddGroups, $wgRemoveGroups, $wgGroupsAddToSelf, $wgGroupsRemoveFromSelf; - - $groups = [ - 'add' => [], - 'remove' => [], - 'add-self' => [], - 'remove-self' => [] - ]; - - if ( empty( $wgAddGroups[$group] ) ) { - // Don't add anything to $groups - } elseif ( $wgAddGroups[$group] === true ) { - // You get everything - $groups['add'] = self::getAllGroups(); - } elseif ( is_array( $wgAddGroups[$group] ) ) { - $groups['add'] = $wgAddGroups[$group]; - } - - // Same thing for remove - if ( empty( $wgRemoveGroups[$group] ) ) { - // Do nothing - } elseif ( $wgRemoveGroups[$group] === true ) { - $groups['remove'] = self::getAllGroups(); - } elseif ( is_array( $wgRemoveGroups[$group] ) ) { - $groups['remove'] = $wgRemoveGroups[$group]; - } - - // Re-map numeric keys of AddToSelf/RemoveFromSelf to the 'user' key for backwards compatibility - if ( empty( $wgGroupsAddToSelf['user'] ) || $wgGroupsAddToSelf['user'] !== true ) { - foreach ( $wgGroupsAddToSelf as $key => $value ) { - if ( is_int( $key ) ) { - $wgGroupsAddToSelf['user'][] = $value; - } - } - } - - if ( empty( $wgGroupsRemoveFromSelf['user'] ) || $wgGroupsRemoveFromSelf['user'] !== true ) { - foreach ( $wgGroupsRemoveFromSelf as $key => $value ) { - if ( is_int( $key ) ) { - $wgGroupsRemoveFromSelf['user'][] = $value; - } - } - } - - // Now figure out what groups the user can add to him/herself - if ( empty( $wgGroupsAddToSelf[$group] ) ) { - // Do nothing - } elseif ( $wgGroupsAddToSelf[$group] === true ) { - // No idea WHY this would be used, but it's there - $groups['add-self'] = self::getAllGroups(); - } elseif ( is_array( $wgGroupsAddToSelf[$group] ) ) { - $groups['add-self'] = $wgGroupsAddToSelf[$group]; - } - - if ( empty( $wgGroupsRemoveFromSelf[$group] ) ) { - // Do nothing - } elseif ( $wgGroupsRemoveFromSelf[$group] === true ) { - $groups['remove-self'] = self::getAllGroups(); - } elseif ( is_array( $wgGroupsRemoveFromSelf[$group] ) ) { - $groups['remove-self'] = $wgGroupsRemoveFromSelf[$group]; - } - - return $groups; + return MediaWikiServices::getInstance() + ->getUserGroupManager() + ->getGroupsChangeableByGroup( $group ); } /** @@ -4141,41 +4081,12 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact * 'remove' => [ removablegroups ], * 'add-self' => [ addablegroups to self ], * 'remove-self' => [ removable groups from self ] ] + * @deprecated since 1.37 Use UserGroupManager::getGroupsChangeableBy instead. */ public function changeableGroups() { - if ( $this->isAllowed( 'userrights' ) ) { - // This group gives the right to modify everything (reverse- - // compatibility with old "userrights lets you change - // everything") - // Using array_merge to make the groups reindexed - $all = array_merge( self::getAllGroups() ); - return [ - 'add' => $all, - 'remove' => $all, - 'add-self' => [], - 'remove-self' => [] - ]; - } - - // Okay, it's not so simple, we will have to go through the arrays - $groups = [ - 'add' => [], - 'remove' => [], - 'add-self' => [], - 'remove-self' => [] - ]; - $addergroups = $this->getEffectiveGroups(); - - foreach ( $addergroups as $addergroup ) { - $groups = array_merge_recursive( - $groups, $this->changeableByGroup( $addergroup ) - ); - $groups['add'] = array_unique( $groups['add'] ); - $groups['remove'] = array_unique( $groups['remove'] ); - $groups['add-self'] = array_unique( $groups['add-self'] ); - $groups['remove-self'] = array_unique( $groups['remove-self'] ); - } - return $groups; + return MediaWikiServices::getInstance() + ->getUserGroupManager() + ->getGroupsChangeableBy( $this ); } /** diff --git a/includes/user/UserGroupManager.php b/includes/user/UserGroupManager.php index f720da139a2..341b9866028 100644 --- a/includes/user/UserGroupManager.php +++ b/includes/user/UserGroupManager.php @@ -30,6 +30,7 @@ use ManualLogEntry; use MediaWiki\Config\ServiceOptions; use MediaWiki\HookContainer\HookContainer; use MediaWiki\HookContainer\HookRunner; +use MediaWiki\Permissions\Authority; use MediaWiki\Permissions\GroupPermissionsLookup; use Psr\Log\LoggerInterface; use ReadOnlyMode; @@ -54,13 +55,17 @@ class UserGroupManager implements IDBAccessObject { * @internal For use by ServiceWiring */ public const CONSTRUCTOR_OPTIONS = [ + 'AddGroups', 'Autopromote', 'AutopromoteOnce', 'AutopromoteOnceLogInRC', 'EmailAuthentication', 'ImplicitGroups', 'GroupPermissions', + 'GroupsAddToSelf', + 'GroupsRemoveFromSelf', 'RevokePermissions', + 'RemoveGroups', ]; /** @var ServiceOptions */ @@ -973,6 +978,99 @@ class UserGroupManager implements IDBAccessObject { return $purgedRows; } + /** + * @param array $config + * @param string $group + * @return string[] + */ + private function expandChangeableGroupConfig( array $config, string $group ): array { + if ( empty( $config[$group] ) ) { + return []; + } elseif ( $config[$group] === true ) { + // You get everything + return $this->listAllGroups(); + } elseif ( is_array( $config[$group] ) ) { + return $config[$group]; + } + return []; + } + + /** + * Returns an array of the groups that a particular group can add/remove. + * + * @since 1.37 + * @param string $group The group to check for whether it can add/remove + * @return array [ + * 'add' => [ addablegroups ], + * 'remove' => [ removablegroups ], + * 'add-self' => [ addablegroups to self ], + * 'remove-self' => [ removable groups from self ] ] + */ + public function getGroupsChangeableByGroup( string $group ): array { + return [ + 'add' => $this->expandChangeableGroupConfig( + $this->options->get( 'AddGroups' ), $group + ), + 'remove' => $this->expandChangeableGroupConfig( + $this->options->get( 'RemoveGroups' ), $group + ), + 'add-self' => $this->expandChangeableGroupConfig( + $this->options->get( 'GroupsAddToSelf' ), $group + ), + 'remove-self' => $this->expandChangeableGroupConfig( + $this->options->get( 'GroupsRemoveFromSelf' ), $group + ), + ]; + } + + /** + * Returns an array of groups that this $actor can add and remove. + * + * @since 1.37 + * @param Authority $authority + * @return array [ + * 'add' => [ addablegroups ], + * 'remove' => [ removablegroups ], + * 'add-self' => [ addablegroups to self ], + * 'remove-self' => [ removable groups from self ] + * ] + */ + public function getGroupsChangeableBy( Authority $authority ): array { + if ( $authority->isAllowed( 'userrights' ) ) { + // This group gives the right to modify everything (reverse- + // compatibility with old "userrights lets you change + // everything") + // Using array_merge to make the groups reindexed + $all = array_merge( $this->listAllGroups() ); + return [ + 'add' => $all, + 'remove' => $all, + 'add-self' => [], + 'remove-self' => [] + ]; + } + + // Okay, it's not so simple, we will have to go through the arrays + $groups = [ + 'add' => [], + 'remove' => [], + 'add-self' => [], + 'remove-self' => [] + ]; + $actorGroups = $this->getUserEffectiveGroups( $authority->getUser() ); + + foreach ( $actorGroups as $actorGroup ) { + $groups = array_merge_recursive( + $groups, $this->getGroupsChangeableByGroup( $actorGroup ) + ); + $groups['add'] = array_unique( $groups['add'] ); + $groups['remove'] = array_unique( $groups['remove'] ); + $groups['add-self'] = array_unique( $groups['add-self'] ); + $groups['remove-self'] = array_unique( $groups['remove-self'] ); + } + return $groups; + } + /** * Cleans cached group memberships for a given user * diff --git a/tests/phpunit/includes/user/UserGroupManagerTest.php b/tests/phpunit/includes/user/UserGroupManagerTest.php index 5d8bb4a922f..4a1aadf4650 100644 --- a/tests/phpunit/includes/user/UserGroupManagerTest.php +++ b/tests/phpunit/includes/user/UserGroupManagerTest.php @@ -26,6 +26,7 @@ use LogEntryBase; use MediaWiki\Block\DatabaseBlock; use MediaWiki\Config\ServiceOptions; use MediaWiki\MediaWikiServices; +use MediaWiki\Permissions\SimpleAuthority; use MediaWiki\Session\PHPSessionHandler; use MediaWiki\Session\SessionManager; use MediaWiki\User\UserEditTracker; @@ -68,6 +69,7 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase { UserGroupManager::CONSTRUCTOR_OPTIONS, $configOverrides, [ + 'AddGroups' => [], 'Autopromote' => [ 'autoconfirmed' => [ APCOND_EDITCOUNT, 0 ] ], @@ -77,7 +79,10 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase { 'runtest' => true, ] ], + 'GroupsAddToSelf' => [], + 'GroupsRemoveFromSelf' => [], 'ImplicitGroups' => [ '*', 'user', 'autoconfirmed' ], + 'RemoveGroups' => [], 'RevokePermissions' => [], ], $services->getMainConfig() @@ -1053,4 +1058,102 @@ class UserGroupManagerTest extends MediaWikiIntegrationTestCase { ] ] ); } + + private const CHANGEABLE_GROUPS_TEST_CONFIG = [ + 'GroupPermissions' => [], + 'AddGroups' => [ + 'sysop' => [ 'rollback' ], + 'bureaucrat' => [ 'sysop', 'bureaucrat' ], + ], + 'RemoveGroups' => [ + 'sysop' => [ 'rollback' ], + 'bureaucrat' => [ 'sysop' ], + ], + 'GroupsAddToSelf' => [ + 'sysop' => [ 'flood' ], + ], + 'GroupsRemoveFromSelf' => [ + 'flood' => [ 'flood' ], + ], + ]; + + private function assertGroupsEquals( array $expected, array $actual ) { + // assertArrayEquals can compare without requiring the same order, + // but the elements of an array are still required to be in the same order, + // so just compare each element + $this->assertArrayEquals( $expected['add'], $actual['add'], 'Add must match' ); + $this->assertArrayEquals( $expected['remove'], $actual['remove'], 'Remove must match' ); + $this->assertArrayEquals( $expected['add-self'], $actual['add-self'], 'Add-self must match' ); + $this->assertArrayEquals( $expected['remove-self'], $actual['remove-self'], 'Remove-self must match' ); + } + + /** + * @covers \MediaWiki\User\UserGroupManager::getGroupsChangeableBy + */ + public function testChangeableGroups() { + $manager = $this->getManager( self::CHANGEABLE_GROUPS_TEST_CONFIG ); + $allGroups = $manager->listAllGroups(); + + $user = $this->getTestUser()->getUser(); + $changeableGroups = $manager->getGroupsChangeableBy( new SimpleAuthority( $user, [ 'userrights' ] ) ); + $this->assertGroupsEquals( + [ + 'add' => $allGroups, + 'remove' => $allGroups, + 'add-self' => [], + 'remove-self' => [], + ], + $changeableGroups + ); + + $user = $this->getTestUser( [ 'bureaucrat', 'sysop' ] )->getUser(); + $changeableGroups = $manager->getGroupsChangeableBy( new SimpleAuthority( $user, [] ) ); + $this->assertGroupsEquals( + [ + 'add' => [ 'sysop', 'bureaucrat', 'rollback' ], + 'remove' => [ 'sysop', 'rollback' ], + 'add-self' => [ 'flood' ], + 'remove-self' => [], + ], + $changeableGroups + ); + + $user = $this->getTestUser( [ 'flood' ] )->getUser(); + $changeableGroups = $manager->getGroupsChangeableBy( new SimpleAuthority( $user, [] ) ); + $this->assertGroupsEquals( + [ + 'add' => [], + 'remove' => [], + 'add-self' => [], + 'remove-self' => [ 'flood' ], + ], + $changeableGroups + ); + } + + public function provideChangeableByGroup() { + yield 'sysop' => [ 'sysop', [ + 'add' => [ 'rollback' ], + 'remove' => [ 'rollback' ], + 'add-self' => [ 'flood' ], + 'remove-self' => [], + ] ]; + yield 'flood' => [ 'flood', [ + 'add' => [], + 'remove' => [], + 'add-self' => [], + 'remove-self' => [ 'flood' ], + ] ]; + } + + /** + * @dataProvider provideChangeableByGroup + * @covers \MediaWiki\User\UserGroupManager::getGroupsChangeableByGroup + * @param string $group + * @param array $expected + */ + public function testChangeableByGroup( string $group, array $expected ) { + $manager = $this->getManager( self::CHANGEABLE_GROUPS_TEST_CONFIG ); + $this->assertGroupsEquals( $expected, $manager->getGroupsChangeableByGroup( $group ) ); + } } diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 2ca0da5ba14..27cc7db2ea8 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -1946,32 +1946,33 @@ class UserTest extends MediaWikiIntegrationTestCase { $this->assertSame( [ 'test3' ], $user->getGroups(), 'Hooks can stop removal of a group' ); } + private const CHANGEABLE_GROUPS_TEST_CONFIG = [ + 'wgGroupPermissions' => [ + 'doEverything' => [ + 'userrights' => true, + ], + ], + 'wgAddGroups' => [ + 'sysop' => [ 'rollback' ], + 'bureaucrat' => [ 'sysop', 'bureaucrat' ], + ], + 'wgRemoveGroups' => [ + 'sysop' => [ 'rollback' ], + 'bureaucrat' => [ 'sysop' ], + ], + 'wgGroupsAddToSelf' => [ + 'sysop' => [ 'flood' ], + ], + 'wgGroupsRemoveFromSelf' => [ + 'flood' => [ 'flood' ], + ], + ]; + /** * @covers User::changeableGroups */ public function testChangeableGroups() { - // todo: test changeableByGroup here as well - $this->setMwGlobals( [ - 'wgGroupPermissions' => [ - 'doEverything' => [ - 'userrights' => true, - ], - ], - 'wgAddGroups' => [ - 'sysop' => [ 'rollback' ], - 'bureaucrat' => [ 'sysop', 'bureaucrat' ], - ], - 'wgRemoveGroups' => [ - 'sysop' => [ 'rollback' ], - 'bureaucrat' => [ 'sysop' ], - ], - 'wgGroupsAddToSelf' => [ - 'sysop' => [ 'flood' ], - ], - 'wgGroupsRemoveFromSelf' => [ - 'flood' => [ 'flood' ], - ], - ] ); + $this->setMwGlobals( self::CHANGEABLE_GROUPS_TEST_CONFIG ); $allGroups = User::getAllGroups(); @@ -2012,6 +2013,32 @@ class UserTest extends MediaWikiIntegrationTestCase { ); } + public function provideChangeableByGroup() { + yield 'sysop' => [ 'sysop', [ + 'add' => [ 'rollback' ], + 'remove' => [ 'rollback' ], + 'add-self' => [ 'flood' ], + 'remove-self' => [], + ] ]; + yield 'flood' => [ 'flood', [ + 'add' => [], + 'remove' => [], + 'add-self' => [], + 'remove-self' => [ 'flood' ], + ] ]; + } + + /** + * @dataProvider provideChangeableByGroup + * @covers User::changeableByGroup + * @param string $group + * @param array $expected + */ + public function testChangeableByGroup( string $group, array $expected ) { + $this->setMwGlobals( self::CHANGEABLE_GROUPS_TEST_CONFIG ); + $this->assertGroupsEquals( $expected, User::changeableByGroup( $group ) ); + } + private function assertGroupsEquals( array $expected, array $actual ) { // assertArrayEquals can compare without requiring the same order, // but the elements of an array are still required to be in the same order,