HTTPCLIENT-2414 - Fix Basic auth cache scoping across path prefixes

httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java

@@ -26,13 +26,17 @@
  */
 package org.apache.hc.client5.testing.sync;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+
 import java.io.ByteArrayInputStream;
 import java.nio.charset.StandardCharsets;
 import java.security.SecureRandom;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.List;
 import java.util.Queue;
 import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
@@ -57,6 +61,7 @@
 import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder;
 import org.apache.hc.client5.http.protocol.HttpClientContext;
 import org.apache.hc.client5.testing.BasicTestAuthenticator;
+import org.apache.hc.client5.testing.auth.AuthResult;
 import org.apache.hc.client5.testing.auth.Authenticator;
 import org.apache.hc.client5.testing.auth.BearerAuthenticationHandler;
 import org.apache.hc.client5.testing.classic.AuthenticatingDecorator;
@@ -82,6 +87,7 @@
 import org.apache.hc.core5.http.protocol.HttpContext;
 import org.apache.hc.core5.http.support.BasicResponseBuilder;
 import org.apache.hc.core5.net.URIAuthority;
+import org.hamcrest.CoreMatchers;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
@@ -339,8 +345,9 @@ void testBasicAuthenticationCredentialsCaching() throws Exception {
 
         Mockito.verify(authStrategy).select(Mockito.any(), Mockito.any(), Mockito.any());
 
-        Assertions.assertEquals(Arrays.asList(401, 200, 200, 200, 200, 200),
-                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()));
+        assertThat(
+                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()),
+                CoreMatchers.equalTo(Arrays.asList(401, 200, 200, 200, 200, 200)));
     }
 
     @Test
@@ -381,8 +388,9 @@ void testBasicAuthenticationCredentialsCachingByPathPrefix() throws Exception {
         // There should be only single auth strategy call for all successful message exchanges
         Mockito.verify(authStrategy).select(Mockito.any(), Mockito.any(), Mockito.any());
 
-        Assertions.assertEquals(Arrays.asList(401, 200, 200, 200, 200),
-                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()));
+        assertThat(
+                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()),
+                CoreMatchers.equalTo(Arrays.asList(401, 200, 200, 200, 200)));
 
         responseQueue.clear();
         authCache.clear();
@@ -400,10 +408,11 @@ void testBasicAuthenticationCredentialsCachingByPathPrefix() throws Exception {
         }
 
         // There should be an auth strategy call for all successful message exchanges
-        Mockito.verify(authStrategy, Mockito.times(2)).select(Mockito.any(), Mockito.any(), Mockito.any());
+        Mockito.verify(authStrategy, Mockito.times(3)).select(Mockito.any(), Mockito.any(), Mockito.any());
 
-        Assertions.assertEquals(Arrays.asList(200, 401, 200, 200, 401, 200),
-                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()));
+        assertThat(
+                responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()),
+                CoreMatchers.equalTo(Arrays.asList(200, 401, 200, 401, 200, 401, 200)));
     }
 
     @Test
@@ -814,4 +823,124 @@ void testBearerTokenAuthentication() throws Exception {
         });
     }
 
+    @Test
+    void testBasicAuthenticationCredentialsCachingDifferentPathPrefixesSameContext() throws Exception {
+        final List<RequestSnapshot> requests = new CopyOnWriteArrayList<>();
+        final Authenticator authenticator = new BasicTestAuthenticator("test:test", "test realm") {
+            @Override
+            public AuthResult perform(final URIAuthority authority,
+                                      final String requestUri,
+                                      final String credentials) {
+                requests.add(new RequestSnapshot(requestUri, credentials != null));
+                return super.perform(authority, requestUri, credentials);
+            }
+        };
+        configureServerWithBasicAuth(authenticator, bootstrap -> bootstrap.register("*", new EchoHandler()));
+        final HttpHost target = startServer();
+
+        final List<Integer> responseCodes = new CopyOnWriteArrayList<>();
+
+        configureClient(builder -> builder
+                .addResponseInterceptorLast((response, entity, context) -> responseCodes.add(response.getCode())));
+
+        final TestClient client = client();
+
+        final CredentialsProvider credentialsProvider = CredentialsProviderBuilder.create()
+                .add(target, "test", "test".toCharArray())
+                .build();
+
+        final HttpClientContext context = HttpClientContext.create();
+        context.setAuthCache(new BasicAuthCache());
+        context.setCredentialsProvider(credentialsProvider);
+
+        for (final String requestPath : new String[]{"/blah/a", "/blubb/b"}) {
+            final HttpGet httpGet = new HttpGet(requestPath);
+            client.execute(target, httpGet, context, response -> {
+                EntityUtils.consume(response.getEntity());
+                return null;
+            });
+        }
+
+        Assertions.assertEquals(Arrays.asList(401, 200, 401, 200), responseCodes);
+
+        assertHandshakePerPath(requests, "/blah/a");
+        assertHandshakePerPath(requests, "/blubb/b");
+    }
+
+    private static void assertHandshakePerPath(final List<RequestSnapshot> requests, final String path) {
+        final List<RequestSnapshot> filtered = requests.stream()
+                .filter(r -> path.equals(r.path))
+                .collect(Collectors.toList());
+
+        Assertions.assertEquals(2, filtered.size(), "Expected 2 requests for " + path + " (challenge + retry)");
+        Assertions.assertFalse(filtered.get(0).hasAuthorization, "First request to " + path + " must not be preemptive");
+        Assertions.assertTrue(filtered.get(1).hasAuthorization, "Second request to " + path + " must carry Authorization");
+    }
+
+    private static final class RequestSnapshot {
+        private final String path;
+        private final boolean hasAuthorization;
+
+        private RequestSnapshot(final String path, final boolean hasAuthorization) {
+            this.path = path;
+            this.hasAuthorization = hasAuthorization;
+        }
+    }
+
+    @Test
+    void testBasicAuthenticationCredentialsCachingPerPrefixAndReuseWithinPrefix() throws Exception {
+        final List<RequestSnapshot> requests = new CopyOnWriteArrayList<>();
+        final Authenticator authenticator = new BasicTestAuthenticator("test:test", "test realm") {
+            @Override
+            public AuthResult perform(final URIAuthority authority,
+                                      final String requestUri,
+                                      final String credentials) {
+                requests.add(new RequestSnapshot(requestUri, credentials != null));
+                return super.perform(authority, requestUri, credentials);
+            }
+        };
+        configureServerWithBasicAuth(authenticator, bootstrap -> bootstrap.register("*", new EchoHandler()));
+        final HttpHost target = startServer();
+
+        final List<Integer> responseCodes = new CopyOnWriteArrayList<>();
+        configureClient(builder -> builder
+                .addResponseInterceptorLast((response, entity, context) -> responseCodes.add(response.getCode())));
+
+        final TestClient client = client();
+
+        final CredentialsProvider credentialsProvider = CredentialsProviderBuilder.create()
+                .add(target, "test", "test".toCharArray())
+                .build();
+
+        final HttpClientContext context = HttpClientContext.create();
+        context.setAuthCache(new BasicAuthCache());
+        context.setCredentialsProvider(credentialsProvider);
+
+        // First hit per prefix must challenge; subsequent hits under same prefix should be preemptive (200 only)
+        for (final String requestPath : new String[]{"/blah/a", "/blubb/b", "/blubb/c", "/blah/d"}) {
+            final HttpGet httpGet = new HttpGet(requestPath);
+            client.execute(target, httpGet, context, response -> {
+                EntityUtils.consume(response.getEntity());
+                return null;
+            });
+        }
+
+        Assertions.assertEquals(Arrays.asList(401, 200, 401, 200, 200, 200), responseCodes);
+
+        assertHandshakePerPath(requests, "/blah/a");
+        assertHandshakePerPath(requests, "/blubb/b");
+        assertPreemptivePerPath(requests, "/blubb/c");
+        assertPreemptivePerPath(requests, "/blah/d");
+    }
+
+    private static void assertPreemptivePerPath(final List<RequestSnapshot> requests, final String path) {
+        final List<RequestSnapshot> filtered = requests.stream()
+                .filter(r -> path.equals(r.path))
+                .collect(Collectors.toList());
+
+        Assertions.assertEquals(1, filtered.size(), "Expected 1 request for " + path + " (preemptive)");
+        Assertions.assertTrue(filtered.get(0).hasAuthorization, "Request to " + path + " must carry Authorization");
+    }
+
+
 }

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java

@@ -159,9 +159,6 @@ public void execute(
             // that of the previous authentication exchange.
             targetAuthExchange.reset();
         }
-        if (targetAuthExchange.getPathPrefix() == null) {
-            targetAuthExchange.setPathPrefix(pathPrefix);
-        }
 
         if (authCacheKeeper != null) {
             authCacheKeeper.loadPreemptively(target, pathPrefix, targetAuthExchange, clientContext);
@@ -340,6 +337,9 @@ private boolean needAuthentication(
                 targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response,
                         targetAuthStrategy, targetAuthExchange, context);
 
+                if (!targetAuthExchange.isConnectionBased() && targetAuthExchange.getPathPrefix() == null) {
+                    targetAuthExchange.setPathPrefix(pathPrefix);
+                }
                 if (authCacheKeeper != null) {
                     authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context);
                 }

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java

@@ -163,9 +163,6 @@ public ClassicHttpResponse execute(
                 // that of the previous authentication exchange.
                 targetAuthExchange.reset();
             }
-            if (targetAuthExchange.getPathPrefix() == null) {
-                targetAuthExchange.setPathPrefix(pathPrefix);
-            }
 
             if (authCacheKeeper != null) {
                 authCacheKeeper.loadPreemptively(target, pathPrefix, targetAuthExchange, context);
@@ -301,6 +298,9 @@ private boolean needAuthentication(
                 targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response,
                         targetAuthStrategy, targetAuthExchange, context);
 
+                if (!targetAuthExchange.isConnectionBased() && targetAuthExchange.getPathPrefix() == null) {
+                    targetAuthExchange.setPathPrefix(pathPrefix);
+                }
                 if (authCacheKeeper != null) {
                     authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context);
                 }

httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java

@@ -0,0 +1,159 @@
+/*
+ * ====================================================================
+ * 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hc.client5.http.AuthenticationStrategy;
+import org.apache.hc.client5.http.HttpRoute;
+import org.apache.hc.client5.http.async.AsyncExecCallback;
+import org.apache.hc.client5.http.async.AsyncExecChain;
+import org.apache.hc.client5.http.async.AsyncExecRuntime;
+import org.apache.hc.client5.http.auth.AuthExchange;
+import org.apache.hc.client5.http.auth.AuthScope;
+import org.apache.hc.client5.http.auth.ChallengeType;
+import org.apache.hc.client5.http.auth.StandardAuthScheme;
+import org.apache.hc.client5.http.impl.auth.BasicScheme;
+import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder;
+import org.apache.hc.client5.http.protocol.HttpClientContext;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.message.BasicHttpResponse;
+import org.apache.hc.core5.http.nio.AsyncDataConsumer;
+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+import org.apache.hc.core5.http.support.BasicRequestBuilder;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+
+class TestAsyncProtocolExec {
+
+    @Mock
+    private AuthenticationStrategy targetAuthStrategy;
+    @Mock
+    private AuthenticationStrategy proxyAuthStrategy;
+    @Mock
+    private AsyncExecChain chain;
+    @Mock
+    private AsyncExecRuntime execRuntime;
+
+    private AsyncProtocolExec protocolExec;
+    private HttpHost target;
+
+    @BeforeEach
+    void setup() {
+        MockitoAnnotations.openMocks(this);
+        protocolExec = new AsyncProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, true);
+        target = new HttpHost("http", "foo", 80);
+    }
+
+    @Test
+    void testAuthExchangePathPrefixRestoredAfterChallenge() throws Exception {
+        final HttpRoute route = new HttpRoute(target);
+        final HttpClientContext context = HttpClientContext.create();
+        context.setCredentialsProvider(CredentialsProviderBuilder.create()
+                .add(new AuthScope(target), "user", "pass".toCharArray())
+                .build());
+
+        Mockito.when(targetAuthStrategy.select(
+                        Mockito.eq(ChallengeType.TARGET),
+                        Mockito.any(),
+                        Mockito.<HttpClientContext>any()))
+                .thenReturn(Collections.singletonList(new BasicScheme()));
+
+        Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true);
+
+        final AtomicInteger proceedCount = new AtomicInteger(0);
+
+        Mockito.doAnswer(invocation -> {
+            final AsyncExecCallback cb = invocation.getArgument(3, AsyncExecCallback.class);
+            final int i = proceedCount.getAndIncrement();
+
+            if (i == 0) {
+                final HttpResponse resp1 = new BasicHttpResponse(401, "Huh?");
+                resp1.setHeader(HttpHeaders.WWW_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test");
+                cb.handleResponse(resp1, (EntityDetails) null);
+                cb.completed();
+            } else {
+                final HttpResponse resp2 = new BasicHttpResponse(200, "OK");
+                cb.handleResponse(resp2, (EntityDetails) null);
+                cb.completed();
+            }
+            return null;
+        }).when(chain).proceed(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
+
+        final HttpRequest request = BasicRequestBuilder.get("http://foo/blah/a").build();
+
+        final CancellableDependency dependency = Mockito.mock(CancellableDependency.class);
+        final AsyncExecChain.Scope scope = new AsyncExecChain.Scope(
+                "test",
+                route,
+                request,
+                dependency,
+                context,
+                execRuntime,
+                null,
+                new AtomicInteger(1));
+
+        final AsyncExecCallback asyncCallback = new AsyncExecCallback() {
+            @Override
+            public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails)
+                    throws IOException {
+                return Mockito.mock(AsyncDataConsumer.class);
+            }
+
+            @Override
+            public void handleInformationResponse(final HttpResponse response) {
+            }
+
+            @Override
+            public void completed() {
+            }
+
+            @Override
+            public void failed(final Exception cause) {
+                Assertions.fail(cause);
+            }
+        };
+
+        protocolExec.execute(request, (AsyncEntityProducer) null, scope, chain, asyncCallback);
+
+        Assertions.assertEquals(2, proceedCount.get());
+
+        final AuthExchange authExchange = context.getAuthExchange(target);
+        Assertions.assertEquals("/blah/", authExchange.getPathPrefix());
+    }
+}