Allow checkers to read all OWNERS files
* Fix problem of some submit checkers that might submit a change
without full owner info/approval.
* GetOwners API and UI Action still obey the read permission.
* Log more messages when a project or file cannot be read.
* Check and log errors when owner preferred email is null.
Change-Id: Id3d39fd11f8f24db8ff703d803dffbf6bcbfa2d7
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
index 2a7ffd9..8b99f17 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -22,7 +22,6 @@
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.rules.StoredValues;
@@ -41,7 +40,6 @@
private static final String EXEMPT_MESSAGE2 = "Exempted-From-Owner-Approval:";
private final GitRepositoryManager repoManager;
- private final PermissionBackend permissionBackend;
private final PluginConfigFactory configFactory;
private final Config config;
private final ProjectState projectState; // could be null when used by FindOwnersIT
@@ -50,13 +48,11 @@
Checker(
GitRepositoryManager repoManager,
- PermissionBackend permissionBackend,
PluginConfigFactory configFactory,
ProjectState projectState,
ChangeData changeData,
int v) {
this.repoManager = repoManager;
- this.permissionBackend = permissionBackend;
this.configFactory = configFactory;
this.projectState = projectState;
this.changeData = changeData;
@@ -97,8 +93,11 @@
boolean findOwnersInVotes(Set<String> owners, Map<String, Integer> votes) {
boolean foundVeto = false;
boolean foundApproval = false;
+ boolean foundNull = false;
for (String owner : owners) {
- if (votes.containsKey(owner)) {
+ if (owner == null) {
+ foundNull = true; // Something is wrong in OwnersDb!
+ } else if (votes.containsKey(owner)) {
int v = votes.get(owner);
foundApproval |= (v >= minVoteLevel);
foundVeto |= (v < 0); // an owner's -1 vote is a veto
@@ -106,6 +105,9 @@
foundApproval = true; // no specific owner
}
}
+ if (foundNull) {
+ logger.atSevere().log("Unexpected null owner email for %s", Config.getChangeId(changeData));
+ }
return foundApproval && !foundVeto;
}
@@ -132,7 +134,6 @@
Checker checker =
new Checker(
StoredValues.REPO_MANAGER.get(engine),
- StoredValues.PERMISSION_BACKEND.get(engine),
StoredValues.PLUGIN_CONFIG_FACTORY.get(engine),
StoredValues.PROJECT_STATE.get(engine),
changeData,
@@ -156,7 +157,7 @@
Cache.getInstance(configFactory, repoManager)
.get(
true,
- permissionBackend,
+ null, /* allow submit checker to read all OWNERS files */
projectState,
accountCache,
emails,
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
index cfa46f1..e3e7059 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -190,7 +190,9 @@
}
}
} catch (Exception e) {
- logger.atSevere().log("OwnersDb failed to find repository of project %s", projectName);
+ logger.atSevere().log(
+ "OwnersDb failed to find repository of project %s for %s",
+ projectName, Config.getChangeId(changeData));
logException(logs, "OwnersDb get repository", e);
}
countNumOwners(files);
@@ -259,6 +261,11 @@
logger.atSevere().withCause(e).log("Fail to find preferred email of %s", owner);
errors.add(owner);
}
+ if (email == null) {
+ logger.atSevere().log("accountCache failed to find preferred email of %s", owner);
+ errors.add(owner);
+ email = owner;
+ }
preferredEmails.put(owner, email);
}
}
@@ -281,6 +288,10 @@
addPreferredEmails(result.owner2paths.keySet());
for (String owner : result.owner2paths.keySet()) {
String email = preferredEmails.get(owner);
+ if (email == null) {
+ logger.atSevere().log("found null preferredEmail of %s", owner);
+ email = owner;
+ }
for (String path : result.owner2paths.get(owner)) {
addOwnerPathPair(email, path);
}
@@ -484,6 +495,7 @@
String content = findReadFile(readFiles, project, file);
if (content == null) {
if (!hasReadAccess(permissionBackend, project, branch, logs)) {
+ logger.atSevere().log("getRepoFile cannot read %s:%s", project, file);
return ""; // treat as read error
}
content = "";
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
index f5586b1..7669d55 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -36,7 +36,6 @@
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.CommitValidationMessage;
-import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
@@ -89,7 +88,6 @@
}
private final String pluginName;
- private final PermissionBackend permissionBackend;
private final PluginConfigFactory cfgFactory;
private final GitRepositoryManager repoManager;
private final Emails emails;
@@ -97,12 +95,10 @@
@Inject
OwnersValidator(
@PluginName String pluginName,
- PermissionBackend permissionBackend,
PluginConfigFactory cfgFactory,
GitRepositoryManager repoManager,
Emails emails) {
this.pluginName = pluginName;
- this.permissionBackend = permissionBackend;
this.cfgFactory = cfgFactory;
this.repoManager = repoManager;
this.emails = emails;
@@ -134,7 +130,7 @@
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent event)
throws CommitValidationException {
- Checker checker = new Checker(event, false, permissionBackend);
+ Checker checker = new Checker(event, false);
try {
Project.NameKey project = event.project.getNameKey();
PluginConfig cfg = cfgFactory.getFromProjectConfigWithInheritance(project, pluginName);
@@ -158,7 +154,6 @@
// An inner class to keep needed data specific to one commit event.
CommitReceivedEvent event;
boolean verbose;
- PermissionBackend permissionBackend;
List<CommitValidationMessage> messages;
Map<String, ObjectId> allFiles; // changedFilePath => ObjectId
Map<String, String> readFiles; // project:file => content
@@ -166,10 +161,9 @@
// Collect all email addresses from all files and check each address only once.
Map<String, Set<String>> email2lines;
- Checker(CommitReceivedEvent event, boolean verbose, PermissionBackend permissionBackend) {
+ Checker(CommitReceivedEvent event, boolean verbose) {
this.event = event;
this.verbose = verbose;
- this.permissionBackend = permissionBackend;
messages = new ArrayList<>();
readFiles = new HashMap<>();
checkedFiles = new HashSet<>();
@@ -359,7 +353,7 @@
addVerboseMsg("check repo file " + key);
String content =
OwnersDb.getRepoFile(
- permissionBackend,
+ null, /* permissionBackend */
readFiles,
repoManager,
null,
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
index 00e7217..f0dcd43 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
@@ -171,7 +171,7 @@
OwnersDb db =
cache.get(
true,
- permissionBackend,
+ null,
projectCache.get(project),
accountCache,
emails,
@@ -179,7 +179,7 @@
pluginConfig,
r.getChange(),
1);
- Checker c = new Checker(repoManager, permissionBackend, pluginConfig, null, r.getChange(), 1);
+ Checker c = new Checker(repoManager, pluginConfig, null, r.getChange(), 1);
return c.findApproval(accountCache, db);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
index 276e741..0030953 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
@@ -322,8 +322,8 @@
@Test
public void simpleIncludeACLTest() throws Exception {
- // If the user cannot read an included file,
- // the file is not seen and errors won't be detected.
+ // Even if the user cannot read an included file,
+ // the upload validator should still check the included file.
// Use pA/pB, because addProjectFile cannot create the same project again.
addProjectFile("pA", "d2/owners", "wrong\nxyz\n");
addProjectFile("pB", "d2/owners", "x@g.com\nerr\ninclude ../d2/owners\n");
@@ -335,7 +335,9 @@
+ "include pA:/d2/owners\ninclude pB:/d2/owners\n");
ImmutableSet<String> expected =
ImmutableSet.of(
- "MSG: unchecked: d1/OWNERS:4: include pB:/d2/owners", // cannot read pB
+ // "MSG: unchecked: d1/OWNERS:4: include pB:/d2/owners", // cannot read pB
+ "ERROR: unknown: x@g.com at pB:d2/owners:1",
+ "ERROR: syntax: pB:d2/owners:2: err",
"ERROR: syntax: d2/owners:1: wrong",
"ERROR: syntax: d2/owners:2: xyz");
CommitReceivedEvent event = makeCommitEvent("pA", "T", files);
@@ -391,9 +393,8 @@
private List<String> validate(CommitReceivedEvent event, boolean verbose, PluginConfig cfg)
throws Exception {
OwnersValidator validator =
- new OwnersValidator(
- "find-owners", permissionBackend, pluginConfig, repoManager, new MockedEmails());
- OwnersValidator.Checker checker = validator.new Checker(event, verbose, permissionBackend);
+ new OwnersValidator("find-owners", pluginConfig, repoManager, new MockedEmails());
+ OwnersValidator.Checker checker = validator.new Checker(event, verbose);
checker.check(OwnersValidator.getOwnersFileName(cfg));
return transformMessages(checker.messages);
}
