New, overhauled data storage management #1954

mario · 2018-01-11T19:57:33Z

Improved & rebased version of the awesome community PR found here:
#1836

We don't do files migration automatically yet, I'd suggest we implement that for 2.2 since it needs some UI as well, but push keys are properly migrated if they exist.

przybylski · 2018-01-11T20:43:42Z

src/main/java/com/owncloud/android/MainApp.java

+
+            String keyPath = getAppContext().getFilesDir().getAbsolutePath() +
+                    File.separator + MainApp.getDataFolder() + File.separator + "nc-keypair";
+            File privateKey = new File(keyPath + File.separator + "push_key.priv");


Equivalent of: File privateKey = new File(keyPath, "push_key.priv");
same for the line up and down

Would you be so kind to improve in the branch directly all things you think need improving? :)

przybylski · 2018-01-11T20:46:35Z

src/main/java/com/owncloud/android/MainApp.java

+            File publicKey = new File(keyPath + File.separator + "push_key.pub");
+
+            if (oldPrivateKey.exists() && oldPublicKey.exists()) {
+                if (oldPrivateKey.renameTo(privateKey) && oldPublicKey.renameTo(publicKey)) {


This doesn't handle the scenario when first rename succeeds and other fails.

przybylski · 2018-01-11T20:49:29Z

src/main/java/com/owncloud/android/datastorage/providers/SystemDefaultStoragePointProvider.java

-        }
+        // Add private internal storage data directory.
+        result.add(new StoragePoint(defaultStringDesc,
+                MainApp.getAppContext().getFilesDir().getAbsolutePath()));


I am not a fan of this storage point using context. IMO there should be a method in MainApp which directly returns the files dir and use that.

I see we use this at a couple other places, so not worried about this much.

Would indeed be better to centralize this, but I agree with @mario that this does not need to be done now.

mario · 2018-01-11T21:20:03Z

Feel free to improve directly in the branch?:)

…

On Thu, 11 Jan 2018 at 21:55, Bartosz Przybylski ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/com/owncloud/android/MainApp.java <#1954 (comment)>: > @@ -236,6 +236,31 @@ public static void initAutoUpload() { } } + public static void migratePushKeys() { + Context context = getAppContext(); + + if (!PreferenceManager.getKeysMigration(context)) { + String oldKeyPath = getStoragePath() + File.separator + MainApp.getDataFolder() + File.separator + "nc-keypair"; + File oldPrivateKey = new File(oldKeyPath + File.separator + "push_key.priv"); + File oldPublicKey = new File(oldKeyPath + File.separator + "push_key.pub"); + + String keyPath = getAppContext().getFilesDir().getAbsolutePath() + + File.separator + MainApp.getDataFolder() + File.separator + "nc-keypair"; + File privateKey = new File(keyPath + File.separator + "push_key.priv"); Equivalent of: File privateKey = new File(keyPath, "push_key.priv"); same for the line up and down ------------------------------ In src/main/java/com/owncloud/android/MainApp.java <#1954 (comment)>: > @@ -236,6 +236,31 @@ public static void initAutoUpload() { } } + public static void migratePushKeys() { + Context context = getAppContext(); + + if (!PreferenceManager.getKeysMigration(context)) { + String oldKeyPath = getStoragePath() + File.separator + MainApp.getDataFolder() + File.separator + "nc-keypair"; + File oldPrivateKey = new File(oldKeyPath + File.separator + "push_key.priv"); + File oldPublicKey = new File(oldKeyPath + File.separator + "push_key.pub"); + + String keyPath = getAppContext().getFilesDir().getAbsolutePath() + + File.separator + MainApp.getDataFolder() + File.separator + "nc-keypair"; + File privateKey = new File(keyPath + File.separator + "push_key.priv"); + File publicKey = new File(keyPath + File.separator + "push_key.pub"); + + if (oldPrivateKey.exists() && oldPublicKey.exists()) { + if (oldPrivateKey.renameTo(privateKey) && oldPublicKey.renameTo(publicKey)) { This doesn't handle the scenario when first rename succeeds and other fails. ------------------------------ In src/main/java/com/owncloud/android/datastorage/providers/SystemDefaultStoragePointProvider.java <#1954 (comment)>: > @@ -44,16 +41,9 @@ public boolean canProvideStoragePoints() { Vector<StoragePoint> result = new Vector<>(); final String defaultStringDesc = MainApp.getAppContext().getString(R.string.storage_description_default); - File path; - if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP) { - path = MainApp.getAppContext().getExternalMediaDirs()[0]; - } else { - path = getExternalStorageDirectory(); - } - - if (path != null && path.canWrite()) { - result.add(new StoragePoint(defaultStringDesc, path.getAbsolutePath())); - } + // Add private internal storage data directory. + result.add(new StoragePoint(defaultStringDesc, + MainApp.getAppContext().getFilesDir().getAbsolutePath())); I am not a fan of this storage point using context. IMO there should be a method in MainApp which directly returns the files dir and use that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1954 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAWssYdqFmMe4R9rBtAukFyqofOwRHeks5tJnVXgaJpZM4RbXi8> .

mario · 2018-01-11T23:45:56Z

Fixed everything @przybylski except for the context thing which I'm unsure if we should fix.

tobiasKaminsky · 2018-01-12T16:19:49Z

src/main/java/com/owncloud/android/db/PreferenceManager.java

@@ -286,6 +287,11 @@ public static boolean getLegacyClean(Context context) {
        return getDefaultSharedPreferences(context).getBoolean(PREF__LEGACY_CLEAN, false);
    }

+    public static boolean getKeysMigration(Context context) {
+        return getDefaultSharedPreferences(context).getBoolean(PREF__KEYS_MIGRATION, false);
+    }


I thought we wanted to start using ArbitraryDataProvider for stuff like this instead of shared preferences?

Yes, but later. Not less than a day before RC :P

tobiasKaminsky · 2018-01-12T16:21:12Z

src/main/java/com/owncloud/android/MainApp.java

+                PreferenceManager.setKeysMigration(context, true);
+            } else {
+                if (oldPrivateKey.exists()) {
+                    oldPrivateKey.renameTo(privateKey);


Is renameTo working if used across filesystem boundary, e.g. from external sdcard to internal storage? As far as I remember this does not work, but we have to copy and delete instead.

Edit: doc says:

Both paths be on the same mount point. On Android, applications are most likely to hit this restriction when attempting to copy between internal storage and an SD card.

Will investigate.

tobiasKaminsky · 2018-01-12T16:23:39Z

src/main/java/com/owncloud/android/MainApp.java

+                    oldPublicKey.renameTo(publicKey);
+                }
+
+                if (privateKey.exists() && publicKey.exists()) {


This check will always be true as those files are created at Line 245/246, or? A more sane check would be if length > 0?

Not always. Maybe just one file was created :)

tobiasKaminsky · 2018-01-12T16:26:07Z

src/main/java/com/owncloud/android/datastorage/DataStorageProvider.java

+
+        // Add external storage directory if available.
+        if (isExternalStorageWritable()) {
+            mCachedStoragePoints.add(new StoragePoint(MainApp.getAppContext().getExternalFilesDir(null).getAbsolutePath(),


"getExternalFilesDir(null)" can be null:

May return null if shared storage is not currently available.

So this should be checked, otherwise we get a NPE

tobiasKaminsky · 2018-01-12T16:41:00Z

src/main/java/com/owncloud/android/providers/FileContentProvider.java

            selection = "(?)";
+            selectionArgs = new String[]{selection};


This was done to prevent sql injections: instead of querying the args directly those are inserted via "(?):

select * where test = " + $foo; (foo can be dangerous)
becomes to
"select * where test = (?)", $foo --> then we have use of built in sql prevention.

Changing this, would mean that selectionArgs is always "(?)" which is not what we want to have: if args is null, then we want to "force" selection to use "(?)":
selection: "1=1"
--> select * from xy where "1=1"
becomes
--> select * from xy where (?), "1" and this sql injection save

So this must be reverted

But then the Uploads view crashes for me?

When? With which uploads? Do you have a stacktrace?
This is for quite a time in dev version and I never experienced a crash on upload view (except of the TimSort exception).
Or it is a new thing with this branch...

No, was a problem with master. The Uploads view. But no stacktrace :(

tobiasKaminsky

Sql prevention stuff needs to be changed/discussed.

tobiasKaminsky · 2018-01-12T18:11:10Z

There is another severe bug:

fresh installation
open auto upload and enable any folder
close auto upload view
re-open: folder is not enabled and also upload does not work

(works on master)

mario · 2018-01-12T18:12:38Z

I will definitely take a look, but what about the Uploads? That is crashing for me (sometimes) on master, and my change fixes it.

tobiasKaminsky · 2018-01-12T18:14:34Z

As said, give me a stacktrace/way to reproduce.
I think that the fix reverts sql injection protection...

mario · 2018-01-12T18:15:49Z

@tobiasKaminsky ok, will revert the fix for now and we'll see in RC :)

mario · 2018-01-12T20:37:10Z

@tobiasKaminsky just tried, works. so this PR is ready.

tobiasKaminsky · 2018-01-13T09:15:16Z

Will test it with real device with external storage on monday.

Signed-off-by: Mario Danic <[email protected]>

Signed-off-by: tobiaskaminsky <[email protected]>

Signed-off-by: Mario Danic <[email protected]>

tobiasKaminsky · 2018-01-15T10:48:18Z

src/main/java/com/owncloud/android/MainApp.java

@@ -236,6 +243,53 @@ public static void initAutoUpload() {
        }
    }

+    public static void migratePushKeys() {


This is only use in PushUtils on gplay flavor, so this can be moved there, or?

Maybe. :) Or just leave it here and don't worry :D

I guess it should be moved to not have the dependency in the f-droid build where this might be an issue then, isn't it?

I moved it already :-)

Wouldn't be a problem because it's just moving files, no Google Play deps 📦

Signed-off-by: tobiasKaminsky <[email protected]>

mario · 2018-01-15T11:42:47Z

👍

AndyScherzinger · 2018-01-15T12:06:30Z

👍 only one minor comment about the f-droid build and the PushUtils dependency: #1954 (comment)

tobiasKaminsky · 2018-01-15T12:08:08Z

Welcome back @AndyScherzinger 🎉

mario · 2018-01-15T12:08:54Z

src/gplay/java/com/owncloud/android/utils/PushUtils.java

@@ -335,4 +336,44 @@ private static int saveKeyToFile(Key key, String path) {

        return -1;
    }
+
+    public static void migratePushKeys() {


this can be private void migratePushKeys() then :P

mario · 2018-01-15T12:10:00Z

src/modified/java/com/owncloud/android/utils/PushUtils.java

@@ -99,8 +99,9 @@ public static String bytesToHex(byte[] bytes) {
    }

    private static int generateRsa2048KeyPair() {
-        String keyPath = MainApp.getStoragePath() + File.separator + MainApp.getDataFolder() + File.separator
-                + KEYPAIR_FOLDER;
+        MainApp.migratePushKeys();


Also this won't work if you remove it from the MainApp :P

fix modified, but will soon be removed Signed-off-by: tobiasKaminsky <[email protected]>

mario mentioned this pull request Jan 11, 2018

Overhauled data storage management #1836

Closed

mario added the 3. to review label Jan 11, 2018

mario added this to the Nextcloud App 2.1.0 milestone Jan 11, 2018

przybylski reviewed Jan 11, 2018

View reviewed changes

mario force-pushed the new-ardevd-storage branch from 1d879ee to 10923fb Compare January 11, 2018 23:57

tobiasKaminsky reviewed Jan 12, 2018

View reviewed changes

tobiasKaminsky requested changes Jan 12, 2018

View reviewed changes

mario force-pushed the new-ardevd-storage branch from 19e8b0b to a71e3aa Compare January 12, 2018 18:11

nextcloud deleted a comment Jan 12, 2018

tobiasKaminsky force-pushed the new-ardevd-storage branch from 2150617 to 6d5e79e Compare January 13, 2018 09:12

tobiasKaminsky approved these changes Jan 13, 2018

View reviewed changes

nextcloud deleted a comment Jan 13, 2018

mario added 5 commits January 15, 2018 11:23

Most improvements

845e85f

Signed-off-by: Mario Danic <[email protected]>

Fix uploads

d56f53b

Signed-off-by: Mario Danic <[email protected]>

Implement keys migration

ee7d4ed

Signed-off-by: Mario Danic <[email protected]>

Add new line

4c6a98d

Signed-off-by: Mario Danic <[email protected]>

Fix code review issues

66711f2

Signed-off-by: Mario Danic <[email protected]>

tobiasKaminsky and others added 4 commits January 15, 2018 11:23

linebreaks

45f7cb1

Signed-off-by: tobiaskaminsky <[email protected]>

Attempt to fix moves

f626f58

Signed-off-by: Mario Danic <[email protected]>

Remove double dep

ffb33a3

Signed-off-by: Mario Danic <[email protected]>

Revert change

1c8d46f

Signed-off-by: Mario Danic <[email protected]>

tobiasKaminsky reviewed Jan 15, 2018

View reviewed changes

use copy & delete instead of move

88498e4

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky force-pushed the new-ardevd-storage branch from 6d5e79e to 88498e4 Compare January 15, 2018 11:40

nextcloud deleted a comment Jan 15, 2018

AndyScherzinger changed the title ~~New ardevd storage~~ New, overhauled data storage management Jan 15, 2018

nextcloud deleted a comment Jan 15, 2018

mario commented Jan 15, 2018

View reviewed changes

function is private

59fafc7

fix modified, but will soon be removed Signed-off-by: tobiasKaminsky <[email protected]>

nextcloud deleted a comment Jan 15, 2018

tobiasKaminsky merged commit 4394320 into master Jan 15, 2018

AndyScherzinger deleted the new-ardevd-storage branch January 15, 2018 13:09

AndyScherzinger removed the 3. to review label Jan 15, 2018

This was referenced Jan 15, 2018

Push doesn't work #1707

Closed

nc-keypair files on /sdcard #1833

Closed

This was referenced Jan 18, 2018

Missing Files in Android #1989

Closed

Fix storage stuff #1998

Merged

New, overhauled data storage management #1954

New, overhauled data storage management #1954

Conversation

mario commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mario commented Jan 11, 2018 via email

mario commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasKaminsky Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasKaminsky Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasKaminsky left a comment

Choose a reason for hiding this comment

tobiasKaminsky commented Jan 12, 2018

mario commented Jan 12, 2018

tobiasKaminsky commented Jan 12, 2018

mario commented Jan 12, 2018

mario commented Jan 12, 2018

tobiasKaminsky commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mario commented Jan 15, 2018

AndyScherzinger commented Jan 15, 2018 • edited Loading

tobiasKaminsky commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasKaminsky Jan 12, 2018 •

edited

Loading

tobiasKaminsky Jan 12, 2018 •

edited

Loading

AndyScherzinger commented Jan 15, 2018 •

edited

Loading