From bc3d8a89b6a80e993911aef3e2210799269747a8 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Wed, 23 Mar 2016 16:18:53 +0100 Subject: [PATCH 1/6] add tests of HttpDownloader --- app/build.gradle | 2 + .../fdroid/fdroid/net/HttpDownloaderTest.java | 59 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java diff --git a/app/build.gradle b/app/build.gradle index 1280c28d3..8f53c13bc 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -28,6 +28,8 @@ dependencies { } compile 'io.reactivex:rxjava:1.1.0' compile 'io.reactivex:rxandroid:0.23.0' + + testCompile 'junit:junit:4.12' } if (!hasProperty('sourceDeps')) { diff --git a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java new file mode 100644 index 000000000..a12f34cd6 --- /dev/null +++ b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -0,0 +1,59 @@ + +package org.fdroid.fdroid.net; + +import org.junit.Test; + +import java.io.File; +import java.io.IOException; +import java.net.URL; + +import static org.junit.Assert.assertTrue; + +public class HttpDownloaderTest { + + String[] urls = { + "https://www.google.com", + "https://en.wikipedia.org/wiki/Index.html", + "https://mirrors.kernel.org/debian/dists/stable/Release", + "https://f-droid.org/archive/de.we.acaldav_5.apk", + // sites that use SNI for HTTPS + "https://guardianproject.info/fdroid/repo/index.jar", + "https://firstlook.org", + }; + + private boolean receivedProgress; + + @Test + public void downloadUninterruptedTest() throws IOException, InterruptedException { + for (String urlString : urls) { + URL url = new URL(urlString); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + httpDownloader.download(); + assertTrue(destFile.exists()); + assertTrue(destFile.canRead()); + } + } + + @Test + public void downloadUninterruptedTestWithProgress() throws IOException, InterruptedException { + for (String urlString : urls) { + receivedProgress = false; + URL url = new URL(urlString); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + httpDownloader.setListener(new Downloader.DownloaderProgressListener() { + @Override + public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + receivedProgress = true; + } + }); + httpDownloader.download(); + assertTrue(destFile.exists()); + assertTrue(destFile.canRead()); + assertTrue(receivedProgress); + } + } +} From 74274d21b4948c19eea57ab85ef4bdfe02aeac0a Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 22 Mar 2016 21:02:24 +0100 Subject: [PATCH 2/6] move SanitizedFileTest into non-Android tests It can run in plain java, so might as well. --- .../org/fdroid/fdroid/data}/SanitizedFileTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) rename app/src/{androidTest/java/org/fdroid/fdroid => test/java/org/fdroid/fdroid/data}/SanitizedFileTest.java (87%) diff --git a/app/src/androidTest/java/org/fdroid/fdroid/SanitizedFileTest.java b/app/src/test/java/org/fdroid/fdroid/data/SanitizedFileTest.java similarity index 87% rename from app/src/androidTest/java/org/fdroid/fdroid/SanitizedFileTest.java rename to app/src/test/java/org/fdroid/fdroid/data/SanitizedFileTest.java index 6a9a9068b..f64c74a1a 100644 --- a/app/src/androidTest/java/org/fdroid/fdroid/SanitizedFileTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/SanitizedFileTest.java @@ -1,14 +1,14 @@ -package org.fdroid.fdroid; +package org.fdroid.fdroid.data; -import android.test.AndroidTestCase; - -import org.fdroid.fdroid.data.SanitizedFile; +import org.junit.Test; import java.io.File; -@SuppressWarnings("PMD") // TODO port this to JUnit 4 semantics -public class SanitizedFileTest extends AndroidTestCase { +import static org.junit.Assert.assertEquals; +public class SanitizedFileTest { + + @Test public void testSanitizedFile() { File directory = new File("/tmp/blah"); From 591b23b5ab465d8ce18bdeee823df74daba86d5e Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 29 Mar 2016 19:19:09 +0200 Subject: [PATCH 3/6] Downloader.cancelDownload() instead of using external Thread logic This is needed so that downloads can be canceled from within an IntentService. Since the Downloader classes do not have any Thread logic in them, they shouldn't use Thread logic within them anyway. This also removes the unused argument to AsyncDownloader.attemptCancel(). --- .../java/org/fdroid/fdroid/AppDetails.java | 4 +- .../org/fdroid/fdroid/net/ApkDownloader.java | 6 +-- .../fdroid/net/AsyncDownloadWrapper.java | 6 +-- .../fdroid/fdroid/net/AsyncDownloader.java | 2 +- .../org/fdroid/fdroid/net/Downloader.java | 11 +++++- .../fdroid/fdroid/net/HttpDownloaderTest.java | 37 +++++++++++++++++++ 6 files changed, 55 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 9786e0437..7f2cd3e07 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -556,7 +556,7 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A @Override protected void onDestroy() { if (downloadHandler != null && !inProcessOfChangingConfiguration) { - downloadHandler.cancel(false); + downloadHandler.cancel(); cleanUpFinishedDownload(); } inProcessOfChangingConfiguration = false; @@ -1553,7 +1553,7 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A return; } - activity.downloadHandler.cancel(true); + activity.downloadHandler.cancel(); activity.cleanUpFinishedDownload(); setProgressVisible(false); updateViews(); diff --git a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java index 54b3bcc61..203c41893 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java @@ -253,12 +253,10 @@ public class ApkDownloader implements AsyncDownloader.Listener { /** * Attempts to cancel the download (if in progress) and also removes the progress * listener - * - * @param userRequested - true if the user requested the cancel (via button click), otherwise false. */ - public void cancel(boolean userRequested) { + public void cancel() { if (dlWrapper != null) { - dlWrapper.attemptCancel(userRequested); + dlWrapper.attemptCancel(); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java index 9d67c0b46..e56300c64 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java @@ -43,9 +43,9 @@ class AsyncDownloadWrapper extends Handler implements AsyncDownloader { downloadThread.start(); } - public void attemptCancel(boolean userRequested) { - if (downloadThread != null) { - downloadThread.interrupt(); + public void attemptCancel() { + if (downloader != null) { + downloader.cancelDownload(); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java index 0cf19d4b4..4f098c003 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java @@ -16,6 +16,6 @@ public interface AsyncDownloader { void download(); - void attemptCancel(boolean userRequested); + void attemptCancel(); } diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index 180d3aebd..f48b7e84a 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -23,6 +23,8 @@ public abstract class Downloader { public static final String EXTRA_BYTES_READ = "extraBytesRead"; public static final String EXTRA_TOTAL_BYTES = "extraTotalBytes"; + private volatile boolean cancelled = false; + private final OutputStream outputStream; private final File outputFile; @@ -124,12 +126,19 @@ public abstract class Downloader { * @throws InterruptedException */ private void throwExceptionIfInterrupted() throws InterruptedException { - if (Thread.interrupted()) { + if (cancelled) { Utils.debugLog(TAG, "Received interrupt, cancelling download"); throw new InterruptedException(); } } + /** + * Cancel a running download, triggering an {@link InterruptedException} + */ + public void cancelDownload() { + cancelled = true; + } + /** * This copies the downloaded data from the InputStream to the OutputStream, * keeping track of the number of bytes that have flowed through for the diff --git a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java index a12f34cd6..86284e5b2 100644 --- a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -6,8 +6,11 @@ import org.junit.Test; import java.io.File; import java.io.IOException; import java.net.URL; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; public class HttpDownloaderTest { @@ -56,4 +59,38 @@ public class HttpDownloaderTest { assertTrue(receivedProgress); } } + + @Test + public void downloadThenCancel() throws IOException, InterruptedException { + final CountDownLatch latch = new CountDownLatch(5); + URL url = new URL("https://f-droid.org/repo/index.jar"); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + final HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + httpDownloader.setListener(new Downloader.DownloaderProgressListener() { + @Override + public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { + System.out.println("DownloaderProgressListener.sendProgress " + bytesRead + " / " + totalBytes); + receivedProgress = true; + latch.countDown(); + } + }); + new Thread(){ + @Override + public void run() { + try { + httpDownloader.download(); + fail(); + } catch (IOException e) { + e.printStackTrace(); + fail(); + } catch (InterruptedException e) { + // success! + } + } + }.start(); + latch.await(100, TimeUnit.SECONDS); // either 5 progress reports or 100 seconds + httpDownloader.cancelDownload(); + assertTrue(receivedProgress); + } } From ae0976d24a57ac7033392f134ed8668de816a4fb Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Mon, 28 Mar 2016 22:17:36 +0200 Subject: [PATCH 4/6] move HTTP Auth to HttpDownloader to make it testable This also encapsulates the HTTP Auth stuff better so that it will be easier to wrap it all into a event-based service. --- .../java/org/fdroid/fdroid/AppDetails.java | 12 ----- .../java/org/fdroid/fdroid/RepoUpdater.java | 4 +- .../org/fdroid/fdroid/data/Credentials.java | 36 ------------- .../java/org/fdroid/fdroid/data/Repo.java | 16 ------ .../org/fdroid/fdroid/net/ApkDownloader.java | 8 +-- .../fdroid/fdroid/net/DownloaderFactory.java | 21 +++++--- .../org/fdroid/fdroid/net/HttpDownloader.java | 29 ++++++++--- .../fdroid/net/auth/HttpBasicCredentials.java | 51 ------------------- .../fdroid/fdroid/net/HttpDownloaderTest.java | 40 +++++++++++++-- 9 files changed, 75 insertions(+), 142 deletions(-) delete mode 100644 app/src/main/java/org/fdroid/fdroid/data/Credentials.java delete mode 100644 app/src/main/java/org/fdroid/fdroid/net/auth/HttpBasicCredentials.java diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index 7f2cd3e07..f54517aef 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -84,7 +84,6 @@ import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.ApkProvider; import org.fdroid.fdroid.data.App; import org.fdroid.fdroid.data.AppProvider; -import org.fdroid.fdroid.data.Credentials; import org.fdroid.fdroid.data.InstalledAppProvider; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; @@ -857,19 +856,8 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A return repo.address; } - @Nullable - private Credentials getRepoCredentials(Apk apk) { - final String[] projection = {RepoProvider.DataColumns.USERNAME, RepoProvider.DataColumns.PASSWORD}; - Repo repo = RepoProvider.Helper.findById(this, apk.repo, projection); - if (repo == null || repo.username == null || repo.password == null) { - return null; - } - return repo.getCredentials(); - } - private void startDownload(Apk apk, String repoAddress) { downloadHandler = new ApkDownloader(getBaseContext(), app, apk, repoAddress); - downloadHandler.setCredentials(getRepoCredentials(apk)); localBroadcastManager.registerReceiver(downloaderProgressReceiver, new IntentFilter(Downloader.LOCAL_ACTION_PROGRESS)); diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index f38c4c3df..a03992450 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -98,9 +98,7 @@ public class RepoUpdater { Downloader downloader = null; try { downloader = DownloaderFactory.create(context, - getIndexAddress(), File.createTempFile("index-", "-downloaded", context.getCacheDir()), - repo.getCredentials() - ); + getIndexAddress(), File.createTempFile("index-", "-downloaded", context.getCacheDir())); downloader.setCacheTag(repo.lastetag); downloader.download(); diff --git a/app/src/main/java/org/fdroid/fdroid/data/Credentials.java b/app/src/main/java/org/fdroid/fdroid/data/Credentials.java deleted file mode 100644 index db6aea0d7..000000000 --- a/app/src/main/java/org/fdroid/fdroid/data/Credentials.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright (C) 2015 Christian Morgner (christian.morgner@structr.com) - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version 3 - * of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, - * MA 02110-1301, USA. - */ - -package org.fdroid.fdroid.data; - -import java.net.HttpURLConnection; - -/** - * Credentials to authenticate HTTP requests. Implementations if this interface - * should encapsulate the authentication of an HTTP request in the authenticate - * method. - */ -public interface Credentials { - - /** - * Implement this method to provide authentication for the given connection. - * @param connection the HTTP connection to authenticate - */ - void authenticate(final HttpURLConnection connection); -} diff --git a/app/src/main/java/org/fdroid/fdroid/data/Repo.java b/app/src/main/java/org/fdroid/fdroid/data/Repo.java index abbbf791a..ff4fa6711 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Repo.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Repo.java @@ -5,7 +5,6 @@ import android.database.Cursor; import android.text.TextUtils; import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.net.auth.HttpBasicCredentials; import java.net.MalformedURLException; import java.net.URL; @@ -149,21 +148,6 @@ public class Repo extends ValueObject { return value; } - /** - * Returns the credentials for this repo, or null of no authentication - * method is configured. - * @return the credentials or null - */ - public Credentials getCredentials() { - - // return the only credentials implementation we have right now - if (!TextUtils.isEmpty(username) && !TextUtils.isEmpty(password)) { - return new HttpBasicCredentials(username, password); - } - - return null; - } - public void setValues(ContentValues values) { if (values.containsKey(RepoProvider.DataColumns._ID)) { diff --git a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java index 203c41893..d1f89f6e8 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java @@ -36,7 +36,6 @@ import org.fdroid.fdroid.Utils; import org.fdroid.fdroid.compat.FileCompat; import org.fdroid.fdroid.data.Apk; import org.fdroid.fdroid.data.App; -import org.fdroid.fdroid.data.Credentials; import org.fdroid.fdroid.data.SanitizedFile; import java.io.File; @@ -69,7 +68,6 @@ public class ApkDownloader implements AsyncDownloader.Listener { private ProgressListener listener; private AsyncDownloader dlWrapper; - private Credentials credentials; private boolean isComplete; private final long id = ++downloadIdCounter; @@ -188,7 +186,7 @@ public class ApkDownloader implements AsyncDownloader.Listener { Utils.debugLog(TAG, "Downloading apk from " + remoteAddress + " to " + localFile); try { - dlWrapper = DownloaderFactory.createAsync(context, remoteAddress, localFile, credentials, this); + dlWrapper = DownloaderFactory.createAsync(context, remoteAddress, localFile, this); dlWrapper.download(); return true; } catch (IOException e) { @@ -271,8 +269,4 @@ public class ApkDownloader implements AsyncDownloader.Listener { public int getTotalBytes() { return dlWrapper != null ? dlWrapper.getTotalBytes() : 0; } - - public void setCredentials(final Credentials credentials) { - this.credentials = credentials; - } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/DownloaderFactory.java b/app/src/main/java/org/fdroid/fdroid/net/DownloaderFactory.java index 4a159e3f6..57ea2b7b4 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/DownloaderFactory.java +++ b/app/src/main/java/org/fdroid/fdroid/net/DownloaderFactory.java @@ -4,7 +4,9 @@ import android.content.Context; import android.content.Intent; import android.support.v4.content.LocalBroadcastManager; -import org.fdroid.fdroid.data.Credentials; +import org.apache.commons.io.FilenameUtils; +import org.fdroid.fdroid.data.Repo; +import org.fdroid.fdroid.data.RepoProvider; import java.io.File; import java.io.IOException; @@ -23,10 +25,10 @@ public class DownloaderFactory { throws IOException { File destFile = File.createTempFile("dl-", "", context.getCacheDir()); destFile.deleteOnExit(); // this probably does nothing, but maybe... - return create(context, new URL(urlString), destFile, null); + return create(context, new URL(urlString), destFile); } - public static Downloader create(Context context, URL url, File destFile, Credentials credentials) + public static Downloader create(Context context, URL url, File destFile) throws IOException { Downloader downloader = null; if (localBroadcastManager == null) { @@ -39,7 +41,14 @@ public class DownloaderFactory { } else if (isLocalFile(url)) { downloader = new LocalFileDownloader(url, destFile); } else { - downloader = new HttpDownloader(url, destFile, credentials); + final String[] projection = {RepoProvider.DataColumns.USERNAME, RepoProvider.DataColumns.PASSWORD}; + String repoUrlString = FilenameUtils.getBaseName(url.toString()); + Repo repo = RepoProvider.Helper.findByAddress(context, repoUrlString, projection); + if (repo == null) { + downloader = new HttpDownloader(url, destFile); + } else { + downloader = new HttpDownloader(url, destFile, repo.username, repo.password); + } } downloader.setListener(new Downloader.DownloaderProgressListener() { @@ -63,9 +72,9 @@ public class DownloaderFactory { return "file".equalsIgnoreCase(url.getProtocol()); } - public static AsyncDownloader createAsync(Context context, String urlString, File destFile, Credentials credentials, AsyncDownloader.Listener listener) + public static AsyncDownloader createAsync(Context context, String urlString, File destFile, AsyncDownloader.Listener listener) throws IOException { URL url = new URL(urlString); - return new AsyncDownloadWrapper(create(context, url, destFile, credentials), listener); + return new AsyncDownloadWrapper(create(context, url, destFile), listener); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java index a7bfac24d..8550d9eaf 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/HttpDownloader.java @@ -6,7 +6,7 @@ import com.nostra13.universalimageloader.core.download.BaseImageDownloader; import org.fdroid.fdroid.FDroidApp; import org.fdroid.fdroid.Utils; -import org.fdroid.fdroid.data.Credentials; +import org.spongycastle.util.encoders.Base64; import java.io.BufferedInputStream; import java.io.File; @@ -28,20 +28,33 @@ public class HttpDownloader extends Downloader { protected static final String HEADER_IF_NONE_MATCH = "If-None-Match"; protected static final String HEADER_FIELD_ETAG = "ETag"; + public final String username; + public final String password; protected HttpURLConnection connection; - private Credentials credentials; private int statusCode = -1; HttpDownloader(URL url, File destFile) throws FileNotFoundException, MalformedURLException { - this(url, destFile, null); + this(url, destFile, null, null); } - HttpDownloader(URL url, File destFile, final Credentials credentials) + /** + * Create a downloader that can authenticate via HTTP Basic Auth using the supplied + * {@code username} and {@code password}. + * + * @param url The file to download + * @param destFile Where the download is saved + * @param username Username for HTTP Basic Auth, use {@code null} to ignore + * @param password Password for HTTP Basic Auth, use {@code null} to ignore + * @throws FileNotFoundException + * @throws MalformedURLException + */ + HttpDownloader(URL url, File destFile, String username, String password) throws FileNotFoundException, MalformedURLException { super(url, destFile); - this.credentials = credentials; + this.username = username; + this.password = password; } /** @@ -106,8 +119,10 @@ public class HttpDownloader extends Downloader { ((HttpsURLConnection) connection).setSSLSocketFactory(HttpsURLConnection.getDefaultSSLSocketFactory()); } - if (credentials != null) { - credentials.authenticate(connection); + if (username != null && password != null) { + // add authorization header from username / password if set + String authString = username + ":" + password; + connection.setRequestProperty("Authorization", "Basic " + Base64.toBase64String(authString.getBytes())); } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/auth/HttpBasicCredentials.java b/app/src/main/java/org/fdroid/fdroid/net/auth/HttpBasicCredentials.java deleted file mode 100644 index 6557b26c8..000000000 --- a/app/src/main/java/org/fdroid/fdroid/net/auth/HttpBasicCredentials.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright (C) 2015 Christian Morgner (christian.morgner@structr.com) - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU General Public License - * as published by the Free Software Foundation; either version 3 - * of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, - * MA 02110-1301, USA. - */ - -package org.fdroid.fdroid.net.auth; - -import android.text.TextUtils; - -import org.apache.commons.net.util.Base64; -import org.fdroid.fdroid.data.Credentials; - -import java.net.HttpURLConnection; - -/** - * Credentials implementation for HTTP Basic Authentication. - */ -public class HttpBasicCredentials implements Credentials { - - private final String username; - private final String password; - - public HttpBasicCredentials(final String username, final String password) { - this.username = username; - this.password = password; - } - - @Override - public void authenticate(final HttpURLConnection connection) { - - if (!TextUtils.isEmpty(username) && !TextUtils.isEmpty(password)) { - - // add authorization header from username / password if set - connection.setRequestProperty("Authorization", "Basic " + Base64.encodeBase64String((username + ":" + password).getBytes())); - } - } -} diff --git a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java index 86284e5b2..7075e3260 100644 --- a/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/net/HttpDownloaderTest.java @@ -9,6 +9,7 @@ import java.net.URL; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -32,7 +33,7 @@ public class HttpDownloaderTest { URL url = new URL(urlString); File destFile = File.createTempFile("dl-", ""); destFile.deleteOnExit(); // this probably does nothing, but maybe... - HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + HttpDownloader httpDownloader = new HttpDownloader(url, destFile); httpDownloader.download(); assertTrue(destFile.exists()); assertTrue(destFile.canRead()); @@ -46,7 +47,7 @@ public class HttpDownloaderTest { URL url = new URL(urlString); File destFile = File.createTempFile("dl-", ""); destFile.deleteOnExit(); // this probably does nothing, but maybe... - HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + HttpDownloader httpDownloader = new HttpDownloader(url, destFile); httpDownloader.setListener(new Downloader.DownloaderProgressListener() { @Override public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { @@ -60,13 +61,44 @@ public class HttpDownloaderTest { } } + @Test + public void downloadHttpBasicAuth() throws IOException, InterruptedException { + URL url = new URL("https://httpbin.org/basic-auth/myusername/supersecretpassword"); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + HttpDownloader httpDownloader = new HttpDownloader(url, destFile, "myusername", "supersecretpassword"); + httpDownloader.download(); + assertTrue(destFile.exists()); + assertTrue(destFile.canRead()); + } + + @Test(expected = IOException.class) + public void downloadHttpBasicAuthWrongPassword() throws IOException, InterruptedException { + URL url = new URL("https://httpbin.org/basic-auth/myusername/supersecretpassword"); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + HttpDownloader httpDownloader = new HttpDownloader(url, destFile, "myusername", "wrongpassword"); + httpDownloader.download(); + assertFalse(destFile.exists()); + } + + @Test(expected = IOException.class) + public void downloadHttpBasicAuthWrongUsername() throws IOException, InterruptedException { + URL url = new URL("https://httpbin.org/basic-auth/myusername/supersecretpassword"); + File destFile = File.createTempFile("dl-", ""); + destFile.deleteOnExit(); // this probably does nothing, but maybe... + HttpDownloader httpDownloader = new HttpDownloader(url, destFile, "wrongusername", "supersecretpassword"); + httpDownloader.download(); + assertFalse(destFile.exists()); + } + @Test public void downloadThenCancel() throws IOException, InterruptedException { final CountDownLatch latch = new CountDownLatch(5); URL url = new URL("https://f-droid.org/repo/index.jar"); File destFile = File.createTempFile("dl-", ""); destFile.deleteOnExit(); // this probably does nothing, but maybe... - final HttpDownloader httpDownloader = new HttpDownloader(url, destFile, null); + final HttpDownloader httpDownloader = new HttpDownloader(url, destFile); httpDownloader.setListener(new Downloader.DownloaderProgressListener() { @Override public void sendProgress(URL sourceUrl, int bytesRead, int totalBytes) { @@ -75,7 +107,7 @@ public class HttpDownloaderTest { latch.countDown(); } }); - new Thread(){ + new Thread() { @Override public void run() { try { From 8befba0522228d074e77d088f8bc853ab00d72d7 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 29 Mar 2016 16:27:49 +0200 Subject: [PATCH 5/6] send Downloader progress only via its DownloaderProgressListener Instead of duplicate APIs, standardize on a single API, and use that everywhere via the Downloader.LOCAL_ACTION_PROGRESS event that is already wired in. --- app/src/main/java/org/fdroid/fdroid/AppDetails.java | 7 +------ .../java/org/fdroid/fdroid/net/ApkDownloader.java | 8 -------- .../org/fdroid/fdroid/net/AsyncDownloadWrapper.java | 8 -------- .../java/org/fdroid/fdroid/net/AsyncDownloader.java | 4 ---- .../main/java/org/fdroid/fdroid/net/Downloader.java | 13 +------------ 5 files changed, 2 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/AppDetails.java b/app/src/main/java/org/fdroid/fdroid/AppDetails.java index f54517aef..5cda16ec3 100644 --- a/app/src/main/java/org/fdroid/fdroid/AppDetails.java +++ b/app/src/main/java/org/fdroid/fdroid/AppDetails.java @@ -459,12 +459,7 @@ public class AppDetails extends AppCompatActivity implements ProgressListener, A localBroadcastManager.registerReceiver(downloaderProgressReceiver, new IntentFilter(Downloader.LOCAL_ACTION_PROGRESS)); downloadHandler.setProgressListener(this); - - if (downloadHandler.getTotalBytes() == 0) { - headerFragment.startProgress(); - } else { - headerFragment.updateProgress(downloadHandler.getBytesRead(), downloadHandler.getTotalBytes()); - } + headerFragment.startProgress(); } } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java index d1f89f6e8..ac3a5ff09 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/ApkDownloader.java @@ -261,12 +261,4 @@ public class ApkDownloader implements AsyncDownloader.Listener { public Apk getApk() { return curApk; } - - public int getBytesRead() { - return dlWrapper != null ? dlWrapper.getBytesRead() : 0; - } - - public int getTotalBytes() { - return dlWrapper != null ? dlWrapper.getTotalBytes() : 0; - } } diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java index e56300c64..37992a85a 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloadWrapper.java @@ -30,14 +30,6 @@ class AsyncDownloadWrapper extends Handler implements AsyncDownloader { this.listener = listener; } - public int getBytesRead() { - return downloader.getBytesRead(); - } - - public int getTotalBytes() { - return downloader.getTotalBytes(); - } - public void download() { downloadThread = new DownloadThread(); downloadThread.start(); diff --git a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java index 4f098c003..413b76a05 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/AsyncDownloader.java @@ -10,10 +10,6 @@ public interface AsyncDownloader { void onDownloadComplete(); } - int getBytesRead(); - - int getTotalBytes(); - void download(); void attemptCancel(); diff --git a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java index f48b7e84a..61b6af116 100644 --- a/app/src/main/java/org/fdroid/fdroid/net/Downloader.java +++ b/app/src/main/java/org/fdroid/fdroid/net/Downloader.java @@ -31,8 +31,6 @@ public abstract class Downloader { protected final URL sourceUrl; protected String cacheTag; - private int bytesRead; - private int totalBytes; interface DownloaderProgressListener { void sendProgress(URL sourceUrl, int bytesRead, int totalBytes); @@ -147,7 +145,7 @@ public abstract class Downloader { private void copyInputToOutputStream(InputStream input, int bufferSize) throws IOException, InterruptedException { int bytesRead = 0; - this.totalBytes = totalDownloadSize(); + int totalBytes = totalDownloadSize(); byte[] buffer = new byte[bufferSize]; // Getting the total download size could potentially take time, depending on how @@ -182,20 +180,11 @@ public abstract class Downloader { } private void sendProgress(int bytesRead, int totalBytes) { - this.bytesRead = bytesRead; if (downloaderProgressListener != null) { downloaderProgressListener.sendProgress(sourceUrl, bytesRead, totalBytes); } } - public int getBytesRead() { - return bytesRead; - } - - public int getTotalBytes() { - return totalBytes; - } - /** * Overrides every method in {@link InputStream} and delegates to the wrapped stream. * The only difference is that when we call the {@link WrappedInputStream#close()} method, From 0bf221383b0d02da0d7199cdfc4aee26f85669a0 Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Thu, 31 Mar 2016 17:27:15 +0200 Subject: [PATCH 6/6] make RepoUpdater's index URL a property for easy use Since the DownloaderService's events are all based on the complete download URLs, and RepoUpdater is where the update URLs are built, this makes the full download URL into a read-only property of RepoUpdater so it can be used wherever there is an instance of RepoUpdater This is also important because having the `final` property highlights the lifecycle of that variable: it does not change during the entire life of a RepoUpdater Instance. --- .../java/org/fdroid/fdroid/RepoUpdater.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java index a03992450..2c75c06bd 100644 --- a/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java +++ b/app/src/main/java/org/fdroid/fdroid/RepoUpdater.java @@ -21,8 +21,6 @@ import org.xml.sax.XMLReader; import java.io.File; import java.io.IOException; import java.io.InputStream; -import java.net.MalformedURLException; -import java.net.URL; import java.security.CodeSigner; import java.security.cert.Certificate; import java.security.cert.X509Certificate; @@ -54,6 +52,8 @@ public class RepoUpdater { public static final String PROGRESS_COMMITTING = "committing"; public static final String PROGRESS_DATA_REPO_ADDRESS = "repoAddress"; + public final String indexUrl; + @NonNull private final Context context; @NonNull @@ -75,6 +75,13 @@ public class RepoUpdater { this.context = context; this.repo = repo; this.persister = new RepoPersister(context, repo); + + String url = repo.address + "/index.jar"; + String versionName = Utils.getVersionName(context); + if (versionName != null) { + url += "?client_version=" + versionName; + } + this.indexUrl = url; } public void setProgressListener(@Nullable ProgressListener progressListener) { @@ -85,27 +92,17 @@ public class RepoUpdater { return hasChanged; } - private URL getIndexAddress() throws MalformedURLException { - String urlString = repo.address + "/index.jar"; - String versionName = Utils.getVersionName(context); - if (versionName != null) { - urlString += "?client_version=" + versionName; - } - return new URL(urlString); - } - private Downloader downloadIndex() throws UpdateException { Downloader downloader = null; try { - downloader = DownloaderFactory.create(context, - getIndexAddress(), File.createTempFile("index-", "-downloaded", context.getCacheDir())); + downloader = DownloaderFactory.create(context, indexUrl); downloader.setCacheTag(repo.lastetag); downloader.download(); if (downloader.isCached()) { // The index is unchanged since we last read it. We just mark // everything that came from this repo as being updated. - Utils.debugLog(TAG, "Repo index for " + getIndexAddress() + " is up to date (by etag)"); + Utils.debugLog(TAG, "Repo index for " + indexUrl + " is up to date (by etag)"); } } catch (IOException e) {