Fix a bug where the default HeadersSanitizer in logging decorators

Motivation:

`HeadersSanitizer` was added in line#5188. If no `HeadersSanitizer` is set,
the default santizer masks `Authorization`, `Cookie` `Set-Cookie` and
`Proxy-Authorization` with `****`.

In addition to `HeadersSantizer`, other new features are added to
`LoggingDecoratorBuilder` and the code became messy to maintain
backward compatibility. `LoggingDecoratorBuilder`, `LogWriterBuilder`,
and `LogFormatterBuilder` each have their own properties, so it was not
easy to make the overall operation consistent.

As a result, while `LoggingClient.newDecorator()` santizes sensitive
headers, LoggingClient.builder().requestLogLevel(...).newDecorator()`
did not work. Neither code set a sanitizer, so a default sanitizer
should have been set, but it didn't.

Modifications:

- Refactor `LoggingDecoratorBuilder` to delegate all builder properties
  to either `LogWriterBuilder` or `LogFormatterBuilder`.
- Add missing `@Deprecated` annotation to `LoggingClientBuilder` and
  `LoggingRpcClientBuilder` and `LoggingServiceBuilder`.
  - `@Deprecated` in the super method aren't inspected by IntelliJ line#5395
- Breaking) Change the signature of `LogWriterBuilder.responseCauseFilter()`
  to have `RequestContext` as the first parameter.
  - It may be a bug or a mistake when adding the API. Our APIs
    usually provide `RequestContext` for predicates or handlers.

Result:

`LoggingClient` and `LoggingService` now correctly mask sensitive
headers by default.

Author: ikhoon

core/src/main/java/com/linecorp/armeria/client/logging/LoggingClientBuilder.java

@@ -61,8 +61,8 @@ public Function<? super HttpClient, LoggingClient> newDecorator() {
     // Override the return type of the chaining methods in the superclass.
 
     @Override
-    protected LoggingClientBuilder defaultLogger(Logger logger) {
-        return (LoggingClientBuilder) super.defaultLogger(logger);
+    protected LoggingClientBuilder defaultLogger(Logger defaultLogger) {
+        return (LoggingClientBuilder) super.defaultLogger(defaultLogger);
     }
 
     @Override
@@ -95,23 +95,33 @@ public LoggingClientBuilder failureSamplingRate(float samplingRate) {
         return (LoggingClientBuilder) super.failureSamplingRate(samplingRate);
     }
 
+    @Override
+    public LoggingClientBuilder logWriter(LogWriter logWriter) {
+        return (LoggingClientBuilder) super.logWriter(logWriter);
+    }
+
     // Override the return type of the chaining methods in the super-superclass.
+    // All methods below are deprecated.
 
+    @Deprecated
     @Override
     public LoggingClientBuilder logger(Logger logger) {
         return (LoggingClientBuilder) super.logger(logger);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder logger(String loggerName) {
         return (LoggingClientBuilder) super.logger(loggerName);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder requestLogLevel(LogLevel requestLogLevel) {
         return (LoggingClientBuilder) super.requestLogLevel(requestLogLevel);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder requestLogLevel(Class<? extends Throwable> clazz, LogLevel requestLogLevel) {
         return (LoggingClientBuilder) super.requestLogLevel(clazz, requestLogLevel);
@@ -124,31 +134,37 @@ public LoggingClientBuilder requestLogLevelMapper(
         return (LoggingClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder requestLogLevelMapper(RequestLogLevelMapper requestLogLevelMapper) {
         return (LoggingClientBuilder) super.requestLogLevelMapper(requestLogLevelMapper);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder responseLogLevel(HttpStatus status, LogLevel logLevel) {
         return (LoggingClientBuilder) super.responseLogLevel(status, logLevel);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder responseLogLevel(HttpStatusClass statusClass, LogLevel logLevel) {
         return (LoggingClientBuilder) super.responseLogLevel(statusClass, logLevel);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder responseLogLevel(Class<? extends Throwable> clazz, LogLevel logLevel) {
         return (LoggingClientBuilder) super.responseLogLevel(clazz, logLevel);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder successfulResponseLogLevel(LogLevel successfulResponseLogLevel) {
         return (LoggingClientBuilder) super.successfulResponseLogLevel(successfulResponseLogLevel);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder failureResponseLogLevel(LogLevel failureResponseLogLevel) {
         return (LoggingClientBuilder) super.failureResponseLogLevel(failureResponseLogLevel);
@@ -161,6 +177,7 @@ public LoggingClientBuilder responseLogLevelMapper(
         return (LoggingClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder responseLogLevelMapper(ResponseLogLevelMapper responseLogLevelMapper) {
         return (LoggingClientBuilder) super.responseLogLevelMapper(responseLogLevelMapper);
@@ -238,13 +255,9 @@ public LoggingClientBuilder responseCauseSanitizer(
         return (LoggingClientBuilder) super.responseCauseSanitizer(responseCauseSanitizer);
     }
 
+    @Deprecated
     @Override
     public LoggingClientBuilder responseCauseFilter(Predicate<Throwable> responseCauseFilter) {
         return (LoggingClientBuilder) super.responseCauseFilter(responseCauseFilter);
     }
-
-    @Override
-    public LoggingClientBuilder logWriter(LogWriter logWriter) {
-        return (LoggingClientBuilder) super.logWriter(logWriter);
-    }
 }

core/src/main/java/com/linecorp/armeria/client/logging/LoggingRpcClientBuilder.java

@@ -64,8 +64,8 @@ public Function<? super RpcClient, LoggingRpcClient> newDecorator() {
     // Override the return type of the chaining methods in the superclass.
 
     @Override
-    protected LoggingRpcClientBuilder defaultLogger(Logger logger) {
-        return (LoggingRpcClientBuilder) super.defaultLogger(logger);
+    protected LoggingRpcClientBuilder defaultLogger(Logger defaultLogger) {
+        return (LoggingRpcClientBuilder) super.defaultLogger(defaultLogger);
     }
 
     @Override

core/src/main/java/com/linecorp/armeria/common/logging/AbstractLogFormatterBuilder.java

@@ -66,6 +66,8 @@ abstract class AbstractLogFormatterBuilder<T> {
      *    .requestHeadersSanitizer(headersSanitizer)
      *    ...
      * }</pre>
+     *
+     * @see HeadersSanitizer
      */
     public AbstractLogFormatterBuilder<T> requestHeadersSanitizer(
             BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> requestHeadersSanitizer) {
@@ -101,6 +103,8 @@ final HeadersSanitizer<T> requestHeadersSanitizer() {
      *    .responseHeadersSanitizer(headersSanitizer)
      *    ...
      * }</pre>
+     *
+     * @see HeadersSanitizer
      */
     public AbstractLogFormatterBuilder<T> responseHeadersSanitizer(
             BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> responseHeadersSanitizer) {
@@ -135,6 +139,8 @@ final HeadersSanitizer<T> responseHeadersSanitizer() {
      *    .requestTrailersSanitizer(headersSanitizer)
      *    ...
      * }</pre>
+     *
+     * @see HeadersSanitizer
      */
     public AbstractLogFormatterBuilder<T> requestTrailersSanitizer(
             BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> requestTrailersSanitizer) {
@@ -169,6 +175,8 @@ final HeadersSanitizer<T> requestTrailersSanitizer() {
      *    .responseTrailersSanitizer(headersSanitizer)
      *    ...
      * }</pre>
+     *
+     * @see HeadersSanitizer
      */
     public AbstractLogFormatterBuilder<T> responseTrailersSanitizer(
             BiFunction<? super RequestContext, ? super HttpHeaders, ? extends T> responseTrailersSanitizer) {
@@ -204,6 +212,7 @@ final HeadersSanitizer<T> responseTrailersSanitizer() {
      * builder.responseTrailersSanitizer(headersSanitizer);
      * }</pre>
      *
+     * @see HeadersSanitizer
      * @see #requestHeadersSanitizer(BiFunction)
      * @see #requestTrailersSanitizer(BiFunction)
      * @see #responseHeadersSanitizer(BiFunction)

core/src/main/java/com/linecorp/armeria/common/logging/DefaultLogWriter.java

@@ -21,13 +21,14 @@
 import static com.linecorp.armeria.common.logging.LogWriterBuilder.DEFAULT_RESPONSE_LOG_LEVEL_MAPPER;
 import static java.util.Objects.requireNonNull;
 
-import java.util.function.Predicate;
+import java.util.function.BiPredicate;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.google.common.base.MoreObjects;
 
+import com.linecorp.armeria.common.RequestContext;
 import com.linecorp.armeria.common.util.SafeCloseable;
 
 final class DefaultLogWriter implements LogWriter {
@@ -37,20 +38,21 @@ final class DefaultLogWriter implements LogWriter {
     static final DefaultLogWriter DEFAULT =
             new DefaultLogWriter(defaultLogger, DEFAULT_REQUEST_LOG_LEVEL_MAPPER,
                                  DEFAULT_RESPONSE_LOG_LEVEL_MAPPER,
-                                 throwable -> false, LogFormatter.ofText());
+                                 (ctx, throwable) -> false, LogFormatter.ofText());
 
     private static boolean warnedNullRequestLogLevelMapper;
     private static boolean warnedNullResponseLogLevelMapper;
 
     private final Logger logger;
     private final RequestLogLevelMapper requestLogLevelMapper;
     private final ResponseLogLevelMapper responseLogLevelMapper;
-    private final Predicate<? super Throwable> responseCauseFilter;
+    private final BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter;
     private final LogFormatter logFormatter;
 
     DefaultLogWriter(Logger logger, RequestLogLevelMapper requestLogLevelMapper,
                      ResponseLogLevelMapper responseLogLevelMapper,
-                     Predicate<? super Throwable> responseCauseFilter, LogFormatter logFormatter) {
+                     BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter,
+                     LogFormatter logFormatter) {
         this.logger = logger;
         this.requestLogLevelMapper = requestLogLevelMapper;
         this.responseLogLevelMapper = responseLogLevelMapper;
@@ -94,7 +96,8 @@ public void logResponse(RequestLog log) {
 
         if (responseLogLevel.isEnabled(logger)) {
             final String responseStr = logFormatter.formatResponse(log);
-            try (SafeCloseable ignored = log.context().push()) {
+            final RequestContext ctx = log.context();
+            try (SafeCloseable ignored = ctx.push()) {
                 if (responseCause == null) {
                     responseLogLevel.log(logger, responseStr);
                     return;
@@ -108,7 +111,7 @@ public void logResponse(RequestLog log) {
                     responseLogLevel.log(logger, logFormatter.formatRequest(log));
                 }
 
-                if (responseCauseFilter.test(responseCause)) {
+                if (responseCauseFilter.test(ctx, responseCause)) {
                     responseLogLevel.log(logger, responseStr);
                 } else {
                     responseLogLevel.log(logger, responseStr, responseCause);

core/src/main/java/com/linecorp/armeria/common/logging/LogWriterBuilder.java

@@ -16,15 +16,18 @@
 
 package com.linecorp.armeria.common.logging;
 
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static java.util.Objects.requireNonNull;
 
+import java.util.function.BiPredicate;
 import java.util.function.Predicate;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.linecorp.armeria.common.HttpStatus;
 import com.linecorp.armeria.common.HttpStatusClass;
+import com.linecorp.armeria.common.RequestContext;
 import com.linecorp.armeria.common.annotation.Nullable;
 import com.linecorp.armeria.common.annotation.UnstableApi;
 
@@ -42,12 +45,14 @@ public final class LogWriterBuilder {
     static final ResponseLogLevelMapper DEFAULT_RESPONSE_LOG_LEVEL_MAPPER =
             ResponseLogLevelMapper.of(LogLevel.DEBUG, LogLevel.WARN);
 
-    private Logger logger = DefaultLogWriter.defaultLogger;
+    @Nullable
+    private Logger logger;
     @Nullable
     private RequestLogLevelMapper requestLogLevelMapper;
     @Nullable
     private ResponseLogLevelMapper responseLogLevelMapper;
-    private Predicate<? super Throwable> responseCauseFilter = throwable -> false;
+    private BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter =
+            (ctx, cause) -> false;
     private LogFormatter logFormatter = LogFormatter.ofText();
 
     LogWriterBuilder() {}
@@ -71,6 +76,11 @@ public LogWriterBuilder logger(String loggerName) {
         return this;
     }
 
+    @Nullable
+    Logger logger() {
+        return logger;
+    }
+
     /**
      * Sets the {@link LogLevel} to use when logging requests. If unset, will use {@link LogLevel#DEBUG}.
      */
@@ -101,7 +111,7 @@ public LogWriterBuilder requestLogLevelMapper(RequestLogLevelMapper requestLogLe
         return this;
     }
 
-    private RequestLogLevelMapper requestLogLevelMapper() {
+    RequestLogLevelMapper requestLogLevelMapper() {
         if (requestLogLevelMapper == null) {
             return DEFAULT_REQUEST_LOG_LEVEL_MAPPER;
         }
@@ -164,7 +174,7 @@ public LogWriterBuilder responseLogLevelMapper(ResponseLogLevelMapper responseLo
         return this;
     }
 
-    private ResponseLogLevelMapper responseLogLevelMapper() {
+    ResponseLogLevelMapper responseLogLevelMapper() {
         if (responseLogLevelMapper == null) {
             return DEFAULT_RESPONSE_LOG_LEVEL_MAPPER;
         }
@@ -176,7 +186,8 @@ private ResponseLogLevelMapper responseLogLevelMapper() {
      * You can prevent logging the response cause by returning {@code true}
      * in the {@link Predicate}. By default, the response cause will always be logged.
      */
-    public LogWriterBuilder responseCauseFilter(Predicate<? super Throwable> responseCauseFilter) {
+    public LogWriterBuilder responseCauseFilter(
+            BiPredicate<? super RequestContext, ? super Throwable> responseCauseFilter) {
         this.responseCauseFilter = requireNonNull(responseCauseFilter, "responseCauseFilter");
         return this;
     }
@@ -196,7 +207,8 @@ public LogWriterBuilder logFormatter(LogFormatter logFormatter) {
      * Returns a newly-created {@link LogWriter} based on the properties of this builder.
      */
     public LogWriter build() {
-        return new DefaultLogWriter(logger, requestLogLevelMapper(), responseLogLevelMapper(),
+        return new DefaultLogWriter(firstNonNull(logger, DefaultLogWriter.defaultLogger),
+                                    requestLogLevelMapper(), responseLogLevelMapper(),
                                     responseCauseFilter, logFormatter);
     }
 }