opendev
/
gerrit
Code Issues Proposed changes

SubmitRules: discontinue permission checks 

Currently, a change could be submittable if:
1- It gets all required votes.
2- The reviewers don't lose their permissions for those votes.

This change adjusts the submit rules so that a change could be
submittable without condition 2, which means a change will still
be submittable even though its approvers lose their permissions
after voting.

Change-Id: I7eca7e88a954229d830e8eb4c61015004119bfe4
This commit is contained in:
Changcheng Xiao 2017-07-21 16:10:23 +02:00 committed by Dave Borowitz
parent ff5fbbfc08
commit cc43faa72b
4 changed files with 91 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -21,6 +21,8 @@ import static com.google.gerrit.extensions.api.changes.SubmittedTogetherOption.N
import static com.google.gerrit.reviewdb.client.Patch.COMMIT_MSG;
import static com.google.gerrit.reviewdb.client.Patch.MERGE_LIST;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.HEAD;
@ -37,6 +39,8 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.GerritApi;
@ -1398,4 +1402,18 @@ public abstract class AbstractDaemonTest {
config.getProject().setCreateNewChangeForAllNotInTarget(InheritableBoolean.TRUE);
saveProjectConfig(project, config);
}
protected void configLabel(String label, String func) throws Exception {
configLabel(
project, label, func, value(1, "Passes"), value(0, "No score"), value(-1, "Failed"));
}
protected void configLabel(
Project.NameKey project, String label, String func, LabelValue... value) throws Exception {
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
LabelType labelType = category(label, value);
labelType.setFunctionName(func);
cfg.getLabelSections().put(labelType.getName(), labelType);
saveProjectConfig(project, cfg);
}
}

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

@ -133,6 +133,7 @@ import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.update.BatchUpdate;
@ -3188,6 +3189,68 @@ public class ChangeIT extends AbstractDaemonTest {
gApi.changes().id(changeId).topic(topic);
}
@Test
public void submittableAfterLosingPermissions_MaxWithBlock() throws Exception {
configLabel("Label", "MaxWithBlock");
submittableAfterLosingPermissions("Label");
}
@Test
public void submittableAfterLosingPermissions_AnyWithBlock() throws Exception {
configLabel("Label", "AnyWithBlock");
submittableAfterLosingPermissions("Label");
}
public void submittableAfterLosingPermissions(String label) throws Exception {
String codeReviewLabel = "Code-Review";
ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
AccountGroup.UUID registered = SystemGroupBackend.REGISTERED_USERS;
Util.allow(cfg, Permission.forLabel(label), -1, +1, registered, "refs/heads/*");
Util.allow(cfg, Permission.forLabel(codeReviewLabel), -2, +2, registered, "refs/heads/*");
saveProjectConfig(cfg);
setApiUser(user);
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
// Verify user's permitted range.
ChangeInfo change = gApi.changes().id(changeId).get();
assertPermitted(change, label, -1, 0, 1);
assertPermitted(change, codeReviewLabel, -2, -1, 0, 1, 2);
ReviewInput input = new ReviewInput();
input.label(codeReviewLabel, 2);
input.label(label, 1);
gApi.changes().id(changeId).current().review(input);
assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().keySet())
.containsExactly(codeReviewLabel, label);
assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
.containsExactly((short) 2, (short) 1);
assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
setApiUser(admin);
// Remove user's permission for 'Label'.
Util.remove(cfg, Permission.forLabel(label), registered, "refs/heads/*");
// Update user's permitted range for 'Code-Review' to be -1...+1.
Util.remove(cfg, Permission.forLabel(codeReviewLabel), registered, "refs/heads/*");
Util.allow(cfg, Permission.forLabel(codeReviewLabel), -1, +1, registered, "refs/heads/*");
saveProjectConfig(cfg);
// Verify user's new permitted range.
setApiUser(user);
change = gApi.changes().id(changeId).get();
assertPermitted(change, label);
assertPermitted(change, codeReviewLabel, -1, 0, 1);
assertThat(gApi.changes().id(changeId).current().reviewer(user.email).votes().values())
.containsExactly((short) 2, (short) 1);
assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
setApiUser(admin);
gApi.changes().id(changeId).current().submit();
}
private String getCommitMessage(String changeId) throws RestApiException, IOException {
return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString();
}

10
gerrit-server/src/main/prolog/gerrit_common.pl
View File

@ -279,12 +279,12 @@ max_with_block(Min, Max, Label, label(Label, S)) :-
!,
max_with_block(Label, Min, Max, S).
max_with_block(Label, Min, Max, reject(Who)) :-
check_label_range_permission(Label, Min, ok(Who)),
commit_label(label(Label, Min), Who),
!
.
max_with_block(Label, Min, Max, ok(Who)) :-
\+ check_label_range_permission(Label, Min, ok(_)),
check_label_range_permission(Label, Max, ok(Who)),
\+ commit_label(label(Label, Min), _),
commit_label(label(Label, Max), Who),
!
.
max_with_block(Label, Min, Max, need(Max)) :-
@ -306,7 +306,7 @@ max_with_block(Label, Min, Max, need(Max)) :-
%%
any_with_block(Label, Min, reject(Who)) :-
Min < 0,
check_label_range_permission(Label, Min, ok(Who)),
commit_label(label(Label, Min), Who),
!
.
any_with_block(Label, Min, may(_)).
@ -321,7 +321,7 @@ max_no_block(Max, Label, label(Label, S)) :-
!,
max_no_block(Label, Max, S).
max_no_block(Label, Max, ok(Who)) :-
check_label_range_permission(Label, Max, ok(Who)),
commit_label(label(Label, Max), Who),
!
.
max_no_block(Label, Max, need(Max)) :-

10
gerrit-server/src/test/resources/com/google/gerrit/rules/gerrit_common_test.pl
View File

@ -65,7 +65,7 @@ skip_test(max_with_block_success_impossible) :-
test(default_submit_fails) :-
findall(P, default_submit(P), All),
All = [submit(C, V)],
C = label('Code-Review', ok(test_user(alice))),
C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
@ -84,7 +84,7 @@ test(can_submit_ok) :-
test(can_submit_not_ready) :-
can_submit(gerrit:default_submit, S),
S = not_ready(submit(C, V)),
C = label('Code-Review', ok(test_user(alice))),
C = label('Code-Review', ok(_)),
V = label('Verified', need(1)).
test(can_submit_only_verified_not_ready) :-
@ -99,7 +99,7 @@ test(filter_submit_remove_verified) :-
can_submit(gerrit:default_submit, R),
filter_submit_results(filter_out_v, [R], S),
S = [ok(submit(C))],
C = label('Code-Review', ok(test_user(alice))).
C = label('Code-Review', ok(_)).
test(filter_submit_add_code_review) :-
set_commit_labels([
@ -119,7 +119,7 @@ test(find_default_code_review) :-
can_submit(gerrit:default_submit, R),
arg(1, R, S),
find_label(S, 'Code-Review', L),
L = label('Code-Review', ok(test_user(alice))).
L = label('Code-Review', ok(_)).
test(find_default_verified) :-
can_submit(gerrit:default_submit, R),
@ -133,7 +133,7 @@ test(find_default_verified) :-
test(remove_default_code_review) :-
can_submit(gerrit:default_submit, R),
arg(1, R, S),
C = label('Code-Review', ok(test_user(alice))),
C = label('Code-Review', ok(_)),
remove_label(S, C, Out),
Out = submit(V),
V = label('Verified', need(1)).