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.
This is important for the ability to refactor the database for better performance in the future.
See #511 for details.
For those interested in the details, here is a query plan of selecting the "All" category of apps before
this commit:
* `SCAN TABLE fdroid_app USING INDEX app_id`
* `SEARCH TABLE fdroid_apk USING INDEX apk_id (id=?)`
* `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)`
* `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)`
* `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX sqlite_autoindex_fdroid_apk_1 (id=? AND vercode=?)`
* `USE TEMP B-TREE FOR ORDER BY`
And here is a query plan of afterwards:
* `SCAN TABLE fdroid_app`
* `SEARCH TABLE fdroid_apk USING INDEX apk_appId (appId=?)`
* `SEARCH TABLE fdroid_repo USING INTEGER PRIMARY KEY (rowid=?)`
* `SEARCH TABLE fdroid_installedApp AS installed USING INDEX sqlite_autoindex_fdroid_installedApp_1 (appId=?)`
* `SEARCH TABLE fdroid_apk AS suggestedApk USING INDEX apk_appId (appId=?)`
* `USE TEMP B-TREE FOR ORDER BY`
The things of note are:
* `SCAN TABLE` doesn't use an index, which means that it is really using the rowid index. Shouldn't behave much differently.
* The second item now uses an integer primary key index rather than a package name index. Should increase search speed marginally which was the goal of this commit. As more apks exist, the speed improvement will also increase.
Provide content Uris to downloaded apks via FileProvider
* moves apk verification back inside the Installer class
* uses support libs FileProvider for content Uris
* move apk file caching and storage methods into ApkFileProvider class
Some of the ugly version checks for Android N can be removed after Android N has been released. Unfortunately Google decided to keep SDK version at 23 for Android N dev preview and only change the CODENAME, thus ``Build.VERSION.SDK_INT <= Build.VERSION_CODES.M`` returns true on Android N preview :/ , see https://commonsware.com/blog/2016/03/17/backwards-compatibility-n-developer-preview.html
Tested on Android N dev preview 3 emulator, Android 6 stock and Android 5.1 rooted with priv extension.
See merge request !331
The two helper methods alleviate the need for copious null checks. They also provide
consistent behaviour when there are zero elements (i.e. they return null, rather than
an empty string or empty array, as was the case before).
This is a combination of:
* `String[].split(",")` and
* `TextUtils.join(",", values)`
It seems a bit wastefull to have our own implementation of these two things
which lightly wrap this code, and produce a datastructure which is non standard
and foreign to Java developers.
* moves apk verification back inside the Installer class
* uses support libs FileProvider for content Uris
* move apk file caching and storage methods into
ApkCache class
A previous commit accidentally pushed the code which queries the
`PackageManager` to a different method, but then still used the
`packageInfo` which was supposed to be populated by that code.
This change rectifies this, and in the process also clarifies/documents under
what circumstances the `PackageManager` needs to be queried, rather than
relying on the incoming intent.
Fixes#686.
Check permissions for unattended installer
This PR introduces the class ``ApkVerifier`` which checks the permissions of the downloaded apk file against the expected permissions from the F-Droid listing (``Apk`` class).
* I removed ``AndroidXMLDecompress`` because everything which it has been used for can also be done with ``PackageManager.getPackageArchiveInfo()``, to the best of my knowledge. I even asked in at a similar project why ``PackageManager.getPackageArchiveInfo()``may not be enough: https://github.com/jaredrummler/APKParser/issues/3 It turns out in our case it should do everything we need.
* The code responsible for sanitizing the local apk file and making it world readable has also been moved into ``ApkVerifier`` for now. This can change in a later PR when I introduce the FileProvider for downloaded apks.
We still need to check the target sdk version (see TODO in ``ApkVerifier``). This depends on https://gitlab.com/fdroid/fdroidclient/merge_requests/323
See merge request !322
Like PMD, we also had to add a concession to allow static imports.
This time, it was achieved by moving the assertions to a more generally
named `Assert` class, and then allowing static imports from that.
To appease PMD, we now have a three rulesets in `config/pmd/*.xml`:
* `rules.xml`: The bulk of the rules, used by both main and test code.
* `rules-main.xml`: Rules specific to the andoid client code.
* `rules-test.xml`: Rules specific to test code.
The rationale is because checkstyle by default checks for "too many static
imports", which is a fair call. However in JUnit4 code, it is common to
import many `assert*` static methods.
The tests pass, but there is a lingering message that gets logged:
```
Jun 08, 2016 7:31:13 AM com.almworks.sqlite4java.Internal log
WARNING: [sqlite] [DETACH DATABASE temp_update_db]DB[1][C]: exception when clearing
com.almworks.sqlite4java.SQLiteException: [1] DB[1] reset [no such database: temp_update_db]
at com.almworks.sqlite4java.SQLiteConnection.throwResult(SQLiteConnection.java:1309)
at com.almworks.sqlite4java.SQLiteConnection.throwResult(SQLiteConnection.java:1282)
at com.almworks.sqlite4java.SQLiteConnection.cacheStatementHandle(SQLiteConnection.java:1211)
at com.almworks.sqlite4java.SQLiteConnection.access$900(SQLiteConnection.java:54)
at com.almworks.sqlite4java.SQLiteConnection$CachedController.dispose(SQLiteConnection.java:1606)
at com.almworks.sqlite4java.SQLiteStatement.dispose(SQLiteStatement.java:187)
at org.robolectric.shadows.ShadowSQLiteConnection$Connections$4.call(ShadowSQLiteConnection.java:421)
at org.robolectric.shadows.ShadowSQLiteConnection$Connections$6.call(ShadowSQLiteConnection.java:449)
at org.robolectric.shadows.ShadowSQLiteConnection$Connections$6.call(ShadowSQLiteConnection.java:443)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
```
The `temp_update_db` is the one used for repo updates, but I thought that it
correctly gets dropped/detached by the `TempAppProvider` when required. In fact,
given the nature of the error message (no such database: temp_update_db), that
hints at the fact that it is indeed dropped. I'm struggling to figure out what
causes this, but it should not be harmful to the running of the tests. If a test
actually fails, then it is picked up correctly by JUnit.
Get around silly `final` methods in `ContentResolver` with Mockito and `delegatesTo`.
The Robolectric library presumes that people always want to test content providers by
manually invoking the `query`/`update`/`delete` methods on the `ShadowContentResolver`.
While that is a great feature for testing, we have helper methods that require testing,
and these methods accept either a _real_ `ContentResolver` or `Context`. Robolectric
did some cool magic in terms of intercepting runtime calls to content resolvers and
forwarding them to the "shadow" verison, to deal with final/package private/etc methods.
However, as a side effect, the `ShadowContentProvider` _is not a `ContentProvider` as
far as the Java compiler is concerned.
By utilising Mockito + `delegatesTo` method, we are able to achieve what is required:
* An actual `ContentProvider` instance.
* It forwards calls to the `ShadowContentProvider` provided by Robolectric.
Robolectric provides testing support for Android via the JVM, including testing
of content providers. In order to get these tests to work, we need to avoid
the default behaviour of starting up FDroidApp.onCreate(). This method has a lot
of static state which fails if set multiple times. Instead of trying to ensure
we correctly zero out that state each test, it is preferable to instead never
bother with that in the first place. Expecially when that is not what is under
test (as is the case with content provider tests).
Test downloads with actual files, not dynamically generated things.
Testing with the progress reports is really hard with multiple URLs, so
just test progress with a single URL for now, and multiple URLs can still
be tested without the progress check.
fixes#650https://gitlab.com/fdroid/fdroidclient/issues/650
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().
