Merge branch 'stable-2.14' into stable-2.15 * stable-2.14: Update git submodules Update git submodules ldap.Helper: Use local logger and make logger in LdapRealm private Remove ValidationError#createLoggerSink to avoid passing around loggers LdapLoginServlet: Improve exception handling OperatingSystemMXBeanProvider: Log exception for ReflectiveOperationException WorkQueue: Don't fail when queue metric already exists WorkQueue: Sanitize metric name when queue is created DropWizardMetricMaker: Introduce method to sanitize metric name Change-Id: I4729d537aeb5ef934fcae90b610e28966a6ada9a
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java index 24ba4ac..4671475 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java
@@ -30,6 +30,7 @@ import com.google.gerrit.server.account.AccountUserNameException; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.AuthResult; +import com.google.gerrit.server.account.AuthenticationFailedException; import com.google.gerrit.server.auth.AuthenticationUnavailableException; import com.google.gwtexpui.server.CacheHeaders; import com.google.inject.Inject; @@ -126,10 +127,16 @@ } catch (AuthenticationUnavailableException e) { sendForm(req, res, "Authentication unavailable at this time."); return; - } catch (AccountException e) { - log.info(String.format("'%s' failed to sign in: %s", username, e.getMessage())); + } catch (AuthenticationFailedException e) { + // This exception is thrown if the user provided wrong credentials, we don't need to log a + // stacktrace for it. + log.warn("'{}' failed to sign in: {}", username, e.getMessage()); sendForm(req, res, "Invalid username or password."); return; + } catch (AccountException e) { + log.warn("'{}' failed to sign in", username, e); + sendForm(req, res, "Authentication failed."); + return; } catch (RuntimeException e) { log.error("LDAP authentication failed", e); sendForm(req, res, "Authentication unavailable at this time.");
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java index 349cd05..8c013ed 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java
@@ -30,7 +30,6 @@ import org.slf4j.LoggerFactory; public class AllProjectsConfig extends VersionedMetaDataOnInit { - private static final Logger log = LoggerFactory.getLogger(AllProjectsConfig.class); private Config cfg; @@ -65,7 +64,7 @@ return GroupList.parse( new Project.NameKey(project), readUTF8(GroupList.FILE_NAME), - GroupList.createLoggerSink(GroupList.FILE_NAME, log)); + error -> log.error("Error parsing file {}: {}", GroupList.FILE_NAME, error.getMessage())); } public void save(String pluginName, String message) throws IOException, ConfigInvalidException {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java index af5350e..37efb78 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java
@@ -344,6 +344,25 @@ METRIC_NAME_PATTERN.pattern()); } + public static String sanitizeMetricName(String input) { + if (METRIC_NAME_PATTERN.matcher(input).matches()) { + return input; + } + + String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_"); + if (input.length() == 1) { + return first; + } + + String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_"); + + if (result.endsWith("/")) { + result = result.substring(0, result.length() - 1); + } + + return result; + } + static String name(Description.FieldOrdering ordering, String codeName, String fieldValues) { if (ordering == FieldOrdering.PREFIX_FIELDS_BASENAME) { int s = codeName.lastIndexOf('/');
diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java index 7256e8c..bc2846a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java
@@ -41,7 +41,7 @@ return new OperatingSystemMXBeanProvider(sys); } } catch (ReflectiveOperationException e) { - log.debug(String.format("No implementation for %s: %s", name, e.getMessage())); + log.debug("No implementation for {}", name, e); } } log.warn("No implementation of UnixOperatingSystemMXBean found");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java index 1e1e54a..7808edd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java
@@ -17,8 +17,6 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.git.DestinationList; -import com.google.gerrit.server.git.TabFile; -import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.git.VersionedMetaData; import java.io.IOException; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -62,8 +60,10 @@ String path = p.path; if (path.startsWith(prefix)) { String label = path.substring(prefix.length()); - ValidationError.Sink errors = TabFile.createLoggerSink(path, log); - destinations.parseLabel(label, readUTF8(path), errors); + destinations.parseLabel( + label, + readUTF8(path), + error -> log.error("Error parsing file {}: {}", path, error.getMessage())); } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java index e1fa8d6..af0463a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java
@@ -52,7 +52,9 @@ protected void onLoad() throws IOException, ConfigInvalidException { queryList = QueryList.parse( - readUTF8(QueryList.FILE_NAME), QueryList.createLoggerSink(QueryList.FILE_NAME, log)); + readUTF8(QueryList.FILE_NAME), + error -> + log.error("Error parsing file {}: {}", QueryList.FILE_NAME, error.getMessage())); } @Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java index 20a2be6..fd88845 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
@@ -52,9 +52,13 @@ import javax.security.auth.login.LoginContext; import javax.security.auth.login.LoginException; import org.eclipse.jgit.lib.Config; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @Singleton class Helper { + private static final Logger log = LoggerFactory.getLogger(Helper.class); + static final String LDAP_UUID = "ldap:"; private final Cache<String, ImmutableSet<String>> parentGroups; @@ -151,7 +155,7 @@ } catch (PrivilegedActionException e) { Throwables.throwIfInstanceOf(e.getException(), NamingException.class); Throwables.throwIfInstanceOf(e.getException(), RuntimeException.class); - LdapRealm.log.warn("Internal error", e.getException()); + log.warn("Internal error", e.getException()); return null; } finally { ctx.logout(); @@ -296,7 +300,7 @@ } } } catch (NamingException e) { - LdapRealm.log.warn("Could not find group " + groupDN, e); + log.warn("Could not find group {}", groupDN, e); } cachedParentsDNs = dns.build(); parentGroups.put(groupDN, cachedParentsDNs); @@ -429,10 +433,10 @@ try { return LdapType.guessType(ctx); } catch (NamingException e) { - LdapRealm.log.warn( - "Cannot discover type of LDAP server at " - + server - + ", assuming the server is RFC 2307 compliant.", + log.warn( + "Cannot discover type of LDAP server at {}," + + " assuming the server is RFC 2307 compliant.", + server, e); return LdapType.RFC_2307; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java index e0226d4..84f68f9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
@@ -60,7 +60,8 @@ @Singleton class LdapRealm extends AbstractRealm { - static final Logger log = LoggerFactory.getLogger(LdapRealm.class); + private static final Logger log = LoggerFactory.getLogger(LdapRealm.class); + static final String LDAP = "com.sun.jndi.ldap.LdapCtxFactory"; static final String USERNAME = "username";
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java index ea041f4..c417965 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java
@@ -23,7 +23,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import org.slf4j.Logger; public class TabFile { public interface Parser { @@ -145,8 +144,4 @@ } return r.toString(); } - - public static ValidationError.Sink createLoggerSink(String file, Logger log) { - return ValidationError.createLoggerSink("Error parsing file " + file + ": ", log); - } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java index 448ab15..ad84046 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java
@@ -14,8 +14,6 @@ package com.google.gerrit.server.git; -import org.slf4j.Logger; - /** Indicates a problem with Git based data. */ public class ValidationError { private final String message; @@ -44,13 +42,4 @@ public interface Sink { void error(ValidationError error); } - - public static Sink createLoggerSink(String message, Logger log) { - return new ValidationError.Sink() { - @Override - public void error(ValidationError error) { - log.error(message + error.getMessage()); - } - }; - } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java index 6c1d3b0..bdd5e61 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
@@ -14,6 +14,8 @@ package com.google.gerrit.server.git; +import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName; + import com.google.common.base.CaseFormat; import com.google.common.base.Supplier; import com.google.gerrit.extensions.events.LifecycleListener; @@ -220,7 +222,15 @@ corePoolSize + 4 // concurrency level ); queueName = prefix; - buildMetrics(queueName, metrics); + try { + buildMetrics(queueName, metrics); + } catch (IllegalArgumentException e) { + if (e.getMessage().contains("already")) { + log.warn("Not creating metrics for queue '{}': already exists", queueName); + } else { + throw e; + } + } } private void buildMetrics(String queueName, MetricMaker metric) { @@ -299,7 +309,7 @@ CaseFormat.UPPER_CAMEL.to( CaseFormat.LOWER_UNDERSCORE, queueName.replaceFirst("SSH", "Ssh").replaceAll("-", "")); - return String.format("queue/%s/%s", name, metricName); + return sanitizeMetricName(String.format("queue/%s/%s", name, metricName)); } @Override
diff --git a/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java new file mode 100644 index 0000000..26b98c6 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
@@ -0,0 +1,42 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.metrics.dropwizard; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName; + +import org.junit.Test; + +public class DropWizardMetricMakerTest { + @Test + public void shouldSanitizeUnwantedChars() throws Exception { + assertThat(sanitizeMetricName("very+confusing$long#metric@net/name^1")) + .isEqualTo("very_confusing_long_metric_net/name_1"); + assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric"); + } + + @Test + public void shouldReduceConsecutiveSlashesToOne() throws Exception { + assertThat(sanitizeMetricName("/metric//submetric1///submetric2/submetric3")) + .isEqualTo("_metric/submetric1/submetric2/submetric3"); + } + + @Test + public void shouldNotFinishWithSlash() throws Exception { + assertThat(sanitizeMetricName("metric/")).isEqualTo("metric"); + assertThat(sanitizeMetricName("metric//")).isEqualTo("metric"); + assertThat(sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric"); + } +}