AccountManager: Create account and username atomically

So far the account was created first and then in a second transaction
the external ID for the username was added. This meant that the account
creation needed to be rolled back if the creation of the external ID for
the username failed.

With this change the account and the external ID for the username are
now created within the same transaction.

ChangeUserName is now only used by PutUsername and hence it is merged
into this class.

Change-Id: I8f06f792b02331d10b943a52c6c890830087caec
Signed-off-by: Edwin Kempin <ekempin@google.com>

java/com/google/gerrit/server/account/AccountManager.java

@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.account;
 
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
+
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -21,7 +24,6 @@ import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.common.errors.NameAlreadyUsedException;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.extensions.client.AccountFieldName;
 import com.google.gerrit.reviewdb.client.Account;
@@ -39,6 +41,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.ssh.SshKeyCache;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -67,7 +70,7 @@ public class AccountManager {
   private final AccountCache byIdCache;
   private final Realm realm;
   private final IdentifiedUser.GenericFactory userFactory;
-  private final ChangeUserName.Factory changeUserNameFactory;
+  private final SshKeyCache sshKeyCache;
   private final ProjectCache projectCache;
   private final AtomicBoolean awaitsFirstAccountCheck;
   private final ExternalIds externalIds;
@@ -86,7 +89,7 @@ public class AccountManager {
       AccountCache byIdCache,
       Realm accountMapper,
       IdentifiedUser.GenericFactory userFactory,
-      ChangeUserName.Factory changeUserNameFactory,
+      SshKeyCache sshKeyCache,
       ProjectCache projectCache,
       ExternalIds externalIds,
       ExternalIdsUpdate.Server externalIdsUpdateFactory,
@@ -99,7 +102,7 @@ public class AccountManager {
     this.byIdCache = byIdCache;
     this.realm = accountMapper;
     this.userFactory = userFactory;
-    this.changeUserNameFactory = changeUserNameFactory;
+    this.sshKeyCache = sshKeyCache;
     this.projectCache = projectCache;
     this.awaitsFirstAccountCheck =
         new AtomicBoolean(cfg.getBoolean("capability", "makeFirstUserAdmin", true));
@@ -262,6 +265,8 @@ public class AccountManager {
 
     ExternalId extId =
         ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress());
+    ExternalId userNameExtId =
+        !Strings.isNullOrEmpty(who.getUserName()) ? createUsername(newId, who.getUserName()) : null;
 
     boolean isFirstAccount = awaitsFirstAccountCheck.getAndSet(false) && !accounts.hasAnyAccount();
 
@@ -273,10 +278,14 @@ public class AccountManager {
               .insert(
                   "Create Account on First Login",
                   newId,
-                  u ->
+                  u -> {
-                      u.setFullName(who.getDisplayName())
+                    u.setFullName(who.getDisplayName())
-                          .setPreferredEmail(extId.email())
+                        .setPreferredEmail(extId.email())
-                          .addExternalId(extId));
+                        .addExternalId(extId);
+                    if (userNameExtId != null) {
+                      u.addExternalId(userNameExtId);
+                    }
+                  });
     } catch (DuplicateExternalIdKeyException e) {
       throw new AccountException(
           "Cannot assign external ID \""
@@ -291,6 +300,10 @@ public class AccountManager {
       awaitsFirstAccountCheck.set(isFirstAccount);
     }
 
+    if (userNameExtId != null) {
+      sshKeyCache.evict(who.getUserName());
+    }
+
     IdentifiedUser user = userFactory.create(newId);
 
     if (isFirstAccount) {
@@ -309,37 +322,23 @@ public class AccountManager {
       addGroupMember(db, adminGroupUuid, user);
     }
 
-    if (who.getUserName() != null) {
-      // Only set if the name hasn't been used yet, but was given to us.
-      //
-      try {
-        changeUserNameFactory.create("Set Username on Login", user, who.getUserName()).call();
-      } catch (NameAlreadyUsedException e) {
-        String message =
-            "Cannot assign user name \""
-                + who.getUserName()
-                + "\" to account "
-                + newId
-                + "; name already in use.";
-        handleSettingUserNameFailure(account, extId, message, e, false);
-      } catch (InvalidUserNameException e) {
-        String message =
-            "Cannot assign user name \""
-                + who.getUserName()
-                + "\" to account "
-                + newId
-                + "; name does not conform.";
-        handleSettingUserNameFailure(account, extId, message, e, false);
-      } catch (OrmException e) {
-        String message = "Cannot assign user name";
-        handleSettingUserNameFailure(account, extId, message, e, true);
-      }
-    }
-
     realm.onCreateAccount(who, account);
     return new AuthResult(newId, extId.key(), true);
   }
 
+  private ExternalId createUsername(Account.Id accountId, String username)
+      throws AccountUserNameException {
+    checkArgument(!Strings.isNullOrEmpty(username));
+
+    if (!ExternalId.isValidUsername(username)) {
+      throw new AccountUserNameException(
+          String.format(
+              "Cannot assign user name \"%s\" to account %s; name does not conform.",
+              username, accountId));
+    }
+    return ExternalId.create(SCHEME_USERNAME, username, accountId);
+  }
+
   private void addGroupMember(ReviewDb db, AccountGroup.UUID groupUuid, IdentifiedUser user)
       throws OrmException, IOException, ConfigInvalidException, AccountException {
     // The user initiated this request by logging in. -> Attribute all modifications to that user.
@@ -356,43 +355,6 @@ public class AccountManager {
     }
   }
 
-  /**
-   * This method handles an exception that occurred during the setting of the user name for a newly
-   * created account. If the realm does not allow the user to set a user name manually this method
-   * deletes the newly created account and throws an {@link AccountUserNameException}. In any case
-   * the error message is logged.
-   *
-   * @param account the newly created account
-   * @param extId the newly created external id
-   * @param errorMessage the error message
-   * @param e the exception that occurred during the setting of the user name for the new account
-   * @param logException flag that decides whether the exception should be included into the log
-   * @throws AccountUserNameException thrown if the realm does not allow the user to manually set
-   *     the user name
-   * @throws OrmException thrown if cleaning the database failed
-   */
-  private void handleSettingUserNameFailure(
-      Account account, ExternalId extId, String errorMessage, Exception e, boolean logException)
-      throws AccountUserNameException, OrmException, IOException, ConfigInvalidException {
-    if (logException) {
-      log.error(errorMessage, e);
-    } else {
-      log.error(errorMessage);
-    }
-    if (!realm.allowsEdit(AccountFieldName.USER_NAME)) {
-      // setting the given user name has failed, but the realm does not
-      // allow the user to manually set a user name,
-      // this means we would end with an account without user name
-      // (without 'username:<USERNAME>' external ID),
-      // such an account cannot be used for uploading changes,
-      // this is why the best we can do here is to fail early and cleanup
-      // the database
-      accountsUpdateFactory.create().delete(account);
-      externalIdsUpdateFactory.create().delete(extId);
-      throw new AccountUserNameException(errorMessage, e);
-    }
-  }
-
   /**
    * Link another authentication identity to an existing account.
    *

java/com/google/gerrit/server/account/AccountUserNameException.java

@@ -21,6 +21,10 @@ package com.google.gerrit.server.account;
 public class AccountUserNameException extends AccountException {
   private static final long serialVersionUID = 1L;
 
+  public AccountUserNameException(String message) {
+    super(message);
+  }
+
   public AccountUserNameException(String message, Throwable why) {
     super(message, why);
   }

java/com/google/gerrit/server/account/ChangeUserName.java

@@ -1,112 +0,0 @@
-// Copyright (C) 2009 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.account;
-
-import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
-
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.errors.NameAlreadyUsedException;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.externalids.ExternalId;
-import com.google.gerrit.server.account.externalids.ExternalIds;
-import com.google.gerrit.server.ssh.SshKeyCache;
-import com.google.gwtjsonrpc.common.VoidResult;
-import com.google.gwtorm.server.OrmDuplicateKeyException;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
-import java.util.concurrent.Callable;
-import java.util.regex.Pattern;
-import org.eclipse.jgit.errors.ConfigInvalidException;
-
-/** Operation to change the username of an account. */
-public class ChangeUserName implements Callable<VoidResult> {
-  public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed.";
-
-  private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN);
-
-  /** Generic factory to change any user's username. */
-  public interface Factory {
-    ChangeUserName create(
-        @Assisted("message") String message,
-        IdentifiedUser user,
-        @Assisted("newUsername") String newUsername);
-  }
-
-  private final SshKeyCache sshKeyCache;
-  private final ExternalIds externalIds;
-  private final AccountsUpdate.Server accountsUpdate;
-
-  private final String message;
-  private final IdentifiedUser user;
-  private final String newUsername;
-
-  @Inject
-  ChangeUserName(
-      SshKeyCache sshKeyCache,
-      ExternalIds externalIds,
-      AccountsUpdate.Server accountsUpdate,
-      @Assisted("message") String message,
-      @Assisted IdentifiedUser user,
-      @Nullable @Assisted("newUsername") String newUsername) {
-    this.sshKeyCache = sshKeyCache;
-    this.externalIds = externalIds;
-    this.accountsUpdate = accountsUpdate;
-    this.message = message;
-    this.user = user;
-    this.newUsername = newUsername;
-  }
-
-  @Override
-  public VoidResult call()
-      throws OrmException, NameAlreadyUsedException, InvalidUserNameException, IOException,
-          ConfigInvalidException {
-    if (!externalIds.byAccount(user.getAccountId(), SCHEME_USERNAME).isEmpty()) {
-      throw new IllegalStateException(USERNAME_CANNOT_BE_CHANGED);
-    }
-
-    if (newUsername != null && !newUsername.isEmpty()) {
-      if (!USER_NAME_PATTERN.matcher(newUsername).matches()) {
-        throw new InvalidUserNameException();
-      }
-
-      ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, newUsername);
-      try {
-        accountsUpdate
-            .create()
-            .update(
-                message,
-                user.getAccountId(),
-                u -> u.addExternalId(ExternalId.create(key, user.getAccountId(), null, null)));
-      } catch (OrmDuplicateKeyException dupeErr) {
-        // If we are using this identity, don't report the exception.
-        //
-        ExternalId other = externalIds.get(key);
-        if (other != null && other.accountId().equals(user.getAccountId())) {
-          return VoidResult.INSTANCE;
-        }
-
-        // Otherwise, someone else has this identity.
-        //
-        throw new NameAlreadyUsedException(newUsername);
-      }
-    }
-
-    sshKeyCache.evict(newUsername);
-    return VoidResult.INSTANCE;
-  }
-}

java/com/google/gerrit/server/account/externalids/ExternalId.java

@@ -33,6 +33,7 @@ import java.io.Serializable;
 import java.util.Collection;
 import java.util.Objects;
 import java.util.Set;
+import java.util.regex.Pattern;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
@@ -40,6 +41,12 @@ import org.eclipse.jgit.lib.ObjectId;
 
 @AutoValue
 public abstract class ExternalId implements Serializable {
+  private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN);
+
+  public static boolean isValidUsername(String username) {
+    return USER_NAME_PATTERN.matcher(username).matches();
+  }
+
   private static final long serialVersionUID = 1L;
 
   private static final String EXTERNAL_ID_SECTION = "externalId";

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -86,7 +86,6 @@ import com.google.gerrit.server.account.AccountManager;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.AccountVisibilityProvider;
 import com.google.gerrit.server.account.CapabilityCollection;
-import com.google.gerrit.server.account.ChangeUserName;
 import com.google.gerrit.server.account.EmailExpander;
 import com.google.gerrit.server.account.GroupCacheImpl;
 import com.google.gerrit.server.account.GroupControl;
@@ -419,7 +418,6 @@ public class GerritGlobalModule extends FactoryModule {
     factory(VersionedAuthorizedKeys.Factory.class);
 
     bind(AccountManager.class);
-    factory(ChangeUserName.Factory.class);
 
     bind(new TypeLiteral<List<CommentLinkInfo>>() {})
         .toProvider(CommentLinkProvider.class)

java/com/google/gerrit/server/restapi/account/PutUsername.java

@@ -14,7 +14,9 @@
 
 package com.google.gerrit.server.restapi.account;
 
-import com.google.gerrit.common.errors.NameAlreadyUsedException;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
+
+import com.google.common.base.Strings;
 import com.google.gerrit.extensions.api.accounts.UsernameInput;
 import com.google.gerrit.extensions.client.AccountFieldName;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -22,14 +24,18 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.account.AccountResource;
-import com.google.gerrit.server.account.ChangeUserName;
+import com.google.gerrit.server.account.AccountsUpdate;
-import com.google.gerrit.server.account.InvalidUserNameException;
 import com.google.gerrit.server.account.Realm;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.permissions.GlobalPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.ssh.SshKeyCache;
+import com.google.gwtorm.server.OrmDuplicateKeyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -40,19 +46,25 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 @Singleton
 public class PutUsername implements RestModifyView<AccountResource, UsernameInput> {
   private final Provider<CurrentUser> self;
-  private final ChangeUserName.Factory changeUserNameFactory;
   private final PermissionBackend permissionBackend;
+  private final ExternalIds externalIds;
+  private final AccountsUpdate.Server accountsUpdate;
+  private final SshKeyCache sshKeyCache;
   private final Realm realm;
 
   @Inject
   PutUsername(
       Provider<CurrentUser> self,
-      ChangeUserName.Factory changeUserNameFactory,
       PermissionBackend permissionBackend,
+      ExternalIds externalIds,
+      AccountsUpdate.Server accountsUpdate,
+      SshKeyCache sshKeyCache,
       Realm realm) {
     this.self = self;
-    this.changeUserNameFactory = changeUserNameFactory;
     this.permissionBackend = permissionBackend;
+    this.externalIds = externalIds;
+    this.accountsUpdate = accountsUpdate;
+    this.sshKeyCache = sshKeyCache;
     this.realm = realm;
   }
 
@@ -73,19 +85,39 @@ public class PutUsername implements RestModifyView<AccountResource, UsernameInpu
       input = new UsernameInput();
     }
 
-    try {
+    Account.Id accountId = rsrc.getUser().getAccountId();
-      changeUserNameFactory.create("Set Username via API", rsrc.getUser(), input.username).call();
+    if (!externalIds.byAccount(accountId, SCHEME_USERNAME).isEmpty()) {
-    } catch (IllegalStateException e) {
+      throw new MethodNotAllowedException("Username cannot be changed.");
-      if (ChangeUserName.USERNAME_CANNOT_BE_CHANGED.equals(e.getMessage())) {
+    }
-        throw new MethodNotAllowedException(e.getMessage());
+
-      }
+    if (Strings.isNullOrEmpty(input.username)) {
-      throw e;
+      return input.username;
-    } catch (InvalidUserNameException e) {
+    }
+
+    if (!ExternalId.isValidUsername(input.username)) {
       throw new UnprocessableEntityException("invalid username");
-    } catch (NameAlreadyUsedException e) {
+    }
+
+    ExternalId.Key key = ExternalId.Key.create(SCHEME_USERNAME, input.username);
+    try {
+      accountsUpdate
+          .create()
+          .update(
+              "Set Username via API",
+              accountId,
+              u -> u.addExternalId(ExternalId.create(key, accountId, null, null)));
+    } catch (OrmDuplicateKeyException dupeErr) {
+      // If we are using this identity, don't report the exception.
+      ExternalId other = externalIds.get(key);
+      if (other != null && other.accountId().equals(accountId)) {
+        return input.username;
+      }
+
+      // Otherwise, someone else has this identity.
       throw new ResourceConflictException("username already used");
     }
 
+    sshKeyCache.evict(input.username);
     return input.username;
   }
 }