Check read permission in getRepoFile * Inject PermissionBackend to all API entry points and pass it to hasReadAccess. * Add new ACL read tests in IncludeIT and OwnersValidatorIT; share code in FindOwners. * Improve readability in IncludeIT and ParserTest. Change-Id: Ia10601eb00980b62f08c240b91da81ee0c38d0df
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java index d6a182d..7a654da 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java
@@ -33,6 +33,7 @@ import com.google.gerrit.server.change.RevisionResource; 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.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; @@ -56,6 +57,7 @@ private final Emails emails; private final ChangeData.Factory changeDataFactory; private final GitRepositoryManager repoManager; + private final PermissionBackend permissionBackend; private final PluginConfigFactory configFactory; private final Provider<CurrentUser> userProvider; private final ProjectCache projectCache; @@ -69,6 +71,7 @@ @Inject Action( + PermissionBackend permissionBackend, PluginConfigFactory configFactory, Provider<CurrentUser> userProvider, ChangeData.Factory changeDataFactory, @@ -76,6 +79,7 @@ Emails emails, GitRepositoryManager repoManager, ProjectCache projectCache) { + this.permissionBackend = permissionBackend; this.userProvider = userProvider; this.changeDataFactory = changeDataFactory; this.accountCache = accountCache; @@ -163,6 +167,7 @@ Cache.getInstance(configFactory, repoManager) .get( useCache, + permissionBackend, projectState, accountCache, emails, @@ -222,6 +227,7 @@ Cache.getInstance(configFactory, repoManager) .get( true, // use cached OwnersDb + permissionBackend, projectCache.get(resource.getProject()), accountCache, emails,
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java index 0c2af59..7d2cfa5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Cache.java
@@ -23,6 +23,7 @@ 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 java.util.Collection; @@ -91,6 +92,7 @@ /** Returns a cached or new OwnersDb, for the current patchset. */ OwnersDb get( Boolean useCache, + PermissionBackend permissionBackend, ProjectState projectState, AccountCache accountCache, Emails emails, @@ -100,6 +102,7 @@ throws StorageException { return get( useCache, + permissionBackend, projectState, accountCache, emails, @@ -112,6 +115,7 @@ /** Returns a cached or new OwnersDb, for the specified patchset. */ OwnersDb get( Boolean useCache, + PermissionBackend permissionBackend, ProjectState projectState, AccountCache accountCache, Emails emails, @@ -125,6 +129,7 @@ // TODO: get changed files of the given patchset? return get( useCache, + permissionBackend, projectState, accountCache, emails, @@ -139,6 +144,7 @@ /** Returns a cached or new OwnersDb, for the specified branch and changed files. */ OwnersDb get( Boolean useCache, + PermissionBackend permissionBackend, ProjectState projectState, AccountCache accountCache, Emails emails, @@ -151,6 +157,7 @@ if (dbCache == null || !useCache) { // Do not cache OwnersDb logger.atFiner().log("Create new OwnersDb, key=%s", key); return new OwnersDb( + permissionBackend, projectState, accountCache, emails, @@ -171,6 +178,7 @@ public OwnersDb call() { logger.atFiner().log("Create new OwnersDb, key=%s", key); return new OwnersDb( + permissionBackend, projectState, accountCache, emails, @@ -186,6 +194,7 @@ logger.atSevere().withCause(e).log( "Cache.get has exception for %s", Config.getChangeId(changeData)); return new OwnersDb( + permissionBackend, projectState, accountCache, emails,
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 6399771..2a7ffd9 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Checker.java
@@ -22,6 +22,7 @@ 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; @@ -40,6 +41,7 @@ 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 @@ -48,11 +50,13 @@ 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; @@ -128,6 +132,7 @@ 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, @@ -149,7 +154,15 @@ // many times. So this function should use cached values. OwnersDb db = Cache.getInstance(configFactory, repoManager) - .get(true, projectState, accountCache, emails, repoManager, configFactory, changeData); + .get( + true, + permissionBackend, + projectState, + accountCache, + emails, + repoManager, + configFactory, + changeData); if (db.getNumOwners() <= 0) { return 0; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java index 5735ced..ce3fdef 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java
@@ -25,6 +25,7 @@ import com.google.gerrit.server.change.ChangeResource; 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.ProjectCache; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; @@ -52,6 +53,7 @@ @Inject GetOwners( + PermissionBackend permissionBackend, PluginConfigFactory configFactory, Provider<CurrentUser> userProvider, ChangeData.Factory dataFactory, @@ -61,6 +63,7 @@ ProjectCache projectCache) { this.action = new Action( + permissionBackend, configFactory, userProvider, dataFactory,
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 7058842..cfa46f1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersDb.java
@@ -19,12 +19,16 @@ import com.google.common.collect.Multimap; import com.google.common.collect.Ordering; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.account.AccountCache; 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.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; import java.net.InetAddress; @@ -52,6 +56,7 @@ class OwnersDb { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final PermissionBackend permissionBackend; private final AccountCache accountCache; private final GitRepositoryManager repoManager; private final Emails emails; @@ -71,6 +76,7 @@ List<String> logs = new ArrayList<>(); // trace/debug messages OwnersDb( + PermissionBackend permissionBackend, ProjectState projectState, AccountCache accountCache, Emails emails, @@ -80,6 +86,7 @@ ChangeData changeData, String branch, Collection<String> files) { + this.permissionBackend = permissionBackend; this.accountCache = accountCache; this.repoManager = repoManager; this.emails = emails; @@ -111,7 +118,17 @@ // this project should have a non-empty root file of that name. // We added this requirement to detect errors in project config files // and Gerrit server bugs that return wrong value of "ownersFileName". - String content = getFile(readFiles, repo, projectName, id, "/" + ownersFileName, logs); + String content = + getRepoFile( + permissionBackend, + readFiles, + null, + repo, + id, + projectName, + branch, + "/" + ownersFileName, + logs); String found = "Found"; if (content.isEmpty()) { String changeId = Config.getChangeId(changeData); @@ -138,7 +155,17 @@ readDirs.add(dir); logs.add("findOwnersFileIn:" + dir); String filePath = dir + "/" + ownersFileName; - String content = getFile(readFiles, repo, projectName, id, filePath, logs); + String content = + getRepoFile( + permissionBackend, + readFiles, + null, + repo, + id, + projectName, + branch, + filePath, + logs); if (content != null && !content.isEmpty()) { addFile( readFiles, @@ -244,7 +271,8 @@ String dirPath, String filePath, String[] lines) { - Parser parser = new Parser(readFiles, repoManager, project, branch, filePath, logs); + Parser parser = + new Parser(permissionBackend, readFiles, repoManager, project, branch, filePath, logs); Parser.Result result = parser.parseFile(dirPath, lines); if (result.stopLooking) { stopLooking.add(dirPath); @@ -416,10 +444,36 @@ } } + private static boolean hasReadAccess( + PermissionBackend permissionBackend, String project, String branch, List<String> logs) { + if (permissionBackend == null || branch == null || project == null) { + return true; // cannot check, so assume okay + } + if (!branch.startsWith("refs/")) { + branch = "refs/heads/" + branch; + } + try { + permissionBackend + .currentUser() + .project(Project.nameKey(project)) + .ref(branch) + .check(RefPermission.READ); + } catch (AuthException | PermissionBackendException e) { + logger.atSevere().withCause(e).log( + "getFile cannot read file in project %s branch %s", project, branch); + logException(logs, "hasReadAccess", e); + return false; + } + return true; + } + /** Returns file content or empty string; uses project+branch+file names. */ public static String getRepoFile( + PermissionBackend permissionBackend, Map<String, String> readFiles, GitRepositoryManager repoManager, + Repository repository, + ObjectId id, String project, String branch, String file, @@ -429,51 +483,48 @@ file = Util.gitRepoFilePath(file); String content = findReadFile(readFiles, project, file); if (content == null) { + if (!hasReadAccess(permissionBackend, project, branch, logs)) { + return ""; // treat as read error + } content = ""; - if (repoManager != null) { // ParserTest can call with null repoManager + if ((id == null || repository == null) && repoManager != null) { + // create ObjectId from repoManager try (Repository repo = repoManager.openRepository(Project.nameKey(project))) { - ObjectId id = repo.resolve(branch); + id = repo.resolve(branch); if (id != null) { - return getFile(readFiles, repo, project, id, file, logs); + content = getFile(repo, id, file, logs); + } else { + logs.add("getRepoFile not found branch " + branch); } - logs.add("getRepoFile not found branch " + branch); } catch (Exception e) { logger.atSevere().log("getRepoFile failed to find repository of project %s", project); logException(logs, "getRepoFile", e); } + } else if (id != null && repository != null) { + content = getFile(repository, id, file, logs); } + saveReadFile(readFiles, project, file, content); } return content; } /** Returns file content or empty string; uses Repository. */ - private static String getFile( - Map<String, String> readFiles, - Repository repo, - String project, - ObjectId id, - String file, - List<String> logs) { - file = Util.gitRepoFilePath(file); - String content = findReadFile(readFiles, project, file); - if (content == null) { - content = ""; - try (RevWalk revWalk = new RevWalk(repo)) { - String header = "getFile:" + file; - RevTree tree = revWalk.parseCommit(id).getTree(); - ObjectReader reader = revWalk.getObjectReader(); - TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree); - if (treeWalk != null) { - content = new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8); - logs.add(header + ":(...)"); - } else { - logs.add(header + " (NOT FOUND)"); - } - } catch (Exception e) { - logger.atSevere().withCause(e).log("get file %s", file); - logException(logs, "getFile", e); + private static String getFile(Repository repo, ObjectId id, String file, List<String> logs) { + String content = ""; + try (RevWalk revWalk = new RevWalk(repo)) { + String header = "getFile:" + file; + RevTree tree = revWalk.parseCommit(id).getTree(); + ObjectReader reader = revWalk.getObjectReader(); + TreeWalk treeWalk = TreeWalk.forPath(reader, file, tree); + if (treeWalk != null) { + content = new String(reader.open(treeWalk.getObjectId(0)).getBytes(), UTF_8); + logs.add(header + ":(...)"); + } else { + logs.add(header + " (NOT FOUND)"); } - saveReadFile(readFiles, project, file, content); + } catch (Exception e) { + logger.atSevere().withCause(e).log("get file %s", file); + logException(logs, "getFile", e); } return 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 11559df..f5586b1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/OwnersValidator.java
@@ -36,6 +36,7 @@ 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; @@ -88,6 +89,7 @@ } private final String pluginName; + private final PermissionBackend permissionBackend; private final PluginConfigFactory cfgFactory; private final GitRepositoryManager repoManager; private final Emails emails; @@ -95,12 +97,14 @@ @Inject OwnersValidator( @PluginName String pluginName, + PermissionBackend permissionBackend, PluginConfigFactory cfgFactory, GitRepositoryManager repoManager, Emails emails) { this.pluginName = pluginName; - this.repoManager = repoManager; + this.permissionBackend = permissionBackend; this.cfgFactory = cfgFactory; + this.repoManager = repoManager; this.emails = emails; } @@ -130,7 +134,7 @@ @Override public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent event) throws CommitValidationException { - Checker checker = new Checker(event, false); + Checker checker = new Checker(event, false, permissionBackend); try { Project.NameKey project = event.project.getNameKey(); PluginConfig cfg = cfgFactory.getFromProjectConfigWithInheritance(project, pluginName); @@ -154,6 +158,7 @@ // 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 @@ -161,9 +166,10 @@ // Collect all email addresses from all files and check each address only once. Map<String, Set<String>> email2lines; - Checker(CommitReceivedEvent event, boolean verbose) { + Checker(CommitReceivedEvent event, boolean verbose, PermissionBackend permissionBackend) { this.event = event; this.verbose = verbose; + this.permissionBackend = permissionBackend; messages = new ArrayList<>(); readFiles = new HashMap<>(); checkedFiles = new HashSet<>(); @@ -353,8 +359,16 @@ addVerboseMsg("check repo file " + key); String content = OwnersDb.getRepoFile( - readFiles, repoManager, KPF[1], event.refName, repoFile, new ArrayList<>()); - if (isNullOrEmpty(content)) { + permissionBackend, + readFiles, + repoManager, + null, + null, + KPF[1], + event.refName, + repoFile, + new ArrayList<>()); + if (isNullOrEmpty(content)) { // file not found or not readable. addVerboseMsg("cannot find file: " + key); // unchecked: including-file-path : line number : source line addMsg("unchecked: " + qualifiedPath(project, path) + ":" + num + ": " + directive);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java index faf6141..b3ecda3 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java +++ b/src/main/java/com/googlesource/gerrit/plugins/findowners/Parser.java
@@ -16,6 +16,7 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.permissions.PermissionBackend; import java.io.IOException; import java.util.ArrayDeque; import java.util.ArrayList; @@ -31,12 +32,17 @@ /** * Parse lines in an OWNERS file and put them into an OwnersDb. One Parser object should be created - * to parse only one OWNERS file. It keeps repoManager, project, branch, and filePath of the OWNERS - * file so it can find files that are included by OWNERS. + * to parse only one OWNERS file. It keeps permissionBackend, readFiles, repoManager, project, + * branch, and filePath of the OWNERS file so it can find files that are included by OWNERS. * - * <p>The usage pattern is: Parser parser = new Parser(repoManager, project, branch, repoFilePath); - * String content = OwnersDb.getRepoFile(readFiles, repoManager, project, branch, repoFilePath, - * logs); Parser.Result result = parser.parseFile(dirPath, content); + * <p>The usage pattern is: + * + * <pre> + * Parser parser = new Parser(permissionBackend, readFiles, repoManager, project, branch, repoFilePath); + * String content = OwnersDb.getRepoFile(permissionBackend, readFiles, repoManager, null, null, + * project, branch, repoFilePath, logs); + * Parser.Result result = parser.parseFile(dirPath, content); + * </pre> * * <p>OWNERS file syntax, semantics, and examples are included in syntax.md. */ @@ -95,8 +101,9 @@ private static final Pattern PAT_INCLUDE_OR_FILE = Pattern.compile("^.*(" + INCLUDE_OR_FILE + PROJECT_AND_FILE + ")" + EOL); - // A parser keeps current readFiles, repoManager, project, branch, + // A parser keeps current permissionBackend, readFiles, repoManager, project, branch, // included file path, and debug/trace logs. + private final PermissionBackend permissionBackend; private Map<String, String> readFiles; private GitRepositoryManager repoManager; private String branch; // All owners files are read from the same branch. @@ -141,32 +148,30 @@ } } + // For simple unit tests without a repository. + Parser(String project, String branch, String file) { + this(null, null, null, project, branch, file, new ArrayList<>()); + } + Parser( + PermissionBackend permissionBackend, Map<String, String> readFiles, GitRepositoryManager repoManager, String project, String branch, String file) { - init(readFiles, repoManager, project, branch, file, new ArrayList<>()); + this(permissionBackend, readFiles, repoManager, project, branch, file, new ArrayList<>()); } Parser( + PermissionBackend permissionBackend, Map<String, String> readFiles, GitRepositoryManager repoManager, String project, String branch, String file, List<String> logs) { - init(readFiles, repoManager, project, branch, file, logs); - } - - private void init( - Map<String, String> readFiles, - GitRepositoryManager repoManager, - String project, - String branch, - String file, - List<String> logs) { + this.permissionBackend = permissionBackend; this.readFiles = readFiles; this.repoManager = repoManager; this.branch = branch; @@ -405,7 +410,16 @@ stack.push(project, repoFile); logs.add("parseLine:" + includeKPF); String content = - OwnersDb.getRepoFile(readFiles, repoManager, project, branch, repoFile, logs); + OwnersDb.getRepoFile( + permissionBackend, + readFiles, + repoManager, + null, + null, + project, + branch, + repoFile, + logs); if (content != null && !content.isEmpty()) { includedFileResult = parseFile("", content); } else {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java index 8e82a1d..9c487cb 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ApiIT.java
@@ -17,7 +17,6 @@ import static com.google.common.truth.OptionalSubject.optionals; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; -import static com.google.common.truth.Truth8.assertThat; import com.google.common.collect.Multimap; import com.google.common.flogger.FluentLogger; @@ -110,7 +109,14 @@ Action.Parameters param = new Action.Parameters(); Action action = new Action( - pluginConfig, null, changeDataFactory, accountCache, emails, repoManager, projectCache); + permissionBackend, + pluginConfig, + null, + changeDataFactory, + accountCache, + emails, + repoManager, + projectCache); Response<RestResult> response = action.apply(cr, param); RestResult result = response.value(); verifyRestResult(result, 1, 1, changeInfo._number, false);
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 9e3e3d2..00e7217 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java +++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/FindOwners.java
@@ -15,13 +15,16 @@ package com.googlesource.gerrit.plugins.findowners; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; +import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.api.projects.BranchApi; import com.google.gerrit.extensions.client.ChangeStatus; @@ -31,6 +34,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.account.Emails; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.inject.Inject; import org.eclipse.jgit.lib.ObjectLoader; import org.eclipse.jgit.revwalk.RevObject; @@ -45,6 +49,7 @@ public abstract class FindOwners extends LightweightPluginDaemonTest { @Inject protected Emails emails; + @Inject protected PermissionBackend permissionBackend; @Inject protected ProjectOperations projectOperations; protected static final String PLUGIN_NAME = "find-owners"; @@ -112,6 +117,14 @@ testRepo = cloneProject(project); } + protected void blockRead(Project.NameKey p) throws Exception { + projectOperations + .project(p) + .forUpdate() + .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS)) + .update(); + } + protected org.eclipse.jgit.lib.Config readProjectConfig() throws Exception { git().fetch().setRefSpecs(new RefSpec(REFS_CONFIG + ":" + REFS_CONFIG)).call(); testRepo.reset(RefNames.REFS_CONFIG); @@ -158,6 +171,7 @@ OwnersDb db = cache.get( true, + permissionBackend, projectCache.get(project), accountCache, emails, @@ -165,7 +179,7 @@ pluginConfig, r.getChange(), 1); - Checker c = new Checker(repoManager, pluginConfig, null, r.getChange(), 1); + Checker c = new Checker(repoManager, permissionBackend, pluginConfig, null, r.getChange(), 1); return c.findApproval(accountCache, db); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/IncludeIT.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/IncludeIT.java index f5829a2..be64ad3 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/findowners/IncludeIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/IncludeIT.java
@@ -29,6 +29,18 @@ private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @Rule public Watcher watcher = new Watcher(logger); + private String getRepoFileLog(String msg1, String msg2) { + return "getRepoFile:" + msg1 + ", getFile:" + msg2 + ", "; + } + + private String concat(String s1, String s2) { + return s1 + s2; + } + + private String concat(String s1, String s2, String s3) { + return s1 + s2 + s3; + } + @Test public void includeNotFoundTest() throws Exception { // c2 and c1 are both submitted before existence of OWNERS. @@ -43,25 +55,18 @@ String owner2paths = "owner2paths:{ a@a:[ ./ ], x@x:[ ./ ] }"; String projectName = project.get(); String expectedInLog = - "project:" - + projectName - + ", " + concat("project:", projectName, ", ") + "ownersFileName:OWNERS, " + "getBranchId:refs/heads/master(FOUND), " + "findOwnersFileFor:./t.c, " + "findOwnersFileIn:., " - + "getFile:OWNERS:(...), " + + getRepoFileLog(projectName + ":refs/heads/master:./OWNERS", "OWNERS:(...)") + "parseLine:include:P1/P2:f1, " + "getRepoFile:P1/P2:refs/heads/master:f1, " - + "getRepoFileException:repositorynotfound:P1/P2, " // repository not found + + "hasReadAccessException:project\\u0027P1/P2\\u0027isunavailable, " // cannot read + "parseLine:include:(), " // missing file is treated as empty - + "parseLine:include:" - + projectName - + ":./d1/d2/../../f2, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f2, " - + "getFile:f2(NOTFOUND), " // same repository but f2 is missing + + concat("parseLine:include:", projectName, ":./d1/d2/../../f2, ") + + getRepoFileLog(projectName + ":refs/heads/master:f2", "f2(NOTFOUND)") + "parseLine:include:(), " // missing file is treated as empty + "countNumOwners, " + "findOwners, " @@ -101,25 +106,18 @@ String owner2paths = "owner2paths:{ a@a:[ ./ ], g1@g:[ ./ ], g2@g:[ ./ ], x@x:[ ./ ] }"; String projectName = project.get(); String expectedInLog = - "project:" - + projectName - + ", " + concat("project:", projectName, ", ") + "ownersFileName:OWNERS, " + "getBranchId:refs/heads/master(FOUND), " + "findOwnersFileFor:./t.c, " + "findOwnersFileIn:., " - + "getFile:OWNERS:(...), " + + getRepoFileLog(projectName + ":refs/heads/master:./OWNERS", "OWNERS:(...)") + "parseLine:include:P1/P2:f1, " + "getRepoFile:P1/P2:refs/heads/master:f1, " - + "getRepoFileException:repositorynotfound:P1/P2, " + + "hasReadAccessException:project\\u0027P1/P2\\u0027isunavailable, " + "parseLine:include:(), " // P1/P2 is still not found - + "parseLine:include:" - + projectName - + ":./d1/d2/../../f2, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f2, " - + "getFile:f2:(...), " // f2 is included + + concat("parseLine:include:", projectName, ":./d1/d2/../../f2, ") + + getRepoFileLog(projectName + ":refs/heads/master:f2", "f2:(...)") + "countNumOwners, " + "findOwners, " + "checkFile:./t.c, " @@ -162,14 +160,10 @@ assertThat(getOwnersResponse(c1)) .contains( "owners:[ " - + ownerD2 - + ", " - + ownerF2 - + ", " - + ownerD3 - + ", " - + ownerX - + " ], files:[ d3/t.c ]"); + + concat(ownerD2, ", ") + + concat(ownerF2, ", ") + + concat(ownerD3, ", ") + + concat(ownerX, " ], files:[ d3/t.c ]")); } @Test @@ -247,45 +241,21 @@ String response = getOwnersDebugResponse(c); String projectName = project.get(); String expectedInLog = - "project:" - + projectName - + ", " + concat("project:", projectName, ", ") + "ownersFileName:OWNERS, " + "getBranchId:refs/heads/master(FOUND), " + "findOwnersFileFor:./t.c, " + "findOwnersFileIn:., " - + "getFile:OWNERS:(...), " - + "parseLine:include:" - + projectName - + ":./d1/../f1, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f1, " - + "getFile:f1:(...), " - + "parseLine:include:" - + projectName - + ":./f2, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f2, " - + "getFile:f2:(...), " - + "parseLine:include:" - + projectName - + ":d1/../f3, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f3, " - + "getFile:f3:(...), " - + "parseLine:include:" - + projectName - + ":/f4, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:f4, " - + "getFile:f4:(...), " - + "parseLine:errorRecursion:include:" - + projectName - + ":d2/../f2, " + + getRepoFileLog(projectName + ":refs/heads/master:./OWNERS", "OWNERS:(...)") + + concat("parseLine:include:", projectName, ":./d1/../f1, ") + + getRepoFileLog(projectName + ":refs/heads/master:f1", "f1:(...)") + + concat("parseLine:include:", projectName, ":./f2, ") + + getRepoFileLog(projectName + ":refs/heads/master:f2", "f2:(...)") + + concat("parseLine:include:", projectName, ":d1/../f3, ") + + getRepoFileLog(projectName + ":refs/heads/master:f3", "f3:(...)") + + concat("parseLine:include:", projectName, ":/f4, ") + + getRepoFileLog(projectName + ":refs/heads/master:f4", "f4:(...)") + + concat("parseLine:errorRecursion:include:", projectName, ":d2/../f2, ") + "countNumOwners, " + "findOwners, " + "checkFile:./t.c, " @@ -323,76 +293,32 @@ assertThat(result).contains(skipLog + path); } String expectedInLog = - "project:" - + projectName - + ", " + concat("project:", projectName, ", ") + "ownersFileName:OWNERS, " + "getBranchId:refs/heads/master(FOUND), " + "findOwnersFileFor:./d6/OWNERS, " + "findOwnersFileIn:./d6, " - + "getFile:d6/OWNERS:(...), " - + "parseLine:include:" - + projectName - + ":/d0/f0, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d0/f0, " - + "getFile:d0/f0:(...), " - + "parseLine:include:" - + projectName - + ":../d1/d2/f1, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d1/d2/f1, " - + "getFile:d1/d2/f1:(...), " - + "parseLine:useSaved:include:" - + projectName - + ":../../d0/f0, " - + "parseLine:include:" - + projectName - + ":../d2/f2, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d2/f2, " - + "getFile:d2/f2:(...), " - + "parseLine:useSaved:include:" - + projectName - + ":../d0/f0, " - + "parseLine:include:" - + projectName - + ":/d2/d3/f3, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d2/d3/f3, " - + "getFile:d2/d3/f3:(...), " - + "parseLine:useSaved:include:" - + projectName - + ":/d0/f0, " - + "parseLine:include:" - + projectName - + ":/d2/../d4/d5/f5, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d4/d5/f5, " - + "getFile:d4/d5/f5:(...), " - + "parseLine:useSaved:include:" - + projectName - + ":/d2/f2, " - + "parseLine:include:" - + projectName - + ":../f4, " - + "getRepoFile:" - + projectName - + ":refs/heads/master:d4/f4, " - + "getFile:d4/f4:(...), " - + "parseLine:useSaved:include:" - + projectName - + ":../d2/f2, " - + "parseLine:useSaved:include:" - + projectName - + ":/d4/f4, " + + getRepoFileLog(projectName + ":refs/heads/master:./d6/OWNERS", "d6/OWNERS:(...)") + + concat("parseLine:include:", projectName, ":/d0/f0, ") + + getRepoFileLog(projectName + ":refs/heads/master:d0/f0", "d0/f0:(...)") + + concat("parseLine:include:", projectName, ":../d1/d2/f1, ") + + getRepoFileLog(projectName + ":refs/heads/master:d1/d2/f1", "d1/d2/f1:(...)") + + concat("parseLine:useSaved:include:", projectName, ":../../d0/f0, ") + + concat("parseLine:include:", projectName, ":../d2/f2, ") + + getRepoFileLog(projectName + ":refs/heads/master:d2/f2", "d2/f2:(...)") + + concat("parseLine:useSaved:include:", projectName, ":../d0/f0, ") + + concat("parseLine:include:", projectName, ":/d2/d3/f3, ") + + getRepoFileLog(projectName + ":refs/heads/master:d2/d3/f3", "d2/d3/f3:(...)") + + concat("parseLine:useSaved:include:", projectName, ":/d0/f0, ") + + concat("parseLine:include:", projectName, ":/d2/../d4/d5/f5, ") + + getRepoFileLog(projectName + ":refs/heads/master:d4/d5/f5", "d4/d5/f5:(...)") + + concat("parseLine:useSaved:include:", projectName, ":/d2/f2, ") + + concat("parseLine:include:", projectName, ":../f4, ") + + getRepoFileLog(projectName + ":refs/heads/master:d4/f4", "d4/f4:(...)") + + concat("parseLine:useSaved:include:", projectName, ":../d2/f2, ") + + concat("parseLine:useSaved:include:", projectName, ":/d4/f4, ") + "findOwnersFileIn:., " - + "getFile:OWNERS(NOTFOUND), " + + getRepoFileLog(projectName + ":refs/heads/master:./OWNERS", "OWNERS(NOTFOUND)") + "countNumOwners, " + "findOwners, " + "checkFile:./d6/OWNERS, " @@ -426,20 +352,46 @@ // inherited: pA:OWNERS String owners = "owners:[ " - + ownerJson("pAd1f1@g") - + ", " - + ownerJson("pAd2@g") - + ", " - + ownerJson("pAf1@g") - + ", " - + ownerJson("pBd1f1@g") - + ", " - + ownerJson("pBd2f2@g") - + ", " - + ownerJson("pBf1@g") - + ", " - + ownerJson("pA@g", 0, 1, 0) - + " ]"; + + concat(ownerJson("pAd1f1@g"), ", ") + + concat(ownerJson("pAd2@g"), ", ") + + concat(ownerJson("pAf1@g"), ", ") + + concat(ownerJson("pBd1f1@g"), ", ") + + concat(ownerJson("pBd2f2@g"), ", ") + + concat(ownerJson("pBf1@g"), ", ") + + concat(ownerJson("pA@g", 0, 1, 0), " ]"); + assertThat(getOwnersResponse(c1)).contains(owners); + } + + @Test + public void includeProjectOwnerACLTest() throws Exception { + // Test include directive with other unreadable project. + Project.NameKey pA = newProject("PA2"); + Project.NameKey pB = newProject("PB2"); + String nameA = pA.get(); + String nameB = pB.get(); + switchProject(pA); + addFile("1", "f1", "pAf1@g\ninclude ./d1/f1\n"); + addFile("2", "d1/f1", "pAd1f1@g\ninclude " + nameB + ":" + "/d2/f2\n"); + addFile("3", "d2/OWNERS", "pAd2@g\n include " + nameA + " : " + "../f1\n"); + addFile("4", "OWNERS", "pA@g\n"); + switchProject(pB); + addFile("5", "f1", "pBf1@g\ninclude ./d1/f1\n"); + addFile("6", "f2", "pBf2@g\n"); + addFile("7", "d1/f1", "pBd1f1@g\n"); + addFile("8", "d2/f2", "pBd2f2@g\ninclude ../f1\n"); + blockRead(project); // now cannot read pB + switchProject(pA); + PushOneCommit.Result c1 = createChange("c1", "d2/t.c", "Hello!"); + // included: pA:d2/OWNERS, pA:d2/../f1, pA:d1/f1 + // inherited: pA:OWNERS + // pB's OWNERS files are not readable + String owners = + "owners:[ " + + concat(ownerJson("pAd1f1@g"), ", ") + + concat(ownerJson("pAd2@g"), ", ") + + concat(ownerJson("pAf1@g"), ", ") + + concat(ownerJson("pA@g", 0, 1, 0), " ]"); + // The "owners:[...]" substring contains only owners from pA. assertThat(getOwnersResponse(c1)).contains(owners); } }
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 f46b7ad..276e741 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/OwnersValidatorIT.java
@@ -25,10 +25,8 @@ import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Lists; import com.google.common.flogger.FluentLogger; -import com.google.gerrit.acceptance.LightweightPluginDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestPlugin; -import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Project; @@ -36,7 +34,6 @@ import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.validators.CommitValidationMessage; -import com.google.inject.Inject; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -62,25 +59,10 @@ /** Test OwnersValidator, which checks syntax of changed OWNERS files. */ @TestPlugin(name = "find-owners", sysModule = "com.googlesource.gerrit.plugins.findowners.Module") -public class OwnersValidatorIT extends LightweightPluginDaemonTest { +public class OwnersValidatorIT extends FindOwners { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @Rule public Watcher watcher = new Watcher(logger); - @Inject private ProjectOperations projectOperations; - - protected Project.NameKey newProject(String name) { - return projectOperations - .newProject() - .parent(Project.nameKey("All-Projects")) - .name(name) - .create(); - } - - private void switchProject(Project.NameKey p) throws Exception { - project = p; - testRepo = cloneProject(project); - } - private void addProjectFile(String project, String file, String content) throws Exception { switchProject(newProject(project)); PushOneCommit.Result c = createChange("c", file, content); @@ -338,6 +320,28 @@ assertThat(validate(event, false, ENABLED_CONFIG)).containsExactlyElementsIn(expected); } + @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. + // 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"); + blockRead(project); // current project is pB; set it to not readable + ImmutableMap<String, String> files = + ImmutableMap.of( + "d1/" + OWNERS, + "include ../d2/owners\ninclude /d2/owners\n" + + "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 + "ERROR: syntax: d2/owners:1: wrong", + "ERROR: syntax: d2/owners:2: xyz"); + CommitReceivedEvent event = makeCommitEvent("pA", "T", files); + assertThat(validate(event, false, ENABLED_CONFIG)).containsExactlyElementsIn(expected); + } + private static PluginConfig createEnabledConfig() { PluginConfig c = new PluginConfig("", new Config()); c.setBoolean(REJECT_ERROR_IN_OWNERS, true); @@ -387,8 +391,9 @@ private List<String> validate(CommitReceivedEvent event, boolean verbose, PluginConfig cfg) throws Exception { OwnersValidator validator = - new OwnersValidator("find-owners", pluginConfig, repoManager, new MockedEmails()); - OwnersValidator.Checker checker = validator.new Checker(event, verbose); + new OwnersValidator( + "find-owners", permissionBackend, pluginConfig, repoManager, new MockedEmails()); + OwnersValidator.Checker checker = validator.new Checker(event, verbose, permissionBackend); checker.check(OwnersValidator.getOwnersFileName(cfg)); return transformMessages(checker.messages); }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java index fee0569..8a0481b 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/findowners/ParserTest.java
@@ -18,6 +18,8 @@ import com.google.common.flogger.FluentLogger; import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -39,8 +41,8 @@ private static Parser.Result testLine(String line) { Parser.Result result = new Parser.Result(); - // Single line parser tests do not need repoManager. - Parser parser = new Parser(null, null, mockedProject(), "master", "OWNERS"); + // Single line parser tests do not need a repository. + Parser parser = new Parser(mockedProject(), "master", "OWNERS"); parser.parseLine(result, mockedTestDir(), line, 3); return result; } @@ -273,32 +275,18 @@ @Test public void getIncludeOrFileTest() { - String[] line = - new String[] { - "", - "wrong input", - "INCLUDE X", - "include //f2.txt # ", - " include P1/P2: ../f1 # ", - " file://f3 # ", - "file: P1:f3", - " per-file *.c,file.c = file: /OWNERS # ", - "per-file *=file:P1/P2: /O# ", - }; - String[] expected = - new String[] { - "", - "", - "", - "include //f2.txt", - "include P1/P2:../f1", - "file://f3", - "file:P1:f3", - "file:/OWNERS", - "file:P1/P2:/O", - }; - for (int i = 0; i < line.length; i++) { - assertThat(Parser.getIncludeOrFile(line[i])).isEqualTo(expected[i]); + Map<String, String> tests = new HashMap<>(); // map from input to expected result + tests.put("", ""); + tests.put("wrong input", ""); + tests.put("INCLUDE X", ""); + tests.put("include //f2.txt # ", "include //f2.txt"); + tests.put(" include P1/P2: ../f1 # ", "include P1/P2:../f1"); + tests.put(" file://f3 # ", "file://f3"); + tests.put("file: P1:f3", "file:P1:f3"); + tests.put(" per-file *.c,file.c = file: /OWNERS # ", "file:/OWNERS"); + tests.put("per-file *=file:P1/P2: /O# ", "file:P1/P2:/O"); + for (String line : tests.keySet()) { + assertThat(Parser.getIncludeOrFile(line)).isEqualTo(tests.get(line)); } }