From a686529ba5d00dd499f634fecaf90dc34ea52c2c Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Jul 2016 10:35:41 +1000 Subject: [PATCH 1/2] Added tests for repo provider. These tests would've prevented the problem in #717, by ensuring that only a single repo is deleted at a time. --- .../org/fdroid/fdroid/data/RepoProvider.java | 2 +- .../fdroid/fdroid/data/RepoProviderTest.java | 155 ++++++++++++++++++ 2 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java diff --git a/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java b/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java index 27f8b0329..93e721e2d 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java @@ -313,7 +313,7 @@ public class RepoProvider extends FDroidProvider { values.put(Cols.VERSION, 0); } - if (!values.containsKey(Cols.NAME)) { + if (!values.containsKey(Cols.NAME) || values.get(Cols.NAME) == null) { final String address = values.getAsString(Cols.ADDRESS); values.put(Cols.NAME, Repo.addressToName(address)); } diff --git a/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java new file mode 100644 index 000000000..114cffca0 --- /dev/null +++ b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java @@ -0,0 +1,155 @@ +package org.fdroid.fdroid.data; + +import android.app.Application; +import android.content.ContentValues; +import android.support.annotation.Nullable; + +import org.fdroid.fdroid.BuildConfig; +import org.fdroid.fdroid.R; +import org.fdroid.fdroid.Utils; +import org.fdroid.fdroid.data.Schema.RepoTable; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricGradleTestRunner; +import org.robolectric.annotation.Config; + +import java.util.List; + +import static org.junit.Assert.assertEquals; + +@Config(constants = BuildConfig.class, application = Application.class) +@RunWith(RobolectricGradleTestRunner.class) +public class RepoProviderTest extends FDroidProviderTest { + + /** + * The {@link DBHelper} class populates four default repos when it first creates a database: + * * F-Droid + * * F-Droid (Archive) + * * Guardian Project + * * Guardian Project (Archive) + * The names/URLs/signing certificates for these repos are all hard coded in the source/res. + */ + @Test + public void defaultRepos() { + List defaultRepos = RepoProvider.Helper.all(context); + assertEquals(defaultRepos.size(), 4); + assertRepo( + defaultRepos.get(0), + context.getString(R.string.fdroid_repo_address), + context.getString(R.string.fdroid_repo_description), + Utils.calcFingerprint(context.getString(R.string.fdroid_repo_pubkey)), + context.getString(R.string.fdroid_repo_name) + ); + + assertRepo( + defaultRepos.get(1), + context.getString(R.string.fdroid_archive_address), + context.getString(R.string.fdroid_archive_description), + Utils.calcFingerprint(context.getString(R.string.fdroid_archive_pubkey)), + context.getString(R.string.fdroid_archive_name) + ); + + assertRepo( + defaultRepos.get(2), + context.getString(R.string.guardianproject_repo_address), + context.getString(R.string.guardianproject_repo_description), + Utils.calcFingerprint(context.getString(R.string.guardianproject_repo_pubkey)), + context.getString(R.string.guardianproject_repo_name) + ); + + assertRepo( + defaultRepos.get(3), + context.getString(R.string.guardianproject_archive_address), + context.getString(R.string.guardianproject_archive_description), + Utils.calcFingerprint(context.getString(R.string.guardianproject_archive_pubkey)), + context.getString(R.string.guardianproject_archive_name) + ); + } + + @Test + public void canAddRepo() { + + assertEquals(4, RepoProvider.Helper.all(context).size()); + + Repo mock1 = insertRepo( + "https://mock-repo-1.example.com/fdroid/repo", + "Just a made up repo", + "ABCDEF1234567890", + "Mock Repo 1" + ); + + Repo mock2 = insertRepo( + "http://mock-repo-2.example.com/fdroid/repo", + "Mock repo without a name", + "0123456789ABCDEF" + ); + + assertEquals(6, RepoProvider.Helper.all(context).size()); + + assertRepo( + mock1, + "https://mock-repo-1.example.com/fdroid/repo", + "Just a made up repo", + "ABCDEF1234567890", + "Mock Repo 1" + ); + + assertRepo( + mock2, + "http://mock-repo-2.example.com/fdroid/repo", + "Mock repo without a name", + "0123456789ABCDEF", + "mock-repo-2.example.com/fdroid/repo" + ); + } + + private static void assertRepo(Repo actualRepo, String expectedAddress, String expectedDescription, + String expectedFingerprint, String expectedName) { + assertEquals(expectedAddress, actualRepo.address); + assertEquals(expectedDescription, actualRepo.description); + assertEquals(expectedFingerprint, actualRepo.fingerprint); + assertEquals(expectedName, actualRepo.name); + } + + @Test + public void canDeleteRepo() { + Repo mock1 = insertRepo( + "https://mock-repo-1.example.com/fdroid/repo", + "Just a made up repo", + "ABCDEF1234567890", + "Mock Repo 1" + ); + + Repo mock2 = insertRepo( + "http://mock-repo-2.example.com/fdroid/repo", + "Mock repo without a name", + "0123456789ABCDEF" + ); + + List beforeDelete = RepoProvider.Helper.all(context); + assertEquals(6, beforeDelete.size()); // Expect six repos, because of the four default ones. + assertEquals(mock1.id, beforeDelete.get(4).id); + assertEquals(mock2.id, beforeDelete.get(5).id); + + RepoProvider.Helper.remove(context, mock1.getId()); + + List afterDelete = RepoProvider.Helper.all(context); + assertEquals(5, afterDelete.size()); + assertEquals(mock2.id, afterDelete.get(4).id); + } + + protected Repo insertRepo(String address, String description, String fingerprint) { + return insertRepo(address, description, fingerprint, null); + } + + protected Repo insertRepo(String address, String description, String fingerprint, @Nullable String name) { + ContentValues values = new ContentValues(); + values.put(RepoTable.Cols.ADDRESS, address); + values.put(RepoTable.Cols.DESCRIPTION, description); + values.put(RepoTable.Cols.FINGERPRINT, fingerprint); + values.put(RepoTable.Cols.NAME, name); + + RepoProvider.Helper.insert(context, values); + return RepoProvider.Helper.findByAddress(context, address); + } +} From 9fbcc255ab5b45f9a48491af5df3eebeb343718b Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Thu, 21 Jul 2016 14:46:14 +1000 Subject: [PATCH 2/2] More robust method to find repository from URLs When downloading arbitrary URLs using F-Droid (e.g. icons, .apk files, indexes) then it may be the case that the repo requires authentication. As such, we try to infer the repository based purely on the URL. The old code took the basename of the URL, which means remove the last fragment (e.g. "index.jar") and use the remaining portion of the URL to lookup the repo. This is broken for many reasons, partly because of the presence of a query string, partly because there are other things which are not just in the root directory of the repo (e.g. "/icons/*.png"). This new method iteratively peels off the right most segment of the URLs path, then looks to see if a repo exists at that address. Note that this breaks down if you have nested repositories on a server, where one of the repositories is nested inside a directory that F-Droid knows about, such as "icons". In such a case, the following repositories: * https://f-droid.org/repo (requires auth) * https://f-droid.org/repo/icons (doesn't require auth) will break down. If requesting something from the repo requiring auth: * https://f-droid.org/repo/icons/org.fdroid.fdroid.png Then it will lookup the database and find the repo which lives in "/icons" and doesn't require auth (or requires a different auth username/password). Not sure there is a lot that can be done about this without major refactoring. Such refactoring would require making sure a `Repo` is always given to a downloader for any HTTP request, and is probably a bit out of scope of this bug. Also added tests for this behaviour. Fixes #711. --- .../org/fdroid/fdroid/data/RepoProvider.java | 44 +++++++++++++++- .../fdroid/fdroid/localrepo/SwapService.java | 2 +- .../fdroid/fdroid/net/DownloaderFactory.java | 6 +-- .../fdroid/fdroid/data/RepoProviderTest.java | 51 +++++++++++++++++++ 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java b/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java index 93e721e2d..caa4f8c39 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java +++ b/app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java @@ -7,6 +7,7 @@ import android.content.Context; import android.content.UriMatcher; import android.database.Cursor; import android.net.Uri; +import android.support.annotation.Nullable; import android.text.TextUtils; import android.util.Log; @@ -27,7 +28,10 @@ public class RepoProvider extends FDroidProvider { private Helper() { } - public static Repo findByUri(Context context, Uri uri) { + /** + * Find by the content URI of a repo ({@link RepoProvider#getContentUri(long)}). + */ + public static Repo get(Context context, Uri uri) { ContentResolver resolver = context.getContentResolver(); Cursor cursor = resolver.query(uri, Cols.ALL, null, null, null); return cursorToRepo(cursor); @@ -45,6 +49,44 @@ public class RepoProvider extends FDroidProvider { return cursorToRepo(cursor); } + /** + * This method decides what repo a URL belongs to by iteratively removing path fragments and + * checking if it belongs to a repo or not. It will match the most specific repository which + * could serve the file at the given URL. + * + * For any given HTTP resource requested by F-Droid, it should belong to a repository. + * Whether that resource is an index.jar, an icon, or a .apk file, they all belong to a + * repository. Therefore, that repository must exist in the database. The way to find out + * which repository a particular URL came from requires some consideration: + * * Repositories can exist at particular paths on a server (e.g. /fdroid/repo) + * * Individual files can exist at a more specific path on the repo (e.g. /fdroid/repo/icons/org.fdroid.fdroid.png) + * + * So for a given URL "/fdroid/repo/icons/org.fdroid.fdroid.png" we don't actually know + * whether it is for the file "org.fdroid.fdroid.png" at repository "/fdroid/repo/icons" or + * the file "icons/org.fdroid.fdroid.png" at the repository at "/fdroid/repo". + */ + @Nullable + public static Repo findByUrl(Context context, Uri uri, String[] projection) { + Uri withoutQuery = uri.buildUpon().query(null).build(); + Repo repo = findByAddress(context, withoutQuery.toString(), projection); + + // Take a copy of this, because the result of getPathSegments() is an AbstractList + // which doesn't support the remove() operation. + List pathSegments = new ArrayList<>(withoutQuery.getPathSegments()); + + boolean haveTriedWithoutPath = false; + while (repo == null && !haveTriedWithoutPath) { + if (pathSegments.size() == 0) { + haveTriedWithoutPath = true; + } else { + pathSegments.remove(pathSegments.size() - 1); + withoutQuery = withoutQuery.buildUpon().path(TextUtils.join("/", pathSegments)).build(); + } + repo = findByAddress(context, withoutQuery.toString(), projection); + } + return repo; + } + public static Repo findByAddress(Context context, String address) { return findByAddress(context, address, Cols.ALL); } diff --git a/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java b/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java index f89ea3479..19f8d9161 100644 --- a/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java +++ b/app/src/main/java/org/fdroid/fdroid/localrepo/SwapService.java @@ -280,7 +280,7 @@ public class SwapService extends Service { values.put(Schema.RepoTable.Cols.IN_USE, true); values.put(Schema.RepoTable.Cols.IS_SWAP, true); Uri uri = RepoProvider.Helper.insert(this, values); - repo = RepoProvider.Helper.findByUri(this, uri); + repo = RepoProvider.Helper.get(this, uri); } return repo; 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 65e35e6b2..9c908e7a7 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,6 @@ import android.content.Context; import android.net.Uri; import android.support.v4.content.LocalBroadcastManager; -import org.apache.commons.io.FilenameUtils; import org.fdroid.fdroid.data.Repo; import org.fdroid.fdroid.data.RepoProvider; import org.fdroid.fdroid.data.Schema; @@ -37,7 +36,7 @@ public class DownloaderFactory { public static Downloader create(Context context, String urlString, File destFile) throws IOException { URL url = new URL(urlString); - Downloader downloader = null; + Downloader downloader; if (localBroadcastManager == null) { localBroadcastManager = LocalBroadcastManager.getInstance(context); } @@ -49,8 +48,7 @@ public class DownloaderFactory { downloader = new LocalFileDownloader(url, destFile); } else { final String[] projection = {Schema.RepoTable.Cols.USERNAME, Schema.RepoTable.Cols.PASSWORD}; - String repoUrlString = FilenameUtils.getBaseName(url.toString()); - Repo repo = RepoProvider.Helper.findByAddress(context, repoUrlString, projection); + Repo repo = RepoProvider.Helper.findByUrl(context, Uri.parse(url.toString()), projection); if (repo == null) { downloader = new HttpDownloader(url, destFile); } else { diff --git a/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java index 114cffca0..88f3409b9 100644 --- a/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java +++ b/app/src/test/java/org/fdroid/fdroid/data/RepoProviderTest.java @@ -2,6 +2,7 @@ package org.fdroid.fdroid.data; import android.app.Application; import android.content.ContentValues; +import android.net.Uri; import android.support.annotation.Nullable; import org.fdroid.fdroid.BuildConfig; @@ -16,11 +17,61 @@ import org.robolectric.annotation.Config; import java.util.List; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; @Config(constants = BuildConfig.class, application = Application.class) @RunWith(RobolectricGradleTestRunner.class) public class RepoProviderTest extends FDroidProviderTest { + private static final String[] COLS = RepoTable.Cols.ALL; + + @Test + public void findByUrl() { + + Repo fdroidRepo = RepoProvider.Helper.findByAddress(context, "https://f-droid.org/repo"); + Repo fdroidArchiveRepo = RepoProvider.Helper.findByAddress(context, "https://f-droid.org/archive"); + + String[] noRepos = { + "https://not-a-repo.example.com", + "https://f-droid.org", + "https://f-droid.org/", + }; + + for (String url : noRepos) { + assertNull(RepoProvider.Helper.findByUrl(context, Uri.parse(url), COLS)); + } + + String[] fdroidRepoUrls = { + "https://f-droid.org/repo/index.jar", + "https://f-droid.org/repo/index.jar?random-junk-in-query=yes", + "https://f-droid.org/repo/index.jar?random-junk-in-query=yes&more-junk", + "https://f-droid.org/repo/icons/org.fdroid.fdroid.100.png", + "https://f-droid.org/repo/icons-640/org.fdroid.fdroid.100.png", + }; + + assertUrlsBelongToRepo(fdroidRepoUrls, fdroidRepo); + + String[] fdroidArchiveUrls = { + "https://f-droid.org/archive/index.jar", + "https://f-droid.org/archive/index.jar?random-junk-in-query=yes", + "https://f-droid.org/archive/index.jar?random-junk-in-query=yes&more-junk", + "https://f-droid.org/archive/icons/org.fdroid.fdroid.100.png", + "https://f-droid.org/archive/icons-640/org.fdroid.fdroid.100.png", + }; + + assertUrlsBelongToRepo(fdroidArchiveUrls, fdroidArchiveRepo); + } + + private void assertUrlsBelongToRepo(String[] urls, Repo expectedRepo) { + for (String url : urls) { + Repo actualRepo = RepoProvider.Helper.findByUrl(context, Uri.parse(url), COLS); + assertNotNull("No repo matching URL " + url, actualRepo); + assertEquals("Invalid repo for URL [" + url + "]. Expected [" + expectedRepo.address + "] but got [" + actualRepo.address + "]", expectedRepo.id, actualRepo.id); + } + + } + /** * The {@link DBHelper} class populates four default repos when it first creates a database: * * F-Droid