Disallow current_user/1 predicate in submit type rules
Submit type is used as part of the mergeability bit computation, which
is persistent and so needs a consistent view of submit type. An
informal poll of repo-discuss[1] indicated that current_user/1 is
pretty much only useful for submit rules, not submit type rules.
[1] https://groups.google.com/d/topic/repo-discuss/vW6XhUOkqik/discussion
Change-Id: Ia938a685409025b36e16979fcafd92afbed2d91f
diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
index 356b7d5..c7c94f8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
@@ -25,6 +25,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
@@ -50,7 +51,13 @@
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<ChangeData> CHANGE_DATA = create(ChangeData.class);
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
+
+ // Note: no guarantees are made about the user passed in the ChangeControl; do
+ // not depend on this directly. Either use .forUser(otherUser) to get a
+ // control for a specific known user, or use CURRENT_USER, which may be null
+ // for rule types that may not depend on the current user.
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);
+ public static final StoredValue<CurrentUser> CURRENT_USER = create(CurrentUser.class);
public static Change getChange(Prolog engine) throws SystemException {
ChangeData cd = CHANGE_DATA.get(engine);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
index 48c67ed..54a95f1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -28,6 +28,7 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.StoredValues;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -209,7 +210,8 @@
List<Term> results;
try {
results = evaluateImpl("locate_submit_rule", "can_submit",
- "locate_submit_filter", "filter_submit_results");
+ "locate_submit_filter", "filter_submit_results",
+ control.getCurrentUser());
} catch (RuleEvalException e) {
return ruleError(e.getMessage(), e);
}
@@ -386,7 +388,11 @@
List<Term> results;
try {
results = evaluateImpl("locate_submit_type", "get_submit_type",
- "locate_submit_type_filter", "filter_submit_type_results");
+ "locate_submit_type_filter", "filter_submit_type_results",
+ // Do not include current user in submit type evaluation. This is used
+ // for mergeability checks, which are stored persistently and so must
+ // have a consistent view of the submit type.
+ null);
} catch (RuleEvalException e) {
return typeError(e.getMessage(), e);
}
@@ -436,8 +442,9 @@
String userRuleLocatorName,
String userRuleWrapperName,
String filterRuleLocatorName,
- String filterRuleWrapperName) throws RuleEvalException {
- PrologEnvironment env = getPrologEnvironment();
+ String filterRuleWrapperName,
+ CurrentUser user) throws RuleEvalException {
+ PrologEnvironment env = getPrologEnvironment(user);
try {
Term sr = env.once("gerrit", userRuleLocatorName, new VariableTerm());
if (fastEvalLabels) {
@@ -479,7 +486,8 @@
}
}
- private PrologEnvironment getPrologEnvironment() throws RuleEvalException {
+ private PrologEnvironment getPrologEnvironment(CurrentUser user)
+ throws RuleEvalException {
checkState(patchSet != null,
"getPrologEnvironment() called before initPatchSet()");
ProjectState projectState = control.getProjectControl().getProjectState();
@@ -499,6 +507,9 @@
env.set(StoredValues.CHANGE_DATA, cd);
env.set(StoredValues.PATCH_SET, patchSet);
env.set(StoredValues.CHANGE_CONTROL, control);
+ if (user != null) {
+ env.set(StoredValues.CURRENT_USER, user);
+ }
return env;
}
diff --git a/gerrit-server/src/main/java/gerrit/PRED_current_user_1.java b/gerrit-server/src/main/java/gerrit/PRED_current_user_1.java
index dd2a008..6d0dd0f 100644
--- a/gerrit-server/src/main/java/gerrit/PRED_current_user_1.java
+++ b/gerrit-server/src/main/java/gerrit/PRED_current_user_1.java
@@ -20,7 +20,6 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PeerDaemonUser;
-import com.google.gerrit.server.project.ChangeControl;
import com.googlecode.prolog_cafe.lang.EvaluationException;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
@@ -47,8 +46,11 @@
engine.setB0();
Term a1 = arg1.dereference();
- ChangeControl cControl = StoredValues.CHANGE_CONTROL.get(engine);
- CurrentUser curUser = cControl.getCurrentUser();
+ CurrentUser curUser = StoredValues.CURRENT_USER.getOrNull(engine);
+ if (curUser == null) {
+ throw new EvaluationException(
+ "Current user not available in this rule type");
+ }
Term resultTerm;
if (curUser.isIdentifiedUser()) {
@@ -67,4 +69,4 @@
}
return cont;
}
-}
\ No newline at end of file
+}
