opendev/gerrit
Code Issues Proposed changes

Use AccountResolver2 to resolve strings to IdentifiedUsers

Browse Source 
In many callers, this allows us to remove special error handling code:
problems will propagate as UnprocessableEntityExceptions with helpful
messages. The main exception is in AccountsCollection, which is used to
parse users out of URLs (as opposed to, say, JSON inputs). In this case
the correct behavior is to wrap the exception in
ResourceNotFoundException, or AuthException in the case of "self".

The one place that gets more ugly is ReviewerAdder, where we need to
propagate the exception message for a NOT_FOUND result and combine that
with the hard-coded error message when a group is missing. This produces
a somewhat unnecessarily verbose message. The idea is that this
AccountResolver series is a prerequisite for rewriting ReviewerAdder,
which will reduce this ugliness.

Change-Id: I99c03df3da853cba7958bc027d0f67a0320c9e5a
This commit is contained in:
Dave Borowitz
2019-01-23 17:02:11 -08:00
parent 5b5ee594b8
commit b920f3f407
16 changed files with 161 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
java/com/google/gerrit/server/account/AccountResolver2.java
View File

@@ -189,6 +189,17 @@ public class AccountResolver2 {
return userFactory.create(asUnique());
}
public IdentifiedUser asUniqueUserOnBehalfOf(CurrentUser caller)
throws UnresolvableAccountException {
ensureUnique();
if (isSelf()) {
// TODO(dborowitz): This preserves old behavior, but it seems wrong to discard the caller.
return self.get().asIdentifiedUser();
}
return userFactory.runAs(
null, list.get(0).getAccount().getId(), requireNonNull(caller).getRealUser());
}
@VisibleForTesting
ImmutableList<AccountState> filteredInactive() {
return filteredInactive;

8
java/com/google/gerrit/server/api/groups/GroupsImpl.java
View File

@@ -26,7 +26,7 @@ import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver2;
import com.google.gerrit.server.group.GroupResolver;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -44,7 +44,7 @@ import java.util.SortedMap;
@Singleton
class GroupsImpl implements Groups {
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final GroupsCollection groups;
private final GroupResolver groupResolver;
private final ProjectsCollection projects;
@@ -56,7 +56,7 @@ class GroupsImpl implements Groups {
@Inject
GroupsImpl(
AccountResolver accountResolver,
AccountResolver2 accountResolver,
GroupsCollection groups,
GroupResolver groupResolver,
ProjectsCollection projects,
@@ -141,7 +141,7 @@ class GroupsImpl implements Groups {
if (req.getUser() != null) {
try {
list.setUser(accountResolver.parse(req.getUser()).getAccountId());
list.setUser(accountResolver.resolve(req.getUser()).asUnique().getAccount().getId());
} catch (Exception e) {
throw asRestApiException("Error looking up user " + req.getUser(), e);
}

2
java/com/google/gerrit/server/change/ChangeMessages.java
View File

@@ -25,9 +25,7 @@ public class ChangeMessages extends TranslationBundle {
public String revertChangeDefaultMessage;
public String reviewerCantSeeChange;
public String reviewerInactive;
public String reviewerInvalid;
public String reviewerNotFoundUser;
public String reviewerNotFoundUserOrGroup;
public String groupIsNotAllowed;

56
java/com/google/gerrit/server/change/ReviewerAdder.java
View File

@@ -56,7 +56,7 @@ 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.AccountResolver2;
import com.google.gerrit.server.account.GroupMembers;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.GroupResolver;
@@ -144,7 +144,7 @@ public class ReviewerAdder {
return Optional.of(in);
}
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final PermissionBackend permissionBackend;
private final GroupResolver groupResolver;
private final GroupMembers groupMembers;
@@ -159,7 +159,7 @@ public class ReviewerAdder {
@Inject
ReviewerAdder(
AccountResolver accountResolver,
AccountResolver2 accountResolver,
PermissionBackend permissionBackend,
GroupResolver groupResolver,
GroupMembers groupMembers,
@@ -216,11 +216,10 @@ public class ReviewerAdder {
.checkedGet(notes.getProjectName())
.is(BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL);
ReviewerAddition byAccountId =
addByAccountId(input, notes, user, accountsToNotify, allowGroup, allowByEmail);
ReviewerAddition byAccountId = addByAccountId(input, notes, user, accountsToNotify);
ReviewerAddition wholeGroup = null;
if (byAccountId == null || !byAccountId.exactMatchFound) {
if (!byAccountId.exactMatchFound) {
wholeGroup =
addWholeGroup(input, notes, user, accountsToNotify, confirmed, allowGroup, allowByEmail);
if (wholeGroup != null && wholeGroup.exactMatchFound) {
@@ -228,7 +227,17 @@ public class ReviewerAdder {
}
}
if (byAccountId != null) {
if (byAccountId != null
&& wholeGroup != null
&& byAccountId.failureType == FailureType.NOT_FOUND
&& wholeGroup.failureType == FailureType.NOT_FOUND) {
return fail(
byAccountId.input,
FailureType.NOT_FOUND,
byAccountId.result.error + "\n" + wholeGroup.result.error);
}
if (byAccountId != null && byAccountId.failureType != FailureType.NOT_FOUND) {
return byAccountId;
}
if (wholeGroup != null) {
@@ -254,28 +263,20 @@ public class ReviewerAdder {
AddReviewerInput input,
ChangeNotes notes,
CurrentUser user,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
boolean allowGroup,
boolean allowByEmail)
ListMultimap<RecipientType, Account.Id> accountsToNotify)
throws OrmException, PermissionBackendException, IOException, ConfigInvalidException {
IdentifiedUser reviewerUser;
boolean exactMatchFound = false;
try {
reviewerUser = accountResolver.parse(input.reviewer);
reviewerUser = accountResolver.resolve(input.reviewer).asUniqueUser();
if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
|| input.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(
input,
FailureType.NOT_FOUND,
MessageFormat.format(ChangeMessages.get().reviewerNotFoundUser, input.reviewer));
}
return null;
} catch (UnprocessableEntityException e) {
// Caller might choose to ignore this NOT_FOUND result if they find another result e.g. by
// group, but if not, the error message will be useful.
return fail(input, FailureType.NOT_FOUND, e.getMessage());
}
if (isValidReviewer(notes.getChange().getDest(), reviewerUser.getAccount())) {
@@ -288,15 +289,6 @@ public class ReviewerAdder {
accountsToNotify,
exactMatchFound);
}
if (!reviewerUser.getAccount().isActive()) {
if (allowByEmail && input.state() == CC) {
return null;
}
return fail(
input,
FailureType.OTHER,
MessageFormat.format(ChangeMessages.get().reviewerInactive, input.reviewer));
}
return fail(
input,
FailureType.OTHER,
@@ -412,10 +404,6 @@ public class ReviewerAdder {
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

24
java/com/google/gerrit/server/restapi/account/AccountsCollection.java
View File

@@ -21,9 +21,8 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestCollection;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver2;
import com.google.gerrit.server.account.AccountResolver2.UnresolvableAccountException;
import com.google.gerrit.server.account.AccountResource;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -34,19 +33,16 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
@Singleton
public class AccountsCollection implements RestCollection<TopLevelResource, AccountResource> {
private final AccountResolver accountResolver;
private final AccountControl.Factory accountControlFactory;
private final AccountResolver2 accountResolver;
private final Provider<QueryAccounts> list;
private final DynamicMap<RestView<AccountResource>> views;
@Inject
public AccountsCollection(
AccountResolver accountResolver,
AccountControl.Factory accountControlFactory,
AccountResolver2 accountResolver,
Provider<QueryAccounts> list,
DynamicMap<RestView<AccountResource>> views) {
this.accountResolver = accountResolver;
this.accountControlFactory = accountControlFactory;
this.list = list;
this.views = views;
}
@@ -55,12 +51,14 @@ public class AccountsCollection implements RestCollection<TopLevelResource, Acco
public AccountResource parse(TopLevelResource root, IdString id)
throws ResourceNotFoundException, AuthException, OrmException, IOException,
ConfigInvalidException {
IdentifiedUser user = accountResolver.parseId(id.get());
if (user == null || !accountControlFactory.get().canSee(user.getAccount().getId())) {
throw new ResourceNotFoundException(
String.format("Account '%s' is not found or ambiguous", id));
try {
return new AccountResource(accountResolver.resolve(id.get()).asUniqueUser());
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new AuthException(e.getMessage(), e);
}
throw new ResourceNotFoundException(e.getMessage(), e);
}
return new AccountResource(user);
}
@Override

8
java/com/google/gerrit/server/restapi/change/PostReview.java
View File

@@ -86,7 +86,7 @@ import com.google.gerrit.server.IdentifiedUser;
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.account.AccountResolver2;
import com.google.gerrit.server.change.AddReviewersEmail;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.EmailReviewComments;
@@ -164,7 +164,7 @@ public class PostReview
private final PublishCommentUtil publishCommentUtil;
private final PatchSetUtil psUtil;
private final PatchListCache patchListCache;
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final EmailReviewComments.Factory email;
private final CommentAdded commentAdded;
private final ReviewerAdder reviewerAdder;
@@ -187,7 +187,7 @@ public class PostReview
PublishCommentUtil publishCommentUtil,
PatchSetUtil psUtil,
PatchListCache patchListCache,
AccountResolver accountResolver,
AccountResolver2 accountResolver,
EmailReviewComments.Factory email,
CommentAdded commentAdded,
ReviewerAdder reviewerAdder,
@@ -491,7 +491,7 @@ public class PostReview
String.format("label required to post review on behalf of \"%s\"", in.onBehalfOf));
}
IdentifiedUser reviewer = accountResolver.parseOnBehalfOf(caller, in.onBehalfOf);
IdentifiedUser reviewer = accountResolver.resolve(in.onBehalfOf).asUniqueUserOnBehalfOf(caller);
try {
permissionBackend.user(reviewer).change(rev.getNotes()).check(ChangePermission.READ);
} catch (AuthException e) {

12
java/com/google/gerrit/server/restapi/change/PutAssignee.java
View File

@@ -23,11 +23,10 @@ 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.extensions.webui.UiAction;
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.AccountResolver2;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ReviewerAdder;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
@@ -50,7 +49,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
public class PutAssignee extends RetryingRestModifyView<ChangeResource, AssigneeInput, AccountInfo>
implements UiAction<ChangeResource> {
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final SetAssigneeOp.Factory assigneeFactory;
private final ReviewerAdder reviewerAdder;
private final AccountLoader.Factory accountLoaderFactory;
@@ -58,7 +57,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
@Inject
PutAssignee(
AccountResolver accountResolver,
AccountResolver2 accountResolver,
SetAssigneeOp.Factory assigneeFactory,
RetryHelper retryHelper,
ReviewerAdder reviewerAdder,
@@ -84,10 +83,7 @@ public class PutAssignee extends RetryingRestModifyView<ChangeResource, Assignee
throw new BadRequestException("missing assignee field");
}
IdentifiedUser assignee = accountResolver.parse(input.assignee);
if (!assignee.getAccount().isActive()) {
throw new UnprocessableEntityException(input.assignee + " is not active");
}
IdentifiedUser assignee = accountResolver.resolve(input.assignee).asUniqueUser();
try {
permissionBackend
.absentUser(assignee.getAccountId())

9
java/com/google/gerrit/server/restapi/change/Submit.java
View File

@@ -41,7 +41,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver2;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
@@ -112,7 +112,7 @@ public class Submit
private final ChangeNotes.Factory changeNotesFactory;
private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeSuperSet> mergeSuperSet;
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final String label;
private final String labelWithParents;
private final ParameterizedString titlePattern;
@@ -132,7 +132,7 @@ public class Submit
ChangeNotes.Factory changeNotesFactory,
Provider<MergeOp> mergeOpProvider,
Provider<MergeSuperSet> mergeSuperSet,
AccountResolver accountResolver,
AccountResolver2 accountResolver,
@GerritServerConfig Config cfg,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil,
@@ -463,7 +463,8 @@ public class Submit
perm.check(ChangePermission.SUBMIT_AS);
CurrentUser caller = rsrc.getUser();
IdentifiedUser submitter = accountResolver.parseOnBehalfOf(caller, in.onBehalfOf);
IdentifiedUser submitter =
accountResolver.resolve(in.onBehalfOf).asUniqueUserOnBehalfOf(caller);
try {
permissionBackend.user(submitter).change(rsrc.getNotes()).check(ChangePermission.READ);
} catch (AuthException e) {

22
java/com/google/gerrit/server/restapi/group/AddMembers.java
View File

@@ -36,7 +36,8 @@ import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountException;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver2;
import com.google.gerrit.server.account.AccountResolver2.UnresolvableAccountException;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.GroupControl;
@@ -89,7 +90,7 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
private final AccountManager accountManager;
private final AuthType authType;
private final AccountResolver accountResolver;
private final AccountResolver2 accountResolver;
private final AccountCache accountCache;
private final AccountLoader.Factory infoFactory;
private final Provider<GroupsUpdate> groupsUpdateProvider;
@@ -98,7 +99,7 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
AddMembers(
AccountManager accountManager,
AuthConfig authConfig,
AccountResolver accountResolver,
AccountResolver2 accountResolver,
AccountCache accountCache,
AccountLoader.Factory infoFactory,
@UserInitiated Provider<GroupsUpdate> groupsUpdateProvider) {
@@ -144,19 +145,18 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
}
Account findAccount(String nameOrEmailOrId)
throws AuthException, UnprocessableEntityException, OrmException, IOException,
ConfigInvalidException {
throws UnprocessableEntityException, OrmException, IOException, ConfigInvalidException {
AccountResolver2.Result result = accountResolver.resolve(nameOrEmailOrId);
try {
return accountResolver.parse(nameOrEmailOrId).getAccount();
} catch (UnprocessableEntityException e) {
// might be because the account does not exist or because the account is
// not visible
return result.asUnique().getAccount();
} catch (UnresolvableAccountException e) {
switch (authType) {
case HTTP_LDAP:
case CLIENT_SSL_CERT_LDAP:
case LDAP:
if (accountResolver.find(nameOrEmailOrId) == null) {
// account does not exist, try to create it
if (!e.isSelf() && result.asList().isEmpty()) {
// Account does not exist, try to create it. This may leak account existence, since we
// can't distinguish between a nonexistent account and one that the caller can't see.
Optional<Account> a = createAccountByLdap(nameOrEmailOrId);
if (a.isPresent()) {
return a.get();

10
java/com/google/gerrit/server/restapi/group/DeleteMembers.java
View File

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

28
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -516,12 +516,27 @@ public class AccountIT extends AbstractDaemonTest {
@Test
public void active() throws Exception {
int id = gApi.accounts().id("user").get()._accountId;
assertThat(gApi.accounts().id("user").getActive()).isTrue();
gApi.accounts().id("user").setActive(false);
assertThat(gApi.accounts().id("user").getActive()).isFalse();
accountIndexedCounter.assertReindexOf(user);
gApi.accounts().id("user").setActive(true);
// Inactive users may only be resolved by ID.
try {
gApi.accounts().id("user");
assert_().fail("expected ResourceNotFoundException");
} catch (ResourceNotFoundException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"Account 'user' only matches inactive accounts. To use an inactive account, retry"
+ " with one of the following exact account IDs:\n"
+ id
+ ": User <user@example.com>");
}
assertThat(gApi.accounts().id(id).getActive()).isFalse();
gApi.accounts().id(id).setActive(true);
assertThat(gApi.accounts().id("user").getActive()).isTrue();
accountIndexedCounter.assertReindexOf(user);
}
@@ -620,16 +635,17 @@ public class AccountIT extends AbstractDaemonTest {
@Test
public void deactivateNotActive() throws Exception {
int id = gApi.accounts().id("user").get()._accountId;
assertThat(gApi.accounts().id("user").getActive()).isTrue();
gApi.accounts().id("user").setActive(false);
assertThat(gApi.accounts().id("user").getActive()).isFalse();
assertThat(gApi.accounts().id(id).getActive()).isFalse();
try {
gApi.accounts().id("user").setActive(false);
gApi.accounts().id(id).setActive(false);
fail("Expected exception");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("account not active");
}
gApi.accounts().id("user").setActive(true);
gApi.accounts().id(id).setActive(true);
}
@Test
@@ -2121,7 +2137,7 @@ public class AccountIT extends AbstractDaemonTest {
TestAccount foo2 = accountCreator.create(name + "-2");
gApi.accounts().id(foo2.username).setActive(false);
assertThat(gApi.accounts().id(foo2.username).getActive()).isFalse();
assertThat(gApi.accounts().id(foo2.id.get()).getActive()).isFalse();
assertThat(accountQueryProvider.get().byDefault(name)).hasSize(2);
}

37
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -86,6 +86,7 @@ import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.changes.StarsInput;
import com.google.gerrit.extensions.api.groups.GroupApi;
@@ -1676,17 +1677,47 @@ public class ChangeIT extends AbstractDaemonTest {
PushOneCommit.Result result = createChange();
String username = name("new-user");
accountOperations.newAccount().username(username).inactive().create();
Account.Id id = accountOperations.newAccount().username(username).inactive().create();
AddReviewerInput in = new AddReviewerInput();
in.reviewer = username;
AddReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in);
assertThat(r.input).isEqualTo(username);
assertThat(r.error).contains("identifies an inactive account");
assertThat(r.input).isEqualTo(in.reviewer);
assertThat(r.error)
.isEqualTo(
"Account '"
+ username
+ "' only matches inactive accounts. To use an inactive account, retry with one of"
+ " the following exact account IDs:\n"
+ id
+ ": Name of user not set ("
+ id
+ ")\n"
+ username
+ " does not identify a registered user or group");
assertThat(r.reviewers).isNull();
}
@Test
public void addReviewerThatIsInactiveById() throws Exception {
PushOneCommit.Result result = createChange();
String username = name("new-user");
Account.Id id = accountOperations.newAccount().username(username).inactive().create();
AddReviewerInput in = new AddReviewerInput();
in.reviewer = Integer.toString(id.get());
AddReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in);
assertThat(r.input).isEqualTo(in.reviewer);
assertThat(r.error).isNull();
assertThat(r.reviewers).hasSize(1);
ReviewerInfo reviewer = r.reviewers.get(0);
assertThat(reviewer._accountId).isEqualTo(id.get());
assertThat(reviewer.username).isEqualTo(username);
}
@Test
public void addReviewerThatIsInactiveEmailFallback() throws Exception {
ConfigInput conf = new ConfigInput();

28
javatests/com/google/gerrit/acceptance/rest/change/AssigneeIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -29,8 +30,8 @@ import com.google.gerrit.extensions.api.changes.AssigneeInput;
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.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.AccountResolver2.UnresolvableAccountException;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gerrit.testing.TestTimeUtil;
import com.google.inject.Inject;
@@ -129,9 +130,28 @@ public class AssigneeIT extends AbstractDaemonTest {
public void setAssigneeToInactiveUser() throws Exception {
PushOneCommit.Result r = createChange();
gApi.accounts().id(user.getId().get()).setActive(false);
exception.expect(UnprocessableEntityException.class);
exception.expectMessage("is not active");
setAssignee(r, user.email);
try {
setAssignee(r, user.email);
assert_().fail("expected UnresolvableAccountException");
} catch (UnresolvableAccountException e) {
assertThat(e)
.hasMessageThat()
.isEqualTo(
"Account '"
+ user.email
+ "' only matches inactive accounts. To use an inactive account, retry with one"
+ " of the following exact account IDs:\n"
+ user.id
+ ": User <user@example.com>");
}
}
@Test
public void setAssigneeToInactiveUserById() throws Exception {
PushOneCommit.Result r = createChange();
gApi.accounts().id(user.getId().get()).setActive(false);
setAssignee(r, user.getId().toString());
assertThat(getAssignee(r)._accountId).isEqualTo(user.getId().get());
}
@Test

4
javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
View File

@@ -306,7 +306,9 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
gApi.changes().id(r.getChangeId()).addReviewer("Foo Bar <foo.bar@gerritcodereview.com>");
assertThat(result.error)
.isEqualTo(
"Foo Bar <foo.bar@gerritcodereview.com> does not identify a registered user or group");
"Account 'Foo Bar <foo.bar@gerritcodereview.com>' not found\n"
+ "Foo Bar <foo.bar@gerritcodereview.com> does not identify a registered user or"
+ " group");
assertThat(result.reviewers).isNull();
}

2
javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
View File

@@ -470,7 +470,7 @@ public class SuggestReviewersIT extends AbstractDaemonTest {
suggestReviewers(changeId, name), ImmutableList.of(foo1, foo2), ImmutableList.of());
gApi.accounts().id(foo2.username).setActive(false);
assertThat(gApi.accounts().id(foo2.username).getActive()).isFalse();
assertThat(gApi.accounts().id(foo2.id.get()).getActive()).isFalse();
assertReviewers(suggestReviewers(changeId, name), ImmutableList.of(foo1), ImmutableList.of());
}

2
resources/com/google/gerrit/server/change/ChangeMessages.properties
View File

@@ -1,9 +1,7 @@
revertChangeDefaultMessage = Revert \"{0}\"\n\nThis reverts commit {1}.
reviewerCantSeeChange = {0} does not have permission to see this change
reviewerInactive = {0} identifies an inactive account
reviewerInvalid = {0} is not a valid user identifier
reviewerNotFoundUser = {0} does not identify a registered user
reviewerNotFoundUserOrGroup = {0} does not identify a registered user or group
groupIsNotAllowed = The group {0} cannot be added as reviewer.