Merge 'bugfix/repo_url_querystring' into 'master'

* commit 'a81140be4749189861b2961f84e2704eb5bb467b':
  run Android Studio default code formatter with Ctrl-Alt-L
  Add Repo.getFileUrl() method to get file URL in a standard way
  RepoUrlsTest: Add new tests for correct repo URL formatting

fdroid/fdroidclient!935

app/src/main/java/org/fdroid/fdroid/IndexUpdater.java

@@ -44,7 +44,6 @@ import org.fdroid.fdroid.installer.InstallManagerService;
 import org.fdroid.fdroid.installer.InstallerService;
 import org.fdroid.fdroid.net.Downloader;
 import org.fdroid.fdroid.net.DownloaderFactory;
-import org.fdroid.fdroid.net.TreeUriDownloader;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 import org.xml.sax.XMLReader;
@@ -116,11 +115,7 @@ public class IndexUpdater {
     }
 
     protected String getIndexUrl(@NonNull Repo repo) {
-        if (repo.address.startsWith("content://")) {
+        return repo.getFileUrl(SIGNED_FILE_NAME);
-            return repo.address + TreeUriDownloader.ESCAPED_SLASH + SIGNED_FILE_NAME;
-        } else {
-            return repo.address + "/" + SIGNED_FILE_NAME;
-        }
     }
 
     public boolean hasChanged() {

app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java

@@ -24,7 +24,7 @@ package org.fdroid.fdroid;
 
 import android.content.ContentValues;
 import android.content.Context;
-import android.net.Uri;
+
 import androidx.annotation.NonNull;
 import android.text.TextUtils;
 import android.util.Log;
@@ -97,15 +97,8 @@ public class IndexV1Updater extends IndexUpdater {
     }
 
     @Override
-    /**
-     * Storage Access Framework URLs have a crazy encoded path within the URL path.
-     */
     protected String getIndexUrl(@NonNull Repo repo) {
-        if (repo.address.startsWith("content://")) {
+        return repo.getFileUrl(SIGNED_FILE_NAME);
-            return repo.address + "%2F" + SIGNED_FILE_NAME;
-        } else {
-            return Uri.parse(repo.address).buildUpon().appendPath(SIGNED_FILE_NAME).build().toString();
-        }
     }
 
     /**

app/src/main/java/org/fdroid/fdroid/Utils.java

@@ -120,7 +120,7 @@ public final class Utils {
 
     private static Handler toastHandler;
 
-    public static final String FALLBACK_ICONS_DIR = "/icons/";
+    public static final String FALLBACK_ICONS_DIR = "icons";
 
     /*
      * @param dpiMultiplier Lets you grab icons for densities larger or
@@ -132,22 +132,22 @@ public final class Utils {
         final DisplayMetrics metrics = context.getResources().getDisplayMetrics();
         final double dpi = metrics.densityDpi * dpiMultiplier;
         if (dpi >= 640) {
-            return "/icons-640/";
+            return "icons-640";
         }
         if (dpi >= 480) {
-            return "/icons-480/";
+            return "icons-480";
         }
         if (dpi >= 320) {
-            return "/icons-320/";
+            return "icons-320";
         }
         if (dpi >= 240) {
-            return "/icons-240/";
+            return "icons-240";
         }
         if (dpi >= 160) {
-            return "/icons-160/";
+            return "icons-160";
         }
 
-        return "/icons-120/";
+        return "icons-120";
     }
 
     /**

app/src/main/java/org/fdroid/fdroid/data/Apk.java

@@ -58,7 +58,7 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
 
     // these are never set by the Apk/package index metadata
     @JsonIgnore
-    String repoAddress;
+    protected String repoAddress;
     @JsonIgnore
     int repoVersion;
     @JsonIgnore
@@ -280,7 +280,8 @@ public class Apk extends ValueObject implements Comparable<Apk>, Parcelable {
     @JsonIgnore  // prevent tests from failing due to nulls in checkRepoAddress()
     public String getCanonicalUrl() {
         checkRepoAddress();
-        return repoAddress + "/" + apkName.replace(" ", "%20");
+        Repo repo = new Repo(repoAddress);
+        return repo.getFileUrl(apkName);
     }
 
     /**

app/src/main/java/org/fdroid/fdroid/data/App.java

@@ -735,9 +735,9 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
             } else {
                 iconsDir = Utils.FALLBACK_ICONS_DIR;
             }
-            return repo.address + iconsDir + iconFromApk;
+            return repo.getFileUrl(iconsDir, iconFromApk);
         }
-        return repo.address + "/" + packageName + "/" + iconUrl;
+        return repo.getFileUrl(packageName, iconUrl);
     }
 
     public String getFeatureGraphicUrl(Context context) {
@@ -745,7 +745,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
             return null;
         }
         Repo repo = RepoProvider.Helper.findById(context, repoId);
-        return repo.address + "/" + packageName + "/" + featureGraphic;
+        return repo.getFileUrl(packageName, featureGraphic);
     }
 
     public String getPromoGraphic(Context context) {
@@ -753,7 +753,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
             return null;
         }
         Repo repo = RepoProvider.Helper.findById(context, repoId);
-        return repo.address + "/" + packageName + "/" + promoGraphic;
+        return repo.getFileUrl(packageName, promoGraphic);
     }
 
     public String getTvBanner(Context context) {
@@ -761,7 +761,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
             return null;
         }
         Repo repo = RepoProvider.Helper.findById(context, repoId);
-        return repo.address + "/" + packageName + "/" + tvBanner;
+        return repo.getFileUrl(packageName, tvBanner);
     }
 
     public String[] getAllScreenshots(Context context) {
@@ -785,7 +785,7 @@ public class App extends ValueObject implements Comparable<App>, Parcelable {
         String[] result = new String[list.size()];
         int i = 0;
         for (String url : list) {
-            result[i] = repo.address + "/" + packageName + "/" + url;
+            result[i] = repo.getFileUrl(packageName, url);
             i++;
         }
         return result;

app/src/main/java/org/fdroid/fdroid/data/Repo.java

@@ -25,12 +25,14 @@ package org.fdroid.fdroid.data;
 
 import android.content.ContentValues;
 import android.database.Cursor;
+import android.net.Uri;
 import android.text.TextUtils;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import org.fdroid.fdroid.FDroidApp;
 import org.fdroid.fdroid.Preferences;
 import org.fdroid.fdroid.Utils;
 import org.fdroid.fdroid.data.Schema.RepoTable.Cols;
+import org.fdroid.fdroid.net.TreeUriDownloader;
 
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -141,6 +143,10 @@ public class Repo extends ValueObject {
     public Repo() {
     }
 
+    public Repo(String address) {
+        this.address = address;
+    }
+
     public Repo(Cursor cursor) {
 
         checkCursorPosition(cursor);
@@ -263,6 +269,40 @@ public class Repo extends ValueObject {
         return tempName;
     }
 
+    public String getFileUrl(String... pathElements) {
+        /* Each String in pathElements might contain a /, should keep these as path elements */
+        List<String> elements = new ArrayList();
+        for (String element : pathElements) {
+            for (String elementPart : element.split("/")) {
+                elements.add(elementPart);
+            }
+        }
+
+        /**
+         * Storage Access Framework URLs have this wacky URL-encoded path within the URL path.
+         *
+         * i.e.
+         * content://authority/tree/313E-1F1C%3A/document/313E-1F1C%3Aguardianproject.info%2Ffdroid%2Frepo
+         *
+         * Currently don't know a better way to identify these than by content:// prefix,
+         * seems the Android SDK expects apps to consider them as opaque identifiers.
+         */
+        if (address.startsWith("content://")) {
+            StringBuilder result = new StringBuilder(address);
+            for (String element : elements) {
+                result.append(TreeUriDownloader.ESCAPED_SLASH);
+                result.append(element);
+            }
+            return result.toString();
+        } else { // Normal URL
+            Uri.Builder result = Uri.parse(address).buildUpon();
+            for (String element : elements) {
+                result.appendPath(element);
+            }
+            return result.build().toString();
+        }
+    }
+
     private static int toInt(Integer value) {
         if (value == null) {
             return 0;

app/src/test/java/org/fdroid/fdroid/RepoUrlsTest.java

@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2021 Angus Gratton
+ * Copyright (C) 2018 Senecto Limited
+ *
+ * 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;
+
+import org.fdroid.fdroid.data.Apk;
+import org.fdroid.fdroid.data.FDroidProviderTest;
+import org.fdroid.fdroid.data.Repo;
+import org.fdroid.fdroid.mock.MockApk;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(RobolectricTestRunner.class)
+public class RepoUrlsTest extends FDroidProviderTest {
+    public static final String TAG = "RepoUrlsTest";
+
+    /**
+     * Private class describing a repository URL we're going to test, and
+     * the file pattern for any files within that URL.
+     */
+    private static class TestRepo {
+        // Repo URL for the test case
+        public String repoUrl;
+        // String format pattern for generating file URLs, should contain a single %s for the filename
+        public String fileUrlPattern;
+
+        TestRepo(String repoUrl, String fileUrlPattern) {
+            this.repoUrl = repoUrl;
+            this.fileUrlPattern = fileUrlPattern;
+        }
+    }
+
+    private static final String APK_NAME = "test-v1.apk";
+
+    private static final TestRepo[] REPOS = {
+            new TestRepo(
+                    "https://microg.org/fdroid/repo",
+                    "https://microg.org/fdroid/repo/%s"),
+            new TestRepo(
+                    "http://bdf2wcxujkg6qqff.onion/fdroid/repo",
+                    "http://bdf2wcxujkg6qqff.onion/fdroid/repo/%s"),
+            new TestRepo(
+                    "http://lysator7eknrfl47rlyxvgeamrv7ucefgrrlhk7rouv3sna25asetwid.onion/pub/fdroid/repo",
+                    "http://lysator7eknrfl47rlyxvgeamrv7ucefgrrlhk7rouv3sna25asetwid.onion/pub/fdroid/repo/%s"),
+            new TestRepo(
+                    "https://mirrors.nju.edu.cn/fdroid/repo?fingerprint=43238D512C1E5EB2D6569F4A3AFBF5523418B82E0A3ED1552770ABB9A9C9CCAB",
+                    "https://mirrors.nju.edu.cn/fdroid/repo/%s?fingerprint=43238D512C1E5EB2D6569F4A3AFBF5523418B82E0A3ED1552770ABB9A9C9CCAB"),
+            new TestRepo(
+                    "https://raw.githubusercontent.com/guardianproject/fdroid-repo/master/fdroid/repo",
+                    "https://raw.githubusercontent.com/guardianproject/fdroid-repo/master/fdroid/repo/%s"),
+            new TestRepo(
+                    "content://com.android.externalstorage.documents/tree/1AFB-2402%3A/document/1AFB-2402%3Atesty.at.or.at%2Ffdroid%2Frepo",
+                    // note: to have a URL-escaped path in a format string pattern, we need to
+                    // %-escape all URL %
+                    "content://com.android.externalstorage.documents/tree/1AFB-2402%%3A/document/1AFB-2402%%3Atesty.at.or.at%%2Ffdroid%%2Frepo%%2F%s"),
+            new TestRepo(
+                    "content://authority/tree/313E-1F1C%3A/document/313E-1F1C%3Aguardianproject.info%2Ffdroid%2Frepo",
+                    // note: to have a URL-escaped path in a format string pattern, we need to
+                    // %-escape all URL %
+                    "content://authority/tree/313E-1F1C%%3A/document/313E-1F1C%%3Aguardianproject.info%%2Ffdroid%%2Frepo%%2F%s"),
+            new TestRepo(
+                    "http://10.20.31.244:8888/fdroid/repo?FINGERPRINT=35521D88285A9D06FBE33D35FB8B4BB872D753666CF981728E2249FEE6D2D0F2&SWAP=1&BSSID=FE:EE:DA:45:2D:4E",
+                    "http://10.20.31.244:8888/fdroid/repo/%s?FINGERPRINT=35521D88285A9D06FBE33D35FB8B4BB872D753666CF981728E2249FEE6D2D0F2&SWAP=1&BSSID=FE:EE:DA:45:2D:4E"),
+            new TestRepo(
+                    "fdroidrepos://briarproject.org/fdroid/repo?fingerprint=1FB874BEE7276D28ECB2C9B06E8A122EC4BCB4008161436CE474C257CBF49BD6",
+                    "fdroidrepos://briarproject.org/fdroid/repo/%s?fingerprint=1FB874BEE7276D28ECB2C9B06E8A122EC4BCB4008161436CE474C257CBF49BD6"),
+    };
+
+    @Before
+    public void setup() {
+        Preferences.setupForTests(context);
+    }
+
+    interface GetFileFromRepo {
+        String get(TestRepo tr);
+    }
+
+    /**
+     * Utility test function - go through the list of test repos,
+     * using the useOfRepo interface to instantiate a repo from the URL
+     * and return a file of some kind (Apk, index, etc.) and check that
+     * it matches the test repo's expected URL format.
+     *
+     * @param fileName  File that 'useOfRepo' will return in the repo, when called
+     * @param useOfRepo Instance of the function that uses the repo to build a file URL
+     */
+    private void testReposWithFile(String fileName, GetFileFromRepo useOfRepo) {
+        for (TestRepo tr : REPOS) {
+            String expectedUrl = String.format(tr.fileUrlPattern, fileName);
+            System.out.println("Testing URL " + expectedUrl);
+            String actualUrl = useOfRepo.get(tr);
+            assertEquals(expectedUrl, actualUrl);
+        }
+    }
+
+    @Test
+    public void testIndexUrls() {
+        testReposWithFile(IndexUpdater.SIGNED_FILE_NAME, new GetFileFromRepo() {
+            @Override
+            public String get(TestRepo tr) {
+                Repo repo = new Repo();
+                repo.address = tr.repoUrl;
+                IndexUpdater updater = new IndexUpdater(context, repo);
+                return updater.getIndexUrl(repo);
+            }
+        });
+    }
+
+    @Test
+    public void testIndexV1Urls() {
+        testReposWithFile(IndexV1Updater.SIGNED_FILE_NAME, new GetFileFromRepo() {
+            @Override
+            public String get(TestRepo tr) {
+                Repo repo = new Repo();
+                repo.address = tr.repoUrl;
+                IndexV1Updater updater = new IndexV1Updater(context, repo);
+                return updater.getIndexUrl(repo);
+            }
+        });
+    }
+
+    @Test
+    public void testApkUrls() {
+        testReposWithFile(APK_NAME, new GetFileFromRepo() {
+            @Override
+            public String get(TestRepo tr) {
+                Apk apk = new MockApk(APK_NAME, 1, tr.repoUrl, APK_NAME);
+                return apk.getCanonicalUrl();
+            }
+        });
+    }
+}

app/src/test/java/org/fdroid/fdroid/mock/MockApk.java

@@ -10,6 +10,13 @@ public class MockApk extends Apk {
         this.versionCode = versionCode;
     }
 
+    public MockApk(String id, int versionCode, String repoAddress, String apkName) {
+        this.packageName = id;
+        this.versionCode = versionCode;
+        this.repoAddress = repoAddress;
+        this.apkName = apkName;
+    }
+
     public MockApk(App app, int versionCode) {
         this.appId = app.getId();
         this.versionCode = versionCode;