From 1feb1d19149f3a508cdabf45fb112f0ea294dd4c Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sun, 19 Feb 2017 14:28:18 -0800 Subject: [PATCH] Verify labels in Prolog using PermissionBackend user_label_range/4 needs to compute the min and max values, so this predicate uses test(LabelType) to determine the set of allowed values and picks the min and max. The new check_user_label/3 takes a specific value and uses the backend's check method to assert the label can be set to that value. This improves logic at submit time to use check just before the change is submitted to the destination branch. Change-Id: Ia29f16ffc50712ed1b57ec964b9e7930b35a5673 --- .../gerrit/rules/PrologEnvironment.java | 8 ++ .../com/google/gerrit/rules/StoredValues.java | 10 ++ .../server/query/change/ChangeData.java | 5 + .../java/gerrit/PRED__check_user_label_3.java | 104 ++++++++++++++++++ .../gerrit/PRED__load_commit_labels_1.java | 2 +- .../java/gerrit/PRED__user_label_range_4.java | 38 +++++-- .../gerrit/PRED_get_legacy_label_types_1.java | 2 +- .../src/main/prolog/gerrit_common.pl | 24 +++- .../google/gerrit/rules/GerritCommonTest.java | 13 ++- 9 files changed, 185 insertions(+), 21 deletions(-) create mode 100644 gerrit-server/src/main/java/gerrit/PRED__check_user_label_3.java diff --git a/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java b/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java index 9538121a41..23c59f5359 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/rules/PrologEnvironment.java @@ -20,6 +20,7 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchSetInfoFactory; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ProjectCache; import com.google.inject.Inject; import com.google.inject.Provider; @@ -166,6 +167,7 @@ public class PrologEnvironment extends BufferingPrologControl { } private final ProjectCache projectCache; + private final PermissionBackend permissionBackend; private final GitRepositoryManager repositoryManager; private final PatchListCache patchListCache; private final PatchSetInfoFactory patchSetInfoFactory; @@ -177,6 +179,7 @@ public class PrologEnvironment extends BufferingPrologControl { @Inject Args( ProjectCache projectCache, + PermissionBackend permissionBackend, GitRepositoryManager repositoryManager, PatchListCache patchListCache, PatchSetInfoFactory patchSetInfoFactory, @@ -184,6 +187,7 @@ public class PrologEnvironment extends BufferingPrologControl { Provider anonymousUser, @GerritServerConfig Config config) { this.projectCache = projectCache; + this.permissionBackend = permissionBackend; this.repositoryManager = repositoryManager; this.patchListCache = patchListCache; this.patchSetInfoFactory = patchSetInfoFactory; @@ -213,6 +217,10 @@ public class PrologEnvironment extends BufferingPrologControl { return projectCache; } + public PermissionBackend getPermissionBackend() { + return permissionBackend; + } + public GitRepositoryManager getGitRepositoryManager() { return repositoryManager; } 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 98ec56975d..e35171b076 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 @@ -33,6 +33,7 @@ import com.google.gerrit.server.patch.PatchListKey; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -130,6 +131,15 @@ public final class StoredValues { } }; + public static final StoredValue PERMISSION_BACKEND = + new StoredValue() { + @Override + protected PermissionBackend createValue(Prolog engine) { + PrologEnvironment env = (PrologEnvironment) engine.control; + return env.getArgs().getPermissionBackend(); + } + }; + public static final StoredValue ANONYMOUS_USER = new StoredValue() { @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 1dbe5cd28f..64ef09160a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -33,6 +33,7 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.reviewdb.client.Account; @@ -757,6 +758,10 @@ public class ChangeData { return change; } + public LabelTypes getLabelTypes() { + return changeControl.getLabelTypes(); + } + public ChangeNotes notes() throws OrmException { if (notes == null) { if (!lazyLoad) { diff --git a/gerrit-server/src/main/java/gerrit/PRED__check_user_label_3.java b/gerrit-server/src/main/java/gerrit/PRED__check_user_label_3.java new file mode 100644 index 0000000000..b2b9890c1d --- /dev/null +++ b/gerrit-server/src/main/java/gerrit/PRED__check_user_label_3.java @@ -0,0 +1,104 @@ +// Copyright (C) 2017 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 gerrit; + +import com.google.gerrit.common.data.LabelType; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.rules.StoredValues; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.permissions.LabelPermission; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.query.change.ChangeData; +import com.googlecode.prolog_cafe.exceptions.IllegalTypeException; +import com.googlecode.prolog_cafe.exceptions.PInstantiationException; +import com.googlecode.prolog_cafe.exceptions.PrologException; +import com.googlecode.prolog_cafe.exceptions.SystemException; +import com.googlecode.prolog_cafe.lang.IntegerTerm; +import com.googlecode.prolog_cafe.lang.JavaObjectTerm; +import com.googlecode.prolog_cafe.lang.Operation; +import com.googlecode.prolog_cafe.lang.Predicate; +import com.googlecode.prolog_cafe.lang.Prolog; +import com.googlecode.prolog_cafe.lang.SymbolTerm; +import com.googlecode.prolog_cafe.lang.Term; +import com.googlecode.prolog_cafe.lang.VariableTerm; + +/** + * Checks user can set label to val. + * + * 
+ *   '_check_user_label'(+Label, +CurrentUser, +Val)
+ *
+ */ +class PRED__check_user_label_3 extends Predicate.P3 { + PRED__check_user_label_3(Term a1, Term a2, Term a3, Operation n) { + arg1 = a1; + arg2 = a2; + arg3 = a3; + cont = n; + } + + @Override + public Operation exec(Prolog engine) throws PrologException { + engine.setB0(); + Term a1 = arg1.dereference(); + Term a2 = arg2.dereference(); + Term a3 = arg3.dereference(); + + if (a1 instanceof VariableTerm) { + throw new PInstantiationException(this, 1); + } + if (!(a1 instanceof SymbolTerm)) { + throw new IllegalTypeException(this, 1, "atom", a1); + } + String label = a1.name(); + + if (a2 instanceof VariableTerm) { + throw new PInstantiationException(this, 2); + } + if (!(a2 instanceof JavaObjectTerm) || !a2.convertible(CurrentUser.class)) { + throw new IllegalTypeException(this, 2, "CurrentUser)", a2); + } + CurrentUser user = (CurrentUser) ((JavaObjectTerm) a2).object(); + + if (a3 instanceof VariableTerm) { + throw new PInstantiationException(this, 3); + } + if (!(a3 instanceof IntegerTerm)) { + throw new IllegalTypeException(this, 3, "integer", a3); + } + short val = (short) ((IntegerTerm) a3).intValue(); + + ChangeData cd = StoredValues.CHANGE_DATA.get(engine); + LabelType type = cd.getLabelTypes().byLabel(label); + if (type == null) { + return engine.fail(); + } + + try { + StoredValues.PERMISSION_BACKEND + .get(engine) + .user(user) + .change(cd) + .check(new LabelPermission.WithValue(type, val)); + return cont; + } catch (AuthException err) { + return engine.fail(); + } catch (PermissionBackendException err) { + SystemException se = new SystemException(err.getMessage()); + se.initCause(err); + throw se; + } + } +} diff --git a/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java b/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java index 8b5a33d576..5a3d6566ce 100644 --- a/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED__load_commit_labels_1.java @@ -38,7 +38,7 @@ class PRED__load_commit_labels_1 extends Predicate.P1 { Term listHead = Prolog.Nil; try { ChangeData cd = StoredValues.CHANGE_DATA.get(engine); - LabelTypes types = StoredValues.CHANGE_CONTROL.get(engine).getLabelTypes(); + LabelTypes types = cd.getLabelTypes(); for (PatchSetApproval a : cd.currentApprovals()) { LabelType t = types.byLabel(a.getLabelId()); diff --git a/gerrit-server/src/main/java/gerrit/PRED__user_label_range_4.java b/gerrit-server/src/main/java/gerrit/PRED__user_label_range_4.java index d06664ecc9..5c61007edf 100644 --- a/gerrit-server/src/main/java/gerrit/PRED__user_label_range_4.java +++ b/gerrit-server/src/main/java/gerrit/PRED__user_label_range_4.java @@ -14,14 +14,16 @@ package gerrit; -import com.google.gerrit.common.data.Permission; -import com.google.gerrit.common.data.PermissionRange; +import com.google.gerrit.common.data.LabelType; import com.google.gerrit.rules.StoredValues; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.permissions.LabelPermission; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.query.change.ChangeData; import com.googlecode.prolog_cafe.exceptions.IllegalTypeException; import com.googlecode.prolog_cafe.exceptions.PInstantiationException; import com.googlecode.prolog_cafe.exceptions.PrologException; +import com.googlecode.prolog_cafe.exceptions.SystemException; import com.googlecode.prolog_cafe.lang.IntegerTerm; import com.googlecode.prolog_cafe.lang.JavaObjectTerm; import com.googlecode.prolog_cafe.lang.Operation; @@ -30,12 +32,13 @@ import com.googlecode.prolog_cafe.lang.Prolog; import com.googlecode.prolog_cafe.lang.SymbolTerm; import com.googlecode.prolog_cafe.lang.Term; import com.googlecode.prolog_cafe.lang.VariableTerm; +import java.util.Set; /** * Resolves the valid range for a label on a CurrentUser. * * 
- *   '$user_label_range'(+Label, +CurrentUser, -Min, -Max)
+ *   '_user_label_range'(+Label, +CurrentUser, -Min, -Max)
  *
*/ class PRED__user_label_range_4 extends Predicate.P4 { @@ -71,20 +74,33 @@ class PRED__user_label_range_4 extends Predicate.P4 { } CurrentUser user = (CurrentUser) ((JavaObjectTerm) a2).object(); - ChangeControl ctl = StoredValues.CHANGE_CONTROL.get(engine).forUser(user); - PermissionRange range = ctl.getRange(Permission.LABEL + label); - if (range == null) { + ChangeData cd = StoredValues.CHANGE_DATA.get(engine); + LabelType type = cd.getLabelTypes().byLabel(label); + if (type == null) { return engine.fail(); } - IntegerTerm min = new IntegerTerm(range.getMin()); - IntegerTerm max = new IntegerTerm(range.getMax()); + Set can; + try { + can = StoredValues.PERMISSION_BACKEND.get(engine).user(user).change(cd).test(type); + } catch (PermissionBackendException err) { + SystemException se = new SystemException(err.getMessage()); + se.initCause(err); + throw se; + } - if (!a3.unify(min, engine.trail)) { + int min = 0; + int max = 0; + for (LabelPermission.WithValue v : can) { + min = Math.min(min, v.value()); + max = Math.max(max, v.value()); + } + + if (!a3.unify(new IntegerTerm(min), engine.trail)) { return engine.fail(); } - if (!a4.unify(max, engine.trail)) { + if (!a4.unify(new IntegerTerm(max), engine.trail)) { return engine.fail(); } diff --git a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java index ea3fb174fd..33d63c400f 100644 --- a/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java +++ b/gerrit-server/src/main/java/gerrit/PRED_get_legacy_label_types_1.java @@ -51,7 +51,7 @@ class PRED_get_legacy_label_types_1 extends Predicate.P1 { public Operation exec(Prolog engine) throws PrologException { engine.setB0(); Term a1 = arg1.dereference(); - List list = StoredValues.CHANGE_CONTROL.get(engine).getLabelTypes().getLabelTypes(); + List list = StoredValues.CHANGE_DATA.get(engine).getLabelTypes().getLabelTypes(); Term head = Prolog.Nil; for (int idx = list.size() - 1; 0 <= idx; idx--) { head = new ListTerm(export(list.get(idx)), head); diff --git a/gerrit-server/src/main/prolog/gerrit_common.pl b/gerrit-server/src/main/prolog/gerrit_common.pl index 59c926fc3a..4671e0d134 100644 --- a/gerrit-server/src/main/prolog/gerrit_common.pl +++ b/gerrit-server/src/main/prolog/gerrit_common.pl @@ -90,6 +90,27 @@ index_commit_labels([_ | Rs]) :- index_commit_labels(Rs). +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +%% +%% check_user_label/3: +%% +%% Check Who can set Label to Val. +%% +check_user_label(Label, Who, Val) :- + hash_get(commit_labels, '$fast_range', true), !, + atom(Label), + assume_range_from_label(Label, Who, Min, Max), + Min @=< Val, Val @=< Max. +check_user_label(Label, Who, Val) :- + Who = user(_), !, + atom(Label), + current_user(Who, User), + '_check_user_label'(Label, User, Val). +check_user_label(Label, test_user(Name), Val) :- + clause(user:test_grant(Label, test_user(Name), range(Min, Max)), _), + Min @=< Val, Val @=< Max + . + %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% %% user_label_range/4: @@ -319,8 +340,7 @@ max_no_block(Label, Max, need(Max)) :- %% check_label_range_permission(Label, ExpValue, ok(Who)) :- commit_label(label(Label, ExpValue), Who), - user_label_range(Label, Who, Min, Max), - Min @=< ExpValue, ExpValue @=< Max + check_user_label(Label, Who, ExpValue) . %TODO Uncomment this clause when group suggesting is possible. %check_label_range_permission(Label, ExpValue, ask(Group)) :- diff --git a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java index fa4a9510ba..3dfcaec2a3 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/rules/GerritCommonTest.java @@ -17,8 +17,8 @@ package com.google.gerrit.rules; import static org.easymock.EasyMock.expect; import com.google.gerrit.common.data.LabelTypes; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.Util; +import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.AbstractModule; import com.googlecode.prolog_cafe.exceptions.CompileException; import com.googlecode.prolog_cafe.exceptions.ReductionLimitException; @@ -47,7 +47,8 @@ public class GerritCommonTest extends PrologTestCase { cfg.setInt("rules", null, "reductionLimit", 1300); cfg.setInt("rules", null, "compileReductionLimit", (int) 1e6); bind(PrologEnvironment.Args.class) - .toInstance(new PrologEnvironment.Args(null, null, null, null, null, null, cfg)); + .toInstance( + new PrologEnvironment.Args(null, null, null, null, null, null, null, cfg)); } }); } @@ -55,10 +56,10 @@ public class GerritCommonTest extends PrologTestCase { @Override protected void setUpEnvironment(PrologEnvironment env) { LabelTypes labelTypes = new LabelTypes(Arrays.asList(Util.codeReview(), Util.verified())); - ChangeControl ctl = EasyMock.createMock(ChangeControl.class); - expect(ctl.getLabelTypes()).andStubReturn(labelTypes); - EasyMock.replay(ctl); - env.set(StoredValues.CHANGE_CONTROL, ctl); + ChangeData cd = EasyMock.createMock(ChangeData.class); + expect(cd.getLabelTypes()).andStubReturn(labelTypes); + EasyMock.replay(cd); + env.set(StoredValues.CHANGE_DATA, cd); } @Test