CheckCodeOwner: Also consider fallback code owners
CheckCodeOwner returned isCodeOwner = false for users that are fallback
code owners, which can be very confusing. Fix CheckCodeOwner so that it
returns isCodeOwner = true for fallback code owners and add a new
isFallbackCodeOwner field.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I6d27f4a71063de3755a6d9ca4ac911124de24f0b
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
index 4b33e11..ec3cb4f 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
@@ -33,7 +33,8 @@
* <li>any code owner config file assigns codeownership to the email for the path (see {@link
* #codeOwnerConfigFilePaths}) or the email is configured as default code owner (see {@link
* CodeOwnerCheckInfo#isDefaultCodeOwner} field) or the email is configured as global code
- * owner (see {@link #isGlobalCodeOwner} field)
+ * owner (see {@link #isGlobalCodeOwner} field) or the user is a fallback code owner (see
+ * {@link #isFallbackCodeOwner} field)
* </ul>
*/
public boolean isCodeOwner;
@@ -54,6 +55,19 @@
public List<String> codeOwnerConfigFilePaths;
/**
+ * Whether the given email is a fallback code owner of the specified path in the branch.
+ *
+ * <p>True if:
+ *
+ * <ul>
+ * <li>the given email is resolvable (see {@link #isResolvable}) and
+ * <li>no code owners are defined for the specified path in the branch and
+ * <li>parent code owners are not ignored and
+ * <li>the user is a fallback code owner according to the configured fallback code owner policy
+ */
+ public boolean isFallbackCodeOwner;
+
+ /**
* Whether the given email is configured as a default code owner.
*
* <p>If the email is configured as default code owner, but the email is not resolvable (see
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index f9a0655..7167b28 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -18,6 +18,7 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
@@ -31,6 +32,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwners;
+import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.OptionalResultWithMessages;
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwnersResult;
@@ -38,8 +40,10 @@
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.AccountResource;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.restapi.account.AccountsCollection;
import com.google.inject.Inject;
@@ -130,6 +134,8 @@
List<Path> codeOwnerConfigFilePaths = new ArrayList<>();
AtomicBoolean isCodeOwnershipAssignedToEmail = new AtomicBoolean(false);
AtomicBoolean isDefaultCodeOwner = new AtomicBoolean(false);
+ AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false);
+ AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false);
codeOwnerConfigHierarchy.visit(
branchResource.getBranchKey(),
ObjectId.fromString(branchResource.getRevision()),
@@ -166,10 +172,16 @@
"found email %s as code owner in %s", email, codeOwnerConfigFilePath));
codeOwnerConfigFilePaths.add(codeOwnerConfigFilePath);
}
+ } else if (codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, absolutePath)
+ .hasRevelantCodeOwnerDefinitions()) {
+ hasRevelantCodeOwnerDefinitions.set(true);
}
if (pathCodeOwnersResult.get().ignoreParentCodeOwners()) {
messages.add("parent code owners are ignored");
+ parentCodeOwnersAreIgnored.set(true);
}
return !pathCodeOwnersResult.get().ignoreParentCodeOwners();
@@ -185,11 +197,19 @@
boolean isResolvable = isResolvableResult.get();
messages.addAll(isResolvableResult.messages());
+ boolean isFallbackCodeOwner =
+ !isCodeOwnershipAssignedToEmail.get()
+ && !hasRevelantCodeOwnerDefinitions.get()
+ && !parentCodeOwnersAreIgnored.get()
+ && isFallbackCodeOwner(branchResource.getNameKey());
+
CodeOwnerCheckInfo codeOwnerCheckInfo = new CodeOwnerCheckInfo();
- codeOwnerCheckInfo.isCodeOwner = isCodeOwnershipAssignedToEmail.get() && isResolvable;
+ codeOwnerCheckInfo.isCodeOwner =
+ (isCodeOwnershipAssignedToEmail.get() || isFallbackCodeOwner) && isResolvable;
codeOwnerCheckInfo.isResolvable = isResolvable;
codeOwnerCheckInfo.codeOwnerConfigFilePaths =
codeOwnerConfigFilePaths.stream().map(Path::toString).collect(toList());
+ codeOwnerCheckInfo.isFallbackCodeOwner = isFallbackCodeOwner && isResolvable;
codeOwnerCheckInfo.isDefaultCodeOwner = isDefaultCodeOwner.get();
codeOwnerCheckInfo.isGlobalCodeOwner = isGlobalCodeOwner;
codeOwnerCheckInfo.debugLogs = messages;
@@ -223,6 +243,43 @@
.isPresent();
}
+ private boolean isFallbackCodeOwner(Project.NameKey projectName) {
+ FallbackCodeOwners fallbackCodeOwners =
+ codeOwnersPluginConfiguration.getFallbackCodeOwners(projectName);
+ switch (fallbackCodeOwners) {
+ case NONE:
+ return false;
+ case PROJECT_OWNERS:
+ return isProjectOwner(projectName);
+ case ALL_USERS:
+ return true;
+ }
+ throw new IllegalStateException(
+ String.format(
+ "unknown value %s for fallbackCodeOwners in project %s",
+ fallbackCodeOwners.name(), projectName));
+ }
+
+ private boolean isProjectOwner(Project.NameKey projectName) {
+ try {
+ AccountResource accountResource =
+ accountsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(email));
+ // There is no dedicated project owner permission, but project owners are detected by checking
+ // the permission to write the project config. Only project owners can do this.
+ return permissionBackend
+ .absentUser(accountResource.getUser().getAccountId())
+ .project(projectName)
+ .test(ProjectPermission.WRITE_CONFIG);
+ } catch (PermissionBackendException
+ | ResourceNotFoundException
+ | AuthException
+ | IOException
+ | ConfigInvalidException e) {
+ throw new StorageException(
+ String.format("failed if email %s is owner of project %s", email, projectName.get()), e);
+ }
+ }
+
private OptionalResultWithMessages<Boolean> isResolvable() {
if (email.equals(CodeOwnerResolver.ALL_USERS_WILDCARD)) {
return OptionalResultWithMessages.create(true);
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index f9f8e7b..8d6524b 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -53,6 +53,14 @@
check("isCodeOwner").that(codeOwnerCheckInfo().isCodeOwner).isFalse();
}
+ public void isFallbackCodeOwner() {
+ check("isFallbackCodeOwner").that(codeOwnerCheckInfo().isFallbackCodeOwner).isTrue();
+ }
+
+ public void isNotFallbackCodeOwner() {
+ check("isFallbackCodeOwner").that(codeOwnerCheckInfo().isFallbackCodeOwner).isFalse();
+ }
+
public void isResolvable() {
check("isResolvable").that(codeOwnerCheckInfo().isResolvable).isTrue();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index b3e26d8..955c142 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -32,6 +32,7 @@
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions;
@@ -1028,6 +1029,134 @@
.hasDebugLogsThatDoNotContainAnyOf(String.format("email %s", mdCodeOwner.email()));
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ public void checkFallbackCodeOwner_AllUsers() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner);
+
+ // 1. Check for a file to which fallback code owners do not apply because code owners are
+ // defined
+ String path = "/foo/bar/baz.md";
+
+ // 1a. by a code owner
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 1b. by a non code owner
+ checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 2. Check for a file to which fallback code owners apply because no code owners are defined
+ path = "/other/bar/baz.md";
+ checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isFallbackCodeOwner();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "PROJECT_OWNERS")
+ public void checkFallbackCodeOwner_ProjectOwners() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner);
+
+ // 1. Check for a file to which fallback code owners do not apply because code owners are
+ // defined
+ String path = "/foo/bar/baz.md";
+
+ // 1a. by a code owner
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 1b. by a project owner
+ checkCodeOwnerInfo = checkCodeOwner(path, admin.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 1c. by a non code owner
+ checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 2. Check for a file to which fallback code owners apply because no code owners are defined
+ path = "/other/bar/baz.md";
+
+ // 2b. by a project owner
+ checkCodeOwnerInfo = checkCodeOwner(path, admin.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isFallbackCodeOwner();
+
+ // 2b. by a non project owner
+ checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ public void noFallbackCodeOwnerIfParentCodeOwnersIgnored() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .ignoreParentCodeOwners()
+ .create();
+
+ // 1. Check for a file to which parent code owners are ignored
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+
+ // 2. Check for a file to which parent code owners are not ignored
+ path = "/other/bar/baz.md";
+ checkCodeOwnerInfo = checkCodeOwner(path, user.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isFallbackCodeOwner();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void noFallbackCodeOwnerIfNonVisibleRelevantCodeOwnerExists() throws Exception {
+ TestAccount nonVisibleCodeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(nonVisibleCodeOwner.email())
+ .create();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // verify that the account is not visible
+ assertThrows(
+ ResourceNotFoundException.class, () -> gApi.accounts().id(nonVisibleCodeOwner.id().get()));
+
+ // allow user to call the check code owner REST endpoint
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability("code-owners-" + CheckCodeOwnerCapability.ID).group(REGISTERED_USERS))
+ .update();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, admin.email());
+ assertThat(checkCodeOwnerInfo).isNotCodeOwner();
+ assertThat(checkCodeOwnerInfo).isNotFallbackCodeOwner();
+ }
+
private CodeOwnerCheckInfo checkCodeOwner(String path, String email) throws RestApiException {
return checkCodeOwner(path, email, null);
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index a4be277..a4dc690 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -303,6 +303,7 @@
"code_owner_config_file_paths": [
"/OWNERS",
],
+ "is_fallback_code_owner": "false",
"is_default_code_owner": "false",
"is_global_code_owner": "false",
"debug_logs": [
@@ -804,9 +805,10 @@
| Field Name | Description |
| --------------- | ----------- |
-| `is_code_owner` | Whether the given email owns the specified path in the branch. True if: a) the given email is resolvable (see field `is_resolvable') and b) any code owner config file assigns codeownership to the email for the path (see field `code_owner_config_file_paths`) or the email is configured as default code owner (see field `is_default_code_owner` or the email is configured as global code owner (see field `is_global_code_owner`).
+| `is_code_owner` | Whether the given email owns the specified path in the branch. True if: a) the given email is resolvable (see field `is_resolvable') and b) any code owner config file assigns codeownership to the email for the path (see field `code_owner_config_file_paths`) or the email is configured as default code owner (see field `is_default_code_owner` or the email is configured as global code owner (see field `is_global_code_owner`) or the user is a fallback code owner (see field `is_fallback_code_owner`).
| `is_resolvable` | Whether the given email is resolvable for the specified user or the calling user if no user was specified.
| `code_owner_config_file_paths` | Paths of the code owner config files that assign code ownership to the specified email and path as a list. Note that if code ownership is assigned to the email via a code owner config files, but the email is not resolvable (see field `is_resolvable` field), the user is not a code owner.
+| `is_fallback_code_owner` | Whether the given email is a fallback code owner of the specified path in the branch. True if: a) the given email is resolvable (see field `is_resolvable') and b) no code owners are defined for the specified path in the branch and c) parent code owners are not ignored and d) the user is a fallback code owner according to the [configured fallback code owner policy](config.html#pluginCodeOwnersFallbackCodeOwners)
| `is_default_code_owner` | Whether the given email is configured as a default code owner in the code owner config file in `refs/meta/config`. Note that if the email is configured as default code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner.
| `is_global_code_owner` | Whether the given email is configured as a global
code owner. Note that if the email is configured as global code owner, but the email is not resolvable (see `is_resolvable` field), the user is not a code owner.
