Page MenuHomePhabricator

Translate sandbox signup is broken
Closed, ResolvedPublic

Description

2019-07-15 10:31:16 web1.translatewiki.net dev_translatewiki_net-bw_: [d1709137d0f3e4804f9c6dc0] /w/api.php   MWException from line 59 of /www/dev.translatewiki.net/docroot/w/extensions/Translate/utils/TranslateSandbox.php: Account creation failed: The action you have requested is limited to users in the group: [[Project:Administrators|ylläpitäjät]].
#0 /www/dev.translatewiki.net/docroot/w/extensions/Translate/api/ApiTranslateSandbox.php(68): TranslateSandbox::addUser(string, string, string)
#1 /www/dev.translatewiki.net/docroot/w/extensions/Translate/api/ApiTranslateSandbox.php(24): ApiTranslateSandbox->doCreate()
#2 /www/dev.translatewiki.net/docroot/w/includes/api/ApiMain.php(1583): ApiTranslateSandbox->execute()
#3 /www/dev.translatewiki.net/docroot/w/includes/api/ApiMain.php(531): ApiMain->executeAction()
#4 /www/dev.translatewiki.net/docroot/w/includes/api/ApiMain.php(502): ApiMain->executeActionWithErrorHandling()
#5 /www/dev.translatewiki.net/docroot/w/api.php(87): ApiMain->execute()
#6 {main}

Git bisect places the blame on commit rMWdd6b94024c53: Re-apply: Factors out permissions check from User into PermissionManager service

Impact: New users cannot register on translatewiki.net

Details

Related Gerrit Patches:
mediawiki/extensions/Translate : masterFix broken translate sandbox signup

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 15 2019, 10:37 AM
Nikerabbit triaged this task as Unbreak Now! priority.Jul 15 2019, 10:37 AM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptJul 15 2019, 10:37 AM

It looks like AuthManager::singleton()->beginAccountCreation is now checking for permissions which it did not do previously. The code needs a version that bypasses createaccount permission check.

abi_ added a subscriber: abi_.Jul 16 2019, 6:20 AM

I tried the following but it doesn't work either (perhaps because the user doesn't exist yet?)

$permissionManager->addTemporaryUserRights( $user, 'createaccount' );

Currently the code is using UserGetRights hook to give the right temporarily. It's probably a change when the hook is called or caching of the results that may have broken this, rather than a direct change in authmanager itself.

Tgr added a comment.Jul 17 2019, 8:19 AM

Previously, the rights were cached on the User object. PermissionManager caches them by user ID, which is 0 when AuthManager checks it, so maybe somehow related to that, although I don't see any obvious reason why that would break.

In any case, that should probably be changed. Using a shared user rights cache for all anonymous users is an accident waiting to happen.

Tgr added a comment.Jul 17 2019, 9:02 AM

In any case, that should probably be changed. Using a shared user rights cache for all anonymous users is an accident waiting to happen.

Filed as T228253.

Tgr added a comment.Jul 17 2019, 12:34 PM

Previously user rights were cached per user object, and TranslateSandbox::addUser created its own user object so it was unaffected. Now any call to get the rights of a given user will cause it to be cached for all user objects representing that user (which is saner, but a behavior change).

Also tried with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Translate/+/509168 but it doesn't solve this issue either.

The permission check fails on line 48, you'd have to reset the cache between that and line 45 (which triggers the hook behavior).

EvanProdromou added a subscriber: EvanProdromou.

OK, this came up in scrum-of-scrums. I'll see if I can find the right person to help fix it.

Also tried with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Translate/+/509168 but it doesn't solve this issue either.

The permission check fails on line 48, you'd have to reset the cache between that and line 45 (which triggers the hook behavior).

$user->clearInstanceCache( 'name' ); or $user->clearInstanceCache();? That would be third cache clear call in that method, unless some of them are redundant.

Tgr added a comment.EditedJul 18 2019, 10:34 AM

$user->clearInstanceCache( 'name' ); or $user->clearInstanceCache();?

Both clear the rights cache so clearInstanceCache() should be enough.

That would be third cache clear call in that method, unless some of them are redundant.

Changing $userToCreate changes the UserGetRights hook behavior so the cache needs to be cleared. The existing two cache invalidations (they are on different branches, so it's only done once) are needed since the user has just been created by AuthManager.

IMO that whole approach is broken, and instead if temporarily messing with permissions of a user that doesn't even exist yet you should just use a system user.

public static function addUser( $name, $email, $password ) {
    if ( !User::isCreatableName( $name ) ) { // ** somewhat pointless, AuthManager would check this anyway
        throw new MWException( 'Invalid user name' );
    }
    $data = [
        'username' => $name,
        'password' => $password,
        'retype' => $password,
        'email' => $email,
        'realname' => '',
    ];

    $creator = User::newSystemUser( 'Translate user manager', [ 'steal' => true ] );
    $guard = $permissionManager->addTemporaryUserRights( $creator, 'accountcreate' ); // or you could just give it the rights properly
    $reqs = AuthManager::singleton()->getAuthenticationRequests( AuthManager::ACTION_CREATE );
    $reqs = AuthenticationRequest::loadRequestsFromSubmission( $reqs, $data );
    $res = AuthManager::singleton()->beginAccountCreation( $creator, $reqs, 'null:' );

    $user = User::newFromName( $name );
    if ( $res->status !== AuthenticationResponse::PASS ) {
        $reason = 'AuthManager returned ' . $res->status . ' instead of PASS';
        if ( $res->status === AuthenticationResponse::FAIL ) {
            // Unless things are misconfigured, this will handle errors such as username taken,
            // invalid user name or too short password. The WebAPI is prechecking these to
            // provide nicer error messages.
            $reason = $res->message->inLanguage( 'en' )->useDatabase( false )->text();
        } elseif ( $user->getId() ) {
            // A provider requested further user input. Abort but clean up first if it was a
            // secondary provider (in which case the user was created).
            // ** or you could handle this as success if the user exists. It will be in some halfway
            // ** state but the same could happen with a real human navigating away in the middle of 
            // ** account creation, and for that reason changes made by secondary providers
            // ** should not be critical anyway.
            self::deleteUser( $user, 'force' );
        }
        throw new MWException( "Account creation failed: $reason" );
    }

    // group-translate-sandboxed group-translate-sandboxed-member
    $user->addGroup( 'translate-sandboxed' );

    return $user;
}

Neither $creator nor $guard is used anywhere. in your example How is that supposed to work?

Tgr added a comment.Jul 18 2019, 10:40 AM

The creator should be passed to AuthManager; fixed that in the snippet. The guard is a ScopedCallback, it only needs to exist in the scope. You could add a ScopedCallback::consume( $guard ); after the AuthManager call for readability.

Nikerabbit lowered the priority of this task from Unbreak Now! to High.Jul 18 2019, 1:34 PM
abi_ moved this task from Backlog to In Progress on the User-abi_ board.Jul 22 2019, 2:48 PM
abi_ added a subscriber: Pginer-WMF.

Note that the $permissionManager->addTemporaryUserRights method was added recently so we will have to ensure that we maintain backward compatibility with older versions of MediaWiki as per MLEB policy.

@Pginer-WMF - Can you please add this task to the current Language team board?

Change 524936 had a related patch set uploaded (by Abijeet Patro; owner: Abijeet Patro):
[mediawiki/extensions/Translate@master] Fix broken translate sandbox signup

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

abi_ claimed this task.EditedJul 25 2019, 3:35 AM
abi_ edited projects, added MediaWiki-extensions-Translate; removed User-abi_.

Assigning this to myself since I'm working on the patch to fix this issue on the Translate extension.

I've submitted a patch with slight modifications to the patch submitted by Tgr. Few things to note,

  1. Did not add support for MediaWiki <= 1.34 since I think that the Translator Sandbox is not used anywhere other than Translatewiki.net
  2. We'll need to decide a username for the user that we are using as the creator for these new accounts. Using existing FuzzyBot may not be a good idea.

Change 524936 merged by jenkins-bot:
[mediawiki/extensions/Translate@master] Fix broken translate sandbox signup

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

Jpita closed this task as Resolved.Aug 8 2019, 9:16 PM