diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 4c209e2c5d..ee34d23f8d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 4e6bb5ba18..40d900a599 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/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 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); } @@ -1096,7 +1101,8 @@ public class ChangeJson { return result; } - private Collection removableReviewers(ChangeControl ctl, ChangeInfo out) { + private Collection removableReviewers(ChangeControl ctl, ChangeInfo out) + throws PermissionBackendException, NoSuchChangeException { // Although this is called removableReviewers, this method also determines // which CCs are removable. // @@ -1116,7 +1122,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); @@ -1133,7 +1141,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); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java index df4b43554a..933705a20f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewerOp.java +++ b/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 del = new ArrayList<>(); boolean votesRemoved = false; for (PatchSetApproval a : approvals(ctx, reviewerId)) { - if (ctx.getControl().canRemoveReviewer(a)) { - del.add(a); - if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) { - oldApprovals.put(a.getLabel(), a.getValue()); - removedVotesMsg - .append("* ") - .append(a.getLabel()) - .append(formatLabelValue(a.getValue())) - .append(" by ") - .append(userFactory.create(a.getAccountId()).getNameEmail()) - .append("\n"); - votesRemoved = true; - } - } else { - throw new AuthException("delete reviewer not permitted"); + removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a); + del.add(a); + if (a.getPatchSetId().equals(currPs.getId()) && a.getValue() != 0) { + oldApprovals.put(a.getLabel(), a.getValue()); + removedVotesMsg + .append("* ") + .append(a.getLabel()) + .append(formatLabelValue(a.getValue())) + .append(" by ") + .append(userFactory.create(a.getAccountId()).getNameEmail()) + .append("\n"); + votesRemoved = true; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java index c31f72d39a..7029101bb5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java +++ b/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 dbProvider; + private final ChangeControl.GenericFactory changeControlFactory; + + @Inject + RemoveReviewerControl( + PermissionBackend permissionBackend, + Provider 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; + } +}