Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #4825 - Implement spec requirements for Request.newPushBuilder #4829

Merged
merged 5 commits into from
May 1, 2020
Merged

Issue #4825 - Implement spec requirements for Request.newPushBuilder #4829

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fcf0fd
Issue #4825 Implement spec requirements for Request.newPushBuilder
janbartel Apr 29, 2020
fa758c2
Issue #4825 Check method for PushBuilder.
janbartel Apr 29, 2020
40d316d
Issue #4825 Add extra tests for PushBuilder method
janbartel Apr 29, 2020
3fd7183
Issue #4825 Fix checkstyle problems
janbartel Apr 29, 2020
087d91b
Issue #4825 Changes after review
janbartel Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions jetty-server/src/main/java/org/eclipse/jetty/server/PushBuilderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

package org.eclipse.jetty.server;

import java.util.EnumSet;
import java.util.Objects;
import java.util.Set;
import javax.servlet.http.PushBuilder;

Expand All @@ -27,6 +29,7 @@
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.URIUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -39,6 +42,13 @@ public class PushBuilderImpl implements PushBuilder
private static final Logger LOG = LoggerFactory.getLogger(PushBuilderImpl.class);

private static final HttpField JettyPush = new HttpField("x-http2-push", "PushBuilder");
janbartel marked this conversation as resolved.
Show resolved Hide resolved
private static EnumSet<HttpMethod> UNSAFE_METHODS = EnumSet.of(
HttpMethod.POST,
HttpMethod.PUT,
HttpMethod.DELETE,
HttpMethod.CONNECT,
HttpMethod.OPTIONS,
HttpMethod.TRACE);

private final Request _request;
private final HttpFields.Mutable _fields;
Expand Down Expand Up @@ -70,6 +80,10 @@ public String getMethod()
@Override
public PushBuilder method(String method)
{
Objects.requireNonNull(method);

if (StringUtil.isBlank(method) || UNSAFE_METHODS.contains(HttpMethod.fromString(method)))
throw new IllegalArgumentException("Method not allowed for push: " + method);
_method = method;
return this;
}
Expand Down Expand Up @@ -149,9 +163,6 @@ public PushBuilder path(String path)
@Override
public void push()
{
if (HttpMethod.POST.is(_method) || HttpMethod.PUT.is(_method))
throw new IllegalStateException("Bad Method " + _method);

if (_path == null || _path.length() == 0)
throw new IllegalStateException("Bad Path " + _path);

Expand Down
55 changes: 52 additions & 3 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HostPortHttpField;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
Expand Down Expand Up @@ -393,6 +394,11 @@ public PushBuilder newPushBuilder()

HttpFields.Mutable fields = HttpFields.build(getHttpFields(), NOT_PUSHED_HEADERS);

HttpField authField = getHttpFields().getField(HttpHeader.AUTHORIZATION);
//TODO check what to do for digest etc etc
if (getUserPrincipal() != null && authField.getValue().startsWith("Basic"))
fields.add(authField);

String id;
try
{
Expand All @@ -410,12 +416,55 @@ public PushBuilder newPushBuilder()
id = getRequestedSessionId();
}

Map<String,String> cookies = new HashMap<>();
Cookie[] existingCookies = getCookies();
if (existingCookies != null)
{
for (Cookie c: getCookies())
{
cookies.put(c.getName(), c.getValue());
}
}

//Any Set-Cookies that were set on the response must be set as Cookies on the
//PushBuilder, unless the max-age of the cookie is <= 0
HttpFields responseFields = getResponse().getHttpFields();
for (HttpField field : responseFields)
{
HttpHeader header = field.getHeader();
if (header == null)
continue;
switch (header)
{
case SET_COOKIE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can easily coalesce the null-check and the switch into a single if (header == SET_COOKIE).
How about SET_COOKIE2 (I'm ok to not consider it, just raising the issue of whether we want to explicitly not consider it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set-Cookie2 is an actual header from a valid spec.
https://tools.ietf.org/html/rfc2965

{
HttpCookie cookie = ((SetCookieHttpField)field).getHttpCookie();
if (cookie.getMaxAge() > 0)
cookies.put(cookie.getName(), cookie.getValue());
else
cookies.remove(cookie.getName());
continue;
}
default:
continue;
}
}

if (!cookies.isEmpty())
{
StringBuilder buff = new StringBuilder();
for (Map.Entry<String,String> entry : cookies.entrySet())
{
if (buff.length() > 0)
buff.append("; ");
buff.append(entry.getKey()).append('=').append(entry.getValue());
}
fields.add(new HttpField(HttpHeader.COOKIE, buff.toString()));
}

janbartel marked this conversation as resolved.
Show resolved Hide resolved
PushBuilder builder = new PushBuilderImpl(this, fields, getMethod(), getQueryString(), id);
builder.addHeader("referer", getRequestURL().toString());

// TODO process any set cookies
// TODO process any user_identity

return builder;
}

Expand Down
130 changes: 130 additions & 0 deletions jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.io.Reader;
import java.io.UnsupportedEncodingException;
import java.nio.charset.StandardCharsets;
import java.security.Principal;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -45,10 +46,19 @@
import javax.servlet.http.HttpServletMapping;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import javax.servlet.http.Part;
import javax.servlet.http.PushBuilder;

import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.http.CookieCompliance;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.http.MetaData;
import org.eclipse.jetty.http.MimeTypes;
import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.http.tools.HttpTester;
Expand All @@ -57,6 +67,9 @@
import org.eclipse.jetty.server.handler.AbstractHandler;
import org.eclipse.jetty.server.handler.ContextHandler;
import org.eclipse.jetty.server.handler.ErrorHandler;
import org.eclipse.jetty.server.session.Session;
import org.eclipse.jetty.server.session.SessionData;
import org.eclipse.jetty.server.session.SessionHandler;
import org.eclipse.jetty.toolchain.test.FS;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.BufferUtil;
Expand Down Expand Up @@ -1860,11 +1873,128 @@ public void testGetterSafeFromNullPointerException()
assertNotNull(request.getParameterMap());
assertEquals(0, request.getParameterMap().size());
}

@Test
public void testPushBuilder() throws Exception
{
String uri = "/foo/something";
Request request = new TestRequest(null, null);
request.getResponse().getHttpFields().add(new HttpCookie.SetCookieHttpField(new HttpCookie("good","thumbsup", 100), CookieCompliance.RFC6265));
request.getResponse().getHttpFields().add(new HttpCookie.SetCookieHttpField(new HttpCookie("bonza","bewdy", 1), CookieCompliance.RFC6265));
request.getResponse().getHttpFields().add(new HttpCookie.SetCookieHttpField(new HttpCookie("bad", "thumbsdown", 0), CookieCompliance.RFC6265));
HttpFields.Mutable fields = HttpFields.build();
fields.add(HttpHeader.AUTHORIZATION, "Basic foo");
request.setMetaData(new MetaData.Request("GET", HttpURI.from(uri), HttpVersion.HTTP_1_0, fields));
assertTrue(request.isPushSupported());
PushBuilder builder = request.newPushBuilder();
assertNotNull(builder);
assertEquals("GET", builder.getMethod());
assertThrows(NullPointerException.class, () ->
{
builder.method(null);
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method(" ");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("POST");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("PUT");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("DELETE");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("CONNECT");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("OPTIONS");
});
assertThrows(IllegalArgumentException.class, () ->
{
builder.method("TRACE");
});
assertEquals(TestRequest.TEST_SESSION_ID, builder.getSessionId());
builder.path("/foo/something-else.txt");
assertEquals("/foo/something-else.txt", builder.getPath());
assertEquals("Basic foo", builder.getHeader("Authorization"));
assertThat(builder.getHeader("Cookie"), containsString("bonza"));
assertThat(builder.getHeader("Cookie"), containsString("good"));
assertThat(builder.getHeader("Cookie"), containsString("maxpos"));
assertThat(builder.getHeader("Cookie"), not(containsString("bad")));
}

interface RequestTester
{
boolean check(HttpServletRequest request, HttpServletResponse response) throws IOException;
}

private class TestRequest extends Request
{
public static final String TEST_SESSION_ID = "abc123";
Response _response = new Response(null, null);
Cookie c1;
Cookie c2;

public TestRequest(HttpChannel channel, HttpInput input)
{
super(channel, input);
c1 = new Cookie("maxpos", "xxx");
c1.setMaxAge(1);
c2 = new Cookie("maxneg", "yyy");
c2.setMaxAge(-1);
}

@Override
public boolean isPushSupported()
{
return true;
}

@Override
public HttpSession getSession()
{
return new Session(new SessionHandler(), new SessionData(TEST_SESSION_ID, "", "0.0.0.0", 0, 0, 0, 300));
}

@Override
public Principal getUserPrincipal()
{
return new Principal()
{

@Override
public String getName()
{
return "user";
}

};
}

@Override
public Response getResponse()
{
return _response;
}

@Override
public Cookie[] getCookies()
{
return new Cookie[] {c1,c2};
}
}

private class RequestHandler extends AbstractHandler
{
Expand Down