opendev/gerrit
Code Issues Proposed changes

Remove current_user predicates

Browse Source 
The result of a submittability check should not depend on the user who
is asking for it. If administrators want to make a distinction on who
should be allowed to submit a change, they can use permissions to do so.

This is especially true for the submit rule that would enforce that only
change owners can submit a change. This can be achieved by using the
change owners group in a permission rule.

More discussions can be found at:
https://groups.google.com/forum/#!msg/repo-discuss/vW6XhUOkqik/Ea4pco6vlCQJ

Change-Id: I73b370bdd4e9729365e8daac3f1a8a7b155ffc4d
This commit is contained in:
Changcheng Xiao
2017-11-21 11:22:48 +01:00
committed by Edwin Kempin
parent a3d0ed721a
commit d7990904e4
13 changed files with 19 additions and 265 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/prolog-change-facts.txt
View File

@@ -51,13 +51,6 @@ of them we must use a qualified name like `gerrit:change_branch(X)`.
|`commit_stats/3` |`commit_stats(5,20,50).`
|Number of files modified, number of insertions and the number of deletions.
.4+|`current_user/1` |`current_user(user(1000000)).`
.4+|Current user as one of the four given possibilities
|`current_user(user(anonymous)).`
|`current_user(user(peer_daemon)).`
|`current_user(user(replication)).`
|`pure_revert/1` |`pure_revert(1).`
|link:rest-api-changes.html#get-pure-revert[Pure revert] as integer atom (1 if
the change is a pure revert, 0 otherwise)

27
Documentation/prolog-cookbook.txt
View File

@@ -977,30 +977,7 @@ add_apprentice_master(S1, S2) :-
add_apprentice_master(S, S).
----
=== Example 15: Only allow Author to submit change
This example adds a new needed category `Only-Author-Can-Submit` for any user
that is not the author of the patch. This effectively blocks all users except
the author from submitting the change. This could result in an impossible
situation if the author does not have permissions for submitting the change.
`rules.pl`
[source,prolog]
----
submit_rule(S) :-
gerrit:default_submit(In),
In =.. [submit | Ls],
only_allow_author_to_submit(Ls, R),
S =.. [submit | R].
only_allow_author_to_submit(S, S) :-
gerrit:commit_author(Id),
gerrit:current_user(Id),
!.
only_allow_author_to_submit(S1, [label('Only-Author-Can-Submit', need(_)) | S1]).
----
=== Example 16: Make change submittable if all comments have been resolved
=== Example 15: Make change submittable if all comments have been resolved
In this example we will use the `unresolved_comments_count` fact about a
change. Our goal is to block the submission of any change with some
unresolved comments. Basically, it can be achieved by the following rules:
@@ -1050,7 +1027,7 @@ It's only used to show `'Needs All-Comments-Resolved'` in the UI to clearly
indicate to the user that all the comments have to be resolved for the
change to become submittable.
=== Example 17: Make change submittable if it is a pure revert
=== Example 16: Make change submittable if it is a pure revert
In this example we will use the `pure_revert` fact about a
change. Our goal is to block the submission of any change that is not a
pure revert. Basically, it can be achieved by the following rules:

28
java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
View File

@@ -24,7 +24,6 @@ import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.Emails;
@@ -87,7 +86,7 @@ public class SubmitRuleEvaluator {
}
public interface Factory {
SubmitRuleEvaluator create(CurrentUser user, ChangeData cd);
SubmitRuleEvaluator create(ChangeData cd);
}
private final AccountCache accountCache;
@@ -99,7 +98,6 @@ public class SubmitRuleEvaluator {
private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.builder();
private SubmitRuleOptions opts;
private Change change;
private CurrentUser user;
private PatchSet patchSet;
private boolean logErrors = true;
private long reductionsConsumed;
@@ -113,13 +111,11 @@ public class SubmitRuleEvaluator {
Accounts accounts,
Emails emails,
ProjectCache projectCache,
@Assisted CurrentUser user,
@Assisted ChangeData cd) {
this.accountCache = accountCache;
this.accounts = accounts;
this.emails = emails;
this.projectCache = projectCache;
this.user = user;
this.cd = cd;
}
@@ -229,11 +225,7 @@ public class SubmitRuleEvaluator {
try {
results =
evaluateImpl(
"locate_submit_rule",
"can_submit",
"locate_submit_filter",
"filter_submit_results",
user);
"locate_submit_rule", "can_submit", "locate_submit_filter", "filter_submit_results");
} catch (RuleEvalException e) {
return ruleError(e.getMessage(), e);
}
@@ -391,11 +383,7 @@ public class SubmitRuleEvaluator {
"locate_submit_type",
"get_submit_type",
"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);
"filter_submit_type_results");
} catch (RuleEvalException e) {
return typeError(e.getMessage(), e);
}
@@ -460,10 +448,9 @@ public class SubmitRuleEvaluator {
String userRuleLocatorName,
String userRuleWrapperName,
String filterRuleLocatorName,
String filterRuleWrapperName,
CurrentUser user)
String filterRuleWrapperName)
throws RuleEvalException {
PrologEnvironment env = getPrologEnvironment(user);
PrologEnvironment env = getPrologEnvironment();
try {
Term sr = env.once("gerrit", userRuleLocatorName, new VariableTerm());
List<Term> results = new ArrayList<>();
@@ -507,7 +494,7 @@ public class SubmitRuleEvaluator {
}
}
private PrologEnvironment getPrologEnvironment(CurrentUser user) throws RuleEvalException {
private PrologEnvironment getPrologEnvironment() throws RuleEvalException {
PrologEnvironment env;
try {
if (opts.rule() == null) {
@@ -529,9 +516,6 @@ public class SubmitRuleEvaluator {
env.set(StoredValues.EMAILS, emails);
env.set(StoredValues.REVIEW_DB, cd.db());
env.set(StoredValues.CHANGE_DATA, cd);
if (user != null) {
env.set(StoredValues.CURRENT_USER, user);
}
env.set(StoredValues.PROJECT_STATE, projectState);
return env;
}

11
java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -911,11 +911,7 @@ public class ChangeData {
if (!lazyLoad) {
return Collections.emptyList();
}
records =
submitRuleEvaluatorFactory
.create(userFactory.create(change().getOwner()), this)
.setOptions(options)
.evaluate();
records = submitRuleEvaluatorFactory.create(this).setOptions(options).evaluate();
submitRecords.put(options, records);
}
return records;
@@ -932,10 +928,7 @@ public class ChangeData {
public SubmitTypeRecord submitTypeRecord() throws OrmException {
if (submitTypeRecord == null) {
submitTypeRecord =
submitRuleEvaluatorFactory
.create(userFactory.create(change().getOwner()), this)
.getSubmitType();
submitTypeRecord = submitRuleEvaluatorFactory.create(this).getSubmitType();
}
return submitTypeRecord;
}

6
java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@@ -24,7 +24,6 @@ import com.google.gerrit.index.query.QueryResult;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.data.ChangeAttribute;
import com.google.gerrit.server.data.PatchSetAttribute;
@@ -82,7 +81,6 @@ public class OutputStreamQuery {
private final ChangeQueryProcessor queryProcessor;
private final EventFactory eventFactory;
private final TrackingFooters trackingFooters;
private final CurrentUser user;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
private OutputFormat outputFormat = OutputFormat.TEXT;
@@ -107,7 +105,6 @@ public class OutputStreamQuery {
ChangeQueryProcessor queryProcessor,
EventFactory eventFactory,
TrackingFooters trackingFooters,
CurrentUser user,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) {
this.db = db;
this.repoManager = repoManager;
@@ -115,7 +112,6 @@ public class OutputStreamQuery {
this.queryProcessor = queryProcessor;
this.eventFactory = eventFactory;
this.trackingFooters = trackingFooters;
this.user = user;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
}
@@ -254,7 +250,7 @@ public class OutputStreamQuery {
if (includeSubmitRecords) {
eventFactory.addSubmitRecords(
c, submitRuleEvaluatorFactory.create(user, d).setAllowClosed(true).evaluate());
c, submitRuleEvaluatorFactory.create(d).setAllowClosed(true).evaluate());
}
if (includeCommitMessage) {

8
java/com/google/gerrit/server/restapi/change/Mergeable.java
View File

@@ -26,7 +26,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.BranchOrderSection;
@@ -113,7 +112,7 @@ public class Mergeable implements RestReadView<RevisionResource> {
}
ChangeData cd = changeDataFactory.create(db.get(), resource.getNotes());
result.submitType = getSubmitType(resource.getUser(), cd, ps);
result.submitType = getSubmitType(cd, ps);
try (Repository git = gitManager.openRepository(change.getProject())) {
ObjectId commit = toId(ps);
@@ -145,10 +144,9 @@ public class Mergeable implements RestReadView<RevisionResource> {
return result;
}
private SubmitType getSubmitType(CurrentUser user, ChangeData cd, PatchSet patchSet)
throws OrmException {
private SubmitType getSubmitType(ChangeData cd, PatchSet patchSet) throws OrmException {
SubmitTypeRecord rec =
submitRuleEvaluatorFactory.create(user, cd).setPatchSet(patchSet).getSubmitType();
submitRuleEvaluatorFactory.create(cd).setPatchSet(patchSet).getSubmitType();
if (rec.status != SubmitTypeRecord.Status.OK) {
throw new OrmException("Submit type rule failed: " + rec);
}

3
java/com/google/gerrit/server/restapi/change/PostReviewers.java
View File

@@ -451,7 +451,7 @@ public class PostReviewers
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) {
IdentifiedUser u = identifiedUserFactory.create(accountId);
result.ccs.add(json.format(caller, new ReviewerInfo(accountId.get()), perm.user(u), cd));
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm.user(u), cd));
}
accountLoaderFactory.create(true).fill(result.ccs);
for (Address a : reviewersByEmail) {
@@ -464,7 +464,6 @@ public class PostReviewers
IdentifiedUser u = identifiedUserFactory.create(psa.getAccountId());
result.reviewers.add(
json.format(
caller,
new ReviewerInfo(psa.getAccountId().get()),
perm.user(u),
cd,

9
java/com/google/gerrit/server/restapi/change/ReviewerJson.java
View File

@@ -27,7 +27,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.change.ReviewerResource;
import com.google.gerrit.server.permissions.LabelPermission;
@@ -79,7 +78,6 @@ public class ReviewerJson {
}
ReviewerInfo info =
format(
rsrc.getChangeResource().getUser(),
new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()),
permissionBackend.user(rsrc.getReviewerUser()).database(db).change(cd),
cd);
@@ -95,12 +93,10 @@ public class ReviewerJson {
return format(ImmutableList.<ReviewerResource>of(rsrc));
}
public ReviewerInfo format(
CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException, PermissionBackendException {
PatchSet.Id psId = cd.change().currentPatchSetId();
return format(
user,
out,
perm,
cd,
@@ -109,7 +105,6 @@ public class ReviewerJson {
}
public ReviewerInfo format(
CurrentUser user,
ReviewerInfo out,
PermissionBackend.ForChange perm,
ChangeData cd,
@@ -129,7 +124,7 @@ public class ReviewerJson {
// do not exist in the DB.
PatchSet ps = cd.currentPatchSet();
if (ps != null) {
for (SubmitRecord rec : submitRuleEvaluatorFactory.create(user, cd).evaluate()) {
for (SubmitRecord rec : submitRuleEvaluatorFactory.create(cd).evaluate()) {
if (rec.labels == null) {
continue;
}

3
java/com/google/gerrit/server/restapi/change/TestSubmitRule.java
View File

@@ -71,8 +71,7 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
SubmitRuleEvaluator evaluator =
submitRuleEvaluatorFactory.create(
rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes()));
submitRuleEvaluatorFactory.create(changeDataFactory.create(db.get(), rsrc.getNotes()));
List<SubmitRecord> records =
evaluator

3
java/com/google/gerrit/server/restapi/change/TestSubmitType.java
View File

@@ -65,8 +65,7 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
}
input.filters = MoreObjects.firstNonNull(input.filters, filters);
SubmitRuleEvaluator evaluator =
submitRuleEvaluatorFactory.create(
rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes()));
submitRuleEvaluatorFactory.create(changeDataFactory.create(db.get(), rsrc.getNotes()));
SubmitTypeRecord rec =
evaluator

2
java/com/google/gerrit/server/rules/StoredValues.java
View File

@@ -24,7 +24,6 @@ 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.account.AccountCache;
import com.google.gerrit.server.account.Accounts;
@@ -54,7 +53,6 @@ public final class StoredValues {
public static final StoredValue<Emails> EMAILS = create(Emails.class);
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<ChangeData> CHANGE_DATA = create(ChangeData.class);
public static final StoredValue<CurrentUser> CURRENT_USER = create(CurrentUser.class);
public static final StoredValue<ProjectState> PROJECT_STATE = create(ProjectState.class);
public static Change getChange(Prolog engine) throws SystemException {

69
java/gerrit/PRED_current_user_1.java
View File

@@ -1,69 +0,0 @@
// Copyright (C) 2011 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.reviewdb.client.Account;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.rules.StoredValues;
import com.googlecode.prolog_cafe.exceptions.EvaluationException;
import com.googlecode.prolog_cafe.exceptions.PrologException;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
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.StructureTerm;
import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term;
public class PRED_current_user_1 extends Predicate.P1 {
private static final SymbolTerm user = SymbolTerm.intern("user", 1);
private static final SymbolTerm anonymous = SymbolTerm.intern("anonymous");
private static final SymbolTerm peerDaemon = SymbolTerm.intern("peer_daemon");
public PRED_current_user_1(Term a1, Operation n) {
arg1 = a1;
cont = n;
}
@Override
public Operation exec(Prolog engine) throws PrologException {
engine.setB0();
Term a1 = arg1.dereference();
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()) {
Account.Id id = curUser.getAccountId();
resultTerm = new IntegerTerm(id.get());
} else if (curUser instanceof AnonymousUser) {
resultTerm = anonymous;
} else if (curUser instanceof PeerDaemonUser) {
resultTerm = peerDaemon;
} else {
throw new EvaluationException("Unknown user type");
}
if (!a1.unify(new StructureTerm(user, resultTerm), engine.trail)) {
return engine.fail();
}
return cont;
}
}

108
java/gerrit/PRED_current_user_2.java
View File

@@ -1,108 +0,0 @@
// Copyright (C) 2011 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 static com.googlecode.prolog_cafe.lang.SymbolTerm.intern;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.rules.PrologEnvironment;
import com.google.gerrit.server.rules.StoredValues;
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.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.StructureTerm;
import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term;
import com.googlecode.prolog_cafe.lang.VariableTerm;
import java.util.Map;
/**
* Loads a CurrentUser object for a user identity.
*
* <p>Values are cached in the hash {@code current_user}, avoiding recreation during a single
* evaluation.
*
* <pre>
* current_user(user(+AccountId), -CurrentUser).
* </pre>
*/
class PRED_current_user_2 extends Predicate.P2 {
private static final SymbolTerm user = intern("user", 1);
private static final SymbolTerm anonymous = intern("anonymous");
PRED_current_user_2(Term a1, Term a2, Operation n) {
arg1 = a1;
arg2 = a2;
cont = n;
}
@Override
public Operation exec(Prolog engine) throws PrologException {
engine.setB0();
Term a1 = arg1.dereference();
Term a2 = arg2.dereference();
if (a1 instanceof VariableTerm) {
throw new PInstantiationException(this, 1);
}
if (!a2.unify(createUser(engine, a1), engine.trail)) {
return engine.fail();
}
return cont;
}
public Term createUser(Prolog engine, Term key) {
if (!(key instanceof StructureTerm)
|| key.arity() != 1
|| !((StructureTerm) key).functor().equals(user)) {
throw new IllegalTypeException(this, 1, "user(int)", key);
}
Term idTerm = key.arg(0);
CurrentUser user;
if (idTerm instanceof IntegerTerm) {
Map<Account.Id, IdentifiedUser> cache = StoredValues.USERS.get(engine);
Account.Id accountId = new Account.Id(((IntegerTerm) idTerm).intValue());
user = cache.get(accountId);
if (user == null) {
IdentifiedUser.GenericFactory userFactory = userFactory(engine);
IdentifiedUser who = userFactory.create(accountId);
cache.put(accountId, who);
user = who;
}
} else if (idTerm.equals(anonymous)) {
user = StoredValues.ANONYMOUS_USER.get(engine);
} else {
throw new IllegalTypeException(this, 1, "user(int)", key);
}
return new JavaObjectTerm(user);
}
private static IdentifiedUser.GenericFactory userFactory(Prolog engine) {
return ((PrologEnvironment) engine.control).getArgs().getUserFactory();
}
}