Karl/BobStore
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

do not let Jackson set instance vars that never come from index

Browse Source 
Tell the Jackson JSON parser to ignore App/Apk fields that should never
come from the index, but instead are set locally to keep track of the
current state of things on the device.

There are two forms of tests to enforce that the proper things get
ignored.  It is not possible to do this with decorators alone, so I
chose to use @JsonIgnore and leave the variables we want filled in
undecorated.  Also, all of the instance variables in Apk/App/Repo
should come directly from the index metadata so that they are pure
data classes.  Currently some state info is stored in them, those are
decorated with @JsonIgnore.

The tests then include lists of accepted and ignored properties, and
anything that is not in those lists will cause the tests to fail.  So
if someone is adding a new instance variable, they will get a fail
until the tests are updated.  One set of tests actually writes blank
instances out as JSON since that's the easiest test to write, and
Jackson treats @JsonIgnore the same in both directions.  Then there is
another test that reads a JSON file with added, unsupported values to
make sure that they are properly ignored.
This commit is contained in:
Hans-Christoph Steiner 2017-03-22 16:14:43 +01:00 committed by Peter Serwylo
parent 281dd435e7
commit 40643855c4
6 changed files with 360 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
app/src/main/java/org/fdroid/fdroid/data/Apk.java
View File

@ -8,7 +8,7 @@ import android.os.Build;
import android.os.Parcel;
import android.os.Parcelable;
import android.support.annotation.NonNull;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.fdroid.fdroid.BuildConfig;
import org.fdroid.fdroid.RepoXMLHandler;
@ -27,24 +27,41 @@ import java.util.HashSet;
* <b>Do not rename these instance variables without careful consideration!</b>
* They are mapped to JSON field names, the {@code fdroidserver} internal variable
* names, and the {@code fdroiddata} YAML field names. Only the instance variables
* listed in {@code @JsonIgnoreProperties} are not directly mapped.
* decorated with {@code @JsonIgnore} are not directly mapped.
* <p>
* <b>NOTE:</b>If an instance variable is only meant for internal state, and not for
* representing data coming from the server, then it must also be decorated with
* {@code @JsonIgnore} to prevent abuse! The tests for
* {@link org.fdroid.fdroid.IndexV1Updater} will also have to be updated.
*
* @see <a href="https://gitlab.com/fdroid/fdroiddata">fdroiddata</a>
* @see <a href="https://gitlab.com/fdroid/fdroidserver">fdroidserver</a>
*/
@JsonIgnoreProperties({"compatible", "CREATOR", "installedFile", "repo", "repoAddress",
"repoVersion",})
public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
// Using only byte-range keeps it only 8-bits in the SQLite database
@JsonIgnore
public static final int SDK_VERSION_MAX_VALUE = Byte.MAX_VALUE;
@JsonIgnore
public static final int SDK_VERSION_MIN_VALUE = 0;
// these are never set by the Apk/package index metadata
@JsonIgnore
public long repo; // ID of the repo it comes from
@JsonIgnore
String repoAddress;
@JsonIgnore
int repoVersion;
@JsonIgnore
public SanitizedFile installedFile; // the .apk file on this device's filesystem
@JsonIgnore
public boolean compatible; // True if compatible with the device.
// these come directly from the index metadata
public String packageName;
public String versionName;
public int versionCode;
public int size; // Size in bytes - 0 means we don't know!
public long repo; // ID of the repo it comes from
public String hash; // checksum of the APK, in lowercase hex
public String hashType;
public int minSdkVersion = SDK_VERSION_MIN_VALUE; // 0 if unknown
@ -72,13 +89,7 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
*/
public String sig;
/**
* True if compatible with the device.
*/
public boolean compatible;
public String apkName; // F-Droid style APK name
public SanitizedFile installedFile; // the .apk file on this device's filesystem
/**
* If not null, this is the name of the source tarball for the
@ -87,8 +98,6 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
*/
public String srcname;
public int repoVersion;
public String repoAddress;
public String[] incompatibleReasons;
/**
@ -114,11 +123,13 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
* Note: Many of the fields on this instance will not be known in this circumstance. Currently
* the only things that are known are:
* <p>
* + {@link Apk#packageName}
* + {@link Apk#versionName}
* + {@link Apk#versionCode}
* + {@link Apk#hash}
* + {@link Apk#hashType}
* <ul>
* <li>{@link Apk#packageName}
* <li>{@link Apk#versionName}
* <li>{@link Apk#versionCode}
* <li>{@link Apk#hash}
* <li>{@link Apk#hashType}
* </ul>
* <p>
* This could instead be implemented by accepting a {@link PackageInfo} and it would get much
* the same information, but it wouldn't have the hash of the package. Seeing as we've already
@ -248,6 +259,7 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
}
}
@JsonIgnore // prevent tests from failing due to nulls in checkRepoAddress()
public String getUrl() {
checkRepoAddress();
return repoAddress + "/" + apkName.replace(" ", "%20");

49
app/src/main/java/org/fdroid/fdroid/data/App.java
View File

@ -16,7 +16,7 @@ import android.support.annotation.Nullable;
import android.text.TextUtils;
import android.util.Log;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.commons.io.filefilter.RegexFileFilter;
@ -56,25 +56,27 @@ import java.util.regex.Pattern;
* <b>Do not rename these instance variables without careful consideration!</b>
* They are mapped to JSON field names, the {@code fdroidserver} internal variable
* names, and the {@code fdroiddata} YAML field names. Only the instance variables
* listed in {@code @JsonIgnoreProperties} are not directly mapped.
* decorated with {@code @JsonIgnore} are not directly mapped.
* <p>
* <b>NOTE:</b>If an instance variable is only meant for internal state, and not for
* representing data coming from the server, then it must also be decorated with
* {@code @JsonIgnore} to prevent abuse! The tests for
* {@link org.fdroid.fdroid.IndexV1Updater} will also have to be updated.
*
* @see <a href="https://gitlab.com/fdroid/fdroiddata">fdroiddata</a>
* @see <a href="https://gitlab.com/fdroid/fdroidserver">fdroidserver</a>
*/
@JsonIgnoreProperties({"compatible", "CREATOR", "id", "installedApk", "installedSig",
"installedVersionCode", "installedVersionName", "prefs", "repoId", })
public class App extends ValueObject implements Comparable<App>, Parcelable {
@JsonIgnore
private static final String TAG = "App";
// these properties are not from the index metadata, but represent the state on the device
/**
* True if compatible with the device (i.e. if at least one apk is)
*/
@JsonIgnore
public boolean compatible;
public String packageName = "unknown";
public String name = "Unknown";
/**
* This is primarily for the purpose of saving app metadata when parsing an index.xml file.
* At most other times, we don't particularly care which repo an {@link App} object came from.
@ -82,7 +84,25 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
* the highest priority. The UI doesn't care normally _which_ repo provided the metadata.
* This is required for getting the full URL to the various graphics and screenshots.
*/
@JsonIgnore
public long repoId;
@JsonIgnore
public Apk installedApk; // might be null if not installed
@JsonIgnore
public String installedSig;
@JsonIgnore
public int installedVersionCode;
@JsonIgnore
public String installedVersionName;
@JsonIgnore
private long id;
@JsonIgnore
private AppPrefs prefs;
// the remaining properties are set directly from the index metadata
public String packageName = "unknown";
public String name = "Unknown";
public String summary = "Unknown application";
public String icon;
@ -155,8 +175,6 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
@Deprecated
public String[] requirements;
private AppPrefs prefs;
/**
* To be displayed at 48dp (x1.0)
*/
@ -167,16 +185,6 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
*/
public String iconUrlLarge;
public String installedVersionName;
public int installedVersionCode;
public Apk installedApk; // might be null if not installed
public String installedSig;
private long id;
public static String getIconName(String packageName, int versionCode) {
return packageName + "_" + versionCode + ".png";
}
@ -957,6 +965,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
this.id = in.readLong();
}
@JsonIgnore
public static final Parcelable.Creator<App> CREATOR = new Parcelable.Creator<App>() {
@Override
public App createFromParcel(Parcel source) {

5
app/src/main/java/org/fdroid/fdroid/data/Repo.java
View File

@ -38,6 +38,11 @@ import java.util.Date;
/**
* Represents a the descriptive info and metadata about a given repo, as provided
* by the repo index. This also keeps track of the state of the repo.
* <p>
* <b>Do not rename these instance variables without careful consideration!</b>
* They are mapped to JSON field names, the {@code fdroidserver} internal variable
* names, and the {@code fdroiddata} YAML field names. Only the instance variables
* decorated with {@code @JsonIgnore} are not directly mapped.
*
* @see <a href="https://gitlab.com/fdroid/fdroiddata">fdroiddata</a>
* @see <a href="https://gitlab.com/fdroid/fdroidserver">fdroidserver</a>

225
app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java
View File

@ -2,47 +2,35 @@ package org.fdroid.fdroid.updater;
import android.support.annotation.NonNull;
import android.util.Log;
import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectReader;
import org.apache.commons.io.IOUtils;
import org.fdroid.fdroid.BuildConfig;
import org.fdroid.fdroid.IndexV1Updater;
import org.fdroid.fdroid.Preferences;
import org.fdroid.fdroid.RepoUpdater;
import org.fdroid.fdroid.TestUtils;
import org.fdroid.fdroid.data.Apk;
import org.fdroid.fdroid.data.App;
import org.fdroid.fdroid.data.FDroidProviderTest;
import org.fdroid.fdroid.data.Repo;
import org.fdroid.fdroid.data.RepoPushRequest;
import org.fdroid.fdroid.*;
import org.fdroid.fdroid.data.*;
import org.fdroid.fdroid.mock.RepoDetails;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricGradleTestRunner;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.annotation.Config;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.lang.reflect.Field;
import java.util.*;
import java.util.jar.JarEntry;
import java.util.jar.JarFile;
import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.core.IsNot.not;
import static org.junit.Assert.*;
// TODO: Use sdk=24 when Robolectric supports this
@Config(constants = BuildConfig.class, sdk = 23)
@RunWith(RobolectricGradleTestRunner.class)
@Config(constants = BuildConfig.class, sdk = 24)
@RunWith(RobolectricTestRunner.class)
public class IndexV1UpdaterTest extends FDroidProviderTest {
public static final String TAG = "IndexV1UpdaterTest";
@ -169,6 +157,197 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
assertArrayEquals(installRequests.toArray(), requests.get("install"));
}
/**
* Test that all the fields are properly marked as whether Jackson should ignore them
* or not. Technically this test goes the opposite direction of how its being used:
* it writes out {@link App} and {@link Apk} instances to JSON using Jackson. With the
* index parsing, Jackson is always reading JSON into {@link App} and {@link Apk}
* instances. {@code @JsonIgnoreProperties} applies to both directions.
* <p>
* The test sets come from the top of {@link App} and {@link Apk}.
*/
@Test
public void testJsonIgnoreApp() throws JsonProcessingException {
String[] allowedInApp = new String[]{
"added",
"antiFeatures",
"authorEmail",
"authorName",
"bitcoin",
"categories",
"changelog",
"description",
"donate",
"featureGraphic",
"flattrID",
"icon",
"iconUrl",
"iconUrlLarge",
"issueTracker",
"lastUpdated",
"license",
"litecoin",
"name",
"packageName",
"phoneScreenshots",
"promoGraphic",
"requirements",
"sevenInchScreenshots",
"sourceCode",
"suggestedVersionCode",
"suggestedVersionName",
"summary",
"tenInchScreenshots",
"tvBanner",
"tvScreenshots",
"upstreamVersionCode",
"upstreamVersionName",
"video",
"wearScreenshots",
"webSite",
};
String[] ignoredInApp = new String[]{
"compatible",
"CREATOR",
"id",
"installedApk",
"installedSig",
"installedVersionCode",
"installedVersionName",
"prefs",
"repoId",
"TAG",
};
runJsonIgnoreTest(new App(), allowedInApp, ignoredInApp);
}
@Test
public void testJsonIgnoreApk() throws JsonProcessingException {
String[] allowedInApk = new String[]{
"added",
"antiFeatures",
"apkName",
"appId",
"features",
"hash",
"hashType",
"incompatibleReasons",
"maxSdkVersion",
"minSdkVersion",
"nativecode",
"obbMainFile",
"obbMainFileSha256",
"obbPatchFile",
"obbPatchFileSha256",
"packageName",
"requestedPermissions",
"sig",
"size",
"srcname",
"targetSdkVersion",
"versionCode",
"versionName",
};
String[] ignoredInApk = new String[]{
"compatible",
"CREATOR",
"installedFile",
"repo",
"repoAddress",
"repoVersion",
"SDK_VERSION_MAX_VALUE",
"SDK_VERSION_MIN_VALUE",
"url",
};
runJsonIgnoreTest(new Apk(), allowedInApk, ignoredInApk);
}
private void runJsonIgnoreTest(Object instance, String[] allowed, String[] ignored)
throws JsonProcessingException {
ObjectMapper mapper = new ObjectMapper();
String objectAsString = mapper.writeValueAsString(instance);
Set<String> fields = getFields(instance);
for (String field : ignored) {
assertThat(objectAsString, not(containsString("\"" + field + "\"")));
fields.remove(field);
}
for (String field : allowed) {
fields.remove(field);
}
if (fields.size() > 0) {
System.out.print(instance.getClass() + " has fields not setup for Jackson: ");
for (String field : fields) {
System.out.print("\"" + field + "\", ");
}
System.out.println("\nRead class javadoc for more info.");
}
assertEquals(0, fields.size());
}
@Test
public void testInstanceVariablesAreProperlyMarked() throws IOException {
ObjectMapper mapper = new ObjectMapper();
// testing with unknown metadata in it
mapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
JsonFactory f = mapper.getFactory();
JsonParser parser = f.createParser(TestUtils.copyResourceToTempFile("all_fields_index-v1.json"));
Repo repo = null;
App[] apps = null;
Map<String, List<Apk>> packages = null;
parser.nextToken(); // go into the main object block
while (true) {
String fieldName = parser.nextFieldName();
if (fieldName == null) {
break;
}
switch (fieldName) {
case "repo":
repo = parseRepo(mapper, parser);
break;
case "apps":
apps = parseApps(mapper, parser);
break;
case "packages":
packages = parsePackages(mapper, parser);
break;
}
}
parser.close(); // ensure resources get cleaned up timely and properly
assertEquals(1, apps.length);
assertEquals(1, packages.size());
assertEquals(1488828510109L, repo.timestamp);
assertEquals("GPLv3", apps[0].license);
Set<String> appFields = getFields(apps[0]);
for (String field : appFields) {
assertNotEquals("secret", field);
}
Apk apk = packages.get("info.guardianproject.cacert").get(0);
assertEquals("e013db095e8da843fae5ac44be6152e51377ee717e5c8a7b6d913d7720566b5a", apk.hash);
Set<String> packageFields = getFields(apk);
for (String field : packageFields) {
assertNotEquals("secret", field);
}
}
private Set<String> getFields(Object instance) {
SortedSet<String> output = new TreeSet<>();
//determine fields declared in this class only (no fields of superclass)
Field[] fields = instance.getClass().getDeclaredFields();
//print field names paired with their values
for (Field field : fields) {
output.add(field.getName());
}
return output;
}
private Repo parseRepo(ObjectMapper mapper, JsonParser parser) throws IOException {
System.out.println("parseRepo ");
parser.nextToken();

93
app/src/test/resources/all_fields_index-v1.json Normal file
View File

@ -0,0 +1,93 @@
{
"repo": {
"timestamp": 1488828510109,
"version": 18,
"name": "Guardian Project Official Releases",
"icon": "guardianproject.png",
"address": "https://guardianproject.info/fdroid/repo",
"description": "The official app repository of The Guardian Project. Applications in this repository are official binaries build by the original application developers and signed by the same key as the APKs that are released in the Google Play store. ",
"secret": "trying to sneak something in",
"mirrors": [
"http://bdf2wcxujkg6qqff.onion/fdroid/repo",
"https://guardianproject.info/fdroid/repo",
"https://s3.amazonaws.com/guardianproject/fdroid/repo"
]
},
"apps": [
{
"categories": [
"Security",
"GuardianProject"
],
"suggestedVersionCode": "999999999",
"description": "<p>Android 4+ allows you to disable certificates from the system Settings and root isn't required, so try that first if you want to manually mess with the certificates. The app won't work with Android 4+ anyway.</p><p>An app to manage security certificates on your phone also containing a version of the Android CACert keystore derived from Mozilla. If a certificate has recently become untrusted you can either install an update to this app or you can backup and remove certificates by yourself.</p><p>Requires root: Yes, it writes to the system partition. You will need a device that has the \u2018grep\u2019 command on it (via busybox: present on most custom ROMs). If the \u2018save\u2019 doesn\u2019t work, then you will need to make your /system partition read-write by using a file explorer like <a href=\"fdroid.app:com.ghostsq.commander\">Ghost Commander</a> or via a command in <a href=\"fdroid.app:jackpal.androidterm\">Terminal Emulator</a>.</p>",
"issueTracker": "https://github.com/guardianproject/cacert/issues",
"license": "GPLv3",
"name": "CACertMan",
"secret": "trying to sneak something in",
"sourceCode": "https://github.com/guardianproject/cacert",
"summary": "Disable untrusted certificates",
"webSite": "https://guardianproject.info/2011/09/05/cacertman-app-to-address-diginotar-other-bad-cas",
"added": 1376863200000,
"icon": "info.guardianproject.cacert.4.png",
"packageName": "info.guardianproject.cacert",
"lastUpdated": 1383174000000
}
],
"packages": {
"info.guardianproject.cacert": [
{
"added": 1400018400000,
"apkName": "Courier-0.1.8.apk",
"hash": "e013db095e8da843fae5ac44be6152e51377ee717e5c8a7b6d913d7720566b5a",
"hashType": "sha256",
"minSdkVersion": "9",
"nativecode": [
"armeabi",
"x86"
],
"packageName": "info.guardianproject.courier",
"secret": "trying to sneak something in",
"sig": "d70ac6a02b53ebdd1354ea7af7b9ceee",
"size": 16536125,
"targetSdkVersion": "15",
"uses-permission": [
[
"android.permission.READ_EXTERNAL_STORAGE",
null
],
[
"android.permission.INTERNET",
null
],
[
"android.permission.BLUETOOTH",
null
],
[
"android.permission.BLUETOOTH_ADMIN",
null
],
[
"android.permission.VIBRATE",
null
],
[
"android.permission.ACCESS_NETWORK_STATE",
null
],
[
"android.permission.WRITE_EXTERNAL_STORAGE",
null
],
[
"android.permission.ACCESS_WIFI_STATE",
null
]
],
"versionCode": 14,
"versionName": "0.1.8"
}
]
}
}

2
config/checkstyle/checkstyle.xml
View File

@ -35,7 +35,7 @@
<module name="AvoidStarImport" />
<module name="AvoidStaticImport">
<property name="excludes"
value="org.fdroid.fdroid.Assert.*, org.assertj.core.api.Assertions.*, org.junit.Assert.*, org.junit.Assume.*, org.junit.internal.matchers.ThrowableMessageMatcher.*, org.hamcrest.CoreMatchers.*, org.hamcrest.Matchers.*, org.springframework.boot.configurationprocessor.ConfigurationMetadataMatchers.*, org.springframework.boot.configurationprocessor.TestCompiler.*, org.mockito.Mockito.*, org.mockito.BDDMockito.*, org.mockito.Matchers.*, org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*, org.springframework.test.web.servlet.result.MockMvcResultMatchers.*, org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.*, org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers.*, org.springframework.hateoas.mvc.ControllerLinkBuilder.linkTo" />
value="org.fdroid.fdroid.Assert.*, org.assertj.core.api.Assertions.*, org.junit.Assert.*, org.junit.Assume.*, org.junit.internal.matchers.ThrowableMessageMatcher.*, org.hamcrest.core.IsNot.*, org.hamcrest.CoreMatchers.*, org.hamcrest.Matchers.*, org.springframework.boot.configurationprocessor.ConfigurationMetadataMatchers.*, org.springframework.boot.configurationprocessor.TestCompiler.*, org.mockito.Mockito.*, org.mockito.BDDMockito.*, org.mockito.Matchers.*, org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*, org.springframework.test.web.servlet.result.MockMvcResultMatchers.*, org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.*, org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers.*, org.springframework.hateoas.mvc.ControllerLinkBuilder.linkTo" />
</module>
<module name="RedundantImport" />
<module name="UnusedImports" />