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); }