Merge branch 'stable-3.0' into stable-3.1

* stable-3.0:
  Fix Access when the only readable ref is a RegExp with a gerrit pattern
  Fix refPattern when using username/shardeduserid pattern with regexp
  Enable optional DEBUG level logs for query tests

Adjust RefControlTest to use projectOperations.

Change-Id: I80f6158993e0b9e4389f73ad25c3364f2a1eef62

Documentation/dev-bazel.txt

@@ -364,6 +364,21 @@ If Docker is not available, the Elasticsearch tests will be skipped.
 Note that Bazel currently does not show
 link:https://github.com/bazelbuild/bazel/issues/3476[the skipped tests].
 
+[[debug]]
+=== Index Query Tests
+
+The `DEBUG` log level can optionally be enabled for the index query tests. That log level applies to
+both Elasticsearch and Lucene tests.
+
+In Eclipse, set `-Ddebug=true` as a VM argument under the Run Configuration's `Arguments` tab.
+
+With `bazel`, here is an example for the Lucene `account` test:
+
+----
+bazel test --jvmopt='-Ddebug=true' \
+javatests/com/google/gerrit/server/query/account:lucene_query_test
+----
+
 == Dependencies
 
 Dependency JARs are normally downloaded as needed, but you can

java/com/google/gerrit/server/permissions/PermissionCollection.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.permissions;
 
 import static com.google.gerrit.common.data.PermissionRule.Action.BLOCK;
+import static com.google.gerrit.server.project.RefPattern.containsParameters;
 import static com.google.gerrit.server.project.RefPattern.isRE;
 import static java.util.stream.Collectors.mapping;
 import static java.util.stream.Collectors.toList;
@@ -131,7 +132,9 @@ public class PermissionCollection {
         Iterable<SectionMatcher> matcherList, String ref, CurrentUser user) {
       try (Timer0.Context ignored = filterLatency.start()) {
         if (isRE(ref)) {
+          if (!containsParameters(ref)) {
             ref = RefPattern.shortestExample(ref);
+          }
         } else if (ref.endsWith("/*")) {
           ref = ref.substring(0, ref.length() - 1);
         }

java/com/google/gerrit/server/project/RefPattern.java

@@ -18,9 +18,12 @@ import com.google.common.base.Throwables;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
+import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.common.data.AccessSection;
+import com.google.gerrit.common.data.ParameterizedString;
 import com.google.gerrit.exceptions.InvalidNameException;
 import dk.brics.automaton.RegExp;
+import java.util.Map;
 import java.util.concurrent.ExecutionException;
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
@@ -69,11 +72,21 @@ public class RefPattern {
     return refPattern.startsWith(AccessSection.REGEX_PREFIX);
   }
 
+  public static boolean containsParameters(String refPattern) {
+    return refPattern.contains("${");
+  }
+
   public static RegExp toRegExp(String refPattern) {
     if (isRE(refPattern)) {
       refPattern = refPattern.substring(1);
     }
-    return new RegExp(refPattern, RegExp.NONE);
+    ParameterizedString template = new ParameterizedString(refPattern);
+    String replacement = "_PLACEHOLDER_";
+    Map<String, String> params =
+        ImmutableMap.of(
+            RefPattern.USERID_SHARDED, replacement,
+            RefPattern.USERNAME, replacement);
+    return new RegExp(template.replace(params), RegExp.NONE);
   }
 
   public static void validate(String refPattern) throws InvalidNameException {

java/com/google/gerrit/server/project/RefPatternMatcher.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.project;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.server.project.RefPattern.containsParameters;
 import static com.google.gerrit.server.project.RefPattern.isRE;
 
 import com.google.common.collect.ImmutableMap;
@@ -32,7 +33,7 @@ import java.util.stream.Stream;
 
 public abstract class RefPatternMatcher {
   public static RefPatternMatcher getMatcher(String pattern) {
-    if (pattern.contains("${")) {
+    if (containsParameters(pattern)) {
       return new ExpandParameters(pattern);
     } else if (isRE(pattern)) {
       return new Regexp(pattern);
@@ -80,7 +81,7 @@ public abstract class RefPatternMatcher {
 
     @Override
     public boolean match(String ref, CurrentUser user) {
-      return pattern.matcher(ref).matches();
+      return pattern.matcher(ref).matches() || (isRE(ref) && pattern.pattern().equals(ref));
     }
   }
 
@@ -112,7 +113,11 @@ public abstract class RefPatternMatcher {
 
     @Override
     public boolean match(String ref, CurrentUser user) {
-      if (!ref.startsWith(prefix)) {
+      if (isRE(ref)) {
+        if (!ref.substring(1).startsWith(prefix)) {
+          return false;
+        }
+      } else if (!ref.startsWith(prefix)) {
         return false;
       }
 
@@ -142,6 +147,9 @@ public abstract class RefPatternMatcher {
     }
 
     public boolean matchPrefix(String ref) {
+      if (isRE(ref)) {
+        return ref.substring(1).startsWith(prefix);
+      }
       return ref.startsWith(prefix);
     }
 

javatests/com/google/gerrit/server/permissions/RefControlTest.java

@@ -476,6 +476,21 @@ public class RefControlTest {
     assertCanUpload(u);
   }
 
+  @Test
+  public void usernamePatternRegExpCanUploadToAnyRef() throws Exception {
+    projectOperations
+        .project(localKey)
+        .forUpdate()
+        .add(
+            allow(PUSH)
+                .ref("^refs/heads/users/${username}/(public|private)/.+")
+                .group(REGISTERED_USERS))
+        .update();
+    ProjectControl u = user(localKey, "a-registered-user");
+    assertCanUpload(u);
+    assertCanUpdate("refs/heads/users/a-registered-user/private/a", u);
+  }
+
   @Test
   public void usernamePatternNonRegex() throws Exception {
     projectOperations
@@ -500,6 +515,8 @@ public class RefControlTest {
 
     ProjectControl u = user(localKey, "d.v", DEVS);
     ProjectControl d = user(localKey, "dev", DEVS);
+    assertCanAccess(u);
+    assertCanAccess(d);
     assertCannotRead("refs/sb/dev/heads/foobar", u);
     assertCanRead("refs/sb/dev/heads/foobar", d);
   }
@@ -1127,6 +1144,7 @@ public class RefControlTest {
     RefPattern.validate("^refs/heads/*");
     RefPattern.validate("^refs/tags/[0-9a-zA-Z-_.]+");
     RefPattern.validate("refs/heads/review/${username}/*");
+    RefPattern.validate("^refs/heads/review/${username}/.+");
   }
 
   @Test

javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java

@@ -91,10 +91,14 @@ import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Repository;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -146,6 +150,20 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests {
 
   protected abstract Injector createInjector();
 
+  @BeforeClass
+  public static void setLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.DEBUG);
+    }
+  }
+
+  @AfterClass
+  public static void resetLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.INFO);
+    }
+  }
+
   @Before
   public void setUpInjector() throws Exception {
     lifecycle = new LifecycleManager();

javatests/com/google/gerrit/server/query/account/BUILD

@@ -23,8 +23,10 @@ java_library(
         "//lib:guava",
         "//lib:jgit",
         "//lib/guice",
+        "//lib/log:log4j",
         "//lib/truth",
         "//lib/truth:truth-java8-extension",
+        "//resources:log4j-config",
     ],
 )
 

javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -130,6 +130,8 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -141,7 +143,9 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.SystemReader;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -204,6 +208,20 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
   protected abstract Injector createInjector();
 
+  @BeforeClass
+  public static void setLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.DEBUG);
+    }
+  }
+
+  @AfterClass
+  public static void resetLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.INFO);
+    }
+  }
+
   @Before
   public void setUpInjector() throws Exception {
     lifecycle = new LifecycleManager();

javatests/com/google/gerrit/server/query/change/BUILD

@@ -32,7 +32,9 @@ java_library(
         "//lib:jgit",
         "//lib:jgit-junit",
         "//lib/guice",
+        "//lib/log:log4j",
         "//lib/truth",
+        "//resources:log4j-config",
     ],
 )
 

javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java

@@ -68,8 +68,12 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Optional;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -115,6 +119,20 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests {
 
   protected abstract Injector createInjector();
 
+  @BeforeClass
+  public static void setLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.DEBUG);
+    }
+  }
+
+  @AfterClass
+  public static void resetLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.INFO);
+    }
+  }
+
   @Before
   public void setUpInjector() throws Exception {
     lifecycle = new LifecycleManager();

javatests/com/google/gerrit/server/query/group/BUILD

@@ -20,8 +20,10 @@ java_library(
         "//lib:guava",
         "//lib:jgit",
         "//lib/guice",
+        "//lib/log:log4j",
         "//lib/truth",
         "//lib/truth:truth-java8-extension",
+        "//resources:log4j-config",
     ],
 )
 

javatests/com/google/gerrit/server/query/project/AbstractQueryProjectsTest.java

@@ -68,8 +68,12 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Set;
+import org.apache.log4j.Level;
+import org.apache.log4j.LogManager;
 import org.junit.After;
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -111,6 +115,20 @@ public abstract class AbstractQueryProjectsTest extends GerritServerTests {
 
   protected abstract Injector createInjector();
 
+  @BeforeClass
+  public static void setLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.DEBUG);
+    }
+  }
+
+  @AfterClass
+  public static void resetLogLevel() {
+    if (Boolean.getBoolean("debug")) {
+      LogManager.getRootLogger().setLevel(Level.INFO);
+    }
+  }
+
   @Before
   public void setUpInjector() throws Exception {
     lifecycle = new LifecycleManager();

javatests/com/google/gerrit/server/query/project/BUILD

@@ -21,7 +21,9 @@ java_library(
         "//lib:guava",
         "//lib:jgit",
         "//lib/guice",
+        "//lib/log:log4j",
         "//lib/truth",
+        "//resources:log4j-config",
     ],
 )