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