Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Clean up CustomRequestLog and fix the handling of the %u code. (#4397) 

* Clean up CustomRequestLog and fix the handling of the %u code.
* Add test for logging of remote user with %u and %{d}u
* update javadoc to clarify that %u is only for servlet auth
* remove the prepended '?' when deferred authentication is checked
This commit is contained in:
Lachlan 2019-12-17 11:28:39 +11:00 committed by GitHub
parent 9f93577054
commit 584e264b0b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 102 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
jetty-server/src/main/java/org/eclipse/jetty/server/CustomRequestLog.java
View File

@ -31,12 +31,14 @@ import java.util.function.Supplier;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.QuotedCSV;
import org.eclipse.jetty.http.pathmap.PathMappings;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.util.DateCache;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.annotation.ManagedAttribute;
import org.eclipse.jetty.util.annotation.ManagedObject;
import org.eclipse.jetty.util.component.ContainerLifeCycle;
@ -233,8 +235,9 @@ import static java.lang.invoke.MethodType.methodType;
* <tr>
* <td valign="top">%{d}u</td>
* <td>
* Remote user if the request was authenticated. May be bogus if return status (%s) is 401 (unauthorized).
* Optional parameter d, with this parameter deferred authentication will also be checked.
* Remote user if the request was authenticated with servlet authentication. May be bogus if return status (%s) is 401 (unauthorized).
* Optional parameter d, with this parameter deferred authentication will also be checked,
* this is equivalent to {@link HttpServletRequest#getRemoteUser()}.
* </td>
* </tr>
*
@ -294,11 +297,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
{
_logHandle = getLogHandle(formatString);
}
catch (NoSuchMethodException e)
{
throw new IllegalStateException(e);
}
catch (IllegalAccessException e)
catch (NoSuchMethodException | IllegalAccessException e)
{
throw new IllegalStateException(e);
}
@ -357,20 +356,14 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
protected static String getAuthentication(Request request, boolean checkDeferred)
{
Authentication authentication = request.getAuthentication();
if (checkDeferred && authentication instanceof Authentication.Deferred)
authentication = ((Authentication.Deferred)authentication).authenticate(request);
String name = null;
boolean deferred = false;
if (checkDeferred && authentication instanceof Authentication.Deferred)
{
authentication = ((Authentication.Deferred)authentication).authenticate(request);
deferred = true;
}
if (authentication instanceof Authentication.User)
name = ((Authentication.User)authentication).getUserIdentity().getUserPrincipal().getName();
return (name == null) ? null : (deferred ? ("?" + name) : name);
return name;
}
/**
@ -415,9 +408,9 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
if (_ignorePaths != null && _ignorePaths.length > 0)
{
_ignorePathMap = new PathMappings<>();
for (int i = 0; i < _ignorePaths.length; i++)
for (String ignorePath : _ignorePaths)
{
_ignorePathMap.put(_ignorePaths[i], _ignorePaths[i]);
_ignorePathMap.put(ignorePath, ignorePath);
}
}
else
@ -442,8 +435,8 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
private MethodHandle getLogHandle(String formatString) throws NoSuchMethodException, IllegalAccessException
{
MethodHandles.Lookup lookup = MethodHandles.lookup();
MethodHandle append = lookup.findStatic(CustomRequestLog.class, "append", methodType(Void.TYPE, String.class, StringBuilder.class));
MethodHandle logHandle = lookup.findStatic(CustomRequestLog.class, "logNothing", methodType(Void.TYPE, StringBuilder.class, Request.class, Response.class));
MethodHandle append = lookup.findStatic(CustomRequestLog.class, "append", methodType(void.class, String.class, StringBuilder.class));
MethodHandle logHandle = lookup.findStatic(CustomRequestLog.class, "logNothing", methodType(void.class, StringBuilder.class, Request.class, Response.class));
List<Token> tokens = getTokens(formatString);
Collections.reverse(tokens);
@ -486,7 +479,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
String arg = m.group("ARG");
String modifierString = m.group("MOD");
Boolean negated = false;
boolean negated = false;
if (modifierString != null)
{
if (modifierString.startsWith("!"))
@ -581,8 +574,8 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
private MethodHandle updateLogHandle(MethodHandle logHandle, MethodHandle append, MethodHandles.Lookup lookup, String code, String arg, List<String> modifiers, boolean negated) throws NoSuchMethodException, IllegalAccessException
{
MethodType logType = methodType(Void.TYPE, StringBuilder.class, Request.class, Response.class);
MethodType logTypeArg = methodType(Void.TYPE, String.class, StringBuilder.class, Request.class, Response.class);
MethodType logType = methodType(void.class, StringBuilder.class, Request.class, Response.class);
MethodType logTypeArg = methodType(void.class, String.class, StringBuilder.class, Request.class, Response.class);
//TODO should we throw IllegalArgumentExceptions when given arguments for codes which do not take them
MethodHandle specificHandle;
@ -596,7 +589,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "a":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
arg = "server";
String method;
@ -628,7 +621,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "p":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
arg = "server";
String method;
@ -662,7 +655,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "I":
{
String method;
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
method = "logBytesReceived";
else if (arg.equalsIgnoreCase("clf"))
method = "logBytesReceivedCLF";
@ -676,7 +669,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "O":
{
String method;
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
method = "logBytesSent";
else if (arg.equalsIgnoreCase("clf"))
method = "logBytesSentCLF";
@ -690,7 +683,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "S":
{
String method;
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
method = "logBytesTransferred";
else if (arg.equalsIgnoreCase("clf"))
method = "logBytesTransferredCLF";
@ -703,7 +696,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "C":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
{
specificHandle = lookup.findStatic(CustomRequestLog.class, "logRequestCookies", logType);
}
@ -723,7 +716,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "e":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
throw new IllegalArgumentException("No arg for %e");
specificHandle = lookup.findStatic(CustomRequestLog.class, "logEnvironmentVar", logTypeArg);
@ -745,7 +738,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "i":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
throw new IllegalArgumentException("No arg for %i");
specificHandle = lookup.findStatic(CustomRequestLog.class, "logRequestHeader", logTypeArg);
@ -767,7 +760,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "o":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
throw new IllegalArgumentException("No arg for %o");
specificHandle = lookup.findStatic(CustomRequestLog.class, "logResponseHeader", logTypeArg);
@ -832,7 +825,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
DateCache logDateCache = new DateCache(format, locale, timeZone);
MethodType logTypeDateCache = methodType(Void.TYPE, DateCache.class, StringBuilder.class, Request.class, Response.class);
MethodType logTypeDateCache = methodType(void.class, DateCache.class, StringBuilder.class, Request.class, Response.class);
specificHandle = lookup.findStatic(CustomRequestLog.class, "logRequestTime", logTypeDateCache);
specificHandle = specificHandle.bindTo(logDateCache);
break;
@ -866,10 +859,12 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "u":
{
String method;
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
method = "logRequestAuthentication";
else if ("d".equals(arg))
method = "logRequestAuthenticationWithDeferred";
else
method = "logRequestAuthentication";
throw new IllegalArgumentException("Invalid arg for %u: " + arg);
specificHandle = lookup.findStatic(CustomRequestLog.class, method, logType);
break;
@ -889,7 +884,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "ti":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
throw new IllegalArgumentException("No arg for %ti");
specificHandle = lookup.findStatic(CustomRequestLog.class, "logRequestTrailer", logTypeArg);
@ -899,7 +894,7 @@ public class CustomRequestLog extends ContainerLifeCycle implements RequestLog
case "to":
{
if (arg == null || arg.isEmpty())
if (StringUtil.isEmpty(arg))
throw new IllegalArgumentException("No arg for %to");
specificHandle = lookup.findStatic(CustomRequestLog.class, "logResponseTrailer", logTypeArg);

81
jetty-server/src/test/java/org/eclipse/jetty/server/handler/CustomRequestLogTest.java → tests/test-integration/src/test/java/org/eclipse/jetty/test/CustomRequestLogTest.java
View File

@ -16,7 +16,7 @@
// ========================================================================
//
package org.eclipse.jetty.server.handler;
package org.eclipse.jetty.test;
import java.io.IOException;
import java.io.InputStream;
@ -26,15 +26,25 @@ import java.net.NetworkInterface;
import java.net.Socket;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.Enumeration;
import java.util.Locale;
import java.util.Objects;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.TimeUnit;
import javax.servlet.ServletException;
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.security.ConstraintMapping;
import org.eclipse.jetty.security.ConstraintSecurityHandler;
import org.eclipse.jetty.security.HashLoginService;
import org.eclipse.jetty.security.SecurityHandler;
import org.eclipse.jetty.security.UserStore;
import org.eclipse.jetty.security.authentication.BasicAuthenticator;
import org.eclipse.jetty.server.CustomRequestLog;
import org.eclipse.jetty.server.ForwardedRequestCustomizer;
import org.eclipse.jetty.server.HttpConnectionFactory;
@ -44,8 +54,12 @@ import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.RequestLog;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;
import org.eclipse.jetty.util.BlockingArrayQueue;
import org.eclipse.jetty.util.DateCache;
import org.eclipse.jetty.util.security.Constraint;
import org.eclipse.jetty.util.security.Credential;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
@ -88,24 +102,70 @@ public class CustomRequestLogTest
TestRequestLogWriter writer = new TestRequestLogWriter();
_log = new CustomRequestLog(writer, formatString);
_server.setRequestLog(_log);
_server.setHandler(new TestHandler());
ServletContextHandler contextHandler = new ServletContextHandler();
contextHandler.setSecurityHandler(getSecurityHandler("username", "password", "testRealm"));
contextHandler.addServlet(new ServletHolder(new TestServlet()), "/");
_server.setHandler(contextHandler);
_server.start();
String host = _serverConnector.getHost();
if (host == null)
{
host = "localhost";
}
int localPort = _serverConnector.getLocalPort();
_serverURI = new URI(String.format("http://%s:%d/", host, localPort));
}
private static SecurityHandler getSecurityHandler(String username, String password, String realm)
{
HashLoginService loginService = new HashLoginService();
UserStore userStore = new UserStore();
userStore.addUser(username, Credential.getCredential(password), new String[]{"user"});
loginService.setUserStore(userStore);
loginService.setName(realm);
Constraint constraint = new Constraint();
constraint.setName("auth");
constraint.setAuthenticate(true);
constraint.setRoles(new String[]{"**"});
ConstraintMapping mapping = new ConstraintMapping();
mapping.setPathSpec("/secure/*");
mapping.setConstraint(constraint);
ConstraintSecurityHandler security = new ConstraintSecurityHandler();
security.addConstraintMapping(mapping);
security.setAuthenticator(new BasicAuthenticator());
security.setLoginService(loginService);
return security;
}
@AfterEach
public void after() throws Exception
{
_server.stop();
}
@Test
public void testLogRemoteUser() throws Exception
{
String authHeader = HttpHeader.AUTHORIZATION + ": Basic " + Base64.getEncoder().encodeToString("username:password".getBytes());
testHandlerServerStart("%u %{d}u");
_connector.getResponse("GET / HTTP/1.0\n\n\n");
String log = _entries.poll(5, TimeUnit.SECONDS);
assertThat(log, is("- -"));
_connector.getResponse("GET / HTTP/1.0\n" + authHeader + "\n\n\n");
log = _entries.poll(5, TimeUnit.SECONDS);
assertThat(log, is("- username"));
_connector.getResponse("GET /secure HTTP/1.0\n" + authHeader + "\n\n\n");
log = _entries.poll(5, TimeUnit.SECONDS);
assertThat(log, is("username username"));
}
@Test
public void testModifier() throws Exception
{
@ -374,7 +434,7 @@ public class CustomRequestLogTest
_connector.getResponse("GET / HTTP/1.0\n\n");
String log = _entries.poll(5, TimeUnit.SECONDS);
long requestTime = requestTimes.poll(5, TimeUnit.SECONDS);
DateCache dateCache = new DateCache(_log.DEFAULT_DATE_FORMAT, Locale.getDefault(), "GMT");
DateCache dateCache = new DateCache(CustomRequestLog.DEFAULT_DATE_FORMAT, Locale.getDefault(), "GMT");
assertThat(log, is("RequestTime: [" + dateCache.format(requestTime) + "]"));
}
@ -549,11 +609,13 @@ public class CustomRequestLogTest
}
}
private class TestHandler extends AbstractHandler
private class TestServlet extends HttpServlet
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
{
Request baseRequest = Objects.requireNonNull(Request.getBaseRequest(request));
if (request.getRequestURI().contains("error404"))
{
response.setStatus(404);
@ -596,10 +658,7 @@ public class CustomRequestLogTest
if (request.getContentLength() > 0)
{
InputStream in = request.getInputStream();
while (in.read() > 0)
{
;
}
while (in.read() > 0);
}
}
}