PutAssignee: Fail with 400 Bad Request if in input no assignee is set

So far SetAssigneeOp accepted null as assignee if there was no old
assignee, but then trying to add the null assignee as reviewer failed
with a NullPointerException in PostReviewers#prepareApplication(...).

Deleting of assignees should be done via the DeleteAssignee REST
endpoint.

Move the permission check out of SetAssigneeOp, because we always do
this first before checking for bad requests (e.g. see SetHead).

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

gerrit-server/src/main/java/com/google/gerrit/server/change/PutAssignee.java

@@ -14,12 +14,15 @@
 
 package com.google.gerrit.server.change;
 
+import com.google.common.base.Strings;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.AddReviewerInput;
 import com.google.gerrit.extensions.api.changes.AssigneeInput;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 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.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -59,10 +62,17 @@ public class PutAssignee implements
   @Override
   public Response<AccountInfo> apply(ChangeResource rsrc, AssigneeInput input)
       throws RestApiException, UpdateException, OrmException, IOException {
+    if (!rsrc.getControl().canEditAssignee()) {
+      throw new AuthException("Changing Assignee not permitted");
+    }
+    if (Strings.isNullOrEmpty(input.assignee)) {
+      throw new BadRequestException("missing assignee field");
+    }
+
     try (BatchUpdate bu = batchUpdateFactory.create(db.get(),
         rsrc.getChange().getProject(), rsrc.getControl().getUser(),
         TimeUtil.nowTs())) {
-      SetAssigneeOp op = assigneeFactory.create(input);
+      SetAssigneeOp op = assigneeFactory.create(input.assignee);
       bu.addOp(rsrc.getId(), op);
 
       PostReviewers.Addition reviewersAddition =

gerrit-server/src/main/java/com/google/gerrit/server/change/SetAssigneeOp.java

@@ -14,8 +14,9 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.base.Optional;
-import com.google.gerrit.extensions.api.changes.AssigneeInput;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -41,14 +42,14 @@ import com.google.inject.assistedinject.AssistedInject;
 
 public class SetAssigneeOp extends BatchUpdate.Op {
   public interface Factory {
-    SetAssigneeOp create(AssigneeInput input);
+    SetAssigneeOp create(String assignee);
   }
 
   private final AccountsCollection accounts;
   private final ChangeMessagesUtil cmUtil;
   private final AccountInfoCacheFactory.Factory accountInfosFactory;
   private final DynamicSet<AssigneeValidationListener> validationListeners;
-  private final AssigneeInput input;
+  private final String assignee;
   private final String anonymousCowardName;
   private final AssigneeChanged assigneeChanged;
 
@@ -63,37 +64,28 @@ public class SetAssigneeOp extends BatchUpdate.Op {
       DynamicSet<AssigneeValidationListener> validationListeners,
       AssigneeChanged assigneeChanged,
       @AnonymousCowardName String anonymousCowardName,
-      @Assisted AssigneeInput input) {
+      @Assisted String assignee) {
     this.accounts = accounts;
     this.cmUtil = cmUtil;
     this.accountInfosFactory = accountInfosFactory;
     this.validationListeners = validationListeners;
     this.assigneeChanged = assigneeChanged;
     this.anonymousCowardName = anonymousCowardName;
-    this.input = input;
+    this.assignee = checkNotNull(assignee);
   }
 
   @Override
   public boolean updateChange(BatchUpdate.ChangeContext ctx)
       throws OrmException, RestApiException {
-    if (!ctx.getControl().canEditAssignee()) {
-      throw new AuthException("Changing Assignee not permitted");
-    }
     change = ctx.getChange();
     ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
     Optional<Account.Id> oldAssigneeId =
         Optional.fromNullable(change.getAssignee());
-    if (input.assignee == null) {
-      if (oldAssigneeId.isPresent()) {
-        throw new BadRequestException("Cannot set Assignee to empty");
-      }
-      return false;
-    }
     oldAssignee = null;
     if (oldAssigneeId.isPresent()) {
       oldAssignee = accountInfosFactory.create().get(oldAssigneeId.get());
     }
-    IdentifiedUser newAssigneeUser = accounts.parse(input.assignee);
+    IdentifiedUser newAssigneeUser = accounts.parse(assignee);
     if (oldAssigneeId.isPresent() &&
         oldAssigneeId.get().equals(newAssigneeUser.getAccountId())) {
       newAssignee = oldAssignee;
@@ -101,13 +93,13 @@ public class SetAssigneeOp extends BatchUpdate.Op {
     }
     if (!newAssigneeUser.getAccount().isActive()) {
       throw new UnprocessableEntityException(String.format(
-          "Account of %s is not active", input.assignee));
+          "Account of %s is not active", assignee));
     }
     if (!ctx.getControl().forUser(newAssigneeUser).isRefVisible()) {
       throw new AuthException(String.format(
           "Change %s is not visible to %s.",
           change.getChangeId(),
-          input.assignee));
+          assignee));
     }
     try {
       for (AssigneeValidationListener validator : validationListeners) {