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