Move some VPN logic out of ConnectivityService

This cleanup helps declutter ConnectivityService, and encapsulates the
always-on setting inside of Vpn instead of spreading it across two
classes.

In particular having the save code in one file and the load code in
another file was weird and I apologise for that.

Added a SystemServices wrapper for Settings.Secure and PendingIntent
calls to decouple some of the global state nastiness and make it
testable without forcing ConnectivityService to drive the load/save.

Test: runtest -x tests/net/java/com/android/server/ConnectivityServiceTest.java
Test: runtest -x tests/net/java/com/android/server/connectivity/VpnTest.java
Bug: 33159037
Change-Id: Ie2adb1c377adfcef0a5900dc866e6118f451b265

3 files changed, 133 insertions, 57 deletions

diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index fe0c17a94bf0..5b029d1cc007 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -3766,8 +3766,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 vpn.setAlwaysOnPackage(null, false);
                 return false;
             }
-
-            vpn.saveAlwaysOnPackage();
         }
         return true;
     }
@@ -3928,15 +3926,6 @@ public class ConnectivityService extends IConnectivityManager.Stub
             }
             userVpn = new Vpn(mHandler.getLooper(), mContext, mNetd, userId);
             mVpns.put(userId, userVpn);
-
-            final ContentResolver cr = mContext.getContentResolver();
-            String alwaysOnPackage = Settings.Secure.getStringForUser(cr,
-                    Settings.Secure.ALWAYS_ON_VPN_APP, userId);
-            final boolean alwaysOnLockdown = Settings.Secure.getIntForUser(cr,
-                    Settings.Secure.ALWAYS_ON_VPN_LOCKDOWN, /* default */ 0, userId) != 0;
-            if (alwaysOnPackage != null) {
-                userVpn.setAlwaysOnPackage(alwaysOnPackage, alwaysOnLockdown);
-            }
         }
         if (mUserManager.getUserInfo(userId).isPrimary() && LockdownVpnTracker.isEnabled()) {
             updateLockdownVpn();
diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java
index a5876ddda745..584994a785ec 100644
--- a/services/core/java/com/android/server/connectivity/Vpn.java
+++ b/services/core/java/com/android/server/connectivity/Vpn.java
@@ -32,7 +32,6 @@ import android.app.NotificationManager;
 import android.app.PendingIntent;
 import android.content.BroadcastReceiver;
 import android.content.ComponentName;
-import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.content.IntentFilter;
@@ -132,6 +131,7 @@ public class Vpn {
     private NetworkAgent mNetworkAgent;
     private final Looper mLooper;
     private final NetworkCapabilities mNetworkCapabilities;
+    private final SystemServices mSystemServices;
 
     /**
      * Whether to keep the connection active after rebooting, or upgrading or reinstalling. This
@@ -199,7 +199,7 @@ public class Vpn {
                         final boolean isPackageRemoved = !intent.getBooleanExtra(
                                 Intent.EXTRA_REPLACING, false);
                         if (isPackageRemoved) {
-                            setAndSaveAlwaysOnPackage(null, false);
+                            setAlwaysOnPackage(null, false);
                         }
                         break;
                 }
@@ -210,11 +210,18 @@ public class Vpn {
     private boolean mIsPackageIntentReceiverRegistered = false;
 
     public Vpn(Looper looper, Context context, INetworkManagementService netService,
-            int userHandle) {
+            @UserIdInt int userHandle) {
+        this(looper, context, netService, userHandle, new SystemServices(context));
+    }
+
+    @VisibleForTesting
+    protected Vpn(Looper looper, Context context, INetworkManagementService netService,
+            int userHandle, SystemServices systemServices) {
         mContext = context;
         mNetd = netService;
         mUserHandle = userHandle;
         mLooper = looper;
+        mSystemServices = systemServices;
 
         mPackage = VpnConfig.LEGACY_VPN;
         mOwnerUID = getAppUid(mPackage, mUserHandle);
@@ -230,6 +237,8 @@ public class Vpn {
         mNetworkCapabilities = new NetworkCapabilities();
         mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
         mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
+
+        loadAlwaysOnPackage();
     }
 
     /**
@@ -268,6 +277,26 @@ public class Vpn {
      */
     public synchronized boolean setAlwaysOnPackage(String packageName, boolean lockdown) {
         enforceControlPermissionOrInternalCaller();
+
+        if (setAlwaysOnPackageInternal(packageName, lockdown)) {
+            saveAlwaysOnPackage();
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * Configures an always-on VPN connection through a specific application, the same as
+     * {@link #setAlwaysOnPackage}.
+     *
+     * Does not perform permission checks. Does not persist any of the changes to storage.
+     *
+     * @param packageName the package to designate as always-on VPN supplier.
+     * @param lockdown whether to prevent traffic outside of a VPN, for example while connecting.
+     * @return {@code true} if the package has been set as always-on, {@code false} otherwise.
+     */
+    @GuardedBy("this")
+    private boolean setAlwaysOnPackageInternal(String packageName, boolean lockdown) {
         if (VpnConfig.LEGACY_VPN.equals(packageName)) {
             Log.w(TAG, "Not setting legacy VPN \"" + packageName + "\" as always-on.");
             return false;
@@ -340,13 +369,13 @@ public class Vpn {
     /**
      * Save the always-on package and lockdown config into Settings.Secure
      */
-    public synchronized void saveAlwaysOnPackage() {
+    @GuardedBy("this")
+    private void saveAlwaysOnPackage() {
         final long token = Binder.clearCallingIdentity();
         try {
-            final ContentResolver cr = mContext.getContentResolver();
-            Settings.Secure.putStringForUser(cr, Settings.Secure.ALWAYS_ON_VPN_APP,
+            mSystemServices.settingsSecurePutStringForUser(Settings.Secure.ALWAYS_ON_VPN_APP,
                     getAlwaysOnPackage(), mUserHandle);
-            Settings.Secure.putIntForUser(cr, Settings.Secure.ALWAYS_ON_VPN_LOCKDOWN,
+            mSystemServices.settingsSecurePutIntForUser(Settings.Secure.ALWAYS_ON_VPN_LOCKDOWN,
                     (mLockdown ? 1 : 0), mUserHandle);
         } finally {
             Binder.restoreCallingIdentity(token);
@@ -354,18 +383,19 @@ public class Vpn {
     }
 
     /**
-     * Set and save always-on package and lockdown config
-     * @see Vpn#setAlwaysOnPackage(String, boolean)
-     * @see Vpn#saveAlwaysOnPackage()
-     *
-     * @return result of Vpn#setAndSaveAlwaysOnPackage(String, boolean)
+     * Load the always-on package and lockdown config from Settings.Secure
      */
-    private synchronized boolean setAndSaveAlwaysOnPackage(String packageName, boolean lockdown) {
-        if (setAlwaysOnPackage(packageName, lockdown)) {
-            saveAlwaysOnPackage();
-            return true;
-        } else {
-            return false;
+    @GuardedBy("this")
+    private void loadAlwaysOnPackage() {
+        final long token = Binder.clearCallingIdentity();
+        try {
+            final String alwaysOnPackage = mSystemServices.settingsSecureGetStringForUser(
+                    Settings.Secure.ALWAYS_ON_VPN_APP, mUserHandle);
+            final boolean alwaysOnLockdown = mSystemServices.settingsSecureGetIntForUser(
+                    Settings.Secure.ALWAYS_ON_VPN_LOCKDOWN, 0 /*default*/, mUserHandle) != 0;
+            setAlwaysOnPackageInternal(alwaysOnPackage, alwaysOnLockdown);
+        } finally {
+            Binder.restoreCallingIdentity(token);
         }
     }
 
@@ -1257,11 +1287,7 @@ public class Vpn {
 
     private void updateAlwaysOnNotification(DetailedState networkState) {
         final boolean visible = (mAlwaysOn && networkState != DetailedState.CONNECTED);
-        updateAlwaysOnNotificationInternal(visible);
-    }
 
-    @VisibleForTesting
-    protected void updateAlwaysOnNotificationInternal(boolean visible) {
         final UserHandle user = UserHandle.of(mUserHandle);
         final long token = Binder.clearCallingIdentity();
         try {
@@ -1271,10 +1297,8 @@ public class Vpn {
                 return;
             }
             final Intent intent = new Intent(Settings.ACTION_VPN_SETTINGS);
-            final PendingIntent configIntent = PendingIntent.getActivityAsUser(
-                    mContext, /* request */ 0, intent,
-                    PendingIntent.FLAG_IMMUTABLE | PendingIntent.FLAG_UPDATE_CURRENT,
-                    null, user);
+            final PendingIntent configIntent = mSystemServices.pendingIntentGetActivityAsUser(
+                    intent, PendingIntent.FLAG_IMMUTABLE | PendingIntent.FLAG_UPDATE_CURRENT, user);
             final Notification.Builder builder = new Notification.Builder(mContext)
                     .setDefaults(0)
                     .setSmallIcon(R.drawable.vpn_connected)
@@ -1292,6 +1316,58 @@ public class Vpn {
         }
     }
 
+    /**
+     * Facade for system service calls that change, or depend on, state outside of
+     * {@link ConnectivityService} and have hard-to-mock interfaces.
+     *
+     * @see com.android.server.connectivity.VpnTest
+     */
+    @VisibleForTesting
+    public static class SystemServices {
+        private final Context mContext;
+
+        public SystemServices(@NonNull Context context) {
+            mContext = context;
+        }
+
+        /**
+         * @see PendingIntent#getActivityAsUser()
+         */
+        public PendingIntent pendingIntentGetActivityAsUser(
+                Intent intent, int flags, UserHandle user) {
+            return PendingIntent.getActivityAsUser(mContext, 0 /*request*/, intent, flags,
+                    null /*options*/, user);
+        }
+
+        /**
+         * @see Settings.Secure#putStringForUser
+         */
+        public void settingsSecurePutStringForUser(String key, String value, int userId) {
+            Settings.Secure.putStringForUser(mContext.getContentResolver(), key, value, userId);
+        }
+
+        /**
+         * @see Settings.Secure#putIntForUser
+         */
+        public void settingsSecurePutIntForUser(String key, int value, int userId) {
+            Settings.Secure.putIntForUser(mContext.getContentResolver(), key, value, userId);
+        }
+
+        /**
+         * @see Settings.Secure#getStringForUser
+         */
+        public String settingsSecureGetStringForUser(String key, int userId) {
+            return Settings.Secure.getStringForUser(mContext.getContentResolver(), key, userId);
+        }
+
+        /**
+         * @see Settings.Secure#getIntForUser
+         */
+        public int settingsSecureGetIntForUser(String key, int def, int userId) {
+            return Settings.Secure.getIntForUser(mContext.getContentResolver(), key, def, userId);
+        }
+    }
+
     private native int jniCreate(int mtu);
     private native String jniGetName(int tun);
     private native int jniSetAddresses(String interfaze, String addresses);
diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java
index b51b2771db16..efe6fec6fc7d 100644
--- a/tests/net/java/com/android/server/connectivity/VpnTest.java
+++ b/tests/net/java/com/android/server/connectivity/VpnTest.java
@@ -27,10 +27,12 @@ import android.annotation.UserIdInt;
 import android.app.AppOpsManager;
 import android.app.NotificationManager;
 import android.content.Context;
+import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageManager;
 import android.content.pm.UserInfo;
 import android.net.NetworkInfo.DetailedState;
 import android.net.UidRange;
+import android.os.Build;
 import android.os.INetworkManagementService;
 import android.os.Looper;
 import android.os.UserHandle;
@@ -45,6 +47,7 @@ import java.util.Arrays;
 import java.util.Map;
 import java.util.Set;
 
+import org.mockito.Answers;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InOrder;
 import org.mockito.Mock;
@@ -87,12 +90,13 @@ public class VpnTest extends AndroidTestCase {
         }
     }
 
-    @Mock private Context mContext;
+    @Mock(answer = Answers.RETURNS_DEEP_STUBS) private Context mContext;
     @Mock private UserManager mUserManager;
     @Mock private PackageManager mPackageManager;
     @Mock private INetworkManagementService mNetService;
     @Mock private AppOpsManager mAppOps;
     @Mock private NotificationManager mNotificationManager;
+    @Mock private Vpn.SystemServices mSystemServices;
 
     @Override
     public void setUp() throws Exception {
@@ -104,6 +108,12 @@ public class VpnTest extends AndroidTestCase {
         when(mContext.getSystemService(eq(Context.APP_OPS_SERVICE))).thenReturn(mAppOps);
         when(mContext.getSystemService(eq(Context.NOTIFICATION_SERVICE)))
                 .thenReturn(mNotificationManager);
+
+        // Used by {@link Notification.Builder}
+        ApplicationInfo applicationInfo = new ApplicationInfo();
+        applicationInfo.targetSdkVersion = Build.VERSION_CODES.CUR_DEVELOPMENT;
+        when(mContext.getApplicationInfo()).thenReturn(applicationInfo);
+
         doNothing().when(mNetService).registerObserver(any());
     }
 
@@ -111,7 +121,7 @@ public class VpnTest extends AndroidTestCase {
     public void testRestrictedProfilesAreAddedToVpn() {
         setMockedUsers(primaryUser, secondaryUser, restrictedProfileA, restrictedProfileB);
 
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         final Set<UidRange> ranges = vpn.createUserAndRestrictedProfilesRanges(primaryUser.id,
                 null, null);
 
@@ -125,7 +135,7 @@ public class VpnTest extends AndroidTestCase {
     public void testManagedProfilesAreNotAddedToVpn() {
         setMockedUsers(primaryUser, managedProfileA);
 
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         final Set<UidRange> ranges = vpn.createUserAndRestrictedProfilesRanges(primaryUser.id,
                 null, null);
 
@@ -138,7 +148,7 @@ public class VpnTest extends AndroidTestCase {
     public void testAddUserToVpnOnlyAddsOneUser() {
         setMockedUsers(primaryUser, restrictedProfileA, managedProfileA);
 
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         final Set<UidRange> ranges = new ArraySet<>();
         vpn.addUserToRanges(ranges, primaryUser.id, null, null);
 
@@ -149,7 +159,7 @@ public class VpnTest extends AndroidTestCase {
 
     @SmallTest
     public void testUidWhiteAndBlacklist() throws Exception {
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         final UidRange user = UidRange.createForUser(primaryUser.id);
         final String[] packages = {PKGS[0], PKGS[1], PKGS[2]};
 
@@ -174,7 +184,7 @@ public class VpnTest extends AndroidTestCase {
 
     @SmallTest
     public void testLockdownChangingPackage() throws Exception {
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         final UidRange user = UidRange.createForUser(primaryUser.id);
 
         // Default state.
@@ -209,7 +219,7 @@ public class VpnTest extends AndroidTestCase {
 
     @SmallTest
     public void testLockdownAddingAProfile() throws Exception {
-        final Vpn vpn = spyVpn(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         setMockedUsers(primaryUser);
 
         // Make a copy of the restricted profile, as we're going to mark it deleted halfway through.
@@ -249,40 +259,41 @@ public class VpnTest extends AndroidTestCase {
 
     @SmallTest
     public void testNotificationShownForAlwaysOnApp() {
-        final Vpn vpn = spyVpn(primaryUser.id);
-        final InOrder order = inOrder(vpn);
+        final UserHandle userHandle = UserHandle.of(primaryUser.id);
+        final Vpn vpn = createVpn(primaryUser.id);
         setMockedUsers(primaryUser);
 
+        final InOrder order = inOrder(mNotificationManager);
+
         // Don't show a notification for regular disconnected states.
         vpn.updateState(DetailedState.DISCONNECTED, TAG);
-        order.verify(vpn).updateAlwaysOnNotificationInternal(false);
+        order.verify(mNotificationManager, atLeastOnce())
+                .cancelAsUser(anyString(), anyInt(), eq(userHandle));
 
         // Start showing a notification for disconnected once always-on.
         vpn.setAlwaysOnPackage(PKGS[0], false);
-        order.verify(vpn).updateAlwaysOnNotificationInternal(true);
+        order.verify(mNotificationManager)
+                .notifyAsUser(anyString(), anyInt(), any(), eq(userHandle));
 
         // Stop showing the notification once connected.
         vpn.updateState(DetailedState.CONNECTED, TAG);
-        order.verify(vpn).updateAlwaysOnNotificationInternal(false);
+        order.verify(mNotificationManager).cancelAsUser(anyString(), anyInt(), eq(userHandle));
 
         // Show the notification if we disconnect again.
         vpn.updateState(DetailedState.DISCONNECTED, TAG);
-        order.verify(vpn).updateAlwaysOnNotificationInternal(true);
+        order.verify(mNotificationManager)
+                .notifyAsUser(anyString(), anyInt(), any(), eq(userHandle));
 
         // Notification should be cleared after unsetting always-on package.
         vpn.setAlwaysOnPackage(null, false);
-        order.verify(vpn).updateAlwaysOnNotificationInternal(false);
+        order.verify(mNotificationManager).cancelAsUser(anyString(), anyInt(), eq(userHandle));
     }
 
     /**
      * Mock some methods of vpn object.
      */
-    private Vpn spyVpn(@UserIdInt int userId) {
-        final Vpn vpn = spy(new Vpn(Looper.myLooper(), mContext, mNetService, userId));
-
-        // Block calls to the NotificationManager or PendingIntent#getActivity.
-        doNothing().when(vpn).updateAlwaysOnNotificationInternal(anyBoolean());
-        return vpn;
+    private Vpn createVpn(@UserIdInt int userId) {
+        return new Vpn(Looper.myLooper(), mContext, mNetService, userId, mSystemServices);
     }
 
     private static void assertBlocked(Vpn vpn, int... uids) {