diff --git a/app/src/main/java/org/fdroid/fdroid/data/Apk.java b/app/src/main/java/org/fdroid/fdroid/data/Apk.java index 3ba7d9359..00fe9711d 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Apk.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Apk.java @@ -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; * Do not rename these instance variables without careful consideration! * 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. + *

+ * NOTE: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 fdroiddata * @see fdroidserver */ -@JsonIgnoreProperties({"compatible", "CREATOR", "installedFile", "repo", "repoAddress", - "repoVersion",}) public class Apk extends ValueObject implements Comparable, 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, 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, Parcelable { */ public String srcname; - public int repoVersion; - public String repoAddress; public String[] incompatibleReasons; /** @@ -114,11 +123,13 @@ public class Apk extends ValueObject implements Comparable, Parcelable { * Note: Many of the fields on this instance will not be known in this circumstance. Currently * the only things that are known are: *

- * + {@link Apk#packageName} - * + {@link Apk#versionName} - * + {@link Apk#versionCode} - * + {@link Apk#hash} - * + {@link Apk#hashType} + *

*

* 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, Parcelable { } } + @JsonIgnore // prevent tests from failing due to nulls in checkRepoAddress() public String getUrl() { checkRepoAddress(); return repoAddress + "/" + apkName.replace(" ", "%20"); diff --git a/app/src/main/java/org/fdroid/fdroid/data/App.java b/app/src/main/java/org/fdroid/fdroid/data/App.java index cedeaf062..beae0e747 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/App.java +++ b/app/src/main/java/org/fdroid/fdroid/data/App.java @@ -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; * Do not rename these instance variables without careful consideration! * 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. + *

+ * NOTE: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 fdroiddata * @see fdroidserver */ -@JsonIgnoreProperties({"compatible", "CREATOR", "id", "installedApk", "installedSig", - "installedVersionCode", "installedVersionName", "prefs", "repoId", }) public class App extends ValueObject implements Comparable, 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, 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, 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, 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, Parcelable { this.id = in.readLong(); } + @JsonIgnore public static final Parcelable.Creator CREATOR = new Parcelable.Creator() { @Override public App createFromParcel(Parcel source) { 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 288a41388..e4dc96ebd 100644 --- a/app/src/main/java/org/fdroid/fdroid/data/Repo.java +++ b/app/src/main/java/org/fdroid/fdroid/data/Repo.java @@ -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. + *

+ * Do not rename these instance variables without careful consideration! + * 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 fdroiddata * @see fdroidserver diff --git a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java index 1731bf53b..9967a3113 100644 --- a/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java +++ b/app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java @@ -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. + *

+ * 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 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> 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 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 packageFields = getFields(apk); + for (String field : packageFields) { + assertNotEquals("secret", field); + } + } + + private Set getFields(Object instance) { + SortedSet 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(); diff --git a/app/src/test/resources/all_fields_index-v1.json b/app/src/test/resources/all_fields_index-v1.json new file mode 100644 index 000000000..9b31fe47e --- /dev/null +++ b/app/src/test/resources/all_fields_index-v1.json @@ -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": "

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.

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.

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 Ghost Commander or via a command in Terminal Emulator.

", + "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" + } + ] + } +} diff --git a/config/checkstyle/checkstyle.xml b/config/checkstyle/checkstyle.xml index a6650dd65..a18e6537c 100644 --- a/config/checkstyle/checkstyle.xml +++ b/config/checkstyle/checkstyle.xml @@ -35,7 +35,7 @@ + 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" />