VisibleRefFilter: Fix short-circuiting when all refs visible
ProjectControl.allRefsVisibleExcept was confusingly named, and as a
result incorrectly used by VisibleRefFilter. The caller in
VisibleRefFilter was treating "except" as "check that the set of
visible refs is all refs, minus the refs in this set I'm passing."
However, the actual behavior was "check that the set of visible refs
is all refs, but ignore any permissions relating to refs in this set
I'm passing."
This means that VisibleRefFilter was erroneously removing
refs/meta/config from the result set in this fast path, even if
refs/meta/config was in fact visible. Fix this by double-checking
visibility of refs/meta/config before removing it.
Change-Id: I0b1af9a72b48548cb5be8e0af96dc05eb4b69dc1
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java
index 100c62a..82a03181 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/VisibleRefFilterIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.git;
import static com.google.gerrit.acceptance.GitUtil.createProject;
+import static com.google.gerrit.server.group.SystemGroupBackend.PROJECT_OWNERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
@@ -29,6 +30,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.project.Util;
import com.google.inject.Inject;
import org.junit.Before;
@@ -72,7 +74,12 @@
@Test
public void allRefsVisibleNoRefsMetaConfig() throws Exception {
- allow(Permission.READ, REGISTERED_USERS, "refs/*");
+ ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ Util.allow(cfg, Permission.READ, REGISTERED_USERS, "refs/*");
+ Util.allow(cfg, Permission.READ, PROJECT_OWNERS, "refs/meta/config");
+ Util.doNotInherit(cfg, Permission.READ, "refs/meta/config");
+ saveProjectConfig(project, cfg);
+
assertRefs(
"HEAD",
"refs/changes/01/1/1",
@@ -91,9 +98,8 @@
"refs/changes/01/1/1",
"refs/changes/02/2/1",
"refs/heads/branch",
- "refs/heads/master");
- // TODO(dborowitz): Fix bug so this is included.
- //"refs/meta/config");
+ "refs/heads/master",
+ "refs/meta/config");
}
@Test
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index 62d80fa..3b1a8fb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -69,10 +69,11 @@
}
public Map<String, Ref> filter(Map<String, Ref> refs, boolean filterTagsSeperately) {
- if (projectCtl.allRefsAreVisibleExcept(
- ImmutableSet.of(RefNames.REFS_CONFIG))) {
+ if (projectCtl.allRefsAreVisible(ImmutableSet.of(RefNames.REFS_CONFIG))) {
Map<String, Ref> r = Maps.newHashMap(refs);
- r.remove(RefNames.REFS_CONFIG);
+ if (!projectCtl.controlForRef(RefNames.REFS_CONFIG).isVisible()) {
+ r.remove(RefNames.REFS_CONFIG);
+ }
return r;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index 53b7368..6b09260 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -263,12 +263,12 @@
/** Can this user see all the refs in this projects? */
public boolean allRefsAreVisible() {
- return allRefsAreVisibleExcept(Collections.<String> emptySet());
+ return allRefsAreVisible(Collections.<String> emptySet());
}
- public boolean allRefsAreVisibleExcept(Set<String> except) {
+ public boolean allRefsAreVisible(Set<String> ignore) {
return user instanceof InternalUser
- || canPerformOnAllRefs(Permission.READ, except);
+ || canPerformOnAllRefs(Permission.READ, ignore);
}
/** Is this user a project owner? Ownership does not imply {@link #isVisible()} */
@@ -426,7 +426,7 @@
return false;
}
- private boolean canPerformOnAllRefs(String permission, Set<String> except) {
+ private boolean canPerformOnAllRefs(String permission, Set<String> ignore) {
boolean canPerform = false;
Set<String> patterns = allRefPatterns(permission);
if (patterns.contains(AccessSection.ALL)) {
@@ -437,7 +437,7 @@
for (final String pattern : patterns) {
if (controlForRef(pattern).canPerform(permission)) {
canPerform = true;
- } else if (except.contains(pattern)) {
+ } else if (ignore.contains(pattern)) {
continue;
} else {
return false;
