From d8bea3bb9057645cdd05fa6f434f1d99efbefee5 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 2 Dec 2019 18:59:27 +0900 Subject: [PATCH 1/6] [NS A26] Move available callbacks out of the rematch computation Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I3a804a9f6eaf50a3995eaaf6469a1c2b9387be14 --- .../android/server/ConnectivityService.java | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 478b87c010..2852a21a0a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6323,12 +6323,34 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + static class RequestReassignment { + @NonNull public final NetworkRequestInfo mRequest; + @Nullable public final NetworkAgentInfo mOldNetwork; + @Nullable public final NetworkAgentInfo mNewNetwork; + RequestReassignment(@NonNull final NetworkRequestInfo request, + @Nullable final NetworkAgentInfo oldNetwork, + @Nullable final NetworkAgentInfo newNetwork) { + mRequest = request; + mOldNetwork = oldNetwork; + mNewNetwork = newNetwork; + } + } + @NonNull private final Set mRematchedNetworks = new ArraySet<>(); + @NonNull private final List mReassignments = new ArrayList<>(); @NonNull Iterable getRematchedNetworks() { return mRematchedNetworks; } + @NonNull Iterable getRequestReassignments() { + return mReassignments; + } + + void addRequestReassignment(@NonNull final RequestReassignment reassignment) { + mReassignments.add(reassignment); + } + void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { mRematchedNetworks.add(network); } @@ -6417,7 +6439,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // Find and migrate to this Network any NetworkRequests for // which this network is now the best. final ArrayList removedRequests = new ArrayList<>(); - final ArrayList addedRequests = new ArrayList<>(); for (final Map.Entry entry : reassignedRequests.entrySet()) { final NetworkRequestInfo nri = entry.getKey(); @@ -6440,7 +6461,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!newSatisfier.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); } - addedRequests.add(nri); + changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( + nri, previousSatisfier, newSatisfier)); // Tell NetworkProviders about the new score, so they can stop // trying to connect if they know they cannot match it. // TODO - this could get expensive if we have a lot of requests for this @@ -6508,10 +6530,6 @@ public class ConnectivityService extends IConnectivityManager.Stub "BUG: %s changed score during rematch: %d -> %d", newNetwork.name(), score, newNetwork.getCurrentScore())); } - - // Notify requested networks are available after the default net is switched, but - // before LegacyTypeTracker sends legacy broadcasts - for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); } /** @@ -6540,6 +6558,15 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo newDefaultNetwork = getDefaultNetwork(); + // Notify requested networks are available after the default net is switched, but + // before LegacyTypeTracker sends legacy broadcasts + for (final NetworkReassignment.RequestReassignment event : + changes.getRequestReassignments()) { + if (null != event.mNewNetwork) { + notifyNetworkAvailable(event.mNewNetwork, event.mRequest); + } + } + for (final NetworkReassignment.NetworkBgStatePair event : changes.getRematchedNetworks()) { // Process listen requests and update capabilities if the background state has // changed for this network. For consistency with previous behavior, send onLost From 0c7b9a9eb7d7790d590b0e7b8eaa1471bd82ffff Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 14:07:57 +0900 Subject: [PATCH 2/6] [NS A27] Remove useless logs and a useless var These logs haven't found a bug in a long time and we now have some structural guarantees that the conditions they check for can't happen (like the checks that everything is happening on the same thread). Maybe we'll reinstate similar checks later, but for now they are in the way and removing them is a small sacrifice for the intended benefit. The local was simply not used any more. Test: FrameworksNetTests Change-Id: If8c8d1f3eb883ffcf0fbdb70824b87dd70da507c --- .../android/server/ConnectivityService.java | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2852a21a0a..31edc5606a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6426,19 +6426,13 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, newNetwork.isBackgroundNetwork())); - final int score = newNetwork.getCurrentScore(); - if (VDBG || DDBG) log("rematching " + newNetwork.name()); final ArrayMap reassignedRequests = computeRequestReassignmentForNetwork(newNetwork); - NetworkCapabilities nc = newNetwork.networkCapabilities; - if (VDBG) log(" network has: " + nc); - // Find and migrate to this Network any NetworkRequests for // which this network is now the best. - final ArrayList removedRequests = new ArrayList<>(); for (final Map.Entry entry : reassignedRequests.entrySet()) { final NetworkRequestInfo nri = entry.getKey(); @@ -6452,7 +6446,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } previousSatisfier.removeRequest(nri.request.requestId); previousSatisfier.lingerRequest(nri.request, now, mLingerDelayMs); - removedRequests.add(previousSatisfier); } else { if (VDBG || DDBG) log(" accepting network in place of null"); } @@ -6519,17 +6512,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // Have a new default network, release the transition wakelock in scheduleReleaseNetworkTransitionWakelock(); } - - if (!newNetwork.networkCapabilities.equalRequestableCapabilities(nc)) { - Slog.wtf(TAG, String.format( - "BUG: %s changed requestable capabilities during rematch: %s -> %s", - newNetwork.name(), nc, newNetwork.networkCapabilities)); - } - if (newNetwork.getCurrentScore() != score) { - Slog.wtf(TAG, String.format( - "BUG: %s changed score during rematch: %d -> %d", - newNetwork.name(), score, newNetwork.getCurrentScore())); - } } /** From df936cf1a706a3a798dbf74e164060f1063fe662 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 5 Nov 2019 12:56:25 -0800 Subject: [PATCH 3/6] Add basic logic for profile-based VPNs This change adds stubs for the Platform built-in VPNs, along with implementing some basic permissions checks. Bug: 144246837 Test: FrameworksNetTests passing, new tests added Change-Id: I68d2293fc1468544f0d9f64d02ea7e1c80c8d18c --- .../android/net/IConnectivityManager.aidl | 8 + .../android/server/ConnectivityService.java | 81 ++++++- .../android/server/connectivity/VpnTest.java | 206 +++++++++++++++++- 3 files changed, 284 insertions(+), 11 deletions(-) diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index 3e9e7faccb..b050e479d2 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -120,6 +120,14 @@ interface IConnectivityManager ParcelFileDescriptor establishVpn(in VpnConfig config); + boolean provisionVpnProfile(in VpnProfile profile, String packageName); + + void deleteVpnProfile(String packageName); + + void startVpnProfile(String packageName); + + void stopVpnProfile(String packageName); + VpnConfig getVpnConfig(int userId); @UnsupportedAppUsage diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 478b87c010..caf185145f 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -4293,7 +4293,7 @@ public class ConnectivityService extends IConnectivityManager.Stub throwIfLockdownEnabled(); Vpn vpn = mVpns.get(userId); if (vpn != null) { - return vpn.prepare(oldPackage, newPackage); + return vpn.prepare(oldPackage, newPackage, false); } else { return false; } @@ -4341,6 +4341,78 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** + * Stores the given VPN profile based on the provisioning package name. + * + *

If there is already a VPN profile stored for the provisioning package, this call will + * overwrite the profile. + * + *

This is designed to serve the VpnManager only; settings-based VPN profiles are managed + * exclusively by the Settings app, and passed into the platform at startup time. + * + * @return {@code true} if user consent has already been granted, {@code false} otherwise. + * @hide + */ + @Override + public boolean provisionVpnProfile(@NonNull VpnProfile profile, @NonNull String packageName) { + final int user = UserHandle.getUserId(Binder.getCallingUid()); + synchronized (mVpns) { + return mVpns.get(user).provisionVpnProfile(packageName, profile, mKeyStore); + } + } + + /** + * Deletes the stored VPN profile for the provisioning package + * + *

If there are no profiles for the given package, this method will silently succeed. + * + *

This is designed to serve the VpnManager only; settings-based VPN profiles are managed + * exclusively by the Settings app, and passed into the platform at startup time. + * + * @hide + */ + @Override + public void deleteVpnProfile(@NonNull String packageName) { + final int user = UserHandle.getUserId(Binder.getCallingUid()); + synchronized (mVpns) { + mVpns.get(user).deleteVpnProfile(packageName, mKeyStore); + } + } + + /** + * Starts the VPN based on the stored profile for the given package + * + *

This is designed to serve the VpnManager only; settings-based VPN profiles are managed + * exclusively by the Settings app, and passed into the platform at startup time. + * + * @throws IllegalArgumentException if no profile was found for the given package name. + * @hide + */ + @Override + public void startVpnProfile(@NonNull String packageName) { + final int user = UserHandle.getUserId(Binder.getCallingUid()); + synchronized (mVpns) { + throwIfLockdownEnabled(); + mVpns.get(user).startVpnProfile(packageName, mKeyStore); + } + } + + /** + * Stops the Platform VPN if the provided package is running one. + * + *

This is designed to serve the VpnManager only; settings-based VPN profiles are managed + * exclusively by the Settings app, and passed into the platform at startup time. + * + * @hide + */ + @Override + public void stopVpnProfile(@NonNull String packageName) { + final int user = UserHandle.getUserId(Binder.getCallingUid()); + synchronized (mVpns) { + mVpns.get(user).stopVpnProfile(packageName); + } + } + /** * Start legacy VPN, controlling native daemons as needed. Creates a * secondary thread to perform connection work, returning quickly. @@ -4544,6 +4616,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** + * Throws if there is any currently running, always-on Legacy VPN. + * + *

The LockdownVpnTracker and mLockdownEnabled both track whether an always-on Legacy VPN is + * running across the entire system. Tracking for app-based VPNs is done on a per-user, + * per-package basis in Vpn.java + */ @GuardedBy("mVpns") private void throwIfLockdownEnabled() { if (mLockdownEnabled) { diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index ce50bef53d..084ec73302 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -28,11 +28,11 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING; import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; -import static android.net.RouteInfo.RTN_UNREACHABLE; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.AdditionalMatchers.aryEq; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -43,6 +43,7 @@ import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -58,21 +59,20 @@ import android.content.pm.ServiceInfo; import android.content.pm.UserInfo; import android.content.res.Resources; import android.net.ConnectivityManager; -import android.net.IpPrefix; -import android.net.LinkProperties; import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkInfo.DetailedState; -import android.net.RouteInfo; import android.net.UidRange; import android.net.VpnService; import android.os.Build.VERSION_CODES; import android.os.Bundle; import android.os.INetworkManagementService; import android.os.Looper; -import android.os.SystemClock; +import android.os.Process; import android.os.UserHandle; import android.os.UserManager; +import android.security.Credentials; +import android.security.KeyStore; import android.util.ArrayMap; import android.util.ArraySet; @@ -81,6 +81,7 @@ import androidx.test.runner.AndroidJUnit4; import com.android.internal.R; import com.android.internal.net.VpnConfig; +import com.android.internal.net.VpnProfile; import org.junit.Before; import org.junit.Test; @@ -90,9 +91,6 @@ import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import java.net.Inet4Address; -import java.net.Inet6Address; -import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -124,6 +122,8 @@ public class VpnTest { managedProfileA.profileGroupId = primaryUser.id; } + static final String TEST_VPN_PKG = "com.dummy.vpn"; + /** * Names and UIDs for some fake packages. Important points: * - UID is ordered increasing. @@ -148,6 +148,8 @@ public class VpnTest { @Mock private NotificationManager mNotificationManager; @Mock private Vpn.SystemServices mSystemServices; @Mock private ConnectivityManager mConnectivityManager; + @Mock private KeyStore mKeyStore; + private final VpnProfile mVpnProfile = new VpnProfile("key"); @Before public void setUp() throws Exception { @@ -166,6 +168,7 @@ public class VpnTest { when(mContext.getString(R.string.config_customVpnAlwaysOnDisconnectedDialogComponent)) .thenReturn(Resources.getSystem().getString( R.string.config_customVpnAlwaysOnDisconnectedDialogComponent)); + when(mSystemServices.isCallerSystem()).thenReturn(true); // Used by {@link Notification.Builder} ApplicationInfo applicationInfo = new ApplicationInfo(); @@ -175,6 +178,10 @@ public class VpnTest { .thenReturn(applicationInfo); doNothing().when(mNetService).registerObserver(any()); + + // Deny all appops by default. + when(mAppOps.noteOpNoThrow(anyInt(), anyInt(), anyString())) + .thenReturn(AppOpsManager.MODE_IGNORED); } @Test @@ -464,12 +471,12 @@ public class VpnTest { order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(entireUser)); // When a new VPN package is set the rules should change to cover that package. - vpn.prepare(null, PKGS[0]); + vpn.prepare(null, PKGS[0], false /* isPlatformVpn */); order.verify(mNetService).setAllowOnlyVpnForUids(eq(false), aryEq(entireUser)); order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(exceptPkg0)); // When that VPN package is unset, everything should be undone again in reverse. - vpn.prepare(null, VpnConfig.LEGACY_VPN); + vpn.prepare(null, VpnConfig.LEGACY_VPN, false /* isPlatformVpn */); order.verify(mNetService).setAllowOnlyVpnForUids(eq(false), aryEq(exceptPkg0)); order.verify(mNetService).setAllowOnlyVpnForUids(eq(true), aryEq(entireUser)); } @@ -631,6 +638,185 @@ public class VpnTest { assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED)); } + /** + * The profile name should NOT change between releases for backwards compatibility + * + *

If this is changed between releases, the {@link Vpn#getVpnProfilePrivileged()} method MUST + * be updated to ensure backward compatibility. + */ + @Test + public void testGetProfileNameForPackage() throws Exception { + final Vpn vpn = createVpn(primaryUser.id); + setMockedUsers(primaryUser); + + final String expected = Credentials.PLATFORM_VPN + primaryUser.id + "_" + TEST_VPN_PKG; + assertEquals(expected, vpn.getProfileNameForPackage(TEST_VPN_PKG)); + } + + private Vpn createVpnAndSetupUidChecks(int... grantedOps) throws Exception { + final Vpn vpn = createVpn(primaryUser.id); + setMockedUsers(primaryUser); + + when(mPackageManager.getPackageUidAsUser(eq(TEST_VPN_PKG), anyInt())) + .thenReturn(Process.myUid()); + + for (final int op : grantedOps) { + when(mAppOps.noteOpNoThrow(op, Process.myUid(), TEST_VPN_PKG)) + .thenReturn(AppOpsManager.MODE_ALLOWED); + } + + return vpn; + } + + private void checkProvisionVpnProfile(Vpn vpn, boolean expectedResult, int... checkedOps) { + assertEquals(expectedResult, vpn.provisionVpnProfile(TEST_VPN_PKG, mVpnProfile, mKeyStore)); + + // The profile should always be stored, whether or not consent has been previously granted. + verify(mKeyStore) + .put( + eq(vpn.getProfileNameForPackage(TEST_VPN_PKG)), + eq(mVpnProfile.encode()), + eq(Process.SYSTEM_UID), + eq(0)); + + for (final int checkedOp : checkedOps) { + verify(mAppOps).noteOpNoThrow(checkedOp, Process.myUid(), TEST_VPN_PKG); + } + } + + @Test + public void testProvisionVpnProfilePreconsented() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + checkProvisionVpnProfile( + vpn, true /* expectedResult */, AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + } + + @Test + public void testProvisionVpnProfileNotPreconsented() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + // Expect that both the ACTIVATE_VPN and ACTIVATE_PLATFORM_VPN were tried, but the caller + // had neither. + checkProvisionVpnProfile(vpn, false /* expectedResult */, + AppOpsManager.OP_ACTIVATE_PLATFORM_VPN, AppOpsManager.OP_ACTIVATE_VPN); + } + + @Test + public void testProvisionVpnProfileVpnServicePreconsented() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_VPN); + + checkProvisionVpnProfile(vpn, true /* expectedResult */, AppOpsManager.OP_ACTIVATE_VPN); + } + + @Test + public void testProvisionVpnProfileTooLarge() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + final VpnProfile bigProfile = new VpnProfile(""); + bigProfile.name = new String(new byte[Vpn.MAX_VPN_PROFILE_SIZE_BYTES + 1]); + + try { + vpn.provisionVpnProfile(TEST_VPN_PKG, bigProfile, mKeyStore); + fail("Expected IAE due to profile size"); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testDeleteVpnProfile() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + vpn.deleteVpnProfile(TEST_VPN_PKG, mKeyStore); + + verify(mKeyStore) + .delete(eq(vpn.getProfileNameForPackage(TEST_VPN_PKG)), eq(Process.SYSTEM_UID)); + } + + @Test + public void testGetVpnProfilePrivileged() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + when(mKeyStore.get(vpn.getProfileNameForPackage(TEST_VPN_PKG))) + .thenReturn(new VpnProfile("").encode()); + + vpn.getVpnProfilePrivileged(TEST_VPN_PKG, mKeyStore); + + verify(mKeyStore).get(eq(vpn.getProfileNameForPackage(TEST_VPN_PKG))); + } + + @Test + public void testStartVpnProfile() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + when(mKeyStore.get(vpn.getProfileNameForPackage(TEST_VPN_PKG))) + .thenReturn(mVpnProfile.encode()); + + vpn.startVpnProfile(TEST_VPN_PKG, mKeyStore); + + verify(mKeyStore).get(eq(vpn.getProfileNameForPackage(TEST_VPN_PKG))); + verify(mAppOps) + .noteOpNoThrow( + eq(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG)); + } + + @Test + public void testStartVpnProfileVpnServicePreconsented() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_VPN); + + when(mKeyStore.get(vpn.getProfileNameForPackage(TEST_VPN_PKG))) + .thenReturn(mVpnProfile.encode()); + + vpn.startVpnProfile(TEST_VPN_PKG, mKeyStore); + + // Verify that the the ACTIVATE_VPN appop was checked, but no error was thrown. + verify(mAppOps).noteOpNoThrow(AppOpsManager.OP_ACTIVATE_VPN, Process.myUid(), TEST_VPN_PKG); + } + + @Test + public void testStartVpnProfileNotConsented() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(); + + try { + vpn.startVpnProfile(TEST_VPN_PKG, mKeyStore); + fail("Expected failure due to no user consent"); + } catch (SecurityException expected) { + } + + // Verify both appops were checked. + verify(mAppOps) + .noteOpNoThrow( + eq(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG)); + verify(mAppOps).noteOpNoThrow(AppOpsManager.OP_ACTIVATE_VPN, Process.myUid(), TEST_VPN_PKG); + + // Keystore should never have been accessed. + verify(mKeyStore, never()).get(any()); + } + + @Test + public void testStartVpnProfileMissingProfile() throws Exception { + final Vpn vpn = createVpnAndSetupUidChecks(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN); + + when(mKeyStore.get(vpn.getProfileNameForPackage(TEST_VPN_PKG))).thenReturn(null); + + try { + vpn.startVpnProfile(TEST_VPN_PKG, mKeyStore); + fail("Expected failure due to missing profile"); + } catch (IllegalArgumentException expected) { + } + + verify(mKeyStore).get(vpn.getProfileNameForPackage(TEST_VPN_PKG)); + verify(mAppOps) + .noteOpNoThrow( + eq(AppOpsManager.OP_ACTIVATE_PLATFORM_VPN), + eq(Process.myUid()), + eq(TEST_VPN_PKG)); + } + /** * Mock some methods of vpn object. */ From 79ea64f963ad0f7ef8331acadc5cd2f526f74b8c Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Thu, 12 Dec 2019 23:38:36 -0800 Subject: [PATCH 4/6] Add VpnManager calls to ConnectivityService This commit adds the relevant calls to ConnectivityService for the VpnManager API to be functional Bug: 144246837 Test: VpnManagerTest updated, FrameworksNetTests passing Change-Id: I446a8595e3583a842a7f89c4f8d74526a85e311c --- .../net/java/android/net/VpnManagerTest.java | 66 +++++++++++++------ 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/tests/net/java/android/net/VpnManagerTest.java b/tests/net/java/android/net/VpnManagerTest.java index 655c4d1185..97551c94e2 100644 --- a/tests/net/java/android/net/VpnManagerTest.java +++ b/tests/net/java/android/net/VpnManagerTest.java @@ -16,13 +16,21 @@ package android.net; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import android.test.mock.MockContext; import androidx.test.filters.SmallTest; import androidx.test.runner.AndroidJUnit4; +import com.android.internal.net.VpnProfile; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -31,7 +39,12 @@ import org.junit.runner.RunWith; @SmallTest @RunWith(AndroidJUnit4.class) public class VpnManagerTest { - private static final String VPN_PROFILE_KEY = "KEY"; + private static final String PKG_NAME = "fooPackage"; + + private static final String SESSION_NAME_STRING = "testSession"; + private static final String SERVER_ADDR_STRING = "1.2.3.4"; + private static final String IDENTITY_STRING = "Identity"; + private static final byte[] PSK_BYTES = "preSharedKey".getBytes(); private IConnectivityManager mMockCs; private VpnManager mVpnManager; @@ -39,7 +52,7 @@ public class VpnManagerTest { new MockContext() { @Override public String getOpPackageName() { - return "fooPackage"; + return PKG_NAME; } }; @@ -50,34 +63,49 @@ public class VpnManagerTest { } @Test - public void testProvisionVpnProfile() throws Exception { - try { - mVpnManager.provisionVpnProfile(mock(PlatformVpnProfile.class)); - } catch (UnsupportedOperationException expected) { - } + public void testProvisionVpnProfilePreconsented() throws Exception { + final PlatformVpnProfile profile = getPlatformVpnProfile(); + when(mMockCs.provisionVpnProfile(any(VpnProfile.class), eq(PKG_NAME))).thenReturn(true); + + // Expect there to be no intent returned, as consent has already been granted. + assertNull(mVpnManager.provisionVpnProfile(profile)); + verify(mMockCs).provisionVpnProfile(eq(profile.toVpnProfile()), eq(PKG_NAME)); + } + + @Test + public void testProvisionVpnProfileNeedsConsent() throws Exception { + final PlatformVpnProfile profile = getPlatformVpnProfile(); + when(mMockCs.provisionVpnProfile(any(VpnProfile.class), eq(PKG_NAME))).thenReturn(false); + + // Expect intent to be returned, as consent has not already been granted. + assertNotNull(mVpnManager.provisionVpnProfile(profile)); + verify(mMockCs).provisionVpnProfile(eq(profile.toVpnProfile()), eq(PKG_NAME)); } @Test public void testDeleteProvisionedVpnProfile() throws Exception { - try { - mVpnManager.deleteProvisionedVpnProfile(); - } catch (UnsupportedOperationException expected) { - } + mVpnManager.deleteProvisionedVpnProfile(); + verify(mMockCs).deleteVpnProfile(eq(PKG_NAME)); } @Test public void testStartProvisionedVpnProfile() throws Exception { - try { - mVpnManager.startProvisionedVpnProfile(); - } catch (UnsupportedOperationException expected) { - } + mVpnManager.startProvisionedVpnProfile(); + verify(mMockCs).startVpnProfile(eq(PKG_NAME)); } @Test public void testStopProvisionedVpnProfile() throws Exception { - try { - mVpnManager.stopProvisionedVpnProfile(); - } catch (UnsupportedOperationException expected) { - } + mVpnManager.stopProvisionedVpnProfile(); + verify(mMockCs).stopVpnProfile(eq(PKG_NAME)); + } + + private Ikev2VpnProfile getPlatformVpnProfile() throws Exception { + return new Ikev2VpnProfile.Builder(SERVER_ADDR_STRING, IDENTITY_STRING) + .setBypassable(true) + .setMaxMtu(1300) + .setMetered(true) + .setAuthPsk(PSK_BYTES) + .build(); } } From 73708bff553858764fc9f2edba5ebf48040fc18b Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Wed, 18 Dec 2019 10:57:50 -0800 Subject: [PATCH 5/6] Add callback registration in ConnectivityService. ConnectivityDiagnosticsManager will send callbacks to ConnectivityService for registering and unregistering them with the system. ConnectivityService needs to do the processing for persisting (and deleting) these callbacks on the ConnectivityService Thread, so messages are sent to the Connectivity Diagnostics Handler, which runs on the ConnectivityService Thread. Bug: 146444622 Bug: 143187964 Bug: 147848028 Test: compiles Test: atest FrameworksNetTests Change-Id: Ia5c8f90a60c050504e8676de9564a7607a9b03bc --- .../net/ConnectivityDiagnosticsManager.java | 40 +++- .../android/server/ConnectivityService.java | 201 ++++++++++++++++-- .../ConnectivityDiagnosticsManagerTest.java | 67 ++++++ .../server/ConnectivityServiceTest.java | 71 +++++++ 4 files changed, 350 insertions(+), 29 deletions(-) diff --git a/core/java/android/net/ConnectivityDiagnosticsManager.java b/core/java/android/net/ConnectivityDiagnosticsManager.java index b13e4b72aa..d018cbd904 100644 --- a/core/java/android/net/ConnectivityDiagnosticsManager.java +++ b/core/java/android/net/ConnectivityDiagnosticsManager.java @@ -25,13 +25,16 @@ import android.os.Binder; import android.os.Parcel; import android.os.Parcelable; import android.os.PersistableBundle; +import android.os.RemoteException; import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.Preconditions; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.util.Map; import java.util.Objects; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; /** @@ -57,6 +60,11 @@ import java.util.concurrent.Executor; * */ public class ConnectivityDiagnosticsManager { + /** @hide */ + @VisibleForTesting + public static final Map + sCallbacks = new ConcurrentHashMap<>(); + private final Context mContext; private final IConnectivityManager mService; @@ -646,9 +654,9 @@ public class ConnectivityDiagnosticsManager { *

If a registering app loses its relevant permissions, any callbacks it registered will * silently stop receiving callbacks. * - *

Each register() call MUST use a unique ConnectivityDiagnosticsCallback instance. If - * a single instance is registered with multiple NetworkRequests, an IllegalArgumentException - * will be thrown. + *

Each register() call MUST use a ConnectivityDiagnosticsCallback instance that is + * not currently registered. If a ConnectivityDiagnosticsCallback instance is registered with + * multiple NetworkRequests, an IllegalArgumentException will be thrown. * * @param request The NetworkRequest that will be used to match with Networks for which * callbacks will be fired @@ -657,15 +665,21 @@ public class ConnectivityDiagnosticsManager { * System * @throws IllegalArgumentException if the same callback instance is registered with multiple * NetworkRequests - * @throws SecurityException if the caller does not have appropriate permissions to register a - * callback */ public void registerConnectivityDiagnosticsCallback( @NonNull NetworkRequest request, @NonNull Executor e, @NonNull ConnectivityDiagnosticsCallback callback) { - // TODO(b/143187964): implement ConnectivityDiagnostics functionality - throw new UnsupportedOperationException("registerCallback() not supported yet"); + final ConnectivityDiagnosticsBinder binder = new ConnectivityDiagnosticsBinder(callback, e); + if (sCallbacks.putIfAbsent(callback, binder) != null) { + throw new IllegalArgumentException("Callback is currently registered"); + } + + try { + mService.registerConnectivityDiagnosticsCallback(binder, request); + } catch (RemoteException exception) { + exception.rethrowFromSystemServer(); + } } /** @@ -678,7 +692,15 @@ public class ConnectivityDiagnosticsManager { */ public void unregisterConnectivityDiagnosticsCallback( @NonNull ConnectivityDiagnosticsCallback callback) { - // TODO(b/143187964): implement ConnectivityDiagnostics functionality - throw new UnsupportedOperationException("registerCallback() not supported yet"); + // unconditionally removing from sCallbacks prevents race conditions here, since remove() is + // atomic. + final ConnectivityDiagnosticsBinder binder = sCallbacks.remove(callback); + if (binder == null) return; + + try { + mService.unregisterConnectivityDiagnosticsCallback(binder); + } catch (RemoteException exception) { + exception.rethrowFromSystemServer(); + } } } diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 478b87c010..0fe3b7874a 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -559,13 +559,17 @@ public class ConnectivityService extends IConnectivityManager.Stub .asInterface(ServiceManager.getService("dnsresolver")); } - /** Handler thread used for both of the handlers below. */ + /** Handler thread used for all of the handlers below. */ @VisibleForTesting protected final HandlerThread mHandlerThread; /** Handler used for internal events. */ final private InternalHandler mHandler; /** Handler used for incoming {@link NetworkStateTracker} events. */ final private NetworkStateTrackerHandler mTrackerHandler; + /** Handler used for processing {@link android.net.ConnectivityDiagnosticsManager} events */ + @VisibleForTesting + final ConnectivityDiagnosticsHandler mConnectivityDiagnosticsHandler; + private final DnsManager mDnsManager; private boolean mSystemReady; @@ -632,6 +636,10 @@ public class ConnectivityService extends IConnectivityManager.Stub @VisibleForTesting final MultipathPolicyTracker mMultipathPolicyTracker; + @VisibleForTesting + final Map + mConnectivityDiagnosticsCallbacks = new HashMap<>(); + /** * Implements support for the legacy "one network per network type" model. * @@ -964,6 +972,8 @@ public class ConnectivityService extends IConnectivityManager.Stub mHandlerThread.start(); mHandler = new InternalHandler(mHandlerThread.getLooper()); mTrackerHandler = new NetworkStateTrackerHandler(mHandlerThread.getLooper()); + mConnectivityDiagnosticsHandler = + new ConnectivityDiagnosticsHandler(mHandlerThread.getLooper()); mReleasePendingIntentDelayMs = Settings.Secure.getInt(context.getContentResolver(), Settings.Secure.CONNECTIVITY_RELEASE_PENDING_INTENT_DELAY_MS, 5_000); @@ -3379,18 +3389,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nri.unlinkDeathRecipient(); mNetworkRequests.remove(nri.request); - synchronized (mUidToNetworkRequestCount) { - int requests = mUidToNetworkRequestCount.get(nri.mUid, 0); - if (requests < 1) { - Slog.wtf(TAG, "BUG: too small request count " + requests + " for UID " + - nri.mUid); - } else if (requests == 1) { - mUidToNetworkRequestCount.removeAt( - mUidToNetworkRequestCount.indexOfKey(nri.mUid)); - } else { - mUidToNetworkRequestCount.put(nri.mUid, requests - 1); - } - } + decrementNetworkRequestPerUidCount(nri); mNetworkRequestInfoLogs.log("RELEASE " + nri); if (nri.request.isRequest()) { @@ -3461,6 +3460,19 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + private void decrementNetworkRequestPerUidCount(final NetworkRequestInfo nri) { + synchronized (mUidToNetworkRequestCount) { + final int requests = mUidToNetworkRequestCount.get(nri.mUid, 0); + if (requests < 1) { + Slog.wtf(TAG, "BUG: too small request count " + requests + " for UID " + nri.mUid); + } else if (requests == 1) { + mUidToNetworkRequestCount.removeAt(mUidToNetworkRequestCount.indexOfKey(nri.mUid)); + } else { + mUidToNetworkRequestCount.put(nri.mUid, requests - 1); + } + } + } + @Override public void setAcceptUnvalidated(Network network, boolean accept, boolean always) { enforceNetworkStackSettingsOrSetup(); @@ -5079,6 +5091,10 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + NetworkRequestInfo(NetworkRequest r) { + this(r, null); + } + private void enforceRequestCountLimit() { synchronized (mUidToNetworkRequestCount) { int networkRequests = mUidToNetworkRequestCount.get(mUid, 0) + 1; @@ -6169,7 +6185,10 @@ public class ConnectivityService extends IConnectivityManager.Stub private void callCallbackForRequest(NetworkRequestInfo nri, NetworkAgentInfo networkAgent, int notificationType, int arg1) { if (nri.messenger == null) { - return; // Default request has no msgr + // Default request has no msgr. Also prevents callbacks from being invoked for + // NetworkRequestInfos registered with ConnectivityDiagnostics requests. Those callbacks + // are Type.LISTEN, but should not have NetworkCallbacks invoked. + return; } Bundle bundle = new Bundle(); // TODO: check if defensive copies of data is needed. @@ -7325,19 +7344,161 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** + * Handler used for managing all Connectivity Diagnostics related functions. + * + * @see android.net.ConnectivityDiagnosticsManager + * + * TODO(b/147816404): Explore moving ConnectivityDiagnosticsHandler to a separate file + */ + @VisibleForTesting + class ConnectivityDiagnosticsHandler extends Handler { + /** + * Used to handle ConnectivityDiagnosticsCallback registration events from {@link + * android.net.ConnectivityDiagnosticsManager}. + * obj = ConnectivityDiagnosticsCallbackInfo with IConnectivityDiagnosticsCallback and + * NetworkRequestInfo to be registered + */ + private static final int EVENT_REGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK = 1; + + /** + * Used to handle ConnectivityDiagnosticsCallback unregister events from {@link + * android.net.ConnectivityDiagnosticsManager}. + * obj = the IConnectivityDiagnosticsCallback to be unregistered + * arg1 = the uid of the caller + */ + private static final int EVENT_UNREGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK = 2; + + private ConnectivityDiagnosticsHandler(Looper looper) { + super(looper); + } + + @Override + public void handleMessage(Message msg) { + switch (msg.what) { + case EVENT_REGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK: { + handleRegisterConnectivityDiagnosticsCallback( + (ConnectivityDiagnosticsCallbackInfo) msg.obj); + break; + } + case EVENT_UNREGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK: { + handleUnregisterConnectivityDiagnosticsCallback( + (IConnectivityDiagnosticsCallback) msg.obj, msg.arg1); + break; + } + } + } + } + + /** Class used for cleaning up IConnectivityDiagnosticsCallback instances after their death. */ + @VisibleForTesting + class ConnectivityDiagnosticsCallbackInfo implements Binder.DeathRecipient { + @NonNull private final IConnectivityDiagnosticsCallback mCb; + @NonNull private final NetworkRequestInfo mRequestInfo; + + @VisibleForTesting + ConnectivityDiagnosticsCallbackInfo( + @NonNull IConnectivityDiagnosticsCallback cb, @NonNull NetworkRequestInfo nri) { + mCb = cb; + mRequestInfo = nri; + } + + @Override + public void binderDied() { + log("ConnectivityDiagnosticsCallback IBinder died."); + unregisterConnectivityDiagnosticsCallback(mCb); + } + } + + private void handleRegisterConnectivityDiagnosticsCallback( + @NonNull ConnectivityDiagnosticsCallbackInfo cbInfo) { + ensureRunningOnConnectivityServiceThread(); + + final IConnectivityDiagnosticsCallback cb = cbInfo.mCb; + final NetworkRequestInfo nri = cbInfo.mRequestInfo; + + // This means that the client registered the same callback multiple times. Do + // not override the previous entry, and exit silently. + if (mConnectivityDiagnosticsCallbacks.containsKey(cb)) { + if (VDBG) log("Diagnostics callback is already registered"); + + // Decrement the reference count for this NetworkRequestInfo. The reference count is + // incremented when the NetworkRequestInfo is created as part of + // enforceRequestCountLimit(). + decrementNetworkRequestPerUidCount(nri); + return; + } + + mConnectivityDiagnosticsCallbacks.put(cb, cbInfo); + + try { + cb.asBinder().linkToDeath(cbInfo, 0); + } catch (RemoteException e) { + cbInfo.binderDied(); + } + } + + private void handleUnregisterConnectivityDiagnosticsCallback( + @NonNull IConnectivityDiagnosticsCallback cb, int uid) { + ensureRunningOnConnectivityServiceThread(); + + if (!mConnectivityDiagnosticsCallbacks.containsKey(cb)) { + if (VDBG) log("Removing diagnostics callback that is not currently registered"); + return; + } + + final NetworkRequestInfo nri = mConnectivityDiagnosticsCallbacks.get(cb).mRequestInfo; + + if (uid != nri.mUid) { + if (VDBG) loge("Different uid than registrant attempting to unregister cb"); + return; + } + + cb.asBinder().unlinkToDeath(mConnectivityDiagnosticsCallbacks.remove(cb), 0); + } + @Override public void registerConnectivityDiagnosticsCallback( @NonNull IConnectivityDiagnosticsCallback callback, @NonNull NetworkRequest request) { - // TODO(b/146444622): implement register IConnectivityDiagnosticsCallback functionality - throw new UnsupportedOperationException( - "registerConnectivityDiagnosticsCallback not yet implemented"); + if (request.legacyType != TYPE_NONE) { + throw new IllegalArgumentException("ConnectivityManager.TYPE_* are deprecated." + + " Please use NetworkCapabilities instead."); + } + + // This NetworkCapabilities is only used for matching to Networks. Clear out its owner uid + // and administrator uids to be safe. + final NetworkCapabilities nc = new NetworkCapabilities(request.networkCapabilities); + restrictRequestUidsForCaller(nc); + + final NetworkRequest requestWithId = + new NetworkRequest( + nc, TYPE_NONE, nextNetworkRequestId(), NetworkRequest.Type.LISTEN); + + // NetworkRequestInfos created here count towards MAX_NETWORK_REQUESTS_PER_UID limit. + // + // nri is not bound to the death of callback. Instead, callback.bindToDeath() is set in + // handleRegisterConnectivityDiagnosticsCallback(). nri will be cleaned up as part of the + // callback's binder death. + final NetworkRequestInfo nri = new NetworkRequestInfo(requestWithId); + final ConnectivityDiagnosticsCallbackInfo cbInfo = + new ConnectivityDiagnosticsCallbackInfo(callback, nri); + + mConnectivityDiagnosticsHandler.sendMessage( + mConnectivityDiagnosticsHandler.obtainMessage( + ConnectivityDiagnosticsHandler + .EVENT_REGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK, + cbInfo)); } @Override public void unregisterConnectivityDiagnosticsCallback( @NonNull IConnectivityDiagnosticsCallback callback) { - // TODO(b/146444622): implement register IConnectivityDiagnosticsCallback functionality - throw new UnsupportedOperationException( - "unregisterConnectivityDiagnosticsCallback not yet implemented"); + mConnectivityDiagnosticsHandler.sendMessage( + mConnectivityDiagnosticsHandler.obtainMessage( + ConnectivityDiagnosticsHandler + .EVENT_UNREGISTER_CONNECTIVITY_DIAGNOSTICS_CALLBACK, + Binder.getCallingUid(), + 0, + callback)); } } diff --git a/tests/net/java/android/net/ConnectivityDiagnosticsManagerTest.java b/tests/net/java/android/net/ConnectivityDiagnosticsManagerTest.java index 7ab4b56fae..2d5df4f47e 100644 --- a/tests/net/java/android/net/ConnectivityDiagnosticsManagerTest.java +++ b/tests/net/java/android/net/ConnectivityDiagnosticsManagerTest.java @@ -27,12 +27,18 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import android.content.Context; import android.os.PersistableBundle; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -52,15 +58,27 @@ public class ConnectivityDiagnosticsManagerTest { private static final Executor INLINE_EXECUTOR = x -> x.run(); + @Mock private Context mContext; + @Mock private IConnectivityManager mService; @Mock private ConnectivityDiagnosticsCallback mCb; private ConnectivityDiagnosticsBinder mBinder; + private ConnectivityDiagnosticsManager mManager; @Before public void setUp() { + mContext = mock(Context.class); + mService = mock(IConnectivityManager.class); mCb = mock(ConnectivityDiagnosticsCallback.class); mBinder = new ConnectivityDiagnosticsBinder(mCb, INLINE_EXECUTOR); + mManager = new ConnectivityDiagnosticsManager(mContext, mService); + } + + @After + public void tearDown() { + // clear ConnectivityDiagnosticsManager callbacks map + ConnectivityDiagnosticsManager.sCallbacks.clear(); } private ConnectivityReport createSampleConnectivityReport() { @@ -245,4 +263,53 @@ public class ConnectivityDiagnosticsManagerTest { // latch without waiting. verify(mCb).onNetworkConnectivityReported(eq(n), eq(connectivity)); } + + @Test + public void testRegisterConnectivityDiagnosticsCallback() throws Exception { + final NetworkRequest request = new NetworkRequest.Builder().build(); + + mManager.registerConnectivityDiagnosticsCallback(request, INLINE_EXECUTOR, mCb); + + verify(mService).registerConnectivityDiagnosticsCallback( + any(ConnectivityDiagnosticsBinder.class), eq(request)); + assertTrue(ConnectivityDiagnosticsManager.sCallbacks.containsKey(mCb)); + } + + @Test + public void testRegisterDuplicateConnectivityDiagnosticsCallback() throws Exception { + final NetworkRequest request = new NetworkRequest.Builder().build(); + + mManager.registerConnectivityDiagnosticsCallback(request, INLINE_EXECUTOR, mCb); + + try { + mManager.registerConnectivityDiagnosticsCallback(request, INLINE_EXECUTOR, mCb); + fail("Duplicate callback registration should fail"); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testUnregisterConnectivityDiagnosticsCallback() throws Exception { + final NetworkRequest request = new NetworkRequest.Builder().build(); + mManager.registerConnectivityDiagnosticsCallback(request, INLINE_EXECUTOR, mCb); + + mManager.unregisterConnectivityDiagnosticsCallback(mCb); + + verify(mService).unregisterConnectivityDiagnosticsCallback( + any(ConnectivityDiagnosticsBinder.class)); + assertFalse(ConnectivityDiagnosticsManager.sCallbacks.containsKey(mCb)); + + // verify that re-registering is successful + mManager.registerConnectivityDiagnosticsCallback(request, INLINE_EXECUTOR, mCb); + verify(mService, times(2)).registerConnectivityDiagnosticsCallback( + any(ConnectivityDiagnosticsBinder.class), eq(request)); + assertTrue(ConnectivityDiagnosticsManager.sCallbacks.containsKey(mCb)); + } + + @Test + public void testUnregisterUnknownConnectivityDiagnosticsCallback() throws Exception { + mManager.unregisterConnectivityDiagnosticsCallback(mCb); + + verifyNoMoreInteractions(mService); + } } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index e80b7c9d01..50f1bbeed0 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -139,6 +139,7 @@ import android.net.ConnectivityManager.PacketKeepalive; import android.net.ConnectivityManager.PacketKeepaliveCallback; import android.net.ConnectivityManager.TooManyRequestsException; import android.net.ConnectivityThread; +import android.net.IConnectivityDiagnosticsCallback; import android.net.IDnsResolver; import android.net.IIpConnectivityMetrics; import android.net.INetd; @@ -180,6 +181,7 @@ import android.os.Bundle; import android.os.ConditionVariable; import android.os.Handler; import android.os.HandlerThread; +import android.os.IBinder; import android.os.INetworkManagementService; import android.os.Looper; import android.os.Parcel; @@ -210,6 +212,7 @@ import com.android.internal.util.ArrayUtils; import com.android.internal.util.WakeupMessage; import com.android.internal.util.test.BroadcastInterceptingContext; import com.android.internal.util.test.FakeSettingsProvider; +import com.android.server.ConnectivityService.ConnectivityDiagnosticsCallbackInfo; import com.android.server.connectivity.ConnectivityConstants; import com.android.server.connectivity.DefaultNetworkMetrics; import com.android.server.connectivity.IpConnectivityMetrics; @@ -322,6 +325,8 @@ public class ConnectivityServiceTest { @Mock UserManager mUserManager; @Mock NotificationManager mNotificationManager; @Mock AlarmManager mAlarmManager; + @Mock IConnectivityDiagnosticsCallback mConnectivityDiagnosticsCallback; + @Mock IBinder mIBinder; private ArgumentCaptor mResolverParamsParcelCaptor = ArgumentCaptor.forClass(ResolverParamsParcel.class); @@ -6355,4 +6360,70 @@ public class ConnectivityServiceTest { UserHandle.getAppId(uid)); return packageInfo; } + + @Test + public void testRegisterConnectivityDiagnosticsCallbackInvalidRequest() throws Exception { + final NetworkRequest request = + new NetworkRequest( + new NetworkCapabilities(), TYPE_ETHERNET, 0, NetworkRequest.Type.NONE); + try { + mService.registerConnectivityDiagnosticsCallback( + mConnectivityDiagnosticsCallback, request); + fail("registerConnectivityDiagnosticsCallback should throw on invalid NetworkRequest"); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testRegisterUnregisterConnectivityDiagnosticsCallback() throws Exception { + final NetworkRequest wifiRequest = + new NetworkRequest.Builder().addTransportType(TRANSPORT_WIFI).build(); + + when(mConnectivityDiagnosticsCallback.asBinder()).thenReturn(mIBinder); + + mService.registerConnectivityDiagnosticsCallback( + mConnectivityDiagnosticsCallback, wifiRequest); + + verify(mIBinder, timeout(TIMEOUT_MS)) + .linkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); + assertTrue( + mService.mConnectivityDiagnosticsCallbacks.containsKey( + mConnectivityDiagnosticsCallback)); + + mService.unregisterConnectivityDiagnosticsCallback(mConnectivityDiagnosticsCallback); + verify(mIBinder, timeout(TIMEOUT_MS)) + .unlinkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); + assertFalse( + mService.mConnectivityDiagnosticsCallbacks.containsKey( + mConnectivityDiagnosticsCallback)); + verify(mConnectivityDiagnosticsCallback, atLeastOnce()).asBinder(); + } + + @Test + public void testRegisterDuplicateConnectivityDiagnosticsCallback() throws Exception { + final NetworkRequest wifiRequest = + new NetworkRequest.Builder().addTransportType(TRANSPORT_WIFI).build(); + when(mConnectivityDiagnosticsCallback.asBinder()).thenReturn(mIBinder); + + mService.registerConnectivityDiagnosticsCallback( + mConnectivityDiagnosticsCallback, wifiRequest); + + verify(mIBinder, timeout(TIMEOUT_MS)) + .linkToDeath(any(ConnectivityDiagnosticsCallbackInfo.class), anyInt()); + verify(mConnectivityDiagnosticsCallback).asBinder(); + assertTrue( + mService.mConnectivityDiagnosticsCallbacks.containsKey( + mConnectivityDiagnosticsCallback)); + + // Register the same callback again + mService.registerConnectivityDiagnosticsCallback( + mConnectivityDiagnosticsCallback, wifiRequest); + + // Block until all other events are done processing. + HandlerUtilsKt.waitForIdle(mCsHandlerThread, TIMEOUT_MS); + + assertTrue( + mService.mConnectivityDiagnosticsCallbacks.containsKey( + mConnectivityDiagnosticsCallback)); + } } From ad51e01b3c763ea051e74cc14c27b830d9c7563d Mon Sep 17 00:00:00 2001 From: Cody Kesting Date: Thu, 19 Dec 2019 12:36:18 -0800 Subject: [PATCH 6/6] Update javadocs for ConnectivityDiagnosticsManager. ConnectivityDiagnosticsManager comments for registerConnectivityDiagnosticsCallback and unregisterConnectivityDiagnosticsCallback are updated to reflect several changes. For register calls, any app will be able to register callbacks, but only permissioned applications will have their callbacks invoked (and only for networks managed by the application). Additionally, only the registering app (uid) will be able to unregister a callback once registered. Bug: 143187964 Test: docs change only. compiles. Change-Id: Ie7ae86a1afccb22d6c84027dbac49d7b8e431e8c --- core/java/android/net/ConnectivityDiagnosticsManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/java/android/net/ConnectivityDiagnosticsManager.java b/core/java/android/net/ConnectivityDiagnosticsManager.java index d018cbd904..140363c482 100644 --- a/core/java/android/net/ConnectivityDiagnosticsManager.java +++ b/core/java/android/net/ConnectivityDiagnosticsManager.java @@ -639,8 +639,9 @@ public class ConnectivityDiagnosticsManager { /** * Registers a ConnectivityDiagnosticsCallback with the System. * - *

Only apps that offer network connectivity to the user are allowed to register callbacks. - * This includes: + *

Only apps that offer network connectivity to the user should be registering callbacks. + * These are the only apps whose callbacks will be invoked by the system. Apps considered to + * meet these conditions include: * *

    *
  • Carrier apps with active subscriptions @@ -648,8 +649,7 @@ public class ConnectivityDiagnosticsManager { *
  • WiFi Suggesters *
* - *

Callbacks will be limited to receiving notifications for networks over which apps provide - * connectivity. + *

Callbacks registered by apps not meeting the above criteria will not be invoked. * *

If a registering app loses its relevant permissions, any callbacks it registered will * silently stop receiving callbacks.