diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt index 3f6acf6cc9..121fcadd61 100644 --- a/Documentation/cmd-review.txt +++ b/Documentation/cmd-review.txt @@ -124,12 +124,6 @@ branch. or invalid value) and votes that are not permitted for the user are silently ignored. ---strict-labels:: - Require ability to vote on all specified labels before reviewing change. - If the vote is invalid (invalid label or invalid name), the vote is not - permitted for the user, or the vote is on an outdated or closed patch set, - return an error instead of silently discarding the vote. - --tag:: -t:: Apply a 'TAG' to the change message, votes, and inline comments. The 'TAG' diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 50afe40004..d9e356d2b7 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -6693,13 +6693,6 @@ list of link:#comment-input[CommentInput] entities. |`robot_comments` |optional| The robot comments that should be added as a map that maps a file path to a list of link:#robot-comment-input[RobotCommentInput] entities. -|`strict_labels` |`true` if not set| -Whether all labels are required to be within the user's permitted ranges -based on access controls. + -If `true`, attempting to use a label not granted to the user will fail -the entire modify operation early. + -If `false`, the operation will execute anyway, but the proposed labels -will be modified to be the "best" value allowed by the access controls. |`drafts` |optional| Draft handling that defines how draft comments are handled that are already in the database but that were not also described in this diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index baa0a68cf2..eef9d87579 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -1582,7 +1582,6 @@ public class ChangeIT extends AbstractDaemonTest { // Exact request format made by GWT UI at ddc6b7160fe416fed9e7e3180489d44c82fd64f8. ReviewInput in = new ReviewInput(); in.labels = ImmutableMap.of("Code-Review", (short) 0); - in.strictLabels = true; in.drafts = DraftHandling.PUBLISH_ALL_REVISIONS; in.message = "comment"; gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java index 36843a5e07..9b88e0d004 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ImpersonationIT.java @@ -146,7 +146,6 @@ public class ImpersonationIT extends AbstractDaemonTest { ReviewInput in = new ReviewInput(); in.onBehalfOf = user.id.toString(); - in.strictLabels = true; in.label("Not-A-Label", 5); exception.expect(BadRequestException.class); @@ -154,23 +153,6 @@ public class ImpersonationIT extends AbstractDaemonTest { revision.review(in); } - @Test - public void voteOnBehalfOfInvalidLabelIgnoredWithoutStrictLabels() throws Exception { - allowCodeReviewOnBehalfOf(); - PushOneCommit.Result r = createChange(); - RevisionApi revision = gApi.changes().id(r.getChangeId()).current(); - - ReviewInput in = new ReviewInput(); - in.onBehalfOf = user.id.toString(); - in.strictLabels = false; - in.label("Code-Review", 1); - in.label("Not-A-Label", 5); - - revision.review(in); - - assertThat(gApi.changes().id(r.getChangeId()).get().labels).doesNotContainKey("Not-A-Label"); - } - @Test public void voteOnBehalfOfLabelNotPermitted() throws Exception { ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java index 113651beca..f851d5e72e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ReviewInput.java @@ -60,7 +60,6 @@ public class ReviewInput extends JavaScriptObject { private native void init() /*-{ this.labels = {}; - this.strict_labels = true; }-*/; public final native void prePost() /*-{ diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 58634a5a52..0022656f7a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -233,7 +233,7 @@ public class PostReview input.drafts = DraftHandling.DELETE; } if (input.labels != null) { - checkLabels(revision, labelTypes, input.strictLabels, input.labels); + checkLabels(revision, labelTypes, input.labels); } if (input.comments != null) { cleanUpComments(input.comments); @@ -443,12 +443,9 @@ public class PostReview while (itr.hasNext()) { Map.Entry ent = itr.next(); LabelType type = labelTypes.byLabel(ent.getKey()); - if (type == null && in.strictLabels) { + if (type == null) { throw new BadRequestException( String.format("label \"%s\" is not a configured label", ent.getKey())); - } else if (type == null) { - itr.remove(); - continue; } if (!caller.isInternalUser()) { @@ -479,8 +476,7 @@ public class PostReview changeResourceFactory.create(rev.getNotes(), reviewer), rev.getPatchSet()); } - private void checkLabels( - RevisionResource rsrc, LabelTypes labelTypes, boolean strict, Map labels) + private void checkLabels(RevisionResource rsrc, LabelTypes labelTypes, Map labels) throws BadRequestException, AuthException, PermissionBackendException { PermissionBackend.ForChange perm = rsrc.permissions(); Iterator> itr = labels.entrySet().iterator(); @@ -488,12 +484,8 @@ public class PostReview Map.Entry ent = itr.next(); LabelType lt = labelTypes.byLabel(ent.getKey()); if (lt == null) { - if (strict) { - throw new BadRequestException( - String.format("label \"%s\" is not a configured label", ent.getKey())); - } - itr.remove(); - continue; + throw new BadRequestException( + String.format("label \"%s\" is not a configured label", ent.getKey())); } if (ent.getValue() == null || ent.getValue() == 0) { @@ -503,23 +495,16 @@ public class PostReview } if (lt.getValue(ent.getValue()) == null) { - if (strict) { - throw new BadRequestException( - String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue())); - } - itr.remove(); - continue; + throw new BadRequestException( + String.format("label \"%s\": %d is not a valid value", ent.getKey(), ent.getValue())); } short val = ent.getValue(); try { perm.check(new LabelPermission.WithValue(lt, val)); } catch (AuthException e) { - if (strict) { - throw new AuthException( - String.format("Applying label \"%s\": %d is restricted", lt.getName(), val)); - } - ent.setValue(perm.squashThenCheck(lt, val)); + throw new AuthException( + String.format("Applying label \"%s\": %d is restricted", lt.getName(), val)); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java index 1917d6f7e4..785ae38a58 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java @@ -79,7 +79,7 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate { for (PatchSetApproval p : object.currentApprovals()) { if (labelType.matches(p)) { hasVote = true; - if (match(object, p.getValue(), p.getAccountId(), labelType)) { + if (match(object, p.getValue(), p.getAccountId())) { return true; } } @@ -105,7 +105,7 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate { return null; } - protected boolean match(ChangeData cd, short value, Account.Id approver, LabelType type) { + protected boolean match(ChangeData cd, short value, Account.Id approver) { if (value != expVal) { return false; } @@ -119,11 +119,11 @@ public class EqualsLabelPredicate extends ChangeIndexPredicate { return false; } - // Double check the value is still permitted for the user. + // Check the user has 'READ' permission. try { PermissionBackend.ForChange perm = permissionBackend.user(reviewer).database(dbProvider).change(cd); - return perm.test(ChangePermission.READ) && expVal == perm.squashByTest(type, value); + return perm.test(ChangePermission.READ); } catch (PermissionBackendException e) { return false; } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index 07648fa3f8..1d764b9b90 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -133,12 +133,6 @@ public class ReviewCommand extends SshCommand { @Option(name = "--json", aliases = "-j", usage = "read review input json from stdin") private boolean json; - @Option( - name = "--strict-labels", - usage = "Strictly check if the labels specified can be applied to the given patch set(s)" - ) - private boolean strictLabels; - @Option( name = "--tag", aliases = "-t", @@ -273,7 +267,6 @@ public class ReviewCommand extends SshCommand { review.notify = notify; review.labels = new TreeMap<>(); review.drafts = ReviewInput.DraftHandling.PUBLISH; - review.strictLabels = strictLabels; for (ApproveOption ao : optionList) { Short v = ao.value(); if (v != null) {