Tagged UBN! because patch cannot be reverted cleanly.

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.

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.

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

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.

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.

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).

In T228025#5340086, @Nikerabbit wrote:

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).

In T228025#5340809, @Tgr wrote:

In T228025#5340086, @Nikerabbit wrote:

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.

In T228025#5344500, @Nikerabbit wrote:

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

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.

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

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

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