From 0e7f5769fd11fa0a0dfcf665e723f4589e00eb19 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 17 Oct 2014 09:02:02 -0700 Subject: [PATCH] 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 --- .../com/google/gerrit/rules/StoredValues.java | 7 +++++++ .../server/project/SubmitRuleEvaluator.java | 21 ++++++++++++++----- .../main/java/gerrit/PRED_current_user_1.java | 10 +++++---- 3 files changed, 29 insertions(+), 9 deletions(-) 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 356b7d5b92..c7c94f81b4 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.PatchSetInfo; 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 final class StoredValues { public static final StoredValue REVIEW_DB = create(ReviewDb.class); public static final StoredValue CHANGE_DATA = create(ChangeData.class); public static final StoredValue 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 CHANGE_CONTROL = create(ChangeControl.class); + public static final StoredValue 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 48c67edad4..54a95f160f 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.Change; 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 @@ public class SubmitRuleEvaluator { List 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 @@ public class SubmitRuleEvaluator { List 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 @@ public class SubmitRuleEvaluator { 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 @@ public class SubmitRuleEvaluator { } } - 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 @@ public class SubmitRuleEvaluator { 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 dd2a008894..6d0dd0f8df 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.AnonymousUser; 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 @@ public class PRED_current_user_1 extends Predicate.P1 { 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 @@ public class PRED_current_user_1 extends Predicate.P1 { } return cont; } -} \ No newline at end of file +}