From 165c136beae31514d701791c9f654b2568338f46 Mon Sep 17 00:00:00 2001 From: Catfriend1 Date: Sat, 2 Jun 2018 21:49:55 +0200 Subject: [PATCH] Multiple fixes (fixes #871, fixes #1115, fixes #1116) Handle storage permissions Fix multiple processes being started. --- app/src/main/AndroidManifest.xml | 3 +- .../activities/FirstStartActivity.java | 1 + .../activities/MainActivity.java | 13 ++ .../service/DeviceStateHolder.java | 33 +++-- .../service/NotificationHandler.java | 14 ++ .../service/SyncthingService.java | 133 +++++++++++------- app/src/main/res/values/strings.xml | 2 + 7 files changed, 140 insertions(+), 59 deletions(-) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index df9a980a..41cd1894 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -30,7 +30,8 @@ android:name=".SyncthingApp"> + android:label="@string/app_name" + android:launchMode="singleInstance"> diff --git a/app/src/main/java/com/nutomic/syncthingandroid/activities/FirstStartActivity.java b/app/src/main/java/com/nutomic/syncthingandroid/activities/FirstStartActivity.java index d9f42f4e..6b35f9e9 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/activities/FirstStartActivity.java +++ b/app/src/main/java/com/nutomic/syncthingandroid/activities/FirstStartActivity.java @@ -107,6 +107,7 @@ public class FirstStartActivity extends Activity implements Button.OnClickListen grantResults[0] != PackageManager.PERMISSION_GRANTED) { Toast.makeText(this, R.string.toast_write_storage_permission_required, Toast.LENGTH_LONG).show(); + this.finish(); } else { startApp(); } diff --git a/app/src/main/java/com/nutomic/syncthingandroid/activities/MainActivity.java b/app/src/main/java/com/nutomic/syncthingandroid/activities/MainActivity.java index be833516..fca5ac1f 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/activities/MainActivity.java +++ b/app/src/main/java/com/nutomic/syncthingandroid/activities/MainActivity.java @@ -14,6 +14,7 @@ import android.content.pm.PackageManager; import android.content.res.Configuration; import android.graphics.Bitmap; import android.graphics.drawable.BitmapDrawable; +import android.Manifest; import android.net.Uri; import android.os.Build; import android.os.Bundle; @@ -24,6 +25,7 @@ import android.support.design.widget.TabLayout; import android.support.v4.app.Fragment; import android.support.v4.app.FragmentManager; import android.support.v4.app.FragmentPagerAdapter; +import android.support.v4.content.ContextCompat; import android.support.v4.view.GravityCompat; import android.support.v4.view.ViewPager; import android.support.v4.widget.DrawerLayout; @@ -255,6 +257,17 @@ public class MainActivity extends StateDialogActivity onNewIntent(getIntent()); } + @Override + public void onResume() { + // Check if storage permission has been revoked at runtime. + if ((ContextCompat.checkSelfPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE) != + PackageManager.PERMISSION_GRANTED)) { + startActivity(new Intent(this, FirstStartActivity.class)); + this.finish(); + } + super.onResume(); + } + @Override public void onDestroy() { super.onDestroy(); diff --git a/app/src/main/java/com/nutomic/syncthingandroid/service/DeviceStateHolder.java b/app/src/main/java/com/nutomic/syncthingandroid/service/DeviceStateHolder.java index e4611ac1..e2f33446 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/service/DeviceStateHolder.java +++ b/app/src/main/java/com/nutomic/syncthingandroid/service/DeviceStateHolder.java @@ -65,7 +65,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh } public interface OnDeviceStateChangedListener { - void onDeviceStateChanged(); + void onDeviceStateChanged(boolean shouldRun); } private final Context mContext; @@ -74,16 +74,22 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh private final OnDeviceStateChangedListener mListener; @Inject SharedPreferences mPreferences; - private @Nullable NetworkReceiver mNetworkReceiver; - private @Nullable BatteryReceiver mBatteryReceiver; - private @Nullable BroadcastReceiver mPowerSaveModeChangedReceiver; + private @Nullable NetworkReceiver mNetworkReceiver = null; + private @Nullable BatteryReceiver mBatteryReceiver = null; + private @Nullable BroadcastReceiver mPowerSaveModeChangedReceiver = null; private boolean mIsAllowedNetworkConnection; private String mWifiSsid; private boolean mIsCharging; private boolean mIsPowerSaving; + /** + * Stores the result of the last call to {@link decideShouldRun}. + */ + private boolean lastDeterminedShouldRun = false; + public DeviceStateHolder(Context context, OnDeviceStateChangedListener listener) { + Log.v(TAG, "Created new instance"); ((SyncthingApp) context.getApplicationContext()).component().inject(this); mContext = context; mBroadcastManager = LocalBroadcastManager.getInstance(mContext); @@ -94,6 +100,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh } public void shutdown() { + Log.v(TAG, "Shutting down"); mBroadcastManager.unregisterReceiver(mReceiver); mPreferences.unregisterOnSharedPreferenceChangeListener(this); @@ -166,9 +173,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh mIsPowerSaving = intent.getBooleanExtra(EXTRA_IS_POWER_SAVING, mIsPowerSaving); Log.i(TAG, "State updated, allowed network connection: " + mIsAllowedNetworkConnection + ", charging: " + mIsCharging + ", power saving: " + mIsPowerSaving); - - updateWifiSsid(); - mListener.onDeviceStateChanged(); + updateShouldRunDecision(); } } @@ -183,15 +188,20 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh } } - public void refreshNetworkInfo() { - updateWifiSsid(); - mListener.onDeviceStateChanged(); + public void updateShouldRunDecision() { + // Check if the current conditions changed the result of decideShouldRun() + // compared to the last determined result. + boolean newShouldRun = decideShouldRun(); + if (newShouldRun != lastDeterminedShouldRun) { + mListener.onDeviceStateChanged(newShouldRun); + lastDeterminedShouldRun = newShouldRun; + } } /** * Determines if Syncthing should currently run. */ - boolean shouldRun() { + private boolean decideShouldRun() { boolean prefRespectPowerSaving = mPreferences.getBoolean("respect_battery_saving", true); if (prefRespectPowerSaving && mIsPowerSaving) return false; @@ -200,6 +210,7 @@ public class DeviceStateHolder implements SharedPreferences.OnSharedPreferenceCh boolean prefStopMobileData = mPreferences.getBoolean(Constants.PREF_SYNC_ONLY_WIFI, false); boolean prefStopNotCharging = mPreferences.getBoolean(Constants.PREF_SYNC_ONLY_CHARGING, false); + updateWifiSsid(); if (prefStopMobileData && !isWhitelistedNetworkConnection()) return false; diff --git a/app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java b/app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java index 01a7b2fd..8ea69a81 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java +++ b/app/src/main/java/com/nutomic/syncthingandroid/service/NotificationHandler.java @@ -26,6 +26,7 @@ public class NotificationHandler { private static final int ID_RESTART = 2; private static final int ID_STOP_BACKGROUND_WARNING = 3; private static final int ID_CRASH = 9; + private static final int ID_MISSING_PERM = 10; private static final String CHANNEL_PERSISTENT = "01_syncthing_persistent"; private static final String CHANNEL_INFO = "02_syncthing_notifications"; private static final String CHANNEL_PERSISTENT_WAITING = "03_syncthing_persistent_waiting"; @@ -171,6 +172,19 @@ public class NotificationHandler { mNotificationManager.notify(id, n); } + public void showStoragePermissionRevokedNotification() { + Intent intent = new Intent(mContext, FirstStartActivity.class); + Notification n = getNotificationBuilder(mInfoChannel) + .setContentTitle(mContext.getString(R.string.syncthing_terminated)) + .setContentText(mContext.getString(R.string.toast_write_storage_permission_required)) + .setSmallIcon(R.drawable.ic_stat_notify) + .setContentIntent(PendingIntent.getActivity(mContext, 0, intent, 0)) + .setAutoCancel(true) + .setOnlyAlertOnce(true) + .build(); + mNotificationManager.notify(ID_MISSING_PERM, n); + } + public void showRestartNotification() { Intent intent = new Intent(mContext, SyncthingService.class) .setAction(SyncthingService.ACTION_RESTART); diff --git a/app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java b/app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java index e25f31b7..bd904719 100644 --- a/app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java +++ b/app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java @@ -2,11 +2,14 @@ package com.nutomic.syncthingandroid.service; import android.app.Service; import android.content.Intent; +import android.content.pm.PackageManager; import android.content.SharedPreferences; +import android.Manifest; import android.os.AsyncTask; import android.os.Handler; import android.os.IBinder; import android.support.annotation.Nullable; +import android.support.v4.content.ContextCompat; import android.util.Log; import android.widget.Toast; @@ -91,7 +94,7 @@ public class SyncthingService extends Service { private ConfigXml mConfig; private RestApi mApi; private EventProcessor mEventProcessor; - private DeviceStateHolder mDeviceStateHolder; + private @Nullable DeviceStateHolder mDeviceStateHolder = null; private SyncthingRunnable mSyncthingRunnable; private Handler mHandler; @@ -113,6 +116,31 @@ public class SyncthingService extends Service { */ private boolean mStopScheduled = false; + /** + * True if the user granted the storage permission. + */ + private boolean mStoragePermissionGranted = false; + + /** + * Starts the native binary. + */ + @Override + public void onCreate() { + super.onCreate(); + PRNGFixes.apply(); + ((SyncthingApp) getApplication()).component().inject(this); + mHandler = new Handler(); + + /** + * If runtime permissions are revoked, android kills and restarts the service. + * see issue: https://github.com/syncthing/syncthing-android/issues/871 + * We need to recheck if we still have the storage permission. + */ + mStoragePermissionGranted = (ContextCompat.checkSelfPermission(this, + Manifest.permission.WRITE_EXTERNAL_STORAGE) == + PackageManager.PERMISSION_GRANTED); + } + /** * Handles intents, either {@link #ACTION_RESTART}, or intents having * {@link DeviceStateHolder#EXTRA_IS_ALLOWED_NETWORK_CONNECTION} or @@ -120,6 +148,18 @@ public class SyncthingService extends Service { */ @Override public int onStartCommand(Intent intent, int flags, int startId) { + if (!mStoragePermissionGranted) { + Log.e(TAG, "User revoked storage permission. Stopping service."); + if (mNotificationHandler != null) { + mNotificationHandler.showStoragePermissionRevokedNotification(); + } + stopSelf(); + return START_NOT_STICKY; + } + + mDeviceStateHolder = new DeviceStateHolder(SyncthingService.this, this::onUpdatedShouldRunDecision); + mNotificationHandler.updatePersistentNotification(this); + if (intent == null) return START_STICKY; @@ -136,35 +176,39 @@ public class SyncthingService extends Service { new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); }); } else if (ACTION_REFRESH_NETWORK_INFO.equals(intent.getAction())) { - mDeviceStateHolder.refreshNetworkInfo(); + mDeviceStateHolder.updateShouldRunDecision(); } return START_STICKY; } /** - * Checks according to preferences and charging/wifi state, whether syncthing should be enabled - * or not. - * - * Depending on the result, syncthing is started or stopped, and {@link #onApiChange} is - * called. + * After run conditions monitored by {@link DeviceStateHolder} changed and + * it had an influence on the decision to run/terminate syncthing, this + * function is called to notify this class to run/terminate the syncthing binary. + * {@link #onApiChange} is called while applying the decision change. */ - private void updateState() { - // Start syncthing. - if (mDeviceStateHolder.shouldRun()) { - if (mCurrentState == State.ACTIVE || mCurrentState == State.STARTING) { - mStopScheduled = false; - return; + private void onUpdatedShouldRunDecision(boolean shouldRun) { + if (shouldRun) { + // Start syncthing. + switch (mCurrentState) { + case DISABLED: + case INIT: + // HACK: Make sure there is no syncthing binary left running from an improper + // shutdown (eg Play Store update). + shutdown(State.INIT, () -> { + Log.i(TAG, "Starting syncthing according to current state and preferences after State.INIT"); + new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); + }); + break; + case STARTING: + case ACTIVE: + mStopScheduled = false; + break; + default: + break; } - - // HACK: Make sure there is no syncthing binary left running from an improper - // shutdown (eg Play Store update). - shutdown(State.INIT, () -> { - Log.i(TAG, "Starting syncthing according to current state and preferences"); - new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); - }); - } - // Stop syncthing. - else { + } else { + // Stop syncthing. if (mCurrentState == State.DISABLED) return; @@ -173,20 +217,6 @@ public class SyncthingService extends Service { } } - /** - * Starts the native binary. - */ - @Override - public void onCreate() { - super.onCreate(); - PRNGFixes.apply(); - ((SyncthingApp) getApplication()).component().inject(this); - mHandler = new Handler(); - - mDeviceStateHolder = new DeviceStateHolder(SyncthingService.this, this::updateState); - mNotificationHandler.updatePersistentNotification(this); - } - /** * Sets up the initial configuration, and updates the config when coming from an old * version. @@ -248,18 +278,26 @@ public class SyncthingService extends Service { */ @Override public void onDestroy() { - synchronized (mStateLock) { - if (mCurrentState == State.INIT || mCurrentState == State.STARTING) { - Log.i(TAG, "Delay shutting down service until initialisation of Syncthing finished"); - mStopScheduled = true; - - } else { - Log.i(TAG, "Shutting down service immediately"); - shutdown(State.DISABLED, () -> {}); + if (mStoragePermissionGranted) { + synchronized (mStateLock) { + if (mCurrentState == State.INIT || mCurrentState == State.STARTING) { + Log.i(TAG, "Delay shutting down synchting binary until initialisation finished"); + mStopScheduled = true; + } else { + Log.i(TAG, "Shutting down syncthing binary immediately"); + shutdown(State.DISABLED, () -> {}); + } } + } else { + // If the storage permission got revoked, we did not start the binary and + // are in State.INIT requiring an immediate shutdown of this service class. + Log.i(TAG, "Shutting down syncthing binary due to missing storage permission."); + shutdown(State.DISABLED, () -> {}); } - mDeviceStateHolder.shutdown(); + if (mDeviceStateHolder != null) { + mDeviceStateHolder.shutdown(); + } } /** @@ -277,7 +315,8 @@ public class SyncthingService extends Service { if (mApi != null) mApi.shutdown(); - mNotificationHandler.cancelPersistentNotification(this); + if (mNotificationHandler != null) + mNotificationHandler.cancelPersistentNotification(this); if (mSyncthingRunnable != null) { mSyncthingRunnable.killSyncthing(onKilledListener); diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index c87bf647..6c0e7ff6 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -578,6 +578,8 @@ Please report any problems you encounter via Github. Syncthing is disabled + Syncthing was terminated + Failed to create config file