Fixes #1949 - Client-side problems with digest authentication. (#1991)

Introduced ContentProvider.isReproducible() to detect whether the
request content can be provided more than once, and modified
ContentProvider implementation accordingly.

Modified AuthenticationProtocolHandler to not send an authenticated
request if the content is not reproducible.

Modified AuthenticationProtocolHandler to tolerate request failures.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-client/src/main/java/org/eclipse/jetty/client/AuthenticationProtocolHandler.java

@@ -27,6 +27,7 @@ import java.util.regex.Pattern;
 
 import org.eclipse.jetty.client.api.Authentication;
 import org.eclipse.jetty.client.api.Connection;
+import org.eclipse.jetty.client.api.ContentProvider;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
@@ -86,7 +87,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler
         {
             HttpRequest request = (HttpRequest)result.getRequest();
             ContentResponse response = new HttpContentResponse(result.getResponse(), getContent(), getMediaType(), getEncoding());
-            if (result.isFailed())
+            if (result.getResponseFailure() != null)
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("Authentication challenge failed {}", result.getFailure());
@@ -98,7 +99,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler
             HttpConversation conversation = request.getConversation();
             if (conversation.getAttribute(authenticationAttribute) != null)
             {
-                // We have already tried to authenticate, but we failed again
+                // We have already tried to authenticate, but we failed again.
                 if (LOG.isDebugEnabled())
                     LOG.debug("Bad credentials for {}", request);
                 forwardSuccessComplete(request, response);
@@ -111,7 +112,7 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("Authentication challenge without {} header", header);
-                forwardFailureComplete(request, null, response, new HttpResponseException("HTTP protocol violation: Authentication challenge without " + header + " header", response));
+                forwardFailureComplete(request, result.getRequestFailure(), response, new HttpResponseException("HTTP protocol violation: Authentication challenge without " + header + " header", response));
                 return;
             }
 
@@ -138,9 +139,18 @@ public abstract class AuthenticationProtocolHandler implements ProtocolHandler
                 return;
             }
 
+            ContentProvider requestContent = request.getContent();
+            if (requestContent != null && !requestContent.isReproducible())
+            {
+                if (LOG.isDebugEnabled())
+                    LOG.debug("Request content not reproducible for {}", request);
+                forwardSuccessComplete(request, response);
+                return;
+            }
+
             try
             {
-                final Authentication.Result authnResult = authentication.authenticate(request, response, headerInfo, conversation);
+                Authentication.Result authnResult = authentication.authenticate(request, response, headerInfo, conversation);
                 if (LOG.isDebugEnabled())
                     LOG.debug("Authentication result {}", authnResult);
                 if (authnResult == null)

jetty-client/src/main/java/org/eclipse/jetty/client/api/ContentProvider.java

@@ -22,6 +22,7 @@ import java.io.Closeable;
 import java.nio.ByteBuffer;
 import java.util.Iterator;
 
+import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.client.util.ByteBufferContentProvider;
 import org.eclipse.jetty.client.util.PathContentProvider;
 
@@ -48,6 +49,22 @@ public interface ContentProvider extends Iterable<ByteBuffer>
      */
     long getLength();
 
+    /**
+     * <p>Whether this ContentProvider can produce exactly the same content more
+     * than once.</p>
+     * <p>Implementations should return {@code true} only if the content can be
+     * produced more than once, which means that invocations to {@link #iterator()}
+     * must return a new, independent, iterator instance over the content.</p>
+     * <p>The {@link HttpClient} implementation may use this method in particular
+     * cases where it detects that it is safe to retry a request that failed.</p>
+     *
+     * @return whether the content can be produced more than once
+     */
+    default boolean isReproducible()
+    {
+        return false;
+    }
+
     /**
      * An extension of {@link ContentProvider} that provides a content type string
      * to be used as a {@code Content-Type} HTTP header in requests.

jetty-client/src/main/java/org/eclipse/jetty/client/util/ByteBufferContentProvider.java

@@ -57,6 +57,12 @@ public class ByteBufferContentProvider extends AbstractTypedContentProvider
         return length;
     }
 
+    @Override
+    public boolean isReproducible()
+    {
+        return true;
+    }
+
     @Override
     public Iterator<ByteBuffer> iterator()
     {
@@ -85,12 +91,6 @@ public class ByteBufferContentProvider extends AbstractTypedContentProvider
                     throw new NoSuchElementException();
                 }
             }
-
-            @Override
-            public void remove()
-            {
-                throw new UnsupportedOperationException();
-            }
         };
     }
 }

jetty-client/src/main/java/org/eclipse/jetty/client/util/BytesContentProvider.java

@@ -53,6 +53,12 @@ public class BytesContentProvider extends AbstractTypedContentProvider
         return length;
     }
 
+    @Override
+    public boolean isReproducible()
+    {
+        return true;
+    }
+
     @Override
     public Iterator<ByteBuffer> iterator()
     {
@@ -78,12 +84,6 @@ public class BytesContentProvider extends AbstractTypedContentProvider
                     throw new NoSuchElementException();
                 }
             }
-
-            @Override
-            public void remove()
-            {
-                throw new UnsupportedOperationException();
-            }
         };
     }
 }

jetty-client/src/main/java/org/eclipse/jetty/client/util/PathContentProvider.java

@@ -85,6 +85,12 @@ public class PathContentProvider extends AbstractTypedContentProvider
         return fileSize;
     }
 
+    @Override
+    public boolean isReproducible()
+    {
+        return true;
+    }
+
     public ByteBufferPool getByteBufferPool()
     {
         return bufferPool;

jetty-client/src/test/java/org/eclipse/jetty/client/AbstractHttpClientServerTest.java

@@ -27,6 +27,7 @@ import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
 import org.eclipse.jetty.toolchain.test.TestTracker;
+import org.eclipse.jetty.util.SocketAddressResolver;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
 import org.junit.After;
@@ -96,6 +97,7 @@ public abstract class AbstractHttpClientServerTest
         clientThreads.setName("client");
         client = new HttpClient(transport, sslContextFactory);
         client.setExecutor(clientThreads);
+        client.setSocketAddressResolver(new SocketAddressResolver.Sync());
         client.start();
     }
 

jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientAuthenticationTest.java

@@ -22,11 +22,14 @@ import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 import java.nio.ByteBuffer;
-import java.nio.charset.StandardCharsets;
+import java.util.Iterator;
+import java.util.NoSuchElementException;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.IntFunction;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
@@ -34,6 +37,7 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jetty.client.api.Authentication;
 import org.eclipse.jetty.client.api.AuthenticationStore;
+import org.eclipse.jetty.client.api.ContentProvider;
 import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.client.api.Response;
@@ -41,6 +45,7 @@ import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.client.util.BasicAuthentication;
 import org.eclipse.jetty.client.util.DeferredContentProvider;
 import org.eclipse.jetty.client.util.DigestAuthentication;
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.security.Authenticator;
 import org.eclipse.jetty.security.ConstraintMapping;
 import org.eclipse.jetty.security.ConstraintSecurityHandler;
@@ -220,7 +225,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
             {
                 baseRequest.setHandled(true);
                 if (requests.incrementAndGet() == 1)
-                    response.sendRedirect(URIUtil.newURI(scheme,request.getServerName(),request.getServerPort(),request.getRequestURI(),null));
+                    response.sendRedirect(URIUtil.newURI(scheme, request.getServerName(), request.getServerPort(), request.getRequestURI(), null));
             }
         });
 
@@ -259,7 +264,7 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
             {
                 baseRequest.setHandled(true);
                 if (request.getRequestURI().endsWith("/redirect"))
-                    response.sendRedirect(URIUtil.newURI(scheme,request.getServerName(),request.getServerPort(),"/secure",null));
+                    response.sendRedirect(URIUtil.newURI(scheme, request.getServerName(), request.getServerPort(), "/secure", null));
             }
         });
 
@@ -424,6 +429,40 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
         Assert.assertEquals(1, requests.get());
     }
 
+    @Test
+    public void test_NonReproducibleContent() throws Exception
+    {
+        startBasic(new EmptyServerHandler());
+
+        AuthenticationStore authenticationStore = client.getAuthenticationStore();
+        URI uri = URI.create(scheme + "://localhost:" + connector.getLocalPort());
+        BasicAuthentication authentication = new BasicAuthentication(uri, realm, "basic", "basic");
+        authenticationStore.addAuthentication(authentication);
+
+        CountDownLatch resultLatch = new CountDownLatch(1);
+        byte[] data = new byte[]{'h', 'e', 'l', 'l', 'o'};
+        DeferredContentProvider content = new DeferredContentProvider(ByteBuffer.wrap(data))
+        {
+            @Override
+            public boolean isReproducible()
+            {
+                return false;
+            }
+        };
+        Request request = client.newRequest(uri)
+                .path("/secure")
+                .content(content);
+        request.send(result ->
+        {
+            if (result.isSucceeded() && result.getResponse().getStatus() == HttpStatus.UNAUTHORIZED_401)
+                resultLatch.countDown();
+        });
+
+        content.close();
+
+        Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS));
+    }
+
     @Test
     public void test_RequestFailsAfterResponse() throws Exception
     {
@@ -434,32 +473,111 @@ public class HttpClientAuthenticationTest extends AbstractHttpClientServerTest
         BasicAuthentication authentication = new BasicAuthentication(uri, realm, "basic", "basic");
         authenticationStore.addAuthentication(authentication);
 
-        CountDownLatch successLatch = new CountDownLatch(1);
+        AtomicBoolean fail = new AtomicBoolean(true);
+        GeneratingContentProvider content = new GeneratingContentProvider(index ->
+        {
+            switch (index)
+            {
+                case 0:
+                    return ByteBuffer.wrap(new byte[]{'h', 'e', 'l', 'l', 'o'});
+                case 1:
+                    return ByteBuffer.wrap(new byte[]{'w', 'o', 'r', 'l', 'd'});
+                case 2:
+                    if (fail.compareAndSet(true, false))
+                    {
+                        // Wait for the 401 response to arrive
+                        // to the authentication protocol handler.
+                        sleep(1000);
+                        // Trigger request failure.
+                        throw new RuntimeException();
+                    }
+                    else
+                    {
+                        return null;
+                    }
+                default:
+                    throw new IllegalStateException();
+            }
+        });
         CountDownLatch resultLatch = new CountDownLatch(1);
-        DeferredContentProvider content = new DeferredContentProvider();
+        client.newRequest("localhost", connector.getLocalPort())
-        Request request = client.newRequest("localhost", connector.getLocalPort())
                 .scheme(scheme)
                 .path("/secure")
                 .content(content)
-                .onResponseSuccess(response -> successLatch.countDown());
+                .send(result ->
-        request.send(result ->
+                {
-        {
+                    if (result.isSucceeded() && result.getResponse().getStatus() == HttpStatus.OK_200)
-            if (result.isFailed() && result.getResponseFailure() == null)
+                        resultLatch.countDown();
-                resultLatch.countDown();
+                });
-        });
 
-        // Send some content to make sure the request is dispatched on the server.
-        content.offer(ByteBuffer.wrap("hello".getBytes(StandardCharsets.UTF_8)));
-
-        // Wait for the response to arrive to
-        // the authentication protocol handler.
-        Thread.sleep(1000);
-
-        // Trigger request failure.
-        request.abort(new Exception());
-
-        // Verify that the response was successful, it's the request that failed.
-        Assert.assertTrue(successLatch.await(5, TimeUnit.SECONDS));
         Assert.assertTrue(resultLatch.await(5, TimeUnit.SECONDS));
     }
+
+    private void sleep(long time)
+    {
+        try
+        {
+            Thread.sleep(time);
+        }
+        catch (InterruptedException x)
+        {
+            throw new RuntimeException(x);
+        }
+    }
+
+    private static class GeneratingContentProvider implements ContentProvider
+    {
+        private static final ByteBuffer DONE = ByteBuffer.allocate(0);
+
+        private final IntFunction<ByteBuffer> generator;
+
+        private GeneratingContentProvider(IntFunction<ByteBuffer> generator)
+        {
+            this.generator = generator;
+        }
+
+        @Override
+        public long getLength()
+        {
+            return -1;
+        }
+
+        @Override
+        public boolean isReproducible()
+        {
+            return true;
+        }
+
+        @Override
+        public Iterator<ByteBuffer> iterator()
+        {
+            return new Iterator<ByteBuffer>()
+            {
+                private int index;
+                public ByteBuffer current;
+
+                @Override
+                public boolean hasNext()
+                {
+                    if (current == null)
+                    {
+                        current = generator.apply(index++);
+                        if (current == null)
+                            current = DONE;
+                    }
+                    return current != DONE;
+                }
+
+                @Override
+                public ByteBuffer next()
+                {
+                    ByteBuffer result = current;
+                    current = null;
+                    if (result == null)
+                        throw new NoSuchElementException();
+                    return result;
+                }
+            };
+        }
+    }
 }