From 31d332470134899081bd4742806b3f5bcc544f28 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Fri, 1 Mar 2019 16:49:19 -0300 Subject: [PATCH 1/2] [android] stop livecycle before delete app data and exit cleanly Fixes #1380 --- .../account/AndroidAccountManager.java | 30 ++++++++++++------ .../account/AndroidAccountManagerTest.java | 12 +++++++ .../lifecycle/LifecycleManagerImpl.java | 1 + .../android/panic/PanicResponderActivity.java | 31 +++++++++++++------ 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java index f23e249ae..924fe4a41 100644 --- a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java +++ b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java @@ -12,11 +12,15 @@ import org.briarproject.bramble.api.identity.IdentityManager; import org.briarproject.bramble.util.IoUtils; import java.io.File; +import java.util.HashSet; import java.util.logging.Logger; import javax.annotation.Nullable; import javax.inject.Inject; +import static android.os.Build.VERSION.SDK_INT; +import static java.util.Arrays.asList; + class AndroidAccountManager extends AccountManagerImpl implements AccountManager { @@ -90,19 +94,27 @@ class AndroidAccountManager extends AccountManagerImpl } // Delete files, except lib and shared_prefs directories File dataDir = new File(appContext.getApplicationInfo().dataDir); - File[] children = dataDir.listFiles(); - if (children == null) { + HashSet files = new HashSet<>(asList(dataDir.listFiles())); + if (files.isEmpty()) { LOG.warning("Could not list files in app data dir"); - } else { - for (File child : children) { - String name = child.getName(); - if (!name.equals("lib") && !name.equals("shared_prefs")) { - IoUtils.deleteFileOrDir(child); - } + } + files.add(appContext.getFilesDir()); + files.add(appContext.getCacheDir()); + files.add(appContext.getExternalCacheDir()); + if (SDK_INT >= 19) { + files.addAll(asList(appContext.getExternalCacheDirs())); + } + if (SDK_INT >= 21) { + files.addAll(asList(appContext.getExternalMediaDirs())); + } + for (File file : files) { + String name = file.getName(); + if (!name.equals("lib") && !name.equals("shared_prefs")) { + IoUtils.deleteFileOrDir(file); } } // Recreate the cache dir as some OpenGL drivers expect it to exist - if (!new File(dataDir, "cache").mkdir()) + if (!new File(dataDir, "cache").mkdirs()) LOG.warning("Could not recreate cache dir"); } } diff --git a/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java b/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java index 6736533a6..cdda0ca04 100644 --- a/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java +++ b/bramble-android/src/test/java/org/briarproject/bramble/account/AndroidAccountManagerTest.java @@ -112,6 +112,8 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { // Other directories should be deleted File potatoDir = new File(testDir, ".potato"); File potatoFile = new File(potatoDir, "file"); + File filesDir = new File(testDir, "filesDir"); + File externalCacheDir = new File(testDir, "externalCacheDir"); context.checking(new Expectations() {{ oneOf(prefs).edit(); @@ -128,6 +130,12 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { will(returnValue(true)); oneOf(app).getApplicationInfo(); will(returnValue(applicationInfo)); + oneOf(app).getFilesDir(); + will(returnValue(filesDir)); + oneOf(app).getCacheDir(); + will(returnValue(cacheDir)); + oneOf(app).getExternalCacheDir(); + will(returnValue(externalCacheDir)); }}); assertTrue(dbDir.mkdirs()); @@ -140,6 +148,8 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { assertTrue(cacheFile.createNewFile()); assertTrue(potatoDir.mkdirs()); assertTrue(potatoFile.createNewFile()); + assertTrue(filesDir.mkdirs()); + assertTrue(externalCacheDir.mkdirs()); accountManager.deleteAccount(); @@ -153,6 +163,8 @@ public class AndroidAccountManagerTest extends BrambleMockTestCase { assertFalse(cacheFile.exists()); assertFalse(potatoDir.exists()); assertFalse(potatoFile.exists()); + assertFalse(filesDir.exists()); + assertFalse(externalCacheDir.exists()); } @After diff --git a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java index da55a780c..67791f5e3 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java @@ -168,6 +168,7 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { @Override public void stopServices() { + if (state == STOPPING) return; try { startStopSemaphore.acquire(); } catch (InterruptedException e) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java index 8b5550104..7cec90c43 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java @@ -6,12 +6,16 @@ import android.os.Bundle; import android.support.v7.preference.PreferenceManager; import org.briarproject.bramble.api.account.AccountManager; +import org.briarproject.bramble.api.lifecycle.LifecycleManager; +import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; +import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault; import org.briarproject.bramble.api.system.AndroidExecutor; import org.briarproject.briar.android.activity.ActivityComponent; import org.briarproject.briar.android.activity.BriarActivity; import java.util.logging.Logger; +import javax.annotation.Nullable; import javax.inject.Inject; import info.guardianproject.GuardianProjectRSA4096; @@ -20,21 +24,27 @@ import info.guardianproject.panic.PanicResponder; import info.guardianproject.trustedintents.TrustedIntents; import static android.os.Build.VERSION.SDK_INT; +import static java.util.logging.Level.WARNING; +import static org.briarproject.bramble.util.LogUtils.logException; import static org.briarproject.briar.android.panic.PanicPreferencesFragment.KEY_LOCK; import static org.briarproject.briar.android.panic.PanicPreferencesFragment.KEY_PURGE; +@MethodsNotNullByDefault +@ParametersNotNullByDefault public class PanicResponderActivity extends BriarActivity { private static final Logger LOG = Logger.getLogger(PanicResponderActivity.class.getName()); + @Inject + protected LifecycleManager lifecycleManager; @Inject protected AccountManager accountManager; @Inject protected AndroidExecutor androidExecutor; @Override - public void onCreate(Bundle savedInstanceState) { + public void onCreate(@Nullable Bundle savedInstanceState) { super.onCreate(savedInstanceState); TrustedIntents trustedIntents = TrustedIntents.get(this); @@ -83,14 +93,17 @@ public class PanicResponderActivity extends BriarActivity { private void deleteAllData() { androidExecutor.runOnBackgroundThread(() -> { - accountManager.deleteAccount(); - // TODO somehow delete/shred the database more thoroughly - PanicResponder.deleteAllAppData(PanicResponderActivity.this); - - // nothing left to do after everything is deleted, - // so still sign out - LOG.info("Signing out..."); - signOut(true); + lifecycleManager.stopServices(); + try { + lifecycleManager.waitForShutdown(); + } catch (InterruptedException e) { + logException(LOG, WARNING, e); + } finally { + accountManager.deleteAccount(); + // nothing left to do after everything is deleted, so sign out + LOG.info("Signing out..."); + signOut(true); + } }); } } From 270b8af39fc952eda960782bf4a83c0f22773062 Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Fri, 8 Mar 2019 17:14:03 -0300 Subject: [PATCH 2/2] [android] add review comments for panic induced account deletion --- .../account/AndroidAccountManager.java | 8 ++++++-- .../lifecycle/LifecycleManagerImpl.java | 1 - .../briar/android/activity/BriarActivity.java | 6 ++++-- .../android/controller/BriarController.java | 5 ++++- .../controller/BriarControllerImpl.java | 10 +++++++++- .../android/navdrawer/NavDrawerActivity.java | 4 ++-- .../android/panic/PanicResponderActivity.java | 20 +++---------------- 7 files changed, 28 insertions(+), 26 deletions(-) diff --git a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java index 924fe4a41..b2745aa8c 100644 --- a/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java +++ b/bramble-android/src/main/java/org/briarproject/bramble/account/AndroidAccountManager.java @@ -93,10 +93,14 @@ class AndroidAccountManager extends AccountManagerImpl LOG.warning("Could not clear shared preferences"); } // Delete files, except lib and shared_prefs directories + HashSet files = new HashSet<>(); File dataDir = new File(appContext.getApplicationInfo().dataDir); - HashSet files = new HashSet<>(asList(dataDir.listFiles())); - if (files.isEmpty()) { + @Nullable + File[] fileArray = dataDir.listFiles(); + if (fileArray == null) { LOG.warning("Could not list files in app data dir"); + } else { + files.addAll(asList(fileArray)); } files.add(appContext.getFilesDir()); files.add(appContext.getCacheDir()); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java index 67791f5e3..da55a780c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/lifecycle/LifecycleManagerImpl.java @@ -168,7 +168,6 @@ class LifecycleManagerImpl implements LifecycleManager, MigrationListener { @Override public void stopServices() { - if (state == STOPPING) return; try { startStopSemaphore.acquire(); } catch (InterruptedException e) { diff --git a/briar-android/src/main/java/org/briarproject/briar/android/activity/BriarActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/activity/BriarActivity.java index 7cbd63944..917dce9b6 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/activity/BriarActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/activity/BriarActivity.java @@ -185,13 +185,15 @@ public abstract class BriarActivity extends BaseActivity { b.show(); } - protected void signOut(boolean removeFromRecentApps) { + protected void signOut(boolean removeFromRecentApps, + boolean deleteAccount) { if (briarController.accountSignedIn()) { // Don't use UiResultHandler because we want the result even if // this activity has been destroyed briarController.signOut(result -> runOnUiThread( - () -> exit(removeFromRecentApps))); + () -> exit(removeFromRecentApps)), deleteAccount); } else { + if (deleteAccount) briarController.deleteAccount(); exit(removeFromRecentApps); } } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarController.java b/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarController.java index e4e60c692..c86e6f905 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarController.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarController.java @@ -16,5 +16,8 @@ public interface BriarController extends ActivityLifecycleController { void doNotAskAgainForDozeWhiteListing(); - void signOut(ResultHandler eventHandler); + void signOut(ResultHandler eventHandler, boolean deleteAccount); + + void deleteAccount(); + } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarControllerImpl.java b/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarControllerImpl.java index 1271f4d21..d492f0472 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarControllerImpl.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/controller/BriarControllerImpl.java @@ -120,7 +120,8 @@ public class BriarControllerImpl implements BriarController { } @Override - public void signOut(ResultHandler eventHandler) { + public void signOut(ResultHandler eventHandler, + boolean deleteAccount) { new Thread(() -> { try { // Wait for the service to finish starting up @@ -134,11 +135,18 @@ public class BriarControllerImpl implements BriarController { service.waitForShutdown(); } catch (InterruptedException e) { LOG.warning("Interrupted while waiting for service"); + } finally { + if (deleteAccount) accountManager.deleteAccount(); } eventHandler.onResult(null); }).start(); } + @Override + public void deleteAccount() { + accountManager.deleteAccount(); + } + private void unbindService() { if (bound) activity.unbindService(serviceConnection); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/navdrawer/NavDrawerActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/navdrawer/NavDrawerActivity.java index 67d6d8195..49a49d5bc 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/navdrawer/NavDrawerActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/navdrawer/NavDrawerActivity.java @@ -109,7 +109,7 @@ public class NavDrawerActivity extends BriarActivity implements } else if (intent.getBooleanExtra(INTENT_BLOGS, false)) { startFragment(FeedFragment.newInstance(), R.id.nav_btn_blogs); } else if (intent.getBooleanExtra(INTENT_SIGN_OUT, false)) { - signOut(false); + signOut(false, false); } setIntent(null); } @@ -279,7 +279,7 @@ public class NavDrawerActivity extends BriarActivity implements private void signOut() { drawerLayout.setDrawerLockMode(LOCK_MODE_LOCKED_CLOSED); - signOut(false); + signOut(false, false); finish(); } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java b/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java index 7cec90c43..dfa9fa079 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/panic/PanicResponderActivity.java @@ -5,7 +5,6 @@ import android.content.SharedPreferences; import android.os.Bundle; import android.support.v7.preference.PreferenceManager; -import org.briarproject.bramble.api.account.AccountManager; import org.briarproject.bramble.api.lifecycle.LifecycleManager; import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault; import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault; @@ -24,8 +23,6 @@ import info.guardianproject.panic.PanicResponder; import info.guardianproject.trustedintents.TrustedIntents; import static android.os.Build.VERSION.SDK_INT; -import static java.util.logging.Level.WARNING; -import static org.briarproject.bramble.util.LogUtils.logException; import static org.briarproject.briar.android.panic.PanicPreferencesFragment.KEY_LOCK; import static org.briarproject.briar.android.panic.PanicPreferencesFragment.KEY_PURGE; @@ -39,8 +36,6 @@ public class PanicResponderActivity extends BriarActivity { @Inject protected LifecycleManager lifecycleManager; @Inject - protected AccountManager accountManager; - @Inject protected AndroidExecutor androidExecutor; @Override @@ -74,7 +69,7 @@ public class PanicResponderActivity extends BriarActivity { // non-destructive actions are allowed by non-connected trusted apps if (sharedPref.getBoolean(KEY_LOCK, true)) { LOG.info("Signing out..."); - signOut(true); + signOut(true, false); } } } @@ -93,17 +88,8 @@ public class PanicResponderActivity extends BriarActivity { private void deleteAllData() { androidExecutor.runOnBackgroundThread(() -> { - lifecycleManager.stopServices(); - try { - lifecycleManager.waitForShutdown(); - } catch (InterruptedException e) { - logException(LOG, WARNING, e); - } finally { - accountManager.deleteAccount(); - // nothing left to do after everything is deleted, so sign out - LOG.info("Signing out..."); - signOut(true); - } + LOG.info("Signing out..."); + signOut(true, true); }); } }