Merge changes from topic "group-resolver"

* changes:
  AddReviewersOp: Get user and ReviewDb from context
  Move ReviewerAdder and its dependencies out of restapi package
  Rename PostReviewers{Op,Email} to AddReviewers{Op,Email}
  Move most methods from AccountsCollection to AccountResolver
  Factor a non-restapi GroupResolver out of GroupsCollection
  Move common reviewer addition code out of PostReviewers
  PostReviewersOp: Pass correct to/CC arguments to emailReviewers
  AbstractNotificationTest: Improve readability of failure message
  Add push tests for auto-adding forged identities as reviewers
  Add tests for re-adding the same reviewers
  Add notification tests for adding reviewer/CC by email

java/com/google/gerrit/acceptance/AbstractNotificationTest.java

@@ -195,8 +195,8 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest {
       if (recipients.get(type).contains(email) != expected) {
         failWithoutActual(
             fact(
-                expected ? "notifies" : "doesn't notify",
+                expected ? "should notify" : "shouldn't notify",
-                "[\n" + type + ": " + users.emailToName(email) + "\n]"));
+                type + ": " + users.emailToName(email)));
       }
       if (expected) {
         accountedFor.add(email);

java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java

@@ -38,7 +38,7 @@ public class ReviewerInfo extends AccountInfo {
 
   @Override
   public String toString() {
-    return username;
+    return username != null ? username : email;
   }
 
   private ReviewerInfo() {}

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

@@ -19,7 +19,13 @@ import static java.util.stream.Collectors.toSet;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Streams;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.query.account.InternalAccountQuery;
 import com.google.gwtorm.server.OrmException;
@@ -38,22 +44,31 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class AccountResolver {
+  private final Provider<CurrentUser> self;
   private final Realm realm;
   private final Accounts accounts;
   private final AccountCache byId;
+  private final IdentifiedUser.GenericFactory userFactory;
+  private final AccountControl.Factory accountControlFactory;
   private final Provider<InternalAccountQuery> accountQueryProvider;
   private final Emails emails;
 
   @Inject
   AccountResolver(
+      Provider<CurrentUser> self,
       Realm realm,
       Accounts accounts,
       AccountCache byId,
+      IdentifiedUser.GenericFactory userFactory,
+      AccountControl.Factory accountControlFactory,
       Provider<InternalAccountQuery> accountQueryProvider,
       Emails emails) {
+    this.self = self;
     this.realm = realm;
     this.accounts = accounts;
     this.byId = byId;
+    this.userFactory = userFactory;
+    this.accountControlFactory = accountControlFactory;
     this.accountQueryProvider = accountQueryProvider;
     this.emails = emails;
   }
@@ -197,4 +212,74 @@ public class AccountResolver {
         .map(a -> a.getAccount().getId())
         .collect(toSet());
   }
+
+  /**
+   * Parses a account ID from a request body and returns the user.
+   *
+   * @param id ID of the account, can be a string of the format "{@code Full Name
+   *     <email@example.com>}", just the email address, a full name if it is unique, an account ID,
+   *     a user name or "{@code self}" for the calling user
+   * @return the user, never null.
+   * @throws UnprocessableEntityException thrown if the account ID cannot be resolved or if the
+   *     account is not visible to the calling user
+   */
+  public IdentifiedUser parse(String id)
+      throws AuthException, UnprocessableEntityException, OrmException, IOException,
+          ConfigInvalidException {
+    return parseOnBehalfOf(null, id);
+  }
+
+  /**
+   * Parses an account ID and returns the user without making any permission check whether the
+   * current user can see the account.
+   *
+   * @param id ID of the account, can be a string of the format "{@code Full Name
+   *     <email@example.com>}", just the email address, a full name if it is unique, an account ID,
+   *     a user name or "{@code self}" for the calling user
+   * @return the user, null if no user is found for the given account ID
+   * @throws AuthException thrown if 'self' is used as account ID and the current user is not
+   *     authenticated
+   * @throws OrmException
+   * @throws ConfigInvalidException
+   * @throws IOException
+   */
+  public IdentifiedUser parseId(String id)
+      throws AuthException, OrmException, IOException, ConfigInvalidException {
+    return parseIdOnBehalfOf(null, id);
+  }
+
+  /**
+   * Like {@link #parse(String)}, but also sets the {@link CurrentUser#getRealUser()} on the result.
+   */
+  public IdentifiedUser parseOnBehalfOf(@Nullable CurrentUser caller, String id)
+      throws AuthException, UnprocessableEntityException, OrmException, IOException,
+          ConfigInvalidException {
+    IdentifiedUser user = parseIdOnBehalfOf(caller, id);
+    if (user == null || !accountControlFactory.get().canSee(user.getAccount())) {
+      throw new UnprocessableEntityException(
+          String.format("Account '%s' is not found or ambiguous", id));
+    }
+    return user;
+  }
+
+  private IdentifiedUser parseIdOnBehalfOf(@Nullable CurrentUser caller, String id)
+      throws AuthException, OrmException, IOException, ConfigInvalidException {
+    if (id.equals("self")) {
+      CurrentUser user = self.get();
+      if (user.isIdentifiedUser()) {
+        return user.asIdentifiedUser();
+      } else if (user instanceof AnonymousUser) {
+        throw new AuthException("Authentication required");
+      } else {
+        return null;
+      }
+    }
+
+    Account match = find(id);
+    if (match == null) {
+      return null;
+    }
+    CurrentUser realUser = caller != null ? caller.getRealUser() : null;
+    return userFactory.runAs(null, match.getId(), realUser);
+  }
 }

java/com/google/gerrit/server/api/groups/GroupsImpl.java

@@ -26,10 +26,11 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.permissions.GlobalPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.project.ProjectResource;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.gerrit.server.restapi.group.CreateGroup;
 import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.gerrit.server.restapi.group.ListGroups;
@@ -43,8 +44,9 @@ import java.util.SortedMap;
 
 @Singleton
 class GroupsImpl implements Groups {
-  private final AccountsCollection accounts;
+  private final AccountResolver accountResolver;
   private final GroupsCollection groups;
+  private final GroupResolver groupResolver;
   private final ProjectsCollection projects;
   private final Provider<ListGroups> listGroups;
   private final Provider<QueryGroups> queryGroups;
@@ -54,16 +56,18 @@ class GroupsImpl implements Groups {
 
   @Inject
   GroupsImpl(
-      AccountsCollection accounts,
+      AccountResolver accountResolver,
       GroupsCollection groups,
+      GroupResolver groupResolver,
       ProjectsCollection projects,
       Provider<ListGroups> listGroups,
       Provider<QueryGroups> queryGroups,
       PermissionBackend permissionBackend,
       CreateGroup createGroup,
       GroupApiImpl.Factory api) {
-    this.accounts = accounts;
+    this.accountResolver = accountResolver;
     this.groups = groups;
+    this.groupResolver = groupResolver;
     this.projects = projects;
     this.listGroups = listGroups;
     this.queryGroups = queryGroups;
@@ -126,7 +130,7 @@ class GroupsImpl implements Groups {
     }
 
     for (String group : req.getGroups()) {
-      list.addGroup(groups.parse(group).getGroupUUID());
+      list.addGroup(groupResolver.parse(group).getGroupUUID());
     }
 
     list.setVisibleToAll(req.getVisibleToAll());
@@ -137,7 +141,7 @@ class GroupsImpl implements Groups {
 
     if (req.getUser() != null) {
       try {
-        list.setUser(accounts.parse(req.getUser()).getAccountId());
+        list.setUser(accountResolver.parse(req.getUser()).getAccountId());
       } catch (Exception e) {
         throw asRestApiException("Error looking up user " + req.getUser(), e);
       }

java/com/google/gerrit/server/change/AddReviewersEmail.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.server.restapi.change;
+package com.google.gerrit.server.change;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
@@ -32,17 +32,17 @@ import com.google.inject.Singleton;
 import java.util.Collection;
 
 @Singleton
-class PostReviewersEmail {
+public class AddReviewersEmail {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final AddReviewerSender.Factory addReviewerSenderFactory;
 
   @Inject
-  PostReviewersEmail(AddReviewerSender.Factory addReviewerSenderFactory) {
+  AddReviewersEmail(AddReviewerSender.Factory addReviewerSenderFactory) {
     this.addReviewerSenderFactory = addReviewerSenderFactory;
   }
 
-  void emailReviewers(
+  public void emailReviewers(
       IdentifiedUser user,
       Change change,
       Collection<Account.Id> added,

java/com/google/gerrit/server/change/AddReviewersOp.java

@@ -12,16 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.server.restapi.change;
+package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
 import static java.util.stream.Collectors.toList;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Streams;
@@ -35,9 +37,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
-import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountState;
@@ -50,19 +50,32 @@ import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.Context;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 
-public class PostReviewersOp implements BatchUpdateOp {
+public class AddReviewersOp implements BatchUpdateOp {
   public interface Factory {
-    PostReviewersOp create(
+
-        Set<Account.Id> reviewers,
+    /**
-        Collection<Address> reviewersByEmail,
+     * Create a new op.
+     *
+     * <p>Users may be added by account or by email addresses, as determined by {@code accountIds}
+     * and {@code addresses}. The reviewer state for both accounts and email addresses is determined
+     * by {@code state}.
+     *
+     * @param accountIds account IDs to add.
+     * @param addresses email addresses to add.
+     * @param state resulting reviewer state.
+     * @param notify notification handling.
+     * @param accountsToNotify additional accounts to notify.
+     * @return batch update operation.
+     */
+    AddReviewersOp create(
+        Set<Account.Id> accountIds,
+        Collection<Address> addresses,
         ReviewerState state,
         @Nullable NotifyHandling notify,
         ListMultimap<RecipientType, Account.Id> accountsToNotify);
@@ -72,17 +85,25 @@ public class PostReviewersOp implements BatchUpdateOp {
   public abstract static class Result {
     public abstract ImmutableList<PatchSetApproval> addedReviewers();
 
+    public abstract ImmutableList<Address> addedReviewersByEmail();
+
     public abstract ImmutableList<Account.Id> addedCCs();
 
+    public abstract ImmutableList<Address> addedCCsByEmail();
+
     static Builder builder() {
-      return new AutoValue_PostReviewersOp_Result.Builder();
+      return new AutoValue_AddReviewersOp_Result.Builder();
     }
 
     @AutoValue.Builder
     abstract static class Builder {
-      abstract Builder setAddedReviewers(ImmutableList<PatchSetApproval> addedReviewers);
+      abstract Builder setAddedReviewers(Iterable<PatchSetApproval> addedReviewers);
 
-      abstract Builder setAddedCCs(ImmutableList<Account.Id> addedCCs);
+      abstract Builder setAddedReviewersByEmail(Iterable<Address> addedReviewersByEmail);
+
+      abstract Builder setAddedCCs(Iterable<Account.Id> addedCCs);
+
+      abstract Builder setAddedCCsByEmail(Iterable<Address> addedCCsByEmail);
 
       abstract Result build();
     }
@@ -93,36 +114,36 @@ public class PostReviewersOp implements BatchUpdateOp {
   private final ReviewerAdded reviewerAdded;
   private final AccountCache accountCache;
   private final ProjectCache projectCache;
-  private final PostReviewersEmail postReviewersEmail;
+  private final AddReviewersEmail addReviewersEmail;
   private final NotesMigration migration;
-  private final Provider<IdentifiedUser> user;
+  private final Set<Account.Id> accountIds;
-  private final Provider<ReviewDb> dbProvider;
+  private final Collection<Address> addresses;
-  private final Set<Account.Id> reviewers;
-  private final Collection<Address> reviewersByEmail;
   private final ReviewerState state;
   private final NotifyHandling notify;
   private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
 
-  private List<PatchSetApproval> addedReviewers = new ArrayList<>();
+  // Unlike addedCCs, addedReviewers is a PatchSetApproval because the AddReviewerResult returned
-  private Collection<Account.Id> addedCCs = new ArrayList<>();
+  // via the REST API is supposed to include vote information.
-  private Collection<Address> addedCCsByEmail = new ArrayList<>();
+  private List<PatchSetApproval> addedReviewers = ImmutableList.of();
+  private Collection<Address> addedReviewersByEmail = ImmutableList.of();
+  private Collection<Account.Id> addedCCs = ImmutableList.of();
+  private Collection<Address> addedCCsByEmail = ImmutableList.of();
+
   private Change change;
   private PatchSet patchSet;
   private Result opResult;
 
   @Inject
-  PostReviewersOp(
+  AddReviewersOp(
       ApprovalsUtil approvalsUtil,
       PatchSetUtil psUtil,
       ReviewerAdded reviewerAdded,
       AccountCache accountCache,
       ProjectCache projectCache,
-      PostReviewersEmail postReviewersEmail,
+      AddReviewersEmail addReviewersEmail,
       NotesMigration migration,
-      Provider<IdentifiedUser> user,
+      @Assisted Set<Account.Id> accountIds,
-      Provider<ReviewDb> dbProvider,
+      @Assisted Collection<Address> addresses,
-      @Assisted Set<Account.Id> reviewers,
-      @Assisted Collection<Address> reviewersByEmail,
       @Assisted ReviewerState state,
       @Assisted @Nullable NotifyHandling notify,
       @Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify) {
@@ -132,13 +153,11 @@ public class PostReviewersOp implements BatchUpdateOp {
     this.reviewerAdded = reviewerAdded;
     this.accountCache = accountCache;
     this.projectCache = projectCache;
-    this.postReviewersEmail = postReviewersEmail;
+    this.addReviewersEmail = addReviewersEmail;
     this.migration = migration;
-    this.user = user;
-    this.dbProvider = dbProvider;
 
-    this.reviewers = reviewers;
+    this.accountIds = accountIds;
-    this.reviewersByEmail = reviewersByEmail;
+    this.addresses = addresses;
     this.state = state;
     this.notify = notify;
     this.accountsToNotify = accountsToNotify;
@@ -148,14 +167,11 @@ public class PostReviewersOp implements BatchUpdateOp {
   public boolean updateChange(ChangeContext ctx)
       throws RestApiException, OrmException, IOException {
     change = ctx.getChange();
-    if (!reviewers.isEmpty()) {
+    if (!accountIds.isEmpty()) {
       if (migration.readChanges() && state == CC) {
         addedCCs =
             approvalsUtil.addCcs(
-                ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), reviewers);
+                ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), accountIds);
-        if (addedCCs.isEmpty()) {
-          return false;
-        }
       } else {
         addedReviewers =
             approvalsUtil.addReviewers(
@@ -164,35 +180,76 @@ public class PostReviewersOp implements BatchUpdateOp {
                 ctx.getUpdate(change.currentPatchSetId()),
                 projectCache.checkedGet(change.getProject()).getLabelTypes(change.getDest()),
                 change,
-                reviewers);
+                accountIds);
-        if (addedReviewers.isEmpty()) {
+      }
+    }
+
+    ImmutableList<Address> addressesToAdd = ImmutableList.of();
+    ReviewerStateInternal internalState = ReviewerStateInternal.fromReviewerState(state);
+    if (migration.readChanges()) {
+      // TODO(dborowitz): This behavior should live in ApprovalsUtil or something, like addCcs does.
+      ImmutableSet<Address> existing = ctx.getNotes().getReviewersByEmail().byState(internalState);
+      addressesToAdd =
+          addresses.stream().filter(a -> !existing.contains(a)).collect(toImmutableList());
+
+      if (state == CC) {
+        addedCCsByEmail = addressesToAdd;
+      } else {
+        addedReviewersByEmail = addressesToAdd;
+      }
+      for (Address a : addressesToAdd) {
+        ctx.getUpdate(change.currentPatchSetId()).putReviewerByEmail(a, internalState);
+      }
+    }
+    if (addedCCs.isEmpty() && addedReviewers.isEmpty() && addressesToAdd.isEmpty()) {
       return false;
     }
-      }
-    }
 
-    for (Address a : reviewersByEmail) {
+    checkAdded();
-      ctx.getUpdate(change.currentPatchSetId())
-          .putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
-    }
 
-    patchSet = psUtil.current(dbProvider.get(), ctx.getNotes());
+    patchSet = psUtil.current(ctx.getDb(), ctx.getNotes());
     return true;
   }
 
+  private void checkAdded() {
+    // Should only affect either reviewers or CCs, not both. But the logic in updateChange is
+    // complex, so programmer error is conceivable.
+    boolean addedAnyReviewers = !addedReviewers.isEmpty() || !addedReviewersByEmail.isEmpty();
+    boolean addedAnyCCs = !addedCCs.isEmpty() || !addedCCsByEmail.isEmpty();
+    checkState(
+        !(addedAnyReviewers && addedAnyCCs),
+        "should not have added both reviewers and CCs:\n"
+            + "Arguments:\n"
+            + "  accountIds=%s\n"
+            + "  addresses=%s\n"
+            + "Results:\n"
+            + "  addedReviewers=%s\n"
+            + "  addedReviewersByEmail=%s\n"
+            + "  addedCCs=%s\n"
+            + "  addedCCsByEmail=%s",
+        accountIds,
+        addresses,
+        addedReviewers,
+        addedReviewersByEmail,
+        addedCCs,
+        addedCCsByEmail);
+  }
+
   @Override
   public void postUpdate(Context ctx) throws Exception {
     opResult =
         Result.builder()
-            .setAddedReviewers(ImmutableList.copyOf(addedReviewers))
+            .setAddedReviewers(addedReviewers)
-            .setAddedCCs(ImmutableList.copyOf(addedCCs))
+            .setAddedReviewersByEmail(addedReviewersByEmail)
+            .setAddedCCs(addedCCs)
+            .setAddedCCsByEmail(addedCCsByEmail)
             .build();
-    postReviewersEmail.emailReviewers(
+    addReviewersEmail.emailReviewers(
-        user.get(),
+        ctx.getUser().asIdentifiedUser(),
         change,
         Lists.transform(addedReviewers, PatchSetApproval::getAccountId),
-        addedCCs == null ? ImmutableList.of() : addedCCs,
+        addedCCs,
-        reviewersByEmail,
+        addedReviewersByEmail,
         addedCCsByEmail,
         notify,
         accountsToNotify,

java/com/google/gerrit/server/change/ReviewerAdder.java

@@ -0,0 +1,473 @@
+// Copyright (C) 2018 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.change;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.extensions.client.ReviewerState.CC;
+import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Lists;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GroupDescription;
+import com.google.gerrit.extensions.api.changes.AddReviewerInput;
+import com.google.gerrit.extensions.api.changes.AddReviewerResult;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
+import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.mail.Address;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
+import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.GroupMembers;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.group.GroupResolver;
+import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.RefPermission;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import java.io.IOException;
+import java.text.MessageFormat;
+import java.util.HashSet;
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+
+public class ReviewerAdder {
+  public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
+  public static final int DEFAULT_MAX_REVIEWERS = 20;
+
+  private final AccountResolver accountResolver;
+  private final PermissionBackend permissionBackend;
+  private final GroupResolver groupResolver;
+  private final GroupMembers groupMembers;
+  private final AccountLoader.Factory accountLoaderFactory;
+  private final Provider<ReviewDb> dbProvider;
+  private final Config cfg;
+  private final ReviewerJson json;
+  private final NotesMigration migration;
+  private final NotifyUtil notifyUtil;
+  private final ProjectCache projectCache;
+  private final Provider<AnonymousUser> anonymousProvider;
+  private final AddReviewersOp.Factory addReviewersOpFactory;
+  private final OutgoingEmailValidator validator;
+
+  @Inject
+  ReviewerAdder(
+      AccountResolver accountResolver,
+      PermissionBackend permissionBackend,
+      GroupResolver groupResolver,
+      GroupMembers groupMembers,
+      AccountLoader.Factory accountLoaderFactory,
+      Provider<ReviewDb> db,
+      @GerritServerConfig Config cfg,
+      ReviewerJson json,
+      NotesMigration migration,
+      NotifyUtil notifyUtil,
+      ProjectCache projectCache,
+      Provider<AnonymousUser> anonymousProvider,
+      AddReviewersOp.Factory addReviewersOpFactory,
+      OutgoingEmailValidator validator) {
+    this.accountResolver = accountResolver;
+    this.permissionBackend = permissionBackend;
+    this.groupResolver = groupResolver;
+    this.groupMembers = groupMembers;
+    this.accountLoaderFactory = accountLoaderFactory;
+    this.dbProvider = db;
+    this.cfg = cfg;
+    this.json = json;
+    this.migration = migration;
+    this.notifyUtil = notifyUtil;
+    this.projectCache = projectCache;
+    this.anonymousProvider = anonymousProvider;
+    this.addReviewersOpFactory = addReviewersOpFactory;
+    this.validator = validator;
+  }
+
+  /**
+   * Prepare application of a single {@link AddReviewerInput}.
+   *
+   * @param notes change notes.
+   * @param user user performing the reviewer addition.
+   * @param input input describing user or group to add as a reviewer.
+   * @param allowGroup whether to allow
+   * @return handle describing the addition operation. If the {@code op} field is present, this
+   *     operation may be added to a {@code BatchUpdate}. Otherwise, the {@code error} field
+   *     contains information about an error that occurred
+   * @throws OrmException
+   * @throws IOException
+   * @throws PermissionBackendException
+   * @throws ConfigInvalidException
+   */
+  public ReviewerAddition prepare(
+      ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup)
+      throws OrmException, IOException, PermissionBackendException, ConfigInvalidException {
+    Branch.NameKey dest = notes.getChange().getDest();
+    String reviewer = checkNotNull(input.reviewer);
+    ReviewerState state = input.state();
+    NotifyHandling notify = input.notify;
+    ListMultimap<RecipientType, Account.Id> accountsToNotify;
+    try {
+      accountsToNotify = notifyUtil.resolveAccounts(input.notifyDetails);
+    } catch (BadRequestException e) {
+      return fail(reviewer, e.getMessage());
+    }
+    boolean confirmed = input.confirmed();
+    boolean allowByEmail =
+        projectCache
+            .checkedGet(dest.getParentKey())
+            .is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
+
+    ReviewerAddition byAccountId =
+        addByAccountId(
+            reviewer, dest, user, state, notify, accountsToNotify, allowGroup, allowByEmail);
+
+    ReviewerAddition wholeGroup = null;
+    if (byAccountId == null || !byAccountId.exactMatchFound) {
+      wholeGroup =
+          addWholeGroup(
+              reviewer,
+              dest,
+              user,
+              state,
+              notify,
+              accountsToNotify,
+              confirmed,
+              allowGroup,
+              allowByEmail);
+      if (wholeGroup != null && wholeGroup.exactMatchFound) {
+        return wholeGroup;
+      }
+    }
+
+    if (byAccountId != null) {
+      return byAccountId;
+    }
+    if (wholeGroup != null) {
+      return wholeGroup;
+    }
+
+    return addByEmail(reviewer, notes, user, state, notify, accountsToNotify);
+  }
+
+  public ReviewerAddition ccCurrentUser(CurrentUser user, RevisionResource revision) {
+    return new ReviewerAddition(
+        user.getUserName().orElse(null),
+        revision.getUser(),
+        ImmutableSet.of(user.getAccountId()),
+        null,
+        CC,
+        NotifyHandling.NONE,
+        ImmutableListMultimap.of(),
+        true);
+  }
+
+  @Nullable
+  private ReviewerAddition addByAccountId(
+      String reviewer,
+      Branch.NameKey dest,
+      CurrentUser user,
+      ReviewerState state,
+      NotifyHandling notify,
+      ListMultimap<RecipientType, Account.Id> accountsToNotify,
+      boolean allowGroup,
+      boolean allowByEmail)
+      throws OrmException, PermissionBackendException, IOException, ConfigInvalidException {
+    IdentifiedUser reviewerUser;
+    boolean exactMatchFound = false;
+    try {
+      reviewerUser = accountResolver.parse(reviewer);
+      if (reviewer.equalsIgnoreCase(reviewerUser.getName())
+          || reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
+        exactMatchFound = true;
+      }
+    } catch (UnprocessableEntityException | AuthException e) {
+      // AuthException won't occur since the user is authenticated at this point.
+      if (!allowGroup && !allowByEmail) {
+        // Only return failure if we aren't going to try other interpretations.
+        return fail(
+            reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+      }
+      return null;
+    }
+
+    if (isValidReviewer(dest, reviewerUser.getAccount())) {
+      return new ReviewerAddition(
+          reviewer,
+          user,
+          ImmutableSet.of(reviewerUser.getAccountId()),
+          null,
+          state,
+          notify,
+          accountsToNotify,
+          exactMatchFound);
+    }
+    if (!reviewerUser.getAccount().isActive()) {
+      if (allowByEmail && state == CC) {
+        return null;
+      }
+      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInactive, reviewer));
+    }
+    return fail(
+        reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
+  }
+
+  @Nullable
+  private ReviewerAddition addWholeGroup(
+      String reviewer,
+      Branch.NameKey dest,
+      CurrentUser user,
+      ReviewerState state,
+      NotifyHandling notify,
+      ListMultimap<RecipientType, Account.Id> accountsToNotify,
+      boolean confirmed,
+      boolean allowGroup,
+      boolean allowByEmail)
+      throws IOException, PermissionBackendException {
+    if (!allowGroup) {
+      return null;
+    }
+
+    GroupDescription.Basic group;
+    try {
+      group = groupResolver.parseInternal(reviewer);
+    } catch (UnprocessableEntityException e) {
+      if (!allowByEmail) {
+        return fail(
+            reviewer,
+            MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, reviewer));
+      }
+      return null;
+    }
+
+    if (!isLegalReviewerGroup(group.getGroupUUID())) {
+      return fail(
+          reviewer, MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
+    }
+
+    Set<Account.Id> reviewers = new HashSet<>();
+    Set<Account> members;
+    try {
+      members = groupMembers.listAccounts(group.getGroupUUID(), dest.getParentKey());
+    } catch (NoSuchProjectException e) {
+      return fail(reviewer, e.getMessage());
+    }
+
+    // if maxAllowed is set to 0, it is allowed to add any number of
+    // reviewers
+    int maxAllowed = cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS);
+    if (maxAllowed > 0 && members.size() > maxAllowed) {
+      return fail(
+          reviewer,
+          MessageFormat.format(ChangeMessages.get().groupHasTooManyMembers, group.getName()));
+    }
+
+    // if maxWithoutCheck is set to 0, we never ask for confirmation
+    int maxWithoutConfirmation =
+        cfg.getInt("addreviewer", "maxWithoutConfirmation", DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
+    if (!confirmed && maxWithoutConfirmation > 0 && members.size() > maxWithoutConfirmation) {
+      return fail(
+          reviewer,
+          true,
+          MessageFormat.format(
+              ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
+    }
+
+    for (Account member : members) {
+      if (isValidReviewer(dest, member)) {
+        reviewers.add(member.getId());
+      }
+    }
+
+    return new ReviewerAddition(
+        reviewer, user, reviewers, null, state, notify, accountsToNotify, true);
+  }
+
+  @Nullable
+  private ReviewerAddition addByEmail(
+      String reviewer,
+      ChangeNotes notes,
+      CurrentUser user,
+      ReviewerState state,
+      NotifyHandling notify,
+      ListMultimap<RecipientType, Account.Id> accountsToNotify)
+      throws PermissionBackendException {
+    try {
+      permissionBackend
+          .user(anonymousProvider.get())
+          .database(dbProvider)
+          .change(notes)
+          .check(ChangePermission.READ);
+    } catch (AuthException e) {
+      return fail(
+          reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
+    }
+
+    if (!migration.readChanges()) {
+      // addByEmail depends on NoteDb.
+      return fail(
+          reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
+    }
+    Address adr = Address.tryParse(reviewer);
+    if (adr == null || !validator.isValid(adr.getEmail())) {
+      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
+    }
+    return new ReviewerAddition(
+        reviewer, user, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
+  }
+
+  private boolean isValidReviewer(Branch.NameKey branch, Account member)
+      throws PermissionBackendException {
+    if (!member.isActive()) {
+      return false;
+    }
+
+    try {
+      // Check ref permission instead of change permission, since change permissions take into
+      // account the private bit, whereas adding a user as a reviewer is explicitly allowing them to
+      // see private changes.
+      permissionBackend
+          .absentUser(member.getId())
+          .database(dbProvider)
+          .ref(branch)
+          .check(RefPermission.READ);
+      return true;
+    } catch (AuthException e) {
+      return false;
+    }
+  }
+
+  private ReviewerAddition fail(String reviewer, String error) {
+    return fail(reviewer, false, error);
+  }
+
+  private ReviewerAddition fail(String reviewer, boolean confirm, String error) {
+    ReviewerAddition addition = new ReviewerAddition(reviewer);
+    addition.result.confirm = confirm ? true : null;
+    addition.result.error = error;
+    return addition;
+  }
+
+  public class ReviewerAddition {
+    public final AddReviewerResult result;
+    @Nullable public final AddReviewersOp op;
+    public final ImmutableSet<Account.Id> reviewers;
+    public final ImmutableSet<Address> reviewersByEmail;
+    public final ReviewerState state;
+    @Nullable final IdentifiedUser caller;
+    final boolean exactMatchFound;
+
+    private ReviewerAddition(String reviewer) {
+      result = new AddReviewerResult(reviewer);
+      op = null;
+      reviewers = ImmutableSet.of();
+      reviewersByEmail = ImmutableSet.of();
+      state = REVIEWER;
+      caller = null;
+      exactMatchFound = false;
+    }
+
+    private ReviewerAddition(
+        String reviewer,
+        CurrentUser caller,
+        @Nullable Iterable<Account.Id> reviewers,
+        @Nullable Iterable<Address> reviewersByEmail,
+        ReviewerState state,
+        @Nullable NotifyHandling notify,
+        ListMultimap<RecipientType, Account.Id> accountsToNotify,
+        boolean exactMatchFound) {
+      checkArgument(
+          reviewers != null || reviewersByEmail != null,
+          "must have either reviewers or reviewersByEmail");
+
+      result = new AddReviewerResult(reviewer);
+      this.reviewers = reviewers == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewers);
+      this.reviewersByEmail =
+          reviewersByEmail == null ? ImmutableSet.of() : ImmutableSet.copyOf(reviewersByEmail);
+      this.state = state;
+      this.caller = caller.asIdentifiedUser();
+      op =
+          addReviewersOpFactory.create(
+              this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
+      this.exactMatchFound = exactMatchFound;
+    }
+
+    public void gatherResults(ChangeData cd) throws OrmException, PermissionBackendException {
+      checkState(op != null, "addition did not result in an update op");
+      checkState(op.getResult() != null, "op did not return a result");
+
+      // Generate result details and fill AccountLoader. This occurs outside
+      // the Op because the accounts are in a different table.
+      AddReviewersOp.Result opResult = op.getResult();
+      if (migration.readChanges() && state == CC) {
+        result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
+        for (Account.Id accountId : opResult.addedCCs()) {
+          result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
+        }
+        accountLoaderFactory.create(true).fill(result.ccs);
+        for (Address a : opResult.addedCCsByEmail()) {
+          result.ccs.add(new AccountInfo(a.getName(), a.getEmail()));
+        }
+      } else {
+        result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
+        for (PatchSetApproval psa : opResult.addedReviewers()) {
+          // New reviewers have value 0, don't bother normalizing.
+          result.reviewers.add(
+              json.format(
+                  new ReviewerInfo(psa.getAccountId().get()),
+                  psa.getAccountId(),
+                  cd,
+                  ImmutableList.of(psa)));
+        }
+        accountLoaderFactory.create(true).fill(result.reviewers);
+        for (Address a : opResult.addedReviewersByEmail()) {
+          result.reviewers.add(ReviewerInfo.byEmail(a.getName(), a.getEmail()));
+        }
+      }
+    }
+  }
+
+  public static boolean isLegalReviewerGroup(AccountGroup.UUID groupUUID) {
+    return !SystemGroupBackend.isSystemGroup(groupUUID);
+  }
+}

java/com/google/gerrit/server/change/ReviewerJson.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.server.restapi.change;
+package com.google.gerrit.server.change;
 
 import static com.google.gerrit.common.data.LabelValue.formatValue;
 
@@ -29,7 +29,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.account.AccountLoader;
-import com.google.gerrit.server.change.ReviewerResource;
 import com.google.gerrit.server.permissions.LabelPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;

java/com/google/gerrit/server/group/GroupResolver.java

@@ -0,0 +1,116 @@
+// Copyright (C) 2018 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.group;
+
+import com.google.gerrit.common.data.GroupDescription;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.GroupBackends;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.GroupControl;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.Optional;
+
+@Singleton
+public class GroupResolver {
+  private final GroupBackend groupBackend;
+  private final GroupCache groupCache;
+  private final GroupControl.Factory groupControlFactory;
+
+  @Inject
+  GroupResolver(
+      GroupBackend groupBackend, GroupCache groupCache, GroupControl.Factory groupControlFactory) {
+    this.groupBackend = groupBackend;
+    this.groupCache = groupCache;
+    this.groupControlFactory = groupControlFactory;
+  }
+
+  /**
+   * Parses a group ID from a request body and returns the group.
+   *
+   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
+   * @return the group
+   * @throws UnprocessableEntityException thrown if the group ID cannot be resolved or if the group
+   *     is not visible to the calling user
+   */
+  public GroupDescription.Basic parse(String id) throws UnprocessableEntityException {
+    GroupDescription.Basic group = parseId(id);
+    if (group == null || !groupControlFactory.controlFor(group).isVisible()) {
+      throw new UnprocessableEntityException(String.format("Group Not Found: %s", id));
+    }
+    return group;
+  }
+
+  /**
+   * Parses a group ID from a request body and returns the group if it is a Gerrit internal group.
+   *
+   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
+   * @return the group
+   * @throws UnprocessableEntityException thrown if the group ID cannot be resolved, if the group is
+   *     not visible to the calling user or if it's an external group
+   */
+  public GroupDescription.Internal parseInternal(String id) throws UnprocessableEntityException {
+    GroupDescription.Basic group = parse(id);
+    if (group instanceof GroupDescription.Internal) {
+      return (GroupDescription.Internal) group;
+    }
+
+    throw new UnprocessableEntityException(String.format("External Group Not Allowed: %s", id));
+  }
+
+  /**
+   * Parses a group ID and returns the group without making any permission check whether the current
+   * user can see the group.
+   *
+   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
+   * @return the group, null if no group is found for the given group ID
+   */
+  public GroupDescription.Basic parseId(String id) {
+    AccountGroup.UUID uuid = new AccountGroup.UUID(id);
+    if (groupBackend.handles(uuid)) {
+      GroupDescription.Basic d = groupBackend.get(uuid);
+      if (d != null) {
+        return d;
+      }
+    }
+
+    // Might be a numeric AccountGroup.Id. -> Internal group.
+    if (id.matches("^[1-9][0-9]*$")) {
+      try {
+        AccountGroup.Id groupId = AccountGroup.Id.parse(id);
+        Optional<InternalGroup> group = groupCache.get(groupId);
+        if (group.isPresent()) {
+          return new InternalGroupDescription(group.get());
+        }
+      } catch (IllegalArgumentException e) {
+        // Ignored
+      }
+    }
+
+    // Might be a group name, be nice and accept unique names.
+    GroupReference ref = GroupBackends.findExactSuggestion(groupBackend, id);
+    if (ref != null) {
+      GroupDescription.Basic d = groupBackend.get(ref.getUUID());
+      if (d != null) {
+        return d;
+      }
+    }
+
+    return null;
+  }
+}

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

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.restapi.account;
 
-import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.IdString;
@@ -22,10 +21,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.server.AnonymousUser;
-import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountControl;
 import com.google.gerrit.server.account.AccountResolver;
@@ -39,25 +34,19 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class AccountsCollection implements RestCollection<TopLevelResource, AccountResource> {
-  private final Provider<CurrentUser> self;
+  private final AccountResolver accountResolver;
-  private final AccountResolver resolver;
   private final AccountControl.Factory accountControlFactory;
-  private final IdentifiedUser.GenericFactory userFactory;
   private final Provider<QueryAccounts> list;
   private final DynamicMap<RestView<AccountResource>> views;
 
   @Inject
   public AccountsCollection(
-      Provider<CurrentUser> self,
+      AccountResolver accountResolver,
-      AccountResolver resolver,
       AccountControl.Factory accountControlFactory,
-      IdentifiedUser.GenericFactory userFactory,
       Provider<QueryAccounts> list,
       DynamicMap<RestView<AccountResource>> views) {
-    this.self = self;
+    this.accountResolver = accountResolver;
-    this.resolver = resolver;
     this.accountControlFactory = accountControlFactory;
-    this.userFactory = userFactory;
     this.list = list;
     this.views = views;
   }
@@ -66,7 +55,7 @@ public class AccountsCollection implements RestCollection<TopLevelResource, Acco
   public AccountResource parse(TopLevelResource root, IdString id)
       throws ResourceNotFoundException, AuthException, OrmException, IOException,
           ConfigInvalidException {
-    IdentifiedUser user = parseId(id.get());
+    IdentifiedUser user = accountResolver.parseId(id.get());
     if (user == null || !accountControlFactory.get().canSee(user.getAccount())) {
       throw new ResourceNotFoundException(
           String.format("Account '%s' is not found or ambiguous", id));
@@ -74,76 +63,6 @@ public class AccountsCollection implements RestCollection<TopLevelResource, Acco
     return new AccountResource(user);
   }
 
-  /**
-   * Parses a account ID from a request body and returns the user.
-   *
-   * @param id ID of the account, can be a string of the format "{@code Full Name
-   *     <email@example.com>}", just the email address, a full name if it is unique, an account ID,
-   *     a user name or "{@code self}" for the calling user
-   * @return the user, never null.
-   * @throws UnprocessableEntityException thrown if the account ID cannot be resolved or if the
-   *     account is not visible to the calling user
-   */
-  public IdentifiedUser parse(String id)
-      throws AuthException, UnprocessableEntityException, OrmException, IOException,
-          ConfigInvalidException {
-    return parseOnBehalfOf(null, id);
-  }
-
-  /**
-   * Parses an account ID and returns the user without making any permission check whether the
-   * current user can see the account.
-   *
-   * @param id ID of the account, can be a string of the format "{@code Full Name
-   *     <email@example.com>}", just the email address, a full name if it is unique, an account ID,
-   *     a user name or "{@code self}" for the calling user
-   * @return the user, null if no user is found for the given account ID
-   * @throws AuthException thrown if 'self' is used as account ID and the current user is not
-   *     authenticated
-   * @throws OrmException
-   * @throws ConfigInvalidException
-   * @throws IOException
-   */
-  public IdentifiedUser parseId(String id)
-      throws AuthException, OrmException, IOException, ConfigInvalidException {
-    return parseIdOnBehalfOf(null, id);
-  }
-
-  /**
-   * Like {@link #parse(String)}, but also sets the {@link CurrentUser#getRealUser()} on the result.
-   */
-  public IdentifiedUser parseOnBehalfOf(@Nullable CurrentUser caller, String id)
-      throws AuthException, UnprocessableEntityException, OrmException, IOException,
-          ConfigInvalidException {
-    IdentifiedUser user = parseIdOnBehalfOf(caller, id);
-    if (user == null || !accountControlFactory.get().canSee(user.getAccount())) {
-      throw new UnprocessableEntityException(
-          String.format("Account '%s' is not found or ambiguous", id));
-    }
-    return user;
-  }
-
-  private IdentifiedUser parseIdOnBehalfOf(@Nullable CurrentUser caller, String id)
-      throws AuthException, OrmException, IOException, ConfigInvalidException {
-    if (id.equals("self")) {
-      CurrentUser user = self.get();
-      if (user.isIdentifiedUser()) {
-        return user.asIdentifiedUser();
-      } else if (user instanceof AnonymousUser) {
-        throw new AuthException("Authentication required");
-      } else {
-        return null;
-      }
-    }
-
-    Account match = resolver.find(id);
-    if (match == null) {
-      return null;
-    }
-    CurrentUser realUser = caller != null ? caller.getRealUser() : null;
-    return userFactory.runAs(null, match.getId(), realUser);
-  }
-
   @Override
   public RestView<TopLevelResource> list() throws ResourceNotFoundException {
     return list.get();

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

@@ -46,11 +46,11 @@ import com.google.gerrit.server.account.AccountsUpdate;
 import com.google.gerrit.server.account.VersionedAuthorizedKeys;
 import com.google.gerrit.server.account.externalids.DuplicateExternalIdKeyException;
 import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
 import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.gerrit.server.ssh.SshKeyCache;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -68,7 +68,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 public class CreateAccount
     implements RestCollectionCreateView<TopLevelResource, AccountResource, AccountInput> {
   private final Sequences seq;
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final VersionedAuthorizedKeys.Accessor authorizedKeys;
   private final SshKeyCache sshKeyCache;
   private final Provider<AccountsUpdate> accountsUpdateProvider;
@@ -80,7 +80,7 @@ public class CreateAccount
   @Inject
   CreateAccount(
       Sequences seq,
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver,
       VersionedAuthorizedKeys.Accessor authorizedKeys,
       SshKeyCache sshKeyCache,
       @UserInitiated Provider<AccountsUpdate> accountsUpdateProvider,
@@ -89,7 +89,7 @@ public class CreateAccount
       @UserInitiated Provider<GroupsUpdate> groupsUpdate,
       OutgoingEmailValidator validator) {
     this.seq = seq;
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.authorizedKeys = authorizedKeys;
     this.sshKeyCache = sshKeyCache;
     this.accountsUpdateProvider = accountsUpdateProvider;
@@ -183,7 +183,7 @@ public class CreateAccount
     Set<AccountGroup.UUID> groupUuids = new HashSet<>();
     if (groups != null) {
       for (String g : groups) {
-        GroupDescription.Internal internalGroup = groupsCollection.parseInternal(g);
+        GroupDescription.Internal internalGroup = groupResolver.parseInternal(g);
         groupUuids.add(internalGroup.getGroupUUID());
       }
     }

java/com/google/gerrit/server/restapi/change/GetReviewer.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change;
 
 import com.google.gerrit.extensions.api.changes.ReviewerInfo;
 import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.server.change.ReviewerJson;
 import com.google.gerrit.server.change.ReviewerResource;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gwtorm.server.OrmException;

java/com/google/gerrit/server/restapi/change/ListReviewers.java

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ReviewerJson;
 import com.google.gerrit.server.change.ReviewerResource;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gwtorm.server.OrmException;

java/com/google/gerrit/server/restapi/change/ListRevisionReviewers.java

@@ -21,6 +21,7 @@ import com.google.gerrit.mail.Address;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.change.ReviewerJson;
 import com.google.gerrit.server.change.ReviewerResource;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.permissions.PermissionBackendException;

java/com/google/gerrit/server/restapi/change/Module.java

@@ -29,6 +29,7 @@ import static com.google.gerrit.server.change.VoteResource.VOTE_KIND;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.RestApiModule;
 import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.change.AddReviewersOp;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.EmailReviewComments;
@@ -190,7 +191,7 @@ public class Module extends RestApiModule {
     factory(DeleteReviewerOp.Factory.class);
     factory(EmailReviewComments.Factory.class);
     factory(PatchSetInserter.Factory.class);
-    factory(PostReviewersOp.Factory.class);
+    factory(AddReviewersOp.Factory.class);
     factory(RebaseChangeOp.Factory.class);
     factory(ReviewerResource.Factory.class);
     factory(SetAssigneeOp.Factory.class);

java/com/google/gerrit/server/restapi/change/PostReview.java

@@ -88,10 +88,13 @@ import com.google.gerrit.server.OutputFormat;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.PublishCommentUtil;
 import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.change.AddReviewersEmail;
 import com.google.gerrit.server.change.ChangeResource;
-import com.google.gerrit.server.change.ChangeResource.Factory;
 import com.google.gerrit.server.change.EmailReviewComments;
 import com.google.gerrit.server.change.NotifyUtil;
+import com.google.gerrit.server.change.ReviewerAdder;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.change.WorkInProgressOp;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -111,7 +114,6 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
@@ -167,11 +169,11 @@ public class PostReview
   private final PublishCommentUtil publishCommentUtil;
   private final PatchSetUtil psUtil;
   private final PatchListCache patchListCache;
-  private final AccountsCollection accounts;
+  private final AccountResolver accountResolver;
   private final EmailReviewComments.Factory email;
   private final CommentAdded commentAdded;
-  private final PostReviewers postReviewers;
+  private final ReviewerAdder reviewerAdder;
-  private final PostReviewersEmail postReviewersEmail;
+  private final AddReviewersEmail addReviewersEmail;
   private final NotesMigration migration;
   private final NotifyUtil notifyUtil;
   private final Config gerritConfig;
@@ -184,7 +186,7 @@ public class PostReview
   PostReview(
       Provider<ReviewDb> db,
       RetryHelper retryHelper,
-      Factory changeResourceFactory,
+      ChangeResource.Factory changeResourceFactory,
       ChangeData.Factory changeDataFactory,
       ApprovalsUtil approvalsUtil,
       ChangeMessagesUtil cmUtil,
@@ -192,11 +194,11 @@ public class PostReview
       PublishCommentUtil publishCommentUtil,
       PatchSetUtil psUtil,
       PatchListCache patchListCache,
-      AccountsCollection accounts,
+      AccountResolver accountResolver,
       EmailReviewComments.Factory email,
       CommentAdded commentAdded,
-      PostReviewers postReviewers,
+      ReviewerAdder reviewerAdder,
-      PostReviewersEmail postReviewersEmail,
+      AddReviewersEmail addReviewersEmail,
       NotesMigration migration,
       NotifyUtil notifyUtil,
       @GerritServerConfig Config gerritConfig,
@@ -213,11 +215,11 @@ public class PostReview
     this.patchListCache = patchListCache;
     this.approvalsUtil = approvalsUtil;
     this.cmUtil = cmUtil;
-    this.accounts = accounts;
+    this.accountResolver = accountResolver;
     this.email = email;
     this.commentAdded = commentAdded;
-    this.postReviewers = postReviewers;
+    this.reviewerAdder = reviewerAdder;
-    this.postReviewersEmail = postReviewersEmail;
+    this.addReviewersEmail = addReviewersEmail;
     this.migration = migration;
     this.notifyUtil = notifyUtil;
     this.gerritConfig = gerritConfig;
@@ -273,21 +275,20 @@ public class PostReview
         notifyUtil.resolveAccounts(input.notifyDetails);
 
     Map<String, AddReviewerResult> reviewerJsonResults = null;
-    List<PostReviewers.Addition> reviewerResults = Lists.newArrayList();
+    List<ReviewerAddition> reviewerResults = Lists.newArrayList();
     boolean hasError = false;
     boolean confirm = false;
     if (input.reviewers != null) {
       reviewerJsonResults = Maps.newHashMap();
       for (AddReviewerInput reviewerInput : input.reviewers) {
-        // Prevent individual PostReviewersOps from sending one email each. Instead, we call
+        // Prevent individual AddReviewersOps from sending one email each. Instead, we call
         // batchEmailReviewers at the very end to send out a single email.
         // TODO(dborowitz): I think this still sends out separate emails if any of input.reviewers
         // specifies explicit accountsToNotify. Unclear whether that's a good thing.
         reviewerInput.notify = NotifyHandling.NONE;
 
-        PostReviewers.Addition result =
+        ReviewerAddition result =
-            postReviewers.prepareApplication(
+            reviewerAdder.prepare(revision.getNotes(), revision.getUser(), reviewerInput, true);
-                revision.getNotes(), revision.getUser(), reviewerInput, true);
         reviewerJsonResults.put(reviewerInput.reviewer, result.result);
         if (result.result.error != null) {
           hasError = true;
@@ -327,7 +328,7 @@ public class PostReview
       // Apply reviewer changes first. Revision emails should be sent to the
       // updated set of reviewers. Also keep track of whether the user added
       // themselves as a reviewer or to the CC list.
-      for (PostReviewers.Addition reviewerResult : reviewerResults) {
+      for (ReviewerAddition reviewerResult : reviewerResults) {
         bu.addOp(revision.getChange().getId(), reviewerResult.op);
         if (!ccOrReviewer && reviewerResult.result.reviewers != null) {
           for (ReviewerInfo reviewerInfo : reviewerResult.result.reviewers) {
@@ -351,8 +352,7 @@ public class PostReview
         // User posting this review isn't currently in the reviewer or CC list,
         // isn't being explicitly added, and isn't voting on any label.
         // Automatically CC them on this change so they receive replies.
-        PostReviewers.Addition selfAddition =
+        ReviewerAddition selfAddition = reviewerAdder.ccCurrentUser(revision.getUser(), revision);
-            postReviewers.ccCurrentUser(revision.getUser(), revision);
         bu.addOp(revision.getChange().getId(), selfAddition.op);
       }
 
@@ -389,13 +389,13 @@ public class PostReview
       // Re-read change to take into account results of the update.
       ChangeData cd =
           changeDataFactory.create(db.get(), revision.getProject(), revision.getChange().getId());
-      for (PostReviewers.Addition reviewerResult : reviewerResults) {
+      for (ReviewerAddition reviewerResult : reviewerResults) {
         reviewerResult.gatherResults(cd);
       }
 
       boolean readyForReview =
           (output.ready != null && output.ready) || !revision.getChange().isWorkInProgress();
-      // Sending from PostReviewersOp was suppressed so we can send a single batch email here.
+      // Sending from AddReviewersOp was suppressed so we can send a single batch email here.
       batchEmailReviewers(
           revision.getUser(),
           revision.getChange(),
@@ -434,7 +434,7 @@ public class PostReview
   private void batchEmailReviewers(
       CurrentUser user,
       Change change,
-      List<PostReviewers.Addition> reviewerAdditions,
+      List<ReviewerAddition> reviewerAdditions,
       @Nullable NotifyHandling notify,
       ListMultimap<RecipientType, Account.Id> accountsToNotify,
       boolean readyForReview) {
@@ -442,7 +442,7 @@ public class PostReview
     List<Account.Id> cc = new ArrayList<>();
     List<Address> toByEmail = new ArrayList<>();
     List<Address> ccByEmail = new ArrayList<>();
-    for (PostReviewers.Addition addition : reviewerAdditions) {
+    for (ReviewerAddition addition : reviewerAdditions) {
       if (addition.state == ReviewerState.REVIEWER) {
         to.addAll(addition.reviewers);
         toByEmail.addAll(addition.reviewersByEmail);
@@ -451,7 +451,7 @@ public class PostReview
         ccByEmail.addAll(addition.reviewersByEmail);
       }
     }
-    postReviewersEmail.emailReviewers(
+    addReviewersEmail.emailReviewers(
         user.asIdentifiedUser(),
         change,
         to,
@@ -505,7 +505,7 @@ public class PostReview
           String.format("label required to post review on behalf of \"%s\"", in.onBehalfOf));
     }
 
-    IdentifiedUser reviewer = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
+    IdentifiedUser reviewer = accountResolver.parseOnBehalfOf(caller, in.onBehalfOf);
     try {
       permissionBackend
           .user(reviewer)

java/com/google/gerrit/server/restapi/change/PostReviewers.java

@@ -14,61 +14,18 @@
 
 package com.google.gerrit.server.restapi.change;
 
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
-import static com.google.gerrit.extensions.client.ReviewerState.CC;
-import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
-
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ListMultimap;
-import com.google.common.collect.Lists;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.data.GroupDescription;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerResult;
-import com.google.gerrit.extensions.api.changes.NotifyHandling;
-import com.google.gerrit.extensions.api.changes.RecipientType;
-import com.google.gerrit.extensions.api.changes.ReviewerInfo;
-import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.extensions.common.AccountInfo;
-import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
-import com.google.gerrit.mail.Address;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
-import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.AnonymousUser;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountLoader;
-import com.google.gerrit.server.account.GroupMembers;
-import com.google.gerrit.server.change.ChangeMessages;
 import com.google.gerrit.server.change.ChangeResource;
-import com.google.gerrit.server.change.NotifyUtil;
+import com.google.gerrit.server.change.ReviewerAdder;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
 import com.google.gerrit.server.change.ReviewerResource;
-import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.group.SystemGroupBackend;
-import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.permissions.ChangePermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.RefPermission;
-import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
-import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.update.RetryingRestCollectionModifyView;
@@ -79,72 +36,27 @@ import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
-import java.text.MessageFormat;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
 
 @Singleton
 public class PostReviewers
     extends RetryingRestCollectionModifyView<
         ChangeResource, ReviewerResource, AddReviewerInput, AddReviewerResult> {
 
-  public static final int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
-  public static final int DEFAULT_MAX_REVIEWERS = 20;
-
-  private final AccountsCollection accounts;
-  private final PermissionBackend permissionBackend;
-
-  private final GroupsCollection groupsCollection;
-  private final GroupMembers groupMembers;
-  private final AccountLoader.Factory accountLoaderFactory;
   private final Provider<ReviewDb> dbProvider;
   private final ChangeData.Factory changeDataFactory;
-  private final Config cfg;
+  private final ReviewerAdder reviewerAdder;
-  private final ReviewerJson json;
-  private final NotesMigration migration;
-  private final NotifyUtil notifyUtil;
-  private final ProjectCache projectCache;
-  private final Provider<AnonymousUser> anonymousProvider;
-  private final PostReviewersOp.Factory postReviewersOpFactory;
-  private final OutgoingEmailValidator validator;
 
   @Inject
   PostReviewers(
-      AccountsCollection accounts,
-      PermissionBackend permissionBackend,
-      GroupsCollection groupsCollection,
-      GroupMembers groupMembers,
-      AccountLoader.Factory accountLoaderFactory,
       Provider<ReviewDb> db,
       ChangeData.Factory changeDataFactory,
       RetryHelper retryHelper,
-      @GerritServerConfig Config cfg,
+      ReviewerAdder reviewerAdder) {
-      ReviewerJson json,
-      NotesMigration migration,
-      NotifyUtil notifyUtil,
-      ProjectCache projectCache,
-      Provider<AnonymousUser> anonymousProvider,
-      PostReviewersOp.Factory postReviewersOpFactory,
-      OutgoingEmailValidator validator) {
     super(retryHelper);
-    this.accounts = accounts;
-    this.permissionBackend = permissionBackend;
-    this.groupsCollection = groupsCollection;
-    this.groupMembers = groupMembers;
-    this.accountLoaderFactory = accountLoaderFactory;
     this.dbProvider = db;
     this.changeDataFactory = changeDataFactory;
-    this.cfg = cfg;
+    this.reviewerAdder = reviewerAdder;
-    this.json = json;
-    this.migration = migration;
-    this.notifyUtil = notifyUtil;
-    this.projectCache = projectCache;
-    this.anonymousProvider = anonymousProvider;
-    this.postReviewersOpFactory = postReviewersOpFactory;
-    this.validator = validator;
   }
 
   @Override
@@ -156,7 +68,7 @@ public class PostReviewers
       throw new BadRequestException("missing reviewer field");
     }
 
-    Addition addition = prepareApplication(rsrc.getNotes(), rsrc.getUser(), input, true);
+    ReviewerAddition addition = reviewerAdder.prepare(rsrc.getNotes(), rsrc.getUser(), input, true);
     if (addition.op == null) {
       return addition.result;
     }
@@ -173,349 +85,4 @@ public class PostReviewers
         changeDataFactory.create(dbProvider.get(), rsrc.getProject(), rsrc.getId()));
     return addition.result;
   }
-
-  /**
-   * Prepare application of a single {@link AddReviewerInput}.
-   *
-   * @param notes change notes.
-   * @param user user performing the reviewer addition.
-   * @param input input describing user or group to add as a reviewer.
-   * @param allowGroup whether to allow
-   * @return handle describing the addition operation. If the {@code op} field is present, this
-   *     operation may be added to a {@code BatchUpdate}. Otherwise, the {@code error} field
-   *     contains information about an error that occurred
-   * @throws OrmException
-   * @throws IOException
-   * @throws PermissionBackendException
-   * @throws ConfigInvalidException
-   */
-  public Addition prepareApplication(
-      ChangeNotes notes, CurrentUser user, AddReviewerInput input, boolean allowGroup)
-      throws OrmException, IOException, PermissionBackendException, ConfigInvalidException {
-    Branch.NameKey dest = notes.getChange().getDest();
-    String reviewer = input.reviewer;
-    ReviewerState state = input.state();
-    NotifyHandling notify = input.notify;
-    ListMultimap<RecipientType, Account.Id> accountsToNotify;
-    try {
-      accountsToNotify = notifyUtil.resolveAccounts(input.notifyDetails);
-    } catch (BadRequestException e) {
-      return fail(reviewer, e.getMessage());
-    }
-    boolean confirmed = input.confirmed();
-    boolean allowByEmail =
-        projectCache
-            .checkedGet(dest.getParentKey())
-            .is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
-
-    Addition byAccountId =
-        addByAccountId(
-            reviewer, dest, user, state, notify, accountsToNotify, allowGroup, allowByEmail);
-
-    Addition wholeGroup = null;
-    if (byAccountId == null || !byAccountId.exactMatchFound) {
-      wholeGroup =
-          addWholeGroup(
-              reviewer,
-              dest,
-              user,
-              state,
-              notify,
-              accountsToNotify,
-              confirmed,
-              allowGroup,
-              allowByEmail);
-      if (wholeGroup != null && wholeGroup.exactMatchFound) {
-        return wholeGroup;
-      }
-    }
-
-    if (byAccountId != null) {
-      return byAccountId;
-    }
-    if (wholeGroup != null) {
-      return wholeGroup;
-    }
-
-    return addByEmail(reviewer, notes, user, state, notify, accountsToNotify);
-  }
-
-  Addition ccCurrentUser(CurrentUser user, RevisionResource revision) {
-    return new Addition(
-        user.getUserName().orElse(null),
-        revision.getUser(),
-        ImmutableSet.of(user.getAccountId()),
-        null,
-        CC,
-        NotifyHandling.NONE,
-        ImmutableListMultimap.of(),
-        true);
-  }
-
-  @Nullable
-  private Addition addByAccountId(
-      String reviewer,
-      Branch.NameKey dest,
-      CurrentUser user,
-      ReviewerState state,
-      NotifyHandling notify,
-      ListMultimap<RecipientType, Account.Id> accountsToNotify,
-      boolean allowGroup,
-      boolean allowByEmail)
-      throws OrmException, PermissionBackendException, IOException, ConfigInvalidException {
-    IdentifiedUser reviewerUser;
-    boolean exactMatchFound = false;
-    try {
-      reviewerUser = accounts.parse(reviewer);
-      if (reviewer.equalsIgnoreCase(reviewerUser.getName())
-          || reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
-        exactMatchFound = true;
-      }
-    } catch (UnprocessableEntityException | AuthException e) {
-      // AuthException won't occur since the user is authenticated at this point.
-      if (!allowGroup && !allowByEmail) {
-        // Only return failure if we aren't going to try other interpretations.
-        return fail(
-            reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
-      }
-      return null;
-    }
-
-    if (isValidReviewer(dest, reviewerUser.getAccount())) {
-      return new Addition(
-          reviewer,
-          user,
-          ImmutableSet.of(reviewerUser.getAccountId()),
-          null,
-          state,
-          notify,
-          accountsToNotify,
-          exactMatchFound);
-    }
-    if (!reviewerUser.getAccount().isActive()) {
-      if (allowByEmail && state == CC) {
-        return null;
-      }
-      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInactive, reviewer));
-    }
-    return fail(
-        reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
-  }
-
-  @Nullable
-  private Addition addWholeGroup(
-      String reviewer,
-      Branch.NameKey dest,
-      CurrentUser user,
-      ReviewerState state,
-      NotifyHandling notify,
-      ListMultimap<RecipientType, Account.Id> accountsToNotify,
-      boolean confirmed,
-      boolean allowGroup,
-      boolean allowByEmail)
-      throws IOException, PermissionBackendException {
-    if (!allowGroup) {
-      return null;
-    }
-
-    GroupDescription.Basic group;
-    try {
-      group = groupsCollection.parseInternal(reviewer);
-    } catch (UnprocessableEntityException e) {
-      if (!allowByEmail) {
-        return fail(
-            reviewer,
-            MessageFormat.format(ChangeMessages.get().reviewerNotFoundUserOrGroup, reviewer));
-      }
-      return null;
-    }
-
-    if (!isLegalReviewerGroup(group.getGroupUUID())) {
-      return fail(
-          reviewer, MessageFormat.format(ChangeMessages.get().groupIsNotAllowed, group.getName()));
-    }
-
-    Set<Account.Id> reviewers = new HashSet<>();
-    Set<Account> members;
-    try {
-      members = groupMembers.listAccounts(group.getGroupUUID(), dest.getParentKey());
-    } catch (NoSuchProjectException e) {
-      return fail(reviewer, e.getMessage());
-    }
-
-    // if maxAllowed is set to 0, it is allowed to add any number of
-    // reviewers
-    int maxAllowed = cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS);
-    if (maxAllowed > 0 && members.size() > maxAllowed) {
-      return fail(
-          reviewer,
-          MessageFormat.format(ChangeMessages.get().groupHasTooManyMembers, group.getName()));
-    }
-
-    // if maxWithoutCheck is set to 0, we never ask for confirmation
-    int maxWithoutConfirmation =
-        cfg.getInt("addreviewer", "maxWithoutConfirmation", DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
-    if (!confirmed && maxWithoutConfirmation > 0 && members.size() > maxWithoutConfirmation) {
-      return fail(
-          reviewer,
-          true,
-          MessageFormat.format(
-              ChangeMessages.get().groupManyMembersConfirmation, group.getName(), members.size()));
-    }
-
-    for (Account member : members) {
-      if (isValidReviewer(dest, member)) {
-        reviewers.add(member.getId());
-      }
-    }
-
-    return new Addition(reviewer, user, reviewers, null, state, notify, accountsToNotify, true);
-  }
-
-  @Nullable
-  private Addition addByEmail(
-      String reviewer,
-      ChangeNotes notes,
-      CurrentUser user,
-      ReviewerState state,
-      NotifyHandling notify,
-      ListMultimap<RecipientType, Account.Id> accountsToNotify)
-      throws PermissionBackendException {
-    try {
-      permissionBackend
-          .user(anonymousProvider.get())
-          .database(dbProvider)
-          .change(notes)
-          .check(ChangePermission.READ);
-    } catch (AuthException e) {
-      return fail(
-          reviewer, MessageFormat.format(ChangeMessages.get().reviewerCantSeeChange, reviewer));
-    }
-
-    if (!migration.readChanges()) {
-      // addByEmail depends on NoteDb.
-      return fail(
-          reviewer, MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, reviewer));
-    }
-    Address adr = Address.tryParse(reviewer);
-    if (adr == null || !validator.isValid(adr.getEmail())) {
-      return fail(reviewer, MessageFormat.format(ChangeMessages.get().reviewerInvalid, reviewer));
-    }
-    return new Addition(
-        reviewer, user, null, ImmutableList.of(adr), state, notify, accountsToNotify, true);
-  }
-
-  private boolean isValidReviewer(Branch.NameKey branch, Account member)
-      throws PermissionBackendException {
-    if (!member.isActive()) {
-      return false;
-    }
-
-    try {
-      // Check ref permission instead of change permission, since change permissions take into
-      // account the private bit, whereas adding a user as a reviewer is explicitly allowing them to
-      // see private changes.
-      permissionBackend
-          .absentUser(member.getId())
-          .database(dbProvider)
-          .ref(branch)
-          .check(RefPermission.READ);
-      return true;
-    } catch (AuthException e) {
-      return false;
-    }
-  }
-
-  private Addition fail(String reviewer, String error) {
-    return fail(reviewer, false, error);
-  }
-
-  private Addition fail(String reviewer, boolean confirm, String error) {
-    Addition addition = new Addition(reviewer);
-    addition.result.confirm = confirm ? true : null;
-    addition.result.error = error;
-    return addition;
-  }
-
-  public class Addition {
-    public final AddReviewerResult result;
-    @Nullable public final PostReviewersOp op;
-    final Set<Account.Id> reviewers;
-    final Collection<Address> reviewersByEmail;
-    final ReviewerState state;
-    @Nullable final IdentifiedUser caller;
-    final boolean exactMatchFound;
-
-    Addition(String reviewer) {
-      result = new AddReviewerResult(reviewer);
-      op = null;
-      reviewers = ImmutableSet.of();
-      reviewersByEmail = ImmutableSet.of();
-      state = REVIEWER;
-      caller = null;
-      exactMatchFound = false;
-    }
-
-    Addition(
-        String reviewer,
-        CurrentUser caller,
-        @Nullable Set<Account.Id> reviewers,
-        @Nullable Collection<Address> reviewersByEmail,
-        ReviewerState state,
-        @Nullable NotifyHandling notify,
-        ListMultimap<RecipientType, Account.Id> accountsToNotify,
-        boolean exactMatchFound) {
-      checkArgument(
-          reviewers != null || reviewersByEmail != null,
-          "must have either reviewers or reviewersByEmail");
-
-      result = new AddReviewerResult(reviewer);
-      this.reviewers = reviewers == null ? ImmutableSet.of() : reviewers;
-      this.reviewersByEmail = reviewersByEmail == null ? ImmutableList.of() : reviewersByEmail;
-      this.state = state;
-      this.caller = caller.asIdentifiedUser();
-      op =
-          postReviewersOpFactory.create(
-              this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
-      this.exactMatchFound = exactMatchFound;
-    }
-
-    void gatherResults(ChangeData cd) throws OrmException, PermissionBackendException {
-      checkState(op != null, "addition did not result in an update op");
-      checkState(op.getResult() != null, "op did not return a result");
-
-      // Generate result details and fill AccountLoader. This occurs outside
-      // the Op because the accounts are in a different table.
-      PostReviewersOp.Result opResult = op.getResult();
-      if (migration.readChanges() && state == CC) {
-        result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
-        for (Account.Id accountId : opResult.addedCCs()) {
-          result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
-        }
-        accountLoaderFactory.create(true).fill(result.ccs);
-        for (Address a : reviewersByEmail) {
-          result.ccs.add(new AccountInfo(a.getName(), a.getEmail()));
-        }
-      } else {
-        result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
-        for (PatchSetApproval psa : opResult.addedReviewers()) {
-          // New reviewers have value 0, don't bother normalizing.
-          result.reviewers.add(
-              json.format(
-                  new ReviewerInfo(psa.getAccountId().get()),
-                  psa.getAccountId(),
-                  cd,
-                  ImmutableList.of(psa)));
-        }
-        accountLoaderFactory.create(true).fill(result.reviewers);
-        for (Address a : reviewersByEmail) {
-          result.reviewers.add(ReviewerInfo.byEmail(a.getName(), a.getEmail()));
-        }
-      }
-    }
-  }
-
-  public static boolean isLegalReviewerGroup(AccountGroup.UUID groupUUID) {
-    return !SystemGroupBackend.isSystemGroup(groupUUID);
-  }
 }

java/com/google/gerrit/server/restapi/change/PutAssignee.java

@@ -28,13 +28,14 @@ import com.google.gerrit.extensions.webui.UiAction;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.ReviewerAdder;
+import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
 import com.google.gerrit.server.change.SetAssigneeOp;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
-import com.google.gerrit.server.restapi.change.PostReviewers.Addition;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.gerrit.server.update.RetryingRestModifyView;
@@ -51,27 +52,27 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 public class PutAssignee extends RetryingRestModifyView<ChangeResource, AssigneeInput, AccountInfo>
     implements UiAction<ChangeResource> {
 
-  private final AccountsCollection accounts;
+  private final AccountResolver accountResolver;
   private final SetAssigneeOp.Factory assigneeFactory;
   private final Provider<ReviewDb> db;
-  private final PostReviewers postReviewers;
+  private final ReviewerAdder reviewerAdder;
   private final AccountLoader.Factory accountLoaderFactory;
   private final PermissionBackend permissionBackend;
 
   @Inject
   PutAssignee(
-      AccountsCollection accounts,
+      AccountResolver accountResolver,
       SetAssigneeOp.Factory assigneeFactory,
       RetryHelper retryHelper,
       Provider<ReviewDb> db,
-      PostReviewers postReviewers,
+      ReviewerAdder reviewerAdder,
       AccountLoader.Factory accountLoaderFactory,
       PermissionBackend permissionBackend) {
     super(retryHelper);
-    this.accounts = accounts;
+    this.accountResolver = accountResolver;
     this.assigneeFactory = assigneeFactory;
     this.db = db;
-    this.postReviewers = postReviewers;
+    this.reviewerAdder = reviewerAdder;
     this.accountLoaderFactory = accountLoaderFactory;
     this.permissionBackend = permissionBackend;
   }
@@ -88,7 +89,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
       throw new BadRequestException("missing assignee field");
     }
 
-    IdentifiedUser assignee = accounts.parse(input.assignee);
+    IdentifiedUser assignee = accountResolver.parse(input.assignee);
     if (!assignee.getAccount().isActive()) {
       throw new UnprocessableEntityException(input.assignee + " is not active");
     }
@@ -108,7 +109,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
       SetAssigneeOp op = assigneeFactory.create(assignee);
       bu.addOp(rsrc.getId(), op);
 
-      PostReviewers.Addition reviewersAddition = addAssigneeAsCC(rsrc, input.assignee);
+      ReviewerAddition reviewersAddition = addAssigneeAsCC(rsrc, input.assignee);
       bu.addOp(rsrc.getId(), reviewersAddition.op);
 
       bu.execute();
@@ -116,14 +117,14 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
     }
   }
 
-  private Addition addAssigneeAsCC(ChangeResource rsrc, String assignee)
+  private ReviewerAddition addAssigneeAsCC(ChangeResource rsrc, String assignee)
       throws OrmException, IOException, PermissionBackendException, ConfigInvalidException {
     AddReviewerInput reviewerInput = new AddReviewerInput();
     reviewerInput.reviewer = assignee;
     reviewerInput.state = ReviewerState.CC;
     reviewerInput.confirmed = true;
     reviewerInput.notify = NotifyHandling.NONE;
-    return postReviewers.prepareApplication(rsrc.getNotes(), rsrc.getUser(), reviewerInput, false);
+    return reviewerAdder.prepare(rsrc.getNotes(), rsrc.getUser(), reviewerInput, false);
   }
 
   @Override

java/com/google/gerrit/server/restapi/change/ReviewersUtil.java

@@ -47,6 +47,7 @@ import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupMembers;
+import com.google.gerrit.server.change.ReviewerAdder;
 import com.google.gerrit.server.index.account.AccountField;
 import com.google.gerrit.server.index.account.AccountIndexCollection;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -383,7 +384,7 @@ public class ReviewersUtil {
     int maxAllowedWithoutConfirmation = suggestReviewers.getMaxAllowedWithoutConfirmation();
     logger.atFine().log("maxAllowedWithoutConfirmation: " + maxAllowedWithoutConfirmation);
 
-    if (!PostReviewers.isLegalReviewerGroup(group.getUUID())) {
+    if (!ReviewerAdder.isLegalReviewerGroup(group.getUUID())) {
       logger.atFine().log("Ignore group %s that is not legal as reviewer", group.getUUID());
       return result;
     }

java/com/google/gerrit/server/restapi/change/Submit.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.ProjectUtil;
+import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeJson;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.RevisionResource;
@@ -55,7 +56,6 @@ import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.gerrit.server.submit.ChangeSet;
 import com.google.gerrit.server.submit.MergeOp;
 import com.google.gerrit.server.submit.MergeSuperSet;
@@ -114,7 +114,7 @@ public class Submit
   private final ChangeNotes.Factory changeNotesFactory;
   private final Provider<MergeOp> mergeOpProvider;
   private final Provider<MergeSuperSet> mergeSuperSet;
-  private final AccountsCollection accounts;
+  private final AccountResolver accountResolver;
   private final String label;
   private final String labelWithParents;
   private final ParameterizedString titlePattern;
@@ -135,7 +135,7 @@ public class Submit
       ChangeNotes.Factory changeNotesFactory,
       Provider<MergeOp> mergeOpProvider,
       Provider<MergeSuperSet> mergeSuperSet,
-      AccountsCollection accounts,
+      AccountResolver accountResolver,
       @GerritServerConfig Config cfg,
       Provider<InternalChangeQuery> queryProvider,
       PatchSetUtil psUtil,
@@ -147,7 +147,7 @@ public class Submit
     this.changeNotesFactory = changeNotesFactory;
     this.mergeOpProvider = mergeOpProvider;
     this.mergeSuperSet = mergeSuperSet;
-    this.accounts = accounts;
+    this.accountResolver = accountResolver;
     this.label =
         MoreObjects.firstNonNull(
             Strings.emptyToNull(cfg.getString("change", null, "submitLabel")), "Submit");
@@ -472,7 +472,7 @@ public class Submit
     perm.check(ChangePermission.SUBMIT_AS);
 
     CurrentUser caller = rsrc.getUser();
-    IdentifiedUser submitter = accounts.parseOnBehalfOf(caller, in.onBehalfOf);
+    IdentifiedUser submitter = accountResolver.parseOnBehalfOf(caller, in.onBehalfOf);
     try {
       permissionBackend
           .user(submitter)

java/com/google/gerrit/server/restapi/change/SuggestReviewers.java

@@ -19,6 +19,7 @@ import static com.google.gerrit.server.config.GerritConfigListenerHelper.acceptI
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.extensions.common.AccountVisibility;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.change.ReviewerAdder;
 import com.google.gerrit.server.config.ConfigKey;
 import com.google.gerrit.server.config.GerritConfigListener;
 import com.google.gerrit.server.config.GerritServerConfig;
@@ -98,12 +99,12 @@ public class SuggestReviewers {
       this.suggestAccounts = (av != AccountVisibility.NONE);
     }
 
-    this.maxAllowed = cfg.getInt("addreviewer", "maxAllowed", PostReviewers.DEFAULT_MAX_REVIEWERS);
+    this.maxAllowed = cfg.getInt("addreviewer", "maxAllowed", ReviewerAdder.DEFAULT_MAX_REVIEWERS);
     this.maxAllowedWithoutConfirmation =
         cfg.getInt(
             "addreviewer",
             "maxWithoutConfirmation",
-            PostReviewers.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
+            ReviewerAdder.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
 
     logger.atFine().log("AccountVisibility: %s", av.name());
   }

java/com/google/gerrit/server/restapi/group/AddMembers.java

@@ -47,7 +47,6 @@ import com.google.gerrit.server.group.MemberResource;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.server.group.db.InternalGroupUpdate;
 import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.gerrit.server.restapi.group.AddMembers.Input;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -90,7 +89,6 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
 
   private final AccountManager accountManager;
   private final AuthType authType;
-  private final AccountsCollection accounts;
   private final AccountResolver accountResolver;
   private final AccountCache accountCache;
   private final AccountLoader.Factory infoFactory;
@@ -100,14 +98,12 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
   AddMembers(
       AccountManager accountManager,
       AuthConfig authConfig,
-      AccountsCollection accounts,
       AccountResolver accountResolver,
       AccountCache accountCache,
       AccountLoader.Factory infoFactory,
       @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
     this.accountManager = accountManager;
     this.authType = authConfig.getAuthType();
-    this.accounts = accounts;
     this.accountResolver = accountResolver;
     this.accountCache = accountCache;
     this.infoFactory = infoFactory;
@@ -151,7 +147,7 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
       throws AuthException, UnprocessableEntityException, OrmException, IOException,
           ConfigInvalidException {
     try {
-      return accounts.parse(nameOrEmailOrId).getAccount();
+      return accountResolver.parse(nameOrEmailOrId).getAccount();
     } catch (UnprocessableEntityException e) {
       // might be because the account does not exist or because the account is
       // not visible

java/com/google/gerrit/server/restapi/group/AddSubgroups.java

@@ -32,6 +32,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.UserInitiated;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.GroupResource;
 import com.google.gerrit.server.group.SubgroupResource;
 import com.google.gerrit.server.group.db.GroupsUpdate;
@@ -76,16 +77,16 @@ public class AddSubgroups implements RestModifyView<GroupResource, Input> {
     }
   }
 
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final Provider<GroupsUpdate> groupsUpdateProvider;
   private final GroupJson json;
 
   @Inject
   public AddSubgroups(
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver,
       @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider,
       GroupJson json) {
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.groupsUpdateProvider = groupsUpdateProvider;
     this.json = json;
   }
@@ -107,7 +108,7 @@ public class AddSubgroups implements RestModifyView<GroupResource, Input> {
     List<GroupInfo> result = new ArrayList<>();
     Set<AccountGroup.UUID> subgroupUuids = new LinkedHashSet<>();
     for (String subgroupIdentifier : input.groups) {
-      GroupDescription.Basic subgroup = groupsCollection.parse(subgroupIdentifier);
+      GroupDescription.Basic subgroup = groupResolver.parse(subgroupIdentifier);
       subgroupUuids.add(subgroup.getGroupUUID());
       result.add(json.format(subgroup));
     }

java/com/google/gerrit/server/restapi/group/CreateGroup.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.account.CreateGroupArgs;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupUUID;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.GroupResource;
 import com.google.gerrit.server.group.InternalGroup;
 import com.google.gerrit.server.group.InternalGroupDescription;
@@ -77,7 +78,7 @@ public class CreateGroup
   private final PersonIdent serverIdent;
   private final Provider<GroupsUpdate> groupsUpdateProvider;
   private final GroupCache groupCache;
-  private final GroupsCollection groups;
+  private final GroupResolver groups;
   private final GroupJson json;
   private final PluginSetContext<GroupCreationValidationListener> groupCreationValidationListeners;
   private final AddMembers addMembers;
@@ -91,7 +92,7 @@ public class CreateGroup
       @GerritPersonIdent PersonIdent serverIdent,
       @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider,
       GroupCache groupCache,
-      GroupsCollection groups,
+      GroupResolver groups,
       GroupJson json,
       PluginSetContext<GroupCreationValidationListener> groupCreationValidationListeners,
       AddMembers addMembers,

java/com/google/gerrit/server/restapi/group/DeleteMembers.java

@@ -26,12 +26,12 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.UserInitiated;
+import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.group.GroupResource;
 import com.google.gerrit.server.group.MemberResource;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.server.group.db.InternalGroupUpdate;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.gerrit.server.restapi.group.AddMembers.Input;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -44,13 +44,13 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class DeleteMembers implements RestModifyView<GroupResource, Input> {
-  private final AccountsCollection accounts;
+  private final AccountResolver accountResolver;
   private final Provider<GroupsUpdate> groupsUpdateProvider;
 
   @Inject
   DeleteMembers(
-      AccountsCollection accounts, @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
+      AccountResolver accountResolver, @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
-    this.accounts = accounts;
+    this.accountResolver = accountResolver;
     this.groupsUpdateProvider = groupsUpdateProvider;
   }
 
@@ -69,7 +69,7 @@ public class DeleteMembers implements RestModifyView<GroupResource, Input> {
 
     Set<Account.Id> membersToRemove = new HashSet<>();
     for (String nameOrEmail : input.members) {
-      Account a = accounts.parse(nameOrEmail).getAccount();
+      Account a = accountResolver.parse(nameOrEmail).getAccount();
       membersToRemove.add(a.getId());
     }
     AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();

java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java

@@ -27,6 +27,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.UserInitiated;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.GroupResource;
 import com.google.gerrit.server.group.SubgroupResource;
 import com.google.gerrit.server.group.db.GroupsUpdate;
@@ -43,14 +44,13 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class DeleteSubgroups implements RestModifyView<GroupResource, Input> {
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final Provider<GroupsUpdate> groupsUpdateProvider;
 
   @Inject
   DeleteSubgroups(
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver, @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
-      @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
+    this.groupResolver = groupResolver;
-    this.groupsCollection = groupsCollection;
     this.groupsUpdateProvider = groupsUpdateProvider;
   }
 
@@ -70,7 +70,7 @@ public class DeleteSubgroups implements RestModifyView<GroupResource, Input> {
 
     Set<AccountGroup.UUID> subgroupsToRemove = new HashSet<>();
     for (String subgroupIdentifier : input.groups) {
-      GroupDescription.Basic subgroup = groupsCollection.parse(subgroupIdentifier);
+      GroupDescription.Basic subgroup = groupResolver.parse(subgroupIdentifier);
       subgroupsToRemove.add(subgroup.getGroupUUID());
     }
 

java/com/google/gerrit/server/restapi/group/GroupsCollection.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.restapi.group;
 
 import com.google.common.collect.ListMultimap;
 import com.google.gerrit.common.data.GroupDescription;
-import com.google.gerrit.common.data.GroupReference;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -26,20 +25,13 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestCollection;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
-import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
-import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.AnonymousUser;
 import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupBackends;
-import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.GroupResource;
-import com.google.gerrit.server.group.InternalGroup;
-import com.google.gerrit.server.group.InternalGroupDescription;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
-import java.util.Optional;
 
 public class GroupsCollection
     implements RestCollection<TopLevelResource, GroupResource>, NeedsParams {
@@ -47,8 +39,7 @@ public class GroupsCollection
   private final Provider<ListGroups> list;
   private final Provider<QueryGroups> queryGroups;
   private final GroupControl.Factory groupControlFactory;
-  private final GroupBackend groupBackend;
+  private final GroupResolver groupResolver;
-  private final GroupCache groupCache;
   private final Provider<CurrentUser> self;
 
   private boolean hasQuery2;
@@ -59,15 +50,13 @@ public class GroupsCollection
       Provider<ListGroups> list,
       Provider<QueryGroups> queryGroups,
       GroupControl.Factory groupControlFactory,
-      GroupBackend groupBackend,
+      GroupResolver groupResolver,
-      GroupCache groupCache,
       Provider<CurrentUser> self) {
     this.views = views;
     this.list = list;
     this.queryGroups = queryGroups;
     this.groupControlFactory = groupControlFactory;
-    this.groupBackend = groupBackend;
+    this.groupResolver = groupResolver;
-    this.groupCache = groupCache;
     this.self = self;
   }
 
@@ -107,7 +96,7 @@ public class GroupsCollection
       throw new ResourceNotFoundException(id);
     }
 
-    GroupDescription.Basic group = parseId(id.get());
+    GroupDescription.Basic group = groupResolver.parseId(id.get());
     if (group == null) {
       throw new ResourceNotFoundException(id.get());
     }
@@ -118,80 +107,6 @@ public class GroupsCollection
     return new GroupResource(ctl);
   }
 
-  /**
-   * Parses a group ID from a request body and returns the group.
-   *
-   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
-   * @return the group
-   * @throws UnprocessableEntityException thrown if the group ID cannot be resolved or if the group
-   *     is not visible to the calling user
-   */
-  public GroupDescription.Basic parse(String id) throws UnprocessableEntityException {
-    GroupDescription.Basic group = parseId(id);
-    if (group == null || !groupControlFactory.controlFor(group).isVisible()) {
-      throw new UnprocessableEntityException(String.format("Group Not Found: %s", id));
-    }
-    return group;
-  }
-
-  /**
-   * Parses a group ID from a request body and returns the group if it is a Gerrit internal group.
-   *
-   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
-   * @return the group
-   * @throws UnprocessableEntityException thrown if the group ID cannot be resolved, if the group is
-   *     not visible to the calling user or if it's an external group
-   */
-  public GroupDescription.Internal parseInternal(String id) throws UnprocessableEntityException {
-    GroupDescription.Basic group = parse(id);
-    if (group instanceof GroupDescription.Internal) {
-      return (GroupDescription.Internal) group;
-    }
-
-    throw new UnprocessableEntityException(String.format("External Group Not Allowed: %s", id));
-  }
-
-  /**
-   * Parses a group ID and returns the group without making any permission check whether the current
-   * user can see the group.
-   *
-   * @param id ID of the group, can be a group UUID, a group name or a legacy group ID
-   * @return the group, null if no group is found for the given group ID
-   */
-  public GroupDescription.Basic parseId(String id) {
-    AccountGroup.UUID uuid = new AccountGroup.UUID(id);
-    if (groupBackend.handles(uuid)) {
-      GroupDescription.Basic d = groupBackend.get(uuid);
-      if (d != null) {
-        return d;
-      }
-    }
-
-    // Might be a numeric AccountGroup.Id. -> Internal group.
-    if (id.matches("^[1-9][0-9]*$")) {
-      try {
-        AccountGroup.Id groupId = AccountGroup.Id.parse(id);
-        Optional<InternalGroup> group = groupCache.get(groupId);
-        if (group.isPresent()) {
-          return new InternalGroupDescription(group.get());
-        }
-      } catch (IllegalArgumentException e) {
-        // Ignored
-      }
-    }
-
-    // Might be a group name, be nice and accept unique names.
-    GroupReference ref = GroupBackends.findExactSuggestion(groupBackend, id);
-    if (ref != null) {
-      GroupDescription.Basic d = groupBackend.get(ref.getUUID());
-      if (d != null) {
-        return d;
-      }
-    }
-
-    return null;
-  }
-
   @Override
   public DynamicMap<RestView<GroupResource>> views() {
     return views;

java/com/google/gerrit/server/restapi/group/ListGroups.java

@@ -39,6 +39,7 @@ import com.google.gerrit.server.account.AccountResource;
 import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.InternalGroupDescription;
 import com.google.gerrit.server.group.db.Groups;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -82,7 +83,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   private final GroupJson json;
   private final GroupBackend groupBackend;
   private final Groups groups;
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
 
   private EnumSet<ListGroupsOption> options = EnumSet.noneOf(ListGroupsOption.class);
   private boolean visibleToAll;
@@ -217,7 +218,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
       final Provider<IdentifiedUser> identifiedUser,
       final IdentifiedUser.GenericFactory userFactory,
       final GetGroups accountGetGroups,
-      final GroupsCollection groupsCollection,
+      final GroupResolver groupResolver,
       GroupJson json,
       GroupBackend groupBackend,
       Groups groups) {
@@ -230,7 +231,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
     this.json = json;
     this.groupBackend = groupBackend;
     this.groups = groups;
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
   }
 
   public void setOptions(EnumSet<ListGroupsOption> options) {
@@ -403,7 +404,7 @@ public class ListGroups implements RestReadView<TopLevelResource> {
   private List<GroupInfo> getGroupsOwnedBy(String id)
       throws OrmException, RestApiException, IOException, ConfigInvalidException,
           PermissionBackendException {
-    String uuid = groupsCollection.parse(id).getGroupUUID().get();
+    String uuid = groupResolver.parse(id).getGroupUUID().get();
     return filterGroupsOwnedBy(group -> group.getOwnerGroupUUID().get().equals(uuid));
   }
 

java/com/google/gerrit/server/restapi/group/PutOwner.java

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.server.UserInitiated;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.group.GroupResource;
 import com.google.gerrit.server.group.db.GroupsUpdate;
 import com.google.gerrit.server.group.db.InternalGroupUpdate;
@@ -39,16 +40,16 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 
 @Singleton
 public class PutOwner implements RestModifyView<GroupResource, OwnerInput> {
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final Provider<GroupsUpdate> groupsUpdateProvider;
   private final GroupJson json;
 
   @Inject
   PutOwner(
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver,
       @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider,
       GroupJson json) {
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.groupsUpdateProvider = groupsUpdateProvider;
     this.json = json;
   }
@@ -68,7 +69,7 @@ public class PutOwner implements RestModifyView<GroupResource, OwnerInput> {
       throw new BadRequestException("owner is required");
     }
 
-    GroupDescription.Basic owner = groupsCollection.parse(input.owner);
+    GroupDescription.Basic owner = groupResolver.parse(input.owner);
     if (!internalGroup.getOwnerGroupUUID().equals(owner.getGroupUUID())) {
       AccountGroup.UUID groupUuid = internalGroup.getGroupUUID();
       InternalGroupUpdate groupUpdate =

java/com/google/gerrit/server/restapi/project/CreateProject.java

@@ -57,6 +57,7 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.RepositoryCaseMismatchException;
 import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.plugincontext.PluginItemContext;
 import com.google.gerrit.server.plugincontext.PluginSetContext;
@@ -67,7 +68,6 @@ import com.google.gerrit.server.project.ProjectJson;
 import com.google.gerrit.server.project.ProjectNameLockManager;
 import com.google.gerrit.server.project.ProjectResource;
 import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.gerrit.server.validators.ProjectCreationValidationListener;
 import com.google.gerrit.server.validators.ValidationException;
 import com.google.inject.Inject;
@@ -97,7 +97,7 @@ public class CreateProject
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final Provider<ProjectsCollection> projectsCollection;
-  private final Provider<GroupsCollection> groupsCollection;
+  private final Provider<GroupResolver> groupResolver;
   private final PluginSetContext<ProjectCreationValidationListener>
       projectCreationValidationListeners;
   private final ProjectJson json;
@@ -119,7 +119,7 @@ public class CreateProject
   @Inject
   CreateProject(
       Provider<ProjectsCollection> projectsCollection,
-      Provider<GroupsCollection> groupsCollection,
+      Provider<GroupResolver> groupResolver,
       ProjectJson json,
       PluginSetContext<ProjectCreationValidationListener> projectCreationValidationListeners,
       GitRepositoryManager repoManager,
@@ -137,7 +137,7 @@ public class CreateProject
       AllUsersName allUsers,
       PluginItemContext<ProjectNameLockManager> lockManager) {
     this.projectsCollection = projectsCollection;
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.projectCreationValidationListeners = projectCreationValidationListeners;
     this.json = json;
     this.repoManager = repoManager;
@@ -187,7 +187,7 @@ public class CreateProject
     } else {
       args.ownerIds = Lists.newArrayListWithCapacity(input.owners.size());
       for (String owner : input.owners) {
-        args.ownerIds.add(groupsCollection.get().parse(owner).getGroupUUID());
+        args.ownerIds.add(groupResolver.get().parse(owner).getGroupUUID());
       }
     }
     args.contributorAgreements =

java/com/google/gerrit/server/restapi/project/ListProjects.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.OutputFormat;
 import com.google.gerrit.server.WebLinks;
 import com.google.gerrit.server.account.GroupControl;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.ioutil.RegexListSearcher;
 import com.google.gerrit.server.ioutil.StringUtil;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -50,7 +51,6 @@ import com.google.gerrit.server.permissions.ProjectPermission;
 import com.google.gerrit.server.permissions.RefPermission;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.gerrit.server.util.TreeFormatter;
 import com.google.gson.reflect.TypeToken;
 import com.google.inject.Inject;
@@ -141,7 +141,7 @@ public class ListProjects implements RestReadView<TopLevelResource> {
 
   private final CurrentUser currentUser;
   private final ProjectCache projectCache;
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final GroupControl.Factory groupControlFactory;
   private final GitRepositoryManager repoManager;
   private final PermissionBackend permissionBackend;
@@ -262,7 +262,7 @@ public class ListProjects implements RestReadView<TopLevelResource> {
   protected ListProjects(
       CurrentUser currentUser,
       ProjectCache projectCache,
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver,
       GroupControl.Factory groupControlFactory,
       GitRepositoryManager repoManager,
       PermissionBackend permissionBackend,
@@ -270,7 +270,7 @@ public class ListProjects implements RestReadView<TopLevelResource> {
       WebLinks webLinks) {
     this.currentUser = currentUser;
     this.projectCache = projectCache;
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.groupControlFactory = groupControlFactory;
     this.repoManager = repoManager;
     this.permissionBackend = permissionBackend;
@@ -367,7 +367,7 @@ public class ListProjects implements RestReadView<TopLevelResource> {
 
         if (groupUuid != null
             && !e.getLocalGroups()
-                .contains(GroupReference.forGroup(groupsCollection.parseId(groupUuid.get())))) {
+                .contains(GroupReference.forGroup(groupResolver.parseId(groupUuid.get())))) {
           continue;
         }
 

java/com/google/gerrit/server/restapi/project/SetAccessUtil.java

@@ -32,11 +32,11 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.group.GroupResolver;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ProjectConfig;
 import com.google.gerrit.server.project.RefPattern;
 import com.google.gerrit.server.restapi.config.ListCapabilities;
-import com.google.gerrit.server.restapi.group.GroupsCollection;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
@@ -48,18 +48,18 @@ import java.util.Set;
 
 @Singleton
 public class SetAccessUtil {
-  private final GroupsCollection groupsCollection;
+  private final GroupResolver groupResolver;
   private final AllProjectsName allProjects;
   private final Provider<SetParent> setParent;
   private final ListCapabilities listCapabilities;
 
   @Inject
   private SetAccessUtil(
-      GroupsCollection groupsCollection,
+      GroupResolver groupResolver,
       AllProjectsName allProjects,
       Provider<SetParent> setParent,
       ListCapabilities listCapabilities) {
-    this.groupsCollection = groupsCollection;
+    this.groupResolver = groupResolver;
     this.allProjects = allProjects;
     this.setParent = setParent;
     this.listCapabilities = listCapabilities;
@@ -91,7 +91,7 @@ public class SetAccessUtil {
 
         for (Map.Entry<String, PermissionRuleInfo> permissionRuleInfoEntry :
             permissionEntry.getValue().rules.entrySet()) {
-          GroupDescription.Basic group = groupsCollection.parseId(permissionRuleInfoEntry.getKey());
+          GroupDescription.Basic group = groupResolver.parseId(permissionRuleInfoEntry.getKey());
           if (group == null) {
             throw new UnprocessableEntityException(
                 permissionRuleInfoEntry.getKey() + " is not a valid group ID");

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.acceptance.git;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth8.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
@@ -64,6 +65,7 @@ import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.ChangeMessageInfo;
 import com.google.gerrit.extensions.common.CommentInfo;
@@ -945,6 +947,55 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     assertThat(cr.all.get(0).value).isEqualTo(2);
   }
 
+  @Test
+  public void pushForMasterWithForgedAuthorAndCommitter() throws Exception {
+    TestAccount user2 = accountCreator.user2();
+    // Create a commit with different forged author and committer.
+    RevCommit c =
+        commitBuilder()
+            .author(user.getIdent())
+            .committer(user2.getIdent())
+            .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT)
+            .message(PushOneCommit.SUBJECT)
+            .create();
+    // Push commit as "Admnistrator".
+    pushHead(testRepo, "refs/for/master");
+
+    String changeId = GitUtil.getChangeId(testRepo, c).get();
+    assertThat(getOwnerEmail(changeId)).isEqualTo(admin.email);
+    assertThat(getReviewerEmails(changeId, ReviewerState.REVIEWER))
+        .containsExactly(user.email, user2.email);
+
+    assertThat(sender.getMessages()).hasSize(1);
+    assertThat(sender.getMessages().get(0).rcpt())
+        .containsExactly(user.emailAddress, user2.emailAddress);
+  }
+
+  @Test
+  public void pushNewPatchSetForMasterWithForgedAuthorAndCommitter() throws Exception {
+    TestAccount user2 = accountCreator.user2();
+    // First patch set has author and committer matching change owner.
+    PushOneCommit.Result r = pushTo("refs/for/master");
+
+    assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email);
+    assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER)).isEmpty();
+
+    amendBuilder()
+        .author(user.getIdent())
+        .committer(user2.getIdent())
+        .add(PushOneCommit.FILE_NAME, PushOneCommit.FILE_CONTENT + "2")
+        .create();
+    pushHead(testRepo, "refs/for/master");
+
+    assertThat(getOwnerEmail(r.getChangeId())).isEqualTo(admin.email);
+    assertThat(getReviewerEmails(r.getChangeId(), ReviewerState.REVIEWER))
+        .containsExactly(user.email, user2.email);
+
+    assertThat(sender.getMessages()).hasSize(1);
+    assertThat(sender.getMessages().get(0).rcpt())
+        .containsExactly(user.emailAddress, user2.emailAddress);
+  }
+
   /**
    * There was a bug that allowed a user with Forge Committer Identity access right to upload a
    * commit and put *votes on behalf of another user* on it. This test checks that this is not
@@ -2336,4 +2387,17 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
   private PushOneCommit.Result amendChange(String changeId, String ref) throws Exception {
     return amendChange(changeId, ref, admin, testRepo);
   }
+
+  private String getOwnerEmail(String changeId) throws Exception {
+    return get(changeId, DETAILED_ACCOUNTS).owner.email;
+  }
+
+  private ImmutableList<String> getReviewerEmails(String changeId, ReviewerState state)
+      throws Exception {
+    Collection<AccountInfo> infos =
+        get(changeId, DETAILED_LABELS, DETAILED_ACCOUNTS).reviewers.get(state);
+    return infos != null
+        ? infos.stream().map(a -> a.email).collect(toImmutableList())
+        : ImmutableList.of();
+  }
 }

javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java

@@ -27,6 +27,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AddReviewerResult;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ReviewerState;
@@ -319,6 +320,42 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void addExistingReviewerByEmailShortCircuits() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    PushOneCommit.Result r = createChange();
+
+    AddReviewerInput input = new AddReviewerInput();
+    input.reviewer = "nonexisting@example.com";
+    input.state = ReviewerState.REVIEWER;
+
+    AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(input);
+    assertThat(result.reviewers).hasSize(1);
+    ReviewerInfo info = result.reviewers.get(0);
+    assertThat(info._accountId).isNull();
+    assertThat(info.email).isEqualTo(input.reviewer);
+
+    assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).reviewers).isEmpty();
+  }
+
+  @Test
+  public void addExistingCcByEmailShortCircuits() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    PushOneCommit.Result r = createChange();
+
+    AddReviewerInput input = new AddReviewerInput();
+    input.reviewer = "nonexisting@example.com";
+    input.state = ReviewerState.CC;
+    AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(input);
+
+    assertThat(result.ccs).hasSize(1);
+    AccountInfo info = result.ccs.get(0);
+    assertThat(info._accountId).isNull();
+    assertThat(info.email).isEqualTo(input.reviewer);
+
+    assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).ccs).isEmpty();
+  }
+
   private static String toRfcAddressString(AccountInfo info) {
     return (new Address(info.name, info.email)).toString();
   }

javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java

@@ -39,6 +39,7 @@ import com.google.gerrit.extensions.api.changes.NotifyInfo;
 import com.google.gerrit.extensions.api.changes.RecipientType;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.ReviewResult;
+import com.google.gerrit.extensions.api.changes.ReviewerInfo;
 import com.google.gerrit.extensions.client.ReviewerState;
 import com.google.gerrit.extensions.common.AccountInfo;
 import com.google.gerrit.extensions.common.ApprovalInfo;
@@ -48,7 +49,7 @@ import com.google.gerrit.extensions.common.ReviewerUpdateInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.mail.Address;
 import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.server.restapi.change.PostReviewers;
+import com.google.gerrit.server.change.ReviewerAdder;
 import com.google.gerrit.testing.FakeEmailSender.Message;
 import com.google.gson.stream.JsonReader;
 import java.util.ArrayList;
@@ -68,8 +69,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     String largeGroup = createGroup("largeGroup");
     String mediumGroup = createGroup("mediumGroup");
 
-    int largeGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS + 1;
+    int largeGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS + 1;
-    int mediumGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
+    int mediumGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
     List<TestAccount> users = createAccounts(largeGroupSize, "addGroupAsReviewer");
     List<String> largeGroupUsernames = new ArrayList<>(mediumGroupSize);
     for (TestAccount u : users) {
@@ -470,8 +471,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
 
   @Test
   public void reviewAndAddGroupReviewers() throws Exception {
-    int largeGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS + 1;
+    int largeGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS + 1;
-    int mediumGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
+    int mediumGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
     List<TestAccount> users = createAccounts(largeGroupSize, "reviewAndAddGroupReviewers");
     List<String> usernames = new ArrayList<>(largeGroupSize);
     for (TestAccount u : users) {
@@ -784,6 +785,40 @@ public class ChangeReviewersIT extends AbstractDaemonTest {
     gApi.changes().id(r.getChangeId()).reviewer(user.email).remove();
   }
 
+  @Test
+  public void addExistingReviewerShortCircuits() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    PushOneCommit.Result r = createChange();
+
+    AddReviewerInput input = new AddReviewerInput();
+    input.reviewer = user.email;
+    input.state = ReviewerState.REVIEWER;
+
+    AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(input);
+    assertThat(result.reviewers).hasSize(1);
+    ReviewerInfo info = result.reviewers.get(0);
+    assertThat(info._accountId).isEqualTo(user.id.get());
+
+    assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).reviewers).isEmpty();
+  }
+
+  @Test
+  public void addExistingCcShortCircuits() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    PushOneCommit.Result r = createChange();
+
+    AddReviewerInput input = new AddReviewerInput();
+    input.reviewer = user.email;
+    input.state = ReviewerState.CC;
+
+    AddReviewerResult result = gApi.changes().id(r.getChangeId()).addReviewer(input);
+    assertThat(result.ccs).hasSize(1);
+    AccountInfo info = result.ccs.get(0);
+    assertThat(info._accountId).isEqualTo(user.id.get());
+
+    assertThat(gApi.changes().id(r.getChangeId()).addReviewer(input).ccs).isEmpty();
+  }
+
   private void assertThatUserIsOnlyReviewer(String changeId) throws Exception {
     AccountInfo userInfo = new AccountInfo(user.fullName, user.emailAddress.getEmail());
     userInfo._accountId = user.id.get();

javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java

@@ -566,15 +566,64 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
     addReviewerToReviewableChangeInNoteDbByOwnerCcingSelfNotifyNone(batch());
   }
 
+  private void addNonUserReviewerByEmailInNoteDb(Adder adder) throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    StagedChange sc = stageReviewableChange();
+    addReviewer(adder, sc.changeId, sc.owner, "nonexistent@example.com");
+    assertThat(sender)
+        .sent("newchange", sc)
+        .to("nonexistent@example.com")
+        .cc(sc.reviewer)
+        .cc(sc.ccerByEmail, sc.reviewerByEmail)
+        .noOneElse();
+  }
+
+  @Test
+  public void addNonUserReviewerByEmailInNoteDbSingly() throws Exception {
+    addNonUserReviewerByEmailInNoteDb(singly(ReviewerState.REVIEWER));
+  }
+
+  @Test
+  public void addNonUserReviewerByEmailInNoteDbBatch() throws Exception {
+    addNonUserReviewerByEmailInNoteDb(batch(ReviewerState.REVIEWER));
+  }
+
+  private void addNonUserCcByEmailInNoteDb(Adder adder) throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    StagedChange sc = stageReviewableChange();
+    addReviewer(adder, sc.changeId, sc.owner, "nonexistent@example.com");
+    assertThat(sender)
+        .sent("newchange", sc)
+        .cc("nonexistent@example.com")
+        .cc(sc.reviewer)
+        .cc(sc.ccerByEmail, sc.reviewerByEmail)
+        .noOneElse();
+  }
+
+  @Test
+  public void addNonUserCcByEmailInNoteDbSingly() throws Exception {
+    addNonUserCcByEmailInNoteDb(singly(ReviewerState.CC));
+  }
+
+  @Test
+  public void addNonUserCcByEmailInNoteDbBatch() throws Exception {
+    addNonUserCcByEmailInNoteDb(batch(ReviewerState.CC));
+  }
+
   private interface Adder {
     void addReviewer(String changeId, String reviewer, @Nullable NotifyHandling notify)
         throws Exception;
   }
 
   private Adder singly() {
+    return singly(ReviewerState.REVIEWER);
+  }
+
+  private Adder singly(ReviewerState reviewerState) {
     return (String changeId, String reviewer, @Nullable NotifyHandling notify) -> {
       AddReviewerInput in = new AddReviewerInput();
       in.reviewer = reviewer;
+      in.state = reviewerState;
       if (notify != null) {
         in.notify = notify;
       }
@@ -583,9 +632,13 @@ public class ChangeNotificationsIT extends AbstractNotificationTest {
   }
 
   private Adder batch() {
+    return batch(ReviewerState.REVIEWER);
+  }
+
+  private Adder batch(ReviewerState reviewerState) {
     return (String changeId, String reviewer, @Nullable NotifyHandling notify) -> {
       ReviewInput in = ReviewInput.noScore();
-      in.reviewer(reviewer);
+      in.reviewer(reviewer, reviewerState, false);
       if (notify != null) {
         in.notify = notify;
       }