Merge "Cache all parsed config values to avoid reparsing them"
diff --git a/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
index 0508bc7..1155e35 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/BranchCodeOwners.java
@@ -96,6 +96,7 @@
abstract class CodeOwnerCheckRequest {
private String email;
private String path;
+ private String change;
private String user;
/**
@@ -131,6 +132,24 @@
}
/**
+ * Sets the change for which permissions should be checked.
+ *
+ * <p>If not specified change permissions are not checked.
+ *
+ * @param change the change for which permissions should be checked
+ */
+ public CodeOwnerCheckRequest change(@Nullable String change) {
+ this.change = change;
+ return this;
+ }
+
+ /** Returns the change for which permissions should be checked. */
+ @Nullable
+ public String getChange() {
+ return change;
+ }
+
+ /**
* Sets the user for which the code owner visibility should be checked.
*
* <p>If not specified the code owner visibility is not checked.
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
index c22449d..c96645c 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerCheckInfo.java
@@ -46,6 +46,51 @@
public boolean isResolvable;
/**
+ * Whether the user to which the given email was resolved has read permissions on the branch.
+ *
+ * <p>Not set if:
+ *
+ * <ul>
+ * <li>the given email is not resolvable
+ * <li>the given email is the all users wildcard (aka {@code *})
+ * </ul>
+ */
+ public Boolean canReadRef;
+
+ /**
+ * Whether the user to which the given email was resolved can see the specified change.
+ *
+ * <p>Not set if:
+ *
+ * <ul>
+ * <li>the given email is not resolvable
+ * <li>the given email is the all users wildcard (aka {@code *})
+ * <li>no change was specified
+ * </ul>
+ */
+ public Boolean canSeeChange;
+
+ /**
+ * Whether the user to which the given email was resolved can code-owner approve the specified
+ * change.
+ *
+ * <p>Being able to code-owner approve the change means that the user has permissions to vote on
+ * the label that is required as code owner approval. Other permissions are not considered for
+ * computing this flag. In particular missing read permissions on the change don't have any effect
+ * on this flag. Whether the user misses read permissions on the change (and hence cannot apply
+ * the code owner approval) can be seen from the {@link #canSeeChange} flag.
+ *
+ * <p>Not set if:
+ *
+ * <ul>
+ * <li>the given email is not resolvable
+ * <li>the given email is the all users wildcard (aka {@code *})
+ * <li>no change was specified
+ * </ul>
+ */
+ public Boolean canApproveChange;
+
+ /**
* Paths of the code owner config files that assign code ownership to the given email for the
* specified path.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/BranchCodeOwnersImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/BranchCodeOwnersImpl.java
index 231d591..0bf1917 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/BranchCodeOwnersImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/BranchCodeOwnersImpl.java
@@ -99,6 +99,7 @@
CheckCodeOwner checkCodeOwner = checkCodeOwnerProvider.get();
checkCodeOwner.setEmail(getEmail());
checkCodeOwner.setPath(getPath());
+ checkCodeOwner.setChange(getChange());
checkCodeOwner.setUser(getUser());
try {
return checkCodeOwner.apply(branchResource).value();
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index 64aa0dd..f4e5c0f 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -38,12 +38,18 @@
import com.google.gerrit.plugins.codeowners.backend.PathCodeOwnersResult;
import com.google.gerrit.plugins.codeowners.backend.UnresolvedImportFormatter;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
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.change.ChangeFinder;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.LabelPermission;
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.permissions.RefPermission;
import com.google.gerrit.server.project.BranchResource;
import com.google.gerrit.server.restapi.account.AccountsCollection;
import com.google.inject.Inject;
@@ -74,9 +80,12 @@
private final CodeOwners codeOwners;
private final AccountsCollection accountsCollection;
private final UnresolvedImportFormatter unresolvedImportFormatter;
+ private final ChangeFinder changeFinder;
private String email;
private String path;
+ private String change;
+ private ChangeNotes changeNotes;
private String user;
private IdentifiedUser identifiedUser;
@@ -90,7 +99,8 @@
Provider<CodeOwnerResolver> codeOwnerResolverProvider,
CodeOwners codeOwners,
AccountsCollection accountsCollection,
- UnresolvedImportFormatter unresolvedImportFormatter) {
+ UnresolvedImportFormatter unresolvedImportFormatter,
+ ChangeFinder changeFinder) {
this.checkCodeOwnerCapability = checkCodeOwnerCapability;
this.permissionBackend = permissionBackend;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
@@ -100,6 +110,7 @@
this.codeOwners = codeOwners;
this.accountsCollection = accountsCollection;
this.unresolvedImportFormatter = unresolvedImportFormatter;
+ this.changeFinder = changeFinder;
}
@Option(name = "--email", usage = "email for which the code ownership should be checked")
@@ -113,6 +124,15 @@
}
@Option(
+ name = "--change",
+ usage =
+ "change for which permissions should be checked,"
+ + " if not specified change permissions are not checked")
+ public void setChange(String change) {
+ this.change = change;
+ }
+
+ @Option(
name = "--user",
usage =
"user for which the code owner visibility should be checked,"
@@ -127,7 +147,7 @@
PermissionBackendException {
permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission());
- validateInput();
+ validateInput(branchResource);
Path absolutePath = JgitPath.of(path).getAsAbsolutePath();
List<String> messages = new ArrayList<>();
@@ -231,9 +251,36 @@
isCodeOwnershipAssignedToAllUsers.set(true);
}
- OptionalResultWithMessages<Boolean> isResolvableResult = isResolvable();
- boolean isResolvable = isResolvableResult.get();
- messages.addAll(isResolvableResult.messages());
+ boolean isResolvable;
+ Boolean canReadRef = null;
+ Boolean canSeeChange = null;
+ Boolean canApproveChange = null;
+ if (email.equals(CodeOwnerResolver.ALL_USERS_WILDCARD)) {
+ isResolvable = true;
+ } else {
+ OptionalResultWithMessages<CodeOwner> isResolvableResult = isResolvable();
+ isResolvable = isResolvableResult.isPresent();
+ messages.addAll(isResolvableResult.messages());
+
+ if (isResolvable) {
+ PermissionBackend.WithUser withUser =
+ permissionBackend.absentUser(isResolvableResult.get().accountId());
+ canReadRef = withUser.ref(branchResource.getBranchKey()).test(RefPermission.READ);
+
+ if (changeNotes != null) {
+ PermissionBackend.ForChange forChange = withUser.change(changeNotes);
+ canSeeChange = forChange.test(ChangePermission.READ);
+ RequiredApproval requiredApproval =
+ codeOwnersPluginConfiguration
+ .getProjectConfig(branchResource.getNameKey())
+ .getRequiredApproval();
+ canApproveChange =
+ forChange.test(
+ new LabelPermission.WithValue(
+ requiredApproval.labelType(), requiredApproval.value()));
+ }
+ }
+ }
boolean isFallbackCodeOwner =
!isCodeOwnershipAssignedToEmail.get()
@@ -249,6 +296,9 @@
|| isFallbackCodeOwner)
&& isResolvable;
codeOwnerCheckInfo.isResolvable = isResolvable;
+ codeOwnerCheckInfo.canReadRef = canReadRef;
+ codeOwnerCheckInfo.canSeeChange = canSeeChange;
+ codeOwnerCheckInfo.canApproveChange = canApproveChange;
codeOwnerCheckInfo.codeOwnerConfigFilePaths =
codeOwnerConfigFilePaths.stream().map(Path::toString).collect(toList());
codeOwnerCheckInfo.isFallbackCodeOwner = isFallbackCodeOwner && isResolvable;
@@ -259,8 +309,9 @@
return Response.ok(codeOwnerCheckInfo);
}
- private void validateInput()
- throws BadRequestException, AuthException, IOException, ConfigInvalidException {
+ private void validateInput(BranchResource branchResource)
+ throws BadRequestException, AuthException, IOException, ConfigInvalidException,
+ PermissionBackendException {
if (email == null) {
throw new BadRequestException("email required");
}
@@ -277,6 +328,21 @@
throw new BadRequestException(String.format("user %s not found", user), e);
}
}
+ if (change != null) {
+ Optional<ChangeNotes> changeNotes = changeFinder.findOne(change);
+ if (!changeNotes.isPresent()
+ || !permissionBackend
+ .currentUser()
+ .change(changeNotes.get())
+ .test(ChangePermission.READ)) {
+ throw new BadRequestException(String.format("change %s not found", change));
+ }
+ if (!changeNotes.get().getChange().getDest().equals(branchResource.getBranchKey())) {
+ throw new BadRequestException(
+ "target branch of specified change must match branch from the request URL");
+ }
+ this.changeNotes = changeNotes.get();
+ }
}
private boolean isGlobalCodeOwner(Project.NameKey projectName, String email) {
@@ -324,11 +390,7 @@
}
}
- private OptionalResultWithMessages<Boolean> isResolvable() {
- if (email.equals(CodeOwnerResolver.ALL_USERS_WILDCARD)) {
- return OptionalResultWithMessages.create(true);
- }
-
+ private OptionalResultWithMessages<CodeOwner> isResolvable() {
CodeOwnerResolver codeOwnerResolver = codeOwnerResolverProvider.get();
if (identifiedUser != null) {
codeOwnerResolver.forUser(identifiedUser);
@@ -341,6 +403,9 @@
List<String> messages = new ArrayList<>();
messages.add(String.format("trying to resolve email %s", email));
messages.addAll(resolveResult.messages());
- return OptionalResultWithMessages.create(resolveResult.isPresent(), messages);
+ if (resolveResult.isPresent()) {
+ return OptionalResultWithMessages.create(resolveResult.get(), messages);
+ }
+ return OptionalResultWithMessages.createEmpty(messages);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index 2d33006..791705c 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -69,6 +69,42 @@
check("isResolvable").that(codeOwnerCheckInfo().isResolvable).isFalse();
}
+ public void canReadRef() {
+ check("canReadRef").that(codeOwnerCheckInfo().canReadRef).isTrue();
+ }
+
+ public void cannotReadRef() {
+ check("canReadRef").that(codeOwnerCheckInfo().canReadRef).isFalse();
+ }
+
+ public void canReadRefNotSet() {
+ check("canReadRef").that(codeOwnerCheckInfo().canReadRef).isNull();
+ }
+
+ public void canSeeChange() {
+ check("canSeeChange").that(codeOwnerCheckInfo().canSeeChange).isTrue();
+ }
+
+ public void cannotSeeChange() {
+ check("canSeeChange").that(codeOwnerCheckInfo().canSeeChange).isFalse();
+ }
+
+ public void canSeeChangeNotSet() {
+ check("canSeeChange").that(codeOwnerCheckInfo().canSeeChange).isNull();
+ }
+
+ public void canApproveChange() {
+ check("canApproveChange").that(codeOwnerCheckInfo().canApproveChange).isTrue();
+ }
+
+ public void cannotApproveChange() {
+ check("canApproveChange").that(codeOwnerCheckInfo().canApproveChange).isFalse();
+ }
+
+ public void canApproveChangeNotSet() {
+ check("canApproveChange").that(codeOwnerCheckInfo().canApproveChange).isNull();
+ }
+
public IterableSubject hasCodeOwnerConfigFilePathsThat() {
return check("codeOwnerConfigFilePaths").that(codeOwnerCheckInfo().codeOwnerConfigFilePaths);
}
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 238dc9f..0db2388 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -15,7 +15,10 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerCheckInfoSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -27,8 +30,10 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -130,6 +135,9 @@
CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
assertThat(checkCodeOwnerInfo).isCodeOwner();
assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).canReadRef();
+ assertThat(checkCodeOwnerInfo).canSeeChangeNotSet();
+ assertThat(checkCodeOwnerInfo).canApproveChangeNotSet();
assertThat(checkCodeOwnerInfo)
.hasCodeOwnerConfigFilePathsThat()
.containsExactly(getCodeOwnerConfigFilePath("/foo/"));
@@ -508,6 +516,9 @@
checkCodeOwner(ROOT_PATH, CodeOwnerResolver.ALL_USERS_WILDCARD);
assertThat(checkCodeOwnerInfo).isNotCodeOwner();
assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).canReadRefNotSet();
+ assertThat(checkCodeOwnerInfo).canSeeChangeNotSet();
+ assertThat(checkCodeOwnerInfo).canApproveChangeNotSet();
assertThat(checkCodeOwnerInfo).hasCodeOwnerConfigFilePathsThat().isEmpty();
assertThat(checkCodeOwnerInfo).isNotDefaultCodeOwner();
assertThat(checkCodeOwnerInfo).isNotGlobalCodeOwner();
@@ -1290,8 +1301,150 @@
assertThat(checkCodeOwnerInfo).hasDebugLogsThatDoNotContainAnyOf("");
}
+ @Test
+ public void checkCodeOwnerThatCannotReadRef() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner);
+
+ // Make read permission on master branch exclusive for admins, so that the code owner cannot
+ // read master.
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allow(Permission.READ).ref("refs/heads/master").group(adminGroupUuid()))
+ .setExclusiveGroup(permissionKey(Permission.READ).ref("refs/heads/master"), true)
+ .update();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo = checkCodeOwner(path, codeOwner.email());
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).cannotReadRef();
+ assertThat(checkCodeOwnerInfo).canSeeChangeNotSet();
+ assertThat(checkCodeOwnerInfo).canApproveChangeNotSet();
+ }
+
+ @Test
+ public void cannotCheckForNonExistingChange() throws Exception {
+ String nonExistingChange = "non-existing";
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class,
+ () -> checkCodeOwnerForChange("/", user.email(), nonExistingChange));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("change %s not found", nonExistingChange));
+ }
+
+ @Test
+ public void cannotCheckForNonVisibleChange() throws Exception {
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).setPrivate(true);
+
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability("code-owners-" + CheckCodeOwnerCapability.ID).group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class, () -> checkCodeOwnerForChange("/", user.email(), changeId));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(String.format("change %s not found", changeId));
+ }
+
+ @Test
+ public void cannotCheckForChangeOfOtherBranch() throws Exception {
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ String changeId = createChange("refs/for/" + branchName).getChangeId();
+ BadRequestException exception =
+ assertThrows(
+ BadRequestException.class, () -> checkCodeOwnerForChange("/", user.email(), changeId));
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo("target branch of specified change must match branch from the request URL");
+ }
+
+ @Test
+ public void checkCodeOwnerThatCannotSeeChange_privateChange() throws Exception {
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).setPrivate(true);
+
+ testCheckCodeOwnerThatCannotSeeChange(changeId, /* canReadRef= */ true);
+ }
+
+ @Test
+ public void checkCodeOwnerThatCannotSeeChange_cannotReadRef() throws Exception {
+ String changeId = createChange().getChangeId();
+
+ // Make read permission on master branch exclusive for admins, so that the code owner cannot
+ // read master.
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allow(Permission.READ).ref("refs/heads/master").group(adminGroupUuid()))
+ .setExclusiveGroup(permissionKey(Permission.READ).ref("refs/heads/master"), true)
+ .update();
+
+ testCheckCodeOwnerThatCannotSeeChange(changeId, /* canReadRef= */ false);
+ }
+
+ private void testCheckCodeOwnerThatCannotSeeChange(String changeId, boolean canReadRef)
+ throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner);
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo =
+ checkCodeOwnerForChange(path, codeOwner.email(), changeId);
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ if (canReadRef) {
+ assertThat(checkCodeOwnerInfo).canReadRef();
+ } else {
+ assertThat(checkCodeOwnerInfo).cannotReadRef();
+ }
+ assertThat(checkCodeOwnerInfo).cannotSeeChange();
+ assertThat(checkCodeOwnerInfo).canApproveChange();
+ }
+
+ @Test
+ public void checkCodeOwnerThatCannotApproveChange() throws Exception {
+ String changeId = createChange().getChangeId();
+
+ // Remove permission to vote on the Code-Review label.
+ projectOperations
+ .allProjectsForUpdate()
+ .remove(labelPermissionKey("Code-Review").ref("refs/heads/*"))
+ .update();
+
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "Code Owner", /* displayName= */ null);
+ setAsCodeOwners("/foo/", codeOwner);
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnerCheckInfo checkCodeOwnerInfo =
+ checkCodeOwnerForChange(path, codeOwner.email(), changeId);
+ assertThat(checkCodeOwnerInfo).isCodeOwner();
+ assertThat(checkCodeOwnerInfo).isResolvable();
+ assertThat(checkCodeOwnerInfo).canReadRef();
+ assertThat(checkCodeOwnerInfo).canSeeChange();
+ assertThat(checkCodeOwnerInfo).cannotApproveChange();
+ }
+
private CodeOwnerCheckInfo checkCodeOwner(String path, String email) throws RestApiException {
- return checkCodeOwner(path, email, null);
+ return checkCodeOwner(path, email, /* user= */ null);
}
private CodeOwnerCheckInfo checkCodeOwner(String path, String email, @Nullable String user)
@@ -1306,6 +1459,18 @@
.check();
}
+ private CodeOwnerCheckInfo checkCodeOwnerForChange(
+ String path, String email, @Nullable String change) throws RestApiException {
+ return projectCodeOwnersApiFactory
+ .project(project)
+ .branch("master")
+ .checkCodeOwner()
+ .path(path)
+ .email(email)
+ .change(change)
+ .check();
+ }
+
private String getCodeOwnerConfigFilePath(String folderPath) {
assertThat(folderPath).startsWith("/");
assertThat(folderPath).endsWith("/");
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index ab8b46a..3202450 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -106,13 +106,14 @@
Root code owners can differ from branch to branch.
3. Default code owners:
[Default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration)
- are stored in the code owner config file in the `refs/meta/config` branch
- that apply for all branches (unless inheritance is ignored).\
- The same as root code owners these are experienced developers that can
- approve changes to all the code base if needed.\
- However in contrast to root code owners that apply to all branches (including
- newly created branches), and hence can be used if code owners should be kept
- consistent across all branches.\
+ are stored in the code owner config file (e.g. the `OWNERS` file) in the
+ `refs/meta/config` branch and apply for all branches (unless inheritance is
+ ignored).\
+ The same as root code owners, default code owners are experienced developers
+ that can approve changes to all the code base if needed.\
+ However in contrast to root code owners, default code owners apply to all
+ branches (including newly created branches), and hence can be used if code
+ owners should be kept consistent across all branches.\
A small disadvantage is that this code owner definition is not very well
discoverable since it is stored in the `refs/meta/config` branch, but default
code owners are suggested to users the same way as other code owners.
diff --git a/resources/Documentation/how-to-use.md b/resources/Documentation/how-to-use.md
index d412c2c..23337be 100644
--- a/resources/Documentation/how-to-use.md
+++ b/resources/Documentation/how-to-use.md
@@ -151,20 +151,25 @@
-- No owners were defined for these files.
+- No code owners were defined for these files.
Reason: This could be due to missing `OWNERS` defined for these files.
- None of the code owners of these files are visible.
Reason: The code owners accounts are not visible to you.
+- None of the code owners can see the change.
+ Reason: The code owners have no read permission on the target branch of the
+ change and hence cannot approve the change.
+
- Code owners defined for these files are invalid.
Reason: The emails cannot be resolved.
-For these 3 cases, we advise you to:
+For these cases, we advise you to:
1. Ask someone with override powers (e.g. sheriff) to grant an override vote to
unblock the change submission.
-2. Contact the project owner to ask them to fix the code owner definitions.
+2. Contact the project owner to ask them to fix the code owner definitions, or
+ permissions if needed.
### Renamed files
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index 20a6e5f..df39f6f 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -268,6 +268,7 @@
| ----------- | --------- | ----------- |
| `email` | mandatory | Email for which the code ownership should be checked.
| `path` | mandatory | Path for which the code ownership should be checked.
+| `change` | optional | Change for which permissions should be checked. If not specified change permissions are not checked.
| `user` | optional | User for which the code owner visibility should be checked. If not specified the code owner visibility is not checked. Can be used to investigate why a code owner is not shown/suggested to this user.
Requires that the caller has the [Check Code Owner](#checkCodeOwner) or the
@@ -300,6 +301,7 @@
{
"is_code_owner": "false",
"is_resolvable": "false",
+ "can_read_ref": "true",
"code_owner_config_file_paths": [
"/OWNERS",
],
@@ -823,6 +825,9 @@
| --------------- | ----------- |
| `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.
+| `can_read_ref` | Whether the user to which the given email was resolved has read permissions on the branch. Not set if the given email is not resolvable or if the given email is the all users wildcard (aka '*').
+| `can_see_change`| Whether the user to which the given email was resolved can see the specified change. Not set if the given email is not resolvable, if the given email is the all users wildcard (aka '*') or if no change was specified.
+| `can_approve_change`| Whether the user to which the given email was resolved can code-owner approve the specified change. Being able to code-owner approve the change means that the user has permissions to vote on the label that is [required as code owner approval](config.html#pluginCodeOwnersRequiredApproval). Other permissions are not considered for computing this flag. In particular missing read permissions on the change don't have any effect on this flag. Whether the user misses read permissions on the change (and hence cannot apply the code owner approval) can be seen from the `can_see_change` flag. Not set if the given email is not resolvable, if the given email is the all users wildcard (aka '*') or if no change 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.
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js
index bf9fa4e..f7ee6d2 100644
--- a/ui/code-owners-service.js
+++ b/ui/code-owners-service.js
@@ -136,11 +136,13 @@
async getStatus() {
const status = await this._getStatus();
- if (status.enabled && !this.isOnLatestPatchset(status.patchsetNumber)) {
- // status is outdated, abort and re-init
+ if (status.enabled && this._isOnOlderPatchset(status.patchsetNumber)) {
+ // status is returned for an older patchset. Abort, re-init and refetch
+ // new status - it is expected, that after several retry a status
+ // for the newest patchset is returned
this.reset();
this.prefetch();
- return await this.codeOwnersCacheApi.getStatus();
+ return await this.getStatus();
}
return status;
}
@@ -153,18 +155,21 @@
enabled: false,
codeOwnerStatusMap: new Map(),
rawStatuses: [],
+ newerPatchsetUploaded: false,
};
}
- const onwerStatus = await this.codeOwnersCacheApi.listOwnerStatus();
+ const ownerStatus = await this.codeOwnersCacheApi.listOwnerStatus();
return {
enabled: true,
- patchsetNumber: onwerStatus.patch_set_number,
+ patchsetNumber: ownerStatus.patch_set_number,
codeOwnerStatusMap: this._formatStatuses(
- onwerStatus.file_code_owner_statuses
+ ownerStatus.file_code_owner_statuses
),
- rawStatuses: onwerStatus.file_code_owner_statuses,
+ rawStatuses: ownerStatus.file_code_owner_statuses,
+ newerPatchsetUploaded:
+ this._isOnNewerPatchset(ownerStatus.patch_set_number),
};
}
@@ -274,9 +279,14 @@
});
}
- isOnLatestPatchset(patchsetId) {
+ _isOnNewerPatchset(patchsetId) {
const latestRevision = this.change.revisions[this.change.current_revision];
- return `${latestRevision._number}` === `${patchsetId}`;
+ return patchsetId > latestRevision._number;
+ }
+
+ _isOnOlderPatchset(patchsetId) {
+ const latestRevision = this.change.revisions[this.change.current_revision];
+ return patchsetId < latestRevision._number;
}
reset() {
diff --git a/ui/owner-requirement.js b/ui/owner-requirement.js
index 5fcbeec..6bc3465 100644
--- a/ui/owner-requirement.js
+++ b/ui/owner-requirement.js
@@ -66,17 +66,22 @@
<template is="dom-if" if="[[!_isLoading]]">
<template is="dom-if" if="[[!_pluginFailed(model.pluginStatus)]]">
<template is="dom-if" if="[[!model.branchConfig.no_code_owners_defined]]">
- <span>[[_computeStatusText(_statusCount, _isOverriden)]]</span>
- <template is="dom-if" if="[[_overrideInfoUrl]]">
- <a on-click="_reportDocClick" href="[[_overrideInfoUrl]]"
- target="_blank">
- <iron-icon icon="gr-icons:help-outline"
- title="Documentation for overriding code owners"></iron-icon>
- </a>
+ <template is="dom-if" if="[[!_newerPatchsetUploaded]]">
+ <span>[[_computeStatusText(_statusCount, _isOverriden)]]</span>
+ <template is="dom-if" if="[[_overrideInfoUrl]]">
+ <a on-click="_reportDocClick" href="[[_overrideInfoUrl]]"
+ target="_blank">
+ <iron-icon icon="gr-icons:help-outline"
+ title="Documentation for overriding code owners"></iron-icon>
+ </a>
+ </template>
+ <gr-button link on-click="_openReplyDialog">
+ [[_getSuggestOwnersText(_statusCount)]]
+ </gr-button>
</template>
- <gr-button link on-click="_openReplyDialog">
- [[_getSuggestOwnersText(_statusCount)]]
- </gr-button>
+ <template is="dom-if" if="[[_newerPatchsetUploaded]]">
+ <span>A newer patch set has been uploaded.</span>
+ </template>
</template>
<template is="dom-if" if="[[model.branchConfig.no_code_owners_defined]]">
<span>No code-owners file</span>
@@ -99,6 +104,7 @@
static get properties() {
return {
_statusCount: Object,
+ _newerPatchsetUploaded: Boolean,
_isLoading: {
type: Boolean,
computed: '_computeIsLoading(model.branchConfig, model.status, ' +
@@ -143,10 +149,12 @@
_onStatusChanged(status, userRole) {
if (!status || !userRole) {
this._statusCount = undefined;
+ this._newerPatchsetUploaded = undefined;
return;
}
const rawStatuses = status.rawStatuses;
this._statusCount = this._getStatusCount(rawStatuses);
+ this._newerPatchsetUploaded = status.newerPatchsetUploaded;
this.reporting.reportLifeCycle('owners-submit-requirement-summary-shown',
{...this._statusCount, user_role: userRole});
}
diff --git a/ui/owner-status-column.js b/ui/owner-status-column.js
index 8f57a2b..ae56d36 100644
--- a/ui/owner-status-column.js
+++ b/ui/owner-status-column.js
@@ -48,14 +48,18 @@
};
class BaseEl extends CodeOwnersModelMixin(Polymer.Element) {
- computeHidden(change, patchRange) {
- if ([change, patchRange].includes(undefined)) return true;
+ computeHidden(change, patchRange, newerPatchsetUploaded) {
+ if ([change, patchRange, newerPatchsetUploaded].includes(undefined)) {
+ return true;
+ }
// if code-owners is not a submit requirement, don't show status column
if (change.requirements &&
!change.requirements.find(r => r.type === 'code-owners')) {
return true;
}
+ if (newerPatchsetUploaded) return true;
+
const latestPatchset = change.revisions[change.current_revision];
// only show if its comparing against base
if (patchRange.basePatchNum !== 'PARENT') return true;
@@ -97,7 +101,8 @@
hidden: {
type: Boolean,
reflectToAttribute: true,
- computed: 'computeHidden(change, patchRange)',
+ computed: 'computeHidden(change, patchRange, ' +
+ 'model.status.newerPatchsetUploaded)',
},
};
}
