Merge "Factor ChangeControl#canRemoveReviewer out"

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -1797,7 +1797,7 @@ public class ChangeIT extends AbstractDaemonTest {
 
     setApiUser(user);
     exception.expect(AuthException.class);
-    exception.expectMessage("delete reviewer not permitted");
+    exception.expectMessage("remove reviewer not permitted");
     gApi.changes().id(r.getChangeId()).reviewer(admin.getId().toString()).remove();
   }
 

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

@@ -116,7 +116,9 @@ import com.google.gerrit.server.permissions.LabelPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.RemoveReviewerControl;
 import com.google.gerrit.server.project.SubmitRuleOptions;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
@@ -212,6 +214,7 @@ public class ChangeJson {
   private final ChangeKindCache changeKindCache;
   private final ChangeIndexCollection indexes;
   private final ApprovalsUtil approvalsUtil;
+  private final RemoveReviewerControl removeReviewerControl;
 
   private boolean lazyLoad = true;
   private AccountLoader accountLoader;
@@ -243,6 +246,7 @@ public class ChangeJson {
       ChangeKindCache changeKindCache,
       ChangeIndexCollection indexes,
       ApprovalsUtil approvalsUtil,
+      RemoveReviewerControl removeReviewerControl,
       @Assisted Iterable<ListChangesOption> options) {
     this.db = db;
     this.userProvider = user;
@@ -267,6 +271,7 @@ public class ChangeJson {
     this.changeKindCache = changeKindCache;
     this.indexes = indexes;
     this.approvalsUtil = approvalsUtil;
+    this.removeReviewerControl = removeReviewerControl;
     this.options = Sets.immutableEnumSet(options);
   }
 
@@ -1100,7 +1105,8 @@ public class ChangeJson {
     return result;
   }
 
-  private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out) {
+  private Collection<AccountInfo> removableReviewers(ChangeControl ctl, ChangeInfo out)
+      throws PermissionBackendException, NoSuchChangeException {
     // Although this is called removableReviewers, this method also determines
     // which CCs are removable.
     //
@@ -1120,7 +1126,9 @@ public class ChangeJson {
       }
       for (ApprovalInfo ai : label.all) {
         Account.Id id = new Account.Id(ai._accountId);
-        if (ctl.canRemoveReviewer(id, MoreObjects.firstNonNull(ai.value, 0))) {
+
+        if (removeReviewerControl.testRemoveReviewer(
+            ctl.getNotes(), ctl.getUser(), id, MoreObjects.firstNonNull(ai.value, 0))) {
           removable.add(id);
         } else {
           fixed.add(id);
@@ -1137,7 +1145,7 @@ public class ChangeJson {
       for (AccountInfo ai : ccs) {
         if (ai._accountId != null) {
           Account.Id id = new Account.Id(ai._accountId);
-          if (ctl.canRemoveReviewer(id, 0)) {
+          if (removeReviewerControl.testRemoveReviewer(ctl.getNotes(), ctl.getUser(), id, 0)) {
             removable.add(id);
           }
         }

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

@@ -38,6 +38,8 @@ import com.google.gerrit.server.mail.send.DeleteReviewerSender;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.project.RemoveReviewerControl;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.BatchUpdateReviewDb;
 import com.google.gerrit.server.update.ChangeContext;
@@ -70,6 +72,7 @@ public class DeleteReviewerOp implements BatchUpdateOp {
   private final DeleteReviewerSender.Factory deleteReviewerSenderFactory;
   private final NotesMigration migration;
   private final NotifyUtil notifyUtil;
+  private final RemoveReviewerControl removeReviewerControl;
 
   private final Account reviewer;
   private final DeleteReviewerInput input;
@@ -91,6 +94,7 @@ public class DeleteReviewerOp implements BatchUpdateOp {
       DeleteReviewerSender.Factory deleteReviewerSenderFactory,
       NotesMigration migration,
       NotifyUtil notifyUtil,
+      RemoveReviewerControl removeReviewerControl,
       @Assisted Account reviewerAccount,
       @Assisted DeleteReviewerInput input) {
     this.approvalsUtil = approvalsUtil;
@@ -102,6 +106,7 @@ public class DeleteReviewerOp implements BatchUpdateOp {
     this.deleteReviewerSenderFactory = deleteReviewerSenderFactory;
     this.migration = migration;
     this.notifyUtil = notifyUtil;
+    this.removeReviewerControl = removeReviewerControl;
 
     this.reviewer = reviewerAccount;
     this.input = input;
@@ -109,7 +114,7 @@ public class DeleteReviewerOp implements BatchUpdateOp {
 
   @Override
   public boolean updateChange(ChangeContext ctx)
-      throws AuthException, ResourceNotFoundException, OrmException {
+      throws AuthException, ResourceNotFoundException, OrmException, PermissionBackendException {
     Account.Id reviewerId = reviewer.getId();
     if (!approvalsUtil.getReviewers(ctx.getDb(), ctx.getNotes()).all().contains(reviewerId)) {
       throw new ResourceNotFoundException();
@@ -130,21 +135,18 @@ public class DeleteReviewerOp implements BatchUpdateOp {
     List<PatchSetApproval> del = new ArrayList<>();
     boolean votesRemoved = false;
     for (PatchSetApproval a : approvals(ctx, reviewerId)) {
-      if (ctx.getControl().canRemoveReviewer(a)) {
+      removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
-        del.add(a);
+      del.add(a);
-        if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
+      if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) {
-          oldApprovals.put(a.getLabel(), a.getValue());
+        oldApprovals.put(a.getLabel(), a.getValue());
-          removedVotesMsg
+        removedVotesMsg
-              .append("* ")
+            .append("* ")
-              .append(a.getLabel())
+            .append(a.getLabel())
-              .append(formatLabelValue(a.getValue()))
+            .append(formatLabelValue(a.getValue()))
-              .append(" by ")
+            .append(" by ")
-              .append(userFactory.create(a.getAccountId()).getNameEmail())
+            .append(userFactory.create(a.getAccountId()).getNameEmail())
-              .append("\n");
+            .append("\n");
-          votesRemoved = true;
+        votesRemoved = true;
-        }
-      } else {
-        throw new AuthException("delete reviewer not permitted");
       }
     }
 

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

@@ -40,7 +40,9 @@ import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.extensions.events.VoteDeleted;
 import com.google.gerrit.server.mail.send.DeleteVoteSender;
 import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.RemoveReviewerControl;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
@@ -72,6 +74,7 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
   private final VoteDeleted voteDeleted;
   private final DeleteVoteSender.Factory deleteVoteSenderFactory;
   private final NotifyUtil notifyUtil;
+  private final RemoveReviewerControl removeReviewerControl;
 
   @Inject
   DeleteVote(
@@ -83,7 +86,8 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
       IdentifiedUser.GenericFactory userFactory,
       VoteDeleted voteDeleted,
       DeleteVoteSender.Factory deleteVoteSenderFactory,
-      NotifyUtil notifyUtil) {
+      NotifyUtil notifyUtil,
+      RemoveReviewerControl removeReviewerControl) {
     super(retryHelper);
     this.db = db;
     this.approvalsUtil = approvalsUtil;
@@ -93,6 +97,7 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
     this.voteDeleted = voteDeleted;
     this.deleteVoteSenderFactory = deleteVoteSenderFactory;
     this.notifyUtil = notifyUtil;
+    this.removeReviewerControl = removeReviewerControl;
   }
 
   @Override
@@ -143,7 +148,8 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
 
     @Override
     public boolean updateChange(ChangeContext ctx)
-        throws OrmException, AuthException, ResourceNotFoundException, IOException {
+        throws OrmException, AuthException, ResourceNotFoundException, IOException,
+            PermissionBackendException {
       ChangeControl ctl = ctx.getControl();
       change = ctl.getChange();
       PatchSet.Id psId = change.currentPatchSetId();
@@ -166,8 +172,12 @@ public class DeleteVote extends RetryingRestModifyView<VoteResource, DeleteVoteI
           // Populate map for non-matching labels, needed by VoteDeleted.
           newApprovals.put(a.getLabel(), a.getValue());
           continue;
-        } else if (!ctl.canRemoveReviewer(a)) {
+        } else {
-          throw new AuthException("delete vote not permitted");
+          try {
+            removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a);
+          } catch (AuthException e) {
+            throw new AuthException("delete vote not permitted", e);
+          }
         }
         // Set the approval to 0 if vote is being removed.
         newApprovals.put(a.getLabel(), (short) 0);

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -331,7 +331,7 @@ public class ChangeControl {
   }
 
   /** Is this user the owner of the change? */
-  private boolean isOwner() {
+  boolean isOwner() {
     if (getUser().isIdentifiedUser()) {
       Account.Id id = getUser().asIdentifiedUser().getAccountId();
       return id.equals(getChange().getOwner());
@@ -358,40 +358,6 @@ public class ChangeControl {
     return false;
   }
 
-  /** @return true if the user is allowed to remove this reviewer. */
-  public boolean canRemoveReviewer(PatchSetApproval approval) {
-    return canRemoveReviewer(approval.getAccountId(), approval.getValue());
-  }
-
-  public boolean canRemoveReviewer(Account.Id reviewer, int value) {
-    if (getChange().getStatus().isOpen()) {
-      // A user can always remove themselves.
-      //
-      if (getUser().isIdentifiedUser()) {
-        if (getUser().getAccountId().equals(reviewer)) {
-          return true; // can remove self
-        }
-      }
-
-      // The change owner may remove any zero or positive score.
-      //
-      if (isOwner() && 0 <= value) {
-        return true;
-      }
-
-      // Users with the remove reviewer permission, the branch owner, project
-      // owner and site admin can remove anyone
-      if (getRefControl().canRemoveReviewer() // has removal permissions
-          || getRefControl().isOwner() // branch owner
-          || getProjectControl().isOwner() // project owner
-          || getProjectControl().isAdmin()) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
   /** Can this user edit the topic name? */
   private boolean canEditTopicName() {
     if (getChange().getStatus().isOpen()) {
@@ -553,7 +519,7 @@ public class ChangeControl {
           case SUBMIT:
             return getRefControl().canSubmit(isOwner());
 
-          case REMOVE_REVIEWER: // TODO Honor specific removal filters?
+          case REMOVE_REVIEWER:
           case SUBMIT_AS:
             return getRefControl().canPerform(perm.permissionName().get());
         }

gerrit-server/src/main/java/com/google/gerrit/server/project/RemoveReviewerControl.java

@@ -0,0 +1,102 @@
+// 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.project;
+
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+@Singleton
+public class RemoveReviewerControl {
+  private final PermissionBackend permissionBackend;
+  private final Provider<ReviewDb> dbProvider;
+  private final ChangeControl.GenericFactory changeControlFactory;
+
+  @Inject
+  RemoveReviewerControl(
+      PermissionBackend permissionBackend,
+      Provider<ReviewDb> dbProvider,
+      ChangeControl.GenericFactory changeControlFactory) {
+    this.permissionBackend = permissionBackend;
+    this.dbProvider = dbProvider;
+    this.changeControlFactory = changeControlFactory;
+  }
+
+  /** @throws AuthException if this user is not allowed to remove this approval. */
+  public void checkRemoveReviewer(
+      ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval)
+      throws PermissionBackendException, AuthException, NoSuchChangeException {
+    if (canRemoveReviewerWithoutPermissionCheck(
+        notes, currentUser, approval.getAccountId(), approval.getValue())) {
+      return;
+    }
+
+    permissionBackend
+        .user(currentUser)
+        .change(notes)
+        .database(dbProvider)
+        .check(ChangePermission.REMOVE_REVIEWER);
+  }
+
+  /** @return true if the user is allowed to remove this reviewer. */
+  public boolean testRemoveReviewer(
+      ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+      throws PermissionBackendException, NoSuchChangeException {
+    if (canRemoveReviewerWithoutPermissionCheck(notes, currentUser, reviewer, value)) {
+      return true;
+    }
+    return permissionBackend
+        .user(currentUser)
+        .change(notes)
+        .database(dbProvider)
+        .test(ChangePermission.REMOVE_REVIEWER);
+  }
+
+  private boolean canRemoveReviewerWithoutPermissionCheck(
+      ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value)
+      throws NoSuchChangeException {
+    ChangeControl changeControl = changeControlFactory.controlFor(notes, currentUser);
+    if (!changeControl.getChange().getStatus().isOpen()) {
+      return false;
+    }
+    // A user can always remove themselves.
+    if (changeControl.getUser().isIdentifiedUser()) {
+      if (changeControl.getUser().getAccountId().equals(reviewer)) {
+        return true; // can remove self
+      }
+    }
+    // The change owner may remove any zero or positive score.
+    if (changeControl.isOwner() && 0 <= value) {
+      return true;
+    }
+    // Users with the remove reviewer permission, the branch owner, project
+    // owner and site admin can remove anyone
+    if (changeControl.getRefControl().isOwner() // branch owner
+        || changeControl.getProjectControl().isOwner() // project owner
+        || changeControl.getProjectControl().isAdmin()) { // project admin
+      return true;
+    }
+    return false;
+  }
+}