Revert "Add project permission for removing votes/labels."
This reverts commit 9cb80417318290174eaa04aaaf395fb0b53415c1.
Reason for revert:
Three issues were identified with this change:
(1) Usage of ChangeNotes in a path used for change queries
causes exceptions. Addressed in I190a855144
(2) Addition of a new permission without a fallback or
migration plan causes callers to not be able to remove
votes anymore. WiP fix worked on in I75319a.
(3) Breakage of ChangeInfoDiffer fixed in I7d032f7365
The release of Gerrit at Google is quite old by now, so we
are rolling back changes that are release-blocking.
Forward-Compatible: checked
Change-Id: I119f75944156d94fbbb52cca1d4c8a108dd9deec
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 9a40b27..f2f5a66 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -800,22 +800,6 @@
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 8992f86..950049b 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -824,26 +824,6 @@
"+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,
@@ -7091,13 +7071,6 @@
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 ccf74d1..5b4a9e5 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1393,17 +1393,6 @@
}
}
- 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 69139ce..b1cd506 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java
@@ -197,13 +197,8 @@
PermissionRule.Builder rule = newRule(projectConfig, p.group());
rule.setAction(p.action());
rule.setRange(p.min(), p.max());
- String permissionName;
- if (p.isAddPermission()) {
- permissionName =
- p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name());
- } else {
- permissionName = Permission.forRemoveLabel(p.name());
- }
+ String permissionName =
+ p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(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 5634c78..9a9a21a 100644
--- a/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
+++ b/java/com/google/gerrit/acceptance/testsuite/project/TestProjectUpdate.java
@@ -162,34 +162,12 @@
/** Starts a builder for allowing a label permission. */
public static TestLabelPermission.Builder allowLabel(String name) {
- return TestLabelPermission.builder()
- .name(name)
- .isAddPermission(true)
- .action(PermissionRule.Action.ALLOW);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.ALLOW);
}
/** Starts a builder for denying a label permission. */
public static TestLabelPermission.Builder blockLabel(String name) {
- 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);
+ return TestLabelPermission.builder().name(name).action(PermissionRule.Action.BLOCK);
}
/** Records a label permission to be updated. */
@@ -213,8 +191,6 @@
abstract boolean impersonation();
- abstract boolean isAddPermission();
-
/** Builder for {@link TestLabelPermission}. */
@AutoValue.Builder
public abstract static class Builder {
@@ -232,8 +208,6 @@
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");
@@ -269,12 +243,6 @@
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 d029fad..6d2fa32 100644
--- a/java/com/google/gerrit/entities/Permission.java
+++ b/java/com/google/gerrit/entities/Permission.java
@@ -43,7 +43,6 @@
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";
@@ -61,7 +60,6 @@
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<>();
@@ -81,7 +79,6 @@
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());
@@ -96,19 +93,15 @@
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)
- || isRemoveLabel(varName)
- || NAMES_LC.contains(varName.toLowerCase());
+ return isLabel(varName) || isLabelAs(varName) || NAMES_LC.contains(varName.toLowerCase());
}
public static boolean hasRange(String varName) {
- return isLabel(varName) || isLabelAs(varName) || isRemoveLabel(varName);
+ return isLabel(varName) || isLabelAs(varName);
}
/** Returns true if the permission name is actually for a review label. */
@@ -121,11 +114,6 @@
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;
@@ -136,19 +124,12 @@
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;
}
@@ -224,8 +205,6 @@
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 a865187..40ae2ec 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -105,7 +105,6 @@
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/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index d26af7b..500bb77 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -688,7 +688,6 @@
!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 06e41ff..cfa15ae 100644
--- a/java/com/google/gerrit/server/change/LabelsJson.java
+++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -36,18 +36,15 @@
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.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -72,12 +69,10 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PermissionBackend permissionBackend;
- private final DeleteVoteControl deleteVoteControl;
@Inject
- LabelsJson(PermissionBackend permissionBackend, DeleteVoteControl deleteVoteControl) {
+ LabelsJson(PermissionBackend permissionBackend) {
this.permissionBackend = permissionBackend;
- this.deleteVoteControl = deleteVoteControl;
}
/**
@@ -138,45 +133,6 @@
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.notes(), approval, labelType.get())) {
- 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()) {
@@ -261,10 +217,10 @@
}
}
- private Map<String, Short> currentLabels(@Nullable Account.Id accountId, ChangeData cd) {
+ private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd) {
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa : cd.currentApprovals()) {
- if (accountId == null || psa.accountId().equals(accountId)) {
+ if (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
deleted file mode 100644
index 622f0cf..0000000
--- a/java/com/google/gerrit/server/permissions/AbstractLabelPermission.java
+++ /dev/null
@@ -1,155 +0,0 @@
-// 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 6f7d761..8d432c8 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 AbstractLabelPermission) {
- return can((AbstractLabelPermission) perm);
- } else if (perm instanceof AbstractLabelPermission.WithValue) {
- return can((AbstractLabelPermission.WithValue) perm);
+ } else if (perm instanceof LabelPermission) {
+ return can((LabelPermission) perm);
+ } else if (perm instanceof LabelPermission.WithValue) {
+ return can((LabelPermission.WithValue) perm);
}
throw new PermissionBackendException(perm + " unsupported");
}
@@ -288,11 +288,11 @@
throw new PermissionBackendException(perm + " unsupported");
}
- private boolean can(AbstractLabelPermission perm) {
+ private boolean can(LabelPermission perm) {
return !label(labelPermissionName(perm)).isEmpty();
}
- private boolean can(AbstractLabelPermission.WithValue perm) {
+ private boolean can(LabelPermission.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 f59ba02..2824efd 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 AbstractLabelPermission}. */
+/** A {@link ChangePermission} or a {@link LabelPermission}. */
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 89f0493..9d69d9b 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.AbstractLabelPermission.ForUser;
+import com.google.gerrit.server.permissions.LabelPermission.ForUser;
import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;
@@ -160,29 +160,19 @@
return Optional.ofNullable(CHANGE_PERMISSIONS.inverse().get(permissionName));
}
- 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());
+ public static String labelPermissionName(LabelPermission labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission subtype");
+ return Permission.forLabel(labelPermission.label());
}
// TODO(dborowitz): Can these share a common superinterface?
- 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());
+ public static String labelPermissionName(LabelPermission.WithValue labelPermission) {
+ if (labelPermission.forUser() == ForUser.ON_BEHALF_OF) {
+ return Permission.forLabelAs(labelPermission.label());
}
- throw new IllegalStateException("invalid AbstractLabelPermission.WithValue subtype");
+ return Permission.forLabel(labelPermission.label());
}
private DefaultPermissionMappings() {}
diff --git a/java/com/google/gerrit/server/permissions/LabelPermission.java b/java/com/google/gerrit/server/permissions/LabelPermission.java
index 4652364..c266caa 100644
--- a/java/com/google/gerrit/server/permissions/LabelPermission.java
+++ b/java/com/google/gerrit/server/permissions/LabelPermission.java
@@ -14,14 +14,24 @@
package com.google.gerrit.server.permissions;
-import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.SELF;
+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 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 extends AbstractLabelPermission {
+public class LabelPermission implements ChangePermissionOrLabel {
+ public enum ForUser {
+ SELF,
+ ON_BEHALF_OF;
+ }
+
+ private final ForUser forUser;
+ private final String name;
+
/**
* Construct a reference to a label permission.
*
@@ -57,16 +67,55 @@
* @param name name of the label, e.g. {@code "Code-Review"} or {@code "Verified"}.
*/
public LabelPermission(ForUser forUser, String name) {
- super(forUser, 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;
}
@Override
- public String permissionPrefix() {
- return "label";
+ 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 + ']';
}
/** A {@link LabelPermission} at a specific value. */
- public static class WithValue extends AbstractLabelPermission.WithValue {
+ public static class WithValue implements ChangePermissionOrLabel {
+ private final ForUser forUser;
+ private final LabelVote label;
+
/**
* Construct a reference to a label at a specific value.
*
@@ -146,12 +195,53 @@
* @param label label name and vote.
*/
public WithValue(ForUser forUser, LabelVote label) {
- super(forUser, 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();
}
@Override
- public String permissionName() {
- return "label";
+ 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() + ']';
}
}
}
diff --git a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java b/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
deleted file mode 100644
index 2553601..0000000
--- a/java/com/google/gerrit/server/permissions/LabelRemovalPermission.java
+++ /dev/null
@@ -1,94 +0,0 @@
-// 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 eb5e053..fea2827 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -474,18 +474,6 @@
}
/**
- * 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.
@@ -498,29 +486,10 @@
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
deleted file mode 100644
index 93c0451..0000000
--- a/java/com/google/gerrit/server/project/DeleteVoteControl.java
+++ /dev/null
@@ -1,83 +0,0 @@
-// 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.extensions.restapi.AuthException;
-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.inject.Inject;
-import java.util.Set;
-
-public class DeleteVoteControl {
- private final PermissionBackend permissionBackend;
-
- @Inject
- public DeleteVoteControl(PermissionBackend permissionBackend) {
- this.permissionBackend = permissionBackend;
- }
-
- public void checkDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws AuthException, PermissionBackendException {
- if (testDeleteVotePermissions(user, notes, approval, labelType)) {
- return;
- }
- throw new AuthException(
- new LabelRemovalPermission.WithValue(labelType, approval.value()).describeForException()
- + " not permitted");
- }
-
- public boolean testDeleteVotePermissions(
- CurrentUser user, ChangeNotes notes, PatchSetApproval approval, LabelType labelType)
- throws PermissionBackendException {
- if (canRemoveReviewerWithoutRemoveLabelPermission(
- notes.getChange(), 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(notes).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/restapi/change/DeleteVoteOp.java b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
index 239b485..a0fc121 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVoteOp.java
@@ -38,8 +38,8 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
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;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
@@ -75,7 +75,7 @@
private final VoteDeleted voteDeleted;
private final DeleteVoteSender.Factory deleteVoteSenderFactory;
- private final DeleteVoteControl deleteVoteControl;
+ private final RemoveReviewerControl removeReviewerControl;
private final MessageIdGenerator messageIdGenerator;
private final String label;
@@ -96,7 +96,7 @@
ChangeMessagesUtil cmUtil,
VoteDeleted voteDeleted,
DeleteVoteSender.Factory deleteVoteSenderFactory,
- DeleteVoteControl deleteVoteControl,
+ RemoveReviewerControl removeReviewerControl,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey projectName,
@Assisted AccountState reviewerToDeleteVoteFor,
@@ -109,7 +109,7 @@
this.cmUtil = cmUtil;
this.voteDeleted = voteDeleted;
this.deleteVoteSenderFactory = deleteVoteSenderFactory;
- this.deleteVoteControl = deleteVoteControl;
+ this.removeReviewerControl = removeReviewerControl;
this.messageIdGenerator = messageIdGenerator;
this.projectName = projectName;
@@ -143,8 +143,12 @@
newApprovals.put(a.label(), a.value());
continue;
} else if (enforcePermissions) {
- deleteVoteControl.checkDeleteVotePermissions(
- ctx.getUser(), ctx.getNotes(), a, labelTypes.byLabel(a.labelId()).get());
+ // 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);
+ }
}
// Set the approval to 0 if vote is being removed.
newApprovals.put(a.label(), (short) 0);
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a8ba052..9a6b03e 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.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
+import static com.google.gerrit.server.permissions.LabelPermission.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 92d19bb..211baeb 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -26,10 +26,8 @@
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.labelPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.entities.RefNames.changeMetaRef;
@@ -2268,16 +2266,6 @@
@Test
public void deleteVote() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2321,16 +2309,6 @@
@Test
public void deleteVoteNotifyNone() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2348,16 +2326,6 @@
@Test
public void deleteVoteWithReason() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2379,16 +2347,6 @@
@Test
public void deleteVoteNotifyAccount() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
@@ -2448,66 +2406,7 @@
.id(r.getChangeId())
.reviewer(admin.id().toString())
.deleteVote(LabelId.CODE_REVIEW));
- assertThat(thrown).hasMessageThat().contains("removeLabel Code-Review=+2 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))
- .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))
- .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);
+ assertThat(thrown).hasMessageThat().contains("delete vote not permitted");
}
@Test
@@ -3376,7 +3275,6 @@
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();
@@ -3391,7 +3289,6 @@
.project(project)
.forUpdate()
.add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
- .add(allowLabelRemoval(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
.update();
change = gApi.changes().id(r.getChangeId()).get();
@@ -3407,9 +3304,6 @@
.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
@@ -3429,7 +3323,6 @@
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.
@@ -3438,7 +3331,6 @@
assertThat(change.status).isEqualTo(ChangeStatus.ABANDONED);
assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
assertThat(change.permittedLabels).isEmpty();
- assertThat(change.removableLabels).isEmpty();
}
@Test
@@ -3555,7 +3447,6 @@
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
@@ -3573,7 +3464,6 @@
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
@@ -3670,7 +3560,6 @@
.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
@@ -3686,7 +3575,6 @@
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
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e707949..0291f33 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -22,7 +22,6 @@
import static com.google.gerrit.acceptance.PushOneCommit.PATCH_FILE_ONLY;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
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.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
@@ -235,15 +234,6 @@
revision(r).review(ReviewInput.recommend());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes().id(changeId).reviewer(user.username()).deleteVote(LabelId.CODE_REVIEW);
Optional<ApprovalInfo> crUser =
get(changeId, DETAILED_LABELS).labels.get(LabelId.CODE_REVIEW).all.stream()
@@ -2011,15 +2001,6 @@
recommend(r.getChangeId());
requestScopeOperations.setApiUser(admin.id());
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
gApi.changes()
.id(r.getChangeId())
.current()
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index 7e8ff62..ea52690 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
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.extensions.restapi.testing.AttentionSetUpdateSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -1945,16 +1944,6 @@
@Test
public void deleteVotesDoesNotAffectAttentionSetWhenIgnoreAutomaticRulesIsSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
@@ -1979,16 +1968,6 @@
@Test
public void deleteVotesOfOthersAddThemToAttentionSet() throws Exception {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
-
PushOneCommit.Result r = createChange();
requestScopeOperations.setApiUser(user.id());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
index c29b265..c57d285 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -15,16 +15,13 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabelRemoval;
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.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.LabelId;
@@ -39,26 +36,11 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
-import org.junit.Before;
import org.junit.Test;
public class DeleteVoteIT extends AbstractDaemonTest {
- @Inject private ProjectOperations projectOperations;
@Inject private RequestScopeOperations requestScopeOperations;
- @Before
- public void allowVoteDeletion() {
- projectOperations
- .project(project)
- .forUpdate()
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/heads/*")
- .group(REGISTERED_USERS)
- .range(-2, 2))
- .update();
- }
-
@Test
public void deleteVoteOnChange() throws Exception {
deleteVote(false);
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
index b1277c0..b68afc5 100644
--- a/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/mail/ChangeNotificationsIT.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
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.entities.NotifyConfig.NotifyType.ABANDONED_CHANGES;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.ALL_COMMENTS;
import static com.google.gerrit.entities.NotifyConfig.NotifyType.NEW_CHANGES;
@@ -99,11 +98,6 @@
.add(allow(Permission.SUBMIT_AS).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.ABANDON).ref("refs/*").group(REGISTERED_USERS))
.add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, +2))
- .add(
- allowLabelRemoval(LabelId.CODE_REVIEW)
- .ref("refs/*")
- .group(REGISTERED_USERS)
- .range(-2, +2))
.update();
}
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
index 661802e..c1a7627 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImplTest.java
@@ -18,14 +18,11 @@
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;
@@ -446,73 +443,6 @@
}
@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)
@@ -610,31 +540,6 @@
}
@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 d25d833..3175671 100644
--- a/javatests/com/google/gerrit/entities/PermissionTest.java
+++ b/javatests/com/google/gerrit/entities/PermissionTest.java
@@ -36,7 +36,6 @@
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();
}
@@ -57,7 +56,6 @@
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();
}
@@ -68,22 +66,10 @@
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);
@@ -96,19 +82,11 @@
}
@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();
}
@@ -125,10 +103,6 @@
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();
@@ -139,10 +113,6 @@
Permission.canBeOnAllProjects(
"refs/heads/*", Permission.LABEL_AS + LabelId.CODE_REVIEW))
.isTrue();
- assertThat(
- Permission.canBeOnAllProjects(
- "refs/heads/*", Permission.REMOVE_LABEL + LabelId.CODE_REVIEW))
- .isTrue();
}
@Test
@@ -156,8 +126,6 @@
.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();
}
