From e505cbe6602824aeebafd94c23ea07ae137d3266 Mon Sep 17 00:00:00 2001 From: Alin Dreghiciu Date: Tue, 25 Sep 2012 16:47:12 +0300 Subject: [PATCH] Use the path of remote url to check if the remote storage request is made for a collection Our check was using the full url that could include the query part as in for example "http://foo.com/bar?x=y/", case when we get a false positive Also added UTs for checking. The problem is only on http client 4, the old one behaves as expected Signed-off-by: Alin Dreghiciu --- .../httpclient/HttpClientRemoteStorage.java | 10 +- .../CommonsHttpClientRemoteStorageTest.java | 184 ++++++++++++++++++ .../HttpClientRemoteStorageTest.java | 168 ++++++++++++++++ 3 files changed, 358 insertions(+), 4 deletions(-) create mode 100644 nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/commonshttpclient/CommonsHttpClientRemoteStorageTest.java create mode 100644 nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorageTest.java diff --git a/nexus/nexus-proxy/src/main/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorage.java b/nexus/nexus-proxy/src/main/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorage.java index 3551a3edbc..fc1d584160 100644 --- a/nexus/nexus-proxy/src/main/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorage.java +++ b/nexus/nexus-proxy/src/main/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorage.java @@ -58,6 +58,7 @@ import org.sonatype.nexus.proxy.storage.remote.RemoteStorageContext; import org.sonatype.nexus.proxy.storage.remote.http.QueryStringBuilder; import org.sonatype.nexus.proxy.utils.UserAgentBuilder; +import com.google.common.annotations.VisibleForTesting; /** * Apache HTTP client (4) {@link RemoteRepositoryStorage} implementation. @@ -135,7 +136,7 @@ public AbstractStorageItem retrieveItem( final ProxyRepository repository, appendQueryString( getAbsoluteUrlFromBase( baseUrl, request.getRequestPath() ), repository ); final String url = remoteURL.toExternalForm(); - if ( url.endsWith( "/" ) ) + if ( remoteURL.getPath().endsWith( "/" ) ) { // NEXUS-5125 we do not want to fetch any collection // Even though it is unlikely that we actually see a request for a collection here, @@ -425,9 +426,10 @@ protected String getS3FlagKey() * @return response of making the request * @throws RemoteStorageException If an error occurred during execution of HTTP request */ - private HttpResponse executeRequest( final ProxyRepository repository, - final ResourceStoreRequest request, - final HttpUriRequest httpRequest ) + @VisibleForTesting + HttpResponse executeRequest( final ProxyRepository repository, + final ResourceStoreRequest request, + final HttpUriRequest httpRequest ) throws RemoteStorageException { final URI methodUri = httpRequest.getURI(); diff --git a/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/commonshttpclient/CommonsHttpClientRemoteStorageTest.java b/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/commonshttpclient/CommonsHttpClientRemoteStorageTest.java new file mode 100644 index 0000000000..a37f78853b --- /dev/null +++ b/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/commonshttpclient/CommonsHttpClientRemoteStorageTest.java @@ -0,0 +1,184 @@ +/** + * Sonatype Nexus (TM) Open Source Version + * Copyright (c) 2007-2012 Sonatype, Inc. + * All rights reserved. Includes the third-party code listed at http://links.sonatype.com/products/nexus/oss/attributions. + * + * This program and the accompanying materials are made available under the terms of the Eclipse Public License Version 1.0, + * which accompanies this distribution and is available at http://www.eclipse.org/legal/epl-v10.html. + * + * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks + * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the + * Eclipse Foundation. All other trademarks are the property of their respective owners. + */ +package org.sonatype.nexus.proxy.storage.remote.commonshttpclient; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.URL; + +import org.apache.commons.httpclient.HttpMethod; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.sonatype.nexus.ApplicationStatusSource; +import org.sonatype.nexus.mime.MimeSupport; +import org.sonatype.nexus.proxy.RemoteStorageException; +import org.sonatype.nexus.proxy.ResourceStoreRequest; +import org.sonatype.nexus.proxy.repository.ProxyRepository; +import org.sonatype.nexus.proxy.storage.remote.RemoteItemNotFoundException; +import org.sonatype.nexus.proxy.utils.UserAgentBuilder; +import org.sonatype.sisu.litmus.testsupport.TestSupport; + +/** + * {@link CommonsHttpClientRemoteStorage} UTs. + * + * @since 2.2 + */ +public class CommonsHttpClientRemoteStorageTest + extends TestSupport +{ + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + /** + * When retrieving an item with a path that ends with a "/" and without a query, + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndNoQuery() + throws Exception + { + final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ) + ) + { + @Override + protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request, + final HttpMethod method, + final URL remoteUrl ) + throws RemoteStorageException + { + return 200; + } + }; + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that ends with a "/" and a query string that ends with a "/", + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndQueryEndsWithSlash() + throws Exception + { + final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ) + ) + { + @Override + protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request, + final HttpMethod method, + final URL remoteUrl ) + throws RemoteStorageException + { + return 200; + } + }; + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/?param=x/" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that ends with a "/" and a query string that does not end with a "/", + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndQueryDoesNotEndWithSlash() + throws Exception + { + final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ) + ) + { + @Override + protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request, + final HttpMethod method, + final URL remoteUrl ) + throws RemoteStorageException + { + return 200; + } + }; + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/?param=x" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that does not end with a "/" and a query string that does not end with a "/", + * no exception should be thrown. + */ + @Test + public void retrieveCollectionWhenPathDoesNotEndWithSlashAndQueryDoesNotEndWithSlash() + throws Exception + { + final CommonsHttpClientRemoteStorage underTest = new CommonsHttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ) + ) + { + @Override + protected int executeMethod( final ProxyRepository repository, final ResourceStoreRequest request, + final HttpMethod method, + final URL remoteUrl ) + throws RemoteStorageException + { + return 200; + } + }; + + final ProxyRepository repository = mock( ProxyRepository.class ); + when( repository.getId() ).thenReturn( "foo" ); + + underTest.retrieveItem( + repository, + new ResourceStoreRequest( "bar?param=x" ), + "http://foo.com" + ); + } + +} diff --git a/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorageTest.java b/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorageTest.java new file mode 100644 index 0000000000..ee819f3081 --- /dev/null +++ b/nexus/nexus-proxy/src/test/java/org/sonatype/nexus/proxy/storage/remote/httpclient/HttpClientRemoteStorageTest.java @@ -0,0 +1,168 @@ +/** + * Sonatype Nexus (TM) Open Source Version + * Copyright (c) 2007-2012 Sonatype, Inc. + * All rights reserved. Includes the third-party code listed at http://links.sonatype.com/products/nexus/oss/attributions. + * + * This program and the accompanying materials are made available under the terms of the Eclipse Public License Version 1.0, + * which accompanies this distribution and is available at http://www.eclipse.org/legal/epl-v10.html. + * + * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks + * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the + * Eclipse Foundation. All other trademarks are the property of their respective owners. + */ +package org.sonatype.nexus.proxy.storage.remote.httpclient; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.URL; + +import org.apache.commons.httpclient.HttpMethod; +import org.apache.http.HttpEntity; +import org.apache.http.HttpResponse; +import org.apache.http.StatusLine; +import org.apache.http.client.methods.HttpUriRequest; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.sonatype.nexus.ApplicationStatusSource; +import org.sonatype.nexus.mime.MimeSupport; +import org.sonatype.nexus.proxy.RemoteStorageException; +import org.sonatype.nexus.proxy.ResourceStoreRequest; +import org.sonatype.nexus.proxy.repository.ProxyRepository; +import org.sonatype.nexus.proxy.storage.remote.RemoteItemNotFoundException; +import org.sonatype.nexus.proxy.storage.remote.commonshttpclient.CommonsHttpClientRemoteStorage; +import org.sonatype.nexus.proxy.storage.remote.http.QueryStringBuilder; +import org.sonatype.nexus.proxy.utils.UserAgentBuilder; +import org.sonatype.sisu.litmus.testsupport.TestSupport; + +/** + * {@link HttpClientRemoteStorage} UTs. + * + * @since 2.2 + */ +public class HttpClientRemoteStorageTest + extends TestSupport +{ + + @Rule + public ExpectedException thrown = ExpectedException.none(); + + /** + * When retrieving an item with a path that ends with a "/" and without a query, + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndNoQuery() + throws Exception + { + final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ), + mock( QueryStringBuilder.class ) + ); + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that ends with a "/" and a query string that ends with a "/", + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndQueryEndsWithSlash() + throws Exception + { + final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ), + mock( QueryStringBuilder.class ) + ); + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/?param=x/" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that ends with a "/" and a query string that does not end with a "/", + * an RemoteItemNotFoundException with a message that a collection could not be downloaded over HTTP should be + * thrown. + */ + @Test + public void retrieveCollectionWhenPathEndsWithSlashAndQueryDoesNotEndWithSlash() + throws Exception + { + final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ), + mock( QueryStringBuilder.class ) + ); + + thrown.expect( RemoteItemNotFoundException.class ); + thrown.expectMessage( "The remoteURL we got to looks like is a collection" ); + + underTest.retrieveItem( + mock( ProxyRepository.class ), + new ResourceStoreRequest( "bar/?param=x" ), + "http://foo.com" + ); + } + + /** + * When retrieving an item with a path that does not end with a "/" and a query string that does not end with a "/", + * no exception should be thrown. + */ + @Test + public void retrieveCollectionWhenPathDoesNotEndWithSlashAndQueryDoesNotEndWithSlash() + throws Exception + { + final HttpClientRemoteStorage underTest = new HttpClientRemoteStorage( + mock( UserAgentBuilder.class ), + mock( ApplicationStatusSource.class ), + mock( MimeSupport.class ), + mock( QueryStringBuilder.class ) + ) + { + @Override + HttpResponse executeRequest( final ProxyRepository repository, final ResourceStoreRequest request, + final HttpUriRequest httpRequest ) + throws RemoteStorageException + { + final HttpResponse httpResponse = mock( HttpResponse.class ); + final StatusLine statusLine = mock( StatusLine.class ); + when( httpResponse.getStatusLine() ).thenReturn( statusLine ); + when( statusLine.getStatusCode() ).thenReturn( 200 ); + when( httpResponse.getEntity() ).thenReturn( mock( HttpEntity.class ) ); + return httpResponse; + } + }; + + final ProxyRepository repository = mock( ProxyRepository.class ); + when( repository.getId() ).thenReturn( "foo" ); + + underTest.retrieveItem( + repository, + new ResourceStoreRequest( "bar?param=x" ), + "http://foo.com" + ); + } + +}