diff --git a/README.md b/README.md index 2f25d92..dc6d684 100644 --- a/README.md +++ b/README.md @@ -134,21 +134,6 @@ limit. Codes are stored hashed and are burned after five wrong entries. Every is failure and rate-limit hit is logged through the `MemberAccess` log channel, with the email address hashed. -### The roster - -A member's name gives nothing away, so what is left to keep is that they exist at all. The action -API query modules whose purpose is enumerating accounts are closed to the reader group, and three -logs are closed to anyone who cannot manage members: the new user log, where every member's account -creation is recorded, the block log, where every deactivation is, and the rename log, which names -both sides of a rename performed by hand. Restricting a log type also keeps it out of recent -changes. The special pages that list or resolve accounts are closed to the reader group as well, and -transcluding `Special:ListUsers` renders it empty for everyone, since what a transclusion renders is -not kept to the reader who asked for it. `Special:Redirect` is closed whole, so members lose its -other lookups too, and `Special:FilePath`, which redirects through it. - -Page histories and recent changes still name whoever acted, which on a members-only wiki means the -staff who edit: members cannot appear there, since they cannot change anything. - ### Login routes Two settings, one per login route, say what the allowlist governs there and whether the code route @@ -196,14 +181,11 @@ Whatever the login routes are set to, loading the extension: sending email, reading the abuse filters and their log, and reading or changing their own private information or preferences, which closes `Special:ChangeEmail` to them; * sets `$wgBlockDisablesLogin`, so blocking a member keeps them out of a private wiki; -* restricts the `newusers`, `block` and `renameuser` logs to the `memberaccess-manage` right, unless - the wiki already restricted them, so that who joined, who was deactivated, and what an account - renamed by hand was called before stay out of view; +* restricts the `renameuser` log to the `memberaccess-manage` right, unless the wiki already + restricted it, since an entry there names what an account was called before, which for a member + can be their email address; * refuses members a password, whatever the routes: setting one and having a temporary one mailed - stay refused; -* closes the account-listing API modules to the reader group; -* closes the special pages that list or resolve accounts to the reader group, and transcluding - `Special:ListUsers` to everyone. + stay refused. While the code route is turned on, it also turns off ConfirmEdit's `badloginperuser` captcha trigger, so failed logins no longer escalate to a captcha for the account they name, for everyone on the wiki @@ -346,8 +328,6 @@ body it cannot read — carries MediaWiki's error shape rather than this one. | `$wgMemberAccessIpDailyLimit` | int | `50` | Maximum code requests per client IP within 24 hours | | `$wgMemberAccessSenderAddress` | ?string | `null` | Address that login codes and invitations are sent from. Falls back to `$wgPasswordSender` | | `$wgMemberAccessSessionDurationSeconds` | int | `2592000` | How long a remembered login lasts, wiki-wide. Thirty days, against core's 180 days. `0` leaves `$wgExtendedLoginCookieExpiration` alone | -| `$wgMemberAccessBlockedApiModules` | string[] | `[ 'allusers', 'users', 'blocks' ]` | Action API query submodules the reader group may not use | -| `$wgMemberAccessBlockedSpecialPages` | string[] | `[ 'Listusers', 'Activeusers', 'BlockList', 'Redirect', 'Userrights' ]` | Special pages the reader group may not open. Canonical names; an alias does not match. Setting it adds to the shipped list rather than replacing it, so those pages cannot be dropped | Issued codes and rate-limit counters are held in the main object stash (`$wgMainStash`), which is database-backed by default. Point it at Redis or Valkey to keep them out of the database. diff --git a/docs/screenshots/code-entry.png b/docs/screenshots/code-entry.png new file mode 100644 index 0000000..26a8f58 Binary files /dev/null and b/docs/screenshots/code-entry.png differ diff --git a/extension.json b/extension.json index b83e6bc..bd616c0 100644 --- a/extension.json +++ b/extension.json @@ -35,12 +35,6 @@ "passwordReset": { "factory": "ProfessionalWiki\\MemberAccess\\MemberAccessExtension::newPasswordResetHookHandler" }, - "api": { - "factory": "ProfessionalWiki\\MemberAccess\\MemberAccessExtension::newUserListApiHookHandler" - }, - "specialPages": { - "factory": "ProfessionalWiki\\MemberAccess\\MemberAccessExtension::newUserListSpecialPageHookHandler" - }, "loginForm": { "class": "ProfessionalWiki\\MemberAccess\\EntryPoints\\LoginFormHandler" } @@ -50,8 +44,6 @@ "PluggableAuthUserAuthorization": "sso", "UserLoggedIn": "login", "SpecialPasswordResetOnSubmit": "passwordReset", - "ApiCheckCanExecute": "api", - "SpecialPageBeforeExecute": "specialPages", "AuthChangeFormFields": "loginForm", "BeforePageDisplay": "loginForm" }, @@ -145,14 +137,6 @@ "MemberAccessSessionDurationSeconds": { "value": 2592000, "description": "How long a remembered login lasts, in seconds. Sets $wgExtendedLoginCookieExpiration while a route can log a member in, so it applies to everyone on the wiki, not only to members. Set to 0 to leave that setting alone." - }, - "MemberAccessBlockedApiModules": { - "value": [ "allusers", "users", "blocks" ], - "description": "Names of action API query submodules the reader group may not use." - }, - "MemberAccessBlockedSpecialPages": { - "value": [ "Listusers", "Activeusers", "BlockList", "Redirect", "Userrights" ], - "description": "Canonical names of special pages the reader group may not open." } }, "RestRoutes": [ diff --git a/i18n/en.json b/i18n/en.json index 2081408..35b6278 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -41,8 +41,6 @@ "memberaccess-auth-not-authorized": "This address cannot be used to log in here.", "memberaccess-auth-failed": "Logging in did not complete. Request a new login code; if this keeps happening, contact an administrator of this wiki.", "memberaccess-auth-password-refused": "Members log in with a one-time code, so this account cannot have a password.", - "memberaccess-api-module-denied": "The \"$1\" API module is not available to members.", - "memberaccess-special-page-denied": "This special page is not available to members.", "memberaccess-block-reason": "Membership ended", "memberaccess-unblock-reason": "Membership resumed", "right-memberaccess-manage": "Manage the member allowlist and roster", diff --git a/i18n/qqq.json b/i18n/qqq.json index 600aa9c..1dfc50d 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -41,8 +41,6 @@ "memberaccess-auth-not-authorized": "Error shown when a correct code was entered but the address is not on the allowlist.", "memberaccess-auth-failed": "Error shown when a code login cannot continue, for any of several internal reasons. Deliberately vague, so keep the translation unspecific rather than naming a cause.", "memberaccess-auth-password-refused": "Error shown when setting or resetting the password of a member account is attempted.", - "memberaccess-api-module-denied": "Action API error shown when a member asks for a query module that lists the wiki's accounts.\n\nParameters:\n* $1 - the name of the query submodule, for example allusers", - "memberaccess-special-page-denied": "Error page shown when a member opens a special page that lists or resolves the wiki's accounts.", "memberaccess-block-reason": "Reason recorded in the block log when a member is deactivated. Reactivation recognises the extension's own block by this text in the wiki's content language: after a rewording, blocks placed under the old wording are reported as foreign instead of lifted.", "memberaccess-unblock-reason": "Reason recorded in the block log when a member is reactivated.", "right-memberaccess-manage": "{{doc-right|memberaccess-manage}}", diff --git a/src/EntryPoints/RegistrationHandler.php b/src/EntryPoints/RegistrationHandler.php index eea755c..5bfec8d 100644 --- a/src/EntryPoints/RegistrationHandler.php +++ b/src/EntryPoints/RegistrationHandler.php @@ -21,7 +21,7 @@ class RegistrationHandler { private const MANAGE_RIGHT = 'memberaccess-manage'; private const PUBLIC_LOG = '*'; - private const LOGS_THAT_RECORD_MEMBERS = [ 'newusers', 'block', 'renameuser' ]; + private const RENAME_LOG = 'renameuser'; private const PER_ADDRESS_CAPTCHA_TRIGGER = 'badloginperuser'; private const SSO_EMAIL_PROCESSOR_SETTING = 'wgOpenIDConnect_EmailProcessor'; private const SSO_USERNAME_PROCESSOR_SETTING = 'wgOpenIDConnect_PreferredUsernameProcessor'; @@ -39,7 +39,7 @@ public static function onRegistration(): void { */ private static function applyWhatMembersNeed(): void { self::moveReaderRevocationsToTheConfiguredGroup(); - self::closeTheLogsThatRecordMembers(); + self::closeTheRenameLog(); // A deactivated member is blocked, and only this makes a block keep them out of a private wiki. $GLOBALS['wgBlockDisablesLogin'] = true; @@ -64,24 +64,21 @@ private static function moveReaderRevocationsToTheConfiguredGroup(): void { } /** - * Three core logs record members: the new user log, where every account creation is, the block - * log, where every deactivation is, and the rename log, where a rename performed by hand names - * the account on both sides of it. All are closed to everyone who cannot manage members, which - * also keeps them out of recent changes, since a restricted log type is never written there. + * A rename log entry names what an account was called before, which for a member can be their + * email address, and the entry is there for good, so the log is closed to everyone who cannot + * manage members. That also keeps it out of recent changes, since a restricted log type is never + * written there. * - * A member's name says nothing about them, so what they give away is that somebody joined, was - * deactivated or was renamed, and when. A member still under a name from before the extension - * minted them is the one whose rename would name an address. + * Every other log names accounts under the names the wiki shows anyway, and a member's name says + * nothing about them, so this is the only log the extension closes. * - * A wiki that restricted one of them further keeps its own setting. + * A wiki that restricted it further keeps its own setting. */ - private static function closeTheLogsThatRecordMembers(): void { + private static function closeTheRenameLog(): void { $restrictions = self::globalArray( 'wgLogRestrictions' ); - foreach ( self::LOGS_THAT_RECORD_MEMBERS as $logType ) { - if ( ( $restrictions[$logType] ?? self::PUBLIC_LOG ) === self::PUBLIC_LOG ) { - $restrictions[$logType] = self::MANAGE_RIGHT; - } + if ( ( $restrictions[self::RENAME_LOG] ?? self::PUBLIC_LOG ) === self::PUBLIC_LOG ) { + $restrictions[self::RENAME_LOG] = self::MANAGE_RIGHT; } $GLOBALS['wgLogRestrictions'] = $restrictions; diff --git a/src/EntryPoints/UserListApiHandler.php b/src/EntryPoints/UserListApiHandler.php deleted file mode 100644 index ab8843f..0000000 --- a/src/EntryPoints/UserListApiHandler.php +++ /dev/null @@ -1,107 +0,0 @@ -|ApiMessage &$message - */ - public function onApiCheckCanExecute( $module, $user, &$message ): bool { - if ( !$module instanceof ApiQuery ) { - return true; - } - - $blocked = $this->firstBlockedSubmodule( $module ); - - if ( $blocked === null || !$this->holdsTheReaderGroup( $user ) ) { - return true; - } - - $message = new ApiMessage( - [ 'memberaccess-api-module-denied', $blocked ], - 'memberaccess-module-denied' - ); - - return false; - } - - private function firstBlockedSubmodule( ApiQuery $query ): ?string { - foreach ( $this->requestedSubmodules( $query ) as $submodule ) { - if ( in_array( $submodule, $this->blockedModules, true ) ) { - return $submodule; - } - } - - return null; - } - - /** - * The generator is not one of the query's own parameters: it belongs to the page set that - * feeds it, and is asked for separately. - * - * @return string[] - */ - private function requestedSubmodules( ApiQuery $query ): array { - $params = $query->extractRequestParams(); - - return array_merge( - $this->asStrings( $params['list'] ?? null ), - $this->asStrings( $params['prop'] ?? null ), - $this->asStrings( $params['meta'] ?? null ), - $this->asStrings( $query->getPageSet()->extractRequestParams()['generator'] ?? null ) - ); - } - - /** - * @return string[] - */ - private function asStrings( mixed $value ): array { - $names = []; - - foreach ( is_array( $value ) ? $value : [ $value ] as $name ) { - if ( is_string( $name ) ) { - $names[] = $name; - } - } - - return $names; - } - - private function holdsTheReaderGroup( UserIdentity $user ): bool { - return in_array( $this->readerGroup, $this->userGroups->getUserGroups( $user ), true ); - } - -} diff --git a/src/EntryPoints/UserListSpecialPageHandler.php b/src/EntryPoints/UserListSpecialPageHandler.php deleted file mode 100644 index 86a7a92..0000000 --- a/src/EntryPoints/UserListSpecialPageHandler.php +++ /dev/null @@ -1,63 +0,0 @@ -getName(), $this->blockedPages, true ) ) { - return true; - } - - // Refusing by exception would end the parse of the page holding the transclusion, so the - // inclusion is left empty instead. - if ( $special->including() ) { - return false; - } - - if ( $this->holdsTheReaderGroup( $special->getUser() ) ) { - throw new PermissionsError( null, [ 'memberaccess-special-page-denied' ] ); - } - - return true; - } - - private function holdsTheReaderGroup( UserIdentity $user ): bool { - return in_array( $this->readerGroup, $this->userGroups->getUserGroups( $user ), true ); - } - -} diff --git a/src/MemberAccessExtension.php b/src/MemberAccessExtension.php index 89b55ad..8e8896a 100644 --- a/src/MemberAccessExtension.php +++ b/src/MemberAccessExtension.php @@ -60,8 +60,6 @@ use ProfessionalWiki\MemberAccess\EntryPoints\REST\SendInvitationApi; use ProfessionalWiki\MemberAccess\EntryPoints\Auth\SsoAuthorizationHandler; use ProfessionalWiki\MemberAccess\EntryPoints\Auth\SsoUsernameProcessor; -use ProfessionalWiki\MemberAccess\EntryPoints\UserListApiHandler; -use ProfessionalWiki\MemberAccess\EntryPoints\UserListSpecialPageHandler; use ProfessionalWiki\MemberAccess\Persistence\DatabaseAllowlistRepository; use ProfessionalWiki\MemberAccess\Persistence\DatabaseMemberGroupRepository; use ProfessionalWiki\MemberAccess\Persistence\DatabaseMemberRepository; @@ -317,30 +315,6 @@ private function newCsrfTokenSet(): CsrfTokenSet { return new CsrfTokenSet( RequestContext::getMain()->getRequest() ); } - public static function newUserListApiHookHandler(): UserListApiHandler { - return self::getInstance()->newUserListApiHandler(); - } - - private function newUserListApiHandler(): UserListApiHandler { - return new UserListApiHandler( - userGroups: MediaWikiServices::getInstance()->getUserGroupManager(), - readerGroup: $this->getReaderGroup(), - blockedModules: $this->getStringListConfig( 'MemberAccessBlockedApiModules' ) - ); - } - - public static function newUserListSpecialPageHookHandler(): UserListSpecialPageHandler { - return self::getInstance()->newUserListSpecialPageHandler(); - } - - private function newUserListSpecialPageHandler(): UserListSpecialPageHandler { - return new UserListSpecialPageHandler( - userGroups: MediaWikiServices::getInstance()->getUserGroupManager(), - readerGroup: $this->getReaderGroup(), - blockedPages: $this->getStringListConfig( 'MemberAccessBlockedSpecialPages' ) - ); - } - public static function newMemberLoginHookHandler(): MemberLoginHandler { return self::getInstance()->newMemberLoginHandler(); } @@ -621,22 +595,6 @@ private function getIntConfig( string $name ): int { return is_scalar( $value ) ? intval( $value ) : 0; } - /** - * @return string[] - */ - private function getStringListConfig( string $name ): array { - $value = $this->getConfigValue( $name ); - $names = []; - - foreach ( is_array( $value ) ? $value : [] as $entry ) { - if ( is_string( $entry ) ) { - $names[] = $entry; - } - } - - return $names; - } - private function getConfigValue( string $name ): mixed { return MediaWikiServices::getInstance()->getMainConfig()->get( $name ); } diff --git a/tests/phpunit/Integration/MemberLogVisibilityTest.php b/tests/phpunit/Integration/MemberLogVisibilityTest.php deleted file mode 100644 index 0a2108a..0000000 --- a/tests/phpunit/Integration/MemberLogVisibilityTest.php +++ /dev/null @@ -1,268 +0,0 @@ -setService( 'Emailer', new SpyEmailer() ); - $this->registerOurAuthenticationProvider(); - - // The members whose log entries these tests are about are made by logging in over the - // code route, which a wiki has to turn on. - $this->overrideConfigValue( 'MemberAccessCodeLogin', 'allowlisted' ); - - $this->overrideConfigValues( [ - MainConfigNames::NewUserLog => true, - MainConfigNames::GroupPermissions => array_replace_recursive( - $this->getConfVar( MainConfigNames::GroupPermissions ), - [ '*' => [ 'autocreateaccount' => true ] ] - ) - ] ); - - MemberAccessExtension::getInstance()->setStashOverride( new HashBagOStuff() ); - MemberAccessExtension::getInstance()->setSecretGeneratorOverride( new FixedSecretGenerator( self::CODE ) ); - } - - protected function tearDown(): void { - MemberAccessExtension::getInstance()->setStashOverride( null ); - MemberAccessExtension::getInstance()->setSecretGeneratorOverride( null ); - - parent::tearDown(); - } - - public function testNewUserLogNamesTheMemberToAnAdmin(): void { - $member = $this->admitAMember(); - - $entries = $this->logEventsFor( $this->admin(), 'newusers' ); - - $this->assertSame( [ $member->getName() ], array_column( $entries, 'user' ) ); - } - - public function testNewUserLogIsClosedToAMember(): void { - $this->admitAMember(); - - $this->assertSame( [], $this->logEventsFor( $this->member(), 'newusers' ) ); - } - - public function testBlockLogNamesTheDeactivatedMemberToAnAdmin(): void { - $member = $this->admitAMember(); - $this->deactivate( $member ); - - $entries = $this->logEventsFor( $this->admin(), 'block' ); - - $this->assertSame( [ 'User:' . $member->getName() ], array_column( $entries, 'title' ) ); - } - - public function testBlockLogIsClosedToAMember(): void { - $this->deactivate( $this->admitAMember() ); - - $this->assertSame( [], $this->logEventsFor( $this->member(), 'block' ) ); - } - - /** - * The rename log holds what an account renamed by hand was called before, which on a member - * still under a name from before the extension minted them is their address. - */ - public function testRenameLogNamesTheRenamedAccountToAnAdmin(): void { - $renamed = $this->renameAnAccount(); - - $entries = $this->logEventsFor( $this->admin(), 'renameuser' ); - - $this->assertSame( [ 'User:' . $renamed ], array_column( $entries, 'title' ) ); - } - - public function testRenameLogIsClosedToAMember(): void { - $this->renameAnAccount(); - - $this->assertSame( [], $this->logEventsFor( $this->member(), 'renameuser' ) ); - } - - /** - * @return string The name the account was renamed away from - */ - private function renameAnAccount(): string { - $account = $this->getMutableTestUser()->getUser(); - $oldName = $account->getName(); - - $renamed = ( new RenameuserSQL( - $oldName, - 'Member AB2345', - $account->getId(), - $this->getTestSysop()->getUser() - ) )->rename(); - - $this->assertTrue( $renamed ); - DeferredUpdates::doUpdates(); - - return $oldName; - } - - /** - * Log entries of a restricted type are kept out of recent changes when they are written, so - * there is nothing there for the query to filter. - */ - public function testDeactivatingAMemberDoesNotShowUpInRecentChanges(): void { - $this->editPage( 'Visible page', 'Some text' ); - $this->deactivate( $this->admitAMember() ); - - $changes = $this->recentChangesFor( $this->member() ); - - $this->assertNotSame( [], $changes ); - $this->assertSame( [], array_column( $changes, 'logtype' ) ); - } - - public function testSpecialLogNamesTheMemberToAnAdmin(): void { - $member = $this->admitAMember(); - - $html = $this->specialLogFor( $this->admin(), 'newusers' ); - - $this->assertStringContainsString( $member->getName(), $html ); - } - - public function testSpecialLogRefusesTheNewUserLogToAMember(): void { - $this->admitAMember(); - - $this->expectException( PermissionsError::class ); - $this->specialLogFor( $this->member(), 'newusers' ); - } - - public function testSpecialLogWithoutAChosenTypeNamesNoMember(): void { - $member = $this->admitAMember(); - - $this->assertStringNotContainsString( $member->getName(), $this->specialLogFor( $this->member(), '' ) ); - } - - private function admin(): Authority { - return $this->getTestSysop()->getAuthority(); - } - - private function member(): Authority { - return $this->getMutableTestUser( [ 'reader' ] )->getAuthority(); - } - - /** - * @return array> - */ - private function logEventsFor( Authority $performer, string $type ): array { - [ $result ] = $this->doApiRequest( [ - 'action' => 'query', - 'list' => 'logevents', - 'letype' => $type, - 'leprop' => 'user|title|type|comment' - ], null, false, $performer ); - - return $result['query']['logevents']; - } - - /** - * @return array> - */ - private function recentChangesFor( Authority $performer ): array { - [ $result ] = $this->doApiRequest( [ - 'action' => 'query', - 'list' => 'recentchanges', - 'rcprop' => 'title|loginfo' - ], null, false, $performer ); - - return $result['query']['recentchanges']; - } - - private function specialLogFor( Authority $performer, string $logType ): string { - [ $html ] = ( new SpecialPageExecutor() )->executeSpecialPage( - $this->getServiceContainer()->getSpecialPageFactory()->getPage( 'Log' ), - $logType, - null, - null, - $performer - ); - - return $html; - } - - private function admitAMember(): User { - $extension = MemberAccessExtension::getInstance(); - $value = AllowlistValue::fromString( self::MEMBER_EMAIL ); - - $this->assertNotNull( $value ); - - $extension->newAllowlistRepository()->addEntry( - groupId: $extension->newMemberGroupRepository()->createGroup( 'Acme' )->id, - value: $value, - actorId: 1 - ); - - $user = $this->getServiceContainer()->getUserFactory()->newFromName( $this->logIn() ); - - $this->assertNotNull( $user ); - - return $user; - } - - /** - * @return string The name the member's account was created under - */ - private function logIn(): string { - $this->getServiceContainer()->getAuthManager() - ->beginAuthentication( $this->submittedCodeRequest( self::MEMBER_EMAIL ), self::RETURN_TO_URL ); - DeferredUpdates::doUpdates(); - - $codeEntry = new EnterCodeRequest(); - $codeEntry->memberaccessCode = self::CODE; - - $response = $this->getServiceContainer()->getAuthManager()->continueAuthentication( [ $codeEntry ] ); - - $this->assertSame( AuthenticationResponse::PASS, $response->status ); - - DeferredUpdates::doUpdates(); - - return (string)$response->username; - } - - private function deactivate( User $member ): void { - MemberAccessExtension::getInstance()->newDeactivateMemberUseCase()->deactivate( - $member->getId(), - $this->getTestSysop()->getUser()->getId() - ); - - DeferredUpdates::doUpdates(); - } - -} diff --git a/tests/phpunit/Integration/MemberReadAccessTest.php b/tests/phpunit/Integration/MemberReadAccessTest.php new file mode 100644 index 0000000..9cb0d79 --- /dev/null +++ b/tests/phpunit/Integration/MemberReadAccessTest.php @@ -0,0 +1,162 @@ +newMember(); + + $html = $this->open( + $page, + $member, + $subPage === false ? false : $this->fillIn( $subPage, $member ) + ); + + $this->assertStringContainsString( $this->fillIn( $expected, $member ), $html ); + } + + /** + * The file lookup is here because Special:FilePath answers a file name with the file's URL by + * redirecting through it, so a member who cannot open it cannot follow a file path either. + * + * @return iterable + */ + public static function accountNamingPageProvider(): iterable { + yield 'the account list' => [ 'Listusers', false, '{name}' ]; + yield 'the active account list' => [ 'Activeusers', false, '(activeusers-noresult)' ]; + yield 'the block list' => [ 'BlockList', false, '(ipblocklist-empty)' ]; + yield 'an account looked up by id' => [ 'Userrights', '#{id}', '{name}' ]; + yield 'the user page of an account looked up by id' => [ 'Redirect', 'user/{id}', '{userpage}' ]; + yield 'a file looked up by name' => [ 'Redirect', 'file/Example.png', '(redirect-not-exists)' ]; + } + + /** + * A transclusion renders for whoever asks and outlives the request that asked, so a refusal here + * emptied the list for everyone, members and staff alike. + */ + public function testTranscludedAccountListNamesAnAccountToAMember(): void { + $member = $this->newMember(); + + $this->assertStringContainsString( + $member->getName(), + $this->parseAs( '{{Special:Listusers}}', $member ) + ); + } + + /** + * @dataProvider accountListingModuleProvider + * @param array $params + */ + public function testMemberCanUseTheApiModulesThatNameAccounts( array $params, string $resultKey ): void { + $this->assertArrayHasKey( $resultKey, $this->queryAs( $params, $this->newMember() ) ); + } + + /** + * @return iterable, string}> + */ + public static function accountListingModuleProvider(): iterable { + yield 'allusers' => [ [ 'list' => 'allusers' ], 'allusers' ]; + yield 'users' => [ [ 'list' => 'users', 'ususers' => 'Someone' ], 'users' ]; + yield 'blocks' => [ [ 'list' => 'blocks' ], 'blocks' ]; + } + + public function testTheAccountListApiNamesTheMember(): void { + $member = $this->newMember(); + + $accounts = $this->queryAs( [ 'list' => 'allusers' ], $member )['allusers']; + + $this->assertContains( $member->getName(), array_column( $accounts, 'name' ) ); + } + + private function newMember(): User { + return $this->getMutableTestUser( [ 'reader' ] )->getUser(); + } + + /** + * A data provider runs before there is an account to name, so it names the placeholders filled + * in here instead. + */ + private function fillIn( string $text, User $member ): string { + return strtr( $text, [ + '{name}' => $member->getName(), + '{id}' => (string)$member->getId(), + // The spelling a URL carries, which is how the page names an account it redirects to. + '{userpage}' => $member->getUserPage()->getDBkey() + ] ); + } + + /** + * Opened through the special page factory rather than executed directly, so that the page runs + * the way a request for it runs. The title is the one a link would carry, since the factory + * answers any other spelling with a redirect to it. A page that answers with a redirect renders + * no body, so its target is part of what is returned. + */ + private function open( string $page, User $performer, string|false $subPage = false ): string { + $context = new DerivativeContext( RequestContext::getMain() ); + $context->setAuthority( $performer ); + $context->setRequest( new FauxRequest() ); + $context->setLanguage( 'qqx' ); + + $output = new OutputPage( $context ); + $context->setOutput( $output ); + + $this->getServiceContainer()->getSpecialPageFactory()->executePath( + SpecialPage::getTitleFor( $page, $subPage ), + $context + ); + + return $output->getRedirect() . $output->getHTML(); + } + + /** + * The parse action is the transclusion path a member can ask for directly, whatever the wiki + * has on its pages. + */ + private function parseAs( string $wikitext, User $performer ): string { + [ $result ] = $this->doApiRequest( [ + 'action' => 'parse', + 'text' => $wikitext, + 'contentmodel' => 'wikitext', + 'prop' => 'text', + 'formatversion' => '2' + ], null, false, $performer ); + + return $result['parse']['text']; + } + + /** + * @param array $params + * @return array + */ + private function queryAs( array $params, User $performer ): array { + [ $result ] = $this->doApiRequest( [ 'action' => 'query' ] + $params, null, false, $performer ); + + return $result['query']; + } + +} diff --git a/tests/phpunit/Integration/RegistrationHandlerTest.php b/tests/phpunit/Integration/RegistrationHandlerTest.php index d3976a0..6d95131 100644 --- a/tests/phpunit/Integration/RegistrationHandlerTest.php +++ b/tests/phpunit/Integration/RegistrationHandlerTest.php @@ -49,10 +49,10 @@ class RegistrationHandlerTest extends MediaWikiIntegrationTestCase { /** * extension.json has to name the callback for any of what follows to reach a wiki. Nothing else - * closes the new user log. + * closes the rename log. */ public function testLoadingTheExtensionAppliesItsSettings(): void { - $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['newusers'] ?? null ); + $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['renameuser'] ?? null ); } /** @@ -117,23 +117,13 @@ public function testBlockingAMemberKeepsThemOutOfAPrivateWiki( } /** + * A rename log entry names what an account was called before, which for a member can be their + * email address. Every other log names accounts under the names the wiki shows anyway, so the + * rename log is the only one the extension closes. + * * @dataProvider everyRouteStateProvider */ - public function testAccountCreationsAreKeptOutOfTheReadableNewUserLog( - string $codeLogin, - bool $allowlistAppliesToSso - ): void { - $this->setMwGlobals( 'wgLogRestrictions', [] ); - - $this->registerWithRoutes( $codeLogin, $allowlistAppliesToSso ); - - $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['newusers'] ?? null ); - } - - /** - * @dataProvider everyRouteStateProvider - */ - public function testDeactivationsAreKeptOutOfTheReadableBlockLog( + public function testTheRenameLogIsTheOnlyLogClosed( string $codeLogin, bool $allowlistAppliesToSso ): void { @@ -141,37 +131,23 @@ public function testDeactivationsAreKeptOutOfTheReadableBlockLog( $this->registerWithRoutes( $codeLogin, $allowlistAppliesToSso ); - $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['block'] ?? null ); + $this->assertSame( [ 'renameuser' => 'memberaccess-manage' ], $GLOBALS['wgLogRestrictions'] ); } public function testALogTypeTheWikiRestrictedFurtherIsLeftAlone(): void { - $this->setMwGlobals( 'wgLogRestrictions', [ 'newusers' => 'suppressionlog' ] ); + $this->setMwGlobals( 'wgLogRestrictions', [ 'renameuser' => 'suppressionlog' ] ); $this->registerWithRoutes( 'allowlisted', true ); - $this->assertSame( 'suppressionlog', $GLOBALS['wgLogRestrictions']['newusers'] ); + $this->assertSame( 'suppressionlog', $GLOBALS['wgLogRestrictions']['renameuser'] ); } public function testALogTypeTheWikiDeclaredPublicIsStillClosed(): void { - $this->setMwGlobals( 'wgLogRestrictions', [ 'newusers' => '*' ] ); + $this->setMwGlobals( 'wgLogRestrictions', [ 'renameuser' => '*' ] ); $this->registerWithRoutes( 'allowlisted', true ); - $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['newusers'] ); - } - - /** - * @dataProvider everyRouteStateProvider - */ - public function testRenamesAreKeptOutOfTheReadableRenameLog( - string $codeLogin, - bool $allowlistAppliesToSso - ): void { - $this->setMwGlobals( 'wgLogRestrictions', [] ); - - $this->registerWithRoutes( $codeLogin, $allowlistAppliesToSso ); - - $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['renameuser'] ?? null ); + $this->assertSame( 'memberaccess-manage', $GLOBALS['wgLogRestrictions']['renameuser'] ); } /** diff --git a/tests/phpunit/Integration/RenameLogVisibilityTest.php b/tests/phpunit/Integration/RenameLogVisibilityTest.php new file mode 100644 index 0000000..3c5897b --- /dev/null +++ b/tests/phpunit/Integration/RenameLogVisibilityTest.php @@ -0,0 +1,153 @@ +renameAnAccount(); + + $entries = $this->logEventsFor( $this->admin(), 'renameuser' ); + + $this->assertSame( [ 'User:' . $renamed ], array_column( $entries, 'title' ) ); + } + + public function testRenameLogIsClosedToAMember(): void { + $this->renameAnAccount(); + + $this->assertSame( [], $this->logEventsFor( $this->member(), 'renameuser' ) ); + } + + /** + * Log entries of a restricted type are kept out of recent changes when they are written, so + * there is nothing there for the query to filter. + */ + public function testRenamingAnAccountDoesNotShowUpInRecentChanges(): void { + $this->editPage( 'Visible page', 'Some text' ); + $this->renameAnAccount(); + + $changes = $this->recentChangesFor( $this->member() ); + + $this->assertNotSame( [], $changes ); + $this->assertNotContains( 'renameuser', array_column( $changes, 'logtype' ) ); + } + + /** + * What keeps the entry out of recent changes, and its subject out of the log without a chosen + * type, is the restriction rather than anything else about a rename. + */ + public function testAnUnrestrictedRenameReachesRecentChangesAndTheLog(): void { + $this->overrideConfigValue( MainConfigNames::LogRestrictions, [] ); + $renamed = $this->renameAnAccount(); + + $changes = $this->recentChangesFor( $this->member() ); + + $this->assertContains( 'renameuser', array_column( $changes, 'logtype' ) ); + $this->assertStringContainsString( $renamed, $this->specialLogFor( $this->member(), '' ) ); + } + + public function testSpecialLogNamesTheRenamedAccountToAnAdmin(): void { + $renamed = $this->renameAnAccount(); + + $html = $this->specialLogFor( $this->admin(), 'renameuser' ); + + $this->assertStringContainsString( $renamed, $html ); + } + + public function testSpecialLogRefusesTheRenameLogToAMember(): void { + $this->renameAnAccount(); + + $this->expectException( PermissionsError::class ); + $this->specialLogFor( $this->member(), 'renameuser' ); + } + + public function testSpecialLogWithoutAChosenTypeNamesNoRenamedAccount(): void { + $renamed = $this->renameAnAccount(); + + $this->assertStringNotContainsString( $renamed, $this->specialLogFor( $this->member(), '' ) ); + } + + /** + * @return string The name the account was renamed away from + */ + private function renameAnAccount(): string { + $account = $this->getMutableTestUser()->getUser(); + $oldName = $account->getName(); + + $renamed = ( new RenameuserSQL( + $oldName, + 'Member AB2345', + $account->getId(), + $this->getTestSysop()->getUser() + ) )->rename(); + + $this->assertTrue( $renamed ); + DeferredUpdates::doUpdates(); + + return $oldName; + } + + private function admin(): Authority { + return $this->getTestSysop()->getAuthority(); + } + + private function member(): Authority { + return $this->getMutableTestUser( [ 'reader' ] )->getAuthority(); + } + + /** + * @return array> + */ + private function logEventsFor( Authority $performer, string $type ): array { + [ $result ] = $this->doApiRequest( [ + 'action' => 'query', + 'list' => 'logevents', + 'letype' => $type, + 'leprop' => 'user|title|type|comment' + ], null, false, $performer ); + + return $result['query']['logevents']; + } + + /** + * @return array> + */ + private function recentChangesFor( Authority $performer ): array { + [ $result ] = $this->doApiRequest( [ + 'action' => 'query', + 'list' => 'recentchanges', + 'rcprop' => 'title|loginfo' + ], null, false, $performer ); + + return $result['query']['recentchanges']; + } + + private function specialLogFor( Authority $performer, string $logType ): string { + [ $html ] = ( new SpecialPageExecutor() )->executeSpecialPage( + $this->getServiceContainer()->getSpecialPageFactory()->getPage( 'Log' ), + $logType, + null, + null, + $performer + ); + + return $html; + } + +} diff --git a/tests/phpunit/Integration/UserListApiHandlerTest.php b/tests/phpunit/Integration/UserListApiHandlerTest.php deleted file mode 100644 index 71f26ba..0000000 --- a/tests/phpunit/Integration/UserListApiHandlerTest.php +++ /dev/null @@ -1,85 +0,0 @@ -expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'list' => 'allusers' ] ); - } - - public function testMemberCannotLookUpAccounts(): void { - $this->expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'list' => 'users', 'ususers' => 'Someone' ] ); - } - - public function testMemberCannotListBlockedAccounts(): void { - $this->expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'list' => 'blocks' ] ); - } - - /** - * No module that lists accounts is generator capable, since accounts are not pages, so this - * uses a module that is. The point is that asking through a generator is refused as well. - */ - public function testBlockedModuleUsedAsAGeneratorIsRefused(): void { - $this->overrideConfigValue( 'MemberAccessBlockedApiModules', [ 'allpages' ] ); - - $this->expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'generator' => 'allpages' ] ); - } - - public function testAskingForOneBlockedModuleAmongOthersIsRefused(): void { - $this->expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'list' => 'logevents|allusers' ] ); - } - - public function testMemberKeepsTheRestOfTheQueryApi(): void { - $result = $this->queryAs( $this->newMember(), [ 'meta' => 'siteinfo' ] ); - - $this->assertArrayHasKey( 'query', $result[0] ); - } - - public function testAccountsOutsideTheReaderGroupCanStillListEveryAccount(): void { - $result = $this->queryAs( $this->getTestUser()->getUser(), [ 'list' => 'allusers' ] ); - - $this->assertArrayHasKey( 'query', $result[0] ); - } - - public function testTheBlockedModulesAreConfigurable(): void { - $this->overrideConfigValue( 'MemberAccessBlockedApiModules', [ 'siteinfo' ] ); - - $this->expectApiErrorCode( 'memberaccess-module-denied' ); - - $this->queryAs( $this->newMember(), [ 'meta' => 'siteinfo' ] ); - } - - private function newMember(): User { - return $this->getMutableTestUser( [ 'reader' ] )->getUser(); - } - - /** - * @param array $params - * @return array - */ - private function queryAs( User $user, array $params ): array { - return $this->doApiRequest( [ 'action' => 'query' ] + $params, null, false, $user ); - } - -} diff --git a/tests/phpunit/Integration/UserListSpecialPageHandlerTest.php b/tests/phpunit/Integration/UserListSpecialPageHandlerTest.php deleted file mode 100644 index 88f83dc..0000000 --- a/tests/phpunit/Integration/UserListSpecialPageHandlerTest.php +++ /dev/null @@ -1,211 +0,0 @@ -expectException( PermissionsError::class ); - - $this->openAsMember( 'Listusers' ); - } - - public function testMemberCannotOpenTheActiveUserList(): void { - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'Activeusers' ); - } - - public function testMemberCannotOpenTheBlockList(): void { - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'BlockList' ); - } - - /** - * Special:Redirect answers a numeric user ID with a redirect to that account's user page, so - * walking IDs names every member without a listing page. - */ - public function testMemberCannotResolveAnAccountThroughRedirect(): void { - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'Redirect', 'user/' . $this->newMember()->getId() ); - } - - /** - * Special:UserRights answers a numeric user ID with that account's name and its groups, before - * it asks whether the reader may change anything, so walking IDs names every member there too. - */ - public function testMemberCannotResolveAnAccountThroughUserRights(): void { - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'Userrights', '#' . $this->newMember()->getId() ); - } - - /** - * Its form resolves an account without a subpage to match on, so the page is closed whole. - */ - public function testMemberCannotOpenTheRedirectFormItself(): void { - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'Redirect' ); - } - - public function testTheRefusalNamesTheMemberMessage(): void { - $this->assertSame( - 'memberaccess-special-page-denied', - $this->refusalFor( 'Listusers' )->getMessageObject()->getKey() - ); - } - - public function testAccountsOutsideTheReaderGroupKeepTheUserList(): void { - $member = $this->newMember(); - - $html = $this->open( 'Listusers', $this->getTestUser()->getUser() ); - - $this->assertStringContainsString( $member->getName(), $html ); - } - - public function testMemberKeepsSpecialPagesThatNameNoAccount(): void { - $this->assertStringContainsString( '(intentionallyblankpage)', $this->openAsMember( 'Blankpage' ) ); - } - - public function testTheBlockedPagesAreConfigurable(): void { - $this->blockAmongOthers( 'Blankpage' ); - - $this->expectException( PermissionsError::class ); - - $this->openAsMember( 'Blankpage' ); - } - - public function testPageLeftOutOfTheConfiguredListStaysOpen(): void { - $this->blockAmongOthers( 'Blankpage' ); - $member = $this->newMember(); - - $this->assertStringContainsString( $member->getName(), $this->open( 'Listusers', $member ) ); - } - - public function testTranscludedUserListNamesNoAccountToAMember(): void { - $member = $this->newMember(); - - $html = $this->parseAs( '{{Special:Listusers}}', $member ); - - $this->assertStringNotContainsString( $member->getName(), $html ); - } - - /** - * The refusal is not held to the reader group, since a page's rendered text is also produced by - * parses nobody asked for: secondary data updates and search index builds parse as an anonymous - * user, and a check on the group would let the roster through into those. - */ - public function testTranscludedUserListNamesNoAccountToAnAdmin(): void { - $member = $this->newMember(); - - $html = $this->parseAs( '{{Special:Listusers}}', $this->getTestSysop()->getAuthority() ); - - $this->assertStringNotContainsString( $member->getName(), $html ); - } - - /** - * Refusing the inclusion by exception would end the parse of the whole page holding it, so it - * renders empty instead. - */ - public function testAPageTranscludingTheUserListRendersWithoutIt(): void { - $member = $this->newMember(); - - $html = $this->parseAs( 'before {{Special:Listusers}} after', $member ); - - $this->assertStringContainsString( 'before', $html ); - $this->assertStringContainsString( 'after', $html ); - $this->assertStringNotContainsString( $member->getName(), $html ); - } - - public function testTranscludingAPageLeftOutOfTheConfiguredListStillWorks(): void { - $this->blockAmongOthers( 'Blankpage' ); - $member = $this->newMember(); - - $html = $this->parseAs( '{{Special:Listusers}}', $member ); - - $this->assertStringContainsString( $member->getName(), $html ); - } - - /** - * Neighbours on either side, so that reading only the first or the last name of the list fails. - */ - private function blockAmongOthers( string $page ): void { - $this->overrideConfigValue( 'MemberAccessBlockedSpecialPages', [ 'Allmessages', $page, 'Random' ] ); - } - - private function newMember(): User { - return $this->getMutableTestUser( [ 'reader' ] )->getUser(); - } - - private function refusalFor( string $page ): PermissionsError { - try { - $this->openAsMember( $page ); - } catch ( PermissionsError $refusal ) { - return $refusal; - } - - $this->fail( "Special:$page was not refused" ); - } - - private function openAsMember( string $page, string|false $subPage = false ): string { - return $this->open( $page, $this->newMember(), $subPage ); - } - - /** - * Special pages are opened through the factory rather than executed directly, since the hook - * this tests is fired by SpecialPage::run() rather than by the page itself. The title is the one - * a link would carry, since the factory answers any other spelling with a redirect to it. - */ - private function open( string $page, Authority $performer, string|false $subPage = false ): string { - $context = new DerivativeContext( RequestContext::getMain() ); - $context->setAuthority( $performer ); - $context->setRequest( new FauxRequest() ); - $context->setLanguage( 'qqx' ); - - $output = new OutputPage( $context ); - $context->setOutput( $output ); - - $this->getServiceContainer()->getSpecialPageFactory()->executePath( - SpecialPage::getTitleFor( $page, $subPage ), - $context - ); - - return $output->getHTML(); - } - - /** - * The parse action is the transclusion path a member can ask for directly, whatever the wiki - * has on its pages. - */ - private function parseAs( string $wikitext, Authority $performer ): string { - [ $result ] = $this->doApiRequest( [ - 'action' => 'parse', - 'text' => $wikitext, - 'contentmodel' => 'wikitext', - 'prop' => 'text', - 'formatversion' => '2' - ], null, false, $performer ); - - return $result['parse']['text']; - } - -}