Revert "Revert "Add project permission for removing votes/labels."" This reverts commit 7b91e4e06fd1ff5bdfcebb4561fcee32cf48caf2. Reason for revert: Re-adding the feature with the needed fixes. Issues with the original change: 1. The added dependency on ChangeData::notes() while checking for permissions broke the query flow. 2. For backward compatibility, removing label should be allowed with either remove-reviewer or remove-vote permissions. 3. ChangeInfoDiffer.getAddedForMap failed when the old map was null. Bug: Google b/245706858 Bug: Google b/265447204 Change-Id: I5c74804d1dbb51cf6d97de653e47654acd2bf263 Forward-Compatible: checked Release-Notes: New project permission for removing votes/labels
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index f2f5a66..9a40b27 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt
@@ -800,6 +800,22 @@ Users without this access right who are able to upload changes can still do the revert locally and upload the revert commit as a new change. +[[category_remove_label]] +=== Remove Label (Remove Vote) + +For every configured label `My-Name` in the project, there is a +corresponding permission `removeLabel-My-Name` with a range corresponding to +the defined values. For these values, the users are permitted to remove +other users' votes from a change. + +Change owners can always remove zero or positive votes (even without +having the `Remove Vote` access right assigned). + +Project owners and site administrators can always remove any vote (even +without having the `Remove Vote` access right assigned). + +Users without this access right can still remove their own votes. + [[category_remove_reviewer]] === Remove Reviewer
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 359361c..2f144c6 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -824,6 +824,26 @@ "+2" ] }, + "removable_labels": { + "Code-Review": { + "-1": [ + { + "_account_id": 1000096, + "name": "John Doe", + "email": "john.doe@example.com", + "username": "jdoe" + } + ], + "+1": [ + { + "_account_id": 1000097, + "name": "Jane Roe", + "email": "jane.roe@example.com", + "username": "jroe" + } + ] + } + }, "removable_reviewers": [ { "_account_id": 1000096, @@ -7071,6 +7091,13 @@ A map of the permitted labels that maps a label name to the list of values that are allowed for that label. + Only set if link:#detailed-labels[detailed labels] are requested. +|`removable_labels` |optional| +A map of the removable labels that maps a label name to the map of +values and reviewers ( +link:rest-api-accounts.html#account-info[AccountInfo] entities) +that are allowed to be removed from the change. + +Only set if link:#labels[labels] or +link:#detailed-labels[detailed labels] are requested. |`removable_reviewers`|optional| The reviewers that can be removed by the calling user as a list of link:rest-api-accounts.html#account-info[AccountInfo] entities. +
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 5b4a9e5..ccf74d1 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1393,6 +1393,17 @@ } } + protected void assertOnlyRemovableLabel( + ChangeInfo info, String labelId, String labelValue, TestAccount reviewer) { + assertThat(info.removableLabels).hasSize(1); + assertThat(info.removableLabels).containsKey(labelId); + assertThat(info.removableLabels.get(labelId)).hasSize(1); + assertThat(info.removableLabels.get(labelId)).containsKey(labelValue); + assertThat(info.removableLabels.get(labelId).get(labelValue)).hasSize(1); + assertThat(info.removableLabels.get(labelId).get(labelValue).get(0).email) + .isEqualTo(reviewer.email()); + } + protected void assertPermissions( Project.NameKey project, GroupReference groupReference,
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java index b1cd506..69139ce 100644 --- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -197,8 +197,13 @@ PermissionRule.Builder rule = newRule(projectConfig, p.group()); rule.setAction(p.action()); rule.setRange(p.min(), p.max()); - String permissionName = - p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name()); + String permissionName; + if (p.isAddPermission()) { + permissionName = + p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name()); + } else { + permissionName = Permission.forRemoveLabel(p.name()); + } projectConfig.upsertAccessSection( p.ref(), as -> as.upsertPermission(permissionName).add(rule)); }
diff --git a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java index 9a9a21a..5634c78 100644 --- a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java +++ b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
@@ -162,12 +162,34 @@ /** Starts a builder for allowing a label permission. */ public static TestLabelPermission.Builder allowLabel(String name) { - return TestLabelPermission.builder().name(name).action(PermissionRule.Action.ALLOW); + return TestLabelPermission.builder() + .name(name) + .isAddPermission(true) + .action(PermissionRule.Action.ALLOW); } /** Starts a builder for denying a label permission. */ public static TestLabelPermission.Builder blockLabel(String name) { - return TestLabelPermission.builder().name(name).action(PermissionRule.Action.BLOCK); + return TestLabelPermission.builder() + .name(name) + .isAddPermission(true) + .action(PermissionRule.Action.BLOCK); + } + + /** Starts a builder for allowing a remove-label permission. */ + public static TestLabelPermission.Builder allowLabelRemoval(String name) { + return TestLabelPermission.builder() + .name(name) + .isAddPermission(false) + .action(PermissionRule.Action.ALLOW); + } + + /** Starts a builder for denying a remove-label permission. */ + public static TestLabelPermission.Builder blockLabelRemoval(String name) { + return TestLabelPermission.builder() + .name(name) + .isAddPermission(false) + .action(PermissionRule.Action.BLOCK); } /** Records a label permission to be updated. */ @@ -191,6 +213,8 @@ abstract boolean impersonation(); + abstract boolean isAddPermission(); + /** Builder for {@link TestLabelPermission}. */ @AutoValue.Builder public abstract static class Builder { @@ -208,6 +232,8 @@ abstract Builder max(int max); + abstract Builder isAddPermission(boolean isAddPermission); + /** Sets the minimum and maximum values for the permission. */ public Builder range(int min, int max) { checkArgument(min != 0 || max != 0, "empty range"); @@ -243,6 +269,12 @@ return TestPermissionKey.builder().name(Permission.forLabel(name)); } + /** Starts a builder for describing a label removal permission key for deletion. */ + public static TestPermissionKey.Builder labelRemovalPermissionKey(String name) { + checkLabelName(name); + return TestPermissionKey.builder().name(Permission.forRemoveLabel(name)); + } + /** Starts a builder for describing a capability key for deletion. */ public static TestPermissionKey.Builder capabilityKey(String name) { return TestPermissionKey.builder().name(name).section(GLOBAL_CAPABILITIES);
diff --git a/java/com/google/gerrit/entities/Permission.java b/java/com/google/gerrit/entities/Permission.java index 6d2fa32..d029fad 100644 --- a/java/com/google/gerrit/entities/Permission.java +++ b/java/com/google/gerrit/entities/Permission.java
@@ -43,6 +43,7 @@ public static final String FORGE_SERVER = "forgeServerAsCommitter"; public static final String LABEL = "label-"; public static final String LABEL_AS = "labelAs-"; + public static final String REMOVE_LABEL = "removeLabel-"; public static final String OWNER = "owner"; public static final String PUSH = "push"; public static final String PUSH_MERGE = "pushMerge"; @@ -60,6 +61,7 @@ private static final List<String> NAMES_LC; private static final int LABEL_INDEX; private static final int LABEL_AS_INDEX; + private static final int REMOVE_LABEL_INDEX; static { NAMES_LC = new ArrayList<>(); @@ -79,6 +81,7 @@ NAMES_LC.add(FORGE_SERVER.toLowerCase()); NAMES_LC.add(LABEL.toLowerCase()); NAMES_LC.add(LABEL_AS.toLowerCase()); + NAMES_LC.add(REMOVE_LABEL.toLowerCase()); NAMES_LC.add(OWNER.toLowerCase()); NAMES_LC.add(PUSH.toLowerCase()); NAMES_LC.add(PUSH_MERGE.toLowerCase()); @@ -93,15 +96,19 @@ LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL); LABEL_AS_INDEX = NAMES_LC.indexOf(Permission.LABEL_AS.toLowerCase()); + REMOVE_LABEL_INDEX = NAMES_LC.indexOf(Permission.REMOVE_LABEL.toLowerCase()); } /** Returns true if the name is recognized as a permission name. */ public static boolean isPermission(String varName) { - return isLabel(varName) || isLabelAs(varName) || NAMES_LC.contains(varName.toLowerCase()); + return isLabel(varName) + || isLabelAs(varName) + || isRemoveLabel(varName) + || NAMES_LC.contains(varName.toLowerCase()); } public static boolean hasRange(String varName) { - return isLabel(varName) || isLabelAs(varName); + return isLabel(varName) || isLabelAs(varName) || isRemoveLabel(varName); } /** Returns true if the permission name is actually for a review label. */ @@ -114,6 +121,11 @@ return var.startsWith(LABEL_AS) && LABEL_AS.length() < var.length(); } + /** Returns true if the permission is for impersonated review labels. */ + public static boolean isRemoveLabel(String var) { + return var.startsWith(REMOVE_LABEL) && REMOVE_LABEL.length() < var.length(); + } + /** Returns permission name for the given review label. */ public static String forLabel(String labelName) { return LABEL + labelName; @@ -124,12 +136,19 @@ return LABEL_AS + labelName; } + /** Returns permission name to remove a label for another user. */ + public static String forRemoveLabel(String labelName) { + return REMOVE_LABEL + labelName; + } + @Nullable public static String extractLabel(String varName) { if (isLabel(varName)) { return varName.substring(LABEL.length()); } else if (isLabelAs(varName)) { return varName.substring(LABEL_AS.length()); + } else if (isRemoveLabel(varName)) { + return varName.substring(REMOVE_LABEL.length()); } return null; } @@ -205,6 +224,8 @@ return LABEL_INDEX; } else if (isLabelAs(a.getName())) { return LABEL_AS_INDEX; + } else if (isRemoveLabel(a.getName())) { + return REMOVE_LABEL_INDEX; } int index = NAMES_LC.indexOf(a.getName().toLowerCase());
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java index 40ae2ec..a865187 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -105,6 +105,7 @@ public Map<String, ActionInfo> actions; public Map<String, LabelInfo> labels; public Map<String, Collection<String>> permittedLabels; + public Map<String, Map<String, List<AccountInfo>>> removableLabels; public Collection<AccountInfo> removableReviewers; public Map<ReviewerState, Collection<AccountInfo>> reviewers; public Map<ReviewerState, Collection<AccountInfo>> pendingReviewers;
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java index 24182cc..51c35dc 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java +++ b/java/com/google/gerrit/extensions/common/ChangeInfoDiffer.java
@@ -150,16 +150,17 @@ /** Returns {@code null} if nothing has been added to {@code oldCollection} */ @Nullable private static ImmutableList<?> getAddedForCollection( - Collection<?> oldCollection, Collection<?> newCollection) { - ImmutableList<?> notInOldCollection = getAdditions(oldCollection, newCollection); + @Nullable Collection<?> oldCollection, Collection<?> newCollection) { + ImmutableList<?> notInOldCollection = getAdditionsForCollection(oldCollection, newCollection); return notInOldCollection.isEmpty() ? null : notInOldCollection; } @Nullable - private static ImmutableList<Object> getAdditions( - Collection<?> oldCollection, Collection<?> newCollection) { - if (oldCollection == null) - return newCollection != null ? ImmutableList.copyOf(newCollection) : null; + private static ImmutableList<Object> getAdditionsForCollection( + @Nullable Collection<?> oldCollection, Collection<?> newCollection) { + if (oldCollection == null) { + return ImmutableList.copyOf(newCollection); + } Map<Object, List<Object>> duplicatesMap = newCollection.stream().collect(groupingBy(v -> v)); oldCollection.forEach( @@ -173,7 +174,18 @@ /** Returns {@code null} if nothing has been added to {@code oldMap} */ @Nullable - private static ImmutableMap<Object, Object> getAddedForMap(Map<?, ?> oldMap, Map<?, ?> newMap) { + private static ImmutableMap<Object, Object> getAddedForMap( + @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) { + ImmutableMap<Object, Object> notInOldMap = getAdditionsForMap(oldMap, newMap); + return notInOldMap.isEmpty() ? null : notInOldMap; + } + + @Nullable + private static ImmutableMap<Object, Object> getAdditionsForMap( + @Nullable Map<?, ?> oldMap, Map<?, ?> newMap) { + if (oldMap == null) { + return ImmutableMap.copyOf(newMap); + } ImmutableMap.Builder<Object, Object> additionsBuilder = ImmutableMap.builder(); for (Map.Entry<?, ?> entry : newMap.entrySet()) { Object added = getAdded(oldMap.get(entry.getKey()), entry.getValue()); @@ -181,8 +193,7 @@ additionsBuilder.put(entry.getKey(), added); } } - ImmutableMap<Object, Object> additions = additionsBuilder.build(); - return additions.isEmpty() ? null : additions; + return additionsBuilder.build(); } private static Object get(Field field, Object obj) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 500bb77..912d202 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -146,7 +146,7 @@ public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_STRICT = ChangeField.SUBMIT_RULE_OPTIONS_STRICT.toBuilder().build(); - static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD = + public static final ImmutableSet<ListChangesOption> REQUIRE_LAZY_LOAD = ImmutableSet.of( ALL_COMMITS, ALL_REVISIONS, @@ -688,6 +688,7 @@ !cd.change().isAbandoned() ? labelsJson.permittedLabels(user.getAccountId(), cd) : ImmutableMap.of(); + out.removableLabels = labelsJson.removableLabels(accountLoader, user, cd); } }
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java index cfa15ae..5555ba6 100644 --- a/java/com/google/gerrit/server/change/LabelsJson.java +++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -36,15 +36,19 @@ import com.google.gerrit.entities.LabelValue; import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.SubmitRecord; +import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ApprovalInfo; import com.google.gerrit.extensions.common.LabelInfo; import com.google.gerrit.extensions.common.VotingRangeInfo; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.notedb.ReviewerStateInternal; 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.DeleteVoteControl; +import com.google.gerrit.server.project.RemoveReviewerControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -69,10 +73,17 @@ private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final PermissionBackend permissionBackend; + private final DeleteVoteControl deleteVoteControl; + private final RemoveReviewerControl removeReviewerControl; @Inject - LabelsJson(PermissionBackend permissionBackend) { + LabelsJson( + PermissionBackend permissionBackend, + DeleteVoteControl deleteVoteControl, + RemoveReviewerControl removeReviewerControl) { this.permissionBackend = permissionBackend; + this.deleteVoteControl = deleteVoteControl; + this.removeReviewerControl = removeReviewerControl; } /** @@ -133,6 +144,46 @@ return permitted.asMap(); } + /** + * Returns A map of all labels that the provided user has permission to remove. + * + * @param accountLoader to load the reviewers' data with. + * @param user a Gerrit user. + * @param cd {@link ChangeData} corresponding to a specific gerrit change. + * @return A Map of {@code labelName} -> {Map of {@code value} -> List of {@link AccountInfo}} + * that the user can remove votes from. + */ + Map<String, Map<String, List<AccountInfo>>> removableLabels( + AccountLoader accountLoader, CurrentUser user, ChangeData cd) + throws PermissionBackendException { + if (cd.change().isMerged()) { + return new HashMap<>(); + } + + Map<String, Map<String, List<AccountInfo>>> res = new HashMap<>(); + LabelTypes labelTypes = cd.getLabelTypes(); + for (PatchSetApproval approval : cd.currentApprovals()) { + Optional<LabelType> labelType = labelTypes.byLabel(approval.labelId()); + if (!labelType.isPresent()) { + continue; + } + if (!(deleteVoteControl.testDeleteVotePermissions(user, cd, approval, labelType.get()) + || removeReviewerControl.testRemoveReviewer( + cd, user, approval.accountId(), approval.value()))) { + continue; + } + if (!res.containsKey(approval.label())) { + res.put(approval.label(), new HashMap<>()); + } + String labelValue = LabelValue.formatValue(approval.value()); + if (!res.get(approval.label()).containsKey(labelValue)) { + res.get(approval.label()).put(labelValue, new ArrayList<>()); + } + res.get(approval.label()).get(labelValue).add(accountLoader.get(approval.accountId())); + } + return res; + } + private static void clearOnlyZerosEntries(SetMultimap<String, String> permitted) { List<String> toClear = Lists.newArrayListWithCapacity(permitted.keySet().size()); for (Map.Entry<String, Collection<String>> e : permitted.asMap().entrySet()) { @@ -217,10 +268,10 @@ } } - private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) { + private Map<String, Short> currentLabels(@Nullable Account.Id accountId, ChangeData cd) { Map<String, Short> result = new HashMap<>(); for (PatchSetApproval psa : cd.currentApprovals()) { - if (psa.accountId().equals(accountId)) { + if (accountId == null || psa.accountId().equals(accountId)) { result.put(psa.label(), psa.value()); } }
diff --git a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java new file mode 100644 index 0000000..622f0cf --- /dev/null +++ b/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
@@ -0,0 +1,155 @@ +// Copyright (C) 2022 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.permissions; + +import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF; +import static java.util.Objects.requireNonNull; + +import com.google.gerrit.entities.LabelType; +import com.google.gerrit.server.util.LabelVote; + +/** Abstract permission representing a label. */ +public abstract class AbstractLabelPermission implements ChangePermissionOrLabel { + public enum ForUser { + SELF, + ON_BEHALF_OF + } + + protected final ForUser forUser; + protected final String name; + + /** + * Construct a reference to an abstract label permission. + * + * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior. + * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}. + */ + public AbstractLabelPermission(ForUser forUser, String name) { + this.forUser = requireNonNull(forUser, "ForUser"); + this.name = LabelType.checkName(name); + } + + /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */ + public ForUser forUser() { + return forUser; + } + + /** Returns name of the label, e.g. {@code "Code-Review"}. */ + public String label() { + return name; + } + + protected abstract String permissionPrefix(); + + protected String permissionName() { + if (forUser == ON_BEHALF_OF) { + return permissionPrefix() + "As"; + } + return permissionPrefix(); + } + + @Override + public final String describeForException() { + if (forUser == ON_BEHALF_OF) { + return permissionPrefix() + " on behalf of " + name; + } + return permissionPrefix() + " " + name; + } + + @Override + public final int hashCode() { + return (permissionPrefix() + name).hashCode(); + } + + @Override + @SuppressWarnings("EqualsGetClass") + public final boolean equals(Object other) { + if (this.getClass().isAssignableFrom(other.getClass())) { + AbstractLabelPermission b = (AbstractLabelPermission) other; + return forUser == b.forUser && name.equals(b.name); + } + return false; + } + + @Override + public final String toString() { + return permissionName() + "[" + name + ']'; + } + + /** A {@link AbstractLabelPermission} at a specific value. */ + public abstract static class WithValue implements ChangePermissionOrLabel { + private final ForUser forUser; + private final LabelVote label; + + /** + * Construct a reference to an abstract label permission at a specific value. + * + * @param forUser {@code SELF} (default) or {@code ON_BEHALF_OF} for labelAs behavior. + * @param label label name and vote. + */ + public WithValue(ForUser forUser, LabelVote label) { + this.forUser = requireNonNull(forUser, "ForUser"); + this.label = requireNonNull(label, "LabelVote"); + } + + /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */ + public ForUser forUser() { + return forUser; + } + + /** Returns name of the label, e.g. {@code "Code-Review"}. */ + public String label() { + return label.label(); + } + + /** Returns specific value of the label, e.g. 1 or 2. */ + public short value() { + return label.value(); + } + + public abstract String permissionName(); + + @Override + public final String describeForException() { + if (forUser == ON_BEHALF_OF) { + return permissionName() + " on behalf of " + label.formatWithEquals(); + } + return permissionName() + " " + label.formatWithEquals(); + } + + @Override + public final int hashCode() { + return (permissionName() + label).hashCode(); + } + + @Override + @SuppressWarnings("EqualsGetClass") + public final boolean equals(Object other) { + if (this.getClass().isAssignableFrom(other.getClass())) { + AbstractLabelPermission.WithValue b = (AbstractLabelPermission.WithValue) other; + return forUser == b.forUser && label.equals(b.label); + } + return false; + } + + @Override + public final String toString() { + if (forUser == ON_BEHALF_OF) { + return permissionName() + "As[" + label.format() + ']'; + } + return permissionName() + "[" + label.format() + ']'; + } + } +}
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index 8d432c8..6f7d761 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -14,8 +14,8 @@ package com.google.gerrit.server.permissions; +import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF; import static com.google.gerrit.server.permissions.DefaultPermissionMappings.labelPermissionName; -import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF; import com.google.common.collect.Maps; import com.google.common.collect.Sets; @@ -240,10 +240,10 @@ private boolean can(ChangePermissionOrLabel perm) throws PermissionBackendException { if (perm instanceof ChangePermission) { return can((ChangePermission) perm); - } else if (perm instanceof LabelPermission) { - return can((LabelPermission) perm); - } else if (perm instanceof LabelPermission.WithValue) { - return can((LabelPermission.WithValue) perm); + } else if (perm instanceof AbstractLabelPermission) { + return can((AbstractLabelPermission) perm); + } else if (perm instanceof AbstractLabelPermission.WithValue) { + return can((AbstractLabelPermission.WithValue) perm); } throw new PermissionBackendException(perm + " unsupported"); } @@ -288,11 +288,11 @@ throw new PermissionBackendException(perm + " unsupported"); } - private boolean can(LabelPermission perm) { + private boolean can(AbstractLabelPermission perm) { return !label(labelPermissionName(perm)).isEmpty(); } - private boolean can(LabelPermission.WithValue perm) { + private boolean can(AbstractLabelPermission.WithValue perm) { PermissionRange r = label(labelPermissionName(perm)); if (perm.forUser() == ON_BEHALF_OF && r.isEmpty()) { return false;
diff --git a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java index 2824efd..f59ba02 100644 --- a/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java +++ b/java/com/google/gerrit/server/permissions/ChangePermissionOrLabel.java
@@ -16,5 +16,5 @@ import com.google.gerrit.extensions.api.access.GerritPermission; -/** A {@link ChangePermission} or a {@link LabelPermission}. */ +/** A {@link ChangePermission} or a {@link AbstractLabelPermission}. */ public interface ChangePermissionOrLabel extends GerritPermission {}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java index 9d69d9b..89f0493 100644 --- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java +++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -24,7 +24,7 @@ import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission; import com.google.gerrit.extensions.api.access.PluginPermission; import com.google.gerrit.extensions.api.access.PluginProjectPermission; -import com.google.gerrit.server.permissions.LabelPermission.ForUser; +import com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser; import java.util.EnumSet; import java.util.Optional; import java.util.Set; @@ -160,19 +160,29 @@ return Optional.ofNullable(CHANGE_PERMISSIONS.inverse().get(permissionName)); } - public static String labelPermissionName(LabelPermission labelPermission) { - if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) { - return Permission.forLabelAs(labelPermission.label()); + public static String labelPermissionName(AbstractLabelPermission labelPermission) { + if (labelPermission instanceof LabelPermission) { + if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) { + return Permission.forLabelAs(labelPermission.label()); + } + return Permission.forLabel(labelPermission.label()); + } else if (labelPermission instanceof LabelRemovalPermission) { + return Permission.forRemoveLabel(labelPermission.label()); } - return Permission.forLabel(labelPermission.label()); + throw new IllegalStateException("invalid AbstractLabelPermission subtype"); } // TODO(dborowitz): Can these share a common superinterface? - public static String labelPermissionName(LabelPermission.WithValue labelPermission) { - if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) { - return Permission.forLabelAs(labelPermission.label()); + public static String labelPermissionName(AbstractLabelPermission.WithValue labelPermission) { + if (labelPermission instanceof LabelPermission.WithValue) { + if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) { + return Permission.forLabelAs(labelPermission.label()); + } + return Permission.forLabel(labelPermission.label()); + } else if (labelPermission instanceof LabelRemovalPermission.WithValue) { + return Permission.forRemoveLabel(labelPermission.label()); } - return Permission.forLabel(labelPermission.label()); + throw new IllegalStateException("invalid AbstractLabelPermission.WithValue subtype"); } private DefaultPermissionMappings() {}
diff --git a/java/com/google/gerrit/server/permissions/LabelPermission.java b/java/com/google/gerrit/server/permissions/LabelPermission.java index c266caa..4652364 100644 --- a/java/com/google/gerrit/server/permissions/LabelPermission.java +++ b/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -14,24 +14,14 @@ package com.google.gerrit.server.permissions; -import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF; -import static com.google.gerrit.server.permissions.LabelPermission.ForUser.SELF; -import static java.util.Objects.requireNonNull; +import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF; import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.LabelValue; import com.google.gerrit.server.util.LabelVote; /** Permission representing a label. */ -public class LabelPermission implements ChangePermissionOrLabel { - public enum ForUser { - SELF, - ON_BEHALF_OF; - } - - private final ForUser forUser; - private final String name; - +public class LabelPermission extends AbstractLabelPermission { /** * Construct a reference to a label permission. * @@ -67,55 +57,16 @@ * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}. */ public LabelPermission(ForUser forUser, String name) { - this.forUser = requireNonNull(forUser, "ForUser"); - this.name = LabelType.checkName(name); - } - - /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */ - public ForUser forUser() { - return forUser; - } - - /** Returns name of the label, e.g. {@code "Code-Review"}. */ - public String label() { - return name; + super(forUser, name); } @Override - public String describeForException() { - if (forUser == ON_BEHALF_OF) { - return "label on behalf of " + name; - } - return "label " + name; - } - - @Override - public int hashCode() { - return name.hashCode(); - } - - @Override - public boolean equals(Object other) { - if (other instanceof LabelPermission) { - LabelPermission b = (LabelPermission) other; - return forUser == b.forUser && name.equals(b.name); - } - return false; - } - - @Override - public String toString() { - if (forUser == ON_BEHALF_OF) { - return "LabelAs[" + name + ']'; - } - return "Label[" + name + ']'; + public String permissionPrefix() { + return "label"; } /** A {@link LabelPermission} at a specific value. */ - public static class WithValue implements ChangePermissionOrLabel { - private final ForUser forUser; - private final LabelVote label; - + public static class WithValue extends AbstractLabelPermission.WithValue { /** * Construct a reference to a label at a specific value. * @@ -195,53 +146,12 @@ * @param label label name and vote. */ public WithValue(ForUser forUser, LabelVote label) { - this.forUser = requireNonNull(forUser, "ForUser"); - this.label = requireNonNull(label, "LabelVote"); - } - - /** Returns {@code SELF} or {@code ON_BEHALF_OF} (or labelAs). */ - public ForUser forUser() { - return forUser; - } - - /** Returns name of the label, e.g. {@code "Code-Review"}. */ - public String label() { - return label.label(); - } - - /** Returns specific value of the label, e.g. 1 or 2. */ - public short value() { - return label.value(); + super(forUser, label); } @Override - public String describeForException() { - if (forUser == ON_BEHALF_OF) { - return "label on behalf of " + label.formatWithEquals(); - } - return "label " + label.formatWithEquals(); - } - - @Override - public int hashCode() { - return label.hashCode(); - } - - @Override - public boolean equals(Object other) { - if (other instanceof WithValue) { - WithValue b = (WithValue) other; - return forUser == b.forUser && label.equals(b.label); - } - return false; - } - - @Override - public String toString() { - if (forUser == ON_BEHALF_OF) { - return "LabelAs[" + label.format() + ']'; - } - return "Label[" + label.format() + ']'; + public String permissionName() { + return "label"; } } }
diff --git a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java new file mode 100644 index 0000000..2553601 --- /dev/null +++ b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
@@ -0,0 +1,94 @@ +// Copyright (C) 2022 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.permissions; + +import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF; + +import com.google.gerrit.entities.LabelType; +import com.google.gerrit.entities.LabelValue; +import com.google.gerrit.server.util.LabelVote; + +/** Permission representing a label removal. */ +public class LabelRemovalPermission extends AbstractLabelPermission { + /** + * Construct a reference to a label removal permission. + * + * @param type type description of the label. + */ + public LabelRemovalPermission(LabelType type) { + this(type.getName()); + } + + /** + * Construct a reference to a label removal permission. + * + * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}. + */ + public LabelRemovalPermission(String name) { + super(SELF, name); + } + + @Override + public String permissionPrefix() { + return "removeLabel"; + } + + /** A {@link LabelRemovalPermission} at a specific value. */ + public static class WithValue extends AbstractLabelPermission.WithValue { + /** + * Construct a reference to a label removal at a specific value. + * + * @param type description of the label. + * @param value numeric score assigned to the label. + */ + public WithValue(LabelType type, LabelValue value) { + this(type.getName(), value.getValue()); + } + + /** + * Construct a reference to a label removal at a specific value. + * + * @param type description of the label. + * @param value numeric score assigned to the label. + */ + public WithValue(LabelType type, short value) { + this(type.getName(), value); + } + + /** + * Construct a reference to a label removal at a specific value. + * + * @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}. + * @param value numeric score assigned to the label. + */ + public WithValue(String name, short value) { + this(LabelVote.create(name, value)); + } + + /** + * Construct a reference to a label removal at a specific value. + * + * @param label label name and vote. + */ + public WithValue(LabelVote label) { + super(SELF, label); + } + + @Override + public String permissionName() { + return "removeLabel"; + } + } +}
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java index fea2827..eb5e053 100644 --- a/java/com/google/gerrit/server/permissions/PermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -474,6 +474,18 @@ } /** + * Test which values of a label the user may be able to remove. + * + * @param label definition of the label to test values of. + * @return set containing values the user may be able to use; may be empty if none. + * @throws PermissionBackendException if failure consulting backend configuration. + */ + public Set<LabelRemovalPermission.WithValue> testRemoval(LabelType label) + throws PermissionBackendException { + return test(removalValuesOf(requireNonNull(label, "LabelType"))); + } + + /** * Test which values of a group of labels the user may be able to set. * * @param types definition of the labels to test values of. @@ -486,10 +498,29 @@ return test(types.stream().flatMap(t -> valuesOf(t).stream()).collect(toSet())); } + /** + * Test which values of a group of labels the user may be able to remove. + * + * @param types definition of the labels to test values of. + * @return set containing values the user may be able to use; may be empty if none. + * @throws PermissionBackendException if failure consulting backend configuration. + */ + public Set<LabelRemovalPermission.WithValue> testLabelRemovals(Collection<LabelType> types) + throws PermissionBackendException { + requireNonNull(types, "LabelType"); + return test(types.stream().flatMap(t -> removalValuesOf(t).stream()).collect(toSet())); + } + private static Set<LabelPermission.WithValue> valuesOf(LabelType label) { return label.getValues().stream() .map(v -> new LabelPermission.WithValue(label, v)) .collect(toSet()); } + + private static Set<LabelRemovalPermission.WithValue> removalValuesOf(LabelType label) { + return label.getValues().stream() + .map(v -> new LabelRemovalPermission.WithValue(label, v)) + .collect(toSet()); + } } }
diff --git a/java/com/google/gerrit/server/project/DeleteVoteControl.java b/java/com/google/gerrit/server/project/DeleteVoteControl.java new file mode 100644 index 0000000..3f3f88a --- /dev/null +++ b/java/com/google/gerrit/server/project/DeleteVoteControl.java
@@ -0,0 +1,81 @@ +// Copyright (C) 2022 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.entities.Account; +import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.LabelType; +import com.google.gerrit.entities.PatchSetApproval; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.LabelRemovalPermission; +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.query.change.ChangeData; +import com.google.inject.Inject; +import java.util.Set; + +public class DeleteVoteControl { + private final PermissionBackend permissionBackend; + private final ChangeData.Factory changeDataFactory; + + @Inject + public DeleteVoteControl( + PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory) { + this.permissionBackend = permissionBackend; + this.changeDataFactory = changeDataFactory; + } + + public boolean testDeleteVotePermissions( + CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType) + throws PermissionBackendException { + return testDeleteVotePermissions(user, changeDataFactory.create(notes), approval, labelType); + } + + public boolean testDeleteVotePermissions( + CurrentUser user, ChangeData cd, PatchSetApproval approval, LabelType labelType) + throws PermissionBackendException { + if (canRemoveReviewerWithoutRemoveLabelPermission( + cd.change(), user, approval.accountId(), approval.value())) { + return true; + } + // Test if the user is allowed to remove vote of the given label type and value. + Set<LabelRemovalPermission.WithValue> allowed = + permissionBackend.user(user).change(cd).testRemoval(labelType); + return allowed.contains(new LabelRemovalPermission.WithValue(labelType, approval.value())); + } + + private boolean canRemoveReviewerWithoutRemoveLabelPermission( + Change change, CurrentUser user, Account.Id reviewer, int value) + throws PermissionBackendException { + if (user.isIdentifiedUser()) { + Account.Id aId = user.getAccountId(); + if (aId.equals(reviewer)) { + return true; // A user can always remove their own votes. + } else if (aId.equals(change.getOwner()) && 0 <= value) { + return true; // The change owner may remove any zero or positive score. + } + } + + // Users with the remove reviewer permission, the branch owner, project + // owner and site admin can remove anyone + PermissionBackend.WithUser withUser = permissionBackend.user(user); + PermissionBackend.ForProject forProject = withUser.project(change.getProject()); + return forProject.ref(change.getDest().branch()).test(RefPermission.WRITE_CONFIG) + || withUser.test(GlobalPermission.ADMINISTRATE_SERVER); + } +}
diff --git a/java/com/google/gerrit/server/project/RemoveReviewerControl.java b/java/com/google/gerrit/server/project/RemoveReviewerControl.java index 1bc309c..3fda87a 100644 --- a/java/com/google/gerrit/server/project/RemoveReviewerControl.java +++ b/java/com/google/gerrit/server/project/RemoveReviewerControl.java
@@ -32,10 +32,12 @@ @Singleton public class RemoveReviewerControl { private final PermissionBackend permissionBackend; + private final ChangeData.Factory changeDataFactory; @Inject - RemoveReviewerControl(PermissionBackend permissionBackend) { + RemoveReviewerControl(PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory) { this.permissionBackend = permissionBackend; + this.changeDataFactory = changeDataFactory; } /** @@ -64,6 +66,20 @@ /** Returns true if the user is allowed to remove this reviewer. */ public boolean testRemoveReviewer( + ChangeNotes notes, CurrentUser currentUser, PatchSetApproval approval) + throws PermissionBackendException { + return testRemoveReviewer(notes, currentUser, approval.accountId(), approval.value()); + } + + /** Returns true if the user is allowed to remove this reviewer. */ + public boolean testRemoveReviewer( + ChangeNotes notes, CurrentUser currentUser, Account.Id reviewer, int value) + throws PermissionBackendException { + return testRemoveReviewer(changeDataFactory.create(notes), currentUser, reviewer, value); + } + + /** Returns true if the user is allowed to remove this reviewer. */ + public boolean testRemoveReviewer( ChangeData cd, CurrentUser currentUser, Account.Id reviewer, int value) throws PermissionBackendException { if (canRemoveReviewerWithoutPermissionCheck(
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java index a0fc121..0e1a218 100644 --- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java +++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -20,6 +20,7 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.LabelType; import com.google.gerrit.entities.LabelTypes; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.PatchSetApproval; @@ -37,7 +38,9 @@ import com.google.gerrit.server.mail.send.DeleteVoteSender; import com.google.gerrit.server.mail.send.MessageIdGenerator; import com.google.gerrit.server.mail.send.ReplyToChangeSender; +import com.google.gerrit.server.permissions.LabelRemovalPermission; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.project.DeleteVoteControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.RemoveReviewerControl; import com.google.gerrit.server.update.BatchUpdateOp; @@ -75,6 +78,7 @@ private final VoteDeleted voteDeleted; private final DeleteVoteSender.Factory deleteVoteSenderFactory; + private final DeleteVoteControl deleteVoteControl; private final RemoveReviewerControl removeReviewerControl; private final MessageIdGenerator messageIdGenerator; @@ -96,8 +100,9 @@ ChangeMessagesUtil cmUtil, VoteDeleted voteDeleted, DeleteVoteSender.Factory deleteVoteSenderFactory, - RemoveReviewerControl removeReviewerControl, + DeleteVoteControl deleteVoteControl, MessageIdGenerator messageIdGenerator, + RemoveReviewerControl removeReviewerControl, @Assisted Project.NameKey projectName, @Assisted AccountState reviewerToDeleteVoteFor, @Assisted String label, @@ -109,6 +114,7 @@ this.cmUtil = cmUtil; this.voteDeleted = voteDeleted; this.deleteVoteSenderFactory = deleteVoteSenderFactory; + this.deleteVoteControl = deleteVoteControl; this.removeReviewerControl = removeReviewerControl; this.messageIdGenerator = messageIdGenerator; @@ -143,12 +149,7 @@ newApprovals.put(a.label(), a.value()); continue; } else if (enforcePermissions) { - // For regular users, check if they are allowed to remove the vote. - try { - removeReviewerControl.checkRemoveReviewer(ctx.getNotes(), ctx.getUser(), a); - } catch (AuthException e) { - throw new AuthException("delete vote not permitted", e); - } + checkPermissions(ctx, labelTypes.byLabel(a.labelId()).get(), a); } // Set the approval to 0 if vote is being removed. newApprovals.put(a.label(), (short) 0); @@ -209,4 +210,21 @@ user.isIdentifiedUser() ? user.asIdentifiedUser().state() : null, ctx.getWhen()); } + + private void checkPermissions(ChangeContext ctx, LabelType labelType, PatchSetApproval approval) + throws PermissionBackendException, AuthException { + boolean permitted = + removeReviewerControl.testRemoveReviewer(ctx.getNotes(), ctx.getUser(), approval) + || deleteVoteControl.testDeleteVotePermissions( + ctx.getUser(), ctx.getNotes(), approval, labelType); + if (!permitted) { + throw new AuthException( + "Delete vote not permitted.", + new AuthException( + "Both " + + new LabelRemovalPermission.WithValue(labelType, approval.value()) + .describeForException() + + " and remove-reviewer are not permitted")); + } + } }
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java index 259e71d..22eb32c 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReview.java +++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -17,7 +17,7 @@ import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL; -import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BEHALF_OF; +import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF; import static com.google.gerrit.server.project.ProjectCache.illegalState; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.groupingBy;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 211baeb..215d1e8 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -28,6 +28,7 @@ import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey; import static com.google.gerrit.entities.RefNames.changeMetaRef; @@ -160,6 +161,7 @@ import com.google.gerrit.index.query.PostFilterPredicate; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.account.AccountControl; +import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeMessages; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.testing.TestChangeETagComputation; @@ -2406,7 +2408,68 @@ .id(r.getChangeId()) .reviewer(admin.id().toString()) .deleteVote(LabelId.CODE_REVIEW)); - assertThat(thrown).hasMessageThat().contains("delete vote not permitted"); + assertThat(thrown).hasMessageThat().contains("Delete vote not permitted"); + } + + @Test + public void deleteVoteAlwaysPermittedForSelfVotes() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + allowLabel(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/heads/*").group(REGISTERED_USERS)) + .update(); + + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + + requestScopeOperations.setApiUser(user.id()); + gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve()); + + gApi.changes() + .id(r.getChangeId()) + .reviewer(user.id().toString()) + .deleteVote(LabelId.CODE_REVIEW); + } + + @Test + public void deleteVoteAlwaysPermittedForAdmin() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + allowLabel(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/heads/*").group(REGISTERED_USERS)) + .update(); + + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + + requestScopeOperations.setApiUser(user.id()); + gApi.changes().id(changeId).revision(r.getCommit().name()).review(ReviewInput.approve()); + + requestScopeOperations.setApiUser(admin.id()); + gApi.changes() + .id(r.getChangeId()) + .reviewer(user.id().toString()) + .deleteVote(LabelId.CODE_REVIEW); } @Test @@ -2996,7 +3059,11 @@ .review(ReviewInput.approve()); gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).submit(); - createChange(); + PushOneCommit.Result change = createChange(); + // Populate change with a reasonable set of fields. We can't exhaustively + // test all possible variations, but can try to cover a reasonable set. + approve(change.getChangeId()); + gApi.changes().id(change.getChangeId()).addReviewer(user.email()); requestScopeOperations.setApiUser(user.id()); try (AutoCloseable ignored = disableNoteDb()) { @@ -3011,6 +3078,34 @@ } @Test + public void nonLazyloadQueryOptionsDoNotTouchDatabase() throws Exception { + requestScopeOperations.setApiUser(admin.id()); + PushOneCommit.Result r1 = createChange(); + gApi.changes() + .id(r1.getChangeId()) + .revision(r1.getCommit().name()) + .review(ReviewInput.approve()); + gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).submit(); + + PushOneCommit.Result change = createChange(); + // Populate change with a reasonable set of fields. We can't exhaustively + // test all possible variations, but can try to cover a reasonable set. + approve(change.getChangeId()); + gApi.changes().id(change.getChangeId()).addReviewer(user.email()); + + requestScopeOperations.setApiUser(user.id()); + try (AutoCloseable ignored = disableNoteDb()) { + assertThat( + gApi.changes() + .query() + .withQuery("project:{" + project.get() + "} (status:open OR status:closed)") + .withOptions(EnumSet.complementOf(EnumSet.copyOf(ChangeJson.REQUIRE_LAZY_LOAD))) + .get()) + .hasSize(2); + } + } + + @Test public void votable() throws Exception { PushOneCommit.Result r = createChange(); String triplet = project.get() + "~master~" + r.getChangeId(); @@ -3275,6 +3370,7 @@ assertThat(change.status).isEqualTo(ChangeStatus.NEW); assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW); + assertThat(change.removableLabels).isEmpty(); // add new label and assert that it's returned for existing changes AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID(); @@ -3304,6 +3400,9 @@ .id(r.getChangeId()) .revision(r.getCommit().name()) .review(new ReviewInput().label(verified.getName(), verified.getMax().getValue())); + change = gApi.changes().id(r.getChangeId()).get(); + assertPermitted(change, LabelId.VERIFIED, -1, 0, 1); + assertOnlyRemovableLabel(change, LabelId.VERIFIED, "+1", admin); try (ProjectConfigUpdate u = updateProject(project)) { // remove label and assert that it's no longer returned for existing @@ -3323,6 +3422,7 @@ change = gApi.changes().id(r.getChangeId()).get(); assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW); + assertThat(change.removableLabels).isEmpty(); // abandon the change and see that the returned labels stay the same // while all permitted labels disappear. @@ -3331,6 +3431,7 @@ assertThat(change.status).isEqualTo(ChangeStatus.ABANDONED); assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertThat(change.permittedLabels).isEmpty(); + assertThat(change.removableLabels).isEmpty(); } @Test @@ -3447,6 +3548,7 @@ assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW, LabelId.VERIFIED); assertPermitted(change, LabelId.CODE_REVIEW, 2); assertPermitted(change, LabelId.VERIFIED, 1); + assertThat(change.removableLabels).isEmpty(); // remove label and assert that it's no longer returned for existing // changes, even if there is an approval for it @@ -3464,6 +3566,7 @@ assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertPermitted(change, LabelId.CODE_REVIEW, 2); + assertThat(change.removableLabels).isEmpty(); } @Test @@ -3560,6 +3663,7 @@ .containsExactly(LabelId.CODE_REVIEW, "Non-Author-Code-Review"); assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2); + assertThat(change.removableLabels).isEmpty(); } @Test @@ -3575,6 +3679,7 @@ assertThat(change.submissionId).isNotNull(); assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); assertPermitted(change, LabelId.CODE_REVIEW, 0, 1, 2); + assertThat(change.removableLabels).isEmpty(); } @Test @@ -4281,8 +4386,12 @@ ListChangesOption.SKIP_DIFFSTAT); PushOneCommit.Result change = createChange(); - int number = gApi.changes().id(change.getChangeId()).get()._number; + // Populate change with a reasonable set of fields. We can't exhaustively + // test all possible variations, but can try to cover a reasonable set. + approve(change.getChangeId()); + gApi.changes().id(change.getChangeId()).addReviewer(user.email()); + int number = gApi.changes().id(change.getChangeId()).get()._number; try (AutoCloseable ignored = changeIndexOperations.disableReadsAndWrites()) { assertThat(gApi.changes().id(project.get(), number).get(options).changeId) .isEqualTo(change.getChangeId());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java index c57d285..016b1e6 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -15,16 +15,24 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval; import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.LabelId; +import com.google.gerrit.entities.Permission; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ChangeInfo; @@ -39,56 +47,141 @@ import org.junit.Test; public class DeleteVoteIT extends AbstractDaemonTest { + @Inject private ProjectOperations projectOperations; @Inject private RequestScopeOperations requestScopeOperations; @Test - public void deleteVoteOnChange() throws Exception { - deleteVote(false); + public void deleteVoteOnChange_withRemoveLabelPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + allowLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyDeleteVote(false); } @Test - public void deleteVoteOnRevision() throws Exception { - deleteVote(true); + public void deleteVoteOnChange_withRemoveReviewerPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyDeleteVote(false); } - private void deleteVote(boolean onRevisionLevel) throws Exception { + @Test + public void deleteVoteOnChange_noPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyCannotDeleteVote(false); + } + + @Test + public void deleteVoteOnRevision_withRemoveLabelPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + allowLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyDeleteVote(true); + } + + @Test + public void deleteVoteOnRevision_withRemoveReviewerPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(allow(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyDeleteVote(true); + } + + @Test + public void deleteVoteOnRevision_noPermission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add( + blockLabelRemoval(LabelId.CODE_REVIEW) + .ref("refs/heads/*") + .group(REGISTERED_USERS) + .range(-2, 2)) + .add(block(Permission.REMOVE_REVIEWER).ref("refs/*").group(REGISTERED_USERS)) + .update(); + verifyCannotDeleteVote(true); + } + + private void verifyDeleteVote(boolean onRevisionLevel) throws Exception { PushOneCommit.Result r = createChange(); gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve()); PushOneCommit.Result r2 = amendChange(r.getChangeId()); - requestScopeOperations.setApiUser(user.id()); + requestScopeOperations.setApiUser(admin.id()); + recommend(r.getChangeId()); + + TestAccount user2 = accountCreator.user2(); + requestScopeOperations.setApiUser(user2.id()); recommend(r.getChangeId()); sender.clear(); - String endPoint = + String deleteAdminVoteEndPoint = "/changes/" + r.getChangeId() + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "") + "/reviewers/" - + user.id().toString() + + admin.id().toString() + "/votes/Code-Review"; - RestResponse response = adminRestSession.delete(endPoint); + RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint); response.assertNoContent(); List<FakeEmailSender.Message> messages = sender.getMessages(); assertThat(messages).hasSize(1); FakeEmailSender.Message msg = messages.get(0); - assertThat(msg.rcpt()).containsExactly(user.getNameEmail()); - assertThat(msg.body()).contains(admin.fullName() + " has removed a vote from this change."); + assertThat(msg.rcpt()).containsExactly(admin.getNameEmail(), user2.getNameEmail()); + assertThat(msg.body()).contains(user.fullName() + " has removed a vote from this change."); assertThat(msg.body()) - .contains("Removed Code-Review+1 by " + user.fullName() + " <" + user.email() + ">\n"); + .contains("Removed Code-Review+1 by " + admin.fullName() + " <" + admin.email() + ">\n"); - endPoint = + String viewVotesEndPoint = "/changes/" + r.getChangeId() + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "") + "/reviewers/" - + user.id().toString() + + admin.id().toString() + "/votes"; - response = adminRestSession.get(endPoint); + response = userRestSession.get(viewVotesEndPoint); response.assertOK(); Map<String, Short> m = @@ -99,14 +192,38 @@ ChangeInfo c = gApi.changes().id(r.getChangeId()).get(); ChangeMessageInfo message = Iterables.getLast(c.messages); - assertThat(message.author._accountId).isEqualTo(admin.id().get()); + assertThat(message.author._accountId).isEqualTo(user.id().get()); assertThat(message.message) .isEqualTo( String.format( "Removed Code-Review+1 by %s\n", - AccountTemplateUtil.getAccountTemplate(user.id()))); + AccountTemplateUtil.getAccountTemplate(admin.id()))); assertThat(getReviewers(c.reviewers.get(REVIEWER))) - .containsExactlyElementsIn(ImmutableSet.of(admin.id(), user.id())); + .containsExactlyElementsIn(ImmutableSet.of(user2.id(), admin.id())); + } + + private void verifyCannotDeleteVote(boolean onRevisionLevel) throws Exception { + PushOneCommit.Result r = createChange(); + gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve()); + + PushOneCommit.Result r2 = amendChange(r.getChangeId()); + + requestScopeOperations.setApiUser(admin.id()); + recommend(r.getChangeId()); + + sender.clear(); + String deleteAdminVoteEndPoint = + "/changes/" + + r.getChangeId() + + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "") + + "/reviewers/" + + admin.id().toString() + + "/votes/Code-Review"; + + RestResponse response = userRestSession.delete(deleteAdminVoteEndPoint); + response.assertForbidden(); + + assertThat(sender.getMessages()).isEmpty(); } private Iterable<Account.Id> getReviewers(Collection<AccountInfo> r) {
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java index c1a7627..661802e 100644 --- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java +++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -18,11 +18,14 @@ import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabel; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.blockLabelRemoval; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.capabilityKey; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelRemovalPermissionKey; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey; import static com.google.gerrit.common.data.GlobalCapability.ADMINISTRATE_SERVER; import static com.google.gerrit.common.data.GlobalCapability.DEFAULT_MAX_QUERY_LIMIT; @@ -443,6 +446,73 @@ } @Test + public void addAllowLabelRemovalPermission() throws Exception { + Project.NameKey key = projectOperations.newProject().create(); + projectOperations + .project(key) + .forUpdate() + .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2)) + .update(); + + Config config = projectOperations.project(key).getConfig(); + assertThat(config).sections().containsExactly("access", "submit"); + assertThat(config).subsections("access").containsExactly("refs/foo"); + assertThat(config) + .subsectionValues("access", "refs/foo") + .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users"); + } + + @Test + public void addBlockLabelRemovalPermission() throws Exception { + Project.NameKey key = projectOperations.newProject().create(); + projectOperations + .project(key) + .forUpdate() + .add(blockLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2)) + .update(); + + Config config = projectOperations.project(key).getConfig(); + assertThat(config).sections().containsExactly("access", "submit"); + assertThat(config).subsections("access").containsExactly("refs/foo"); + assertThat(config) + .subsectionValues("access", "refs/foo") + .containsExactly("removeLabel-Code-Review", "block -1..+2 group global:Registered-Users"); + } + + @Test + public void addAllowExclusiveLabelRemovalPermission() throws Exception { + Project.NameKey key = projectOperations.newProject().create(); + projectOperations + .project(key) + .forUpdate() + .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2)) + .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), true) + .update(); + + Config config = projectOperations.project(key).getConfig(); + assertThat(config).sections().containsExactly("access", "submit"); + assertThat(config).subsections("access").containsExactly("refs/foo"); + assertThat(config) + .subsectionValues("access", "refs/foo") + .containsExactly( + "removeLabel-Code-Review", "-1..+2 group global:Registered-Users", + "exclusiveGroupPermissions", "removeLabel-Code-Review"); + + projectOperations + .project(key) + .forUpdate() + .setExclusiveGroup(labelRemovalPermissionKey("Code-Review").ref("refs/foo"), false) + .update(); + + config = projectOperations.project(key).getConfig(); + assertThat(config).sections().containsExactly("access", "submit"); + assertThat(config).subsections("access").containsExactly("refs/foo"); + assertThat(config) + .subsectionValues("access", "refs/foo") + .containsExactly("removeLabel-Code-Review", "-1..+2 group global:Registered-Users"); + } + + @Test public void addAllowCapability() throws Exception { Config config = projectOperations.project(allProjects).getConfig(); assertThat(config) @@ -540,6 +610,31 @@ } @Test + public void removeLabelRemovalPermission() throws Exception { + Project.NameKey key = projectOperations.newProject().create(); + projectOperations + .project(key) + .forUpdate() + .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(REGISTERED_USERS).range(-1, 2)) + .add(allowLabelRemoval("Code-Review").ref("refs/foo").group(PROJECT_OWNERS).range(-2, 1)) + .update(); + assertThat(projectOperations.project(key).getConfig()) + .subsectionValues("access", "refs/foo") + .containsExactly( + "removeLabel-Code-Review", "-1..+2 group global:Registered-Users", + "removeLabel-Code-Review", "-2..+1 group global:Project-Owners"); + + projectOperations + .project(key) + .forUpdate() + .remove(labelRemovalPermissionKey("Code-Review").ref("refs/foo").group(REGISTERED_USERS)) + .update(); + assertThat(projectOperations.project(key).getConfig()) + .subsectionValues("access", "refs/foo") + .containsExactly("removeLabel-Code-Review", "-2..+1 group global:Project-Owners"); + } + + @Test public void removeCapability() throws Exception { projectOperations .allProjectsForUpdate()
diff --git a/javatests/com/google/gerrit/entities/PermissionTest.java b/javatests/com/google/gerrit/entities/PermissionTest.java index 3175671..d25d833 100644 --- a/javatests/com/google/gerrit/entities/PermissionTest.java +++ b/javatests/com/google/gerrit/entities/PermissionTest.java
@@ -36,6 +36,7 @@ assertThat(Permission.isPermission(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue(); assertThat(Permission.isPermission(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue(); + assertThat(Permission.isPermission(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue(); assertThat(Permission.isPermission(LabelId.CODE_REVIEW)).isFalse(); } @@ -56,6 +57,7 @@ assertThat(Permission.isLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isTrue(); assertThat(Permission.isLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse(); + assertThat(Permission.isLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse(); assertThat(Permission.isLabel(LabelId.CODE_REVIEW)).isFalse(); } @@ -66,10 +68,22 @@ assertThat(Permission.isLabelAs(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse(); assertThat(Permission.isLabelAs(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isTrue(); + assertThat(Permission.isLabelAs(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isFalse(); assertThat(Permission.isLabelAs(LabelId.CODE_REVIEW)).isFalse(); } @Test + public void isRemoveLabel() { + assertThat(Permission.isRemoveLabel(Permission.ABANDON)).isFalse(); + assertThat(Permission.isRemoveLabel("no-permission")).isFalse(); + + assertThat(Permission.isRemoveLabel(Permission.LABEL + LabelId.CODE_REVIEW)).isFalse(); + assertThat(Permission.isRemoveLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)).isFalse(); + assertThat(Permission.isRemoveLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)).isTrue(); + assertThat(Permission.isRemoveLabel(LabelId.CODE_REVIEW)).isFalse(); + } + + @Test public void forLabel() { assertThat(Permission.forLabel(LabelId.CODE_REVIEW)) .isEqualTo(Permission.LABEL + LabelId.CODE_REVIEW); @@ -82,11 +96,19 @@ } @Test + public void forRemoveLabel() { + assertThat(Permission.forRemoveLabel(LabelId.CODE_REVIEW)) + .isEqualTo(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW); + } + + @Test public void extractLabel() { assertThat(Permission.extractLabel(Permission.LABEL + LabelId.CODE_REVIEW)) .isEqualTo(LabelId.CODE_REVIEW); assertThat(Permission.extractLabel(Permission.LABEL_AS + LabelId.CODE_REVIEW)) .isEqualTo(LabelId.CODE_REVIEW); + assertThat(Permission.extractLabel(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)) + .isEqualTo(LabelId.CODE_REVIEW); assertThat(Permission.extractLabel(LabelId.CODE_REVIEW)).isNull(); assertThat(Permission.extractLabel(Permission.ABANDON)).isNull(); } @@ -103,6 +125,10 @@ Permission.canBeOnAllProjects( AccessSection.ALL, Permission.LABEL_AS + LabelId.CODE_REVIEW)) .isTrue(); + assertThat( + Permission.canBeOnAllProjects( + AccessSection.ALL, Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)) + .isTrue(); assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.ABANDON)).isTrue(); assertThat(Permission.canBeOnAllProjects("refs/heads/*", Permission.OWNER)).isTrue(); @@ -113,6 +139,10 @@ Permission.canBeOnAllProjects( "refs/heads/*", Permission.LABEL_AS + LabelId.CODE_REVIEW)) .isTrue(); + assertThat( + Permission.canBeOnAllProjects( + "refs/heads/*", Permission.REMOVE_LABEL + LabelId.CODE_REVIEW)) + .isTrue(); } @Test @@ -126,6 +156,8 @@ .isEqualTo(LabelId.CODE_REVIEW); assertThat(Permission.create(Permission.LABEL_AS + LabelId.CODE_REVIEW).getLabel()) .isEqualTo(LabelId.CODE_REVIEW); + assertThat(Permission.create(Permission.REMOVE_LABEL + LabelId.CODE_REVIEW).getLabel()) + .isEqualTo(LabelId.CODE_REVIEW); assertThat(Permission.create(LabelId.CODE_REVIEW).getLabel()).isNull(); assertThat(Permission.create(Permission.ABANDON).getLabel()).isNull(); }
diff --git a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java index 7ed236a..f45d33b 100644 --- a/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java +++ b/javatests/com/google/gerrit/extensions/common/ChangeInfoDifferTest.java
@@ -48,6 +48,7 @@ assertThat(diff.added().messages).isNull(); assertThat(diff.added().reviewers).isNull(); assertThat(diff.added().hashtags).isNull(); + assertThat(diff.added().removableLabels).isNull(); assertThat(diff.removed()._number).isNull(); assertThat(diff.removed().branch).isNull(); assertThat(diff.removed().project).isNull(); @@ -56,6 +57,7 @@ assertThat(diff.removed().messages).isNull(); assertThat(diff.removed().reviewers).isNull(); assertThat(diff.removed().hashtags).isNull(); + assertThat(diff.removed().removableLabels).isNull(); } @Test @@ -315,6 +317,295 @@ } @Test + public void getDiff_removableLabelsEmpty_returnsNullRemovableLabels() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + oldChangeInfo.removableLabels = ImmutableMap.of(); + newChangeInfo.removableLabels = ImmutableMap.of(); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels).isNull(); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsNullAndEmpty_returnsEmptyRemovableLabels() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + newChangeInfo.removableLabels = ImmutableMap.of(); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels).isEmpty(); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsEmptyAndNull_returnsEmptyRemovableLabels() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + oldChangeInfo.removableLabels = ImmutableMap.of(); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels).isNull(); + assertThat(diff.removed().removableLabels).isEmpty(); + } + + @Test + public void getDiff_removableLabelsLabelAdded() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "Cow"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "Pig"; + AccountInfo acc3 = new AccountInfo(); + acc3.name = "Cat"; + AccountInfo acc4 = new AccountInfo(); + acc4.name = "Dog"; + + oldChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3))); + newChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)), + "Verified", + ImmutableMap.of("-1", ImmutableList.of(acc4))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4)))); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsLabelRemoved() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "Cow"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "Pig"; + AccountInfo acc3 = new AccountInfo(); + acc3.name = "Cat"; + AccountInfo acc4 = new AccountInfo(); + acc4.name = "Dog"; + + oldChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3)), + "Verified", + ImmutableMap.of("-1", ImmutableList.of(acc4))); + newChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels).isNull(); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc4)))); + } + + @Test + public void getDiff_removableLabelsVoteAdded() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + AccountInfo acc3 = new AccountInfo(); + acc3.name = "acc3"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3)))); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsVoteRemoved() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + AccountInfo acc3 = new AccountInfo(); + acc3.name = "acc3"; + + oldChangeInfo.removableLabels = + ImmutableMap.of( + "Code-Review", + ImmutableMap.of("+1", ImmutableList.of(acc1), "-1", ImmutableList.of(acc2, acc3))); + newChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels).isNull(); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc2, acc3)))); + } + + @Test + public void getDiff_removableLabelsAccountAdded() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2)))); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsAccountRemoved() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1, acc2))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2)))); + assertThat(diff.removed().removableLabels).isNull(); + } + + @Test + public void getDiff_removableLabelsAccountChanged() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc2)))); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)))); + } + + @Test + public void getDiff_removableLabelsScoreChanged() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("-1", ImmutableList.of(acc1)))); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)))); + } + + @Test + public void getDiff_removableLabelsLabelChanged() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Verified", ImmutableMap.of("+1", ImmutableList.of(acc1)))); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)))); + } + + @Test + public void getDiff_removableLabelsLabelScoreAndAccountChanged() { + ChangeInfo oldChangeInfo = new ChangeInfo(); + ChangeInfo newChangeInfo = new ChangeInfo(); + AccountInfo acc1 = new AccountInfo(); + acc1.name = "acc1"; + AccountInfo acc2 = new AccountInfo(); + acc2.name = "acc2"; + + oldChangeInfo.removableLabels = + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1))); + newChangeInfo.removableLabels = + ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2))); + + ChangeInfoDifference diff = ChangeInfoDiffer.getDifference(oldChangeInfo, newChangeInfo); + + assertThat(diff.added().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Verified", ImmutableMap.of("-1", ImmutableList.of(acc2)))); + assertThat(diff.removed().removableLabels) + .containsExactlyEntriesIn( + ImmutableMap.of("Code-Review", ImmutableMap.of("+1", ImmutableList.of(acc1)))); + } + + @Test public void getDiff_assertCanConstructAllChangeInfoReferences() throws Exception { buildObjectWithFullFields(ChangeInfo.class); }