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
diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
index fe0c17a..5b029d1 100644
--- a/services/core/java/com/android/server/ConnectivityService.java
+++ b/services/core/java/com/android/server/ConnectivityService.java
@@ -3766,8 +3766,6 @@
vpn.setAlwaysOnPackage(null, false);
return false;
}
-
- vpn.saveAlwaysOnPackage();
}
return true;
}
@@ -3928,15 +3926,6 @@
}
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 a5876dd..584994a 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.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 @@
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 @@
final boolean isPackageRemoved = !intent.getBooleanExtra(
Intent.EXTRA_REPLACING, false);
if (isPackageRemoved) {
- setAndSaveAlwaysOnPackage(null, false);
+ setAlwaysOnPackage(null, false);
}
break;
}
@@ -210,11 +210,18 @@
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 @@
mNetworkCapabilities = new NetworkCapabilities();
mNetworkCapabilities.addTransportType(NetworkCapabilities.TRANSPORT_VPN);
mNetworkCapabilities.removeCapability(NetworkCapabilities.NET_CAPABILITY_NOT_VPN);
+
+ loadAlwaysOnPackage();
}
/**
@@ -268,6 +277,26 @@
*/
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 @@
/**
* 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 @@
}
/**
- * 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 @@
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 @@
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 @@
}
}
+ /**
+ * 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 b51b277..efe6fec 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.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.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 @@
}
}
- @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 @@
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 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 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 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 @@
@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 @@
@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 @@
@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 @@
@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) {