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
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index df57c27..a4945c7 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -195,8 +195,8 @@
if (recipients.get(type).contains(email) != expected) {
failWithoutActual(
fact(
- expected ? "notifies" : "doesn't notify",
- "[\n" + type + ": " + users.emailToName(email) + "\n]"));
+ expected ? "should notify" : "shouldn't notify",
+ type + ": " + users.emailToName(email)));
}
if (expected) {
accountedFor.add(email);
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java b/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
index 3a33de9..3c32d29 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewerInfo.java
@@ -38,7 +38,7 @@
@Override
public String toString() {
- return username;
+ return username != null ? username : email;
}
private ReviewerInfo() {}
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index d2a483a..48bd1c1 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -19,7 +19,13 @@
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 @@
@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 @@
.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);
+ }
}
diff --git a/java/com/google/gerrit/server/api/groups/GroupsImpl.java b/java/com/google/gerrit/server/api/groups/GroupsImpl.java
index bac6649..1fdeac1 100644
--- a/java/com/google/gerrit/server/api/groups/GroupsImpl.java
+++ b/java/com/google/gerrit/server/api/groups/GroupsImpl.java
@@ -26,10 +26,11 @@
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 @@
@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 @@
@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 @@
}
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 @@
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);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewersEmail.java b/java/com/google/gerrit/server/change/AddReviewersEmail.java
similarity index 94%
rename from java/com/google/gerrit/server/restapi/change/PostReviewersEmail.java
rename to java/com/google/gerrit/server/change/AddReviewersEmail.java
index 7540222..4173950 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewersEmail.java
+++ b/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 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,
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
new file mode 100644
index 0000000..e558d00
--- /dev/null
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -0,0 +1,272 @@
+// Copyright (C) 2017 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.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;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.client.ReviewerState;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.mail.Address;
+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.server.ApprovalsUtil;
+import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.extensions.events.ReviewerAdded;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.update.BatchUpdateOp;
+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.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Set;
+
+public class AddReviewersOp implements BatchUpdateOp {
+ public interface Factory {
+
+ /**
+ * 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);
+ }
+
+ @AutoValue
+ 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_AddReviewersOp_Result.Builder();
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder setAddedReviewers(Iterable<PatchSetApproval> addedReviewers);
+
+ abstract Builder setAddedReviewersByEmail(Iterable<Address> addedReviewersByEmail);
+
+ abstract Builder setAddedCCs(Iterable<Account.Id> addedCCs);
+
+ abstract Builder setAddedCCsByEmail(Iterable<Address> addedCCsByEmail);
+
+ abstract Result build();
+ }
+ }
+
+ private final ApprovalsUtil approvalsUtil;
+ private final PatchSetUtil psUtil;
+ private final ReviewerAdded reviewerAdded;
+ private final AccountCache accountCache;
+ private final ProjectCache projectCache;
+ private final AddReviewersEmail addReviewersEmail;
+ private final NotesMigration migration;
+ private final Set<Account.Id> accountIds;
+ private final Collection<Address> addresses;
+ private final ReviewerState state;
+ private final NotifyHandling notify;
+ private final ListMultimap<RecipientType, Account.Id> accountsToNotify;
+
+ // Unlike addedCCs, addedReviewers is a PatchSetApproval because the AddReviewerResult returned
+ // via the REST API is supposed to include vote information.
+ 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
+ AddReviewersOp(
+ ApprovalsUtil approvalsUtil,
+ PatchSetUtil psUtil,
+ ReviewerAdded reviewerAdded,
+ AccountCache accountCache,
+ ProjectCache projectCache,
+ AddReviewersEmail addReviewersEmail,
+ NotesMigration migration,
+ @Assisted Set<Account.Id> accountIds,
+ @Assisted Collection<Address> addresses,
+ @Assisted ReviewerState state,
+ @Assisted @Nullable NotifyHandling notify,
+ @Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify) {
+ checkArgument(state == REVIEWER || state == CC, "must be %s or %s: %s", REVIEWER, CC, state);
+ this.approvalsUtil = approvalsUtil;
+ this.psUtil = psUtil;
+ this.reviewerAdded = reviewerAdded;
+ this.accountCache = accountCache;
+ this.projectCache = projectCache;
+ this.addReviewersEmail = addReviewersEmail;
+ this.migration = migration;
+
+ this.accountIds = accountIds;
+ this.addresses = addresses;
+ this.state = state;
+ this.notify = notify;
+ this.accountsToNotify = accountsToNotify;
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx)
+ throws RestApiException, OrmException, IOException {
+ change = ctx.getChange();
+ if (!accountIds.isEmpty()) {
+ if (migration.readChanges() && state == CC) {
+ addedCCs =
+ approvalsUtil.addCcs(
+ ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), accountIds);
+ } else {
+ addedReviewers =
+ approvalsUtil.addReviewers(
+ ctx.getDb(),
+ ctx.getNotes(),
+ ctx.getUpdate(change.currentPatchSetId()),
+ projectCache.checkedGet(change.getProject()).getLabelTypes(change.getDest()),
+ change,
+ accountIds);
+ }
+ }
+
+ 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;
+ }
+
+ checkAdded();
+
+ 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(addedReviewers)
+ .setAddedReviewersByEmail(addedReviewersByEmail)
+ .setAddedCCs(addedCCs)
+ .setAddedCCsByEmail(addedCCsByEmail)
+ .build();
+ addReviewersEmail.emailReviewers(
+ ctx.getUser().asIdentifiedUser(),
+ change,
+ Lists.transform(addedReviewers, PatchSetApproval::getAccountId),
+ addedCCs,
+ addedReviewersByEmail,
+ addedCCsByEmail,
+ notify,
+ accountsToNotify,
+ !change.isWorkInProgress());
+ if (!addedReviewers.isEmpty()) {
+ List<AccountState> reviewers =
+ addedReviewers
+ .stream()
+ .map(r -> accountCache.get(r.getAccountId()))
+ .flatMap(Streams::stream)
+ .collect(toList());
+ reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+ }
+ }
+
+ public Result getResult() {
+ checkState(opResult != null, "Batch update wasn't executed yet");
+ return opResult;
+ }
+}
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
new file mode 100644
index 0000000..f2be90d
--- /dev/null
+++ b/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);
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerJson.java b/java/com/google/gerrit/server/change/ReviewerJson.java
similarity index 97%
rename from java/com/google/gerrit/server/restapi/change/ReviewerJson.java
rename to java/com/google/gerrit/server/change/ReviewerJson.java
index 29c5649..6502569 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewerJson.java
+++ b/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.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;
diff --git a/java/com/google/gerrit/server/group/GroupResolver.java b/java/com/google/gerrit/server/group/GroupResolver.java
new file mode 100644
index 0000000..5fe3e8e
--- /dev/null
+++ b/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;
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/account/AccountsCollection.java b/java/com/google/gerrit/server/restapi/account/AccountsCollection.java
index 370833c..c301ab2 100644
--- a/java/com/google/gerrit/server/restapi/account/AccountsCollection.java
+++ b/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.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 @@
@Singleton
public class AccountsCollection implements RestCollection<TopLevelResource, AccountResource> {
- private final Provider<CurrentUser> self;
- private final AccountResolver resolver;
+ private final AccountResolver accountResolver;
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 resolver,
+ AccountResolver accountResolver,
AccountControl.Factory accountControlFactory,
- IdentifiedUser.GenericFactory userFactory,
Provider<QueryAccounts> list,
DynamicMap<RestView<AccountResource>> views) {
- this.self = self;
- this.resolver = resolver;
+ this.accountResolver = accountResolver;
this.accountControlFactory = accountControlFactory;
- this.userFactory = userFactory;
this.list = list;
this.views = views;
}
@@ -66,7 +55,7 @@
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 @@
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();
diff --git a/java/com/google/gerrit/server/restapi/account/CreateAccount.java b/java/com/google/gerrit/server/restapi/account/CreateAccount.java
index 0e8eb70..4185f36 100644
--- a/java/com/google/gerrit/server/restapi/account/CreateAccount.java
+++ b/java/com/google/gerrit/server/restapi/account/CreateAccount.java
@@ -46,11 +46,11 @@
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 @@
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 @@
@Inject
CreateAccount(
Sequences seq,
- GroupsCollection groupsCollection,
+ GroupResolver groupResolver,
VersionedAuthorizedKeys.Accessor authorizedKeys,
SshKeyCache sshKeyCache,
@UserInitiated Provider<AccountsUpdate> accountsUpdateProvider,
@@ -89,7 +89,7 @@
@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 @@
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());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetReviewer.java b/java/com/google/gerrit/server/restapi/change/GetReviewer.java
index b9b6b09..a11380b 100644
--- a/java/com/google/gerrit/server/restapi/change/GetReviewer.java
+++ b/java/com/google/gerrit/server/restapi/change/GetReviewer.java
@@ -16,6 +16,7 @@
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;
diff --git a/java/com/google/gerrit/server/restapi/change/ListReviewers.java b/java/com/google/gerrit/server/restapi/change/ListReviewers.java
index 46bb33f..99d8746 100644
--- a/java/com/google/gerrit/server/restapi/change/ListReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/ListReviewers.java
@@ -21,6 +21,7 @@
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;
diff --git a/java/com/google/gerrit/server/restapi/change/ListRevisionReviewers.java b/java/com/google/gerrit/server/restapi/change/ListRevisionReviewers.java
index 32c4ea3..7add548 100644
--- a/java/com/google/gerrit/server/restapi/change/ListRevisionReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/ListRevisionReviewers.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.ReviewerJson;
import com.google.gerrit.server.change.ReviewerResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java
index f8b0cc4..a45a6d8 100644
--- a/java/com/google/gerrit/server/restapi/change/Module.java
+++ b/java/com/google/gerrit/server/restapi/change/Module.java
@@ -29,6 +29,7 @@
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 @@
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);
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index c7028ab..e47c582 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -88,10 +88,13 @@
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.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 @@
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 PostReviewersEmail postReviewersEmail;
+ private final ReviewerAdder reviewerAdder;
+ private final AddReviewersEmail addReviewersEmail;
private final NotesMigration migration;
private final NotifyUtil notifyUtil;
private final Config gerritConfig;
@@ -184,7 +186,7 @@
PostReview(
Provider<ReviewDb> db,
RetryHelper retryHelper,
- Factory changeResourceFactory,
+ ChangeResource.Factory changeResourceFactory,
ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
@@ -192,11 +194,11 @@
PublishCommentUtil publishCommentUtil,
PatchSetUtil psUtil,
PatchListCache patchListCache,
- AccountsCollection accounts,
+ AccountResolver accountResolver,
EmailReviewComments.Factory email,
CommentAdded commentAdded,
- PostReviewers postReviewers,
- PostReviewersEmail postReviewersEmail,
+ ReviewerAdder reviewerAdder,
+ AddReviewersEmail addReviewersEmail,
NotesMigration migration,
NotifyUtil notifyUtil,
@GerritServerConfig Config gerritConfig,
@@ -213,11 +215,11 @@
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.postReviewersEmail = postReviewersEmail;
+ this.reviewerAdder = reviewerAdder;
+ this.addReviewersEmail = addReviewersEmail;
this.migration = migration;
this.notifyUtil = notifyUtil;
this.gerritConfig = gerritConfig;
@@ -273,21 +275,20 @@
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 =
- postReviewers.prepareApplication(
- revision.getNotes(), revision.getUser(), reviewerInput, true);
+ ReviewerAddition result =
+ reviewerAdder.prepare(revision.getNotes(), revision.getUser(), reviewerInput, true);
reviewerJsonResults.put(reviewerInput.reviewer, result.result);
if (result.result.error != null) {
hasError = true;
@@ -327,7 +328,7 @@
// 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 @@
// 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 =
- postReviewers.ccCurrentUser(revision.getUser(), revision);
+ ReviewerAddition selfAddition = reviewerAdder.ccCurrentUser(revision.getUser(), revision);
bu.addOp(revision.getChange().getId(), selfAddition.op);
}
@@ -389,13 +389,13 @@
// 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 @@
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 @@
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 @@
ccByEmail.addAll(addition.reviewersByEmail);
}
}
- postReviewersEmail.emailReviewers(
+ addReviewersEmail.emailReviewers(
user.asIdentifiedUser(),
change,
to,
@@ -505,7 +505,7 @@
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)
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewers.java b/java/com/google/gerrit/server/restapi/change/PostReviewers.java
index dd6e457..9147d44 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewers.java
+++ b/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.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 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;
+ private final ReviewerAdder reviewerAdder;
@Inject
PostReviewers(
- AccountsCollection accounts,
- PermissionBackend permissionBackend,
- GroupsCollection groupsCollection,
- GroupMembers groupMembers,
- AccountLoader.Factory accountLoaderFactory,
Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
RetryHelper retryHelper,
- @GerritServerConfig Config cfg,
- ReviewerJson json,
- NotesMigration migration,
- NotifyUtil notifyUtil,
- ProjectCache projectCache,
- Provider<AnonymousUser> anonymousProvider,
- PostReviewersOp.Factory postReviewersOpFactory,
- OutgoingEmailValidator validator) {
+ ReviewerAdder reviewerAdder) {
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.json = json;
- this.migration = migration;
- this.notifyUtil = notifyUtil;
- this.projectCache = projectCache;
- this.anonymousProvider = anonymousProvider;
- this.postReviewersOpFactory = postReviewersOpFactory;
- this.validator = validator;
+ this.reviewerAdder = reviewerAdder;
}
@Override
@@ -156,7 +68,7 @@
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 @@
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);
- }
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java
deleted file mode 100644
index 6379393..0000000
--- a/java/com/google/gerrit/server/restapi/change/PostReviewersOp.java
+++ /dev/null
@@ -1,215 +0,0 @@
-// Copyright (C) 2017 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.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 static java.util.stream.Collectors.toList;
-
-import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ListMultimap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Streams;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.extensions.api.changes.NotifyHandling;
-import com.google.gerrit.extensions.api.changes.RecipientType;
-import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.mail.Address;
-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;
-import com.google.gerrit.server.extensions.events.ReviewerAdded;
-import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.update.BatchUpdateOp;
-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 interface Factory {
- PostReviewersOp create(
- Set<Account.Id> reviewers,
- Collection<Address> reviewersByEmail,
- ReviewerState state,
- @Nullable NotifyHandling notify,
- ListMultimap<RecipientType, Account.Id> accountsToNotify);
- }
-
- @AutoValue
- public abstract static class Result {
- public abstract ImmutableList<PatchSetApproval> addedReviewers();
-
- public abstract ImmutableList<Account.Id> addedCCs();
-
- static Builder builder() {
- return new AutoValue_PostReviewersOp_Result.Builder();
- }
-
- @AutoValue.Builder
- abstract static class Builder {
- abstract Builder setAddedReviewers(ImmutableList<PatchSetApproval> addedReviewers);
-
- abstract Builder setAddedCCs(ImmutableList<Account.Id> addedCCs);
-
- abstract Result build();
- }
- }
-
- private final ApprovalsUtil approvalsUtil;
- private final PatchSetUtil psUtil;
- private final ReviewerAdded reviewerAdded;
- private final AccountCache accountCache;
- private final ProjectCache projectCache;
- private final PostReviewersEmail postReviewersEmail;
- private final NotesMigration migration;
- private final Provider<IdentifiedUser> user;
- private final Provider<ReviewDb> dbProvider;
- 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<>();
- private Collection<Account.Id> addedCCs = new ArrayList<>();
- private Collection<Address> addedCCsByEmail = new ArrayList<>();
- private Change change;
- private PatchSet patchSet;
- private Result opResult;
-
- @Inject
- PostReviewersOp(
- ApprovalsUtil approvalsUtil,
- PatchSetUtil psUtil,
- ReviewerAdded reviewerAdded,
- AccountCache accountCache,
- ProjectCache projectCache,
- PostReviewersEmail postReviewersEmail,
- NotesMigration migration,
- Provider<IdentifiedUser> user,
- Provider<ReviewDb> dbProvider,
- @Assisted Set<Account.Id> reviewers,
- @Assisted Collection<Address> reviewersByEmail,
- @Assisted ReviewerState state,
- @Assisted @Nullable NotifyHandling notify,
- @Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify) {
- checkArgument(state == REVIEWER || state == CC, "must be %s or %s: %s", REVIEWER, CC, state);
- this.approvalsUtil = approvalsUtil;
- this.psUtil = psUtil;
- this.reviewerAdded = reviewerAdded;
- this.accountCache = accountCache;
- this.projectCache = projectCache;
- this.postReviewersEmail = postReviewersEmail;
- this.migration = migration;
- this.user = user;
- this.dbProvider = dbProvider;
-
- this.reviewers = reviewers;
- this.reviewersByEmail = reviewersByEmail;
- this.state = state;
- this.notify = notify;
- this.accountsToNotify = accountsToNotify;
- }
-
- @Override
- public boolean updateChange(ChangeContext ctx)
- throws RestApiException, OrmException, IOException {
- change = ctx.getChange();
- if (!reviewers.isEmpty()) {
- if (migration.readChanges() && state == CC) {
- addedCCs =
- approvalsUtil.addCcs(
- ctx.getNotes(), ctx.getUpdate(change.currentPatchSetId()), reviewers);
- if (addedCCs.isEmpty()) {
- return false;
- }
- } else {
- addedReviewers =
- approvalsUtil.addReviewers(
- ctx.getDb(),
- ctx.getNotes(),
- ctx.getUpdate(change.currentPatchSetId()),
- projectCache.checkedGet(change.getProject()).getLabelTypes(change.getDest()),
- change,
- reviewers);
- if (addedReviewers.isEmpty()) {
- return false;
- }
- }
- }
-
- for (Address a : reviewersByEmail) {
- ctx.getUpdate(change.currentPatchSetId())
- .putReviewerByEmail(a, ReviewerStateInternal.fromReviewerState(state));
- }
-
- patchSet = psUtil.current(dbProvider.get(), ctx.getNotes());
- return true;
- }
-
- @Override
- public void postUpdate(Context ctx) throws Exception {
- opResult =
- Result.builder()
- .setAddedReviewers(ImmutableList.copyOf(addedReviewers))
- .setAddedCCs(ImmutableList.copyOf(addedCCs))
- .build();
- postReviewersEmail.emailReviewers(
- user.get(),
- change,
- Lists.transform(addedReviewers, PatchSetApproval::getAccountId),
- addedCCs == null ? ImmutableList.of() : addedCCs,
- reviewersByEmail,
- addedCCsByEmail,
- notify,
- accountsToNotify,
- !change.isWorkInProgress());
- if (!addedReviewers.isEmpty()) {
- List<AccountState> reviewers =
- addedReviewers
- .stream()
- .map(r -> accountCache.get(r.getAccountId()))
- .flatMap(Streams::stream)
- .collect(toList());
- reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
- }
- }
-
- public Result getResult() {
- checkState(opResult != null, "Batch update wasn't executed yet");
- return opResult;
- }
-}
diff --git a/java/com/google/gerrit/server/restapi/change/PutAssignee.java b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
index 9d626fc..7ca674f 100644
--- a/java/com/google/gerrit/server/restapi/change/PutAssignee.java
+++ b/java/com/google/gerrit/server/restapi/change/PutAssignee.java
@@ -28,13 +28,14 @@
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 @@
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 @@
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 @@
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 @@
}
}
- 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
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
index 3becf24..f7959c8 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
@@ -47,6 +47,7 @@
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 @@
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;
}
diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java
index 8ceddf0..773d12d 100644
--- a/java/com/google/gerrit/server/restapi/change/Submit.java
+++ b/java/com/google/gerrit/server/restapi/change/Submit.java
@@ -42,6 +42,7 @@
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.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 @@
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 @@
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 @@
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 @@
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)
diff --git a/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java b/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
index b1d49f2..6e94218 100644
--- a/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
+++ b/java/com/google/gerrit/server/restapi/change/SuggestReviewers.java
@@ -19,6 +19,7 @@
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 @@
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());
}
diff --git a/java/com/google/gerrit/server/restapi/group/AddMembers.java b/java/com/google/gerrit/server/restapi/group/AddMembers.java
index a897e1a..bdf1c74 100644
--- a/java/com/google/gerrit/server/restapi/group/AddMembers.java
+++ b/java/com/google/gerrit/server/restapi/group/AddMembers.java
@@ -47,7 +47,6 @@
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 @@
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 @@
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 @@
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
diff --git a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
index ca77ebf..9782ad3 100644
--- a/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
+++ b/java/com/google/gerrit/server/restapi/group/AddSubgroups.java
@@ -32,6 +32,7 @@
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 @@
}
}
- 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 @@
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));
}
diff --git a/java/com/google/gerrit/server/restapi/group/CreateGroup.java b/java/com/google/gerrit/server/restapi/group/CreateGroup.java
index 6b01cda..0572114 100644
--- a/java/com/google/gerrit/server/restapi/group/CreateGroup.java
+++ b/java/com/google/gerrit/server/restapi/group/CreateGroup.java
@@ -42,6 +42,7 @@
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 @@
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 @@
@GerritPersonIdent PersonIdent serverIdent,
@UserInitiated Provider<GroupsUpdate> groupsUpdateProvider,
GroupCache groupCache,
- GroupsCollection groups,
+ GroupResolver groups,
GroupJson json,
PluginSetContext<GroupCreationValidationListener> groupCreationValidationListeners,
AddMembers addMembers,
diff --git a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
index bcacb65..d197cb8 100644
--- a/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
+++ b/java/com/google/gerrit/server/restapi/group/DeleteMembers.java
@@ -26,12 +26,12 @@
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 @@
@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) {
- this.accounts = accounts;
+ AccountResolver accountResolver, @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
+ this.accountResolver = accountResolver;
this.groupsUpdateProvider = groupsUpdateProvider;
}
@@ -69,7 +69,7 @@
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();
diff --git a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
index 934698b..c486af4 100644
--- a/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
+++ b/java/com/google/gerrit/server/restapi/group/DeleteSubgroups.java
@@ -27,6 +27,7 @@
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 @@
@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,
- @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
- this.groupsCollection = groupsCollection;
+ GroupResolver groupResolver, @UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
+ this.groupResolver = groupResolver;
this.groupsUpdateProvider = groupsUpdateProvider;
}
@@ -70,7 +70,7 @@
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());
}
diff --git a/java/com/google/gerrit/server/restapi/group/GroupsCollection.java b/java/com/google/gerrit/server/restapi/group/GroupsCollection.java
index 40a11c7..52fe9d0 100644
--- a/java/com/google/gerrit/server/restapi/group/GroupsCollection.java
+++ b/java/com/google/gerrit/server/restapi/group/GroupsCollection.java
@@ -16,7 +16,6 @@
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.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 @@
private final Provider<ListGroups> list;
private final Provider<QueryGroups> queryGroups;
private final GroupControl.Factory groupControlFactory;
- private final GroupBackend groupBackend;
- private final GroupCache groupCache;
+ private final GroupResolver groupResolver;
private final Provider<CurrentUser> self;
private boolean hasQuery2;
@@ -59,15 +50,13 @@
Provider<ListGroups> list,
Provider<QueryGroups> queryGroups,
GroupControl.Factory groupControlFactory,
- GroupBackend groupBackend,
- GroupCache groupCache,
+ GroupResolver groupResolver,
Provider<CurrentUser> self) {
this.views = views;
this.list = list;
this.queryGroups = queryGroups;
this.groupControlFactory = groupControlFactory;
- this.groupBackend = groupBackend;
- this.groupCache = groupCache;
+ this.groupResolver = groupResolver;
this.self = self;
}
@@ -107,7 +96,7 @@
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 @@
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;
diff --git a/java/com/google/gerrit/server/restapi/group/ListGroups.java b/java/com/google/gerrit/server/restapi/group/ListGroups.java
index bae5eff..968a7dd 100644
--- a/java/com/google/gerrit/server/restapi/group/ListGroups.java
+++ b/java/com/google/gerrit/server/restapi/group/ListGroups.java
@@ -39,6 +39,7 @@
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 @@
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 @@
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 @@
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 @@
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));
}
diff --git a/java/com/google/gerrit/server/restapi/group/PutOwner.java b/java/com/google/gerrit/server/restapi/group/PutOwner.java
index 56f856d..6ebec05 100644
--- a/java/com/google/gerrit/server/restapi/group/PutOwner.java
+++ b/java/com/google/gerrit/server/restapi/group/PutOwner.java
@@ -26,6 +26,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.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 @@
@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 @@
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 =
diff --git a/java/com/google/gerrit/server/restapi/project/CreateProject.java b/java/com/google/gerrit/server/restapi/project/CreateProject.java
index 79ff3c9..09f8ab7 100644
--- a/java/com/google/gerrit/server/restapi/project/CreateProject.java
+++ b/java/com/google/gerrit/server/restapi/project/CreateProject.java
@@ -57,6 +57,7 @@
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.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 @@
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 @@
@Inject
CreateProject(
Provider<ProjectsCollection> projectsCollection,
- Provider<GroupsCollection> groupsCollection,
+ Provider<GroupResolver> groupResolver,
ProjectJson json,
PluginSetContext<ProjectCreationValidationListener> projectCreationValidationListeners,
GitRepositoryManager repoManager,
@@ -137,7 +137,7 @@
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 @@
} 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 =
diff --git a/java/com/google/gerrit/server/restapi/project/ListProjects.java b/java/com/google/gerrit/server/restapi/project/ListProjects.java
index 8c1b0b3..1fab368 100644
--- a/java/com/google/gerrit/server/restapi/project/ListProjects.java
+++ b/java/com/google/gerrit/server/restapi/project/ListProjects.java
@@ -42,6 +42,7 @@
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.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 @@
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 @@
protected ListProjects(
CurrentUser currentUser,
ProjectCache projectCache,
- GroupsCollection groupsCollection,
+ GroupResolver groupResolver,
GroupControl.Factory groupControlFactory,
GitRepositoryManager repoManager,
PermissionBackend permissionBackend,
@@ -270,7 +270,7 @@
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 @@
if (groupUuid != null
&& !e.getLocalGroups()
- .contains(GroupReference.forGroup(groupsCollection.parseId(groupUuid.get())))) {
+ .contains(GroupReference.forGroup(groupResolver.parseId(groupUuid.get())))) {
continue;
}
diff --git a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
index c5c8cf0..c8857a2 100644
--- a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
+++ b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java
@@ -32,11 +32,11 @@
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 @@
@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 @@
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");
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 8891dee..488eb9f 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/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.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 @@
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 @@
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();
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
index 06b67d8..257c88b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
@@ -27,6 +27,7 @@
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 @@
}
}
+ @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();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 2259999..d1f4d84 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -39,6 +39,7 @@
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.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 @@
String largeGroup = createGroup("largeGroup");
String mediumGroup = createGroup("mediumGroup");
- int largeGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS + 1;
- int mediumGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
+ int largeGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS + 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 @@
@Test
public void reviewAndAddGroupReviewers() throws Exception {
- int largeGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS + 1;
- int mediumGroupSize = PostReviewers.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1;
+ int largeGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS + 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 @@
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();
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index 9645a94..2daccc0 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -566,15 +566,64 @@
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 @@
}
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;
}