This is an automated email from the ASF dual-hosted git repository. btellier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 0148ce3f5daadaf2d22c578174a733be9feca511 Author: Benoit Tellier <[email protected]> AuthorDate: Thu Oct 8 12:38:05 2020 +0700 JAMES-3420 Avoid logging user password upon webadmin user creation --- .../james/modules/server/DataRoutesModules.java | 4 ++ .../james/modules/server/WebAdminServerModule.java | 3 + .../integration/WebAdminServerIntegrationTest.java | 30 ++++++++++ .../org/apache/james/webadmin/WebAdminServer.java | 11 ++-- .../james/webadmin/mdc/LoggingRequestFilter.java | 65 ++++++++++++++++------ .../apache/james/webadmin/mdc/RequestLogger.java | 28 ++++++++++ .../org/apache/james/webadmin/WebAdminUtils.java | 3 +- .../routes/UserCreationRequestLogger.java} | 50 ++++++++--------- 8 files changed, 147 insertions(+), 47 deletions(-) diff --git a/server/container/guice/protocols/webadmin-data/src/main/java/org/apache/james/modules/server/DataRoutesModules.java b/server/container/guice/protocols/webadmin-data/src/main/java/org/apache/james/modules/server/DataRoutesModules.java index d198069..1014264 100644 --- a/server/container/guice/protocols/webadmin-data/src/main/java/org/apache/james/modules/server/DataRoutesModules.java +++ b/server/container/guice/protocols/webadmin-data/src/main/java/org/apache/james/modules/server/DataRoutesModules.java @@ -21,6 +21,7 @@ package org.apache.james.modules.server; import org.apache.james.webadmin.Routes; import org.apache.james.webadmin.dto.MappingSourceModule; +import org.apache.james.webadmin.mdc.RequestLogger; import org.apache.james.webadmin.routes.AddressMappingRoutes; import org.apache.james.webadmin.routes.AliasRoutes; import org.apache.james.webadmin.routes.DomainMappingsRoutes; @@ -29,6 +30,7 @@ import org.apache.james.webadmin.routes.ForwardRoutes; import org.apache.james.webadmin.routes.GroupsRoutes; import org.apache.james.webadmin.routes.MappingRoutes; import org.apache.james.webadmin.routes.RegexMappingRoutes; +import org.apache.james.webadmin.routes.UserCreationRequestLogger; import org.apache.james.webadmin.routes.UserRoutes; import org.apache.james.webadmin.utils.JsonTransformerModule; @@ -52,5 +54,7 @@ public class DataRoutesModules extends AbstractModule { Multibinder<JsonTransformerModule> jsonTransformerModuleMultibinder = Multibinder.newSetBinder(binder(), JsonTransformerModule.class); jsonTransformerModuleMultibinder.addBinding().to(MappingSourceModule.class); + + Multibinder.newSetBinder(binder(), RequestLogger.class).addBinding().to(UserCreationRequestLogger.class); } } diff --git a/server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java b/server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java index ff2f263..a2625be 100644 --- a/server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java +++ b/server/container/guice/protocols/webadmin/src/main/java/org/apache/james/modules/server/WebAdminServerModule.java @@ -46,6 +46,7 @@ import org.apache.james.webadmin.WebAdminServer; import org.apache.james.webadmin.authentication.AuthenticationFilter; import org.apache.james.webadmin.authentication.JwtFilter; import org.apache.james.webadmin.authentication.NoAuthenticationFilter; +import org.apache.james.webadmin.mdc.RequestLogger; import org.apache.james.webadmin.utils.JsonTransformer; import org.apache.james.webadmin.utils.JsonTransformerModule; import org.slf4j.Logger; @@ -84,6 +85,8 @@ public class WebAdminServerModule extends AbstractModule { Multibinder.newSetBinder(binder(), GuiceProbe.class).addBinding().to(WebAdminGuiceProbe.class); Multibinder.newSetBinder(binder(), JsonTransformerModule.class); + + Multibinder.newSetBinder(binder(), RequestLogger.class); } @Provides diff --git a/server/protocols/webadmin-integration-test/webadmin-integration-test-common/src/main/java/org/apache/james/webadmin/integration/WebAdminServerIntegrationTest.java b/server/protocols/webadmin-integration-test/webadmin-integration-test-common/src/main/java/org/apache/james/webadmin/integration/WebAdminServerIntegrationTest.java index 4d303a4..992a2e9 100644 --- a/server/protocols/webadmin-integration-test/webadmin-integration-test-common/src/main/java/org/apache/james/webadmin/integration/WebAdminServerIntegrationTest.java +++ b/server/protocols/webadmin-integration-test/webadmin-integration-test-common/src/main/java/org/apache/james/webadmin/integration/WebAdminServerIntegrationTest.java @@ -37,6 +37,7 @@ import org.apache.james.probe.DataProbe; import org.apache.james.utils.DataProbeImpl; import org.apache.james.utils.WebAdminGuiceProbe; import org.apache.james.webadmin.WebAdminUtils; +import org.apache.james.webadmin.mdc.LoggingRequestFilter; import org.apache.james.webadmin.routes.AliasRoutes; import org.apache.james.webadmin.routes.DomainsRoutes; import org.apache.james.webadmin.routes.ForwardRoutes; @@ -51,7 +52,11 @@ import org.apache.james.webadmin.swagger.routes.SwaggerRoutes; import org.eclipse.jetty.http.HttpStatus; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import io.restassured.RestAssured; public abstract class WebAdminServerIntegrationTest { @@ -175,6 +180,19 @@ public abstract class WebAdminServerIntegrationTest { } @Test + void putShouldNotLogThePassword() { + ListAppender<ILoggingEvent> loggingEvents = getListAppenderForClass(LoggingRequestFilter.class); + + with() + .body("{\"password\":\"password\"}") + .put(SPECIFIC_USER); + + assertThat(loggingEvents.list) + .hasSize(1) + .allSatisfy(loggingEvent -> assertThat(loggingEvent.getMDCPropertyMap()).doesNotContainKey("request-body")); + } + + @Test void deleteShouldRemoveTheUser() throws Exception { dataProbe.addUser(USERNAME, "anyPassword"); @@ -366,4 +384,16 @@ public abstract class WebAdminServerIntegrationTest { .body("status", is("completed")) .body("type", is("MailboxesExportTask")); } + + + public static ListAppender<ILoggingEvent> getListAppenderForClass(Class clazz) { + Logger logger = (Logger) LoggerFactory.getLogger(clazz); + + ListAppender<ILoggingEvent> loggingEventListAppender = new ListAppender<>(); + loggingEventListAppender.start(); + + logger.addAppender(loggingEventListAppender); + + return loggingEventListAppender; + } } \ No newline at end of file diff --git a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/WebAdminServer.java b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/WebAdminServer.java index 1c9f00a..9a13bdcc 100644 --- a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/WebAdminServer.java +++ b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/WebAdminServer.java @@ -63,17 +63,20 @@ public class WebAdminServer implements Startable { private final Service service; private final AuthenticationFilter authenticationFilter; private final MetricFactory metricFactory; + private final LoggingRequestFilter loggingRequestFilter; @Inject protected WebAdminServer(WebAdminConfiguration configuration, - @Named("webAdminRoutes") List<Routes> routesList, - AuthenticationFilter authenticationFilter, - MetricFactory metricFactory) { + @Named("webAdminRoutes") List<Routes> routesList, + AuthenticationFilter authenticationFilter, + MetricFactory metricFactory, + LoggingRequestFilter loggingRequestFilter) { this.configuration = configuration; this.privateRoutes = privateRoutes(routesList); this.publicRoutes = publicRoutes(routesList); this.authenticationFilter = authenticationFilter; this.metricFactory = metricFactory; + this.loggingRequestFilter = loggingRequestFilter; this.service = Service.ignite(); } @@ -116,7 +119,7 @@ public class WebAdminServer implements Startable { private void configureMDC() { service.before(new MDCFilter()); - service.before(new LoggingRequestFilter()); + service.before(loggingRequestFilter); service.after(new LoggingResponseFilter()); service.after(new MDCCleanupFilter()); } diff --git a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java index 32ef13e..06a7edf 100644 --- a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java +++ b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java @@ -21,6 +21,10 @@ package org.apache.james.webadmin.mdc; import static org.apache.james.webadmin.authentication.AuthenticationFilter.LOGIN; +import java.util.Set; + +import javax.inject.Inject; + import org.apache.james.util.MDCStructuredLogger; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -32,13 +36,46 @@ import spark.Request; import spark.Response; public class LoggingRequestFilter implements Filter { - private static final Logger LOGGER = LoggerFactory.getLogger(LoggingRequestFilter.class); - static final String REQUEST_BODY = "request-body"; - static final String METHOD = "method"; - static final String ENDPOINT = "endpoint"; - static final String QUERY_PARAMETERS = "queryParameters"; - static final String IP = "ip"; - static final String REQUEST_ID = "requestId"; + private static class DefaultRequestLogger implements RequestLogger { + private static final DefaultRequestLogger INSTANCE = new DefaultRequestLogger(); + + @Override + public boolean applies(Request request) { + return true; + } + + @Override + public void log(Request request, RequestId requestId) { + MDCStructuredLogger.forLogger(LOGGER) + .addField(REQUEST_ID, requestId.asString()) + .addField(IP, request.ip()) + .addField(ENDPOINT, request.url()) + .addField(METHOD, request.requestMethod()) + .addField(LOGIN, request.attribute(LOGIN)) + .addField(QUERY_PARAMETERS, ImmutableSet.copyOf(request.queryParams())) + .addField(REQUEST_BODY, request.body()) + .log(logger -> logger.info("WebAdmin request received")); + } + } + + public static final Logger LOGGER = LoggerFactory.getLogger(LoggingRequestFilter.class); + public static final String REQUEST_BODY = "request-body"; + public static final String METHOD = "method"; + public static final String ENDPOINT = "endpoint"; + public static final String QUERY_PARAMETERS = "queryParameters"; + public static final String IP = "ip"; + public static final String REQUEST_ID = "requestId"; + + public static LoggingRequestFilter create() { + return new LoggingRequestFilter(ImmutableSet.of()); + } + + private final Set<RequestLogger> requestLoggers; + + @Inject + public LoggingRequestFilter(Set<RequestLogger> requestLoggers) { + this.requestLoggers = requestLoggers; + } @Override public void handle(Request request, Response response) { @@ -46,14 +83,10 @@ public class LoggingRequestFilter implements Filter { request.attribute(REQUEST_ID, requestId); - MDCStructuredLogger.forLogger(LOGGER) - .addField(REQUEST_ID, requestId.asString()) - .addField(IP, request.ip()) - .addField(ENDPOINT, request.url()) - .addField(METHOD, request.requestMethod()) - .addField(LOGIN, request.attribute(LOGIN)) - .addField(QUERY_PARAMETERS, ImmutableSet.copyOf(request.queryParams())) - .addField(REQUEST_BODY, request.body()) - .log(logger -> logger.info("WebAdmin request received")); + requestLoggers.stream() + .filter(requestLogger -> requestLogger.applies(request)) + .findFirst() + .orElse(DefaultRequestLogger.INSTANCE) + .log(request, requestId); } } diff --git a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/RequestLogger.java b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/RequestLogger.java new file mode 100644 index 0000000..4d8ca10 --- /dev/null +++ b/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/RequestLogger.java @@ -0,0 +1,28 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you 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 org.apache.james.webadmin.mdc; + +import spark.Request; + +public interface RequestLogger { + boolean applies(Request request); + + void log(Request request, RequestId requestId); +} diff --git a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/WebAdminUtils.java b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/WebAdminUtils.java index 60aeb11..f63ac3f 100644 --- a/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/WebAdminUtils.java +++ b/server/protocols/webadmin/webadmin-core/src/test/java/org/apache/james/webadmin/WebAdminUtils.java @@ -32,6 +32,7 @@ import org.apache.james.metrics.tests.RecordingMetricFactory; import org.apache.james.util.Port; import org.apache.james.webadmin.authentication.AuthenticationFilter; import org.apache.james.webadmin.authentication.NoAuthenticationFilter; +import org.apache.james.webadmin.mdc.LoggingRequestFilter; import com.google.common.collect.ImmutableList; @@ -45,7 +46,7 @@ import reactor.util.retry.Retry; public class WebAdminUtils { private static class ConcurrentSafeWebAdminServer extends WebAdminServer { ConcurrentSafeWebAdminServer(WebAdminConfiguration configuration, List<Routes> routesList, AuthenticationFilter authenticationFilter, MetricFactory metricFactory) { - super(configuration, routesList, authenticationFilter, metricFactory); + super(configuration, routesList, authenticationFilter, metricFactory, LoggingRequestFilter.create()); } /** diff --git a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/UserCreationRequestLogger.java similarity index 50% copy from server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java copy to server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/UserCreationRequestLogger.java index 32ef13e..8bcbe5c 100644 --- a/server/protocols/webadmin/webadmin-core/src/main/java/org/apache/james/webadmin/mdc/LoggingRequestFilter.java +++ b/server/protocols/webadmin/webadmin-data/src/main/java/org/apache/james/webadmin/routes/UserCreationRequestLogger.java @@ -17,43 +17,41 @@ * under the License. * ****************************************************************/ -package org.apache.james.webadmin.mdc; +package org.apache.james.webadmin.routes; import static org.apache.james.webadmin.authentication.AuthenticationFilter.LOGIN; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.ENDPOINT; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.IP; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.LOGGER; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.METHOD; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.QUERY_PARAMETERS; +import static org.apache.james.webadmin.mdc.LoggingRequestFilter.REQUEST_ID; import org.apache.james.util.MDCStructuredLogger; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.apache.james.webadmin.mdc.RequestId; +import org.apache.james.webadmin.mdc.RequestLogger; import com.google.common.collect.ImmutableSet; -import spark.Filter; import spark.Request; -import spark.Response; - -public class LoggingRequestFilter implements Filter { - private static final Logger LOGGER = LoggerFactory.getLogger(LoggingRequestFilter.class); - static final String REQUEST_BODY = "request-body"; - static final String METHOD = "method"; - static final String ENDPOINT = "endpoint"; - static final String QUERY_PARAMETERS = "queryParameters"; - static final String IP = "ip"; - static final String REQUEST_ID = "requestId"; +// This class skips logging of the body for user creation requests as it contains the user password +public class UserCreationRequestLogger implements RequestLogger { @Override - public void handle(Request request, Response response) { - RequestId requestId = RequestId.random(); - - request.attribute(REQUEST_ID, requestId); + public boolean applies(Request request) { + return request.pathInfo().startsWith(UserRoutes.USERS) + && request.requestMethod().equals("PUT"); + } + @Override + public void log(Request request, RequestId requestId) { MDCStructuredLogger.forLogger(LOGGER) - .addField(REQUEST_ID, requestId.asString()) - .addField(IP, request.ip()) - .addField(ENDPOINT, request.url()) - .addField(METHOD, request.requestMethod()) - .addField(LOGIN, request.attribute(LOGIN)) - .addField(QUERY_PARAMETERS, ImmutableSet.copyOf(request.queryParams())) - .addField(REQUEST_BODY, request.body()) - .log(logger -> logger.info("WebAdmin request received")); + .addField(REQUEST_ID, requestId.asString()) + .addField(IP, request.ip()) + .addField(ENDPOINT, request.url()) + .addField(METHOD, request.requestMethod()) + .addField(LOGIN, request.attribute(LOGIN)) + .addField(QUERY_PARAMETERS, ImmutableSet.copyOf(request.queryParams())) + .log(logger -> logger.info("WebAdmin request received: user creation request")); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
